-
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
Changes from 9 commits
612bb69
176a56b
8bfc97c
8ffa41d
92d13d3
9194a8c
a6e8951
d52d72f
5b14147
4c0310c
ec53fcc
88199d2
7a2dba0
d388f83
b4893a1
78f68eb
ea5cf9c
792b086
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -572,6 +573,15 @@ export const OPTIONS = { | |
autoEncryption: { | ||
type: 'record' | ||
}, | ||
serverApi: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' | ||
}, | ||
|
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. Choose a reason for hiding this commentThe 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) |
||
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. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of freezing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok great, thanks! |
||
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 */ | ||
|
@@ -306,6 +334,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 +633,7 @@ export interface MongoOptions | |
credentials?: MongoCredentials; | ||
readPreference: ReadPreference; | ||
readConcern: ReadConcern; | ||
serverApi: ServerApi; | ||
writeConcern: WriteConcern; | ||
dbName: string; | ||
metadata: ClientMetadata; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 |
||
op.multi = 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.
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.