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.

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-2714 POC: Eliminate *Context types used by BSON Encoder or Decoder. #1612

Closed
wants to merge 16 commits into from

Conversation

qingyang-hu
Copy link
Collaborator

@qingyang-hu qingyang-hu commented Apr 22, 2024

GODRIVER-2714
GODRIVER-2796
GODRIVER-3142

Summary

  • Eliminate *Context types used by BSON Encoder or Decoder.
  • Keep codec options with each codec.
  • Merge intCodec, uintCodec, and floatCodec into numCodec so the minSize and truncate options can be set in one place.
  • Remove DefaultRegistry.

Background & Motivation

This PR proposes passing only a Registry without extra codec options in the *Context type for EncodeValue/DecodeValue interfaces of codecs. It decouples the Registry with each codec.

Moreover, a new RegistryOpt type is introduced with the help of generics and reflection, so each codec is responsible for its options from a single method provided by Registry. In this way, the option setting is more flexible, especially for custom codecs.

@mongodb-drivers-pr-bot mongodb-drivers-pr-bot bot added the priority-3-low Low Priority PR for Review label Apr 22, 2024
Copy link
Contributor

mongodb-drivers-pr-bot bot commented Apr 22, 2024

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 ""
first: old is comparable, new is not
second: removed
##! second, different message for obj type ./bson.EncodeContext struct{
./bson.Registry; MinSize bool; errorOnInlineDuplicates bool; stringifyMapKeysWithFmt bool; nilMapAsEmpty bool; nilSliceAsEmpty bool; nilByteSliceAsEmpty bool; omitZeroStruct bool; useJSONStructTags bool}, isNew false, part ""
first: changed from struct{*Registry; MinSize bool; errorOnInlineDuplicates bool; stringifyMapKeysWithFmt bool; nilMapAsEmpty bool; nilSliceAsEmpty bool; nilByteSliceAsEmpty bool; omitZeroStruct bool; useJSONStructTags bool} to interface{LookupEncoder(reflect.Type) (ValueEncoder, error)}
second: removed

./bson

incompatible changes

(*DecodeContext).BinaryAsSlice: removed
(*DecodeContext).DefaultDocumentD: removed
(*DecodeContext).DefaultDocumentM: removed
(*DecodeContext).UseJSONStructTags: removed
(*DecodeContext).UseLocalTimeZone: removed
(*DecodeContext).ZeroMaps: removed
(*DecodeContext).ZeroStructs: removed
(*Decoder).AllowTruncatingDoubles: removed
(*Decoder).BinaryAsSlice: removed
(*Decoder).DefaultDocumentD: removed
(*Decoder).DefaultDocumentM: removed
(*Decoder).Reset: removed
(*Decoder).SetRegistry: removed
(*Decoder).UseJSONStructTags: removed
(*Decoder).UseLocalTimeZone: removed
(*Decoder).ZeroMaps: removed
(*Decoder).ZeroStructs: removed
(*Encoder).ErrorOnInlineDuplicates: removed
(*Encoder).IntMinSize: removed
(*Encoder).NilByteSliceAsEmpty: removed
(*Encoder).NilMapAsEmpty: removed
(*Encoder).NilSliceAsEmpty: removed
(*Encoder).OmitZeroStruct: removed
(*Encoder).Reset: removed
(*Encoder).SetRegistry: removed
(*Encoder).StringifyMapKeysWithFmt: removed
(*Encoder).UseJSONStructTags: removed
(*Registry).LookupDecoder, method set of DecodeContext: removed
(*Registry).LookupEncoder, method set of DecodeContext: removed
(*Registry).LookupTypeMapEntry, method set of DecodeContext: removed
(*Registry).RegisterInterfaceDecoder, method set of *DecodeContext: removed
(*Registry).RegisterInterfaceDecoder, method set of DecodeContext: removed
(*Registry).RegisterInterfaceDecoder: removed
(*Registry).RegisterInterfaceEncoder, method set of *DecodeContext: removed
(*Registry).RegisterInterfaceEncoder, method set of DecodeContext: removed
(*Registry).RegisterInterfaceEncoder: removed
(*Registry).RegisterKindDecoder, method set of *DecodeContext: removed
(*Registry).RegisterKindDecoder, method set of DecodeContext: removed
(*Registry).RegisterKindDecoder: removed
(*Registry).RegisterKindEncoder, method set of *DecodeContext: removed
(*Registry).RegisterKindEncoder, method set of DecodeContext: removed
(*Registry).RegisterKindEncoder: removed
(*Registry).RegisterTypeDecoder, method set of *DecodeContext: removed
(*Registry).RegisterTypeDecoder, method set of DecodeContext: removed
(*Registry).RegisterTypeDecoder: removed
(*Registry).RegisterTypeEncoder, method set of *DecodeContext: removed
(*Registry).RegisterTypeEncoder, method set of DecodeContext: removed
(*Registry).RegisterTypeEncoder: removed
(*Registry).RegisterTypeMapEntry, method set of *DecodeContext: removed
(*Registry).RegisterTypeMapEntry, method set of DecodeContext: removed
(*Registry).RegisterTypeMapEntry: removed
(*RegistryBuilder).RegisterCodec: removed
(*RegistryBuilder).RegisterDecoder: removed
(*RegistryBuilder).RegisterDefaultDecoder: removed
(*RegistryBuilder).RegisterDefaultEncoder: removed
(*RegistryBuilder).RegisterEncoder: removed
(*RegistryBuilder).RegisterHookDecoder: removed
(*RegistryBuilder).RegisterHookEncoder: removed
(*RegistryBuilder).RegisterTypeDecoder: changed from func(reflect.Type, ValueDecoder) *RegistryBuilder to func(reflect.Type, DecoderFactory) *RegistryBuilder
(*RegistryBuilder).RegisterTypeEncoder: changed from func(reflect.Type, ValueEncoder) *RegistryBuilder to func(reflect.Type, EncoderFactory) *RegistryBuilder
ArrayCodec: removed
ByteSliceCodec: removed
DecodeContext.Ancestor: removed
DecodeContext.Registry: removed
DecodeContext.Truncate: removed
DecodeContext: removed
DefaultRegistry: removed
DefaultStructTagParser: removed
DefaultValueDecoders: removed
DefaultValueEncoders: removed
EmptyInterfaceCodec: removed
EncodeContext: removed
ErrNilType: removed
ErrNotInterface: removed
ErrNotPointer: removed
JSONFallbackStructTagParser: removed
MapCodec: removed
NewArrayCodec: removed
NewByteSliceCodec: removed
NewEmptyInterfaceCodec: removed
NewMapCodec: removed
NewPointerCodec: removed
NewRegistry: removed
NewSliceCodec: removed
NewStringCodec: removed
NewStructCodec: removed
NewTimeCodec: removed
NewUIntCodec: removed
PointerCodec: removed
PrimitiveCodecs: removed
RegistryBuilder: old is comparable, new is not
SliceCodec: removed
StringCodec: removed
StructCodec: removed
StructTagParser: removed
StructTagParserFunc: removed
StructTags: removed
TimeCodec: removed
UIntCodec: removed
UnmarshalExtJSONWithContext: changed from func(DecodeContext, []byte, bool, interface{}) error to func(*Registry, []byte, bool, interface{}) error
UnmarshalWithContext: removed
ValueCodec: removed
ValueDecoder.DecodeValue: changed from func(DecodeContext, ValueReader, reflect.Value) error to func(DecoderRegistry, ValueReader, reflect.Value) error
ValueDecoderFunc.DecodeValue: changed from func(DecodeContext, ValueReader, reflect.Value) error to func(DecoderRegistry, ValueReader, reflect.Value) error
ValueDecoderFunc: changed from func(DecodeContext, ValueReader, reflect.Value) error to func(DecoderRegistry, ValueReader, reflect.Value) error

compatible changes

(*Decoder).SetBehavior: added
(*Encoder).SetBehavior: added
(*Registry).SetCodecOption: added
(*RegistryBuilder).RegisterInterfaceDecoder: added
(*RegistryBuilder).RegisterInterfaceEncoder: added
(*RegistryBuilder).RegisterKindDecoder: added
(*RegistryBuilder).RegisterKindEncoder: added
AllowTruncatingDoubles: added
BinaryAsSlice: added
ConfigurableDecoderRegistry: added
ConfigurableEncoderRegistry: added
DecoderFactory: added
DecoderRegistry: added
DefaultDocumentD: added
DefaultDocumentM: added
EncoderFactory: added
EncoderRegistry: added
ErrSetZero: added
ErrorOnInlineDuplicates: added
Getter: added
GetterEncodeValue: added
IntMinSize: added
NewDecoderWithRegistry: added
NewEncoderWithRegistry: added
NewMgoRegistry: added
NewRegistryOpt: added
NewRespectNilValuesMgoRegistry: added
NilByteSliceAsEmpty: added
NilMapAsEmpty: added
NilSliceAsEmpty: added
ObjectIDAsHex: added
OmitZeroStruct: added
RegistryOpt: added
Setter: added
SetterDecodeValue: added
StringifyMapKeysWithFmt: added
UseJSONStructTags: added
UseLocalTimeZone: added
ZeroMaps: added
ZeroStructs: added

./bson/bsonoptions

incompatible changes

package removed

./bson/mgocompat

incompatible changes

ErrSetZero: removed
Getter: removed
GetterEncodeValue: removed
NewRegistryBuilder: removed
NewRespectNilValuesRegistryBuilder: removed
Setter: removed
SetterDecodeValue: removed

@qingyang-hu qingyang-hu force-pushed the godriver2796 branch 2 times, most recently from 666fad1 to 591a57c Compare April 24, 2024 15:48
@qingyang-hu qingyang-hu force-pushed the godriver2796 branch 3 times, most recently from bbdc996 to fcdd960 Compare April 29, 2024 21:18
@qingyang-hu qingyang-hu force-pushed the godriver2796 branch 6 times, most recently from 8d43b10 to 896be3d Compare May 21, 2024 16:08
@qingyang-hu qingyang-hu force-pushed the godriver2796 branch 2 times, most recently from 912a6b0 to 08ec80e Compare May 22, 2024 17:53
//
// 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)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The builder pattern is applied to generate new instances of codecs, so options will not impact across Registries.

@qingyang-hu qingyang-hu changed the title GODRIVER-2796 GODRIVER-2714 POC: Eliminate *Context types used by BSON Encoder or Decoder. May 28, 2024
@qingyang-hu qingyang-hu marked this pull request as ready for review May 28, 2024 15:39
@qingyang-hu qingyang-hu added priority-2-medium Medium Priority PR for Review and removed priority-3-low Low Priority PR for Review labels May 28, 2024
Comment on lines -167 to -169
// value passed to Decode before unmarshaling BSON documents into them.
func (d *Decoder) ZeroStructs() {
d.zeroStructs = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't need the SetBahavior if we only want to remove EncodeContext/DecodeContext from the bson.Encoder/Decoder API.

Comment on lines -16 to -18
// DefaultRegistry is the default Registry. It contains the default codecs and the
// primitive codecs.
var DefaultRegistry = NewRegistry()
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Comment on lines +338 to +340
// SetCodecOption configures Registry using a *RegistryOpt.
func (r *Registry) SetCodecOption(opt *RegistryOpt) error {
v, ok := r.codecTypeMap[opt.typ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Comment on lines 34 to 36
type Setter interface {
SetBSON(raw bson.RawValue) error
SetBSON(raw RawValue) error
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved these methods into the bson package because we un-exposed codec structs and their fields, which made them inaccessible from mgocompat.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-2-medium Medium Priority PR for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants