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:1 by Simon Charette, 5 years ago

Something seems off with your models setup, unless BaseModel is abstract then the key column will be defined on its table so it's simply not possible to have a foreign key from OtherModel to ChildModel with a to_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 a ChildModel so this breaks referential integrity; to_field should only be allowed to point a local fields.

e.g.

class BaseModel(models.Model):
    key = models.IntegerField(unique=True)

class ChildModel(BaseModel):
    pass

class OtherChildModel(BaseModel):
    pass

class OtherModel(models.Model):
    ref = models.ForeignKey(ChildModel, to_field='key')

>>> child = OtherChildModel.objects.create(key='foo')
>>> other = OtherModel.objects.create(ref=child.key)
>>> other.refresh_from_db()
>>> other.ref  # raises ChildModel.DoesNotExist

Looks like what you are looking for is

class BaseModel(models.Model):
    key = models.IntegerField(unique=True)

class ChildModel(BaseModel):
    pass

class OtherModel(models.Model):
    ref = models.ForeignKey(BaseModel, to_field='key')
    child_ref = ForeignObject(ChildModel, from_fields=('ref_id',), to_fields=('basemodel_ptr_id',))

And that ForeignKey should be adjusted to prevent usage of to_field pointing to non-local fields.

Version 0, edited 5 years ago by Simon Charette (next)

comment:2 by un.def, 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).
Last edited 5 years ago by un.def (previous) (diff)

comment:3 by Simon Charette, 5 years ago

Triage Stage: UnreviewedAccepted

So, my suggestions are as follows: ...

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

https://github.com/django/django/blob/5d654e1e7104d2ce86ec1b9fe52865a7dca4b4be/django/db/models/fields/related.py#L614-L615

comment:4 by Simon Charette, 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 Hasan Ramezani, 5 years ago

Owner: changed from nobody to Hasan Ramezani
Status: newassigned

comment:6 by Hasan Ramezani, 5 years ago

Has patch: set
Last edited 5 years ago by Mariusz Felisiak (previous) (diff)

comment:7 by Mariusz Felisiak, 5 years ago

Patch needs improvement: set

comment:8 by Hasan Ramezani, 5 years ago

Patch needs improvement: unset

comment:9 by Claude Paroz, 5 years ago

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top