Opened 8 years ago

Closed 8 years ago

Last modified 5 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: master
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: UI/UX:

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 8 years ago.
Tests demonstrating the bug
10992_fix.diff (1.5 KB) - added by Zain Memon 8 years ago.
Fix for this bug
10992-fix_and_tests-v2.diff (8.6 KB) - added by Zain Memon 8 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 Changed 8 years ago by Russell Keith-Magee

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 Changed 8 years ago by marcob

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 Changed 8 years ago by Russell Keith-Magee

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 Changed 8 years ago by anonymous

Owner: changed from nobody to anonymous
Status: newassigned

comment:5 Changed 8 years ago by Zain Memon

Owner: changed from anonymous to Zain Memon
Status: assignednew

comment:6 Changed 8 years ago by Zain Memon

Status: newassigned

comment:7 Changed 8 years ago by Zain Memon

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.

Changed 8 years ago by Zain Memon

Attachment: 10992_tests.diff added

Tests demonstrating the bug

Changed 8 years ago by Zain Memon

Attachment: 10992_fix.diff added

Fix for this bug

comment:8 Changed 8 years ago by Russell Keith-Magee

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 Changed 8 years ago by Russell Keith-Magee

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 Changed 8 years ago by Zain Memon

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.

Changed 8 years ago by Zain Memon

Attachment: 10992-fix_and_tests-v2.diff added

Fix incorporating russellm's suggestions in comment 8 + uncommenting the tests from [10725]

comment:11 Changed 8 years ago by Jacob

Resolution: fixed
Status: assignedclosed

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

comment:12 Changed 5 years ago by Jacob

milestone: 1.1

Milestone 1.1 deleted

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