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.

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

fix(NODE-3998): metadata duplication in handshake #3615

Merged
merged 12 commits into from
Mar 31, 2023
8 changes: 2 additions & 6 deletions src/cmap/auth/auth_provider.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
import type { Document } from '../../bson';
import { MongoRuntimeError } from '../../error';
import type { ClientMetadataOptions } from '../../utils';
import type { HandshakeDocument } from '../connect';
import type { Connection, ConnectionOptions } from '../connection';
import type { MongoCredentials } from './mongo_credentials';

/** @internal */
export type AuthContextOptions = ConnectionOptions & ClientMetadataOptions;

/**
* Context used during authentication
* @internal
Expand All @@ -20,7 +16,7 @@ export class AuthContext {
/** If the context is for reauthentication. */
reauthenticating = false;
/** The options passed to the `connect` method */
options: AuthContextOptions;
options: ConnectionOptions;

/** A response from an initial auth attempt, only some mechanisms use this (e.g, SCRAM) */
response?: Document;
Expand All @@ -30,7 +26,7 @@ export class AuthContext {
constructor(
connection: Connection,
credentials: MongoCredentials | undefined,
options: AuthContextOptions
options: ConnectionOptions
) {
this.connection = connection;
this.credentials = credentials;
Expand Down
4 changes: 2 additions & 2 deletions src/cmap/connect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
MongoRuntimeError,
needsRetryableWriteLabel
} from '../error';
import { Callback, ClientMetadata, HostAddress, makeClientMetadata, ns } from '../utils';
import { Callback, ClientMetadata, HostAddress, ns } from '../utils';
import { AuthContext, AuthProvider } from './auth/auth_provider';
import { GSSAPI } from './auth/gssapi';
import { MongoCR } from './auth/mongocr';
Expand Down Expand Up @@ -213,7 +213,7 @@ export async function prepareHandshakeDocument(
const handshakeDoc: HandshakeDocument = {
[serverApi?.version ? 'hello' : LEGACY_HELLO_COMMAND]: 1,
helloOk: true,
client: options.metadata || makeClientMetadata(options),
client: options.metadata,
compression: compressors
};

Expand Down
19 changes: 5 additions & 14 deletions src/connection_string.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {
MongoParseError
} from './error';
import {
DriverInfo,
MongoClient,
MongoClientOptions,
MongoOptions,
Expand Down Expand Up @@ -543,6 +542,8 @@ export function parseOptions(
loggerClientOptions
);

mongoOptions.metadata = makeClientMetadata(mongoOptions);

return mongoOptions;
}

Expand Down Expand Up @@ -644,10 +645,7 @@ interface OptionDescriptor {

export const OPTIONS = {
appName: {
target: 'metadata',
transform({ options, values: [value] }): DriverInfo {
return makeClientMetadata({ ...options.driverInfo, appName: String(value) });
}
type: 'string'
},
auth: {
target: 'credentials',
Expand Down Expand Up @@ -798,15 +796,8 @@ export const OPTIONS = {
type: 'boolean'
},
driverInfo: {
target: 'metadata',
default: makeClientMetadata(),
transform({ options, values: [value] }) {
if (!isRecord(value)) throw new MongoParseError('DriverInfo must be an object');
return makeClientMetadata({
driverInfo: value,
appName: options.metadata?.application?.name
});
}
default: {},
type: 'record'
},
enableUtf8Validation: { type: 'boolean', default: true },
family: {
Expand Down
2 changes: 1 addition & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ export type {
ResumeToken,
UpdateDescription
} from './change_stream';
export type { AuthContext, AuthContextOptions } from './cmap/auth/auth_provider';
export type { AuthContext } from './cmap/auth/auth_provider';
export type {
AuthMechanismProperties,
MongoCredentials,
Expand Down
1 change: 1 addition & 0 deletions src/mongo_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -700,6 +700,7 @@ export interface MongoOptions
>
>,
SupportedNodeConnectionOptions {
appName?: string;
hosts: HostAddress[];
srvHost?: string;
credentials?: MongoCredentials;
Expand Down
50 changes: 23 additions & 27 deletions src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
MongoRuntimeError
} from './error';
import type { Explain } from './explain';
import type { MongoClient } from './mongo_client';
import type { MongoClient, MongoOptions } from './mongo_client';
import type { CommandOperationOptions, OperationParent } from './operations/command';
import type { Hint, OperationOptions } from './operations/operation';
import { ReadConcern } from './read_concern';
Expand Down Expand Up @@ -513,7 +513,10 @@ export function makeStateMachine(stateTable: StateTable): StateTransitionFunctio
};
}

/** @public */
/**
* @public
* @see https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#hello-command
*/
export interface ClientMetadata {
driver: {
name: string;
Expand All @@ -526,7 +529,6 @@ export interface ClientMetadata {
version: string;
};
platform: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was wrong, according to the spec: https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#hello-command

this modifies a public interface but it's also technically a bug fix, so I think it's okay to remove

version?: string;
application?: {
name: string;
};
Expand All @@ -545,44 +547,38 @@ export interface ClientMetadataOptions {
// eslint-disable-next-line @typescript-eslint/no-var-requires
const NODE_DRIVER_VERSION = require('../package.json').version;

export function makeClientMetadata(options?: ClientMetadataOptions): ClientMetadata {
options = options ?? {};
export function makeClientMetadata(
options: Pick<MongoOptions, 'appName' | 'driverInfo'>
): ClientMetadata {
const name = options.driverInfo.name ? `nodejs|${options.driverInfo.name}` : 'nodejs';
const version = options.driverInfo.version
? `${NODE_DRIVER_VERSION}|${options.driverInfo.version}`
: NODE_DRIVER_VERSION;
const platform = options.driverInfo.platform
? `Node.js ${process.version}, ${os.endianness()}|${options.driverInfo.platform}`
: `Node.js ${process.version}, ${os.endianness()}`;

const metadata: ClientMetadata = {
driver: {
name: 'nodejs',
version: NODE_DRIVER_VERSION
name,
version
},
os: {
type: os.type(),
name: process.platform,
architecture: process.arch,
version: os.release()
},
platform: `Node.js ${process.version}, ${os.endianness()} (unified)`
platform
};

// support optionally provided wrapping driver info
if (options.driverInfo) {
if (options.driverInfo.name) {
metadata.driver.name = `${metadata.driver.name}|${options.driverInfo.name}`;
}

if (options.driverInfo.version) {
metadata.version = `${metadata.driver.version}|${options.driverInfo.version}`;
}

if (options.driverInfo.platform) {
metadata.platform = `${metadata.platform}|${options.driverInfo.platform}`;
}
}

if (options.appName) {
// MongoDB requires the appName not exceed a byte length of 128
const buffer = Buffer.from(options.appName);
metadata.application = {
name: buffer.byteLength > 128 ? buffer.slice(0, 128).toString('utf8') : options.appName
};
const name =
Buffer.byteLength(options.appName, 'utf8') <= 128
? options.appName
: Buffer.from(options.appName, 'utf8').subarray(0, 128).toString('utf8');
metadata.application = { name };
}

return metadata;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ import { expect } from 'chai';
import {
connect,
Connection,
ConnectionOptions,
LEGACY_HELLO_COMMAND,
makeClientMetadata,
MongoClient,
MongoServerError,
ns,
Expand Down Expand Up @@ -31,12 +33,13 @@ describe('Connection', function () {
it('should execute a command against a server', {
metadata: { requires: { apiVersion: false, topology: '!load-balanced' } },
test: function (done) {
const connectOptions = Object.assign(
{ connectionType: Connection },
this.configuration.options
);
const connectOptions: Partial<ConnectionOptions> = {
connectionType: Connection,
...this.configuration.options,
metadata: makeClientMetadata({ driverInfo: {} })
};

connect(connectOptions, (err, conn) => {
connect(connectOptions as any as ConnectionOptions, (err, conn) => {
expect(err).to.not.exist;
this.defer(_done => conn.destroy(_done));

Expand All @@ -53,12 +56,14 @@ describe('Connection', function () {
it('should emit command monitoring events', {
metadata: { requires: { apiVersion: false, topology: '!load-balanced' } },
test: function (done) {
const connectOptions = Object.assign(
{ connectionType: Connection, monitorCommands: true },
this.configuration.options
);

connect(connectOptions, (err, conn) => {
const connectOptions: Partial<ConnectionOptions> = {
connectionType: Connection,
monitorCommands: true,
...this.configuration.options,
metadata: makeClientMetadata({ driverInfo: {} })
};

connect(connectOptions as any as ConnectionOptions, (err, conn) => {
expect(err).to.not.exist;
this.defer(_done => conn.destroy(_done));

Expand All @@ -84,12 +89,13 @@ describe('Connection', function () {
},
test: function (done) {
const namespace = ns(`${this.configuration.db}.$cmd`);
const connectOptions = Object.assign(
{ connectionType: Connection },
this.configuration.options
);
const connectOptions: Partial<ConnectionOptions> = {
connectionType: Connection,
...this.configuration.options,
metadata: makeClientMetadata({ driverInfo: {} })
};

connect(connectOptions, (err, conn) => {
connect(connectOptions as any as ConnectionOptions, (err, conn) => {
expect(err).to.not.exist;
this.defer(_done => conn.destroy(_done));

Expand Down
5 changes: 4 additions & 1 deletion test/integration/node-specific/topology.test.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
'use strict';
const { expect } = require('chai');
const { makeClientMetadata } = require('../../mongodb');

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

const states = [];
topology.on('stateChanged', (_, newState) => states.push(newState));
Expand Down
7 changes: 2 additions & 5 deletions test/tools/cmap_spec_runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -370,11 +370,8 @@ async function runCmapTest(test: CmapTest, threadContext: ThreadContext) {
delete poolOptions.backgroundThreadIntervalMS;
}

let metadata;
if (poolOptions.appName) {
metadata = makeClientMetadata({ appName: poolOptions.appName });
delete poolOptions.appName;
}
const metadata = makeClientMetadata({ appName: poolOptions.appName, driverInfo: {} });
delete poolOptions.appName;

const operations = test.operations;
const expectedError = test.error;
Expand Down
Loading