Fixed #35520 -- Avoided opening transaction for read-only ModelAdmin requests.

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)
 

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)
 

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):

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)

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()