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

Saving document with projected sub document modifies the wrong sub document #1334

Closed
mseegers opened this issue Feb 5, 2013 · 15 comments
Closed
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. docs This issue is due to a mistake or omission in the mongoosejs.com documentation

Comments

@mseegers
Copy link

mseegers commented Feb 5, 2013

Hi,

when retrieving a document using findOne while limiting the result of an array of sub documents via an $elemMatch projection, the wrong array element will get modified in the database when saving the document back.

Please see the following test case:

https://gist.github.com/4711139

Which produced the following debug output:

Mongoose: docs.findOne({ someId: 1 }) { fields: { subDocs: { '$elemMatch': { someValue: 3 } } }, safe: undefined }
Mongoose: docs.update({ _id: ObjectId("511054f985a8270000000002") }) { '$set': { 'subDocs.0.someValue': 4 } } {}

Notice up the update command sets 'subDocs.0.someValue' instead of 'subDocs.2.someValue'. I assume this is because an array with only a single element was returned due to the projection.

We have been relying on mongoose's ability to selectively update parts of large documents including many subdocuments depending on what data had been modified. Without the ability to use projection, the document retrieval kills our performance as node will spend a bulk of its time parsing the bson data. See also the following issue for a similar issue that we've encountered:

#1303

Thanks!

This is using mongoose 3.5.4

@aheckmann
Copy link
Collaborator

Mongoose is not a good fit for this use case because it was designed such that document.save() result in a single atomic write to the database, either all changes are commited or nothing is when an error occurs. To facilitate this, mongoose uses positional notation (subDocs.0.someValue) to handle multiple subdocument updates. MongoDB currently does not provide any other syntax to update more than one array element at a time. This has been a solid solution until recently when MongoDB added $elemMatch in a projection which ruins knowledge of element positions. More generally, nobodys code can rely on positional notation in updates based on results returned from projections which include $elemMatch.

Work-around:

  • When using $elemMatch in a projection, do not use document.save(). Instead, manually update your document using Model.update().

You may also consider using lean() in combination with $elemMatch projections since lean returns plain objects (not Mongoose documents) which do not have a save() method at all, further forcing you to use Model.update manually.

@aheckmann
Copy link
Collaborator

I'm thinking returning an error to the save callback when this is attempted is the "right thing" in this circumstance.

@wclr
Copy link

wclr commented Mar 17, 2013

Are you going to redesign something in mongoose in future to fit more such cases? Mongoose is great but sometimes it forces to query exessive data from db. (

@aheckmann
Copy link
Collaborator

Not in the foreseeable future. If mongodb adds some new feature we'll revisit.

@mephju
Copy link

mephju commented Nov 18, 2014

It should also be noted that this doesn't work for queries where only the matching subdocument is returned.

    User.findOne(
        {'orders._id': orderId},
        {'orders.$': 1},
        done
    );

Changes made to user.orders[0] will be saved to the very first subdocument within user.
No error is returned here, unfortunately.

@vkarpov15
Copy link
Collaborator

Related to #2031

@geloescht
Copy link
Contributor

This might actually be doable using $(update) since version 2.6 of MongoDB: https://docs.mongodb.org/v2.6/reference/operator/update/positional/
We'd have to pull out the projection clause (for $elemMatch) or query criterion (for $) from the original query and copy it to our update filter. Then, instead of using "array.0.element" to update the array we need to use "array.$.element". This works only with projection operators that select a single array element (thus, not with $slice) and if no new elements were added to the array.
I admit, it sounds quite complicated and fragile, but do you see any chance that this is ever going to be implemented @vkarpov15 ? I had a look at the code generating queries for Document.save() myself, but do not feel confident enough to touch it.

@vkarpov15
Copy link
Collaborator

@geloescht can you provide a code sample that demonstrates what you're looking to do?

@geloescht
Copy link
Contributor

I am developing a solution for content localization. Basically, instead of modifying a translatable document property directly, I use a virtual that modifies an entry inside an array with localized data. To save memory and computing power for documents with a lot of translations I use projection to only retrieve localized data for a single language. This is all wired up using mongoose middleware.
Basically it boils down to:

apps.findOne(
{
  _id: ObjectId("567452bae5b25d6e6c1a0f7e")
},
{
  localization: { '$elemMatch': { language: 'de' } }, dummy_: 0
}).exec(function(err, app)
{
  //a real gentleman woud handle this error
  app.localization[0].name =  "Neuer Name";
  app.save(); //throws DivergentArrayError
  //this is the query that saves the way I want
  apps.findOneAndUpdate(
  {
    _id: ObjectId("567452bae5b25d6e6c1a0f7e"),
    localization: { '$elemMatch': { language: 'de' } }
  },
  {
    $set: { 'localization.$.name' : "Neuer Name" }
  }).exec(//...
});

@vkarpov15 vkarpov15 added this to the 4.6 milestone Feb 19, 2016
@vkarpov15
Copy link
Collaborator

Yeah supporting that behavior in app.save() would be tricky. Will investigate what it would take to support this behavior for a future release. In the meantime, your findOneAndUdate() solution works well.

@vkarpov15 vkarpov15 reopened this Feb 19, 2016
@geloescht
Copy link
Contributor

Thanks for looking into this!

@filipetedim
Copy link

@geloescht a thousand thanks to you. I've spent the last 2 hours trying to code a workaround, then 1 more trying to find a solution until I stumbled upon yours, great work! I guess besides the '.$' there is still no other support for elemMatch and saving?

@geloescht
Copy link
Contributor

@filipetedim My PR adds support for saving fields selected via $elemMatch but it is not merged into Mongoose yet. In the meantime you need to use update and '$.' or maintain your own fork with my patch.

@filipetedim
Copy link

@geloescht Yeah I'm using the '$.' for the moment - tbh I thought that was your code. So how will I call your function to use it? Or will I just hit "save()" and it will work? Again, nice work finding the $. solution and implementing a new feature on the PR.

@vkarpov15 vkarpov15 modified the milestones: 4.9, 4.8 Jan 2, 2017
@vkarpov15 vkarpov15 modified the milestones: 4.9, 4.10 Feb 14, 2017
@vkarpov15
Copy link
Collaborator

Yeah upon closer inspection OP's issue was fixed in 4.5.0 with #4053

@vkarpov15 vkarpov15 removed this from the 4.9 milestone Feb 27, 2017
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. docs This issue is due to a mistake or omission in the mongoosejs.com documentation
Projects
None yet
Development

No branches or pull requests

7 participants