GODRIVER-2407 Add DocumentDecodeType to DecodeContext #951

prestonvasquez · 2022-05-17T18:56:42Z

This PR creates a way for a user/client to write a document-type override to the DecodeContext per this JIRA comment.

Work Around

dec, err := NewDecoder(bsonrw.NewBSONDocumentReader(bytes))
if err != nil {
	t.Fatal(err)
}

// This will default empty interfaces into primitive.D.
dec.DefaultDocumentD()

Long-Term

My understanding is that we would like the encoder/decoder map to be bijective.

        encoder                decoder
go type ───────► bsontype.Type ───────► driver type

I believe that the canonical argument for this is that the purpose of a BSON decoder should be to decode BSON and although we support bson.M in go, the concept of a BSON Map per se doesn’t exist. Maps are documents, despite our driver's extension types. So to maintain bijectivity in our encoding, we should always enforce that maps encode to documents and documents decode to primitive.D.

This could probably be resolved by removing the ancestor logic from the MapCodec decoder and updating the tests to reflect that embedded documents map to primitive.D in the default case.

I don't think that the long-term solution proposed here will deprecate the "work around" changes suggested in this PR, having a work around will probably be something we want long-term as well.

benjirewis

Looking great; the concept of a new DocumentDecodeType on DecodeContext makes sense to me. After discussion offline, sounds like we want to defer the "long term" solution to another ticket?

bson/bsoncodec/bsoncodec.go

bson/bsoncodec/empty_interface_codec.go

bson/bsoncodec/map_codec.go

bson/decoder_test.go

bson/bsoncodec/bsoncodec.go

bson/decoder_test.go

Co-authored-by: Benjamin Rewis <32186188+benjirewis@users.noreply.github.com>

matthewdale · 2022-05-17T22:34:50Z

bson/bsoncodec/bsoncodec.go

+
+	// DocumentDecodeType will decode embedded documents into the defined type, rather than into primitive.D.  This is
+	// work-around for custom typing and is All-Or-None.
+	DocumentDecodeType *reflect.Type


reflect.Type is an interface, so we can already set it to nil without being a pointer. Update DocumentDecodeType to be type reflect.Type.

matthewdale · 2022-05-20T05:28:29Z

bson/bsoncodec/bsoncodec.go

+
+	// DocumentDecodeType will decode embedded documents into the defined type, rather than into primitive.D. This is a
+	// work-around for custom typing.
+	DocumentDecodeType *reflect.Type


Considering some alternative names:

DocumentType - The option is on a DecodeContext, so the "Decode" part is redundant. Still, DocumentType doesn't make it clear what we're configuring.

DefaultDocumentType - Technically more correct because we'll still ignore this configuration if there is type information in the Decode/Unmarshal output parameter.

OverrideDefaultDocumentType - The most descriptive, but pretty wordy.

I think DefaultDocumentType is the best option. Consider renaming DocumentDecodeType to DefaultDocumentType.

I actually like DocumentType the best. I think it reads well as part of the DecodeContext, i.e. "For decoding context, the document type is X". What do you think? I'm definitely not married to anything, this just makes the most sense to me ATM.

matthewdale · 2022-05-20T06:01:14Z

bson/bsoncodec/bsoncodec.go

@@ -123,6 +123,16 @@ type DecodeContext struct {
 	// Ancestor is a bson.M, BSON embedded document values being decoded into an empty interface
 	// will be decoded into a bson.M.
 	Ancestor reflect.Type
+
+	// DocumentDecodeType will decode embedded documents into the defined type, rather than into primitive.D. This is a
+	// work-around for custom typing.


We should make this comment more descriptive to help users understand when and how to use this configuration.

E.g.

// DefaultDocumentType configures the default Go type to unmarshal top-level and nested BSON documents // into. The DefaultDocumentType is only used if the Decoder can't determine type information based on // the Decode output parameter (e.g. is type "interface{}" or "map[string]interface{}"). Setting // DefaultDocumentType overrides the Ancestor field. If DefaultDocumentType is set to a type that a // BSON document cannot be unmarshaled into (e.g. "string"), unmarshalling will result in an error.

Done, with a few tweaks. What are your thoughts?

// DocumentType specifies the Go type to decode top-level and nested BSON documents into. In particular, the // usage for this field is restricted to data typed as "interface{}" or "map[string]interface{}". If DocumentType is // set to a type that a BSON document cannot be unmarshaled into (e.g. "string"), unmarshalling will result in an // error. DocumentType overrides the Ancestor field.

matthewdale · 2022-05-20T06:07:13Z

bson/bsoncodec/bsoncodec.go

@@ -123,6 +123,16 @@ type DecodeContext struct {
 	// Ancestor is a bson.M, BSON embedded document values being decoded into an empty interface
 	// will be decoded into a bson.M.
 	Ancestor reflect.Type


We should deprecate the Ancestor field with this change because it doesn't really do what it describes (e.g. it's effectively ignored when the Decode output parameter is a map[string]interface{}).

E.g.

// Ancestor is a bson.M, BSON embedded document values being decoded into an empty interface // will be decoded into a bson.M. // // Deprecated: Use DefaultDocumentType instead.

Now that the PR uses DefaultDocumentM and DefaultDocumentD, we should update the "Deprecated" message to say

Deprecated: Use DefaultDocumentM or DefaultDocumentD instead.

matthewdale · 2022-05-20T06:10:21Z

bson/decoder_test.go

+		bsonFooOutType := reflect.TypeOf(bsonOut["foo"]).String()
+		documentType := reflect.TypeOf(map[string]interface{}{}).String()
+		assert.Equal(t, documentType, bsonFooOutType, "expected %v to equal %v", inFooType, bsonFooOutType)
+	})


We should add some test cases:

Checks what happens when we SetDocumentDecodeType and the Decode output parameter is type interface{}. Do we expect the SetDocumentDecodeType type to be used when decoding the top-level BSON document as well?

Check what happens when we SetDocumentDecodeType to a string or something that can't represent a BSON document. Does unmarshalling return an error?

I would expect that if the output type is an empty interface, or a map pointing to an empty interface, that the logic would convert the top-level type.

Absolutely, will do.

benjirewis

Looks great! Awesome work.

benjirewis · 2022-05-20T21:12:13Z

bson/decoder_test.go

+		}
+
+		// We should only be able to decode data typed as interface{}, map[string]interface{}, or map[string]string.
+		nonDocumentTypes := []reflect.Type{


Awesome! 🧑‍🔧

prestonvasquez · 2022-05-25T20:32:46Z

bson/decoder.go

-	vr bsonrw.ValueReader
+	dc               bsoncodec.DecodeContext
+	vr               bsonrw.ValueReader
+	defaultDocumentM bool


We do this instead of setting directly on dc because the Decoder exposes a setter for the DecodeContext: (*Decoder).SetContext. If the user set the default document behavior and then the context, the default behavior will be overwritten. Persisting the behavior on the Decoder allows us to set the documentType on the decoder before the DecodeValue call.

dec.DefaultDocumentD() // If we didn't do it this way, this would cause the default behavior to be overwritten. dec.SetContext(bsoncodec.DecodeContext{})

Optional: Consider leaving a comment about this choice in the code as well so future devs understand the decision.

matthewdale

A few doc improvement suggestions, but looks good! 👍

matthewdale · 2022-05-23T23:36:15Z

bson/bsoncodec/bsoncodec.go

+	// usage for this field is restricted to data typed as "interface{}" or "map[string]interface{}". If DocumentType is
+	// set to a type that a BSON document cannot be unmarshaled into (e.g. "string"), unmarshalling will result in an
+	// error. DocumentType overrides the Ancestor field.
+	DocumentType reflect.Type


Optional: Consider adding a note about the typical values that someone would set DocumentType to in the comment.

E.g. comment with additional clarification:

DocumentType must be a Go type that can represent any BSON document, such as a "map[string]interface{}" or "bson.M".

// DocumentType specifies the Go type to decode top-level and nested BSON documents into. In particular, the // usage for this field is restricted to data typed as "interface{}" or "map[string]interface{}". DocumentType // must be a Go type that can represent any BSON document, such as a "map[string]interface{}" or "bson.M". If // DocumentType is set to a type that a BSON document cannot be unmarshaled into (e.g. "string"), // unmarshalling will result in an error. DocumentType overrides the Ancestor field.

matthewdale · 2022-05-25T21:52:58Z

bson/bsoncodec/bsoncodec.go

+
+// DefaultDocumentM will set the defaultDocumentType as primitive.M, to be used when decoding "interface{}" and
+// "map[string]interface{}".
+func (dc *DecodeContext) DefaultDocumentM() error {


Nice, I like having identical APIs on the Decoder and DecodeContext.

matthewdale · 2022-05-25T21:54:23Z

bson/bsoncodec/bsoncodec.go

@@ -123,6 +123,16 @@ type DecodeContext struct {
 	// Ancestor is a bson.M, BSON embedded document values being decoded into an empty interface
 	// will be decoded into a bson.M.
 	Ancestor reflect.Type


Now that the PR uses DefaultDocumentM and DefaultDocumentD, we should update the "Deprecated" message to say

Deprecated: Use DefaultDocumentM or DefaultDocumentD instead.

matthewdale · 2022-05-25T21:56:05Z

bson/bsoncodec/bsoncodec.go

+}
+
+// DefaultDocumentM will set the defaultDocumentType as primitive.M, to be used when decoding "interface{}" and
+// "map[string]interface{}".


Optional: Consider using the same doc comment as on Decoder.DefaultDocumentM which is more clear and doesn't require knowing about the unexported field defaultDocumentType.

matthewdale · 2022-05-25T21:56:23Z

bson/bsoncodec/bsoncodec.go

+}
+
+// DefaultDocumentD will set the defaultDocumentType as primitive.D, to be used when decoding "interface{}" and
+// "map[string]interface{}".


Optional: Consider using the same doc comment as on Decoder.DefaultDocumentD which is more clear and doesn't require knowing about the unexported field defaultDocumentType.

matthewdale · 2022-05-25T21:57:51Z

bson/decoder.go

-	vr bsonrw.ValueReader
+	dc               bsoncodec.DecodeContext
+	vr               bsonrw.ValueReader
+	defaultDocumentM bool


Optional: Consider leaving a comment about this choice in the code as well so future devs understand the decision.

GODRIVER-2407 Add DocumentDecodeType to DecodeContext

4837cf8

prestonvasquez requested review from matthewdale and benjirewis May 17, 2022 18:56

Merge branch 'master' into GODRIVER-2407

5179ed7

prestonvasquez marked this pull request as ready for review May 17, 2022 19:39

GODRVIER-2407 Fix linting errors

7a2977b

benjirewis reviewed May 18, 2022

View reviewed changes

prestonvasquez and others added 2 commits May 19, 2022 16:17

Update bson/bsoncodec/bsoncodec.go

955bfda

Co-authored-by: Benjamin Rewis <32186188+benjirewis@users.noreply.github.com>

GODRIVER-2407 Review clean-up

0c12bbb

prestonvasquez requested a review from benjirewis May 19, 2022 22:26

GODRIVER-2407 remove TODO from ancestor block

c8cbbd3

matthewdale reviewed May 20, 2022

View reviewed changes

prestonvasquez added 3 commits May 20, 2022 12:13

GODRIVER-2407 Syntax cleanup. Expand test coverage

6f01b3e

GODRIVER-2407 Add more testing coverage

a8b2552

GODRIVER-2407 Clean up DocumentType description comment

119a2cd

prestonvasquez requested a review from matthewdale May 20, 2022 18:30

benjirewis approved these changes May 20, 2022

View reviewed changes

GODRIVER-2407 functionalize decoding options

6ce817a

prestonvasquez commented May 25, 2022

View reviewed changes

matthewdale approved these changes May 25, 2022

View reviewed changes

prestonvasquez added 2 commits May 25, 2022 17:18

GODRIVER-2407 Clean up comments

41cecf0

GODRIVER-2407 no returning errors on setters

a717891

prestonvasquez merged commit 8f19bdb into master May 26, 2022

prestonvasquez deleted the GODRIVER-2407 branch June 22, 2022 19:33

prestonvasquez mentioned this pull request Mar 14, 2023

GODRIVER-2766 Support inherited defaultDocumentType #1202

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GODRIVER-2407 Add DocumentDecodeType to DecodeContext #951

GODRIVER-2407 Add DocumentDecodeType to DecodeContext #951

prestonvasquez commented May 17, 2022 •

edited

Loading

benjirewis left a comment

matthewdale May 17, 2022

prestonvasquez May 20, 2022

matthewdale May 20, 2022

prestonvasquez May 20, 2022

matthewdale May 20, 2022

prestonvasquez May 20, 2022

matthewdale May 20, 2022

prestonvasquez May 20, 2022

matthewdale May 25, 2022

matthewdale May 20, 2022

prestonvasquez May 20, 2022

prestonvasquez May 20, 2022

benjirewis left a comment

benjirewis May 20, 2022

prestonvasquez May 25, 2022

matthewdale May 25, 2022

matthewdale left a comment

matthewdale May 23, 2022

matthewdale May 25, 2022

matthewdale May 25, 2022

matthewdale May 25, 2022

matthewdale May 25, 2022

matthewdale May 25, 2022

GODRIVER-2407 Add DocumentDecodeType to DecodeContext #951

GODRIVER-2407 Add DocumentDecodeType to DecodeContext #951

Conversation

prestonvasquez commented May 17, 2022 • edited Loading

Work Around

Long-Term

benjirewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjirewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewdale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestonvasquez commented May 17, 2022 •

edited

Loading