[DRIVERS-2725] BulkWriteResult.insertedIds does not reflect actual inserted documents Created: 15/Sep/23 Updated: 19/Sep/23 |
|
| Status: | Backlog |
| Project: | Drivers |
| Component/s: | CRUD |
| Fix Version/s: | None |
| Type: | Task | Priority: | Unknown |
| Reporter: | Aditi Khare | Assignee: | Unassigned |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | leads-triage | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||
| Driver Changes: | Needed | ||||||||||||
| Description |
SummaryCurrently, the drivers specification for BulkWriteResult provides an optional property `insertedIds` (see CRUD spec, MongoDB manual). A user on Version 5.6.0 of the Node Driver reported the following bug on BulkWriteResult.insertedIds: If there are writeErrors and an _id is not correctly inserted, it will still incorrectly appear in the insertedIds Object. This bug likely reproduce on other drivers variations that support BulkWriteResult.insertedIds. MotivationWho is the affected end user?The affected end user is any user using BulkWrite or InsertMany with inserts that contain any errors. How does this affect the end user?In the event of an error case, the user will be confused as to which _ids were inserted. This is because BulkWriteResult.writeErrors will signal that certain inserts were unsuccessful, but BulkWriteResult.insertedIds will include all attempted inserts. This can lead the user to be misinformed on which inserts were successful. How likely is it that this problem or use case will occur?Edge Case If the problem does occur, what are the consequences and how severe are they?Misinformation, potentially minor Is this issue urgent?N/A Is this ticket required by a downstream team?May affect Mongosh Is this ticket only for tests?No Acceptance Criteria
|
| Comments |
| Comment by Jeremy Mikola [ 18/Sep/23 ] |
|
I believe this exact behavior was previously discussed in SPEC-82, where the outcome was to allow drivers to report the auto-generated ID even in the event of a write error. The current wording of the CRUD spec does not address how to populate or interpret insertedIds when dealing with write errors, so the first step would be making a spec change to clarify that one way or another. If we keep the existing behavior, the spec could be improved to mention that insertedIds may contain IDs of failed inserts and that the map should be interpreted alongside writeErrors to infer exactly which IDs were successfully inserted. Whether we keep the existing behavior or change it (to prune insertedIds of errored inserts), we can consider adding test coverage. This could be tested using expectedError.expectResult in the Unified Test Format to assert that erroneous inserts (e.g. duplicate key violations) are pruned from the insertedIds map on the BulkWriteResult exposed on a BulkWriteError. An additional test could also be created for insertMany operations, since those also throw BulkWriteError exceptions. Since the insertedIds map is optional, these tests should use $$unsetOrMatches for their assertions (as is done in existing non-error tests for insertedIds). |