-
Notifications
You must be signed in to change notification settings - Fork 892
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-2443 Make Distinct return a decodable struct #1603
GODRIVER-2443 Make Distinct return a decodable struct #1603
Conversation
API Change Report./mongoincompatible changes##(Collection).Distinct: changed from func(context.Context, string, interface{}, ..../mongo/options.DistinctOptions) ([]interface{}, error) to func(context.Context, string, interface{}, ...*./mongo/options.DistinctOptions) *DistinctResult compatible changesDistinctResult: added |
mongo/single_result.go
Outdated
@@ -22,11 +22,11 @@ var ErrNoDocuments = errors.New("mongo: no documents in result") | |||
// SingleResult represents a single document returned from an operation. If the operation resulted in an error, all | |||
// SingleResult methods will return that error. If the operation did not return any documents, all SingleResult methods | |||
// will return ErrNoDocuments. | |||
type SingleResult struct { | |||
type SingleResult[T bson.Raw | bson.RawArray] struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
T defines the return type of the SingleResult.Raw
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned about the scope of the change required to add this type parameter. One of the challenges with generic types is that they require parameterizing all functions and types that use them, unless only a single type is ever used (see verifySingleResult
).
An alternate approach that doesn't impact any existing types is to add a new type DistinctResult
that holds a SingleResult
and mostly reproduces the API of SingleResult
, changing the return type of Raw
. That creates fewer parameterized type changes in existing code and provides the ability to independently extend the APIs of DistinctResult
and SingleResult
.
E.g.
type DistinctResult struct {
sr *SingleResult
}
func (dr *DistinctResult) Decode(v interface{}) error { return dr.sr.Decode(v) }
func (dr *DistinctResult) Err() error { return dr.sr.Err() }
func (dr *DistinctResult) Raw() (bson.RawArray, error) {
r, err := dr.sr.Raw()
return bson.RawArray(r), err
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some examples of user code that would be impacted by adding a type parameter to SingleResult
. They seem to fall in a few categories:
- Database client wrappers that return a
*mongo.SingleResult
. - Interfaces used for mocking the
Collection
API. - Adding helper functions for using a
*mongo.SingleResult
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see what you mean. It's worth noting that most of these examples will require updates due to GODRIVER-2696. However, I believe it's reasonable to minimize the impact where possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the API of SingleResult
seems to work great for decoding results from Distinct
. I have concerns about the parameterized type.
mongo/single_result.go
Outdated
return dec.Decode(v) | ||
} | ||
|
||
return sr.rdr.Unmarshal(v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to use the configured Registry
and BSONOptions
when unmarshaling the RawValue
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to decode distinct data with a registry, but the BSONOptions are specific to bson.Decoder
which expects the value to decode into to be a map / struct. E.g., the following would throw a runtime error:
var i32s []int32
distincResult.Decode(i32s)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct, there's currently no way to do that. However, when combined with the changes in #1584, the following code using a bson.Decoder
to decode a value works:
func (dr *DistinctResult) Decode(v any) error {
doc := bsoncore.NewDocumentBuilder().
AppendValue("Arr", bsoncore.Value{
Type: bsoncore.TypeArray,
Data: dr.arr,
}).Build()
dec := getDecoder(doc, dr.opts, dr.reg)
s := struct {
Arr any
}{
Arr: v,
}
return dec.Decode(&s)
}
I created GODRIVER-3205 and added it to the Go Driver 2.0 epic to track that improvement, so we can consider that is resolved for now.
mongo/single_result.go
Outdated
@@ -22,11 +22,11 @@ var ErrNoDocuments = errors.New("mongo: no documents in result") | |||
// SingleResult represents a single document returned from an operation. If the operation resulted in an error, all | |||
// SingleResult methods will return that error. If the operation did not return any documents, all SingleResult methods | |||
// will return ErrNoDocuments. | |||
type SingleResult struct { | |||
type SingleResult[T bson.Raw | bson.RawArray] struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned about the scope of the change required to add this type parameter. One of the challenges with generic types is that they require parameterizing all functions and types that use them, unless only a single type is ever used (see verifySingleResult
).
An alternate approach that doesn't impact any existing types is to add a new type DistinctResult
that holds a SingleResult
and mostly reproduces the API of SingleResult
, changing the return type of Raw
. That creates fewer parameterized type changes in existing code and provides the ability to independently extend the APIs of DistinctResult
and SingleResult
.
E.g.
type DistinctResult struct {
sr *SingleResult
}
func (dr *DistinctResult) Decode(v interface{}) error { return dr.sr.Decode(v) }
func (dr *DistinctResult) Err() error { return dr.sr.Err() }
func (dr *DistinctResult) Raw() (bson.RawArray, error) {
r, err := dr.sr.Raw()
return bson.RawArray(r), err
}
@matthewdale would you mind expanding on this concern: #1603 (comment) ? Genericizing |
…ongo-go-driver into GODRIVER-2443-raw-array
@prestonvasquez I added some Sourcegraph search results for code that references |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
0f6cb6e
GODRIVER-2443
Summary
The
Distinct
collection method should return a struct that can be decoded, similar toFindOne
.Background & Motivation
Providing a decodable type will improve user experience. Instead of iterating through an any slice, users can decode same-type data using conventional Go syntax:
Alternatively, if the data returned is not same-type a user can iterate it directly as a bson.RawArray type: