Re: [Django] #31503: Moving a unique constraint from unique_together to Field.unique generate an invalid migration.
Django
invalid migration.
-------------------------------------+-------------------------------------
Reporter: Xiang Wang | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 3.0
Severity: Normal | Resolution:
Keywords: unique_together | Triage Stage: Accepted
unique migrations |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Xiang Wang):
I think we can consider limit the length of unique_together. If a user set
unique_together containing only one field, we can raise an error and ask
him to use unique=True instead.
--
Ticket URL: <https://code.djangoproject.com/ticket/31503#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
invalid migration.
-------------------------------------+-------------------------------------
| Wobrock
Type: Bug | Status: assigned
Component: Migrations | Version: 3.0
Severity: Normal | Resolution:
Keywords: unique_together | Triage Stage: Accepted
unique migrations |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: David Wobrock (added)
* owner: nobody => David Wobrock
* status: new => assigned
Comment:
Hi!
I looked a bit into the issue. I'll share my thoughts and suggest/discuss
some solutions so that we can agree on the approach. I'd be more than
happy to tackle the issue then, if that is okay, once we know how we want
to fix the issue :D
TL;DR: some suboptimal solutions are presented, but 5/ and 6/ look the
most promising to me. Please let's discuss those a bit at least :)
---------------
So the issue occurs because we are changing an `unique_together` to a
`unique=True` on the field (similarly, I believe the bug occurs with
`index_together` and `db_index`), which will first try to create an
existing index before deleting it.
Some solutions:
1/ Changing the index name
> I think changing the format of index name if the CONSTRAINT is create by
the unique_together will work either.
It would somehow work and mitigate the issue at hand, but it might
introduce complexity for backward compatibility. When upgrading your
Django version and wanting to drop an index, Django will have to know
whether the name of the index comes the previous or current version of
Django, in order to know how the index is called and drop it.
2/ Forbid using `unique_together` (and `index_together`) with a single
field
> If a user set unique_together containing only one field, we can raise an
error and ask him to use unique=True instead
It feels more like a workaround than a real fix of the issue. And more
generally, we will have issues with backward compatibility. We can't break
`unique_together`s with one field from a release to the next. I guess we
would need to add a deprecation warning, and really introduce a breaking
behaviour in the next major release (Django 5.x then?). Which seems IMHO
like a lot of trouble for a rather narrow issue.
3/ Leverage the existing `foo_together_change` dependency mecanism
The autodetector has a similar re-order behaviour so the one we would need
already implemented. When dropping a field, we add a dependency called
`foo_together_change` to the field, which would re-order the
`AlterUniqueTogether` operations, for the index to be dropped before the
removal of the field.
I tried it out for field altering (see code block below), and it would fix
the issue at hand, but it wouldn't fix the reverse operation. If we
changed from a `unique=True` to a `unique_together`, the dependency would
still re-order the `AlterUniqueTogether` operation to happen before the
`AlterField`, but we would need to first drop the index through the
`AlterField`.
So the very same issue occurs, just the other way around.
{{{
diff --git a/django/db/migrations/autodetector.py
b/django/db/migrations/autodetector.py
index 2848adce7d..598d4649e9 100644
--- a/django/db/migrations/autodetector.py
+++ b/django/db/migrations/autodetector.py
@@ -987,7 +987,9 @@ class MigrationAutodetector:
field=field,
preserve_default=preserve_default,
),
- dependencies=dependencies,
+ dependencies=dependencies + [
+ (app_label, model_name, field_name,
"foo_together_change"),
+ ],
)
else:
# We cannot alter between m2m and concrete fields
}}}
4/ Split the index dropping/creation out of the AlterField operation
The bug seems related to the fact that `AlterField` does a lot of things,
and among them is the creation/drop of indexes, which can clash with other
structures.
So one could probably move this part out of `AlterField`, but it feels
like a very heavy and error-prone change to a part that is currently core
to the autodetector and migrations framework.
This idea is a long-shot, and also pretty vague in my head. I wouldn't
actually consider this solution.
5/ Do multi-step AlterFooTogether operations
This solution, and the next one, focus on the idea that index creation
should operate in two steps (in migrations). First, index drops, and then,
after field removal/creation/altering, add indexes.
Today, `AlterUniqueTogether` is one step that will both create and drop
indexes, which is a part of the problem. So would de-couple this, and
first do the `AlterFooTogether` that would drop indexes, then field
changes, and then again `AlterFooTogether` to add indexes.
A small issue is that this operation is declarative with respect to the
expected model state, we just go from one state to the next.
So the idea would be to split the `AlterFooTogether` operation (in two
mostly), so that the migration ends up in the correct state, but with an
intermediate state that, by design, only drops indexes.
An example would be (in pseudo-code):
Initial model
{{{
class MyModel(models.Model):
name = models.CharField(max_length=32)
age = models.IntegerField()
class Meta:
unique_together = ("name", )
}}}
becomes:
{{{
class MyModel(models.Model):
name = models.CharField(max_length=32, unique=True)
age = models.IntegerField()
class Meta:
unique_together = ("age", )
}}}
would do operations like
{{{
operations = [
# Dropping the "name" index
migrations.AlterUniqueTogether(
name='mymodel',
unique_together=set(),
),
# Adding the "name" index
migrations.AlterField(
model_name='mymodel',
name='name',
field=models.CharField(max_length=32, unique=True),
),
# Adding the "age" index
migrations.AlterUniqueTogether(
name='mymodel',
unique_together={("age", )},
),
]
}}}
(one could also imagine `age` being a `unique=True` first, where the
`AlterField` will drop the index)
I believe the amount of work is not that high for this solution, and we
should have no issue with backward compatibility, since we keep the same
logic for the operation itself.
The tricky part is to the generate the intermediate state of foo_together,
that will only drop the related indexes.
An issue I see: we use implicitly the `AlterFooTogether` behaviour to
serve our purpose, and not the purpose it was made for.
6/ Introduce explicit CreateFooTogether and DropFooTogether operations
The cleaner solution to 5/ would be to introduce four new operations,
creating and dropping unique_together and index_together. This would mimic
what is done for constraints and indexes in the autodetector, having two
separate steps.
The issue is that, a) it will introduce a lot of new code and operations.
Even if the code is not complicated, it's still content to document and
maintain. And b) for backward compatibility, we would need to keep
`AlterFooTogether` operations (forever?) the way they are, even if the
operation is not generated by Django anymore.
End note:
From a quick look, 5/ seems like the most realistic approach, but I'd like
to confirm with more experienced Django contributors/maintainers :)
Thanks!
David
--
Ticket URL: <https://code.djangoproject.com/ticket/31503#comment:4>
Django
invalid migration.
-------------------------------------+-------------------------------------
Reporter: Xiang Wang | Owner: David
| Wobrock
Type: Bug | Status: assigned
Component: Migrations | Version: 3.0
Severity: Normal | Resolution:
Keywords: unique_together | Triage Stage: Accepted
unique migrations |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by David Wobrock):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
A first PR https://github.com/django/django/pull/14722 with approach 5/
(from my comment above) basically.
--
Ticket URL: <https://code.djangoproject.com/ticket/31503#comment:5>
Django
invalid migration.
-------------------------------------+-------------------------------------
Reporter: Xiang Wang | Owner: David
| Wobrock
Type: Bug | Status: assigned
Component: Migrations | Version: 3.0
Severity: Normal | Resolution:
Keywords: unique_together | Triage Stage: Accepted
unique migrations |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by David Wobrock):
* needs_better_patch: 1 => 0
Comment:
Removing "patch needs improvement" to get some eyes to look at it :)
--
Ticket URL: <https://code.djangoproject.com/ticket/31503#comment:6>
Django
invalid migration.
-------------------------------------+-------------------------------------
Reporter: Xiang Wang | Owner: David
| Wobrock
Type: Bug | Status: assigned
Component: Migrations | Version: 3.0
Severity: Normal | Resolution:
Keywords: unique_together | Triage Stage: Accepted
unique migrations |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31503#comment:7>
Django
invalid migration.
-------------------------------------+-------------------------------------
Reporter: Xiang Wang | Owner: David
| Wobrock
Type: Bug | Status: assigned
Component: Migrations | Version: 3.0
Severity: Normal | Resolution:
Keywords: unique_together | Triage Stage: Accepted
unique migrations |
Has patch: 1 | Needs documentation: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31503#comment:8>
Django
invalid migration.
-------------------------------------+-------------------------------------
Reporter: Xiang Wang | Owner: David
| Wobrock
Type: Bug | Status: assigned
Component: Migrations | Version: 3.0
Severity: Normal | Resolution:
unique migrations | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31503#comment:9>
Django
invalid migration.
-------------------------------------+-------------------------------------
Reporter: Xiang Wang | Owner: David
| Wobrock
Component: Migrations | Version: 3.0
Severity: Normal | Resolution: fixed
Keywords: unique_together | Triage Stage: Ready for
unique migrations | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"ea00a0843eb7a7bb074625a663ca4f5c86b8c5bd" ea00a084]:
{{{
#!CommitTicketReference repository=""
revision="ea00a0843eb7a7bb074625a663ca4f5c86b8c5bd"
[4.0.x] Fixed #31503 -- Made autodetector remove unique/index_together
before altering fields.
Backport of 0314593fe8e7dc685bbb6585eee40e755588864e from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31503#comment:10>
Django
invalid migration.
-------------------------------------+-------------------------------------
Reporter: Xiang Wang | Owner: David
| Wobrock
Type: Bug | Status: closed
Component: Migrations | Version: 3.0
Severity: Normal | Resolution: fixed
Keywords: unique_together | Triage Stage: Ready for
unique migrations | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"0314593fe8e7dc685bbb6585eee40e755588864e" 0314593f]:
{{{
#!CommitTicketReference repository=""
revision="0314593fe8e7dc685bbb6585eee40e755588864e"
Fixed #31503 -- Made autodetector remove unique/index_together before
altering fields.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31503#comment:11>