fix(params): wrap evaluator exception surface behind ConfigurationStepEvalError

yashhzd · amilcarlucas · commit a4e62f504493 · 2026-04-20T15:21:50.000+02:00
compute_parameters wrapped the call to safe_evaluate in except (ZeroDivisionError, ValueError), which only covered math errors. Expressions could also raise: - simpleeval.NameNotDefined / InvalidExpression for undefined names and disallowed constructs - KeyError from fc_parameters['MISSING'] subscript lookups - TypeError from mismatched operand types - AttributeError, OverflowError, SyntaxError for malformed input Those fell through to a broader outer except that returned the generic "could not be computed" message for any non-math failure, and in some shapes (for example KeyError on a missing fc_parameters key) surfaced no useful detail at all. Typos in a single derived or forced parameter expression could therefore abort a step with an opaque error or, if an exception type was not in either tuple, crash the GUI. Move the exception-surface concern inside safe_evaluate itself by catching the expected set of runtime errors and re-raising a new ConfigurationStepEvalError whose message includes the original exception's class name and arguments. The original exception is preserved as __cause__ for debuggers and tracebacks. compute_parameters now catches a single domain exception (ConfigurationStepEvalError) and surfaces the wrapped message in a "could not be evaluated: <ClassName>: <details>" error, so users get the specific failure (for example "KeyError: 'DOES_NOT_EXIST'") right next to the offending parameter name. InvalidExpression is removed from the outer catch because it can no longer reach that block. Add focused unit tests in tests/test_data_model_safe_evaluator.py covering the wrapping behaviour (undefined name, missing dict key, zero division, math domain error, type error, disallowed feature) and the successful paths. Extend tests/test_backend_filesystem_configuration_steps.py with cases exercising an undefined fc_parameters key and an operand type mismatch end-to-end through compute_parameters. Closes #1526 Signed-off-by: Yash Goel <yashvardhan664@gmail.com>
diff --git a/ardupilot_methodic_configurator/backend_filesystem_configuration_steps.py b/ardupilot_methodic_configurator/backend_filesystem_configuration_steps.py
@@ -21,11 +21,10 @@
 # from logging import debug as logging_debug
 from jsonschema import validate as json_validate
 from jsonschema.exceptions import ValidationError
-from simpleeval import InvalidExpression
 
 from ardupilot_methodic_configurator import _
 from ardupilot_methodic_configurator.data_model_par_dict import Par, ParDict
-from ardupilot_methodic_configurator.data_model_safe_evaluator import safe_evaluate
+from ardupilot_methodic_configurator.data_model_safe_evaluator import ConfigurationStepEvalError, safe_evaluate
 
 
 class PhaseData(TypedDict, total=False):
@@ -204,13 +203,15 @@ def log_parameter_error(
 
                 try:
                     result = safe_evaluate(str(parameter_info["New Value"]), variables)
-                except (ZeroDivisionError, ValueError):
-                    # Handle math errors like:
-                    # - ZeroDivisionError: division by zero or 0.0 raised to negative power
-                    # - ValueError: math domain error (e.g., Diameter_inches**-0.838 when Diameter_inches is 0)
+                except ConfigurationStepEvalError as _eval_err:
+                    # safe_evaluate wraps the full exception surface (malformed
+                    # expression, undefined name, missing dict key, math error,
+                    # type mismatch, overflow) into a single domain exception so
+                    # the error can be surfaced here with useful diagnostics
+                    # without crashing the configuration-step load.
                     error_msg = _(
                         "In file '{self.configuration_steps_filename}': '{filename}' {parameter_type} "
-                        "parameter '{parameter}' evaluation resulted in math error: {math_error}"
+                        "parameter '{parameter}' could not be evaluated: {_eval_err}"
                     )
                     error_msg = error_msg.format(**locals())
                     log_parameter_error(parameter_type, ignore_fc_derived_param_warnings, errors, error_msg)
@@ -248,7 +249,7 @@ def log_parameter_error(
                     destination[filename] = ParDict()
                 change_reason = _(parameter_info["Change Reason"]) if parameter_info["Change Reason"] else ""
                 destination[filename][parameter] = Par(float(result), change_reason)
-            except (SyntaxError, NameError, KeyError, StopIteration, InvalidExpression) as _e:
+            except (SyntaxError, NameError, KeyError, StopIteration) as _e:
                 error_msg = _(
                     "In file '{self.configuration_steps_filename}': '{filename}' {parameter_type} "
                     "parameter '{parameter}' could not be computed: {_e}"
diff --git a/ardupilot_methodic_configurator/data_model_safe_evaluator.py b/ardupilot_methodic_configurator/data_model_safe_evaluator.py
@@ -13,9 +13,9 @@
 
 from math import log
 from types import MappingProxyType
-from typing import Union
+from typing import Union, cast
 
-from simpleeval import simple_eval
+from simpleeval import InvalidExpression, simple_eval
 
 SAFE_FUNCTIONS = MappingProxyType(
     {
@@ -29,6 +29,19 @@
 )
 
 
+class ConfigurationStepEvalError(Exception):
+    """
+    Raised when evaluating a configuration step expression fails.
+
+    Wraps the full exception surface of :func:`safe_evaluate` so callers
+    can catch a single domain-level exception instead of enumerating
+    every error type that simpleeval, Python arithmetic, or dict
+    lookups inside the evaluated expression can raise. The original
+    exception is preserved as ``__cause__`` and its class name is
+    included in the string form for diagnostics.
+    """
+
+
 def safe_evaluate(expression: str, variables: dict) -> Union[int, float, str]:
     """
     Evaluate a parameter expression safely using simpleeval.
@@ -46,8 +59,25 @@ def safe_evaluate(expression: str, variables: dict) -> Union[int, float, str]:
         The result of evaluating the expression.
 
     Raises:
-        InvalidExpression: If the expression uses a disallowed feature,
-            calls an unknown function, or references an undefined variable.
+        ConfigurationStepEvalError: If the expression is malformed,
+            references an undefined name or missing dict key, performs a
+            disallowed operation, or triggers a runtime error
+            (ZeroDivisionError, OverflowError, ValueError, TypeError,
+            AttributeError) while evaluating. The original exception is
+            attached as ``__cause__``.
 
     """
-    return simple_eval(expression, names=variables, functions=SAFE_FUNCTIONS)
+    try:
+        return cast("Union[int, float, str]", simple_eval(expression, names=variables, functions=SAFE_FUNCTIONS))
+    except (
+        InvalidExpression,
+        SyntaxError,
+        KeyError,
+        TypeError,
+        AttributeError,
+        ZeroDivisionError,
+        OverflowError,
+        ValueError,
+    ) as e:
+        msg = f"{type(e).__name__}: {e}"
+        raise ConfigurationStepEvalError(msg) from e
diff --git a/tests/test_backend_filesystem_configuration_steps.py b/tests/test_backend_filesystem_configuration_steps.py
@@ -99,7 +99,27 @@ def test_compute_parameters_with_invalid_expression(self) -> None:
         file_info = {"forced_parameters": {"PARAM1": {"New Value": "invalid_expression", "Change Reason": "Test reason"}}}
         variables: dict[str, dict] = {"doc_dict": {"PARAM1": {"values": {10: "value"}}}}
         result = self.config_steps.compute_parameters("test_file", file_info, "forced", variables)
-        assert "could not be computed" in result
+        assert "could not be evaluated" in result
+        assert "NameNotDefined" in result
+
+    def test_compute_parameters_with_undefined_fc_parameter_key(self) -> None:
+        """A forced expression that subscripts a missing fc_parameters key is reported cleanly."""
+        file_info = {
+            "forced_parameters": {"PARAM1": {"New Value": "fc_parameters['DOES_NOT_EXIST'] * 2", "Change Reason": "Test"}}
+        }
+        variables: dict[str, dict] = {"doc_dict": {"PARAM1": {"values": {}}}, "fc_parameters": {"OTHER": 1.0}}
+        result = self.config_steps.compute_parameters("test_file", file_info, "forced", variables)
+        assert "could not be evaluated" in result
+        assert "KeyError" in result
+        assert "DOES_NOT_EXIST" in result
+
+    def test_compute_parameters_with_type_error_in_expression(self) -> None:
+        """A forced expression that mixes incompatible operand types is reported cleanly."""
+        file_info = {"forced_parameters": {"PARAM1": {"New Value": "'abc' + 1", "Change Reason": "Test"}}}
+        variables: dict[str, dict] = {"doc_dict": {"PARAM1": {"values": {}}}}
+        result = self.config_steps.compute_parameters("test_file", file_info, "forced", variables)
+        assert "could not be evaluated" in result
+        assert "TypeError" in result
 
     def test_compute_parameters_with_missing_doc_dict(self) -> None:
         file_info = {"forced_parameters": {"PARAM1": {"New Value": "10", "Change Reason": "Test reason"}}}
diff --git a/tests/test_data_model_safe_evaluator.py b/tests/test_data_model_safe_evaluator.py
@@ -0,0 +1,137 @@
+#!/usr/bin/env python3
+
+"""
+Tests for data_model_safe_evaluator.
+
+This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator
+
+SPDX-FileCopyrightText: 2024-2026 Amilcar do Carmo Lucas <amilcar.lucas@iav.de>
+
+SPDX-License-Identifier: GPL-3.0-or-later
+"""
+
+import pytest
+from simpleeval import NameNotDefined
+
+from ardupilot_methodic_configurator.data_model_safe_evaluator import (
+    ConfigurationStepEvalError,
+    safe_evaluate,
+)
+
+
+class TestSafeEvaluateSuccess:
+    """Test that successful expressions still return the expected value."""
+
+    def test_evaluates_literal_integer(self) -> None:
+        """
+        A bare integer literal is returned unchanged.
+
+        GIVEN: The expression '42' and no variables
+        WHEN: safe_evaluate is called
+        THEN: The integer 42 is returned
+        """
+        assert safe_evaluate("42", {}) == 42
+
+    def test_evaluates_arithmetic_with_variables(self) -> None:
+        """
+        An arithmetic expression using supplied variables is evaluated correctly.
+
+        GIVEN: The expression 'a * b + c' and variables {a: 2, b: 3, c: 4}
+        WHEN: safe_evaluate is called
+        THEN: The result 10 is returned
+        """
+        assert safe_evaluate("a * b + c", {"a": 2, "b": 3, "c": 4}) == 10
+
+    def test_evaluates_fc_parameters_subscript(self) -> None:
+        """
+        Subscript access into fc_parameters returns the looked-up value.
+
+        GIVEN: An fc_parameters dict with a numeric entry
+        WHEN: safe_evaluate indexes it by key
+        THEN: The stored value is returned
+        """
+        assert safe_evaluate("fc_parameters['INS_GYRO_FILTER']", {"fc_parameters": {"INS_GYRO_FILTER": 20.0}}) == 20.0
+
+
+class TestSafeEvaluateWrapsExceptions:
+    """Test that the full exception surface is wrapped in ConfigurationStepEvalError."""
+
+    def test_wraps_undefined_name(self) -> None:
+        """
+        Referencing a name that is not in the variables dict raises ConfigurationStepEvalError.
+
+        GIVEN: An expression referencing an undefined bare name
+        WHEN: safe_evaluate is called
+        THEN: ConfigurationStepEvalError is raised
+        AND: The original NameNotDefined is preserved as __cause__
+        """
+        with pytest.raises(ConfigurationStepEvalError) as excinfo:
+            safe_evaluate("UNKNOWN_NAME + 1", {})
+        assert "NameNotDefined" in str(excinfo.value)
+        assert isinstance(excinfo.value.__cause__, NameNotDefined)
+
+    def test_wraps_missing_dict_key(self) -> None:
+        """
+        A subscript into a dict with a missing key raises ConfigurationStepEvalError.
+
+        GIVEN: fc_parameters does not contain 'MISSING'
+        WHEN: safe_evaluate tries to subscript fc_parameters['MISSING']
+        THEN: ConfigurationStepEvalError is raised
+        AND: The original KeyError is preserved as __cause__
+        AND: The missing key is mentioned in the error message
+        """
+        with pytest.raises(ConfigurationStepEvalError) as excinfo:
+            safe_evaluate("fc_parameters['MISSING']", {"fc_parameters": {}})
+        assert "KeyError" in str(excinfo.value)
+        assert "MISSING" in str(excinfo.value)
+        assert isinstance(excinfo.value.__cause__, KeyError)
+
+    def test_wraps_zero_division(self) -> None:
+        """
+        Dividing by zero inside an expression raises ConfigurationStepEvalError.
+
+        GIVEN: A denominator of zero in the expression
+        WHEN: safe_evaluate is called
+        THEN: ConfigurationStepEvalError is raised wrapping ZeroDivisionError
+        """
+        with pytest.raises(ConfigurationStepEvalError) as excinfo:
+            safe_evaluate("1 / 0", {})
+        assert "ZeroDivisionError" in str(excinfo.value)
+        assert isinstance(excinfo.value.__cause__, ZeroDivisionError)
+
+    def test_wraps_math_domain_error(self) -> None:
+        """
+        A math domain error (log of zero) raises ConfigurationStepEvalError.
+
+        GIVEN: log(0), which is mathematically undefined
+        WHEN: safe_evaluate is called
+        THEN: ConfigurationStepEvalError is raised wrapping ValueError
+        """
+        with pytest.raises(ConfigurationStepEvalError) as excinfo:
+            safe_evaluate("log(0)", {})
+        assert "ValueError" in str(excinfo.value)
+        assert isinstance(excinfo.value.__cause__, ValueError)
+
+    def test_wraps_type_error(self) -> None:
+        """
+        An operand type mismatch raises ConfigurationStepEvalError.
+
+        GIVEN: A string added to an integer
+        WHEN: safe_evaluate is called
+        THEN: ConfigurationStepEvalError is raised wrapping TypeError
+        """
+        with pytest.raises(ConfigurationStepEvalError) as excinfo:
+            safe_evaluate("'abc' + 1", {})
+        assert "TypeError" in str(excinfo.value)
+        assert isinstance(excinfo.value.__cause__, TypeError)
+
+    def test_wraps_disallowed_feature(self) -> None:
+        """
+        Using a non-whitelisted function raises ConfigurationStepEvalError.
+
+        GIVEN: An expression that calls a function not in SAFE_FUNCTIONS
+        WHEN: safe_evaluate is called
+        THEN: ConfigurationStepEvalError is raised
+        """
+        with pytest.raises(ConfigurationStepEvalError):
+            safe_evaluate("open('/etc/passwd')", {})
