diff --git a/src/openjd/model/v2023_09/_model.py b/src/openjd/model/v2023_09/_model.py index 5817ffb7..923af19c 100644 --- a/src/openjd/model/v2023_09/_model.py +++ b/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)", ), ] diff --git a/test/openjd/model_v0/v2023_09/test_parameter_space.py b/test/openjd/model_v0/v2023_09/test_parameter_space.py index aa354b76..70a4f7fe 100644 --- a/test/openjd/model_v0/v2023_09/test_parameter_space.py +++ b/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 ```` plus the space and the operators. §7.1 + puts ``_`` in an ````, 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"), + ] diff --git a/test/openjd/model_v1/test_step_param_space_def.py b/test/openjd/model_v1/test_step_param_space_def.py index 8999f9e7..a26c0837 100644 --- a/test/openjd/model_v1/test_step_param_space_def.py +++ b/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 ````, 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``."""