fix: Allow underscores in a combination expression

src/openjd/model/v2023_09/_model.py

@@ -193,7 +193,9 @@ class ValueReferenceConstants(Enum):
 _standard_string_regex = rf"(?-m:^[^{_Cc_characters}]+\z)"
 
 # Latin alphanumeric, starting with a letter
-_identifier_regex = r"(?-m:^[A-Za-z_][A-Za-z0-9_]*\z)"
+# Shared with CombinationExpr, which §3.4.3 defines in terms of these characters.
+_identifier_chars = r"A-Za-z0-9_"
+_identifier_regex = rf"(?-m:^[A-Za-z_][{_identifier_chars}]*\z)"
 
 # Regex for defining file filter patterns allowed for use in file dialogs.
 # 1. Allowable values: "*", "*.*", and "*.[:file-extension-chars:]+".
@@ -1777,12 +1779,17 @@ def _validate_range_elements(cls, value: Any) -> Any:
         max_length=16,
     ),
 ]
-# Limit the CombinationExpr to characters allowed in an Identifier plus whitespace
-# and the operator characters.
+# §3.4.3: an Identifier's characters plus the space and the operators. Only the
+# character class is shared with Identifier, not its leading-character rule.
+# The space is deliberately just U+0020. The shared TokenStream folds all
+# whitespace, which is wider than §3.4.3 allows, so a newline is refused here.
 CombinationExpr = Annotated[
     str,
     StringConstraints(
-        min_length=1, max_length=1280, strict=True, pattern=r"(?-m:^[A-Za-z0-9\*\(\), ]+\z)"
+        min_length=1,
+        max_length=1280,
+        strict=True,
+        pattern=rf"(?-m:^[{_identifier_chars}\*\(\), ]+\z)",
     ),
 ]
 

test/openjd/model_v0/v2023_09/test_parameter_space.py

@@ -6,7 +6,13 @@
 import pytest
 from pydantic import ValidationError
 
-from openjd.model import DecodeValidationError, decode_job_template, parse_model
+from openjd.model import (
+    DecodeValidationError,
+    StepParameterSpaceIterator,
+    create_job,
+    decode_job_template,
+    parse_model,
+)
 from openjd.model._parse import _parse_model
 from openjd.model.v2023_09 import (
     FloatTaskParameterDefinition,
@@ -17,6 +23,7 @@
     StepParameterSpaceDefinition,
     StringTaskParameterDefinition,
 )
+from openjd.model.v2023_09._model import _identifier_chars
 
 
 class TestIntTaskParameterDefinition:
@@ -1071,3 +1078,132 @@ def test_two_miscased_chunk_int_report_the_one_chunk_rule(self) -> None:
                     ],
                 }
             )
+
+
+class TestCombinationExprCharacterClass:
+    """Template Schemas §3.4.3 constraint 1 gives a combination expression the
+    characters of an ``<Identifier>`` plus the space and the operators. §7.1
+    puts ``_`` in an ``<Identifier>``, so a parameter named ``Frame_Range`` must
+    be referenceable. The character class omitted ``_``, which made every such
+    name unusable: the name itself parsed, and the reference to it did not.
+
+    The rejection landed on the pattern, before the expression parser ran, so
+    these go through the model rather than
+    ``openjd.model._internal._combination_expr.Parser`` (which accepted ``_``
+    all along).
+
+    Whitespace is the other place the pattern and that parser disagree, and it is
+    left disagreeing on purpose: §3.4.3 allows "the space", so U+0020 only, while
+    the shared ``TokenStream`` folds every whitespace run to a space before
+    lexing. Widening the class to ``\\s`` would accept a multi-line expression
+    that openjd-rs rejects, so the pattern stays the narrower, conformant side.
+    ``test_disallowed_characters_still_rejected`` pins that.
+    """
+
+    @staticmethod
+    def _space(names: list[str], combination: str) -> dict[str, Any]:
+        return {
+            "taskParameterDefinitions": [
+                {"name": name, "type": "INT", "range": [1, 2]} for name in names
+            ],
+            "combination": combination,
+        }
+
+    @pytest.mark.parametrize(
+        "names,combination",
+        (
+            pytest.param(
+                ["Frame_Range", "Quality"], "Frame_Range * Quality", id="interior underscore"
+            ),
+            pytest.param(["_Frame", "_Quality"], "(_Frame, _Quality)", id="leading underscore"),
+            pytest.param(["A_", "B_"], "A_ * B_", id="trailing underscore"),
+            pytest.param(["_", "A"], "_ * A", id="name is a bare underscore"),
+            pytest.param(
+                ["A_1", "B_2", "C_3"], "A_1 * ( B_2, C_3 )", id="underscore inside an association"
+            ),
+        ),
+    )
+    def test_underscore_names_accepted(self, names: list[str], combination: str) -> None:
+        # WHEN
+        model = _parse_model(
+            model=StepParameterSpaceDefinition, obj=self._space(names, combination)
+        )
+
+        # THEN the expression is carried through verbatim
+        assert model.combination == combination
+
+    @pytest.mark.parametrize(
+        "combination",
+        (
+            pytest.param("Frame-Range * Quality", id="hyphen"),
+            pytest.param("Frame.Range * Quality", id="dot"),
+            pytest.param("Frame+Range * Quality", id="plus"),
+            pytest.param("Frame\tRange * Quality", id="tab"),
+            pytest.param("Frame *\nRange * Quality", id="newline"),
+        ),
+    )
+    def test_disallowed_characters_still_rejected(self, combination: str) -> None:
+        # Negative control. Widening the class to admit '_' must not admit
+        # anything else, and the rejection must come from the pattern rather than
+        # from the expression parser downstream of it.
+        #
+        # The tab and newline cases are deliberate, not incidental: §3.4.3 allows
+        # "the space", and the shared TokenStream folds all whitespace to U+0020
+        # before lexing, so ``CombinationExpressionParser`` on its own accepts
+        # both. This is the narrower, conformant side of that disagreement, and it
+        # is what openjd-rs does too.
+        # WHEN
+        with pytest.raises(ValidationError) as excinfo:
+            _parse_model(
+                model=StepParameterSpaceDefinition,
+                obj=self._space(["Frame", "Range", "Quality"], combination),
+            )
+
+        # THEN it failed on the character class, and that class is the shared
+        # constant. Asserting the constant rather than the rendered pattern keeps
+        # a deliberate widening of it from breaking a test about hyphens.
+        message = str(excinfo.value)
+        assert "combination" in message, message
+        assert "String should match pattern" in message, message
+        assert _identifier_chars in message, message
+
+    def test_underscore_name_iterates_the_full_parameter_space(self) -> None:
+        # The character class was the only gate, so a template that clears it must
+        # produce the same space as one with underscore-free names: 3 x 2 = 6 tasks
+        # keyed by the underscore-bearing name.
+        # GIVEN
+        template = decode_job_template(
+            template={
+                "specificationVersion": "jobtemplate-2023-09",
+                "name": "T",
+                "steps": [
+                    {
+                        "name": "S",
+                        "parameterSpace": {
+                            "taskParameterDefinitions": [
+                                {"name": "Frame_Range", "type": "INT", "range": "1-3"},
+                                {"name": "Quality", "type": "STRING", "range": ["low", "high"]},
+                            ],
+                            "combination": "Frame_Range * Quality",
+                        },
+                        "script": {"actions": {"onRun": {"command": "echo", "args": ["hi"]}}},
+                    }
+                ],
+            }
+        )
+        job = create_job(job_template=template, job_parameter_values={})
+
+        # WHEN
+        space = StepParameterSpaceIterator(space=job.steps[0].parameterSpace)
+
+        # THEN
+        assert space.names == {"Frame_Range", "Quality"}
+        tasks = [(params["Frame_Range"].value, params["Quality"].value) for params in space]
+        assert tasks == [
+            ("1", "low"),
+            ("1", "high"),
+            ("2", "low"),
+            ("2", "high"),
+            ("3", "low"),
+            ("3", "high"),
+        ]

test/openjd/model_v1/test_step_param_space_def.py

@@ -112,6 +112,25 @@ def test_combination_string(self) -> None:
         )
         assert step.parameter_space.combination == "A * B"
 
+    def test_combination_accepts_underscore_names(self) -> None:
+        """Template Schemas §3.4.3 constraint 1 gives a combination expression
+        the characters of an ``<Identifier>``, and §7.1 puts ``_`` among them.
+
+        Control for the v0 side, where the character class omitted ``_`` and made
+        such names unreferenceable. This path already accepted them, so the two
+        lanes disagreed; this pins the v1 half of the parity.
+        """
+        step = _decode_step(
+            {
+                "taskParameterDefinitions": [
+                    {"name": "Frame_Range", "type": "INT", "range": [1, 2]},
+                    {"name": "_Quality", "type": "STRING", "range": ["x", "y"]},
+                ],
+                "combination": "Frame_Range * _Quality",
+            }
+        )
+        assert step.parameter_space.combination == "Frame_Range * _Quality"
+
     def test_camelcase_alias(self) -> None:
         """``taskParameterDefinitions`` is a camelCase alias for
         ``task_parameter_definitions``."""