[DRIVERS-660] Change updates to prefer target by the query Created: 22/May/19 Updated: 27/Oct/23 Resolved: 18/Jul/19 |
|
| Status: | Closed |
| Project: | Drivers |
| Component/s: | None |
| Fix Version/s: | None |
| Type: | Task | Priority: | Major - P3 |
| Reporter: | Arun Banala | Assignee: | Unassigned |
| Resolution: | Gone away | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||
| Server Compat: | 4.1 | ||||||||||||||||
| Description |
|
Currently, replacement style updates target using the update rather than the query. We should change this to target using the query instead. As of today, we do not allow users to change the shard key for a document. If shard 1 owns the chunk with range (shardKey : min, shardKey : 500) and shard 2 owns the chunk with range (shardKey : 500, shardKey : max) and a user sends
Once we allow a user to update the shard key for a document (PM-1163), the above update should cause the document to change shards. However, since the update would be sent to shard 2, shard 1 will never actually see this update and the update will not be applied. |
| Comments |
| Comment by Jeremy Mikola [ 30/May/19 ] |
|
Previous comments on this issue seem preoccupied with the save() CRUD method. I don't think that's the right thing to be focus on. While it's true that the deprecated save() CRUD method may do a replacement-style update with upsert: true if the _id is known at runtime (see: save flowchart), this more generally applies to any replaceOne operation. The behavioral change in the OP is simply:
The scenario described in the OP demonstrates a case where an update could return successfully and not match anything, when the desired behavior would be to raise an ImmutableField error. While that seems harmless, if we revise that scenario to include an upsert: true option on the update command then we risk inserting a new document instead of raising an error. AFAICT, the relevant audit for ODMs and any other libraries would be any case where a replaceOne (with or without an upsert) might be executed internally. We can probably forego any query builders that allow users/applications to craft their own replacements, as that's no different than a user calling replaceOne() directly through a driver. |
| Comment by Bernie Hackett [ 29/May/19 ] |
|
oleg.pudeyev, a user of a driver can (and should, years ago) stop using the deprecated save() method. If the ODM they are using does a save() internally there is no way a user can avoid it. |
| Comment by Jeffrey Yemin [ 29/May/19 ] |
|
I don't think Morphia has the sort of behavior that you seem to be describing. All updates are done explicitly (either with a save or an update method call). See https://morphia.dev/1.5/getting-started/quick-tour/#saving-data. |
| Comment by Oleg Pudeyev (Inactive) [ 29/May/19 ] |
|
I understand that. What confuses me is why ODMs are highlighted in the present discussion because it is equally possible to do those kinds of updates via drivers and my impression is they would result in incorrect behavior (lost updates) as well. |
| Comment by Bernie Hackett [ 29/May/19 ] |
|
The issue isn't the ODM providing a save() method. The problem is ODMs essentially doing a save() internally to update model documents. |
| Comment by Oleg Pudeyev (Inactive) [ 29/May/19 ] |
|
I will investigate whether Mongoid is affected after https://jira.mongodb.org/browse/HELP-10000 unless priorities should be different. |
| Comment by Jeffrey Yemin [ 28/May/19 ] |
|
Morphia does have a save method IIRC, so best we can do is to open a ticket to request that it be deprecated. |
| Comment by Bernie Hackett [ 28/May/19 ] |
|
Moving this back to triage. shane.harvey pointed out that this will break PyMODM. He also thinks this might affect Morphia (jeff.yemin?). oleg.pudeyev, will this break Mongoid? |
| Comment by Ian Whalen (Inactive) [ 28/May/19 ] |
|
all drivers have already deprecated save() so no need to do anything here. |