[GODRIVER-304] update (replace) operations return error that key doesn't start with $ Created: 24/Mar/18  Updated: 10/Sep/18  Resolved: 10/Sep/18

Status: Closed
Project: Go Driver
Component/s: CRUD
Affects Version/s: None
Fix Version/s: None

Type: Bug Priority: Major - P3
Reporter: Sam Kleinman (Inactive) Assignee: Kristofer Brandow (Inactive)
Resolution: Won't Fix Votes: 0
Labels: evg
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
Epic Link: mgo migration tool

 Description   

this might be related to GODRIVER-111, but seems like a different use case.

I tried to use the driver to specify a replacement-style update, and it returns an error, which it should not.



 Comments   
Comment by Sam Kleinman (Inactive) [ 23/May/18 ]

I've put this in the "mgo migration tool" epic, which is, I think, the best place for this to live?

Comment by Sam Kleinman (Inactive) [ 15/May/18 ]

If we do GODRIVER-30, then we'll end up doing this anyway. Eh? 

I see the rationale in not having it, but not having makes supporting migrations from MGO more difficult. While there are technical solutions to work around this (e.g. implementing your own update document that introspects all update specifications looking for a $ operator), not doing this just makes the initial experience with the new driver less intuitive and complicated for people who are committed to using go and want to use the new driver.

These users, and their experience seem like a really important segment for us, and throwing roadblocks in their way in the name of strict compliance with the spec just doesn't feel right, but your call.

Comment by Jeffrey Yemin [ 14/May/18 ]

Drivers have deprecated the old method, so that will have caused some percentage of legacy applications to move to the new API.

As this issue is written up as a bug, I'm inclined to close as Won't Fix.  I also don't think we should consider a new feature that works as you described.  It's out of (CRUD) spec, and I am not inclined to add a method to a new driver that other drivers have long deprecated.

Comment by Sam Kleinman (Inactive) [ 14/May/18 ]

It looks like (python and the shell at least) still provide general "update" operations that don't comply with the spec but nevertheless still work.

I think you're right that I'm asking for a method to ease the transition. Has any driver actually removed the multi-mode update operation? I'm not convinced that anyone has actually migrated legacy code to the crud-spec methods given that the legacy methods were never removed.

 

Comment by Jeffrey Yemin [ 14/May/18 ]

Legacy drivers generally had a single update method that handled every case.  For drivers following the CRUD spec (which the Go driver should), updateOne should reject documents that don't contain update operators.

I think your point is that mgo migrators will have a hard time dealing with the change. But in practice users migrating to the CRUD spec in legacy drivers have not complained about this behavior.

 

Comment by Sam Kleinman (Inactive) [ 14/May/18 ]

I'm asking about the inverse case: does update/update_one error in the replace case for legacy drivers.

it's reasonable to make replace_one error in the set case, as this is a new method added by the CRUD spec that doesn't exist in legacy drivers (like mgo) and doesn't map directly onto a mongodb operation.

Comment by Jeffrey Yemin [ 26/Apr/18 ]

ReplaceOne (there is no ReplaceMany) should only accept an update document that has no $-prefixed keys. In PyMongo this throws:

collection.replace_one({'x': 1}, {'$set': {'y': 1}})

>>> collection.replace_one({'x': 1}, {'$set': {'y': 1}})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python2.7/site-packages/pymongo/collection.py", line 902, in replace_one
    common.validate_ok_for_replace(replacement)
  File "/usr/local/lib/python2.7/site-packages/pymongo/common.py", line 455, in validate_ok_for_replace
    raise ValueError('replacement can not include $ operators')
ValueError: replacement can not include $ operators

Other drivers work the same.

Comment by Sam Kleinman (Inactive) [ 26/Apr/18 ]

I understand that this is the content of the specification, but I'm not sure that its particularly workable. mgo doesn't do this validation (and neither, from what I can tell, does pymongo; the server decides its behavior based on the content of the document as it always has.) This will significantly complicate our adoption of the new API since the error can only appear at runtime.

cc jeff.yemin for context on the decision and the ways in which the drivers implement this specification.

Comment by Kristofer Brandow (Inactive) [ 24/Apr/18 ]

For replace operations, you use the ReplaceOne and ReplaceMany methods when you want to do a replace: https://github.com/mongodb/specifications/blob/master/source/crud/crud.rst#update-vs-replace-validation.

Comment by Sam Kleinman (Inactive) [ 24/Apr/18 ]

I made the following edit to work around:

--- a/vendor/github.com/mongodb/mongo-go-driver/mongo/collection.go
+++ b/vendor/github.com/mongodb/mongo-go-driver/mongo/collection.go
@@ -474,10 +474,6 @@ func (coll *Collection) UpdateOne(ctx context.Context, filter interface{}, updat
                return nil, err
        }
 
-       if err := ensureDollarKey(u); err != nil {
-               return nil, err
-       }
-
        return coll.updateOrReplaceOne(ctx, f, u, options...)
 }
 
@@ -504,10 +500,6 @@ func (coll *Collection) UpdateMany(ctx context.Context, filter interface{}, upda
                return nil, err
        }
 
-       if err = ensureDollarKey(u); err != nil {
-               return nil, err
-       }
-
        updateDocs := []*bson.Document{
                bson.NewDocument(
                        bson.EC.SubDocument("q", f),

The driver enforces that the second argument to the update operation use update operators, when in fact its perfectly legal to pass a document without update operators to the update spec which will then replace the document(s) that match the query.

Comment by Kristofer Brandow (Inactive) [ 26/Mar/18 ]

Hi Sam,

Can you please provide an example of the replacement-style update you are attempting to perform and the resulting error?

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