Skip to content

Commit b790142

Browse files
nbbeekenbaileympearson
andauthoredApr 7, 2023
fix(NODE-5161): metadata duplication in handshake (#3628)
Co-authored-by: Bailey Pearson <bailey.pearson@mongodb.com>

File tree

10 files changed

+225
-84
lines changed

10 files changed

+225
-84
lines changed
 

‎src/cmap/auth/auth_provider.ts

+6-3
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,17 @@ import type { MongoCredentials } from './mongo_credentials';
77

88
export type AuthContextOptions = ConnectionOptions & ClientMetadataOptions;
99

10-
/** Context used during authentication */
10+
/**
11+
* Context used during authentication
12+
* @internal
13+
*/
1114
export class AuthContext {
1215
/** The connection to authenticate */
1316
connection: Connection;
1417
/** The credentials to use for authentication */
1518
credentials?: MongoCredentials;
1619
/** The options passed to the `connect` method */
17-
options: AuthContextOptions;
20+
options: ConnectionOptions;
1821

1922
/** A response from an initial auth attempt, only some mechanisms use this (e.g, SCRAM) */
2023
response?: Document;
@@ -24,7 +27,7 @@ export class AuthContext {
2427
constructor(
2528
connection: Connection,
2629
credentials: MongoCredentials | undefined,
27-
options: AuthContextOptions
30+
options: ConnectionOptions
2831
) {
2932
this.connection = connection;
3033
this.credentials = credentials;

‎src/cmap/connect.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import {
1818
MongoServerError,
1919
needsRetryableWriteLabel
2020
} from '../error';
21-
import { Callback, ClientMetadata, HostAddress, makeClientMetadata, ns } from '../utils';
21+
import { Callback, ClientMetadata, HostAddress, ns } from '../utils';
2222
import { AuthContext, AuthProvider } from './auth/auth_provider';
2323
import { GSSAPI } from './auth/gssapi';
2424
import { MongoCR } from './auth/mongocr';
@@ -233,7 +233,7 @@ export function prepareHandshakeDocument(
233233
const handshakeDoc: HandshakeDocument = {
234234
[serverApi?.version ? 'hello' : LEGACY_HELLO_COMMAND]: true,
235235
helloOk: true,
236-
client: options.metadata || makeClientMetadata(options),
236+
client: options.metadata,
237237
compression: compressors
238238
};
239239

‎src/connection_string.ts

+5-14
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import {
1616
} from './error';
1717
import { Logger as LegacyLogger, LoggerLevel as LegacyLoggerLevel } from './logger';
1818
import {
19-
DriverInfo,
2019
MongoClient,
2120
MongoClientOptions,
2221
MongoOptions,
@@ -534,6 +533,8 @@ export function parseOptions(
534533
loggerClientOptions
535534
);
536535

536+
mongoOptions.metadata = makeClientMetadata(mongoOptions);
537+
537538
return mongoOptions;
538539
}
539540

@@ -635,10 +636,7 @@ interface OptionDescriptor {
635636

636637
export const OPTIONS = {
637638
appName: {
638-
target: 'metadata',
639-
transform({ options, values: [value] }): DriverInfo {
640-
return makeClientMetadata({ ...options.driverInfo, appName: String(value) });
641-
}
639+
type: 'string'
642640
},
643641
auth: {
644642
target: 'credentials',
@@ -784,15 +782,8 @@ export const OPTIONS = {
784782
type: 'boolean'
785783
},
786784
driverInfo: {
787-
target: 'metadata',
788-
default: makeClientMetadata(),
789-
transform({ options, values: [value] }) {
790-
if (!isRecord(value)) throw new MongoParseError('DriverInfo must be an object');
791-
return makeClientMetadata({
792-
driverInfo: value,
793-
appName: options.metadata?.application?.name
794-
});
795-
}
785+
default: {},
786+
type: 'record'
796787
},
797788
enableUtf8Validation: { type: 'boolean', default: true },
798789
family: {

‎src/mongo_client.ts

+1
Original file line numberDiff line numberDiff line change
@@ -770,6 +770,7 @@ export interface MongoOptions
770770
>
771771
>,
772772
SupportedNodeConnectionOptions {
773+
appName?: string;
773774
hosts: HostAddress[];
774775
srvHost?: string;
775776
credentials?: MongoCredentials;

‎src/utils.ts

+23-27
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import {
2020
MongoRuntimeError
2121
} from './error';
2222
import type { Explain } from './explain';
23-
import type { MongoClient } from './mongo_client';
23+
import type { MongoClient, MongoOptions } from './mongo_client';
2424
import type { CommandOperationOptions, OperationParent } from './operations/command';
2525
import type { Hint, OperationOptions } from './operations/operation';
2626
import { PromiseProvider } from './promise_provider';
@@ -657,7 +657,10 @@ export function makeStateMachine(stateTable: StateTable): StateTransitionFunctio
657657
};
658658
}
659659

660-
/** @public */
660+
/**
661+
* @public
662+
* @see https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#hello-command
663+
*/
661664
export interface ClientMetadata {
662665
driver: {
663666
name: string;
@@ -670,7 +673,6 @@ export interface ClientMetadata {
670673
version: string;
671674
};
672675
platform: string;
673-
version?: string;
674676
application?: {
675677
name: string;
676678
};
@@ -689,44 +691,38 @@ export interface ClientMetadataOptions {
689691
// eslint-disable-next-line @typescript-eslint/no-var-requires
690692
const NODE_DRIVER_VERSION = require('../package.json').version;
691693

692-
export function makeClientMetadata(options?: ClientMetadataOptions): ClientMetadata {
693-
options = options ?? {};
694+
export function makeClientMetadata(
695+
options: Pick<MongoOptions, 'appName' | 'driverInfo'>
696+
): ClientMetadata {
697+
const name = options.driverInfo.name ? `nodejs|${options.driverInfo.name}` : 'nodejs';
698+
const version = options.driverInfo.version
699+
? `${NODE_DRIVER_VERSION}|${options.driverInfo.version}`
700+
: NODE_DRIVER_VERSION;
701+
const platform = options.driverInfo.platform
702+
? `Node.js ${process.version}, ${os.endianness()}|${options.driverInfo.platform}`
703+
: `Node.js ${process.version}, ${os.endianness()}`;
694704

695705
const metadata: ClientMetadata = {
696706
driver: {
697-
name: 'nodejs',
698-
version: NODE_DRIVER_VERSION
707+
name,
708+
version
699709
},
700710
os: {
701711
type: os.type(),
702712
name: process.platform,
703713
architecture: process.arch,
704714
version: os.release()
705715
},
706-
platform: `Node.js ${process.version}, ${os.endianness()} (unified)`
716+
platform
707717
};
708718

709-
// support optionally provided wrapping driver info
710-
if (options.driverInfo) {
711-
if (options.driverInfo.name) {
712-
metadata.driver.name = `${metadata.driver.name}|${options.driverInfo.name}`;
713-
}
714-
715-
if (options.driverInfo.version) {
716-
metadata.version = `${metadata.driver.version}|${options.driverInfo.version}`;
717-
}
718-
719-
if (options.driverInfo.platform) {
720-
metadata.platform = `${metadata.platform}|${options.driverInfo.platform}`;
721-
}
722-
}
723-
724719
if (options.appName) {
725720
// MongoDB requires the appName not exceed a byte length of 128
726-
const buffer = Buffer.from(options.appName);
727-
metadata.application = {
728-
name: buffer.byteLength > 128 ? buffer.slice(0, 128).toString('utf8') : options.appName
729-
};
721+
const name =
722+
Buffer.byteLength(options.appName, 'utf8') <= 128
723+
? options.appName
724+
: Buffer.from(options.appName, 'utf8').subarray(0, 128).toString('utf8');
725+
metadata.application = { name };
730726
}
731727

732728
return metadata;

‎test/integration/connection-monitoring-and-pooling/connection.test.ts

+21-17
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { connect } from '../../../src/cmap/connect';
55
import { Connection } from '../../../src/cmap/connection';
66
import { LEGACY_HELLO_COMMAND } from '../../../src/constants';
77
import { Topology } from '../../../src/sdam/topology';
8-
import { ns } from '../../../src/utils';
8+
import { makeClientMetadata, ns } from '../../../src/utils';
99
import { skipBrokenAuthTestBeforeEachHook } from '../../tools/runner/hooks/configuration';
1010
import { assert as test, setupDatabase } from '../shared';
1111

@@ -27,12 +27,13 @@ describe('Connection', function () {
2727
it('should execute a command against a server', {
2828
metadata: { requires: { apiVersion: false, topology: '!load-balanced' } },
2929
test: function (done) {
30-
const connectOptions = Object.assign(
31-
{ connectionType: Connection },
32-
this.configuration.options
33-
);
30+
const connectOptions: Partial<ConnectionOptions> = {
31+
connectionType: Connection,
32+
...this.configuration.options,
33+
metadata: makeClientMetadata({ driverInfo: {} })
34+
};
3435

35-
connect(connectOptions, (err, conn) => {
36+
connect(connectOptions as any as ConnectionOptions, (err, conn) => {
3637
expect(err).to.not.exist;
3738
this.defer(_done => conn.destroy(_done));
3839

@@ -49,12 +50,14 @@ describe('Connection', function () {
4950
it('should emit command monitoring events', {
5051
metadata: { requires: { apiVersion: false, topology: '!load-balanced' } },
5152
test: function (done) {
52-
const connectOptions = Object.assign(
53-
{ connectionType: Connection, monitorCommands: true },
54-
this.configuration.options
55-
);
56-
57-
connect(connectOptions, (err, conn) => {
53+
const connectOptions: Partial<ConnectionOptions> = {
54+
connectionType: Connection,
55+
monitorCommands: true,
56+
...this.configuration.options,
57+
metadata: makeClientMetadata({ driverInfo: {} })
58+
};
59+
60+
connect(connectOptions as any as ConnectionOptions, (err, conn) => {
5861
expect(err).to.not.exist;
5962
this.defer(_done => conn.destroy(_done));
6063

@@ -80,12 +83,13 @@ describe('Connection', function () {
8083
},
8184
test: function (done) {
8285
const namespace = ns(`${this.configuration.db}.$cmd`);
83-
const connectOptions = Object.assign(
84-
{ connectionType: Connection },
85-
this.configuration.options
86-
);
86+
const connectOptions: Partial<ConnectionOptions> = {
87+
connectionType: Connection,
88+
...this.configuration.options,
89+
metadata: makeClientMetadata({ driverInfo: {} })
90+
};
8791

88-
connect(connectOptions, (err, conn) => {
92+
connect(connectOptions as any as ConnectionOptions, (err, conn) => {
8993
expect(err).to.not.exist;
9094
this.defer(_done => conn.destroy(_done));
9195

‎test/integration/node-specific/topology.test.js

+4-1
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,14 @@
11
'use strict';
22
const { expect } = require('chai');
3+
const { makeClientMetadata } = require('../../../src/utils');
34

45
describe('Topology', function () {
56
it('should correctly track states of a topology', {
67
metadata: { requires: { apiVersion: false, topology: '!load-balanced' } }, // apiVersion not supported by newTopology()
78
test: function (done) {
8-
const topology = this.configuration.newTopology();
9+
const topology = this.configuration.newTopology({
10+
metadata: makeClientMetadata({ driverInfo: {} })
11+
});
912

1013
const states = [];
1114
topology.on('stateChanged', (_, newState) => states.push(newState));

‎test/tools/cmap_spec_runner.ts

+2-5
Original file line numberDiff line numberDiff line change
@@ -363,11 +363,8 @@ async function runCmapTest(test: CmapTest, threadContext: ThreadContext) {
363363
delete poolOptions.backgroundThreadIntervalMS;
364364
}
365365

366-
let metadata;
367-
if (poolOptions.appName) {
368-
metadata = makeClientMetadata({ appName: poolOptions.appName });
369-
delete poolOptions.appName;
370-
}
366+
const metadata = makeClientMetadata({ appName: poolOptions.appName, driverInfo: {} });
367+
delete poolOptions.appName;
371368

372369
const operations = test.operations;
373370
const expectedError = test.error;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
import { expect } from 'chai';
2+
import * as os from 'os';
3+
4+
import { makeClientMetadata } from '../../../../src/utils';
5+
6+
// eslint-disable-next-line @typescript-eslint/no-var-requires
7+
const NODE_DRIVER_VERSION = require('../../../../package.json').version;
8+
9+
describe('makeClientMetadata()', () => {
10+
context('when driverInfo.platform is provided', () => {
11+
it('appends driverInfo.platform to the platform field', () => {
12+
const options = {
13+
driverInfo: { platform: 'myPlatform' }
14+
};
15+
const metadata = makeClientMetadata(options);
16+
expect(metadata).to.deep.equal({
17+
driver: {
18+
name: 'nodejs',
19+
version: NODE_DRIVER_VERSION
20+
},
21+
os: {
22+
type: os.type(),
23+
name: process.platform,
24+
architecture: process.arch,
25+
version: os.release()
26+
},
27+
platform: `Node.js ${process.version}, ${os.endianness()}|myPlatform`
28+
});
29+
});
30+
});
31+
32+
context('when driverInfo.name is provided', () => {
33+
it('appends driverInfo.name to the driver.name field', () => {
34+
const options = {
35+
driverInfo: { name: 'myName' }
36+
};
37+
const metadata = makeClientMetadata(options);
38+
expect(metadata).to.deep.equal({
39+
driver: {
40+
name: 'nodejs|myName',
41+
version: NODE_DRIVER_VERSION
42+
},
43+
os: {
44+
type: os.type(),
45+
name: process.platform,
46+
architecture: process.arch,
47+
version: os.release()
48+
},
49+
platform: `Node.js ${process.version}, ${os.endianness()}`
50+
});
51+
});
52+
});
53+
54+
context('when driverInfo.version is provided', () => {
55+
it('appends driverInfo.version to the version field', () => {
56+
const options = {
57+
driverInfo: { version: 'myVersion' }
58+
};
59+
const metadata = makeClientMetadata(options);
60+
expect(metadata).to.deep.equal({
61+
driver: {
62+
name: 'nodejs',
63+
version: `${NODE_DRIVER_VERSION}|myVersion`
64+
},
65+
os: {
66+
type: os.type(),
67+
name: process.platform,
68+
architecture: process.arch,
69+
version: os.release()
70+
},
71+
platform: `Node.js ${process.version}, ${os.endianness()}`
72+
});
73+
});
74+
});
75+
76+
context('when no custom driverInfo is provided', () => {
77+
const metadata = makeClientMetadata({ driverInfo: {} });
78+
79+
it('does not append the driver info to the metadata', () => {
80+
expect(metadata).to.deep.equal({
81+
driver: {
82+
name: 'nodejs',
83+
version: NODE_DRIVER_VERSION
84+
},
85+
os: {
86+
type: os.type(),
87+
name: process.platform,
88+
architecture: process.arch,
89+
version: os.release()
90+
},
91+
platform: `Node.js ${process.version}, ${os.endianness()}`
92+
});
93+
});
94+
95+
it('does not set the application field', () => {
96+
expect(metadata).not.to.have.property('application');
97+
});
98+
});
99+
100+
context('when app name is provided', () => {
101+
context('when the app name is over 128 bytes', () => {
102+
const longString = 'a'.repeat(300);
103+
const options = {
104+
appName: longString,
105+
driverInfo: {}
106+
};
107+
const metadata = makeClientMetadata(options);
108+
109+
it('truncates the application name to <=128 bytes', () => {
110+
expect(metadata.application?.name).to.be.a('string');
111+
// the above assertion fails if `metadata.application?.name` is undefined, so
112+
// we can safely assert that it exists
113+
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
114+
expect(Buffer.byteLength(metadata.application!.name, 'utf8')).to.equal(128);
115+
});
116+
});
117+
118+
context(
119+
'TODO(NODE-5150): fix appName truncation when multi-byte unicode charaters straddle byte 128',
120+
() => {
121+
const longString = '€'.repeat(300);
122+
const options = {
123+
appName: longString,
124+
driverInfo: {}
125+
};
126+
const metadata = makeClientMetadata(options);
127+
128+
it('truncates the application name to 129 bytes', () => {
129+
expect(metadata.application?.name).to.be.a('string');
130+
// the above assertion fails if `metadata.application?.name` is undefined, so
131+
// we can safely assert that it exists
132+
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
133+
expect(Buffer.byteLength(metadata.application!.name, 'utf8')).to.equal(129);
134+
});
135+
}
136+
);
137+
138+
context('when the app name is under 128 bytes', () => {
139+
const options = {
140+
appName: 'myApplication',
141+
driverInfo: {}
142+
};
143+
const metadata = makeClientMetadata(options);
144+
145+
it('sets the application name to the value', () => {
146+
expect(metadata.application?.name).to.equal('myApplication');
147+
});
148+
});
149+
});
150+
});

‎test/unit/sdam/topology.test.js

+11-15
Original file line numberDiff line numberDiff line change
@@ -38,15 +38,16 @@ describe('Topology (unit)', function () {
3838
it('should correctly pass appname', function (done) {
3939
const server = new Topology([`localhost:27017`], {
4040
metadata: makeClientMetadata({
41-
appName: 'My application name'
41+
appName: 'My application name',
42+
driverInfo: {}
4243
})
4344
});
4445

4546
expect(server.clientMetadata.application.name).to.equal('My application name');
4647
done();
4748
});
4849

49-
it('should report the correct platform in client metadata', function (done) {
50+
it('should report the correct platform in client metadata', async function () {
5051
const helloRequests = [];
5152
mockServer.setMessageHandler(request => {
5253
const doc = request.document;
@@ -59,22 +60,17 @@ describe('Topology (unit)', function () {
5960
});
6061

6162
client = new MongoClient(`mongodb://${mockServer.uri()}/`);
62-
client.connect(err => {
63-
expect(err).to.not.exist;
6463

65-
client.db().command({ ping: 1 }, err => {
66-
expect(err).to.not.exist;
64+
await client.connect();
6765

68-
expect(helloRequests).to.have.length.greaterThan(1);
69-
helloRequests.forEach(helloRequest =>
70-
expect(helloRequest)
71-
.nested.property('client.platform')
72-
.to.match(/unified/)
73-
);
66+
await client.db().command({ ping: 1 });
7467

75-
done();
76-
});
77-
});
68+
expect(helloRequests).to.have.length.greaterThan(1);
69+
for (const request of helloRequests) {
70+
expect(request)
71+
.nested.property('client.platform')
72+
.to.match(/Node.js /);
73+
}
7874
});
7975
});
8076

0 commit comments

Comments
 (0)
Please sign in to comment.