Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GODRIVER-2896 Add IsZero to BSON RawValue #1332

Merged
merged 8 commits into from
Jul 28, 2023
Merged

GODRIVER-2896 Add IsZero to BSON RawValue #1332

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
17bb347
GODRIVER-2896 Add IsZero to BSON RawValue
prestonvasquez Jul 26, 2023
a3472c9
GODRIVER-2896 Tighten invalid type error conditions
prestonvasquez Jul 26, 2023
3970d95
GODRIVER-2896 Clean up IsZero comment
prestonvasquez Jul 26, 2023
3bfa20d
Update bson/raw_value.go
prestonvasquez Jul 27, 2023
4938834
GODRIVER-2896 Add IsValid method to bsontype.Type
prestonvasquez Jul 27, 2023
63d9d8d
GODRIVER-2896 Use more obvious error
prestonvasquez Jul 28, 2023
11923dd
GODRIVER-2896 Use switch condition in IsValid type method
prestonvasquez Jul 28, 2023
e0114b9
GODRIVER-2896 Correct comment
prestonvasquez Jul 28, 2023
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
20 changes: 17 additions & 3 deletions bson/primitive_codecs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,29 @@ func (pc PrimitiveCodecs) RegisterPrimitiveCodecs(rb *bsoncodec.RegistryBuilder)

// RawValueEncodeValue is the ValueEncoderFunc for RawValue.
//
// Deprecated: Use bson.NewRegistry to get a registry with all primitive encoders and decoders
// registered.
// If the RawValue's Type is "invalid" and the RawValue's Value is not empty or
// nil, then this method will return an error.
//
// Deprecated: Use bson.NewRegistry to get a registry with all primitive
// encoders and decoders registered.
func (PrimitiveCodecs) RawValueEncodeValue(_ bsoncodec.EncodeContext, vw bsonrw.ValueWriter, val reflect.Value) error {
if !val.IsValid() || val.Type() != tRawValue {
return bsoncodec.ValueEncoderError{Name: "RawValueEncodeValue", Types: []reflect.Type{tRawValue}, Received: val}
return bsoncodec.ValueEncoderError{
Name: "RawValueEncodeValue",
Types: []reflect.Type{tRawValue},
Received: val,
}
}

rawvalue := val.Interface().(RawValue)

if rawvalue.Type.String() == "invalid" {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a magic string that depends on the logic in the bsontype.Type's String() method, here. Do we want to create a function to contain this logic in the bsontype package? Something like "Type.Valid()"?

Copy link
Member

Choose a reason for hiding this comment

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

That seems reasonable to me

Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding a "check validity" method sounds like a good idea. I recommend naming it IsValid to stick with the convention provided by reflect and (some) other methods in the Go driver.

return bsoncodec.ValueEncoderError{
Name: "RawValueEncodeValue",
Types: []reflect.Type{tRawValue},
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This error value will result in an error message like:

"RawValueEncodeValue can only encode valid reflect.Kind, but got "

which doesn't really describe the reason for the error. Instead, we should return an error that specifically describes the error condition. Something like:

fmt.Errorf("the RawValue Type specifies an invalid BSON type: %#x", rawvalue.Type)
// E.g. error: "the RawValue Type specifies an invalid BSON type: 0x1e"

Copy link
Collaborator Author

@prestonvasquez prestonvasquez Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The error should result in a message like this:

RawValueEncodeValue can only encode valid bson.RawValue, but got invalid

Do you have an example that excludes in the invalid part? I would expect any invalidly typed RawValue to return "invalid" because of the bsontype.Type String method. See here for the error logic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're correct, I misunderstood the logic in ValueEncoderError.Error (and didn't test it 🤦 ). Nevertheless, I'm concerned that users won't know what to fix if they got that error message because it's not immediately obvious that the error is caused by RawValue.Type being invalid. We should try to provide an error message that gives users a clear action to resolve the error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

}

return bsonrw.Copier{}.CopyValueFromBytes(vw, rawvalue.Type, rawvalue.Value)
}

Expand Down
44 changes: 44 additions & 0 deletions bson/primitive_codecs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ func compareErrors(err1, err2 error) bool {
}

func TestDefaultValueEncoders(t *testing.T) {
t.Parallel()

var pc PrimitiveCodecs

var wrong = func(string, string) string { return "wrong" }
Expand Down Expand Up @@ -107,6 +109,34 @@ func TestDefaultValueEncoders(t *testing.T) {
bsonrwtest.WriteDouble,
nil,
},
{
"RawValue Type is zero with non-zero value",
RawValue{
Type: 0x00,
Value: bsoncore.AppendDouble(nil, 3.14159),
},
nil,
nil,
bsonrwtest.Nothing,
bsoncodec.ValueEncoderError{
Name: "RawValueEncodeValue",
Types: []reflect.Type{tRawValue},
},
},
{
"RawValue Type is invalid",
RawValue{
Type: 0x8F,
Value: bsoncore.AppendDouble(nil, 3.14159),
},
nil,
nil,
bsonrwtest.Nothing,
bsoncodec.ValueEncoderError{
Name: "RawValueEncodeValue",
Types: []reflect.Type{tRawValue},
},
},
},
},
{
Expand Down Expand Up @@ -166,9 +196,17 @@ func TestDefaultValueEncoders(t *testing.T) {
}

for _, tc := range testCases {
tc := tc // Capture the range variable

t.Run(tc.name, func(t *testing.T) {
t.Parallel()

for _, subtest := range tc.subtests {
subtest := subtest // Capture the range variable

t.Run(subtest.name, func(t *testing.T) {
t.Parallel()

var ec bsoncodec.EncodeContext
if subtest.ectx != nil {
ec = *subtest.ectx
Expand All @@ -192,6 +230,8 @@ func TestDefaultValueEncoders(t *testing.T) {
}

t.Run("success path", func(t *testing.T) {
t.Parallel()

oid := primitive.NewObjectID()
oids := []primitive.ObjectID{primitive.NewObjectID(), primitive.NewObjectID(), primitive.NewObjectID()}
var str = new(string)
Expand Down Expand Up @@ -426,7 +466,11 @@ func TestDefaultValueEncoders(t *testing.T) {
}

for _, tc := range testCases {
tc := tc // Capture the range variable

t.Run(tc.name, func(t *testing.T) {
t.Parallel()

b := make(bsonrw.SliceWriter, 0, 512)
vw, err := bsonrw.NewBSONValueWriter(&b)
noerr(t, err)
Expand Down
7 changes: 7 additions & 0 deletions bson/raw_value.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,13 @@ type RawValue struct {
r *bsoncodec.Registry
}

// IsZero reports whether the RawValue is zero, i.e. no data is present on
// the RawValue. Since a raw value is defined by the Type and Value, the
// bsoncodec.Registry will not be considered when IsZero is called.
func (rv RawValue) IsZero() bool {
return rv.Type == 0x00 && len(rv.Value) == 0
}

// Unmarshal deserializes BSON into the provided val. If RawValue cannot be unmarshaled into val, an
// error is returned. This method will use the registry used to create the RawValue, if the RawValue
// was created from partial BSON processing, or it will use the default registry. Users wishing to
Expand Down
84 changes: 84 additions & 0 deletions bson/raw_value_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,19 @@ 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/x/bsonx/bsoncore"
)

func TestRawValue(t *testing.T) {
t.Parallel()

t.Run("Unmarshal", func(t *testing.T) {
t.Parallel()

t.Run("Uses registry attached to value", func(t *testing.T) {
t.Parallel()

reg := bsoncodec.NewRegistryBuilder().Build()
val := RawValue{Type: bsontype.String, Value: bsoncore.AppendString(nil, "foobar"), r: reg}
var s string
Expand All @@ -29,6 +36,8 @@ func TestRawValue(t *testing.T) {
}
})
t.Run("Uses default registry if no registry attached", func(t *testing.T) {
t.Parallel()

want := "foobar"
val := RawValue{Type: bsontype.String, Value: bsoncore.AppendString(nil, want)}
var got string
Expand All @@ -40,7 +49,11 @@ func TestRawValue(t *testing.T) {
})
})
t.Run("UnmarshalWithRegistry", func(t *testing.T) {
t.Parallel()

t.Run("Returns error when registry is nil", func(t *testing.T) {
t.Parallel()

want := ErrNilRegistry
var val RawValue
got := val.UnmarshalWithRegistry(nil, &D{})
Expand All @@ -49,6 +62,8 @@ func TestRawValue(t *testing.T) {
}
})
t.Run("Returns lookup error", func(t *testing.T) {
t.Parallel()

reg := bsoncodec.NewRegistryBuilder().Build()
var val RawValue
var s string
Expand All @@ -59,6 +74,8 @@ func TestRawValue(t *testing.T) {
}
})
t.Run("Returns DecodeValue error", func(t *testing.T) {
t.Parallel()

reg := NewRegistryBuilder().Build()
val := RawValue{Type: bsontype.Double, Value: bsoncore.AppendDouble(nil, 3.14159)}
var s string
Expand All @@ -69,6 +86,8 @@ func TestRawValue(t *testing.T) {
}
})
t.Run("Success", func(t *testing.T) {
t.Parallel()

reg := NewRegistryBuilder().Build()
want := float64(3.14159)
val := RawValue{Type: bsontype.Double, Value: bsoncore.AppendDouble(nil, want)}
Expand All @@ -81,7 +100,11 @@ func TestRawValue(t *testing.T) {
})
})
t.Run("UnmarshalWithContext", func(t *testing.T) {
t.Parallel()

t.Run("Returns error when DecodeContext is nil", func(t *testing.T) {
t.Parallel()

want := ErrNilContext
var val RawValue
got := val.UnmarshalWithContext(nil, &D{})
Expand All @@ -90,6 +113,8 @@ func TestRawValue(t *testing.T) {
}
})
t.Run("Returns lookup error", func(t *testing.T) {
t.Parallel()

dc := bsoncodec.DecodeContext{Registry: bsoncodec.NewRegistryBuilder().Build()}
var val RawValue
var s string
Expand All @@ -100,6 +125,8 @@ func TestRawValue(t *testing.T) {
}
})
t.Run("Returns DecodeValue error", func(t *testing.T) {
t.Parallel()

dc := bsoncodec.DecodeContext{Registry: NewRegistryBuilder().Build()}
val := RawValue{Type: bsontype.Double, Value: bsoncore.AppendDouble(nil, 3.14159)}
var s string
Expand All @@ -110,6 +137,8 @@ func TestRawValue(t *testing.T) {
}
})
t.Run("Success", func(t *testing.T) {
t.Parallel()

dc := bsoncodec.DecodeContext{Registry: NewRegistryBuilder().Build()}
want := float64(3.14159)
val := RawValue{Type: bsontype.Double, Value: bsoncore.AppendDouble(nil, want)}
Expand All @@ -121,4 +150,59 @@ func TestRawValue(t *testing.T) {
}
})
})

t.Run("IsZero", func(t *testing.T) {
t.Parallel()

tests := []struct {
name string
val RawValue
want bool
}{
{
name: "empty",
val: RawValue{},
want: true,
},
{
name: "zero type but non-zero value",
val: RawValue{
Type: 0x00,
Value: bsoncore.AppendInt32(nil, 0),
},
want: false,
},
{
name: "zero type and zero value",
val: RawValue{
Type: 0x00,
Value: bsoncore.AppendInt32(nil, 0),
},
},
{
name: "non-zero type and non-zero value",
val: RawValue{
Type: bsontype.String,
Value: bsoncore.AppendString(nil, "foobar"),
},
want: false,
},
{
name: "non-zero type and zero value",
val: RawValue{
Type: bsontype.String,
Value: bsoncore.AppendString(nil, "foobar"),
},
},
}

for _, tt := range tests {
tt := tt // Capture the range variable
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

assert.Equal(t, tt.want, tt.val.IsZero())
})
}
})
}