strawberry-graphql / strawberry-django

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

Support annotate parameter in field to allow ORM annotations #377

Closed fjsj closed 9 months ago

fjsj commented 9 months ago

Description

Adds support for ORM annotations as fields. Looks like this:

@strawberry_django.type(Project)
class ProjectType(relay.Node):
    milestones_count: int = strawberry_django.field(annotate=Count("milestone"))

And this:

@strawberry_django.type(Milestone)
class MilestoneType(relay.Node):
    @strawberry_django.field(
        annotate={
            "_my_bugs_count": lambda info: Count(
                "issue",
                filter=Q(
                    issue__issue_assignee__user_id=info.context.request.user.id,
                    issue__kind=Issue.Kind.BUG,
                ),
            ),
        },
    )
    def my_bugs_count(self) -> int:
        return self._my_bugs_count  # type: ignore

There are some shortcomings and implementation details that I discuss more in-depth in PR comments. The main one is this only works if DjangoOptimizerExtension is enabled. Although that doesn't look like a major issue to me, because custom prefetches also have the same limitation (see the existing tests/test_optimizer.py::test_query_prefetch_with_callable).

~As of Sept 27 2023, this PR is still missing docs. I'll wait for feedback on implementation and field "syntax" before making the docs.~

I already added some tests, but perhaps we need more. I still also have to test manually a bit more.

--- EDIT: I found more missing stuff:

Types of Changes

Issues Fixed or Closed by This PR

*

Checklist

aprams commented 9 months ago

Would it make sense to split the annotate functionality for ModelProperty, PrefetchInspector and potentially types to a separate PR to avoid complexity creep here?

Regarding type support: The current field support supports also works on the type's queryset itself, correct? I can see how this might help for downstream tasks on the objects which are not explicit fields on the type.

fjsj commented 9 months ago

Would it make sense to split the annotate functionality for ModelProperty, PrefetchInspector and potentially types to a separate PR to avoid complexity creep here?

IMHO, yes, as long as nothing that is expected to work in regular field actually doesn't.

Regarding type support: The current field support supports also works on the type's queryset itself, correct?

Yes, since type already supports select_related and prefetch_related and pass this down to field init, I guess we can just do the same here for annotate.

bellini666 commented 9 months ago

The typing issue is due to pyright's yesterday release.

Going to merge this as is and fix the pyright issue myself, as it is not this PRs fault.