-
Notifications
You must be signed in to change notification settings - Fork 769
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
Fix 1061: DjangoListField should not cache queries #1063
Fix 1061: DjangoListField should not cache queries #1063
Conversation
…e/call queryset at view time, first attempt at solution
Not sure if this is how we want to fix the issue, seems to me default_queryset should be a callable to be called during view execution time. |
graphene_django/fields.py
Outdated
@@ -44,15 +44,15 @@ def model(self): | |||
return self._underlying_type._meta.model | |||
|
|||
def get_default_queryset(self): | |||
return self.model._default_manager.get_queryset() | |||
return self.model._default_manager # .get_queryset() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem comes from line 71
def get_resolver(self, parent_resolver):
_type = self.type
if isinstance(_type, NonNull):
_type = _type.of_type
django_object_type = _type.of_type.of_type
return partial(
self.list_resolver,
django_object_type,
parent_resolver,
self.get_default_queryset(), # <<<< This gets evaluated on init
)
Your change basically move the execution to later by making not return a queryset but the queryset manager (that you then force the evaluation via maybe_queryset).
I think it's a bit convoluted. And it's a bit confusing to call it get_default_queryset
when we already know it does not return a queryset...
I would change it to something like:
def default_queryset_resolver(self):
return self.model._default_manager.get_queryset
@staticmethod
def list_resolver(
django_object_type, resolver, default_queryset_resolver, root, info, **args
):
queryset = maybe_queryset(resolver(root, info, **args))
if queryset is None:
queryset = default_queryset_resolver()
if isinstance(queryset, QuerySet):
# Pass queryset to the DjangoObjectType get_queryset method
queryset = maybe_queryset(django_object_type.get_queryset(queryset, info))
return queryset
def get_resolver(self, parent_resolver):
return partial(
self.list_resolver,
self._underlying_type, # Do you see any reason why we were not doing that here? if there is a good reason you can change it back
parent_resolver,
self.default_queryset_resolver()
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree the naming is wrong. I don't want to drop get_default_queryset
as that is documented in the API, so going forward I think we'll have default_queryset_resolver
return get_default_queryset
which seems a tad odd. Also wondering if we should incorporate info
to be passed to the default_queryset_resolver
callable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[...] we'll have
default_queryset_resolver
returnget_default_queryset
which seems a tad odd
I assume you mean the other way around?
If you want to keep the function get_default_queryset
I would simply add it as:
def self.get_default_queryset(self):
return default_queryset_resolver()()
which is still weird.... :)
But I don't necessarily see the point if it is not used.
You say it's in the documentation but I couldn't find it... all I see is reference to get_queryset
on DjangoObjectType
which is the method we call here (so if a user had overridden it, it would still work).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, you're right about the docs. Also in the same file we have DjangoConnectionField
which defines resolve_queryset
& get_queryset_resolver
as well as get_manager
which acts as the default queryset. I like the semantics there more, maybe it should adopt those methods, certainly asking for a default manager instead of a queryset is a better contract that prevent caching
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense 👍
Did you see my comment/question about why we are not using self._underlying_type
in the get_resolver()
method?
… DjangoConnectionField for a better variable name default_manager instead of default_queryset
Attempt to fix #1061
DjangoListField.get_default_queryset
returns a manager andlist_resolver
calls maybe_queryset on default_queryset