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

Fix 1061: DjangoListField should not cache queries #1063

Merged
merged 3 commits into from Dec 23, 2020

Conversation

zbyte64
Copy link
Collaborator

@zbyte64 zbyte64 commented Nov 13, 2020

Attempt to fix #1061

  1. test that default functionality should resolve/call queryset at view time
  2. DjangoListField.get_default_queryset returns a manager and list_resolver calls maybe_queryset on default_queryset

…e/call queryset at view time, first attempt at solution
@zbyte64
Copy link
Collaborator Author

zbyte64 commented Nov 13, 2020

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.

@zbyte64 zbyte64 requested a review from jkimbo November 20, 2020 23:35
@@ -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()
Copy link
Collaborator

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()
        )

Copy link
Collaborator Author

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.

Copy link
Collaborator

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 return get_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).

Copy link
Collaborator Author

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

Copy link
Collaborator

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
@zbyte64 zbyte64 merged commit 7b35695 into graphql-python:master Dec 23, 2020
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.

DjangoListField returning stale data by default
3 participants