Code

Opened 2 years ago

Closed 11 months ago

#17648 closed New feature (fixed)

Allow `GenericForeignKey` to retreive/store proxy model content type.

Reported by: charettes Owned by: charettes
Component: contrib.contenttypes Version: master
Severity: Normal Keywords: contenttype proxy
Cc: charette.s@…, danny.adair@…, anssi.kaariainen@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

The actual ContentTypeManager's get_for_model and get_for_models methods always return the ContentType of the concrete class of a proxy model. This behaviour is causing GenericForeignKey to save the concrete class of proxy model and creates inconsistencies in the ContentTypeManager's get_... API:

from django.contrib.auth.models import User
from django.contrib.contenttypes.models import ContentType

class Worker(User):
    class Meta:
      app_label = 'app'
      proxy = True

ct_get_for_model = ContentType.objects.get_for_model(Worker)
ct_natural_key = ContentType.objects.get_by_natural_key('app', 'worker')
assert ct_natural_key == ct_get_for_model # raises AssertionError

I don't know if it was a design decision (IHMO it's a mistake and a bug) to do so but changing it now would introduce a small backward compatibility issue for any app relying on get_for_model directly or indirectly through the use of `GenericForeignKey' since they would now benefit from getting the exact ct they were referencing.

If I'm not mistaken, by looking at regressiontests.defer_regress.tests.DeferRegressionTest and #10738, I assume this was introduced by r10523 to make sure deferred models are handled correctly by get_for_model. Checks for proxy reversing should be made against model._defered and not `model._meta.proxy'.

A discussion recently emerged of #11154 (which is caused by this behaviour) suggesting to either remove the proxy following routine of get_for_model(s?) or to allow passing a flag (follow_proxy) to opt-in for the correct behaviour.

I'm attaching a simple patch that only replaces the checks in _get_opts (and automatically fixes #11154) and I'll be marking the ticket as 'DDN' since it involves a small backward compatibility issue that requires feedbac.

Attachments (4)

ticket-17648-contenttype-manager-get_for_model-proxy.diff (1.3 KB) - added by charettes 2 years ago.
Make sure ContentType.get_for_model(s) returns the correct ContentType for proxy models without breaking dereferred model compatibility introduced for #10738.
ticket-17648-correctly-handle-deferred-proxies.diff (2.2 KB) - added by charettes 2 years ago.
ticket-17648-correctly-handle-deferred-proxies.2.diff (2.5 KB) - added by charettes 2 years ago.
Simplified and documented _get_opts
ticket-17648.diff (2.5 KB) - added by charettes 2 years ago.
Updated patch to rely on the corrected behaviour of proxy_for_model fixed in r17573

Download all attachments as: .zip

Change History (25)

comment:1 Changed 2 years ago by charettes

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Type changed from New feature to Bug

Marking as bug since I really believe it was mistakenly introduced by r10523.

comment:2 Changed 2 years ago by charettes

  • Cc charette.s@… added
  • Has patch set
  • Triage Stage changed from Unreviewed to Design decision needed

Changed 2 years ago by charettes

Make sure ContentType.get_for_model(s) returns the correct ContentType for proxy models without breaking dereferred model compatibility introduced for #10738.

comment:3 Changed 2 years ago by danny.adair@…

  • Cc danny.adair@… added

comment:4 Changed 2 years ago by charettes

I was wrong about the proxy_for_model part. It will works for deferred concrete model but it will fail for deferred proxy model. Attaching a patch that correctly handle this case with extra tests. All tests passes on sqlite3 and postgis and the actual branch can be found on github.

Changed 2 years ago by charettes

Simplified and documented _get_opts

comment:5 Changed 2 years ago by akaariai

  • Cc anssi.kaariainen@… added

The problem why the proxy_for_model method fails is that the proxy_for_model does not generate a proxy chain, as is mistakenly assumed in multiple parts of the code. proxy_for_model references the concrete model. So in case you have deferred proxy, proxy_for_model references the concrete model, not the proxy. Fixing the proxy_for_model to actually be an chain would be wise, as this is at least the fifth ticket where I have seen problems caused by it.

I don't particularly like the __bases__[0] thing in your patch. It would be better to fix the proxy_for_model setup to actually produce a proxy chain...

comment:6 Changed 2 years ago by charettes

Is there any ticket concerning the proxy_for_model proxy chain issue directly or are they all side issues? Do you think It would be worth opening a ticket for this issue only?

I agree the __bases__[0] part is quite ugly but it was the only way fixing this reliably since the proxy chain is broken and we can assume that deferred models have only one base which is the model were trying to refer to. But I also agree fixing the proxy_for_model chain issue would be better.

comment:7 Changed 2 years ago by akaariai

Yes, I think a ticket for only the proxy_for_model would be a good thing to do. I don't remember a ticket for that existing, and can't find one.

If you are going to work on this, I think there should be two attributes in the model's Options, proxy_for_model which generates a proxy chain, and then concrete_model which is the concrete model in the end of the chain. The concrete_model could of course be a property, but just as well a real attribute. The concrete model is needed in multiple places of the code, and it is DRY to have it in the opts, so that the "travel proxy chain" loop isn't needed in multiple places.

comment:8 Changed 2 years ago by charettes

Alright, I'll work on this tomorrow. Do you remember if there's any part of core that would benefit from this DRY cleanup except for the ContentType part?

Plus do you think it would be reasonable to accept this ticket? Do you agree that proxy model and concrete model shouldn't share the same ContentType and that this behaviour was probably introduced by mistake in r10523?

comment:9 Changed 2 years ago by akaariai

I don't know if they should share the contenttype or not. Not my expertise. I guess if contenttypes are generated for the proxy models in syncdb, then proxy models have their own contenttypes, otherwise not. No strong opinions to either direction. Maybe ask django-developers?

The best to do for checking DRY chances is doing a project-wide grep for proxy_for_model. Git grep is wonderful for this. I have a feeling I have already done this once in some ticket, but can't find it and can't remember what it was related to...

comment:10 Changed 2 years ago by charettes

Ok, I'll write to django-developers to summarize the issue, consequences (#11154) and the proxy_for_model proxy chain and concrete_model approach.

comment:11 Changed 2 years ago by danny.adair@…

My opinion is that although there may be use cases for both "camps", it will always be easier to handle an additional content type than to try and distinguish objects that have the same. I think the vast majority of proxy models are "different things" that you want to be able to handle separately.
And the permissions check issue in #11154 ("who's app_label?") would be a straightforward affair imho.

Changed 2 years ago by charettes

Updated patch to rely on the corrected behaviour of proxy_for_model fixed in r17573

comment:12 Changed 2 years ago by charettes

Moved this to a pull request.

comment:13 Changed 2 years ago by akaariai

This needs at least some release notes changes.

I will post a short heads-up to django-developers about this ticket. If nobody objects, then lets push this to 1.5. I do agree that returning the proxy content types is the right thing to do as they are installed into the contenttypes table. So this is a bug fix instead of a backwards incompatible change.

comment:14 Changed 2 years ago by akaariai

While writing the django-developers post (which I never sent) I realized this change is going to be pretty bad from backwards compatibility perspective. Consider for example change log style usage: suddenly changes done through proxy models will no longer register to the base model, but to all the different proxy models used. This is a pretty bad violation of backwards compatibility (I think admin log would show this issue for example).

So, I suggest to split this patch to two:

  1. Fix the deferred model handling
  2. Add a kwarg 'for_base_model' or somesuch, defaults to 1.4 behavior, but you can request the proxy model contenttype if you wish.

comment:15 Changed 2 years ago by charettes

I can't see how to break the patch the way you suggest since there's no issue with the deferred model handling until the for_base_model kwarg is introduced. It's not the case because in both scenarios (deferred concrete model and deferred proxy model) the code actually returns the concrete_model just like it always did (even before r17573).

I suggest this approach.

Creating a new ticket to add the kwarg to get_for_model (and get_for_models) defaulting to 1.4 behavior while handling the deferred model case correctly. With tests and doc.

Fix #11154 and #17904 by using this new API.

Then discuss here how GenericForeignKey and GenericRelation should behave with this new API. Should we add a for_base_model kwarg to GenericForeignKey and GenericRelation? Should they automatically use for_base_model = False or maybe just GenericRelation if the referenced model is a proxy? Maybe this is more appropriate for django-dev?

comment:16 Changed 23 months ago by charettes

  • Summary changed from Allow `ContentTypeManager` and `GenericForeignKey` to retreive/store proxy model content type. to Allow `GenericForeignKey` to retreive/store proxy model content type.
  • Type changed from Bug to New feature

Now that #18399 is fixed and it's possible to retrieve ContentType associated with proxy models I think we can move this ticket to New Feature.

Maybe we could add an extra kwargs to GenericForeignKey, the same way we did with ContentTypeManager, in order to maintain backward compatibility and allow storing/retrieving proxy model content type?

comment:17 Changed 13 months ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

comment:18 Changed 11 months ago by gwahl@…

I wrote a patch using the for_concrete_model kwarg in GenericForeignKey: https://github.com/django/django/pull/1188

comment:19 Changed 11 months ago by charettes

  • Needs documentation set
  • Patch needs improvement set

The patch needs a release note and documentation concerning the for_concrete_model kwarg.

comment:20 Changed 11 months ago by gwahl@…

I added documentation and a release note to the pull request.

comment:21 Changed 11 months ago by Simon Charette <charette.s@…>

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

In 48424adaba74379eee311b3d1519f011212357ad:

Fixed #17648 -- Add for_concrete_model to GenericForeignKey.

Allows a GenericForeignKey to reference proxy models. The default
for for_concrete_model is True to keep backwards compatibility.

Also added the analog for_concrete_model kwarg to
generic_inlineformset_factory to provide an API at the form level.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.