Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#33932 closed Bug (fixed)

Altering AutoFields with renaming a column crashes on PostgreSQL.

Reported by: Benoît Vinot Owned by: Benoît Vinot
Component: Migrations Version: 4.1
Severity: Release blocker Keywords:
Cc: Florian Apolloner 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

Hi, here is my problem. I have an independent table with a id primary key. I would like this table to inherit from a new parent table. I have generated migrations which work with Django version 4.0.7 but fail with Django 4.1.

I have renamed the field id into base_ptr (Base is the new parent table) and then, in another migration file, I try to make this column to be a 1 to 1 column. To do that, I have in my migration an operation like this

        migrations.AlterField(
            model_name="son",
            name="base_ptr",
            field=models.OneToOneField(
                auto_created=True,
                on_delete=django.db.models.deletion.CASCADE,
                parent_link=True,
                primary_key=True,
                serialize=False,
                to="app.Base",
            ),
        ),

which leads to this SQL code

BEGIN;
--
-- Alter field base_ptr on son
--
ALTER TABLE "app_son" RENAME COLUMN "base_ptr" TO "base_ptr_id";
ALTER TABLE "app_son" ALTER COLUMN "base_ptr" DROP IDENTITY IF EXISTS;
ALTER TABLE "app_son" ALTER COLUMN "base_ptr_id" TYPE integer;
DROP SEQUENCE IF EXISTS "app_son_base_ptr_id_seq" CASCADE;
ALTER TABLE "app_son" ADD CONSTRAINT "app_son_base_ptr_id_d3673d48_fk_app_base" FOREIGN KEY ("base_ptr_id") REFERENCES "app_base" ("id") DEFERRABLE INITIALLY DEFERRED;
--

This SQL code fails because the second line (DROP IDENTITY EXISTS) references the base_ptr column which has been renamed into base_ptr_id by the first line...

Attachments (3)

0001_initial.py (733 bytes ) - added by Benoît Vinot 2 years ago.
0002_rename_id_son_base_ptr.py (342 bytes ) - added by Benoît Vinot 2 years ago.
0003_alter_son_base_ptr.py (650 bytes ) - added by Benoît Vinot 2 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 by Mariusz Felisiak, 2 years ago

Can you provide a sample project to reproduce? This transition is quite tricky.

comment:2 by Mariusz Felisiak, 2 years ago

Cc: Florian Apolloner added
Severity: NormalRelease blocker
Summary: Error in migration with version 4.1Altering AutoFields with renaming a column crashes on PostgreSQL.
Triage Stage: UnreviewedAccepted

Regression in 2eea361eff58dd98c409c5227064b901f41bd0d6. We should use the new column name when dropping an identity:

  • django/db/backends/postgresql/schema.py

    diff --git a/django/db/backends/postgresql/schema.py b/django/db/backends/postgresql/schema.py
    index 633146a80b..cfc30516e6 100644
    a b class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):  
    175175                self.sql_drop_indentity
    176176                % {
    177177                    "table": self.quote_name(table),
    178                     "column": self.quote_name(strip_quotes(old_field.column)),
     178                    "column": self.quote_name(strip_quotes(new_field.column)),
    179179                }
    180180            )
    181181            column = strip_quotes(new_field.column)

Benoît, would you like to prepare a patch? (tests can be tricky.)

by Benoît Vinot, 2 years ago

Attachment: 0001_initial.py added

by Benoît Vinot, 2 years ago

by Benoît Vinot, 2 years ago

Attachment: 0003_alter_son_base_ptr.py added

comment:3 by Benoît Vinot, 2 years ago

I have attached the three migrations files of the small example but you were too fast for me...

It would be my first contribution...

Version 0, edited 2 years ago by Benoît Vinot (next)

comment:4 by Mariusz Felisiak, 2 years ago

Can you confirm the diff works for you?

comment:5 by Benoît Vinot, 2 years ago

I confirm it works

comment:6 by Mariusz Felisiak, 2 years ago

Great!, I created a regression test inspired by your migrations:

  • tests/schema/tests.py

    diff --git a/tests/schema/tests.py b/tests/schema/tests.py
    index 9e2385f5d8..c5beb18304 100644
    a b class SchemaTests(TransactionTestCase):  
    15881588        # OneToOneField.
    15891589        self.assertEqual(counts, {"fks": expected_fks, "uniques": 1, "indexes": 0})
    15901590
     1591    def test_autofield_to_o2o(self):
     1592        with connection.schema_editor() as editor:
     1593            editor.create_model(Author)
     1594            editor.create_model(Note)
     1595
     1596        # Rename the field.
     1597        old_field = Author._meta.get_field("id")
     1598        new_field = AutoField(primary_key=True)
     1599        new_field.set_attributes_from_name("note_ptr")
     1600        with connection.schema_editor() as editor:
     1601            editor.alter_field(Author, old_field, new_field, strict=True)
     1602        # Alter AutoField to OneToOneField.
     1603        new_field_o2o = OneToOneField(Note, CASCADE)
     1604        new_field_o2o.set_attributes_from_name("note_ptr")
     1605        with connection.schema_editor() as editor:
     1606            editor.alter_field(Author, new_field, new_field_o2o, strict=True)
     1607        columns = self.column_classes(Author)
     1608        field_type, _ = columns["note_ptr_id"]
     1609        self.assertEqual(
     1610            field_type, connection.features.introspected_field_types["IntegerField"]
     1611        )
     1612
    15911613    def test_alter_field_fk_keeps_index(self):
    15921614        with connection.schema_editor() as editor:
    15931615            editor.create_model(Author)
Last edited 2 years ago by Mariusz Felisiak (previous) (diff)

in reply to:  3 comment:7 by Mariusz Felisiak, 2 years ago

Replying to Benoît Vinot Roseau Technologies:

It would be my first contribution... I can start a PR on GitHub

Thanks!

comment:8 by Mariusz Felisiak, 2 years ago

Owner: changed from nobody to Benoît Vinot
Status: newassigned

comment:9 by Benoît Vinot, 2 years ago

I have created a PR here with your test and your solution. https://github.com/django/django/pull/15964

comment:10 by Benoît Vinot, 2 years ago

Has patch: set

comment:11 by Mariusz Felisiak, 2 years ago

Triage Stage: AcceptedReady for checkin

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In e3cb8bc:

Fixed #33932 -- Fixed altering AutoFields to OneToOneField on PostgreSQL.

Regression in 2eea361eff58dd98c409c5227064b901f41bd0d6.

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In 85942cf6:

[4.1.x] Fixed #33932 -- Fixed altering AutoFields to OneToOneField on PostgreSQL.

Regression in 2eea361eff58dd98c409c5227064b901f41bd0d6.

Backport of e3cb8bcb7d2a2d392e726ee1f7e32a8d9038e14c from main

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