Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(NODE-6610): replace feature flag symbols with internal properties #4354

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

refactor(NODE-6610): replace feature flag symbols with internal properties #4354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
eb31ad8
wip
W-A-James Dec 10, 2024
bbf4d82
WIP
W-A-James Dec 11, 2024
22f8edb
wip: replacing feature falgs
W-A-James Dec 13, 2024
16b2962
restore autoencrypter feature flags
W-A-James Dec 13, 2024
1603f02
add flags to options parsing
W-A-James Dec 13, 2024
a4b6c5d
restore callbacks symbol for legacy
W-A-James Dec 13, 2024
303b2d1
chore: fixups
nbbeeken Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 7 additions & 22 deletions src/connection_string.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,13 +249,6 @@ export function parseOptions(

const mongoOptions = Object.create(null);

// Feature flags
for (const flag of Object.getOwnPropertySymbols(options)) {
if (FEATURE_FLAGS.has(flag)) {
mongoOptions[flag] = options[flag];
}
}

mongoOptions.hosts = isSRV ? [] : hosts.map(HostAddress.fromString);

const urlOptions = new CaseInsensitiveMap<unknown[]>();
Expand Down Expand Up @@ -515,12 +508,11 @@ export function parseOptions(
);
}

const loggerFeatureFlag = Symbol.for('@@mdb.enableMongoLogger');
mongoOptions[loggerFeatureFlag] = mongoOptions[loggerFeatureFlag] ?? false;
mongoOptions.__enableMongoLogger = mongoOptions.__enableMongoLogger ?? false;

let loggerEnvOptions: MongoLoggerEnvOptions = {};
let loggerClientOptions: MongoLoggerMongoClientOptions = {};
if (mongoOptions[loggerFeatureFlag]) {
if (mongoOptions.__enableMongoLogger) {
loggerEnvOptions = {
MONGODB_LOG_COMMAND: process.env.MONGODB_LOG_COMMAND,
MONGODB_LOG_TOPOLOGY: process.env.MONGODB_LOG_TOPOLOGY,
Expand All @@ -530,7 +522,7 @@ export function parseOptions(
MONGODB_LOG_ALL: process.env.MONGODB_LOG_ALL,
MONGODB_LOG_MAX_DOCUMENT_LENGTH: process.env.MONGODB_LOG_MAX_DOCUMENT_LENGTH,
MONGODB_LOG_PATH: process.env.MONGODB_LOG_PATH,
...mongoOptions[Symbol.for('@@mdb.internalLoggerConfig')]
...mongoOptions.__internalLoggerConfig
};
loggerClientOptions = {
mongodbLogPath: mongoOptions.mongodbLogPath,
Expand Down Expand Up @@ -1317,21 +1309,14 @@ export const OPTIONS = {
* @internal
* TODO: NODE-5671 - remove internal flag
*/
mongodbLogMaxDocumentLength: { type: 'uint' }
mongodbLogMaxDocumentLength: { type: 'uint' },
__enableMongoLogger: { type: 'boolean' },
__skipPingOnConnect: { type: 'boolean' },
__internalLoggerConfig: { type: 'record' }
} as Record<keyof MongoClientOptions, OptionDescriptor>;

export const DEFAULT_OPTIONS = new CaseInsensitiveMap(
Object.entries(OPTIONS)
.filter(([, descriptor]) => descriptor.default != null)
.map(([k, d]) => [k, d.default])
);

/**
* Set of permitted feature flags
* @internal
*/
export const FEATURE_FLAGS = new Set([
Symbol.for('@@mdb.skipPingOnConnect'),
Symbol.for('@@mdb.enableMongoLogger'),
Symbol.for('@@mdb.internalLoggerConfig')
]);
16 changes: 12 additions & 4 deletions src/mongo_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
type LogComponentSeveritiesClientOptions,
type MongoDBLogWritable,
MongoLogger,
type MongoLoggerEnvOptions,
type MongoLoggerOptions,
SeverityLevel
} from './mongo_logger';
Expand Down Expand Up @@ -299,7 +300,11 @@ export interface MongoClientOptions extends BSONSerializeOptions, SupportedNodeC
mongodbLogMaxDocumentLength?: number;

/** @internal */
[featureFlag: symbol]: any;
__skipPingOnConnect?: boolean;
/** @internal */
__internalLoggerConfig?: MongoLoggerEnvOptions;
/** @internal */
__enableMongoLogger?: boolean;
}

/** @public */
Expand Down Expand Up @@ -1006,9 +1011,6 @@ export interface MongoOptions
tlsCRLFile?: string;
tlsCertificateKeyFile?: string;

/** @internal */
[featureFlag: symbol]: any;

/**
* @internal
* TODO: NODE-5671 - remove internal flag
Expand All @@ -1020,4 +1022,10 @@ export interface MongoOptions
*/
mongodbLogPath?: 'stderr' | 'stdout' | MongoDBLogWritable;
timeoutMS?: number;
/** @internal */
__skipPingOnConnect?: boolean;
/** @internal */
__internalLoggerConfig?: Document;
/** @internal */
__enableMongoLogger?: boolean;
}
4 changes: 2 additions & 2 deletions src/operations/execute_operation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ async function autoConnect(client: MongoClient): Promise<Topology> {
if (client.s.hasBeenClosed) {
throw new MongoNotConnectedError('Client must be connected before running operations');
}
client.s.options[Symbol.for('@@mdb.skipPingOnConnect')] = true;
client.s.options.__skipPingOnConnect = true;
try {
await client.connect();
if (client.topology == null) {
Expand All @@ -141,7 +141,7 @@ async function autoConnect(client: MongoClient): Promise<Topology> {
}
return client.topology;
} finally {
delete client.s.options[Symbol.for('@@mdb.skipPingOnConnect')];
delete client.s.options.__skipPingOnConnect;
}
}
return client.topology;
Expand Down
9 changes: 4 additions & 5 deletions src/sdam/topology.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type { MongoCredentials } from '../cmap/auth/mongo_credentials';
import type { ConnectionEvents } from '../cmap/connection';
import type { ConnectionPoolEvents } from '../cmap/connection_pool';
import type { ClientMetadata } from '../cmap/handshake/client_metadata';
import { DEFAULT_OPTIONS, FEATURE_FLAGS } from '../connection_string';
import { DEFAULT_OPTIONS } from '../connection_string';
import {
CLOSE,
CONNECT,
Expand Down Expand Up @@ -153,7 +153,7 @@ export interface TopologyOptions extends BSONSerializeOptions, ServerOptions {
serverMonitoringMode: ServerMonitoringMode;
/** MongoDB server API version */
serverApi?: ServerApi;
[featureFlag: symbol]: any;
__skipPingOnConnect?: boolean;
}

/** @public */
Expand Down Expand Up @@ -251,8 +251,7 @@ export class Topology extends TypedEventEmitter<TopologyEvents> {
// Options should only be undefined in tests, MongoClient will always have defined options
options = options ?? {
hosts: [HostAddress.fromString('localhost:27017')],
...Object.fromEntries(DEFAULT_OPTIONS.entries()),
...Object.fromEntries(FEATURE_FLAGS.entries())
...Object.fromEntries(DEFAULT_OPTIONS.entries())
};

if (typeof seeds === 'string') {
Expand Down Expand Up @@ -466,7 +465,7 @@ export class Topology extends TypedEventEmitter<TopologyEvents> {
readPreferenceServerSelector(readPreference),
selectServerOptions
);
const skipPingOnConnect = this.s.options[Symbol.for('@@mdb.skipPingOnConnect')] === true;
const skipPingOnConnect = this.s.options.__skipPingOnConnect === true;
if (!skipPingOnConnect && this.s.credentials) {
await server.command(ns('admin.$cmd'), { ping: 1 }, { timeoutContext });
stateTransition(this, STATE_CONNECTED);
Expand Down
2 changes: 1 addition & 1 deletion test/integration/client-side-encryption/client_side_encryption.prose.12.deadlock.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class CapturingMongoClient extends MongoClient {
commandStartedEvents: Array<CommandStartedEvent> = [];
clientsCreated = 0;
constructor(url: string, options: MongoClientOptions = {}) {
options = { ...options, monitorCommands: true, [Symbol.for('@@mdb.skipPingOnConnect')]: true };
options = { ...options, monitorCommands: true, __skipPingOnConnect: true };
if (process.env.MONGODB_API_VERSION) {
options.serverApi = process.env.MONGODB_API_VERSION as MongoClientOptions['serverApi'];
}
Expand Down
7 changes: 3 additions & 4 deletions test/integration/command-logging-and-monitoring/command_logging_and_monitoring.prose.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { expect } from 'chai';
import { DEFAULT_MAX_DOCUMENT_LENGTH, type Document } from '../../mongodb';

describe('Command Logging and Monitoring Prose Tests', function () {
const loggerFeatureFlag = Symbol.for('@@mdb.enableMongoLogger');
const ELLIPSES_LENGTH = 3;
let client;
let writable;
Expand Down Expand Up @@ -40,7 +39,7 @@ describe('Command Logging and Monitoring Prose Tests', function () {
client = this.configuration.newClient(
{},
{
[loggerFeatureFlag]: true,
__enableMongoLogger: true,
mongodbLogPath: writable,
mongodbLogComponentSeverities: {
command: 'debug'
Expand Down Expand Up @@ -124,7 +123,7 @@ describe('Command Logging and Monitoring Prose Tests', function () {
client = this.configuration.newClient(
{},
{
[loggerFeatureFlag]: true,
__enableMongoLogger: true,
mongodbLogPath: writable,
mongodbLogComponentSeverities: {
command: 'debug'
Expand Down Expand Up @@ -181,7 +180,7 @@ describe('Command Logging and Monitoring Prose Tests', function () {
client = this.configuration.newClient(
{},
{
[loggerFeatureFlag]: true,
__enableMongoLogger: true,
mongodbLogPath: writable,
mongodbLogComponentSeverities: {
command: 'debug'
Expand Down
26 changes: 12 additions & 14 deletions test/integration/node-specific/feature_flags.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { expect } from 'chai';
import { MongoClient, SeverityLevel } from '../../mongodb';

describe('Feature Flags', () => {
describe('@@mdb.skipPingOnConnect', () => {
describe('__skipPingOnConnect', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This describe block and test file name is inaccurate now that we're not using feature flags. Can we make the naming more accurate?

beforeEach(function () {
if (process.env.AUTH !== 'auth') {
this.currentTest.skipReason = 'ping count relies on auth to be enabled';
Expand All @@ -23,8 +23,7 @@ describe('Feature Flags', () => {
];
for (const { description, value, expectEvents } of tests) {
it(description, async function () {
const options =
value === undefined ? {} : { [Symbol.for('@@mdb.skipPingOnConnect')]: value };
const options = value === undefined ? {} : { __skipPingOnConnect: value };
const client = this.configuration.newClient({}, { ...options, monitorCommands: true });
const events = [];
client.on('commandStarted', event => events.push(event));
Expand All @@ -46,9 +45,8 @@ describe('Feature Flags', () => {
});

// TODO(NODE-5672): Release Standardized Logger
describe('@@mdb.enableMongoLogger', () => {
describe('__enableMongoLogger', () => {
let cachedEnv;
const loggerFeatureFlag = Symbol.for('@@mdb.enableMongoLogger');

before(() => {
cachedEnv = process.env;
Expand All @@ -66,7 +64,7 @@ describe('Feature Flags', () => {

it('enables logging for the specified component', () => {
const client = new MongoClient('mongodb://localhost:27017', {
[loggerFeatureFlag]: true
__enableMongoLogger: true
});
expect(client.mongoLogger?.componentSeverities).to.have.property(
'command',
Expand All @@ -82,7 +80,7 @@ describe('Feature Flags', () => {

it('does not create logger', () => {
const client = new MongoClient('mongodb://localhost:27017', {
[loggerFeatureFlag]: true
__enableMongoLogger: true
});
expect(client.mongoLogger).to.not.exist;
});
Expand All @@ -98,7 +96,7 @@ describe('Feature Flags', () => {

it('does not instantiate logger', () => {
const client = new MongoClient('mongodb://localhost:27017', {
[loggerFeatureFlag]: featureFlagValue
__enableMongoLogger: featureFlagValue
});
expect(client.mongoLogger).to.not.exist;
});
Expand All @@ -111,7 +109,7 @@ describe('Feature Flags', () => {

it('does not instantiate logger', () => {
const client = new MongoClient('mongodb://localhost:27017', {
[loggerFeatureFlag]: featureFlagValue
__enableMongoLogger: featureFlagValue
});
expect(client.mongoLogger).to.not.exist;
});
Expand All @@ -120,7 +118,7 @@ describe('Feature Flags', () => {
}
});

describe('@@mdb.internalLoggerConfig', () => {
describe('__internalLoggerConfig', () => {
let cachedEnv: NodeJS.ProcessEnv;

before(() => {
Expand All @@ -138,8 +136,8 @@ describe('Feature Flags', () => {

it('falls back to environment options', function () {
const client = new MongoClient('mongodb://localhost:27017', {
[Symbol.for('@@mdb.enableMongoLogger')]: true,
[Symbol.for('@@mdb.internalLoggerConfig')]: undefined
__enableMongoLogger: true,
__internalLoggerConfig: undefined
});

expect(client.mongoLogger?.componentSeverities).to.have.property(
Expand All @@ -152,8 +150,8 @@ describe('Feature Flags', () => {
context('when defined', function () {
it('overrides environment options', function () {
const client = new MongoClient('mongodb://localhost:27017', {
[Symbol.for('@@mdb.enableMongoLogger')]: true,
[Symbol.for('@@mdb.internalLoggerConfig')]: {
__enableMongoLogger: true,
__internalLoggerConfig: {
MONGODB_LOG_COMMAND: SeverityLevel.ALERT
}
});
Expand Down
2 changes: 1 addition & 1 deletion test/integration/node-specific/ipv6.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ describe('IPv6 Addresses', () => {

ipv6Hosts = this.configuration.options.hostAddresses.map(({ port }) => `[::1]:${port}`);
client = this.configuration.newClient(`mongodb://${ipv6Hosts.join(',')}/test`, {
[Symbol.for('@@mdb.skipPingOnConnect')]: true,
__skipPingOnConnect: true,
maxPoolSize: 1
});
});
Expand Down
8 changes: 4 additions & 4 deletions test/integration/node-specific/mongo_client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,8 +456,8 @@ describe('class MongoClient', function () {
const [findCommandStarted] = await findCommandToBeStarted;

expect(findCommandStarted).to.have.property('commandName', 'find');
expect(client.options).to.not.have.property(Symbol.for('@@mdb.skipPingOnConnect'));
expect(client.s.options).to.not.have.property(Symbol.for('@@mdb.skipPingOnConnect'));
expect(client.options).to.not.have.property('__skipPingOnConnect');
expect(client.s.options).to.not.have.property('__skipPingOnConnect');

// Assertion is redundant but it shows that no initial ping is run
expect(findCommandStarted.commandName).to.not.equal('ping');
Expand All @@ -475,8 +475,8 @@ describe('class MongoClient', function () {
const [insertCommandStarted] = await insertOneCommandToBeStarted;

expect(insertCommandStarted).to.have.property('commandName', 'insert');
expect(client.options).to.not.have.property(Symbol.for('@@mdb.skipPingOnConnect'));
expect(client.s.options).to.not.have.property(Symbol.for('@@mdb.skipPingOnConnect'));
expect(client.options).to.not.have.property('__skipPingOnConnect');
expect(client.s.options).to.not.have.property('__skipPingOnConnect');

// Assertion is redundant but it shows that no initial ping is run
expect(insertCommandStarted.commandName).to.not.equal('ping');
Expand Down
2 changes: 1 addition & 1 deletion test/tools/spec-runner/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ function runTestSuiteTest(configuration, spec, context) {
minHeartbeatFrequencyMS: 100,
monitorCommands: true,
...spec.clientOptions,
[Symbol.for('@@mdb.skipPingOnConnect')]: true
__skipPingOnConnect: true
});

if (context.requiresCSFLE) {
Expand Down
6 changes: 3 additions & 3 deletions test/tools/unified-spec-runner/entities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,9 +222,9 @@ export class UnifiedMongoClient extends MongoClient {

super(uri, {
monitorCommands: true,
[Symbol.for('@@mdb.skipPingOnConnect')]: true,
[Symbol.for('@@mdb.enableMongoLogger')]: true,
[Symbol.for('@@mdb.internalLoggerConfig')]: componentSeverities,
__skipPingOnConnect: true,
__enableMongoLogger: true,
__internalLoggerConfig: componentSeverities,
...getEnvironmentalOptions(),
...(description.serverApi ? { serverApi: description.serverApi } : {}),
mongodbLogPath: logCollector,
Expand Down
Loading
Loading