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

Unmodified fields being marked as modified. #11913

Closed
2 tasks done
jjacksonjc opened this issue Jun 8, 2022 · 4 comments
Closed
2 tasks done

Unmodified fields being marked as modified. #11913

jjacksonjc opened this issue Jun 8, 2022 · 4 comments
Labels
docs This issue is due to a mistake or omission in the mongoosejs.com documentation
Milestone

Comments

@jjacksonjc
Copy link

jjacksonjc commented Jun 8, 2022

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Mongoose version

6.3.5

Node.js version

16.x

MongoDB server version

5.x

Description

When a child field is modified, all children sharing a common parent are getting marked as modified, whether or not they were actually modified. This is a deviation from mongoose v5 and lower behavior and seems to be unintentional since it doesn't appear that any unit tests are checking for this case.

Steps to Reproduce

With mongo listening on 27017 run the following script and observe the output.

'use strict';

const mongoose = require('./lib');

const person = new mongoose.Schema({
  name: {
    first: String,
    last: String
  },
  haircolor: String,
  eyecolor: String
});

const Person = mongoose.model('Person', person);

(async function() {
  await mongoose.connect('mongodb://localhost:27017/example');

  const h = {
    name: {
      first: 'bob',
      last: 'jones'
    },
    haircolor: 'brown',
    eyecolor: 'brown'
  };

  const human = new Person(h);

  await human.save();

  h.name.first = 'Larry';
  human.set(h);

  console.log(`expect 'name,name.first': ${human.modifiedPaths({ includeChildren: true })}`);
  console.log(`expect 'false': ${human.isModified('haircolor')}`);
  console.log(`expect 'false': ${human.isModified('eyecolor')}`);
  console.log(`expect 'true': ${human.isModified('name.first')}`);
  console.log(`expect 'false': ${human.isModified('name.last')}`);

  await mongoose.disconnect();
})();

Expected Behavior

In the above example name.last should not be marked as modified.

@IslandRhythms IslandRhythms added the confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. label Jun 9, 2022
@jjacksonjc
Copy link
Author

Here's another repro case. Looks like this behavior is occurring when trying to set via an object. Using the args it's fine.

'use strict';

const mongoose = require('./lib');

const person = new mongoose.Schema({
  name: {
    first: String,
    last: String
  },
  haircolor: String,
  eyecolor: String
});

const Person = mongoose.model('Person', person);

(async function() {
  await mongoose.connect('mongodb://localhost:27017/example');

  const bob = 'bob';

  const h = {
    name: {
      first: bob,
      last: 'jones'
    },
    haircolor: 'brown',
    eyecolor: 'brown'
  };

  const human = new Person(h);

  await human.save();

  human.set('name.first', bob);
  console.log(`human.isModified via args: ${human.isModified('name.first')}`);

  human.set({ name: { first: bob } });
  console.log(`human.isModified via obj: ${human.isModified('name.first')}`);

  await mongoose.disconnect();
})();

@vkarpov15 vkarpov15 added this to the 6.3.8 milestone Jun 13, 2022
@jjacksonjc
Copy link
Author

@vkarpov15 It looks like 6.3.8 was released a couple hours ago, but 6.3.7 was skipped in releases though the milestone was closed, so maybe either this isn't the correct milestone, or their was an error in the version numbering?

@vkarpov15
Copy link
Collaborator

@jjacksonjc we had to release 6.3.8 shortly after 6.3.7, because we accidentally shipped 6.3.7 with failing tests.

In Mongoose, this is exopected behavior, because isModified() returns true if Mongoose will need to update this value in MongoDB, not necessarily if the value has changed. And set() on a nested path modifies the whole path by default, unless the value you're setting is deep equal to the current value.

So human.set({ name: { first: bob } }); means name is modified, because you're unsetting name.last. And, when you do human.set(h); in your original example, you're modifying name because { first: 'bob', last: 'jones' } is not deep equal to { first: 'Larry', last: 'jones' }.

The workaround is to set the merge option on set(), which merges rather than overwriting nested paths. The below script gives you the expected result:

'use strict';

const mongoose = require('mongoose');

const person = new mongoose.Schema({
  name: {
    first: String,
    last: String
  },
  haircolor: String,
  eyecolor: String
});

const Person = mongoose.model('Person', person);

(async function() {
  await mongoose.connect('mongodb://localhost:27017/example');

  const h = {
    name: {
      first: 'bob',
      last: 'jones'
    },
    haircolor: 'brown',
    eyecolor: 'brown'
  };

  const human = new Person(h);
  await human.save();

  h.name.first = 'Larry';
  human.set(h, null, null, { merge: true });

    console.log(`expect 'name,name.first': ${human.modifiedPaths({ includeChildren: true })}`);
  console.log(`expect 'false': ${human.isModified('haircolor')}`);
  console.log(`expect 'false': ${human.isModified('eyecolor')}`);
  console.log(`expect 'true': ${human.isModified('name.first')}`);
  console.log(`expect 'false': ${human.isModified('name.last')}`);

  await mongoose.disconnect();
})();

Does that work for you?

@vkarpov15 vkarpov15 added help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary and removed confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. labels Jun 17, 2022
@vkarpov15 vkarpov15 removed this from the 6.3.9 milestone Jun 17, 2022
@jjacksonjc
Copy link
Author

jjacksonjc commented Jun 17, 2022

That did work, but it brings up the question: where are the available options documented? A docs site search for merge kind of makes mention about the new expected behavior you're talking about, though it doesn't say how to get that old behavior. It's not listed as an option to Document.prototype.set(). https://mongoosejs.com/docs/search.html?q=merge

@vkarpov15 vkarpov15 removed the help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary label Jun 17, 2022
@vkarpov15 vkarpov15 added this to the 6.4.1 milestone Jun 17, 2022
@vkarpov15 vkarpov15 added the docs This issue is due to a mistake or omission in the mongoosejs.com documentation label Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This issue is due to a mistake or omission in the mongoosejs.com documentation
Projects
None yet
Development

No branches or pull requests

3 participants