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 #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.

[firaskafri]

@TomsOverBaghdad this being inside a loop for base classes will break relations in the case of m2m, I think its safer to isolate getting base fields in a different method as in this case ManyToManyRel is two sided, in the case of A it is ManyToManyRel.related while in the case of B(A) it is ManyToManyRel.field. Check this out

Try having the m2m field in the base class just like the example you mentioned in the comment, not a reverse relation

[firaskafri]

Something like this:

elif isinstance(related, models.ManyToManyRel) and not related.symmetrical:
    if _model is not model:
        yield (name, getattr(attr, "field", None))
        continue

But my preference would still be to get base models relations and reverse relations in a different method

[TomsOverBaghdad]

Heard! Good catch @firaskafri ! Let me know what you think of the changes I pushed. I separated out getting local fields and included the models ancestry similar to the reverse fields.

[TomsOverBaghdad]

I changed this to a set, but I can totally change it back to list to not break anything. It looks like get_reverse_fields is public so I didn't want to mess with the signature too much.

[sjdemartini]

Seems like that (switching to a set here) was the right move to me, since you were able to keep the get_reverse_fields signature the same but improve lookup time-complexity for this internal use-case in get_model_fields; nice!

[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?

[sjdemartini]

This looks great! Appreciate the thorough tests, and your effort to make the functions in utils.py better documented and more modular.

[sjdemartini]

Seems like that (switching to a set here) was the right move to me, since you were able to keep the get_reverse_fields signature the same but improve lookup time-complexity for this internal use-case in get_model_fields; nice!

[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?