Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #33563 -- Fixed contenttype reverse data migration crash with a multiple databases setup. #15474

Merged
merged 2 commits into from Mar 8, 2022

Conversation

giff-h
Copy link
Contributor

@giff-h giff-h commented Mar 3, 2022

The ContentType legacy name field population operation would run on the
"default" database regardless of any specified alias


Found this while going through our code to ensure that it would smoothly run database restore operations on a given alias. It didn't cause any issues, I only found it by grepping our installed packages for migration operations to see if I should open any fix PRs.

@github-actions
Copy link

github-actions bot commented Mar 3, 2022

Hello @hamstap85! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

@goophps

This comment was marked as off-topic.

@jacobtylerwalls

This comment was marked as off-topic.

@giff-h giff-h changed the title Allowed migration to run on any database Fixed #33563 -- Allowed migration to run on any database Mar 6, 2022
@giff-h giff-h force-pushed the set_legacy_contenttype_name_on_any_db branch from b4f78e2 to 4a636cd Compare March 6, 2022 07:04
@giff-h
Copy link
Contributor Author

giff-h commented Mar 6, 2022

I have ensured that the test passes in both parallel and non-parallel mode without corrupting the rest of the suite, as well as fails on the unfixed migration code in both parallel and non-parallel mode without corrupting the rest of the suite

@felixxm felixxm force-pushed the set_legacy_contenttype_name_on_any_db branch from 4a636cd to 405db5b Compare March 7, 2022 12:08
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hamstap85 Thanks for the patch 👍 Welcome aboard ⛵

I added an alternative regression test to avoid migrate calls and possible isolation issues. What do you think?

@felixxm felixxm changed the title Fixed #33563 -- Allowed migration to run on any database Fixed #33563 -- Fixed contenttype reverse data migration crash with a multiple databases setup. Mar 7, 2022
@giff-h
Copy link
Contributor Author

giff-h commented Mar 7, 2022

@felixxm oh I like that, very nice.

I had seen how tests/runtests.py sets the following

settings.MIGRATION_MODULES = {
    ...
    "contenttypes": None,
    ...
}

So I spent most of my time trying to fight with it to get the actual migration operation to run, but I hadn't considered importing the object by string lookup. I'm still not completely familiar with the idea of importing a module that isn't a top-level import keyword though I understand how it works.

So with self.assertRaises(AttributeError): would fail without the fix because it would attempt to set on the default db that time, and with no ContentType objects the loop would run 0 times.

@felixxm
Copy link
Member

felixxm commented Mar 7, 2022

So with self.assertRaises(AttributeError): would fail without the fix because it would attempt to set on the default db that time, and with no ContentType objects the loop would run 0 times.

Exactly 👍

@felixxm felixxm force-pushed the set_legacy_contenttype_name_on_any_db branch from 405db5b to 316c68a Compare March 7, 2022 20:04
Copy link
Member

@charettes charettes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The testing approach is clever but I wish we had better testing facilities for migrations in general. It was proposed a few times on the developer mailing list but this is a good example of why a testing utility that would allow for tests to be executed in a context where some migrations are applied and other aren't on different databases would be useful.

This holds the fix in line with the work at this PR:
django#15328
@felixxm felixxm merged commit 58d357f into django:main Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants