From b18650a2634890aa758abae2f33875daa13a9ba3 Mon Sep 17 00:00:00 2001 From: Adam Donaghy Date: Thu, 3 May 2018 23:41:04 +1000 Subject: [PATCH] Fixed #28462 -- Decreased memory usage with ModelAdmin.list_editable. Regression in 917cc288a38f3c114a5440f0749b7e5e1086eb36. --- AUTHORS | 1 + django/contrib/admin/options.py | 25 +++++++++- docs/releases/1.11.14.txt | 3 ++ docs/releases/2.0.6.txt | 3 ++ tests/admin_changelist/models.py | 3 ++ tests/admin_changelist/tests.py | 85 ++++++++++++++++++++++++++++++-- 6 files changed, 116 insertions(+), 4 deletions(-) diff --git a/AUTHORS b/AUTHORS index 9be6412d19..a68a5beb48 100644 --- a/AUTHORS +++ b/AUTHORS @@ -11,6 +11,7 @@ answer newbie questions, and generally made Django that much better: Abeer Upadhyay Abhishek Gautam Adam Bogdał + Adam Donaghy Adam Johnson Adam Malinowski Adam Vandenberg diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py index 5b7de20e2d..206e01a2f0 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -1,6 +1,7 @@ import copy import json import operator +import re from collections import OrderedDict from functools import partial, reduce, update_wrapper from urllib.parse import quote as urlquote @@ -1633,6 +1634,27 @@ class ModelAdmin(BaseModelAdmin): def change_view(self, request, object_id, form_url='', extra_context=None): return self.changeform_view(request, object_id, form_url, extra_context) + def _get_edited_object_pks(self, request, prefix): + """Return POST data values of list_editable primary keys.""" + pk_pattern = re.compile('{}-\d+-{}$'.format(prefix, self.model._meta.pk.name)) + return [value for key, value in request.POST.items() if pk_pattern.match(key)] + + def _get_list_editable_queryset(self, request, prefix): + """ + Based on POST data, return a queryset of the objects that were edited + via list_editable. + """ + object_pks = self._get_edited_object_pks(request, prefix) + queryset = self.get_queryset(request) + validate = queryset.model._meta.pk.to_python + try: + for pk in object_pks: + validate(pk) + except ValidationError: + # Disable the optimization if the POST data was tampered with. + return queryset + return queryset.filter(pk__in=object_pks) + @csrf_protect_m def changelist_view(self, request, extra_context=None): """ @@ -1713,7 +1735,8 @@ class ModelAdmin(BaseModelAdmin): if not self.has_change_permission(request): raise PermissionDenied FormSet = self.get_changelist_formset(request) - formset = cl.formset = FormSet(request.POST, request.FILES, queryset=self.get_queryset(request)) + modified_objects = self._get_list_editable_queryset(request, FormSet.get_default_prefix()) + formset = cl.formset = FormSet(request.POST, request.FILES, queryset=modified_objects) if formset.is_valid(): changecount = 0 for form in formset.forms: diff --git a/docs/releases/1.11.14.txt b/docs/releases/1.11.14.txt index 06fe74464a..c433673111 100644 --- a/docs/releases/1.11.14.txt +++ b/docs/releases/1.11.14.txt @@ -11,3 +11,6 @@ Bugfixes * Fixed ``WKBWriter.write()`` and ``write_hex()`` for empty polygons on GEOS 3.6.1+ (:ticket:`29460`). + +* Fixed a regression in Django 1.10 that could result in large memory usage + when making edits using ``ModelAdmin.list_editable`` (:ticket:`28462`). diff --git a/docs/releases/2.0.6.txt b/docs/releases/2.0.6.txt index 8b07cb2fd8..73943885dd 100644 --- a/docs/releases/2.0.6.txt +++ b/docs/releases/2.0.6.txt @@ -20,3 +20,6 @@ Bugfixes * Fixed ``WKBWriter.write()`` and ``write_hex()`` for empty polygons on GEOS 3.6.1+ (:ticket:`29460`). + +* Fixed a regression in Django 1.10 that could result in large memory usage + when making edits using ``ModelAdmin.list_editable`` (:ticket:`28462`). diff --git a/tests/admin_changelist/models.py b/tests/admin_changelist/models.py index 62268a2b79..81d7fdfb3a 100644 --- a/tests/admin_changelist/models.py +++ b/tests/admin_changelist/models.py @@ -1,3 +1,5 @@ +import uuid + from django.db import models @@ -73,6 +75,7 @@ class Invitation(models.Model): class Swallow(models.Model): + uuid = models.UUIDField(primary_key=True, default=uuid.uuid4) origin = models.CharField(max_length=255) load = models.FloatField() speed = models.FloatField() diff --git a/tests/admin_changelist/tests.py b/tests/admin_changelist/tests.py index 20587613a7..9f9150f34d 100644 --- a/tests/admin_changelist/tests.py +++ b/tests/admin_changelist/tests.py @@ -8,6 +8,7 @@ from django.contrib.admin.tests import AdminSeleniumTestCase from django.contrib.admin.views.main import ALL_VAR, SEARCH_VAR from django.contrib.auth.models import User from django.contrib.contenttypes.models import ContentType +from django.db import connection from django.db.models import F from django.db.models.fields import Field, IntegerField from django.db.models.functions import Upper @@ -15,6 +16,7 @@ from django.db.models.lookups import Contains, Exact from django.template import Context, Template, TemplateSyntaxError from django.test import TestCase, override_settings from django.test.client import RequestFactory +from django.test.utils import CaptureQueriesContext from django.urls import reverse from django.utils import formats @@ -732,9 +734,9 @@ class ChangeListTests(TestCase): 'form-INITIAL_FORMS': '3', 'form-MIN_NUM_FORMS': '0', 'form-MAX_NUM_FORMS': '1000', - 'form-0-id': str(d.pk), - 'form-1-id': str(c.pk), - 'form-2-id': str(a.pk), + 'form-0-uuid': str(d.pk), + 'form-1-uuid': str(c.pk), + 'form-2-uuid': str(a.pk), 'form-0-load': '9.0', 'form-0-speed': '9.0', 'form-1-load': '5.0', @@ -764,6 +766,83 @@ class ChangeListTests(TestCase): # No new swallows were created. self.assertEqual(len(Swallow.objects.all()), 4) + def test_get_edited_object_ids(self): + a = Swallow.objects.create(origin='Swallow A', load=4, speed=1) + b = Swallow.objects.create(origin='Swallow B', load=2, speed=2) + c = Swallow.objects.create(origin='Swallow C', load=5, speed=5) + superuser = self._create_superuser('superuser') + self.client.force_login(superuser) + changelist_url = reverse('admin:admin_changelist_swallow_changelist') + m = SwallowAdmin(Swallow, custom_site) + data = { + 'form-TOTAL_FORMS': '3', + 'form-INITIAL_FORMS': '3', + 'form-MIN_NUM_FORMS': '0', + 'form-MAX_NUM_FORMS': '1000', + 'form-0-uuid': str(a.pk), + 'form-1-uuid': str(b.pk), + 'form-2-uuid': str(c.pk), + 'form-0-load': '9.0', + 'form-0-speed': '9.0', + 'form-1-load': '5.0', + 'form-1-speed': '5.0', + 'form-2-load': '5.0', + 'form-2-speed': '4.0', + '_save': 'Save', + } + request = self.factory.post(changelist_url, data=data) + pks = m._get_edited_object_pks(request, prefix='form') + self.assertEqual(sorted(pks), sorted([str(a.pk), str(b.pk), str(c.pk)])) + + def test_get_list_editable_queryset(self): + a = Swallow.objects.create(origin='Swallow A', load=4, speed=1) + Swallow.objects.create(origin='Swallow B', load=2, speed=2) + data = { + 'form-TOTAL_FORMS': '2', + 'form-INITIAL_FORMS': '2', + 'form-MIN_NUM_FORMS': '0', + 'form-MAX_NUM_FORMS': '1000', + 'form-0-uuid': str(a.pk), + 'form-0-load': '10', + '_save': 'Save', + } + superuser = self._create_superuser('superuser') + self.client.force_login(superuser) + changelist_url = reverse('admin:admin_changelist_swallow_changelist') + m = SwallowAdmin(Swallow, custom_site) + request = self.factory.post(changelist_url, data=data) + queryset = m._get_list_editable_queryset(request, prefix='form') + self.assertEqual(queryset.count(), 1) + data['form-0-uuid'] = 'INVALD_PRIMARY_KEY' + # The unfiltered queryset is returned if there's invalid data. + request = self.factory.post(changelist_url, data=data) + queryset = m._get_list_editable_queryset(request, prefix='form') + self.assertEqual(queryset.count(), 2) + + def test_changelist_view_list_editable_changed_objects_uses_filter(self): + """list_editable edits use a filtered queryset to limit memory usage.""" + a = Swallow.objects.create(origin='Swallow A', load=4, speed=1) + Swallow.objects.create(origin='Swallow B', load=2, speed=2) + data = { + 'form-TOTAL_FORMS': '2', + 'form-INITIAL_FORMS': '2', + 'form-MIN_NUM_FORMS': '0', + 'form-MAX_NUM_FORMS': '1000', + 'form-0-uuid': str(a.pk), + 'form-0-load': '10', + '_save': 'Save', + } + superuser = self._create_superuser('superuser') + self.client.force_login(superuser) + changelist_url = reverse('admin:admin_changelist_swallow_changelist') + with CaptureQueriesContext(connection) as context: + response = self.client.post(changelist_url, data=data) + self.assertEqual(response.status_code, 200) + self.assertIn('WHERE', context.captured_queries[4]['sql']) + self.assertIn('IN', context.captured_queries[4]['sql']) + # Check only the first few characters since the UUID may have dashes. + self.assertIn(str(a.pk)[:8], context.captured_queries[4]['sql']) + def test_deterministic_order_for_unordered_model(self): """ The primary key is used in the ordering of the changelist's results to