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

Use findOneAndUpdate for save? #5488

Open
devnopt opened this issue Jul 19, 2017 · 4 comments
Open

Use findOneAndUpdate for save? #5488

devnopt opened this issue Jul 19, 2017 · 4 comments
Labels
backwards-breaking discussion If you have any thoughts or comments on this issue, please share them!
Milestone

Comments

@devnopt
Copy link

devnopt commented Jul 19, 2017

This is a bug in how the save function operates.

The save function assumes that all values are properly updated and returns the same object passed to save (with minor changes such as version update).

Any updated to the object that mongoose doesn't recognize will be affected. For example if you update a date with setMonth (without using markModified), mongoose.js will not recognize it and will claim the object was properly updated on save, when it fact it wasn't.

The desired outcome should be an error returned if the return value from the database does not match the saved object. If returning an error doesn't work, then the returned object should be the one from the database, not the one that was passed to the save function.

Currently using Node v8.0.0, Mongoose v4.10.5, and MongoDB v3.4.1.

@sobafuchs
Copy link
Contributor

i think i know what you mean, but can you post a repro script so I know exactly what you're talking about? Sometimes .save will not capture some of the change detection, but it would be helpful to know what particular example you're talking about

@sobafuchs sobafuchs added needs clarification This issue doesn't have enough information to be actionable. Close after 14 days of inactivity needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue labels Jul 22, 2017
@devnopt
Copy link
Author

devnopt commented Jul 22, 2017

The save routine in mongoose.js doesn't validate the data being returned from MongoDB, so if the developer forgets to use markModified on a date or Mixed Type, the return result incorrectly indicates the data was saved to the database.

ExampleModel
{
myDate: { type: Date, default: Date.now }
}

async function test() {
  let example = new ExampleModel();
  await example.save();

  example = await ExampleModel.findOne().exec();

  // Note no markModified used
  example.myDate.setMonth(example.myDate.getMonth() + 1);

  // Returns example with myDate one month in the future
  example = await example.save();
  
  // Returns example with myDate on the current month
  example = await ExampleModel.findOne().exec(); 
}

@sobafuchs
Copy link
Contributor

this has always been a problem. I don't know if the .save() op should error if the returned value doesn't match the current value, but it is explicitly in the docs to use markModified on mixed types.

@vkarpov15 what do you think is the best way to handle this edge case?

@sobafuchs sobafuchs added discussion If you have any thoughts or comments on this issue, please share them! help wanted and removed needs clarification This issue doesn't have enough information to be actionable. Close after 14 days of inactivity needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue labels Jul 26, 2017
@vkarpov15
Copy link
Collaborator

We've thought about switching to findOneAndUpdate for save rather than just update, but that adds a lot of performance overhead because findOneAndUpdate is slower and because we would have to hydrate a new mongoose doc after every save. Worth investigating more.

Re: your concerns with setMonth, that's question 4 on the faq. http://mongoosejs.com/docs/faq.html

@vkarpov15 vkarpov15 changed the title Save return value inaccurate Use findOneAndUpdate for save? Jul 29, 2017
@vkarpov15 vkarpov15 added this to the 5.0 milestone Jul 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-breaking discussion If you have any thoughts or comments on this issue, please share them!
Projects
None yet
Development

No branches or pull requests

3 participants