Opened 16 years ago

Closed 15 years ago

Last modified 13 years ago

#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)

10992_tests.diff (3.2 KB ) - added by Zain Memon 16 years ago.
Tests demonstrating the bug
10992_fix.diff (1.5 KB ) - added by Zain Memon 16 years ago.
Fix for this bug
10992-fix_and_tests-v2.diff (8.6 KB ) - added by Zain Memon 15 years ago.
Fix incorporating russellm's suggestions in comment 8 + uncommenting the tests from [10725]

Download all attachments as: .zip

Change History (15)

comment:1 by Russell Keith-Magee, 16 years ago

milestone: 1.1
Triage Stage: UnreviewedAccepted

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).

comment:2 by marcob, 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 Russell Keith-Magee, 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 anonymous, 16 years ago

Owner: changed from nobody to anonymous
Status: newassigned

comment:5 by Zain Memon, 16 years ago

Owner: changed from anonymous to Zain Memon
Status: assignednew

comment:6 by Zain Memon, 16 years ago

Status: newassigned

comment:7 by Zain Memon, 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.

by Zain Memon, 16 years ago

Attachment: 10992_tests.diff added

Tests demonstrating the bug

by Zain Memon, 16 years ago

Attachment: 10992_fix.diff added

Fix for this bug

comment:8 by Russell Keith-Magee, 16 years ago

Patch needs improvement: set

General approach to this problem looks good, but two problems with the patch:

  1. It breaks the modeltests.model_formsets unit test suite.
  2. 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 Russell Keith-Magee, 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 Zain Memon, 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 Zain Memon, 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 Jacob, 15 years ago

Resolution: fixed
Status: assignedclosed

(In [10777]) Fixed #10992: fixed a bug saving inlines with custom primary key fields. Thanks, Zain.

comment:12 by Jacob, 13 years ago

milestone: 1.1

Milestone 1.1 deleted

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