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

Optimiser not working with custom prefetch query #389

Closed
AlphaRishi1229 opened this issue Oct 11, 2023 · 6 comments · Fixed by #540
Closed

Optimiser not working with custom prefetch query #389

AlphaRishi1229 opened this issue Oct 11, 2023 · 6 comments · Fixed by #540
Labels
enhancement New feature or request

Comments

@AlphaRishi1229
Copy link

AlphaRishi1229 commented Oct 11, 2023

The strawberry django optimiser is not working as expected when we provide hints (like custom prefetch) for a field in a type.
It results in n+1 queries when we do that

Describe the Bug

Suppose I have 2 models Model and ModelVariables, ModelVariables has a foreign key reference to Model. So, in django we can refer ModelVariables from Model using the related_name.
Consider my models to be something like this:

class Model(model.Model):
    id = models.UUIDField(primary_key=True)
    name = models.CharField(max_length=255)

class ModelVariable(model.Model):
    id = models.UUIDField(primary_key=True)
    value = models.CharField(max_length=255)
    related_model = models.ForeignKey(
        "Model",
        on_delete=models.CASCADE,
        related_name="model_variables"
    )
    relational_field = models.ForeignKey(
        "RelatedField",
        on_delete=models.CASCADE,
        related_name="model_variables"
    )

class RelatedField(model.Model):
    id = models.UUIDField(primary_key=True)
    is_editable = models.BooleanField(default=False)

Since, ModelVariable has a foreign key to Model. I can get all the ModelVariable for a Model using -> Model.model_variables.all()

Now, for the above models I created strawberry django types in the following way

@strawberry_django.type(Model)
class ModelType(Node):
    id: NodeID

    @strawberry_django.field(prefetch_related=[
        Prefetch(
            "model_variables",
            queryset=ModelVariable.objects.filter(
                related_field__is_editable=True
            ),
            to_attr="editable_model_variables"
        )
    ])
    def editable_model_variables(self) -> list["ModelVariableType"]:
        return self.editable_model_variables

@strawberry_django.type(ModelVariable)
class ModelVariableType(Node):
    id: NodeID
    value: auto
    related_model: "ModelType"
    related_field: "RelatedFieldType"

@strawberry_django.type(RelatedField)
class RelatedFieldType(Node):
    id: NodeID
    is_editable: auto

Now, in my ModelType, I want editable_model_variables field to return all ModelVariables that are editable, so I added a prefetch_related for it and updated the query. Which works.
Now, when I execute a query and mention related_field in the query. It causes n+1 queries. The obvious reason is that, in my prefetch related query I did not select_related the related_field.

AFAIK the optimiser takes these hints and not just override the optimised queries.

I also tried passing field_name="model_variables" to the @strawberry_django.field decorator for hinting but that didn't work too.

System Information

  • Operating system: Ubuntu 22.04
  • Strawberry version (if applicable):
    • strawberry-graphql-django==0.20.0
    • strawberry-graphql==0.209.5

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
@AlphaRishi1229 AlphaRishi1229 added the bug Something isn't working label Oct 11, 2023
@bellini666 bellini666 self-assigned this Oct 26, 2023
@bellini666 bellini666 added enhancement New feature or request and removed bug Something isn't working labels Nov 5, 2023
@bellini666 bellini666 removed their assignment Nov 5, 2023
@diesieben07
Copy link

I think this should be possible fairly easily. If I understand this correctly, here an inspection of the field's prefetches should be added. If a Prefetch exists with to_attr being the field, then use that Prefetch to figure out the actual name of the model field. I will play around with this tomorrow if I can.

@bellini666
Copy link
Member

Hey @diesieben07 . Thanks for trying to help with this :)

Left me know if you need any help with that. Either ping me here or on our discord community

@diesieben07
Copy link

Thank you for the heads up @bellini666.
I got something working, it would be great to get some feedback about whether there are additional things that I have missed, as I am not deeply familiar with the optimizer's workings. I have created a pull request with more information about my changes: #473

@bellini666
Copy link
Member

Thanks @diesieben07 :)

Will take a look at it very soon!

@Sanyambansal76
Copy link

@bellini666 and @diesieben07 any update on this?

@diesieben07
Copy link

@Sanyambansal76 I will hopefully have time to look at the remaining work on my pull request on the weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants