GODRIVER-1576 fix non-addr type with pointer implementation for bson (#379)

Author: iwysiu

bson/bsoncodec/cond_addr_codec.go

@@ -0,0 +1,63 @@
+// Copyright (C) MongoDB, Inc. 2017-present.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License. You may obtain
+// a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
+
+package bsoncodec
+
+import (
+	"reflect"
+
+	"go.mongodb.org/mongo-driver/bson/bsonrw"
+)
+
+// condAddrEncoder is the encoder used when a pointer to the encoding value has an encoder.
+type condAddrEncoder struct {
+	canAddrEnc ValueEncoder
+	elseEnc    ValueEncoder
+}
+
+var _ ValueEncoder = (*condAddrEncoder)(nil)
+
+// newCondAddrEncoder returns an condAddrEncoder.
+func newCondAddrEncoder(canAddrEnc, elseEnc ValueEncoder) *condAddrEncoder {
+	encoder := condAddrEncoder{canAddrEnc: canAddrEnc, elseEnc: elseEnc}
+	return &encoder
+}
+
+// EncodeValue is the ValueEncoderFunc for a value that may be addressable.
+func (cae *condAddrEncoder) EncodeValue(ec EncodeContext, vw bsonrw.ValueWriter, val reflect.Value) error {
+	if val.CanAddr() {
+		return cae.canAddrEnc.EncodeValue(ec, vw, val)
+	}
+	if cae.elseEnc != nil {
+		return cae.elseEnc.EncodeValue(ec, vw, val)
+	}
+	return ErrNoEncoder{Type: val.Type()}
+}
+
+// condAddrDecoder is the decoder used when a pointer to the value has a decoder.
+type condAddrDecoder struct {
+	canAddrDec ValueDecoder
+	elseDec    ValueDecoder
+}
+
+var _ ValueDecoder = (*condAddrDecoder)(nil)
+
+// newCondAddrDecoder returns an CondAddrDecoder.
+func newCondAddrDecoder(canAddrDec, elseDec ValueDecoder) *condAddrDecoder {
+	decoder := condAddrDecoder{canAddrDec: canAddrDec, elseDec: elseDec}
+	return &decoder
+}
+
+// DecodeValue is the ValueDecoderFunc for a value that may be addressable.
+func (cad *condAddrDecoder) DecodeValue(dc DecodeContext, vr bsonrw.ValueReader, val reflect.Value) error {
+	if val.CanAddr() {
+		return cad.canAddrDec.DecodeValue(dc, vr, val)
+	}
+	if cad.elseDec != nil {
+		return cad.elseDec.DecodeValue(dc, vr, val)
+	}
+	return ErrNoDecoder{Type: val.Type()}
+}

bson/bsoncodec/cond_addr_codec_test.go

@@ -0,0 +1,97 @@
+// Copyright (C) MongoDB, Inc. 2017-present.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License. You may obtain
+// a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
+
+package bsoncodec
+
+import (
+	"reflect"
+	"testing"
+
+	"go.mongodb.org/mongo-driver/bson/bsonrw"
+	"go.mongodb.org/mongo-driver/bson/bsonrw/bsonrwtest"
+	"go.mongodb.org/mongo-driver/internal/testutil/assert"
+)
+
+func TestCondAddrCodec(t *testing.T) {
+	var inner int
+	canAddrVal := reflect.ValueOf(&inner)
+	addressable := canAddrVal.Elem()
+	unaddressable := reflect.ValueOf(inner)
+	rw := &bsonrwtest.ValueReaderWriter{}
+
+	t.Run("addressEncode", func(t *testing.T) {
+		invoked := 0
+		encode1 := func(EncodeContext, bsonrw.ValueWriter, reflect.Value) error {
+			invoked = 1
+			return nil
+		}
+		encode2 := func(EncodeContext, bsonrw.ValueWriter, reflect.Value) error {
+			invoked = 2
+			return nil
+		}
+		condEncoder := newCondAddrEncoder(ValueEncoderFunc(encode1), ValueEncoderFunc(encode2))
+
+		testCases := []struct {
+			name    string
+			val     reflect.Value
+			invoked int
+		}{
+			{"canAddr", addressable, 1},
+			{"else", unaddressable, 2},
+		}
+		for _, tc := range testCases {
+			t.Run(tc.name, func(t *testing.T) {
+				err := condEncoder.EncodeValue(EncodeContext{}, rw, tc.val)
+				assert.Nil(t, err, "CondAddrEncoder error: %v", err)
+
+				assert.Equal(t, invoked, tc.invoked, "Expected function %v to be called, called %v", tc.invoked, invoked)
+			})
+		}
+
+		t.Run("error", func(t *testing.T) {
+			errEncoder := newCondAddrEncoder(ValueEncoderFunc(encode1), nil)
+			err := errEncoder.EncodeValue(EncodeContext{}, rw, unaddressable)
+			want := ErrNoEncoder{Type: unaddressable.Type()}
+			assert.Equal(t, err, want, "expected error %v, got %v", want, err)
+		})
+	})
+	t.Run("addressDecode", func(t *testing.T) {
+		invoked := 0
+		decode1 := func(dctx DecodeContext, vr bsonrw.ValueReader, val reflect.Value) error {
+			invoked = 1
+			return nil
+		}
+		decode2 := func(dctx DecodeContext, vr bsonrw.ValueReader, val reflect.Value) error {
+			invoked = 2
+			return nil
+		}
+		condDecoder := newCondAddrDecoder(ValueDecoderFunc(decode1), ValueDecoderFunc(decode2))
+
+		testCases := []struct {
+			name    string
+			val     reflect.Value
+			invoked int
+		}{
+			{"canAddr", addressable, 1},
+			{"else", unaddressable, 2},
+		}
+		for _, tc := range testCases {
+			t.Run(tc.name, func(t *testing.T) {
+				err := condDecoder.DecodeValue(DecodeContext{}, rw, tc.val)
+				assert.Nil(t, err, "CondAddrDecoder error: %v", err)
+
+				assert.Equal(t, invoked, tc.invoked, "Expected function %v to be called, called %v", tc.invoked, invoked)
+			})
+		}
+
+		t.Run("error", func(t *testing.T) {
+			errDecoder := newCondAddrDecoder(ValueDecoderFunc(decode1), nil)
+			err := errDecoder.DecodeValue(DecodeContext{}, rw, unaddressable)
+			want := ErrNoDecoder{Type: unaddressable.Type()}
+			assert.Equal(t, err, want, "expected error %v, got %v", want, err)
+		})
+	})
+}

bson/bsoncodec/registry.go

@@ -325,7 +325,7 @@ func (r *Registry) LookupEncoder(t reflect.Type) (ValueEncoder, error) {
 		return enc, nil
 	}
 
-	enc, found = r.lookupInterfaceEncoder(t)
+	enc, found = r.lookupInterfaceEncoder(t, true)
 	if found {
 		r.mu.Lock()
 		r.typeEncoders[t] = enc
@@ -359,14 +359,23 @@ func (r *Registry) lookupTypeEncoder(t reflect.Type) (ValueEncoder, bool) {
 	return enc, found
 }
 
-func (r *Registry) lookupInterfaceEncoder(t reflect.Type) (ValueEncoder, bool) {
+func (r *Registry) lookupInterfaceEncoder(t reflect.Type, allowAddr bool) (ValueEncoder, bool) {
 	if t == nil {
 		return nil, false
 	}
 	for _, ienc := range r.interfaceEncoders {
-		if t.Implements(ienc.i) || reflect.PtrTo(t).Implements(ienc.i) {
+		if t.Implements(ienc.i) {
 			return ienc.ve, true
 		}
+		if allowAddr && t.Kind() != reflect.Ptr && reflect.PtrTo(t).Implements(ienc.i) {
+			// if *t implements an interface, this will catch if t implements an interface further ahead
+			// in interfaceEncoders
+			defaultEnc, found := r.lookupInterfaceEncoder(t, false)
+			if !found {
+				defaultEnc, _ = r.kindEncoders[t.Kind()]
+			}
+			return newCondAddrEncoder(ienc.ve, defaultEnc), true
+		}
 	}
 	return nil, false
 }
@@ -397,7 +406,7 @@ func (r *Registry) LookupDecoder(t reflect.Type) (ValueDecoder, error) {
 		return dec, nil
 	}
 
-	dec, found = r.lookupInterfaceDecoder(t)
+	dec, found = r.lookupInterfaceDecoder(t, true)
 	if found {
 		r.mu.Lock()
 		r.typeDecoders[t] = dec
@@ -424,13 +433,20 @@ func (r *Registry) lookupTypeDecoder(t reflect.Type) (ValueDecoder, bool) {
 	return dec, found
 }
 
-func (r *Registry) lookupInterfaceDecoder(t reflect.Type) (ValueDecoder, bool) {
+func (r *Registry) lookupInterfaceDecoder(t reflect.Type, allowAddr bool) (ValueDecoder, bool) {
 	for _, idec := range r.interfaceDecoders {
-		if !t.Implements(idec.i) && !reflect.PtrTo(t).Implements(idec.i) {
-			continue
+		if t.Implements(idec.i) {
+			return idec.vd, true
+		}
+		if allowAddr && t.Kind() != reflect.Ptr && reflect.PtrTo(t).Implements(idec.i) {
+			// if *t implements an interface, this will catch if t implements an interface further ahead
+			// in interfaceDecoders
+			defaultDec, found := r.lookupInterfaceDecoder(t, false)
+			if !found {
+				defaultDec, _ = r.kindDecoders[t.Kind()]
+			}
+			return newCondAddrDecoder(idec.vd, defaultDec), true
 		}
-
-		return idec.vd, true
 	}
 	return nil, false
 }

bson/bsoncodec/registry_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"go.mongodb.org/mongo-driver/bson/bsonrw"
 	"go.mongodb.org/mongo-driver/bson/bsontype"
+	"go.mongodb.org/mongo-driver/internal/testutil/assert"
 )
 
 func TestRegistry(t *testing.T) {
@@ -251,15 +252,6 @@ func TestRegistry(t *testing.T) {
 					nil,
 					false,
 				},
-				{
-					// lookup a type whose pointer implements an interface and expect that the registered hook is
-					// returned
-					"interface implementation with hook (pointer)",
-					ti3Impl,
-					fc3,
-					nil,
-					false,
-				},
 				{
 					// lookup a pointer to a type where the pointer implements an interface and expect that the
 					// registered hook is returned
@@ -351,6 +343,36 @@ func TestRegistry(t *testing.T) {
 					})
 				})
 			}
+			// lookup a type whose pointer implements an interface and expect that the registered hook is
+			// returned
+			t.Run("interface implementation with hook (pointer)", func(t *testing.T) {
+				t.Run("Encoder", func(t *testing.T) {
+					gotEnc, err := reg.LookupEncoder(ti3Impl)
+					assert.Nil(t, err, "LookupEncoder error: %v", err)
+
+					cae, ok := gotEnc.(*condAddrEncoder)
+					assert.True(t, ok, "Expected CondAddrEncoder, got %T", gotEnc)
+					if !cmp.Equal(cae.canAddrEnc, fc3, allowunexported, cmp.Comparer(comparepc)) {
+						t.Errorf("expected canAddrEnc %v, got %v", cae.canAddrEnc, fc3)
+					}
+					if !cmp.Equal(cae.elseEnc, fsc, allowunexported, cmp.Comparer(comparepc)) {
+						t.Errorf("expected elseEnc %v, got %v", cae.elseEnc, fsc)
+					}
+				})
+				t.Run("Decoder", func(t *testing.T) {
+					gotDec, err := reg.LookupDecoder(ti3Impl)
+					assert.Nil(t, err, "LookupDecoder error: %v", err)
+
+					cad, ok := gotDec.(*condAddrDecoder)
+					assert.True(t, ok, "Expected CondAddrDecoder, got %T", gotDec)
+					if !cmp.Equal(cad.canAddrDec, fc3, allowunexported, cmp.Comparer(comparepc)) {
+						t.Errorf("expected canAddrDec %v, got %v", cad.canAddrDec, fc3)
+					}
+					if !cmp.Equal(cad.elseDec, fsc, allowunexported, cmp.Comparer(comparepc)) {
+						t.Errorf("expected elseDec %v, got %v", cad.elseDec, fsc)
+					}
+				})
+			})
 		})
 	})
 	t.Run("Type Map", func(t *testing.T) {