feat: re-write get all routes to use pagination

[michael-genson]

Starting a draft since I'm expecting changes and a bunch of feedback. I haven't fully rolled out all the route changes yet, I've just started with shopping lists since I'm more familiar with those.

---

This feature aims to solve for task #1411: to implement the existing pagination logic in place of the ad-hoc get_all logic we're using today. I think this implementation is pretty straightforward, but I have a few things I'd like some feedback/help on.

I extended the PaginationBase per #1411, and I created a ShoppingListPagination schema using the base.

General questions:

1. Did I use the GetAll schema correctly? I'm still new to some of these patterns and I kind of guessed on this one. The GetAll schema is being used in the route. I'm unsure what purpose it serves that the PaginationQuery doesn't, but I see similar patterns on other routes so I'm probably missing something.

@router.get("", response_model=ShoppingListPagination)
    def get_all(self, q: GetAll = Depends(GetAll)):
        return self.repo.pagination(
            pagination=q,
            override=ShoppingListSummary,
        )

2. I'm unsure of the best way to implement the next and previous attributes. The logic is simple (if we're not on page 1, include the previous page; if there are more records to fetch, include the next page) but I'm not sure what the best way to generate the route URL is without reusing a ton of code. I thought about capturing the original route's URL and parsing/modifying the parameters, but I wanted to hear your thoughts before I implemented that.

Next steps

• confirm GetAll is being used correctly
• implement next and previous pages in the pagination response
• create pagination classes for all query-able objects and implement their route changes
• check frontend references and adjust as needed
• verify if any tests need to be revised
• create new pagination tests
• remove old get_all ad-hoc methods requires additional work to fully deprecate, out of the scope of this PR
• debug total_pages calculation (see below)

[hay-kot]

Did I use the GetAll schema correctly? I'm still new to some of these patterns and I kind of guessed on this one. The GetAll schema is being used in the route. I'm unsure what purpose it serves that the PaginationQuery doesn't, but I see similar patterns on other routes so I'm probably missing something.

Pretty sure that's how it works. You can check if the parameters are being picked up by using the API Docs a :9000/docs. I was developing the Pagination stuff in isolation with the ultimate goal to replace the GetAll with the PaginationQuery, so eventually the GetAll would go away once it's not needed anymore.

I'm unsure of the best way to implement the next and previous attributes. The logic is simple (if we're not on page 1, include the previous page; if there are more records to fetch, include the next page) but I'm not sure what the best way to generate the route URL is without reusing a ton of code. I thought about capturing the original route's URL and parsing/modifying the parameters, but I wanted to hear your thoughts before I implemented that.

What about attaching a method set to the PaginationBase model for set_next() and set_prev().

class PaginationBase(GenericModel, Generic[DataT]):
    total: int = 0
    total_pages: int = 0
    data: list[DataT]
    next: Optional[str]
    previous: Optional[str]

    def set_next(self, route: str, query_params: dict[str, Any]) -> None:
        if self.page >= self.total_pages:
            self.next = None

        query_params["page"] = self.page + 1
        self.next = f"{route}?" + "&".join([f"{k}={v}" for k, v in query_params.items()])

    def set_prev(self, route: str, query_params: dict[str, Any]) -> None:
        if self.page <= 1:
            self.previous = None

        query_params["page"] = self.page - 1
        self.next = f"{route}?" + "&".join([f"{k}={v}" for k, v in query_params.items()])

Then you could do the page calculation and just attach whatever other parameters came with the original request and it could be reused across implementation of the PaginationBase class.

You could probably get the host/url from the Request object and then just use the defined Pydantic Model that is being used for the Query Parameters and pass that to the PaginationResponse as a dict before you return it, or just hard code it for the routes as needed.

There's probably some edge cases we'd need to handle for list type queries, but that's what tests are for 😄

---

Hopefully that was helpful, and not just nonsense

[michael-genson]

Ah yeah, that helps a bunch, thanks! That gives me enough to go on I think

[michael-genson]

These commits replace the GetAll class and implement the pagination guides (next and previous). The FastAPI route doesn't provide the base URL, but I think that's fine, unless you think we should get it from the app config and merge them.

So it looks like this:

{
  "page": 1,
  "per_page": 1,
  "total": 6,
  "total_pages": 7,
  "data": [...],
  "next": "/groups/shopping/lists?page=2&per_page=1&order_by=name&order_direction=asc",
  "previous": null
}

So it's up to the API consumer to append the route to the base URL. But you need the base URL stored somewhere anyway to connect to the API, so I feel like that's fine.

Unrelated, it looks like I need to debug total_pages

[michael-genson]

This is close to being done, there are still a few additional tasks (particularly tests). Some things we're going to want to address after this PR is completed:

1. I couldn't fully deprecate the get_all method. This is probably fine, but it may be worth checking the last few dependencies and seeing if we can move them to paging. The major blocker is filtering. Speaking of filtering...
2. Implement filtering options. This shouldn't be too difficult, we just need to add an additional parameter on the page_all method that adds filters to the query. A generic filter can be subclassed (like the PaginationBase schema). This will also open the door for more efficient searching options.
3. The frontend should properly paginate now that we have a unified backend for it. I might tackle that after this PR is done.

[michael-genson]

I changed all of the frontend routes to support the paging route by hard-coding page = 1 and pageSize = -1 just so the behavior wasn't changed (except for the "all recipes" page which does actually use paging). We should properly implement paging on the frontend, though.

[michael-genson]

@hay-kot do you know why the postgres tests would be failing and not the sqlite tests? I'm trying to read the details but I'm lost. I'm not familiar enough with the differences between the two. Unsurprisingly the tests that are failing are the ones I had to tweak.

[michael-genson]

Need to resolve the postgres test failures and create new test cases for pagination before this PR is complete/ready for review

[michael-genson]

Should be ready now! Worth some extra attention to the frontend, though nothing should have changed

[hay-kot]

Heyo! This looks EXCELLENT, thank you so much for all the work here. I'm working on getting this merged today, there's a few small changed I want to make.

1. change data to items - Seeing data.data everyone made me think it was just going to lead to confusion at some point.
2. Upgrade some type annotations to use the | instead of optional
3. Fix some Frontend Type errors

I'm going to wrap up those small changes and try to get the TS errors to show up in CI so you might see a lot of traffic on this PR this afternoon...

[michael-genson]

Thank you! Yeah good call on the data.items, it smelled a bit to leave it as data.data but I couldn't think of anything less confusing at the time. Items is great haha.

Ah yeah the pipes are much easier to read, didn't realize that was new in 3.10.

I'm excited to see this one merged! It unblocks a lot of optimizations, particularly the frontend sorting fun

[hay-kot]

FYI I had to revert some changes you made to the headers in

• feature: added "last-modified" header to supported record types #1379

Basically it rendered the shopping list unusable. If you created a list and tried to add items it would never refetch because it would use the some built in browser caching to determine that it didn't need to make another request and would use it's last result. I'm not sure clear on all the details that caused it, but removing the route_class arg fixed it. Not sure how wide spread the issue is either. The Cookbooks at least seemed to work, but I didn't do a thorough check on everything that was changed.

This should be ready to merge provided CI passes, then we can take another looking at the caching issue.

[michael-genson]

FYI I had to revert some changes you made to the headers in

• feature: added "last-modified" header to supported record types #1379

Basically it rendered the shopping list unusable. If you created a list and tried to add items it would never refetch because it would use the some built in browser caching to determine that it didn't need to make another request and would use it's last result. I'm not sure clear on all the details that caused it, but removing the route_class arg fixed it. Not sure how wide spread the issue is either. The Cookbooks at least seemed to work, but I didn't do a thorough check on everything that was changed.

Huh, that sucks. I haven't had any issues with the shopping list, but since it's cache related I can imagine there's a ton of variables there.

This should be ready to merge provided CI passes, then we can take another looking at the caching issue.

Awesome news!!