From a1cf2dc39ee02878064d8084dd5aabaa3db7f9cb Mon Sep 17 00:00:00 2001 From: guregu Date: Tue, 17 Dec 2024 04:24:32 +0900 Subject: [PATCH 1/4] fix Query.One + Filter behavior (#248) --- query.go | 44 ++++++++++++++++++-------------------------- query_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 26 deletions(-) diff --git a/query.go b/query.go index ed82375..bc6b553 100644 --- a/query.go +++ b/query.go @@ -239,34 +239,19 @@ func (q *Query) One(ctx context.Context, out interface{}) error { } // If not, try a Query. - req := q.queryInput() - - var res *dynamodb.QueryOutput - err := q.table.db.retry(ctx, func() error { - var err error - res, err = q.table.db.client.Query(ctx, req) - q.cc.incRequests() - if err != nil { - return err - } - - switch { - case len(res.Items) == 0: - return ErrNotFound - case len(res.Items) > 1 && q.limit != 1: - return ErrTooMany - case res.LastEvaluatedKey != nil && q.searchLimit != 0: - return ErrTooMany - } - - return nil - }) - if err != nil { + iter := q.Iter().(*queryIter) + ok := iter.Next(ctx, out) + if err := iter.Err(); err != nil { return err } - q.cc.add(res.ConsumedCapacity) - - return unmarshalItem(res.Items[0], out) + if !ok { + return ErrNotFound + } + // Best effort: do we have any pending unused items? + if iter.hasMore() { + return ErrTooMany + } + return nil } // Count executes this request, returning the number of results. @@ -422,6 +407,13 @@ func (itr *queryIter) Next(ctx context.Context, out interface{}) bool { return itr.err == nil } +func (itr *queryIter) hasMore() bool { + if itr.query.limit > 0 && itr.n == itr.query.limit { + return false + } + return itr.output != nil && itr.idx < len(itr.output.Items) +} + // Err returns the error encountered, if any. // You should check this after Next is finished. func (itr *queryIter) Err() error { diff --git a/query_test.go b/query_test.go index d4ac428..4942067 100644 --- a/query_test.go +++ b/query_test.go @@ -2,6 +2,7 @@ package dynamo import ( "context" + "errors" "reflect" "testing" "time" @@ -111,6 +112,30 @@ func TestGetAllCount(t *testing.T) { t.Errorf("bad result for get one. %v ≠ %v", one, item) } + // trigger ErrTooMany + one = widget{} + err = table.Get("UserID", 42).Range("Time", Greater, "0").Consistent(true).One(ctx, &one) + if !errors.Is(err, ErrTooMany) { + t.Errorf("bad error from get one. %v ≠ %v", err, ErrTooMany) + } + + // suppress ErrTooMany with Limit(1) + one = widget{} + err = table.Get("UserID", 42).Range("Time", Greater, "0").Consistent(true).Limit(1).One(ctx, &one) + if err != nil { + t.Error("unexpected error:", err) + } + if one.UserID == 0 { + t.Errorf("bad result for get one: %v", one) + } + + // trigger ErrNotFound via SearchLimit + Filter + One + one = widget{} + err = table.Get("UserID", 42).Range("Time", Greater, "0").Filter("Msg = ?", item.Msg).Consistent(true).SearchLimit(1).One(ctx, &one) + if !errors.Is(err, ErrNotFound) { + t.Errorf("bad error from get one. %v ≠ %v", err, ErrNotFound) + } + // GetItem + Project one = widget{} projected := widget{ From 4252c1f7b4a85e0e847e6e9badb81b194faf250b Mon Sep 17 00:00:00 2001 From: guregu Date: Tue, 17 Dec 2024 04:48:57 +0900 Subject: [PATCH 2/4] Query.One: delay unmarshaling until success (preserves old behavior) --- query.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/query.go b/query.go index bc6b553..b998e5a 100644 --- a/query.go +++ b/query.go @@ -240,7 +240,8 @@ func (q *Query) One(ctx context.Context, out interface{}) error { // If not, try a Query. iter := q.Iter().(*queryIter) - ok := iter.Next(ctx, out) + var item Item + ok := iter.Next(ctx, &item) if err := iter.Err(); err != nil { return err } @@ -251,7 +252,7 @@ func (q *Query) One(ctx context.Context, out interface{}) error { if iter.hasMore() { return ErrTooMany } - return nil + return unmarshalItem(item, out) } // Count executes this request, returning the number of results. From 9fd42c94d377d4d479de17fd81e40e9293362fe7 Mon Sep 17 00:00:00 2001 From: guregu Date: Tue, 17 Dec 2024 08:18:35 +0900 Subject: [PATCH 3/4] clean this a bit --- query.go | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/query.go b/query.go index b998e5a..767a825 100644 --- a/query.go +++ b/query.go @@ -239,7 +239,7 @@ func (q *Query) One(ctx context.Context, out interface{}) error { } // If not, try a Query. - iter := q.Iter().(*queryIter) + iter := q.newIter(unmarshalItem) var item Item ok := iter.Next(ctx, &item) if err := iter.Err(); err != nil { @@ -300,6 +300,14 @@ func (q *Query) Count(ctx context.Context) (int, error) { return count, nil } +func (q *Query) newIter(unmarshal unmarshalFunc) *queryIter { + return &queryIter{ + query: q, + unmarshal: unmarshal, + err: q.err, + } +} + // queryIter is the iterator for Query operations type queryIter struct { query *Query @@ -451,11 +459,7 @@ func (itr *queryIter) LastEvaluatedKey(ctx context.Context) (PagingKey, error) { // All executes this request and unmarshals all results to out, which must be a pointer to a slice. func (q *Query) All(ctx context.Context, out interface{}) error { - iter := &queryIter{ - query: q, - unmarshal: unmarshalAppendTo(out), - err: q.err, - } + iter := q.newIter(unmarshalAppendTo(out)) for iter.Next(ctx, out) { } return iter.Err() @@ -464,11 +468,7 @@ func (q *Query) All(ctx context.Context, out interface{}) error { // AllWithLastEvaluatedKey executes this request and unmarshals all results to out, which must be a pointer to a slice. // This returns a PagingKey you can use with StartFrom to split up results. func (q *Query) AllWithLastEvaluatedKey(ctx context.Context, out interface{}) (PagingKey, error) { - iter := &queryIter{ - query: q, - unmarshal: unmarshalAppendTo(out), - err: q.err, - } + iter := q.newIter(unmarshalAppendTo(out)) for iter.Next(ctx, out) { } lek, err := iter.LastEvaluatedKey(ctx) @@ -477,12 +477,7 @@ func (q *Query) AllWithLastEvaluatedKey(ctx context.Context, out interface{}) (P // Iter returns a results iterator for this request. func (q *Query) Iter() PagingIter { - iter := &queryIter{ - query: q, - unmarshal: unmarshalItem, - err: q.err, - } - return iter + return q.newIter(unmarshalItem) } // can we use the get item API? From c3fe79b971d9873c2743a35b3fa94cf1283eb134 Mon Sep 17 00:00:00 2001 From: guregu Date: Wed, 18 Dec 2024 04:56:16 +0900 Subject: [PATCH 4/4] add some docs explaining ErrTooMany --- query.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/query.go b/query.go index 767a825..2231e07 100644 --- a/query.go +++ b/query.go @@ -208,6 +208,9 @@ func (q *Query) ConsumedCapacity(cc *ConsumedCapacity) *Query { // One executes this query and retrieves a single result, // unmarshaling the result to out. +// This uses the DynamoDB GetItem API when possible, otherwise Query. +// If the query returns more than one result, [ErrTooMany] may be returned. This is intended as a diagnostic for query mistakes. +// To avoid [ErrTooMany], set the [Query.Limit] to 1. func (q *Query) One(ctx context.Context, out interface{}) error { if q.err != nil { return q.err