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 order_by and descending options to scan and fetch_all queries #291

Merged
merged 15 commits into from
Aug 20, 2024

Conversation

ff137
Copy link
Contributor

@ff137 ff137 commented Jul 25, 2024

Closes #290

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

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>
@ff137 ff137 changed the title 🚧 WIP: add ordering options to scan query ✨ add ordering options to scan query Jul 26, 2024
@ff137 ff137 marked this pull request as ready for review July 26, 2024 12:10
@ff137
Copy link
Contributor Author

ff137 commented Jul 26, 2024

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
Copy link
Contributor Author

ff137 commented Jul 26, 2024

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

@ff137
Copy link
Contributor Author

ff137 commented Aug 5, 2024

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 ff137 marked this pull request as draft August 7, 2024 07:38
@ff137 ff137 marked this pull request as ready for review August 15, 2024 12:10
@ff137
Copy link
Contributor Author

ff137 commented Aug 15, 2024

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 ff137 changed the title ✨ add ordering options to scan query ✨ add order_by and descending options to scan and fetch_all queries Aug 16, 2024
Signed-off-by: ff137 <ff137@proton.me>
@ff137
Copy link
Contributor Author

ff137 commented Aug 16, 2024

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

Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
ff137 added a commit to ff137/aries-cloudagent-python that referenced this pull request Aug 16, 2024
Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
ff137 added a commit to ff137/aries-cloudagent-python that referenced this pull request Aug 19, 2024
@ff137
Copy link
Contributor Author

ff137 commented Aug 19, 2024

@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: hyperledger/aries-cloudagent-python#3173

And our end-to-end tests, asserting unique results across pages, are passing now (as described here:
hyperledger/aries-cloudagent-python#3173 (comment))

Copy link
Member

@andrewwhitehead andrewwhitehead left a comment

Choose a reason for hiding this comment

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

Thanks, I think it looks good!

@andrewwhitehead andrewwhitehead merged commit 75226af into hyperledger:main Aug 20, 2024
29 checks passed
@ff137 ff137 deleted the feat/order-by-scan branch August 20, 2024 22:33
ff137 added a commit to ff137/aries-cloudagent-python that referenced this pull request Aug 21, 2024
ff137 added a commit to ff137/aries-cloudagent-python that referenced this pull request Aug 29, 2024
ff137 added a commit to didx-xyz/aries-cloudagent-python that referenced this pull request Aug 29, 2024
* ✨ add `order_by` and `descending` options to query / scan and fetch_all methods

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

* ✨ add `order_by` and `descending` options to PaginatedQuerySchema

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

* ✨ modify `get_limit_offset` to `get_paginated_query_params`

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

* ✨ add ordering to InMemoryStorage scan and fetch_all methods

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

* 🚧 test in-progress aries-askar PR: hyperledger/aries-askar#291

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

* ⬆️ Update lock file

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

* 🎨 fix ruff warning

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

* ✅ fix assertions

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

* 🚧 test aries-askar with TestPyPI package

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

* 🚧 test latest askar testpypi package

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

* 🎨 Update order_by description and default value. Include in schema

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

* ⬆️ Update aries-askar test pypi package to pre-orjson feat release

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

---------

Signed-off-by: ff137 <ff137@proton.me>
ff137 added a commit to didx-xyz/aries-cloudagent-python that referenced this pull request Aug 29, 2024
* ✨ add `order_by` and `descending` options to query / scan and fetch_all methods

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

* ✨ add `order_by` and `descending` options to PaginatedQuerySchema

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

* ✨ modify `get_limit_offset` to `get_paginated_query_params`

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

* ✨ add ordering to InMemoryStorage scan and fetch_all methods

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

* 🚧 test in-progress aries-askar PR: hyperledger/aries-askar#291

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

* ⬆️ Update lock file

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

* 🎨 fix ruff warning

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

* ✅ fix assertions

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

* 🚧 test aries-askar with TestPyPI package

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

* 🚧 test latest askar testpypi package

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

* 🎨 Update order_by description and default value. Include in schema

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

* ⬆️ Update aries-askar test pypi package to pre-orjson feat release

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

---------

Signed-off-by: ff137 <ff137@proton.me>
@TimoGlastra TimoGlastra mentioned this pull request Sep 13, 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 custom ordering in paginated queries
2 participants