GODRIVER-3472: Address code review improvements

Author: kumarlokesh

bson/primitive_codecs_test.go

@@ -8,6 +8,7 @@ package bson
 
 import (
 	"bytes"
+	"encoding/binary"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -1116,3 +1117,50 @@ func compareDecimal128(d1, d2 Decimal128) bool {
 
 	return true
 }
+
+func TestSliceCodec(t *testing.T) {
+	t.Run("[]byte is treated as binary data", func(t *testing.T) {
+		type testStruct struct {
+			B []byte `bson:"b"`
+		}
+
+		testData := testStruct{B: []byte{0x01, 0x02, 0x03}}
+		data, err := Marshal(testData)
+		assert.Nil(t, err, "Marshal error: %v", err)
+		var doc D
+		err = Unmarshal(data, &doc)
+		assert.Nil(t, err, "Unmarshal error: %v", err)
+
+		offset := 4 + 1 + 2
+		length := int32(binary.LittleEndian.Uint32(data[offset:]))
+		offset += 4 // Skip length
+		subtype := data[offset]
+		offset++ // Skip subtype
+		dataBytes := data[offset : offset+int(length)]
+
+		assert.Equal(t, byte(0x00), subtype, "Expected binary subtype 0x00")
+		assert.Equal(t, []byte{0x01, 0x02, 0x03}, dataBytes, "Binary data mismatch")
+	})
+
+	t.Run("[]int8 is not treated as binary data", func(t *testing.T) {
+		type testStruct struct {
+			I []int8 `bson:"i"`
+		}
+		testData := testStruct{I: []int8{1, 2, 3}}
+		data, err := Marshal(testData)
+		assert.Nil(t, err, "Marshal error: %v", err)
+
+		offset := 4 // Skip document length
+		assert.Equal(t, byte(0x04), data[offset], "Expected array type (0x04), got: 0x%02x", data[offset])
+
+		var result struct {
+			I []int32 `bson:"i"`
+		}
+		err = Unmarshal(data, &result)
+		assert.Nil(t, err, "Unmarshal result error: %v", err)
+		assert.Equal(t, 3, len(result.I), "Expected array length 3")
+		assert.Equal(t, int32(1), result.I[0], "Array element 0 mismatch")
+		assert.Equal(t, int32(2), result.I[1], "Array element 1 mismatch")
+		assert.Equal(t, int32(3), result.I[2], "Array element 2 mismatch")
+	})
+}

bson/slice_codec.go

@@ -7,8 +7,10 @@
 package bson
 
 import (
+	"encoding/binary"
 	"errors"
 	"fmt"
+	"math"
 	"reflect"
 )
 
@@ -25,7 +27,7 @@ type sliceCodec struct {
 func (sc *sliceCodec) decodeVectorBinary(vr ValueReader, val reflect.Value) error {
 	elemType := val.Type().Elem()
 
-	if elemType != TInt8 && elemType != TFloat32 {
+	if elemType != tInt8 && elemType != tFloat32 {
 		return errNotAVectorBinary
 	}
 
@@ -39,14 +41,14 @@ func (sc *sliceCodec) decodeVectorBinary(vr ValueReader, val reflect.Value) erro
 	}
 
 	switch elemType {
-	case TInt8:
-		int8Slice, err := DecodeVectorInt8(data)
+	case tInt8:
+		int8Slice, err := decodeVectorInt8(data)
 		if err != nil {
 			return err
 		}
 		val.Set(reflect.ValueOf(int8Slice))
-	case TFloat32:
-		float32Slice, err := DecodeVectorFloat32(data)
+	case tFloat32:
+		float32Slice, err := decodeVectorFloat32(data)
 		if err != nil {
 			return err
 		}
@@ -66,8 +68,9 @@ func (sc *sliceCodec) EncodeValue(ec EncodeContext, vw ValueWriter, val reflect.
 		return vw.WriteNull()
 	}
 
-	// Treat []byte as binary data, but skip for []int8 since it's a different type
-	// even though byte is an alias for uint8 which has the same underlying type as int8
+	// Treat []byte as binary data, but skip for []int8 since it's a different type.
+	// Even though byte is an alias for uint8 which has the same underlying type as int8,
+	// we want to maintain the semantic difference between []byte (binary data) and []int8 (array of integers).
 	if val.Type().Elem() == tByte && val.Type() != reflect.TypeOf([]int8{}) {
 		byteSlice := make([]byte, val.Len())
 		reflect.Copy(reflect.ValueOf(byteSlice), val)
@@ -137,12 +140,6 @@ func (sc *sliceCodec) DecodeValue(dc DecodeContext, vr ValueReader, val reflect.
 		return ValueDecoderError{Name: "SliceDecodeValue", Kinds: []reflect.Kind{reflect.Slice}, Received: val}
 	}
 
-	if vr.Type() == TypeBinary {
-		if err := sc.decodeVectorBinary(vr, val); err != errNotAVectorBinary {
-			return err
-		}
-	}
-
 	switch vrType := vr.Type(); vrType {
 	case TypeArray:
 	case TypeNull:
@@ -156,6 +153,16 @@ func (sc *sliceCodec) DecodeValue(dc DecodeContext, vr ValueReader, val reflect.
 			return fmt.Errorf("cannot decode document into %s", val.Type())
 		}
 	case TypeBinary:
+		// First try to decode as a vector binary
+		err := sc.decodeVectorBinary(vr, val)
+		if err == nil {
+			return nil // Successfully decoded as vector
+		}
+		if err != errNotAVectorBinary {
+			return err // Return any actual errors
+		}
+
+		// If not a vector binary, handle as regular binary data
 		if val.Type().Elem() != tByte {
 			return fmt.Errorf("SliceDecodeValue can only decode a binary into a byte array, got %v", vrType)
 		}
@@ -215,3 +222,62 @@ func (sc *sliceCodec) DecodeValue(dc DecodeContext, vr ValueReader, val reflect.
 
 	return nil
 }
+
+// decodeVectorInt8 decodes a BSON Vector binary value (subtype 9) into a []int8 slice.
+// The binary data should be in the format: [<vector type> <padding> <data>]
+// For int8 vectors, the vector type is Int8Vector (0x03).
+func decodeVectorInt8(data []byte) ([]int8, error) {
+	if len(data) < 2 {
+		return nil, fmt.Errorf("insufficient bytes to decode vector: expected at least 2 bytes")
+	}
+
+	vectorType := data[0]
+	if vectorType != Int8Vector {
+		return nil, fmt.Errorf("invalid vector type: expected int8 vector (0x%02x), got 0x%02x", Int8Vector, vectorType)
+	}
+
+	if padding := data[1]; padding != 0 {
+		return nil, fmt.Errorf("invalid vector: padding byte must be 0")
+	}
+
+	values := make([]int8, 0, len(data)-2)
+	for i := 2; i < len(data); i++ {
+		values = append(values, int8(data[i]))
+	}
+
+	return values, nil
+}
+
+// decodeVectorFloat32 decodes a BSON Vector binary value (subtype 9) into a []float32 slice.
+// The binary data should be in the format: [<vector type> <padding> <data>]
+// For float32 vectors, the vector type is Float32Vector (0x27) and data must be a multiple of 4 bytes.
+func decodeVectorFloat32(data []byte) ([]float32, error) {
+	if len(data) < 2 {
+		return nil, fmt.Errorf("insufficient bytes to decode vector: expected at least 2 bytes")
+	}
+
+	vectorType := data[0]
+	if vectorType != Float32Vector {
+		return nil, fmt.Errorf("invalid vector type: expected float32 vector (0x%02x), got 0x%02x", Float32Vector, vectorType)
+	}
+
+	if padding := data[1]; padding != 0 {
+		return nil, fmt.Errorf("invalid vector: padding byte must be 0")
+	}
+
+	floatData := data[2:]
+	if len(floatData)%4 != 0 {
+		return nil, fmt.Errorf("invalid float32 vector: data length must be a multiple of 4")
+	}
+
+	values := make([]float32, 0, len(floatData)/4)
+	for i := 0; i < len(floatData); i += 4 {
+		if i+4 > len(floatData) {
+			return nil, fmt.Errorf("invalid float32 vector: truncated data")
+		}
+		bits := binary.LittleEndian.Uint32(floatData[i : i+4])
+		values = append(values, math.Float32frombits(bits))
+	}
+
+	return values, nil
+}

bson/types.go

@@ -77,7 +77,9 @@ const (
 )
 
 var tBool = reflect.TypeOf(false)
+var tFloat32 = reflect.TypeOf(float32(0))
 var tFloat64 = reflect.TypeOf(float64(0))
+var tInt8 = reflect.TypeOf(int8(0))
 var tInt32 = reflect.TypeOf(int32(0))
 var tInt64 = reflect.TypeOf(int64(0))
 var tString = reflect.TypeOf("")

bson/vector.go

@@ -11,7 +11,6 @@ import (
 	"errors"
 	"fmt"
 	"math"
-	"reflect"
 )
 
 // BSON binary vector types as described in https://bsonspec.org/spec.html.
@@ -29,13 +28,6 @@ var (
 	errNotAVectorBinary       = errors.New("not a vector binary")
 )
 
-var (
-	// TInt8 is the reflect.Type for int8
-	TInt8 = reflect.TypeOf(int8(0))
-	// TFloat32 is the reflect.Type for float32
-	TFloat32 = reflect.TypeOf(float32(0))
-)
-
 type vectorTypeError struct {
 	Method string
 	Type   byte
@@ -275,60 +267,3 @@ func newBitVector(b []byte) (Vector, error) {
 	}
 	return NewPackedBitVector(b[1:], b[0])
 }
-
-// DecodeVectorInt8 decodes a BSON Vector binary value (subtype 9) into a []int8 slice.
-// The binary data should be in the format: [<vector type> <padding> <data>]
-// For int8 vectors, the vector type is 0x01.
-func DecodeVectorInt8(data []byte) ([]int8, error) {
-	if len(data) < 2 {
-		return nil, errors.New("insufficient bytes to decode vector: expected at least 2 bytes")
-	}
-
-	vectorType := data[0]
-	if vectorType != 0x01 { // Int8Vector
-		return nil, errors.New("invalid vector type: expected int8 vector (0x01)")
-	}
-
-	if padding := data[1]; padding != 0 {
-		return nil, errors.New("invalid vector: padding byte must be 0")
-	}
-	values := make([]int8, 0, len(data)-2)
-	for i := 2; i < len(data); i++ {
-		values = append(values, int8(data[i]))
-	}
-
-	return values, nil
-}
-
-// DecodeVectorFloat32 decodes a BSON Vector binary value (subtype 9) into a []float32 slice.
-// The binary data should be in the format: [<vector type> <padding> <data>]
-// For float32 vectors, the vector type is 0x02 and data must be a multiple of 4 bytes.
-func DecodeVectorFloat32(data []byte) ([]float32, error) {
-	if len(data) < 2 {
-		return nil, errors.New("insufficient bytes to decode vector: expected at least 2 bytes")
-	}
-
-	vectorType := data[0]
-	if vectorType != 0x02 { // Float32Vector
-		return nil, errors.New("invalid vector type: expected float32 vector (0x02)")
-	}
-
-	if padding := data[1]; padding != 0 {
-		return nil, errors.New("invalid vector: padding byte must be 0")
-	}
-	floatData := data[2:]
-	if len(floatData)%4 != 0 {
-		return nil, errors.New("invalid float32 vector: data length must be a multiple of 4")
-	}
-
-	values := make([]float32, 0, len(floatData)/4)
-	for i := 0; i < len(floatData); i += 4 {
-		if i+4 > len(floatData) {
-			return nil, errors.New("invalid float32 vector: truncated data")
-		}
-		bits := binary.LittleEndian.Uint32(floatData[i : i+4])
-		values = append(values, math.Float32frombits(bits))
-	}
-
-	return values, nil
-}