Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#23444 closed Cleanup/optimization (fixed)

Deprecate django.contrib.admin.helpers.InlineAdminForm.original_content_type_id

Reported by: ILYA Owned by: nobody
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: 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

First of all it must be said that at the moment django does not create ContentType objects for through models.
Moreover, django suggests to remove such objects when trying to syncdb/migrate apps.
But if you try to use your through model in inlines django will create ContentType object for it.

Consider the example. Every time you visit A or B admin pages ContentType is created.

# models.py

class A(models.Model):
    items = models.ManyToManyField('B')

class B(models.Model):
    pass

# admin.py

class Inline(admin.TabularInline):
    model = A.items.through

class A_Admin(admin.ModelAdmin):
    exclude = ('items',)
    inlines = (Inline,)

class B_Admin(admin.ModelAdmin):
    inlines = (Inline,)

The problem is here:
https://github.com/django/django/blob/stable/1.7.x/django/contrib/admin/helpers.py#L274

But the thing is that original_content_type_id is not used anywhere in 1.7 anymore. I've checked it by greping through all sources.
It was used in 1.6 and lower to show "View on site" links:
https://github.com/django/django/blob/stable/1.6.x/django/contrib/admin/templates/admin/edit_inline/tabular.html#L30

Now this part of template is refactored (while legacy view part remained):
https://github.com/django/django/blob/stable/1.7.x/django/contrib/admin/templates/admin/edit_inline/tabular.html#L30

It can be solved easily by removing several legacy lines.
If I didn't miss something I can make PR with that change.

Change History (16)

comment:1 Changed 6 years ago by Omer Katz

What if someone relied on this attribute being set? Many applications theme their admin app.
I suggest to wrap this in a property with an expedited deprecation process. That is, a DeprecationWarning will be issued.

comment:2 in reply to:  1 Changed 6 years ago by ILYA

Replying to thedrow:

What if someone relied on this attribute being set? Many applications theme their admin app.
I suggest to wrap this in a property with an expedited deprecation process. That is, a DeprecationWarning will be issued.

Yes, that was on my mind too when I was submitting this issue but I've forgotten to mention it. Thanks!

So now we have this code:

if original is not None:
    # Since this module gets imported in the application's root package,
    # it cannot import models from other applications at the module level.
    from django.contrib.contenttypes.models import ContentType
    self.original_content_type_id = ContentType.objects.get_for_model(original).pk

...and we need to keep original_content_type_id but prevent ContentType creating (and raise DeprecationWarning).
So smth like this may work (simplified):

if original is not None:
    from django.utils.functional import cached_property

    def original_content_type_id(self):
        import warnings
        from django.contrib.contenttypes.models import ContentType
        from django.utils.deprecation import RemovedInDjango18Warning

        warnings.warn("Use absolute_url attr instead.", RemovedInDjango18Warning)
        return ContentType.objects.get_for_model(original).pk

    self.__class__.original_content_type_id = cached_property(original_content_type_id)

comment:3 Changed 6 years ago by Tim Graham

Triage Stage: UnreviewedAccepted
Version: 1.7master

Deprecation sounds good, but we'd have start the process in 1.8 and I don't think there's any reason to accelerate it.

comment:4 in reply to:  3 Changed 6 years ago by ILYA

Replying to timgraham:

Deprecation sounds good, but we'd have start the process in 1.8 and I don't think there's any reason to accelerate it.


I'm ready to make PR but there 2 questions.

1

Should I extract original_content_type_id into the separate InlineAdminForm method?
The good part is that it will make code slightly more readable.
But what I don't like is that we'll get a bit of inconsistency: for those forms without original original_content_type_id property didn't even exist.
On the other side we still modify class that is not very good.

2

I didn't undestand about deprecation warning. Which one should I use - 1.8 or 1.9?

Last edited 6 years ago by ILYA (previous) (diff)

comment:5 Changed 6 years ago by Tim Graham

I don't think a method is really needed, but you could use an underscore method (_original_content_type_id) and assign it to original_content_type_id in __init__() if you want.

The class to use is RemovedInDjango20Warning, see the Deprecating a feature guide for details.

comment:6 Changed 6 years ago by ILYA

Has patch: set

I've made initial pull request:
https://github.com/django/django/pull/3201

comment:7 Changed 6 years ago by Loic Bistuer

IMO InlineAdminForm is the wrong place for a fix, can't we fix it at the get_for_model() level?

I didn't check in detail but I think Model._meta.auto_created can be used to this effect.

comment:8 Changed 6 years ago by Tim Graham

Loic, are you suggesting that we should modify get_for_model() so it doesn't create ContentTypes for through models? If so, can we create a separate ticket and keep this one focused on deprecating original_content_type_id?

comment:9 Changed 6 years ago by ILYA

Moving it to get_for_model might be logical. I've checked some details to be sure.

At the moment syncdb/migrate creates CT objects here:
https://github.com/django/django/blob/master/django%2Fcontrib%2Fcontenttypes%2Fmanagement.py

So it just calls app_config.get_models() and then creates corresponding objects.
Those app CT objects that were not found are suggested to be deleted.

Moving on to get_models.__doc__:

Returns a list of all installed models.

By default, the following models aren't included:

- auto-created models for many-to-many relations without
  an explicit intermediate table,
- models created to satisfy deferred attribute queries,
- models that have been swapped out.

So may be some other checks in addition to "being a through model" should be added there.

And one more thing. I doesn't make much sense but some users could use their through model in admin not only in inlines and therefore they might need its CT object. So that change can affect them in some way.

comment:10 Changed 6 years ago by Tim Graham

Patch needs improvement: set

comment:11 Changed 6 years ago by ILYA

Patch needs improvement: unset

I've updated the patch and created new ticket about get_for_model: #23687.

comment:12 Changed 6 years ago by Berker Peksag

Patch needs improvement: set

comment:13 Changed 6 years ago by ILYA

Patch needs improvement: unset

comment:14 Changed 6 years ago by Tim Graham

Summary: Legacy code unnecessarily creates ContentType object for `through` model when using it in inlinesDeprecate django.contrib.admin.helpers.InlineAdminForm.original_content_type_id
Triage Stage: AcceptedReady for checkin

comment:15 Changed 6 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 2d75515a4c4fe726e26ebb1a2d2acbc9d047cba6:

Fixed #23444 -- Deprecated django.contrib.admin.helpers.InlineAdminForm.original_content_type_id

comment:16 Changed 5 years ago by Tim Graham <timograham@…>

In 7140d4a:

Refs #23444 -- Removed InlineAdminForm.original_content_type_id per deprecation timeline.

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