Opened 13 years ago

Closed 4 months ago

#11715 closed Cleanup/optimization (fixed)

If change mutable list "inlines" in one admin options then it will change "inlines" for all admin options.

Reported by: Alexander Ivanov Owned by: Jacob Walls
Component: contrib.admin Version: 1.1
Severity: Normal Keywords: inlines admin options
Cc: summer.is.gone@…, Marc Tamlyn 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

Now attribute "inlines" is mutable and declared in the body of ModelAdmin class:

class ModelAdmin(BaseModelAdmin):
    ...
    inlines = []
    ...

When we change this attribute in one sub-class, python will change it in all other sub-classes:

from django.db import models

class Author(models.Model):
   name = models.CharField(max_length=100)

class Book(models.Model):
   author = models.ForeignKey(Author)
   title = models.CharField(max_length=100)
from django.contrib import admin
from models import *

class BookInline(admin.TabularInline):
    model = Book

class AuthorAdmin(admin.ModelAdmin):
    def __init__(self, *args, **kwds):
        self.inlines.append(BookInline)
        return super(AuthorAdmin, self).__init__(*args, **kwds)

class BookAdmin(admin.ModelAdmin):
   pass

admin.site.register(Author, AuthorAdmin)
admin.site.register(Book, BookAdmin)

This code will add BookInline to BookAdmin, too!

So, when we will go to admin and try to change Book, we will see exception:

<class 'test.models.Book'> has no ForeignKey to <class 'test.models.Book'>

We can fix it like this:

--- django/contrib/admin/options.py
+++ django/contrib/admin/options.py
@@ -187,7 +187,7 @@ class ModelAdmin(BaseModelAdmin):
     save_as = False
     save_on_top = False
     ordering = None
-    inlines = []
+    inlines = None

     # Custom templates (designed to be over-ridden in subclasses)
     change_form_template = None
@@ -206,6 +206,8 @@ class ModelAdmin(BaseModelAdmin):
         self.opts = model._meta
         self.admin_site = admin_site
         self.inline_instances = []
+        if self.inlines is None:
+            self.inlines = []
         for inline_class in self.inlines:
             inline_instance = inline_class(self.model, self.admin_site)
             self.inline_instances.append(inline_instance)

--- django/contrib/admin/validation.py
+++ django/contrib/admin/validation.py
@@ -133,7 +133,7 @@ def validate(cls, model):


     # inlines = []
-    if hasattr(cls, 'inlines'):
+    if hasattr(cls, 'inlines') and cls.inlines is not None:
         check_isseq(cls, 'inlines', cls.inlines)
         for idx, inline in enumerate(cls.inlines):
             if not issubclass(inline, BaseModelAdmin):

But then our sample will raise:

'NoneType' object has no attribute 'append'

I think, we can do it like this:

--- django/contrib/admin/options.py
+++ django/contrib/admin/options.py
@@ -187,7 +187,7 @@ class ModelAdmin(BaseModelAdmin):
     save_as = False
     save_on_top = False
     ordering = None
-    inlines = []
+    inlines = ()

     # Custom templates (designed to be over-ridden in subclasses)
     change_form_template = None

It will fix logic with mutable attribute, but users will have to write:

 from django.contrib import admin
 from models import *

 class BookInline(admin.TabularInline):
     model = Book

 class AuthorAdmin(admin.ModelAdmin):
+    inlines = []
     def __init__(self, *args, **kwds):
         self.inlines.append(BookInline)
         return super(AuthorAdmin, self).__init__(*args, **kwds)

 class BookAdmin(admin.ModelAdmin):
    pass

 admin.site.register(Author, AuthorAdmin)
 admin.site.register(Book, BookAdmin)

I believe, this will be more correct variant.

Attachments (1)

patch.diff (330 bytes) - added by Alexander Ivanov 13 years ago.

Download all attachments as: .zip

Change History (9)

Changed 13 years ago by Alexander Ivanov

Attachment: patch.diff added

comment:1 Changed 13 years ago by Ivan Gromov

Cc: summer.is.gone@… added

comment:2 Changed 12 years ago by Russell Keith-Magee

Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

I'm not sure I agree with your solution, but the problem appears real.

comment:3 Changed 11 years ago by Julien Phalip

Severity: Normal
Type: Bug

comment:4 Changed 10 years ago by Marc Tamlyn

Cc: Marc Tamlyn added
Easy pickings: unset
UI/UX: unset

I agree with this being an issue - and a weird one to debug too!

I'd extend it to actions as well which would suffer from the same issue. Whilst it would potentially break some code to change it to a tuple, I would say this is more correct, an also consistent with the other attributes such as list_filter.

What exactly is the issue with the current implementation?

comment:5 Changed 5 months ago by Jacob Walls

Owner: changed from nobody to Jacob Walls
Patch needs improvement: unset
Status: newassigned
Type: BugCleanup/optimization

This issue predates the system check framework. Trying the original example (I created a new class in the admin_inlines test suite) now catches errors before failing at the db layer:

<class 'admin_inlines.admin.FootNoteAuditInline'>: (admin.E202) 'admin_inlines.FootNoteAudit' has no ForeignKey to 'admin_inlines.ShowInlineParent'.

What exactly is the issue with the current implementation?

I think if there is one, it's that folks might believe they can mutate the inlines on instances of ModelAdmin, but that's user error if they're not willing to accept it applying to all model admins.

PR clarifies the intended immutability of the inlines attribute of ModelAdmin by defaulting to (), but we could also just close as user error.

comment:6 Changed 5 months ago by Jacob Walls

Needs tests: unset

comment:7 Changed 4 months ago by Carlton Gibson

Triage Stage: AcceptedReady for checkin

comment:8 Changed 4 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In dc9deea8:

Fixed #11715 -- Changed default value of ModelAdmin.actions/inlines to empty tuples.

This clarifies the intended pattern of overwriting the default value
rather than mutating it.

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