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

Adding OrderingSchema for ordering QuerySets #1291

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

aryan-curiel
Copy link
Contributor

@aryan-curiel aryan-curiel commented Sep 2, 2024

Problem

The FilterSchema definition is a simple but effective approach to centralize logic around filtering QuerySets. A similar approach could be followed for ordering.
Current for ordering and filtering in the same handler we would the following:

@api.get("/books")
def list_books(request, filters: BookFilterSchema = Query(...), order_by: list[str] | None = None):
    books = Book.objects.order_by(**order_by)
    books = filters.filter(books)
    return books

But what if we want to limit the API to allow ordering by just some of the fields. Or if we want to customize ordering based on custom fields and logic. What if we want to have a similar approach for other data sources, for example ElasticSearch.

Proposal

This PR propose to include a helper schema class, similar to FilteringSchema, but for ordering. It';s a simple schema class, with only one field: order_by, that accepts a list of string.
The allowed fields can be specified through the Config inner class, and a Pydantic validator will check that the provided query values are part of the allowed fields.
The schema then will provide a .sort() method (similar to .filter()) that we can use to pass the query set, and expect it ordered as a returned value.
The values can be provided using django standard behavior for descending order.

Example

Using it with out-of-the-box definition, allowing all fields from the model.

from ninja import OrderingSchema

@api.get("/books")
def list_books(request, filters: BookFilterSchema = Query(...), ordering: OrderingSchema = Query(...)):
    books = Book.objects.all()
    books = filters.filter(books)
    books = ordering.sort(books)
    return books

Using it with custom definition of allowed fields

from ninja import OrderingSchema

class BookOrderingSchema(OrderingSchema):
    class Config(OrderingSchema.Config):
        allowed_fields = ['name', 'created_at']  # Leaving out `author` field

@api.get("/books")
def list_books(request, filters: BookFilterSchema = Query(...), ordering: BookOrderingSchema = Query(...)):
    books = Book.objects.all()
    books = filters.filter(books)
    books = ordering.sort(books)
    return books

Other ideas not followed

I also considered to have a default value field in the config, but decided to go with field default definition on custom schema level
Another consideration was to create a class method factory in the OrderingSchema, so it can be define inline, but I wasn't sure if it would be used:

@api.get("/books")
def list_books(
    request,
    filters: BookFilterSchema = Query(...),
    ordering: OrderingSchema.with_allowed_fields('name', 'created_at') = Query(...)
):
    ...

Notes

I didn't add field validation with Model definition, to keep the practices followed in the FilterSchema definition.
Also, I think is a good idea to keep this helpers class as simple as possible, and give room to personalization for more complex scenarios. However, let me know if validating allowed_fields with Model fields is something we would like to have, and I can update the PR.

This was really useful in a personal project, were we needed to provide different ordering behaviors for a QuerySet and for an OpenSearch query. We had to do similar personalizations for pagination and filtering.

Added class OrderBaseSchema as an initial placeholder to allow using as
endpoint query params the same way as FilterSchema
Verifies if the provided values are part of the allowed fields
It uses the provided order_by field value and order a provided queryset
with it
@shijl0925
Copy link

How about OrderingSchema.sort support list type sort?

class OrderingSchema(OrderingBaseSchema):
    def sort(self, items: Union[QuerySet, List]) -> Union[QuerySet, List]:
        if self.order_by:
            if isinstance(items, QuerySet):  # type:ignore
                return items.order_by(*self.order_by)
            elif isinstance(items, list) and items:
                def multisort(xs: List, specs: List[Tuple[str, bool]]) -> List:
                    getter = itemgetter if isinstance(xs[0], dict) else attrgetter
                    for key, reverse in specs:
                        xs.sort(key=getter(key), reverse=reverse)
                    return xs

                return multisort(
                    items,
                    [
                        (o[int(o.startswith("-")):], o.startswith("-"))
                        for o in self.order_by
                    ],
                )

        return items

@aryan-curiel
Copy link
Contributor Author

How about OrderingSchema.sort support list type sort?

Adding support for list type is definitely a good idea. However, not sure adding it to this sort method is the most convenient approach.
For example, if we want to support other collections, then we would need to add another type hint, and another elif.
Any extension for supporting other collections or types, might be more suitable as a subclass of the base OrderingBaseSchema. However, we might need to make some small arrangements in the base class for a better definition.

@shijl0925
Copy link

May I ask if there is any progress on this OrderingSchema? I found same failure in these pull request actions. But I can not see these failed logs. What happened about this pull request?

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.

2 participants