Merge pull request #671 from lindsay-stevens/pyxform-669

#669: reject form with unknown columns in entities sheet

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"

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,34 +93,47 @@ 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):
         if isinstance(save_to, bytes):
             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)

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()

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())

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'"
+            ],
+        )