[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:
Depends
depends on SERVER-39158 Change updates to prefer target by th... Closed
Related
related to MONGOID-4759 Change updates to prefer target by th... Backlog
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

db.coll.update({shardKey : 100}, {shardKey : 800})

the update will be sent to shard 2 and we return

{ok: 1.0, nMatched : 0)

We should actually get an ImmutableField error.

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:

  • Previously, replacement-style update commands would be routed to shards by the update document (i.e. replacement document).
  • After SERVER-39158, those same update commands will be routed by the filter document.

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.

Generated at Thu Feb 08 08:22:03 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.