Opened 14 years ago

Closed 10 years ago

#13803 closed Bug (fixed)

Model attribute cannot be named pk and set primary_key=True

Reported by: Jon Gales Owned by: hjeffrey
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: inspectdb
Cc: hjeffrey Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Alex Gaynor)

I had an existing database that I wanted to manipulate with Django and ran in through the inspectdb management command. Opening up the admin to try and edit the table led to a consistent Python crash (OS X popped up the crash log which I attached). After trial and error I narrowed it down to the name of the model attribute: "pk". Changing the attribute name to something else and then setting db_column='pk' allowed everything to function normally.

When accessing the model through the shell Python doesn't crash, but no data is returned on queries and nothing can be inserted. Here's an example model that shows the problem, run syncdb to set it up and then try to interact with it.

from django.db import models

class Example(models.Model):
    pk = models.AutoField(primary_key=True, db_column='pk')

---

>> test = Example()

File "/Users/jonknee/src/envs/django_1.2/lib/python2.6/site-packages/django/db/models/base.py", line 403, in _set_pk_val
    return setattr(self, self._meta.pk.attname, value)
RuntimeError: maximum recursion depth exceeded while calling a Python object

Changing the attribute name makes everything work normally:

class Example(models.Model):
    pkey = models.AutoField(primary_key=True, db_column='pk')

This is an issue in both 1.1 and 1.2, I'm using Python 2.6.1.

Attachments (2)

python_2010-06-20-143821_jonny.crash (66.9 KB ) - added by Jon Gales 14 years ago.
Python crash log
13803.diff (661 bytes ) - added by hjeffrey 13 years ago.

Download all attachments as: .zip

Change History (12)

by Jon Gales, 14 years ago

Python crash log

comment:1 by Alex Gaynor, 14 years ago

Description: modified (diff)

Fixed up formatting. Also this is just going to be a docs issue IMO.

in reply to:  1 ; comment:2 by Karen Tracey, 14 years ago

Replying to Alex:

Fixed up formatting. Also this is just going to be a docs issue IMO.

Well, inspectdb could be made smarter and attempt to avoid the problem. #12460 is already open for some improvements along those lines to prevent generation of bad field names. Also if you manually attempt to name a field pk that could get flagged as an error...

in reply to:  2 comment:3 by Jon Gales, 14 years ago

Replying to kmtracey:

#12460 is already open for some improvements along those lines to prevent generation of bad field names. Also if you manually attempt to name a field pk that could get flagged as an error...

In a related issue, Django doesn't throw up any roadblocks to calling a non primary key field pk which leads to odd behavior (the attribute pk and the actual primary key can't be separately set). Flagging that as an error gets a +1 from me. It's not a good thing when valid Python takes the ship down, doubly so when it's code created by inspectdb and a table cleanly made by syncdb. It took me a long time to figure out because the crashlog had nothing that hinted at what it was.

comment:4 by rasca, 13 years ago

milestone: 1.3
Needs documentation: set
Needs tests: set
Summary: Mmodel attribute cannot be named pk and set primary_key=TrueModel attribute cannot be named pk and set primary_key=True
Triage Stage: UnreviewedAccepted
Version: 1.2SVN

by hjeffrey, 13 years ago

Attachment: 13803.diff added

comment:5 by hjeffrey, 13 years ago

Cc: hjeffrey added
Has patch: set
Needs documentation: unset
Needs tests: unset
Owner: changed from nobody to hjeffrey
Status: newassigned
Triage Stage: AcceptedReady for checkin

I think that this is at least a partial fix. The core should throw an exception when you name a model attribute "pk"

comment:6 by Graham King, 13 years ago

Severity: Normal
Type: Bug

comment:7 by Luke Plant, 13 years ago

Easy pickings: unset
Needs tests: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

This is the wrong place to put the check, since it will only trigger when the table is created by syncdb, which is not a necessary step for using a Django model. It should probably be in django/core/management/validation.py, with tests in modeltests/invalid_models/models.py

Also, AttributeError would certainly not be the right exception to raise - http://docs.python.org/library/exceptions.html#exceptions.AttributeError

By the way, you should not normally set 'Ready for checkin' yourself, but rather get someone else to review your patch and set that.

comment:8 by Ramiro Morales, 13 years ago

Keywords: inspectdb added
UI/UX: unset

comment:9 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:10 by Olivier Le Thanh Duong, 10 years ago

Resolution: fixed
Status: assignedclosed

Looks like this bug was fixed in ee9fcb1672ddf5910ed8c45c37a00f32ebbe2bb1

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