-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat: versioned api #2736
Conversation
294f901
to
70f8935
Compare
bd5cd04
to
8bfc97c
Compare
src/cursor/abstract_cursor.ts
Outdated
@@ -572,6 +572,10 @@ export abstract class AbstractCursor extends EventEmitter { | |||
return; | |||
} | |||
|
|||
if (!this[kSession]) { | |||
throw new Error('Should have a session when calling getMore'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
export type ServerApiVersionId = typeof ServerApiVersion[keyof typeof ServerApiVersion]; | ||
|
||
/** @public */ | ||
export interface ServerApi { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@@ -274,7 +274,7 @@ export function makeUpdateStatement( | |||
op.upsert = options.upsert; | |||
} | |||
|
|||
if (typeof options.multi === 'boolean') { | |||
if (options.multi) { |
There was a problem hiding this comment.
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.
src/cmap/connection.ts
Outdated
// a. only in the initial command of a transaction | ||
// b. only in a Cursor's initiating command, not subsequent getMore commands | ||
if ( | ||
this.serverApi && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -572,6 +573,15 @@ export const OPTIONS = { | |||
autoEncryption: { | |||
type: 'record' | |||
}, | |||
serverApi: { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
||
/** @public */ | ||
export interface ServerApi { | ||
version: string | ServerApiVersionId; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok great, thanks!
@@ -211,6 +211,9 @@ export type { | |||
Auth, | |||
DriverInfo, | |||
MongoOptions, | |||
ServerApi, |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Nice job!
Allows users to select an API version when connecting to a MongoDB instance. NODE-2950
Allows users to select an API version when connecting to a MongoDB instance.
NODE-2950