Fixed #23916 -- Allowed makemigrations to handle related model name case changes.

[adamchainz]

Ticket

I believe this is the correct fix.

The reason continuous migrations were being generated was the comparison of the before and after deconstructed ForeignKeys in MigrationAutodetector.generate_altered_fields:

if old_field_dec != new_field_dec:

The model name in the migration history is based on the migration that added the model, but nothing ever changes this casing since the autodetector only uses the "model name" (lowercased class name) throughout. ForeignObject.deconstruct() seems to have been a place where this was missed.

[felixxm]

@adamchainz Thanks 👍 Please add a test for scenario described in the ticket.

[adamchainz]

Oh yes ofc

I added the below to the autodetector tests but it already passed before the patch. I think it's because the error wasn't in the autodetector but in the difference between model states / deconstructed fields loaded from migration history and actual models.

    def test_model_case_change(self):
        AuthoR = ModelState('testapp', 'AuthoR', [("id", models.AutoField(primary_key=True))])
        changes = self.get_changes([self.author_empty, self.book], [AuthoR, self.book])
        self.assertNumberMigrations(changes, 'app', 0)

That said, this patch does fix the problem on $clientproject that had the same bug as the ticket.

I will come back to this tomorrow to try add a unit test that does reproduce the scenario.

[adamchainz]

I'm finding it too hard to make a replicating test. Tried this as well but it passes on master.

    def test_rename_model_case(self):
        """
        Tests that a model changing case doesn't lead to any autodetected
        operations.
        """
        author1 = ModelState('testapp', 'author', [("id", models.AutoField(primary_key=True))])
        book1 = ModelState("otherapp", "Book", [
            ("id", models.AutoField(primary_key=True)),
            ("author", models.ForeignKey("testapp.Author", models.CASCADE)),
        ])
        author2 = ModelState('testapp', 'Author', [("id", models.AutoField(primary_key=True))])
        book2 = ModelState("otherapp", "Book", [
            ("id", models.AutoField(primary_key=True)),
            ("author", models.ForeignKey("testapp.Author", models.CASCADE)),
        ])
        changes = self.get_changes([author1, book1], [author2, book2])
        self.assertNumberMigrations(changes, 'app', 0)

🤷‍♂

[felixxm]

I'm finding it too hard to make a replicating test. Tried this as well but it passes on master.

I will play with this.

[felixxm]

@MarkusH Can you take a look? At first glance, I would expect to get two operations: RenameModel and AlterField 🤔 , but with this PR we ignore case changes.

[MarkusH]

Looks good. The only concern I have is what implications this change has on existing migration files. Will the change trigger the generation of new migrations?

[adamchainz]

Verified it won't trigger the generation of new migrations with a test project: https://github.com/adamchainz/django-23916 .

[charettes]

Is the only thing blocking the merge of this change a valid regression test? If it's the case I could likely build one from a simplified scenario.

[felixxm]

@charettes Yes, a regression test is missing.

[adamchainz]

Took another pass. Found a way to fix up my above test to fail on master and pass on this PR. Mostly it was my call to assertNumberMigrations using the wrong app label(s) 🤦‍♂️

[felixxm]

@adamchainz Thanks 👍 I pushed small edits.

[adamchainz]

Great, LGTM