Opened 4 years ago

Closed 12 months ago

Last modified 11 months ago

#18012 closed Cleanup/optimization (fixed)

Propagate reverse foreign keys from proxy models to base class

Reported by: Anssi Kääriäinen Owned by: Simon Charette
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Simon Charette, james@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (30)

comment:1 Changed 4 years ago by Carl Meyer

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

comment:2 Changed 4 years ago by Simon Charette

Cc: charette.s@… added

comment:3 Changed 4 years ago by Alex Gaynor

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

comment:4 Changed 4 years ago by 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?

comment:5 Changed 4 years ago by Simon Charette

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

Last edited 4 years ago by Simon Charette (previous) (diff)

comment:6 Changed 4 years ago by Anssi Kääriäinen

Component: Database layer (models, ORM)Documentation

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

comment:7 Changed 12 months ago by Simon Charette

Cc: Simon Charette added; charette.s@… removed
Component: DocumentationDatabase layer (models, ORM)

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

comment:8 Changed 12 months ago by Simon Charette

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

comment:9 Changed 12 months ago by Simon Charette

Has patch: set
Owner: changed from nobody to Simon Charette
Status: newassigned

comment:10 Changed 12 months ago by 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".

comment:11 Changed 12 months ago by 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.

comment:12 Changed 12 months ago by 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.

comment:13 Changed 12 months ago by 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.

comment:14 Changed 12 months ago by 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.

comment:15 in reply to:  14 Changed 12 months ago by 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.

comment:16 Changed 12 months ago by 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.

comment:17 in reply to:  16 Changed 12 months ago by 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.

comment:18 Changed 12 months ago by Simon Charette

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

comment:19 Changed 12 months ago by James Murty

Cc: james@… added

comment:20 Changed 12 months ago by Tim Graham

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

comment:21 Changed 12 months ago by 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.

comment:22 Changed 12 months ago by Carl Meyer

If it fixes bugs, +1 from me.

comment:23 Changed 12 months ago by Tim Graham

Needs documentation: set

#24762 is duplicate.

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

comment:24 Changed 12 months ago by Simon Charette

Needs documentation: unset
Version: 1.4master

Just added the required release notes.

comment:25 Changed 12 months ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:26 Changed 12 months ago by Simon Charette <charette.s@…>

Resolution: fixed
Status: assignedclosed

In 6c9f37e:

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

Thanks to Anssi for the review.

comment:27 Changed 12 months ago by 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.

comment:28 Changed 12 months ago by 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.

comment:29 Changed 12 months ago by Simon Charette <charette.s@…>

In a82e21b:

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

Thanks to Tim for the review.

comment:30 Changed 11 months ago by 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.

Note: See TracTickets for help on using tickets.
Back to Top