Opened 9 years ago

Closed 6 years 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 8 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:2 by Akshesh Doshi, 8 years ago

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 8 years ago by Akshesh Doshi (previous) (diff)

by Akshesh Doshi, 8 years ago

Attachment: ticket26180-temp-fix.diff added

comment:3 by Akshesh Doshi, 8 years ago

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 by Akshesh Doshi, 8 years ago

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

comment:5 by Tim Graham, 8 years ago

Has patch: unset

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

comment:6 by Akshesh Doshi, 8 years ago

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 by Tim Graham, 8 years ago

I closed #27933 and #28084 as duplicates.

Version 1, edited 8 years ago by Tim Graham (previous) (next) (diff)

comment:8 by Cliff Dyer, 7 years ago

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

comment:9 by Simon Charette, 6 years ago

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