strawberry-graphql / strawberry-django

Strawberry GraphQL Django extension
https://strawberry.rocks/docs/django
MIT License
410 stars 118 forks source link

Optimizer ignores custom type querysets for non-list `ForeignKey` relations #572

Closed SupImDos closed 2 months ago

SupImDos commented 3 months ago

Hi!

I need to override the base get_queryset method of all of my types due to a pre-existing approach to multi-tenancy and permissions. This means my relations can only ever be resolved correctly with prefetch_related (at the cost of extra queries) and not with select_related which bypasses the custom querysets.

Currently, the DjangoOptimizerExtension always uses select_related for non-list ForeignKey relations, regardless of whether there is a custom get_queryset defined on the type.

I'm wondering if there's a way to either:

a) Configurably force the optimiser to never use select_related, only prefetch_related b) Have the optimiser detect when a type has a custom get_queryset, and use prefetch_related instead of select_related

Describe the Bug

Here is a contrived example:

@strawberry.type
class BaseType:
    @classmethod
    def get_queryset(cls, queryset, info, **kwargs):
        # Enforce some *global* multi-tenancy and permissions filtering here
        return queryset.filter(example="contrived")  # Contrived example

@strawberry_django.type(models.Region)
class RegionType(BaseType):
    id: auto
    name: auto

@strawberry_django.type(models.User)
class UserType(BaseType):
    id: auto
    email: auto
    region: RegionType = django_strawberry.field()

With the above, a query such as:

query GetUsersWithRegion {
    user {
        id
        email
        region {
            id
            name
        }
    }
}

Results in the following single database query, using select_related - which is missing the filtering on the nested regions:

# users and regions
SELECT 
  "accounts_user"."id", 
  "accounts_user"."email", 
  "accounts_user"."region_id", 
  "regions_region"."id", 
  "regions_region"."name" 
FROM 
  "accounts_user" 
  INNER JOIN "regions_region" ON "accounts_user"."region_id" = "regions_region"."id"
WHERE 
  "accounts_user"."example" = "contrived" 
ORDER BY 
  "accounts_user"."id" ASC

Whereas my desired behaviour would be two database queries, using prefetch_related:

# users
SELECT 
  "accounts_user"."id", 
  "accounts_user"."email", 
  "accounts_user"."region_id" 
FROM 
  "accounts_user" 
WHERE 
  "accounts_user"."example" = "contrived" 
ORDER BY 
  "accounts_user"."id" ASC 

# regions
SELECT 
  "regions_region"."id", 
  "regions_region"."name" 
FROM 
  "regions_region" 
WHERE 
  "regions_region"."example" = "contrived" AND "regions_region"."id" IN (1, 2, 3, ...)
ORDER BY 
  "regions_region"."id" ASC

System Information

Additional Context

Upvote & Fund

Fund with Polar

bellini666 commented 3 months ago

a) Configurably force the optimiser to never use select_related, only prefetch_related

A config for that, which can be used both globally and per-field, sounds interesting...

b) Have the optimiser detect when a type has a custom get_queryset, and use prefetch_related instead of select_related

The optimizer will probably have to have a way to detect when to fallback to prefetch_related for cases like https://github.com/strawberry-graphql/strawberry-django/issues/549

A custom get_queryset might indeed indicate a second need for this, indeed.

Would you say that by doing this, it solves any problems you have here? Or would you still need the option? Because I'm thinking about going this way, which will solve 2 issues at once, and avoid having to add an extra option that is probably not going to be needed.

SupImDos commented 2 months ago

The optimizer will probably have to have a way to detect when to fallback to prefetch_related for cases like #549

A custom get_queryset might indeed indicate a second need for this, indeed.

Would you say that by doing this, it solves any problems you have here? Or would you still need the option? Because I'm thinking about going this way, which will solve 2 issues at once, and avoid having to add an extra option that is probably not going to be needed.

Yes, I think so. As you suggest, falling back to .prefetch_related when a type has a custom get_queryset would solve my problem and seems to be a better approach than adding another option 😁