From 6f7ceecea58721ee7c5e3a888e693ce767b6fe6d Mon Sep 17 00:00:00 2001 From: lindsay stevens Date: Sat, 25 Jul 2026 00:24:57 +1000 Subject: [PATCH 1/5] add: nicer error when entity for update has no secondary instance - this problem is caught by ODK Validate, but the error is somewhat cryptic since it refers to instance() expressions generated by pyxform for entity attributes (as shown in the replaced test test_implicit_update_mode__instance_required__error). - xls2json.py: existing secondary_instances tracking to include the file type, update question_types/geo.py accordingly --- pyxform/entities/entities_parsing.py | 38 +++++++++-- pyxform/errors.py | 11 +++ .../validators/pyxform/question_types/geo.py | 7 +- pyxform/xls2json.py | 24 ++++--- tests/entities/test_entities.py | 68 +++++++++++++++---- 5 files changed, 120 insertions(+), 28 deletions(-) diff --git a/pyxform/entities/entities_parsing.py b/pyxform/entities/entities_parsing.py index 859b3dd3..4268c9b2 100644 --- a/pyxform/entities/entities_parsing.py +++ b/pyxform/entities/entities_parsing.py @@ -433,6 +433,16 @@ def get_entity_declaration(row: dict, row_number: int) -> dict[str, Any]: return entity +def get_entity_property(entity: dict[str, Any], name: str) -> dict[str, Any] | None: + """ + From the entity "children" list, lookup a property item by the "name" key. + + :param entity: The entity declaration to search. + :param name: The property name to look for. + """ + return next(iter(c for c in entity[const.CHILDREN] if c[const.NAME] == name), None) + + def validate_dataset_name(dataset_name: str | None, row_number: int) -> None: """ Check the dataset_name passes all naming rules. @@ -462,6 +472,29 @@ def validate_dataset_name(dataset_name: str | None, row_number: int) -> None: ) +def validate_update_dataset_references( + entity_declarations: dict[str, dict[str, Any]], + secondary_instances: set[tuple[str, str]], +) -> None: + """ + Check that the entities in update mode refer to a secondary instance. + + :param entity_declarations: The entities data `{list_name: declaration]}`. + :param secondary_instances: + :return: + """ + secondary_instance_csvs = {n for n, t in secondary_instances if t.lower() == ".csv"} + for ed in entity_declarations.values(): + update = get_entity_property(entity=ed, name="update") + if update is not None: + dataset = get_entity_property(entity=ed, name="dataset") + if dataset is not None and dataset["value"] not in secondary_instance_csvs: + raise PyXFormError( + code=ErrorCode.ENTITY_014, + context={"row": ed["__row_number"], "dataset": dataset["value"]}, + ) + + def validate_saveto( saveto: str | None, row_number: int, @@ -726,10 +759,7 @@ def inject_entities_into_json( if dataset_name and dataset_name not in entities_allocated: entity_decl = entity_declarations[dataset_name] if has_repeat_ancestor: - id_attr = next( - iter(c for c in entity_decl[const.CHILDREN] if c[const.NAME] == "id"), - None, - ) + id_attr = get_entity_property(entity=entity_decl, name="id") if id_attr and len(id_attr["actions"]) == 1: new_repeat = action.ActionLibrary.setvalue_new_repeat.value.to_dict() new_repeat["value"] = id_attr["actions"][0]["value"] diff --git a/pyxform/errors.py b/pyxform/errors.py index c5e531ce..da46fd45 100644 --- a/pyxform/errors.py +++ b/pyxform/errors.py @@ -176,6 +176,17 @@ class ErrorCode(Enum): "Please check the spelling of this 'save_to' value." ), ) + ENTITY_014 = Detail( + name="Entities - missing secondary instance for update", + msg=( + "[row : {row}] On the 'entities' sheet, the entity declaration is invalid. " + "The entity list name '{dataset}' does not match the name of a secondary instance, " + "which is required when updating entities. " + "Please either: add a question on the 'survey' sheet with the type " + "'select_*_from_file' or 'csv-external', or check the spelling of existing " + "questions using these types and the entity list name." + ), + ) HEADER_001: Detail = Detail( name="Headers - invalid missing header row", msg=( diff --git a/pyxform/validators/pyxform/question_types/geo.py b/pyxform/validators/pyxform/question_types/geo.py index c1cf3bd7..b4432927 100644 --- a/pyxform/validators/pyxform/question_types/geo.py +++ b/pyxform/validators/pyxform/question_types/geo.py @@ -21,7 +21,7 @@ def validate_parameter_incremental(value: str) -> None: def validate_parameter_reference_geometry( geo_references: Iterable[Iterable[str, int]], - secondary_instances: set[str], + secondary_instances: set[tuple[str, str]], repeat_names: set[str], choices: dict[str, list[dict]], entity_declarations: dict[str, dict[str, Any]] | None = None, @@ -35,14 +35,15 @@ def validate_parameter_reference_geometry( - last-saved usages in variables :param geo_references: Pairs of (target, source row_num) for reference_geometry usage. - :param secondary_instances: The names of valid secondary instances in the form. + :param secondary_instances: The (name, ext) of valid secondary instances in the form. :param repeat_names: Names of repeat groups in the form. :param choices: The choices data as `{list_name: [choice_items[options], ...]}`. :param entity_declarations: The entities data `{list_name: declaration]}`. """ + secondary_instance_names = {n for n, t in secondary_instances if t} for target, row_num in geo_references: if ( - target in secondary_instances + target in secondary_instance_names or target in choices or (entity_declarations and target in entity_declarations) ): diff --git a/pyxform/xls2json.py b/pyxform/xls2json.py index a1266e9f..35242547 100644 --- a/pyxform/xls2json.py +++ b/pyxform/xls2json.py @@ -21,6 +21,7 @@ get_entity_declarations, get_entity_references_by_question, get_entity_variable_references, + validate_update_dataset_references, ) from pyxform.errors import ErrorCode, PyXFormError from pyxform.parsing.expression import is_xml_tag @@ -456,7 +457,8 @@ def workbook_to_json( element_names = Counter() trigger_references: list[tuple[str, int]] = [] geo_references: list[tuple[str, int]] = [] - secondary_instances: set[str] = set() + # secondary_instances items: tuple[name, file_extension] + secondary_instances: set[tuple[str, str]] = set() repeat_names: set[str] = set() entity_references_by_question = {} @@ -887,14 +889,17 @@ def workbook_to_json( question_names.add(question_name) if row[constants.TYPE] in constants.EXTERNAL_INSTANCE_TYPES: qt_external_instance.validate_scope(row_number=row_number, stack=stack) - secondary_instances.add(os.path.splitext(question_name)[0]) + secondary_instances.add( + (question_name, f".{row[constants.TYPE].split('-')[0]}") + ) # Try to parse question as a select: select_parse = RE_SELECT.search(question_type) if select_parse: parse_dict = select_parse.groupdict() if parse_dict.get("select_command"): - select_type = aliases.select[parse_dict["select_command"]] + select_command = parse_dict["select_command"] + select_type = aliases.select[select_command] if ( select_type == constants.SELECT_ONE_EXTERNAL and constants.CHOICE_FILTER not in row @@ -924,8 +929,10 @@ def workbook_to_json( + "List name not in external choices sheet: " + list_name ) + elif select_command in aliases.select_from_file: + secondary_instances.add((instance_name, file_extension)) else: - secondary_instances.add(instance_name) + secondary_instances.add((instance_name, "")) select_from_file.validate_list_name_extension( select_command=parse_dict["select_command"], @@ -1029,10 +1036,7 @@ def workbook_to_json( new_json_dict = row.copy() new_json_dict[constants.TYPE] = select_type - if parse_dict["select_command"] in { - "select_one_from_file", - "select_multiple_from_file", - }: + if select_command in aliases.select_from_file: qt_params = constants.ParametersSelectFromFile pv.validate( parameters=parameters, @@ -1372,6 +1376,10 @@ def workbook_to_json( ) if entity_declarations: + validate_update_dataset_references( + entity_declarations=entity_declarations, + secondary_instances=secondary_instances, + ) apply_entities_declarations( entity_declarations=entity_declarations, entity_references_by_question=entity_references_by_question, diff --git a/tests/entities/test_entities.py b/tests/entities/test_entities.py index 2a6055a3..ecb8b3b4 100644 --- a/tests/entities/test_entities.py +++ b/tests/entities/test_entities.py @@ -38,6 +38,7 @@ - EV022: save_to name invalid reserved names error - EV023: save_to name invalid underscore prefix error - EV024: Entity name missing error + - EV025: Entity name does not match secondary instance error - Behaviour - EB001: Dataset column alias - EB002: implicit entity_id=0, create_if=0, update_if=0 (create) @@ -61,7 +62,8 @@ - EB021: Allocation to survey meta is compatible with other meta settings - EB022: Allocation searches path ancestors only (not children or siblings) - EB023: Allocation selects deepest boundary scope (pyxform/#822) - - EB024: ALlocation is to survey for only one entity not in repeats (pyxform/#825) + - EB024: Allocation is to survey for only one entity not in repeats (pyxform/#825) + - EB025: Update instance csv can be declared with select*from_file or csv-external ## Topological constraint solver regression suite @@ -2119,6 +2121,42 @@ def test_no_allocations__multiple_entity__no_sibling_search__error(self): ], ) + def test_update_mode__missing_secondary_instance__none__error(self): + """Should find that when an update mode, an instance for the entity is required.""" + # ES004 EB006 EB015 EB019 EV025 + md = """ + | survey | + | | type | name | label | + | | text | q1 | Q1 | + + | entities | + | | list_name | entity_id | + | | e1 | ${q1} | + """ + self.assertPyxformXform( + md=md, + errored=True, + error__contains=[ErrorCode.ENTITY_014.value.format(row="2", dataset="e1")], + ) + + def test_update_mode__missing_secondary_instance__misspelling__error(self): + """Should find that when an update mode, an instance for the entity is required.""" + # ES004 EB006 EB015 EB019 EV025 + md = """ + | survey | + | | type | name | label | + | | select_one_from_file e1s.csv | q1 | Q1 | + + | entities | + | | list_name | entity_id | + | | e1 | ${q1} | + """ + self.assertPyxformXform( + md=md, + errored=True, + error__contains=[ErrorCode.ENTITY_014.value.format(row="2", dataset="e1")], + ) + class TestEntitiesOutput(PyxformTestCase): def test_namespace__entities_not_used__not_exists(self): @@ -2479,13 +2517,13 @@ def test_implicit_create_mode__create_if__repeat(self): ], ) - def test_implicit_update_mode__instance_required__error(self): - """Should find that when an update mode, an instance for the entity is required.""" - # ES004 EB006 EB012 EB014 EB015 EB019 + def test_implicit_update_mode__entity_id__survey__from_file(self): + """Should find that when an entity_id is provided, the entity is in update mode.""" + # ES004 EB006 EB012 EB014 EB015 EB019 EB025 md = """ | survey | - | | type | name | label | - | | text | q1 | Q1 | + | | type | name | label | + | | select_one_from_file e1.csv | q1 | Q1 | | entities | | | list_name | entity_id | @@ -2493,18 +2531,22 @@ def test_implicit_update_mode__instance_required__error(self): """ self.assertPyxformXform( md=md, - run_odk_validate=True, - odk_validate_error__contains=[ - "Error evaluating field", - "The problem was located in Calculate expression for ${entity}", - "XPath evaluation: Instance referenced by instance(e1)/root", - "does not exist", + xml__xpath_match=[ + xpe.model_bind_meta_instanceid(), + xpe.model_instance_meta("e1", update=True), + xpe.model_bind_meta_id(" /test_name/q1 "), + xpe.model_bind_meta_baseversion("e1", "/test_name/q1"), + xpe.model_bind_meta_trunkversion("e1", "/test_name/q1"), + xpe.model_bind_meta_branchid("e1", "/test_name/q1"), + ], + xml__xpath_count=[ + ("/h:html//x:setvalue", 0), ], ) def test_implicit_update_mode__entity_id__survey(self): """Should find that when an entity_id is provided, the entity is in update mode.""" - # ES004 EB006 EB012 EB014 EB015 EB019 + # ES004 EB006 EB012 EB014 EB015 EB019 EB025 md = """ | survey | | | type | name | label | From 04ab0881c42b59f253d25d0de4c4a5a3dfbc5031 Mon Sep 17 00:00:00 2001 From: lindsay stevens Date: Sat, 25 Jul 2026 00:58:30 +1000 Subject: [PATCH 2/5] chg: refactor selects validation/processing into type branches - previously the code seemed to be repetitively and inconsistently identifying the select types, so now there's 3 distinct branches: external selects, internal selects, select_from_file. --- .../validators/pyxform/select_from_file.py | 3 +- pyxform/xls2json.py | 78 +++++++++---------- 2 files changed, 38 insertions(+), 43 deletions(-) diff --git a/pyxform/validators/pyxform/select_from_file.py b/pyxform/validators/pyxform/select_from_file.py index 1009a88b..7bd694fe 100644 --- a/pyxform/validators/pyxform/select_from_file.py +++ b/pyxform/validators/pyxform/select_from_file.py @@ -1,6 +1,5 @@ from pathlib import Path -from pyxform import aliases from pyxform import constants as co from pyxform.constants import EXTERNAL_INSTANCE_EXTENSIONS, ROW_FORMAT_STRING from pyxform.errors import ErrorCode, PyXFormError @@ -35,7 +34,7 @@ def validate_list_name_extension( ) -> None: """For select_from_file types, the list_name should end with a supported extension.""" list_path = Path(list_name) - if select_command in aliases.select_from_file and ( + if ( 1 != len(list_path.suffixes) or list_path.suffix not in EXTERNAL_INSTANCE_EXTENSIONS ): diff --git a/pyxform/xls2json.py b/pyxform/xls2json.py index 35242547..eb615998 100644 --- a/pyxform/xls2json.py +++ b/pyxform/xls2json.py @@ -900,18 +900,16 @@ def workbook_to_json( if parse_dict.get("select_command"): select_command = parse_dict["select_command"] select_type = aliases.select[select_command] - if ( - select_type == constants.SELECT_ONE_EXTERNAL - and constants.CHOICE_FILTER not in row - ): - warnings.append( - ROW_FORMAT_STRING % row_number - + " select one external is only meant for filtered selects." - ) list_name = parse_dict[constants.LIST_NAME_U] instance_name, file_extension = os.path.splitext(list_name) + # Validate external selects. if select_type == constants.SELECT_ONE_EXTERNAL: + if constants.CHOICE_FILTER not in row: + warnings.append( + ROW_FORMAT_STRING % row_number + + " select one external is only meant for filtered selects." + ) if not external_choices: k = constants.EXTERNAL_CHOICES msg = "There should be an external_choices sheet in this xlsform." @@ -929,20 +927,11 @@ def workbook_to_json( + "List name not in external choices sheet: " + list_name ) - elif select_command in aliases.select_from_file: - secondary_instances.add((instance_name, file_extension)) - else: - secondary_instances.add((instance_name, "")) - select_from_file.validate_list_name_extension( - select_command=parse_dict["select_command"], - list_name=list_name, - row_number=row_number, - ) + # Validate internal selects. if ( - list_name not in choices - and select_type != constants.SELECT_ONE_EXTERNAL - and file_extension not in EXTERNAL_INSTANCE_EXTENSIONS + select_type != constants.SELECT_ONE_EXTERNAL + and select_command not in aliases.select_from_file and not has_pyxform_reference(list_name) ): if not choices: @@ -955,28 +944,28 @@ def workbook_to_json( f"{msg} Please ensure that the choices sheet has the" " mandatory columns 'list_name', 'name', and 'label'." ) - raise PyXFormError( - ROW_FORMAT_STRING % row_number - + " List name not in choices sheet: " - + list_name - ) + elif list_name not in choices: + raise PyXFormError( + ROW_FORMAT_STRING % row_number + + " List name not in choices sheet: " + + list_name + ) - # Validate select_multiple choice names by making sure - # they have no spaces (will cause errors in exports). - if ( - select_type == constants.SELECT_ALL_THAT_APPLY - and file_extension not in EXTERNAL_INSTANCE_EXTENSIONS - ): - for choice in choices[list_name]: - if " " in choice[constants.NAME]: - raise PyXFormError( - "Choice names with spaces cannot be added " - "to multiple choice selects. See [" - + choice[constants.NAME] - + "] in [" - + list_name - + "]" - ) + # Validate select_multiple choice names by making sure + # they have no spaces (will cause errors in exports). + if select_type == constants.SELECT_ALL_THAT_APPLY: + for choice in choices[list_name]: + if " " in choice[constants.NAME]: + raise PyXFormError( + "Choice names with spaces cannot be added " + "to multiple choice selects. See [" + + choice[constants.NAME] + + "] in [" + + list_name + + "]" + ) + # Track the secondary instance. + secondary_instances.add((instance_name, "")) specify_other_question = None if parse_dict.get("specify_other") is not None: @@ -1037,6 +1026,11 @@ def workbook_to_json( new_json_dict[constants.TYPE] = select_type if select_command in aliases.select_from_file: + select_from_file.validate_list_name_extension( + select_command=select_command, + list_name=list_name, + row_number=row_number, + ) qt_params = constants.ParametersSelectFromFile pv.validate( parameters=parameters, @@ -1055,6 +1049,8 @@ def workbook_to_json( value=parameters[qt_params.LABEL], row_number=row_number, ) + # Track the secondary instance. + secondary_instances.add((instance_name, file_extension)) else: qt_params = constants.ParametersSelect pv.validate( From c1e273b781a53c77a1d0fa22b4b8df7695980e49 Mon Sep 17 00:00:00 2001 From: lindsay stevens Date: Sat, 25 Jul 2026 01:29:24 +1000 Subject: [PATCH 3/5] chg: replace missing list_name error with detailed errorcode message - now suggests adding a list or checking spelling. - it seems there was no existing test for the old message so new tests are added for the pass/fail cases. --- pyxform/errors.py | 10 ++++++ pyxform/xls2json.py | 4 +-- tests/question_types/test_select_internal.py | 33 ++++++++++++++++++++ 3 files changed, 44 insertions(+), 3 deletions(-) create mode 100644 tests/question_types/test_select_internal.py diff --git a/pyxform/errors.py b/pyxform/errors.py index da46fd45..65818189 100644 --- a/pyxform/errors.py +++ b/pyxform/errors.py @@ -369,6 +369,16 @@ class ErrorCode(Enum): "Entity lists must have a name." ), ) + NAMES_016 = Detail( + name="Names - select list_name not found on choices sheet", + msg=( + "[row : {row}] On the 'survey' sheet, the 'type' value is invalid. " + "The select list name was not found in the 'choices' sheet. " + "Please add one or more rows to the 'choices' sheet for this list_name, or " + "check the spelling of the list name in the 'type' column and existing " + "choices 'list_name' rows." + ), + ) PYREF_001: Detail = Detail( name="PyXForm reference - parsing failed", msg=( diff --git a/pyxform/xls2json.py b/pyxform/xls2json.py index eb615998..093c6d40 100644 --- a/pyxform/xls2json.py +++ b/pyxform/xls2json.py @@ -946,9 +946,7 @@ def workbook_to_json( ) elif list_name not in choices: raise PyXFormError( - ROW_FORMAT_STRING % row_number - + " List name not in choices sheet: " - + list_name + code=ErrorCode.NAMES_016, context={"row": row_number} ) # Validate select_multiple choice names by making sure diff --git a/tests/question_types/test_select_internal.py b/tests/question_types/test_select_internal.py new file mode 100644 index 00000000..b383eb2c --- /dev/null +++ b/tests/question_types/test_select_internal.py @@ -0,0 +1,33 @@ +from pyxform.errors import ErrorCode + +from tests.pyxform_test_case import PyxformTestCase + + +class TestSelectInternalParsing(PyxformTestCase): + def test_select_list_name__match__ok(self): + """Should not raise an error if the select list is found in the choices sheet.""" + md = """ + | survey | + | | type | name | label | + | | select_one c1 | q1 | Q1 | + + | choices | + | | list_name | name | label | + | | c1 | n1 | N1 | + """ + self.assertPyxformXform(md=md, warnings_count=0) + + def test_select_list_name__missing__error(self): + """Should raise an error if the select list is not found in the choices sheet.""" + md = """ + | survey | + | | type | name | label | + | | select_one c2 | q1 | Q1 | + + | choices | + | | list_name | name | label | + | | c1 | n1 | N1 | + """ + self.assertPyxformXform( + md=md, errored=True, error__contains=[ErrorCode.NAMES_016.value.format(row=2)] + ) From 98882dd5c0dce0acb530812f1ac959911902993d Mon Sep 17 00:00:00 2001 From: lindsay stevens Date: Thu, 6 Aug 2026 12:19:52 +1000 Subject: [PATCH 4/5] fix: update assertion in test added in concurrent remove-xls branch - commit 29cf8c2d refactored or_other tests and added this test case, which now needs to reference the error message added in c1e273b7. --- tests/question_types/test_select_or_other.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/question_types/test_select_or_other.py b/tests/question_types/test_select_or_other.py index 21511b50..c0b29447 100644 --- a/tests/question_types/test_select_or_other.py +++ b/tests/question_types/test_select_or_other.py @@ -2,6 +2,7 @@ from unittest import expectedFailure from pyxform.aliases import select, select_multiple, select_one +from pyxform.errors import ErrorCode from tests.pyxform_test_case import PyxformTestCase from tests.xpath_helpers.choices import xpc @@ -91,9 +92,7 @@ def test_aliases__select_from_file__error(self): self.assertPyxformXform( md=md.format(case[0]), errored=True, - error__contains=[ - "[row : 2] Please specify choices for this 'or other' question." - ], + error__contains=[ErrorCode.NAMES_016.value.format(row=2)], ) # Does not raise an error, just outputs a form with `q1` but no `q1_other`. From 548ee3f574c77cf4514e0d6af1398778963c61e4 Mon Sep 17 00:00:00 2001 From: lindsay stevens Date: Thu, 6 Aug 2026 12:38:27 +1000 Subject: [PATCH 5/5] fix: flit build broken due to uncapped transitive dependency - although pyproject.toml specifies the flit_core version under the build-system section, pip doesn't read that. The flit project doesn't have an upper bound on flit_core dependency so when a newer version released with a breaking API change, it broke the build. --- .github/workflows/release.yml | 2 +- .github/workflows/verify.yml | 2 +- README.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index f3666088..c923958a 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -35,7 +35,7 @@ jobs: - name: Publish release to PyPI if: success() run: | - pip install flit==3.12.0 + pip install flit==3.12.0 "flit_core >=3.2,<4" flit --debug publish --no-use-vcs env: FLIT_USERNAME: __token__ diff --git a/.github/workflows/verify.yml b/.github/workflows/verify.yml index d3451bd0..e125c9b7 100644 --- a/.github/workflows/verify.yml +++ b/.github/workflows/verify.yml @@ -78,7 +78,7 @@ jobs: - name: Build sdist and wheel. if: success() && matrix.PYXFORM_TESTS_RUN_ODK_VALIDATE == 'false' run: | - pip install flit==3.12.0 + pip install flit==3.12.0 "flit_core >=3.2,<4" flit --debug build --no-use-vcs - name: Upload sdist and wheel. if: success() && matrix.PYXFORM_TESTS_RUN_ODK_VALIDATE == 'false' diff --git a/README.md b/README.md index df9cf476..280fde60 100644 --- a/README.md +++ b/README.md @@ -157,7 +157,7 @@ Releases are now automatic. These instructions are provided for forks or for a f 3. Install the production and packaging requirements: pip install -e . - pip install flit==3.12.0 + pip install flit==3.12.0 "flit_core >=3.2,<4" 4. Clean up build and dist folders: