Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deadlocks and races when making form variables state consistent #5064

Merged
merged 6 commits into from
Jan 31, 2025
Merged
Prev Previous commit
Next Next commit
🐛 [#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.
sergei-maertens committed Jan 31, 2025

Verified

This commit was signed with the committer’s verified signature.
gastaldi George Gastaldi
commit d86a1bbde94e55239f9eb238f3b5a6c33906b01f
6 changes: 5 additions & 1 deletion src/openforms/api/serializers.py
Original file line number Diff line number Diff line change
@@ -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:
45 changes: 36 additions & 9 deletions src/openforms/forms/api/serializers/form_variable.py
Original file line number Diff line number Diff line change
@@ -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):
24 changes: 14 additions & 10 deletions src/openforms/forms/api/viewsets.py
Original file line number Diff line number Diff line change
@@ -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)

36 changes: 0 additions & 36 deletions src/openforms/forms/tasks.py
Original file line number Diff line number Diff line change
@@ -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."""
79 changes: 0 additions & 79 deletions src/openforms/forms/tests/variables/test_tasks.py

This file was deleted.

80 changes: 3 additions & 77 deletions src/openforms/forms/tests/variables/test_viewset.py
Original file line number Diff line number Diff line change
@@ -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):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is covered entirely in the new test now

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,