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

feat: versioned api #2736

Merged
merged 18 commits into from
Mar 22, 2021
Merged
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
26 changes: 24 additions & 2 deletions .evergreen/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ functions:
MONGODB_VERSION=${VERSION} TOPOLOGY=${TOPOLOGY} \
AUTH=${AUTH} SSL=${SSL} \
ORCHESTRATION_FILE=${ORCHESTRATION_FILE} \
REQUIRE_API_VERSION=${REQUIRE_API_VERSION} \
bash ${DRIVERS_TOOLS}/.evergreen/run-orchestration.sh
- command: expansions.update
params:
Expand Down Expand Up @@ -125,8 +126,10 @@ functions:
rm -f ./prepare_client_encryption.sh
fi

AUTH=${AUTH} SSL=${SSL} UNIFIED=${UNIFIED} MONGODB_URI="${MONGODB_URI}" \
NODE_VERSION=${NODE_VERSION} SKIP_DEPS=1 NO_EXIT=1 \
MONGODB_URI="${MONGODB_URI}" \
AUTH=${AUTH} SSL=${SSL} UNIFIED=${UNIFIED} \
MONGODB_API_VERSION="${MONGODB_API_VERSION}" \
NODE_VERSION=${NODE_VERSION} SKIP_DEPS=${SKIP_DEPS|1} NO_EXIT=${NO_EXIT|1} \
bash ${PROJECT_DIRECTORY}/.evergreen/run-tests.sh
run checks:
- command: shell.exec
Expand Down Expand Up @@ -780,6 +783,22 @@ tasks:
VERSION: '2.6'
TOPOLOGY: sharded_cluster
- func: run tests
- name: test-latest-server-v1-api
tags:
- latest
- server
- v1-api
commands:
- func: install dependencies
- func: bootstrap mongo-orchestration
vars:
VERSION: latest
TOPOLOGY: server
REQUIRE_API_VERSION: '1'
- func: run tests
vars:
MONGODB_API_VERSION: '1'
NO_EXIT: ''
- name: test-atlas-connectivity
tags:
- atlas-connect
Expand Down Expand Up @@ -1192,6 +1211,7 @@ buildvariants:
- test-2.6-server
- test-2.6-replica_set
- test-2.6-sharded_cluster
- test-latest-server-v1-api
- test-atlas-connectivity
- test-atlas-data-lake
- test-auth-kerberos
Expand Down Expand Up @@ -1258,6 +1278,7 @@ buildvariants:
- test-2.6-server
- test-2.6-replica_set
- test-2.6-sharded_cluster
- test-latest-server-v1-api
- test-atlas-connectivity
- test-atlas-data-lake
- test-auth-kerberos
Expand Down Expand Up @@ -1353,6 +1374,7 @@ buildvariants:
- test-3.2-server
- test-3.2-replica_set
- test-3.2-sharded_cluster
- test-latest-server-v1-api
- test-atlas-connectivity
- test-atlas-data-lake
- test-auth-kerberos
Expand Down
7 changes: 5 additions & 2 deletions .evergreen/config.yml.in
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ functions:
MONGODB_VERSION=${VERSION} TOPOLOGY=${TOPOLOGY} \
AUTH=${AUTH} SSL=${SSL} \
ORCHESTRATION_FILE=${ORCHESTRATION_FILE} \
REQUIRE_API_VERSION=${REQUIRE_API_VERSION} \
bash ${DRIVERS_TOOLS}/.evergreen/run-orchestration.sh
# run-orchestration generates expansion file with the MONGODB_URI for the cluster
- command: expansions.update
Expand Down Expand Up @@ -145,8 +146,10 @@ functions:
rm -f ./prepare_client_encryption.sh
fi

AUTH=${AUTH} SSL=${SSL} UNIFIED=${UNIFIED} MONGODB_URI="${MONGODB_URI}" \
NODE_VERSION=${NODE_VERSION} SKIP_DEPS=1 NO_EXIT=1 \
MONGODB_URI="${MONGODB_URI}" \
AUTH=${AUTH} SSL=${SSL} UNIFIED=${UNIFIED} \
MONGODB_API_VERSION="${MONGODB_API_VERSION}" \
NODE_VERSION=${NODE_VERSION} SKIP_DEPS=${SKIP_DEPS|1} NO_EXIT=${NO_EXIT|1} \
bash ${PROJECT_DIRECTORY}/.evergreen/run-tests.sh

"run checks":
Expand Down
23 changes: 23 additions & 0 deletions .evergreen/generate_evergreen_tasks.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ const OPERATING_SYSTEMS = [
)
);

// TODO: NODE-3060: enable skipped tests on windows
const WINDOWS_SKIP_TAGS = new Set(['atlas-connect', 'auth']);

const TASKS = [];
Expand Down Expand Up @@ -87,6 +88,28 @@ const BASE_TASKS = [];
MONGODB_VERSIONS.forEach(mongoVersion => {
TOPOLOGIES.forEach(topology => BASE_TASKS.push(makeTask({ mongoVersion, topology })));
});
BASE_TASKS.push({
name: `test-latest-server-v1-api`,
tags: ['latest', 'server', 'v1-api'],
commands: [
{ func: 'install dependencies' },
{
func: 'bootstrap mongo-orchestration',
vars: {
VERSION: 'latest',
TOPOLOGY: 'server',
REQUIRE_API_VERSION: '1'
}
},
{
func: 'run tests',
vars: {
MONGODB_API_VERSION: '1',
NO_EXIT: ''
}
}
]
});

// manually added tasks
Array.prototype.push.apply(TASKS, [
Expand Down
2 changes: 1 addition & 1 deletion .evergreen/run-tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,4 @@ else
npm install mongodb-client-encryption@">=1.2.1"
fi

MONGODB_UNIFIED_TOPOLOGY=${UNIFIED} MONGODB_URI=${MONGODB_URI} npm run ${TEST_NPM_SCRIPT}
MONGODB_API_VERSION=${MONGODB_API_VERSION} MONGODB_UNIFIED_TOPOLOGY=${UNIFIED} MONGODB_URI=${MONGODB_URI} npm run ${TEST_NPM_SCRIPT}
2 changes: 1 addition & 1 deletion src/cmap/auth/scram.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as crypto from 'crypto';
import { Binary, Document } from '../../bson';
import { MongoError, AnyError } from '../../error';
import { AuthProvider, AuthContext } from './auth_provider';
import { Callback, emitWarning, ns } from '../../utils';
import { Callback, ns, emitWarning } from '../../utils';
import type { MongoCredentials } from './mongo_credentials';
import type { HandshakeDocument } from '../connect';

Expand Down
25 changes: 23 additions & 2 deletions src/cmap/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import { applyCommonQueryOptions, getReadPreference, isSharded } from './wire_pr
import { ReadPreference, ReadPreferenceLike } from '../read_preference';
import { isTransactionCommand } from '../transactions';
import type { W, WriteConcern, WriteConcernOptions } from '../write_concern';
import type { SupportedNodeConnectionOptions } from '../mongo_client';
import type { ServerApi, SupportedNodeConnectionOptions } from '../mongo_client';

const kStream = Symbol('stream');
const kQueue = Symbol('queue');
Expand Down Expand Up @@ -107,6 +107,7 @@ export interface ConnectionOptions
hostAddress: HostAddress;
// Settings
autoEncrypter?: AutoEncrypter;
serverApi?: ServerApi;
monitorCommands: boolean;
connectionType: typeof Connection;
credentials?: MongoCredentials;
Expand Down Expand Up @@ -136,6 +137,7 @@ export class Connection extends EventEmitter {
closed: boolean;
destroyed: boolean;
lastIsMasterMS?: number;
serverApi?: ServerApi;
/** @internal */
[kDescription]: StreamDescription;
/** @internal */
Expand Down Expand Up @@ -168,6 +170,7 @@ export class Connection extends EventEmitter {
this.address = streamIdentifier(stream);
this.socketTimeout = options.socketTimeout ?? 0;
this.monitorCommands = options.monitorCommands;
this.serverApi = options.serverApi;
this.closed = false;
this.destroyed = false;

Expand Down Expand Up @@ -317,6 +320,15 @@ export class Connection extends EventEmitter {

let clusterTime = this.clusterTime;
let finalCmd = Object.assign({}, cmd);
const inTransaction = session && (session.inTransaction() || isTransactionCommand(finalCmd));

if (this.serverApi && supportsVersionedApi(cmd, session)) {
const { version, strict, deprecationErrors } = this.serverApi;
finalCmd.apiVersion = version;
if (strict != null) finalCmd.apiStrict = strict;
if (deprecationErrors != null) finalCmd.apiDeprecationErrors = deprecationErrors;
}

if (hasSessionSupport(this) && session) {
if (
session.clusterTime &&
Expand Down Expand Up @@ -361,7 +373,6 @@ export class Connection extends EventEmitter {
? new Msg(cmdNs, finalCmd, commandOptions)
: new Query(cmdNs, finalCmd, commandOptions);

const inTransaction = session && (session.inTransaction() || isTransactionCommand(finalCmd));
const commandResponseHandler = inTransaction
? (err?: AnyError, ...args: Document[]) => {
// We need to add a TransientTransactionError errorLabel, as stated in the transaction spec.
Expand Down Expand Up @@ -630,6 +641,16 @@ function supportsOpMsg(conn: Connection) {
return maxWireVersion(conn) >= 6 && !description.__nodejs_mock_server__;
}

function supportsVersionedApi(cmd: Document, session?: ClientSession) {
const inTransaction = session && (session.inTransaction() || isTransactionCommand(cmd));
// if an API version was declared, add the apiVersion option to every command, except:
// a. only in the initial command of a transaction
// b. only in a Cursor's initiating command, not subsequent getMore commands
return (
(!inTransaction || session?.transaction.isStarting) && !cmd.commitTransaction && !cmd.getMore
);
}

function messageHandler(conn: Connection) {
return function messageHandler(message: BinMsg | Response) {
// always emit the message, in case we are streaming
Expand Down
18 changes: 17 additions & 1 deletion src/connection_string.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ import {
MongoClient,
MongoClientOptions,
MongoOptions,
PkFactory
PkFactory,
ServerApi
} from './mongo_client';
import { MongoCredentials } from './cmap/auth/mongo_credentials';
import type { TagSet } from './sdam/server_description';
Expand Down Expand Up @@ -323,6 +324,12 @@ export function parseOptions(
throw new MongoParseError('URI cannot contain options with no value');
}

if (key.toLowerCase() === 'serverapi') {
throw new MongoParseError(
'URI cannot contain `serverApi`, it can only be passed to the client'
);
}

if (key.toLowerCase() === 'authsource' && urlOptions.has('authSource')) {
// If authSource is an explicit key in the urlOptions we need to remove the implicit dbName
urlOptions.delete('authSource');
Expand Down Expand Up @@ -572,6 +579,15 @@ export const OPTIONS = {
autoEncryption: {
type: 'record'
},
serverApi: {
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean we are allowing the user to specify the api version in the connection string? The spec specifically states not to allow it as we don't want developers changing behaviour without code changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it seems that way, but I was unable to add the option to MongoClientOptions without adding an entry to this list of transformers; I'll take another look at this now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I added a check to throw an error if it is provided in the URI options, and a corresponding test.

target: 'serverApi',
transform({ values: [version] }): ServerApi {
if (typeof version === 'string') {
return { version };
}
return version as ServerApi;
}
},
checkKeys: {
type: 'boolean'
},
Expand Down
3 changes: 3 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ export { Compressor } from './cmap/wire_protocol/compression';
export { ExplainVerbosity } from './explain';
export { ReadConcernLevel } from './read_concern';
export { ReadPreferenceMode } from './read_preference';
export { ServerApiVersion } from './mongo_client';

// events
export {
Expand Down Expand Up @@ -211,6 +212,8 @@ export type {
Auth,
DriverInfo,
MongoOptions,
ServerApi,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah just noticed, we don't have tooling to catch this unfortunately but api-extractor is satisfied when this is exported as a type, but if we want users to be able to use the enum it has to be exported normally, you can see on line 76 is where I grouped the "enums".

Maybe a test/tooling we want to add (not here, just at some point) is importing index.js in our javascript tests and assert the actual imports we expect at runtime.

(@durran just pinging you here for knowledge share since we all might run into this gap in tooling)

ServerApiVersionId,
SupportedNodeConnectionOptions,
SupportedTLSConnectionOptions,
SupportedTLSSocketOptions,
Expand Down
22 changes: 22 additions & 0 deletions src/mongo_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,21 @@ import type { SrvPoller } from './sdam/srv_polling';
import type { Connection } from './cmap/connection';
import type { LEGAL_TLS_SOCKET_OPTIONS, LEGAL_TCP_SOCKET_OPTIONS } from './cmap/connect';

/** @public */
export const ServerApiVersion = {
emadum marked this conversation as resolved.
Show resolved Hide resolved
v1: '1'
};

/** @public */
export type ServerApiVersionId = typeof ServerApiVersion[keyof typeof ServerApiVersion];

/** @public */
export interface ServerApi {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spec calls for this to be a class, but I didn't think it would be idiomatic in Node to make users import the ServerApi class and pass a new ServerVersion(...) in the constructor, versus accepting a plain object and providing a TypeScript interface. Does this make sense, or should it be a class like the spec advises?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree about it being idiomatic for nodejs
I guess just to consider an alternative we could make a class similar to our ReadPreference where we have static predefined instances of it? looking something like

new MongoClient('...', { serverApi: ServerApi.V1 })

Just putting this out there to say we considered it but I think avoiding an import is more satisfying for users.

version: string | ServerApiVersionId;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of freezing the ServerApi on the getter in MongoClient, could we instead just define the properties as readonly here? Or does that not give us enough protection somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think readonly only applies at the level of the TypeScript compiler, so someone using plain JS would be able mutate the property. I'd be interested to hear your thoughts on this @nbbeeken

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed the readonly modifier only effects type checking. I think freezing is worth it here, since it is a construction time only setting, we can just jump ahead of any bugs trying to change serverApi at a later time by doing this.

Copy link
Member

Choose a reason for hiding this comment

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

Ok great, thanks!

strict?: boolean;
deprecationErrors?: boolean;
}

/** @public */
export interface DriverInfo {
name?: string;
Expand Down Expand Up @@ -198,6 +213,8 @@ export interface MongoClientOptions extends BSONSerializeOptions, SupportedNodeC
logger?: Logger;
/** Enable command monitoring for this client */
monitorCommands?: boolean;
/** Server API version */
serverApi?: ServerApi | ServerApiVersionId;
/** Optionally enable client side auto encryption */
autoEncryption?: AutoEncryptionOptions;
/** Allows a wrapping driver to amend the client metadata generated by the driver to include information about the wrapping driver */
Expand Down Expand Up @@ -306,6 +323,10 @@ export class MongoClient extends EventEmitter {
return Object.freeze({ ...this[kOptions] });
}

get serverApi(): Readonly<ServerApi | undefined> {
return this[kOptions].serverApi && Object.freeze({ ...this[kOptions].serverApi });
}

get autoEncrypter(): AutoEncrypter | undefined {
return this[kOptions].autoEncrypter;
}
Expand Down Expand Up @@ -597,6 +618,7 @@ export interface MongoOptions
credentials?: MongoCredentials;
readPreference: ReadPreference;
readConcern: ReadConcern;
serverApi: ServerApi;
writeConcern: WriteConcern;
dbName: string;
metadata: ClientMetadata;
Expand Down
2 changes: 1 addition & 1 deletion src/operations/update.ts
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ export function makeUpdateStatement(
op.upsert = options.upsert;
}

if (typeof options.multi === 'boolean') {
if (options.multi) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this should only be sent when true, since it's false by default - was causing some of the versioned api spec tests which weren't expecting multi: false to be there to fail.

op.multi = options.multi;
}

Expand Down
6 changes: 6 additions & 0 deletions src/sdam/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import type { ServerHeartbeatSucceededEvent } from './events';
import type { ClientSession } from '../sessions';
import type { Document, Long } from '../bson';
import type { AutoEncrypter } from '../deps';
import type { ServerApi } from '../mongo_client';

// Used for filtering out fields for logging
const DEBUG_FIELDS = [
Expand Down Expand Up @@ -100,12 +101,15 @@ export interface ServerPrivate {
topology: Topology;
/** A connection pool for this server */
pool: ConnectionPool;
/** MongoDB server API version */
serverApi?: ServerApi;
}

/** @public */
export class Server extends EventEmitter {
/** @internal */
s: ServerPrivate;
serverApi?: ServerApi;
clusterTime?: ClusterTime;
ismaster?: Document;
[kMonitor]: Monitor;
Expand All @@ -131,6 +135,8 @@ export class Server extends EventEmitter {
constructor(topology: Topology, description: ServerDescription, options: ServerOptions) {
super();

this.serverApi = options.serverApi;

const poolOptions = { hostAddress: description.hostAddress, ...options };

this.s = {
Expand Down
4 changes: 3 additions & 1 deletion src/sdam/topology.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ import type { MongoCredentials } from '../cmap/auth/mongo_credentials';
import type { Transaction } from '../transactions';
import type { CloseOptions } from '../cmap/connection_pool';
import { DestroyOptions, Connection } from '../cmap/connection';
import type { MongoClientOptions } from '../mongo_client';
import type { MongoClientOptions, ServerApi } from '../mongo_client';
import { DEFAULT_OPTIONS } from '../connection_string';
import { serialize, deserialize } from '../bson';

Expand Down Expand Up @@ -139,6 +139,8 @@ export interface TopologyOptions extends BSONSerializeOptions, ServerOptions {
/** Indicates that a client should directly connect to a node without attempting to discover its topology type */
directConnection: boolean;
metadata: ClientMetadata;
/** MongoDB server API version */
serverApi?: ServerApi;
}

/** @public */
Expand Down
5 changes: 5 additions & 0 deletions src/transactions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ export class Transaction {
return !!this.server;
}

/** @returns Whether the transaction has started */
get isStarting(): boolean {
return this.state === TxnState.STARTING_TRANSACTION;
}

/**
* @returns Whether this session is presently in a transaction
*/
Expand Down
Loading