fastcrud icon indicating copy to clipboard operation
fastcrud copied to clipboard

Pagination fix

Open Simon128 opened this issue 11 months ago • 4 comments

Pull Request Template for FastCRUD

Description

This pull request is about fixing issue #197 including some pagination related changes listed below.

Changes

  • [x] Support for cursor based pagination on _read_items function in EndpointCreator (single PK and composite)
  • [x] Added ASC column sorting by model primary key for pagination in _read_items
  • [x] Added cursor field in pagination response and changed has_more to work with cursor based pagination
  • [x] Added cursor parsing function based on SqlAlchemy column.type.python_type (see considerations below)
  • [x] Allow for multiple sort columns and tuple cursor in FastCRUD.get_multi_by_cursor for composite PKs
  • [x] Added missing _parse_filters in FastCRUD.get_multi_by_cursor
  • [x] Added missing total_count in return value of FastCRUD.get_multi_by_cursor if corresponding argument is set to True
  • [x] Changed dev-dependency of testcontainers in pyproject.toml, as this was a source of errors for me when running pytest

Considerations

  1. Using SQLAlchemy column.type.python_type to parse the cursor might be error prone, as I assume a str-type if python_type is not implemented (e.g. AutoStr for SqlModel) -- maybe there is a better way?
  2. The first "page" with cursor pagination can be retrieved by setting cursor=True in the query params. This is considered when parsing the cursor.
  3. Composite cursors are assumed to be comma-separated in the url query param
  4. FastCRUD.get_multi_by_cursor has now the arguments sort_columns as well as the previously existing sort_column for backward compatibility. Do we want to include a deprecation warning for sort_column, as this is a bit redundant, or do we want to keep both for convenience? Multiple sort orders for different parts of the (possibly) composite PK is not an option, as I use tuple-comparison for the cursor in the case of composite PKs (stolen from here: https://stackoverflow.com/questions/68953290/sqlalchemy-query-with-where-with-comparison-on-tuple-of-multiple-columns).
  5. I did not include https://github.com/igorbenav/fastcrud/issues/197#issuecomment-2639966906 yet, as this could be a breaking change. Until now, sorting_columns in get_multi_joined are only allowed if they exist on the (not-joined) original model. If I introduce the discussed changes, It would suddenly be necessary for the user to for example set "test.name" as sorting column and not just "name" anymore. Even If I introduce a uniqueness check on the columns when applying the sorting, this could still break if for example the joined model also has a name attribute. I think this breaking change would be necessary though, as imo just supplying "name" is unclear to the user of the function if they join multiple tables with a name attribute (or is it stated somewhere, that they can only sort on the main tables attributes?) I think checking for unique columns and allowing sort_columns without the table name if the attribute is unique might be a good compromise between backwards compatibility and clearness.

Tests

All test stuff was equivalently added to sqlalchemy and sqlmodel.

  • Added test_data_multipk_list function to conftest.py as I did not wanna touch test_data_multipk
  • Added several more tests in crud/test_get_multi as well as endpoint/test_get_paginated to test the fixed Bug as well as the new cursor based pagination endpoint.

Checklist

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

Additional Notes

Not done yet (have to check the code style stuff and add to the documentation). But wanted to get early feedback as well as open the discussion to the points of considerations.

Simon128 avatar Feb 08 '25 21:02 Simon128

@Simon128 is this ready for review?

igorbenav avatar Feb 25 '25 21:02 igorbenav

@Simon128 is this ready for review?

It's ready for review. What do you use to generate the github pages from the docs folder? Is there something I can install to serve them locally, to checkout my changes in the doc?

Also, be aware that a lot of files have been changed by the "ruff format" command. If you don't want this, we can just revert that single commit.

Simon128 avatar Mar 15 '25 10:03 Simon128

@Simon128 you can just install mkdocs material and run mkdocs serve. Tbh I should add this to the pyproject with a in a "docs" group.

igorbenav avatar Mar 20 '25 04:03 igorbenav

Plus, since your at it, maybe take a look at this one as well

igorbenav avatar Mar 24 '25 05:03 igorbenav