Support base class relations and reverse for proxy models

[TomsOverBaghdad]

Includes the proxy's base model when determining the reverse fields for a django model

The potential fix for this issue https://github.com/graphql-python/graphene-django/issues/1379

[firaskafri]

It would be great if the solution would be more generic to handle possible base classes such as django-polymorphic

    for base in model.__bases__:
        if is_valid_django_model(base):
            model_ancestry.append(base)

as opposed to:

    if model._meta.proxy:
        model_ancestry.append(model._meta.proxy_for_model)

[TomsOverBaghdad]

@firaskafri Thanks for the suggestion! I implemented it with tests. Let me know if the test changes imply correctly what you were expecting.

[firaskafri]

@TomsOverBaghdad Will definitely do!

Though I wanted to ask you, what the behavior would be if base class had an m2m field, not a reverse one?

[TomsOverBaghdad]

@firaskafri if base class A has many B then class C(A) that inherits from A will also have many B, I updated the query_test to show.

[firaskafri]

@firaskafri if base class A has many B then class C(A) that inherits from A will also have many B, I updated the query_test to show.

Yes reverse m2m is working correctly with this PR. But have you got the chance to deal with base m2m relation?

This PR addresses the reverse relation issue correctly I guess where if Z has an m2m with A, A and `B(A) will have the field added to their nodes.

But the example you provided is different, you are assuming that the field exists in A as an original field not a reverse relation.

[TomsOverBaghdad]

@firaskafri is there anything else I can do for this PR based on the changes requested tag? Seems like the failing tests are expected based on failing with previous merged prs.

[firaskafri]

@firaskafri is there anything else I can do for this PR based on the changes requested tag? Seems like the failing tests are expected based on failing with previous merged prs.

I just updated the tags. We're just waiting for more reviews.

[jaw9c]

Great PR - would it be possible to use something from the Django lib like this to pull the fields from the model rather than rolling our own?

[firaskafri]

Great PR - would it be possible to use something from the Django lib like this to pull the fields from the model rather than rolling our own?

I investigated that, the problem with get_fields is that it does not get the local ManyToMany relationships. @TomsOverBaghdad any idea if this could be done?

[lucas-bremond]

I believe this PR may have introduced a "regression" when combined with django-polymorphic.

In Django Polymorphic, the parent type exposes accessors to the derived types, which are now picked up by the updated get_reverse_fields logic and added to the derived type fields.

Here's an example of what I mean. Assuming the following models:

from polymorphic.models import PolymorphicModel

class Vehicle(PolymorphicModel):
    id = UUIDField()

class Car(Vehicle):
    ...

class Truck(Vehicle):
    ...

now the Car and Truck types will expose car and truck fields (that came from the parent). And these fields don't have associated resolvers, so the following GraphQL query will raise an Exception:

query {
  cars {
    id
    # The presence of these two fields makes no logical sense, and resolving them raises
    car { id }
    truck { id }
  }
}

I suppose this is not really Graphene Django's concern to ensure downstream compatibility, but since Django Polymorphic is a quite popular library, I believe this is worth reporting here?

A quick workaround is to add these duplicated fields to the exclude lists of the derived types:

class CarType(DjangoObjectType):
    class Meta:
        exclude = ["car", "truck"]

but this is not most elegant.

[firaskafri]

I believe this PR may have introduced a "regression" when combined with django-polymorphic.

In Django Polymorphic, the parent type exposes accessors to the derived types, which are now picked up by the updated get_reverse_fields logic and added to the derived type fields.

Here's an example of what I mean. Assuming the following models:

from polymorphic.models import PolymorphicModel

class Vehicle(PolymorphicModel):
    id = UUIDField()

class Car(Vehicle):
    ...

class Truck(Vehicle):
    ...

now the Car and Truck types will expose car and truck fields (that came from the parent). And these fields don't have associated resolvers, so the following GraphQL query will raise an Exception:

query {
  cars {
    id
    # The presence of these two fields makes no logical sense, and resolving them raises
    car { id }
    truck { id }
  }
}

I suppose this is not really Graphene Django's concern to ensure downstream compatibility, but since Django Polymorphic is a quite popular library, I believe this is worth reporting here?

A quick workaround is to add these duplicated fields to the exclude lists of the derived types:

class CarType(DjangoObjectType):
    class Meta:
        exclude = ["car", "truck"]

but this is not most elegant.

@lucas-bremond I believe we are having the same issue as well. Can you create an issue with your comment referencing this PR please?