GODRIVER-2612 Remove RegistryBuilder
#1659
Conversation
API Change Report./bsonincompatible changesArrayCodec: removed compatible changesErrMgoSetZero: added ./bson/bsonoptionsincompatible changespackage removed ./bson/mgocompatincompatible changesErrSetZero: removed |
qingyang-hu
force-pushed
the
godriver2976p3
branch
2 times, most recently
from
f1fce18
to
639d4a8
Compare
qingyang-hu
force-pushed
the
godriver2976p3
branch
from
639d4a8
to
6b3b203
Compare
qingyang-hu
changed the title
RegistryBuilder.RegistryBuilder.
blink1073
changed the title
RegistryBuilder.RegistryBuilder.
blink1073
changed the title
RegistryBuilder.RegistryBuilder
bson/byte_slice_codec.go
Outdated
| // used by collection type decoders (e.g. map, slice, etc) to set individual values in a | ||
| // collection. | ||
| _ typeDecoder = defaultByteSliceCodec | ||
| _ typeDecoder = (*byteSliceCodec)(nil) |
There was a problem hiding this comment.
The traditional way we've done this in the driver is by instantiating a struct, e.g. _ typeDecoder = &byteSliceCodec{}. Is there a reason to do it the way you propose? Additionally, we should put the variable inline since we just make one assertion:
var _ typeDecoder = (*byteSliceCodec)(nil)
bson/default_value_decoders.go
Outdated
|
|
||
| "go.mongodb.org/mongo-driver/x/bsonx/bsoncore" | ||
| ) | ||
|
|
||
| var ( | ||
| defaultValueDecoders DefaultValueDecoders | ||
| errCannotTruncate = errors.New("float64 can only be truncated to a lower precision type when truncation is enabled") | ||
| errCannotTruncate = errors.New("float64 can only be truncated to a lower precision type when truncation is enabled") |
There was a problem hiding this comment.
Suggest putting the variable inline since there is just one:
ver errCannotTruncate = errors.New("float64 can only be truncated to a lower precision type when truncation is enabled")| } | ||
|
|
||
| func (DefaultValueDecoders) javaScriptDecodeType(_ DecodeContext, vr ValueReader, t reflect.Type) (reflect.Value, error) { | ||
| func javaScriptDecodeType(_ DecodeContext, vr ValueReader, t reflect.Type) (reflect.Value, error) { |
There was a problem hiding this comment.
Suggest removing the empty DecodeContext parameter.
There was a problem hiding this comment.
javaScriptDecodeType is used with decodeAdapter to satisfy the ValueDecoder interface. Although most typeDecoderFuncs in decodeAdapter don't use the DecodeContext parameter, the codeWithScopeDecodeType still requires a DecodeContext. Also for the extensibility, it still makes sense to keep the parameter.
| // value decoders registered. | ||
| func (dvd DefaultValueDecoders) JavaScriptDecodeValue(dctx DecodeContext, vr ValueReader, val reflect.Value) error { | ||
| // javaScriptDecodeValue is the ValueDecoderFunc for the JavaScript type. | ||
| func javaScriptDecodeValue(dctx DecodeContext, vr ValueReader, val reflect.Value) error { |
There was a problem hiding this comment.
The dctx parameter is unused, suggest removing it.
There was a problem hiding this comment.
The same appears to be true for symbolDecodeValue, binaryDecodeValue, undefinedDecodeValue, objectIDDecodeValue, dateTimeDecodeValue, nullDecodeValue, regexDecodeValue, dbPointerDecodeValue, timestampDecodeValue, coreDocumentDecodeValue, unmarshalerDecodeValue, valueUnmarshalerDecodeValue, urlDecodeValue, jsonNumberDecodeValue, decimal128DecodeValue, maxKeyDecodeValue, and minKeyDecodeValuem,
There was a problem hiding this comment.
I think it's reasonable to keep the DecodeContext parameter considering the extensibility of the typeDecoderFunc in decodeAdapter, e.g. codeWithScopeDecodeType.
| // value encoders registered. | ||
| func (dve DefaultValueEncoders) FloatEncodeValue(_ EncodeContext, vw ValueWriter, val reflect.Value) error { | ||
| // floatEncodeValue is the ValueEncoderFunc for float types. | ||
| func floatEncodeValue(_ EncodeContext, vw ValueWriter, val reflect.Value) error { |
There was a problem hiding this comment.
The same argument for the decode *Value functions applies here. There doesn't appear to be a need to include the empty EncodeContext parameter for most of these functions.
bson/empty_interface_codec.go
Outdated
| // to be used by collection type decoders (e.g. map, slice, etc) to set individual values in a | ||
| // collection. | ||
| _ typeDecoder = defaultEmptyInterfaceCodec | ||
| _ typeDecoder = (*emptyInterfaceCodec)(nil) |
There was a problem hiding this comment.
Suggest inlining the variable and using the conventional type assertion pattern: var _ interface = &struct{}
bson/struct_codec.go
Outdated
| _ ValueEncoder = (*structCodec)(nil) | ||
| _ ValueDecoder = (*structCodec)(nil) |
There was a problem hiding this comment.
Consider using the traditional convention of _ ValueEncoder = &structCodec{}
qingyang-hu
added
priority-2-medium
bson/mgoregistry.go
Outdated
|
|
||
| var ( | ||
| // ErrSetZero may be returned from a SetBSON method to have the value set to its respective zero value. | ||
| ErrSetZero = errors.New("set to zero") |
There was a problem hiding this comment.
Optional: Consider modifying this name to indicate it's only used with the mgo-compatibility registries. E.g. ErrMgoSetZero
bson/mgoregistry.go
Outdated
| // itself will not be changed as usual. | ||
| // | ||
| // If setting the value works, the method should return nil or alternatively | ||
| // mgocompat.ErrSetZero to set the respective field to its zero value (nil for |
There was a problem hiding this comment.
Optional: Update comment to remove mgocompat package name.
bson/struct_codec.go
Outdated
| // structCodec is the Codec used for struct values. | ||
| type structCodec struct { | ||
| cache sync.Map // map[reflect.Type]*structDescription | ||
| elemEncoder mapElementsEncoder |
There was a problem hiding this comment.
Optional: Consider using a field name that better describes the use case. E.g. inlineMapEncoder.
bson/struct_codec.go
Outdated
| @@ -216,14 +181,17 @@ func (sc *StructCodec) EncodeValue(ec EncodeContext, vw ValueWriter, val reflect | |||
| } | |||
| } | |||
|
|
|||
| if sd.inlineMap >= 0 { | |||
| if sd.inlineMap >= 0 && sc.elemEncoder != nil { | |||
There was a problem hiding this comment.
This check will result in omitting the map elements if there is no elemEncoder, which could result in a subtle bug that drops data. The only code that creates a structCodec is the bson package, so that's not a case we need to handle gracefully. A panic would be a much more obvious indicator of such a bug. We should remove this nil check.
| if sd.inlineMap >= 0 && sc.elemEncoder != nil { | |
| if sd.inlineMap >= 0 { |
Additionally, we could panic in newStructCodec if elemEncoder is nil, although not all code paths that use a structCodec call newStructCodec.
GODRIVER-2612
Summary
RegistryBuilder.