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 unpaginated versions of multi-row get methods (w/tests) #62

Merged
merged 6 commits into from
Apr 27, 2024

Conversation

slaarti
Copy link
Contributor

@slaarti slaarti commented Apr 25, 2024

Description

Per #59, this PR is to add new methods to FastCRUD to do multi-row gets (without and with joins) that return all rows in a single list without pagination.

Changes

Added new methods get_multi_all and get_multi_joined_all that are basically copies minus the offset, limit, and return_total_count args as well as without wrapping the list of result rows in a dict (since that happens in the existing methods to help with pagination).

Tests

test_get_multi.py and test_get_multi_joined.py for both sqlalchemy and sqlmodel were copied into new files and adjusted to test the new methods (including removing tests that only made sense in the context of pagination).

Checklist

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • I have added necessary documentation (if appropriate).
  • I have added tests that cover my changes (if applicable).
  • All new and existing tests passed.

@igorbenav
Copy link
Owner

@slaarti could you please refactor? I think doing it this way would be bettet indeed

@igorbenav igorbenav added enhancement New feature or request FastCRUD Methods Related to FastCRUD methods labels Apr 25, 2024
@slaarti
Copy link
Contributor Author

slaarti commented Apr 25, 2024

Okay, sure. Not tonight, might take a couple days, but I'm fine with the alternate approach. 👍

@igorbenav
Copy link
Owner

Nice, thanks!

Copy link

codecov bot commented Apr 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (5e45aee) to head (661357a).

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #62   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           64        64           
  Lines         4282      4336   +54     
=========================================
+ Hits          4282      4336   +54     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Includes update to docstrings about using limit=0, and tests.
Includes tests and doctest updates. I considered using the allow_limit_0
arg/attribute to also bypass flagging itemsPerPage=0 in get_paginated,
but I ran into complications when setting up the tests. Finally I
decided that it went against the entire point of a pagination method to
be able to bypass pagination and just added the flagging.
@slaarti
Copy link
Contributor Author

slaarti commented Apr 26, 2024

Okay, I rolled back the original branch (well, renamed it and did a fresh checkout) and implemented the alternate method, including checks on the endpoint creator and crud router (with a flag to bypass the checks). The force push is for the new branch.

@igorbenav
Copy link
Owner

Sorry, could you refactor it to use None instead of 0? I think someone using 0 to retrieve no record might make sense (think you have a limit for someone to retrieve x objects, he might get 10 objects, then 5 more, you subtract it from x until it becomes zero, then the person can't retrieve).

It's a dumb example, but I think it makes more sense with limit=None.

@slaarti
Copy link
Contributor Author

slaarti commented Apr 27, 2024

I'd opted for 0 as much to avoid having to deal with changes in typing and looking for None vs. 0 as feeling like saying you want zero rows made no sense... but yeah, okay, you make a good point. Will do. 👍

@slaarti
Copy link
Contributor Author

slaarti commented Apr 27, 2024

...Actually, come to think of it, an interesting aspect of changing it to None is that I'm pretty sure there's no good way to pass None as a path/query parameter, which eliminates the need for the changes made for sanitizing the automatically generated endpoints.

Anyone who wants to provide an unlimited get in their (presumably private, as in my cases) API will have to create an endpoint specifically for it, but I'm okay with that.

No need to do sanitization in the automatically generated endpoints
after all: the refactor coming in the next commit will make it so that
unlimited gets are done with limit=None, which can't really be provided
via path or query params.
@igorbenav
Copy link
Owner

I guess everything worked out even better, great!
Thanks @slaarti and welcome to fastcrud contributors🎉

@igorbenav igorbenav self-assigned this Apr 27, 2024
@igorbenav igorbenav merged commit ea8ab1a into igorbenav:main Apr 27, 2024
9 checks passed
@slaarti slaarti deleted the get_multi_all branch April 27, 2024 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request FastCRUD Methods Related to FastCRUD methods
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants