GODRIVER-2714 POC: Eliminate *Context types used by BSON Encoder or Decoder.
#1612
Conversation
API Change Report##! second, different message for obj type ./bson.DecodeContext struct{./bson.Registry; Truncate bool; Ancestor reflect.Type; defaultDocumentType reflect.Type; binaryAsSlice bool; useJSONStructTags bool; useLocalTimeZone bool; zeroMaps bool; zeroStructs bool}, isNew false, part "" ./bsonincompatible changes(*DecodeContext).BinaryAsSlice: removed compatible changes(*Decoder).SetBehavior: added ./bson/bsonoptionsincompatible changespackage removed ./bson/mgocompatincompatible changesErrSetZero: removed |
qingyang-hu
force-pushed
the
godriver2796
branch
2 times, most recently
from
666fad1
to
591a57c
Compare
qingyang-hu
force-pushed
the
godriver2796
branch
3 times, most recently
from
bbdc996
to
fcdd960
Compare
qingyang-hu
force-pushed
the
godriver2796
branch
from
fcdd960
to
d8fba65
Compare
qingyang-hu
force-pushed
the
godriver2796
branch
6 times, most recently
from
8d43b10
to
896be3d
Compare
qingyang-hu
force-pushed
the
godriver2796
branch
2 times, most recently
from
912a6b0
to
08ec80e
Compare
| // | ||
| // and tries to decode BSON bytes into the map, the decoding will fail if this conversion is not present | ||
| // because we'll try to assign a value of type bool to one of type myBool. | ||
| val = val.Convert(t) |
There was a problem hiding this comment.
The ancestor type used by the empty interface codec is passed as the last argument of the decodeType, so the caller will be responsible for the conversion.
| } | ||
|
|
||
| rtype, err := eic.getEmptyInterfaceDecodeType(dc, vr.Type()) | ||
| func (eic *emptyInterfaceCodec) decodeType(reg DecoderRegistry, vr ValueReader, t reflect.Type) (reflect.Value, error) { |
There was a problem hiding this comment.
The last argument of the function signature indicates the ancestor type.
|
|
||
| // A RegistryBuilder is used to build a Registry. This type is not goroutine | ||
| // safe. | ||
| // | ||
| // Deprecated: Use Registry instead. | ||
| type RegistryBuilder struct { |
There was a problem hiding this comment.
The builder pattern is applied to generate new instances of codecs, so options will not impact across Registries.
qingyang-hu
changed the title
*Context types used by BSON Encoder or Decoder.
qingyang-hu
added
priority-2-medium
| // value passed to Decode before unmarshaling BSON documents into them. | ||
| func (d *Decoder) ZeroStructs() { | ||
| d.zeroStructs = true |
There was a problem hiding this comment.
Removing these configuration methods and replacing them with SetBehavior is a major refactor of the bson.Decoder API and seems outside the scope of GODRIVER-2714. What motivated the change to this API?
There was a problem hiding this comment.
We don't need the SetBahavior if we only want to remove EncodeContext/DecodeContext from the bson.Encoder/Decoder API.
| // DefaultRegistry is the default Registry. It contains the default codecs and the | ||
| // primitive codecs. | ||
| var DefaultRegistry = NewRegistry() |
There was a problem hiding this comment.
This change also covers the scope of GODRIVER-3142, which we should call out in the PR description.
| type ValueEncoder interface { | ||
| EncodeValue(EncodeContext, ValueWriter, reflect.Value) error | ||
| EncodeValue(EncoderRegistry, ValueWriter, reflect.Value) error |
There was a problem hiding this comment.
We don't want to remove EncodeContext from the ValueEncoder API, only from the bson.Encoder API. Keeping EncodeContext in the EncodeValue function may significantly simplify this PR.
There was a problem hiding this comment.
Current EncodeContext/DecodeContext is coupled with the fields in Encoder/Decoder and codecs. For example, OverwriteDuplicatedInlinedFields exists in StructCodec, EncodeContext, and Encoder. Moreover, the example here, OverwriteDuplicatedInlinedFields, is default true. Considering future non-zero default codec fields as well as the current use cases of codec configurations for mgocompat, using an empty struct, such as type structCodec struct {}, and passing EncodeContext/DecodeContext to modify the encoding/decoding will be similarly complex. Meanwhile, the *Context becomes a superset of all codec attributes since it has to be passed into different codecs. Also for custom codecs, *Context is either hard to be extended or useless.
Though SetBehavior and Registry.SetCodecOption are out of the scope of GODRIVER-2714, they also decouple the logic with individual codecs by distributing the configuration response to each codec, yet providing a single entry from outside.
However, I agree that SetBehavior and Registry.SetCodecOption are redundant. We can make Registry.SetCodecOption accessible to the bson package only.
A less radical PR keeping both *Context and codec fields is available at #1659.
| @@ -276,28 +107,28 @@ func (fn ValueEncoderFunc) EncodeValue(ec EncodeContext, vw ValueWriter, val ref | |||
| // ValueDecoder. A DecodeContext instance is provided and serves similar functionality to the | |||
| // EncodeContext. | |||
| type ValueDecoder interface { | |||
| DecodeValue(DecodeContext, ValueReader, reflect.Value) error | |||
| DecodeValue(DecoderRegistry, ValueReader, reflect.Value) error | |||
There was a problem hiding this comment.
We don't want to remove DecodeContext from the ValueDecoder API, only from the bson.Decoder API. Keeping DecodeContext in the DecodeValue function may significantly simplify this PR.
| // SetCodecOption configures Registry using a *RegistryOpt. | ||
| func (r *Registry) SetCodecOption(opt *RegistryOpt) error { | ||
| v, ok := r.codecTypeMap[opt.typ] |
There was a problem hiding this comment.
This method seems only practically usable by Go Driver code. Can we remove it if we continue to pass the optional Encoder/Decoder behaviors via the EncodeContext/DecodeContext types for now?
| type Setter interface { | ||
| SetBSON(raw bson.RawValue) error | ||
| SetBSON(raw RawValue) error | ||
| } |
There was a problem hiding this comment.
Moving this to the bson package creates ambiguous naming because the Setter interface is only relevant to mgo compatibility, but overlaps confusingly with the bson.Unmarshaler interface. Is there a reason we need to move these APIs to the bson package instead of keeping them in the mgocompat package?
There was a problem hiding this comment.
I moved these methods into the bson package because we un-exposed codec structs and their fields, which made them inaccessible from mgocompat.
GODRIVER-2714
GODRIVER-2796
GODRIVER-3142
Summary
*Contexttypes used by BSONEncoderorDecoder.minSizeandtruncateoptions can be set in one place.DefaultRegistry.Background & Motivation
This PR proposes passing only a
Registrywithout extra codec options in the*Contexttype forEncodeValue/DecodeValueinterfaces of codecs. It decouples theRegistrywith each codec.Moreover, a new
RegistryOpttype is introduced with the help of generics and reflection, so each codec is responsible for its options from a single method provided byRegistry. In this way, the option setting is more flexible, especially for custom codecs.