Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(document): handle calling set() underneath a strict: false subdocument path from the top-level document #13339

Merged
merged 4 commits into from
May 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 28 additions & 14 deletions lib/document.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const flattenObjectWithDottedPaths = require('./helpers/path/flattenObjectWithDo
const get = require('./helpers/get');
const getEmbeddedDiscriminatorPath = require('./helpers/document/getEmbeddedDiscriminatorPath');
const getKeysInSchemaOrder = require('./helpers/schema/getKeysInSchemaOrder');
const getSubdocumentStrictValue = require('./helpers/schema/getSubdocumentStrictValue');
const handleSpreadDoc = require('./helpers/document/handleSpreadDoc');
const immediate = require('./helpers/immediate');
const isDefiningProjection = require('./helpers/projection/isDefiningProjection');
Expand All @@ -52,6 +53,7 @@ const populateModelSymbol = require('./helpers/symbols').populateModelSymbol;
const scopeSymbol = require('./helpers/symbols').scopeSymbol;
const schemaMixedSymbol = require('./schema/symbols').schemaMixedSymbol;
const parentPaths = require('./helpers/path/parentPaths');
const getDeepestSubdocumentForPath = require('./helpers/document/getDeepestSubdocumentForPath');

let DocumentArray;
let MongooseArray;
Expand Down Expand Up @@ -1032,7 +1034,8 @@ Document.prototype.$set = function $set(path, val, type, options) {
let key;
let prefix;

const strict = options && 'strict' in options
const userSpecifiedStrict = options && 'strict' in options;
let strict = userSpecifiedStrict
? options.strict
: this.$__.strictMode;

Expand Down Expand Up @@ -1140,8 +1143,20 @@ Document.prototype.$set = function $set(path, val, type, options) {
}

let pathType = this.$__schema.pathType(path);
let parts = null;
if (pathType === 'adhocOrUndefined') {
pathType = getEmbeddedDiscriminatorPath(this, path, { typeOnly: true });
parts = path.indexOf('.') === -1 ? [path] : path.split('.');
pathType = getEmbeddedDiscriminatorPath(this, parts, { typeOnly: true });
}
if (pathType === 'adhocOrUndefined' && !userSpecifiedStrict) {
// May be path underneath non-strict schema
if (parts == null) {
parts = path.indexOf('.') === -1 ? [path] : path.split('.');
}
const subdocStrict = getSubdocumentStrictValue(this.$__schema, parts);
if (subdocStrict !== undefined) {
strict = subdocStrict;
}
}

// Assume this is a Mongoose document that was copied into a POJO using
Expand Down Expand Up @@ -1204,7 +1219,9 @@ Document.prototype.$set = function $set(path, val, type, options) {
}

let schema;
const parts = path.indexOf('.') === -1 ? [path] : path.split('.');
if (parts == null) {
parts = path.indexOf('.') === -1 ? [path] : path.split('.');
}

// Might need to change path for top-level alias
if (typeof this.$__schema.aliases[parts[0]] === 'string') {
Expand Down Expand Up @@ -1377,15 +1394,19 @@ Document.prototype.$set = function $set(path, val, type, options) {
didPopulate = true;
}

if (this.$__schema.singleNestedPaths[path] == null && (!refMatches || !schema.$isSingleNested || !val.$__)) {
if (!refMatches || !schema.$isSingleNested || !val.$__) {
// If this path is underneath a single nested schema, we'll call the setter
// later in `$__set()` because we don't take `_doc` when we iterate through
// a single nested doc. That's to make sure we get the correct context.
// Otherwise we would double-call the setter, see gh-7196.
let setterContext = this;
if (this.$__schema.singleNestedPaths[path] != null && parts.length > 1) {
setterContext = getDeepestSubdocumentForPath(this, parts, this.schema);
}
if (options != null && options.overwriteImmutable) {
val = schema.applySetters(val, this, false, priorVal, { overwriteImmutable: true });
val = schema.applySetters(val, setterContext, false, priorVal, { overwriteImmutable: true });
} else {
val = schema.applySetters(val, this, false, priorVal);
val = schema.applySetters(val, setterContext, false, priorVal);
}
}

Expand Down Expand Up @@ -1560,13 +1581,6 @@ Document.prototype.$__shouldModify = function(pathToMark, path, options, constru
return true;
}

// Re: the note about gh-7196, `val` is the raw value without casting or
// setters if the full path is under a single nested subdoc because we don't
// want to double run setters. So don't set it as modified. See gh-7264.
if (this.$__schema.singleNestedPaths[path] != null) {
return false;
}

if (val === void 0 && !this.$__isSelected(path)) {
// when a path is not selected in a query, its initial
// value will be undefined.
Expand Down Expand Up @@ -1679,7 +1693,7 @@ Document.prototype.$__set = function(pathToMark, path, options, constructing, pa
} else if (obj[parts[i]] && obj[parts[i]] instanceof Embedded) {
obj = obj[parts[i]];
} else if (obj[parts[i]] && !Array.isArray(obj[parts[i]]) && obj[parts[i]].$isSingleNested) {
obj = obj[parts[i]];
obj = obj[parts[i]]._doc;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undoing this change fixes #13876

} else if (obj[parts[i]] && Array.isArray(obj[parts[i]])) {
obj = obj[parts[i]];
} else {
Expand Down
38 changes: 38 additions & 0 deletions lib/helpers/document/getDeepestSubdocumentForPath.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
'use strict';

/**
* Find the deepest subdocument along a given path to ensure setter functions run
* with the correct subdocument as `this`. If no subdocuments, returns the top-level
* document.
*
* @param {Document} doc
* @param {String[]} parts
* @param {Schema} schema
* @returns Document
*/

module.exports = function getDeepestSubdocumentForPath(doc, parts, schema) {
vkarpov15 marked this conversation as resolved.
Show resolved Hide resolved
let curPath = parts[0];
let curSchema = schema;
let subdoc = doc;
for (let i = 0; i < parts.length - 1; ++i) {
const curSchemaType = curSchema.path(curPath);
if (curSchemaType && curSchemaType.schema) {
let newSubdoc = subdoc.get(curPath);
curSchema = curSchemaType.schema;
curPath = parts[i + 1];
if (Array.isArray(newSubdoc) && !isNaN(curPath)) {
newSubdoc = newSubdoc[curPath];
curPath = '';
}
if (newSubdoc == null) {
break;
}
subdoc = newSubdoc;
} else {
curPath += curPath.length ? '.' + parts[i + 1] : parts[i + 1];
}
}

return subdoc;
};
7 changes: 5 additions & 2 deletions lib/helpers/document/getEmbeddedDiscriminatorPath.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,19 @@ const getSchemaDiscriminatorByValue = require('../discriminator/getSchemaDiscrim
/**
* Like `schema.path()`, except with a document, because impossible to
* determine path type without knowing the embedded discriminator key.
*
* @param {Document} doc
* @param {String} path
* @param {String|String[]} path
* @param {Object} [options]
* @api private
*/

module.exports = function getEmbeddedDiscriminatorPath(doc, path, options) {
options = options || {};
const typeOnly = options.typeOnly;
const parts = path.indexOf('.') === -1 ? [path] : path.split('.');
const parts = Array.isArray(path) ?
vkarpov15 marked this conversation as resolved.
Show resolved Hide resolved
path :
(path.indexOf('.') === -1 ? [path] : path.split('.'));
let schemaType = null;
let type = 'adhocOrUndefined';

Expand Down
32 changes: 32 additions & 0 deletions lib/helpers/schema/getSubdocumentStrictValue.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
'use strict';

/**
* Find the `strict` mode setting for the deepest subdocument along a given path
* to ensure we have the correct default value for `strict`. When setting values
* underneath a subdocument, we should use the subdocument's `strict` setting by
* default, not the top-level document's.
*
* @param {Schema} schema
* @param {String[]} parts
* @returns {boolean | 'throw' | undefined}
*/

module.exports = function getSubdocumentStrictValue(schema, parts) {
if (parts.length === 1) {
return undefined;
}
let cur = parts[0];
let strict = undefined;
for (let i = 0; i < parts.length - 1; ++i) {
const curSchemaType = schema.path(cur);
if (curSchemaType && curSchemaType.schema) {
strict = curSchemaType.schema.options.strict;
schema = curSchemaType.schema;
cur = curSchemaType.$isMongooseDocumentArray && !isNaN(parts[i + 1]) ? '' : parts[i + 1];
} else {
cur += cur.length ? ('.' + parts[i + 1]) : parts[i + 1];
}
}

return strict;
};
26 changes: 26 additions & 0 deletions lib/schema.js
Original file line number Diff line number Diff line change
Expand Up @@ -739,6 +739,10 @@ Schema.prototype.add = function add(obj, prefix) {
if (this._userProvidedOptions.typeKey) {
childSchemaOptions.typeKey = this._userProvidedOptions.typeKey;
}
// propagate 'strict' option to child schema
if (this._userProvidedOptions.strict != null) {
childSchemaOptions.strict = this._userProvidedOptions.strict;
}

const _schema = new Schema(_typeDef, childSchemaOptions);
_schema.$implicitlyCreated = true;
Expand Down Expand Up @@ -2059,9 +2063,31 @@ Schema.prototype.set = function(key, value, tags) {
break;
}

// Propagate `strict` and `strictQuery` changes down to implicitly created schemas
if (key === 'strict') {
_propagateOptionsToImplicitlyCreatedSchemas(this, { strict: value });
}
if (key === 'strictQuery') {
_propagateOptionsToImplicitlyCreatedSchemas(this, { strictQuery: value });
}

return this;
};

/*!
* Recursively set options on implicitly created schemas
*/

function _propagateOptionsToImplicitlyCreatedSchemas(baseSchema, options) {
for (const { schema } of baseSchema.childSchemas) {
if (!schema.$implicitlyCreated) {
continue;
}
Object.assign(schema.options, options);
_propagateOptionsToImplicitlyCreatedSchemas(schema, options);
}
}

/**
* Gets a schema option.
*
Expand Down
1 change: 1 addition & 0 deletions lib/schema/string.js
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ SchemaString.checkRequired = SchemaType.checkRequired;
* @param {...String|Object} [args] enumeration values
* @return {SchemaType} this
* @see Customized Error Messages https://mongoosejs.com/docs/api/error.html#Error.messages
* @see Enums in JavaScript https://masteringjs.io/tutorials/fundamentals/enum
* @api public
*/

Expand Down
15 changes: 15 additions & 0 deletions test/document.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12079,6 +12079,21 @@ describe('document', function() {
assert.equal(fromDb.obj.subArr.length, 1);
assert.equal(fromDb.obj.subArr[0].str, 'subArr.test1');
});

it('can set() from top-level on nested schema with strict: false (gh-13327)', async function() {
const testSchema = new Schema({
d: new Schema({}, { strict: false, _id: false })
});
const Test = db.model('Test', testSchema);

const x = new Test();
x.set('d.x.y', 1);
assert.strictEqual(x.get('d.x.y'), 1);
await x.save();

const fromDb = await Test.findById(x._id).lean();
assert.equal(fromDb.d.x.y, 1);
});
});

describe('Check if instance function that is supplied in schema option is availabe', function() {
Expand Down
94 changes: 94 additions & 0 deletions test/helpers/document.getDeepestSubdocumentForPath.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
'use strict';

const assert = require('assert');
const getDeepestSubdocumentForPath = require('../../lib/helpers/document/getDeepestSubdocumentForPath');
const { mongoose } = require('../common');

describe('getDeepestSubdocumentForPath', function() {
beforeEach(() => mongoose.deleteModel(/Test/));
after(() => mongoose.deleteModel(/Test/));

it('returns top-level document if no subdocs', function() {
const Test = mongoose.model('Test', mongoose.Schema({ name: String }));
const doc = new Test({ name: 'foo' });

assert.strictEqual(getDeepestSubdocumentForPath(doc, ['name'], doc.schema), doc);
});

it('picks up single nested subdocs along the path', function() {
const Test = mongoose.model('Test', mongoose.Schema({
name: String,
nested: mongoose.Schema({
nestedPath: String
})
}));
const doc = new Test({ name: 'foo', nested: { nestedPath: 'bar' } });

assert.strictEqual(
getDeepestSubdocumentForPath(doc, ['nested', 'nestedPath'], doc.schema),
doc.nested
);
});

it('picks up document arrays', function() {
const Test = mongoose.model('Test', mongoose.Schema({
name: String,
docArr: [{
nestedPath: String
}]
}));
const doc = new Test({ name: 'foo', docArr: [{ nestedPath: 'bar' }] });

const res = getDeepestSubdocumentForPath(doc, ['docArr', '0', 'nestedPath'], doc.schema);
const expected = doc.docArr[0];
assert.strictEqual(res, expected);
});

it('picks up doubly nested subdocuments', function() {
const Test = mongoose.model('Test', mongoose.Schema({
name: String,
l1: mongoose.Schema({
l2: mongoose.Schema({
nested: String
})
})
}));
const doc = new Test({ name: 'foo', l1: { l2: { nested: 'bar' } } });

const res = getDeepestSubdocumentForPath(doc, ['l1', 'l2', 'nested'], doc.schema);
const expected = doc.l1.l2;
assert.strictEqual(res, expected);
});

it('returns deepest non-null subdoc', function() {
const Test = mongoose.model('Test', mongoose.Schema({
name: String,
l1: mongoose.Schema({
l2: mongoose.Schema({
nested: String
})
})
}));
const doc = new Test({ name: 'foo', l1: { l2: null } });

const res = getDeepestSubdocumentForPath(doc, ['l1', 'l2', 'nested'], doc.schema);
const expected = doc.l1;
assert.strictEqual(res, expected);
});

it('picks up single nested subdocs under document arrays', function() {
const Test = mongoose.model('Test', mongoose.Schema({
name: String,
docArr: [{
nested: mongoose.Schema({
l3Path: String
})
}]
}));
const doc = new Test({ name: 'foo', docArr: [{ nested: { l3Path: 'bar' } }] });

const res = getDeepestSubdocumentForPath(doc, ['docArr', '0', 'nested', 'l3Path'], doc.schema);
const expected = doc.docArr[0].nested;
assert.strictEqual(res, expected);
});
});
Loading