Test that we are inline with the BSON Object ID spec#3
Merged
Conversation
nick96
force-pushed
the
test-by-spec
branch
from
b172a36 to
9c8daa5
Compare
Casting to a u32 would panic if the value was too large. Now just mode it with the max value. This is fine per the BSON Object ID spec.
We just care that each process gets a unique value, not that there is an ordering.
The counter is actually only 24 bits but we have to store it in a 32 bit integer. So can't rely on the natural wrapping of integer types.
The manual implementations boil down to exactly the same thing so just do that.
This doesn't require converting to bytes first and we more clearly codify the ordering.
Implements spec-mandated test cases including timestamp field validation, counter overflow behavior, hex string parsing, ordering verification, and big endian format checking to ensure full BSON ObjectId specification compliance. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
This way you can't forget to compile the extension before running tests.
nick96
force-pushed
the
test-by-spec
branch
from
9c8daa5 to
5a12cc6
Compare
Some of the tests modify the global counter, so they need to be run in isolation to prevent CI being flakey. Nextest runs the each test as its own binary so they can't interfere with one another.
With a normal add, when the counter reaches u32::MAX, adding to it will panic. Make sure we handle this case.
nick96
force-pushed
the
test-by-spec
branch
from
5a12cc6 to
5271894
Compare
shannoncole
reviewed
Sep 18, 2025
| let counter = COUNTER.fetch_add(1, Ordering::SeqCst); | ||
| // We don't care that there is an ordering between threads, just | ||
| // that each ID gets a unique value. | ||
| let counter = COUNTER.fetch_add(1, Ordering::Relaxed) & COUNTER_MAX; |
Contributor
There was a problem hiding this comment.
I don't understand this use of &; does it work? It seems we'll wrap and then mask out any bits that aren't in COUNTER_MAX; is that what we want?
Collaborator
Author
There was a problem hiding this comment.
Yeah it does work. It just zeros out bits on anything larger than COUNTER_MAX causing it to wrap at 24 bits
shannoncole
reviewed
Sep 18, 2025
| // Ensure the timestamp is 4 bytes. For timestamps far in the future | ||
| // this will truncate them but it's how the spec is defined. Note: if | ||
| // `t` is bigger than u32::MAX, the value will be truncated. This is | ||
| // expected. |
Contributor
There was a problem hiding this comment.
What about negative time-stamps?
Collaborator
Author
There was a problem hiding this comment.
It works-ish. It doesn't panic and you'll more than likely get a unique ID but you won't be able to get the same timestamp back. It's called out in the spec that this is explicitly not handled because MongoDB didn't exist before 1970.
shannoncole
reviewed
Sep 18, 2025
| // 0xFFFFFFFF: Feb 7th, 2106 06:28:15 UTC | ||
| let id = ObjectId::from_time(0xFFFFFFFFu32 as i64, false); | ||
| assert_eq!(id.timestamp(), 0xFFFFFFFFu32 as i64); | ||
| } |
Contributor
There was a problem hiding this comment.
can we also test i64 values that don't fit in u32? ie: too big, negative
shannoncole
reviewed
Sep 18, 2025
| @@ -0,0 +1,136 @@ | |||
| # BSON ObjectID | |||
Contributor
There was a problem hiding this comment.
oh good idea adding this!
Timestamps before 1970 will be negative so make sure we handle that by at least not panicking. Your ID will still be unique but you won't be able to get the correct timestamp back. Timestamps after 2038 won't bit into a u32 so will wrap around to 0. Also fine in terms of uniquenessbut has the same issue where you don't get the same timestamp backup.
Contributor
|
new tests look good! brew r+ 👍 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I've added the spec and have done a pass on checking we're inline with it. I've (and claude)
also added tests to make sure we stay inline with it. There were a few places
where we weren't compliant with the spec, see individual commits for details.