Code

Opened 7 years ago

Closed 6 years ago

#4193 closed (duplicate)

Error in creation of foreign key depends on spelling of field class

Reported by: gerry@… Owned by: PhiR
Component: Database layer (models, ORM) Version: master
Severity: Keywords: foreignKey, spelling, SQL error
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Summary

When a field class named "Vehicle" is attempted to be used as a foreign key, the SQL generated by django is incorrect.
Changing the name of the class to "Vahicle" or some other misspelling that does not begin with "Veh" results in the correct behavior.

Example

The following model definition does not work as expected. (N.B. I've trimmed some code to reduce the model definition to its essential bug-revealing form.)

from django.db import models
import datetime

class Vehicle(models.Model):
    type = models.CharField(maxlength=15)           #  e.g. truck, car
    manufacturer = models.CharField(maxlength=15)   #  e.g. Volkswagen, Isuzu
    theModel = models.CharField(maxlength=15)       #  e.g. Passat, Trooper
    year = models.IntegerField()
    
    def __str__(self):
        return repr(self.year) + ' ' + self.manufacturer + ' ' + self.theModel

class Route(models.Model):
    locFrom = models.CharField('Starting Location', maxlength=15)
    locTo   = models.CharField('Destination', maxlength=15) 

    def __str__(self):
        return self.locFrom + ' to ' + self.locTo

class Trip(models.Model):
    vehicle = models.ForeignKey(Vehicle)
    route   = models.ForeignKey(Route)

    def __str__(self):
        return repr(self.start) + ' ' + repr(self.route) + ' ' + repr(self.vehicle)

The models.py file is in the commuteTime app in the commute project

yoshiAshi:django$ ls -al commute
total 56
drwxr-xr-x   11 gerry  gerry   374 Apr 30 23:58 .
drwxr-xr-x    8 gerry  gerry   272 May  1 08:33 ..
-rw-r--r--    1 gerry  gerry     0 Apr 30 07:38 __init__.py
-rw-r--r--    1 gerry  gerry   157 Apr 30 08:07 __init__.pyc
drwxr-xr-x    9 gerry  gerry   306 May  1 08:33 commuteTime
drwxr-xr-x    3 gerry  gerry   102 Apr 30 23:32 db
-rw-r--r--    1 gerry  gerry   546 Apr 30 07:38 manage.py
-rw-r--r--    1 gerry  gerry  2828 Apr 30 23:06 settings.py
-rw-r--r--    1 gerry  gerry  1824 Apr 30 23:06 settings.pyc
-rw-r--r--    1 gerry  gerry   227 Apr 30 07:38 urls.py

yoshiAshi:django$ ls -al commute/commuteTime
total 48
drwxr-xr-x    9 gerry  gerry   306 May  1 08:33 .
drwxr-xr-x   11 gerry  gerry   374 Apr 30 23:58 ..
-rw-r--r--    1 gerry  gerry     0 Apr 30 08:10 __init__.py
-rw-r--r--    1 gerry  gerry   169 Apr 30 23:01 __init__.pyc
drwxr-xr-x    3 gerry  gerry   102 Apr 30 08:13 db
-rw-r--r--    1 gerry  gerry   876 May  1 08:13 models.py
-rw-r--r--    1 gerry  gerry  2312 May  1 08:13 models.pyc
-rw-r--r--    1 gerry  gerry    26 Apr 30 08:10 views.py

Testing the SQL generation give this

yoshiAshi:commute$ python manage.py sql commuteTime
BEGIN;
CREATE TABLE "commuteTime_route" (
    "id" integer NOT NULL PRIMARY KEY,
    "locFrom" varchar(15) NOT NULL,
    "locTo" varchar(15) NOT NULL
);
CREATE TABLE "commuteTime_trip" (
    "id" integer NOT NULL PRIMARY KEY,
    "vehicle_id" integer NOT NULL,
    "route_id" integer NOT NULL REFERENCES "commuteTime_route" ("id")
);
CREATE TABLE "commuteTime_vehicle" (
    "id" integer NOT NULL PRIMARY KEY,
    "type" varchar(15) NOT NULL,
    "manufacturer" varchar(15) NOT NULL,
    "theModel" varchar(15) NOT NULL,
    "year" integer NOT NULL
);
COMMIT;

Note that the commuteTime_trip table does not have commuteTime_vehicle as a proper foreign key. We would expect

    "vehicle_id" integer NOT NULL REFERENCES "commuteTime_vehicle" ("id"),

but instead get

    "vehicle_id" integer NOT NULL,

Work-Around

Misspelling "Vehicle" results in the correct behavior. For example if the Vehicle class and foreign key reference are replaced by

class Vahicle(models.Model):
    type = models.CharField(maxlength=15)           #  e.g. truck, car
    manufacturer = models.CharField(maxlength=15)   #  e.g. Volkswagen, Isuzu
    theModel = models.CharField(maxlength=15)       #  e.g. Passat, Trooper
    year = models.IntegerField()
    ...

class Trip(models.Model):
    vehicle = models.ForeignKey(Vahicle)
    route   = models.ForeignKey(Route)
    ...

the correct SQL is generated

yoshiAshi:commute$ python manage.py sql commuteTime
BEGIN;
CREATE TABLE "commuteTime_route" (
    "id" integer NOT NULL PRIMARY KEY,
    "locFrom" varchar(15) NOT NULL,
    "locTo" varchar(15) NOT NULL
);
CREATE TABLE "commuteTime_vahicle" (
    "id" integer NOT NULL PRIMARY KEY,
    "type" varchar(15) NOT NULL,
    "manufacturer" varchar(15) NOT NULL,
    "theModel" varchar(15) NOT NULL,
    "year" integer NOT NULL
);
CREATE TABLE "commuteTime_trip" (
    "id" integer NOT NULL PRIMARY KEY,
    "vehicle_id" integer NOT NULL REFERENCES "commuteTime_vahicle" ("id"),
    "route_id" integer NOT NULL REFERENCES "commuteTime_route" ("id")
);
COMMIT;

I've experimented with different spellings: "Ve" gives the correct foreign key
definition, but "Veh" does not.

Weird.

Hardware and Software Configuration

Mac OS 10.4.9
python 2.3.5
django svn trunk revision 5127
Database: sqlite (for development server)

I started the discovery of this bug on another Mac running python 2.5, so I don't think it depends on the python version. I'll check later today, or maybe tonight, depending on when I get the time.

Attachments (1)

4193_model_registry_fix.diff (927 bytes) - added by brosner 7 years ago.
possible patch that maintains model ordering in the app cache since SQL creation depends on it.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 7 years ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Version changed from 0.96 to SVN

comment:2 follow-up: Changed 7 years ago by SmileyChris

Thanks for the report, Gerry - weird sounding bug...

I know it's asking a lot, but do you think you could make a model test for this?

comment:3 in reply to: ↑ 2 Changed 7 years ago by anonymous

Replying to SmileyChris:

Thanks for the report, Gerry - weird sounding bug...

I know it's asking a lot, but do you think you could make a model test for this?

I started looking into this yesterday, but got distracted by my real(TM) job. :)
I'll give it a try, but my effort will be sporadic.

I dug through the django documentation and found this page:

http://www.djangoproject.com/documentation/testing/

By "model test" I assume building a doctest string in the models.py file.
I've thought about it, but I haven't gotten any traction on how to test
that the correct SQL was created. Any suggestions on that front are welcome.

Gerry

comment:4 Changed 7 years ago by anonymous

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

comment:5 Changed 7 years ago by jason

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

comment:6 Changed 7 years ago by jason

  • Triage Stage changed from Unreviewed to Design decision needed

SQLite does not support foreign key constraints, though they are parsed. The models are stored as dictionary values per-application, so the ordering of when statements are created is not guaranteed. The SQLite backend has supports_constraints set to False, and thus sql_for_pending_references() does not add the ALTER TABLE statements to add the constraints.

The only problem I see is since the constraint isn't added to the query, the dump of the database does not contain the constraint and executing the queries on another DB like PostgreSQL will not have the proper constraints.

The solution would be to add ordering. Since Vehicle was defined before Trip, Vehicle should already be a known model by the time the constraint is needed in Trip.

comment:7 Changed 7 years ago by PhiR

  • Owner changed from jason to PhiR
  • Status changed from new to assigned

i'm not sure this would work because you're allowed to FK('modelname') if you define your model before the one you want to point to.
by the way this is the same as #5113 and most likely related to #4930.

comment:8 Changed 7 years ago by PhiR

might be related to #2493 too.

comment:9 Changed 7 years ago by PhiR

#2720 mentions the fact that foreign keys might be created after the models. Obviously it doesn't always work.

Changed 7 years ago by brosner

possible patch that maintains model ordering in the app cache since SQL creation depends on it.

comment:10 Changed 7 years ago by PhiR

  • Has patch set

works for me with your patch

comment:11 Changed 7 years ago by PhiR

  • Triage Stage changed from Design decision needed to Accepted

comment:12 Changed 7 years ago by ubernostrum

#4930 was a dupe and has some more info in the discussion.

comment:13 follow-up: Changed 7 years ago by dwich

Hi, I created a duplicate ticket #5669 because I was unable to find this one, sorry about this.

I also tried the patch attached to this ticket and it works for me (the ticket #5669) as well.

Is there any plan when this patch will be merged to trunk?

comment:14 in reply to: ↑ 13 Changed 7 years ago by dwich

Replying to dwich:
Update - I was wrong, this patch doesn't work for me. It produces correct SQL (if you see #5669), but the CONSTRAINT statement is still missing.

comment:15 Changed 6 years ago by brosner

Marked #6374 as a duplicate. It contains a patch as well. I haven't looked into this for a while so not sure what is the correct fix in this matter.

comment:16 Changed 6 years ago by ramiro

Marked #2130 as duplicate too, it contains a patch similar to the one described in #6374

comment:17 Changed 6 years ago by mtredinnick

With reference to comment 14, do you mean the constraint statement is missing for some backend other than SQLite? If so, which backend?

There won't be any constraint for SQLite, since it doesn't support it and the goal isn't to generate SQL for one backend that is completely portable to another one (there are too many subtle differences between backends, so the thought in comment 6, whilst well intentioned, isn't really practical).

comment:18 Changed 6 years ago by ramiro

Marked #6475 as duplicate. In IRC follow-up the person that opened the ticket tried the patch attached to this ticket and it didn't help (it added more commented constraints), but the one pasted on #6374 solved his problems. He was using mysql backend.

comment:19 Changed 6 years ago by mtredinnick

The patch in #6374 looks like it's closer to the root cause of fixing this problem. I haven't completely convinced myself it's the right thing yet, but that's a matter of having time to work through what's going on in there. Just noting it here in case another maintainer comes along wanting to work on this.

comment:20 Changed 6 years ago by dready

The patch I submitted in #6374 has been working for me, even in r7020 of the SVN repository. However, I wonder if this problem is specific to MySQL. I just wouldn't want to break other backends.

comment:21 Changed 6 years ago by jacob

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

I agree with Malcolm that #6374 is closer to the real root of the problem. Also, the description on this ticket confuses the real issue -- though, man, what a nifty bug! So I'm marking this of a duplicate of #6374 and reopening that one.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.