GODRIVER-2896 Add IsZero to BSON RawValue

[prestonvasquez]

GODRIVER-2896

Summary

This PR proposes an "IsZero" method for the BSON "RawValue" type, which will be used by the EncodeValue-implementing types to "skip" encoding when the "omitempty" tag is present. Otherwise, this PR proposes that the PrimitiveCodecs' "RawValueEncodeValue" method should return an error when the RawValue's type is "invalid".

For clarity, after these changes the use case in the "Background & Motivation" section WILL NOT result in a runtime error. However, if the user removes the "omitempty" tag from the "Likes" field, like this:

type P struct {
	Name  string        `bson:"name,omitempty"`
	Likes bson.RawValue `bson:"likes"`
}

then the following error will be thrown:

the RawValue Type specifies an invalid BSON type: 0x0

Background & Motivation

A user has reported an issue where the following code will result in a runtime error:

type P struct {
	Name  string        `bson:"name,omitempty"`
	Likes bson.RawValue `bson:"likes,omitempty"`
}

func main() {
	bytes, err := bson.Marshal(&P{Name: "lilei"})
	
	if err != nil {
		panic(fmt.Sprintf("failed to meep: %v", err))
	}

	p := new(P)
	
	if err = bson.Unmarshal(bytes, p); err != nil {
		panic(err)
	}
}

The error looks like this:

unmarshal error document is invalid, end byte is at 28, but null byte found at 21

This occurs because an empty bson.RawValue has an invalid type, specifically the 0x00 (null) type. When the PrimitiveCodecs' "RawValueEncodeValue" method tries to encode this invalid type, it incidentally puts this "null" byte in the buffer by the "bsoncore.AppendHeader" function, here.

[prestonvasquez]

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()"?

[blink1073]

That seems reasonable to me

[matthewdale]

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.

[matthewdale]

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"

[prestonvasquez]

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.

[matthewdale]

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.

[prestonvasquez]

Done!

[matthewdale]

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.

[blink1073]

LGTM!

[matthewdale]

It's generally unexpected to rely on the exact output of String because it's primarily for debugging. We should either use another switch statement (e.g. readpref.Mode.IsValid), make an underlying function that both String and IsValid share (e.g. stringOK() (string, bool)), or use logic based on valid value ranges, like:

func (bt Type) IsValid() bool {
	return (bt >= 0x01 && bt <= 0x13) || bt == MinKey || bt == MaxKey
}

[prestonvasquez]

Oh good idea. I think I lean more toward a switch condition, it's explicitness and readability make it easier to understand and maintain over time.

[matthewdale]

Looks good 👍