Opened 16 years ago

Closed 16 years ago

Last modified 12 years ago

#8316 closed (fixed)

MySQL error with custom primary_key that is not an IntegerField

Reported by: Julian Bez Owned by: Julian Bez
Component: Database layer (models, ORM) Version: dev
Severity: Keywords: 1.0-blocker
Cc: sebastian@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Create a model with a custom primary key field that is not an IntegerField and another with a foreign key pointing to it:

class Author(models.Model):
    id = models.PositiveIntegerField(primary_key=True)
    name = models.CharField(max_length=200)

class Article(models.Model):
    title = models.CharField(max_length=200)
    author = models.ForeignKey(Author)

Let's see how the SQL looks:

BEGIN;
CREATE TABLE `dbtest_author` (
    `id` integer UNSIGNED NOT NULL PRIMARY KEY,
    `name` varchar(200) NOT NULL
)
;
CREATE TABLE `dbtest_article` (
    `id` integer AUTO_INCREMENT NOT NULL PRIMARY KEY,
    `title` varchar(200) NOT NULL,
    `author_id` integer NOT NULL
)
;
ALTER TABLE `dbtest_article` ADD CONSTRAINT author_id_refs_id_6bbed69f FOREIGN KEY (`author_id`) REFERENCES `dbtest_author` (`id`);
CREATE INDEX `dbtest_article_author_id` ON `dbtest_article` (`author_id`);
COMMIT;

You can see that author_id should reference dbtest_author.id, but they have different data types.

Calling syncdb:

Creating table dbtest_author
Creating table dbtest_article
Traceback (most recent call last):
  File "...manage.py", line 11, in <module>
    execute_manager(settings)
  File "C:\Programme\django_src\django\core\management\__init__.py", line 334, in execute_manager
    utility.execute()
  File "C:\Programme\django_src\django\core\management\__init__.py", line 295, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "C:\Programme\django_src\django\core\management\base.py", line 77, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "C:\Programme\django_src\django\core\management\base.py", line 96, in execute
    output = self.handle(*args, **options)
  File "C:\Programme\django_src\django\core\management\base.py", line 178, in handle
    return self.handle_noargs(**options)
  File "C:\Programme\django_src\django\core\management\commands\syncdb.py", line 80, in handle_noargs
    cursor.execute(statement)
  File "C:\Programme\django_src\django\db\backends\util.py", line 19, in execute
    return self.cursor.execute(sql, params)
  File "C:\Programme\Python25\lib\site-packages\MySQLdb\cursors.py", line 166, in execute
    self.errorhandler(self, exc, value)
  File "C:\Programme\Python25\lib\site-packages\MySQLdb\connections.py", line 35, in defaulterrorhandler
    raise errorclass, errorvalue
_mysql_exceptions.OperationalError: (1005, "Can't create table '.\\db\\#sql-5b4_8.frm' (errno: 150)")

MySQL is unable to add the constraint, because of the different types (integer, integer unsigned) used.

The problem goes back to [1970] where a foreign key pointing to an AutoField, a PositiveIntegerField or a PositiveSmallIntegerField always becomes an IntegerField.

Attachments (8)

db-mysqltypeissue.diff (3.4 KB ) - added by Julian Bez 16 years ago.
ForeignKey that points to an AutoField now becomes KeyField
db-mysqltypeissue.2.diff (5.5 KB ) - added by Julian Bez 16 years ago.
v2
db-mysqltypeissue.3.diff (5.4 KB ) - added by Julian Bez 16 years ago.
v3, removed some more now unneeded imports
db-mysqltypeissue.4.diff (5.9 KB ) - added by Julian Bez 16 years ago.
v4, effectively handling that as a special MySQL case
db-mysqltypeissue.5.diff (2.1 KB ) - added by Julian Bez 16 years ago.
v5, removed some unnecessary stuff
db-mysqltypeissue.6.diff (2.1 KB ) - added by Julian Bez 16 years ago.
v6, fixed an oversight
db-mysqltypetest.diff (1.1 KB ) - added by Julian Bez 16 years ago.
Test case, currently fails when using MySQL+Innodb
db-mysqltypeissue.7.diff (3.3 KB ) - added by Julian Bez 16 years ago.

Download all attachments as: .zip

Change History (17)

by Julian Bez, 16 years ago

Attachment: db-mysqltypeissue.diff added

ForeignKey that points to an AutoField now becomes KeyField

by Julian Bez, 16 years ago

Attachment: db-mysqltypeissue.2.diff added

v2

by Julian Bez, 16 years ago

Attachment: db-mysqltypeissue.3.diff added

v3, removed some more now unneeded imports

comment:1 by Julian Bez, 16 years ago

Whoops, the patch makes no sense yet, because it reverses [1970] for every backend, not just MySQL.

by Julian Bez, 16 years ago

Attachment: db-mysqltypeissue.4.diff added

v4, effectively handling that as a special MySQL case

comment:2 by Malcolm Tredinnick, 16 years ago

Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Only looking at the final patch, it seems to effectively do nothing. Specific problems are:

  • You cannot remove OneToOneField from creation.py. It's a valid field type.
  • This new KeyField class is never used in any way other than as an IntegerField, so it serves no purpose. Maybe it had some point in an earlier patch, but if you replace it with IntegerField here, no functionality changes.

What you probably want to do is use something like the db_type() from rel_field around line 721 of django.db.models.field.related.

The missing tests that need to be written would have shown all those problems. Running the existing test suite would have shown that OneToOneField was used in more than one place.

This is a legimate bug, but the ultimate patch will probably only change about 3 lines of code plus add some tests.

comment:3 by Julian Bez, 16 years ago

I ran existing tests and they showed no problems. I did not quite know how to write tests specifically for that issue, because it needs MySQL with Innodb...

by Julian Bez, 16 years ago

Attachment: db-mysqltypeissue.5.diff added

v5, removed some unnecessary stuff

by Julian Bez, 16 years ago

Attachment: db-mysqltypeissue.6.diff added

v6, fixed an oversight

by Julian Bez, 16 years ago

Attachment: db-mysqltypetest.diff added

Test case, currently fails when using MySQL+Innodb

comment:4 by Malcolm Tredinnick, 16 years ago

Please make one combined patch so that things stay in sync.

Also, better to put the tests in regressiontests/model_fields/, since these are just tests guarding against a regression, not example usage (which is how a log of the modeltests tests are used in our automatically generated example docs).

by Julian Bez, 16 years ago

Attachment: db-mysqltypeissue.7.diff added

comment:5 by Julian Bez, 16 years ago

Thanks for the tips.

I was wondering if keys_need_similar_types = True (in case of MySQL) is the best we can use here. Maybe something like unsigned_keys = True, keys_only_integer = False or so would be more reusable, but they don't really appeal to me. What do you think?

comment:6 by spaetz, 16 years ago

Cc: sebastian@… added

Ouch, got bitten by this today. Latest patch seems to have tests, can people look at it and reset the tests flag if appropriate?

comment:7 by Jacob, 16 years ago

Keywords: 1.0-blocker added

comment:8 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

(In [8782]) Fixed #8316 -- Put tighter restrictions on the type of Foreign Key fields
created for MySQL (because MySQL + InnoDB has those restrictions).
Patch from julianb.

comment:9 by Jacob, 12 years ago

milestone: 1.0

Milestone 1.0 deleted

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