From e2dc55096135f901aaab09912d0a1dd1a82d204d Mon Sep 17 00:00:00 2001 From: Paul Schilling Date: Tue, 4 Apr 2023 09:09:10 +0200 Subject: [PATCH 1/3] :test_tube: [#2632] -- reproduce recursion error in test + preliminary 'fix' by catching the recursion error --- .../data_removal/tests/test_tasks.py | 54 ++++++++++++++++++- .../submissions/models/submission_step.py | 6 ++- 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/openforms/data_removal/tests/test_tasks.py b/src/openforms/data_removal/tests/test_tasks.py index 98945155af..89f22125a1 100644 --- a/src/openforms/data_removal/tests/test_tasks.py +++ b/src/openforms/data_removal/tests/test_tasks.py @@ -1,10 +1,11 @@ from datetime import timedelta from django.core.exceptions import ObjectDoesNotExist -from django.test import TestCase +from django.test import TestCase, tag from django.utils import timezone from openforms.config.models import GlobalConfiguration +from openforms.forms.models.form_step import FormStep from openforms.forms.tests.factories import ( FormDefinitionFactory, FormFactory, @@ -57,6 +58,57 @@ def test_successful_submissions_correctly_deleted(self): with self.assertRaises(ObjectDoesNotExist): submission_to_be_deleted.refresh_from_db() + @tag("gh-2632") + def test_delete_successful_submission_with_deleted_form_step(self): + config = GlobalConfiguration.get_solo() + + # submission to be deleted + sub = SubmissionFactory.create( + registration_status=RegistrationStatuses.success, + ) + sub.created_on = timezone.now() - timedelta( + days=config.successful_submissions_removal_limit + 1 + ) + sub.save() + + # sanity check: create one additional submission that should not be deleted + SubmissionFactory.create( + registration_status=RegistrationStatuses.pending, + ) + + # additional setup: formsteps + submission steps + form_step_1 = FormStepFactory.create( + form=FormFactory.create( + successful_submissions_removal_method=RemovalMethods.delete_permanently, + ), + form_definition=FormDefinitionFactory.create(), + ) + form_step_2 = FormStepFactory.create( + form=FormFactory.create( + successful_submissions_removal_method=RemovalMethods.delete_permanently, + ), + form_definition=FormDefinitionFactory.create(), + ) + SubmissionStepFactory.create( + form_step=form_step_1, + submission=sub, + ) + SubmissionStepFactory.create( + form_step=form_step_2, + submission=sub, + ) + + # delete form step + form_steps = FormStep.objects.all() + form_steps.filter(id=form_step_1.pk).delete() + + delete_submissions() + + self.assertEqual(Submission.objects.count(), 1) + + with self.assertRaises(ObjectDoesNotExist): + sub.refresh_from_db() + def test_successful_submissions_with_form_settings_override_global_configuration( self, ): diff --git a/src/openforms/submissions/models/submission_step.py b/src/openforms/submissions/models/submission_step.py index 7e021982fe..bb45b4161a 100644 --- a/src/openforms/submissions/models/submission_step.py +++ b/src/openforms/submissions/models/submission_step.py @@ -138,7 +138,11 @@ def __str__(self): return f"SubmissionStep {self.pk}: Submission {self.submission_id} submitted on {self.created_on}" def _load_form_step_from_history(self): - history = deepcopy(self.form_step_history) + try: + history = deepcopy(self.form_step_history) + except RecursionError: + return None + if not history: return None From 86a8abdc6fcd15d8f2123f9f5269a794317329ab Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Thu, 6 Apr 2023 15:54:46 +0200 Subject: [PATCH 2/3] :art: [#2632] -- Simplify regression test --- .../data_removal/tests/test_tasks.py | 51 +++++-------------- 1 file changed, 14 insertions(+), 37 deletions(-) diff --git a/src/openforms/data_removal/tests/test_tasks.py b/src/openforms/data_removal/tests/test_tasks.py index 89f22125a1..34b4e87ed9 100644 --- a/src/openforms/data_removal/tests/test_tasks.py +++ b/src/openforms/data_removal/tests/test_tasks.py @@ -4,6 +4,8 @@ from django.test import TestCase, tag from django.utils import timezone +from freezegun import freeze_time + from openforms.config.models import GlobalConfiguration from openforms.forms.models.form_step import FormStep from openforms.forms.tests.factories import ( @@ -63,51 +65,26 @@ def test_delete_successful_submission_with_deleted_form_step(self): config = GlobalConfiguration.get_solo() # submission to be deleted - sub = SubmissionFactory.create( - registration_status=RegistrationStatuses.success, - ) - sub.created_on = timezone.now() - timedelta( + before_limit = timezone.now() - timedelta( days=config.successful_submissions_removal_limit + 1 ) - sub.save() + with freeze_time(before_limit): + sub = SubmissionFactory.create(completed=True) - # sanity check: create one additional submission that should not be deleted - SubmissionFactory.create( - registration_status=RegistrationStatuses.pending, - ) + # create a form step to then delete which introduces the RecursionError + form_step = FormStepFactory.create(form=sub.form) + SubmissionStepFactory.create(submission=sub, form_step=form_step) - # additional setup: formsteps + submission steps - form_step_1 = FormStepFactory.create( - form=FormFactory.create( - successful_submissions_removal_method=RemovalMethods.delete_permanently, - ), - form_definition=FormDefinitionFactory.create(), - ) - form_step_2 = FormStepFactory.create( - form=FormFactory.create( - successful_submissions_removal_method=RemovalMethods.delete_permanently, - ), - form_definition=FormDefinitionFactory.create(), - ) - SubmissionStepFactory.create( - form_step=form_step_1, - submission=sub, - ) - SubmissionStepFactory.create( - form_step=form_step_2, - submission=sub, - ) + # delete form step - this causes the recursion error + form_step.delete() - # delete form step - form_steps = FormStep.objects.all() - form_steps.filter(id=form_step_1.pk).delete() + assert sub.submissionstep_set.count() == 1 + assert isinstance(sub.submissionstep_set.get().form_step, FormStep) + assert not FormStep.objects.exists() delete_submissions() - self.assertEqual(Submission.objects.count(), 1) - - with self.assertRaises(ObjectDoesNotExist): - sub.refresh_from_db() + self.assertFalse(Submission.objects.exists()) def test_successful_submissions_with_form_settings_override_global_configuration( self, From 7532121ddb035b7f860fd10075d948bb19183af4 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Thu, 6 Apr 2023 16:28:43 +0200 Subject: [PATCH 3/3] :adhesive_bandage: [#2632] -- work around recursion when deleting submission querysets --- .../submissions/models/submission_step.py | 33 ++++++++++++++----- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/src/openforms/submissions/models/submission_step.py b/src/openforms/submissions/models/submission_step.py index bb45b4161a..188448c30c 100644 --- a/src/openforms/submissions/models/submission_step.py +++ b/src/openforms/submissions/models/submission_step.py @@ -129,19 +129,36 @@ class Meta: def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - # load the form step from historical data if the FK has been broken (due to a - # deleted form step in the form designer/import functionality) - if not self.form_step_id: - self.form_step = self._load_form_step_from_history() + # See #2632, and a long story here on what happens: + # + # When the Submission queryset is deleted, this happens through the + # django.db.models.deletion.Collector which takes care of finding all related + # objects to manage the cascade delete. + # + # It does this (apparently) in an optimized way where only the primary keys of + # records (SubmssionStep in this case) are being loaded (via + # ``get_candidate_relations_to_delete`` and Collector.related_objects). The + # details are not exactly clear, but it *appears* that the form_step_id + # field/description is deferred. This means that when you *access* this field, + # Django will actually perform a separate query to load the (full) record with + # the additional information from the database row, which in turn leads to a new + # instance of the model being created, which in turn causes the __init__ method + # to be called again and that cycle then repeats (since that field is probably + # again deferred?). + # + # We can check if the field was loaded or not by checking in self.__dict__. Note + # that this is far from public API and a structural fix is still recommended. + if "form_step_id" in self.__dict__: + # load the form step from historical data if the FK has been broken (due to a + # deleted form step in the form designer/import functionality) + if not self.form_step_id: + self.form_step = self._load_form_step_from_history() def __str__(self): return f"SubmissionStep {self.pk}: Submission {self.submission_id} submitted on {self.created_on}" def _load_form_step_from_history(self): - try: - history = deepcopy(self.form_step_history) - except RecursionError: - return None + history = deepcopy(self.form_step_history) if not history: return None