From e396e9db2a0730524d1766a383f794e03ea1956c Mon Sep 17 00:00:00 2001 From: Mohamed Yousef Date: Fri, 9 Jun 2023 17:29:05 +0300 Subject: [PATCH 1/4] fix Model.create failing when the last argument is falsy --- lib/model.js | 16 ++++++++-------- test/model.create.test.js | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/lib/model.js b/lib/model.js index 65e0211e715..c2853f1b38c 100644 --- a/lib/model.js +++ b/lib/model.js @@ -2811,16 +2811,16 @@ Model.create = async function create(doc, options) { options = options != null && typeof options === 'object' ? options : {}; } else { const last = arguments[arguments.length - 1]; - options = {}; - if (typeof last === 'function' || (arguments.length > 1 && !last)) { - if (typeof options === 'function' || - typeof arguments[2] === 'function') { - throw new MongooseError('Model.create() no longer accepts a callback'); - } - } else { - args = [...arguments]; + const argumentsArray = Array.from(arguments); + + if (argumentsArray.some(argument => typeof argument === 'function')) { + throw new MongooseError('Model.create() no longer accepts a callback'); } + options = {}; + + args = [...arguments]; + if (args.length === 2 && args[0] != null && args[1] != null && diff --git a/test/model.create.test.js b/test/model.create.test.js index 646b76c01f5..1adf9e24a41 100644 --- a/test/model.create.test.js +++ b/test/model.create.test.js @@ -115,6 +115,22 @@ describe('model', function() { assert.ok(endTime - startTime < 4 * 100); // serial: >= 4 * 100 parallel: < 4 * 100 }); + it('creates a document when options equals undefined (gh-13487)', async function() { + const posts = await B.create({ title: 'undefined options' }, undefined); + const post1 = posts[0]; + assert.equal(post1.title, 'undefined options'); + }); + + it('creates multiple document when options equals undefined (gh-13487)', async function() { + const posts = await B.create({ title: 'Post1' }, { title: 'Post2' }, undefined); + + const post1 = posts[0]; + const post2 = posts[1]; + + assert.equal(post1.title, 'Post1'); + assert.equal(post2.title, 'Post2'); + }); + describe('callback is optional', function() { it('with one doc', async function() { const doc = await B.create({ title: 'optional callback' }); From 482385dbc0f55ca77f3705584cb999120ca7f06e Mon Sep 17 00:00:00 2001 From: Mohamed Yousef Date: Fri, 9 Jun 2023 19:44:36 +0300 Subject: [PATCH 2/4] throw error if a falsy value is passed to Model.create() after doc --- lib/model.js | 51 ++++++++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/lib/model.js b/lib/model.js index c2853f1b38c..2f435c0696b 100644 --- a/lib/model.js +++ b/lib/model.js @@ -861,10 +861,10 @@ function checkDivergentArray(doc, path, array) { // would be similarly destructive as we never received all // elements of the array and potentially would overwrite data. const check = pop.options.match || - pop.options.options && utils.object.hasOwnProperty(pop.options.options, 'limit') || // 0 is not permitted - pop.options.options && pop.options.options.skip || // 0 is permitted - pop.options.select && // deselected _id? - (pop.options.select._id === 0 || + pop.options.options && utils.object.hasOwnProperty(pop.options.options, 'limit') || // 0 is not permitted + pop.options.options && pop.options.options.skip || // 0 is permitted + pop.options.select && // deselected _id? + (pop.options.select._id === 0 || /\s?-_id\s?/.test(pop.options.select)); if (check) { @@ -992,7 +992,7 @@ Model.prototype.$__where = function _where(where) { Model.prototype.deleteOne = async function deleteOne(options) { if (typeof options === 'function' || - typeof arguments[1] === 'function') { + typeof arguments[1] === 'function') { throw new MongooseError('Model.prototype.deleteOne() no longer accepts a callback'); } @@ -1735,7 +1735,7 @@ function _ensureIndexes(model, options, callback) { function create() { if (options._automatic) { if (model.schema.options.autoIndex === false || - (model.schema.options.autoIndex == null && model.db.config.autoIndex === false)) { + (model.schema.options.autoIndex == null && model.db.config.autoIndex === false)) { return done(); } } @@ -2797,7 +2797,7 @@ Model.findByIdAndRemove = function(id, options) { Model.create = async function create(doc, options) { if (typeof options === 'function' || - typeof arguments[2] === 'function') { + typeof arguments[2] === 'function') { throw new MongooseError('Model.create() no longer accepts a callback'); } @@ -2812,21 +2812,26 @@ Model.create = async function create(doc, options) { } else { const last = arguments[arguments.length - 1]; const argumentsArray = Array.from(arguments); + const argumentsArrayAfterTheFirst = Array.from(arguments).slice(1); if (argumentsArray.some(argument => typeof argument === 'function')) { throw new MongooseError('Model.create() no longer accepts a callback'); } + if (arguments.length > 1 && argumentsArrayAfterTheFirst.some(argument => !argument)) { + throw new MongooseError('Passed a falsy value to Model.create() after doc'); + } + options = {}; args = [...arguments]; if (args.length === 2 && - args[0] != null && - args[1] != null && - args[0].session == null && - getConstructorName(last.session) === 'ClientSession' && - !this.schema.path('session')) { + args[0] != null && + args[1] != null && + args[0].session == null && + getConstructorName(last.session) === 'ClientSession' && + !this.schema.path('session')) { // Probably means the user is running into the common mistake of trying // to use a spread to specify options, see gh-7535 utils.warn('WARNING: to pass a `session` to `Model.create()` in ' + @@ -3188,7 +3193,7 @@ Model.$__insertMany = function(arr, options, callback) { // `writeErrors` is a property reported by the MongoDB driver, // just not if there's only 1 error. if (error.writeErrors == null && - (error.result && error.result.result && error.result.result.writeErrors) != null) { + (error.result && error.result.result && error.result.result.writeErrors) != null) { error.writeErrors = error.result.result.writeErrors; } @@ -3357,7 +3362,7 @@ Model.bulkWrite = async function bulkWrite(ops, options) { _checkContext(this, 'bulkWrite'); if (typeof options === 'function' || - typeof arguments[2] === 'function') { + typeof arguments[2] === 'function') { throw new MongooseError('Model.bulkWrite() no longer accepts a callback'); } options = options || {}; @@ -3909,9 +3914,9 @@ function _update(model, op, conditions, doc, options) { options = typeof options === 'function' ? options : clone(options); const versionKey = model && - model.schema && - model.schema.options && - model.schema.options.versionKey || null; + model.schema && + model.schema.options && + model.schema.options.versionKey || null; _decorateUpdateWithVersionKey(doc, options, versionKey); return mq[op](conditions, doc, options); @@ -4277,9 +4282,9 @@ function populate(model, docs, options, callback) { // to fail. So delay running lean transform until _after_ // `_assign()` if (mod.options && - mod.options.options && - mod.options.options.lean && - mod.options.options.lean.transform) { + mod.options.options && + mod.options.options.lean && + mod.options.options.lean.transform) { mod.options.options._leanTransform = mod.options.options.lean.transform; mod.options.options.lean = true; } @@ -4408,8 +4413,8 @@ function _execPopulateQuery(mod, match, select, assignmentOpts, callback) { // field, that's the client's fault. for (const foreignField of mod.foreignField) { if (foreignField !== '_id' && - query.selectedInclusively() && - !isPathSelectedInclusive(query._fields, foreignField)) { + query.selectedInclusively() && + !isPathSelectedInclusive(query._fields, foreignField)) { query.select(foreignField); } } @@ -4765,7 +4770,7 @@ Model.__subclass = function subclass(conn, schema, collection) { Model.collection = Model.prototype.collection; Model.$__collection = Model.collection; // Errors handled internally, so ignore - Model.init().catch(() => {}); + Model.init().catch(() => { }); return Model; }; From 745d43420879794e8cf239958f666565eb7b85cf Mon Sep 17 00:00:00 2001 From: Mohamed Yousef Date: Fri, 9 Jun 2023 19:46:28 +0300 Subject: [PATCH 3/4] fix lint --- lib/model.js | 48 ++++++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/lib/model.js b/lib/model.js index 2f435c0696b..53fda1bcf81 100644 --- a/lib/model.js +++ b/lib/model.js @@ -861,10 +861,10 @@ function checkDivergentArray(doc, path, array) { // would be similarly destructive as we never received all // elements of the array and potentially would overwrite data. const check = pop.options.match || - pop.options.options && utils.object.hasOwnProperty(pop.options.options, 'limit') || // 0 is not permitted - pop.options.options && pop.options.options.skip || // 0 is permitted - pop.options.select && // deselected _id? - (pop.options.select._id === 0 || + pop.options.options && utils.object.hasOwnProperty(pop.options.options, 'limit') || // 0 is not permitted + pop.options.options && pop.options.options.skip || // 0 is permitted + pop.options.select && // deselected _id? + (pop.options.select._id === 0 || /\s?-_id\s?/.test(pop.options.select)); if (check) { @@ -992,7 +992,7 @@ Model.prototype.$__where = function _where(where) { Model.prototype.deleteOne = async function deleteOne(options) { if (typeof options === 'function' || - typeof arguments[1] === 'function') { + typeof arguments[1] === 'function') { throw new MongooseError('Model.prototype.deleteOne() no longer accepts a callback'); } @@ -1735,7 +1735,7 @@ function _ensureIndexes(model, options, callback) { function create() { if (options._automatic) { if (model.schema.options.autoIndex === false || - (model.schema.options.autoIndex == null && model.db.config.autoIndex === false)) { + (model.schema.options.autoIndex == null && model.db.config.autoIndex === false)) { return done(); } } @@ -2797,7 +2797,7 @@ Model.findByIdAndRemove = function(id, options) { Model.create = async function create(doc, options) { if (typeof options === 'function' || - typeof arguments[2] === 'function') { + typeof arguments[2] === 'function') { throw new MongooseError('Model.create() no longer accepts a callback'); } @@ -2812,7 +2812,7 @@ Model.create = async function create(doc, options) { } else { const last = arguments[arguments.length - 1]; const argumentsArray = Array.from(arguments); - const argumentsArrayAfterTheFirst = Array.from(arguments).slice(1); + const argumentsArrayAfterTheFirst = argumentsArray.slice(1); if (argumentsArray.some(argument => typeof argument === 'function')) { throw new MongooseError('Model.create() no longer accepts a callback'); @@ -2827,11 +2827,11 @@ Model.create = async function create(doc, options) { args = [...arguments]; if (args.length === 2 && - args[0] != null && - args[1] != null && - args[0].session == null && - getConstructorName(last.session) === 'ClientSession' && - !this.schema.path('session')) { + args[0] != null && + args[1] != null && + args[0].session == null && + getConstructorName(last.session) === 'ClientSession' && + !this.schema.path('session')) { // Probably means the user is running into the common mistake of trying // to use a spread to specify options, see gh-7535 utils.warn('WARNING: to pass a `session` to `Model.create()` in ' + @@ -3193,7 +3193,7 @@ Model.$__insertMany = function(arr, options, callback) { // `writeErrors` is a property reported by the MongoDB driver, // just not if there's only 1 error. if (error.writeErrors == null && - (error.result && error.result.result && error.result.result.writeErrors) != null) { + (error.result && error.result.result && error.result.result.writeErrors) != null) { error.writeErrors = error.result.result.writeErrors; } @@ -3362,7 +3362,7 @@ Model.bulkWrite = async function bulkWrite(ops, options) { _checkContext(this, 'bulkWrite'); if (typeof options === 'function' || - typeof arguments[2] === 'function') { + typeof arguments[2] === 'function') { throw new MongooseError('Model.bulkWrite() no longer accepts a callback'); } options = options || {}; @@ -3914,9 +3914,9 @@ function _update(model, op, conditions, doc, options) { options = typeof options === 'function' ? options : clone(options); const versionKey = model && - model.schema && - model.schema.options && - model.schema.options.versionKey || null; + model.schema && + model.schema.options && + model.schema.options.versionKey || null; _decorateUpdateWithVersionKey(doc, options, versionKey); return mq[op](conditions, doc, options); @@ -4282,9 +4282,9 @@ function populate(model, docs, options, callback) { // to fail. So delay running lean transform until _after_ // `_assign()` if (mod.options && - mod.options.options && - mod.options.options.lean && - mod.options.options.lean.transform) { + mod.options.options && + mod.options.options.lean && + mod.options.options.lean.transform) { mod.options.options._leanTransform = mod.options.options.lean.transform; mod.options.options.lean = true; } @@ -4413,8 +4413,8 @@ function _execPopulateQuery(mod, match, select, assignmentOpts, callback) { // field, that's the client's fault. for (const foreignField of mod.foreignField) { if (foreignField !== '_id' && - query.selectedInclusively() && - !isPathSelectedInclusive(query._fields, foreignField)) { + query.selectedInclusively() && + !isPathSelectedInclusive(query._fields, foreignField)) { query.select(foreignField); } } @@ -4770,7 +4770,7 @@ Model.__subclass = function subclass(conn, schema, collection) { Model.collection = Model.prototype.collection; Model.$__collection = Model.collection; // Errors handled internally, so ignore - Model.init().catch(() => { }); + Model.init().catch(() => {}); return Model; }; From ed425c7f4741b3b73011ea1b67f7e35d3261d427 Mon Sep 17 00:00:00 2001 From: Mohamed Yousef Date: Fri, 9 Jun 2023 20:08:48 +0300 Subject: [PATCH 4/4] refactor --- lib/model.js | 20 ++++++++------------ test/model.create.test.js | 16 ---------------- 2 files changed, 8 insertions(+), 28 deletions(-) diff --git a/lib/model.js b/lib/model.js index 53fda1bcf81..b865ae091df 100644 --- a/lib/model.js +++ b/lib/model.js @@ -2796,11 +2796,17 @@ Model.findByIdAndRemove = function(id, options) { */ Model.create = async function create(doc, options) { - if (typeof options === 'function' || - typeof arguments[2] === 'function') { + const argumentsArray = Array.from(arguments); + const argumentsArrayAfterTheFirst = argumentsArray.slice(1); + + if (argumentsArray.some(argument => typeof argument === 'function')) { throw new MongooseError('Model.create() no longer accepts a callback'); } + if (arguments.length > 1 && argumentsArrayAfterTheFirst.some(argument => !argument)) { + throw new MongooseError('Passed a falsy value to Model.create() after doc'); + } + _checkContext(this, 'create'); let args; @@ -2811,16 +2817,6 @@ Model.create = async function create(doc, options) { options = options != null && typeof options === 'object' ? options : {}; } else { const last = arguments[arguments.length - 1]; - const argumentsArray = Array.from(arguments); - const argumentsArrayAfterTheFirst = argumentsArray.slice(1); - - if (argumentsArray.some(argument => typeof argument === 'function')) { - throw new MongooseError('Model.create() no longer accepts a callback'); - } - - if (arguments.length > 1 && argumentsArrayAfterTheFirst.some(argument => !argument)) { - throw new MongooseError('Passed a falsy value to Model.create() after doc'); - } options = {}; diff --git a/test/model.create.test.js b/test/model.create.test.js index 1adf9e24a41..646b76c01f5 100644 --- a/test/model.create.test.js +++ b/test/model.create.test.js @@ -115,22 +115,6 @@ describe('model', function() { assert.ok(endTime - startTime < 4 * 100); // serial: >= 4 * 100 parallel: < 4 * 100 }); - it('creates a document when options equals undefined (gh-13487)', async function() { - const posts = await B.create({ title: 'undefined options' }, undefined); - const post1 = posts[0]; - assert.equal(post1.title, 'undefined options'); - }); - - it('creates multiple document when options equals undefined (gh-13487)', async function() { - const posts = await B.create({ title: 'Post1' }, { title: 'Post2' }, undefined); - - const post1 = posts[0]; - const post2 = posts[1]; - - assert.equal(post1.title, 'Post1'); - assert.equal(post2.title, 'Post2'); - }); - describe('callback is optional', function() { it('with one doc', async function() { const doc = await B.create({ title: 'optional callback' });