Merge pull request #5064 from open-formulieren/issue/5058-fix-deadloc…

…ks-and-races

Fix deadlocks and races when making form variables state consistent

.sdk-release

@@ -1 +1 @@
-3.0.0
+latest

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:

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("<ul>{}</ul>", 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)

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

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

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
@@ -472,10 +470,6 @@ 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.
-        form_variables.delete()
 
         serializer = FormVariableSerializer(
             data=request.data,
@@ -494,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)
 

src/openforms/forms/models/form_variable.py

@@ -1,11 +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 glom import Path, glom
+from glom import glom
 
 from openforms.formio.utils import (
     component_in_editgrid,
@@ -28,7 +30,6 @@
 
 if TYPE_CHECKING:
     from .form import Form
-    from .form_step import FormStep
 
 
 EMPTY_PREFILL_PLUGIN = Q(prefill_plugin="")
@@ -37,66 +38,62 @@
 USER_DEFINED = Q(source=FormVariableSources.user_defined)
 
 
-class FormVariableManager(models.Manager):
+class FormVariableManager(models.Manager["FormVariable"]):
     use_in_migrations = True
 
     @transaction.atomic
     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)
-
-    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)
-            ):
+            self.synchronize_for(form_step.form_definition)
+
+    @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
+            )
 
-            form_variables.append(
+            desired_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,
-                    ),
+                    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,
@@ -105,7 +102,54 @@ def create_for_formstep(self, form_step: "FormStep") -> list["FormVariable"]:
                 )
             )
 
-        return self.bulk_create(form_variables)
+        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):
@@ -206,7 +250,9 @@ class FormVariable(models.Model):
 
     _json_schema = None
 
-    objects = FormVariableManager()
+    objects: ClassVar[  # pyright: ignore[reportIncompatibleVariableOverride]
+        FormVariableManager
+    ] = FormVariableManager()
 
     class Meta:
         verbose_name = _("Form variable")