Support annotate parameter in field to allow ORM annotations #377

fjsj · 2023-09-27T20:56:57Z

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:

Support ModelProperty (see strawberry_django/descriptors.py)
Add annotate support to PrefetchInspector. (EDIT 2: seems this is already done via query.annotations)
Does it make sense to have annotate support in type? (see strawberry_django/type.py)
Ensure 100% coverage of new code

Types of Changes

Issues Fixed or Closed by This PR

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

fjsj · 2023-09-27T20:59:01Z

strawberry_django/utils/typing.py

 TypeOrIterable: TypeAlias = Union[_T, Iterable[_T]]
 UserType: TypeAlias = Union["AbstractBaseUser", "AnonymousUser"]
 PrefetchCallable: TypeAlias = Callable[[GraphQLResolveInfo], Prefetch]
 PrefetchType: TypeAlias = Union[str, Prefetch, PrefetchCallable]
+AnnotateCallable: TypeAlias = Callable[[GraphQLResolveInfo], BaseExpression]
+AnnotateType: TypeAlias = Union[BaseExpression, AnnotateCallable]


I have to double-check if BaseExpression is the best or Expression is better. BaseExpression also allows Subquery, and I believe I have examples in some projects were we had to use annotate with Subquery because other annotation types didn't cover our use cases. I'll check.

I might be wrong here, but I do think BaseExpression is the correct annotation indeed.

strawberry_django/optimizer.py

fjsj · 2023-09-27T21:03:33Z

strawberry_django/optimizer.py

+                # Instead of the more redundant:
+                field_store.annotate = {
+                    field.name: field_store.annotate[_annotate_placeholder],
+                }


I don't love the necessity to use _annotate_placeholder. So please check if my comment here really makes sense or we can get rid of that. I think the field object at instantiation time cannot know about what attr name it has inside the parent Type class, because there's no metaclass in Type. AFAIK, in other libraries (such as Django REST Framework), fields know what attrname they got on their parent class because of metaclasses.

I think your comment does make sense, and I actually love the fact that you can use annotate=Sum("price") as a shortcut to annotate={"total": Sum("price")} :)

Getting back to this comment after checking the whole PR: Maybe we don't need this, and instead we can just keep the expression in annotate directly, and this if would be if isinstance(field_store.annotate, BaseExpression) or even if field_store.annotate is not None and not isinstance(field_store.annotate, dict)?

I mean, it would make sense to use _annotate_placeholder if we could have more than 1 annotation together, but I'm assuming that it will never be the case. Or am I missing something?

I guess the problem of not using _annotate_placeholder is that typing will get more confusing in the OptimizerStore. But I'll check if this is doable.

If we make annotate: dict[str, AnnotateType] | AnnotateType = dataclasses.field(default_factory=dict) inside OptimizerStore, this conditional logic will need handling in __ior__, copy, with_prefix and apply methods. Right now, the placeholder logic is only in with_hints. Therefore, to avoid spreading this logic, I guess it's best to leave as is.

Yes, forgot about that. I agree with you, let's leave as it for now :)

fjsj · 2023-09-27T21:06:37Z

strawberry_django/utils/typing.py

@@ -37,10 +39,13 @@
 _Type = TypeVar("_Type", bound="StrawberryType | type")

 TypeOrSequence: TypeAlias = Union[_T, Sequence[_T]]
+TypeOrMapping: TypeAlias = Union[_T, Mapping[str, _T]]


I don't love that annotate needs a dict instead of a list, but I see no other way of specifying the annotation name in a case like this:

@strawberry_django.type(Milestone) class MilestoneType(relay.Node): @strawberry_django.field( annotate={ "_issues_count": Count("issue") }, ) def issues_count(self) -> int: return self._issues_count # type: ignore

The other way would be to create an object to hold that info, like this:

@dataclasses.dataclass class Annotation: field_name: str annotation: BaseExpression | Callable # and then use as annotate=[Annotation(field_name="_issues_count", annotation=Count("issue"))]

But IMO I actually prefer the dict approach as it seems more straight forward

fjsj · 2023-09-27T21:09:45Z

tests/django_settings.py

@@ -11,6 +11,8 @@

 DEBUG = True
 SECRET_KEY = 1
+USE_TZ = True
+TIME_ZONE = "UTC"


Had to add this to avoid test warnings.

fjsj · 2023-09-27T21:10:04Z

tests/projects/schema.py

+        annotate=ExpressionWrapper(
+            Q(due_date__lt=Now()),
+            output_field=BooleanField(),
+        ),


Complex annotation example.

fjsj · 2023-09-27T21:10:30Z

tests/projects/schema.py

+        },
+    )
+    def my_bugs_count(self) -> int:
+        return self._my_bugs_count  # type: ignore


Complex callable annotation example. Considers current user.

fjsj · 2023-09-27T21:10:59Z

tests/test_optimizer.py

+                    {"node_id": e["id"]},
+                    asserts_errors=False,
+                )
+                assert res.errors


Please LMK about additional tests I should implement.

Of course more tests are always nice, but I think you got basically all of the core features covered nicely :)

tests/test_optimizer.py

aprams · 2023-09-28T08:51:38Z

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 · 2023-09-28T12:31:40Z

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

This looks amazing!!

The -1 is for a couple of nitpicks

docs/guide/optimizer.md

bellini666 · 2023-09-28T15:42:38Z

strawberry_django/optimizer.py

+                # Instead of the more redundant:
+                field_store.annotate = {
+                    field.name: field_store.annotate[_annotate_placeholder],
+                }


I think your comment does make sense, and I actually love the fact that you can use annotate=Sum("price") as a shortcut to annotate={"total": Sum("price")} :)

strawberry_django/optimizer.py

bellini666 · 2023-09-28T15:49:54Z

strawberry_django/utils/typing.py

@@ -37,10 +39,13 @@
 _Type = TypeVar("_Type", bound="StrawberryType | type")

 TypeOrSequence: TypeAlias = Union[_T, Sequence[_T]]
+TypeOrMapping: TypeAlias = Union[_T, Mapping[str, _T]]


The other way would be to create an object to hold that info, like this:

@dataclasses.dataclass class Annotation: field_name: str annotation: BaseExpression | Callable # and then use as annotate=[Annotation(field_name="_issues_count", annotation=Count("issue"))]

But IMO I actually prefer the dict approach as it seems more straight forward

bellini666 · 2023-09-28T15:52:10Z

strawberry_django/utils/typing.py

 TypeOrIterable: TypeAlias = Union[_T, Iterable[_T]]
 UserType: TypeAlias = Union["AbstractBaseUser", "AnonymousUser"]
 PrefetchCallable: TypeAlias = Callable[[GraphQLResolveInfo], Prefetch]
 PrefetchType: TypeAlias = Union[str, Prefetch, PrefetchCallable]
+AnnotateCallable: TypeAlias = Callable[[GraphQLResolveInfo], BaseExpression]
+AnnotateType: TypeAlias = Union[BaseExpression, AnnotateCallable]


I might be wrong here, but I do think BaseExpression is the correct annotation indeed.

tests/test_optimizer.py

bellini666 · 2023-09-28T16:03:38Z

tests/test_optimizer.py

+                    {"node_id": e["id"]},
+                    asserts_errors=False,
+                )
+                assert res.errors


Of course more tests are always nice, but I think you got basically all of the core features covered nicely :)

tests/test_type.py

bellini666 · 2023-09-28T16:09:41Z

strawberry_django/optimizer.py

+                # Instead of the more redundant:
+                field_store.annotate = {
+                    field.name: field_store.annotate[_annotate_placeholder],
+                }


Getting back to this comment after checking the whole PR: Maybe we don't need this, and instead we can just keep the expression in annotate directly, and this if would be if isinstance(field_store.annotate, BaseExpression) or even if field_store.annotate is not None and not isinstance(field_store.annotate, dict)?

I mean, it would make sense to use _annotate_placeholder if we could have more than 1 annotation together, but I'm assuming that it will never be the case. Or am I missing something?

bellini666

LGTM 👍🏼

bellini666 · 2023-09-28T18:25:57Z

strawberry_django/optimizer.py

+                # Instead of the more redundant:
+                field_store.annotate = {
+                    field.name: field_store.annotate[_annotate_placeholder],
+                }


Yes, forgot about that. I agree with you, let's leave as it for now :)

bellini666 · 2023-09-28T18:41:05Z

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.

Support annotate parameter in field to allow ORM annotations

961d14e

fjsj commented Sep 27, 2023

View reviewed changes

strawberry_django/optimizer.py Show resolved Hide resolved

Fix comment at _get_model_hints

11dcd68

fjsj commented Sep 27, 2023

View reviewed changes

tests/test_optimizer.py Show resolved Hide resolved

Allow annotate in type

a431bf2

fjsj force-pushed the feature-orm-annotations branch from cd49dfb to a431bf2 Compare September 28, 2023 13:23

fjsj added 2 commits September 28, 2023 11:57

Add annotate support to model_property

56dc516

Document annotate support in Query Optimizer

c946d02

bellini666 requested changes Sep 28, 2023

View reviewed changes

fjsj added 2 commits September 28, 2023 14:10

Fix snapshot test

294f1da

Fix indent

5a09351

fjsj force-pushed the feature-orm-annotations branch from 5f4bc16 to 5a09351 Compare September 28, 2023 17:16

fjsj added 2 commits September 28, 2023 14:43

Use get_django_definition and root instead of self

145517a

Fix typing for test model

3b7de81

fjsj requested a review from bellini666 September 28, 2023 18:17

bellini666 approved these changes Sep 28, 2023

View reviewed changes

bellini666 merged commit e9e22ba into strawberry-graphql:main Sep 28, 2023

fjsj deleted the feature-orm-annotations branch September 28, 2023 20:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support annotate parameter in field to allow ORM annotations #377

Support annotate parameter in field to allow ORM annotations #377

fjsj commented Sep 27, 2023 •

edited

Loading

fjsj Sep 27, 2023

bellini666 Sep 28, 2023

fjsj Sep 27, 2023 •

edited

Loading

bellini666 Sep 28, 2023

bellini666 Sep 28, 2023

fjsj Sep 28, 2023

fjsj Sep 28, 2023

bellini666 Sep 28, 2023

fjsj Sep 27, 2023 •

edited

Loading

bellini666 Sep 28, 2023

fjsj Sep 27, 2023 •

edited

Loading

fjsj Sep 27, 2023

fjsj Sep 27, 2023

fjsj Sep 27, 2023

bellini666 Sep 28, 2023

aprams commented Sep 28, 2023

fjsj commented Sep 28, 2023

bellini666 left a comment

bellini666 Sep 28, 2023

bellini666 Sep 28, 2023

bellini666 Sep 28, 2023

bellini666 Sep 28, 2023

bellini666 Sep 28, 2023

bellini666 left a comment

bellini666 Sep 28, 2023

bellini666 commented Sep 28, 2023

Support annotate parameter in field to allow ORM annotations #377

Support annotate parameter in field to allow ORM annotations #377

Conversation

fjsj commented Sep 27, 2023 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjsj Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjsj Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjsj Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aprams commented Sep 28, 2023

fjsj commented Sep 28, 2023

bellini666 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellini666 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellini666 commented Sep 28, 2023

fjsj commented Sep 27, 2023 •

edited

Loading

fjsj Sep 27, 2023 •

edited

Loading

fjsj Sep 27, 2023 •

edited

Loading

fjsj Sep 27, 2023 •

edited

Loading