strawberry-graphql / strawberry-django

Strawberry GraphQL Django extension
MIT License
393 stars 115 forks source link

Adding `totalCount` in non-relay pagination: suggestion regarding approaches #408

Open stoicsleuth opened 8 months ago

stoicsleuth commented 8 months ago

Summary

We're using Strawberry Graphql Django in our app and we would need a totalCount attribute for our queries in order show it in the frontend. I had previously raised https://github.com/blb-ventures/strawberry-django-plus/issues/190 in which it was suggested to use a relay node for this purpose.

However, considering our entire application now is built on top of non-relay nodes, we don't necessarily want to change everything to relay nodes at this point. However, we have figured out two approaches to include our custom pagination logic by overriding the StrawberryDjangoField class. Before committing to an approach, we wanted to get some feedback from the team as to which one seems to be more aligned with the library.

Problem Statement

We have create a PaginatedConnection class which we are using as a type for our query nodes to include the count metadata as follows.

@strawberry.type
class PaginationConnection(Generic[ModelReturnType]):
    page_info: PageInfo
    results: List[ModelReturnType]

#which is being used as
paginated_records: PaginationConnection[RecordNode] = CustomStrawberryField()

However, this breaks the strawberry django internal functions as it can't identify the django_type (and consequently django_model) properly anymore.

Proposed Solutions

  1. Override the django_type field in CustomStrawberryField -> Here we are overriding the property to modify the origin to be the RecordNode instead of PaginationConnection. (We saw that this approach is being followed to resolved the relay node type internally in django_type)
    # Because our return type for a paginated node is different than a regular StrawberryNode,
    # we need to figure our the actual StrawberryNode type in order to get results
    @functools.cached_property
    def django_type(self) -> type[WithStrawberryDjangoObjectDefinition] | None:
        # Storing this value so that it can be reassigned to self.type
        origin = self.type
        origin_node = None
        object_definition = get_object_definition(origin)

        if object_definition and issubclass(
            object_definition.origin, PaginationConnection
        ):
            origin_node = object_definition.type_var_map.get("ModelReturnType")

            if isinstance(origin, LazyType):
                origin_node = origin.resolve_type()

        self.type = origin_node
        result_type = super().django_type
        self.type = origin
  1. Passing the Django model explicitly in CustomStrawberryField and then setting the internal django_model in the __init__ function. This will directly set the django_model no matter what the result of `django_type is.
    def __init__(
        self,
        django_model: type[models.Model],
        result_formatter_class: Type[CustomResultFormatMixin],
        *args,
        **kwargs,
    ):
        self.django_model = django_model
        self.result_formatter = result_formatter_class()
        self.non_paginated_qset: QuerySet
        self.paginated_qset: QuerySet
        super().__init__(*args, **kwargs)

     #And then calling the field as 
     paginated_records: PaginationConnection[RecordNode] = CustomStrawberryField(django_model=Record)

As we aren't sure about any proposed future changes to the library's internal API, we wanted to be double sure we are making the correct decision in terms of customizing the StrawberryDjangoField. Please let us know what you folks think about it.

Upvote & Fund

Fund with Polar

bellini666 commented 8 months ago

Hi @stoicsleuth ,

Customizing the StrawberryDjangoField should be safe! I don't see any reason to change its current internal API in the near future.

For curiosity, why is it breaking for you? If you want to show some examples, either here or at discord, I can try to help you find a way to workaround that

stoicsleuth commented 8 months ago

Hey @bellini666, thanks for checking this out.

What was breaking

The thing that was breaking for us was django_model being None, as complained by Strawberry Django. Here are the changes we did to our custom StrawberryDjangoField -

Step 1

Initially we were using the paginated_records query like this.

@strawberry.type
class RecordsQuery:
 paginated_records: RecordNode = strawberry_django.field()

This was working as expected - and Strawberry Django could correctly identity the RecordModel needed to resolve the query, based on the RecordType we have used as the return type of the query.

Step 2

After that, we customized the StrawberryDjangoField and changed the return type of the new CustomStrawberryField to include the metadata about pagination.

These are the changes we did -

CustomStrawberryField:
    ...............
    ...............
    # @override
    def apply_pagination(
        self,
        queryset: QuerySet,
        pagination: Optional[object] = None,
    ) -> QuerySet:
        # apply_pagination is overriden to store the non-paginated queryset
        # originally it can be found in StrawberryDjangoPagination
        self.non_paginated_qset = queryset.all()
        paginated_qset = super().apply_pagination(queryset, pagination)
        paginated_qset = paginated_qset.all()
        return paginated_qset

    def get_result(self, source, info, args, kwargs):
        qset_result = cast(QuerySet, super().get_result(source, info, args, kwargs))
        self.paginated_qset = qset_result
       # This is the custom behavior where we add the `totalCount` metadata to our query results
        return self.result_formatter.format_result(
            self.non_paginated_qset, self.paginated_qset
        )

This will ensure that the query results are in the form of PaginationConnection

@strawberry.type
class PaginationConnection(Generic[ModelReturnType]):
    page_info: PageInfo
    results: List[ModelReturnType]

#which is being used as
paginated_records: PaginationConnection[RecordNode] = CustomStrawberryField()

Step 3

In light of these changes, we then modified the return type in the query definition as well, as follows.

paginated_records: PaginationConnection[RecordNode] = CustomStrawberryField()

And this is where things broke, because Strawberry Django was unable to find the model type of the query from PaginationConnection[RecordNode] type. (It was expecting RecordNode)

Solution

For now, we solved this issue by customizing the django_type property on StrawberryDjangoField like I showed before.

bellini666 commented 8 months ago

Ohhh I see. Yeah, I think that is the way to go right now.

We do have a similar workaround for connections in https://github.com/strawberry-graphql/strawberry-graphql-django/blob/main/strawberry_django/fields/base.py#L74

Wondering if it would make sense to change that code to check for a generic type and retrieve the django_type from its specialized type instead of doing that only for Connection. Although I'm not sure what to do in case the Connection has more than one typevar in it.

Maybe the django_model approach is better, because either way it would have to be used? Feel free to open a PR for this and I'll gladly review it :)