From dc02142ab6530d706fbcaae3cac5f165cb9a3b7b Mon Sep 17 00:00:00 2001 From: Preston Vasquez Date: Tue, 28 Jan 2025 19:51:59 -0700 Subject: [PATCH 1/6] GODRIVER-3470 Correct BSON unmarshaling logic for null values --- bson/bsoncodec/default_value_decoders.go | 8 +++++++- bson/unmarshal.go | 3 +++ bson/unmarshaling_cases_test.go | 26 ++++++++++++++++++++++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/bson/bsoncodec/default_value_decoders.go b/bson/bsoncodec/default_value_decoders.go index 159297ef0a..8702d6d39e 100644 --- a/bson/bsoncodec/default_value_decoders.go +++ b/bson/bsoncodec/default_value_decoders.go @@ -1521,7 +1521,13 @@ func (dvd DefaultValueDecoders) ValueUnmarshalerDecodeValue(_ DecodeContext, vr return ValueDecoderError{Name: "ValueUnmarshalerDecodeValue", Types: []reflect.Type{tValueUnmarshaler}, Received: val} } - if vr.Type() == bsontype.Null { + // If BSON value is null and the go value is a pointer, then don't call + // UnmarshalBSONValue. Even if the Go pointer is already initialized (i.e., + // non-nil), encountering null in BSON will result in the pointer being + // directly set to nil here. Since the pointer is being replaced with nil, + // there is no opportunity (or reason) for the custom UnmarshalBSONValue logic + // to be called. + if vr.Type() == bsontype.Null && val.Kind() == reflect.Ptr { val.Set(reflect.Zero(val.Type())) return vr.ReadNull() diff --git a/bson/unmarshal.go b/bson/unmarshal.go index 66da17ee01..d749ba373b 100644 --- a/bson/unmarshal.go +++ b/bson/unmarshal.go @@ -41,6 +41,9 @@ type ValueUnmarshaler interface { // Unmarshal parses the BSON-encoded data and stores the result in the value // pointed to by val. If val is nil or not a pointer, Unmarshal returns // InvalidUnmarshalError. +// +// When unmarshaling BSON, if the BSON value is null and the Go value is a +// pointer, the pointer is set to nil without calling UnmarshalBSONValue. func Unmarshal(data []byte, val interface{}) error { return UnmarshalWithRegistry(DefaultRegistry, data, val) } diff --git a/bson/unmarshaling_cases_test.go b/bson/unmarshaling_cases_test.go index dd38369bff..24b39d5438 100644 --- a/bson/unmarshaling_cases_test.go +++ b/bson/unmarshaling_cases_test.go @@ -114,6 +114,17 @@ func unmarshalingTestCases() []unmarshalingTestCase { }, data: docToBytes(D{{"fooBar", int32(10)}}), }, + { + name: "nil pointer and non-pointer type with literal null BSON", + sType: reflect.TypeOf(unmarshalBehaviorTestCase{}), + want: &unmarshalBehaviorTestCase{ + Tracker: unmarshalCallTracker{ + unmarshalCalled: true, + }, + PtrTracker: nil, + }, + data: docToBytes(D{{Key: "tracker", Value: nil}, {Key: "ptr_tracker", Value: nil}}), + }, // GODRIVER-2252 // Test that a struct of pointer types with UnmarshalBSON functions defined marshal and // unmarshal to the same Go values when the pointer values are "nil". @@ -269,3 +280,18 @@ func (ms *myString) UnmarshalBSON(bytes []byte) error { *ms = myString(s) return nil } + +type unmarshalCallTracker struct { + unmarshalCalled bool +} + +type unmarshalBehaviorTestCase struct { + Tracker unmarshalCallTracker `bson:"tracker"` + PtrTracker *unmarshalCallTracker `bson:"ptr_tracker"` +} + +func (ms *unmarshalCallTracker) UnmarshalBSONValue(bsontype.Type, []byte) error { + ms.unmarshalCalled = true + + return nil +} From f1e8a1d561f0b4e4e4658179ed4281d0d8f544cd Mon Sep 17 00:00:00 2001 From: Preston Vasquez Date: Tue, 28 Jan 2025 20:12:19 -0700 Subject: [PATCH 2/6] GODRIVER-3470 Add test case for an initialized ptr --- bson/unmarshaling_cases_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/bson/unmarshaling_cases_test.go b/bson/unmarshaling_cases_test.go index 24b39d5438..c3fe1d8e54 100644 --- a/bson/unmarshaling_cases_test.go +++ b/bson/unmarshaling_cases_test.go @@ -8,9 +8,12 @@ package bson import ( "reflect" + "testing" "go.mongodb.org/mongo-driver/bson/bsonrw" "go.mongodb.org/mongo-driver/bson/bsontype" + "go.mongodb.org/mongo-driver/internal/assert" + "go.mongodb.org/mongo-driver/internal/require" ) type unmarshalingTestCase struct { @@ -295,3 +298,20 @@ func (ms *unmarshalCallTracker) UnmarshalBSONValue(bsontype.Type, []byte) error return nil } + +func TestInitializedPointerDataWithBSONNull(t *testing.T) { + // Set up the test case with an initialized pointer. + tc := unmarshalBehaviorTestCase{ + PtrTracker: &unmarshalCallTracker{}, + } + + // Create BSON data where the 'ptr_tracker' field is explicitly set to null. + bytes := docToBytes(D{{Key: "ptr_tracker", Value: nil}}) + + // Unmarshal the BSON data into the test case struct. + // This should set PtrTracker to nil due to the BSON null value. + err := Unmarshal(bytes, &tc) + require.NoError(t, err) + + assert.Nil(t, tc.PtrTracker) +} From 7d2be3c5135b221cde5d8cffcaa809bc2cbedf20 Mon Sep 17 00:00:00 2001 From: Preston Vasquez Date: Mon, 3 Feb 2025 13:05:52 -0700 Subject: [PATCH 3/6] GODRIVER-3470 Add UnmarshalBSON initialization tests --- bson/unmarshal_value_test.go | 24 ++++++++++++ bson/unmarshaling_cases_test.go | 68 +++++++++++++++++++-------------- 2 files changed, 63 insertions(+), 29 deletions(-) diff --git a/bson/unmarshal_value_test.go b/bson/unmarshal_value_test.go index ef91da1659..3455deeaaa 100644 --- a/bson/unmarshal_value_test.go +++ b/bson/unmarshal_value_test.go @@ -14,6 +14,7 @@ import ( "go.mongodb.org/mongo-driver/bson/bsoncodec" "go.mongodb.org/mongo-driver/bson/bsontype" "go.mongodb.org/mongo-driver/internal/assert" + "go.mongodb.org/mongo-driver/internal/require" "go.mongodb.org/mongo-driver/x/bsonx/bsoncore" ) @@ -93,6 +94,29 @@ func TestUnmarshalValue(t *testing.T) { }) } +func TestInitializedPointerDataWithBSONNull(t *testing.T) { + // Set up the test case with initialized pointers. + tc := unmarshalBehaviorTestCase{ + BSONValuePtrTracker: &unmarshalBSONValueCallTracker{}, + BSONPtrTracker: &unmarshalBSONCallTracker{}, + } + + // Create BSON data where the '*_ptr_tracker' fields are explicitly set to + // null. + bytes := docToBytes(D{ + {Key: "bv_ptr_tracker", Value: nil}, + {Key: "b_ptr_tracker", Value: nil}, + }) + + // Unmarshal the BSON data into the test case struct. This should set the + // pointer fields to nil due to the BSON null value. + err := Unmarshal(bytes, &tc) + require.NoError(t, err) + + assert.Nil(t, tc.BSONValuePtrTracker) + assert.Nil(t, tc.BSONPtrTracker) +} + // tests covering GODRIVER-2779 func BenchmarkSliceCodecUnmarshal(b *testing.B) { benchmarks := []struct { diff --git a/bson/unmarshaling_cases_test.go b/bson/unmarshaling_cases_test.go index c3fe1d8e54..e9088f219c 100644 --- a/bson/unmarshaling_cases_test.go +++ b/bson/unmarshaling_cases_test.go @@ -8,12 +8,9 @@ package bson import ( "reflect" - "testing" "go.mongodb.org/mongo-driver/bson/bsonrw" "go.mongodb.org/mongo-driver/bson/bsontype" - "go.mongodb.org/mongo-driver/internal/assert" - "go.mongodb.org/mongo-driver/internal/require" ) type unmarshalingTestCase struct { @@ -121,12 +118,21 @@ func unmarshalingTestCases() []unmarshalingTestCase { name: "nil pointer and non-pointer type with literal null BSON", sType: reflect.TypeOf(unmarshalBehaviorTestCase{}), want: &unmarshalBehaviorTestCase{ - Tracker: unmarshalCallTracker{ - unmarshalCalled: true, + BSONValueTracker: unmarshalBSONValueCallTracker{ + called: true, }, - PtrTracker: nil, + BSONValuePtrTracker: nil, + BSONTracker: unmarshalBSONCallTracker{ + called: true, + }, + BSONPtrTracker: nil, }, - data: docToBytes(D{{Key: "tracker", Value: nil}, {Key: "ptr_tracker", Value: nil}}), + data: docToBytes(D{ + {Key: "bv_tracker", Value: nil}, + {Key: "bv_ptr_tracker", Value: nil}, + {Key: "b_tracker", Value: nil}, + {Key: "b_ptr_tracker", Value: nil}, + }), }, // GODRIVER-2252 // Test that a struct of pointer types with UnmarshalBSON functions defined marshal and @@ -284,34 +290,38 @@ func (ms *myString) UnmarshalBSON(bytes []byte) error { return nil } -type unmarshalCallTracker struct { - unmarshalCalled bool -} - -type unmarshalBehaviorTestCase struct { - Tracker unmarshalCallTracker `bson:"tracker"` - PtrTracker *unmarshalCallTracker `bson:"ptr_tracker"` +// unmarshalBSONValueCallTracker is a test struct that tracks whether the +// UnmarshalBSONValue method has been called. +type unmarshalBSONValueCallTracker struct { + called bool // called is set to true when UnmarshalBSONValue is invoked. } -func (ms *unmarshalCallTracker) UnmarshalBSONValue(bsontype.Type, []byte) error { - ms.unmarshalCalled = true +var _ ValueUnmarshaler = &unmarshalBSONValueCallTracker{} - return nil +// unmarshalBSONCallTracker is a test struct that tracks whether the +// UnmarshalBSON method has been called. +type unmarshalBSONCallTracker struct { + called bool // called is set to true when UnmarshalBSON is invoked. } -func TestInitializedPointerDataWithBSONNull(t *testing.T) { - // Set up the test case with an initialized pointer. - tc := unmarshalBehaviorTestCase{ - PtrTracker: &unmarshalCallTracker{}, - } +// Ensure unmarshalBSONCallTracker implements the Unmarshaler interface. +var _ Unmarshaler = &unmarshalBSONCallTracker{} - // Create BSON data where the 'ptr_tracker' field is explicitly set to null. - bytes := docToBytes(D{{Key: "ptr_tracker", Value: nil}}) +// unmarshalBehaviorTestCase holds instances of call trackers for testing BSON +// unmarshaling behavior. +type unmarshalBehaviorTestCase struct { + BSONValueTracker unmarshalBSONValueCallTracker `bson:"bv_tracker"` // BSON value unmarshaling by value. + BSONValuePtrTracker *unmarshalBSONValueCallTracker `bson:"bv_ptr_tracker"` // BSON value unmarshaling by pointer. + BSONTracker unmarshalBSONCallTracker `bson:"b_tracker"` // BSON unmarshaling by value. + BSONPtrTracker *unmarshalBSONCallTracker `bson:"b_ptr_tracker"` // BSON unmarshaling by pointer. +} - // Unmarshal the BSON data into the test case struct. - // This should set PtrTracker to nil due to the BSON null value. - err := Unmarshal(bytes, &tc) - require.NoError(t, err) +func (tracker *unmarshalBSONValueCallTracker) UnmarshalBSONValue(bsontype.Type, []byte) error { + tracker.called = true + return nil +} - assert.Nil(t, tc.PtrTracker) +func (tracker *unmarshalBSONCallTracker) UnmarshalBSON([]byte) error { + tracker.called = true + return nil } From 32190fe4f265f1aeff785ba8cc51be18cdc9d7ce Mon Sep 17 00:00:00 2001 From: Preston Vasquez Date: Thu, 6 Feb 2025 12:01:27 -0700 Subject: [PATCH 4/6] GODRIVER-3470 UnmarshalerDecodeValue to mirror ValueUnmarshalerDecodeValue null check --- bson/bsoncodec/default_value_decoders.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/bson/bsoncodec/default_value_decoders.go b/bson/bsoncodec/default_value_decoders.go index 8702d6d39e..b66c92154a 100644 --- a/bson/bsoncodec/default_value_decoders.go +++ b/bson/bsoncodec/default_value_decoders.go @@ -1569,6 +1569,18 @@ func (dvd DefaultValueDecoders) UnmarshalerDecodeValue(_ DecodeContext, vr bsonr return ValueDecoderError{Name: "UnmarshalerDecodeValue", Types: []reflect.Type{tUnmarshaler}, Received: val} } + // If BSON value is null and the go value is a pointer, then don't call + // UnmarshalBSONValue. Even if the Go pointer is already initialized (i.e., + // non-nil), encountering null in BSON will result in the pointer being + // directly set to nil here. Since the pointer is being replaced with nil, + // there is no opportunity (or reason) for the custom UnmarshalBSONValue logic + // to be called. + if val.Kind() == reflect.Ptr && vr.Type() == bsontype.Null { + val.Set(reflect.Zero(val.Type())) + + return vr.ReadNull() + } + if val.Kind() == reflect.Ptr && val.IsNil() { if !val.CanSet() { return ValueDecoderError{Name: "UnmarshalerDecodeValue", Types: []reflect.Type{tUnmarshaler}, Received: val} @@ -1581,18 +1593,6 @@ func (dvd DefaultValueDecoders) UnmarshalerDecodeValue(_ DecodeContext, vr bsonr return err } - // If the target Go value is a pointer and the BSON field value is empty, set the value to the - // zero value of the pointer (nil) and don't call UnmarshalBSON. UnmarshalBSON has no way to - // change the pointer value from within the function (only the value at the pointer address), - // so it can't set the pointer to "nil" itself. Since the most common Go value for an empty BSON - // field value is "nil", we set "nil" here and don't call UnmarshalBSON. This behavior matches - // the behavior of the Go "encoding/json" unmarshaler when the target Go value is a pointer and - // the JSON field value is "null". - if val.Kind() == reflect.Ptr && len(src) == 0 { - val.Set(reflect.Zero(val.Type())) - return nil - } - if !val.Type().Implements(tUnmarshaler) { if !val.CanAddr() { return ValueDecoderError{Name: "UnmarshalerDecodeValue", Types: []reflect.Type{tUnmarshaler}, Received: val} From 75ee151860e6fa782282423acd87859eeb6967c7 Mon Sep 17 00:00:00 2001 From: Preston Vasquez Date: Thu, 6 Feb 2025 12:15:06 -0700 Subject: [PATCH 5/6] GODRIVER-3470 Clean up comment --- bson/bsoncodec/default_value_decoders.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bson/bsoncodec/default_value_decoders.go b/bson/bsoncodec/default_value_decoders.go index b66c92154a..7ae7638caf 100644 --- a/bson/bsoncodec/default_value_decoders.go +++ b/bson/bsoncodec/default_value_decoders.go @@ -1570,11 +1570,11 @@ func (dvd DefaultValueDecoders) UnmarshalerDecodeValue(_ DecodeContext, vr bsonr } // If BSON value is null and the go value is a pointer, then don't call - // UnmarshalBSONValue. Even if the Go pointer is already initialized (i.e., + // UnmarshalBSON. Even if the Go pointer is already initialized (i.e., // non-nil), encountering null in BSON will result in the pointer being // directly set to nil here. Since the pointer is being replaced with nil, - // there is no opportunity (or reason) for the custom UnmarshalBSONValue logic - // to be called. + // there is no opportunity (or reason) for the custom UnmarshalBSON logic to + // be called. if val.Kind() == reflect.Ptr && vr.Type() == bsontype.Null { val.Set(reflect.Zero(val.Type())) From cd82ab3d5ddb81d78be2a4c1bd81fe0bb9d3795e Mon Sep 17 00:00:00 2001 From: Preston Vasquez Date: Fri, 14 Feb 2025 13:49:41 -0700 Subject: [PATCH 6/6] GODRIVER-3470 Revert changes to UnmarshalerDecodeValue --- bson/bsoncodec/default_value_decoders.go | 24 ++++++------- bson/unmarshaling_cases_test.go | 45 ++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 12 deletions(-) diff --git a/bson/bsoncodec/default_value_decoders.go b/bson/bsoncodec/default_value_decoders.go index 7ae7638caf..8702d6d39e 100644 --- a/bson/bsoncodec/default_value_decoders.go +++ b/bson/bsoncodec/default_value_decoders.go @@ -1569,18 +1569,6 @@ func (dvd DefaultValueDecoders) UnmarshalerDecodeValue(_ DecodeContext, vr bsonr return ValueDecoderError{Name: "UnmarshalerDecodeValue", Types: []reflect.Type{tUnmarshaler}, Received: val} } - // If BSON value is null and the go value is a pointer, then don't call - // UnmarshalBSON. Even if the Go pointer is already initialized (i.e., - // non-nil), encountering null in BSON will result in the pointer being - // directly set to nil here. Since the pointer is being replaced with nil, - // there is no opportunity (or reason) for the custom UnmarshalBSON logic to - // be called. - if val.Kind() == reflect.Ptr && vr.Type() == bsontype.Null { - val.Set(reflect.Zero(val.Type())) - - return vr.ReadNull() - } - if val.Kind() == reflect.Ptr && val.IsNil() { if !val.CanSet() { return ValueDecoderError{Name: "UnmarshalerDecodeValue", Types: []reflect.Type{tUnmarshaler}, Received: val} @@ -1593,6 +1581,18 @@ func (dvd DefaultValueDecoders) UnmarshalerDecodeValue(_ DecodeContext, vr bsonr return err } + // If the target Go value is a pointer and the BSON field value is empty, set the value to the + // zero value of the pointer (nil) and don't call UnmarshalBSON. UnmarshalBSON has no way to + // change the pointer value from within the function (only the value at the pointer address), + // so it can't set the pointer to "nil" itself. Since the most common Go value for an empty BSON + // field value is "nil", we set "nil" here and don't call UnmarshalBSON. This behavior matches + // the behavior of the Go "encoding/json" unmarshaler when the target Go value is a pointer and + // the JSON field value is "null". + if val.Kind() == reflect.Ptr && len(src) == 0 { + val.Set(reflect.Zero(val.Type())) + return nil + } + if !val.Type().Implements(tUnmarshaler) { if !val.CanAddr() { return ValueDecoderError{Name: "UnmarshalerDecodeValue", Types: []reflect.Type{tUnmarshaler}, Received: val} diff --git a/bson/unmarshaling_cases_test.go b/bson/unmarshaling_cases_test.go index e9088f219c..358088fe84 100644 --- a/bson/unmarshaling_cases_test.go +++ b/bson/unmarshaling_cases_test.go @@ -11,6 +11,7 @@ import ( "go.mongodb.org/mongo-driver/bson/bsonrw" "go.mongodb.org/mongo-driver/bson/bsontype" + "go.mongodb.org/mongo-driver/bson/primitive" ) type unmarshalingTestCase struct { @@ -194,6 +195,50 @@ func unmarshalingTestCases() []unmarshalingTestCase { want: &valNonPtrStruct, data: docToBytes(valNonPtrStruct), }, + { + name: "nil pointer and non-pointer type with BSON minkey", + sType: reflect.TypeOf(unmarshalBehaviorTestCase{}), + want: &unmarshalBehaviorTestCase{ + BSONValueTracker: unmarshalBSONValueCallTracker{ + called: true, + }, + BSONValuePtrTracker: &unmarshalBSONValueCallTracker{ + called: true, + }, + BSONTracker: unmarshalBSONCallTracker{ + called: true, + }, + BSONPtrTracker: nil, + }, + data: docToBytes(D{ + {Key: "bv_tracker", Value: primitive.MinKey{}}, + {Key: "bv_ptr_tracker", Value: primitive.MinKey{}}, + {Key: "b_tracker", Value: primitive.MinKey{}}, + {Key: "b_ptr_tracker", Value: primitive.MinKey{}}, + }), + }, + { + name: "nil pointer and non-pointer type with BSON maxkey", + sType: reflect.TypeOf(unmarshalBehaviorTestCase{}), + want: &unmarshalBehaviorTestCase{ + BSONValueTracker: unmarshalBSONValueCallTracker{ + called: true, + }, + BSONValuePtrTracker: &unmarshalBSONValueCallTracker{ + called: true, + }, + BSONTracker: unmarshalBSONCallTracker{ + called: true, + }, + BSONPtrTracker: nil, + }, + data: docToBytes(D{ + {Key: "bv_tracker", Value: primitive.MaxKey{}}, + {Key: "bv_ptr_tracker", Value: primitive.MaxKey{}}, + {Key: "b_tracker", Value: primitive.MaxKey{}}, + {Key: "b_ptr_tracker", Value: primitive.MaxKey{}}, + }), + }, } }