Merge pull request #5067 from open-formulieren/backport/5058-to-30x

(backport) Fix race conditions leading to database errors

Author: sergei-maertens

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
@@ -172,12 +169,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,4 +1,7 @@
-from typing import TYPE_CHECKING
+from __future__ import annotations
+
+from copy import deepcopy
+from typing import TYPE_CHECKING, ClassVar
 
 from django.db import models, transaction
 from django.db.models import CheckConstraint, Q
@@ -31,17 +34,17 @@
 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)
+            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"]
@@ -101,6 +104,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(
@@ -198,7 +304,9 @@ class FormVariable(models.Model):
         null=True,
     )
 
-    objects = FormVariableManager()
+    objects: ClassVar[  # pyright: ignore[reportIncompatibleVariableOverride]
+        FormVariableManager
+    ] = FormVariableManager()
 
     class Meta:
         verbose_name = _("Form variable")