feat: versioned api #2736
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. This ended up being the cause of a bug I had a very hard time tracking down. It only happens on There was a problem hiding this comment. 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 This is where
There was a problem hiding this comment. 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. |
||
| } | ||
|
|
||
| server.getMore( | ||
| cursorNs, | ||
| cursorId, | ||
|
|
@@ -620,15 +624,17 @@ function next( | |
|
|
||
| if (cursorId == null) { | ||
| // All cursors must operate within a session, one must be made implicitly if not explicitly provided | ||
| if (cursor[kSession] == null /*&& cursor[kTopology].hasSessionSupport()*/) { | ||
| cursor[kSession] = cursor[kTopology].startSession({ owner: cursor, explicit: false }); | ||
| } | ||
|
|
||
| cursor._initialize(cursor[kSession], (err, state) => { | ||
| if (state) { | ||
| const response = state.response; | ||
| cursor[kServer] = state.server; | ||
| if (state.session) { | ||
| cursor[kSession] = state.session; | ||
| } | ||
|
|
||
| if (response.cursor) { | ||
| cursor[kId] = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -211,6 +211,9 @@ export type { | |
| Auth, | ||
| DriverInfo, | ||
| MongoOptions, | ||
| ServerApi, | ||
|
There was a problem hiding this comment. 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) |
||
| ServerApiVersion, | ||
| ServerApiVersionId, | ||
| SupportedNodeConnectionOptions, | ||
| SupportedTLSConnectionOptions, | ||
| SupportedTLSSocketOptions, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,32 @@ 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 LogLevel = { | ||
| error: 'error', | ||
| warn: 'warn', | ||
| info: 'info', | ||
| debug: 'debug' | ||
| } as const; | ||
|
|
||
| /** @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 { | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment. I agree about it being idiomatic for nodejs 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; | ||
|
There was a problem hiding this comment. Instead of freezing the There was a problem hiding this comment. I think There was a problem hiding this comment. Indeed the |
||
| strict?: boolean; | ||
| deprecationErrors?: boolean; | ||
| } | ||
|
|
||
| /** @public */ | ||
| export type LogLevelId = typeof LogLevel[keyof typeof LogLevel]; | ||
|
|
||
| /** @public */ | ||
| export interface DriverInfo { | ||
| name?: string; | ||
|
|
@@ -198,6 +224,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 */ | ||
|
|
@@ -218,6 +246,7 @@ export interface MongoClientPrivate { | |
| readConcern?: ReadConcern; | ||
| writeConcern?: WriteConcern; | ||
| readPreference: ReadPreference; | ||
| serverApi: ServerApi; | ||
| bsonOptions: BSONSerializeOptions; | ||
| namespace: MongoDBNamespace; | ||
| logger: Logger; | ||
|
|
@@ -296,6 +325,7 @@ export class MongoClient extends EventEmitter { | |
| readConcern: this[kOptions].readConcern, | ||
| writeConcern: this[kOptions].writeConcern, | ||
| readPreference: this[kOptions].readPreference, | ||
| serverApi: this[kOptions].serverApi, | ||
emadum marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| bsonOptions: resolveBSONOptions(this[kOptions]), | ||
| namespace: ns('admin'), | ||
| logger: this[kOptions].logger | ||
|
|
@@ -306,6 +336,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; | ||
| } | ||
|
|
@@ -601,6 +635,7 @@ export interface MongoOptions | |
| credentials?: MongoCredentials; | ||
| readPreference: ReadPreference; | ||
| readConcern: ReadConcern; | ||
| serverApi: ServerApi; | ||
| writeConcern: WriteConcern; | ||
| dbName: string; | ||
| metadata: ClientMetadata; | ||
|
|
||
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
MongoClientOptionswithout 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.