fix: fk resolver permissions leak

[superlevure]

This PR re-introduce the changes from https://github.com/graphql-python/graphene-django/pull/1315 to make sure we go through get_queryset when resolving foreign relationships, with some differences:

• It applies the improvement from https://github.com/graphql-python/graphene-django/pull/1391 so we don't perform and additional SQL query when the id of the foreign object can be retrieved from the object itself (via relation_id)
• It applies the fix to OneToOneRel as well
• It makes sure to use the default resolver when get_queryset was not overloaded by the Type, so we can leverage optimization techniques such as prefetch_related and select_related (see https://github.com/graphql-python/graphene-django/pull/1401)

Left to do:

• [x] Update documentation

[firaskafri]

@superlevure this seems to be very interesting! waiting to test on my local environment!

[superlevure]

@superlevure this seems to be very interesting! waiting to test on my local environment!

Thank you!

If the description was not clear enough, this PR allowsprefetch_related and select_related optimizations when no custom get_queryset is defined for the Type.

When one is defined, though, any filtering on the queryset will invalidate the "cache" offered by those as explained by Django documentation here:

https://docs.djangoproject.com/en/4.2/ref/models/querysets/#prefetch-related

I hope this MR can reconcile the two worlds: those who are dependent on select/prefetch_related and those who rely on custom get_queryset. Note that for the latter, the N+1 problem can be tackled by dataloaders instead.

[superlevure]

@sjdemartini, thank you for taking the time to test my PR on graphene-django-optimizer.

I'm trying to reproduce the issue, but I can't seem to make the tests pass on master first. What I have done so far:

1. Cloned https://github.com/tfoxy/graphene-django-optimizer
2. Followed CONTRIBUTING.md:

   virtualenv -p python3 venv
   . venv/bin/activate
   pip install -r dev-requirements.txt
   # run tests:
   python setup.py test

I'm getting the following for each test: AttributeError: 'ExecutionContext' object has no attribute 'collect_fields'

Does that ring a bell to you? Is this the correct repo you are referring to?

[sjdemartini]

@sjdemartini, thank you for taking the time to test my PR on graphene-django-optimizer.

I'm trying to reproduce the issue, but I can't seem to make the tests pass on master first. What I have done so far:

1. Cloned https://github.com/tfoxy/graphene-django-optimizer
2. Followed CONTRIBUTING.md:

virtualenv -p python3 venv
. venv/bin/activate
pip install -r dev-requirements.txt
# run tests:
python setup.py test

I'm getting the following for each test: AttributeError: 'ExecutionContext' object has no attribute 'collect_fields'

Does that ring a bell to you? Is this the correct repo you are referring to?

Thank you for testing it out! Ah, yeah, that is because graphql-core released a newer version that broke some behavior, and the dev-requirements.txt did not pin the version of that package. There is an open PR (https://github.com/tfoxy/graphene-django-optimizer/pull/83) that resolves the problem, so if you use the GitHub CLI, you can gh pr checkout 83 to fix the above error and get tests to pass. (Alternatively, you can add graphql-core==3.1.7 to the dev-env-requirements.txt locally. Caveat that in that situation, there's an unrelated failing test on master test_should_return_valid_result_in_a_relay_query, presumably also due to some graphql-core behavior; that test is patched in the PR I mentioned.)

Updating the documentation is on my to-do for this PR indeed. Also to be noted that the N+1 issue occurs anyway if you use graphene-django without select_related/prefetch_related, so the comment should be more something like "Warning: modifying the queryset in get_queryset will cancel out any optimization technique...

Good point, sounds good to me!

[superlevure]

@sjdemartini I've spent some time on graphene-django-optimizer and realized that if you switch the tests fixture BaseItemType base class from OptimizedDjangoObjectType to DjangoObjectType the tests suite still passes on the fix/graphql-3.2 branch. This means that ultimately the custom get_queryset method defined in OptimizedDjangoObjectType is not executed in tests using that object, and thus OptimizedDjangoObjectType has no effect on the test results.

The reason why tests are failing with this branch of graphene-django is precisely because of this custom get_queryset implemented inOptimizedDjangoObjectType, which was previously not being used (due to the bug we are trying to fix with this PR) now getting executed. Switching to DjangoObjectType makes graphene-django-optimizer tests pass with this PR (see this branch: https://github.com/superlevure/graphene-django-optimizer/tree/fix/use-DjangoObjectType).

Some other thoughts on graphene-django-optimizer, the latest commit on main was on Oct 15, 2021, and as of today, the CI is failing on main. I think it's fair to say that the repo is not really maintained anymore. On the contrary, graphene-django still is, and this MR aims to fix a critical bug that can have profound security implications while preserving performance optimization when possible.

[firaskafri]

I believe there had been discussions to have the optimizer built in graphene-django, maybe that is the way to go? to have these optimizations as part of graphene-django? Specially now that the original library is not being maintained.

@jkimbo @ulgens @sjdemartini what do you think guys? or should we just keep things separated? One thought I have is that using these optimizations is actually a "very" django thing so maybe it is the way to go?

[sjdemartini]

I'm supportive of the idea of baking in the optimizations into graphene-django itself; it seems like that'd lend itself to cleaner/easier maintenance and likely a better implementation. There's an old issue here https://github.com/graphql-python/graphene-django/issues/57 where this was discussed a bit and generally landed on the notion that the optimizations should be part of graphene-django (https://github.com/graphql-python/graphene-django/issues/57#issuecomment-515752072).

Personally I don't think I'd have the time to dig into the details of how it could be done or port that functionality myself—I imagine it's not a small feat—and sadly the graphene-django-optimizer maintainer hasn't been responsive either. But I generally like the concept if someone else wants to pick that up!