openwallet-foundation / askar

Secure storage designed for Hyperledger Aries agents.
Apache License 2.0
63 stars 51 forks source link

✨ add `order_by` and `descending` options to scan and fetch_all queries #291

Closed ff137 closed 3 months ago

ff137 commented 4 months ago

Closes #290

Adds ordering functionality to scan and fetch_all queries, to allow for custom ordering of fetched storage records.

ff137 commented 4 months ago

Ready for review!

I wanted to test compatibility with ACA-Py, but building and installing the python wrapper from my local branch, and running pytest in ACA-Py, gives me:

...
INTERNALERROR>     get_library()
INTERNALERROR>   File "/root/anaconda3/envs/acapy/lib/python3.12/site-packages/aries_askar/bindings/__init__.py", line 44, in get_library
INTERNALERROR>     LIB.loaded
INTERNALERROR>   File "/root/anaconda3/envs/acapy/lib/python3.12/site-packages/aries_askar/bindings/lib.py", line 352, in loaded
INTERNALERROR>     self._lib = LibLoad(self.__class__.LIB_NAME)
INTERNALERROR>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
INTERNALERROR>   File "/root/anaconda3/envs/acapy/lib/python3.12/site-packages/aries_askar/bindings/lib.py", line 106, in __init__
INTERNALERROR>     self._load_library()
INTERNALERROR>   File "/root/anaconda3/envs/acapy/lib/python3.12/site-packages/aries_askar/bindings/lib.py", line 129, in _load_library
INTERNALERROR>     raise AskarError(
INTERNALERROR> aries_askar.error.AskarError: Library not found in path: aries_askar

This resolves when I revert to installing official pypi package with pip install aries_askar. So, there must be some extra step required when calling pip install . in the aries-askar/wrappers/python dir.

I tried adding aries-askar/target/release to LD_LIBRARY_PATH, but no dice.

If anyone can assist or direct me on how to set this up for local testing with ACA-Py, will be much appreciated!

ff137 commented 4 months ago

A logical extension would be to add the custom ordering option to "fetch all" queries as well (in a future PR)

ff137 commented 3 months ago

Good news! Managed to successfully test the changes locally 🚀 after copying target/release/libaries_askar.so to wrappers/python/aries_askar 🚚

Happy to confirm that all the ACA-Py tests pass using this branch's changes ✅ Will begin implementing the order_by functionality for ACA-Py soon™️

ff137 commented 3 months ago

I think I'm gonna try add the ordering options for fetching all records -- not just for scanning. Makes sense imo to include it all in one PR

ff137 commented 3 months ago

I've expanded ordering options to the fetch_all method as well.

I can confirm python wrapper works with changes (ACA-Py tests pass using this build).

JavaScript wrapper still has some tests that remain to be fixed ...

ff137 commented 3 months ago

@andrewwhitehead Please let me know if you've had moment to review, if there are any additional steps necessary here. Maybe changing default behavior, or implementing some specific tests.

ACA-Py tests are passing with these changes here: https://github.com/hyperledger/aries-cloudagent-python/pull/3173

And our end-to-end tests, asserting unique results across pages, are passing now (as described here: https://github.com/hyperledger/aries-cloudagent-python/pull/3173#issuecomment-2297491140)