From e9689f8708866d2360529a3e46db5712104db701 Mon Sep 17 00:00:00 2001 From: Anders <6058745+ddabble@users.noreply.github.com> Date: Wed, 15 May 2024 16:55:22 +0200 Subject: [PATCH] Fixed O(n) queries when adding M2M objects (#1333) --- CHANGES.rst | 3 ++ simple_history/models.py | 11 ++++--- simple_history/tests/tests/test_models.py | 37 +++++++++++++++++++++++ 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 21e5f3d4..798aca50 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -27,6 +27,9 @@ Unreleased "Customizing the History Admin Templates" for overriding its template context (gh-1128) - Fixed the setting ``SIMPLE_HISTORY_ENABLED = False`` not preventing M2M historical records from being created (gh-1328) +- For history-tracked M2M fields, adding M2M objects (using ``add()`` or ``set()``) + used to cause a number of database queries that scaled linearly with the number of + objects; this has been fixed to now be a constant number of queries (gh-1333) 3.5.0 (2024-02-19) ------------------ diff --git a/simple_history/models.py b/simple_history/models.py index 5e09d466..b7e137da 100644 --- a/simple_history/models.py +++ b/simple_history/models.py @@ -687,18 +687,21 @@ def create_historical_record_m2ms(self, history_instance, instance): m2m_history_model = self.m2m_models[field] original_instance = history_instance.instance through_model = getattr(original_instance, field.name).through + through_model_field_names = [f.name for f in through_model._meta.fields] + through_model_fk_field_names = [ + f.name for f in through_model._meta.fields if isinstance(f, ForeignKey) + ] insert_rows = [] through_field_name = utils.get_m2m_field_name(field) rows = through_model.objects.filter(**{through_field_name: instance}) + rows = rows.select_related(*through_model_fk_field_names) for row in rows: insert_row = {"history": history_instance} - for through_model_field in through_model._meta.fields: - insert_row[through_model_field.name] = getattr( - row, through_model_field.name - ) + for field_name in through_model_field_names: + insert_row[field_name] = getattr(row, field_name) insert_rows.append(m2m_history_model(**insert_row)) pre_create_historical_m2m_records.send( diff --git a/simple_history/tests/tests/test_models.py b/simple_history/tests/tests/test_models.py index c047bfc5..4854fc3a 100644 --- a/simple_history/tests/tests/test_models.py +++ b/simple_history/tests/tests/test_models.py @@ -2376,6 +2376,43 @@ def test_bulk_add_remove(self): historical_place = m2m_record.places.first() self.assertEqual(historical_place.place, self.place) + def test_add_remove_set_and_clear_methods_make_expected_num_queries(self): + for num_places in (1, 2, 4): + with self.subTest(num_places=num_places): + start_pk = 100 + num_places + places = Place.objects.bulk_create( + Place(pk=pk, name=f"Place {pk}") + for pk in range(start_pk, start_pk + num_places) + ) + self.assertEqual(len(places), num_places) + self.assertEqual(self.poll.places.count(), 0) + + # The number of queries should stay the same, regardless of + # the number of places added or removed + with self.assertNumQueries(5): + self.poll.places.add(*places) + self.assertEqual(self.poll.places.count(), num_places) + + with self.assertNumQueries(3): + self.poll.places.remove(*places) + self.assertEqual(self.poll.places.count(), 0) + + with self.assertNumQueries(6): + self.poll.places.set(places) + self.assertEqual(self.poll.places.count(), num_places) + + with self.assertNumQueries(4): + self.poll.places.set([]) + self.assertEqual(self.poll.places.count(), 0) + + with self.assertNumQueries(5): + self.poll.places.add(*places) + self.assertEqual(self.poll.places.count(), num_places) + + with self.assertNumQueries(3): + self.poll.places.clear() + self.assertEqual(self.poll.places.count(), 0) + def test_m2m_relation(self): # Ensure only the correct M2Ms are saved and returned for history objects poll_2 = PollWithManyToMany.objects.create(question="Why", pub_date=today)