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-2711 Deprecate BSON *Append and *With functions. #1251

Merged
merged 4 commits into from
May 19, 2023

Conversation

matthewdale
Copy link
Collaborator

@matthewdale matthewdale commented May 8, 2023

GODRIVER-2711

Summary

  • Deprecate all bson package Marshal/Unmarshal functions that append data to an existing byte slice. Recommend people use the Encoder or Decoder types instead.
  • Deprecate all bson package Marshal/Unmarshal functions that accept a Registry. Recommend people use the Encoder or Decoder types instead.
  • Deprecate all functions that use an EncodeContext or DecodeContext during marshaling or unmarshaling. Recommend people use the Encoder or Decoder configuration functions instead.
  • Add testable examples of how to use Encoder and Decoder to achieve most deprecated behaviors.

Background & Motivation

The bson package is cluttered with permutations of *WithRegistry, *WithContext and *Append functions for Marshal and Unmarshal. The same behavior can be now be achieved with an Encoder or Decoder, making the *WithRegistry, *WithContext, and *Append functions redundant. Provide examples to show users how to migrate their use cases to Encoder or Decoder and deprecate the redundant functions in preparation to remove them in Go Driver 2.0.

@matthewdale matthewdale force-pushed the godriver2711-dep-bson-with branch 2 times, most recently from 319ae80 to 10a0e19 Compare May 8, 2023 19:26
@matthewdale matthewdale changed the title GODRIVER2711 Deprecate BSON *Append and *With functions. GODRIVER-2711 Deprecate BSON *Append and *With functions. May 8, 2023
@matthewdale matthewdale marked this pull request as ready for review May 8, 2023 20:21
// Extended JSON by converting them to bson.Raw.
for {
doc, err := bson.ReadDocument(buf)
if err == io.EOF {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think it is necessary to leave a comment on the io.EOF here as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's a good ideal! I'll extend the comment above to mention that ReadDocument returns io.EOF when the input is empty.

// Always end top-level documents with a newline so that multiple documents can be encoded
// to the same writer. That matches the Go json.Encoder behavior and also works with
// bsonrw.NewExtJSONValueReader.
ejvw.buf = append(ejvw.buf, '\n')
Copy link
Collaborator

Choose a reason for hiding this comment

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

This addition causes corpus failures:

go test -run Test_BsonCorpus

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! To match the behavior of the Go encoding/json library, we actually only want to apply this to the output generated by Encode and not by MarshalExtJSON. I'm not sure if the BSON corpus tests use Encode or MarshalExtJSON, but it's possible that change will resolve the failures. If not, I'll look into whether or not it's a good idea to trim whitespace in the BSON corpus tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to only add newlines when using Encoder.Encode.

bson/decoder.go Outdated Show resolved Hide resolved
bson/decoder_example_test.go Outdated Show resolved Hide resolved
bson/encoder.go Outdated Show resolved Hide resolved
bson/encoder_example_test.go Outdated Show resolved Hide resolved
bson/marshal.go Outdated Show resolved Hide resolved
bson/marshal.go Outdated Show resolved Hide resolved
bson/marshal.go Outdated Show resolved Hide resolved
bson/unmarshal.go Outdated Show resolved Hide resolved
bson/unmarshal.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@qingyang-hu qingyang-hu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@qingyang-hu qingyang-hu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@qingyang-hu qingyang-hu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@prestonvasquez prestonvasquez left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for all of the examples

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants