Propagate reverse foreign keys from proxy models to base class

[Anssi Kääriäinen]

The idea of proxy model is that the proxy subclass changes behavior, but the base class and all proxy models derived from it (call this "proxy equivalence class") are identical in the database representation. Foreign keys defined to proxy models break this idea:

class A(models.Model):
    pass
class ProxyA(A):
    class Meta:
       proxy = True
class B(models.Model):
    a = models.ForeignKey(ProxyA)

The above code is allowed currently, but A does not have a reverse relation to B, while ProxyA has that. It would be nice from maintainability perspective to have exactly same field set for every model in the proxy equivalence class. In addition, adding the reverse relation to the equivalence class would make the database representation equivalent to the Python representation.

[Alex Gaynor]

It doesn't seem correct, IMO, for A to have the FKey reverse descriptor on it.

[Anssi Kääriäinen]

The main point here is that in the DB representation A has b_set (there is a foreign key from B to A).

The original problem IIRC was that some users want to use the foreign key to proxy model so that they can get proxy model instances instead of the concrete class instance (auth.User was the use case, again IIRC).

The foreign key to proxy class doesn't make sense from data modeling perspective (there really can't be a foreign key to proxy class in the DB layer). It is a hack to get the related models as a proxy class. Now, if there was an easy way to achieve retrieval as proxy, then the whole FK to proxy could be deprecated (unless I am missing some other important use case). Whole another matter is that I don't have any idea what that API might be. You would need to handle select_related, prefetch_related, and fetch through attribute at least.

Still, I have no need to change the current behavior. Should I just close this ticket?

[Simon Charette]

IMHO when I explicitly create a FK to a proxy model I expect this behaviour. Maybe we should just document this?

[Anssi Kääriäinen]

Seems like I am in the opposition here. So, lets not change the current code.

[Simon Charette]

Rereading this ticket three years later I think I agree with Anssi at last.

Just to make sure, given the scenario defined above, is this what you have in mind?

a = A.objects.create()
a.b_set.all()  # Not actually possible
proxya = ProxyA.objects.get()
proxy.b_set.all()  # Actually possible

b = B.objects.create(a=proxya)  # Should we allow assigning an A instance here?
b.refresh_from_db()
assert isinstance(b.a, ProxyA)

assert A.objects.get(b_set=b) == a  # Not actually possible
assert ProxyA.objects.get(b_set=b) == proxya

[Simon Charette]

Here's a ​PR with what looks like the required changes.

[Tim Graham]

I think Anssi's point about "if there was an easy way to achieve retrieval as proxy, then the whole FK to proxy could be deprecated (unless I am missing some other important use case)" is an interesting one. Maybe this could be achieved if proxy models added an as_<proxy_name>() method to the proxied class. For example, instead of B defining the foreign key to ProxyA, it would defined a foreign key to A and if you want to access ProxyA instead:

>>> b = B.objects.get()
>>> b.a.as_proxya()
<ProxyA>

I'm not a big user of proxy models so there could very well be problems with this approach, but I think we should be sure to thoroughly reject that option before continuing down the road of enhanced support for relationships to proxy models.

A related issue is "#10961 - Allow users to override forward and reverse relationships on proxy models with ForeignKey fields".

[Carl Meyer]

ISTM that by far the most intuitive API for "I want to get a proxy object back from following this relationship" is to simply point the FK at the proxy class instead of the base class, like you do now. Any other API feels hacky to me by comparison, and is one more new thing (without parallel elsewhere) that people have to learn.

I don't really see that anyone has yet provided a strong motivation for this ticket; the current behavior seems fine and right to me. It seems to me that this ticket is mostly a result of seeking too much equivalence between the DB schema and the models API.

That said, I'm not strongly opposed to making the reverse descriptor available on the base class too, it just seems a bit odd to me.

I am strongly opposed to deprecating FKs to proxy classes and replacing them with a new API that people have to learn to get a proxy instance back from an FK.

[Simon Charette]

I also agree with Carl. I gave a try at writing a patch for this in order to see if anything would break by making the reverse descriptor and reverse field available on the base class.

It looks like the only the backward incompatible change would be the at the _meta API level where the moved related objects would be returned.

[Carl Meyer]

To clarify what I meant by "this ticket is mostly a result of seeking too much equivalence between the DB schema and the models API": the entire point of proxy classes is that they are _not_ equivalent to their base class in terms of their Python behavior and API. FK reverse descriptors (although their data is provided by an underlying schema in which there is no distinction between a proxy and its base) are part of the Python behavior and API, so it is entirely reasonable and expected that a proxy class might have different relation descriptors attached to it than its base class does.

That's why I believe that this ticket should simply be closed without changes, though (as I said) if others feel strongly that the behavior should be changed, I won't stand in the way.

[Anssi Kääriäinen]

The place where "reverse relations to proxy models are only a Python thing" breaks is deletion of models. Deletion of a model that points to a proxy child of a concrete model will delete the concrete model, too. Another way to think of this is that the foreign key in the database representation is to the concrete model, hence the concrete model does have the related model set.

Maybe we could either add a Meta method to get reverse relations from proxy children, or just document how to do this properly. This way, if you need to get the proxy children, there is some way to actually do that.

Still, a generic solution to this problem might be something along the lines:

   fk = models.ForeignKey(ConcreteModel, reverse_queryset=ProxyModel.objects.all())

This is a new thing to learn. But reverse_queryset would have other uses, too. You could for example use a custom queryset with custom methods, add annotations, order_by and so on to it. If you filter in the reverse_queryset, then there might be problems, but we could just document that you shouldn't do that. This would also fix Manager.use_for_related_fields flag issues (the flag doesn't work at all consistently).

All this being said I don't feel at all strongly about this issue, and if nobody else does, then we should default to not change existing behavior.

[Carl Meyer]

Replying to akaariai:

The place where "reverse relations to proxy models are only a Python thing" breaks is deletion of models. Deletion of a model that points to a proxy child of a concrete model will delete the concrete model, too.

Deletion also cascades up MTI inheritance chains, but we don't hoist the reverse descriptor for FKs to MTI child models onto the parent model. Do you see something "broken" there also? I don't.

I don't think these two things (cascade deletion and the Python class on which reverse descriptors live) are connected in the way you're suggesting. The mental model for both types of inheritance (MTI and proxy) is that any instance in a given inheritance chain really represents the "same object" -- the non-leaf instances are just generic / incomplete representations of that object. So naturally, if you delete that thing it's gone (and of course that includes its incomplete representations). There's nothing inconsistent with that mental model (or with the deletion behavior, in either the proxy or MTI case) in having a reverse FK descriptor on a child class that isn't there on the parent; it's just one of (possibly many) ways in which a parent instance is an incomplete representation of the full object.

Still, a generic solution to this problem might be something along the lines:

   fk = models.ForeignKey(ConcreteModel, reverse_queryset=ProxyModel.objects.all())

This is a new thing to learn. But reverse_queryset would have other uses, too. You could for example use a custom queryset with custom methods, add annotations, order_by and so on to it. If you filter in the reverse_queryset, then there might be problems, but we could just document that you shouldn't do that.

I'm not necessarily opposed to a feature like this if it has other uses that can stand on their own to justify the feature, but for the specific case of having an FK return proxy model instances, I think this API is a step backwards in usability from just pointing the FK at the proxy model, which already works fine.

[Anssi Kääriäinen]

My example wasn't that great, sorry for that.

What I'm after is that if we add proxy relations to the parent models we don't break anything (except for backwards incompatibility in the Meta API). The same isn't true for MTI inheritance, where a lot would break if we added child model relations to parent models.

We do support an inheritance chain of Concrete -> ProxyConcrete -> ProxyConcreteChild. Interestingly this chain actually creates a foreign key from ProxyConcreteChild directly to Concrete in the model definition. This is inconsistent, the foreign key should be to ProxyConcrete. But we can't do that, as then Concrete.objects.select_related('proxyconcretechild') wouldn't work, as Concrete doesn't have the reverse descriptor for the relation from ProxyConcreteChild to ProxyConcrete.

Also, my reverse_queryset example is broken. This assumes you want to get the *reverse* relation as proxy instances, whereas the foreign key to proxy feature is about getting the *direct* relation as proxy instances. While maybe an useful feature, it doesn't change anything for this ticket. Instead we should have fk = models.ForeignKey(ConcreteModel, fetch_as=ProxyModel) or something like that. This might actually make some sense if done automatically. The user writes ForeignKey(ProxyModel), internally Django interprets it as ForeignKey(ConcreteModel, fetch_as=ProxyModel). But I guess we do have more important problems to solve.

Even if I see a slight advantage in publishing reverse relations to concrete parents, I guess I'm slightly in favor of closing this one and moving on to something more productive.

[Carl Meyer]

Replying to akaariai:

What I'm after is that if we add proxy relations to the parent models we don't break anything (except for backwards incompatibility in the Meta API). The same isn't true for MTI inheritance, where a lot would break if we added child model relations to parent models.

Yep, agreed. That's why, as I said above, I'm not strongly opposed to this ticket; -0 at worst. Just don't think there's much wrong with the status quo either.

We do support an inheritance chain of Concrete -> ProxyConcrete -> ProxyConcreteChild. Interestingly this chain actually creates a foreign key from ProxyConcreteChild directly to Concrete in the model definition. This is inconsistent, the foreign key should be to ProxyConcrete. But we can't do that, as then Concrete.objects.select_related('proxyconcretechild') wouldn't work, as Concrete doesn't have the reverse descriptor for the relation from ProxyConcreteChild to ProxyConcrete.

Yeah, that is slightly odd. But given that the FK (OneToOne, really, right?) in this case is an internal implementation detail that the user isn't likely to ever need to follow directly, it doesn't seem like much of a problem.

[Simon Charette]

Looks like #25505 might be related to the o2o to proxy model scenario described by Anssi.

[Tim Graham]

This also fixes #14887 which I closed as a duplicate.

[Anssi Kääriäinen]

Seems like there is something to gain from propagating reverse descriptors to concrete parents. Notably we aren't deprecating foreign keys to proxy models. I don't see any big downsides in propagating the descriptors. So, I move to +1 camp for this change.

[Carl Meyer]

If it fixes bugs, +1 from me.

[Tim Graham]

#24762 is duplicate.

Marking as "Needs docs" for the release notes that are needed.

[Simon Charette]

Just added the required release notes.

[Simon Charette <charette.s@…>]

In 6c9f37e:

Fixed #18012 -- Propagated reverse foreign keys from proxy to concrete models.

Thanks to Anssi for the review.

[Simon Charette <charette.s@…>]

In 8bdfabed:

Refs #18012 -- Removed special casing for proxy models deletion.

This isn't required anymore now that reverse foreign keys
from proxy models are propagated to their concrete model.

[Simon Charette <charette.s@…>]

In 5b980897:

Refs #18012 -- Made proxy and concrete model reverse fields consistent.

Prior to this change proxy models reverse fields didn't include the
reverse fields pointing to their concrete model.

[Simon Charette <charette.s@…>]

In a82e21b:

Refs #18012 -- Removed the now unused proxied_children model option.

Thanks to Tim for the review.

[Simon Charette <charette.s@…>]

In 63f0e2d:

Refs #18012 -- Accounted for reverse proxy relations in migrations.

Thanks to Markus for the suggestion and Tim for the review.

[Shai Berger]

For the record and for future considerations, this did carry a backwards incompatibility. Consider:

class User(models.Model):
    pass  # Really all sorts of details, which do not matter

class Teacher(User):
    class Meta:
        proxy=True

class Student(User):
    class Meta:
        proxy=True

class Class(models.Model):
    teacher = models.ForeignKey(Teacher, related_name='classes')
    students = models.ManyToMany(Student, related_name='classes')

This works before this was fixed; after the fix, the related name 'classes' gets propagated to User twice, and it breaks.

(I admit that this design is somewhat flawed, but it used to be valid, and it isn't anymore)

[steve yeago]

Without M2Ms this feature seems kind of incomplete, since an M2M from a proxy model is basically the same thing, except that you lose some things like related-name accessors.

[Ivan]

class Worker(models.Model):

    fio = models.CharField(u'ФИО', unique=True, max_length=250)
    active = models.BooleanField(default=True, help_text="Активный")

    def __str__(self):
        return self.fio

class WorkerActive(Worker):

    def __str__(self):
        if not self.active:
            return "%s (Не кативен)" % (self.fio,)
        return self.fio

    class Meta:
        proxy = True

class License(models.Model):
    target_workers = models.ManyToManyField(WorkerActive, verbose_name="Покупалась для", blank=True)

    def __str__(self):
        return u"License for %s (%s)" % (self.soft.name, self.number or '')

I have error (1054, "Unknown column 'inventory_license_target_workers.workeractive_id' in 'field list'")
WHY!?
I believe that this is a gross error of the concept of using Proxy models