chg: make name uniqueness rules more consistent

- main changes:
  - case-insensitive duplicate is a warning rather than an error since
    it is not a hard requirement and only applies to some use cases
  - group names can be re-used in a different context rather than having
    to be unique anywhere in the survey structure (like repeats)
    - if pyxform reference variables ${} are used the target question,
      group, or repeat still has to be unique anywhere
  - add uniqueness validation to xls2json so the errors can use row refs
  - add an error message specific to the 'meta' reserved name
  - use same validation funcs in xls2json and section.py/survey.py
  - update test assertions to use message templates

Author: lindsay-stevens

pyxform/constants.py

@@ -86,6 +86,7 @@
 OSM_TYPE = "binary"
 
 NAMESPACES = "namespaces"
+META = "meta"
 
 # The following are the possible sheet names:
 SUPPORTED_SHEET_NAMES = {
@@ -125,6 +126,7 @@ class EntityColumns(StrEnum):
     ENTITY_ID = "entity_id"
     CREATE_IF = "create_if"
     UPDATE_IF = "update_if"
+    REPEAT = "repeat"
     LABEL = "label"
 
 
@@ -169,3 +171,4 @@ class EntityColumns(StrEnum):
 }
 SUPPORTED_MEDIA_TYPES = {"image", "big-image", "audio", "video"}
 OR_OTHER_CHOICE = {NAME: "other", LABEL: "Other"}
+RESERVED_NAMES_SURVEY_SHEET = {META}

pyxform/errors.py

@@ -2,6 +2,41 @@
 Common base classes for pyxform exceptions.
 """
 
+from string import Formatter
+
+
+class _ErrorFormatter(Formatter):
+    """Allows specifying a default for missing format keys."""
+
+    def __init__(self, default_value: str = "unknown"):
+        self.default_value: str = default_value
+
+    def get_value(self, key, args, kwargs):
+        if isinstance(key, str):
+            value = kwargs.get(key, None)
+            if value is None:
+                return self.default_value
+            else:
+                return value
+        else:
+            return super().get_value(key, args, kwargs)
+
+
+_ERROR_FORMATTER = _ErrorFormatter()
+
+
+class Detail:
+    """ErrorCode details."""
+
+    __slots__ = ("msg", "name")
+
+    def __init__(self, name: str, msg: str) -> None:
+        self.name: str = name
+        self.msg: str = msg
+
+    def format(self, **kwargs):
+        return _ERROR_FORMATTER.format(self.msg, **kwargs)
+
 
 class PyXFormError(Exception):
     """Common base class for pyxform exceptions."""

pyxform/section.py

@@ -7,10 +7,10 @@
 from typing import TYPE_CHECKING
 
 from pyxform import constants
-from pyxform.errors import PyXFormError
 from pyxform.external_instance import ExternalInstance
 from pyxform.survey_element import SURVEY_ELEMENT_FIELDS, SurveyElement
 from pyxform.utils import DetachableElement, node
+from pyxform.validators.pyxform import unique_names
 
 if TYPE_CHECKING:
     from pyxform.question import Question
@@ -94,18 +94,18 @@ def iter_descendants(
                     iter_into_section_items=iter_into_section_items,
                 )
 
-    # there's a stronger test of this when creating the xpath
-    # dictionary for a survey.
     def _validate_uniqueness_of_element_names(self):
-        element_slugs = set()
+        child_names = set()
+        child_names_lower = set()
+        warnings = []
         for element in self.children:
-            elem_lower = element.name.lower()
-            if elem_lower in element_slugs:
-                raise PyXFormError(
-                    f"There are more than one survey elements named '{elem_lower}' "
-                    f"(case-insensitive) in the section named '{self.name}'."
-                )
-            element_slugs.add(elem_lower)
+            unique_names.validate_question_group_repeat_name(
+                name=element.name,
+                seen_names=child_names,
+                seen_names_lower=child_names_lower,
+                warnings=warnings,
+                check_reserved=False,
+            )
 
     def xml_instance(self, survey: "Survey", **kwargs):
         """

pyxform/survey.py

@@ -21,7 +21,7 @@
 from pyxform.parsing.expression import has_last_saved
 from pyxform.parsing.instance_expression import replace_with_output
 from pyxform.question import Itemset, MultipleChoiceQuestion, Option, Question, Tag
-from pyxform.section import SECTION_EXTRA_FIELDS, Section
+from pyxform.section import SECTION_EXTRA_FIELDS, RepeatingSection, Section
 from pyxform.survey_element import SURVEY_ELEMENT_FIELDS, SurveyElement
 from pyxform.utils import (
     BRACKETED_TAG_REGEX,
@@ -31,6 +31,7 @@
     node,
 )
 from pyxform.validators import enketo_validate, odk_validate
+from pyxform.validators.pyxform import unique_names
 from pyxform.validators.pyxform.iana_subtags.validation import get_languages_with_bad_tags
 
 RE_BRACKET = re.compile(r"\[([^]]+)\]")
@@ -292,21 +293,16 @@ def validate(self):
 
     def _validate_uniqueness_of_section_names(self):
         root_node_name = self.name
-        section_names = set()
-        for element in self.iter_descendants(condition=lambda i: isinstance(i, Section)):
-            if element.name in section_names:
-                if element.name == root_node_name:
-                    # The root node name is rarely explictly set; explain
-                    # the problem in a more helpful way (#510)
-                    msg = (
-                        f"The name '{element.name}' is the same as the form name. "
-                        "Use a different section name (or change the form name in "
-                        "the 'name' column of the settings sheet)."
-                    )
-                    raise PyXFormError(msg)
-                msg = f"There are two sections with the name {element.name}."
-                raise PyXFormError(msg)
-            section_names.add(element.name)
+        repeat_names = set()
+        for element in self.iter_descendants(
+            condition=lambda i: isinstance(i, RepeatingSection)
+        ):
+            unique_names.validate_repeat_name(
+                name=element.name,
+                control_type=constants.REPEAT,
+                instance_element_name=root_node_name,
+                seen_names=repeat_names,
+            )
 
     def get_nsmap(self):
         """Add additional namespaces"""

pyxform/validators/pyxform/unique_names.py

@@ -0,0 +1,112 @@
+from pyxform import constants as const
+from pyxform.errors import Detail, PyXFormError
+
+NAMES001 = Detail(
+    name="Invalid duplicate name in same context",
+    msg=(
+        "[row : {row}] On the 'survey' sheet, the 'name' value '{value}' is invalid. "
+        "Questions, groups, and repeats must be unique within their context i.e. nearest "
+        "parent group or repeat, or the survey if not inside a group or repeat."
+    ),
+)
+NAMES002 = Detail(
+    name="Invalid duplicate name in context (case-insensitive)",
+    msg=(
+        "[row : {row}] On the 'survey' sheet, the 'name' value '{value}' is problematic. "
+        "The name is a case-insensitive match to another name. Questions, groups, and "
+        "repeats must be unique within their context i.e. nearest parent group or repeat, "
+        "or the survey if not inside a group or repeat. Some data processing tools are not "
+        "case-sensitive, so the current names may make analysis difficult."
+    ),
+)
+NAMES003 = Detail(
+    name="Invalid repeat name same as survey",
+    msg=(
+        "[row : {row}] On the 'survey' sheet, the 'name' value '{value}' is invalid. "
+        "Repeat names must not be the same as the survey root (which defaults to 'data')."
+    ),
+)
+NAMES004 = Detail(
+    name="Invalid duplicate repeat name in the survey",
+    msg=(
+        "[row : {row}] On the 'survey' sheet, the 'name' value '{value}' is invalid. "
+        "Repeat names must unique anywhere the survey, at all levels of group or repeat nesting."
+    ),
+)
+NAMES005 = Detail(
+    name="Invalid duplicate meta name in the survey",
+    msg=(
+        "[row : {row}] On the 'survey' sheet, the 'name' value 'meta' is invalid. "
+        "The name 'meta' is reserved for form metadata."
+    ),
+)
+
+
+def validate_question_group_repeat_name(
+    name: str | None,
+    seen_names: set[str],
+    seen_names_lower: set[str],
+    warnings: list[str],
+    row_number: int | None = None,
+    check_reserved: bool = True,
+):
+    """
+    Warn about duplicate or problematic names on the survey sheet.
+
+    May append the name to `seen_names` and `neen_names_lower`. May append to `warnings`.
+
+    :param name: Question or group name.
+    :param seen_names: Names already processed in the sheet.
+    :param seen_names_lower: Same as seen_names but lower case.
+    :param warnings: Warnings list.
+    :param row_number: Current sheet row number.
+    :param check_reserved: If True, check the name against any reserved names. When
+      checking names in the context of SurveyElement processing, it's difficult to
+      differentiate user-specified names from names added by pyxform.
+    """
+    if not name:
+        return
+
+    if check_reserved and not seen_names >= const.RESERVED_NAMES_SURVEY_SHEET:
+        seen_names.update(const.RESERVED_NAMES_SURVEY_SHEET)
+
+    if name in seen_names:
+        if name == const.META:
+            raise PyXFormError(NAMES005.format(row=row_number))
+        else:
+            raise PyXFormError(NAMES001.format(row=row_number, value=name))
+    seen_names.add(name)
+
+    question_name_lower = name.lower()
+    if question_name_lower in seen_names_lower:
+        # No case-insensitive warning for 'meta' since it's not an exported data table.
+        warnings.append(NAMES002.format(row=row_number, value=name))
+    seen_names_lower.add(question_name_lower)
+
+
+def validate_repeat_name(
+    name: str | None,
+    control_type: str,
+    instance_element_name: str,
+    seen_names: set[str],
+    row_number: int | None = None,
+):
+    """
+    Warn about duplicate or problematic names.
+
+    May appends the name to `seen_names` and `neen_names_lower`. May append to `warnings`.
+    These checks are additional to the above in validate_survey_sheet_name so does not
+    re-check reserved names etc.
+
+    :param row_number: Current sheet row number.
+    :param name: Question or group name.
+    :param control_type: E.g. group, repeat, or loop.
+    :param instance_element_name: Name of the main survey instance element.
+    :param seen_names: Names already processed in the sheet.
+    """
+    if control_type == const.REPEAT:
+        if name == instance_element_name:
+            raise PyXFormError(NAMES003.format(row=row_number, value=name))
+        elif name in seen_names:
+            raise PyXFormError(NAMES004.format(row=row_number, value=name))
+        seen_names.add(name)

pyxform/xls2json.py

@@ -28,7 +28,7 @@
     default_is_dynamic,
     print_pyobj_to_json,
 )
-from pyxform.validators.pyxform import parameters_generic, select_from_file
+from pyxform.validators.pyxform import parameters_generic, select_from_file, unique_names
 from pyxform.validators.pyxform import question_types as qt
 from pyxform.validators.pyxform.android_package_name import validate_android_package_name
 from pyxform.validators.pyxform.choices import validate_and_clean_choices
@@ -527,6 +527,8 @@ def workbook_to_json(
             "control_type": None,
             "control_name": None,
             "parent_children": json_dict.get(constants.CHILDREN),
+            "child_names": set(),
+            "child_names_lower": set(),
             "row_number": None,
         }
     ]
@@ -539,11 +541,14 @@ def workbook_to_json(
     # To check that questions with triggers refer to other questions that exist.
     question_names = set()
     trigger_references = []
+    repeat_names = set()
 
     # row by row, validate questions, throwing errors and adding warnings where needed.
     for row_number, row in enumerate(survey_sheet.data, start=2):
         prev_control_type = stack[-1]["control_type"]
         parent_children_array = stack[-1]["parent_children"]
+        child_names = stack[-1]["child_names"]
+        child_names_lower = stack[-1]["child_names_lower"]
 
         # Disabled should probably be first
         # so the attributes below can be disabled.
@@ -789,9 +794,9 @@ def workbook_to_json(
 
         # Make sure the row has a valid name
         if constants.NAME not in row:
-            if row["type"] == "note":
+            if row[constants.TYPE] == "note":
                 # autogenerate names for notes without them
-                row["name"] = "generated_note_name_" + str(row_number)
+                row[constants.NAME] = "generated_note_name_" + str(row_number)
             else:
                 raise PyXFormError(
                     ROW_FORMAT_STRING % row_number + " Question or group with no name."
@@ -805,7 +810,17 @@ def workbook_to_json(
                 f"{ROW_FORMAT_STRING % row_number} Invalid question name '{question_name}'. Names {XML_IDENTIFIER_ERROR_MESSAGE}"
             )
 
-        in_repeat = any(ancestor["control_type"] == "repeat" for ancestor in stack)
+        unique_names.validate_question_group_repeat_name(
+            row_number=row_number,
+            name=question_name,
+            seen_names=child_names,
+            seen_names_lower=child_names_lower,
+            warnings=warnings,
+        )
+
+        in_repeat = any(
+            ancestor["control_type"] == constants.REPEAT for ancestor in stack
+        )
         validate_entity_saveto(row, row_number, in_repeat, entity_declaration)
 
         # Try to parse question as begin control statement
@@ -820,7 +835,14 @@ def workbook_to_json(
                 # (so following questions are nested under it)
                 # until an end command is encountered.
                 control_type = aliases.control[parse_dict["type"]]
-                control_name = question_name
+
+                unique_names.validate_repeat_name(
+                    row_number=row_number,
+                    name=question_name,
+                    control_type=control_type,
+                    instance_element_name=json_dict[constants.NAME],
+                    seen_names=repeat_names,
+                )
 
                 # Check if the control item has a label, if applicable.
                 # This label check used to apply to all items, but no longer is
@@ -854,7 +876,7 @@ def workbook_to_json(
                 new_json_dict[constants.TYPE] = control_type
                 child_list = []
                 new_json_dict[constants.CHILDREN] = child_list
-                if control_type is constants.LOOP:
+                if control_type == constants.LOOP:
                     if not parse_dict.get(constants.LIST_NAME_U):
                         # TODO: Perhaps warn and make repeat into a group?
                         raise PyXFormError(
@@ -932,8 +954,10 @@ def workbook_to_json(
                 stack.append(
                     {
                         "control_type": control_type,
-                        "control_name": control_name,
+                        "control_name": question_name,
                         "parent_children": child_list,
+                        "child_names": set(),
+                        "child_names_lower": set(),
                         "row_number": row_number,
                     }
                 )

tests/test_external_instances.py

@@ -6,6 +6,8 @@
 
 from textwrap import dedent
 
+from pyxform.validators.pyxform import unique_names
+
 from tests.pyxform_test_case import PyxformTestCase, PyxformTestError
 from tests.xpath_helpers.choices import xpc
 
@@ -48,9 +50,7 @@ def test_cannot__use_same_external_xml_id_in_same_section(self):
         self.assertPyxformXform(
             md=md,
             errored=True,
-            error__contains=[
-                "There are more than one survey elements named 'mydata' (case-insensitive) in the section named 'test_name'"
-            ],
+            error__contains=[unique_names.NAMES001.format(row=3, value="mydata")],
         )
 
     def test_can__use_unique_external_xml_in_same_section(self):