GODRIVER-3707: Consolidate duplicate binary utility functions from bsoncore and wiremessage packages #2274
+167
−172
Conversation
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
Create new binaryutil package Create new Append32 function based off PutUint32 from go binary standard library Replace use of appendu32 function from bson x package Co-authored-by: Preston Vasquez <[email protected]>
Replace use of appendi32 function from bson x package and x/mongo/driver/wiremessage/wiremessage
Create new Append64 function based off PutUint64 from go binary standard library Co-authored-by: Preston Vasquez <[email protected]>
Replace use of appendi64 function from these packages x/mongo/driver/wiremessage x/bsonx/bsoncore
Replace use of appendu64 function from these packages x/mongo/driver/wiremessage x/bsonx/bsoncore
Create function that can read uint32 and int32 from byte slice
Go can't infer types from return types which means types need to be explicit for binary read functions Instead I created read functions for the unsigned and signed types that call the generic with a type
Replace readu32 with new ReadU32 function in internal binary package
Replace readi32 in bson and wiremessage package with new ReadI32 function in the internal binary package
Create new generic function to read uint64 and int64 from binary slice Create new wrapper functions for unsigned and signed int calls
Replace use of readu64 in bson package with new ReadU64 function in the internal binary utils package
Replace use of readi64 in bson and wiremessage package with new ReadI64 function in the internal binary utils package
Created a ReadCStringBytes function that reads bytes from a byte slice Created a ReadCString function that wraps bytes returned from ReadCStringBytes as a string
Replace readcstringbytes function from bson package with ReadCStringBytes function from binary utils package
Replace readcstring function from bson and wiremessage package with ReadCString function from binary utils package
RafaelCenzano
added
enhancement
review-priority-normal
Contributor
API Change ReportNo changes found! |
Contributor
🧪 Performance ResultsCommit SHA: 815508eThe following benchmark tests for version 6949d2da0e83520007712646 had statistically significant changes (i.e., |z-score| > 1.96):
For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates duplicate binary utility functions from the bsoncore and wiremessage packages into a new shared binaryutil package, reducing code duplication and improving maintainability.
Key Changes:
- Created a new
internal/binaryutilpackage with generic implementations of 32-bit and 64-bit integer append/read functions - Consolidated C-string reading functions to use a single implementation
- Updated all callers in
bsoncoreandwiremessagepackages to use the new utility functions
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/binaryutil/binaryutil.go | New utility package containing generic Append32/64, ReadI32/I64, ReadU32/U64, and ReadCString functions |
| x/mongo/driver/wiremessage/wiremessage.go | Updated to use binaryutil functions; removed duplicate implementations of appendi32, appendi64, readi32, readi64, and readcstring |
| x/mongo/driver/wiremessage/wiremessage_test.go | Updated test calls to use binaryutil.Append32/64 and ReadI32/64 |
| x/bsonx/bsoncore/bsoncore.go | Updated to use binaryutil functions; removed duplicate implementations of append/read functions for signed and unsigned 32/64-bit integers and C-strings |
| x/bsonx/bsoncore/document.go | Updated ReadI32 call to use binaryutil.ReadI32 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // AppendGetMoreZero appends the zero field to dst. | ||
| func AppendGetMoreZero(dst []byte) []byte { | ||
| return appendi32(dst, 0) | ||
| return binaryutil.Append32(dst, int32(0)) |
There was a problem hiding this comment.
The explicit cast to int32(0) is redundant since the literal 0 can be inferred as int32 by the generic type parameter T in Append32. Consider simplifying to just 0.
Suggested change
| return binaryutil.Append32(dst, int32(0)) | |
| return binaryutil.Append32(dst, 0) |
Contributor
Author
There was a problem hiding this comment.
Removing this cast leads to this IDE error: int does not satisfy ~uint32 | ~int32 (int missing in ~uint32 | ~int32)
| // AppendKillCursorsZero appends the zero field to dst. | ||
| func AppendKillCursorsZero(dst []byte) []byte { | ||
| return appendi32(dst, 0) | ||
| return binaryutil.Append32(dst, int32(0)) |
There was a problem hiding this comment.
The explicit cast to int32(0) is redundant since the literal 0 can be inferred as int32 by the generic type parameter T in Append32. Consider simplifying to just 0.
Contributor
Author
There was a problem hiding this comment.
Comment on lines
+1
to
+93
| // Copyright (C) MongoDB, Inc. 2025-present. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); you may | ||
| // not use this file except in compliance with the License. You may obtain | ||
| // a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| // Package binaryutil provides utility functions for working with binary data. | ||
| package binaryutil | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "encoding/binary" | ||
| ) | ||
|
|
||
| func Append32[T ~uint32 | ~int32](dst []byte, v T) []byte { | ||
| n := len(dst) | ||
| dst = append(dst, 0, 0, 0, 0) | ||
|
|
||
| b := dst[n:] | ||
| _ = b[3] // early bounds check | ||
|
|
||
| b[0] = byte(v) | ||
| b[1] = byte(v >> 8) | ||
| b[2] = byte(v >> 16) | ||
| b[3] = byte(v >> 24) | ||
|
|
||
| return dst | ||
| } | ||
|
|
||
| func Append64[T ~uint64 | ~int64](dst []byte, v T) []byte { | ||
| n := len(dst) | ||
| dst = append(dst, 0, 0, 0, 0, 0, 0, 0, 0) | ||
|
|
||
| b := dst[n:] | ||
| _ = b[7] // early bounds check | ||
|
|
||
| b[0] = byte(v) | ||
| b[1] = byte(v >> 8) | ||
| b[2] = byte(v >> 16) | ||
| b[3] = byte(v >> 24) | ||
| b[4] = byte(v >> 32) | ||
| b[5] = byte(v >> 40) | ||
| b[6] = byte(v >> 48) | ||
| b[7] = byte(v >> 56) | ||
|
|
||
| return dst | ||
| } | ||
|
|
||
| func read32[T ~uint32 | ~int32](src []byte) (T, []byte, bool) { | ||
| if len(src) < 4 { | ||
| return 0, src, false | ||
| } | ||
| return T(binary.LittleEndian.Uint32(src)), src[4:], true | ||
| } | ||
|
|
||
| func ReadU32(src []byte) (uint32, []byte, bool) { | ||
| return read32[uint32](src) | ||
| } | ||
|
|
||
| func ReadI32(src []byte) (int32, []byte, bool) { | ||
| return read32[int32](src) | ||
| } | ||
|
|
||
| func read64[T ~uint64 | ~int64](src []byte) (T, []byte, bool) { | ||
| if len(src) < 8 { | ||
| return 0, src, false | ||
| } | ||
| return T(binary.LittleEndian.Uint64(src)), src[8:], true | ||
| } | ||
|
|
||
| func ReadU64(src []byte) (uint64, []byte, bool) { | ||
| return read64[uint64](src) | ||
| } | ||
|
|
||
| func ReadI64(src []byte) (int64, []byte, bool) { | ||
| return read64[int64](src) | ||
| } | ||
|
|
||
| func ReadCStringBytes(src []byte) ([]byte, []byte, bool) { | ||
| idx := bytes.IndexByte(src, 0x00) | ||
| if idx < 0 { | ||
| return nil, src, false | ||
| } | ||
| return src[:idx], src[idx+1:], true | ||
| } | ||
|
|
||
| func ReadCString(src []byte) (string, []byte, bool) { | ||
| cstr, rem, ok := ReadCStringBytes(src) | ||
| if !ok { | ||
| return "", src, false | ||
| } | ||
| return string(cstr), rem, true | ||
| } |
There was a problem hiding this comment.
Consider adding a dedicated test file for the new binaryutil package. While the functions are currently tested indirectly through wiremessage and bsoncore tests, having direct unit tests in binaryutil_test.go would improve code maintainability and make it easier to verify correct behavior independently.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
GODRIVER-3707
Summary
Background & Motivation
There was duplicate code across the bsoncore and wiremessage packages. This PR consolidates it and made some minor improvements including better/safer appending thanks to work from @prestonvasquez and instead of stating read c string functions must be synced, use the same core logic and wrap a cast on the response from the bytes version for the string version.