Opened 13 years ago
Closed 12 years ago
#17648 closed New feature (fixed)
Allow `GenericForeignKey` to retreive/store proxy model content type.
Reported by: | Simon Charette | Owned by: | Simon Charette |
---|---|---|---|
Component: | contrib.contenttypes | Version: | dev |
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)
Change History (25)
comment:1 by , 13 years ago
Type: | New feature → Bug |
---|
comment:2 by , 13 years ago
Cc: | added |
---|---|
Has patch: | set |
Triage Stage: | Unreviewed → Design decision needed |
by , 13 years ago
Attachment: | ticket-17648-contenttype-manager-get_for_model-proxy.diff added |
---|
Make sure ContentType.get_for_model(s)
returns the correct ContentType
for proxy models without breaking dereferred model compatibility introduced for #10738.
comment:3 by , 13 years ago
Cc: | added |
---|
comment:4 by , 13 years ago
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.
by , 13 years ago
Attachment: | ticket-17648-correctly-handle-deferred-proxies.diff added |
---|
by , 13 years ago
Attachment: | ticket-17648-correctly-handle-deferred-proxies.2.diff added |
---|
Simplified and documented _get_opts
comment:5 by , 13 years ago
Cc: | 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 by , 13 years ago
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 by , 13 years ago
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 by , 13 years ago
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 by , 13 years ago
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 by , 13 years ago
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 by , 13 years ago
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.
by , 13 years ago
Attachment: | ticket-17648.diff added |
---|
Updated patch to rely on the corrected behaviour of proxy_for_model
fixed in r17573
comment:13 by , 13 years ago
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 by , 13 years ago
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:
- Fix the deferred model handling
- 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 by , 13 years ago
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 by , 12 years ago
Summary: | Allow `ContentTypeManager` and `GenericForeignKey` to retreive/store proxy model content type. → Allow `GenericForeignKey` to retreive/store proxy model content type. |
---|---|
Type: | Bug → 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 by , 12 years ago
Triage Stage: | Design decision needed → Accepted |
---|
comment:18 by , 12 years ago
I wrote a patch using the for_concrete_model
kwarg in GenericForeignKey
: https://github.com/django/django/pull/1188
comment:19 by , 12 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
The patch needs a release note and documentation concerning the for_concrete_model
kwarg.
comment:21 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Marking as bug since I really believe it was mistakenly introduced by r10523.