Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#24264 closed Bug (fixed)

foreign key constraint error migrating integer pk to CharField

Reported by: Tim Graham Owned by: Markus Holtermann
Component: Migrations Version: 1.8alpha1
Severity: Release blocker Keywords:
Cc: Markus Holtermann Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Discovered while trying to update djangoproject.com to 1.8a1. See https://github.com/django/djangoproject.com/pull/373 for steps to reproduce.

$ python manage.py test fundraising
...
foreign key constraint "fundrais_donor_id_50d7dcbcff28bd90_fk_fundraising_djangohero_id" cannot be implemented
DETAIL:  Key columns "donor_id" and "id" are of incompatible types: integer and character varying.

Bisected to bbbed99f6260a8b3e65cb990e49721b1ce4a441b

Here's a test for tests/schema/tests.py which is at least a start (it fails on 1.7.x too, so it's probably not quite right):

def test_change_int_pk_to_char(self):
    with connection.schema_editor() as editor:
        editor.create_model(Author)
        editor.create_model(Book)
    new_field = CharField(max_length=15, primary_key=True)
    new_field.set_attributes_from_name("id")
    new_field.model = Author
    with connection.schema_editor() as editor:
        editor.alter_field(Author, Author._meta.get_field("id"), new_field)

Change History (12)

comment:1 Changed 4 years ago by Markus Holtermann

Cc: Markus Holtermann added
Triage Stage: UnreviewedAccepted

Although I haven't tried to reproduce this particular issue, I wouldn't be surprised if this is related to #24241

A brief discussion on IRC mentioned this traceback:

Traceback (most recent call last):
  File "manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "/home/tim/code/django/django/core/management/__init__.py", line 338, in execute_from_command_line
    utility.execute()
  File "/home/tim/code/django/django/core/management/__init__.py", line 330, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/tim/code/django/django/core/management/commands/test.py", line 30, in run_from_argv
    super(Command, self).run_from_argv(argv)
  File "/home/tim/code/django/django/core/management/base.py", line 390, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/home/tim/code/django/django/core/management/commands/test.py", line 74, in execute
    super(Command, self).execute(*args, **options)
  File "/home/tim/code/django/django/core/management/base.py", line 441, in execute
    output = self.handle(*args, **options)
  File "/home/tim/code/django/django/core/management/commands/test.py", line 90, in handle
    failures = test_runner.run_tests(test_labels)
  File "/home/tim/code/django/django/test/runner.py", line 210, in run_tests
    old_config = self.setup_databases()
  File "/home/tim/code/django/django/test/runner.py", line 166, in setup_databases
    **kwargs
  File "/home/tim/code/django/django/test/runner.py", line 370, in setup_databases
    serialize=connection.settings_dict.get("TEST", {}).get("SERIALIZE", True),
  File "/home/tim/code/django/django/db/backends/base/creation.py", line 369, in create_test_db
    test_flush=True,
  File "/home/tim/code/django/django/core/management/__init__.py", line 120, in call_command
    return command.execute(*args, **defaults)
  File "/home/tim/code/django/django/core/management/base.py", line 441, in execute
    output = self.handle(*args, **options)
  File "/home/tim/code/django/django/core/management/commands/migrate.py", line 213, in handle
    executor.migrate(targets, plan, fake=options.get("fake", False))
  File "/home/tim/code/django/django/db/migrations/executor.py", line 93, in migrate
    self.apply_migration(states[migration], migration, fake=fake)
  File "/home/tim/code/django/django/db/migrations/executor.py", line 129, in apply_migration
    state = migration.apply(state, schema_editor)
  File "/home/tim/code/django/django/db/migrations/migration.py", line 110, in apply
    operation.database_forwards(self.app_label, schema_editor, old_state, project_state)
  File "/home/tim/code/django/django/db/migrations/operations/fields.py", line 200, in database_forwards
    schema_editor.alter_field(from_model, from_field, to_field)
  File "/home/tim/code/django/django/db/backends/base/schema.py", line 484, in alter_field
    old_db_params, new_db_params, strict)
  File "/home/tim/code/django/django/db/backends/base/schema.py", line 633, in _alter_field
    params,
  File "/home/tim/code/django/django/db/backends/base/schema.py", line 107, in execute
    cursor.execute(sql, params)
  File "/home/tim/code/django/django/db/backends/utils.py", line 65, in execute
    return self.cursor.execute(sql, params)
  File "/home/tim/code/django/django/db/utils.py", line 95, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "/home/tim/code/django/django/db/backends/utils.py", line 65, in execute
    return self.cursor.execute(sql, params)
django.db.utils.ProgrammingError: foreign key constraint "fundrais_donor_id_50d7dcbcff28bd90_fk_fundraising_djangohero_id" cannot be implemented
DETAIL:  Key columns "donor_id" and "id" are of incompatible types: integer and character varying.

and this SQL

BEGIN;
ALTER TABLE "fundraising_donation" DROP CONSTRAINT "fundraising_donation_donor_id_50d7dcbcff28bd90_fk";
ALTER TABLE "fundraising_djangohero" ALTER COLUMN "id" TYPE varchar(12);
ALTER TABLE "fundraising_donation" ALTER COLUMN "donor_id" TYPE varchar(12);
ALTER TABLE "fundraising_donation" ADD CONSTRAINT "fundraising_donation_donor_id_50d7dcbcff28bd90_fk" FOREIGN KEY ("donor_id") REFERENCES "fundraising_djangohero" ("id") DEFERRABLE INITIALLY DEFERRED;

The interesting part is the difference between the constraint names.

comment:2 Changed 4 years ago by Markus Holtermann

I looked into the issue: it is neither related nor unrelated to the bisected commit mentioned above. The problem is that in django.db.backends.base.schema._alter_field() new_field.model._meta.related_objects for DjangoHero is an empty list despite Donation having a FK.

The related_objects_graph after the first for loop looks like:

{<Options for DjangoHero>: [<django.db.models.fields.related.ForeignKey: donor>],
 <Options for Site>: [<django.db.models.fields.related.ForeignKey: site>,
                      <django.db.models.fields.related.ManyToManyField: sites>],
 <Options for FlatPage>: [<django.db.models.fields.related.ForeignKey: flatpage>],
 <Options for DocumentRelease>: [<django.db.models.fields.related.ForeignKey: release>],
 <Options for Category>: [<django.db.models.fields.related.ForeignKey: category>,
                          <django.db.models.fields.related.ForeignKey: category>,
                          <django.db.models.fields.related.ForeignKey: category>,
                          <django.db.models.fields.related.ForeignKey: category>],
 <Options for ContentType>: [<django.db.models.fields.related.ForeignKey: content_type>],
 <Options for CCLA>: [<django.db.models.fields.related.ForeignKey: ccla>],
 <Options for ICLA>: [<django.db.models.fields.related.ForeignKey: icla>],
 <Options for Group>: [<django.db.models.fields.related.ForeignKey: group>,
                       <django.db.models.fields.related.ForeignKey: group>,
                       <django.db.models.fields.related.ManyToManyField: groups>],
 <Options for Permission>: [<django.db.models.fields.related.ForeignKey: permission>,
                            <django.db.models.fields.related.ManyToManyField: user_permissions>],
 <Options for User>: [<django.db.models.fields.related.ForeignKey: user>,
                      <django.db.models.fields.related.ForeignKey: user>,
                      <django.db.models.fields.related.ForeignKey: user>,
                      <django.db.models.fields.related.ForeignKey: user>],
 <Options for Permission>: [<django.db.models.fields.related.ForeignKey: permission>,
                            <django.db.models.fields.related.ManyToManyField: permissions>],
 <Options for FeedType>: [<django.db.models.fields.related.ForeignKey: feed_type>],
 <Options for Feed>: [<django.db.models.fields.related.ForeignKey: feed>],
 <Options for User>: [<django.db.models.fields.related.OneToOneField: user>,
                      <django.db.models.fields.related.ForeignKey: user>,
                      <django.db.models.fields.related.ForeignKey: owner>],
 <Options for ContentType>: [<django.db.models.fields.related.ForeignKey: content_type>,
                             <django.db.models.fields.related.ForeignKey: content_type>]}

However, due to state changes during migrations self.model (new_field.model._meta.model) is not the same instance as the DjangoHero model from all_models()

comment:3 Changed 4 years ago by Markus Holtermann

This patch on stable/1.8.x (edbf6de7536f7a6c1e5df019a5e1947d2c9dadf8) fixes the problem for me, but is more a band-aid than a valid fix:

  • django/db/models/options.py

    diff --git a/django/db/models/options.py b/django/db/models/options.py
    index fa97265..e64990d 100644
    a b class Options(object): 
    687687            for f in fields_with_relations:
    688688                if not isinstance(f.rel.to, six.string_types):
    689689                    # Set options_instance -> field
    690                     related_objects_graph[f.rel.to._meta].append(f)
     690                    opts = f.rel.to._meta
     691                    related_objects_graph[opts.app_label, opts.object_name].append(f)
    691692
    692693        for model in all_models:
    693694            # Set the relation_tree using the internal __dict__. In this way
    class Options(object): 
    695696            # __dict__ takes precedence over a data descriptor (such as
    696697            # @cached_property). This means that the _meta._relation_tree is
    697698            # only called if related_objects is not in __dict__.
    698             related_objects = related_objects_graph[model._meta]
     699            opts = model._meta
     700            related_objects = related_objects_graph[opts.app_label, opts.object_name]
    699701
    700702            # If related_objects are empty, it makes sense to set
    701703            # EMPTY_RELATION_TREE. This will avoid allocating multiple empty

comment:4 Changed 4 years ago by Markus Holtermann

I managed to construct a test_case for this issue.

I made the FK span two apps to make sure it works across apps. Within the same app it boils down to the same problem.

  • new file tests/migrations/migrations_test_apps/alter_fk/author_app/migrations/0001_initial.py

    diff --git a/tests/migrations/migrations_test_apps/alter_fk/__init__.py b/tests/migrations/migrations_test_apps/alter_fk/__init__.py
    new file mode 100644
    index 0000000..e69de29
    diff --git a/tests/migrations/migrations_test_apps/alter_fk/author_app/__init__.py b/tests/migrations/migrations_test_apps/alter_fk/author_app/__init__.py
    new file mode 100644
    index 0000000..e69de29
    diff --git a/tests/migrations/migrations_test_apps/alter_fk/author_app/migrations/0001_initial.py b/tests/migrations/migrations_test_apps/alter_fk/author_app/migrations/0001_initial.py
    new file mode 100644
    index 0000000..6017d16
    - +  
     1# -*- coding: utf-8 -*-
     2from __future__ import unicode_literals
     3
     4from django.db import models, migrations
     5
     6
     7class Migration(migrations.Migration):
     8
     9    dependencies = [
     10    ]
     11
     12    operations = [
     13        migrations.CreateModel(
     14            name='Author',
     15            fields=[
     16                ('id', models.AutoField(serialize=False, auto_created=True, primary_key=True)),
     17                ('name', models.CharField(max_length=50)),
     18            ],
     19        ),
     20    ]
  • new file tests/migrations/migrations_test_apps/alter_fk/author_app/migrations/0002_alter_id.py

    diff --git a/tests/migrations/migrations_test_apps/alter_fk/author_app/migrations/0002_alter_id.py b/tests/migrations/migrations_test_apps/alter_fk/author_app/migrations/0002_alter_id.py
    new file mode 100644
    index 0000000..0256b75
    - +  
     1# -*- coding: utf-8 -*-
     2from __future__ import unicode_literals
     3
     4from django.db import models, migrations
     5
     6
     7class Migration(migrations.Migration):
     8
     9    dependencies = [
     10        ('author_app', '0001_initial'),
     11        ('book_app', '0001_initial'),  # Forces the book table to alter the FK
     12    ]
     13
     14    operations = [
     15        migrations.AlterField(
     16            model_name='author',
     17            name='id',
     18            field=models.CharField(max_length=10, primary_key=True),
     19        ),
     20    ]
  • new file tests/migrations/migrations_test_apps/alter_fk/book_app/migrations/0001_initial.py

    diff --git a/tests/migrations/migrations_test_apps/alter_fk/author_app/migrations/__init__.py b/tests/migrations/migrations_test_apps/alter_fk/author_app/migrations/__init__.py
    new file mode 100644
    index 0000000..e69de29
    diff --git a/tests/migrations/migrations_test_apps/alter_fk/book_app/__init__.py b/tests/migrations/migrations_test_apps/alter_fk/book_app/__init__.py
    new file mode 100644
    index 0000000..e69de29
    diff --git a/tests/migrations/migrations_test_apps/alter_fk/book_app/migrations/0001_initial.py b/tests/migrations/migrations_test_apps/alter_fk/book_app/migrations/0001_initial.py
    new file mode 100644
    index 0000000..eb1862d
    - +  
     1# -*- coding: utf-8 -*-
     2from __future__ import unicode_literals
     3
     4from django.db import models, migrations
     5
     6
     7class Migration(migrations.Migration):
     8
     9    dependencies = [
     10        ('author_app', '0001_initial'),
     11    ]
     12
     13    operations = [
     14        migrations.CreateModel(
     15            name='Book',
     16            fields=[
     17                ('id', models.AutoField(serialize=False, auto_created=True, primary_key=True)),
     18                ('title', models.CharField(max_length=50)),
     19                ('author', models.ForeignKey('author_app.Author')),
     20            ],
     21        ),
     22    ]
  • tests/migrations/test_executor.py

    diff --git a/tests/migrations/migrations_test_apps/alter_fk/book_app/migrations/__init__.py b/tests/migrations/migrations_test_apps/alter_fk/book_app/migrations/__init__.py
    new file mode 100644
    index 0000000..e69de29
    diff --git a/tests/migrations/test_executor.py b/tests/migrations/test_executor.py
    index 12385ac..30de553 100644
    a b class ExecutorTests(MigrationTestBase): 
    368368        ]
    369369        self.assertEqual(call_args_list, expected)
    370370
     371    @override_settings(
     372        INSTALLED_APPS=[
     373            "migrations.migrations_test_apps.alter_fk.author_app",
     374            "migrations.migrations_test_apps.alter_fk.book_app",
     375        ]
     376    )
     377    def test_alter_id_type_with_fk(self):
     378        try:
     379            executor = MigrationExecutor(connection)
     380            self.assertTableNotExists("author_app_author")
     381            self.assertTableNotExists("book_app_book")
     382            # Apply initial migrations
     383            executor.migrate([
     384                ("author_app", "0001_initial"),
     385                ("book_app", "0001_initial"),
     386            ])
     387            self.assertTableExists("author_app_author")
     388            self.assertTableExists("book_app_book")
     389            # Rebuild the graph to reflect the new DB state
     390            executor.loader.build_graph()
     391
     392            # Apply PK type alteration
     393            executor.migrate([("author_app", "0002_alter_id")])
     394
     395            # Rebuild the graph to reflect the new DB state
     396            executor.loader.build_graph()
     397        finally:
     398            # Cleanup
     399            executor.migrate([
     400                ("author_app", None),
     401                ("book_app", None),
     402            ])
     403            self.assertTableNotExists("author_app_author")
     404            self.assertTableNotExists("book_app_book")
     405
    371406
    372407class FakeLoader(object):
    373408    def __init__(self, graph, applied):

comment:5 Changed 4 years ago by Tim Graham

Severity: NormalRelease blocker

comment:6 Changed 4 years ago by Markus Holtermann

Owner: changed from nobody to Markus Holtermann
Status: newassigned

comment:7 Changed 4 years ago by Markus Holtermann

Has patch: set
Needs documentation: set
Needs tests: set
Patch needs improvement: set

Based on Claude's initial patch for #24225 I started working on bigger migration issue regarding states. More on the PR https://github.com/django/django/pull/4097

comment:8 Changed 4 years ago by Tim Graham

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

The PR fixes the issue I encountered on djangoproject.com.

comment:9 Changed 4 years ago by Markus Holtermann <info@…>

In cc22b009e05456e3d9cf3c152fe47fa27772be5e:

Refs #24264 -- Added failing test case for updating a FK when changing a PK

When the primary key column is altered, foreign keys of referencing
models must be aware of a possible data type change as well and thus
need to be re-rendered.

Thanks Tim Graham for the report.

comment:10 Changed 4 years ago by Markus Holtermann <info@…>

Resolution: fixed
Status: assignedclosed

In b29f3b51204d53c1c8745966476543d068c173a2:

Fixed #24225, #24264, #24282 -- Rewrote model reloading in migration project state

Instead of naively reloading only directly related models (FK, O2O, M2M
relationship) the project state needs to reload their relations as well
as the model changes as well. Furthermore inheriting models (and super
models) need to be reloaded in order to keep inherited fields in sync.

To prevent endless recursive calls an iterative approach is taken.

comment:11 Changed 4 years ago by Markus Holtermann <info@…>

In 8ca0eb2af70b7fbfd9689553d40ef6023433f94e:

[1.8.x] Refs #24264 -- Added failing test case for updating a FK when changing a PK

When the primary key column is altered, foreign keys of referencing
models must be aware of a possible data type change as well and thus
need to be re-rendered.

Thanks Tim Graham for the report.

Backport of cc22b009e05456e3d9cf3c152fe47fa27772be5e from master

comment:12 Changed 4 years ago by Markus Holtermann <info@…>

In a1ba4627931591b80afa46e38e261f354151d91a:

[1.8.x] Fixed #24225, #24264, #24282 -- Rewrote model reloading in migration project state

Instead of naively reloading only directly related models (FK, O2O, M2M
relationship) the project state needs to reload their relations as well
as the model changes as well. Furthermore inheriting models (and super
models) need to be reloaded in order to keep inherited fields in sync.

To prevent endless recursive calls an iterative approach is taken.

Backport of b29f3b51204d53c1c8745966476543d068c173a2 from master

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