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

[Simon Charette]

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.

[Simon Charette]

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

[Simon Charette]

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

[Simon Charette]

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.

[Simon Charette]

Simplified and documented _get_opts

[Anssi Kääriäinen]

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

[Simon Charette]

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.

[Anssi Kääriäinen]

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.

[Simon Charette]

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?

[Anssi Kääriäinen]

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

[Simon Charette]

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

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

[Simon Charette]

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

[Simon Charette]

Moved this to a ​pull request.

[Anssi Kääriäinen]

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.

[Anssi Kääriäinen]

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.

[Simon Charette]

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?

[Simon Charette]

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?

[gwahl@…]

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

[Simon Charette]

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

[gwahl@…]

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

[Simon Charette <charette.s@…>]

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.