Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DRIVERS-1408 - Add guidance on adding _id fields to documents to CRUD spec #1688

Merged
merged 6 commits into from
Oct 31, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions source/crud/crud.md
Original file line number Diff line number Diff line change
Expand Up @@ -815,6 +815,12 @@ database-level aggregation will allow users to receive a cursor from these colle

##### Insert, Update, Replace, Delete, and Bulk Writes

###### Generated identifiers

The insert and bulk insert operations described below MUST generate identifiers for all documents that do not already
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted that this is already discussed in the client bulk write spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having this behavior discussed here for general CRUD operations makes sense to me, as the client bulk write spec follows from this broader context.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The client bulk write spec stands on its own, except for a reference back to CRUD for modeling unacknowledged write results.

With my last comment, I just meant to acknowledge that no changes were needed to the client bulk write spec since it already addressed _id ordering. This was in the vein of previous PRs like #1644 that required updates to both specs.

have them. These identifiers SHOULD be prepended to the document so they are the first field, in order to prevent the
server from spending time re-ordering the document.
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved

```typescript
interface Collection {

Expand Down Expand Up @@ -2474,6 +2480,8 @@ aforementioned allowance in the SemVer spec.

## Changelog

- 2024-10-28: Clarified that generated identifiers should be prepended to documents.

- 2024-10-01: Add sort option to `replaceOne` and `updateOne`.

- 2024-09-12: Specify that explain helpers support maxTimeMS.
Expand Down
7 changes: 7 additions & 0 deletions source/crud/tests/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -737,3 +737,10 @@ that `firstEvent.operationId` is equal to `secondEvent.operationId`. Assert both
To force completion of the `w:0` writes, execute `coll.countDocuments` and expect the returned count is
`maxMessageSizeBytes / maxBsonObjectSize + 1`. This is intended to avoid incomplete writes interfering with other tests
that may use this collection.

### 16. Generated document identifiers are the first field in their document
alcaeus marked this conversation as resolved.
Show resolved Hide resolved

Construct a `MongoClient` (referred to as `client`) with
[command monitoring](../../command-logging-and-monitoring/command-logging-and-monitoring.md) enabled to observe
CommandStartedEvents. Perform an `insert` command using `client` and assert that one CommandStartedEvent (referred to as
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perform an insert command

This is ambiguous, as the prose test could be implementing using the generic command runner, in which case I'd expect no ID generation. I'd suggest changing this to explicitly suggesting running an insertOne operation. You can extend that with insertMany and a bulkWrite if you want full test coverage.

`event`) was observed, and that the first field of `event.command.documents[0]` is `_id`.
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
Loading