From c56b680b7c470dbd88dc53579d4cbd91c052c206 Mon Sep 17 00:00:00 2001 From: Bendeguz Csirmaz Date: Sun, 7 Apr 2024 10:32:16 +0800 Subject: [PATCH] Fixed #35953 -- Added composite PK admin support. --- django/contrib/admin/options.py | 13 ++++--- django/contrib/admin/sites.py | 5 --- django/contrib/admin/utils.py | 15 ++++++-- django/contrib/auth/admin.py | 3 +- tests/admin_registration/tests.py | 8 +--- tests/admin_utils/tests.py | 34 ++++++++++++++++- tests/admin_views/admin.py | 2 + tests/admin_views/models.py | 6 +++ tests/admin_views/tests.py | 62 +++++++++++++++++++++++++++++++ 9 files changed, 126 insertions(+), 22 deletions(-) diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py index 69b0cc0373..b461064bb9 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -1828,7 +1828,7 @@ class ModelAdmin(BaseModelAdmin): """ msg = _("%(name)s with ID “%(key)s” doesn’t exist. Perhaps it was deleted?") % { "name": opts.verbose_name, - "key": unquote(object_id), + "key": self.unquote(object_id), } self.message_user(request, msg, messages.WARNING) url = reverse("admin:index", current_app=self.admin_site.name) @@ -1860,7 +1860,7 @@ class ModelAdmin(BaseModelAdmin): obj = None else: - obj = self.get_object(request, unquote(object_id), to_field) + obj = self.get_object(request, self.unquote(object_id), to_field) if request.method == "POST": if not self.has_change_permission(request, obj): @@ -2216,7 +2216,7 @@ class ModelAdmin(BaseModelAdmin): "The field %s cannot be referenced." % to_field ) - obj = self.get_object(request, unquote(object_id), to_field) + obj = self.get_object(request, self.unquote(object_id), to_field) if not self.has_delete_permission(request, obj): raise PermissionDenied @@ -2278,7 +2278,7 @@ class ModelAdmin(BaseModelAdmin): # First check if the user can see this history. model = self.model - obj = self.get_object(request, unquote(object_id)) + obj = self.get_object(request, self.unquote(object_id)) if obj is None: return self._get_obj_does_not_exist_redirect( request, model._meta, object_id @@ -2291,7 +2291,7 @@ class ModelAdmin(BaseModelAdmin): app_label = self.opts.app_label action_list = ( LogEntry.objects.filter( - object_id=unquote(object_id), + object_id=self.unquote(object_id), content_type=get_content_type_for_model(model), ) .select_related() @@ -2382,6 +2382,9 @@ class ModelAdmin(BaseModelAdmin): inline_instances.append(inline) return formsets, inline_instances + def unquote(self, pk): + return unquote(pk, is_composite=self.opts.is_composite_pk) + class InlineModelAdmin(BaseModelAdmin): """ diff --git a/django/contrib/admin/sites.py b/django/contrib/admin/sites.py index 201f28ef37..3399bd87b8 100644 --- a/django/contrib/admin/sites.py +++ b/django/contrib/admin/sites.py @@ -113,11 +113,6 @@ class AdminSite: "The model %s is abstract, so it cannot be registered with admin." % model.__name__ ) - if model._meta.is_composite_pk: - raise ImproperlyConfigured( - "The model %s has a composite primary key, so it cannot be " - "registered with admin." % model.__name__ - ) if self.is_registered(model): registered_admin = str(self.get_model_admin(model)) diff --git a/django/contrib/admin/utils.py b/django/contrib/admin/utils.py index c8e722bcc8..c33f637415 100644 --- a/django/contrib/admin/utils.py +++ b/django/contrib/admin/utils.py @@ -23,6 +23,7 @@ from django.utils.translation import override as translation_override QUOTE_MAP = {i: "_%02X" % i for i in b'":/_#?;@&=+$,"[]<>%\n\\'} UNQUOTE_MAP = {v: chr(k) for k, v in QUOTE_MAP.items()} UNQUOTE_RE = _lazy_re_compile("_(?:%s)" % "|".join([x[1:] for x in UNQUOTE_MAP])) +PK_SEP = "," class FieldIsAForeignKeyColumnName(Exception): @@ -91,12 +92,20 @@ def quote(s): Similar to urllib.parse.quote(), except that the quoting is slightly different so that it doesn't get automatically unquoted by the web browser. """ - return s.translate(QUOTE_MAP) if isinstance(s, str) else s + if isinstance(s, str): + return s.translate(QUOTE_MAP) + elif isinstance(s, tuple): + return PK_SEP.join(str(quote(f)) for f in s) + else: + return s -def unquote(s): +def unquote(s, is_composite=False): """Undo the effects of quote().""" - return UNQUOTE_RE.sub(lambda m: UNQUOTE_MAP[m[0]], s) + if is_composite: + return tuple(unquote(f) for f in s.split(PK_SEP)) + else: + return UNQUOTE_RE.sub(lambda m: UNQUOTE_MAP[m[0]], s) def flatten(fields): diff --git a/django/contrib/auth/admin.py b/django/contrib/auth/admin.py index e977d3ded5..ca57edc62a 100644 --- a/django/contrib/auth/admin.py +++ b/django/contrib/auth/admin.py @@ -1,7 +1,6 @@ from django.conf import settings from django.contrib import admin, messages from django.contrib.admin.options import IS_POPUP_VAR -from django.contrib.admin.utils import unquote from django.contrib.auth import update_session_auth_hash from django.contrib.auth.forms import ( AdminPasswordChangeForm, @@ -153,7 +152,7 @@ class UserAdmin(admin.ModelAdmin): @sensitive_post_parameters_m def user_change_password(self, request, id, form_url=""): - user = self.get_object(request, unquote(id)) + user = self.get_object(request, self.unquote(id)) if not self.has_change_permission(request, user): raise PermissionDenied if user is None: diff --git a/tests/admin_registration/tests.py b/tests/admin_registration/tests.py index 0a881caf65..a17c12e4cb 100644 --- a/tests/admin_registration/tests.py +++ b/tests/admin_registration/tests.py @@ -93,12 +93,8 @@ class TestRegistration(SimpleTestCase): self.site.register(Location) def test_composite_pk_model(self): - msg = ( - "The model Guest has a composite primary key, so it cannot be registered " - "with admin." - ) - with self.assertRaisesMessage(ImproperlyConfigured, msg): - self.site.register(Guest) + self.site.register(Guest) + self.assertTrue(self.site.is_registered(Guest)) def test_is_registered_model(self): "Checks for registered models should return true." diff --git a/tests/admin_utils/tests.py b/tests/admin_utils/tests.py index 56d46324e0..c4347ed0c8 100644 --- a/tests/admin_utils/tests.py +++ b/tests/admin_utils/tests.py @@ -16,6 +16,7 @@ from django.contrib.admin.utils import ( label_for_field, lookup_field, quote, + unquote, ) from django.core.validators import EMPTY_VALUES from django.db import DEFAULT_DB_ALIAS, models @@ -436,7 +437,38 @@ class UtilsTests(SimpleTestCase): ) def test_quote(self): - self.assertEqual(quote("something\nor\nother"), "something_0Aor_0Aother") + test_cases = ( + ("something\nor\nother", "something_0Aor_0Aother"), + ("f,o,o", "f_2Co_2Co"), + ("b-a-r", "b-a-r"), + ((), ""), + ((1, 2), "1,2"), + ((3, "f,o,o"), "3,f_2Co_2Co"), + ((4, "b-a-r"), "4,b-a-r"), + ) + + for s, expected in test_cases: + with self.subTest(s=s, expected=expected): + self.assertEqual(quote(s), expected) + + def test_unquote(self): + test_cases = ( + ("something_0Aor_0Aother", False, "something\nor\nother"), + ("f_2Co_2Co", False, "f,o,o"), + ("b-a-r", False, "b-a-r"), + ("", False, ""), + ("", True, ("",)), + ("1,2,3", False, "1,2,3"), + ("1,2,3", True, ("1", "2", "3")), + ("3,f_2Co_2Co", False, "3,f,o,o"), + ("3,f_2Co_2Co", True, ("3", "f,o,o")), + ("4,b-a-r", False, "4,b-a-r"), + ("4,b-a-r", True, ("4", "b-a-r")), + ) + + for s, is_composite, expected in test_cases: + with self.subTest(s=s, is_composite=is_composite, expected=expected): + self.assertEqual(unquote(s, is_composite=is_composite), expected) def test_build_q_object_from_lookup_parameters(self): parameters = { diff --git a/tests/admin_views/admin.py b/tests/admin_views/admin.py index 5e14069bae..6485846d5d 100644 --- a/tests/admin_views/admin.py +++ b/tests/admin_views/admin.py @@ -46,6 +46,7 @@ from .models import ( Color, Color2, ComplexSortedPerson, + CompositePKModel, Country, CoverLetter, CustomArticle, @@ -1200,6 +1201,7 @@ site.register( search_fields=["name"], ) site.register(ModelWithStringPrimaryKey) +site.register(CompositePKModel) site.register(Color) site.register(Thing, ThingAdmin) site.register(Actor) diff --git a/tests/admin_views/models.py b/tests/admin_views/models.py index 812505de82..35040348af 100644 --- a/tests/admin_views/models.py +++ b/tests/admin_views/models.py @@ -145,6 +145,12 @@ class ModelWithStringPrimaryKey(models.Model): return "/dummy/%s/" % self.string_pk +class CompositePKModel(models.Model): + pk = models.CompositePrimaryKey("foo", "bar") + foo = models.CharField(max_length=10) + bar = models.CharField(max_length=10) + + class Color(models.Model): value = models.CharField(max_length=10) warm = models.BooleanField(default=False) diff --git a/tests/admin_views/tests.py b/tests/admin_views/tests.py index 1ff29fc3db..262068b91f 100644 --- a/tests/admin_views/tests.py +++ b/tests/admin_views/tests.py @@ -72,6 +72,7 @@ from .models import ( Collector, Color, ComplexSortedPerson, + CompositePKModel, CoverLetter, CustomArticle, CyclicOne, @@ -4078,6 +4079,67 @@ class AdminViewStringPrimaryKeyTest(TestCase): self.assertIn("/123_2Fhistory/", response.headers["location"]) # PK is quoted +@override_settings(ROOT_URLCONF="admin_views.urls") +class AdminViewCompositePKTests(TestCase): + MODEL = "compositepkmodel" + CHANGE_VIEW = "admin:admin_views_%s_change" + HISTORY_VIEW = "admin:admin_views_%s_history" + DELETE_VIEW = "admin:admin_views_%s_delete" + CHANGELIST_VIEW = "admin:admin_views_%s_changelist" + + @classmethod + def setUpTestData(cls): + cls.obj = CompositePKModel.objects.create(foo="f,o,o", bar="b-a-r") + cls.ct = ContentType.objects.get_for_model(CompositePKModel) + cls.superuser = User.objects.create_superuser( + username="super", password="secret", email="super@example.com" + ) + + def setUp(self): + self.client.force_login(self.superuser) + + def test_history_view(self): + viewname = self.HISTORY_VIEW % (self.MODEL,) + url = reverse(viewname, args=(quote(self.obj.pk),)) + response = self.client.get(url) + self.assertContains(response, escape(self.obj.pk)) + + def test_history_view_redirects_if_does_not_exist(self): + viewname = self.HISTORY_VIEW % (self.MODEL,) + url = reverse(viewname, args=("1,2,3",)) + response = self.client.get(url) + self.assertEqual(response.status_code, 302) + + def test_change_view(self): + viewname = self.CHANGE_VIEW % (self.MODEL,) + url = reverse(viewname, args=(quote(self.obj.pk),)) + response = self.client.get(url) + self.assertContains(response, escape(self.obj.pk)) + + response = self.client.post(url, {"foo": self.obj.foo, "bar": self.obj.bar}) + self.assertRedirects(response, reverse(self.CHANGELIST_VIEW % (self.MODEL,))) + log_entry = LogEntry.objects.get(content_type=self.ct) + self.assertEqual(log_entry.object_id, '["f,o,o", "b,a,r"]') + + def test_change_view_redirects_if_does_not_exist(self): + viewname = self.CHANGE_VIEW % (self.MODEL,) + url = reverse(viewname, args=("f,o,o",)) + response = self.client.get(url) + self.assertEqual(response.status_code, 302) + + def test_delete_view(self): + viewname = self.DELETE_VIEW % (self.MODEL,) + url = reverse(viewname, args=(quote(self.obj.pk),)) + response = self.client.get(url) + self.assertContains(response, escape(self.obj.pk)) + + def test_delete_view_redirects_if_does_not_exist(self): + viewname = self.DELETE_VIEW % (self.MODEL,) + url = reverse(viewname, args=("1,2",)) + response = self.client.get(url) + self.assertEqual(response.status_code, 302) + + @override_settings(ROOT_URLCONF="admin_views.urls") class SecureViewTests(TestCase): """