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

Edge case where SingleNested value update is not marked as changed and saving fails to update #9208

Closed
trappar opened this issue Jul 6, 2020 · 3 comments
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Milestone

Comments

@trappar
Copy link

trappar commented Jul 6, 2020

The bug

I've found a case where Mongoose's change tracking fails entirely.

Here's the simplest example I could create to reproduce the issue:

import mongoose from "mongoose";

// Do whatever setup/connection is needed first of course

const TestModel = mongoose.model('Test', new mongoose.Schema({
    nested: new mongoose.Schema({
        myBool: Boolean,
        myString: String,
    })
}));

(async function run() {
    const test = new TestModel();

    test.nested = {myBool: true}
    await test.save();
    // DB now has: {nested: {myBool: true}}

    test.nested = {myString: "asdf"};
    await test.save();
    // DB now has: {nested: {myString: "asdf"}}

    test.nested.myBool = true;
    await test.save();
    // DB still has: {nested: {myString: "asdf"}}

    // The change to myBool is completely untracked

    await test.remove();
})().then(() => process.exit());

Here's the log output of running this code:

Mongoose: tests.insertOne({ _id: ObjectId("5f02f528449eb2860795c345"), nested: { _id: ObjectId("5f02f52f449eb2860795c346"), myBool: true }, __v: 0}, { session: null })
Mongoose: tests.updateOne({ _id: ObjectId("5f02f528449eb2860795c345") }, { '$set': { nested: { _id: ObjectId("5f02f574449eb2860795c347"), myString: 'asdf' } }}, { session: null })
Mongoose: tests.findOne({ _id: ObjectId("5f02f528449eb2860795c345") }, { session: null, projection: { _id: 1 } })
Mongoose: tests.deleteOne({ _id: ObjectId("5f02f528449eb2860795c345") }, RemoveOptions { session: null })

Note the 3rd line where Mongoose fails updating myBool = true

Why this is happening

I've found is that this is happening because each time a SingleNested doc is cast by setting it to an object which contains at least one key - then the existing value for that SingleNested is recorded as the "priorDoc". This happens here:

if (Object.keys(val).length === 0) {
return new Constructor({}, selected, doc);
}
return new Constructor(val, selected, doc, undefined, { priorDoc: priorVal });

And when setting a value, Mongoose looks at this priorDoc to see if it contains a matching value to the one that's currently being set. So if you set a value to the same value that was in the priorDoc, then it doesn't consider this to be a change. This happens here:

mongoose/lib/document.js

Lines 1121 to 1134 in a3f61ad

const priorVal = (() => {
if (this.$__.$options.priorDoc != null) {
return this.$__.$options.priorDoc.$__getValue(path);
}
if (constructing) {
return void 0;
}
return this.$__getValue(path);
})();
if (!schema) {
this.$__set(pathToMark, path, constructing, parts, schema, val, priorVal);
return this;
}

Workaround

While a fix for this is unavailable, there is a reasonably simple workaround. Never set a nested document to an object other than an empty object literal. Doing this will ensure that the priorDoc is not kept in memory.

//Change any situation where a SingleNested is set to an object containing at least one key
test.nested = {myString: "asdf"};

// To
test.nested = {};
test.nested.myString = "asdf";

// Or
test.nested = {};
Object.entries({myString: "asdf"}).forEach(([key, value]) => this.nested[key] = value);

Version stuff

mongoose: 5.9.21
Node.js: 14.3.0
MongoDB: 4.0.3

@trappar
Copy link
Author

trappar commented Jul 7, 2020

Here's even simpler reproduction code:

import mongoose from "mongoose";

const TestModel = mongoose.model('Test', new mongoose.Schema({
    nested: new mongoose.Schema({
        type: {myArray: [String]},
        default: {}
    })
}));

(async function run() {
    const test = new TestModel();
    
    test.nested = {myArray: ["Test"]}
    await test.save();
    // DB now has: {nested: {myArray: ["test"]}}

    test.nested.myArray = []; // Mongoose cannot detect this change
    await test.save();
    // DB still has: {nested: {myArray: ["test"]}}

    await test.remove();
})().then(() => process.exit());

Why it happens in this case:

  1. nested has a default value
  2. nested is replaced with an object which must be cast to conform to the schema
  3. Since there is at least one key present in the object nested is casting to, Mongoose saves the previous doc
  4. We attempt to reset the value of nested.myArray, but this matches the previous value, so Mongoose does not record it as saved

@trappar trappar changed the title Casting SingleNested causes potential for change tracking to fail Edge case where SingleNested value update is not marked as changed and saving fails to update Jul 7, 2020
@vkarpov15 vkarpov15 added this to the 5.9.24 milestone Jul 10, 2020
@vkarpov15 vkarpov15 added the has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue label Jul 10, 2020
vkarpov15 added a commit that referenced this issue Jul 11, 2020
@vkarpov15
Copy link
Collaborator

Thanks for reporting this issue, this is a bug in Mongoose. priorDoc is only supposed to be there for the process of overwriting the subdoc (test.nested = {myString: "asdf"}) to help with setters. We should just clear it out after we're done with it.

@vkarpov15 vkarpov15 added confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. and removed has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue labels Jul 11, 2020
@trappar
Copy link
Author

trappar commented Jul 15, 2020

Glad to hear it's a simple fix! And glad to help :)

vkarpov15 added a commit that referenced this issue Jul 25, 2020
vkarpov15 added a commit that referenced this issue Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Projects
None yet
Development

No branches or pull requests

2 participants