aries-cloudagent-python icon indicating copy to clipboard operation
aries-cloudagent-python copied to clipboard

:sparkles: Add ordering options to askar scan and fetch_all methods

Open ff137 opened this issue 1 year ago • 8 comments

:sparkles: Adds order_by and descending options to paginated scan or fetch_all methods

❗ This PR is waiting for next official Askar release

:construction: At present, uses TestPyPI package to test ordering functionality introduced in Askar: https://github.com/hyperledger/aries-askar/pull/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 avatar Aug 16 '24 20:08 ff137

Marking ready for review just to see integration test results

ff137 avatar Aug 19 '24 09:08 ff137

@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.

jamshale avatar Aug 19 '24 19:08 jamshale

@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: https://github.com/didx-xyz/aries-cloudapi-python/pull/989, using this ACA-Py branch, so it seems it builds fine in different contexts

ff137 avatar Aug 19 '24 21:08 ff137

For interest, the following commit shows the end-to-end tests that asserts unique records are returned across pages, when scanning with limit/offset: https://github.com/didx-xyz/aries-cloudapi-python/pull/989/commits/00c3f0f0369481d20a00da997ba56cb632887ee1

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 avatar Aug 19 '24 21:08 ff137

@jamshale I managed to get integration tests passing - using an aries_askar TestPyPI package 👀 https://github.com/hyperledger/aries-cloudagent-python/pull/3173/commits/90cd53cc3f736ffc85128975ffca4f8484950b75

Last couple commits here show how I uploaded to TestPyPI: https://github.com/ff137/aries-askar/pull/1

ff137 avatar Aug 20 '24 15:08 ff137

@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.

jamshale avatar Aug 20 '24 15:08 jamshale

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarCloud

sonarqubecloud[bot] avatar Aug 30 '24 14:08 sonarqubecloud[bot]

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarCloud

sonarqubecloud[bot] avatar Oct 22 '24 13:10 sonarqubecloud[bot]

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?

swcurran avatar Nov 25 '24 18:11 swcurran

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

ff137 avatar Nov 25 '24 19:11 ff137

When the new askar version is pulled in, I'll wrap this one up.

ff137 avatar Jan 21 '25 16:01 ff137

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarQube Cloud

sonarqubecloud[bot] avatar Jan 29 '25 17:01 sonarqubecloud[bot]