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

One() does not page through the table like All() #248

Closed
dharkness opened this issue Dec 5, 2024 · 4 comments
Closed

One() does not page through the table like All() #248

dharkness opened this issue Dec 5, 2024 · 4 comments

Comments

@dharkness
Copy link

When using the One(&item) call, I sometimes get ErrNotFound when the same query works in PartiQL. The same query using All(&list) finds the item I expect.

var profile map[string]any
err := table.Get("target", "company").
	Index("target-name-index").
	Range("name", dynamo.Equal, "profile").
	Filter("$.$ = ?", "value", "external_id", "901174").
	One(&profile)

vs.

select *
from "profile"."target-name-index"
where
  "target" = 'company'
  and "name" = 'profile'
  and "value"."external_id" = '901174'

After some investigation, I found that One() does not continue scanning the table until it finds an item. It returns an error if the first page finds no results even when there are more pages to scan.

switch {
case len(res.Items) == 0:
	// only do this when res.LastEvaluatedKey == nil
	return ErrNotFound
case len(res.Items) > 1 && q.limit != 1:
	return ErrTooMany
case res.LastEvaluatedKey != nil && q.searchLimit != 0:
	return ErrTooMany
}

I can see why you might argue that it is intentional since the fix would require paging through the whole table to ensure only one match exists, which is less performant than stopping after one page. However, an inconsistent matching behavior seems like a useless feature.

The workaround is to call All(&list) and check that a single result is returned, so I will do that for now. However, I can submit a PR if it's a bug.

@dharkness
Copy link
Author

If you want to maintain the existing behavior for backward compatibility, how about a new method First(&item) that iterates through pages and stops when it finds a match?

@guregu
Copy link
Owner

guregu commented Dec 16, 2024

Hello, I agree this is a footgun/bug. It should probably be changed to search until it finds one page with a result. Personally I think that is more important than scanning the whole table for multiple potential results. In hindsight First(&item) would have been a better API design. ErrTooMany is intended to be a diagnostic at best, and can be silenced with Limit(1).
To be honest it's been so long (it predates PartiQL and filter support) that I can't confidently say it's totally intentional. However, I think it's better to be slow and accurate than fast and misleading, so I'd welcome a bug fix. Anyone who doesn't like the change/fix can always limit the paging with the SearchLimit/RequestLimit params.

One's main purpose is to call the GetItem API when you give it the primary keys; its Query support was a bit of a hack/"convenience". Sometimes I'll use it to do things like "get the next item after [range key]". If I were to redesign everything I'd probably just split them.

guregu added a commit that referenced this issue Dec 16, 2024
guregu added a commit that referenced this issue Dec 17, 2024
* fix Query.One + Filter behavior (#248)

* Query.One: delay unmarshaling until success (preserves old behavior)

* add some docs explaining ErrTooMany
@guregu
Copy link
Owner

guregu commented Dec 17, 2024

v2.3.0 has a fix, added some docs to clarify things as well.

@guregu
Copy link
Owner

guregu commented Dec 28, 2024

I'll close this for now but please re-open if you find any problems

@guregu guregu closed this as completed Dec 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants