🚧 [#5139] More WIP

Author: viktorvanwijk

src/openforms/formio/datastructures.py

@@ -162,14 +162,12 @@ class FormioData(UserDict):
 
     data: dict[str, JSONValue]
     _keys: set[str]
-    updates: dict[str, JSONValue]
     """
     A collection of flattened key names, for quicker __contains__ access
     """
 
     def __init__(self, *args, **kwargs):
         self._keys = set()
-        self.updates = {}
         super().__init__(*args, **kwargs)
 
     def __getitem__(self, key: str):
@@ -208,6 +206,3 @@ def __contains__(self, key: object) -> bool:
             return True
         except KeyError:
             return False
-
-    def track_updates(self, m: dict[str, JSONValue]) -> None:
-        self.updates.update(m)

src/openforms/formio/service.py

@@ -61,6 +61,7 @@ def normalize_value_for_component(component: Component, value: Any) -> Any:
     return register.normalize(component, value)
 
 
+# TODO-5139: remove request as an argument from this
 @elasticapm.capture_span(span_type="app.formio")
 def get_dynamic_configuration(
     config_wrapper: FormioConfigurationWrapper,

src/openforms/submissions/form_logic.py

@@ -22,6 +22,7 @@
 
 
 # TODO-5139: replace `data` type with `FormioData`?
+# TODO-5139: replace all DataContainer type hints with FormioData
 @elasticapm.capture_span(span_type="app.submissions.logic")
 def evaluate_form_logic(
     submission: "Submission",
@@ -80,6 +81,7 @@ def evaluate_form_logic(
 
     # 3. Load the (variables) state
     submission_variables_state = submission.load_submission_value_variables_state()
+    actual_initial_data = submission_variables_state.to_python()
 
     # 4. Apply the (dirty) data to the variable state.
     submission_variables_state.set_values(data)
@@ -94,47 +96,36 @@ def evaluate_form_logic(
     # 5.1 - if the action type is to set a variable, update the variable state. This
     # happens inside of iter_evaluate_rules. This is the ONLY operation that is allowed
     # to execute while we're looping through the rules.
+    # TODO-5139: I first put this tracking of data difference inside FormioData, but
+    #  that doesn't seem like it should be its responsibility. Then yielded a complete
+    #  difference in iter_evaluate_rules, but that fails when there is no rules to be
+    #  applied: `data_diff` would be undefined. So decided to track the difference
+    #  outside iter_evaluate_rules, as we also track the mutation operations here.
+    #  Still not sure about it, though
+    data_diff_total = FormioData()
     with elasticapm.capture_span(
         name="collect_logic_operations", span_type="app.submissions.logic"
     ):
-        for operation in iter_evaluate_rules(
+        for operation, mutations in iter_evaluate_rules(
             rules,
             data_for_evaluation,
             submission=submission,
         ):
             mutation_operations.append(operation)
-
-    submission_variables_state.set_values(data_for_evaluation.updates)
-
-    # 6. The variable state is now completely resolved - we can start processing the
-    # dynamic configuration and side effects.
-
-    # Calculate which data has changed from the initial, for the step.
-    # TODO-5139: this should ideally be built up from mutation returns inside
-    #  iter_evaluate_rules
-    relevant_variables = submission_variables_state.get_variables_in_submission_step(
-        step, include_unsaved=True
-    )
-
-    updated_step_data = FormioData()
-    for key, variable in relevant_variables.items():
-        if (
-            not variable.form_variable
-            or variable.value != variable.form_variable.initial_value
-        ):
-            updated_step_data[key] = variable.value
-    step.data = DirtyData(updated_step_data.data)
+            if mutations:
+                data_diff_total.update(mutations)
 
     # 7. finally, apply the dynamic configuration
 
     # we need to apply the context-specific configurations before we can apply
     # mutations based on logic, which is then in turn passed to the serializer(s)
-    # TODO: refactor this to rely on variables state
+    # TODO-5139: rewrite `get_dynamic_configuration` and its callees to work with
+    #  FormioData instance
     config_wrapper = get_dynamic_configuration(
         config_wrapper,
         request=None,
         submission=submission,
-        data=data_for_evaluation,
+        data=data_for_evaluation.data,
     )
 
     # 7.1 Apply the component mutation operations
@@ -145,18 +136,16 @@ def evaluate_form_logic(
     # (eventually) hidden BEFORE we do any further processing. This is only a bandaid
     # fix, as the (stale) data has potentially been input for other logic rules.
     # Note that only the dirty data logic check acts on these differences.
-
-    # only keep the changes in the data, so that old values do not overwrite otherwise
-    # debounced client-side data changes
-    data_diff = FormioData()
     for component in config_wrapper:
         key = component["key"]
-        # TODO-5139: is_visible_in_frontend must accept a FormioData instance instead
+        # TODO-5139: rewrite `is_visible_in_frontend` and its callees to work with
+        #  FormioData instance
         is_visible = config_wrapper.is_visible_in_frontend(key, data_for_evaluation.data)
         if is_visible:
             continue
 
-        # Reset the value of any field that may have become hidden again after evaluating the logic
+        # Reset the value of any field that may have become hidden again after
+        # evaluating the logic
         original_value = initial_data.get(key, empty)
         empty_value = get_component_empty_value(component)
         if original_value is empty or original_value == empty_value:
@@ -166,32 +155,75 @@ def evaluate_form_logic(
             continue
 
         # clear the value
-        data_for_evaluation.update({key: empty_value})
-        data_diff[key] = empty_value
+        data_for_evaluation[key] = empty_value
+        data_diff_total[key] = empty_value
 
     # 7.2 Interpolate the component configuration with the variables.
+    # TODO-5139: rewrite `inject_variables` and its callees to work with FormioData
+    #  instance
     inject_variables(config_wrapper, data_for_evaluation.data)
 
-    # 7.3 Handle custom formio types - TODO: this needs to be lifted out of
-    # :func:`get_dynamic_configuration` so that it can use variables.
+
+    # ---------------------------------------------------------------------------------------
+
+
+    # 8. All the processing is now complete, so we can update the state.
+    # TODO-5139: when setting a value to SubmissionValueVariable.value, it doesn't
+    #  automatically get encoded to JSON with the encoder. Probably only happens when
+    #  saving the model. This means we have to do it manually here or in
+    #  SubmissionValueVariablesState.set_values. Or do we want to always convert to
+    #  python objects, like already suggested in
+    #  SubmissionValueVariableState.set_values? Issue 2324 is related to this
+    submission_variables_state.set_values(data_diff_total)
+
+    # 8.1 build a difference in data for the step. It is important that we only keep the
+    # changes in the data, so that old values do not overwrite otherwise debounced
+    # client-side data changes
+
+    # Calculate which data has changed from the initial, for the step.
+    relevant_variables = submission_variables_state.get_variables_in_submission_step(
+        step, include_unsaved=True
+    )
+
+    # TODO-5139: the problem is that currently `variable.value` can be a json or python
+    #  object, as `iter_evaluate_rules` now returns python objects. This means that a
+    #  comparison with `initial_value` will fail, because it's a json value. We could
+    #  perform this comparison in the python-type domain, but that would mean that
+    #  invalid data will not be in step.data. Is this a bad thing?
+    updated_step_data = FormioData()
+    for key, variable in relevant_variables.items():
+        if (
+            not variable.form_variable
+            or variable.value != variable.form_variable.initial_value
+        ):
+            updated_step_data[key] = variable.value
+    step.data = DirtyData(updated_step_data.data)
 
     # process the output for logic checks with dirty data
     if dirty:
-        # Iterate over all components instead of `step.data`, to take hidden fields into account (See: #1755)
+        data_diff_dirty = FormioData()
+        # Iterate over all components instead of `step.data`, to take hidden fields into
+        # account (See: #1755)
         for component in config_wrapper:
             key = component["key"]
             # already processed, don't process it again
-            if data_diff.get(key, default=empty) is not empty:
+            if data_diff_dirty.get(key, default=empty) is not empty:
                 continue
 
+            # TODO-5139: currently, this might return a python object (because we pass
+            #  python objects to the SubmissionValueVariables.set_values) whereas
+            #  previously it would return a json value. Not sure if this breaks
+            #  anything...
             new_value = updated_step_data.get(key, default=empty)
+            # TODO-5139: previously these were python objects, and they still are, so
+            #  that's ok. Or is it?
             original_value = initial_data.get(key, default=empty)
             if new_value is empty or new_value == original_value:
                 continue
-            data_diff[key] = new_value
+            data_diff_dirty[key] = new_value
 
         # only return the 'overrides'
-        step.data = DirtyData(data_diff.data)
+        step.data = DirtyData(data_diff_dirty.data)
 
     step._form_logic_evaluated = True
 
@@ -220,10 +252,13 @@ def check_submission_logic(
     data_for_evaluation = submission_variables_state.to_python()
 
     mutation_operations: list[ActionOperation] = []
-    for operation in iter_evaluate_rules(rules, data_for_evaluation, submission):
+    data_diff = FormioData({})
+    for operation, mutations in iter_evaluate_rules(rules, data_for_evaluation, submission):
         mutation_operations.append(operation)
+        if mutations:
+            data_diff.update(mutations)
 
-    submission_variables_state.set_values(data_for_evaluation.updates)
+    submission_variables_state.set_values(data_diff)
 
     # we loop over all steps because we have validations that ensure unique component
     # keys across multiple steps for the whole form.

src/openforms/submissions/logic/actions.py

@@ -175,12 +175,8 @@ def eval(
         context: DataMapping,
         submission: Submission,
     ) -> DataMapping:
-        state = submission.load_submission_value_variables_state()
-        value_variable = state.variables[self.variable]
         with log_errors(self.value, self.rule):
-            result = jsonLogic(self.value, context)
-            value_variable.to_python(result)
-            return {self.variable: value_variable.to_python(result)}
+            return {self.variable: jsonLogic(self.value, context)}
 
 
 @dataclass

src/openforms/submissions/logic/rules.py

@@ -1,6 +1,7 @@
 from typing import Iterable, Iterator
 
 import elasticapm
+from glom import mutation
 from json_logic import jsonLogic
 
 from openforms.formio.datastructures import FormioData
@@ -110,7 +111,7 @@ def iter_evaluate_rules(
     rules: Iterable[FormLogic],
     data_container: FormioData,
     submission: Submission,
-) -> Iterator[ActionOperation]:
+) -> Iterator[tuple[ActionOperation, dict]]:
     """
     Iterate over the rules and evaluate the trigger, yielding action operations.
 
@@ -128,6 +129,7 @@ def iter_evaluate_rules(
       sole argument. Useful to gather metadata about rule evaluation.
     :returns: An iterator yielding :class:`ActionOperation` instances.
     """
+    state = submission.load_submission_value_variables_state()
     for rule in rules:
         with elasticapm.capture_span(
             "evaluate_rule",
@@ -147,8 +149,12 @@ def iter_evaluate_rules(
                 if mutations := operation.eval(
                     data_container, submission=submission
                 ):
-                    # TODO-5139: not sure if FormioData should track the changes, seems
-                    #  like a task for iter_evaluate_rules itself.
-                    data_container.update(mutations)
-                    data_container.track_updates(mutations)
-                yield operation
+                    mutations_python = {
+                        key: state.variables[key].to_python(value)
+                        for key, value in mutations.items()
+                    }
+                    data_container.update(mutations_python)
+                else:
+                    mutations_python = {}
+
+                yield operation, mutations_python

src/openforms/submissions/models/submission_value_variable.py

@@ -233,7 +233,7 @@ def save_prefill_data(self, data: dict[str, Any]) -> None:
 
         SubmissionValueVariable.objects.bulk_create(variables_to_create)
 
-    def set_values(self, data: DataMapping) -> None:
+    def set_values(self, data: FormioData | DataMapping) -> None:
         """
         Apply the values from ``data`` to the current state of the variables.
 
@@ -246,7 +246,7 @@ def set_values(self, data: DataMapping) -> None:
         .. todo:: apply variable.datatype/format to obtain python objects? This also
            needs to properly serialize back to JSON though!
         """
-        formio_data = FormioData(data)
+        formio_data = data if isinstance(data, FormioData) else FormioData(data)
         for key, variable in self.variables.items():
             new_value = formio_data.get(key, default=empty)
             if new_value is empty:
@@ -377,7 +377,7 @@ class Meta:
     def __str__(self):
         return _("Submission value variable {key}").format(key=self.key)
 
-    def to_python(self, value: JSONValue = None) -> Any:
+    def to_python(self, value: JSONValue = empty) -> Any:
         """
         Deserialize the value into the appropriate python type.
 
@@ -387,7 +387,7 @@ def to_python(self, value: JSONValue = None) -> Any:
         to correctly interpret the data. For the time being, this is not needed yet
         as we focus on NL first.
         """
-        if value is None:
+        if value is empty:
             value = self.value
 
         if value is None:

src/openforms/submissions/tests/form_logic/test_submission_logic.py

@@ -1121,6 +1121,57 @@ def test_component_value_set_to_today(self):
         new_value = response.json()["step"]["data"]["date"]
         self.assertEqual(new_value, "2024-03-18")
 
+    def test_date_field_not_present_in_step_data_when_assigned_same_value(self):
+        """
+        Assert that the 'date' field is not present in the step data when a logic action
+        sets it to the same value. This illustrates the problem that occurs when setting
+        python objects directly to SubmissionValueVariable.value.
+        """
+        form = FormFactory.create()
+        form_step = FormStepFactory.create(
+            form=form,
+            form_definition__configuration={
+                "components": [
+                    {
+                        "type": "date",
+                        "key": "date",
+                        "defaultValue": "2025-01-01"
+                    },
+                ]
+            },
+        )
+
+        FormLogicFactory.create(
+            form=form,
+            json_logic_trigger=True,
+            actions=[
+                {
+                    "variable": "date",
+                    "action": {
+                        "type": "variable",
+                        "value": "2025-01-01",
+                    },
+                }
+            ],
+        )
+
+        submission = SubmissionFactory.create(form=form)
+
+        self._add_submission_to_session(submission)
+        logic_check_endpoint = reverse(
+            "api:submission-steps-logic-check",
+            kwargs={
+                "submission_uuid": submission.uuid,
+                "step_uuid": form_step.uuid,
+            },
+        )
+        response = self.client.post(logic_check_endpoint, {"data": {}})
+
+        data = response.json()
+
+        # The date hasn't changed, so it should not be present in the step data
+        self.assertNotIn("date", data["step"]["data"])
+
 
 def is_valid_expression(expr: dict):
     try: