Adding totalCount in non-relay pagination: suggestion regarding approaches

[stoicsleuth]

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 blb-ventures/strawberry-django-plus#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

2. 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

• We're using Polar.sh so you can upvote and help fund this issue.
• We receive the funding once the issue is completed & confirmed by you.
• Thank you in advance for helping prioritize & fund our backlog.

[bellini666]

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]

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]

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 :)

[alainburindi]

Hey @stoicsleuth, we are currently facing the same problem where we need to have the totalCount without using relay. Would you mind sharing more about the solution you are using? with more details on how the CustomStrawberryField works and the class it extends.

the CustomStrawberryField I created extends the StrawberryDjangoField but the apply_pagination method method is not being called. As a result non_paginated_qset is not defined

[thclark]

Folks, I've put a significant sponsorship on this issue because it's beginning to kill me a bit. I think it'll take me two days to figure out so there's a 'two days of effort' bounty on there (better to share the money than take the pain!). For me to release that, I think the reasonable criteria are:

• a solution to the problem using either the above or a fully different approach (like @bellini666 's idea to make the relay connection more generic)
• tested, reviewed/accepted and merged to main
• the pagination documentation updated to show usage
• works where there are multiple foreignkeys (each of which are paginated) on a type (ie if using annotations as part of the solution, they need to be subqueried to avoid conflicts per this thread)
• works with the django guardian extension (eg using HasRetValPerm, the count should only display the number of objects the user has permission to see)

I hope that sounds sensible.

[bellini666]

@thclark nice, thank you for the sponsorship!

I'll tackle that during the weekend :). And yes, your criteria sound totally reasonable

[thclark]

@thclark nice, thank you for the sponsorship!

I'll tackle that during the weekend :). And yes, your criteria sound totally reasonable

my hero!

[bellini666]

@thclark I'm leaning toward the idea of making relay connections more generic. I just opened a PR on strawberry for that: strawberry-graphql/strawberry#3669

Let me just wait to get some feedback from the other devs (specially @patrick91 ) to make sure this is something that we really want, and then my idea is to leverage that new generic Connection in here to make it work for non-relay projects (while still working for the relay ones).

What do you think? Do you see any issues with this direction?

UPDATE:

Actually, after thinking more about this, even though it makes sense to make the relay's Connection not relay-specific, which I'm still going to do either way, I think it makes sense for strawberry-django to have its own Paginated results that act together with the limit/offset that we have and that can also be extended, without having to worry about cursors.

Having said that, I started that in this PR: #642

Can you please take a look and give me your feedback on whether this is what you are expecting from this functionality? The initial tests I wrote should give you a good idea on how it would work.

If the idea is good, I'll complete it by adding tests to ensure that it plays well with the optimizer, permissions, etc and also update the documentation to explain how to use it.

[bellini666]

@thclark #642 is basically done code-wise, I just need to update the docs, which I will do tomorrow.

Let me know if the solution there is what you are expecting :)

[bellini666]

@thclark finished the documentation, the PR is done for review