Skip to content

Commit

Permalink
fix(model+document): avoid depopulating manually populated doc as get…
Browse files Browse the repository at this point in the history
…ter value

Fix #14759
  • Loading branch information
vkarpov15 committed Jul 23, 2024
1 parent e4462c6 commit 7c2cfa8
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 9 deletions.
6 changes: 3 additions & 3 deletions lib/document.js
Original file line number Diff line number Diff line change
Expand Up @@ -1394,7 +1394,7 @@ Document.prototype.$set = function $set(path, val, type, options) {

let didPopulate = false;
if (refMatches && val instanceof Document && (!val.$__.wasPopulated || utils.deepEqual(val.$__.wasPopulated.value, val._id))) {
const unpopulatedValue = (schema && schema.$isSingleNested) ? schema.cast(val, this) : val._id;
const unpopulatedValue = (schema && schema.$isSingleNested) ? schema.cast(val, this) : val._doc._id;
this.$populated(path, unpopulatedValue, { [populateModelSymbol]: val.constructor });
val.$__.wasPopulated = { value: unpopulatedValue };
didPopulate = true;
Expand All @@ -1412,7 +1412,7 @@ Document.prototype.$set = function $set(path, val, type, options) {
this.$populated(path, val.map(function(v) { return v._id; }), popOpts);

for (const doc of val) {
doc.$__.wasPopulated = { value: doc._id };
doc.$__.wasPopulated = { value: doc._doc._id };
}
didPopulate = true;
}
Expand Down Expand Up @@ -3840,7 +3840,7 @@ Document.prototype.$toObject = function(options, json) {
// _isNested will only be true if this is not the top level document, we
// should never depopulate the top-level document
if (depopulate && options._isNested && this.$__.wasPopulated) {
return clone(this.$__.wasPopulated.value || this._id, options);
return clone(this.$__.wasPopulated.value || this._doc._id, options);
}

// merge default options with input options.
Expand Down
4 changes: 2 additions & 2 deletions lib/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -4459,7 +4459,7 @@ function _assign(model, vals, mod, assignmentOpts) {
}
} else {
if (_val instanceof Document) {
_val = _val._id;
_val = _val._doc._id;
}
key = String(_val);
if (rawDocs[key]) {
Expand All @@ -4468,7 +4468,7 @@ function _assign(model, vals, mod, assignmentOpts) {
rawOrder[key].push(i);
} else if (isVirtual ||
rawDocs[key].constructor !== val.constructor ||
String(rawDocs[key]._id) !== String(val._id)) {
String(rawDocs[key]._doc._id) !== String(val._doc._id)) {

This comment has been minimized.

Copy link
@Systerr

Systerr Aug 5, 2024

Actually we are getting
Cannot read properties of undefined (reading '_id')
after upgrading to 8.5.2

On our case we populating with lean()

    const someDAta = await SomeModel.find({
      field: { $in: someArray },
    })
      .populate({
        path: 'someID',
        model: 'AnotherModel',
        select: { _id: 1 },
        foreignField: 'FieldA',
      })
      .select({ _id: 1, field2: 1, 'barcode.sku': 1 })
      .lean();

It works fine before last release

This comment has been minimized.

Copy link
@vkarpov15

vkarpov15 Aug 12, 2024

Author Collaborator

Should be fixed by #14799, sorry for the trouble

// May need to store multiple docs with the same id if there's multiple models
// if we have discriminators or a ref function. But avoid converting to an array
// if we have multiple queries on the same model because of `perDocumentLimit` re: gh-9906
Expand Down
4 changes: 2 additions & 2 deletions lib/schemaType.js
Original file line number Diff line number Diff line change
Expand Up @@ -1542,7 +1542,7 @@ SchemaType.prototype._castRef = function _castRef(value, doc, init) {
}

if (value.$__ != null) {
value.$__.wasPopulated = value.$__.wasPopulated || { value: value._id };
value.$__.wasPopulated = value.$__.wasPopulated || { value: value._doc._id };
return value;
}

Expand All @@ -1568,7 +1568,7 @@ SchemaType.prototype._castRef = function _castRef(value, doc, init) {
!doc.$__.populated[path].options.options ||
!doc.$__.populated[path].options.options.lean) {
ret = new pop.options[populateModelSymbol](value);
ret.$__.wasPopulated = { value: ret._id };
ret.$__.wasPopulated = { value: ret._doc._id };
}

return ret;
Expand Down
4 changes: 2 additions & 2 deletions lib/types/map.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,15 +116,15 @@ class MongooseMap extends Map {
v = new populated.options[populateModelSymbol](v);
}
// Doesn't support single nested "in-place" populate
v.$__.wasPopulated = { value: v._id };
v.$__.wasPopulated = { value: v._doc._id };
return v;
});
} else if (value != null) {
if (value.$__ == null) {
value = new populated.options[populateModelSymbol](value);
}
// Doesn't support single nested "in-place" populate
value.$__.wasPopulated = { value: value._id };
value.$__.wasPopulated = { value: value._doc._id };
}
} else {
try {
Expand Down
31 changes: 31 additions & 0 deletions test/model.populate.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11014,4 +11014,35 @@ describe('model: populate:', function() {
assert.equal(latestClass.students[1].name, 'Robert');
assert.equal(latestClass.students[1].grade.grade, 'B');
});

it('avoids depopulating manually populated doc as getter value (gh-14759)', async function() {
const ownerSchema = new mongoose.Schema({
_id: {
type: 'ObjectId',
get(value) {
return value == null ? value : value.toString();
}
},
name: 'String'
});
const petSchema = new mongoose.Schema({
name: 'String',
owner: { type: 'ObjectId', ref: 'Owner' }
});

const Owner = db.model('Owner', ownerSchema);
const Pet = db.model('Pet', petSchema);

const ownerId = new mongoose.Types.ObjectId();
const owner = new Owner({
_id: ownerId,
name: 'Alice'
});
await owner.save();
const pet = new Pet({ name: 'Kitty', owner: owner });
await pet.save();

const fromDb = await Pet.findOne({ owner: ownerId }).lean().orFail();
assert.ok(fromDb.owner instanceof mongoose.Types.ObjectId);
});
});

0 comments on commit 7c2cfa8

Please sign in to comment.