From b91d9b0cf8c131381614de098793af8eec4fc96a Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Thu, 30 Jan 2025 12:13:06 +0100 Subject: [PATCH 1/5] :bug: [#5058] Removed the submission value variable recoupling Before this patch, we ran some reconciliation when a form is edited to ensure that existing SubmissionValueVariable instances don't have 'null' form_variable foreign keys because the FormVariable set of a form was updated after editing (a) step(s) - the idea here was to be eventually consistent. This turns out not to be necessary (if we can trust our test suite) because the load_submission_value_variables_state method on the Submission operates on the variable keys rather than the FK relation and is able to properly resolve everything. This is also the interface that developers should use when accessing the submission values and it appears to be done properly in the registration backends, otherwise tests would likely fail. This re-coupling was extended in #4900, after it was noticed in #4824 that the re-coupling didn't happen for other forms that use the same re-usable form definition. At the time, we didn't understand how this seemingly didn't cause issues or at least didn't result in issues being reported to us, but we can now conclude that it just wasn't a problem in the first place because the proper interfaces/service layer are/were being used and everything is done/reconciled in-memory when comparing/populating submission variables with form variables. A further cleanup step will then also be to remove this FK field from the submission value variable model, as it is not necessary. --- src/openforms/forms/tasks.py | 67 +--- .../forms/tests/variables/test_tasks.py | 335 +----------------- .../models/submission_value_variable.py | 14 +- 3 files changed, 11 insertions(+), 405 deletions(-) diff --git a/src/openforms/forms/tasks.py b/src/openforms/forms/tasks.py index bcb8684b18..5171ee2ce4 100644 --- a/src/openforms/forms/tasks.py +++ b/src/openforms/forms/tasks.py @@ -3,7 +3,6 @@ from functools import partial from django.db import DatabaseError, transaction -from django.db.utils import IntegrityError from django.utils import timezone from celery import chain @@ -18,20 +17,9 @@ def on_variables_bulk_update_event(form_id: int) -> None: """ - Celery chain of tasks to execute on a bulk update of variables event. + Celery tasks to execute on a "bulk update of variables" event. """ - repopulate_reusable_definition_variables_task = ( - repopulate_reusable_definition_variables_to_form_variables.si(form_id) - ) - recouple_submission_variables_task = ( - recouple_submission_variables_to_form_variables.si(form_id) - ) - - actions_chain = chain( - repopulate_reusable_definition_variables_task, - recouple_submission_variables_task, - ) - actions_chain.delay() + repopulate_reusable_definition_variables_to_form_variables.delay(form_id=form_id) def on_formstep_save_event(form_id: int, countdown: int) -> None: @@ -44,65 +32,14 @@ def on_formstep_save_event(form_id: int, countdown: int) -> None: repopulate_reusable_definition_variables_task = ( repopulate_reusable_definition_variables_to_form_variables.si(form_id) ) - recouple_submission_variables_task = ( - recouple_submission_variables_to_form_variables.si(form_id) - ) actions_chain = chain( create_form_variables_for_components_task, repopulate_reusable_definition_variables_task, - recouple_submission_variables_task, ) actions_chain.apply_async(countdown=countdown) -@app.task(ignore_result=True) -def recouple_submission_variables_to_form_variables(form_id: int) -> None: - """Recouple SubmissionValueVariable to FormVariable - - When the FormVariable bulk create/update endpoint is called, all existing FormVariable related to the form are - deleted and new are created. If there are existing submissions for this form, the SubmissionValueVariables don't - have a related FormVariable anymore. This task tries to recouple them and does the same for - other Forms in case of reusable FormDefinitions - """ - from openforms.submissions.models import SubmissionValueVariable - - from .models import FormDefinition # due to circular import - - def recouple(form): - submission_variables_to_recouple = SubmissionValueVariable.objects.filter( - form_variable__isnull=True, submission__form=form - ) - - form_variables = { - variable.key: variable for variable in form.formvariable_set.all() - } - - submission_variables_to_update = [] - for submission_variable in submission_variables_to_recouple: - if form_variable := form_variables.get(submission_variable.key): - submission_variable.form_variable = form_variable - submission_variables_to_update.append(submission_variable) - - try: - SubmissionValueVariable.objects.bulk_update( - submission_variables_to_update, fields=["form_variable"] - ) - except IntegrityError: - # Issue #1970: If the form is saved again from the form editor while this task was running, the form variables - # retrieved don't exist anymore. Another task will be scheduled from the endpoint, so nothing more to do here. - logger.info("Form variables were updated while this task was runnning.") - - recouple(Form.objects.get(id=form_id)) - - fds = FormDefinition.objects.filter(formstep__form=form_id, is_reusable=True) - other_forms = Form.objects.filter(formstep__form_definition__in=fds).exclude( - id=form_id - ) - for form in other_forms: - recouple(form) - - @app.task(ignore_result=True) @transaction.atomic() def repopulate_reusable_definition_variables_to_form_variables(form_id: int) -> None: diff --git a/src/openforms/forms/tests/variables/test_tasks.py b/src/openforms/forms/tests/variables/test_tasks.py index 0d19484d3e..112f9fe60e 100644 --- a/src/openforms/forms/tests/variables/test_tasks.py +++ b/src/openforms/forms/tests/variables/test_tasks.py @@ -1,28 +1,14 @@ -import threading -import time -from unittest.mock import patch +from django.test import TestCase, override_settings, tag -from django.db import close_old_connections -from django.db.models import Q -from django.test import TestCase, TransactionTestCase, override_settings, tag - -from openforms.forms.tasks import ( - recouple_submission_variables_to_form_variables, - repopulate_reusable_definition_variables_to_form_variables, -) from openforms.forms.tests.factories import ( FormDefinitionFactory, FormFactory, FormStepFactory, - FormVariableFactory, ) -from openforms.submissions.models import SubmissionValueVariable from openforms.submissions.tests.factories import ( SubmissionFactory, SubmissionStepFactory, - SubmissionValueVariableFactory, ) -from openforms.variables.constants import FormVariableSources from ...models import FormVariable from ...tasks import ( @@ -32,191 +18,6 @@ ) -class FormVariableTasksTest(TestCase): - def test_recouple_submission_variables(self): - form = FormFactory.create( - generate_minimal_setup=True, - formstep__form_definition__configuration={ - "components": [ - {"type": "textfield", "key": "test1"}, - {"type": "textfield", "key": "test2"}, - ] - }, - ) - form_step = form.formstep_set.first() - submission1 = SubmissionFactory.create(form=form) - submission2 = SubmissionFactory.create(form=form) - - SubmissionStepFactory.create( - submission=submission1, - form_step=form_step, - data={"test1": "some data 1", "test2": "some data 1"}, - ) - SubmissionStepFactory.create( - submission=submission2, - form_step=form_step, - data={"test1": "some other data 1", "test2": "some other data 1"}, - ) - - form.formvariable_set.all().delete() - - self.assertEqual( - 4, - SubmissionValueVariable.objects.filter( - submission__form=form, form_variable__isnull=True - ).count(), - ) - - FormVariableFactory.create(key="test1", form=form) - FormVariableFactory.create(key="test2", form=form) - - recouple_submission_variables_to_form_variables(form.id) - - self.assertEqual( - 0, - SubmissionValueVariable.objects.filter( - submission__form=form, form_variable__isnull=True - ).count(), - ) - - def test_recouple_reusable_definition_variables(self): - reusable_fd = FormDefinitionFactory.create( - is_reusable=True, - configuration={"components": [{"type": "textfield", "key": "test1"}]}, - ) - form1 = FormFactory.create( - generate_minimal_setup=True, formstep__form_definition=reusable_fd - ) - form2 = FormFactory.create( - generate_minimal_setup=True, formstep__form_definition=reusable_fd - ) - - reusable_fd.configuration = { - "components": [{"type": "textfield", "key": "test1Updated"}] - } - reusable_fd.save() - - repopulate_reusable_definition_variables_to_form_variables(form1.id) - - new_variable = form2.formvariable_set.get() - - self.assertEqual(new_variable.key, "test1Updated") - - def test_recouple_reusable_definition_variables_do_not_delete_user_defined(self): - reusable_fd = FormDefinitionFactory.create( - is_reusable=True, - configuration={"components": [{"type": "textfield", "key": "test1"}]}, - ) - form = FormFactory.create( - generate_minimal_setup=True, formstep__form_definition=reusable_fd - ) - user_defined_var = FormVariableFactory.create( - form=form, key="variable4", user_defined=True - ) - - repopulate_reusable_definition_variables_to_form_variables(form.id) - - form_user_defined_var = form.formvariable_set.get( - source=FormVariableSources.user_defined - ) - - self.assertEqual(form_user_defined_var, user_defined_var) - - -class ThreadWithExceptionHandling(threading.Thread): - def run(self): - self.exc = None - try: - super().run() - except Exception as e: - self.exc = e - - def join(self, *args, **kwargs): - super().join(*args, **kwargs) - if self.exc: - raise self.exc - - -class FormVariableRaceConditionTasksTest(TransactionTestCase): - @tag("gh-1970") - def test_recouple_submission_variables_race_condition(self): - """ - Race condition: - - The task recouple_submission_variables_to_form_variables starts, it retrieves - the form variables on a form and tries to relate them to existing submission - variables. - - While it's processing the submission vars, the form is saved again, triggering - an update of form variables. - - The task gives an integrity error when saving the submission variables because - they are being related to non-existing form variables. - """ - form = FormFactory.create( - generate_minimal_setup=True, - formstep__form_definition__configuration={ - "components": [ - {"type": "textfield", "key": "test1"}, - {"type": "textfield", "key": "test2"}, - ] - }, - ) - form_step = form.formstep_set.first() - submission = SubmissionFactory.create(form=form) - - SubmissionStepFactory.create( - submission=submission, - form_step=form_step, - data={"test1": "some data 1", "test2": "some data 1"}, - ) - - form.formvariable_set.all().delete() - - FormVariableFactory.create(key="test1", form=form) - FormVariableFactory.create(key="test2", form=form) - - def recouple_variables_task(): - real_bulk_update = SubmissionValueVariable.objects.bulk_update - - def wrapped_bulk_update(*args, **kwargs): - time.sleep(0.6) - # give some time for the delete to complete before we - # make a DB query - return real_bulk_update(*args, **kwargs) - - with patch( - "openforms.submissions.models.submission_value_variable" - ".SubmissionValueVariableManager.bulk_update", - wraps=wrapped_bulk_update, - ) as mock_bulk_update: - try: - recouple_submission_variables_to_form_variables(form.id) - finally: - close_old_connections() - mock_bulk_update.assert_called_once() - - def race_condition(): - # ensure the delete runs at some point _after_ the - # recouple_variables_thread has started - time.sleep(0.5) - try: - form.formvariable_set.all().delete() - finally: - close_old_connections() - - recouple_variables_thread = ThreadWithExceptionHandling( - target=recouple_variables_task - ) - race_condition_thread = threading.Thread(target=race_condition) - - race_condition_thread.start() - recouple_variables_thread.start() - - race_condition_thread.join() - recouple_variables_thread.join() - - @tag("gh-4824") class CreateFormVariablesForComponentsTests(TestCase): def test_create_form_variables_for_components(self): @@ -259,88 +60,6 @@ def test_create_form_variables_for_components(self): self.assertEqual(variables.count(), 1) -@override_settings(CELERY_TASK_ALWAYS_EAGER=True) -class RecoupleSubmissionVariablesToFormVariables(TestCase): - def test_recouple(self): - form = FormFactory.create() - other_form = FormFactory.create() - form_definition = FormDefinitionFactory.create( - configuration={ - "display": "form", - "components": [ - { - "key": "firstName", - "type": "textfield", - "label": "First Name", - }, - { - "key": "lastName", - "type": "textfield", - "label": "Last Name", - }, - ], - }, - is_reusable=True, - ) - form_step1 = FormStepFactory.create(form=form, form_definition=form_definition) - form_step2 = FormStepFactory.create( - form=other_form, form_definition=form_definition - ) - - submission1 = SubmissionFactory.create(form=form) - submission2 = SubmissionFactory.create(form=other_form) - - SubmissionStepFactory.create( - submission=submission1, - form_step=form_step1, - data={"firstName": "John"}, - ) - SubmissionStepFactory.create( - submission=submission2, - form_step=form_step2, - data={"firstName": "John"}, - ) - - # Task should ignore keys for which there is no FormVariable - SubmissionValueVariableFactory.create( - submission=submission1, key="non-existent", form_variable=None - ) - SubmissionValueVariable.objects.filter( - Q(submission__form=form) | Q(submission__form=other_form), - ).update(form_variable=None) - - first_name_var1 = FormVariable.objects.get(form=form, key="firstName") - first_name_var2 = FormVariable.objects.get(form=other_form, key="firstName") - - recouple_submission_variables_to_form_variables(form.id) - - with self.subTest( - "Existing submission values are recoupled with new variables" - ): - submission_vars1 = SubmissionValueVariable.objects.filter( - submission__form=form - ) - - self.assertEqual(submission_vars1.count(), 2) - - first_name_submission_var1, _ = submission_vars1.order_by("key") - - self.assertEqual(first_name_submission_var1.form_variable, first_name_var1) - - with self.subTest( - "Existing submission values for other form are recoupled with new variables" - ): - submission_vars2 = SubmissionValueVariable.objects.filter( - submission__form=other_form - ) - - self.assertEqual(submission_vars2.count(), 1) - - [first_name_submission_var2] = submission_vars2 - - self.assertEqual(first_name_submission_var2.form_variable, first_name_var2) - - @tag("gh-4824") @override_settings(CELERY_TASK_ALWAYS_EAGER=True) class OnFormStepSaveEventTests(TestCase): @@ -412,32 +131,6 @@ def test_on_formstep_save_event_fixes_broken_form_variables_state(self): self.assertEqual(first_name_var2.key, "firstName") self.assertEqual(last_name_var2.key, "lastName") - with self.subTest( - "Existing submission values are recoupled with new variables" - ): - submission_vars1 = SubmissionValueVariable.objects.filter( - submission__form=form - ) - - self.assertEqual(submission_vars1.count(), 1) - - [first_name_submission_var1] = submission_vars1 - - self.assertEqual(first_name_submission_var1.form_variable, first_name_var1) - - with self.subTest( - "Existing submission values for other form are recoupled with new variables" - ): - submission_vars2 = SubmissionValueVariable.objects.filter( - submission__form=other_form - ) - - self.assertEqual(submission_vars2.count(), 1) - - [first_name_submission_var2] = submission_vars2 - - self.assertEqual(first_name_submission_var2.form_variable, first_name_var2) - @override_settings(CELERY_TASK_ALWAYS_EAGER=True) class OnVariablesBulkUpdateEventTests(TestCase): @@ -502,29 +195,3 @@ def test_on_variables_bulk_update_event(self): self.assertEqual(first_name_var2.key, "firstName") self.assertEqual(last_name_var2.key, "lastName") - - with self.subTest( - "Existing submission values are recoupled with new variables" - ): - submission_vars1 = SubmissionValueVariable.objects.filter( - submission__form=form - ) - - self.assertEqual(submission_vars1.count(), 1) - - [first_name_submission_var1] = submission_vars1 - - self.assertEqual(first_name_submission_var1.form_variable, first_name_var1) - - with self.subTest( - "Existing submission values for other form are recoupled with new variables" - ): - submission_vars2 = SubmissionValueVariable.objects.filter( - submission__form=other_form - ) - - self.assertEqual(submission_vars2.count(), 1) - - [first_name_submission_var2] = submission_vars2 - - self.assertEqual(first_name_submission_var2.form_variable, first_name_var2) diff --git a/src/openforms/submissions/models/submission_value_variable.py b/src/openforms/submissions/models/submission_value_variable.py index 2e3722e55b..1126668d00 100644 --- a/src/openforms/submissions/models/submission_value_variable.py +++ b/src/openforms/submissions/models/submission_value_variable.py @@ -308,6 +308,11 @@ class SubmissionValueVariable(models.Model): help_text=_("The submission to which this variable value is related"), on_delete=models.CASCADE, ) + # XXX DeprecationWarning - this foreign key field actually doesn't serve any purpose + # except for some details in the to_python method. Instead, the + # submission.load_submission_value_variables_state method takes care of populating the + # state and resolving the matching form variables based on the variable key. This field + # is scheduled for removal to avoid confusion. form_variable = models.ForeignKey( to=FormVariable, verbose_name=_("form variable"), @@ -315,8 +320,6 @@ class SubmissionValueVariable(models.Model): on_delete=models.SET_NULL, # In case form definitions are edited after a user has filled in a form. null=True, ) - # Added for the case where a FormVariable has been deleted, but there is still a SubmissionValueValue. - # Having a key will help debug where the SubmissionValueValue came from key = models.TextField( verbose_name=_("key"), help_text=_("Key of the variable"), @@ -362,10 +365,6 @@ class Meta: unique_together = [["submission", "key"], ["submission", "form_variable"]] def __str__(self): - if self.form_variable: - return _("Submission value variable {name}").format( - name=self.form_variable.name - ) return _("Submission value variable {key}").format(key=self.key) def to_python(self) -> Any: @@ -382,6 +381,9 @@ def to_python(self) -> Any: return None # can't see any type information... + # FIXME: this may break when we remove the form_variable FK field. Instead, we'll + # need to look up the form variable from the related form based on the self.key + # value and resolve this in-memory. if not self.form_variable_id: return self.value From 269b0449b343cacb7a1aecb0f27172c007e0584f Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Thu, 30 Jan 2025 13:19:07 +0100 Subject: [PATCH 2/5] :bug: [#5058] Run form variable sync in same transaction Offloading this to celery results in more, separate tasks each competing for database locks and can lead to integrity errors. Changing the business logic of form variable creation/update to make sure a FormDefinition write results in a sync action gives a single trigger for these kind of changes, and it's responsible for updating all the forms that are affected by it. Since multiple FormDefinition/FormStep writes happen in parallel because of parallel client requests, we can only update or create variables and can't delete variables that are not present in our current form definition, as they may belong to another request. This shouldn't immediately cause problems, as old, unused variables don't have an 'execution path'. We piggy back on the variables bulk update endpoint/transaction to clean these up, as that call is made after all the form step persistence succeeded so we know that everything is resolved by then. The left-over variables problem only exists when updating a form step to use a different form definition. It's not relevant when a form definition is updated through the admin or a form step is added to a form. --- src/openforms/forms/admin/form_definition.py | 7 +- .../forms/api/serializers/form_step.py | 12 +- src/openforms/forms/api/viewsets.py | 1 + src/openforms/forms/models/form_variable.py | 118 ++++++++++++++++- src/openforms/forms/tasks.py | 43 ------ .../forms/tests/test_api_formsteps.py | 124 +++++++++++++----- .../forms/tests/variables/test_tasks.py | 122 +---------------- 7 files changed, 220 insertions(+), 207 deletions(-) diff --git a/src/openforms/forms/admin/form_definition.py b/src/openforms/forms/admin/form_definition.py index 9e6b80947c..4cca48fcea 100644 --- a/src/openforms/forms/admin/form_definition.py +++ b/src/openforms/forms/admin/form_definition.py @@ -1,6 +1,7 @@ from django.contrib import admin, messages from django.contrib.admin.actions import delete_selected as _delete_selected from django.db.models import Prefetch +from django.http import HttpRequest from django.urls import reverse from django.utils.html import format_html, format_html_join from django.utils.translation import gettext_lazy as _ @@ -9,7 +10,7 @@ from ...utils.expressions import FirstNotBlank from ..forms import FormDefinitionForm -from ..models import FormDefinition, FormStep +from ..models import FormDefinition, FormStep, FormVariable from .mixins import FormioConfigMixin @@ -100,3 +101,7 @@ def used_in_forms(self, obj) -> str: return format_html("", ret) used_in_forms.short_description = _("used in") + + def save_model(self, request: HttpRequest, obj: FormDefinition, form, change): + super().save_model(request=request, obj=obj, form=form, change=change) + FormVariable.objects.synchronize_for(obj) diff --git a/src/openforms/forms/api/serializers/form_step.py b/src/openforms/forms/api/serializers/form_step.py index e0d14657c0..56aa299850 100644 --- a/src/openforms/forms/api/serializers/form_step.py +++ b/src/openforms/forms/api/serializers/form_step.py @@ -1,5 +1,3 @@ -from functools import partial - from django.db import transaction from django.db.models import Q @@ -9,8 +7,7 @@ from openforms.api.serializers import PublicFieldsSerializerMixin from openforms.translations.api.serializers import ModelTranslationsSerializer -from ...models import FormDefinition, FormStep -from ...tasks import on_formstep_save_event +from ...models import FormDefinition, FormStep, FormVariable from ...validators import validate_no_duplicate_keys_across_steps from ..validators import FormStepIsApplicableIfFirstValidator from .button_text import ButtonTextSerializer @@ -174,12 +171,11 @@ def save(self, **kwargs): Bandaid fix for #4824 Ensure that the FormVariables are in line with the state of the FormDefinitions - after saving + after saving. """ instance = super().save(**kwargs) - transaction.on_commit( - partial(on_formstep_save_event, instance.form.id, countdown=60) - ) + # call this synchronously so that it's part of the same DB transaction. + FormVariable.objects.synchronize_for(instance.form_definition) return instance diff --git a/src/openforms/forms/api/viewsets.py b/src/openforms/forms/api/viewsets.py index e0a81e87e2..a164e4291e 100644 --- a/src/openforms/forms/api/viewsets.py +++ b/src/openforms/forms/api/viewsets.py @@ -472,6 +472,7 @@ def admin_message(self, request, *args, **kwargs): @transaction.atomic def variables_bulk_update(self, request, *args, **kwargs): form = self.get_object() + form_variables = form.formvariable_set.all() # We expect that all the variables that should be associated with a form come in the request. # So we can delete any existing variables because they will be replaced. diff --git a/src/openforms/forms/models/form_variable.py b/src/openforms/forms/models/form_variable.py index 240a19b498..ec135bd345 100644 --- a/src/openforms/forms/models/form_variable.py +++ b/src/openforms/forms/models/form_variable.py @@ -1,10 +1,13 @@ +from __future__ import annotations + from copy import deepcopy -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, ClassVar from django.db import models, transaction from django.db.models import CheckConstraint, Q from django.utils.translation import gettext_lazy as _ +from attr import attributes from glom import Path, glom from openforms.formio.utils import ( @@ -37,7 +40,7 @@ USER_DEFINED = Q(source=FormVariableSources.user_defined) -class FormVariableManager(models.Manager): +class FormVariableManager(models.Manager["FormVariable"]): use_in_migrations = True @transaction.atomic @@ -45,9 +48,9 @@ def create_for_form(self, form: "Form") -> None: form_steps = form.formstep_set.select_related("form_definition") for form_step in form_steps: - self.create_for_formstep(form_step) + self.synchronize_for(form_step.form_definition) - def create_for_formstep(self, form_step: "FormStep") -> list["FormVariable"]: + def create_for_formstep(self, form_step: FormStep) -> list[FormVariable]: form_definition_configuration = form_step.form_definition.configuration component_keys = [ component["key"] @@ -107,6 +110,109 @@ def create_for_formstep(self, form_step: "FormStep") -> list["FormVariable"]: return self.bulk_create(form_variables) + @transaction.atomic + def synchronize_for(self, form_definition: FormDefinition): + """ + Synchronize the form variables for a given form definition. + + This creates, updates and/or removes form variables related to the provided + :class:`FormDefinition` instance. It needs to be called whenever the Formio + configuration of a form definition is changed so that our form variables in each + form making use of the form definition accurately reflect the configuration. + + Note that we *don't* remove variables related to other form definitions, as + multiple isolated transactions for different form definitions can happen at the + same time. + """ + # Build the desired state + desired_variables: list[FormVariable] = [] + # XXX: looping over the configuration_wrapper is not (yet) viable because it + # also yields the components nested inside edit grids, which we need to ignore. + # So, we stick to iter_components. Performance wise this should be okay since we + # only need to do one pass. + configuration = form_definition.configuration + for component in iter_components(configuration=configuration, recursive=True): + # we need to ignore components that don't actually hold any values - there's + # no point to create variables for those. + if is_layout_component(component): + continue + if component["type"] in ("content", "softRequiredErrors"): + continue + if component_in_editgrid(configuration, component): + continue + + # extract options from the component + prefill_plugin = glom(component, "prefill.plugin", default="") or "" + prefill_attribute = glom(component, "prefill.attribute", default="") or "" + prefill_identifier_role = glom( + component, "prefill.identifierRole", default=IdentifierRoles.main + ) + + desired_variables.append( + self.model( + form=None, # will be set later when visiting all affected forms + key=component["key"], + form_definition=form_definition, + prefill_plugin=prefill_plugin, + prefill_attribute=prefill_attribute, + prefill_identifier_role=prefill_identifier_role, + name=component.get("label") or component["key"], + is_sensitive_data=component.get("isSensitiveData", False), + source=FormVariableSources.component, + data_type=get_component_datatype(component), + initial_value=get_component_default_value(component), + ) + ) + + desired_keys = [variable.key for variable in desired_variables] + + # if the Formio configuration of the form definition itself is updated and + # components have been removed or their keys have changed, we know for certain + # we can discard those old form variables - it doesn't matter which form they + # belong to. + stale_variables = self.filter(form_definition=form_definition).exclude( + key__in=desired_keys + ) + stale_variables.delete() + + # check which form (steps) are affected and patch them up. It is irrelevant whether + # the form definition is re-usable or not, though semantically at most one form step + # should be found for single-use form definitions. + # fmt: off + affected_form_steps = ( + form_definition + .formstep_set # pyright: ignore[reportAttributeAccessIssue] + .select_related("form") + ) + # fmt: on + + # Finally, collect all the instances and efficiently upsert them - creating missing + # variables and updating existing variables in a single query. + to_upsert: list[FormVariable] = [] + for step in affected_form_steps: + for variable in desired_variables: + form_specific_variable = deepcopy(variable) + form_specific_variable.form = step.form + to_upsert.append(form_specific_variable) + + self.bulk_create( + to_upsert, + # enables UPSERT behaviour so that existing records get updated and missing + # records inserted + update_conflicts=True, + update_fields=( + "prefill_plugin", + "prefill_attribute", + "prefill_identifier_role", + "name", + "is_sensitive_data", + "source", + "data_type", + "initial_value", + ), + unique_fields=("form", "key"), + ) + class FormVariable(models.Model): form = models.ForeignKey( @@ -206,7 +312,9 @@ class FormVariable(models.Model): _json_schema = None - objects = FormVariableManager() + objects: ClassVar[ # pyright: ignore[reportIncompatibleVariableOverride] + FormVariableManager + ] = FormVariableManager() class Meta: verbose_name = _("Form variable") diff --git a/src/openforms/forms/tasks.py b/src/openforms/forms/tasks.py index 5171ee2ce4..613acf201c 100644 --- a/src/openforms/forms/tasks.py +++ b/src/openforms/forms/tasks.py @@ -5,8 +5,6 @@ from django.db import DatabaseError, transaction from django.utils import timezone -from celery import chain - from openforms.variables.constants import FormVariableSources from ..celery import app @@ -22,24 +20,6 @@ def on_variables_bulk_update_event(form_id: int) -> None: repopulate_reusable_definition_variables_to_form_variables.delay(form_id=form_id) -def on_formstep_save_event(form_id: int, countdown: int) -> None: - """ - Celery chain of tasks to execute after saving a FormStep. - """ - create_form_variables_for_components_task = create_form_variables_for_components.si( - form_id - ) - repopulate_reusable_definition_variables_task = ( - repopulate_reusable_definition_variables_to_form_variables.si(form_id) - ) - - actions_chain = chain( - create_form_variables_for_components_task, - repopulate_reusable_definition_variables_task, - ) - actions_chain.apply_async(countdown=countdown) - - @app.task(ignore_result=True) @transaction.atomic() def repopulate_reusable_definition_variables_to_form_variables(form_id: int) -> None: @@ -67,29 +47,6 @@ def repopulate_reusable_definition_variables_to_form_variables(form_id: int) -> FormVariable.objects.create_for_form(form) -@app.task(ignore_result=True) -@transaction.atomic() -def create_form_variables_for_components(form_id: int) -> None: - """Create FormVariables for each component in the Form - - This task is scheduled after creating/updating a FormStep, to ensure that the saved - Form has the appropriate FormVariables, even if errors occur in the variables update - endpoint. This is done to avoid leaving the Form in a broken state. - - See also: https://github.com/open-formulieren/open-forms/issues/4824#issuecomment-2514913073 - """ - from .models import Form, FormVariable # due to circular import - - form = Form.objects.get(id=form_id) - - # delete the existing form variables, we will re-create them - FormVariable.objects.filter( - form=form_id, source=FormVariableSources.component - ).delete() - - FormVariable.objects.create_for_form(form) - - @app.task() def activate_forms(): """Activate all the forms that should be activated by the specific date and time.""" diff --git a/src/openforms/forms/tests/test_api_formsteps.py b/src/openforms/forms/tests/test_api_formsteps.py index f87e6d4e4a..51156f55de 100644 --- a/src/openforms/forms/tests/test_api_formsteps.py +++ b/src/openforms/forms/tests/test_api_formsteps.py @@ -15,6 +15,7 @@ TokenFactory, UserFactory, ) +from openforms.forms.models.form_variable import FormVariable from openforms.submissions.tests.factories import SubmissionFactory from ..models import FormStep @@ -856,14 +857,8 @@ def test_with_non_unique_fd_slugs(self): slugs = {form_step.slug for form_step in form_steps} self.assertEqual(len(slugs), 2) # we expect two unique slugs - @patch( - "openforms.forms.api.serializers.form_step.on_formstep_save_event", - autospec=True, - ) @tag("gh-4824") - def test_create_form_step_schedules_task_chain_to_fix_form_variables( - self, mock_on_formstep_save_event - ): + def test_create_form_step_synchronizes_form_variables(self): """ Regression test for https://github.com/open-formulieren/open-forms/issues/4824 """ @@ -891,31 +886,35 @@ def test_create_form_step_schedules_task_chain_to_fix_form_variables( "index": 0, } self.client.force_login(user=self.user) + assert not FormVariable.objects.filter(form=form).exists() - with self.captureOnCommitCallbacks(execute=True): - response = self.client.post(url, data=data) - - mock_on_formstep_save_event.assert_called_once_with(form.id, 60) + response = self.client.post(url, data=data) self.assertEqual(response.status_code, status.HTTP_201_CREATED) - self.assertEqual( - FormStep.objects.filter(form_definition=form_definition).count(), - 1, - ) + # we expect the form variables to be populated now + variables = FormVariable.objects.filter(form=form) + self.assertEqual(len(variables), 1) + variable = variables[0] + self.assertEqual(variable.key, "lastName") + self.assertEqual(variable.form_definition, form_definition) - @patch( - "openforms.forms.api.serializers.form_step.on_formstep_save_event", - autospec=True, - ) @tag("gh-4824") - def test_update_form_step_schedules_task_chain_to_fix_form_variables( - self, mock_on_formstep_save_event - ): + def test_update_form_step_synchronizes_form_variables(self): """ Regression test for https://github.com/open-formulieren/open-forms/issues/4824 """ assign_change_form_permissions(self.user) - form_step = FormStepFactory.create() + form_step = FormStepFactory.create( + form_definition__configuration={ + "components": [ + { + "key": "originalComponent", + "type": "textfield", + "label": "Original component", + } + ] + } + ) form_definition = FormDefinitionFactory.create( configuration={ "display": "form", @@ -941,17 +940,82 @@ def test_update_form_step_schedules_task_chain_to_fix_form_variables( "index": 0, } self.client.force_login(user=self.user) + # there's one existing variable from the FormStepFactory + assert FormVariable.objects.filter(form=form_step.form).count() == 1 - with self.captureOnCommitCallbacks(execute=True): - response = self.client.put(url, data=data) + response = self.client.put(url, data=data) + self.assertEqual(response.status_code, status.HTTP_200_OK) + # we expect the form variables to be populated now + variables = { + fv.key: fv for fv in FormVariable.objects.filter(form=form_step.form) + } + self.assertEqual(len(variables), 2) + self.assertIn("lastName", variables) + variable = variables["lastName"] + self.assertEqual(variable.key, "lastName") + self.assertEqual(variable.form_definition, form_definition) - mock_on_formstep_save_event.assert_called_once_with(form_step.form.id, 60) + @tag("gh-4824") + def test_form_step_save_fixes_broken_form_variables_state(self): + assign_change_form_permissions(self.user) + form = FormFactory.create() + other_form = FormFactory.create() + form_definition = FormDefinitionFactory.create( + configuration={ + "display": "form", + "components": [ + { + "key": "firstName", + "type": "textfield", + "label": "First Name", + }, + { + "key": "lastName", + "type": "textfield", + "label": "Last Name", + }, + ], + }, + is_reusable=True, + ) + form_step1 = FormStepFactory.create(form=form, form_definition=form_definition) + FormStepFactory.create(form=other_form, form_definition=form_definition) + # Bring the form in a broken state: no FormVariable exists for `lastName` + # Simulating the scenario where `lastName` was added but no variable was created + # because there are errors in the variables tab + FormVariable.objects.filter(form=form, key="lastName").delete() + FormVariable.objects.filter(form=other_form, key="lastName").delete() + self.client.force_login(user=self.user) + # updating to form 1 should also fix the other form + endpoint = reverse( + "api:form-steps-detail", + kwargs={"form_uuid_or_slug": form_step1.form.uuid, "uuid": form_step1.uuid}, + ) + data = self.client.get(endpoint).json() + + response = self.client.put(endpoint, data=data) self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual( - FormStep.objects.filter(form_definition=form_definition).count(), - 1, - ) + + with self.subTest("Form has appropriate FormVariables"): + self.assertEqual(FormVariable.objects.filter(form=form).count(), 2) + first_name_var1, last_name_var1 = FormVariable.objects.filter( + form=form + ).order_by("pk") + self.assertEqual(first_name_var1.key, "firstName") + self.assertEqual(last_name_var1.key, "lastName") + + with self.subTest( + "other Form that reuses this FormDefinition has appropriate FormVariables" + ): + self.assertEqual(FormVariable.objects.filter(form=other_form).count(), 2) + + first_name_var2, last_name_var2 = FormVariable.objects.filter( + form=other_form + ).order_by("pk") + + self.assertEqual(first_name_var2.key, "firstName") + self.assertEqual(last_name_var2.key, "lastName") class FormStepsAPITranslationTests(APITestCase): diff --git a/src/openforms/forms/tests/variables/test_tasks.py b/src/openforms/forms/tests/variables/test_tasks.py index 112f9fe60e..bdf8db0515 100644 --- a/src/openforms/forms/tests/variables/test_tasks.py +++ b/src/openforms/forms/tests/variables/test_tasks.py @@ -1,4 +1,4 @@ -from django.test import TestCase, override_settings, tag +from django.test import TestCase, override_settings from openforms.forms.tests.factories import ( FormDefinitionFactory, @@ -11,125 +11,7 @@ ) from ...models import FormVariable -from ...tasks import ( - create_form_variables_for_components, - on_formstep_save_event, - on_variables_bulk_update_event, -) - - -@tag("gh-4824") -class CreateFormVariablesForComponentsTests(TestCase): - def test_create_form_variables_for_components(self): - form = FormFactory.create() - form_definition = FormDefinitionFactory.create( - configuration={ - "display": "form", - "components": [ - { - "key": "lastName", - "type": "textfield", - "label": "Last Name", - }, - ], - } - ) - FormStepFactory.create(form=form, form_definition=form_definition) - - # Form is in a broken state, because no FormVariable exists for `lastName` - FormVariable.objects.filter(form=form).delete() - - with self.subTest("create variables for components"): - create_form_variables_for_components(form.id) - - variables = FormVariable.objects.filter(form=form) - - self.assertEqual(variables.count(), 1) - - [variable] = variables - - self.assertEqual(variable.form_definition, form_definition) - self.assertEqual(variable.source, "component") - self.assertEqual(variable.key, "lastName") - - with self.subTest("task is idempotent"): - create_form_variables_for_components(form.id) - - variables = FormVariable.objects.filter(form=form) - - self.assertEqual(variables.count(), 1) - - -@tag("gh-4824") -@override_settings(CELERY_TASK_ALWAYS_EAGER=True) -class OnFormStepSaveEventTests(TestCase): - def test_on_formstep_save_event_fixes_broken_form_variables_state(self): - form = FormFactory.create() - other_form = FormFactory.create() - form_definition = FormDefinitionFactory.create( - configuration={ - "display": "form", - "components": [ - { - "key": "firstName", - "type": "textfield", - "label": "First Name", - }, - { - "key": "lastName", - "type": "textfield", - "label": "Last Name", - }, - ], - }, - is_reusable=True, - ) - form_step1 = FormStepFactory.create(form=form, form_definition=form_definition) - form_step2 = FormStepFactory.create( - form=other_form, form_definition=form_definition - ) - - submission1 = SubmissionFactory.create(form=form) - submission2 = SubmissionFactory.create(form=other_form) - - SubmissionStepFactory.create( - submission=submission1, - form_step=form_step1, - data={"firstName": "John"}, - ) - SubmissionStepFactory.create( - submission=submission2, - form_step=form_step2, - data={"firstName": "John"}, - ) - - # Form is in a broken state, because no FormVariable exists for `lastName` - # Simulating the scenario where `lastName` was added but no variable was created - # because there are errors in the variables tab - FormVariable.objects.filter(form=form, key="lastName").delete() - FormVariable.objects.filter(form=other_form, key="lastName").delete() - - on_formstep_save_event(form.id, 60) - - with self.subTest("Form has appropriate FormVariables"): - self.assertEqual(FormVariable.objects.filter(form=form).count(), 2) - first_name_var1, last_name_var1 = FormVariable.objects.filter( - form=form - ).order_by("pk") - self.assertEqual(first_name_var1.key, "firstName") - self.assertEqual(last_name_var1.key, "lastName") - - with self.subTest( - "other Form that reuses this FormDefinition has appropriate FormVariables" - ): - self.assertEqual(FormVariable.objects.filter(form=other_form).count(), 2) - - first_name_var2, last_name_var2 = FormVariable.objects.filter( - form=other_form - ).order_by("pk") - - self.assertEqual(first_name_var2.key, "firstName") - self.assertEqual(last_name_var2.key, "lastName") +from ...tasks import on_variables_bulk_update_event @override_settings(CELERY_TASK_ALWAYS_EAGER=True) From b5ff149029c67485ed76638c8f2d714bd009a86e Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Thu, 30 Jan 2025 16:29:36 +0100 Subject: [PATCH 3/5] :white_check_mark: [#5058] Add test for expected new behaviour of bulk variable update endpoint The bulk update endpoint may no longer manage the variables for form steps, as that's taken care of by the form step endpoint now. But, user defined variables must still be managed. --- .../forms/tests/variables/test_viewset.py | 199 +++++++++++++++++- 1 file changed, 198 insertions(+), 1 deletion(-) diff --git a/src/openforms/forms/tests/variables/test_viewset.py b/src/openforms/forms/tests/variables/test_viewset.py index 279b89e8c9..07daae4eb2 100644 --- a/src/openforms/forms/tests/variables/test_viewset.py +++ b/src/openforms/forms/tests/variables/test_viewset.py @@ -2,7 +2,7 @@ from unittest.mock import patch from django.conf import settings -from django.test import override_settings +from django.test import override_settings, tag from django.utils.translation import gettext_lazy as _ from factory.django import FileField @@ -19,6 +19,7 @@ ) from openforms.forms.models import FormVariable from openforms.forms.tests.factories import ( + FormDefinitionFactory, FormFactory, FormStepFactory, FormVariableFactory, @@ -1120,3 +1121,199 @@ def test_bulk_create_and_update_with_non_camel_case_initial_values(self): self.assertEqual(status.HTTP_200_OK, response.status_code) self.assertEqual(expected_initial_value, response.json()[0]["initialValue"]) + + @tag("gh-5058", "gh-4824") + def test_form_variables_state_after_step_and_variables_submitted(self): + """ + Test that form variables are left in a consistent state at the end. + + Form variables for each step are managed by the step create/update endpoint. + After those have been submitted, the (user) defined variables are submitted, + and this call is used to bring everything in a resolved consistent state, + without any celery tasks as those lead to race conditions and deadlocks. + + Step variables submitted to this endpoint must be ignored, obsolete step + variables must be removed and user defined variables must be created or + updated. + """ + user = StaffUserFactory.create(user_permissions=["change_form"]) + self.client.force_authenticate(user) + random_fd = FormDefinitionFactory.create( + configuration={ + "components": [ + { + "key": "floating", + "type": "textfield", + "label": "Floating", + } + ] + } + ) + # start with a form with a single step + form = FormFactory.create( + generate_minimal_setup=True, + formstep__form_definition__configuration={ + "components": [ + { + "type": "textfield", + "key": "toBeRemoved", + "label": "To be removed", + }, + { + "type": "date", + "key": "toBeUpdated", + "label": "To be updated", + }, + ] + }, + ) + FormVariable.objects.create( + form=form, + source=FormVariableSources.component, + form_definition=random_fd, + key="floating", + ) + FormVariableFactory.create( + form=form, + user_defined=True, + key="staleUserDefined", + ) + assert set(form.formvariable_set.values_list("key", flat=True)) == { + "toBeRemoved", + "toBeUpdated", + "floating", + "staleUserDefined", + } + form_path = reverse("api:form-detail", kwargs={"uuid_or_slug": form.uuid}) + form_endpoint = f"http://testserver{form_path}" + step_1 = form.formstep_set.get() + # new form definition for step 1 + fd1 = FormDefinitionFactory.create( + configuration={ + "components": [ + { + "type": "number", + "key": "toBeUpdated", + "label": "To be updated", + } + ] + } + ) + fd1_detail_url = reverse("api:formdefinition-detail", kwargs={"uuid": fd1.uuid}) + # new form definition for new second step + fd2 = FormDefinitionFactory.create( + configuration={ + "components": [ + { + "type": "textfield", + "key": "toBeCreated", + "label": "To be created", + } + ] + } + ) + fd2_detail_url = reverse("api:formdefinition-detail", kwargs={"uuid": fd2.uuid}) + + # simulate the calls made by the frontend, and to humour the chaos of real + # networks, do them out of order. + create_step_endpoint = reverse( + "api:form-steps-list", kwargs={"form_uuid_or_slug": form.uuid} + ) + with self.subTest("create step 2"): + response_1 = self.client.post( + create_step_endpoint, + data={ + "formDefinition": f"http://testserver{fd2_detail_url}", + "index": 1, + }, + ) + self.assertEqual(response_1.status_code, status.HTTP_201_CREATED) + with self.subTest("update step 1"): + endpoint_2 = reverse( + "api:form-steps-detail", + kwargs={ + "form_uuid_or_slug": form.uuid, + "uuid": step_1.uuid, + }, + ) + self.client.delete(endpoint_2) + response_2 = self.client.post( + create_step_endpoint, + data={ + "formDefinition": f"http://testserver{fd1_detail_url}", + "index": 1, + }, + ) + self.assertEqual(response_2.status_code, status.HTTP_201_CREATED) + with self.subTest("push form variables"): + variables = [ + # step variables - must be ignored + { + "form": form_endpoint, + "form_definition": f"http://testserver{fd1_detail_url}", + "key": "toBeUpdated", + "name": "Ignore me", + "data_type": FormVariableDataTypes.string, + "source": FormVariableSources.component, + "initial_value": "ignore me", + }, + { + "form": form_endpoint, + "form_definition": f"http://testserver{fd2_detail_url}", + "key": "toBeCreated", + "name": "Ignore me", + "data_type": FormVariableDataTypes.date, + "source": FormVariableSources.component, + "initial_value": "ignore me", + }, + # user defined - must be created + { + "form": form_endpoint, + "form_definition": "", + "key": "userDefined", + "name": "User defined", + "data_type": FormVariableDataTypes.array, + "source": FormVariableSources.user_defined, + "initial_value": ["foo"], + }, + ] + + response = self.client.put( + reverse( + "api:form-variables", + kwargs={"uuid_or_slug": form.uuid}, + ), + data=variables, + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + + with self.subTest("assert variables state"): + form_variables = { + variable.key: variable for variable in form.formvariable_set.all() + } + # one from step 1, one from step 2 and one user defined variable + self.assertEqual(len(form_variables), 1 + 1 + 1) + + self.assertIn("toBeUpdated", form_variables) + to_be_updated = form_variables["toBeUpdated"] + self.assertEqual(to_be_updated.form_definition, fd1) + self.assertEqual(to_be_updated.name, "To be updated") + self.assertEqual(to_be_updated.data_type, FormVariableDataTypes.float) + self.assertNotEqual(to_be_updated.initial_value, "ignore me") + + self.assertIn("toBeCreated", form_variables) + to_be_created = form_variables["toBeCreated"] + self.assertEqual(to_be_created.form_definition, fd2) + self.assertEqual(to_be_created.name, "To be created") + self.assertEqual(to_be_created.data_type, FormVariableDataTypes.string) + self.assertNotEqual(to_be_created.initial_value, "ignore me") + + self.assertNotIn("toBeRemoved", form_variables) + + self.assertIn("userDefined", form_variables) + user_defined = form_variables["userDefined"] + self.assertIsNone(user_defined.form_definition) + self.assertEqual(user_defined.name, "User defined") + self.assertEqual(user_defined.data_type, FormVariableDataTypes.array) + self.assertEqual(user_defined.initial_value, ["foo"]) From 0c727fa9d416d4f69afc56384a9f940471769c36 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Thu, 30 Jan 2025 17:03:08 +0100 Subject: [PATCH 4/5] :bug: [#5058] Use the form variables bulk update for consistency Instead of triggering celery tasks, perform the variable state synchronization for a form in the bulk update endpoint. This endpoint now still validates the state of all component/user defined variables, but no longer drops all the variables and re-creates them, instead it only touches user defined variables and leaves the form step variables alone. Since this is called after the steps have been sorted out, a complete view of left-over variables from earlier form definitions is available and those can be cleaned up safely now. --- src/openforms/api/serializers.py | 6 +- .../forms/api/serializers/form_variable.py | 45 ++++++++--- src/openforms/forms/api/viewsets.py | 24 +++--- src/openforms/forms/tasks.py | 36 --------- .../forms/tests/variables/test_tasks.py | 79 ------------------ .../forms/tests/variables/test_viewset.py | 80 +------------------ 6 files changed, 58 insertions(+), 212 deletions(-) delete mode 100644 src/openforms/forms/tests/variables/test_tasks.py diff --git a/src/openforms/api/serializers.py b/src/openforms/api/serializers.py index 5311ff6bba..3131ce4108 100644 --- a/src/openforms/api/serializers.py +++ b/src/openforms/api/serializers.py @@ -1,3 +1,5 @@ +from typing import Any + from django.utils.module_loading import import_string from django.utils.translation import gettext_lazy as _ @@ -69,6 +71,7 @@ class ValidationErrorSerializer(ExceptionSerializer): class ListWithChildSerializer(serializers.ListSerializer): child_serializer_class = None # class or dotted import path + bulk_create_kwargs: dict[str, Any] | None = None def __init__(self, *args, **kwargs): child_serializer_class = self.get_child_serializer_class() @@ -94,7 +97,8 @@ def create(self, validated_data): obj = model(**data_dict) objects_to_create.append(self.process_object(obj)) - return model._default_manager.bulk_create(objects_to_create) + kwargs = self.bulk_create_kwargs or {} + return model._default_manager.bulk_create(objects_to_create, **kwargs) class PublicFieldsSerializerMixin: diff --git a/src/openforms/forms/api/serializers/form_variable.py b/src/openforms/forms/api/serializers/form_variable.py index a6a827d559..db09e5a9d5 100644 --- a/src/openforms/forms/api/serializers/form_variable.py +++ b/src/openforms/forms/api/serializers/form_variable.py @@ -17,21 +17,48 @@ from ...models import Form, FormDefinition, FormVariable +def save_fetch_config(data): + if config := data.get("service_fetch_configuration"): + config.save() + data["service_fetch_configuration"] = config.instance + return data + + class FormVariableListSerializer(ListWithChildSerializer): + bulk_create_kwargs = { + "update_conflicts": True, + "update_fields": ( + "name", + "key", + "source", + "service_fetch_configuration", + "prefill_plugin", + "prefill_attribute", + "prefill_identifier_role", + "prefill_options", + "data_type", + "data_format", + "is_sensitive_data", + "initial_value", + ), + "unique_fields": ("form", "key"), + } + def get_child_serializer_class(self): return FormVariableSerializer - def process_object(self, variable: FormVariable): - variable.check_data_type_and_initial_value() - return variable + def process_object(self, obj: FormVariable): + obj.check_data_type_and_initial_value() + return obj def preprocess_validated_data(self, validated_data): - def save_fetch_config(data): - if config := data.get("service_fetch_configuration"): - config.save() - data["service_fetch_configuration"] = config.instance - return data - + # only process not-component variables, as those are managed via the form + # step endpoint + validated_data = [ + item + for item in validated_data + if (item["source"] != FormVariableSources.component) + ] return map(save_fetch_config, validated_data) def validate(self, attrs): diff --git a/src/openforms/forms/api/viewsets.py b/src/openforms/forms/api/viewsets.py index a164e4291e..14603385c9 100644 --- a/src/openforms/forms/api/viewsets.py +++ b/src/openforms/forms/api/viewsets.py @@ -1,5 +1,4 @@ import inspect -from functools import partial from uuid import UUID from django.conf import settings @@ -28,7 +27,6 @@ from ..messages import add_success_message from ..models import Form, FormDefinition, FormStep, FormVersion -from ..tasks import on_variables_bulk_update_event from ..utils import export_form, import_form from .datastructures import FormVariableWrapper from .documentation import get_admin_fields_markdown @@ -473,11 +471,6 @@ def admin_message(self, request, *args, **kwargs): def variables_bulk_update(self, request, *args, **kwargs): form = self.get_object() - form_variables = form.formvariable_set.all() - # We expect that all the variables that should be associated with a form come in the request. - # So we can delete any existing variables because they will be replaced. - form_variables.delete() - serializer = FormVariableSerializer( data=request.data, many=True, @@ -495,9 +488,20 @@ def variables_bulk_update(self, request, *args, **kwargs): }, ) serializer.is_valid(raise_exception=True) - serializer.save() - - transaction.on_commit(partial(on_variables_bulk_update_event, form.id)) + variables = serializer.save() + + # clean up the stale variables: + # 1. component variables that are no longer related to the form + stale_component_vars = form.formvariable_set.exclude( + form_definition__formstep__form=form + ).filter(source=FormVariableSources.component) + stale_component_vars.delete() + # 2. User defined variables not present in the submitted variables + keys_to_keep = [variable.key for variable in variables] + stale_user_defined = form.formvariable_set.filter( + source=FormVariableSources.user_defined + ).exclude(key__in=keys_to_keep) + stale_user_defined.delete() return Response(serializer.data, status=status.HTTP_200_OK) diff --git a/src/openforms/forms/tasks.py b/src/openforms/forms/tasks.py index 613acf201c..3026aebe53 100644 --- a/src/openforms/forms/tasks.py +++ b/src/openforms/forms/tasks.py @@ -5,48 +5,12 @@ from django.db import DatabaseError, transaction from django.utils import timezone -from openforms.variables.constants import FormVariableSources - from ..celery import app from .models import Form logger = logging.getLogger(__name__) -def on_variables_bulk_update_event(form_id: int) -> None: - """ - Celery tasks to execute on a "bulk update of variables" event. - """ - repopulate_reusable_definition_variables_to_form_variables.delay(form_id=form_id) - - -@app.task(ignore_result=True) -@transaction.atomic() -def repopulate_reusable_definition_variables_to_form_variables(form_id: int) -> None: - """Fix inconsistencies created by updating a re-usable form definition which is used in other forms. - - When the FormVariable bulk create/update endpoint is called, all existing FormVariable related to the form are - deleted and new are created. If there are any form definitions which are reusable, we want to update all the forms - which are also using these FormDefinitions (concerning the FormVariables). This task updates the FormVariables - of each related form in the database, by replacing the old state with the newly derived set of form variables. - """ - from .models import FormDefinition, FormVariable # due to circular import - - fds = FormDefinition.objects.filter(formstep__form=form_id, is_reusable=True) - - other_forms = Form.objects.filter(formstep__form_definition__in=fds).exclude( - id=form_id - ) - - # delete the existing form variables, we will re-create them - FormVariable.objects.filter( - form__in=other_forms, source=FormVariableSources.component - ).delete() - - for form in other_forms: - FormVariable.objects.create_for_form(form) - - @app.task() def activate_forms(): """Activate all the forms that should be activated by the specific date and time.""" diff --git a/src/openforms/forms/tests/variables/test_tasks.py b/src/openforms/forms/tests/variables/test_tasks.py deleted file mode 100644 index bdf8db0515..0000000000 --- a/src/openforms/forms/tests/variables/test_tasks.py +++ /dev/null @@ -1,79 +0,0 @@ -from django.test import TestCase, override_settings - -from openforms.forms.tests.factories import ( - FormDefinitionFactory, - FormFactory, - FormStepFactory, -) -from openforms.submissions.tests.factories import ( - SubmissionFactory, - SubmissionStepFactory, -) - -from ...models import FormVariable -from ...tasks import on_variables_bulk_update_event - - -@override_settings(CELERY_TASK_ALWAYS_EAGER=True) -class OnVariablesBulkUpdateEventTests(TestCase): - def test_on_variables_bulk_update_event(self): - form = FormFactory.create() - other_form = FormFactory.create() - form_definition = FormDefinitionFactory.create( - configuration={ - "display": "form", - "components": [ - { - "key": "firstName", - "type": "textfield", - "label": "First Name", - }, - { - "key": "lastName", - "type": "textfield", - "label": "Last Name", - }, - ], - }, - is_reusable=True, - ) - form_step1 = FormStepFactory.create(form=form, form_definition=form_definition) - form_step2 = FormStepFactory.create( - form=other_form, form_definition=form_definition - ) - - submission1 = SubmissionFactory.create(form=form) - submission2 = SubmissionFactory.create(form=other_form) - - SubmissionStepFactory.create( - submission=submission1, - form_step=form_step1, - data={"firstName": "John"}, - ) - SubmissionStepFactory.create( - submission=submission2, - form_step=form_step2, - data={"firstName": "John"}, - ) - - on_variables_bulk_update_event(form.id) - - with self.subTest("Form has appropriate FormVariables"): - self.assertEqual(FormVariable.objects.filter(form=form).count(), 2) - first_name_var1, last_name_var1 = FormVariable.objects.filter( - form=form - ).order_by("pk") - self.assertEqual(first_name_var1.key, "firstName") - self.assertEqual(last_name_var1.key, "lastName") - - with self.subTest( - "other Form that reuses this FormDefinition has appropriate FormVariables" - ): - self.assertEqual(FormVariable.objects.filter(form=other_form).count(), 2) - - first_name_var2, last_name_var2 = FormVariable.objects.filter( - form=other_form - ).order_by("pk") - - self.assertEqual(first_name_var2.key, "firstName") - self.assertEqual(last_name_var2.key, "lastName") diff --git a/src/openforms/forms/tests/variables/test_viewset.py b/src/openforms/forms/tests/variables/test_viewset.py index 07daae4eb2..7c99c9ac8d 100644 --- a/src/openforms/forms/tests/variables/test_viewset.py +++ b/src/openforms/forms/tests/variables/test_viewset.py @@ -65,75 +65,6 @@ def test_staff_required(self): self.assertEqual(status.HTTP_403_FORBIDDEN, response.status_code) - def test_bulk_create_and_update(self): - user = StaffUserFactory.create(user_permissions=["change_form"]) - form = FormFactory.create() - form_step = FormStepFactory.create(form=form) - form_definition = form_step.form_definition - - form_path = reverse("api:form-detail", kwargs={"uuid_or_slug": form.uuid}) - form_url = f"http://testserver.com{form_path}" - - form_definition_path = reverse( - "api:formdefinition-detail", kwargs={"uuid": form_definition.uuid} - ) - form_definition_url = f"http://testserver.com{form_definition_path}" - - form_variable1 = FormVariableFactory.create( - form=form, form_definition=form_definition, key="variable1" - ) - FormVariableFactory.create( - form=form, form_definition=form_definition, key="variable2" - ) # This variable will be deleted - another_form_variable = ( - FormVariableFactory.create() - ) # Not related to the same form! - - data = [ - { - "form": form_url, - "form_definition": form_definition_url, - "key": form_variable1.key, - "name": "Test", - "source": form_variable1.source, - "service_fetch_configuration": None, - "data_type": form_variable1.data_type, - "initial_value": form_variable1.initial_value, - }, # Data of form_variable1 - { - "form": form_url, - "form_definition": form_definition_url, - "name": "variable3", - "key": "variable3", - "source": FormVariableSources.user_defined, - "data_type": FormVariableDataTypes.string, - "initial_value": None, - }, # New variable - ] - - self.client.force_authenticate(user) - response = self.client.put( - reverse( - "api:form-variables", - kwargs={"uuid_or_slug": form.uuid}, - ), - data=data, - ) - - self.assertEqual(status.HTTP_200_OK, response.status_code) - - variables = FormVariable.objects.all() - - self.assertEqual(3, variables.count()) - self.assertTrue(variables.filter(key=another_form_variable.key).exists()) - - form_variables = variables.filter(form=form) - - self.assertEqual(2, form_variables.count()) - self.assertTrue(form_variables.filter(key="variable1").exists()) - self.assertFalse(form_variables.filter(key="variable2").exists()) - self.assertTrue(form_variables.filter(key="variable3").exists()) - def test_it_accepts_inline_service_fetch_configs(self): designer = StaffUserFactory.create(user_permissions=["change_form"]) service = ServiceFactory.create( @@ -1074,23 +1005,18 @@ def test_bulk_create_and_update_with_non_camel_case_initial_values(self): self.client.force_authenticate(user) form = FormFactory.create(generate_minimal_setup=True) - form_definition = form.formstep_set.get().form_definition form_path = reverse("api:form-detail", kwargs={"uuid_or_slug": form.uuid}) form_url = f"http://testserver.com{form_path}" - form_definition_path = reverse( - "api:formdefinition-detail", kwargs={"uuid": form_definition.uuid} - ) - form_definition_url = f"http://testserver.com{form_definition_path}" data = [ { "form": form_url, - "form_definition": form_definition_url, - "key": form_definition.configuration["components"][0]["key"], + "form_definition": None, + "key": "someKey", + "source": FormVariableSources.user_defined, "name": "Test", "service_fetch_configuration": None, "data_type": FormVariableDataTypes.object, - "source": FormVariableSources.component, "initial_value": { "VALUE IN UPPERCASE": True, "VALUE-IN-UPPER-KEBAB-CASE": True, From 3e1c78c6001293e4480d87567fde4f1b76326c90 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Thu, 30 Jan 2025 18:04:41 +0100 Subject: [PATCH 5/5] :coffin: [#5058] Clean up code that has become obsolete We can express everything now in terms of synchronize_for form_definition. --- src/openforms/forms/models/form_variable.py | 64 +-------------------- src/openforms/forms/tests/factories.py | 4 +- src/openforms/forms/tests/test_models.py | 10 ++-- 3 files changed, 9 insertions(+), 69 deletions(-) diff --git a/src/openforms/forms/models/form_variable.py b/src/openforms/forms/models/form_variable.py index ec135bd345..d46145337d 100644 --- a/src/openforms/forms/models/form_variable.py +++ b/src/openforms/forms/models/form_variable.py @@ -7,8 +7,7 @@ from django.db.models import CheckConstraint, Q from django.utils.translation import gettext_lazy as _ -from attr import attributes -from glom import Path, glom +from glom import glom from openforms.formio.utils import ( component_in_editgrid, @@ -31,7 +30,6 @@ if TYPE_CHECKING: from .form import Form - from .form_step import FormStep EMPTY_PREFILL_PLUGIN = Q(prefill_plugin="") @@ -50,66 +48,6 @@ def create_for_form(self, form: "Form") -> None: for form_step in form_steps: self.synchronize_for(form_step.form_definition) - def create_for_formstep(self, form_step: FormStep) -> list[FormVariable]: - form_definition_configuration = form_step.form_definition.configuration - component_keys = [ - component["key"] - for component in iter_components( - configuration=form_definition_configuration, recursive=True - ) - ] - existing_form_variables_keys = form_step.form.formvariable_set.filter( - key__in=component_keys, - form_definition=form_step.form_definition, - ).values_list("key", flat=True) - - form_variables = [] - for component in iter_components( - configuration=form_definition_configuration, recursive=True - ): - if ( - is_layout_component(component) - or component["type"] in ("content", "softRequiredErrors") - or component["key"] in existing_form_variables_keys - or component_in_editgrid(form_definition_configuration, component) - ): - continue - - form_variables.append( - self.model( - form=form_step.form, - form_definition=form_step.form_definition, - prefill_plugin=glom( - component, - Path("prefill", "plugin"), - default="", - skip_exc=KeyError, - ) - or "", - prefill_attribute=glom( - component, - Path("prefill", "attribute"), - default="", - skip_exc=KeyError, - ) - or "", - prefill_identifier_role=glom( - component, - Path("prefill", "identifierRole"), - default=IdentifierRoles.main, - skip_exc=KeyError, - ), - key=component["key"], - name=component.get("label") or component["key"], - is_sensitive_data=component.get("isSensitiveData", False), - source=FormVariableSources.component, - data_type=get_component_datatype(component), - initial_value=get_component_default_value(component), - ) - ) - - return self.bulk_create(form_variables) - @transaction.atomic def synchronize_for(self, form_definition: FormDefinition): """ diff --git a/src/openforms/forms/tests/factories.py b/src/openforms/forms/tests/factories.py index 73ad058afb..8398f07d4a 100644 --- a/src/openforms/forms/tests/factories.py +++ b/src/openforms/forms/tests/factories.py @@ -188,14 +188,14 @@ def create( **kwargs, ) -> FormStep: form_step = super().create(**kwargs) - FormVariable.objects.create_for_formstep(form_step) + FormVariable.objects.synchronize_for(form_step.form_definition) return form_step @classmethod def _create(cls, *args, **kwargs): # This method is called instead of create() from the FormFactory with `generte_minimal_setup` form_step = super()._create(*args, **kwargs) - FormVariable.objects.create_for_formstep(form_step) + FormVariable.objects.synchronize_for(form_step.form_definition) return form_step diff --git a/src/openforms/forms/tests/test_models.py b/src/openforms/forms/tests/test_models.py index 1c26253483..4b82ddc25e 100644 --- a/src/openforms/forms/tests/test_models.py +++ b/src/openforms/forms/tests/test_models.py @@ -298,9 +298,10 @@ class RegressionTests(HypothesisTestCase): ) @tag("gh-3922") def test_copy_form_with_corrupt_prefill(self, component_type): - form = FormFactory.create( - generate_minimal_setup=True, - formstep__form_definition__configuration={ + # bypass the factories since those enforce DB constraints + form = FormFactory.create() + fd = FormDefinitionFactory.create( + configuration={ "components": [ { "type": component_type, @@ -313,8 +314,9 @@ def test_copy_form_with_corrupt_prefill(self, component_type): }, } ] - }, + } ) + FormStep.objects.create(form=form, form_definition=fd, order=0) form.copy()