[DRIVERS-1359] Support empty update document when upsert:true Created: 05/Aug/20 Updated: 27/Oct/23 Resolved: 18/Aug/20 |
|
| Status: | Closed |
| Project: | Drivers |
| Component/s: | None |
| Fix Version/s: | None |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Matt Broadstone | Assignee: | Jeremy Mikola |
| Resolution: | Works as Designed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Description |
|
Recent changes in the Node.js driver to ensure the CRUD spec requirement that all update documents MUST contain atomic operators has exposed a limitation in the spec previously supported in the driver: we provide no spec-compliant way to upsert an empty document. The specific language indicates:
However, the following query results in a server error:
I suggest that we loosen the language for the requirement of atomic operators to consider whether upsert: true has been provided. |
| Comments |
| Comment by Valeri Karpov [ 18/Aug/20 ] | ||||||||||||||||||||||||||||||
|
Ok, thanks for clarifying Jeremy. I wasn't aware that the server can't tell the difference between `updateOne` and `replaceOne` - that's a good reason for this change. I will use `updateOne(filter, { $setOnInsert: filter })` instead. This issue is closed as far as I'm concerned | ||||||||||||||||||||||||||||||
| Comment by Jeremy Mikola [ 18/Aug/20 ] | ||||||||||||||||||||||||||||||
Indeed. MongoDB 2.6 introduced insert, update, and delete commands to supersede the original wire protocol messages (e.g. OP_INSERT). The CRUD spec is what created abstractions for updateOne, updateMany, and replaceOne on top of the update command.
In that case, I think the best option may be using $setOnInsert where the client-generated id field is specified in both the update filter _and $setOnInsert. This is what I suggested in the first have of this comment. Let me know if that would work for you. While you could also invoke the update directly, I think it's best to avoid that to ensure you get the common update code path where the driver can do things like inherit write concerns and apply retryable write logic. findAndModify might also be an option if Node provides a method for the basic command (with retryable write and WC inheritance) and not just the findAndModify-derived CRUD methods (e.g. findOneAndUpdate, findOneAndReplace), which would have the same restriction as the OP. | ||||||||||||||||||||||||||||||
| Comment by Valeri Karpov [ 18/Aug/20 ] | ||||||||||||||||||||||||||||||
|
"is it reasonable for Mongoose to utilize replaceOne when it must perform an upsert": No. Because `replaceOne()` will wipe out all other fields in the document if there is a document that matches `filter`.. The primary use case here is to upsert a document if nothing matches `filter`, or leave the document unmodified if it doesn't. Re: "I was purposefully using the shell's db.collection.update() method as a thin abstraction for the update command", does that mean that `updateOne` and `replaceOne` both use the `update` command under the hood? If that's the case, I can understand this change. | ||||||||||||||||||||||||||||||
| Comment by Jeremy Mikola [ 17/Aug/20 ] | ||||||||||||||||||||||||||||||
|
val@karpov.io: To get back to the original question, is it reasonable for Mongoose to utilize replaceOne when it must perform an upsert with nothing more than the filter criteria (i.e. no atomic modifiers and an empty replacement document)? If so, can we resolve this as "Works as Designed"? | ||||||||||||||||||||||||||||||
| Comment by Jeremy Mikola [ 14/Aug/20 ] | ||||||||||||||||||||||||||||||
|
I was purposefully using the shell's db.collection.update() method as a thin abstraction for the update command and to avoid any additional argument validation from the CRUD spec methods. You're correct that updateOne would reject the {z:1} parameter I used in that example, but I was doing so because of this text in the OP:
I assumed this ticket was proposing that when upsert:true is specified drivers would not require an atomic operator in the first key of the update document for updateOne and updateMany. Perhaps it was only suggesting a single exception for the special case of an empty document. That said, I think any library/application could still opt to use replaceOne instead of updateOne in this case, which would not require a spec change. | ||||||||||||||||||||||||||||||
| Comment by Valeri Karpov [ 14/Aug/20 ] | ||||||||||||||||||||||||||||||
|
Hi Jeremy, Your example for "If we relaxed parameter validation on updateOne (and other update methods), the application could inadvertently trigger a replacement operation as in the following example" uses `update()` rather than `updateOne()`. It is my understanding that there is no such ambiguity with `updateOne()`, and `update()` is deprecated. Am I missing something here? Thanks, | ||||||||||||||||||||||||||||||
| Comment by Jeremy Mikola [ 10/Aug/20 ] | ||||||||||||||||||||||||||||||
If "insert and empty document" means that the desired outcome is a new document with just an _id (or perhaps _id with additional filter fields), I think that's already possible given the following two examples. Just the _id and filter fields:
Note that $setOnInsert will ensure that an existing document is never modified. The server doesn't allow an empty document, so you'd need to use at least one value here. In the case of an ODM that's generating its own _id, I suppose you could also use that in place of x in this example (assuming the filter wasn't matching a specific value of x). Just the _id with the filter fields removed:
| ||||||||||||||||||||||||||||||
| Comment by Matt Broadstone [ 10/Aug/20 ] | ||||||||||||||||||||||||||||||
|
jmikola while I agree the potential for accidental full-document replacement is a very real concern, I think I missed a subtle point in my initial description. What if a user wants to insert an empty document if no existing document matches a filter, but otherwise leave the existing document alone? replaceOne in this case will always replace the document which is not the desired behavior. | ||||||||||||||||||||||||||||||
| Comment by Jeremy Mikola [ 05/Aug/20 ] | ||||||||||||||||||||||||||||||
I don't think this is accurate. ReplaceOptions in the CRUD spec do solicit an upsert option. The application/library using the driver can accordingly use replaceOne (or equivalent replace API for findAndModify/bulk) to perform an upsert. I would argue that this behavior (i.e. update methods rejecting an empty document for lack of modifiers) is consistent with the design rationale to force users to select a well-named method for their operation. There is no single "upsert" operation in MongoDB, as the behavior of an upsert very much depends on whether an update or replacement document is used. Consider the following shell session with MongoDB 4.4:
Using upsert:true without atomic modifiers will disregard all but _id from the filter argument. If we relaxed parameter validation on updateOne (and other update methods), the application could inadvertently trigger a replacement operation as in the following example:
At runtime, there's no way for the driver to infer whether the upsert might trigger a full-document replacement, which I believe runs contrary to the CRUD spec's design rationale of intentionally named methods. I don't think the presence of an _id field in the filter parameter is relevant to the driver (e.g. Mongoose generating a new ObjectId), as the driver has no way of knowing whether it's an existing _id or newly generated. In the example you cited, the application has the opportunity to express its intention by using replaceOne instead of updateOne, and that the driver can trust. I'll note that while the CRUD spec does discuss Update vs. Replace validation, it does not appear to have a design rationale for separating the update and replace methods. I don't recall if it did at one time and the text was since removed. I'd support adding rationale to resolve this issue.
I encountered this error years ago through Doctrine ODM, which sometimes produced empty documents from changeset computations. In |