Mongoose: isModified of a field is set to `true` even if value hasn't actually changed

[adamreisnz]

Do you want to request a feature or report a bug?
Depends on whether this is expected behaviour or not. Possibly related to https://github.com/Automattic/mongoose/issues/4487

Repro script

const mongoose = require('mongoose');
const {Schema} = mongoose;
const uri = `mongodb://localhost:27017/test`;

//Schema
const FooSchema = new Schema({
  bar: {
    type: String,
  },
});

//Pre-save hook 1
FooSchema.pre('save', function(next) {
  console.log('HOOK 1: BAR MODIFIED:', this.isModified('bar'), this.bar);
  this.bar = 'bar';
  next();
});

//Pre-save hook 1
FooSchema.pre('save', function(next) {
  console.log('HOOK 2: BAR MODIFIED:', this.isModified('bar'), this.bar);
  next();
});

//Model
const Foo = mongoose.model('Foo', FooSchema);

//Test function
async function test() {

  //Connect to database
  await mongoose.connect(uri);

  //Create new item
  const foo = new Foo({bar: 'bar'});
  await foo.save();

  foo.bar = 'notbar';
  await foo.save();
}

test();

What is the current behavior?
At the second save of the document, mongoose says bar is modified in the second hook, even though the first hook restored the value to what it was pre-save. So while the value was modified, it has not actually changed.

Output:

HOOK 1: BAR MODIFIED: true bar
HOOK 2: BAR MODIFIED: true bar
HOOK 1: BAR MODIFIED: true notbar
HOOK 2: BAR MODIFIED: true bar

What is the expected behavior?
Expect the following output:

HOOK 1: BAR MODIFIED: true bar
HOOK 2: BAR MODIFIED: true bar
HOOK 1: BAR MODIFIED: true notbar
HOOK 2: BAR MODIFIED: false bar <-- expect this to be false because between saved versions, bar has not changed

I realise this may not be how Mongoose change tracking was set to work, but for our use case where we try to track fields that have changed for audit purposes it is annoying that some fields always get reported as changed, even though they are not. One example is where we parse and clean up the mobile number format which comes from the client. The client will always send this "formatted" for the local country, e.g. a number like "022 123 123". However, the server will parse this and store it depending on the country code, e.g. "[phone]". This value does not change, but because we "set" it to "022 123 123" mongoose will always report it as being modified, even though the subsequent hook will restore it to the same value it was before.

It'd be great if the isModified value could revert back to false if Mongoose detects that since the last change, the value has been restored to what it was.

Another example that doesn't use hooks:

  const foo = new Foo({bar: 'bar'});
  await foo.save();
  console.log(foo.bar); //bar
  console.log(foo.isModified('bar')); //false

  foo.bar = 'baz';
  console.log(foo.isModified('bar')); //true, expecting true
  foo.bar = 'bar';
  console.log(foo.isModified('bar')); //true, expecting false

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.
mongoose 5.9.20
mongodb 4.2.5
node 12.16.2

[samtsai15]

I think this can help you

// Pre-save hook 1
FooSchema.pre('save', function (next) {
  if (this.isNew) {
    console.log('HOOK 1: BAR MODIFIED (isNew): ', this.isModified('bar'), this.bar);
  } else {
    console.log('HOOK 1: BAR MODIFIED:', this.isModified('bar'), this.bar);

    this.bar = 'bar';
  }

  next();
});

// Pre-save hook 2
FooSchema.pre('save', function (next) {
  if (this.isNew) {
    console.log('HOOK 2: BAR MODIFIED (isNew): ', this.isModified('bar'), this.bar);
  } else {
    console.log('HOOK 2: BAR MODIFIED:', this.$locals.preBar !== this.bar, this.bar);
  }
  next();
});


// Model
const Foo = mongoose.model('Foo', FooSchema);

// Test function
async function test() {
  // Connect to database
  await mongoose.connect(uri);

  // Create new item
  const foo = new Foo({ bar: 'bar' });
  await foo.save();
  foo.$locals.preBar = foo.bar;
  foo.bar = 'notbar';
  await foo.save();
}

Result

HOOK 1: BAR MODIFIED (isNew):  true bar
HOOK 2: BAR MODIFIED (isNew):  true bar
HOOK 1: BAR MODIFIED: true notbar
HOOK 2: BAR MODIFIED: false bar

[adamreisnz]

Thanks @samtsai15, what is this.$locals.preBar ? Is that something that is documented, or am I supposed to set the value of each field in this.$locals manually?

[vkarpov15]

@adamreisnz See Document#$locals docs. $locals is an empty object used primarily to store data for document middleware in a way that is guaranteed to not conflict with Mongoose internals.

[vkarpov15]

@adamreisnz I added a fix for this in the 5.11 branch. This change is a bit too heavy for a patch release since it affects all set() and all save() calls. We're planning on shipping 5.11 in November 2020.

[adamreisnz]

Thanks, I'll keep an eye out!

[vkarpov15]

One of my concerns with this was perf implications: deepEqual() has historically been pretty slow in Mongoose. But with a couple of small changes I've been able to speed up the below script by 30x (30 seconds to sub 1 second on my macbook):

'use strict';

const mongoose = require('mongoose');

const { deepEqual } = require('mongoose/lib/utils');

mongoose.set('debug', true);

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

async function run() {
  await mongoose.connect('mongodb://localhost:27017/test', {
    useNewUrlParser: true,
    useUnifiedTopology: true
  });

  await mongoose.connection.dropDatabase();

  const Model = mongoose.model('Test', mongoose.Schema({ _id: false, docArr: [{ _id: false, test: String }] }));

  const doc1 = new Model();
  const doc2 = new Model();

    for (let i = 0; i < 10000; ++i) {
    doc1.docArr.push({ test: 'abc' });
    doc2.docArr.push({ test: 'abc' });
  }

  const assert = require('assert');
  console.log('Start compare');

  const start = Date.now();
  for (let i = 0; i < 100; ++i) {
    assert.ok(deepEqual(doc1, doc2));
  }
  console.log('Elapsed', Date.now() - start);
}

So this change should be much less heavy.

I opened a related issue that should make deepEqual() faster going forward: https://github.com/Automattic/mongoose/issues/9571