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

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

Merged
merged 1 commit into from
Oct 15, 2021

Conversation

felixmeziere
Copy link
Contributor

@felixmeziere felixmeziere commented Sep 16, 2021

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 #77)

@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2021

Codecov Report

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

❗ 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
Copy link
Owner

tfoxy commented Sep 19, 2021

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
Copy link
Contributor Author

@tfoxy thank you, sounds good will do ASAP.

@felixmeziere
Copy link
Contributor Author

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

@felixmeziere
Copy link
Contributor Author

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

@felixmeziere
Copy link
Contributor Author

@tfoxy ?

@tfoxy tfoxy merged commit 76465dc into tfoxy:master Oct 15, 2021
@tfoxy
Copy link
Owner

tfoxy commented Oct 15, 2021

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

@felixmeziere
Copy link
Contributor Author

Thank you! Coming with another one soon :P

@tfoxy
Copy link
Owner

tfoxy commented Oct 15, 2021

Great!

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

Successfully merging this pull request may close these issues.

3 participants