Opened 17 years ago

Closed 16 years ago

#4193 closed (duplicate)

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

Reported by: gerry@… Owned by: Philippe Raoult
Component: Database layer (models, ORM) Version: dev
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: no UI/UX: no

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 Brian Rosner 17 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 by anonymous, 17 years ago

Version: 0.96SVN

comment:2 by Chris Beaven, 17 years ago

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?

in reply to:  2 comment:3 by anonymous, 17 years ago

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 by anonymous, 17 years ago

Owner: changed from nobody to anonymous
Status: newassigned

comment:5 by Jason Yan, 17 years ago

Owner: changed from anonymous to Jason Yan
Status: assignednew

comment:6 by Jason Yan, 17 years ago

Triage Stage: UnreviewedDesign 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 by Philippe Raoult, 17 years ago

Owner: changed from Jason Yan to Philippe Raoult
Status: newassigned

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 by Philippe Raoult, 17 years ago

might be related to #2493 too.

comment:9 by Philippe Raoult, 17 years ago

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

by Brian Rosner, 17 years ago

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

comment:10 by Philippe Raoult, 17 years ago

Has patch: set

works for me with your patch

comment:11 by Philippe Raoult, 17 years ago

Triage Stage: Design decision neededAccepted

comment:12 by James Bennett, 17 years ago

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

comment:13 by dwich, 17 years ago

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?

in reply to:  13 comment:14 by dwich, 17 years ago

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 by Brian Rosner, 16 years ago

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 by Ramiro Morales, 16 years ago

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

comment:17 by Malcolm Tredinnick, 16 years ago

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 by Ramiro Morales, 16 years ago

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 by Malcolm Tredinnick, 16 years ago

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 by Wil Tan, 16 years ago

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 by Jacob, 16 years ago

Resolution: duplicate
Status: assignedclosed

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.

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