Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#10992 closed (fixed)

Unable to re-save inlines with custom char primary key

Reported by: marcob Owned by: zain
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 6 years ago.
Tests demonstrating the bug
10992_fix.diff (1.5 KB) - added by zain 6 years ago.
Fix for this bug
10992-fix_and_tests-v2.diff (8.6 KB) - added by zain 6 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 6 years ago by russellm

  • milestone set to 1.1
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 6 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 6 years ago by russellm

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

  • Owner changed from nobody to anonymous
  • Status changed from new to assigned

comment:5 Changed 6 years ago by zain

  • Owner changed from anonymous to zain
  • Status changed from assigned to new

comment:6 Changed 6 years ago by zain

  • Status changed from new to assigned

comment:7 Changed 6 years ago by zain

  • 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 6 years ago by zain

Tests demonstrating the bug

Changed 6 years ago by zain

Fix for this bug

comment:8 Changed 6 years ago by russellm

  • 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 6 years ago by russellm

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 6 years ago by zain

  • 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 6 years ago by zain

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

comment:11 Changed 6 years ago by jacob

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:12 Changed 4 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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