GODRIVER-3924 Fix bug of unmarshaling null.#2401
Merged
Merged
Conversation
Contributor
🧪 Performance ResultsCommit SHA: 7dd7aacThe following benchmark tests for version 6a16211460604d0007574a2c 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
API Change ReportNo changes found! |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes BSON decoding behavior so bson.Raw, bsoncore.Document, and bsoncore.Array correctly handle TypeNull and TypeUndefined during unmarshaling, aligning these decoders with other value decoders in the package and preventing spurious type-compatibility errors.
Changes:
- Add
TypeNull/TypeUndefinedhandling torawDecodeValue,coreDocumentDecodeValue, andarrayCodec.DecodeValue. - Expand unit test coverage for the above decoders (including explicit
TypeUndefinedcases). - Add unmarshaling compatibility tests for decoding
bson.Dandnilintobson.Raw/bsoncoreraw byte types.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| bson/unmarshal_test.go | Adds integration-style unmarshaling compatibility tests for bson.D/nil into raw byte container types. |
| bson/primitive_codecs.go | Updates bson.Raw decoding to accept and properly consume null/undefined. |
| bson/primitive_codecs_test.go | Adds unit tests for bson.Raw decoding of null/undefined. |
| bson/default_value_decoders.go | Updates bson.D and bsoncore.Document decoding to handle undefined (and null for core document). |
| bson/default_value_decoders_test.go | Adds unit tests for bsoncore.Document and bsoncore.Array decoding of null/undefined. |
| bson/array_codec.go | Updates bsoncore.Array decoding to handle null/undefined. |
Comments suppressed due to low confidence (2)
bson/primitive_codecs.go:101
rawDecodeValueallowsTypeArraybut always copies viaappendDocumentBytes, which falls back tocopyDocument(and thereforeValueReader.ReadDocument) when the reader does not implement the internalbytesReaderfast-path. ForTypeArray,ReadDocumentis not a valid transition for aValueReaderimplementation, so decoding arrays intobson.Rawcan fail for non-bytes readers (even thoughTypeArrayis explicitly accepted).
switch vrType := vr.Type(); vrType {
case Type(0), TypeEmbeddedDocument, TypeArray:
case TypeNull:
val.Set(reflect.Zero(val.Type()))
return vr.ReadNull()
case TypeUndefined:
val.Set(reflect.Zero(val.Type()))
return vr.ReadUndefined()
default:
return fmt.Errorf("cannot decode %v into a %s", vrType, val.Type())
}
if val.IsNil() {
val.Set(reflect.MakeSlice(val.Type(), 0, 0))
}
val.SetLen(0)
rdr, err := appendDocumentBytes(val.Interface().(Raw), vr)
val.Set(reflect.ValueOf(rdr))
return err
bson/default_value_decoders.go:1336
coreDocumentDecodeValueacceptsTypeArraybut always copies bytes viaappendDocumentBytes, whose slow path usesValueReader.ReadDocument(). ForTypeArray,ReadDocumentis not a valid operation per theValueReadercontract, so decoding arrays intobsoncore.Documentcan fail forValueReaderimplementations that don't support the internalbytesReadershortcut.
switch vrType := vr.Type(); vrType {
case Type(0), TypeEmbeddedDocument, TypeArray:
case TypeNull:
val.Set(reflect.Zero(val.Type()))
return vr.ReadNull()
case TypeUndefined:
val.Set(reflect.Zero(val.Type()))
return vr.ReadUndefined()
default:
return fmt.Errorf("cannot decode %v into a %s", vrType, val.Type())
}
if val.IsNil() {
val.Set(reflect.MakeSlice(val.Type(), 0, 0))
}
val.SetLen(0)
cdoc, err := appendDocumentBytes(val.Interface().(bsoncore.Document), vr)
val.Set(reflect.ValueOf(cdoc))
return err
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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-3924
Summary
This PR improves type guards and test coverage for
TypeNullorTypeUndefined.Background & Motivation
The type guards added to
rawDecodeValue,coreDocumentDecodeValue, andarrayCodec.DecodeValuedidn't handleTypeNullorTypeUndefined.This PR also improves test coverage: