-
Notifications
You must be signed in to change notification settings - Fork 83
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #76 +/- ##
=======================================
Coverage 93.63% 93.63%
=======================================
Files 7 7
Lines 330 330
=======================================
Hits 309 309
Misses 21 21
Continue to review full report at Codecov.
|
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. |
@tfoxy thank you, sounds good will do ASAP. |
48d4bef
to
9d63e37
Compare
@tfoxy done (amended commit), could you kindly check, please ? :-) |
…given DjangoObjectType.
9d63e37
to
f5fbc23
Compare
@tfoxy do you have an ETA for this? Can I help in any way in getting it merged? |
@tfoxy ? |
Hi @felixmeziere ! Thanks for the reminder. Merged and released in v0.9.1 . Great contribution. |
Thank you! Coming with another one soon :P |
Great! |
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 into_attr
ofPrefetch
, then Django will raise an exception ("Lookup was already seen with a different queryset" fromdjango.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__
forPrefetch
as the equality ofto_attr
and not the rest :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 theDjangoObjectType
, 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)