Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion bson/bsoncodec/default_value_decoders.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a similar block in UnmarshalerDecodeValue after the value is read into a []byte:

if val.Kind() == reflect.Ptr && len(src) == 0 {
	val.Set(reflect.Zero(val.Type()))
	return nil
}

Can we use the same condition in both methods? Or are they distinct scenarios?

Copy link
Member Author

@prestonvasquez prestonvasquez Feb 6, 2025

Choose a reason for hiding this comment

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

The len(src) check was implemented here: https://github.com/mongodb/mongo-go-driver/pull/833/files

Since the bytes represent BSON null are copied, the type is converted from null to invalid:

	_, src, err := bsonrw.Copier{}.CopyValueToBytes(vr)
	if err != nil {
		return err
	}

Which means that this check doesn’t work:

	if val.Kind() == reflect.Ptr && vr.Type() == bsontype.Null {
		val.Set(reflect.Zero(val.Type()))
		return nil
	}

Checking after copying is weaker since validity should be checked on the first block:

	if !val.IsValid() || (!val.Type().Implements(tValueUnmarshaler) && !reflect.PtrTo(val.Type()).Implements(tValueUnmarshaler)) {
		return ValueDecoderError{Name: "ValueUnmarshalerDecodeValue", Types: []reflect.Type{tValueUnmarshaler}, Received: val}
	}

So I think BSON null is the only case where you would get an invalid type after copying. I suggest we mirror ValueUnmarshalerDecodeValue.

val.Set(reflect.Zero(val.Type()))

return vr.ReadNull()
Expand Down
3 changes: 3 additions & 0 deletions bson/unmarshal.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
24 changes: 24 additions & 0 deletions bson/unmarshal_value_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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 {
Expand Down
56 changes: 56 additions & 0 deletions bson/unmarshaling_cases_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,26 @@ 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{
BSONValueTracker: unmarshalBSONValueCallTracker{
called: true,
},
BSONValuePtrTracker: nil,
BSONTracker: unmarshalBSONCallTracker{
called: true,
},
BSONPtrTracker: 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
// unmarshal to the same Go values when the pointer values are "nil".
Expand Down Expand Up @@ -269,3 +289,39 @@ func (ms *myString) UnmarshalBSON(bytes []byte) error {
*ms = myString(s)
return nil
}

// 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.
}

var _ ValueUnmarshaler = &unmarshalBSONValueCallTracker{}

// 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.
}

// Ensure unmarshalBSONCallTracker implements the Unmarshaler interface.
var _ Unmarshaler = &unmarshalBSONCallTracker{}

// 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.
}

func (tracker *unmarshalBSONValueCallTracker) UnmarshalBSONValue(bsontype.Type, []byte) error {
tracker.called = true
return nil
}

func (tracker *unmarshalBSONCallTracker) UnmarshalBSON([]byte) error {
tracker.called = true
return nil
}
Loading