Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
46 changes: 46 additions & 0 deletions bson/unmarshaling_cases_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -114,6 +117,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".
Expand Down Expand Up @@ -269,3 +283,35 @@ 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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also test this behavior for UnmarshalBSON?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea!

ms.unmarshalCalled = true

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)
}
Loading