Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#8316 closed (fixed)

MySQL error with custom primary_key that is not an IntegerField

Reported by: julianb Owned by: julianb
Component: Database layer (models, ORM) Version: master
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: UI/UX:

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

Download all attachments as: .zip

Change History (17)

Changed 7 years ago by julianb

ForeignKey that points to an AutoField now becomes KeyField

Changed 7 years ago by julianb

v2

Changed 7 years ago by julianb

v3, removed some more now unneeded imports

comment:1 Changed 7 years ago by julianb

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

Changed 7 years ago by julianb

v4, effectively handling that as a special MySQL case

comment:2 Changed 7 years ago by mtredinnick

  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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

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

Changed 7 years ago by julianb

v5, removed some unnecessary stuff

Changed 7 years ago by julianb

v6, fixed an oversight

Changed 7 years ago by julianb

Test case, currently fails when using MySQL+Innodb

comment:4 Changed 7 years ago by mtredinnick

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

Changed 7 years ago by julianb

comment:5 Changed 7 years ago by julianb

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

  • 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 Changed 7 years ago by jacob

  • Keywords 1.0-blocker added

comment:8 Changed 7 years ago by mtredinnick

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

(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 Changed 4 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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