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

Possible bug in array assignment to itself #7472

Closed
unusualbob opened this issue Jan 31, 2019 · 0 comments
Closed

Possible bug in array assignment to itself #7472

unusualbob opened this issue Jan 31, 2019 · 0 comments
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Milestone

Comments

@unusualbob
Copy link
Contributor

Bug

What is the current behavior?

If a subdocument array is assigned to itself, the update history before that point appears to be lost.

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

  • Perform operations on subdocument array
  • Assign the sub-array to itself
  • Perform another subdocument operation
  • Only last operation is applied on .save()
const mongoose = require('mongoose');
mongoose.connect('mongodb://localhost/testing-db', {useNewUrlParser: true});

const Dog = mongoose.model('Dog', new mongoose.Schema({
  name: String,
  toys: [{
    name: String
  }]
}));


async function main() {

  const dog = new Dog({ name: 'Dash' });

  dog.toys.push({ name: '1' });
  dog.toys.push({ name: '2' });
  dog.toys.push({ name: '3' });

  console.log('added toys', dog.toys.map(({name}) => name));
  // Emits  [ '1', '2', '3' ] on all versions

  await dog.save();

  // Removes element with name '1'
  dog.toys = dog.toys.filter((toy) => {
    return toy.name !== '1';
  });

  console.log('removed one toy', dog.toys.map(({name}) => name));
  // Emits ['2', '3'] on all versions

  ['4', '5', '6'].forEach((toyName) => {
    let giveToy = !dog.toys.some((toy) => {
      return toy.name === toyName;
    });
    if (giveToy) {
      // Commenting out this assignment line fixes everything
      dog.toys = dog.toys || [];
      dog.toys.push({
        name: toyName,
        count: 1
      });
    }
  });

  console.log('memory', dog.toys.map(({name}) => name));
  // Emits ['2', '3', '4', '5', '6' ] on all versions

  await dog.save();

  let foundDog = await Dog.findOne({_id: dog._id });
  console.log('db version', foundDog.toys.map(({name}) => name));
  // Emits [ '1', '2', '3', '6' ] on >=4.6
  // Emits ['2', '3', '4', '5', '6' ] on <=4.5

  mongoose.connection.close();
}

main().catch((e) => {
  console.log(e);
});

What is the expected behavior?

There is one line in this example that breaks everything, specifically this line in our loop:
dog.toys = dog.toys || [];
It looks innocent enough, but somehow the mongoose getter/setter logic here sees this as a reason to reset the modification history on that array.

This means as far as mongoose is concerned, no operations took place before that assignment happens, which means it does not attempt to make those modifications to the array in the db, and instead writes the only op it knows about, the last push which happens to be '6'.

It seems to me that assigning an object to itself should not delete it's update history, while that specific assignment statement isn't actually necessary here, it may catch some users by surprise. We found this when upgrading to 5.x and in our usage the subdocument array was added to the schema some time after the original model, so the developer had wanted to make sure the array existed before pushing elements to it. Since subdocument arrays should be initialized these days we don't need to do that, but it broke some things for a while and took a while to track down. Not sure if there's a clean way to detect this sort of assignment or not. Would it make sense to warn about this in some way?

Please mention your node.js, mongoose and MongoDB version.
node 8, mongoose 5.4.8, mongo 3.0

@vkarpov15 vkarpov15 modified the milestones: 5.4.x, 5.4.10 Feb 4, 2019
@vkarpov15 vkarpov15 added needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. and removed needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue labels Feb 4, 2019
vkarpov15 added a commit that referenced this issue Feb 5, 2019
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