From 64a41a357c7e84998a08862f02b150c593e9da85 Mon Sep 17 00:00:00 2001 From: lindsay stevens Date: Tue, 28 Nov 2023 20:31:13 +1100 Subject: [PATCH] add: reject form with unknown columns in entities sheet #669 - add tests for new error case(s) + happy path - add copy of python 3.11 StrEnum which can reference members as str - add enum with the expected entities column names - update existing entities modules to use constants --- pyxform/constants.py | 11 ++++ pyxform/entities/entities_parsing.py | 76 +++++++++++++++----------- pyxform/entities/entity_declaration.py | 42 +++++++------- pyxform/util/__init__.py | 0 pyxform/util/enum.py | 32 +++++++++++ tests/test_entities_create.py | 49 ++++++++++++++++- 6 files changed, 159 insertions(+), 51 deletions(-) create mode 100644 pyxform/util/__init__.py create mode 100644 pyxform/util/enum.py diff --git a/pyxform/constants.py b/pyxform/constants.py index 73c0ed650..78f312e89 100644 --- a/pyxform/constants.py +++ b/pyxform/constants.py @@ -8,6 +8,7 @@ """ # TODO: Replace matching strings in the json2xforms code (builder.py, # survey.py, survey_element.py, question.py) with these constants +from pyxform.util.enum import StrEnum TYPE = "type" TITLE = "title" @@ -112,9 +113,19 @@ # The ODK entities spec version that generated forms comply to ENTITIES_CREATE_VERSION = "2022.1.0" CURRENT_ENTITIES_VERSION = "2023.1.0" +ENTITY = "entity" ENTITY_FEATURES = "entity_features" ENTITIES_RESERVED_PREFIX = "__" + +class EntityColumns(StrEnum): + DATASET = "dataset" + ENTITY_ID = "entity_id" + CREATE_IF = "create_if" + UPDATE_IF = "update_if" + LABEL = "label" + + DEPRECATED_DEVICE_ID_METADATA_FIELDS = ["subscriberid", "simserial"] AUDIO_QUALITY_VOICE_ONLY = "voice-only" diff --git a/pyxform/entities/entities_parsing.py b/pyxform/entities/entities_parsing.py index f4c4a6205..d62b74241 100644 --- a/pyxform/entities/entities_parsing.py +++ b/pyxform/entities/entities_parsing.py @@ -1,19 +1,19 @@ from typing import Dict, List -from pyxform import constants +from pyxform import constants as const from pyxform.errors import PyXFormError from pyxform.xlsparseutils import find_sheet_misspellings, is_valid_xml_tag +EC = const.EntityColumns + def get_entity_declaration( entities_sheet: Dict, workbook_dict: Dict, warnings: List ) -> Dict: if len(entities_sheet) == 0: - similar = find_sheet_misspellings( - key=constants.ENTITIES, keys=workbook_dict.keys() - ) + similar = find_sheet_misspellings(key=const.ENTITIES, keys=workbook_dict.keys()) if similar is not None: - warnings.append(similar + constants._MSG_SUPPRESS_SPELLING) + warnings.append(similar + const._MSG_SUPPRESS_SPELLING) return {} elif len(entities_sheet) > 1: raise PyXFormError( @@ -22,11 +22,12 @@ def get_entity_declaration( entity_row = entities_sheet[0] + validate_entities_columns(row=entity_row) dataset_name = get_validated_dataset_name(entity_row) - entity_id = entity_row.get("entity_id", None) - create_condition = entity_row.get("create_if", None) - update_condition = entity_row.get("update_if", None) - entity_label = entity_row.get("label", None) + entity_id = entity_row.get(EC.ENTITY_ID, None) + create_condition = entity_row.get(EC.CREATE_IF, None) + update_condition = entity_row.get(EC.UPDATE_IF, None) + entity_label = entity_row.get(EC.LABEL, None) if update_condition and not entity_id: raise PyXFormError( @@ -44,24 +45,24 @@ def get_entity_declaration( ) return { - "name": "entity", - "type": "entity", - "parameters": { - "dataset": dataset_name, - "entity_id": entity_id, - "create": create_condition, - "update": update_condition, - "label": entity_label, + const.NAME: const.ENTITY, + const.TYPE: const.ENTITY, + const.PARAMETERS: { + EC.DATASET: dataset_name, + EC.ENTITY_ID: entity_id, + EC.CREATE_IF: create_condition, + EC.UPDATE_IF: update_condition, + EC.LABEL: entity_label, }, } def get_validated_dataset_name(entity): - dataset = entity["dataset"] + dataset = entity[EC.DATASET] - if dataset.startswith(constants.ENTITIES_RESERVED_PREFIX): + if dataset.startswith(const.ENTITIES_RESERVED_PREFIX): raise PyXFormError( - f"Invalid entity list name: '{dataset}' starts with reserved prefix {constants.ENTITIES_RESERVED_PREFIX}." + f"Invalid entity list name: '{dataset}' starts with reserved prefix {const.ENTITIES_RESERVED_PREFIX}." ) if "." in dataset: @@ -83,7 +84,7 @@ def get_validated_dataset_name(entity): def validate_entity_saveto( row: Dict, row_number: int, entity_declaration: Dict, in_repeat: bool ): - save_to = row.get("bind", {}).get("entities:saveto", "") + save_to = row.get(const.BIND, {}).get("entities:saveto", "") if not save_to: return @@ -92,28 +93,26 @@ def validate_entity_saveto( "To save entity properties using the save_to column, you must add an entities sheet and declare an entity." ) - if constants.GROUP in row.get(constants.TYPE) or constants.REPEAT in row.get( - constants.TYPE - ): + if const.GROUP in row.get(const.TYPE) or const.REPEAT in row.get(const.TYPE): raise PyXFormError( - f"{constants.ROW_FORMAT_STRING % row_number} Groups and repeats can't be saved as entity properties." + f"{const.ROW_FORMAT_STRING % row_number} Groups and repeats can't be saved as entity properties." ) if in_repeat: raise PyXFormError( - f"{constants.ROW_FORMAT_STRING % row_number} Currently, you can't create entities from repeats. You may only specify save_to values for form fields outside of repeats." + f"{const.ROW_FORMAT_STRING % row_number} Currently, you can't create entities from repeats. You may only specify save_to values for form fields outside of repeats." ) - error_start = f"{constants.ROW_FORMAT_STRING % row_number} Invalid save_to name:" + error_start = f"{const.ROW_FORMAT_STRING % row_number} Invalid save_to name:" - if save_to.lower() == "name" or save_to.lower() == "label": + if save_to.lower() == const.NAME or save_to.lower() == const.LABEL: raise PyXFormError( f"{error_start} the entity property name '{save_to}' is reserved." ) - if save_to.startswith(constants.ENTITIES_RESERVED_PREFIX): + if save_to.startswith(const.ENTITIES_RESERVED_PREFIX): raise PyXFormError( - f"{error_start} the entity property name '{save_to}' starts with reserved prefix {constants.ENTITIES_RESERVED_PREFIX}." + f"{error_start} the entity property name '{save_to}' starts with reserved prefix {const.ENTITIES_RESERVED_PREFIX}." ) if not is_valid_xml_tag(save_to): @@ -121,5 +120,20 @@ def validate_entity_saveto( save_to = save_to.encode("utf-8") raise PyXFormError( - f"{error_start} '{save_to}'. Entity property names {constants.XML_IDENTIFIER_ERROR_MESSAGE}" + f"{error_start} '{save_to}'. Entity property names {const.XML_IDENTIFIER_ERROR_MESSAGE}" + ) + + +def validate_entities_columns(row: Dict): + expected = set(EC.value_list()) + observed = set(row.keys()) + extra = observed.difference(expected) + if 0 < len(extra): + fmt_extra = ", ".join((f"'{k}'" for k in extra)) + msg = ( + f"The entities sheet included the following unexpected column(s): {fmt_extra}. " + f"These columns are not supported by this version of pyxform. Please either: " + f"check the spelling of the column names, remove the columns, or update " + f"pyxform." ) + raise PyXFormError(msg) diff --git a/pyxform/entities/entity_declaration.py b/pyxform/entities/entity_declaration.py index e86576d5e..33ee5ff14 100644 --- a/pyxform/entities/entity_declaration.py +++ b/pyxform/entities/entity_declaration.py @@ -1,8 +1,11 @@ # -*- coding: utf-8 -*- +from pyxform import constants as const from pyxform.survey_element import SurveyElement from pyxform.utils import node +EC = const.EntityColumns + class EntityDeclaration(SurveyElement): """ @@ -23,13 +26,14 @@ class EntityDeclaration(SurveyElement): """ def xml_instance(self, **kwargs): - attributes = {} - attributes["dataset"] = self.get("parameters", {}).get("dataset", "") - attributes["id"] = "" + attributes = { + EC.DATASET.value: self.get(const.PARAMETERS, {}).get(EC.DATASET, ""), + "id": "", + } - entity_id_expression = self.get("parameters", {}).get("entity_id", None) - create_condition = self.get("parameters", {}).get("create", None) - update_condition = self.get("parameters", {}).get("update", None) + entity_id_expression = self.get(const.PARAMETERS, {}).get(EC.ENTITY_ID, None) + create_condition = self.get(const.PARAMETERS, {}).get(EC.CREATE_IF, None) + update_condition = self.get(const.PARAMETERS, {}).get(EC.UPDATE_IF, None) if entity_id_expression: attributes["update"] = "1" @@ -38,20 +42,20 @@ def xml_instance(self, **kwargs): if create_condition or (not update_condition and not entity_id_expression): attributes["create"] = "1" - if self.get("parameters", {}).get("label", None): - return node("entity", node("label"), **attributes) + if self.get(const.PARAMETERS, {}).get(EC.LABEL, None): + return node(const.ENTITY, node(const.LABEL), **attributes) else: - return node("entity", **attributes) + return node(const.ENTITY, **attributes) def xml_bindings(self): """ See the class comment for an explanation of the logic for generating bindings. """ survey = self.get_root() - entity_id_expression = self.get("parameters", {}).get("entity_id", None) - create_condition = self.get("parameters", {}).get("create", None) - update_condition = self.get("parameters", {}).get("update", None) - label_expression = self.get("parameters", {}).get("label", None) + entity_id_expression = self.get(const.PARAMETERS, {}).get(EC.ENTITY_ID, None) + create_condition = self.get(const.PARAMETERS, {}).get(EC.CREATE_IF, None) + update_condition = self.get(const.PARAMETERS, {}).get(EC.UPDATE_IF, None) + label_expression = self.get(const.PARAMETERS, {}).get(EC.LABEL, None) bind_nodes = [] @@ -67,7 +71,7 @@ def xml_bindings(self): bind_nodes.append(self._get_bind_node(survey, update_condition, "/@update")) if entity_id_expression: - dataset_name = self.get("parameters", {}).get("dataset", "") + dataset_name = self.get(const.PARAMETERS, {}).get(EC.DATASET, "") base_version_expression = f"instance('{dataset_name}')/root/item[name={entity_id_expression}]/__version" bind_nodes.append( self._get_bind_node(survey, base_version_expression, "/@baseVersion") @@ -79,19 +83,19 @@ def xml_bindings(self): return bind_nodes def _get_id_bind_node(self, survey, entity_id_expression): - id_bind = {"type": "string", "readonly": "true()"} + id_bind = {const.TYPE: "string", "readonly": "true()"} if entity_id_expression: id_bind["calculate"] = survey.insert_xpaths( entity_id_expression, context=self ) - return node("bind", nodeset=self.get_xpath() + "/@id", **id_bind) + return node(const.BIND, nodeset=self.get_xpath() + "/@id", **id_bind) def _get_id_setvalue_node(self): id_setvalue_attrs = { "event": "odk-instance-first-load", - "type": "string", + const.TYPE: "string", "readonly": "true()", "value": "uuid()", } @@ -102,11 +106,11 @@ def _get_bind_node(self, survey, expression, destination): expr = survey.insert_xpaths(expression, context=self) bind_attrs = { "calculate": expr, - "type": "string", + const.TYPE: "string", "readonly": "true()", } - return node("bind", nodeset=self.get_xpath() + destination, **bind_attrs) + return node(const.BIND, nodeset=self.get_xpath() + destination, **bind_attrs) def xml_control(self): raise NotImplementedError() diff --git a/pyxform/util/__init__.py b/pyxform/util/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/pyxform/util/enum.py b/pyxform/util/enum.py new file mode 100644 index 000000000..4870013a4 --- /dev/null +++ b/pyxform/util/enum.py @@ -0,0 +1,32 @@ +from enum import Enum + + +class StrEnum(str, Enum): + """Base Enum class with common helper function.""" + + # Copied from Python 3.11 enum.py. In many cases can use members as strings, but + # sometimes need to deref with ".value" property e.g. `EnumClass.MEMBERNAME.value`. + def __new__(cls, *values): + "values must already be of type `str`" + if len(values) > 3: + raise TypeError("too many arguments for str(): %r" % (values,)) + if len(values) == 1: + # it must be a string + if not isinstance(values[0], str): + raise TypeError("%r is not a string" % (values[0],)) + if len(values) >= 2: + # check that encoding argument is a string + if not isinstance(values[1], str): + raise TypeError("encoding must be a string, not %r" % (values[1],)) + if len(values) == 3: + # check that errors argument is a string + if not isinstance(values[2], str): + raise TypeError("errors must be a string, not %r" % (values[2])) + value = str(*values) + member = str.__new__(cls, value) + member._value_ = value + return member + + @classmethod + def value_list(cls): + return list(cls.__members__.values()) diff --git a/tests/test_entities_create.py b/tests/test_entities_create.py index 83c40cb15..e940272fa 100644 --- a/tests/test_entities_create.py +++ b/tests/test_entities_create.py @@ -367,7 +367,7 @@ def test_saveto_in_group__works(self): | | type | name | label | save_to | | | begin_group | a | A | | | | text | size | Size | size | - | | end_group | | | | + | | end_group | | | | | entities | | | | | | | dataset | label | | | | | trees | ${size}| | | @@ -391,3 +391,50 @@ def test_list_name_alias_to_dataset(self): '/h:html/h:head/x:model/x:instance/x:data/x:meta/x:entity[@dataset = "trees"]', ], ) + + def test_entities_columns__all_expected(self): + self.assertPyxformXform( + md=""" + | survey | | | | + | | type | name | label | + | | text | id | Treid | + | | text | a | A | + | entities | | | | + | | dataset | label | update_if | create_if | entity_id | + | | trees | a | id != '' | id = '' | ${a} | + """, + errored=False, + warnings_count=0, + ) + + def test_entities_columns__one_unexpected(self): + self.assertPyxformXform( + md=""" + | survey | | | | + | | type | name | label | + | | text | a | A | + | entities | | | | + | | dataset | label | what | + | | trees | a | ! | + """, + errored=True, + error_contains=[ + "The entities sheet included the following unexpected column(s): 'what'" + ], + ) + + def test_entities_columns__multiple_unexpected(self): + self.assertPyxformXform( + md=""" + | survey | | | | + | | type | name | label | + | | text | a | A | + | entities | | | | + | | dataset | label | what | why | + | | trees | a | ! | ? | + """, + errored=True, + error_contains=[ + "The entities sheet included the following unexpected column(s): 'what', 'why'" + ], + )