feat: versioned api #2736

emadum · 2021-02-09T01:55:10Z

Allows users to select an API version when connecting to a MongoDB instance.

test/functional/unified-spec-runner/match.ts

emadum · 2021-03-05T19:12:36Z

src/cursor/abstract_cursor.ts

@@ -572,6 +572,10 @@ export abstract class AbstractCursor extends EventEmitter {
      return;
    }

+    if (!this[kSession]) {
+      throw new Error('Should have a session when calling getMore');


This ended up being the cause of a bug I had a very hard time tracking down. It only happens on latest running with --setParameter requireApiVersion=1. I had to make the changes below to avoid this error being thrown in a number of tests. @nbbeeken might this be related to your sessions work?

Interesting, it could be, but its not obvious at first since it doesn't go through the client startSession method, its going directly to the topology one. If you can reproduce it can you take a look at:

node-mongodb-native/src/cursor/abstract_cursor.ts

Lines 623 to 625 in ee1a4d3

if (cursor[kSession] == null && cursor[kTopology].hasSessionSupport()) {

cursor[kSession] = cursor[kTopology].startSession({ owner: cursor, explicit: false });

}

This is where this[kSession] should be getting assigned so also printing the TopologyDescription with all the server descriptions as well could help

util.inspect(cursor[kTopology].s.description, {depth: 6}) (6 usually goes plenty deep)

Ok whoops, scrolled just a bit to see the changes you made to that section, that looks like the same issue to me, since you moved grabbing the session into the callback, then executeOperation has been called so server selection has run, now the function will return that sessions are supported. I am curious though what state the topology is in such that you're getting a failure, and then what state it transitions to after executeOperation runs.

emadum · 2021-03-05T19:16:51Z

src/mongo_client.ts

+export type ServerApiVersionId = typeof ServerApiVersion[keyof typeof ServerApiVersion];
+
+/** @public */
+export interface ServerApi {


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?

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.

src/mongo_client.ts

src/operations/command.ts

src/mongo_client.ts

emadum · 2021-03-05T23:26:59Z

src/operations/update.ts

@@ -274,7 +274,7 @@ export function makeUpdateStatement(
    op.upsert = options.upsert;
  }

-  if (typeof options.multi === 'boolean') {
+  if (options.multi) {


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.

durran · 2021-03-09T19:31:56Z

src/cmap/connection.ts

+    // a. only in the initial command of a transaction
+    // b. only in a Cursor's initiating command, not subsequent getMore commands
+    if (
+      this.serverApi &&


What do you think about extracting this into a separate method? I tend to personally prefer doing that with complex checks as it makes this code block a bit easier to read.

durran · 2021-03-09T19:41:20Z

src/connection_string.ts

@@ -572,6 +573,15 @@ export const OPTIONS = {
  autoEncryption: {
    type: 'record'
  },
+  serverApi: {


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.

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.

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

durran · 2021-03-09T19:50:14Z

src/mongo_client.ts

+
+/** @public */
+export interface ServerApi {
+  version: string | ServerApiVersionId;


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?

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

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.

Ok great, thanks!

src/sdam/topology.ts

test/functional/spec-runner/index.js

test/tools/runner/index.js

nbbeeken · 2021-03-17T14:49:58Z

src/index.ts

@@ -211,6 +211,9 @@ export type {
  Auth,
  DriverInfo,
  MongoOptions,
+  ServerApi,


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)

nbbeeken

LGTM

durran

LGTM! Nice job!

Allows users to select an API version when connecting to a MongoDB instance. NODE-2950

emadum added 2 commits February 8, 2021 20:54

feat: versioned api

612bb69

Merge remote-tracking branch 'origin/4.0' into NODE-2950/versioned-api

176a56b

emadum force-pushed the NODE-2950/versioned-api branch from 294f901 to 70f8935 Compare February 10, 2021 00:13

nbbeeken reviewed Feb 11, 2021

View reviewed changes

test/functional/unified-spec-runner/match.ts Outdated Show resolved Hide resolved

add versioned api tests to evergreen

8bfc97c

emadum force-pushed the NODE-2950/versioned-api branch from bd5cd04 to 8bfc97c Compare February 12, 2021 17:48

emadum added 5 commits February 12, 2021 13:58

Merge remote-tracking branch 'origin/4.0' into NODE-2950/versioned-api

8ffa41d

fix

92d13d3

remove session support check and unskip tests

9194a8c

fixes

a6e8951

skip error 85 index test on latest

d52d72f

emadum marked this pull request as ready for review March 5, 2021 18:44

emadum commented Mar 5, 2021

View reviewed changes

src/mongo_client.ts Show resolved Hide resolved

emadum commented Mar 5, 2021

View reviewed changes

src/operations/command.ts Outdated Show resolved Hide resolved

emadum requested review from nbbeeken and durran March 5, 2021 19:22

nbbeeken reviewed Mar 5, 2021

View reviewed changes

src/mongo_client.ts Outdated Show resolved Hide resolved

emadum added 2 commits March 5, 2021 18:12

review feedback

5b14147

remove duplicate loglevel exports

4c0310c

emadum commented Mar 5, 2021

View reviewed changes

emadum added 2 commits March 5, 2021 18:34

cleanup

ec53fcc

fix

88199d2

durran requested changes Mar 9, 2021

View reviewed changes

nbbeeken requested changes Mar 11, 2021

View reviewed changes

src/sdam/topology.ts Outdated Show resolved Hide resolved

test/functional/spec-runner/index.js Show resolved Hide resolved

test/tools/runner/index.js Outdated Show resolved Hide resolved

emadum added 2 commits March 17, 2021 10:07

Merge remote-tracking branch 'origin/4.0' into NODE-2950/versioned-api

7a2dba0

review feedback

d388f83

nbbeeken reviewed Mar 17, 2021

View reviewed changes

refactor: extract supportsVersionedApi helper function

b4893a1

emadum added 2 commits March 17, 2021 12:00

fix: disallow serverApi in uri options

78f68eb

fix: dont export ServerApiVersion as type

ea5cf9c

emadum requested review from nbbeeken and durran March 17, 2021 16:31

nbbeeken approved these changes Mar 22, 2021

View reviewed changes

durran approved these changes Mar 22, 2021

View reviewed changes

Merge remote-tracking branch 'origin/4.0' into NODE-2950/versioned-api

792b086

emadum merged commit 93f3ea5 into 4.0 Mar 22, 2021

emadum deleted the NODE-2950/versioned-api branch March 22, 2021 16:10

emadum mentioned this pull request Jun 16, 2021

feat(NODE-3191): backport versioned api #2850

Merged

ljhaywar pushed a commit that referenced this pull request Nov 9, 2021

feat: versioned api (#2736)

d7354c2

Allows users to select an API version when connecting to a MongoDB instance. NODE-2950

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: versioned api #2736

feat: versioned api #2736

emadum commented Feb 9, 2021 •

edited

Loading

emadum Mar 5, 2021

nbbeeken Mar 5, 2021

nbbeeken Mar 5, 2021

emadum Mar 5, 2021

nbbeeken Mar 5, 2021

emadum Mar 5, 2021

durran Mar 9, 2021

durran Mar 9, 2021

emadum Mar 17, 2021

emadum Mar 17, 2021

durran Mar 9, 2021

emadum Mar 17, 2021

nbbeeken Mar 17, 2021

durran Mar 19, 2021

nbbeeken Mar 17, 2021

nbbeeken left a comment

durran left a comment

	if (cursor[kSession] == null && cursor[kTopology].hasSessionSupport()) {
	cursor[kSession] = cursor[kTopology].startSession({ owner: cursor, explicit: false });
	}

feat: versioned api #2736

feat: versioned api #2736

Conversation

emadum commented Feb 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbbeeken left a comment

Choose a reason for hiding this comment

durran left a comment

Choose a reason for hiding this comment

emadum commented Feb 9, 2021 •

edited

Loading