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

Inconsistency in the way ForeignKeys resolutions are treated #1111

Open
tcleonard opened this issue Feb 4, 2021 · 3 comments
Open

Inconsistency in the way ForeignKeys resolutions are treated #1111

tcleonard opened this issue Feb 4, 2021 · 3 comments
Labels

Comments

@tcleonard
Copy link
Collaborator

tcleonard commented Feb 4, 2021

In the converters the way the reverse relation of a foreign key is dealt with differently from the forward relationship.

Indeed you can see in the ManyToOneRel conversion convert_field_to_list_or_connection() we convert the field to either a DjangoConnectionField or a DjangoListField (depending on if it uses relay or not).
Both of those make sure that the resolution of the field is going through those custom classes resolvers.
This is important as those implement some hooks to resolve the queryset in a custom way (or to give a custom queryset manager).

On the other hand, the ForeignKey conversion convert_field_to_djangomodel() implements a simple graphene.Field hence bypassing any potential custom queryset filtering.

This is notably a problem as those resolver hooks are typically used to implement authorization... and this behavior means that we enforce it only in one direction, resulting in permissions leaks.

I have a quick fix for this problem but I think it degrades performances by doing an additional query to the database:

@convert_django_field.register(models.OneToOneField)
@convert_django_field.register(models.ForeignKey)
def convert_field_to_djangomodel(field, registry=None):
    model = field.related_model

    def dynamic_type():
        _type = registry.get_type_for_model(model)
        if not _type:
            return

        class CustomField(Field):
            def get_resolver(self, parent_resolver):
                """
                Implements a custom resolver which goes through the `get_node` method to insure that
                it goes through the `get_queryset` method of the DjangoObjectType.
                """
                resolver = super().get_resolver(parent_resolver)
                return lambda root, info, **args: _type.get_node(info, resolver(root, info, **args).pk)

        return CustomField(_type, description=field.help_text, required=not field.null)

    return Dynamic(dynamic_type)

I am going to submit as a PR with some unit tests but I would love to get some feedback as it seems pretty hacky...

tcleonard pushed a commit to tcleonard/graphene-django that referenced this issue Feb 10, 2021
tcleonard pushed a commit to tcleonard/graphene-django that referenced this issue Feb 10, 2021
tcleonard pushed a commit to tcleonard/graphene-django that referenced this issue Feb 10, 2021
tcleonard pushed a commit to loft-orbital/graphene-django that referenced this issue Feb 10, 2021
@tcleonard
Copy link
Collaborator Author

Submitted 2 PRs:

tcleonard added a commit to loft-orbital/graphene-django that referenced this issue Feb 10, 2021
…ix_foreign_key_field_v2

Issue graphql-python#1111: make ForeignKeys resolve symmetrically (v2)
tcleonard added a commit to loft-orbital/graphene-django that referenced this issue Feb 10, 2021
tcleonard added a commit to loft-orbital/graphene-django that referenced this issue Feb 10, 2021
…ix_foreign_key_field_v2

Issue graphql-python#1111: make ForeignKeys resolve symmetrically (v2)
tcleonard pushed a commit to loft-orbital/graphene-django that referenced this issue Mar 10, 2021
@Rainshaw
Copy link
Contributor

hello, I am facing the same issue, how is this work going?

@tcleonard
Copy link
Collaborator Author

This is blocking on me, I need to review the alternative PR to see if it's a better solution to the problem. Haven't had time to take a look just yet. For the record I have been using my PR for quite some time and other than the performance overhead it does the job.

tcleonard pushed a commit to loft-orbital/graphene-django that referenced this issue Jan 4, 2022
tcleonard pushed a commit to loft-orbital/graphene-django that referenced this issue Mar 20, 2022
tcleonard added a commit to loft-orbital/graphene-django that referenced this issue Mar 20, 2022
…fix_foreign_key_field

Issue graphql-python#1111: foreign key should also call get_queryset method
tcleonard pushed a commit to loft-orbital/graphene-django that referenced this issue Sep 19, 2022
tcleonard pushed a commit to loft-orbital/graphene-django that referenced this issue Sep 19, 2022
tcleonard pushed a commit to loft-orbital/graphene-django that referenced this issue Sep 19, 2022
firaskafri pushed a commit that referenced this issue Sep 23, 2022
* Issue #1111: foreign key should also call get_queryset method

* fix: test for graphene PR graphql-python/graphene#1412

Co-authored-by: Thomas Leonard <thomas@loftorbital.com>
tcleonard pushed a commit to loft-orbital/graphene-django that referenced this issue Sep 26, 2022
superlevure pushed a commit to loft-orbital/graphene-django that referenced this issue Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants