GODRIVER-2796 Update ObjectIDAsHexString behavior.
#1694
Conversation
API Change Report./bsonincompatible changes(*DecodeContext).BinaryAsSlice: removed compatible changes(*Decoder).ObjectIDAsHexString: added |
| @@ -75,6 +76,30 @@ func NewRespectNilValuesMgoRegistry() *Registry { | |||
| return reg | |||
| } | |||
|
|
|||
| func mgoStringDecodeValue(dc DecodeContext, vr ValueReader, val reflect.Value) error { | |||
There was a problem hiding this comment.
Keep the compatible mgo behavior.
bson/string_codec.go
Outdated
| // TODO(GODRIVER-2796): Return an error here instead of decoding to a garbled string. | ||
| byteArray := [12]byte(oid) | ||
| str = string(byteArray[:]) | ||
| return emptyValue, fmt.Errorf("decoding an object ID to a hexadecimal string is disabled by default") |
There was a problem hiding this comment.
As noted in the GODRIVER-2796 the underlying bytes are an object id. What was the original intention of this? Is it because the hex representation requires more bytes? Here is a playground illustrating this point: https://go.dev/play/p/6Iif2VPXb3J
If we determine this is not appropriate behavior, we should update the error to be more clear about what we are preventing:
decoding an object ID to a non-hexadecimal string representation is not supported
There was a problem hiding this comment.
I believe the original intent of this code in the Go Driver was for mgo compatibility. See an example of the mgo behavior here. That is supported by the context of PR #213, which added the code.
However, it's not clear why that behavior was supported by default, either in mgo or in the Go Driver. My best guess is that it was an oversight and unintentional in both cases because the result of interpreting object ID bytes as a string is almost always invalid UTF-8, which could cause issues with lots of systems.
Edit: The mgo bson.ObjectId underlying type is actually a string instead of a [12]byte, which makes it easier to unintentionally convert object ID values to a string during unmarshaling. Interestingly, both the Go Driver and mgo also support storing generic binary BSON data as a string. There's actually explicit code in mgo to decode binary as a string (see here), so that seems intentional.
| @@ -14,7 +14,6 @@ import ( | |||
| // stringCodec is the Codec used for string values. | |||
| type stringCodec struct { | |||
| // DecodeObjectIDAsHex specifies if object IDs should be decoded as their hex representation. | |||
| // If false, a string made from the raw object ID bytes will be used. Defaults to true. | |||
There was a problem hiding this comment.
We should note that a false value will now result in a decoding error.
There was a problem hiding this comment.
I've added the comments in DecodeContext.
bson/string_codec.go
Outdated
| @@ -58,12 +57,10 @@ func (sc *stringCodec) decodeType(_ DecodeContext, vr ValueReader, t reflect.Typ | |||
| if err != nil { | |||
| return emptyValue, err | |||
| } | |||
| if sc.decodeObjectIDAsHex { | |||
| if sc.decodeObjectIDAsHex || dc.objectIDAsHexString { | |||
There was a problem hiding this comment.
It's unclear to me how decodeObjectIDAsHex could ever be set in a non-testing environment. Can we remove this field from stringCodec?
bson/decoder.go
Outdated
| func (d *Decoder) DefaultDocumentD() { | ||
| d.defaultDocumentD = true | ||
| d.dc.defaultDocumentType = reflect.TypeOf(D{}) | ||
| } |
There was a problem hiding this comment.
I believe this is stale and was removed by another PR.
There was a problem hiding this comment.
I've removed DefaultDocumentD after merging the master branch.
There was a problem hiding this comment.
Both positive and negative cases are covered in "decoder_test.go".
qingyang-hu
added
priority-2-medium
GODRIVER-2796
Summary
ObjectIDAsHexStringbehavior: disableDecodeObjectIDAsHexand return an error by defaultEncodeContextandDecodeContext.Background & Motivation