tfoxy / graphene-django-optimizer

Optimize database access inside graphene queries
MIT License
428 stars 84 forks source link

Fix get node QuerySet #57

Closed iorlandini closed 3 years ago

iorlandini commented 4 years ago

The OptimizedDjangoObjectType.get_node method breaks the DjangoObjectType.get_node method default behaviour.

In the first one, the query set is the model manager:

@classmethod
def get_node(cls, info, id):
    return cls.maybe_optimize(info, cls._meta.model.objects, id)

In the second one, the base and original one, the query set is obtained from the ObjectType.get_queryset:

@classmethod
def get_node(cls, info, id):
    queryset = cls.get_queryset(cls._meta.model.objects, info)
    try:
        return queryset.get(pk=id)
    except cls._meta.model.DoesNotExist:
        return None

This is very important and that's why I created a real use case test based on items owned by a logged-in user.

The fix is very simple, the method has to use the get_queryset method based on the model manager instead of the model manager directly:

@classmethod
def get_node(cls, info, id):
    queryset = cls.get_queryset(cls._meta.model.objects, info)
    return cls.maybe_optimize(info, cls._meta.model.objects, id)

And I hope the test itself explains why is so important.

On the other hand, if we set a get_queryset method that automatically affects the scope of the returned elements by querying a specific type, we also expect the same scope to be affected when getting a node from its global ID.

tfoxy commented 3 years ago

@iorlandini it seems that the tests are failing for Django 2.2 in travis. I run the tests locally on this PR branch and they worked just fine. Do you know why the tests are failing?

iorlandini commented 3 years ago

Hi @tfoxy! Yes, I know. It's because of the test itself. I used the django.contrib.auth.models.User model to illustrate a point and it seems to not exist in version 2.2. I'll replace it tomorrow with a dummy model for the test.

iorlandini commented 3 years ago

@tfoxy I did not fix it because of the change proposed by @maxpeterson in https://github.com/tfoxy/graphene-django-optimizer/pull/50. And I will close this PR if this one goes through.