Opened 5 years ago
Closed 5 years ago
#31207 closed Bug (fixed)
ForeignObject.to_fields is allowed to point to non-local remote fields.
Reported by: | un.def | Owned by: | Hasan Ramezani |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 3.0 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
It looks like a regression in Django 3.0, possibly related to SchemaEditor.sql_create_column_inline_fk
.
Setting django.db.backends.postgresql.schema.DatabaseSchemaEditor.sql_create_column_inline_fk = False
fixes the issue.
Prerequisites
- Python 3.8.1
- PostgreSQL 9.6.12
- psycopg2 2.8.4
- Django 2.2.9 vs. 3.0.2
Django 2.2.9
Step 1. Create models:
class BaseModel(models.Model): key = models.IntegerField(unique=True) class ChildModel(BaseModel): field = models.IntegerField() class OtherModel(models.Model): field = models.IntegerField()
Step 2. Generate initial migration:
from django.db import migrations, models import django.db.models.deletion class Migration(migrations.Migration): initial = True dependencies = [ ] operations = [ migrations.CreateModel( name='BaseModel', fields=[ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), ('key', models.IntegerField(unique=True)), ], ), migrations.CreateModel( name='OtherModel', fields=[ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), ('field', models.IntegerField()), ], ), migrations.CreateModel( name='ChildModel', fields=[ ('basemodel_ptr', models.OneToOneField(auto_created=True, on_delete=django.db.models.deletion.CASCADE, parent_link=True, primary_key=True, serialize=False, to='bugapp.BaseModel')), ('field', models.IntegerField()), ], bases=('bugapp.basemodel',), ), ]
Step 3. Add foreign key from OtherModel
to ChildModel
:
class OtherModel(models.Model): field = models.IntegerField() ref = models.ForeignKey(ChildModel, on_delete=models.CASCADE, to_field='key', null=True)
Step 4. Generate migration:
from django.db import migrations, models import django.db.models.deletion class Migration(migrations.Migration): dependencies = [ ('bugapp', '0001_initial'), ] operations = [ migrations.AddField( model_name='othermodel', name='ref', field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.CASCADE, to='bugapp.ChildModel', to_field='key'), ), ]
Step 5. Inspect migration:
BEGIN; -- -- Add field ref to othermodel -- ALTER TABLE "bugapp_othermodel" ADD COLUMN "ref_id" integer NULL; CREATE INDEX "bugapp_othermodel_ref_id_9cbf2643" ON "bugapp_othermodel" ("ref_id"); ALTER TABLE "bugapp_othermodel" ADD CONSTRAINT "bugapp_othermodel_ref_id_9cbf2643_fk_bugapp_basemodel_key" FOREIGN KEY ("ref_id") REFERENCES "bugapp_basemodel" ("key") DEFERRABLE INITIALLY DEFERRED; COMMIT;
Step 6. Migrate:
Running migrations: Applying bugapp.0002_othermodel_ref... OK Applying sessions.0001_initial... OK
Django 3.0.2
Steps 1-4 are the same.
Step 5 produces different SQL code:
BEGIN; -- -- Add field ref to othermodel -- ALTER TABLE "bugapp_othermodel" ADD COLUMN "ref_id" integer NULL CONSTRAINT "bugapp_othermodel_ref_id_9cbf2643_fk_bugapp_basemodel_key" REFERENCES "bugapp_childmodel"("key") DEFERRABLE INITIALLY DEFERRED; SET CONSTRAINTS "bugapp_othermodel_ref_id_9cbf2643_fk_bugapp_basemodel_key" IMMEDIATE; CREATE INDEX "bugapp_othermodel_ref_id_9cbf2643" ON "bugapp_othermodel" ("ref_id"); COMMIT;
As you see, ref_id
column references wrong table — bugapp_childmodel
, not bugapp_basemodel
: "ref_id" ... REFERENCES "bugapp_childmodel"("key")
.
Step 6 fails:
Applying bugapp.0001_initial... OK Traceback (most recent call last): File "<...>/python3.8/site-packages/django/db/backends/utils.py", line 86, in _execute return self.cursor.execute(sql, params) psycopg2.errors.UndefinedColumn: column "key" referenced in foreign key constraint does not exist The above exception was the direct cause of the following exception: <...> File "<...>/python3.8/site-packages/django/db/backends/utils.py", line 86, in _execute return self.cursor.execute(sql, params) django.db.utils.ProgrammingError: column "key" referenced in foreign key constraint does not exist Applying bugapp.0002_othermodel_ref...
Note
It does not happen if the foreign key has been created with the model and not added later:
Step 7. Delete 0001_initial.py
Step 8. Generate migration:
# Generated by Django 3.0.2 on 2020-01-25 14:46 from django.db import migrations, models import django.db.models.deletion class Migration(migrations.Migration): initial = True dependencies = [ ] operations = [ migrations.CreateModel( name='BaseModel', fields=[ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), ('key', models.IntegerField(unique=True)), ], ), migrations.CreateModel( name='ChildModel', fields=[ ('basemodel_ptr', models.OneToOneField(auto_created=True, on_delete=django.db.models.deletion.CASCADE, parent_link=True, primary_key=True, serialize=False, to='bugapp.BaseModel')), ('field', models.IntegerField()), ], bases=('bugapp.basemodel',), ), migrations.CreateModel( name='OtherModel', fields=[ ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), ('field', models.IntegerField()), ('ref', models.ForeignKey(null=True, on_delete=django.db.models.deletion.CASCADE, to='bugapp.ChildModel', to_field='key')), ], ), ]
Step 9. Inspect migration:
BEGIN; -- -- Create model BaseModel -- CREATE TABLE "bugapp_basemodel" ("id" serial NOT NULL PRIMARY KEY, "key" integer NOT NULL UNIQUE); -- -- Create model ChildModel -- CREATE TABLE "bugapp_childmodel" ("basemodel_ptr_id" integer NOT NULL PRIMARY KEY, "field" integer NOT NULL); -- -- Create model OtherModel -- CREATE TABLE "bugapp_othermodel" ("id" serial NOT NULL PRIMARY KEY, "field" integer NOT NULL, "ref_id" integer NULL); ALTER TABLE "bugapp_childmodel" ADD CONSTRAINT "bugapp_childmodel_basemodel_ptr_id_407e3662_fk_bugapp_ba" FOREIGN KEY ("basemodel_ptr_id") REFERENCES "bugapp_basemodel" ("id") DEFERRABLE INITIALLY DEFERRED; ALTER TABLE "bugapp_othermodel" ADD CONSTRAINT "bugapp_othermodel_ref_id_9cbf2643_fk_bugapp_basemodel_key" FOREIGN KEY ("ref_id") REFERENCES "bugapp_basemodel" ("key") DEFERRABLE INITIALLY DEFERRED; CREATE INDEX "bugapp_othermodel_ref_id_9cbf2643" ON "bugapp_othermodel" ("ref_id"); COMMIT;
Note that the reference is correct now: ("ref_id") REFERENCES "bugapp_basemodel" ("key")
.
Change History (9)
comment:2 by , 5 years ago
I totally agree that
to_field should only be allowed to point a local fields
but in this case the behavior should be consistent. As I mentioned before (Note section), I still can create “non-local” foreign key reference (i.e. reference pointing to child model but to field from parent model) with CreateModel(fields=...)
(but not with AddField(...)
).
So, my suggestions are as follows:
- completely forbid
to_field
that points to field of parent non-abstract model (as you noted in the last sentence); - add a note to documentation about changed behavior (previous behavior was not documented but can be used in the wild; actually, I have run into this problem in a legacy project).
comment:3 by , 5 years ago
Triage Stage: | Unreviewed → Accepted |
---|
So, my suggestions are as follows: ...
- completely forbid to_field that points to field of parent non-abstract model;
Makes sense to me, the logic will likely involve ensuring to_field.model == self.remote_field.model
in ForeignObject.resolve_related_fields
comment:4 by , 5 years ago
Summary: | Broken foreing key reference with multi-table inheritance (PostgreSQL, Django 3.0) → ForeignObject.to_fields is allowed to point to non-local remote fields. |
---|
comment:5 by , 5 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:7 by , 5 years ago
Patch needs improvement: | set |
---|
comment:8 by , 5 years ago
Patch needs improvement: | unset |
---|
comment:9 by , 5 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Something seems off with your models setup, unless
BaseModel
is abstract then thekey
column will be defined on its table so it's simply not possible to have a foreign key fromOtherModel
toChildModel
with ato_field='key'
.It looks like something that happened to work by chance previously as the foreign key could be pointing at an instance of
BaseModel
that isn't aChildModel
so this breaks referential integrity;to_field
should only be allowed to point a local fields.e.g.
Looks like what you are looking for is
And that
ForeignKey
should be adjusted to prevent usage ofto_field
pointing to non-local fields.