Fix: Complex jexl issues (refactor(forms): rewrite structure and jexl evaluator) #2356

winged · 2025-01-14T15:57:44Z

refactor(forms): rewrite structure and jexl evaluator

The new structure / jexl evaluator works a bit differently: Instead of
trying to replace evaluation contexts during recursive evaluation (for
example is_hidden checks), we now have a local JEXL runtime for each
field. Also, the JEXL expressions (or their results, rather) are heavily
cached and should speed things up significantly.

Regarding the test cases:

We're trying to keep the test cases' meaning 100% unchanged - the only
modifications currently are some improved assertion messages, so
debugging becomes easier, as well as refactoring some for better readability.

Some tests are extended, and some are now better documented, to cover
more aspects and explain in more detail what our assumptions and
expectations actually are.

BREAKING CHANGE: Code that uses the form jexl and / or structure code
most likely will need to be rewritten. The changes are small-ish, but
still semantically not exactly equal.

refactor: rewrite the calculated-question code to use the new structure

The whole updating code for calculated fields was rather complex and
had quite a few subtle bugs. With the new structure, we have infrastructure
in place to build the same behaviour in a much better, more reliable way.

winged · 2025-02-21T10:25:12Z

Note to the reviewers: This is a complete rewrite of caluma_form/structure.py and caluma_form/jexl.py, so I'd suggest not looking at the diff, and just look at the code "as it were new"

luytena · 2025-02-21T12:42:29Z

caluma/caluma_form/tests/test_document.py

+    # TODO: That 1 should probably be 0 - there are no answers around
+    # in the "bare" document fixture, right?


Can be removed.

luytena · 2025-02-21T12:49:13Z

caluma/caluma_form/tests/test_jexl.py

+    # TODO: This test fails because our new _extend_context() likely doesn't properly
+    # update the chainmaps as expected


Can be removed.

luytena · 2025-02-21T13:32:28Z

caluma/caluma_form/structure.py

+            # Root field is always visible
+            return False
+
+        # do_raise = self.all_dependencies_hidden(self.question.is_hidden)


Can be removed

luytena · 2025-02-21T13:49:47Z

caluma/caluma_core/jexl.py

+        # log.info(
+        #    "JEXL: evaluating expression <<< %s >>> in context: %s",
+        #    str(expression),
+        #    str(dict(context)),
+        # )


Can be removed

nlzet

@winged as mentioned mostly some minor typo changes and a few questions to double check

nlzet · 2025-02-24T08:27:27Z

caluma/caluma_form/structure.py

+        """
+        result = []
+        for formfield in self.get_all_fields():
+            if formfield.question and formfield.slug() == slug:


formfield.slug() already checks for formfield.question presence, so check here might be redundant ?

nlzet · 2025-02-24T08:29:00Z

caluma/caluma_form/signals.py

@@ -103,7 +103,10 @@ def remove_calc_dependents(sender, instance, **kwargs):
 @filter_events(lambda instance: instance.type == models.Question.TYPE_CALCULATED_FLOAT)
 @filter_events(lambda instance: getattr(instance, "calc_expression_changed", False))
 def update_calc_from_question(sender, instance, created, update_fields, **kwargs):
-    for document in models.Document.objects.filter(form__questions=instance):
+    # TODO: we need to find documents that contain this form as a subform
+    # as well. Tis would only find documents where the question is attached


Minor typo Tis should be This ?

nlzet · 2025-02-24T08:29:08Z

caluma/caluma_form/signals.py

@@ -113,5 +116,8 @@ def update_calc_from_question(sender, instance, created, update_fields, **kwargs
    lambda instance: instance.question.type == models.Question.TYPE_CALCULATED_FLOAT
 )
 def update_calc_from_form_question(sender, instance, created, **kwargs):
-    for document in instance.form.documents.all():
+    # TODO: we need to find documents that contain this form as a subform
+    # as well. Tis would only find documents where the question is attached


Same, minor typo Tis should be This ?

nlzet · 2025-02-24T08:30:43Z

caluma/caluma_form/structure.py

+        else:
+            parent_data = None
+        return {
+            # "_type": type(self).__qualname__,


This commented line could be removed ?

nlzet · 2025-02-24T08:30:57Z

caluma/caluma_form/structure.py


-        # no value, no special handling
-        return None
+        In JEXL, the parent refers to th next field up that represents another


Minor typo th should be the ?

nlzet · 2025-02-24T09:03:06Z

caluma/caluma_form/tests/test_document.py

+):
+    """Test saving a document via the Python API.
+
+    For detailled explanation about the expected behaviour, see the docs for


Minor typo detailled should be detailed ?

nlzet · 2025-02-24T09:28:43Z

caluma/caluma_form/structure.py

        if not hasattr(self, "_memoise"):
            self._memoise = {}
+            self._memoise_hit_count = 0
+            self._memoise_miss_count = 0

        key = str([args, kwargs, method])


Should there be some extra sorting of arguments before generating the key here, in order to prevent getting a miss with the same values with a different order ?

If it were fully generic, absolutely. But it's intended for a relatively small scope, so I think it's actually fine.

nlzet · 2025-02-24T09:42:16Z

caluma/caluma_form/structure.py

+            if self.answer.date:
+                return self.answer.date


Is it safe to check for the date answer without checking the question type? Also see similar remark below

nlzet · 2025-02-24T09:50:42Z

caluma/caluma_form/structure.py

+
+    @object_local_memoise
+    def get_value(self):
+        if self.is_hidden() or self.is_empty():


I wonder if it makes sense to either:

switch the is_hidden and is_empty around

or remove is_hidden altogether

Since is_empty will already check for is_hidden

nlzet · 2025-02-24T09:54:30Z

caluma/caluma_form/structure.py

+            yield formfield
+            if isinstance(formfield, FieldSet):
+                yield from formfield.get_all_fields()
+            if isinstance(formfield, RowSet):


elif instead of if since it cannot be both I guess ?

luytena · 2025-02-24T11:13:41Z

caluma/caluma_form/jexl.py

+            else:
+                # No default arg, so we must raise an exception
+                raise QuestionMissing(
+                    f"Question `{question_slug}` could not be found in form {self.field.get_form()}"
+                )


Since the behavior has slightly changed (exception raised, instead of continuing to is_hidden check), I think it would be good to document this for the release (some jexls will need to be updated).

This leads to issues in the current forms, and it is kind of difficult to find and adapt all affected question configs.

luytena · 2025-02-24T13:33:08Z

caluma/caluma_form/structure.py

+            # TODO how is "root" expected to behave if we're *already* on root?
+            "root": self._get_root().get_local_info_context() if self.parent else None,


If I remember correctly, in other places the root points to itself / contains itself, if we're already on root

You mean the form family? Yes - but this is a bit different. I'd keep it as is here.

luytena · 2025-02-24T13:59:24Z

caluma/caluma_form/structure.py

+        # TODO: update / save answer
+        # TODO: reset caches in all dependents (calc dependents are easy, but what
+        # about the rest? Like visibility dependents etc?)


Are these still open TODOs?

luytena · 2025-02-24T14:13:37Z

caluma/caluma_form/structure.py

+        self._own_fields = {}
+
+        if parent:
+            # TODO This likely causes a circular dependency - Verify


Still an open TODO?

luytena · 2025-02-24T14:21:24Z

caluma/caluma_form/structure.py

-        field = self.fields.get(question_slug)
+    def is_required(self) -> bool:
+        # Fieldsets (in other words - subforms) should never be required.
+        # TODO: Verify this assumption


luytena · 2025-02-24T14:27:34Z

caluma/caluma_form/structure.py

-                answer=answers.get(fq.question.slug),
-                parent=self,
+        # This should already be sorted, as the context buildup
+        # is doing that for us. TODO: Verify this claim


luytena · 2025-02-24T21:12:13Z

caluma/caluma_form/validators.py

+        validation_context = validation_context or structure.FieldSet(document.family)

-        jexl = QuestionJexl(validation_context)
-        with jexl.use_field_context(
-            validation_context["structure"].get_field(question.slug)
-        ):
-            return [o.slug for o in options if not jexl.evaluate(o.is_hidden)]
+        my_field = validation_context.find_field_by_document_and_question(
+            document.pk, question.pk
+        )


During manual testing, the provided validation_context doesn't seem to always be a FieldSet. For example when passing a ValueField, the method find_field_by_document_and_question isn't found.

luytena · 2025-02-24T21:23:15Z

caluma/caluma_form/validators.py

@@ -312,165 +311,110 @@ def validate(
        if not validation_context:
            validation_context = self._validation_context(document)


Since we removed _validation_context(...), would it be get_validation_context(...)?

Table rows were sorted, but backwards; questions were not sorted at all, and thus might have lead to unpredictable behaviour. We noe explicitly sort this correctly, therefore making things a bit more testable.

Calculated questions do not work correctly when located inside a table row: The recalculation is currently triggered on the root document, which will only find one of the rows, and update that - while likely ignoring the row where the actual dependency is located. This test is intended to demonstrate the problem, and thus will currently fail.

The new structure / jexl evaluator works a bit differently: Instead of trying to replace evaluation contexts during recursive evaluation (for example `is_hidden` checks), we now have a local JEXL runtime for each field. Also, the JEXL expressions (or their results, rather) are heavily cached and should speed things up significantly. Test cases: We're trying to keep the test cases' meaning 100% unchanged - the only modifications currently are some improved assertion messages, so debugging becomes easier, as well as refactoring some for better readability. Some tests are extended, and some are now better documented, to cover more aspects and explain in more detail what our assumptions and expectations actually are. BREAKING CHANGE: Code that uses the form jexl and / or structure code most likely will need to be rewritten. The changes are small-ish, but still semantically not exactly equal.

The whole updating code for calculated fields was rather complex and had quite a few subtle bugs. With the new structure, we have infrastructure in place to build the same behaviour in a much better, more reliable way. TODO: This is currently not yet fully optimized, and we're doing quite a few more queries than before. Also TODO: A few issues were discovered that still need to be addressed - namely calculated questions not attached to a root form.

Introduce a FastLoader class that is able to preload a full document/form structure into memory, with as few and simple queries as possible. This reduces the number of DB hits the code needs to perform during document validation. Note some tests had to be fixed as well - adding family attributes to some row documents, as the fast-loader is even more picky than the structure code was about it.

This allows you, during debugging, to get the exact location of a field within a form / document structure.

winged force-pushed the complex_jexl_issues branch 3 times, most recently from 54e63bd to 2007067 Compare January 24, 2025 14:56

winged force-pushed the complex_jexl_issues branch 14 times, most recently from 15ea4c2 to 931f0f7 Compare February 18, 2025 15:15

winged requested a review from open-dynaMIX February 18, 2025 15:17

winged changed the title ~~Fix: Complex jexl issues~~ Fix: Complex jexl issues (refactor(forms): rewrite structure and jexl evaluator) Feb 18, 2025

winged requested review from czosel and nlzet February 18, 2025 15:17

luytena reviewed Feb 21, 2025

View reviewed changes

winged force-pushed the complex_jexl_issues branch from 931f0f7 to 6c03ffe Compare February 21, 2025 14:02

nlzet reviewed Feb 24, 2025

View reviewed changes

luytena reviewed Feb 25, 2025

View reviewed changes

winged force-pushed the complex_jexl_issues branch from 7fff11b to 78e60e8 Compare February 25, 2025 16:39

winged added 3 commits February 26, 2025 09:54

chore: update pre-commit config format/structure

85a4d55

fix(structure): correctly sort questions and table rows

36a6080

Table rows were sorted, but backwards; questions were not sorted at all, and thus might have lead to unpredictable behaviour. We noe explicitly sort this correctly, therefore making things a bit more testable.

chore(tests): add test case for calculated questions in tables

e92f84a

winged force-pushed the complex_jexl_issues branch 2 times, most recently from 8291232 to e039193 Compare February 26, 2025 10:02

winged added 4 commits February 26, 2025 11:20

chore(structure): add get_path() method

0c11183

This allows you, during debugging, to get the exact location of a field within a form / document structure.

winged force-pushed the complex_jexl_issues branch from e039193 to 0c11183 Compare February 26, 2025 10:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Complex jexl issues (refactor(forms): rewrite structure and jexl evaluator) #2356

Fix: Complex jexl issues (refactor(forms): rewrite structure and jexl evaluator) #2356

winged commented Jan 14, 2025 •

edited

Loading

winged commented Feb 21, 2025

luytena Feb 21, 2025

luytena Feb 21, 2025

luytena Feb 21, 2025

luytena Feb 21, 2025

nlzet left a comment

nlzet Feb 24, 2025

nlzet Feb 24, 2025

nlzet Feb 24, 2025

nlzet Feb 24, 2025

nlzet Feb 24, 2025

nlzet Feb 24, 2025

nlzet Feb 24, 2025

winged Feb 26, 2025

nlzet Feb 24, 2025

nlzet Feb 24, 2025

nlzet Feb 24, 2025

luytena Feb 24, 2025

luytena Feb 26, 2025

luytena Feb 24, 2025

winged Feb 26, 2025

luytena Feb 24, 2025

luytena Feb 24, 2025

luytena Feb 24, 2025

luytena Feb 24, 2025

luytena Feb 24, 2025

luytena Feb 24, 2025

		# TODO: That 1 should probably be 0 - there are no answers around
		# in the "bare" document fixture, right?

		# TODO: This test fails because our new _extend_context() likely doesn't properly
		# update the chainmaps as expected

		# TODO how is "root" expected to behave if we're already on root?
		"root": self._get_root().get_local_info_context() if self.parent else None,

		@@ -312,165 +311,110 @@ def validate(
		if not validation_context:
		validation_context = self._validation_context(document)

Fix: Complex jexl issues (refactor(forms): rewrite structure and jexl evaluator) #2356

Are you sure you want to change the base?

Fix: Complex jexl issues (refactor(forms): rewrite structure and jexl evaluator) #2356

Conversation

winged commented Jan 14, 2025 • edited Loading

refactor(forms): rewrite structure and jexl evaluator

refactor: rewrite the calculated-question code to use the new structure

winged commented Feb 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlzet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winged commented Jan 14, 2025 •

edited

Loading