From 3ec17eb083ad9ffc1a21b6fe154bd3ae09726853 Mon Sep 17 00:00:00 2001 From: Evander Palacios Date: Thu, 27 Aug 2020 11:17:28 -0400 Subject: [PATCH 01/17] Implement cursor population in batches, only if a custom batchSize is set on the query --- lib/cursor/QueryCursor.js | 91 ++++++++++++++++++++------ test/query.cursor.test.js | 131 ++++++++++++++++++++++++++++++-------- 2 files changed, 176 insertions(+), 46 deletions(-) diff --git a/lib/cursor/QueryCursor.js b/lib/cursor/QueryCursor.js index a77f5bb472..0356d95fb7 100644 --- a/lib/cursor/QueryCursor.js +++ b/lib/cursor/QueryCursor.js @@ -295,33 +295,82 @@ function _next(ctx, cb) { } if (ctx.cursor) { - return ctx.cursor.next(function(error, doc) { - if (error) { - return callback(error); - } - if (!doc) { + function nextDoc(doc, pop) { + return ctx.query._mongooseOptions.lean ? + callback(null, doc) : + _create(ctx, doc, pop, callback); + } + if (ctx.query._mongooseOptions.populate && !ctx._pop) { + ctx._pop = helpers.preparePopulationOptionsMQ(ctx.query, + ctx.query._mongooseOptions); + ctx._pop.__noPromise = true; + } + if (ctx.query._mongooseOptions.populate && ctx.options.batchSize > 1) { + if (ctx._batchDocs && ctx._batchDocs.length) { + // Return a cached populated doc + return nextDoc(ctx._batchDocs.shift(), ctx._pop); + } else if (ctx._batchExhausted) { + // Internal cursor reported no more docs. Act the same here return callback(null, null); - } + } else { + const _batchDocs = [] + // Request as many docs as batchSize, to populate them also in batch + function populateBatch() { + if (!_batchDocs.length) { + return callback(null, null); + } + ctx.query.model.populate(_batchDocs, ctx._pop, function(err, docs) { + if (err) { + return callback(err); + } + + ctx._batchDocs = docs + + nextDoc(ctx._batchDocs.shift(), ctx._pop); + }); + } + + function onNext(error, doc) { + if (error) { + return callback(error); + } + if (!doc) { + ctx._batchExhausted = true + return populateBatch(); + } - const opts = ctx.query._mongooseOptions; - if (!opts.populate) { - return opts.lean ? - callback(null, doc) : - _create(ctx, doc, null, callback); + _batchDocs.push(doc); + + if (_batchDocs.length < ctx.options.batchSize) { + ctx.cursor.next(onNext); + } else { + populateBatch(); + } + } + + return ctx.cursor.next(onNext); } + } else { + return ctx.cursor.next(function(error, doc) { + if (error) { + return callback(error); + } + if (!doc) { + return callback(null, null); + } - const pop = helpers.preparePopulationOptionsMQ(ctx.query, - ctx.query._mongooseOptions); - pop.__noPromise = true; - ctx.query.model.populate(doc, pop, function(err, doc) { - if (err) { - return callback(err); + if (!ctx.query._mongooseOptions.populate) { + return nextDoc(doc, null); } - return opts.lean ? - callback(null, doc) : - _create(ctx, doc, pop, callback); + + ctx.query.model.populate(doc, ctx._pop, function(err, doc) { + if (err) { + return callback(err); + } + return nextDoc(doc, ctx._pop); + }); }); - }); + } } else { ctx.once('cursor', function() { _next(ctx, cb); diff --git a/test/query.cursor.test.js b/test/query.cursor.test.js index b184a2bd3d..639ceac399 100644 --- a/test/query.cursor.test.js +++ b/test/query.cursor.test.js @@ -108,7 +108,7 @@ describe('QueryCursor', function() { }); }); - it('with populate', function(done) { + describe('with populate', function() { const bandSchema = new Schema({ name: String, members: [{ type: mongoose.Schema.ObjectId, ref: 'Person' }] @@ -117,37 +117,118 @@ describe('QueryCursor', function() { name: String }); - const Person = db.model('Person', personSchema); - const Band = db.model('Band', bandSchema); + let Band; - const people = [ - { name: 'Axl Rose' }, - { name: 'Slash' }, - { name: 'Nikki Sixx' }, - { name: 'Vince Neil' } - ]; - Person.create(people, function(error, docs) { - assert.ifError(error); - const bands = [ - { name: 'Guns N\' Roses', members: [docs[0], docs[1]] }, - { name: 'Motley Crue', members: [docs[2], docs[3]] } + beforeEach(function(done) { + const Person = db.model('Person', personSchema); + Band = db.model('Band', bandSchema); + + const people = [ + { name: 'Axl Rose' }, + { name: 'Slash' }, + { name: 'Nikki Sixx' }, + { name: 'Vince Neil' }, + { name: 'Trent Reznor' }, + { name: 'Thom Yorke' }, + { name: 'Billy Corgan' } ]; - Band.create(bands, function(error) { + Person.create(people, function(error, docs) { + assert.ifError(error); + const bands = [ + { name: 'Guns N\' Roses', members: [docs[0], docs[1]] }, + { name: 'Motley Crue', members: [docs[2], docs[3]] }, + { name: 'Nine Inch Nails', members: [docs[4]] }, + { name: 'Radiohead', members: [docs[5]] }, + { name: 'The Smashing Pumpkins', members: [docs[6]] } + ]; + Band.create(bands, function(error) { + assert.ifError(error); + done(); + }); + }); + }); + + it('with populate without specify batchSize', function(done) { + const cursor = + Band.find().sort({ name: 1 }).populate('members').cursor(); + cursor.next(function(error, doc) { + assert.ifError(error); + assert.equal(cursor.cursor.cursorState.currentLimit, 1); + assert.equal(doc.name, 'Guns N\' Roses'); + assert.equal(doc.members.length, 2); + assert.equal(doc.members[0].name, 'Axl Rose'); + assert.equal(doc.members[1].name, 'Slash'); + cursor.next(function(error, doc) { + assert.ifError(error); + assert.equal(cursor.cursor.cursorState.currentLimit, 2); + assert.equal(doc.name, 'Motley Crue'); + assert.equal(doc.members.length, 2); + assert.equal(doc.members[0].name, 'Nikki Sixx'); + assert.equal(doc.members[1].name, 'Vince Neil'); + cursor.next(function(error, doc) { + assert.ifError(error); + assert.equal(cursor.cursor.cursorState.currentLimit, 3); + assert.equal(doc.name, 'Nine Inch Nails'); + assert.equal(doc.members.length, 1); + assert.equal(doc.members[0].name, 'Trent Reznor'); + cursor.next(function(error, doc) { + assert.ifError(error); + assert.equal(cursor.cursor.cursorState.currentLimit, 4); + assert.equal(doc.name, 'Radiohead'); + assert.equal(doc.members.length, 1); + assert.equal(doc.members[0].name, 'Thom Yorke'); + cursor.next(function(error, doc) { + assert.ifError(error); + assert.equal(cursor.cursor.cursorState.currentLimit, 5); + assert.equal(doc.name, 'The Smashing Pumpkins'); + assert.equal(doc.members.length, 1); + assert.equal(doc.members[0].name, 'Billy Corgan'); + done(); + }); + }); + }); + }); + }); + }); + + it('with populate using custom batchSize', function(done) { + const cursor = + Band.find().sort({ name: 1 }).populate('members').batchSize(3).cursor(); + cursor.next(function(error, doc) { assert.ifError(error); - const cursor = - Band.find().sort({ name: 1 }).populate('members').cursor(); + assert.equal(cursor.cursor.cursorState.currentLimit, 3); + assert.equal(doc.name, 'Guns N\' Roses'); + assert.equal(doc.members.length, 2); + assert.equal(doc.members[0].name, 'Axl Rose'); + assert.equal(doc.members[1].name, 'Slash'); cursor.next(function(error, doc) { assert.ifError(error); - assert.equal(doc.name, 'Guns N\' Roses'); + assert.equal(cursor.cursor.cursorState.currentLimit, 3); + assert.equal(doc.name, 'Motley Crue'); assert.equal(doc.members.length, 2); - assert.equal(doc.members[0].name, 'Axl Rose'); - assert.equal(doc.members[1].name, 'Slash'); + assert.equal(doc.members[0].name, 'Nikki Sixx'); + assert.equal(doc.members[1].name, 'Vince Neil'); cursor.next(function(error, doc) { - assert.equal(doc.name, 'Motley Crue'); - assert.equal(doc.members.length, 2); - assert.equal(doc.members[0].name, 'Nikki Sixx'); - assert.equal(doc.members[1].name, 'Vince Neil'); - done(); + assert.ifError(error); + assert.equal(cursor.cursor.cursorState.currentLimit, 3); + assert.equal(doc.name, 'Nine Inch Nails'); + assert.equal(doc.members.length, 1); + assert.equal(doc.members[0].name, 'Trent Reznor'); + cursor.next(function(error, doc) { + assert.ifError(error); + assert.equal(cursor.cursor.cursorState.currentLimit, 5); + assert.equal(doc.name, 'Radiohead'); + assert.equal(doc.members.length, 1); + assert.equal(doc.members[0].name, 'Thom Yorke'); + cursor.next(function(error, doc) { + assert.ifError(error); + assert.equal(cursor.cursor.cursorState.currentLimit, 5); + assert.equal(doc.name, 'The Smashing Pumpkins'); + assert.equal(doc.members.length, 1); + assert.equal(doc.members[0].name, 'Billy Corgan'); + done(); + }); + }); }); }); }); From cf68cdc1f5464cd76e982a56c5a347ea05193f36 Mon Sep 17 00:00:00 2001 From: Evander Palacios Date: Thu, 27 Aug 2020 12:23:26 -0400 Subject: [PATCH 02/17] ESLint changes --- lib/cursor/QueryCursor.js | 99 ++++++++++++++++++++++----------------- 1 file changed, 56 insertions(+), 43 deletions(-) diff --git a/lib/cursor/QueryCursor.js b/lib/cursor/QueryCursor.js index 0356d95fb7..bbcb9f3d99 100644 --- a/lib/cursor/QueryCursor.js +++ b/lib/cursor/QueryCursor.js @@ -295,11 +295,6 @@ function _next(ctx, cb) { } if (ctx.cursor) { - function nextDoc(doc, pop) { - return ctx.query._mongooseOptions.lean ? - callback(null, doc) : - _create(ctx, doc, pop, callback); - } if (ctx.query._mongooseOptions.populate && !ctx._pop) { ctx._pop = helpers.preparePopulationOptionsMQ(ctx.query, ctx.query._mongooseOptions); @@ -308,47 +303,13 @@ function _next(ctx, cb) { if (ctx.query._mongooseOptions.populate && ctx.options.batchSize > 1) { if (ctx._batchDocs && ctx._batchDocs.length) { // Return a cached populated doc - return nextDoc(ctx._batchDocs.shift(), ctx._pop); + return _nextDoc(ctx, ctx._batchDocs.shift(), ctx._pop, callback); } else if (ctx._batchExhausted) { // Internal cursor reported no more docs. Act the same here return callback(null, null); } else { - const _batchDocs = [] // Request as many docs as batchSize, to populate them also in batch - function populateBatch() { - if (!_batchDocs.length) { - return callback(null, null); - } - ctx.query.model.populate(_batchDocs, ctx._pop, function(err, docs) { - if (err) { - return callback(err); - } - - ctx._batchDocs = docs - - nextDoc(ctx._batchDocs.shift(), ctx._pop); - }); - } - - function onNext(error, doc) { - if (error) { - return callback(error); - } - if (!doc) { - ctx._batchExhausted = true - return populateBatch(); - } - - _batchDocs.push(doc); - - if (_batchDocs.length < ctx.options.batchSize) { - ctx.cursor.next(onNext); - } else { - populateBatch(); - } - } - - return ctx.cursor.next(onNext); + return ctx.cursor.next(_onNext.bind({ ctx, callback, batchDocs: [] })); } } else { return ctx.cursor.next(function(error, doc) { @@ -360,14 +321,14 @@ function _next(ctx, cb) { } if (!ctx.query._mongooseOptions.populate) { - return nextDoc(doc, null); + return _nextDoc(ctx, doc, null, callback); } ctx.query.model.populate(doc, ctx._pop, function(err, doc) { if (err) { return callback(err); } - return nextDoc(doc, ctx._pop); + return _nextDoc(ctx, doc, ctx._pop, callback); }); }); } @@ -378,6 +339,58 @@ function _next(ctx, cb) { } } +/*! + * ignore + */ + +function _onNext(error, doc) { + if (error) { + return this.callback(error); + } + if (!doc) { + this.ctx._batchExhausted = true; + return _populateBatch.call(this); + } + + this.batchDocs.push(doc); + + if (this.batchDocs.length < this.ctx.options.batchSize) { + this.ctx.cursor.next(_onNext.bind(this)); + } else { + _populateBatch.call(this); + } +} + +/*! + * ignore + */ + +function _populateBatch() { + if (!this.batchDocs.length) { + return this.callback(null, null); + } + const _this = this; + this.ctx.query.model.populate(this.batchDocs, this.ctx._pop, function(err, docs) { + if (err) { + return _this.callback(err); + } + + _this.ctx._batchDocs = docs; + + _nextDoc(_this.ctx, _this.ctx._batchDocs.shift(), _this.ctx._pop, _this.callback); + }); +} + +/*! + * ignore + */ + +function _nextDoc(ctx, doc, pop, callback) { + return ctx.query._mongooseOptions.lean ? + callback(null, doc) : + _create(ctx, doc, pop, callback); +} + /*! * ignore */ From ef87058203e0cf257e8eeaac3fac950b83ce9057 Mon Sep 17 00:00:00 2001 From: Evander Palacios Date: Fri, 28 Aug 2020 10:53:24 -0400 Subject: [PATCH 03/17] No need to use a separate batchDocs array, as populate modifies docs in-place --- lib/cursor/QueryCursor.js | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/cursor/QueryCursor.js b/lib/cursor/QueryCursor.js index bbcb9f3d99..194b2825e5 100644 --- a/lib/cursor/QueryCursor.js +++ b/lib/cursor/QueryCursor.js @@ -309,7 +309,8 @@ function _next(ctx, cb) { return callback(null, null); } else { // Request as many docs as batchSize, to populate them also in batch - return ctx.cursor.next(_onNext.bind({ ctx, callback, batchDocs: [] })); + ctx._batchDocs = []; + return ctx.cursor.next(_onNext.bind({ ctx, callback })); } } else { return ctx.cursor.next(function(error, doc) { @@ -352,9 +353,9 @@ function _onNext(error, doc) { return _populateBatch.call(this); } - this.batchDocs.push(doc); + this.ctx._batchDocs.push(doc); - if (this.batchDocs.length < this.ctx.options.batchSize) { + if (this.ctx._batchDocs.length < this.ctx.options.batchSize) { this.ctx.cursor.next(_onNext.bind(this)); } else { _populateBatch.call(this); @@ -366,17 +367,15 @@ function _onNext(error, doc) { */ function _populateBatch() { - if (!this.batchDocs.length) { + if (!this.ctx._batchDocs.length) { return this.callback(null, null); } const _this = this; - this.ctx.query.model.populate(this.batchDocs, this.ctx._pop, function(err, docs) { + this.ctx.query.model.populate(this.ctx._batchDocs, this.ctx._pop, function(err) { if (err) { return _this.callback(err); } - _this.ctx._batchDocs = docs; - _nextDoc(_this.ctx, _this.ctx._batchDocs.shift(), _this.ctx._pop, _this.callback); }); } From 63fcb045a54fcf2465d26af3f0b81be934613c24 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Thu, 10 Sep 2020 12:19:32 -0400 Subject: [PATCH 04/17] feat(document+model): make change tracking skip saving if new value matches last saved value Fix #9396 --- lib/document.js | 25 ++++++++++++++++++- lib/model.js | 1 + test/document.test.js | 58 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 1 deletion(-) diff --git a/lib/document.js b/lib/document.js index 11217cecc7..2464b7e7ff 100644 --- a/lib/document.js +++ b/lib/document.js @@ -1021,6 +1021,16 @@ Document.prototype.$set = function $set(path, val, type, options) { if (pathType === 'nested' && val) { if (typeof val === 'object' && val != null) { + const hasPriorVal = this.$__.savedState != null && this.$__.savedState.hasOwnProperty(path); + if (this.$__.savedState != null && !this.isNew && !this.$__.savedState.hasOwnProperty(path)) { + this.$__.savedState[path] = this.$__getValue(path); + + const keys = Object.keys(this.$__.savedState[path]); + for (const key of keys) { + this.$__.savedState[path + '.' + key] = this.$__.savedState[path][key]; + } + } + if (!merge) { this.$__setValue(path, null); cleanModifiedSubpaths(this, path); @@ -1033,7 +1043,12 @@ Document.prototype.$set = function $set(path, val, type, options) { for (const key of keys) { this.$set(path + '.' + key, val[key], constructing); } - this.markModified(path); + + if (hasPriorVal && utils.deepEqual(this.$__.savedState[path], val)) { + this.unmarkModified(path); + } else { + this.markModified(path); + } cleanModifiedSubpaths(this, path, { skipDocArrays: true }); return this; } @@ -1279,6 +1294,14 @@ Document.prototype.$set = function $set(path, val, type, options) { if (shouldSet) { this.$__set(pathToMark, path, constructing, parts, schema, val, priorVal); + + if (this.$__.savedState != null) { + if (!this.isNew && !this.$__.savedState.hasOwnProperty(path)) { + this.$__.savedState[path] = priorVal; + } else if (this.$__.savedState.hasOwnProperty(path) && utils.deepEqual(val, this.$__.savedState[path])) { + this.unmarkModified(path); + } + } } if (schema.$isSingleNested && (this.isDirectModified(path) || val == null)) { diff --git a/lib/model.js b/lib/model.js index 84b4220bd3..6ea40f5ba1 100644 --- a/lib/model.js +++ b/lib/model.js @@ -402,6 +402,7 @@ Model.prototype.$__save = function(options, callback) { } } this.$__.saving = undefined; + this.$__.savedState = {}; this.emit('save', this, numAffected); this.constructor.emit('save', this, numAffected); callback(null, this); diff --git a/test/document.test.js b/test/document.test.js index e58c8f9a5f..3ff19a18e1 100644 --- a/test/document.test.js +++ b/test/document.test.js @@ -9325,4 +9325,62 @@ describe('document', function() { assert.equal(p.nested.test, 'new'); }); + + it('unmarks modified if setting a value to the same value as it was previously (gh-9396)', function() { + const schema = new Schema({ + bar: String + }); + + const Test = db.model('Test', schema); + + return co(function*() { + const foo = new Test({ bar: 'bar' }); + yield foo.save(); + assert.ok(!foo.isModified('bar')); + + foo.bar = 'baz'; + assert.ok(foo.isModified('bar')); + + foo.bar = 'bar'; + assert.ok(!foo.isModified('bar')); + }); + }); + + it('unmarks modified if setting a value to the same subdoc as it was previously (gh-9396)', function() { + const schema = new Schema({ + nested: { bar: String }, + subdoc: new Schema({ bar: String }, { _id: false }) + }); + const Test = db.model('Test', schema); + + return co(function*() { + const foo = new Test({ nested: { bar: 'bar' }, subdoc: { bar: 'bar' } }); + yield foo.save(); + assert.ok(!foo.isModified('nested')); + assert.ok(!foo.isModified('subdoc')); + + foo.nested = { bar: 'baz' }; + foo.subdoc = { bar: 'baz' }; + assert.ok(foo.isModified('nested')); + assert.ok(foo.isModified('subdoc')); + + foo.nested = { bar: 'bar' }; + foo.subdoc = { bar: 'bar' }; + assert.ok(!foo.isModified('nested')); + assert.ok(!foo.isModified('subdoc')); + assert.ok(!foo.isModified('subdoc.bar')); + + foo.nested = { bar: 'baz' }; + foo.subdoc = { bar: 'baz' }; + assert.ok(foo.isModified('nested')); + assert.ok(foo.isModified('subdoc')); + yield foo.save(); + + foo.nested = { bar: 'bar' }; + foo.subdoc = { bar: 'bar' }; + assert.ok(foo.isModified('nested')); + assert.ok(foo.isModified('subdoc')); + assert.ok(foo.isModified('subdoc.bar')); + }); + }); }); From 48dcd05196393a465986b7661c30a065020b75bd Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Thu, 15 Oct 2020 13:44:06 -0400 Subject: [PATCH 05/17] chore: update opencollective sponsors --- index.pug | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/index.pug b/index.pug index a75cbd45af..7802f03e5d 100644 --- a/index.pug +++ b/index.pug @@ -298,9 +298,6 @@ html(lang='en') - - - @@ -355,7 +352,7 @@ html(lang='en') - + From 3f2f8c149bff28ece6b960116b2c5c19e439c42e Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Thu, 15 Oct 2020 14:04:26 -0400 Subject: [PATCH 06/17] fix(schema): handle merging schemas from separate Mongoose module instances when schema has a virtual Fix #9471 --- lib/schema.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/schema.js b/lib/schema.js index 809d80463d..011fc35260 100644 --- a/lib/schema.js +++ b/lib/schema.js @@ -477,7 +477,7 @@ Schema.prototype.add = function add(obj, prefix) { if (key === '_id' && obj[key] === false) { continue; } - if (obj[key] instanceof VirtualType) { + if (obj[key] instanceof VirtualType || obj[key].constructor.name === 'VirtualType') { this.virtual(obj[key]); continue; } @@ -1678,7 +1678,7 @@ Schema.prototype.indexes = function() { */ Schema.prototype.virtual = function(name, options) { - if (name instanceof VirtualType) { + if (name instanceof VirtualType || (name != null && name.constructor.name === 'VirtualType')) { return this.virtual(name.path, name.options); } From e0867ab0d3e9aa7cd6a5f1cdfb874cb12bda93ef Mon Sep 17 00:00:00 2001 From: Tareq Dayya <41693150+tareqdayya@users.noreply.github.com> Date: Sun, 18 Oct 2020 14:49:50 +0300 Subject: [PATCH 07/17] Update connections.pug Make it clear that mongoose.connection.on('error') does not fire once connection is lost any time after the initial connection. The text seems to suggest, to me at least, that it's the code to use to listen to drops in connection after the initial connection. Only 3 Lines were added: 148-150. --- docs/connections.pug | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/connections.pug b/docs/connections.pug index 933c8f1812..ae9c93972a 100644 --- a/docs/connections.pug +++ b/docs/connections.pug @@ -145,6 +145,10 @@ block content }); ``` + Note that the `error` event in the code above does not fire when mongoose loses + connection after the initial connection was established. You can listen to the + `disconnected` event for that purpose. +

Options

The `connect` method also accepts an `options` object which will be passed From d06388ff3168f72b178cb08568f2fdabd23306f3 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Thu, 22 Oct 2020 10:45:19 -0400 Subject: [PATCH 08/17] test(query): repro #9479 --- test/model.query.casting.test.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/model.query.casting.test.js b/test/model.query.casting.test.js index ad45fbb988..9ed7947ca1 100644 --- a/test/model.query.casting.test.js +++ b/test/model.query.casting.test.js @@ -7,6 +7,7 @@ const start = require('./common'); const assert = require('assert'); +const co = require('co'); const random = require('../lib/utils').random; const mongoose = start.mongoose; @@ -900,6 +901,22 @@ describe('model query casting', function() { }); } }); + + it('casts $nor within $elemMatch (gh-9479)', function() { + const Test = db.model('Test', Schema({ + arr: [{ x: Number, y: Number }] + })); + + return co(function*() { + const _doc = yield Test.create({ arr: [{ x: 1 }, { y: 3 }, { x: 2 }] }); + + const doc = yield Test.findOne({ + arr: { $elemMatch: { $nor: [{ x: 1 }, { y: 3 }] } } + }); + + assert.equal(_doc._id.toString(), doc._id.toString()); + }); + }); }); it('works with $all (gh-3394)', function(done) { From 39e54ef657a3d2dffe183bb3c490fe7fa4bbc0c4 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Thu, 22 Oct 2020 10:46:14 -0400 Subject: [PATCH 09/17] fix(query): cast $nor within $elemMatch Fix #9479 --- lib/schema/array.js | 26 ++++++++++++++++---------- test/model.query.casting.test.js | 2 +- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/lib/schema/array.js b/lib/schema/array.js index 044cc3af23..828eb1be09 100644 --- a/lib/schema/array.js +++ b/lib/schema/array.js @@ -537,18 +537,24 @@ handle.$all = cast$all; handle.$options = String; handle.$elemMatch = cast$elemMatch; handle.$geoIntersects = geospatial.cast$geoIntersects; -handle.$or = handle.$and = function(val) { - if (!Array.isArray(val)) { - throw new TypeError('conditional $or/$and require array'); - } +handle.$or = createLogicalQueryOperatorHandler('$or'); +handle.$and = createLogicalQueryOperatorHandler('$and'); +handle.$nor = createLogicalQueryOperatorHandler('$nor'); + +function createLogicalQueryOperatorHandler(op) { + return function logicalQueryOperatorHandler(val) { + if (!Array.isArray(val)) { + throw new TypeError('conditional ' + op + ' requires an array'); + } - const ret = []; - for (const obj of val) { - ret.push(cast(this.casterConstructor.schema, obj)); - } + const ret = []; + for (const obj of val) { + ret.push(cast(this.casterConstructor.schema, obj)); + } - return ret; -}; + return ret; + }; +} handle.$near = handle.$nearSphere = geospatial.cast$near; diff --git a/test/model.query.casting.test.js b/test/model.query.casting.test.js index 9ed7947ca1..357c7c52a9 100644 --- a/test/model.query.casting.test.js +++ b/test/model.query.casting.test.js @@ -911,7 +911,7 @@ describe('model query casting', function() { const _doc = yield Test.create({ arr: [{ x: 1 }, { y: 3 }, { x: 2 }] }); const doc = yield Test.findOne({ - arr: { $elemMatch: { $nor: [{ x: 1 }, { y: 3 }] } } + arr: { $elemMatch: { $nor: [{ x: 1 }, { y: 3 }] } } }); assert.equal(_doc._id.toString(), doc._id.toString()); From 86aace14dbd82a01ec996a6a4d1b4f6205484233 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Thu, 22 Oct 2020 11:05:30 -0400 Subject: [PATCH 10/17] fix(schema): handle objects without a constructor property re: #9471 --- lib/schema.js | 2 +- test/object.create.null.test.js | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/schema.js b/lib/schema.js index 011fc35260..2abd0a01a2 100644 --- a/lib/schema.js +++ b/lib/schema.js @@ -477,7 +477,7 @@ Schema.prototype.add = function add(obj, prefix) { if (key === '_id' && obj[key] === false) { continue; } - if (obj[key] instanceof VirtualType || obj[key].constructor.name === 'VirtualType') { + if (obj[key] instanceof VirtualType || get(obj[key], 'constructor.name', null) === 'VirtualType') { this.virtual(obj[key]); continue; } diff --git a/test/object.create.null.test.js b/test/object.create.null.test.js index 155014c54d..0443cfc8dc 100644 --- a/test/object.create.null.test.js +++ b/test/object.create.null.test.js @@ -118,25 +118,25 @@ describe('is compatible with object created using Object.create(null) (gh-1484)' o.nested = Object.create(null); o.nested.n = Number; - assert.doesNotThrow(function() { + void function() { new Schema(o); - }); + }(); - assert.doesNotThrow(function() { + void function() { const s = new Schema; const o = Object.create(null); o.yay = Number; s.path('works', o); - }); + }(); - assert.doesNotThrow(function() { + void function() { const s = new Schema; let o = Object.create(null); o = {}; o.name = String; const x = { type: [o] }; s.path('works', x); - }); + }(); done(); }); From 88681020db0cadd88f39799a7d56562dd49eb422 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Thu, 22 Oct 2020 12:28:36 -0400 Subject: [PATCH 11/17] feat(connection): add bufferTimeoutMS option that configures how long Mongoose will allow commands to buffer Fix #9469 --- docs/connections.pug | 4 +- docs/faq.pug | 24 ++++---- docs/guide.pug | 11 ++++ lib/collection.js | 40 +++++++++++++ lib/connection.js | 1 + lib/drivers/node-mongodb-native/collection.js | 59 +++++++++++++++---- lib/index.js | 1 + lib/validoptions.js | 1 + 8 files changed, 115 insertions(+), 26 deletions(-) diff --git a/docs/connections.pug b/docs/connections.pug index 933c8f1812..6614b74557 100644 --- a/docs/connections.pug +++ b/docs/connections.pug @@ -158,8 +158,8 @@ block content Mongoose passes options to the driver without modification, modulo a few exceptions that are explained below. - * `bufferCommands` - This is a mongoose-specific option (not passed to the MongoDB driver) that disables [mongoose's buffering mechanism](http://mongoosejs.com/docs/faq.html#callback_never_executes) - * `user`/`pass` - The username and password for authentication. These options are mongoose-specific, they are equivalent to the MongoDB driver's `auth.user` and `auth.password` options. + * `bufferCommands` - This is a mongoose-specific option (not passed to the MongoDB driver) that disables [Mongoose's buffering mechanism](http://mongoosejs.com/docs/faq.html#callback_never_executes) + * `user`/`pass` - The username and password for authentication. These options are Mongoose-specific, they are equivalent to the MongoDB driver's `auth.user` and `auth.password` options. * `autoIndex` - By default, mongoose will automatically build indexes defined in your schema when it connects. This is great for development, but not ideal for large production deployments, because index builds can cause performance degradation. If you set `autoIndex` to false, mongoose will not automatically build indexes for **any** model associated with this connection. * `dbName` - Specifies which database to connect to and overrides any database specified in the connection string. This is useful if you are unable to specify a default database in the connection string like with [some `mongodb+srv` syntax connections](https://stackoverflow.com/questions/48917591/fail-to-connect-mongoose-to-atlas/48917626#48917626). diff --git a/docs/faq.pug b/docs/faq.pug index 37269ea197..9ba681b04f 100644 --- a/docs/faq.pug +++ b/docs/faq.pug @@ -311,26 +311,26 @@ block content **Q**. My `save()` callback never executes. What am I doing wrong? **A**. All `collection` actions (insert, remove, queries, etc.) are queued - until the `connection` opens. It is likely that an error occurred while - attempting to connect. Try adding an error handler to your connection. + until Mongoose successfully connects to MongoDB. It is likely you haven't called Mongoose's + `connect()` or `createConnection()` function yet. - ```javascript - // if connecting on the default mongoose connection - mongoose.connect(..); - mongoose.connection.on('error', handleError); - - // if connecting on a separate connection - const conn = mongoose.createConnection(..); - conn.on('error', handleError); - ``` + In Mongoose 5.11, there is a `bufferTimeoutMS` option (set to 10000 by default) that configures how long + Mongoose will allow an operation to stay buffered before throwing an error. - If you want to opt out of mongoose's buffering mechanism across your entire + If you want to opt out of Mongoose's buffering mechanism across your entire application, set the global `bufferCommands` option to false: ```javascript mongoose.set('bufferCommands', false); ``` + Instead of opting out of Mongoose's buffering mechanism, you may want to instead reduce `bufferTimeoutMS` + to make Mongoose only buffer for a short time. + + ```javascript + // If an operation is buffered for more than 500ms, throw an error. + mongoose.set('bufferTimeoutMS', 500); + ```
diff --git a/docs/guide.pug b/docs/guide.pug index c539e4fa88..5a4df7761f 100644 --- a/docs/guide.pug +++ b/docs/guide.pug @@ -418,6 +418,7 @@ block content - [autoIndex](#autoIndex) - [autoCreate](#autoCreate) - [bufferCommands](#bufferCommands) + - [bufferTimeoutMS](#bufferTimeoutMS) - [capped](#capped) - [collection](#collection) - [id](#id) @@ -503,6 +504,16 @@ block content const schema = new Schema({..}, { bufferCommands: false }); ``` +

option: bufferTimeoutMS

+ + If `bufferCommands` is on, this option sets the maximum amount of time Mongoose buffering will wait before + throwing an error. If not specified, Mongoose will use 10000 (10 seconds). + + ```javascript + // If an operation is buffered for more than 1 second, throw an error. + const schema = new Schema({..}, { bufferTimeoutMS: 1000 }); + ``` +

option: capped

Mongoose supports MongoDBs [capped](http://www.mongodb.org/display/DOCS/Capped+Collections) diff --git a/lib/collection.js b/lib/collection.js index 97f5bb3238..076f88faac 100644 --- a/lib/collection.js +++ b/lib/collection.js @@ -108,6 +108,23 @@ Collection.prototype.addQueue = function(name, args) { return this; }; +/** + * Removes a queued method + * + * @param {String} name name of the method to queue + * @param {Array} args arguments to pass to the method when executed + * @api private + */ + +Collection.prototype.removeQueue = function(name, args) { + const index = this.queue.findIndex(v => v[0] === name && v[1] === args); + if (index === -1) { + return false; + } + this.queue.splice(index, 1); + return true; +}; + /** * Executes all queued methods and clears the queue. * @@ -281,6 +298,29 @@ Collection.prototype._shouldBufferCommands = function _shouldBufferCommands() { return true; }; +/*! + * ignore + */ + +Collection.prototype._getBufferTimeoutMS = function _getBufferTimeoutMS() { + const conn = this.conn; + const opts = this.opts; + + if (opts.bufferTimeoutMS != null) { + return opts.bufferTimeoutMS; + } + if (opts && opts.schemaUserProvidedOptions != null && opts.schemaUserProvidedOptions.bufferTimeoutMS != null) { + return opts.schemaUserProvidedOptions.bufferTimeoutMS; + } + if (conn.config.bufferTimeoutMS != null) { + return conn.config.bufferTimeoutMS; + } + if (conn.base != null && conn.base.get('bufferTimeoutMS') != null) { + return conn.base.get('bufferTimeoutMS'); + } + return 10000; +}; + /*! * Module exports. */ diff --git a/lib/connection.js b/lib/connection.js index dcf616e6e3..d44d045ac3 100644 --- a/lib/connection.js +++ b/lib/connection.js @@ -611,6 +611,7 @@ Connection.prototype.onOpen = function() { * @param {String} uri The URI to connect with. * @param {Object} [options] Passed on to http://mongodb.github.io/node-mongodb-native/2.2/api/MongoClient.html#connect * @param {Boolean} [options.bufferCommands=true] Mongoose specific option. Set to false to [disable buffering](http://mongoosejs.com/docs/faq.html#callback_never_executes) on all models associated with this connection. + * @param {Number} [options.bufferTimeoutMS=true] Mongoose specific option. If `bufferCommands` is true, Mongoose will throw an error after `bufferTimeoutMS` if the operation is still buffered. * @param {String} [options.dbName] The name of the database we want to use. If not provided, use database name from connection string. * @param {String} [options.user] username for authentication, equivalent to `options.auth.user`. Maintained for backwards compatibility. * @param {String} [options.pass] password for authentication, equivalent to `options.auth.password`. Maintained for backwards compatibility. diff --git a/lib/drivers/node-mongodb-native/collection.js b/lib/drivers/node-mongodb-native/collection.js index d660c7de51..62f18bec2a 100644 --- a/lib/drivers/node-mongodb-native/collection.js +++ b/lib/drivers/node-mongodb-native/collection.js @@ -144,21 +144,53 @@ function iter(i) { if (syncCollectionMethods[i]) { throw new Error('Collection method ' + i + ' is synchronous'); } - if (typeof lastArg === 'function') { - this.addQueue(i, args); - return; - } this.conn.emit('buffer', { method: i, args: args }); - return new this.Promise((resolve, reject) => { - this.addQueue(i, [].concat(args).concat([(err, res) => { - if (err != null) { - return reject(err); + let callback; + let _args; + let promise = null; + let timeout = null; + if (typeof lastArg === 'function') { + callback = function collectionOperationCallback() { + if (timeout != null) { + clearTimeout(timeout); } - resolve(res); - }])); - }); + return lastArg.apply(this, arguments); + }; + _args = args.slice(0, args.length - 1).concat([callback]); + } else { + promise = new this.Promise((resolve, reject) => { + callback = function collectionOperationCallback(err, res) { + if (timeout != null) { + clearTimeout(timeout); + } + if (err != null) { + return reject(err); + } + resolve(res); + }; + _args = args.concat([callback]); + this.addQueue(i, _args); + }); + } + + const bufferTimeoutMS = this._getBufferTimeoutMS(); + timeout = setTimeout(() => { + const removed = this.removeQueue(i, _args); + if (removed) { + const message = 'Operation `' + this.name + '.' + i + '()` buffering timed out after ' + + bufferTimeoutMS + 'ms'; + callback(new MongooseError(message)); + } + }, bufferTimeoutMS); + + if (typeof lastArg === 'function') { + this.addQueue(i, _args); + return; + } + + return promise; } if (debug) { @@ -174,7 +206,10 @@ function iter(i) { try { if (collection == null) { - throw new MongooseError('Cannot call `' + this.name + '.' + i + '()` before initial connection is complete if `bufferCommands = false`. Make sure you `await mongoose.connect()` if you have `bufferCommands = false`.'); + const message = 'Cannot call `' + this.name + '.' + i + '()` before initial connection ' + + 'is complete if `bufferCommands = false`. Make sure you `await mongoose.connect()` if ' + + 'you have `bufferCommands = false`.'; + throw new MongooseError(message); } return collection[i].apply(collection, args); diff --git a/lib/index.js b/lib/index.js index fccb625bd6..5769cc5997 100644 --- a/lib/index.js +++ b/lib/index.js @@ -310,6 +310,7 @@ Mongoose.prototype.createConnection = function(uri, options, callback) { * @param {String} uri(s) * @param {Object} [options] passed down to the [MongoDB driver's `connect()` function](http://mongodb.github.io/node-mongodb-native/3.0/api/MongoClient.html), except for 4 mongoose-specific options explained below. * @param {Boolean} [options.bufferCommands=true] Mongoose specific option. Set to false to [disable buffering](http://mongoosejs.com/docs/faq.html#callback_never_executes) on all models associated with this connection. + * @param {Number} [options.bufferTimeoutMS=true] Mongoose specific option. If `bufferCommands` is true, Mongoose will throw an error after `bufferTimeoutMS` if the operation is still buffered. * @param {String} [options.dbName] The name of the database we want to use. If not provided, use database name from connection string. * @param {String} [options.user] username for authentication, equivalent to `options.auth.user`. Maintained for backwards compatibility. * @param {String} [options.pass] password for authentication, equivalent to `options.auth.password`. Maintained for backwards compatibility. diff --git a/lib/validoptions.js b/lib/validoptions.js index 510d7e277e..3eb19550e6 100644 --- a/lib/validoptions.js +++ b/lib/validoptions.js @@ -11,6 +11,7 @@ const VALID_OPTIONS = Object.freeze([ 'autoCreate', 'autoIndex', 'bufferCommands', + 'bufferTimeoutMS', 'cloneSchemas', 'debug', 'maxTimeMS', From f52c589709e73107a84d22b8c8c918f04b1ef88e Mon Sep 17 00:00:00 2001 From: Hafez Date: Fri, 23 Oct 2020 08:08:19 +0200 Subject: [PATCH 12/17] test(connection): repro #9496 --- test/connection.test.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/connection.test.js b/test/connection.test.js index 03c534098c..41e065b023 100644 --- a/test/connection.test.js +++ b/test/connection.test.js @@ -1210,4 +1210,20 @@ describe('connections:', function() { assert.ok(res); }); }); + + it('connection.then(...) resolves to a connection instance (gh-9496)', function() { + return co(function *() { + const m = new mongoose.Mongoose; + + m.connect('mongodb://localhost:27017/test_gh9496', { + useNewUrlParser: true, + useUnifiedTopology: true + }); + const conn = yield m.connection; + + assert.ok(conn instanceof m.Connection); + assert.ok(conn); + }); + }); + }); From 82311de9da270e9ff7fb94cfb43b7700a449b3c2 Mon Sep 17 00:00:00 2001 From: Hafez Date: Fri, 23 Oct 2020 08:08:53 +0200 Subject: [PATCH 13/17] fix(connection): make connection.then(...) resolve to a connection instance re: #9496 --- lib/connection.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/connection.js b/lib/connection.js index dcf616e6e3..6e70a82230 100644 --- a/lib/connection.js +++ b/lib/connection.js @@ -811,7 +811,7 @@ Connection.prototype.openUri = function(uri, options, callback) { throw err; }); this.then = function(resolve, reject) { - return this.$initialConnection.then(resolve, reject); + return this.$initialConnection.then(() => resolve(_this), reject); }; this.catch = function(reject) { return this.$initialConnection.catch(reject); From 394186239e1b778812fa9bb973d45fa05ddd514c Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Fri, 23 Oct 2020 11:59:27 -0400 Subject: [PATCH 14/17] fix(aggregate): when using $search with discriminators, add `$match` as the 2nd stage in pipeline rather than 1st Fix #9487 --- lib/aggregate.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/aggregate.js b/lib/aggregate.js index 6dc43926f8..f562e57104 100644 --- a/lib/aggregate.js +++ b/lib/aggregate.js @@ -1139,6 +1139,14 @@ function prepareDiscriminatorPipeline(aggregate) { originalPipeline[0].$geoNear.query = originalPipeline[0].$geoNear.query || {}; originalPipeline[0].$geoNear.query[discriminatorKey] = discriminatorValue; + } else if (originalPipeline[0] && originalPipeline[0].$search) { + if (originalPipeline[1] && originalPipeline[1].$match != null) { + originalPipeline[1].$match[discriminatorKey] = originalPipeline[1].$match[discriminatorKey] || discriminatorValue; + } else { + const match = {}; + match[discriminatorKey] = discriminatorValue; + originalPipeline.splice(1, 0, { $match: match }); + } } else { const match = {}; match[discriminatorKey] = discriminatorValue; From 1264d7e6c09b6b868dec78bd35cc8db440f562b7 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Fri, 23 Oct 2020 12:51:46 -0400 Subject: [PATCH 15/17] chore: release 5.10.10 --- History.md | 8 ++++++++ package.json | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/History.md b/History.md index e1b569dca6..9086e2c746 100644 --- a/History.md +++ b/History.md @@ -1,3 +1,11 @@ +5.10.10 / 2020-10-23 +==================== + * fix(schema): handle merging schemas from separate Mongoose module instances when schema has a virtual #9471 + * fix(connection): make connection.then(...) resolve to a connection instance #9497 [AbdelrahmanHafez](https://github.com/AbdelrahmanHafez) + * fix(aggregate): when using $search with discriminators, add `$match` as the 2nd stage in pipeline rather than 1st #9487 + * fix(query): cast $nor within $elemMatch #9479 + * docs(connection): add note about 'error' event versus 'disconnected' event #9488 [tareqdayya](https://github.com/tareqdayya) + 5.10.9 / 2020-10-09 =================== * fix(update): strip out unused array filters to avoid "filter was not used in the update" error #9468 diff --git a/package.json b/package.json index a9eae6d386..763a381dfe 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "mongoose", "description": "Mongoose MongoDB ODM", - "version": "5.10.9", + "version": "5.10.10", "author": "Guillermo Rauch ", "keywords": [ "mongodb", From a8970ead786a6cc5c749497499b4e84cf303c367 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Fri, 23 Oct 2020 17:15:19 -0400 Subject: [PATCH 16/17] test(document): repro #9427 --- test/document.test.js | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/document.test.js b/test/document.test.js index 33074529a9..7aebf8ce27 100644 --- a/test/document.test.js +++ b/test/document.test.js @@ -9490,4 +9490,28 @@ describe('document', function() { assert.deepEqual(testUser.toObject().preferences.notifications, { email: true, push: false }); }); + + it('avoids overwriting array subdocument when setting dotted path that is not selected (gh-9427)', function() { + const Test = db.model('Test', Schema({ + arr: [{ _id: false, val: Number }], + name: String, + age: Number + })); + + return co(function*() { + let doc = yield Test.create({ + name: 'Test', + arr: [{ val: 1 }, { val: 2 }], + age: 30 + }); + + doc = yield Test.findById(doc._id).select('name'); + doc.set('arr.0.val', 2); + yield doc.save(); + + const fromDb = yield Test.findById(doc._id); + assert.deepEqual(fromDb.toObject().arr, [{ val: 2 }, { val: 2 }]); + }); + + }); }); From 800e233ef6f168e5ba7768f6d665cbc9336957d3 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Fri, 23 Oct 2020 17:15:32 -0400 Subject: [PATCH 17/17] fix(document): avoid overwriting array subdocument when setting dotted path that isn't selected Fix #9427 --- lib/types/embedded.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/types/embedded.js b/lib/types/embedded.js index d0cfdcfc73..18a977847a 100644 --- a/lib/types/embedded.js +++ b/lib/types/embedded.js @@ -100,7 +100,8 @@ EmbeddedDocument.prototype.markModified = function(path) { return; } - if (this.isNew) { + const pathToCheck = this.__parentArray.$path() + '.0.' + path; + if (this.isNew && this.ownerDocument().isSelected(pathToCheck)) { // Mark the WHOLE parent array as modified // if this is a new document (i.e., we are initializing // a document),