[SERVER-12266] Update no longer allows empty modifier objects Created: 06/Jan/14 Updated: 02/Sep/15 Resolved: 22/Jan/14 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Write Ops |
| Affects Version/s: | 2.5.4 |
| Fix Version/s: | None |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Jeremy Mikola | Assignee: | Scott Hernandez (Inactive) |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||||||||||
| Operating System: | ALL | ||||||||||||||||||||||||||||
| Participants: | |||||||||||||||||||||||||||||
| Description |
|
tl;dr: There is no consistent way to upsert an identifier-only document between 2.4.x and 2.5.x. Allowing modifiers in a newObj to be an empty BSON object would help. I noticed the following while testing Doctrine ODM against 2.5.x. In 2.4.x and earlier versions, I believe there was only a single way to upsert a document that only contained an identifier field:
Using an empty object for the newObj argument results in the upsert ignoring the client-provided _id. $set cannot be used on _id, even if that would technically be OK for an upsert. $setOnInsert would make more sense, but it also doesn't work – it's also 2.4+ only, so I wouldn't rely on it for essential ODM logic. In 2.5.x, the one working method from 2.4.x no longer works. The two methods that didn't work in 2.4.x do work in 2.5.x:
The strict validation that makes the 2.4.x solution no longer work looks to have been introduced in this commit for |
| Comments |
| Comment by Glenn Maynard [ 02/Sep/15 ] | ||||||||||||||||||||||||
|
This is a huge, breaking change and I'm disappointed that it's been ignored. This broken our code in dozens of places and I've had to insert a shim to work around it. This change was harmful, breaking important functionality and making Mongo's update language brittle and illogical. We've stuck with 2.4 for a long time because of this problem. All of our code uses find_and_modify as a single, consistent way to handle all basic queries, including finds, updates, find + update, and cases where we may need to do any combination depending on flags. To do a find, we just do {$set: {}}. We fill in the update dynamically if there are changes to be made, and if there aren't we still run the query in order to retrieve the current document. All of these are obvious uses of findAndModify that this change broke. For other people bit by this, here's a partial workaround for Python. This is against pymongo.Collection; we have no plans to change APIs for software in maintenance mode. It also only patches find_and_modify, since we never use update() directly.
| ||||||||||||||||||||||||
| Comment by Hrachya Mnatsakanyan [ 29/Nov/14 ] | ||||||||||||||||||||||||
|
I think this restriction is not very important and it would be better to revert this change as I have to use version 2.4.12 instead of the latest to avoid this restriction as I would have to do more changes. | ||||||||||||||||||||||||
| Comment by Jacob Heller [X] [ 18/Jul/14 ] | ||||||||||||||||||||||||
|
Ah I did not realize that pymongo.Connection had been replaced. I will replace that right away, but I still think $set and $unset should allow empty arguments. Thanks! | ||||||||||||||||||||||||
| Comment by Bernie Hackett [ 18/Jul/14 ] | ||||||||||||||||||||||||
|
Please make sure you are using pymongo.MongoClient, not pymongo.Connection which has been deprecated since PyMongo 2.4 and is being removed in PyMongo 3.0. With MongoClient there is no need to pass w=1 in the update statement:
Also, make sure your application isn't globally setting w: 0 as its default write concern. | ||||||||||||||||||||||||
| Comment by Jacob Heller [X] [ 18/Jul/14 ] | ||||||||||||||||||||||||
|
I'm using Pymogo (version 2.7.1). The issue arises when I'm using $set and $unset at the same time. If write (in Python): , {'$set': {'setThisField':123}, '$unset':{}}) Then the function returns nothing. There is no indication that setting setThisField failed because the argument to $unset was emtpy. This is what I mean by data being mysteriously lost. Indeed, if I specify "w=1" as an argument to update, then it raises an exception indicating that the save failed, but any code that is designed for older versions of Mongo are potentially going to fall into this pitfall of failing silently. As Glenn Maynard suggested, this is a good place to create a warning, but you shouldn't lose data that you're trying to $set because $unset was empty. | ||||||||||||||||||||||||
| Comment by Bernie Hackett [ 18/Jul/14 ] | ||||||||||||||||||||||||
The default write concern of {w: 1}acknowledges all write operations and reports any errors. This has been the default ever since MongoClient was introduced in PyMongo quite a while back. Are you saying that the server is not reporting errors back to the client in certain cases? | ||||||||||||||||||||||||
| Comment by Jacob Heller [X] [ 18/Jul/14 ] | ||||||||||||||||||||||||
|
I would also like to chime in about the harm that this change has made. +1 to Glenn Maynard's explanation above, which explains a code flow that appears all over my code. Furthermore, with the default write concern the way it is (which I think is new), update exceptions are not explicitly reported back to the system. As a result, data can be mysteriously lost for no apparent reason, which is what I've discovered using the PyMongo drivers. This is dangerous. Please revert this change. | ||||||||||||||||||||||||
| Comment by Glenn Maynard [ 20/Jun/14 ] | ||||||||||||||||||||||||
|
Rejecting empty modifiers is inconsistent, introduces bugs, and creates busywork for users to work around this strange behavior, as evidenced above as people struggle to find workarounds. > I spoke to Scott Hernandez about this today, and he explained the new strictness around empty modifiers is intended to alert users that were inadvertently sending over empty updates. This is a reason to add a warning that you can turn off, not for making it an error. Making it an error is forcing premature optimization on users. This has broken a common pattern that I use all the time in my code: sets = {} This regularly sends empty modifiers, which is intentional and harmless. Sometimes it sends nothing but empty modifiers, which is also intentional, because the update is often a findAndModify where the result is wanted even if nothing has changed. This change is harmful and narrow-sighted. Please revert it. | ||||||||||||||||||||||||
| Comment by Valeri Karpov [ 06/Jun/14 ] | ||||||||||||||||||||||||
|
Great suggestion Nathan, I think I'll use that. Guess I can work around this by including $unset with a reserved keyword. | ||||||||||||||||||||||||
| Comment by Thomas Boutell [ 19/May/14 ] | ||||||||||||||||||||||||
|
This is so MySQL (: I used to appreciate Mongo's willingness not to give me a hard time for asking to fetch any of zero things, or to update nothing. Mongo can optimize that away in one place, why make a thousand query builders do it? | ||||||||||||||||||||||||
| Comment by Nathan Acuff [ 14/May/14 ] | ||||||||||||||||||||||||
|
We have also encountered this issue, and our 'workaround' was to include $unset: _this_field_does_not_exist_ever_ if the $set command is empty. Very annoying behavior change, especially coupled with the changes in the way $set works with _id. | ||||||||||||||||||||||||
| Comment by Valeri Karpov [ 07/May/14 ] | ||||||||||||||||||||||||
|
My conclusions from having wrangled with this for the last couple days in the context of findAndModify: Scott's proposed solution would clobber any existing document, so wouldn't work. The idea of copying the query parameters into `$set` is great, except for 2.2 and 2.4 will throw an error if `$set` attempts to set `_id`. This puts you between a rock and a hard place if you want to support 2.2, 2.4, and 2.6. What mongoose will probably do for now is copy query parameters minus `_id` into `$set`, which will work as expected on 2.2 and 2.4, but leave an unfortunate corner case on 2.6 where if the query parameter to findAndModify only has `_id` and there is nothing to update the findAndModify will fail. If this particular server behavior isn't going to change in the future (although IMO it absolutely should) I can work it into mongoose's API in the next major release. | ||||||||||||||||||||||||
| Comment by Jeremy Mikola [ 22/Jan/14 ] | ||||||||||||||||||||||||
|
I spoke to scotthernandez about this today, and he explained the new strictness around empty modifiers is intended to alert users that were inadvertently sending over empty updates. We discussed the server possibly having some sort of merge functionality (like save, but setting fields instead of overwriting the entire document) down the line, but for now, I will have to attempt a pre-2.6 or 2.6+ update and then fall back on error (unless I can detect the server version up front). For 2.6+, "{$set: {_id:3}}" will be the preferred method. | ||||||||||||||||||||||||
| Comment by Jeremy Mikola [ 10/Jan/14 ] | ||||||||||||||||||||||||
|
This is the conditional in question: https://github.com/mongodb/mongo/blob/8b57f45b149b9df8ed2e9d7f74bf5d33a650cd5c/src/mongo/db/ops/update_driver.cpp#L115 I also spoke with justin.lee@10gen.com yesterday about Morphia and he said they use the second example ("{}" as the newObj argument), which actually disregards any custom identifier before 2.5.x. But since Morphia also tends to store the class name in another field by default, most users would never have such empty updates (with only _id). | ||||||||||||||||||||||||
| Comment by Jeremy Mikola [ 09/Jan/14 ] | ||||||||||||||||||||||||
|
I looked into that, but it opens the possibility of data loss. If a developer creates a document object, assigns their own identifier, and persists/flushes, our ODM opts to do an upsert. If the developer didn't set a specific identifier, the ODM will generate one upon persisting and opt to perform an insert upon flush. When a document's changeset contains only the identifier, we basically want "insert if not exists". That's essentially what the NOOP newObj argument accomplished in the issue description. Other discussions (here and here) suggest performing an insert and ignoring a possible unique index exception. Some additional context: https://github.com/doctrine/mongodb-odm/pull/750/files | ||||||||||||||||||||||||
| Comment by Scott Hernandez (Inactive) [ 08/Jan/14 ] | ||||||||||||||||||||||||
|
You can use save/update with the doc as the query + update (w/upsert true).
|