[Django] #35991: Migrations crash on SQLite when renaming part of CompositePrimaryKey.

[Django]

#35991: Migrations crash on SQLite when renaming part of CompositePrimaryKey.
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Type: Bug
Status: new | Component: Database
| layer (models, ORM)
Version: dev | Severity: Release
| blocker
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
I've created a sample project that tries to rename a column included in
`CompositePrimaryKey`. Unfortunately, it crashes:
{{{
$ python manage.py migrate test_one 0002
...
File "/django/db/backends/base/schema.py", line 509, in create_model
sql, params = self.table_sql(model)
^^^^^^^^^^^^^^^^^^^^^
File "/django/db/backends/base/schema.py", line 290, in table_sql
constraint_sqls.append(self._pk_constraint_sql(pk.columns))
^^^^^^^^^^
File "/django/utils/functional.py", line 47, in __get__
res = instance.__dict__[self.name] = self.func(instance)
^^^^^^^^^^^^^^^^^^^
File "/django/db/models/fields/composite.py", line 81, in columns
return tuple(field.column for field in self.fields)
^^^^^^^^^^^
File "/django/utils/functional.py", line 47, in __get__
res = instance.__dict__[self.name] = self.func(instance)
^^^^^^^^^^^^^^^^^^^
File "/django/db/models/fields/composite.py", line 77, in fields
return tuple(meta.get_field(field_name) for field_name in
self.field_names)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/django/db/models/fields/composite.py", line 77, in <genexpr>
return tuple(meta.get_field(field_name) for field_name in
self.field_names)
^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/django/db/models/options.py", line 685, in get_field
raise FieldDoesNotExist(
django.core.exceptions.FieldDoesNotExist: NewRelease has no field named
'name'
}}}

I've attached a sample project. I can prepare a patch in the coming days.
--
Ticket URL: <https://code.djangoproject.com/ticket/35991>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35991: Migrations crash on SQLite when renaming part of CompositePrimaryKey.
-------------------------------------+-------------------------------------

Reporter: Mariusz Felisiak | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:

Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* Attachment "ticket_373_rename_column.zip" added.

[Django]

#35991: Migrations crash on SQLite when renaming part of CompositePrimaryKey.
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak):

I've found other migrations that crashes in exactly the same way on
SQLite.
--
Ticket URL: <https://code.djangoproject.com/ticket/35991#comment:1>

[Django]

#35991: Migrations crash on SQLite when renaming part of CompositePrimaryKey.
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* stage: Unreviewed => Accepted

Comment:

Thanks for the report!

The issue likely lies in `ProjectState.rename_field` and in
`MigrationAutoDetector.create_renamed_fields` which shouldn't generate the
`AlterField` against the composite primary key in the first place.
--
Ticket URL: <https://code.djangoproject.com/ticket/35991#comment:2>

[Django]

#35991: Migrations crash on SQLite when renaming part of CompositePrimaryKey.
-------------------------------------+-------------------------------------

Reporter: Mariusz Felisiak | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned

Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* owner: (none) => Mariusz Felisiak
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/35991#comment:3>

[Django]

#35991: Migrations crash on SQLite when renaming part of CompositePrimaryKey.
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* cc: Csirmaz Bendegúz (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/35991#comment:4>

[Django]

#35991: Migrations crash on SQLite when renaming part of CompositePrimaryKey.
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak):

Replying to [comment:2 Simon Charette]:

> Thanks for the report!
>
> The issue likely lies in `ProjectState.rename_field` and in
`MigrationAutoDetector.create_renamed_fields` which shouldn't generate the
`AlterField` against the composite primary key in the first place.

In my case the second migration has three operations:
{{{
migrations.RenameField(
model_name='release',
old_name='name',
new_name='name2',
),
migrations.AddField(
model_name='release',
name='rename',
field=models.CharField(default='x', max_length=20),
preserve_default=False,
),
migrations.AlterField(
model_name='release',
name='pk',
field=models.CompositePrimaryKey('version', 'rename',
blank=True, editable=False, primary_key=True, serialize=False),
),
}}}

`AddField` crashes like in the ticket description, not `AlterField`, at
least on SQLite where it tries to remake a table. I think we should update
field references for `CompositePrimaryKey` before remaking a table. The
following draft patch fixes this crash for me:

{{{#!diff
diff --git a/django/db/migrations/state.py b/django/db/migrations/state.py
index 6cf675d0e4..9a4549f1f1 100644
--- a/django/db/migrations/state.py
+++ b/django/db/migrations/state.py
@@ -11,6 +11,7 @@ from django.core.exceptions import FieldDoesNotExist
from django.db import models
from django.db.migrations.utils import field_is_referenced,
get_references
from django.db.models import NOT_PROVIDED
+from django.db.models.fields.composite import CompositePrimaryKey
from django.db.models.fields.related import
RECURSIVE_RELATIONSHIP_CONSTANT
from django.db.models.options import DEFAULT_NAMES, normalize_together
from django.db.models.utils import make_model_tuple
@@ -355,6 +356,15 @@ class ProjectState:
field = to_model[model_key].pop(old_name_lower)
field.name = new_name_lower
to_model[model_key][new_name_lower] = field
+ # Fix field references in a composite primary key.
+ for field_name, field in model_state.fields.items():
+ if isinstance(field, CompositePrimaryKey):
+ if old_name in field.field_names:
+ field.field_names = tuple(
+ new_name if sub_field_name == old_name else
sub_field_name
+ for sub_field_name in field.field_names
+ )
+
self.reload_model(*model_key, delay=delay)

def _find_reload_model(self, app_label, model_name, delay=False):

}}}

What do you think?
--
Ticket URL: <https://code.djangoproject.com/ticket/35991#comment:5>

[Django]

#35991: Migrations crash on SQLite when renaming part of CompositePrimaryKey.
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Natalia Bidart):

* cc: Simon Charette (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/35991#comment:6>

[Django]

#35991: Migrations crash on SQLite when renaming part of CompositePrimaryKey.
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* has_patch: 0 => 1

Comment:

[https://github.com/django/django/pull/18960 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/35991#comment:7>

[Django]

#35991: Migrations crash on SQLite when renaming part of CompositePrimaryKey.
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: Mariusz
| Felisiak
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/35991#comment:8>

[Django]

#35991: Migrations crash on SQLite when renaming part of CompositePrimaryKey.
-------------------------------------+-------------------------------------
Reporter: Mariusz Felisiak | Owner: Mariusz
| Felisiak

Type: Bug | Status: closed

Component: Database layer | Version: dev
(models, ORM) |

Severity: Release blocker | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by GitHub <noreply@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"c534b6c4938cd817e5d7903dce15a689a4862a99" c534b6c]:
{{{#!CommitTicketReference repository=""
revision="c534b6c4938cd817e5d7903dce15a689a4862a99"
Fixed #35991 -- Fixed crash when adding non-nullable field after renaming
part of CompositePrimaryKey on SQLite.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35991#comment:9>