From ed5879ead9b1d8ba9870e5187b2f4f4e016372c5 Mon Sep 17 00:00:00 2001 From: Hafez Date: Fri, 8 Jul 2022 16:28:23 +0200 Subject: [PATCH 01/11] test(model): add tests to verify buildBulkWriteOperations accepts `timestamps` option re #12059 --- test/model.test.js | 60 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 58 insertions(+), 2 deletions(-) diff --git a/test/model.test.js b/test/model.test.js index 39f3a5dfeeb..94f219fb9c0 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -8117,9 +8117,65 @@ describe('Model', function() { assert.equal(writeOperations.length, 3); }); - }); - describe('bulkSave() (gh-9673)', function() { + it('accepts `timestamps: false` (gh-12059)', async() => { + // Arrange + const userSchema = new Schema({ + name: { type: String, minLength: 5 } + }); + + const User = db.model('User', userSchema); + + const newUser = new User({ name: 'Hafez' }); + const userToUpdate = await User.create({ name: 'Hafez' }); + userToUpdate.name = 'John Doe'; + + // Act + const writeOperations = User.buildBulkWriteOperations([newUser, userToUpdate], { timestamps: false, skipValidation: true }); + + // Assert + const timestampsOptions = writeOperations.map(writeOperation => writeOperation.timestamps); + assert.deepEqual(timestampsOptions, [false, false]); + }); + it('accepts `timestamps: true` (gh-12059)', async() => { + // Arrange + const userSchema = new Schema({ + name: { type: String, minLength: 5 } + }); + + const User = db.model('User', userSchema); + + const newUser = new User({ name: 'Hafez' }); + const userToUpdate = await User.create({ name: 'Hafez' }); + userToUpdate.name = 'John Doe'; + + // Act + const writeOperations = User.buildBulkWriteOperations([newUser, userToUpdate], { timestamps: true, skipValidation: true }); + + // Assert + const timestampsOptions = writeOperations.map(writeOperation => writeOperation.timestamps); + assert.deepEqual(timestampsOptions, [true, true]); + }); + it('`timestamps` has `undefined` as default value (gh-12059)', async() => { + // Arrange + const userSchema = new Schema({ + name: { type: String, minLength: 5 } + }); + + const User = db.model('User', userSchema); + + const newUser = new User({ name: 'Hafez' }); + const userToUpdate = await User.create({ name: 'Hafez' }); + userToUpdate.name = 'John Doe'; + + // Act + const writeOperations = User.buildBulkWriteOperations([newUser, userToUpdate], { skipValidation: true }); + + // Assert + const timestampsOptions = writeOperations.map(writeOperation => writeOperation.timestamps); + assert.deepEqual(timestampsOptions, [undefined, undefined]); + }); + }); it('saves new documents', async function() { const userSchema = new Schema({ From 3e159b623f0b6de9ba1711359023ff77fa166127 Mon Sep 17 00:00:00 2001 From: Hafez Date: Fri, 8 Jul 2022 18:29:30 +0200 Subject: [PATCH 02/11] test: add more tests re #12059 for bulkSave and timestamps --- test/model.test.js | 80 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 77 insertions(+), 3 deletions(-) diff --git a/test/model.test.js b/test/model.test.js index 94f219fb9c0..b2f35051c7b 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -8134,7 +8134,10 @@ describe('Model', function() { const writeOperations = User.buildBulkWriteOperations([newUser, userToUpdate], { timestamps: false, skipValidation: true }); // Assert - const timestampsOptions = writeOperations.map(writeOperation => writeOperation.timestamps); + const timestampsOptions = writeOperations.map(writeOperationContainer => { + const operationObject = writeOperationContainer.updateOne || writeOperationContainer.insertOne; + return operationObject.timestamps; + }); assert.deepEqual(timestampsOptions, [false, false]); }); it('accepts `timestamps: true` (gh-12059)', async() => { @@ -8153,7 +8156,10 @@ describe('Model', function() { const writeOperations = User.buildBulkWriteOperations([newUser, userToUpdate], { timestamps: true, skipValidation: true }); // Assert - const timestampsOptions = writeOperations.map(writeOperation => writeOperation.timestamps); + const timestampsOptions = writeOperations.map(writeOperationContainer => { + const operationObject = writeOperationContainer.updateOne || writeOperationContainer.insertOne; + return operationObject.timestamps; + }); assert.deepEqual(timestampsOptions, [true, true]); }); it('`timestamps` has `undefined` as default value (gh-12059)', async() => { @@ -8172,10 +8178,15 @@ describe('Model', function() { const writeOperations = User.buildBulkWriteOperations([newUser, userToUpdate], { skipValidation: true }); // Assert - const timestampsOptions = writeOperations.map(writeOperation => writeOperation.timestamps); + const timestampsOptions = writeOperations.map(writeOperationContainer => { + const operationObject = writeOperationContainer.updateOne || writeOperationContainer.insertOne; + return operationObject.timestamps; + }); assert.deepEqual(timestampsOptions, [undefined, undefined]); }); }); + + describe('bulkSave() (gh-9673)', function() { it('saves new documents', async function() { const userSchema = new Schema({ @@ -8437,6 +8448,69 @@ describe('Model', function() { const res = await model.bulkSave(entries); assert.ok(res); }); + + xit('accepts `timestamps: false` (gh-12059)', async() => { + // Arrange + const userSchema = new Schema({ + name: { type: String, minLength: 5 } + }, { timestamps: true }); + + const User = db.model('User', userSchema); + mongoose.set('debug', true); + const userToUpdate = await User.create({ name: 'Hafez', createdAt: new Date('1994-12-04'), updatedAt: new Date('1994-12-04') }); + userToUpdate.name = 'John Doe'; + + // Act + await User.bulkSave([userToUpdate], { timestamps: false }); + mongoose.set('debug', false); + + // Assert + assert.deepStrictEqual(userToUpdate.createdAt, new Date('1994-12-04')); + assert.deepStrictEqual(userToUpdate.updatedAt, new Date('1994-12-04')); + }); + + it('accepts `timestamps: true` (gh-12059)', async() => { + // Arrange + const userSchema = new Schema({ + name: { type: String, minLength: 5 } + }, { timestamps: true }); + + const User = db.model('User', userSchema); + + const newUser = new User({ name: 'Hafez' }); + const userToUpdate = await User.create({ name: 'Hafez' }); + userToUpdate.name = 'John Doe'; + + // Act + await User.bulkSave([newUser, userToUpdate], { timestamps: true }); + + // Assert + assert.ok(newUser.createdAt); + assert.ok(newUser.updatedAt); + assert.ok(userToUpdate.createdAt); + assert.ok(userToUpdate.updatedAt); + }); + it('`timestamps` has `undefined` as default value (gh-12059)', async() => { + // Arrange + const userSchema = new Schema({ + name: { type: String, minLength: 5 } + }, { timestamps: true }); + + const User = db.model('User', userSchema); + + const newUser = new User({ name: 'Hafez' }); + const userToUpdate = await User.create({ name: 'Hafez' }); + userToUpdate.name = 'John Doe'; + + // Act + await User.bulkSave([newUser, userToUpdate]); + + // Assert + assert.ok(newUser.createdAt); + assert.ok(newUser.updatedAt); + assert.ok(userToUpdate.createdAt); + assert.ok(userToUpdate.updatedAt); + }); }); describe('Setting the explain flag', function() { From a0b5ba4b50236d7a90fe0399044e08628b2db657 Mon Sep 17 00:00:00 2001 From: Hafez Date: Fri, 8 Jul 2022 18:30:01 +0200 Subject: [PATCH 03/11] feat(model): allow passing as an option `timestamps` to bulkSave --- lib/model.js | 21 ++++++++++----------- lib/utils.js | 8 ++++++++ 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/lib/model.js b/lib/model.js index 26443296ea8..bc2ded2f1c9 100644 --- a/lib/model.js +++ b/lib/model.js @@ -3616,6 +3616,7 @@ Model.bulkWrite = function(ops, options, callback) { * * @param {Array} documents * @param {Object} [options] options passed to the underlying `bulkWrite()` + * @param {Boolean} [options.timestamps] defaults to `null`, when set to false, mongoose will not add/update timestamps to the documents. * @param {ClientSession} [options.session=null] The session associated with this bulk write. See [transactions docs](/docs/transactions.html). * @param {String|number} [options.w=1] The [write concern](https://docs.mongodb.com/manual/reference/write-concern/). See [`Query#w()`](/docs/api.html#query_Query-w) for more information. * @param {number} [options.wtimeout=null] The [write concern timeout](https://docs.mongodb.com/manual/reference/write-concern/#wtimeout). @@ -3625,7 +3626,7 @@ Model.bulkWrite = function(ops, options, callback) { Model.bulkSave = function(documents, options) { const preSavePromises = documents.map(buildPreSavePromise); - const writeOperations = this.buildBulkWriteOperations(documents, { skipValidation: true }); + const writeOperations = this.buildBulkWriteOperations(documents, { skipValidation: true, timestamps: options && options.timestamps }); let bulkWriteResultPromise; return Promise.all(preSavePromises) @@ -3691,6 +3692,7 @@ function handleSuccessfulWrite(document, resolve, reject) { * @param {Array} documents The array of documents to build write operations of * @param {Object} options * @param {Boolean} options.skipValidation defaults to `false`, when set to true, building the write operations will bypass validating the documents. + * @param {Boolean} options.timestamps defaults to `null`, when set to false, mongoose will not add/update timestamps to the documents. * @return {Array} Returns a array of all Promises the function executes to be awaited. */ Model.buildBulkWriteOperations = function buildBulkWriteOperations(documents, options) { @@ -3713,9 +3715,9 @@ Model.buildBulkWriteOperations = function buildBulkWriteOperations(documents, op const isANewDocument = document.isNew; if (isANewDocument) { - accumulator.push({ - insertOne: { document } - }); + const writeOperation = { insertOne: { document } }; + utils.injectTimestampsOption(writeOperation.insertOne, options.timestamps); + accumulator.push(writeOperation); return accumulator; } @@ -3730,13 +3732,9 @@ Model.buildBulkWriteOperations = function buildBulkWriteOperations(documents, op _applyCustomWhere(document, where); document.$__version(where, delta); - - accumulator.push({ - updateOne: { - filter: where, - update: changes - } - }); + const writeOperation = { updateOne: { filter: where, update: changes } }; + utils.injectTimestampsOption(writeOperation.updateOne, options.timestamps); + accumulator.push(writeOperation); return accumulator; } @@ -3755,6 +3753,7 @@ Model.buildBulkWriteOperations = function buildBulkWriteOperations(documents, op } }; + /** * Shortcut for creating a new Document from existing raw data, pre-saved in the DB. * The document returned has no paths marked as modified initially. diff --git a/lib/utils.js b/lib/utils.js index 50058acdc13..8ebd81d8835 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -972,3 +972,11 @@ exports.errorToPOJO = function errorToPOJO(error) { exports.warn = function warn(message) { return process.emitWarning(message, { code: 'MONGOOSE' }); }; + + +exports.injectTimestampsOption = function injectTimestampsOption(writeOperation, timestampsOption) { + if (timestampsOption == null) { + return; + } + writeOperation.timestamps = timestampsOption; +}; From 0e813b8edfcc488653351682f19eda9ca4c0793e Mon Sep 17 00:00:00 2001 From: Hafez Date: Fri, 8 Jul 2022 18:32:28 +0200 Subject: [PATCH 04/11] add `timestamps` option to bulkSave TS definition --- types/models.d.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/types/models.d.ts b/types/models.d.ts index d4a6f1a1797..6396a4bebf1 100644 --- a/types/models.d.ts +++ b/types/models.d.ts @@ -149,7 +149,7 @@ declare module 'mongoose' { * sending multiple `save()` calls because with `bulkSave()` there is only one * network round trip to the MongoDB server. */ - bulkSave(documents: Array, options?: mongodb.BulkWriteOptions): Promise; + bulkSave(documents: Array, options?: mongodb.BulkWriteOptions & { timestamps?: boolean }): Promise; /** Collection the model uses. */ collection: Collection; From f2d37c24385d484e14661c0e890b78edc6c08f19 Mon Sep 17 00:00:00 2001 From: Hafez Date: Fri, 8 Jul 2022 18:32:55 +0200 Subject: [PATCH 05/11] test(types): add `timestamps` option to bulkSave --- test/types/models.test.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/types/models.test.ts b/test/types/models.test.ts index 58941ae10aa..dc5a4a65d76 100644 --- a/test/types/models.test.ts +++ b/test/types/models.test.ts @@ -319,3 +319,22 @@ function gh11911() { update: changes }); } + + +function gh12059() { + + interface IAnimal { + name?: string; + } + + const animalSchema = new Schema({ + name: { type: String } + }); + + const Animal = model('Animal', animalSchema); + const animal = new Animal(); + + Animal.bulkSave([animal], { timestamps: false }); + Animal.bulkSave([animal], { timestamps: true }); + Animal.bulkSave([animal], {}); +} From 1cbe0281e8c39cd2bfc2a5de2c3aa7c0d9b00ecd Mon Sep 17 00:00:00 2001 From: Hafez Date: Fri, 8 Jul 2022 18:42:51 +0200 Subject: [PATCH 06/11] enable failing test --- test/model.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/model.test.js b/test/model.test.js index b2f35051c7b..5e534732b0d 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -8449,7 +8449,7 @@ describe('Model', function() { assert.ok(res); }); - xit('accepts `timestamps: false` (gh-12059)', async() => { + it('accepts `timestamps: false` (gh-12059)', async() => { // Arrange const userSchema = new Schema({ name: { type: String, minLength: 5 } From 399668a5a6764cc0de079fe7cff8765b5d475ab4 Mon Sep 17 00:00:00 2001 From: Hafez Date: Fri, 8 Jul 2022 18:52:56 +0200 Subject: [PATCH 07/11] remove debug statements --- test/model.test.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/model.test.js b/test/model.test.js index 5e534732b0d..364e4b6ee9f 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -8456,13 +8456,11 @@ describe('Model', function() { }, { timestamps: true }); const User = db.model('User', userSchema); - mongoose.set('debug', true); const userToUpdate = await User.create({ name: 'Hafez', createdAt: new Date('1994-12-04'), updatedAt: new Date('1994-12-04') }); userToUpdate.name = 'John Doe'; // Act await User.bulkSave([userToUpdate], { timestamps: false }); - mongoose.set('debug', false); // Assert assert.deepStrictEqual(userToUpdate.createdAt, new Date('1994-12-04')); From f6fd9211c2cbce25de4e5e4a8bb51e8523402c2e Mon Sep 17 00:00:00 2001 From: Hafez Date: Fri, 8 Jul 2022 19:19:47 +0200 Subject: [PATCH 08/11] refactor(model): make bulkSave an async function --- lib/model.js | 73 +++++++++++++++++++++------------------------- test/model.test.js | 3 +- 2 files changed, 35 insertions(+), 41 deletions(-) diff --git a/lib/model.js b/lib/model.js index bc2ded2f1c9..b8b235b6672 100644 --- a/lib/model.js +++ b/lib/model.js @@ -3623,35 +3623,33 @@ Model.bulkWrite = function(ops, options, callback) { * @param {Boolean} [options.j=true] If false, disable [journal acknowledgement](https://docs.mongodb.com/manual/reference/write-concern/#j-option) * */ -Model.bulkSave = function(documents, options) { - const preSavePromises = documents.map(buildPreSavePromise); +Model.bulkSave = async function(documents, options) { + await Promise.all(documents.map(buildPreSavePromise)); const writeOperations = this.buildBulkWriteOperations(documents, { skipValidation: true, timestamps: options && options.timestamps }); + const { bulkWriteResult, bulkWriteError } = await this.bulkWrite(writeOperations, options).then( + (res) => ({ bulkWriteResult: res, bulkWriteError: null }), + (err) => ({ bulkWriteResult: null, bulkWriteError: err }) + ); + + await Promise.all( + documents.map(async(document) => { + const documentError = bulkWriteError && bulkWriteError.writeErrors.find(writeError => { + const writeErrorDocumentId = writeError.err.op._id || writeError.err.op.q._id; + return writeErrorDocumentId.toString() === document._id.toString(); + }); - let bulkWriteResultPromise; - return Promise.all(preSavePromises) - .then(() => bulkWriteResultPromise = this.bulkWrite(writeOperations, options)) - .then(() => documents.map(buildSuccessfulWriteHandlerPromise)) - .then(() => bulkWriteResultPromise) - .catch((err) => { - if (!(err && err.writeErrors && err.writeErrors.length)) { - throw err; + if (documentError == null) { + await handleSuccessfulWrite(document); } - return Promise.all( - documents.map((document) => { - const documentError = err.writeErrors.find(writeError => { - const writeErrorDocumentId = writeError.err.op._id || writeError.err.op.q._id; - return writeErrorDocumentId.toString() === document._id.toString(); - }); + }) + ); - if (documentError == null) { - return buildSuccessfulWriteHandlerPromise(document); - } - }) - ).then(() => { - throw err; - }); - }); + if (bulkWriteError && bulkWriteError.writeErrors && bulkWriteError.writeErrors.length) { + throw bulkWriteError; + } + + return bulkWriteResult; }; function buildPreSavePromise(document) { @@ -3666,24 +3664,21 @@ function buildPreSavePromise(document) { }); } -function buildSuccessfulWriteHandlerPromise(document) { +function handleSuccessfulWrite(document) { return new Promise((resolve, reject) => { - handleSuccessfulWrite(document, resolve, reject); - }); -} + if (document.$isNew) { + _setIsNew(document, false); + } -function handleSuccessfulWrite(document, resolve, reject) { - if (document.$isNew) { - _setIsNew(document, false); - } + document.$__reset(); + document.schema.s.hooks.execPost('save', document, {}, (err) => { + if (err) { + reject(err); + return; + } + resolve(); + }); - document.$__reset(); - document.schema.s.hooks.execPost('save', document, {}, (err) => { - if (err) { - reject(err); - return; - } - resolve(); }); } diff --git a/test/model.test.js b/test/model.test.js index 364e4b6ee9f..5747f16069a 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -8272,7 +8272,6 @@ describe('Model', function() { }); it('throws an error on failure', async() => { - const userSchema = new Schema({ name: { type: String, unique: true } }); @@ -8292,8 +8291,8 @@ describe('Model', function() { const err = await User.bulkSave(users).then(() => null, err => err); assert.ok(err); - }); + it('changes document state from `isNew` `false` to `true`', async() => { const userSchema = new Schema({ From b855d5c68f5b80be9d521947649459c815aad9ec Mon Sep 17 00:00:00 2001 From: Hafez Date: Fri, 8 Jul 2022 19:29:34 +0200 Subject: [PATCH 09/11] add default `options` empty object for bulkSave --- lib/model.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/model.js b/lib/model.js index 46ffcc5ed21..9fa2c2eac85 100644 --- a/lib/model.js +++ b/lib/model.js @@ -3624,9 +3624,10 @@ Model.bulkWrite = function(ops, options, callback) { * */ Model.bulkSave = async function(documents, options) { + options = options || {}; await Promise.all(documents.map(buildPreSavePromise)); - const writeOperations = this.buildBulkWriteOperations(documents, { skipValidation: true, timestamps: options && options.timestamps }); + const writeOperations = this.buildBulkWriteOperations(documents, { skipValidation: true, timestamps: options.timestamps }); const { bulkWriteResult, bulkWriteError } = await this.bulkWrite(writeOperations, options).then( (res) => ({ bulkWriteResult: res, bulkWriteError: null }), (err) => ({ bulkWriteResult: null, bulkWriteError: err }) From 0a88950341be36c33721b1344435275648cd7f7d Mon Sep 17 00:00:00 2001 From: Hafez Date: Fri, 15 Jul 2022 07:36:01 +0200 Subject: [PATCH 10/11] test(model): assert timestamps false for bulkSave --- test/model.test.js | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/test/model.test.js b/test/model.test.js index 5747f16069a..f47b864aadd 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -8451,17 +8451,26 @@ describe('Model', function() { it('accepts `timestamps: false` (gh-12059)', async() => { // Arrange const userSchema = new Schema({ - name: { type: String, minLength: 5 } + name: { type: String } }, { timestamps: true }); const User = db.model('User', userSchema); + const newUser = new User({ name: 'Sam' }); + const userToUpdate = await User.create({ name: 'Hafez', createdAt: new Date('1994-12-04'), updatedAt: new Date('1994-12-04') }); userToUpdate.name = 'John Doe'; // Act - await User.bulkSave([userToUpdate], { timestamps: false }); + await User.bulkSave([newUser, userToUpdate], { timestamps: false }); + // Assert + const createdUserPersistedInDB = await User.findOne({ _id: newUser._id }); + assert.deepStrictEqual(newUser.createdAt, undefined); + assert.deepStrictEqual(newUser.updatedAt, undefined); + + assert.deepStrictEqual(createdUserPersistedInDB.createdAt, undefined); + assert.deepStrictEqual(createdUserPersistedInDB.updatedAt, undefined); assert.deepStrictEqual(userToUpdate.createdAt, new Date('1994-12-04')); assert.deepStrictEqual(userToUpdate.updatedAt, new Date('1994-12-04')); }); From 03951951f4ceab98bc67f4754a927e0cfd5b5d99 Mon Sep 17 00:00:00 2001 From: Hafez Date: Fri, 15 Jul 2022 07:41:29 +0200 Subject: [PATCH 11/11] fix(model): make bulkSave and bulkWrite reflect timestamps option on documents --- lib/helpers/model/castBulkWrite.js | 2 +- lib/model.js | 11 ++++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/helpers/model/castBulkWrite.js b/lib/helpers/model/castBulkWrite.js index 5b882b6d5fe..27723faa6e7 100644 --- a/lib/helpers/model/castBulkWrite.js +++ b/lib/helpers/model/castBulkWrite.js @@ -20,7 +20,7 @@ module.exports = function castBulkWrite(originalModel, op, options) { const model = decideModelByObject(originalModel, op['insertOne']['document']); const doc = new model(op['insertOne']['document']); - if (model.schema.options.timestamps) { + if (model.schema.options.timestamps && options.timestamps !== false) { doc.initializeTimestamps(); } if (options.session != null) { diff --git a/lib/model.js b/lib/model.js index 9fa2c2eac85..26fb9a511a0 100644 --- a/lib/model.js +++ b/lib/model.js @@ -3625,9 +3625,18 @@ Model.bulkWrite = function(ops, options, callback) { */ Model.bulkSave = async function(documents, options) { options = options || {}; - await Promise.all(documents.map(buildPreSavePromise)); const writeOperations = this.buildBulkWriteOperations(documents, { skipValidation: true, timestamps: options.timestamps }); + + if (options.timestamps != null) { + for (const document of documents) { + document.$__.saveOptions = document.$__.saveOptions || {}; + document.$__.saveOptions.timestamps = options.timestamps; + } + } + + await Promise.all(documents.map(buildPreSavePromise)); + const { bulkWriteResult, bulkWriteError } = await this.bulkWrite(writeOperations, options).then( (res) => ({ bulkWriteResult: res, bulkWriteError: null }), (err) => ({ bulkWriteResult: null, bulkWriteError: err })