Improve limit and offset in PiccoloCRUD

[dantownsend]

Currently, the __page and __page_size query params aren't documented.

Also, PiccoloCRUD should have a max_page_size argument, to limit abuse of an endpoint.

If the max_page_size is exceeded, an error should be returned. A 403 feels most appropriate, with a body such as The page size limit has been exceeded.

[sinisaos]

Hi Daniel
Do you want set max_page_size in general or in PiccoloCRUD init method so that end user can change that param. For general we can achive this with small code changes like this in crud endpoints.py.

class PiccoloCRUD(Router):
    
    # Same limit as in csv export. 
    # With this we don't need to change anything in piccolo_admin
    max_page_size : int = 1000
    
    # unchanged code 
    
    async def _get_all(
        self, params: t.Optional[t.Dict[str, t.Any]] = None
    ) -> Response:
        
        # unchanged code 
            
        # Pagination
        page_size = split_params.page_size or self.page_size
        if page_size > self.max_page_size:
            return JSONResponse(
                {
                    "error": "The page size limit has been exceeded",
                },
                status_code=403,
            )
                
        # rest of unchanged code 

If we want to change max_page_size in Piccolo CRUD init method we need to adjust CSV export and dynamic page size selector in piccolo_admin if end user set smaller max_page_size than 1000 (for CSV) or 100 (for dynamic page size selector). I think that first choice is easier for prevent abuse of an endpoint and I think that we shouldn't allow (like in second choice) end user ability to change max_page_size param, because end user can set limit too high. What do you think about that?

[dantownsend]

@sinisaos I think just setting max_page_size in the constructor for PiccoloCRUD is OK - we could have it default to something quite large (say 1000).

The user should never be able to override this using URL params.

create_admin could also accept a max_page_size attribute, and pass it to any child PiccoloCRUD instances. We wouldn't be able to set specific limits per endpoint, but this is probably OK.

It's a good point with the CSV download. We could expose the max_page_size value in an endpoint like /api/meta, /api/tables/<tablename>/meta, or potentially even add it to /api/tables/<tablename>/schema somehow. What do you think?

[sinisaos]

@dantownsend We can do this via constructor and make changes like you said, but if we set max_page_size as a class variable and set it to 1000 (like code example in above comment), we only need to make changes to PiccoloCRUD crud endpoints.py because the class variable will be accesible to all instances and we don't need to change anything else. CSV export will work because the limit is already 1000. Also dynamic page size changes will work because max limit of 100 records per page will be smaller then 1000. If you don't like this way we can do it from constructor.

[dantownsend]

@sinisaos I think adding it to the constructor should be OK.

class PiccoloCRUD(Router):
    """
    Wraps a Piccolo table with CRUD methods for use in a REST API.
    """

    def __init__(
        self,
        table: t.Type[Table],
        read_only: bool = True,
        allow_bulk_delete: bool = False,
        page_size: int = 15,
        max_page_size: int = 1000
    ) -> None:
        self.max_page_size = max_page_size
        ...

If the default is set to 1000, it shouldn't effect much of the existing code.

[sinisaos]

@dantownsend It's OK if you don't override in instance with max_size_page in piccolo_admin endpoints.py like this:

table_routes: t.List[BaseRoute] = [
    Mount(
        path=f"/{table._meta.tablename}/",
        app=PiccoloCRUD(
            table,
            read_only=read_only,
            page_size=page_size,
            max_page_size=50, # if you do that CSV export and dynamic page size don't work
        ),
    )
    for table in tables
]

and we need to change more files in piccolo_admin to make this work.

If max_page_size is set as a class variable we can't change it in PiccoloCRUD instance and CSV export (because the limit is already 1000) will work, and dynamic page size will work (because max limit of 100 records per page will be smaller then 1000).
Sorry, I can't explain it better but try it with example from my first comment and you see what I mean :).

[dantownsend]

@sinisaos Ah, I understand now! Sorry.

What you're saying, is don't make the maximum configurable at all. I think that works for a first version, and as you say, it means less changes to other code.

[sinisaos]

@dantownsend Yes exactly that :). If you want I can make PR with this code changes that you can make proper look at this.

[dantownsend]

@sinisaos That would be great - thanks!

[sinisaos]

@dantownsend Done. Cheers.