Re: [Django] #31503: Moving a unique constraint from unique_together to Field.unique generate an invalid migration.

[Django]

#31503: Moving a unique constraint from unique_together to Field.unique 
generate an
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 ):

 In [changeset:"0314593fe8e7dc685bbb6585eee40e755588864e" 0314593f]:
 {{{
 #!CommitTicketReference repository=""
 revision="0314593fe8e7dc685bbb6585eee40e755588864e"
 Fixed #31503 -- Made autodetector remove unique/index_together before
 altering fields.
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.196776d5f38a0bf1f68a4b97fe0cd6aa%40djangoproject.com.

Re: [Django] #31503: Moving a unique constraint from unique_together to Field.unique generate an invalid migration.

[Django]

#31503: Moving a unique constraint from unique_together to Field.unique 
generate an
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
-+-
Changes (by Mariusz Felisiak ):

 * 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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.0f6b02aa10d2cd7635ccf347de96457b%40djangoproject.com.

Re: [Django] #31503: Moving a unique constraint from unique_together to Field.unique generate an invalid migration.

[Django]

#31503: Moving a unique constraint from unique_together to Field.unique 
generate an
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:  Ready for
  unique migrations  |  checkin
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Mariusz Felisiak):

 * stage:  Accepted => Ready for checkin


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.aa67e02e210a3b23a454b54e581d8102%40djangoproject.com.

Re: [Django] #31503: Moving a unique constraint from unique_together to Field.unique generate an invalid migration.

[Django]

#31503: Moving a unique constraint from unique_together to Field.unique 
generate an
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 tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by David Wobrock):

 * needs_better_patch:  1 => 0
 * needs_tests:  1 => 0


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.0cb5c0219e572394efa7bb103771004e%40djangoproject.com.

Re: [Django] #31503: Moving a unique constraint from unique_together to Field.unique generate an invalid migration.

[Django]

#31503: Moving a unique constraint from unique_together to Field.unique 
generate an
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 tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Mariusz Felisiak):

 * needs_better_patch:  0 => 1
 * needs_tests:  0 => 1


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.6fef689e366846867d2b83008b4e5f75%40djangoproject.com.

Re: [Django] #31503: Moving a unique constraint from unique_together to Field.unique generate an invalid migration.

[Django]

#31503: Moving a unique constraint from unique_together to Field.unique 
generate an
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 tests:  0|  Patch needs improvement:  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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.afbe9e2efca07652908d88d3206d1c99%40djangoproject.com.

Re: [Django] #31503: Moving a unique constraint from unique_together to Field.unique generate an invalid migration.

[Django]

#31503: Moving a unique constraint from unique_together to Field.unique 
generate an
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 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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.9c08e4e9a15be84639611195f506469a%40djangoproject.com.

Re: [Django] #31503: Moving a unique constraint from unique_together to Field.unique generate an invalid migration.

[Django]

#31503: Moving a unique constraint from unique_together to Field.unique 
generate an
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:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by David Wobrock):

 * 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, an

Re: [Django] #31503: Moving a unique constraint from unique_together to Field.unique generate an invalid migration.

[Django]

#31503: Moving a unique constraint from unique_together to Field.unique 
generate an
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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.3ff4313bc30a1e3371572048edb275cb%40djangoproject.com.

Re: [Django] #31503: Moving a unique constraint from unique_together to Field.unique generate an invalid migration. (was: I met a problem when I convert a unique_together to the unique=True attribute.

[Django]

#31503: Moving a unique constraint from unique_together to Field.unique 
generate an
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
-+-
Changes (by Simon Charette):

 * stage:  Unreviewed => Accepted


Comment:

 Thank your for your report.

 I guess this is a bug in the auto-detector where the `AlterUniqueTogether`
 should appear before the `AlterField` that adds the `Field.unique=True`. I
 assume this is the case because `generate_altered_unique_together` is run
 after `generate_altered_fields` and the former doesn't add any
 dependencies on ensure operations are properly re-ordered.

 Not sure it's worth adjusting `AddConstraint` ordering as well since the
 chance of adding a `UniqueConstraint` with a colliding `.name` are really
 slim.

 Xiang, can you confirm that re-ordering your operations so that
 `AlterUniqueTogether` is performed before `AlterField` addresses your
 issue?

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.1379d4741abf6e7ca5cf94761ad9%40djangoproject.com.