Fixed #31335 -- Fixed removing composed Meta constraints/indexes on foreign keys on MySQL.

[GeyseR]

ticket-31335

[felixxm]

@GeyseR Thanks for this patch 👍 I left initial comments, please also check the comments from the previous PR.

[felixxm]

Can we add test for renaming a ForeignKey included in the composed index?

[GeyseR]

Should we test that actually? As stated in the ticket, for the field renaming Django adds 3 operations to the migration file: remove an index, rename the field, add the index (back). I believe it should be at least a separate ticket, that changes behavior to one operation instead of three...

[GeyseR]

hi @felixxm, I've added two commits to the PR.
The first one (dc79f87) addresses your comments in this PR.
With the second one (3b02904) I try to apply a smarter logic for checking already existing various types of indexes, do they cover the FK index or not. In this commit, I also moved tests to tests/migrations as it is easier to apply migration operations with both state and DB updates instead of emulating the same behavior in the schema editor tests. For sure, these changes add complexity to the solution, but it behaves in a more optimized manner.

[GeyseR]

hey @felixxm,
could you please help me with debugging failed MySQL CI tests?
they successfully passed on my local MySQL DB v8 and with official django-docker-box docker image.
Here is the output from the latter:

$ export MYSQL_VERSION=8 # also works for 5.7
$ export DJANGO_PATH=<local django path>
$ export PYTHON_VERSION=3.10
% docker-compose run --rm mysql migrations
Creating django-docker-box_mysql_run ... done
wait-for-it.sh: waiting 20 seconds for mysql-db:3306
wait-for-it.sh: mysql-db:3306 is available after 0 seconds
Testing against Django installed in '/tests/django/django' with up to 6 processes
Found 630 test(s).
.....
Ran 630 tests in 22.102s

OK (skipped=3)

Do I use the correct MySQL version that matches the version on the CI server? Do you see any other reasons for the test's failure?

[felixxm]

Do I use the correct MySQL version that matches the version on the CI server? Do you see any other reasons for the test's failure?

CI uses MySQL 5.7 (see the wiki page). This can be an issue with caching docker images, try to remove images before switching to MySQL 5.7, e.g.

$ docker-compose stop
$ docker-compose rm
$ export MYSQL_VERSION=5.7
$ docker-compose run --rm mysql migrations

[GeyseR]

thanks @felixxm, that helped. At least now tests fail locally :)

[GeyseR]

I think the PR is ready to review. The single failed test looks like an unstable one and is not related to the proposed changes.

[GeyseR]

@felixxm did you have a chance to review the updated PR

[jacobtylerwalls]

Hi @GeyseR, if you've accounted for all feedback to date you should uncheck "Needs tests" and "Patch needs improvement" on the ticket so that there's an indication to re-review it. Rebasing from main would help also. Thanks.

[GeyseR]

@jacobtylerwalls thanks for the heads up, done

[felixxm]

@GeyseR Thanks for updates 👍

[GeyseR]

hey @felixxm, I tried to simplify the tests code, how does it look now?

regarding the parts that you've mentioned as covered by other tests, you are partially right - deletion of unique/index_together was covered but by other tests from the schema/tests.py file (the deletion was partially fixed a long time ago). You can find those tests as removed in my commits, I thought that it might be a good idea to keep all the tests cases in one place

[felixxm]

hey @felixxm, I tried to simplify the tests code, how does it look now?

Many thanks 🚀 I have a week off, so I'm going to review it after the holidays (June, 13th+).

[GeyseR]

no problem, anyway I need to fix the failed tests :)

[David-Wobrock]

Nice 👍
I have a few remarks here and there, but mostly questions to confirm my understanding :)

(I didn't have time to look in details at the unit tests yet)

[GeyseR]

hi @David-Wobrock, thanks for the review. After some thinking I decided to change how I check for duplicate FK index: instead of checking model's definition I check the DB table structure. It looks like is much more straightforward way and also more reliable as we can find some unmanaged manual indexes that could be enough for index deletion.

After that we don't need to extract get_first_index_field_name as a helper function anymore :)

[David-Wobrock]

In a more subjective matter, I find the unit tests with their nested functions a bit hard to read. But since I don't have anything better to suggest at the moment, that's fine to me :)

[GeyseR]

@felixxm the PR is ready to review 🙄
Should I squash all commits in one?

[felixxm]

Thanks 👍

@felixxm the PR is ready to review roll_eyes Should I squash all commits in one?

No need, I can squash with final edits.

[felixxm]

@GeyseR Thanks for updated 👍 We're getting closer.

[GeyseR]

@felixxm
regarding this one (GH, for some reason, doesn't allow replying in the discussion thread)

This is an issue in schema so I'd keep these tests in schema/tests.py and also add new tests here

these two tests are now covered by test_composed_indexes_create_implicit_fk_index_when_deleting. Why would we need to keep redundant tests and spend time (and electricity :)) for running them in each build?

I've tried to move tests from operations tests to the schema tests, and this movement also requires adding utility code (apply_operations in particular) to the SchemaTests class. This looks like too much additional knowledge for this class: it should become aware of migrations, the project's state, etc. To avoid that, we need to operate on more low-level schema-related code (which is how I made the first version of the patch, and it looked ugly and confusing)

[felixxm]

these two tests are now covered by test_composed_indexes_create_implicit_fk_index_when_deleting. Why would we need to keep redundant tests and spend time (and electricity :)) for running them in each build?

I was asking for keeping these two and moving new one to schema/tests.py, not about creating redundant tests.

I've tried to move tests from operations tests to the schema tests, and this movement also requires adding utility code (apply_operations in particular) to the SchemaTests class. This looks like too much additional knowledge for this class: it should become aware of migrations, the project's state, etc. To avoid that, we need to operate on more low-level schema-related code (which is how I made the first version of the patch, and it looked ugly and confusing)

I'm not sure why apply_operations hook is needed and what makes the new tests "ugly" and "confusing" 🤔 This bug is related with a specific model that we can create in schema tests and then perform remove_index()/remove_constraint() on it to see if it works. Am I missing something? For example:

    def test_meta_unique_constraint_with_fk(self):
        with connection.schema_editor() as editor:
            editor.create_model(Author)
            editor.create_model(Book)
        constraint = UniqueConstraint(
            fields=["author", "title"], name="book_author_title_uniq"
        )   
        Book._meta.constraints = [constraint]
        with connection.schema_editor() as editor: 
            editor.add_constraint(Book, constraint)
        constraints = self.get_constraints(Book._meta.db_table)
        self.assertIn(constraint.name, constraints)
        Book._meta.constraints = []
        with connection.schema_editor() as editor:
            editor.remove_constraint(Book, constraint)
        constraints = self.get_constraints(Book._meta.db_table)
        self.assertNotIn(constraint.name, constraints)

    def test_meta_index_with_fk(self):
        with connection.schema_editor() as editor:
            editor.create_model(Author)
            editor.create_model(Book)
        index = Index(
            fields=["author", "title"], name="book_author_title_idx"
        )
        Book._meta.indexes = [index]
        with connection.schema_editor() as editor:
            editor.add_index(Book, index)
        indexes = self.get_indexes(Book._meta.db_table)
        self.assertNotIn("author_id", indexes)
        Book._meta.indexes = []
        with connection.schema_editor() as editor:
            editor.remove_index(Book, index)
        indexes = self.get_indexes(Book._meta.db_table)
        self.assertIn("author_id", indexes)

are proper regression tests for removing Index() and UniqueConstraint() with a foreign key.

[GeyseR]

I was asking for keeping these two and moving new one to schema/tests.py, not about creating redundant tests.

I mean that the new tests cover the cases from these two tests, so they will be redundant as they test the same thing.

Let me make one more attempt with schema tests, I'll try to return soon :)

[GeyseR]

hey @felixxm, I've pushed another attempt to add tests to tests/schema instead of tests/migrations
I should take my words back about code ugliness, this time it looks good to me :)

[felixxm]

I rebased after ec13e80 and squashed commits.

[felixxm]

@GeyseR Thanks 👍 It's ready 🚀

[GeyseR]

@felixxm thanks for you patience with reviewing my code 😅
the last updates look good to me 👍