From 53e674d5744faad61e52d8459c9198b2aa6f63dd Mon Sep 17 00:00:00 2001
From: Jake Howard <git@theorangeone.net>
Date: Tue, 11 Jun 2024 19:27:49 +0100
Subject: [PATCH] Fixed #35520 -- Avoided opening transaction for read-only
ModelAdmin requests.
---
django/contrib/admin/options.py | 6 +++
django/contrib/auth/admin.py | 3 ++
tests/admin_views/test_multidb.py | 75 ++++++++++++++++++++++++--
tests/admin_views/tests.py | 4 +-
tests/auth_tests/test_admin_multidb.py | 22 +++++++-
5 files changed, 104 insertions(+), 6 deletions(-)
diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py
index 9cc891d807..e8760c2931 100644
--- a/django/contrib/admin/options.py
+++ b/django/contrib/admin/options.py
@@ -1814,6 +1814,9 @@ class ModelAdmin(BaseModelAdmin):
@csrf_protect_m
def changeform_view(self, request, object_id=None, form_url="", extra_context=None):
+ if request.method in ("GET", "HEAD", "OPTIONS", "TRACE"):
+ return self._changeform_view(request, object_id, form_url, extra_context)
+
with transaction.atomic(using=router.db_for_write(self.model)):
return self._changeform_view(request, object_id, form_url, extra_context)
@@ -2175,6 +2178,9 @@ class ModelAdmin(BaseModelAdmin):
@csrf_protect_m
def delete_view(self, request, object_id, extra_context=None):
+ if request.method in ("GET", "HEAD", "OPTIONS", "TRACE"):
+ return self._delete_view(request, object_id, extra_context)
+
with transaction.atomic(using=router.db_for_write(self.model)):
return self._delete_view(request, object_id, extra_context)
diff --git a/django/contrib/auth/admin.py b/django/contrib/auth/admin.py
index 90a53a142c..8e1d63ef07 100644
--- a/django/contrib/auth/admin.py
+++ b/django/contrib/auth/admin.py
@@ -117,6 +117,9 @@ class UserAdmin(admin.ModelAdmin):
@sensitive_post_parameters_m
@csrf_protect_m
def add_view(self, request, form_url="", extra_context=None):
+ if request.method in ("GET", "HEAD", "OPTIONS", "TRACE"):
+ return self._add_view(request, form_url, extra_context)
+
with transaction.atomic(using=router.db_for_write(self.model)):
return self._add_view(request, form_url, extra_context)
diff --git a/tests/admin_views/test_multidb.py b/tests/admin_views/test_multidb.py
index 654161e11d..0f18aeb315 100644
--- a/tests/admin_views/test_multidb.py
+++ b/tests/admin_views/test_multidb.py
@@ -40,6 +40,7 @@ urlpatterns = [
@override_settings(ROOT_URLCONF=__name__, DATABASE_ROUTERS=["%s.Router" % __name__])
class MultiDatabaseTests(TestCase):
databases = {"default", "other"}
+ READ_ONLY_METHODS = {"get", "options", "head", "trace"}
@classmethod
def setUpTestData(cls):
@@ -56,48 +57,116 @@ class MultiDatabaseTests(TestCase):
b.save(using=db)
cls.test_book_ids[db] = b.id
+ def tearDown(self):
+ # Reset the routers' state between each test.
+ Router.target_db = None
+
@mock.patch("django.contrib.admin.options.transaction")
def test_add_view(self, mock):
for db in self.databases:
with self.subTest(db=db):
+ mock.mock_reset()
Router.target_db = db
self.client.force_login(self.superusers[db])
- self.client.post(
+ response = self.client.post(
reverse("test_adminsite:admin_views_book_add"),
{"name": "Foobar: 5th edition"},
)
+ self.assertEqual(response.status_code, 302)
+ self.assertEqual(
+ response.url, reverse("test_adminsite:admin_views_book_changelist")
+ )
mock.atomic.assert_called_with(using=db)
+ @mock.patch("django.contrib.admin.options.transaction")
+ def test_read_only_methods_add_view(self, mock):
+ for db in self.databases:
+ for method in self.READ_ONLY_METHODS:
+ with self.subTest(db=db, method=method):
+ mock.mock_reset()
+ Router.target_db = db
+ self.client.force_login(self.superusers[db])
+ response = getattr(self.client, method)(
+ reverse("test_adminsite:admin_views_book_add"),
+ )
+ self.assertEqual(response.status_code, 200)
+ mock.atomic.assert_not_called()
+
@mock.patch("django.contrib.admin.options.transaction")
def test_change_view(self, mock):
for db in self.databases:
with self.subTest(db=db):
+ mock.mock_reset()
Router.target_db = db
self.client.force_login(self.superusers[db])
- self.client.post(
+ response = self.client.post(
reverse(
"test_adminsite:admin_views_book_change",
args=[self.test_book_ids[db]],
),
{"name": "Test Book 2: Test more"},
)
+ self.assertEqual(response.status_code, 302)
+ self.assertEqual(
+ response.url, reverse("test_adminsite:admin_views_book_changelist")
+ )
mock.atomic.assert_called_with(using=db)
+ @mock.patch("django.contrib.admin.options.transaction")
+ def test_read_only_methods_change_view(self, mock):
+ for db in self.databases:
+ for method in self.READ_ONLY_METHODS:
+ with self.subTest(db=db, method=method):
+ mock.mock_reset()
+ Router.target_db = db
+ self.client.force_login(self.superusers[db])
+ response = getattr(self.client, method)(
+ reverse(
+ "test_adminsite:admin_views_book_change",
+ args=[self.test_book_ids[db]],
+ ),
+ data={"name": "Test Book 2: Test more"},
+ )
+ self.assertEqual(response.status_code, 200)
+ mock.atomic.assert_not_called()
+
@mock.patch("django.contrib.admin.options.transaction")
def test_delete_view(self, mock):
for db in self.databases:
with self.subTest(db=db):
+ mock.mock_reset()
Router.target_db = db
self.client.force_login(self.superusers[db])
- self.client.post(
+ response = self.client.post(
reverse(
"test_adminsite:admin_views_book_delete",
args=[self.test_book_ids[db]],
),
{"post": "yes"},
)
+ self.assertEqual(response.status_code, 302)
+ self.assertEqual(
+ response.url, reverse("test_adminsite:admin_views_book_changelist")
+ )
mock.atomic.assert_called_with(using=db)
+ @mock.patch("django.contrib.admin.options.transaction")
+ def test_read_only_methods_delete_view(self, mock):
+ for db in self.databases:
+ for method in self.READ_ONLY_METHODS:
+ with self.subTest(db=db, method=method):
+ mock.mock_reset()
+ Router.target_db = db
+ self.client.force_login(self.superusers[db])
+ response = getattr(self.client, method)(
+ reverse(
+ "test_adminsite:admin_views_book_delete",
+ args=[self.test_book_ids[db]],
+ )
+ )
+ self.assertEqual(response.status_code, 200)
+ mock.atomic.assert_not_called()
+
class ViewOnSiteRouter:
def db_for_read(self, model, instance=None, **hints):
diff --git a/tests/admin_views/tests.py b/tests/admin_views/tests.py
index 763fa44ce8..e0a4926b91 100644
--- a/tests/admin_views/tests.py
+++ b/tests/admin_views/tests.py
@@ -7385,7 +7385,7 @@ class UserAdminTest(TestCase):
# Don't depend on a warm cache, see #17377.
ContentType.objects.clear_cache()
- expected_num_queries = 10 if connection.features.uses_savepoints else 8
+ expected_num_queries = 8 if connection.features.uses_savepoints else 6
with self.assertNumQueries(expected_num_queries):
response = self.client.get(reverse("admin:auth_user_change", args=(u.pk,)))
self.assertEqual(response.status_code, 200)
@@ -7433,7 +7433,7 @@ class GroupAdminTest(TestCase):
# Ensure no queries are skipped due to cached content type for Group.
ContentType.objects.clear_cache()
- expected_num_queries = 8 if connection.features.uses_savepoints else 6
+ expected_num_queries = 6 if connection.features.uses_savepoints else 4
with self.assertNumQueries(expected_num_queries):
response = self.client.get(reverse("admin:auth_group_change", args=(g.pk,)))
self.assertEqual(response.status_code, 200)
diff --git a/tests/auth_tests/test_admin_multidb.py b/tests/auth_tests/test_admin_multidb.py
index ce2ae6b103..17b04faa65 100644
--- a/tests/auth_tests/test_admin_multidb.py
+++ b/tests/auth_tests/test_admin_multidb.py
@@ -30,6 +30,7 @@ urlpatterns = [
@override_settings(ROOT_URLCONF=__name__, DATABASE_ROUTERS=["%s.Router" % __name__])
class MultiDatabaseTests(TestCase):
databases = {"default", "other"}
+ READ_ONLY_METHODS = {"get", "options", "head", "trace"}
@classmethod
def setUpTestData(cls):
@@ -42,13 +43,17 @@ class MultiDatabaseTests(TestCase):
email="test@test.org",
)
+ def tearDown(self):
+ # Reset the routers' state between each test.
+ Router.target_db = None
+
@mock.patch("django.contrib.auth.admin.transaction")
def test_add_view(self, mock):
for db in self.databases:
with self.subTest(db_connection=db):
Router.target_db = db
self.client.force_login(self.superusers[db])
- self.client.post(
+ response = self.client.post(
reverse("test_adminsite:auth_user_add"),
{
"username": "some_user",
@@ -56,4 +61,19 @@ class MultiDatabaseTests(TestCase):
"password2": "helloworld",
},
)
+ self.assertEqual(response.status_code, 302)
mock.atomic.assert_called_with(using=db)
+
+ @mock.patch("django.contrib.auth.admin.transaction")
+ def test_read_only_methods_add_view(self, mock):
+ for db in self.databases:
+ for method in self.READ_ONLY_METHODS:
+ with self.subTest(db_connection=db, method=method):
+ mock.mock_reset()
+ Router.target_db = db
+ self.client.force_login(self.superusers[db])
+ response = getattr(self.client, method)(
+ reverse("test_adminsite:auth_user_add")
+ )
+ self.assertEqual(response.status_code, 200)
+ mock.atomic.assert_not_called()