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-2976 Remove deprecated BSON code. #1698

Merged
merged 8 commits into from
Jul 19, 2024

Conversation

qingyang-hu
Copy link
Collaborator

@qingyang-hu qingyang-hu commented Jul 3, 2024

GODRIVER-2976

Summary

Remove all deprecated BSON code.

Background & Motivation

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

mongodb-drivers-pr-bot bot commented Jul 3, 2024

API Change Report

./bson

incompatible changes

CompareTimestamp: removed
CopyValueToBytes: removed
ErrNilType: removed
ErrNoDecoder: removed
ErrNoEncoder: removed
ErrNoTypeMapEntry: removed
ErrNotPointer: removed
ExtJSONValueReaderPool: removed
ExtJSONValueWriterPool: removed
IsValidObjectID: removed
MarshalValueWithRegistry: removed
NewExtJSONValueReaderPool: removed
NewExtJSONValueWriterPool: removed
NewValueReaderPool: removed
NewValueWriterPool: removed
SliceWriter: removed
UnmarshalExtJSONWithContext: removed
UnmarshalExtJSONWithRegistry: removed
UnmarshalValueWithRegistry: removed
UnmarshalWithContext: removed
UnmarshalWithRegistry: removed
ValueReaderPool: removed
ValueWriterFlusher: removed
ValueWriterPool: removed

// ExtJSONValueReaderPool is a pool for ValueReaders that read ExtJSON.
//
// Deprecated: ExtJSONValueReaderPool will not be supported in Go Driver 2.0.
type ExtJSONValueReaderPool 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.

Dead code

//
// Deprecated: Using a custom registry to marshal individual BSON values will not be supported in Go
// Driver 2.0.
func MarshalValueWithRegistry(r *Registry, val interface{}) (Type, []byte, error) {
Copy link
Collaborator Author

@qingyang-hu qingyang-hu Jul 5, 2024

Choose a reason for hiding this comment

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

Only called in the mongo package. Moved there.

bson/reader.go Outdated
// The bytes of the value will be appended to dst.
//
// Deprecated: BytesReader will not be supported in Go Driver 2.0.
type BytesReader interface {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only used by copier.

// getEncoder takes a writer, BSON options, and a BSON registry and returns a properly configured
// bson.Encoder that writes to the given writer.
func getEncoder(
w io.Writer,
opts *options.BSONOptions,
reg *bson.Registry,
) (*bson.Encoder, error) {
vw := bvwPool.Get(w)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Stop pooling since Put is never called.

@@ -30,28 +30,6 @@ func noerr(t *testing.T, err error) {
}
}

func TestCompareTimestamp(t *testing.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.

Dead code

// NewExtJSONValueWriterPool creates a new pool for ValueWriter instances that write to ExtJSON.
//
// Deprecated: ExtJSONValueWriterPool will not be supported in Go Driver 2.0.
func NewExtJSONValueWriterPool() *ExtJSONValueWriterPool {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only used in "marshal.go".

// ErrNotPointer is returned when a non-pointer type is provided to LookupDecoder.
//
// Deprecated: ErrNotPointer will not be supported in Go Driver 2.0.
var ErrNotPointer = errors.New("non-pointer provided to LookupDecoder")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dead code

// NewValueReaderPool instantiates a new ValueReaderPool.
//
// Deprecated: ValueReaderPool will not be supported in Go Driver 2.0.
func NewValueReaderPool() *ValueReaderPool {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dead code

// NewValueWriterPool creates a new pool for ValueWriter instances that write to BSON.
//
// Deprecated: ValueWriterPool will not be supported in Go Driver 2.0.
func NewValueWriterPool() *ValueWriterPool {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only used in "default_value_encoders.go".

bson/writer.go Outdated
// implement ValueWriter may also implement this interface.
//
// Deprecated: BytesWriter will not be supported in Go Driver 2.0.
type BytesWriter interface {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only used in copier.

// ValueWriterFlusher is a superset of ValueWriter that exposes functionality to flush to the underlying buffer.
//
// Deprecated: ValueWriterFlusher will not be supported in Go Driver 2.0.
type ValueWriterFlusher interface {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only used by ValueWriterPool.GetAtModeElement(), which is only called by MarshalValueWithRegistry().

@qingyang-hu qingyang-hu marked this pull request as ready for review July 5, 2024 18:25
@qingyang-hu qingyang-hu added priority-2-medium Medium Priority PR for Review and removed priority-3-low Low Priority PR for Review labels Jul 5, 2024
mongo/util.go Outdated
},
}

func marshalValueWithRegistry(r *bson.Registry, val interface{}) (bsoncore.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.

This is redundant with the existing marshalValue function. The memory optimization is good, but we should make that memory optimization in a single place for all callers, either in this PR or in another 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.

I've switched to marshalValue. Optimization can be done in another PR.

Comment on lines 810 to 811
if !cmp.Equal(goterr.Error(), wanterr, cmp.Comparer(assert.CompareErrors)) {
t.Errorf("errors did not match: got %#v, want %q", goterr, wanterr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should use assert.EqualError.

Suggested change
if !cmp.Equal(goterr.Error(), wanterr, cmp.Comparer(assert.CompareErrors)) {
t.Errorf("errors did not match: got %#v, want %q", goterr, wanterr)
assert.EqualError(t, goterr, wanterr, "errors did not match")

@qingyang-hu qingyang-hu requested a review from matthewdale July 16, 2024 21:59
Comment on lines 519 to 526
t.Run("CopyValue error", func(t *testing.T) {
want := errors.New("CopyValue error")
llvrw := &TestValueReaderWriter{t: t, bsontype: TypeString, err: want, errAfter: llvrwReadString}
_, _, got := appendValueBytes(make([]byte, 0), llvrw)
if !assert.CompareErrors(got, want) {
t.Errorf("Errors do not match. got %v; want %v", got, want)
}
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

This subtest should be moved to CopyValueToBytes.

}
})
// tests covering GODRIVER-2779
t.Run("UnmarshalValueWithRegistry with custom registry", func(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Custom registry's can still be defined using NewDecoder. Do we currently apply the test matrix for this test to NewDecoder. If not, instead of removing this test suggest updating it to the new pattern.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to TestUnmarshalWithRegistry in "unmarshal_test.go"

@@ -110,13 +56,11 @@ func BenchmarkSliceCodecUnmarshal(b *testing.B) {
bytes: bsoncore.AppendString(nil, strings.Repeat("t", 4096)),
},
}
reg := NewRegistry()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest using NewDecoder with SetRegistry to preserve the original intention of this benchmark.

Comment on lines +477 to +479
atomic.AddInt32(&c.numConnsCheckedOut, 1)
case event.ConnectionCheckedIn:
c.numConnsCheckedOut--
atomic.AddInt32(&c.numConnsCheckedOut, -1)
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 for racing

@qingyang-hu qingyang-hu added priority-1-high High Priority PR for Review and removed priority-2-medium Medium Priority PR for Review labels Jul 18, 2024
Copy link
Collaborator

@matthewdale matthewdale left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@qingyang-hu qingyang-hu merged commit 4d84e76 into mongodb:master Jul 19, 2024
24 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-1-high High Priority PR for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants