Skip to content

feat(NODE-7009): add client metadata on demand #4574

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.

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion src/cmap/connection_pool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -610,13 +610,17 @@ export class ConnectionPool extends TypedEventEmitter<ConnectionPoolEvents> {
}

private createConnection(callback: Callback<Connection>) {
// Note that metadata and extendedMetadata may have changed on the client but have
// been frozen here, so we pull the extendedMetadata promise always from the client
// no mattter what options were set at the construction of the pool.
const connectOptions: ConnectionOptions = {
...this.options,
id: this.connectionCounter.next().value,
generation: this.generation,
cancellationToken: this.cancellationToken,
mongoLogger: this.mongoLogger,
authProviders: this.server.topology.client.s.authProviders
authProviders: this.server.topology.client.s.authProviders,
extendedMetadata: this.server.topology.client.options?.extendedMetadata
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible for the client.options to not be set? (parseOptions seems to always return an object at creation)

};

this.pending++;
Expand Down
25 changes: 24 additions & 1 deletion src/cmap/handshake/client_metadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,10 @@ export class LimitedSizeDocument {
}
}

type MakeClientMetadataOptions = Pick<MongoOptions, 'appName' | 'driverInfo'>;
type MakeClientMetadataOptions = Pick<
MongoOptions,
'appName' | 'driverInfo' | 'additionalDriverInfo'
>;
/**
* From the specs:
* Implementors SHOULD cumulatively update fields in the following order until the document is under the size limit:
Expand Down Expand Up @@ -119,6 +122,18 @@ export function makeClientMetadata(options: MakeClientMetadataOptions): ClientMe
version: version.length > 0 ? `${NODE_DRIVER_VERSION}|${version}` : NODE_DRIVER_VERSION
};

// This is where we handle additional driver info added after client construction.
if (options.additionalDriverInfo?.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if additionalDriverInfo is always defaulted to [], what could cause it to become unset?

Copy link
Member Author

Choose a reason for hiding this comment

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

Our test suite doesn't have the same strict type restrictions and can be directly unset there. I considered that out of scope and to defensively program here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am concerned that if we are not expecting it to ever become unset in real life and it does happen, then something has gone terribly wrong; so instead of paving over it, it may be better to throw a MongoRuntimeError so that we are debugging a clear issue and not a weird "it doesn't seem to send the updated info for this one user" issue that gets filed two years from now. Also, as an added bonus, you'd be able to streamline the code by removing the if statement and going straight to the for-loop (same in L148)

for (const { name: n = '', version: v = '' } of options.additionalDriverInfo) {
if (n.length > 0) {
driverInfo.name = `${driverInfo.name}|${n}`;
}
if (v.length > 0) {
driverInfo.version = `${driverInfo.version}|${v}`;
}
}
}

if (!metadataDocument.ifItFitsItSits('driver', driverInfo)) {
throw new MongoInvalidArgumentError(
'Unable to include driverInfo name and version, metadata cannot exceed 512 bytes'
Expand All @@ -130,6 +145,14 @@ export function makeClientMetadata(options: MakeClientMetadataOptions): ClientMe
runtimeInfo = `${runtimeInfo}|${platform}`;
}

if (options.additionalDriverInfo?.length > 0) {
for (const { platform: p = '' } of options.additionalDriverInfo) {
if (p.length > 0) {
runtimeInfo = `${runtimeInfo}|${p}`;
}
}
}

if (!metadataDocument.ifItFitsItSits('platform', runtimeInfo)) {
throw new MongoInvalidArgumentError(
'Unable to include driverInfo platform, metadata cannot exceed 512 bytes'
Expand Down
3 changes: 3 additions & 0 deletions src/connection_string.ts
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,9 @@ export function parseOptions(
}
);

// Set the default for the additional driver info.
mongoOptions.additionalDriverInfo = [];

mongoOptions.metadata = makeClientMetadata(mongoOptions);

mongoOptions.extendedMetadata = addContainerMetadata(mongoOptions.metadata).then(
Expand Down
46 changes: 42 additions & 4 deletions src/mongo_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@ import { type TokenCache } from './cmap/auth/mongodb_oidc/token_cache';
import { AuthMechanism } from './cmap/auth/providers';
import type { LEGAL_TCP_SOCKET_OPTIONS, LEGAL_TLS_SOCKET_OPTIONS } from './cmap/connect';
import type { Connection } from './cmap/connection';
import type { ClientMetadata } from './cmap/handshake/client_metadata';
import {
addContainerMetadata,
type ClientMetadata,
makeClientMetadata
} from './cmap/handshake/client_metadata';
import type { CompressorName } from './cmap/wire_protocol/compression';
import { parseOptions, resolveSRVRecord } from './connection_string';
import { MONGO_CLIENT_EVENTS } from './constants';
Expand Down Expand Up @@ -394,9 +398,31 @@ export class MongoClient extends TypedEventEmitter<MongoClientEvents> implements
* The consolidate, parsed, transformed and merged options.
*/
public readonly options: Readonly<
Omit<MongoOptions, 'monitorCommands' | 'ca' | 'crl' | 'key' | 'cert'>
Omit<
MongoOptions,
| 'monitorCommands'
| 'ca'
| 'crl'
| 'key'
| 'cert'
| 'driverInfo'
| 'additionalDriverInfo'
| 'metadata'
| 'extendedMetadata'
>
> &
Pick<MongoOptions, 'monitorCommands' | 'ca' | 'crl' | 'key' | 'cert'>;
Pick<
MongoOptions,
| 'monitorCommands'
| 'ca'
| 'crl'
| 'key'
| 'cert'
| 'driverInfo'
| 'additionalDriverInfo'
| 'metadata'
| 'extendedMetadata'
>;

constructor(url: string, options?: MongoClientOptions) {
super();
Expand Down Expand Up @@ -455,6 +481,18 @@ export class MongoClient extends TypedEventEmitter<MongoClientEvents> implements
await this.close();
}

/**
* Append metadata to the client metadata after instantiation.
* @param driverInfo - Information about the application or library.
*/
appendMetadata(driverInfo: DriverInfo) {
this.options.additionalDriverInfo.push(driverInfo);
this.options.metadata = makeClientMetadata(this.options);
this.options.extendedMetadata = addContainerMetadata(this.options.metadata)
.then(undefined, squashError)
.then(result => result ?? {}); // ensure Promise<Document>
}

/** @internal */
private checkForNonGenuineHosts() {
const documentDBHostnames = this.options.hosts.filter((hostAddress: HostAddress) =>
Expand Down Expand Up @@ -1037,8 +1075,8 @@ export interface MongoOptions
dbName: string;
/** @deprecated - Will be made internal in a future major release. */
metadata: ClientMetadata;
/** @internal */
Copy link
Member Author

Choose a reason for hiding this comment

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

This put Claude in an infinite loop. We omit/pick some of our readonly client options in order to modify them while the rest remain immutable. This means they cannot be internal if we want to modify them in this fashion, and now the metadata on the client with this feature is no longer immutable.

extendedMetadata: Promise<Document>;
additionalDriverInfo: DriverInfo[];
/** @internal */
autoEncrypter?: AutoEncrypter;
/** @internal */
Expand Down
167 changes: 167 additions & 0 deletions test/integration/mongodb-handshake/mongodb-handshake.prose.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
LEGACY_HELLO_COMMAND,
type MongoClient
} from '../../mongodb';
import { sleep } from '../../tools/utils';

type EnvironmentVariables = Array<[string, string]>;

Expand Down Expand Up @@ -194,3 +195,169 @@ describe('Handshake Prose Tests', function () {
});
});
});

describe('Client Metadata Update Prose Tests', function () {
let client: MongoClient;

afterEach(async function () {
await client?.close();
sinon.restore();
});

describe('Test 2: Multiple Successive Metadata Updates', function () {
let initialClientMetadata;
let updatedClientMetadata;

const tests = [
{ testCase: 1, name: 'framework', version: '2.0', platform: 'Framework Platform' },
{ testCase: 2, name: 'framework', version: '2.0' },
{ testCase: 3, name: 'framework', platform: 'Framework Platform' },
{ testCase: 4, name: 'framework' }
];

for (const { testCase, name, version, platform } of tests) {
context(`Case: ${testCase}`, function () {
// 1. Create a `MongoClient` instance with the following:
// - `maxIdleTimeMS` set to `1ms`
// - Wrapping library metadata:
// | Field | Value |
// | -------- | ---------------- |
// | name | library |
// | version | 1.2 |
// | platform | Library Platform |
// 2. Send a `ping` command to the server and verify that the command succeeds.
// 3. Save intercepted `client` document as `initialClientMetadata`.
// 4. Wait 5ms for the connection to become idle.
beforeEach(async function () {
client = this.configuration.newClient(
{},
{
maxIdleTimeMS: 1,
driverInfo: { name: 'library', version: '1.2', platform: 'Library Platform' }
}
);

sinon.stub(Connection.prototype, 'command').callsFake(async function (ns, cmd, options) {
// @ts-expect-error: sinon will place wrappedMethod on the command method.
const command = Connection.prototype.command.wrappedMethod.bind(this);

if (cmd.hello || cmd[LEGACY_HELLO_COMMAND]) {
if (!initialClientMetadata) {
initialClientMetadata = cmd.client;
} else {
updatedClientMetadata = cmd.client;
}
}
return command(ns, cmd, options);
});

await client.db('test').command({ ping: 1 });
await sleep(5);
});

it('appends the metadata', async function () {
// 1. Append the `DriverInfoOptions` from the selected test case to the `MongoClient` metadata.
// 2. Send a `ping` command to the server and verify:
// - The command succeeds.
// - The framework metadata is appended to the existing `DriverInfoOptions` in the `client.driver` fields of the `hello`
// command, with values separated by a pipe `|`.
client.appendMetadata({ name, version, platform });
await client.db('test').command({ ping: 1 });

expect(updatedClientMetadata.driver.name).to.equal(
name
? `${initialClientMetadata.driver.name}|${name}`
: initialClientMetadata.driver.name
Copy link
Contributor

Choose a reason for hiding this comment

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

Since all prose tests have an explicit string to test against, can we have a sanity check either here or in the before setup to ensure that at least the initial metadata matches the expected string?

);
expect(updatedClientMetadata.driver.version).to.equal(
version
? `${initialClientMetadata.driver.version}|${version}`
: initialClientMetadata.driver.version
);
expect(updatedClientMetadata.platform).to.equal(
platform
? `${initialClientMetadata.platform}|${platform}`
: initialClientMetadata.platform
);
expect(updatedClientMetadata.os).to.deep.equal(initialClientMetadata.os);
});
});
}
});

describe('Test 1: Test that the driver updates metadata', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

was this meant to be 2 and the first one 1?

let initialClientMetadata;
let updatedClientMetadata;

const tests = [
{ testCase: 1, name: 'framework', version: '2.0', platform: 'Framework Platform' },
{ testCase: 2, name: 'framework', version: '2.0' },
{ testCase: 3, name: 'framework', platform: 'Framework Platform' },
{ testCase: 4, name: 'framework' }
];

for (const { testCase, name, version, platform } of tests) {
context(`Case: ${testCase}`, function () {
// 1. Create a `MongoClient` instance with the following:
// - `maxIdleTimeMS` set to `1ms`
// 2. Append the following `DriverInfoOptions` to the `MongoClient` metadata:
// | Field | Value |
// | -------- | ---------------- |
// | name | library |
// | version | 1.2 |
// | platform | Library Platform |
// 3. Send a `ping` command to the server and verify that the command succeeds.
// 4. Save intercepted `client` document as `updatedClientMetadata`.
// 5. Wait 5ms for the connection to become idle.
beforeEach(async function () {
client = this.configuration.newClient({}, { maxIdleTimeMS: 1 });
client.appendMetadata({ name: 'library', version: '1.2', platform: 'Library Platform' });

sinon.stub(Connection.prototype, 'command').callsFake(async function (ns, cmd, options) {
// @ts-expect-error: sinon will place wrappedMethod on the command method.
const command = Connection.prototype.command.wrappedMethod.bind(this);

if (cmd.hello || cmd[LEGACY_HELLO_COMMAND]) {
if (!initialClientMetadata) {
initialClientMetadata = cmd.client;
} else {
updatedClientMetadata = cmd.client;
}
}
return command(ns, cmd, options);
});

await client.db('test').command({ ping: 1 });
await sleep(5);
});

it('appends the metadata', async function () {
// 1. Append the `DriverInfoOptions` from the selected test case to the `MongoClient` metadata.
// 2. Send a `ping` command to the server and verify:
// - The command succeeds.
// - The framework metadata is appended to the existing `DriverInfoOptions` in the `client.driver` fields of the `hello`
// command, with values separated by a pipe `|`.
client.appendMetadata({ name, version, platform });
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add the last test instruction here and above in L263? (I see the assertions that are meant to cover it - was os the only other field that gets set? I don't mind that we use it as a representative, but I want to at least have a comment that states that this is intentional)
- All other subfields in the `client` document remain unchanged from `initialClientMetadata`.

await client.db('test').command({ ping: 1 });

expect(updatedClientMetadata.driver.name).to.equal(
name
? `${initialClientMetadata.driver.name}|${name}`
: initialClientMetadata.driver.name
);
expect(updatedClientMetadata.driver.version).to.equal(
version
? `${initialClientMetadata.driver.version}|${version}`
: initialClientMetadata.driver.version
);
expect(updatedClientMetadata.platform).to.equal(
platform
? `${initialClientMetadata.platform}|${platform}`
: initialClientMetadata.platform
);
expect(updatedClientMetadata.os).to.deep.equal(initialClientMetadata.os);
});
});
}
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { join } from 'path';

import { loadSpecTests } from '../../spec';
import { runUnifiedSuite } from '../../tools/unified-spec-runner/runner';

describe('MongoDB Handshake Tests (Unified)', function () {
runUnifiedSuite(loadSpecTests(join('mongodb-handshake')));
});
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
},
"data": {
"failCommands": [
"isMaster",
"ismaster",
Copy link
Contributor

Choose a reason for hiding this comment

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

was this part of the spec test sync?

"hello"
],
"closeConnection": true,
Expand Down
Loading