GODRIVER-2252 Don't call UnmarshalBSON for pointer values if the BSON field value is empty. (#833)

Author: matthewdale

bson/bsoncodec/default_value_decoders.go

@@ -1504,6 +1504,18 @@ func (dvd DefaultValueDecoders) UnmarshalerDecodeValue(dc DecodeContext, vr bson
 		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
+	}
+
 	fn := val.Convert(tUnmarshaler).MethodByName("UnmarshalBSON")
 	errVal := fn.Call([]reflect.Value{reflect.ValueOf(src)})[0]
 	if !errVal.IsNil() {

bson/decoder_test.go

@@ -13,6 +13,7 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/assert"
 	"go.mongodb.org/mongo-driver/bson/bsoncodec"
 	"go.mongodb.org/mongo-driver/bson/bsonrw"
 	"go.mongodb.org/mongo-driver/bson/bsonrw/bsonrwtest"
@@ -21,7 +22,7 @@ import (
 )
 
 func TestBasicDecode(t *testing.T) {
-	for _, tc := range unmarshalingTestCases {
+	for _, tc := range unmarshalingTestCases() {
 		t.Run(tc.name, func(t *testing.T) {
 			got := reflect.New(tc.sType).Elem()
 			vr := bsonrw.NewBSONDocumentReader(tc.data)
@@ -30,34 +31,22 @@ func TestBasicDecode(t *testing.T) {
 			noerr(t, err)
 			err = decoder.DecodeValue(bsoncodec.DecodeContext{Registry: reg}, vr, got)
 			noerr(t, err)
-
-			if !reflect.DeepEqual(got.Addr().Interface(), tc.want) {
-				t.Errorf("Results do not match. got %+v; want %+v", got, tc.want)
-			}
+			assert.Equal(t, tc.want, got.Addr().Interface(), "Results do not match.")
 		})
 	}
 }
 
 func TestDecoderv2(t *testing.T) {
 	t.Run("Decode", func(t *testing.T) {
-		for _, tc := range unmarshalingTestCases {
+		for _, tc := range unmarshalingTestCases() {
 			t.Run(tc.name, func(t *testing.T) {
 				got := reflect.New(tc.sType).Interface()
 				vr := bsonrw.NewBSONDocumentReader(tc.data)
-				var reg *bsoncodec.Registry
-				if tc.reg != nil {
-					reg = tc.reg
-				} else {
-					reg = DefaultRegistry
-				}
-				dec, err := NewDecoderWithContext(bsoncodec.DecodeContext{Registry: reg}, vr)
+				dec, err := NewDecoderWithContext(bsoncodec.DecodeContext{Registry: DefaultRegistry}, vr)
 				noerr(t, err)
 				err = dec.Decode(got)
 				noerr(t, err)
-
-				if !reflect.DeepEqual(got, tc.want) {
-					t.Errorf("Results do not match. got %+v; want %+v", got, tc.want)
-				}
+				assert.Equal(t, tc.want, got, "Results do not match.")
 			})
 		}
 		t.Run("lookup error", func(t *testing.T) {
@@ -70,9 +59,7 @@ func TestDecoderv2(t *testing.T) {
 			noerr(t, err)
 			want := bsoncodec.ErrNoDecoder{Type: reflect.TypeOf(cdeih)}
 			got := dec.Decode(&cdeih)
-			if !cmp.Equal(got, want, cmp.Comparer(compareErrors)) {
-				t.Errorf("Received unexpected error. got %v; want %v", got, want)
-			}
+			assert.Equal(t, want, got, "Received unexpected error.")
 		})
 		t.Run("Unmarshaler", func(t *testing.T) {
 			testCases := []struct {
@@ -191,9 +178,7 @@ func TestDecoderv2(t *testing.T) {
 		err = dec.Decode(&got)
 		noerr(t, err)
 		want := foo{Item: "canvas", Qty: 4, Bonus: 2}
-		if !reflect.DeepEqual(got, want) {
-			t.Errorf("Results do not match. got %+v; want %+v", got, want)
-		}
+		assert.Equal(t, want, got, "Results do not match.")
 	})
 	t.Run("Reset", func(t *testing.T) {
 		vr1, vr2 := bsonrw.NewBSONDocumentReader([]byte{}), bsonrw.NewBSONDocumentReader([]byte{})

bson/unmarshal_test.go

@@ -10,64 +10,42 @@ import (
 	"reflect"
 	"testing"
 
-	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/assert"
 	"go.mongodb.org/mongo-driver/bson/bsoncodec"
 	"go.mongodb.org/mongo-driver/bson/bsonrw"
-	"go.mongodb.org/mongo-driver/internal/testutil/assert"
 	"go.mongodb.org/mongo-driver/x/bsonx/bsoncore"
 )
 
 func TestUnmarshal(t *testing.T) {
-	for _, tc := range unmarshalingTestCases {
+	for _, tc := range unmarshalingTestCases() {
 		t.Run(tc.name, func(t *testing.T) {
-			if tc.reg != nil {
-				t.Skip() // test requires custom registry
-			}
 			got := reflect.New(tc.sType).Interface()
 			err := Unmarshal(tc.data, got)
 			noerr(t, err)
-			if !cmp.Equal(got, tc.want) {
-				t.Errorf("Did not unmarshal as expected. got %v; want %v", got, tc.want)
-			}
+			assert.Equal(t, tc.want, got, "Did not unmarshal as expected.")
 		})
 	}
 }
 
 func TestUnmarshalWithRegistry(t *testing.T) {
-	for _, tc := range unmarshalingTestCases {
+	for _, tc := range unmarshalingTestCases() {
 		t.Run(tc.name, func(t *testing.T) {
-			var reg *bsoncodec.Registry
-			if tc.reg != nil {
-				reg = tc.reg
-			} else {
-				reg = DefaultRegistry
-			}
 			got := reflect.New(tc.sType).Interface()
-			err := UnmarshalWithRegistry(reg, tc.data, got)
+			err := UnmarshalWithRegistry(DefaultRegistry, tc.data, got)
 			noerr(t, err)
-			if !cmp.Equal(got, tc.want) {
-				t.Errorf("Did not unmarshal as expected. got %v; want %v", got, tc.want)
-			}
+			assert.Equal(t, tc.want, got, "Did not unmarshal as expected.")
 		})
 	}
 }
 
 func TestUnmarshalWithContext(t *testing.T) {
-	for _, tc := range unmarshalingTestCases {
+	for _, tc := range unmarshalingTestCases() {
 		t.Run(tc.name, func(t *testing.T) {
-			var reg *bsoncodec.Registry
-			if tc.reg != nil {
-				reg = tc.reg
-			} else {
-				reg = DefaultRegistry
-			}
-			dc := bsoncodec.DecodeContext{Registry: reg}
+			dc := bsoncodec.DecodeContext{Registry: DefaultRegistry}
 			got := reflect.New(tc.sType).Interface()
 			err := UnmarshalWithContext(dc, tc.data, got)
 			noerr(t, err)
-			if !cmp.Equal(got, tc.want) {
-				t.Errorf("Did not unmarshal as expected. got %v; want %v", got, tc.want)
-			}
+			assert.Equal(t, tc.want, got, "Did not unmarshal as expected.")
 		})
 	}
 }
@@ -80,9 +58,7 @@ func TestUnmarshalExtJSONWithRegistry(t *testing.T) {
 		err := UnmarshalExtJSONWithRegistry(DefaultRegistry, data, true, &got)
 		noerr(t, err)
 		want := teststruct{1}
-		if !cmp.Equal(got, want) {
-			t.Errorf("Did not unmarshal as expected. got %v; want %v", got, want)
-		}
+		assert.Equal(t, want, got, "Did not unmarshal as expected.")
 	})
 
 	t.Run("UnmarshalExtJSONInvalidInput", func(t *testing.T) {
@@ -165,9 +141,7 @@ func TestUnmarshalExtJSONWithContext(t *testing.T) {
 			dc := bsoncodec.DecodeContext{Registry: DefaultRegistry}
 			err := UnmarshalExtJSONWithContext(dc, tc.data, true, got)
 			noerr(t, err)
-			if !cmp.Equal(got, tc.want) {
-				t.Errorf("Did not unmarshal as expected. got %+v; want %+v", got, tc.want)
-			}
+			assert.Equal(t, tc.want, got, "Did not unmarshal as expected.")
 		})
 	}
 }