Fixed #23794 - FieldDoesNotExist error in migration with deleted fields and unique_together constraint

[apragacz]

The code in check_dependency is too specific. For instance the any function for operation.unique_together=set([]) always returned False which caused the bug to happen. This PR contains following changes:

• added test case for this bug
• added simplified condition

https://code.djangoproject.com/ticket/23794

[apragacz]

@andrewgodwin should I also send the patch to the Trac?

[charettes]

@szopu you just have to add a comment in the ticket linking to your PR (I just did it)

[charettes]

Buildbot, test this please.

[MarkusH]

I included the any() call to prevent problems with RenameField operations that that involve a field that's in an foo_together. Turns out, the RenameField handles the state changes itself. No need to check for it here.

[apragacz]

As a newbie, I have no idea why the build failed... I ran the tests using the python runtests.py locally and everything was OK (also created virtualenvs for py2, and py3, where I installed the requirements & "editable" django and ran the tests - same result).

[MarkusH]

buildbot, test this please.

[MarkusH]

@szopu I think the errors were related to some errors on the master branch. They aren't related to your commit, I think.

[MarkusH]

Can you use self.assertNumberMigrations(), self.assertOperationTypes() and self.assertOperationAttributes() instead of things like self.assertEqual(first_action.__class__.__name__, "AlterUniqueTogether"), please. Have a look through at the top of the test case on which arguments they expect and how they work. They are also heavily used all over the place in the autodetector tests. (A overall cleanup is in #3564.)

[apragacz]

Sure! But I guess I should wait with it till the PR #3564 with the refactor will be merged into master?

[MarkusH]

Hehe, if you don't mind, sure :) In that case two additional comments:

• Rename the test to test_foo_together_remove_fk
• Use self.book_foo_together instead of self.book_unique

[apragacz]

@MarkusH rebased the code to current master & rewritten the test according to you suggestions.

The only problem right now is that using self.book_foo_together causes the autodectector to spit out additional AlterIndexTogether. I assumed in the test the order of AlterUniqueTogether and AlterIndexTogether which does not need to hold in the future. I could add some branching for specific cases in the test but I'm not sure if this is the right idea.

[MarkusH]

As of now AlterUniqueTogether is always before AlterIndexTogether, although they don't have any inter-dependencies to guarantee that, just by adding the unique operation before the index operation (https://github.com/szopu/django/blob/ticket_23794/django/db/migrations/autodetector.py#L185-L186).

Just check for them as done in https://github.com/szopu/django/blob/ticket_23794/tests/migrations/test_autodetector.py#L895-L897

[MarkusH]

buildbot, test this please.

[MarkusH]

I'd probably keep those lines unwrapped. Maximum line length is at 119 chars.

[apragacz]

I'm accustomed to the PEP8 79 chars line limit (this is really helpful when using multiple columns for instance in Sublime Text). I know that django does not strictly follow this rule. On the other hand, the code is still readable IMO, so I would leave it as it is now.

[MarkusH]

Patch looks good to me, @szopu .

Last but not least, can you add a line to the 1.7.2 release notes (append it at the end). You can find the release notes in docs/releases/1.7.2.txt. This patch should be backported because #23614 was backported too.

[timgraham]

Added release note and merged in 72729f8, thanks!