Fixed #27489 -- Renamed permissions upon model renaming. #19405

artirix1927 · 2025-04-20T22:38:55Z

Refs#24067
By looking at a code that was used to fix Refs#24067 I added an update permissions function that gets called pre migrate using signals. It looks for rename model operations inside the migrate plan and adds new custom operation to the plan that will change the permission codename and name.

Added regression test that creates permission for fake model, creates fake migration for the model rename and calls the update permission function using signals and manually does 'state_forward' to apply.

Then check for the changes

Checklist

This PR targets the main branch.
The commit message is written in past tense, mentions the ticket number, and ends with a period.
I have checked the "Has patch" ticket flag in the Trac system.
I have added or updated relevant tests.
I have added or updated relevant docs, including release notes if applicable.
I have attached screenshots in both light and dark modes for any UI changes.

sarahboyce

Thank you for the PR ⭐
I have checked the test coverage and we still a few more tests here 👍

sarahboyce · 2025-05-14T14:24:05Z

django/contrib/auth/management/__init__.py

+        if not router.allow_migrate_model(db, Permission):
+            return


Missing tests

Sorry this is still missing a test 👍

Just to clarify — for the missing test, do you mean I should assert that the RenamePermission operation is not in the migration plan when the router disallows it? I want to make sure I understand what you expecting from the test. Sorry for being a little bit slow

I think in this case, the RenamePermission is in the plan but router.allow_migrate_model(db, Permission) returns False and so no permission renames, or queries, should take place

Do you think it would be appropriate to call the RenamePermission manually in this specific case? Because I cant find a way to write the test so the router allows migration at the stage of building the migration plan (so it can actually insert the rename operation) but disallows it in the runtime (when the actual rename happens).

op = RenamePermission( app_label="auth_tests", old_model="OldModel", new_model="NewModel", ) with connection.schema_editor() as schema_editor: op.rename_forward(apps, schema_editor)

Perhaps we just remove it 🤔
We have copied this because it exists in django.contrib.contenttypes.management.RenameContentType._rename but this is also not covered by tests and may not be needed 🤔

django/contrib/auth/management/__init__.py

sarahboyce · 2025-06-13T13:21:51Z

tests/auth_tests/test_management.py

+    def tearDown(self):
+        pre_migrate.disconnect(update_permissions)
+
+    def test_fake_migration_plan(self):


Thank you for the updates!

I just realized we might want the testing to be closer to what was done in f179113 where we call migrate and migrate back to zero to check everything was undone. I think this is cleaner than having some of the "setup" and "clean state" that exists in some of the tests. Would you mind taking a look?

sarahboyce

Thank you for the updates! We're almost there, have a few final questions

sarahboyce · 2025-07-18T10:19:15Z

django/contrib/auth/management/__init__.py

+
+    def _rename(self, apps, schema_editor, old_model, new_model):
+        ContentType = apps.get_model("contenttypes", "ContentType")
+        from django.contrib.auth.models import Permission  # real live model import


Can you explain why this shouldn't be Permission = apps.get_model("auth", "Permission")?
I see there are some test failures but the direct import feels wrong

Okay, so as I understand, the tests don't pass because the apps.get_model() function returns frozen (__fake__) model. Which does not have foreign key relationships yet and does not have content_type field at all. I tried to fix this by avoiding the foreign keys and content types to find the needed permissions, but the permission class has its own ordering that involves contenttypes so this does not work too. I can call .order_by() inside the operation, which will clear the ordering and wont raise errors and will allow me to query for permissions by codename strings and etc, while avoiding contettypes. Other than that, the he only way I could do this is by using raw sql inside the migration operation which seems even worse than using live model.

I think this is the best option we have 👍

sarahboyce · 2025-07-18T10:20:32Z

tests/auth_tests/test_management.py

+            interactive=False,
+            verbosity=0,
+        )
+


Suggested change

self.assertFalse(Permission.objects.filter(codename="change_newmodel").exists())

When I add the following assertion to check this has reversed, this fails. Can you look into this?

I looked into this and to be honest that test is not needed and it didnt pass the test because when creating a contenttype for this test I put the "model=" with camel case which wont ever happen in real app since django manages the names by itself and lowers all the names of the models to lower case automatically.

sarahboyce · 2025-07-21T08:44:38Z

django/contrib/auth/management/__init__.py

+
+    def _rename(self, apps, schema_editor, old_model, new_model):
+        ContentType = apps.get_model("contenttypes", "ContentType")
+        from django.contrib.auth.models import Permission  # real live model import


I think this is the best option we have 👍

sarahboyce · 2025-07-21T08:45:53Z

django/contrib/auth/management/__init__.py

+        for permission in permissions:
+            prefix = permission.codename.split("_")[0]
+            new_codename = f"{prefix}_{new_model}"
+            new_name = f"Can {prefix} {new_model}"


Suggested change

new_name = f"Can {prefix} {new_model}"

default_verbose_name = camel_case_to_spaces(new_model)

new_name = f"Can {prefix} {default_verbose_name}"

I think the Permission name uses the model verbose_name which I'm not sure we can access but we can probably do a better job of building. This will need some test updates 👍

felixxm · 2025-07-21T09:07:17Z

django/contrib/auth/management/__init__.py

+        self._rename(apps, schema_editor, self.new_model, self.old_model)
+
+
+def inject_permission_rename_operations(


Why not to keep the same naming pattern? inject_*_operations sounds unnecessary

Suggested change

def inject_permission_rename_operations(

def rename_permissions(

django/contrib/auth/management/__init__.py

sarahboyce · 2025-07-30T14:30:07Z

Thank you for the updates, I pushed minor stylistic tweaks and updated the commit message
The tests currently fail when ran in reverse (./runtests.py auth_tests.test_management.PermissionRenameOperationsTests --reverse), can you take a look?

sarahboyce

Thank you, pushed minor changes

sarahboyce · 2025-08-04T15:51:43Z

django/contrib/auth/management/__init__.py

+        if not router.allow_migrate_model(db, Permission):
+            return


Sorry this is still missing a test 👍

artirix1927 force-pushed the ticket_27489 branch 2 times, most recently from f78fc46 to 08f9cb0 Compare April 20, 2025 22:48

sarahboyce reviewed May 14, 2025

View reviewed changes

artirix1927 force-pushed the ticket_27489 branch 3 times, most recently from 4595197 to 968975d Compare May 31, 2025 04:59

sarahboyce reviewed Jun 13, 2025

View reviewed changes

artirix1927 force-pushed the ticket_27489 branch 2 times, most recently from 59ec5dc to 453b00b Compare June 30, 2025 05:55

sarahboyce force-pushed the ticket_27489 branch from 453b00b to 48bbc16 Compare July 18, 2025 10:17

sarahboyce changed the title ~~Fixed #27489 permission update on model rename~~ Fixed #24067 -- Renamed permissions upon model renaming. Jul 18, 2025

sarahboyce reviewed Jul 18, 2025

View reviewed changes

sarahboyce reviewed Jul 21, 2025

View reviewed changes

felixxm reviewed Jul 21, 2025

View reviewed changes

django/contrib/auth/management/__init__.py Outdated Show resolved Hide resolved

artirix1927 force-pushed the ticket_27489 branch 3 times, most recently from 4750ec9 to 40e3fb1 Compare July 23, 2025 06:53

sarahboyce force-pushed the ticket_27489 branch from 40e3fb1 to 4c5dc30 Compare July 30, 2025 14:28

artirix1927 changed the title ~~Fixed #24067 -- Renamed permissions upon model renaming.~~ Fixed #27489 -- Renamed permissions upon model renaming. Jul 31, 2025

artirix1927 force-pushed the ticket_27489 branch 2 times, most recently from de634f6 to 4335fd6 Compare August 1, 2025 04:39

sarahboyce force-pushed the ticket_27489 branch 3 times, most recently from 795aaa6 to d2fc87e Compare August 4, 2025 15:55

sarahboyce reviewed Aug 4, 2025

View reviewed changes

artirix1927 force-pushed the ticket_27489 branch 2 times, most recently from e534fb1 to 7ac68eb Compare August 10, 2025 07:18

Fixed #27489 -- Renamed permissions upon model renaming in migrations.

7530d68

artirix1927 force-pushed the ticket_27489 branch from 7ac68eb to 7530d68 Compare August 10, 2025 07:40


	self.assertFalse(Permission.objects.filter(codename="change_newmodel").exists())

	new_name = f"Can {prefix} {new_model}"
	default_verbose_name = camel_case_to_spaces(new_model)
	new_name = f"Can {prefix} {default_verbose_name}"

		self._rename(apps, schema_editor, self.new_model, self.old_model)


		def inject_permission_rename_operations(

	def inject_permission_rename_operations(
	def rename_permissions(

Uh oh!

Fixed #27489 -- Renamed permissions upon model renaming. #19405

Are you sure you want to change the base?

Fixed #27489 -- Renamed permissions upon model renaming. #19405

Conversation

artirix1927 commented Apr 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

sarahboyce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarahboyce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artirix1927 Jul 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sarahboyce commented Jul 30, 2025

Uh oh!

sarahboyce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

artirix1927 commented Apr 20, 2025 •

edited

Loading

artirix1927 Jul 20, 2025 •

edited

Loading