#10992 closed (fixed)
Unable to re-save inlines with custom char primary key
Reported by: | marcob | Owned by: | Zain Memon |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Keywords: | inline custom primary key | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Define a simple models.py like this one:
from django.db import models class Master(models.Model): codice = models.CharField( primary_key=True, max_length=30, ) class Detail(models.Model): codice = models.CharField( primary_key=True, max_length=30, ) fk = models.ForeignKey(Master)
And an admin.py like this one:
from django.contrib import admin from models import Master, Detail class DetailInline(admin.TabularInline): model = Detail class MasterAdmin(admin.ModelAdmin): inlines = [DetailInline,] admin.site.register(Master, MasterAdmin)
Create and save a Master record with a Detail inline record.
Reopen the Master record, try to save it again.
Bang!
AttributeError at /admin/pluto/master/test/ 'unicode' object has no attribute 'pk' Exception Location: C:\work\esempio\lib\django\forms\models.py in save_existing_objects, line 521
I fixed it changing line 521:
try: pk_value = form.fields[pk_name].clean(raw_pk_value).pk except AttributeError: pk_value = form.fields[pk_name].clean(raw_pk_value)
Attachments (3)
Change History (15)
comment:1 by , 16 years ago
milestone: | → 1.1 |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 16 years ago
Russell, sorry for not having attached a patch, but the fix "runs" only with an Attribute Error Exception. So it's impossible it can break other cases.
You have to substitute line 521 with this 4 lines:
try: pk_value = form.fields[pk_name].clean(raw_pk_value).pk except AttributeError: pk_value = form.fields[pk_name].clean(raw_pk_value)
Btw I do agree this is a workaround. We need to fix clean (with charfield it doesn't return the instance).
comment:3 by , 16 years ago
Apologies Marco - I misread your suggestion as just removing the .pk portion. However, you are correct - this is a workaround, not a solution.
comment:4 by , 16 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:5 by , 16 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
comment:6 by , 16 years ago
Status: | new → assigned |
---|
comment:7 by , 16 years ago
Has patch: | set |
---|
When the pk isn't explicitly specified in the model, it is rendered with a ModelChoiceAdmin field. But when it is explicitly specified, it's rendered with the field for the data type of the pk (CharField in this case).
ModelChoiceAdmin's clean() method returns the model object instance, while CharField's clean() method returns a char. Thus the bug.
Fix incoming.
comment:8 by , 16 years ago
Patch needs improvement: | set |
---|
General approach to this problem looks good, but two problems with the patch:
- It breaks the modeltests.model_formsets unit test suite.
- The patch removes the use of "existing_objects". This exists as an optimization - the call to get_queryset() means that all inline objects can be retrieved with a single SQL statement, whereas the patch changes this so that each inline object requires a independent SQL select.
comment:9 by , 16 years ago
I committed (commented out) a slightly more robust version of the provided test case as a part of [10725]. If anyone gets around to looking at this before I do, the new patch just needs to uncomment the appropriate tests.
comment:10 by , 15 years ago
Patch needs improvement: | unset |
---|
Apparently there are cases other than ModelChoiceAdmin where the clean function will return a model instance (as demonstrated by the failing model_formsets test case), so I'm just checking for the existence of a pk attribute and using it if it exists. Also, uncommented your test case.
by , 15 years ago
Attachment: | 10992-fix_and_tests-v2.diff added |
---|
Fix incorporating russellm's suggestions in comment 8 + uncommenting the tests from [10725]
comment:11 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Verified that the problem exists, but the suggested fix doesn't work - it may fix the problem for charfield PK's, but it breaks all other cases (and the test suite verifies this).