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-2124: test that inserts and upserts respect null _id values #1699

Merged
merged 3 commits into from
Oct 31, 2024

Conversation

nbbeeken
Copy link
Contributor

Please complete the following before merging:

  • [ ] Update changelog. N/A
  • Test changes in at least one language driver.
  • Test these changes against all server versions and topologies (including standalone, replica set, sharded
    clusters, and serverless).

Description

Adds tests for null and undefined as _id values to check that drivers do not interpret these types as "unset" and generate an ObjectId and overwrite the value.

@nbbeeken nbbeeken marked this pull request as ready for review October 30, 2024 14:02
@nbbeeken nbbeeken requested a review from a team as a code owner October 30, 2024 14:02
@nbbeeken nbbeeken requested review from jmikola and removed request for a team October 30, 2024 14:02
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

LGTM with suggestion applied.

Comment on lines +7 to +8
observeEvents:
- commandStartedEvent
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
observeEvents:
- commandStartedEvent

Looks like you got rid of APM assertions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured the result of count is the source of truth now since we are able to use a BSON type query avoiding any comparison looseness surrounding null/undefined. But happy to add it back, just saying it was intentional to remove.

Copy link
Member

Choose a reason for hiding this comment

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

countDocuments seems sufficient. APM assertions don't help if we have to worry about how some drivers represent null/undefined values.

@nbbeeken nbbeeken merged commit 83b2938 into master Oct 31, 2024
5 checks passed
@nbbeeken nbbeeken deleted the DRIVERS-2124-id-null branch October 31, 2024 17:25
arguments: {filter: *null_id_filter}
expectResult: 1

- description: inserting _id with type undefined via insertOne
Copy link
Member

Choose a reason for hiding this comment

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

Note: I'm removing these tests in #1713 as the server prohibits undefined values for _id.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants