Automattic / mongoose

MongoDB object modeling designed to work in an asynchronous environment.
https://mongoosejs.com
MIT License
26.97k stars 3.84k forks source link

Changing an object to the same value twice causes the change to not be saved at all #11461

Closed matanarbel closed 2 years ago

matanarbel commented 2 years ago

Do you want to request a feature or report a bug? bug

What is the current behavior? When changing a nested object twice to the same value, the change is ignored (the field is marked as not modified).

If the current behavior is a bug, please provide the steps to reproduce. Schema:

const testModel = new Schema({
    fakeId: Number,
    foo: {
        bar: { type: Number }
    }
});
module.exports = mongoose.model('TestModel', testModel);

Code segment:

const doc = await TestModel.findOne({ fakeId: 1 });
// doc.foo.bar is currently not 10 (in my test it was 3)
doc.foo = { bar: 10 };
doc.foo = { bar: 10 };
return doc.save();

What is the expected behavior? I expect doc.foo.bar to be set to 10.

Some extra info: Running doc.foo = { bar: 10 }; only once works. Manually running doc.markModified('foo'); after the 2nd update also works. After debugging mongoose code, I believe that the issue is in lib/document.js:1225:

if (priorVal != null && utils.deepEqual(hasInitialVal ? this.$__.savedState[path] : priorVal, val)) {
  this.unmarkModified(path);
} else {
  this.markModified(path);
}

The first time I change foo, utils.deepEqual compares {bar: 3} to {bar: 10}, making the if expression return false, and thus marking foo as modified. The second time I change foo, utils.deepEqual compares {bar: 10} to {bar: 10}, making the if expression return true, and thus unmarking foo as modified.

I'd suggest not unmarking already marked "Modified" fields, to avoid such cases, but since there's a code that explicitly unmarks modified - I guess there're other reasons why it's needed. Maybe a reference to the "original" document (as saved in the DB) should be kept, and the comparison should be against it (this "original" document should be updated only after saving the changes).

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version. Node.js 16.13.0 Mongoose 6.2.3 MongoDB 3.6.6

sean-daley commented 2 years ago

This looks like it might be a dupe of https://github.com/Automattic/mongoose/issues/11395? They seem similar at least.

IslandRhythms commented 2 years ago
const mongoose = require('mongoose');
const {Schema} = mongoose;

const testModel = new Schema({
    fakeId: Number,
    foo: {
        bar: { type: Number }
    }
});

const TestModel = mongoose.model('TestModel', testModel);

run().catch(err => console.log(err));

async function run() {
  await mongoose.connect('mongodb://localhost:27017/test-mongoose', {});
  await mongoose.connection.dropDatabase();

  await TestModel.create({
      fakeId: 1,
      foo: {
          bar: 2
      }
  });

  const entry = await TestModel.findOne();
  console.log(entry);
  entry.foo = {bar: 10};
  console.log(entry);
  entry.foo = {bar: 10};
  console.log(entry);
  await entry.save();
  console.log(entry);

  const otherEntry = await TestModel.findOne();
  otherEntry.foo.bar = 5;
  console.log(otherEntry);
  otherEntry.foo = otherEntry.foo;
  console.log(otherEntry)
}
IslandRhythms commented 2 years ago

@sean-daley They do look similar but I think the key difference causing the problem is either how no entry is created or the way the schema is declared.

sean-daley commented 2 years ago

@sean-daley They do look similar but I think the key difference causing the problem is either how no entry is created or the way the schema is declared.

Cool. I'll definitely trust you guys when it comes to the final say on dupes like that ;)

matanarbel commented 2 years ago
const mongoose = require('mongoose');
const {Schema} = mongoose;

const testModel = new Schema({
    fakeId: Number,
    foo: {
        bar: { type: Number }
    }
});

const TestModel = mongoose.model('TestModel', testModel);

run().catch(err => console.log(err));

async function run() {
  await mongoose.connect('mongodb://localhost:27017/test-mongoose', {});
  await mongoose.connection.dropDatabase();

  await TestModel.create({
      fakeId: 1,
      foo: {
          bar: 2
      }
  });

  const entry = await TestModel.findOne();
  console.log(entry);
  entry.foo = {bar: 10};
  console.log(entry);
  entry.foo = {bar: 10};
  console.log(entry);
  await entry.save();
  console.log(entry);

  const otherEntry = await TestModel.findOne();
  otherEntry.foo.bar = 5;
  console.log(otherEntry);
  otherEntry.foo = otherEntry.foo;
  console.log(otherEntry)
}

You did reproduce the issue, but in the first example, after the save, you checked the object in memory (which is updated). If you'd run findOne after the save, you'd see that the value wasn't actually saved to the DB.

IslandRhythms commented 2 years ago
const mongoose = require('mongoose');
const {Schema} = mongoose;

const testModel = new Schema({
    fakeId: Number,
    foo: {
        bar: { type: Number }
    }
});

const TestModel = mongoose.model('TestModel', testModel);

run().catch(err => console.log(err));

async function run() {
  await mongoose.connect('mongodb://localhost:27017/test-mongoose', {});
  await mongoose.connection.dropDatabase();

  await TestModel.create({
      fakeId: 1,
      foo: {
          bar: 2
      }
  });

  const entry = await TestModel.findOne();
  console.log(entry);
  entry.foo = {bar: 10};
  console.log(entry);
  entry.foo = {bar: 10};
  console.log(entry);
  await entry.save();
  console.log('After save', entry);

  const otherEntry = await TestModel.findOne();
  console.log('new find operation', otherEntry);
  otherEntry.foo.bar = 5;
  console.log(otherEntry);
  otherEntry.foo = otherEntry.foo;
  console.log('No save', otherEntry)
}
vkarpov15 commented 2 years ago

I confirmed this was fixed in v6.2.4 with #11428.