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 ordering options to askar scan and fetch_all methods #3173

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

ff137
Copy link
Contributor

@ff137 ff137 commented Aug 16, 2024

✨ Adds order_by and descending options to paginated scan or fetch_all methods

❗ This PR is waiting for next official Askar release

🚧 At present, uses TestPyPI package to test ordering functionality introduced in Askar: openwallet-foundation/askar#291

To-do: test coverage, and point to next aries-askar release that includes this change.

Notes: currently it only appears to ever make sense to order by the "id" column, since record field values are encrypted.
So, it does feel a bit weird allowing users to select an "order_by" column when there's only one valid choice. But, I still believe it's better than nothing. We at least add the functionality for control over ordering, and it can be disabled (for whatever reason) by requesting None. Ordering by id is effectively the same as ordering by time created (AFAICT), so the newest records can be received at the top by selecting descending=True. Perhaps this can be made default behavior.

At present (before this Askar PR is merged), there is no guaranteed ordering when fetching records, for the scan (paginated queries) or fetch all methods. This means there's no solid guarantee that all records will be retrieved while scanning (with increasing offsets), and ordering of records may be inconsistent across many fetch_all calls. So, resolving that is the motive for this contribution.

Will close #3001

@ff137 ff137 marked this pull request as ready for review August 19, 2024 09:40
@ff137
Copy link
Contributor Author

ff137 commented Aug 19, 2024

Marking ready for review just to see integration test results

@ff137 ff137 marked this pull request as draft August 19, 2024 09:57
@jamshale
Copy link
Contributor

@ff137 Is this working for you to test changes to the aries-askar library? I've been trying to do the same thing, with a fork of aries-askar, committing the libaries_askar.* files and installing it with poetry. However, I'm always getting Library not found in path error even though it seems to be there.

I think i might have something wrong with some meta data or my python env.

@ff137
Copy link
Contributor Author

ff137 commented Aug 19, 2024

@jamshale Hmm, I got the same error in the integration run: https://github.com/hyperledger/aries-cloudagent-python/actions/runs/10451102652/job/28936801431
even after committing all the libraries_askar.* files (in the wrappers/python/aries_askar dir)

At first I couldn't run the PR tests locally, but resolved when I added just libraries_askar.so file to the wrappers/python/aries_askar dir. Just needed to reinstall aries_askar. But it seems somethings up in the integration test context, such that having all the library resources committed doesn't resolve it. Not sure what's up there - I'm new to testing askar like this.

But, we could get our end-to-end tests passing here: didx-xyz/aries-cloudapi-python#989, using this ACA-Py branch, so it seems it builds fine in different contexts

@ff137
Copy link
Contributor Author

ff137 commented Aug 19, 2024

For interest, the following commit shows the end-to-end tests that asserts unique records are returned across pages, when scanning with limit/offset: didx-xyz/aries-cloudapi-python@00c3f0f

Those tests had to be commented out because lack of ordering made results inconsistent, and it seems the askar changes + this branch resolves that 😃

Will wait for askar release including those changes, and perhaps adding tests here, before marking this PR ready

@ff137 ff137 marked this pull request as ready for review August 20, 2024 10:35
@ff137 ff137 marked this pull request as draft August 20, 2024 11:30
@ff137 ff137 marked this pull request as ready for review August 20, 2024 13:45
@ff137 ff137 marked this pull request as draft August 20, 2024 15:22
@ff137
Copy link
Contributor Author

ff137 commented Aug 20, 2024

@jamshale I managed to get integration tests passing - using an aries_askar TestPyPI package 👀 90cd53c

Last couple commits here show how I uploaded to TestPyPI: ff137/aries-askar#1

@jamshale
Copy link
Contributor

@jamshale I managed to get integration tests passing - using an aries_askar TestPyPI package 👀 90cd53c

Last couple commits here show how I uploaded to TestPyPI: ff137/aries-askar#1

Great, thanks. I'm going to give this a try.

@ff137 ff137 marked this pull request as ready for review August 21, 2024 17:32
@ff137 ff137 marked this pull request as draft August 21, 2024 18:15
@ff137 ff137 marked this pull request as ready for review August 30, 2024 11:11
@ff137 ff137 marked this pull request as draft August 30, 2024 11:24
Copy link

sonarcloud bot commented Aug 30, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
77.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@ff137 ff137 changed the title 🚧 WIP: test ordering options added to askar scan and fetch_all methods ✨ Add ordering options to askar scan and fetch_all methods Aug 31, 2024
@ff137 ff137 force-pushed the feat/pagination-ordering branch 2 times, most recently from 2a7bc47 to a2cecc4 Compare October 22, 2024 13:34
Signed-off-by: ff137 <[email protected]>
Copy link

sonarcloud bot commented Oct 22, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
76.5% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@swcurran
Copy link
Contributor

Any status update on this PR so we can raise it in the ACA-Pug meeting tomorrow -- 2024.11.16 @ 8:00 Pacific / 17:00 Central Europe?

@ff137
Copy link
Contributor Author

ff137 commented Nov 25, 2024

Essentially just needs an official askar release (currently using my test-pypi package / fork of askar). And some minor rebasing.
We're also using this in our fork for a few months now

Copy link

sonarcloud bot commented Dec 2, 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

Successfully merging this pull request may close these issues.

Support paginated querying of storage records
3 participants