Code

Opened 4 years ago

Last modified 3 years ago

#13803 assigned Bug

Model attribute cannot be named pk and set primary_key=True

Reported by: jonknee Owned by: hjeffrey
Component: Database layer (models, ORM) Version: master
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)

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 jonknee 4 years ago.
Python crash log
13803.diff (661 bytes) - added by hjeffrey 3 years ago.

Download all attachments as: .zip

Change History (11)

Changed 4 years ago by jonknee

Python crash log

comment:1 follow-up: Changed 4 years ago by Alex

  • Description modified (diff)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 in reply to: ↑ 1 ; follow-up: Changed 4 years ago by kmtracey

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

comment:3 in reply to: ↑ 2 Changed 4 years ago by jonknee

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 Changed 3 years ago by rasca

  • milestone set to 1.3
  • Needs documentation set
  • Needs tests set
  • Summary changed from Mmodel attribute cannot be named pk and set primary_key=True to Model attribute cannot be named pk and set primary_key=True
  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from 1.2 to SVN

Changed 3 years ago by hjeffrey

comment:5 Changed 3 years ago by hjeffrey

  • Cc hjeffrey added
  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to hjeffrey
  • Status changed from new to assigned
  • Triage Stage changed from Accepted to Ready 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 Changed 3 years ago by graham_king

  • Severity set to Normal
  • Type set to Bug

comment:7 Changed 3 years ago by lukeplant

  • Easy pickings unset
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 Changed 3 years ago by ramiro

  • Keywords inspectdb added
  • UI/UX unset

comment:9 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from hjeffrey to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.