From abaa176e82a98554a08d68bf4dbe8b1086282eaf Mon Sep 17 00:00:00 2001 From: Daria Pardue <81593090+dariakp@users.noreply.github.com> Date: Mon, 3 May 2021 14:24:43 -0400 Subject: [PATCH 1/6] refactor(NODE-3157)!: Remove legacy fields property from FindAndModifyOptions --- src/operations/find_and_modify.ts | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/operations/find_and_modify.ts b/src/operations/find_and_modify.ts index d0f5e87d846..5024dbf9106 100644 --- a/src/operations/find_and_modify.ts +++ b/src/operations/find_and_modify.ts @@ -23,8 +23,6 @@ export interface FindAndModifyOptions extends CommandOperationOptions { upsert?: boolean; /** Limits the fields to return for all matching documents. */ projection?: Document; - /** @deprecated use `projection` instead */ - fields?: Document; /** Determines which document the operation modifies if the query selects multiple documents. */ sort?: Sort; /** Optional list of array filters referenced in filtered positional operators */ @@ -88,10 +86,8 @@ export class FindAndModifyOperation extends CommandOperation { cmd.remove = options.remove ? true : false; cmd.upsert = options.upsert ? true : false; - const projection = options.projection || options.fields; - - if (projection) { - cmd.fields = projection; + if (options.projection) { + cmd.fields = options.projection; } if (options.arrayFilters) { @@ -162,7 +158,6 @@ export class FindOneAndDeleteOperation extends FindAndModifyOperation { constructor(collection: Collection, filter: Document, options: FindAndModifyOptions) { // Final options const finalOptions = Object.assign({}, options); - finalOptions.fields = options.projection; finalOptions.remove = true; // Basic validation @@ -184,7 +179,6 @@ export class FindOneAndReplaceOperation extends FindAndModifyOperation { ) { // Final options const finalOptions = Object.assign({}, options); - finalOptions.fields = options.projection; finalOptions.update = true; finalOptions.new = options.returnOriginal !== void 0 ? !options.returnOriginal : false; finalOptions.upsert = options.upsert !== void 0 ? !!options.upsert : false; @@ -215,7 +209,6 @@ export class FindOneAndUpdateOperation extends FindAndModifyOperation { ) { // Final options const finalOptions = Object.assign({}, options); - finalOptions.fields = options.projection; finalOptions.update = true; finalOptions.new = typeof options.returnOriginal === 'boolean' ? !options.returnOriginal : false; From 1a0180a23f13d3100ee063b9c8ee56d5cb59ac8b Mon Sep 17 00:00:00 2001 From: Daria Pardue <81593090+dariakp@users.noreply.github.com> Date: Mon, 3 May 2021 15:41:31 -0400 Subject: [PATCH 2/6] refactor(NODE-3157): Create separate TS interfaces for individual find and modify type commands --- src/collection.ts | 22 +++++++------- src/index.ts | 6 +++- src/operations/find_and_modify.ts | 48 ++++++++++++++++++++++++++++++- 3 files changed, 64 insertions(+), 12 deletions(-) diff --git a/src/collection.ts b/src/collection.ts index 3736954ad7e..095de86bc71 100644 --- a/src/collection.ts +++ b/src/collection.ts @@ -45,7 +45,9 @@ import { FindOneAndDeleteOperation, FindOneAndReplaceOperation, FindOneAndUpdateOperation, - FindAndModifyOptions + FindOneAndDeleteOptions, + FindOneAndReplaceOptions, + FindOneAndUpdateOptions } from './operations/find_and_modify'; import { InsertOneOperation, @@ -1098,15 +1100,15 @@ export class Collection { */ findOneAndDelete(filter: Document): Promise; findOneAndDelete(filter: Document, callback: Callback): void; - findOneAndDelete(filter: Document, options: FindAndModifyOptions): Promise; + findOneAndDelete(filter: Document, options: FindOneAndDeleteOptions): Promise; findOneAndDelete( filter: Document, - options: FindAndModifyOptions, + options: FindOneAndDeleteOptions, callback: Callback ): void; findOneAndDelete( filter: Document, - options?: FindAndModifyOptions | Callback, + options?: FindOneAndDeleteOptions | Callback, callback?: Callback ): Promise | void { if (typeof options === 'function') (callback = options), (options = {}); @@ -1131,18 +1133,18 @@ export class Collection { findOneAndReplace( filter: Document, replacement: Document, - options: FindAndModifyOptions + options: FindOneAndReplaceOptions ): Promise; findOneAndReplace( filter: Document, replacement: Document, - options: FindAndModifyOptions, + options: FindOneAndReplaceOptions, callback: Callback ): void; findOneAndReplace( filter: Document, replacement: Document, - options?: FindAndModifyOptions | Callback, + options?: FindOneAndReplaceOptions | Callback, callback?: Callback ): Promise | void { if (typeof options === 'function') (callback = options), (options = {}); @@ -1167,18 +1169,18 @@ export class Collection { findOneAndUpdate( filter: Document, update: Document, - options: FindAndModifyOptions + options: FindOneAndUpdateOptions ): Promise; findOneAndUpdate( filter: Document, update: Document, - options: FindAndModifyOptions, + options: FindOneAndUpdateOptions, callback: Callback ): void; findOneAndUpdate( filter: Document, update: Document, - options?: FindAndModifyOptions | Callback, + options?: FindOneAndUpdateOptions | Callback, callback?: Callback ): Promise | void { if (typeof options === 'function') (callback = options), (options = {}); diff --git a/src/index.ts b/src/index.ts index 5acbc023f99..f0ede7698d1 100644 --- a/src/index.ts +++ b/src/index.ts @@ -225,7 +225,11 @@ export type { EstimatedDocumentCountOptions } from './operations/estimated_docum export type { EvalOptions } from './operations/eval'; export type { FindOptions } from './operations/find'; export type { Sort, SortDirection } from './sort'; -export type { FindAndModifyOptions } from './operations/find_and_modify'; +export type { + FindOneAndDeleteOptions, + FindOneAndReplaceOptions, + FindOneAndUpdateOptions +} from './operations/find_and_modify'; export type { IndexSpecification, CreateIndexesOptions, diff --git a/src/operations/find_and_modify.ts b/src/operations/find_and_modify.ts index 5024dbf9106..b9ff0dc0165 100644 --- a/src/operations/find_and_modify.ts +++ b/src/operations/find_and_modify.ts @@ -16,7 +16,53 @@ import { Sort, formatSort } from '../sort'; import type { ClientSession } from '../sessions'; /** @public */ -export interface FindAndModifyOptions extends CommandOperationOptions { +export interface FindOneAndDeleteOptions extends CommandOperationOptions { + /** An optional hint for query optimization. See the {@link https://docs.mongodb.com/manual/reference/command/update/#update-command-hint|update command} reference for more information.*/ + hint?: Document; + /** Limits the fields to return for all matching documents. */ + projection?: Document; + /** Determines which document the operation modifies if the query selects multiple documents. */ + sort?: Sort; +} + +/** @public */ +export interface FindOneAndReplaceOptions extends CommandOperationOptions { + /** Allow driver to bypass schema validation in MongoDB 3.2 or higher. */ + bypassDocumentValidation?: boolean; + /** An optional hint for query optimization. See the {@link https://docs.mongodb.com/manual/reference/command/update/#update-command-hint|update command} reference for more information.*/ + hint?: Document; + /** Limits the fields to return for all matching documents. */ + projection?: Document; + /** When false, returns the updated document rather than the original. The default is true. */ + returnOriginal?: boolean; + /** Determines which document the operation modifies if the query selects multiple documents. */ + sort?: Sort; + /** Upsert the document if it does not exist. */ + upsert?: boolean; +} + +/** @public */ +export interface FindOneAndUpdateOptions extends CommandOperationOptions { + /** Optional list of array filters referenced in filtered positional operators */ + arrayFilters?: Document[]; + /** Allow driver to bypass schema validation in MongoDB 3.2 or higher. */ + bypassDocumentValidation?: boolean; + /** An optional hint for query optimization. See the {@link https://docs.mongodb.com/manual/reference/command/update/#update-command-hint|update command} reference for more information.*/ + hint?: Document; + /** Limits the fields to return for all matching documents. */ + projection?: Document; + /** When false, returns the updated document rather than the original. The default is true. */ + returnOriginal?: boolean; + /** Determines which document the operation modifies if the query selects multiple documents. */ + sort?: Sort; + /** Upsert the document if it does not exist. */ + upsert?: boolean; +} + +// TODO: NODE-1812 to deprecate returnOriginal for returnDocument + +/** @internal */ +interface FindAndModifyOptions extends CommandOperationOptions { /** When false, returns the updated document rather than the original. The default is true. */ returnOriginal?: boolean; /** Upsert the document if it does not exist. */ From 1d07ef0b9447888f7d8be8813c9cb687d1c25a10 Mon Sep 17 00:00:00 2001 From: Daria Pardue <81593090+dariakp@users.noreply.github.com> Date: Mon, 3 May 2021 16:15:45 -0400 Subject: [PATCH 3/6] refactor(NODE-3157): Remove needless assignment --- src/operations/find_and_modify.ts | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/src/operations/find_and_modify.ts b/src/operations/find_and_modify.ts index b9ff0dc0165..06b3d011b46 100644 --- a/src/operations/find_and_modify.ts +++ b/src/operations/find_and_modify.ts @@ -1,11 +1,5 @@ import { ReadPreference } from '../read_preference'; -import { - maxWireVersion, - applyRetryableWrites, - decorateWithCollation, - hasAtomicOperators, - Callback -} from '../utils'; +import { maxWireVersion, decorateWithCollation, hasAtomicOperators, Callback } from '../utils'; import { MongoError } from '../error'; import { CommandOperation, CommandOperationOptions } from './command'; import { defineAspects, Aspect } from './operation'; @@ -79,7 +73,6 @@ interface FindAndModifyOptions extends CommandOperationOptions { hint?: Document; // NOTE: These types are a misuse of options, can we think of a way to remove them? - update?: boolean; remove?: boolean; new?: boolean; } @@ -116,7 +109,7 @@ export class FindAndModifyOperation extends CommandOperation { const query = this.query; const sort = formatSort(this.sort); const doc = this.doc; - let options = { ...this.options, ...this.bsonOptions }; + const options = { ...this.options, ...this.bsonOptions }; // Create findAndModify command object const cmd: Document = { @@ -148,12 +141,6 @@ export class FindAndModifyOperation extends CommandOperation { cmd.maxTimeMS = options.maxTimeMS; } - // No check on the documents - options.checkKeys = false; - - // Final options for retryable writes - options = applyRetryableWrites(options, coll.s.db); - // Decorate the findAndModify command with the write Concern if (options.writeConcern) { cmd.writeConcern = options.writeConcern; @@ -225,7 +212,6 @@ export class FindOneAndReplaceOperation extends FindAndModifyOperation { ) { // Final options const finalOptions = Object.assign({}, options); - finalOptions.update = true; finalOptions.new = options.returnOriginal !== void 0 ? !options.returnOriginal : false; finalOptions.upsert = options.upsert !== void 0 ? !!options.upsert : false; @@ -255,7 +241,6 @@ export class FindOneAndUpdateOperation extends FindAndModifyOperation { ) { // Final options const finalOptions = Object.assign({}, options); - finalOptions.update = true; finalOptions.new = typeof options.returnOriginal === 'boolean' ? !options.returnOriginal : false; finalOptions.upsert = typeof options.upsert === 'boolean' ? options.upsert : false; From 547a4f75ffc78711b31ce87cec12df8633ca7d57 Mon Sep 17 00:00:00 2001 From: Daria Pardue <81593090+dariakp@users.noreply.github.com> Date: Mon, 3 May 2021 16:43:59 -0400 Subject: [PATCH 4/6] refactor: Make find and modify new/upsert parameter assignments in a uniform way --- src/operations/find_and_modify.ts | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/operations/find_and_modify.ts b/src/operations/find_and_modify.ts index 06b3d011b46..2aa33be28ee 100644 --- a/src/operations/find_and_modify.ts +++ b/src/operations/find_and_modify.ts @@ -74,7 +74,6 @@ interface FindAndModifyOptions extends CommandOperationOptions { // NOTE: These types are a misuse of options, can we think of a way to remove them? remove?: boolean; - new?: boolean; } /** @internal */ @@ -121,9 +120,15 @@ export class FindAndModifyOperation extends CommandOperation { cmd.sort = sort; } - cmd.new = options.new ? true : false; + if (!options.remove) { + cmd.new = typeof options.returnOriginal === 'boolean' ? !options.returnOriginal : false; + cmd.upsert = typeof options.upsert === 'boolean' ? options.upsert : false; + if (doc) { + cmd.update = doc; + } + } + cmd.remove = options.remove ? true : false; - cmd.upsert = options.upsert ? true : false; if (options.projection) { cmd.fields = options.projection; @@ -133,10 +138,6 @@ export class FindAndModifyOperation extends CommandOperation { cmd.arrayFilters = options.arrayFilters; } - if (doc && !options.remove) { - cmd.update = doc; - } - if (options.maxTimeMS) { cmd.maxTimeMS = options.maxTimeMS; } @@ -212,8 +213,6 @@ export class FindOneAndReplaceOperation extends FindAndModifyOperation { ) { // Final options const finalOptions = Object.assign({}, options); - finalOptions.new = options.returnOriginal !== void 0 ? !options.returnOriginal : false; - finalOptions.upsert = options.upsert !== void 0 ? !!options.upsert : false; if (filter == null || typeof filter !== 'object') { throw new TypeError('Filter parameter must be an object'); @@ -241,9 +240,6 @@ export class FindOneAndUpdateOperation extends FindAndModifyOperation { ) { // Final options const finalOptions = Object.assign({}, options); - finalOptions.new = - typeof options.returnOriginal === 'boolean' ? !options.returnOriginal : false; - finalOptions.upsert = typeof options.upsert === 'boolean' ? options.upsert : false; if (filter == null || typeof filter !== 'object') { throw new TypeError('Filter parameter must be an object'); From b75125a0d7fc9b08324fabab65b55fce1d9380a3 Mon Sep 17 00:00:00 2001 From: Daria Pardue <81593090+dariakp@users.noreply.github.com> Date: Mon, 3 May 2021 17:57:23 -0400 Subject: [PATCH 5/6] refactor: Clean up find and modify operation specific logic --- src/operations/find_and_modify.ts | 68 +++++++++++++++++-------------- 1 file changed, 37 insertions(+), 31 deletions(-) diff --git a/src/operations/find_and_modify.ts b/src/operations/find_and_modify.ts index 2aa33be28ee..6f73c85de35 100644 --- a/src/operations/find_and_modify.ts +++ b/src/operations/find_and_modify.ts @@ -71,11 +71,18 @@ interface FindAndModifyOptions extends CommandOperationOptions { bypassDocumentValidation?: boolean; /** An optional hint for query optimization. See the {@link https://docs.mongodb.com/manual/reference/command/update/#update-command-hint|update command} reference for more information.*/ hint?: Document; - - // NOTE: These types are a misuse of options, can we think of a way to remove them? - remove?: boolean; } +/** @internal */ +const OperationTypes = Object.freeze({ + deleteOne: 'deleteOne', + replaceOne: 'replaceOne', + updateOne: 'updateOne' +} as const); + +/** @internal */ +type OperationType = typeof OperationTypes[keyof typeof OperationTypes]; + /** @internal */ export class FindAndModifyOperation extends CommandOperation { options: FindAndModifyOptions; @@ -83,6 +90,7 @@ export class FindAndModifyOperation extends CommandOperation { query: Document; sort?: Sort; doc?: Document; + operationType?: OperationType; constructor( collection: Collection, @@ -116,26 +124,36 @@ export class FindAndModifyOperation extends CommandOperation { query: query }; - if (sort) { - cmd.sort = sort; - } - - if (!options.remove) { + if (this.operationType === OperationTypes.deleteOne) { + cmd.remove = true; + } else { + cmd.remove = false; + // TODO: cmd.new and cmd.upsert were set in two nonequivalent ways, do we want the truthy or the boolean approach? cmd.new = typeof options.returnOriginal === 'boolean' ? !options.returnOriginal : false; + // TODO: technically, cmd.upsert would previously be set "as is" if it were passed in for delete, do we want to preserve + // that behavior? cmd.upsert = typeof options.upsert === 'boolean' ? options.upsert : false; if (doc) { cmd.update = doc; } } - cmd.remove = options.remove ? true : false; + // TODO: arrayFilters is specific to update - do we want to add it to a type-specific block? + if (options.arrayFilters) { + cmd.arrayFilters = options.arrayFilters; + } - if (options.projection) { - cmd.fields = options.projection; + // TODO: bypassDocumentValidation is not applicable to delete - do we want to add it to the else block? + if (options.bypassDocumentValidation === true) { + cmd.bypassDocumentValidation = options.bypassDocumentValidation; } - if (options.arrayFilters) { - cmd.arrayFilters = options.arrayFilters; + if (sort) { + cmd.sort = sort; + } + + if (options.projection) { + cmd.fields = options.projection; } if (options.maxTimeMS) { @@ -147,11 +165,6 @@ export class FindAndModifyOperation extends CommandOperation { cmd.writeConcern = options.writeConcern; } - // Have we specified bypassDocumentValidation - if (options.bypassDocumentValidation === true) { - cmd.bypassDocumentValidation = options.bypassDocumentValidation; - } - // Have we specified collation try { decorateWithCollation(cmd, coll, options); @@ -190,16 +203,13 @@ export class FindAndModifyOperation extends CommandOperation { /** @internal */ export class FindOneAndDeleteOperation extends FindAndModifyOperation { constructor(collection: Collection, filter: Document, options: FindAndModifyOptions) { - // Final options - const finalOptions = Object.assign({}, options); - finalOptions.remove = true; - // Basic validation if (filter == null || typeof filter !== 'object') { throw new TypeError('Filter parameter must be an object'); } - super(collection, filter, finalOptions.sort, undefined, finalOptions); + super(collection, filter, options.sort, undefined, options); + this.operationType = OperationTypes.deleteOne; } } @@ -211,9 +221,6 @@ export class FindOneAndReplaceOperation extends FindAndModifyOperation { replacement: Document, options: FindAndModifyOptions ) { - // Final options - const finalOptions = Object.assign({}, options); - if (filter == null || typeof filter !== 'object') { throw new TypeError('Filter parameter must be an object'); } @@ -226,7 +233,8 @@ export class FindOneAndReplaceOperation extends FindAndModifyOperation { throw new TypeError('Replacement document must not contain atomic operators'); } - super(collection, filter, finalOptions.sort, replacement, finalOptions); + super(collection, filter, options.sort, replacement, options); + this.operationType = OperationTypes.replaceOne; } } @@ -238,9 +246,6 @@ export class FindOneAndUpdateOperation extends FindAndModifyOperation { update: Document, options: FindAndModifyOptions ) { - // Final options - const finalOptions = Object.assign({}, options); - if (filter == null || typeof filter !== 'object') { throw new TypeError('Filter parameter must be an object'); } @@ -253,7 +258,8 @@ export class FindOneAndUpdateOperation extends FindAndModifyOperation { throw new TypeError('Update document requires atomic operators'); } - super(collection, filter, finalOptions.sort, update, finalOptions); + super(collection, filter, options.sort, update, options); + this.operationType = OperationTypes.updateOne; } } From 291921b3ac99fde54c2213060bb94a580c970624 Mon Sep 17 00:00:00 2001 From: Daria Pardue <81593090+dariakp@users.noreply.github.com> Date: Tue, 4 May 2021 11:29:08 -0400 Subject: [PATCH 6/6] refactor: remove redundant code in find and modify --- src/operations/find_and_modify.ts | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/operations/find_and_modify.ts b/src/operations/find_and_modify.ts index 6f73c85de35..4bd8a8f3f6a 100644 --- a/src/operations/find_and_modify.ts +++ b/src/operations/find_and_modify.ts @@ -84,7 +84,7 @@ const OperationTypes = Object.freeze({ type OperationType = typeof OperationTypes[keyof typeof OperationTypes]; /** @internal */ -export class FindAndModifyOperation extends CommandOperation { +class FindAndModifyOperation extends CommandOperation { options: FindAndModifyOptions; collection: Collection; query: Document; @@ -95,9 +95,8 @@ export class FindAndModifyOperation extends CommandOperation { constructor( collection: Collection, query: Document, - sort: Sort | undefined, doc: Document | undefined, - options?: FindAndModifyOptions + options: FindAndModifyOptions ) { super(collection, options); this.options = options ?? {}; @@ -107,7 +106,7 @@ export class FindAndModifyOperation extends CommandOperation { this.collection = collection; this.query = query; - this.sort = sort; + this.sort = options.sort; this.doc = doc; } @@ -202,13 +201,13 @@ export class FindAndModifyOperation extends CommandOperation { /** @internal */ export class FindOneAndDeleteOperation extends FindAndModifyOperation { - constructor(collection: Collection, filter: Document, options: FindAndModifyOptions) { + constructor(collection: Collection, filter: Document, options: FindOneAndDeleteOptions) { // Basic validation if (filter == null || typeof filter !== 'object') { throw new TypeError('Filter parameter must be an object'); } - super(collection, filter, options.sort, undefined, options); + super(collection, filter, undefined, options); this.operationType = OperationTypes.deleteOne; } } @@ -219,7 +218,7 @@ export class FindOneAndReplaceOperation extends FindAndModifyOperation { collection: Collection, filter: Document, replacement: Document, - options: FindAndModifyOptions + options: FindOneAndReplaceOptions ) { if (filter == null || typeof filter !== 'object') { throw new TypeError('Filter parameter must be an object'); @@ -233,7 +232,7 @@ export class FindOneAndReplaceOperation extends FindAndModifyOperation { throw new TypeError('Replacement document must not contain atomic operators'); } - super(collection, filter, options.sort, replacement, options); + super(collection, filter, replacement, options); this.operationType = OperationTypes.replaceOne; } } @@ -244,7 +243,7 @@ export class FindOneAndUpdateOperation extends FindAndModifyOperation { collection: Collection, filter: Document, update: Document, - options: FindAndModifyOptions + options: FindOneAndUpdateOptions ) { if (filter == null || typeof filter !== 'object') { throw new TypeError('Filter parameter must be an object'); @@ -258,7 +257,7 @@ export class FindOneAndUpdateOperation extends FindAndModifyOperation { throw new TypeError('Update document requires atomic operators'); } - super(collection, filter, options.sort, update, options); + super(collection, filter, update, options); this.operationType = OperationTypes.updateOne; } }