Opened 14 years ago
Closed 6 years ago
#15665 closed Bug (duplicate)
Inline admins are broken when primary key is not an AutoField and not editable.
Reported by: | Sebastian Noack | Owned by: | nobody |
---|---|---|---|
Component: | contrib.admin | Version: | 1.2 |
Severity: | Normal | Keywords: | |
Cc: | olivier.dalang@…, Andi Albrecht | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
For each inline object, a hidden field for its primary key (if it is an AutoField) is included in the form. But if you inherit from a concrete model that has a custom (not AutoField) primary key, you end up in a situation where you have a primary key which is a OneToOneField that is not editable in the admin.
from django.db import models class Event(models.Model): name = models.CharField(max_length=100, primary_key=True) def __unicode__(self): return self.name class Person(models.Model): name = models.CharField(max_length=100, primary_key=True) def __unicode__(self): return self.name class Visitor(Person): event = models.ForeignKey(Event) def __unicode__(self): return u"'%s' at '%s'" % (self.person_ptr, self.event)
from django.contrib import admin class VisitorInline(admin.StackedInline): model = Visitor class EventAdmin(admin.ModelAdmin): inlines = [VisitorInline] admin.site.register(Event, EventAdmin)
In that case there is no element in the form that holds the primary key and you get the Exception below, when you try to save an existing object in the admin.
Traceback (most recent call last): File "/home/sebastian/djacap/fam/src/famtest/django/core/servers/basehttp.py", line 280, in run self.result = application(self.environ, self.start_response) File "/home/sebastian/djacap/fam/src/famtest/django/core/servers/basehttp.py", line 674, in __call__ return self.application(environ, start_response) File "/home/sebastian/djacap/fam/src/famtest/django/core/handlers/wsgi.py", line 241, in __call__ response = self.get_response(request) File "/home/sebastian/djacap/fam/src/famtest/django/core/handlers/base.py", line 141, in get_response return self.handle_uncaught_exception(request, resolver, sys.exc_info()) File "/home/sebastian/djacap/fam/src/famtest/django/core/handlers/base.py", line 165, in handle_uncaught_exception return debug.technical_500_response(request, *exc_info) File "/home/sebastian/djacap/fam/src/famtest/django/views/debug.py", line 58, in technical_500_response html = reporter.get_traceback_html() File "/home/sebastian/djacap/fam/src/famtest/django/core/handlers/base.py", line 100, in get_response response = callback(request, *callback_args, **callback_kwargs) File "/home/sebastian/djacap/fam/src/famtest/django/contrib/admin/options.py", line 239, in wrapper return self.admin_site.admin_view(view)(*args, **kwargs) File "/home/sebastian/djacap/fam/src/famtest/django/utils/decorators.py", line 76, in _wrapped_view response = view_func(request, *args, **kwargs) File "/home/sebastian/djacap/fam/src/famtest/django/views/decorators/cache.py", line 69, in _wrapped_view_func response = view_func(request, *args, **kwargs) File "/home/sebastian/djacap/fam/src/famtest/django/contrib/admin/sites.py", line 190, in inner return view(request, *args, **kwargs) File "/home/sebastian/djacap/fam/src/famtest/django/utils/decorators.py", line 21, in _wrapper return decorator(bound_func)(*args, **kwargs) File "/home/sebastian/djacap/fam/src/famtest/django/utils/decorators.py", line 76, in _wrapped_view response = view_func(request, *args, **kwargs) File "/home/sebastian/djacap/fam/src/famtest/django/utils/decorators.py", line 17, in bound_func return func(self, *args2, **kwargs2) File "/home/sebastian/djacap/fam/src/famtest/django/db/transaction.py", line 299, in _commit_on_success res = func(*args, **kw) File "/home/sebastian/djacap/fam/src/famtest/django/contrib/admin/options.py", line 890, in change_view queryset=inline.queryset(request)) File "/home/sebastian/djacap/fam/src/famtest/django/forms/models.py", line 705, in __init__ queryset=qs) File "/home/sebastian/djacap/fam/src/famtest/django/forms/models.py", line 429, in __init__ super(BaseModelFormSet, self).__init__(**defaults) File "/home/sebastian/djacap/fam/src/famtest/django/forms/formsets.py", line 47, in __init__ self._construct_forms() File "/home/sebastian/djacap/fam/src/famtest/django/forms/formsets.py", line 97, in _construct_forms self.forms.append(self._construct_form(i)) File "/home/sebastian/djacap/fam/src/famtest/django/forms/models.py", line 718, in _construct_form form = super(BaseInlineFormSet, self)._construct_form(i, **kwargs) File "/home/sebastian/djacap/fam/src/famtest/django/forms/models.py", line 446, in _construct_form pk = self.data[pk_key] File "/home/sebastian/djacap/fam/src/famtest/django/utils/datastructures.py", line 235, in __getitem__ raise MultiValueDictKeyError("Key %r not found in %r" % (key, self)) MultiValueDictKeyError: "Key 'visitor_set-0-person_ptr' not found in <QueryDict: {u'visitor_set-__prefix__-event': [u'FOSDEM'], u'name': [u'FOSDEM'], u'visitor_set-0-name': [u'Me'], u'visitor_set-3-event': [u'FOSDEM'], u'visitor_set-3-name': [u''], u'visitor_set-1-event': [u'FOSDEM'], u'visitor_set-1-name': [u''], u'visitor_set-2-event': [u'FOSDEM'], u'visitor_set-2-name': [u''], u'visitor_set-INITIAL_FORMS': [u'1'], u'visitor_set-TOTAL_FORMS': [u'4'], u'csrfmiddlewaretoken': [u'85dd78ff278138c4c70f2d1c8107bd95'], u'visitor_set-0-event': [u'FOSDEM'], u'_continue': [u'Save and continue editing'], u'visitor_set-MAX_NUM_FORMS': [u''], u'visitor_set-__prefix__-name': [u'']}>"
I found following line in admin/edit_inline/tabular.html and admin/edit_inline/stacked.html.
{% if inline_admin_form.has_auto_field %}{{ inline_admin_form.pk_field.field }}{% endif %}
Removing the check for has_auto_field fixed the error described above, but leads to wrong behavior, when modifying the value of the concrete base class's primary key (the name field in the example above) in the inline admin, resulting in a new entry.
I have reproduced that issue with django 1.2.3, 1.2.4, 1.2.5 and the latest version from SVN.
Change History (19)
comment:1 by , 14 years ago
Type: | → Bug |
---|
comment:2 by , 14 years ago
Severity: | → Normal |
---|
comment:3 by , 14 years ago
Component: | Database layer (models, ORM) → contrib.admin |
---|
comment:3 by , 14 years ago
Component: | Database layer (models, ORM) → contrib.admin |
---|
comment:4 by , 14 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:5 by , 14 years ago
I was able to work around that issue and make the objects from the inline admin editable including their primary key by following hack. It deletes all objects and re-adds them again. In my opinion django must support that case by default. But maybe their is a better way as first removing all objects.
from django.contrib import admin from django.db import transaction class SuperAdmin(admin.ModelAdmin): inlines = [SubSubInline] def save_model(self, *args, **kwargs): super(SuperAdmin, self).save_model(*args, **kwargs) self._saved = True def change_view(self, request, *args, **kwargs): if request.method != 'POST': return super(SuperAdmin, self).change_view(request, *args, **kwargs) transaction.enter_transaction_management() transaction.managed(True) # ModelAdmin.change_view is decorated with autocommit. So we # have to lazy patch the transaction API in order to bypass # transaction management from within ModelAdmin.change_view. # So we can rollback the transaction afterwards if needed. transaction_api = (transaction.enter_transaction_management, transaction.leave_transaction_management, transaction.commit, transaction.rollback) transaction.enter_transaction_management = lambda *args, **kwargs: None transaction.leave_transaction_management = lambda *args, **kwargs: None transaction.commit = lambda *args, **kwargs: None transaction.rollback = lambda *args, **kwargs: None try: for inline in self.inline_instances: prefix = inline.get_formset(request).get_default_prefix() request.POST = request.POST.copy() request.POST[prefix + '-INITIAL_FORMS'] = '0' inline.queryset(request).delete() return super(SuperAdmin, self).change_view(request, *args, **kwargs) finally: (transaction.enter_transaction_management, transaction.leave_transaction_management, transaction.commit, transaction.rollback) = transaction_api if getattr(self, '_saved', False): del self._saved transaction.commit() else: transaction.rollback() transaction.leave_transaction_management()
comment:8 by , 9 years ago
This remains an issue as of 7a5b7e35bf2e219225b9f26d3dd3e34f26e83e9c (Django 1.10.dev).
comment:9 by , 8 years ago
Cc: | added |
---|
I agree it's kind of tricky when the user can change the ID (as expeced behaviour is not clear), but the issue also happens in a very normal case, simply when one is using UUIDs instead of regular autoincremented IDs :
from django.db import models import uuid class Event( models.Model ): id = models.UUIDField(primary_key=True, default=uuid.uuid4) name = models.CharField(max_length=10, blank=False) def __str__(self): return self.name class Person( models.Model ): id = models.UUIDField(primary_key=True, default=uuid.uuid4) parent = models.ForeignKey('Event', null=True, blank=True, related_name='children') name = models.CharField(max_length=10, blank=True) def __str__(self): return self.name class Visitor(Person): pass
from django.contrib import admin from .models import Event, Visitor class VisitorInline( admin.TabularInline ): model = Visitor show_change_link = True class EventAdmin( admin.ModelAdmin ): inlines = [VisitorInline,] admin.site.register(Event, EventAdmin)
comment:10 by , 8 years ago
Has patch: | set |
---|---|
Needs tests: | set |
I had a look at the source. I see two places where this can be fixed :
- in django/db/models/base.py
Model inheritance is created using OneToOneField but doesn't set the field to editable=False.
This seems wrong, but at the same time, it seem to be consistent with regular primary keys, that for some reason don't set editable=False neither.
The fix consists of settings editable=False on those fields.
I'm not sure of the consequences of this, but this breaks potentially a lot of things.
https://github.com/olivierdalang/django/commit/a4ea0a021b784c5fbbecebed01f86c3987f1a8e9
- in django/contrib/admin/helpers.py
There's a needs_explicit_pk_field() method which is exactly for that.
The fix B consists of return True if the primary key is a OneToOneField.
I guess this has much less consequences and is much safer.
https://github.com/olivierdalang/django/commit/fed7cd7f73f06a26f8a2eb8e867bdf4c81d4ae00
What do you think ? For which fix should I open a PR ? Is there any chance this gets backported to 1.9 ?
comment:13 by , 8 years ago
Patch needs improvement: | set |
---|
comment:15 by , 8 years ago
Patch needs improvement: | set |
---|
As noted on the PR, the fix allows data corruption in some cases.
comment:16 by , 7 years ago
Cc: | added |
---|
comment:18 by , 6 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
Accepted on the basis that we should do something better than crash. What exactly to do is tricky - it is similar to the case of editing the primary key value, which can result a new row rather than a changed row, IIRC.