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

✨ Adds support for paginated storage queries, and implements pagination for the wallets_list endpoint #3000

Merged
merged 27 commits into from
Jun 11, 2024

Conversation

ff137
Copy link
Contributor

@ff137 ff137 commented May 31, 2024

✨ Adds support for paginated queries, and in particular implements pagination to the multitenant.admin endpoint: wallets_list, with new query params: limit and offset.

New default behavior:

  • wallets_list will now return a default limit of 100 wallets, with a maximum allowable limit of 10'000.
  • 💥 This represents a breaking change because it will no longer fetch all wallets (which is a good breaking change, because it can take minutes to fetch all when there are 100s of 1000s of wallets)

Old behavior is preserved for any BaseRecord.query that doesn't specify a limit or offset, it will still fetch all records.

Additional change that may be worth including: add sort_by query param to wallets_list endpoint, so that the client can customise the sorting used by Askar's scan method. Pending support in Askar.

Notes:

  • I added a change to the integration tests workflow, so that it won't run if a PR is marked as a draft. This was to prevent unnecessary integration tests while work was in progress, and I figure it's worth including this on main.
  • I removed an option (retrieveTags: False) being passed to BaseRecords' find_all_records method. The retrieveTags option is not used anywhere and has no effect.

@dbluhm
Copy link
Contributor

dbluhm commented May 31, 2024

These changes look pretty logical so far. Something that popped into my head as I was reviewing was the BaseStorageSearch classes, like this one for Askar: https://github.com/hyperledger/aries-cloudagent-python/blob/main/aries_cloudagent/storage/askar.py#L203

I wondered if it would make sense to use those classes still; however, the storage search would be good for ACA-Py to use internally for iterating through a large collection efficiently but not for providing pagination to an external client.

All that to say that I can't think of a better way to handle this when it's required.

@ff137 ff137 changed the title 🚧 WIP: Pagination support for querying storage records ✨ Adds pagination support for querying storage records, and implements pagination for listing wallets endpoint Jun 3, 2024
@ff137 ff137 changed the title ✨ Adds pagination support for querying storage records, and implements pagination for listing wallets endpoint ✨ Adds support for paginated storage queries, and implements pagination for the wallets_list endpoint Jun 3, 2024
@ff137 ff137 marked this pull request as ready for review June 5, 2024 10:21
@ff137
Copy link
Contributor Author

ff137 commented Jun 10, 2024

Note: I was able to validate that limit/offset works as expected, and there's no duplicate records across pages. But since there's currently no ORDER BY clause in the askar scan SQL query, there may be inconsistent ordering of results over time.

I've started work separately in aries-askar to allow for custom ordering, so client can have more control over which records are on page 1. That functionality can be introduced in a future PR

ff137 added 19 commits June 11, 2024 11:40
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
…ed_records` if requested

Signed-off-by: ff137 <ff137@proton.me>
…_SIZE` and use in validation. use `limit` and `offset` in records query

Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
….store` reference is resolvable

Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Copy link

sonarcloud bot commented Jun 11, 2024

Copy link
Contributor

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

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

LGTM

@jamshale jamshale merged commit 586a30d into openwallet-foundation:main Jun 11, 2024
8 checks passed
@ff137 ff137 deleted the feat/pagination branch July 4, 2024 22:08
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

Successfully merging this pull request may close these issues.

3 participants