From fe403d2d493eaf6cf944394fab0cb96ca2e9a0ea Mon Sep 17 00:00:00 2001 From: Ukang'a Dickson Date: Wed, 19 Aug 2026 13:49:11 +0300 Subject: [PATCH] Detect non-ambiguous dangling XPath operators --- pyxform/errors.py | 7 + pyxform/parsing/expression.py | 45 ++++++ pyxform/validators/pyxform/expression.py | 101 ++++++++++++ pyxform/xls2json.py | 22 +++ tests/parsing/test_expression.py | 77 ++++++++- tests/test_expression_validation.py | 195 +++++++++++++++++++++++ 6 files changed, 446 insertions(+), 1 deletion(-) create mode 100644 pyxform/validators/pyxform/expression.py create mode 100644 tests/test_expression_validation.py diff --git a/pyxform/errors.py b/pyxform/errors.py index 65818189..75bd6a5f 100644 --- a/pyxform/errors.py +++ b/pyxform/errors.py @@ -187,6 +187,13 @@ class ErrorCode(Enum): "questions using these types and the entity list name." ), ) + EXPRESSION_001 = Detail( + name="Expression - dangling operator", + msg=( + "[row : {row}] On the '{sheet}' sheet, the '{column}' value is invalid. " + "An operator must be followed by a value or expression." + ), + ) HEADER_001: Detail = Detail( name="Headers - invalid missing header row", msg=( diff --git a/pyxform/parsing/expression.py b/pyxform/parsing/expression.py index c43a5dac..c42be566 100644 --- a/pyxform/parsing/expression.py +++ b/pyxform/parsing/expression.py @@ -117,6 +117,51 @@ def parse_expression(text: str) -> tuple[Token, ...]: return tuple(_EXPRESSION_LEXER.lex(text)) +_OPERAND_END_TOKEN_TYPES = { + "CLOSE_PAREN", + "DATE", + "DATETIME", + "NAME", + "NUMBER", + "PARENT_REF", + "PYXFORM_REF", + "PYXFORM_REF_END", + "SELF_REF", + "SYSTEM_LITERAL", + "TIME", + "XPATH_PRED_END", +} +_WORD_OPERATORS = {"and", "div", "mod", "or"} + + +def _can_end_operand(token: Token) -> bool: + """Return whether a token can end the left operand of a word operator.""" + return token.type in _OPERAND_END_TOKEN_TYPES or ( + token.type == "OPS_MATH" and token.value == "*" + ) + + +def ends_with_dangling_operator(text: str) -> bool: + """Return whether an expression ends with an operator requiring an operand.""" + tokens = tuple( + token for token in parse_expression(text) if token.type != "WHITESPACE" + ) + if not tokens: + return False + + last_token = tokens[-1] + token_value = str(last_token) + operator = token_value.strip() + if operator in _WORD_OPERATORS: + # Operator words can also be XPath node names. They are operators only when + # preceded by something that can end a left operand. A path separator, for + # example, means the word is a node name such as the `and` in `/data/and`. + return len(tokens) > 1 and _can_end_operand(token=tokens[-2]) + if last_token.type in {"OPS_COMP", "OPS_UNION"}: + return True + return last_token.type == "OPS_MATH" and operator != "*" + + def is_xml_tag(value: str) -> bool: """Check if the input string contains only a valid XML tag / element name.""" return value and bool(RE_NCNAME_NAMESPACED.fullmatch(value)) diff --git a/pyxform/validators/pyxform/expression.py b/pyxform/validators/pyxform/expression.py new file mode 100644 index 00000000..aa290db6 --- /dev/null +++ b/pyxform/validators/pyxform/expression.py @@ -0,0 +1,101 @@ +"""Targeted validation for expressions in XLSForm workbook cells.""" + +from collections.abc import Sequence +from itertools import islice +from typing import Any + +from pyxform import aliases, constants +from pyxform.errors import ErrorCode, PyXFormError +from pyxform.parsing.expression import ends_with_dangling_operator +from pyxform.utils import default_is_dynamic + +ExpressionPath = tuple[str, ...] + +_SURVEY_EXPRESSION_PATHS = { + (constants.BIND, "relevant"), + (constants.BIND, "constraint"), + (constants.BIND, "calculate"), + (constants.BIND, "required"), + (constants.BIND, "readonly"), + (constants.CHOICE_FILTER,), + (constants.CONTROL, "jr:count"), + ("default",), +} +_SETTINGS_EXPRESSION_PATHS = {("instance_name",)} +_ENTITIES_EXPRESSION_PATHS = { + (constants.EntityColumns.ENTITY_ID.value,), + (constants.EntityColumns.CREATE_IF.value,), + (constants.EntityColumns.UPDATE_IF.value,), + (constants.EntityColumns.LABEL.value,), +} +_EXPRESSION_PATHS_BY_SHEET = { + constants.SURVEY: _SURVEY_EXPRESSION_PATHS, + constants.SETTINGS: _SETTINGS_EXPRESSION_PATHS, + constants.ENTITIES: _ENTITIES_EXPRESSION_PATHS, +} + + +def _get_source_headers( + sheet_data: Sequence[dict[str, Any]], + sheet_header: Sequence[dict[str, Any]] | None, +) -> tuple[str, ...]: + """Get original headers in the same order used by header normalization.""" + if sheet_header: + return tuple(sheet_header[0]) + + headers: dict[str, None] = {} + for row in islice(sheet_data, 0, 100): + for header in row: + headers[header] = None + return tuple(headers) + + +def _get_value(row: dict[str, Any], path: ExpressionPath) -> Any: + """Get a possibly nested value from a normalized workbook row.""" + value: Any = row + for token in path: + if not isinstance(value, dict): + return None + value = value.get(token) + return value + + +def validate_dangling_operators( + sheet_name: str, + source_sheet_data: Sequence[dict[str, Any]], + source_sheet_header: Sequence[dict[str, Any]] | None, + normalized_sheet_data: Sequence[dict[str, Any]], + normalized_headers: tuple[ExpressionPath, ...], +) -> None: + """Reject recognized expressions ending with an operator needing an operand.""" + expression_paths = _EXPRESSION_PATHS_BY_SHEET[sheet_name] + source_headers = _get_source_headers( + sheet_data=source_sheet_data, sheet_header=source_sheet_header + ) + expression_columns = tuple( + (path, source_header) + for path, source_header in zip(normalized_headers, source_headers, strict=False) + if path in expression_paths + ) + + for row_number, row in enumerate(normalized_sheet_data, start=2): + if sheet_name == constants.SURVEY and aliases.yes_no.get(row.get("disabled")): + continue + + for path, source_header in expression_columns: + value = _get_value(row=row, path=path) + if not isinstance(value, str) or not value: + continue + if path == ("default",) and not default_is_dynamic( + element_default=value, element_type=row.get(constants.TYPE) + ): + continue + if ends_with_dangling_operator(text=value): + raise PyXFormError( + code=ErrorCode.EXPRESSION_001, + context={ + "row": row_number, + "sheet": sheet_name, + "column": source_header, + }, + ) diff --git a/pyxform/xls2json.py b/pyxform/xls2json.py index 093c6d40..cbcdbf55 100644 --- a/pyxform/xls2json.py +++ b/pyxform/xls2json.py @@ -39,6 +39,7 @@ from pyxform.validators.pyxform import settings as validate_settings from pyxform.validators.pyxform.android_package_name import validate_android_package_name from pyxform.validators.pyxform.choices import validate_and_clean_choices +from pyxform.validators.pyxform.expression import validate_dangling_operators from pyxform.validators.pyxform.pyxform_reference import ( has_pyxform_reference, is_pyxform_reference, @@ -274,6 +275,13 @@ def workbook_to_json( header_aliases=aliases.settings_header, header_columns=set(Survey.get_slot_names()), ) + validate_dangling_operators( + sheet_name=constants.SETTINGS, + source_sheet_data=workbook_dict.settings, + source_sheet_header=settings_sheet_headers, + normalized_sheet_data=settings_sheet.data, + normalized_headers=settings_sheet.headers, + ) settings = settings_sheet.data[0] validate_settings.validate_name(name=settings.get(constants.NAME, None)) else: @@ -379,6 +387,13 @@ def workbook_to_json( header_aliases=aliases.entities_header, header_columns={i.value for i in constants.EntityColumns.value_list()}, ) + validate_dangling_operators( + sheet_name=constants.ENTITIES, + source_sheet_data=workbook_dict.entities, + source_sheet_header=workbook_dict.entities_header, + normalized_sheet_data=entities_sheet.data, + normalized_headers=entities_sheet.headers, + ) entity_declarations = get_entity_declarations(entities_sheet=entities_sheet.data) entity_variable_references = get_entity_variable_references( entity_declarations=entity_declarations @@ -403,6 +418,13 @@ def workbook_to_json( strip_whitespace=clean_text_values_enabled, ) survey_sheet.data = dealias_types(dict_array=survey_sheet.data) + validate_dangling_operators( + sheet_name=constants.SURVEY, + source_sheet_data=workbook_dict.survey, + source_sheet_header=workbook_dict.survey_header, + normalized_sheet_data=survey_sheet.data, + normalized_headers=survey_sheet.headers, + ) # Check for missing translations. The choices sheet is checked here so that the # warning can be combined into one message. diff --git a/tests/parsing/test_expression.py b/tests/parsing/test_expression.py index 59dc646c..c9040bf4 100644 --- a/tests/parsing/test_expression.py +++ b/tests/parsing/test_expression.py @@ -1,6 +1,10 @@ from enum import Enum -from pyxform.parsing.expression import is_xml_tag, parse_expression +from pyxform.parsing.expression import ( + ends_with_dangling_operator, + is_xml_tag, + parse_expression, +) from tests.fixtures.lexer_cases import LexerCases from tests.pyxform_test_case import PyxformTestCase @@ -410,3 +414,74 @@ def test_parse_expression(self): self.assertEqual( token_types, tuple(t.type for t in parse_expression(text=case)) ) + + def test_ends_with_dangling_operator(self): + """Should identify supported operators with varied trailing whitespace.""" + operators = ( + "=", + "!=", + "<", + ">", + "<=", + ">=", + "+", + "-", + "div", + "mod", + "and", + "or", + "|", + ) + for operator in operators: + for whitespace in ("", " ", "\t \n"): + expression = f"${{q1}} {operator}{whitespace}" + with self.subTest(expression=expression): + self.assertTrue(ends_with_dangling_operator(expression)) + + for expression in ("${q1}and", "true()and", "/data/* and"): + with self.subTest(expression=expression): + self.assertTrue(ends_with_dangling_operator(expression)) + + def test_does_not_end_with_dangling_operator(self): + """Should accept complete expressions and quoted operator characters.""" + for expression in ( + "${q1} = 1", + "${q1} != ''", + "${q1} < ${q2}", + "${q1} > 0", + "${q1} <= 5", + "${q1} >= 5", + "${q1} + 1", + "${q1} - 1", + "${q1} div 2", + "${q1} mod 2", + "${q1} and ${q2}", + "${q1} or ${q2}", + "${q1} | ${q2}", + "/data/*", + "/data/and", + "/data/or", + "/data/div", + "/data/mod", + ): + with self.subTest(expression=expression): + self.assertFalse(ends_with_dangling_operator(expression)) + + for operator in ( + "=", + "!=", + "<", + ">", + "<=", + ">=", + "+", + "-", + "div", + "mod", + "and", + "or", + "|", + ): + expression = f"'{operator}'" + with self.subTest(expression=expression): + self.assertFalse(ends_with_dangling_operator(expression)) diff --git a/tests/test_expression_validation.py b/tests/test_expression_validation.py new file mode 100644 index 00000000..b628996b --- /dev/null +++ b/tests/test_expression_validation.py @@ -0,0 +1,195 @@ +"""Tests for targeted validation of dangling XPath operators.""" + +from unittest import TestCase, mock + +from pyxform.errors import ErrorCode, PyXFormError +from pyxform.xls2xform import convert + + +class TestDanglingOperatorValidation(TestCase): + @staticmethod + def _survey_with_expression(column: str, expression: str) -> dict: + expression_row = { + "type": "text", + "name": "q2", + "label": "Q2", + column: expression, + } + if column == "repeat_count": + expression_row.update(type="begin_repeat", name="r1", label="R1") + elif column == "choice_filter": + expression_row.update(type="select_one choices") + return { + "survey": [ + {"type": "text", "name": "q1", "label": "Q1"}, + expression_row, + ] + } + + def assert_expression_error( + self, xlsform: dict, *, sheet: str, row: int, column: str + ) -> PyXFormError: + with self.assertRaises(PyXFormError) as caught: + convert(xlsform=xlsform, validate=False) + self.assertEqual(ErrorCode.EXPRESSION_001, caught.exception.code) + self.assertEqual( + ErrorCode.EXPRESSION_001.value.format(sheet=sheet, row=row, column=column), + str(caught.exception), + ) + return caught.exception + + def test_survey_expression_columns(self): + """Should validate every recognized survey expression source and its aliases.""" + for column in ( + "relevance", + "constraint", + "calculation", + "required", + "read_only", + "choice_filter", + "repeat_count", + "default", + ): + with self.subTest(column=column): + self.assert_expression_error( + self._survey_with_expression(column=column, expression="${q1} ="), + sheet="survey", + row=3, + column=column, + ) + + def test_supported_dangling_operators(self): + """Should reject every operator that unambiguously needs a right operand.""" + for operator in ( + "=", + "!=", + "<", + ">", + "<=", + ">=", + "+", + "-", + "div", + "mod", + "and", + "or", + "|", + ): + with self.subTest(operator=operator): + self.assert_expression_error( + self._survey_with_expression( + column="relevant", expression=f"${{q1}} {operator}" + ), + sheet="survey", + row=3, + column="relevant", + ) + + def test_word_operators_at_token_boundaries(self): + """Should reject word operators without spaces and after wildcard operands.""" + for expression in ("${q1}and", "true()and", "/data/* and"): + with self.subTest(expression=expression): + self.assert_expression_error( + self._survey_with_expression( + column="relevant", expression=expression + ), + sheet="survey", + row=3, + column="relevant", + ) + + def test_example_reports_original_survey_location(self): + """Should report the workbook row and original column before conversion.""" + xlsform = { + "survey": [ + {"type": "begin_group", "name": "intro_module", "label": "Intro"}, + { + "type": "begin_group", + "name": "intro_submodule", + "label": "Intro", + }, + { + "type": "select_one yes_no", + "name": "RESPConsent", + "label": "May we begin?", + }, + { + "type": "integer", + "name": "test_fail_moda", + "label": "Test", + "relevant": "${RESPConsent} =", + }, + ] + } + self.assert_expression_error(xlsform, sheet="survey", row=5, column="relevant") + + def test_settings_instance_name_with_normalized_header(self): + """Should report the original settings header after normalization.""" + xlsform = { + "settings": [{"Instance Name": "${q1} !="}], + "survey": [{"type": "text", "name": "q1", "label": "Q1"}], + } + self.assert_expression_error( + xlsform, sheet="settings", row=2, column="Instance Name" + ) + + def test_entities_expression_columns(self): + """Should validate every recognized entities expression source.""" + for column in ("entity_id", "create_if", "update_if", "label"): + with self.subTest(column=column): + xlsform = { + "entities": [{"list_name": "people", column: "${q1} <="}], + "survey": [{"type": "text", "name": "q1", "label": "Q1"}], + } + self.assert_expression_error( + xlsform, sheet="entities", row=2, column=column + ) + + def test_valid_and_non_expression_values_are_accepted(self): + """Should accept valid paths, disabled rows, and static defaults.""" + xlsform = { + "survey": [ + {"type": "text", "name": "q1", "label": "Q1"}, + { + "type": "text", + "name": "valid_comparison", + "label": "Valid", + "relevant": "${q1} = '='", + }, + { + "type": "text", + "name": "wildcard_path", + "label": "Wildcard", + "relevant": "/data/*", + }, + { + "type": "text", + "name": "operator_node_names", + "label": "Operator node names", + "relevant": "/data/and | /data/or | /data/div | /data/mod", + }, + { + "type": "text", + "name": "disabled_expression", + "label": "Disabled", + "relevant": "${q1} >", + "disabled": "yes", + }, + { + "type": "text", + "name": "static_default", + "label": "Static", + "default": "literal text =", + }, + ] + } + result = convert(xlsform=xlsform, validate=False) + self.assertIn("literal text =", result.xform) + + @mock.patch("pyxform.survey.odk_validate.check_xform") + def test_odk_validate_is_not_invoked(self, odk_validate_mock): + """Should reject the source expression before invoking ODK Validate.""" + xlsform = self._survey_with_expression(column="relevant", expression="${q1} >=") + with self.assertRaises(PyXFormError): + convert(xlsform=xlsform, validate=True) + odk_validate_mock.assert_not_called()