Opened 3 years ago

Closed 3 months ago

#26180 closed Bug (duplicate)

Altering unique_together still sometimes missing deleted fields

Reported by: Julian Andrews Owned by: Akshesh Doshi
Component: Migrations Version: 1.9
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This is likely a missed edge case from #23614.

Steps to reproduce:

  1. Create an app spam
  2. Create a model Spam
    class Spam(models.Model):
        a = models.CharField(max_length=255)
        b = models.CharField(max_length=255)
        c = models.CharField(max_length=255)
    
        class Meta:
            unique_together = (
                ('a', 'b'),
                ('b', 'c'),
            )
    
  3. Make migrations
  4. Delete field Spam.c
  5. Delete the second unique_together constraint (('b', 'c'))
  6. Make migrations
  7. Run migrations

Output:

django.core.exceptions.FieldDoesNotExist: Spam has no field named u'c'

Note that the bug doesn't occur if no unique togther constraints remain.

Manually reversing the order of the operations in the migration (to remove the index first) produces a functional migration.

Attachments (1)

ticket26180-temp-fix.diff (2.2 KB) - added by Akshesh Doshi 2 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 3 years ago by Tim Graham

Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:2 Changed 2 years ago by Akshesh Doshi

This is happening because the RemoveField operation is called before calling the AlterUniqueTogether operation, resulting in the dropping of the field before the index.

In the autodetector.py, although the RemoveField operations are created before the AlterUniqueTogether operations but the `_sort_migrations` method corrects the order of these operations to bring AlterUniqueTogether before RemoveField. But the `_optimize_migrations` method again reverses this order in self.migrations (which is finally what is returned by the _detect_changes method) with no optimization as such.

This change is somewhat related to https://github.com/django/django/commit/e470f311d654267ec86f9a6325ec500345b9dff2

Last edited 2 years ago by Akshesh Doshi (previous) (diff)

Changed 2 years ago by Akshesh Doshi

Attachment: ticket26180-temp-fix.diff added

comment:3 Changed 2 years ago by Akshesh Doshi

The patch temporarily fixes the issue (attaching for anyone who gets stuck due to this bug) but some other edge cases would still get left. The actual fix would involve some changes in migrations.optimizer or the reduce methods of AlterUniqueTogether or RemoveField operations AFAICS.

comment:4 Changed 2 years ago by Akshesh Doshi

Has patch: set
Owner: changed from nobody to Akshesh Doshi
Status: newassigned

comment:5 Changed 2 years ago by Tim Graham

Has patch: unset

Unchecking "Has patch" due to "not the right approach" comment on the PR.

comment:6 Changed 2 years ago by Akshesh Doshi

Has patch: set
Needs documentation: set
Needs tests: set

An initial implementation of the suggested approach - https://github.com/django/django/pull/7038

comment:7 Changed 20 months ago by Tim Graham

I closed #27933, #28084, and #28366 as duplicates.

Last edited 16 months ago by Tim Graham (previous) (diff)

comment:8 Changed 13 months ago by Cliff Dyer

This issue just bit me on django 1.11.4, and required a couple hours of debugging to sort out.

comment:9 Changed 3 months ago by Simon Charette

Resolution: duplicate
Status: assignedclosed

Pretty sure this is a duplicate of #28862 as well. I'll close this one as since the other one has more context.

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