tfoxy / graphene-django-optimizer

Optimize database access inside graphene queries
MIT License
427 stars 86 forks source link

Allow for hints with same value on multiple different resolvers of a given DjangoObjectType. #76

Closed felixmeziere closed 2 years ago

felixmeziere commented 3 years ago

This PR solves the following problem (super open to other ways of doing so if I missed something, please! 🙂):

If two different resolvers of a DjangoObjectType have hints with the same value in to_attr of Prefetch, then Django will raise an exception ("Lookup was already seen with a different queryset" from django.db.models.query.prefetch_related_objects).

What we would like instead is that graphene-django-optimizer lets us add those two hints but makes sure that, if both resolvers are needed in a query, it only adds the "common hint" once (therefore not leading to a duplicate prefetch that will raise an error).

The benefit of this is that we will be able to add duplicate hints where we want, to make sure the hints are triggered even when one of the fields is queried and not the other, while also allowing for both fields being queried together without duplicate prefetches nor exceptions being risen.

Caveat : Django explicitly implements __eq__ for Prefetch as the equality of to_attr and not the rest :

    def __eq__(self, other):
        if not isinstance(other, Prefetch):
            return NotImplemented
        return self.prefetch_to == other.prefetch_to

this is intended behaviour, the code of this PR will do the same : it only checks for a prefetch having the same to_attr in the other hints of the DjangoObjectType, this seems like a good criterion as it is the one responsible for the error being raised by Django.

Hope this can be merged soon! 🙂

(Issue https://github.com/tfoxy/graphene-django-optimizer/issues/77)

codecov-commenter commented 3 years ago

Codecov Report

Merging #76 (48d4bef) into master (2fd8af4) will not change coverage. The diff coverage is 100.00%.

:exclamation: Current head 48d4bef differs from pull request most recent head 9d63e37. Consider uploading reports for the commit 9d63e37 to get more accurate results Impacted file tree graph

@@           Coverage Diff           @@
##           master      #76   +/-   ##
=======================================
  Coverage   93.63%   93.63%           
=======================================
  Files           7        7           
  Lines         330      330           
=======================================
  Hits          309      309           
  Misses         21       21           
Impacted Files Coverage Δ
graphene_django_optimizer/query.py 92.13% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2fd8af4...9d63e37. Read the comment docs.

tfoxy commented 3 years ago

hi @felixmeziere ! Thanks for the PR. Can you add a test that would fail without the change in this PR? Once that is done I will merge this.

felixmeziere commented 3 years ago

@tfoxy thank you, sounds good will do ASAP.

felixmeziere commented 3 years ago

@tfoxy done (amended commit), could you kindly check, please ? :-)

felixmeziere commented 2 years ago

@tfoxy do you have an ETA for this? Can I help in any way in getting it merged?

felixmeziere commented 2 years ago

@tfoxy ?

tfoxy commented 2 years ago

Hi @felixmeziere ! Thanks for the reminder. Merged and released in v0.9.1 . Great contribution.

felixmeziere commented 2 years ago

Thank you! Coming with another one soon :P

tfoxy commented 2 years ago

Great!