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

✨ Add pagination support for listing Connection, Cred Ex, and Pres Ex records #3033

Merged

Conversation

ff137
Copy link
Contributor

@ff137 ff137 commented Jun 14, 2024

Marking as draft to first confirm functionality works as expected

Edit: Success! I've written a local test to assert that limit/offset does indeed work as expected, and that unique records are returned across pages.


This adds limit and offset query parameters to:

  • listing connection records (in v1 connections API)
  • listing credential exchange records (in v1 and v2 issue_credentials API)
  • listing presentation exchange records (in v1 and v2 present_proof API)

As with #3000: the default behavior is now changed to no longer attempt to fetch all records for these endpoints. A default page size of 100 is used, with a maximum allowable page size of 10'000.

As you can see, adding the functionality is as simple as extending PaginatedQuerySchema for the query parameter schemas, and then to read limit and offset, and passing it to the Records.query method.

There are probably more endpoints where this functionality can be added, so I'm open for suggestions to apply the same elsewhere, but it should be straight forward enough for other contributors to add too.

These are just what I believe to be the essential endpoints, such that issuers/verifiers listing connections or exchange records won't try to read potentially millions of records.

@ff137 ff137 marked this pull request as ready for review June 19, 2024 13:13
@ff137
Copy link
Contributor Author

ff137 commented Jun 19, 2024

The PR Tests passed, but something went wrong with artifact upload. Will empty commit to re-run

@ff137 ff137 force-pushed the feat/paginate-conn-cred-pres-records branch from 8b0f6d1 to bcabff6 Compare June 19, 2024 14:05
jamshale
jamshale previously approved these changes Jun 25, 2024
Copy link
Contributor

@jamshale jamshale left a comment

Choose a reason for hiding this comment

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

This might possibly cause issues with existing UI implementations like https://github.com/bcgov/traction. Should make sure there's something about it in the release notes.

@jamshale
Copy link
Contributor

I guess sonarcloud is complaining because the int(request.query.get("limit", DEFAULT_PAGE_SIZE)) could be a helper function. I don't have a problem with it the way it is.

We could see if we can merge it without changing sonarcloud settings.

@ff137 ff137 force-pushed the feat/paginate-conn-cred-pres-records branch 2 times, most recently from ff551d1 to c8bb1e8 Compare June 25, 2024 16:29
@jamshale
Copy link
Contributor

I tried to update the pr branch with main. Hopefully didn't mess anything up.

Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
@ff137 ff137 force-pushed the feat/paginate-conn-cred-pres-records branch from c8bb1e8 to 7507fe1 Compare June 25, 2024 16:39
@ff137
Copy link
Contributor Author

ff137 commented Jun 25, 2024

@jamshale my fork had diverged with extra commits. Managed to resolve it now. And I was gonna post a comment about how deduplication is sometimes just tedious, but then I figured to just do it 😆 so included a helper method

@ff137 ff137 force-pushed the feat/paginate-conn-cred-pres-records branch from 7507fe1 to f3e33ab Compare June 25, 2024 16:43
Copy link
Contributor

@jamshale jamshale left a comment

Choose a reason for hiding this comment

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

It is a bit nicer with the helper method 👍

@jamshale jamshale requested a review from dbluhm June 25, 2024 16:45
Copy link

sonarcloud bot commented Jun 25, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
7.3% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@jamshale jamshale merged commit a556d56 into openwallet-foundation:main Jun 25, 2024
7 of 8 checks passed
@ff137 ff137 deleted the feat/paginate-conn-cred-pres-records branch June 25, 2024 17:02
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.

2 participants