fix: repeat entity label binding path too low

- the bind for the entity attributes was one path too low to find the
  correct target, because the `insert_xpaths` context was the entity
  rather than the attribute. In order to use existing insert_xpaths
  functionality that navigates the form structure using
  .parent/.children, the entity attributes must be SurveyElements so a
  new class is created for it.
  - overall it's moving from procedural generation in entity_declaration
    to a declarative generation in entities_parsing.py, and attributes
    which are the target of bind/actions/etc are responsible for the
    generation of those nodes.
- also simplify xls2json.py entities sheet processing since
  EntityColumns contains all the valid column names, and the slot names
  add a lot of column names which should not be user-specified.

Author: lindsay-stevens

pyxform/entities/entities_parsing.py

@@ -2,6 +2,7 @@
 from typing import Any
 
 from pyxform import constants as const
+from pyxform.elements import action
 from pyxform.errors import Detail, PyXFormError
 from pyxform.parsing.expression import is_xml_tag
 from pyxform.validators.pyxform.pyxform_reference import parse_pyxform_references
@@ -90,25 +91,112 @@ def get_entity_declaration(
     entity = {
         const.NAME: const.ENTITY,
         const.TYPE: const.ENTITY,
-        const.PARAMETERS: {
-            EC.DATASET.value: dataset_name,
-            EC.ENTITY_ID.value: entity_id,
-            EC.CREATE_IF.value: create_condition,
-            EC.UPDATE_IF.value: update_condition,
-            EC.REPEAT.value: entity_repeat,
+        EC.REPEAT.value: entity_repeat,
+        const.CHILDREN: [
+            {
+                const.NAME: "dataset",
+                const.TYPE: "attribute",
+                "value": dataset_name,
+            },
+        ],
+    }
+
+    if create_condition or (not update_condition and not entity_id):
+        create = {const.NAME: "create", const.TYPE: "attribute", "value": "1"}
+        if create_condition:
+            create[const.BIND] = {
+                "calculate": create_condition,
+                "readonly": "true()",
+                "type": "string",
+            }
+        entity[const.CHILDREN].append(create)
+
+    if update_condition or entity_id:
+        update_attr = {
+            const.NAME: "update",
+            const.TYPE: "attribute",
+            "value": "1",
+        }
+
+        if update_condition:
+            update_attr[const.BIND] = {
+                "calculate": update_condition,
+                "readonly": "true()",
+                "type": "string",
+            }
+
+        entity[const.CHILDREN].append(update_attr)
+
+    id_attr = {
+        const.NAME: "id",
+        const.TYPE: "attribute",
+        const.BIND: {
+            "readonly": "true()",
+            "type": "string",
         },
+        "actions": [],
     }
+
+    if create_condition or not entity_id:
+        first_load = action.ActionLibrary.setvalue_first_load.value.to_dict()
+        first_load["value"] = "uuid()"
+        id_attr["actions"].append(first_load)
+
+    if entity_repeat:
+        new_repeat = action.ActionLibrary.setvalue_new_repeat.value.to_dict()
+        new_repeat["value"] = "uuid()"
+        id_attr["actions"].append(new_repeat)
+
+    if entity_id:
+        id_attr[const.BIND]["calculate"] = entity_id
+
+        entity_id_expression = f"instance('{dataset_name}')/root/item[name={entity_id}]"
+        entity[const.CHILDREN].extend(
+            [
+                {
+                    const.NAME: "baseVersion",
+                    const.TYPE: "attribute",
+                    const.BIND: {
+                        "calculate": f"{entity_id_expression}/__version",
+                        "readonly": "true()",
+                        "type": "string",
+                    },
+                },
+                {
+                    const.NAME: "trunkVersion",
+                    const.TYPE: "attribute",
+                    const.BIND: {
+                        "calculate": f"{entity_id_expression}/__trunkVersion",
+                        "readonly": "true()",
+                        "type": "string",
+                    },
+                },
+                {
+                    const.NAME: "branchId",
+                    const.TYPE: "attribute",
+                    const.BIND: {
+                        "calculate": f"{entity_id_expression}/__branchId",
+                        "readonly": "true()",
+                        "type": "string",
+                    },
+                },
+            ]
+        )
+
+    entity[const.CHILDREN].append(id_attr)
+
     if entity_label:
-        entity[const.CHILDREN] = [
+        entity[const.CHILDREN].append(
             {
                 const.TYPE: "label",
+                const.NAME: "label",
                 const.BIND: {
                     "calculate": entity_label,
                     "readonly": "true()",
                     "type": "string",
                 },
-            }
-        ]
+            },
+        )
 
     return entity
 
@@ -173,7 +261,7 @@ def validate_entity_saveto(
             f"{const.ROW_FORMAT_STRING % row_number} Groups and repeats can't be saved as entity properties."
         )
 
-    entity_repeat = entity_declaration[const.PARAMETERS].get(EC.REPEAT, None)
+    entity_repeat = entity_declaration.get(EC.REPEAT, None)
     if entity_repeat and stack:
         # Error: saveto in nested repeat inside entity repeat.
         in_repeat = False
@@ -244,7 +332,7 @@ def validate_entity_repeat_target(
     if not entity_declaration:
         return False
 
-    entity_repeat = entity_declaration[const.PARAMETERS].get(EC.REPEAT, None)
+    entity_repeat = entity_declaration.get(EC.REPEAT, None)
 
     # Ignore: no repeat declared for the entity.
     if not entity_repeat:

pyxform/entities/entity_declaration.py

@@ -1,19 +1,14 @@
-from typing import TYPE_CHECKING
-
 from pyxform import constants as const
-from pyxform.elements import action
-from pyxform.entities.label import Label
 from pyxform.section import Section
 from pyxform.survey_element import SURVEY_ELEMENT_FIELDS
-from pyxform.utils import combine_lists, node
-
-if TYPE_CHECKING:
-    from pyxform.survey import Survey
-
+from pyxform.survey_elements.attribute import Attribute
+from pyxform.survey_elements.label import Label
+from pyxform.utils import combine_lists
 
 EC = const.EntityColumns
-ENTITY_EXTRA_FIELDS = (const.PARAMETERS,)
+ENTITY_EXTRA_FIELDS = (const.CHILDREN,)
 ENTITY_FIELDS = (*SURVEY_ELEMENT_FIELDS, *ENTITY_EXTRA_FIELDS)
+CHILD_TYPES = {"label": Label, "attribute": Attribute}
 
 
 class EntityDeclaration(Section):
@@ -40,109 +35,13 @@ class EntityDeclaration(Section):
     def get_slot_names() -> tuple[str, ...]:
         return ENTITY_FIELDS
 
-    def __init__(self, name: str, type: str, parameters: dict, **kwargs):
+    def __init__(self, name: str, type: str, **kwargs):
         super().__init__(name=name, type=type, **kwargs)
-        self.parameters: dict = parameters
-        self.children: list[Label] | None = None
+        self.children: list[Label | Attribute] = []
 
-        choices = combine_lists(
+        children = combine_lists(
             a=kwargs.pop("children", None), b=kwargs.pop(const.CHILDREN, None)
         )
-        if choices:
-            self.children = [Label(name="label", **c) for c in choices]
+        if children:
+            self.children = [CHILD_TYPES[c["type"]](**c) for c in children]
             self._link_children()
-        else:
-            self.children = []
-
-    def xml_instance(self, survey: "Survey", **kwargs):
-        parameters = self.parameters
-
-        attributes = {
-            EC.DATASET.value: parameters.get(EC.DATASET, ""),
-            "id": "",
-        }
-
-        entity_id_expression = parameters.get(EC.ENTITY_ID, None)
-        create_condition = parameters.get(EC.CREATE_IF, None)
-        update_condition = parameters.get(EC.UPDATE_IF, None)
-
-        if entity_id_expression:
-            attributes["update"] = "1"
-            attributes["baseVersion"] = ""
-            attributes["trunkVersion"] = ""
-            attributes["branchId"] = ""
-
-        if create_condition or (not update_condition and not entity_id_expression):
-            attributes["create"] = "1"
-
-        return super().xml_instance(survey=survey, **attributes)
-
-    def xml_bindings(self, survey: "Survey"):
-        """
-        See the class comment for an explanation of the logic for generating bindings.
-        """
-        parameters = self.parameters
-        entity_id_expression = parameters.get(EC.ENTITY_ID, None)
-        create_condition = parameters.get(EC.CREATE_IF, None)
-        update_condition = parameters.get(EC.UPDATE_IF, None)
-
-        if create_condition:
-            yield self._get_bind_node(survey, create_condition, "/@create")
-
-        yield self._get_id_bind_node(survey, entity_id_expression)
-
-        if update_condition:
-            yield self._get_bind_node(survey, update_condition, "/@update")
-
-        if entity_id_expression:
-            dataset_name = parameters.get(EC.DATASET, "")
-            entity = f"instance('{dataset_name}')/root/item[name={entity_id_expression}]"
-            yield self._get_bind_node(survey, f"{entity}/__version", "/@baseVersion")
-            yield self._get_bind_node(
-                survey, f"{entity}/__trunkVersion", "/@trunkVersion"
-            )
-            yield self._get_bind_node(survey, f"{entity}/__branchId", "/@branchId")
-
-        for e in self.iter_descendants():
-            if e is not self:
-                yield from e.xml_bindings(survey=survey)
-
-    def _get_id_bind_node(self, survey, entity_id_expression):
-        extra_attrs = {}
-
-        if entity_id_expression:
-            extra_attrs["calculate"] = survey.insert_xpaths(
-                text=entity_id_expression, context=self
-            )
-
-        return node(
-            const.BIND,
-            nodeset=f"{self.get_xpath()}/@id",
-            type="string",
-            readonly="true()",
-            **extra_attrs,
-        )
-
-    def xml_actions(self, survey: "Survey", in_repeat: bool = False):
-        if self.parameters.get(EC.CREATE_IF, None) or not self.parameters.get(
-            EC.ENTITY_ID, None
-        ):
-            if in_repeat:
-                setvalue_action = action.ActionLibrary.setvalue_new_repeat.value
-            else:
-                setvalue_action = action.ActionLibrary.setvalue_first_load.value
-
-            yield action.ACTION_CLASSES[setvalue_action.name](
-                ref=f"{self.get_xpath()}/@id",
-                event=action.Event(setvalue_action.event),
-                value="uuid()",
-            ).node()
-
-    def _get_bind_node(self, survey, expression, destination):
-        return node(
-            const.BIND,
-            nodeset=f"{self.get_xpath()}{destination}",
-            calculate=survey.insert_xpaths(text=expression, context=self),
-            type="string",
-            readonly="true()",
-        )

pyxform/question.py

@@ -198,7 +198,12 @@ def xml_actions(self, survey: "Survey", in_repeat: bool = False):
         action_fields = {"name", "event", "value"}
         if self.actions:
             for _action in self.actions:
-                cls = action.ACTION_CLASSES[_action["name"]]
+                event = action.Event(_action["event"])
+                if (not in_repeat and event == action.Event.ODK_NEW_REPEAT.value) or (
+                    in_repeat and event != action.Event.ODK_NEW_REPEAT
+                ):
+                    continue
+
                 if self._dynamic_default is True or (
                     self.default and default_is_dynamic(self.default, self.type)
                 ):
@@ -208,13 +213,7 @@ def xml_actions(self, survey: "Survey", in_repeat: bool = False):
                 else:
                     value = _action.get("value")
 
-                event = action.Event(_action["event"])
-                if (not in_repeat and event == action.Event.ODK_NEW_REPEAT.value) or (
-                    in_repeat and event != action.Event.ODK_NEW_REPEAT
-                ):
-                    continue
-
-                yield cls(
+                yield action.ACTION_CLASSES[_action["name"]](
                     ref=self.get_xpath(),
                     event=event,
                     value=value,

pyxform/section.py

@@ -5,6 +5,7 @@
 from collections.abc import Callable, Generator, Iterable
 from itertools import chain
 from typing import TYPE_CHECKING
+from xml.dom.minidom import Attr
 
 from pyxform import constants
 from pyxform.external_instance import ExternalInstance
@@ -133,9 +134,13 @@ def xml_instance(self, survey: "Survey", **kwargs):
                 if isinstance(child, RepeatingSection) and not append_template:
                     append_template = not append_template
                     repeating_template = child.generate_repeating_template(survey=survey)
-                result.appendChild(
-                    child.xml_instance(survey=survey, append_template=append_template)
+                child_instance = child.xml_instance(
+                    survey=survey, append_template=append_template
                 )
+                if isinstance(child_instance, Attr):
+                    result.setAttributeNode(child_instance)
+                else:
+                    result.appendChild(child_instance)
             if append_template and repeating_template:
                 append_template = not append_template
                 result.insertBefore(repeating_template, result._get_lastChild())
@@ -232,11 +237,11 @@ def xml_control(self, survey: "Survey"):
 
         # Get setvalue nodes for all descendants of this repeat that have dynamic defaults
         # and aren't nested in other repeats. Let nested repeats handle their own defaults
-        from pyxform.entities.entity_declaration import EntityDeclaration
         from pyxform.question import Question
+        from pyxform.survey_elements.attribute import Attribute
 
         def condition(i, parent=self):
-            return isinstance(i, EntityDeclaration | Question) and (
+            return isinstance(i, Attribute | Question) and (
                 i.parent is self
                 or parent
                 == next(

pyxform/survey.py

@@ -14,8 +14,6 @@
 
 from pyxform import aliases, constants
 from pyxform.constants import EXTERNAL_INSTANCE_EXTENSIONS, NSMAP
-from pyxform.entities.entity_declaration import EntityDeclaration
-from pyxform.entities.label import Label
 from pyxform.errors import PyXFormError, ValidationError
 from pyxform.external_instance import ExternalInstance
 from pyxform.instance import SurveyInstance
@@ -24,6 +22,7 @@
 from pyxform.question import Itemset, MultipleChoiceQuestion, Option, Question, Tag
 from pyxform.section import SECTION_EXTRA_FIELDS, RepeatingSection, Section
 from pyxform.survey_element import _GET_SENTINEL, SURVEY_ELEMENT_FIELDS, SurveyElement
+from pyxform.survey_elements.attribute import Attribute
 from pyxform.utils import (
     LAST_SAVED_INSTANCE_NAME,
     DetachableElement,
@@ -600,11 +599,11 @@ def xml_model_bindings(self) -> Generator[DetachableElement | None, None, None]:
         Yield bindings (bind or action elements) for this node and all its descendants.
         """
         for e in self.iter_descendants(
-            condition=lambda i: not isinstance(i, Option | Tag | Label)
+            condition=lambda i: not isinstance(i, Option | Tag)
         ):
             yield from e.xml_bindings(survey=self)
 
-            if isinstance(e, EntityDeclaration | Question):
+            if isinstance(e, Attribute | Question):
                 yield from e.xml_actions(survey=self, in_repeat=False)
 
     def xml_model(self):