Opened 4 years ago

Closed 4 months ago

Last modified 4 months ago

#18012 closed Cleanup/optimization (fixed)

Propagate reverse foreign keys from proxy models to base class

Reported by: akaariai Owned by: charettes
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: charettes, 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 carljm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

comment:2 Changed 4 years ago by charettes

  • Cc charette.s@… added

comment:3 Changed 4 years ago by Alex

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

comment:4 Changed 4 years ago by akaariai

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 charettes

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 charettes (previous) (diff)

comment:6 Changed 4 years ago by akaariai

  • Component changed from Database layer (models, ORM) to Documentation

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

comment:7 Changed 4 months ago by charettes

  • Cc charettes added; charette.s@… removed
  • Component changed from Documentation to Database 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 4 months ago by charettes

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

comment:9 Changed 4 months ago by charettes

  • Has patch set
  • Owner changed from nobody to charettes
  • Status changed from new to assigned

comment:10 Changed 4 months ago by timgraham

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 4 months ago by carljm

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 4 months ago by charettes

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 4 months ago by carljm

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 follow-up: Changed 4 months ago by 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. 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 4 months ago by carljm

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 follow-up: Changed 4 months ago by akaariai

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 4 months ago by carljm

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 4 months ago by charettes

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

comment:19 Changed 4 months ago by jmurty

  • Cc james@… added

comment:20 Changed 4 months ago by timgraham

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

comment:21 Changed 4 months ago by akaariai

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 4 months ago by carljm

If it fixes bugs, +1 from me.

comment:23 Changed 4 months ago by timgraham

  • Needs documentation set

#24762 is duplicate.

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

comment:24 Changed 4 months ago by charettes

  • Needs documentation unset
  • Version changed from 1.4 to master

Just added the required release notes.

comment:25 Changed 4 months ago by timgraham

  • Triage Stage changed from Accepted to Ready for checkin

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

  • Resolution set to fixed
  • Status changed from assigned to closed

In 6c9f37e:

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

Thanks to Anssi for the review.

comment:27 Changed 4 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 4 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 4 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 4 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