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

Improve declaration and documentation for root queries in django (where multiple apps are used) #636

Open
thclark opened this issue Oct 2, 2024 · 3 comments
Labels
documentation Improvements or additions to documentation question Further information is requested

Comments

@thclark
Copy link
Contributor

thclark commented Oct 2, 2024

The problem

The original title of this issue was "Annotate of _count in query optimizer not working as described in docs (attribute error)" and I've left the issue and my own response to it here. Digging through it, the actual issue is the way in which root queries need to be declared in multi-app setups (ie most setups) in django.

I think the real solution to this is for the getting started to contain improved representation of the realistic case where you want your root Query defined in schema.py and import queries from one or more apps. The django example could also adopt a more representative structure. Otherwise it's just incredibly hard to grow out of the 'getting stated' stage.

It wouldn't hurt to have a structuring your application section of the docs either

The original problem

Describe the Bug

In the query optimizer docs there is an example where we can get the count of a related FK field by annotating the queryset:

#models.py
class Artist(models.Model):
    name = models.CharField()


class Album(models.Model):
    name = models.CharField()
    release_date = models.DateTimeField()
    artist = models.ForeignKey("Artist", related_name="albums")
#types.py
from strawberry import auto
import strawberry_django

@strawberry_django.type(Artist)
class ArtistType:
    name: auto
    albums: list["AlbumType"]
    albums_count: int = strawberry_django.field(annotate=Count("albums"))

But if I make a query like the following:

query ExampleQuery($artistId: Int!) {
  artist(id: $artistId) {
    albumsCount
  }
}

I get the following response:

{
  "data": null,
  "errors": [
    {
      "message": "'Artist' object has no attribute 'albums_count'",
      "locations": [
        {
          "line": 6,
          "column": 5
        }
      ],
      "path": [
        "artist",
        "albumsCount"
      ]
    }
  ]
}

Other information

  • strawberry-graphql version): 0.243.1

System Information

  • strawberry-graphql version: 0.243.1
  • strawberry-graphql-django version: 0.48.0

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.
Fund with Polar
@thclark thclark added the bug Something isn't working label Oct 2, 2024
@thclark
Copy link
Contributor Author

thclark commented Oct 2, 2024

OK, so it turns out the root cause is that this is happening because of this discussion on discord.

The problem is that in an application where you have multiple apps (eg beyond-toybox), it makes sense to define your queries on a per-app basis, and you'll of course give them different names to avoid having several classes named Query flying around

#app1/graphql/queries.py  - the app2/queries.py file looks basically identical

@strawberry.type
class App1Queries:

    artists: List[ArtistType] = sd.field()

    # This automagic shown in the strawberry-django docs doesn't work...
    # (see https://discord.com/channels/689806334337482765/1128356723464339496/1128356723464339496)
    # artist: ArtistType = sd.field()

    # So to make single-item queries work you do...
    @sd.field()
    def artist(self, info: Info, id: int) -> ArtistType:
        return Artist.objects.get(id=id)
        # ...  but this ^^^ queryset of course isn't automagically annotated

Workaround 1: You have to solve it EITHER do:

# in app1/graphql/queries.py

@strawberry.type
class Query:
# this ^^^ MUST be called "Query" meaning you have many `Query` classes across your app

    # but it makes the automagic work
    artist: ArtistType = sd.field()

# then in top level schema.py use merge_types as described in discord:

from app1.graphql.queries import Query as App1Queries
from app2.graphql.queries import Query as App2Queries

Query = merge_types(
    name="Query",
    types=(
        App1Queries,
        App2Queries,
    ),
)

Workaround 2: OR you have to annotate twice; once in the type (so it works with lists) then again here

@strawberry.type
class App1Queries:

    artists: List[ArtistType] = sd.field()
    
    @sd.field()
    def artist(self, info: Info, id: int) -> ArtistType:
        return Artist.objects.annotate(albums_count=Count("albums")).get(id=id)

I've said it before, but...

I've raised it before that root queries being decorated as a type is super counterintuitive for new users (and apparently, this afternoon, for me with several months of experience with it), and this is very much connected to that - the discord issue highlights that this arises because naming the class Query is the only way of ascertaining that it's the root.

If we were able to do:

import strawberry_django as sd

class App1Query(sd.RootQuery):  # Or sd.AppQuery, or decorated by @sd.root_query

    artists: List[ArtistType] = sd.field()
    artist: ArtistType = sd.field()

...then the sd.RootQuery class can simply wrap strawberry.type but add an is_root=True attribute to allow you to discern its rootiness?

@thclark thclark changed the title Annotate of _count in query optimizer not working as described in docs (attribute error) Improve declaration and documentation for root queries in django (where multiple apps are used) Oct 2, 2024
@bellini666
Copy link
Member

Hey @thclark ,

Looking at this initially I thought it was related to #549 (comment), but seeing your second post I know how you can workaround this in a better way.

When defining your SomeQuery, you can give it a different name in the @strawberry.type decorator, which should fix your issues. Like:

@strawberry.type(name="Query")
class App1Query:
    ...

Then you can merge the types into a single Query and use it in your schema.

Indeed we need a better way of documenting this... Also, not sure if we could do something codewise to improve this... Like, maybe having a @strawberry.query that is literally the same as @strawberry.type but has name="Query" hardcoded? cc @patrick91 what do you think?

@bellini666 bellini666 added documentation Improvements or additions to documentation question Further information is requested and removed bug Something isn't working labels Oct 19, 2024
@diesieben07
Copy link
Contributor

I have a snippet of code that I copy paste into pretty much all of my Strawberry apps which adds a bit of Django-style autodiscovery:

def create_schema() -> strawberry.Schema:
    schema_types = defaultdict(list)
    extra_types = set()

    for app_config in apps.get_app_configs():
        if module_has_submodule(app_config.module, 'graphql'):
            module_name = f'{app_config.name}.{'graphql'}'
            gql_module = import_module(module_name)
            for type_name in ('Query', 'Mutation'):
                if hasattr(gql_module, type_name):
                    schema_types[type_name].append(getattr(gql_module, type_name))
            extra_types.update(getattr(gql_module, 'EXTRA_TYPES', ()))

    merged_types = {
        type_name.lower(): merge_types(type_name, tuple(types))
        for type_name, types in schema_types.items()
    }
    return strawberry.Schema(
        **merged_types,
        types=extra_types,
    )

Now every Django app can have a graphql.py file where a Query or Mutation type will automatically get loaded and added to the schema.
Maybe something like this could be part of strawberry-django?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants