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: 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..65818189 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=( @@ -358,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/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/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 a1266e9f..093c6d40 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,26 +889,27 @@ 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"]] - 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." - ) + select_command = parse_dict["select_command"] + select_type = aliases.select[select_command] 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." @@ -924,18 +927,11 @@ def workbook_to_json( + "List name not in external choices sheet: " + list_name ) - 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: @@ -948,28 +944,26 @@ 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( + code=ErrorCode.NAMES_016, context={"row": row_number} + ) - # 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: @@ -1029,10 +1023,12 @@ 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: + 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, @@ -1051,6 +1047,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( @@ -1372,6 +1370,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 | 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)] + ) 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`.