Conversation
Template Schemas §3.4.3 constraint 1 gives a <CombinationExpr> the characters
allowed in an <Identifier> plus the space and the operators, and §7.1 puts the
underscore among an <Identifier>'s characters. The character class here omitted
it, so a task parameter named Frame_Range was rejected by the pattern before the
expression parser saw it:
steps[0] -> parameterSpace -> combination:
String should match pattern '(?-m:^[A-Za-z0-9\*\(\), ]+\z)'
The name itself was legal, so such a parameter could be declared and never
referenced. Its own comment already claimed the intended relationship to
<Identifier>, so bind both classes to one constant rather than restating it:
the two cannot drift again. The rendered _identifier_regex is unchanged.
The Rust path accepted these all along, so the two lanes disagreed. Add a
control on the v1 side pinning its half of the parity.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Two findings from the automated review. Scope the comment's claim. It said the shared constant meant Identifier and CombinationExpr "cannot drift", which is stronger than the code provides: only the character class is shared, not the leading-character rule. Say what is actually shared, and record why the space is U+0020 alone. Whitespace stays narrow, deliberately. The shared TokenStream folds every whitespace run to a space before lexing, so CombinationExpressionParser on its own accepts a newline that the pattern refuses. §3.4.3 allows "the space", and openjd-rs refuses a newline too, so the pattern is the conformant side and widening it to \s would create a divergence rather than close one. Pin it with a newline case instead of leaving the decision implicit. Assert the shared constant, not the rendered pattern. Hardcoding the expanded regex meant a deliberate widening of _identifier_chars would break a test about hyphens, which is the opposite of what factoring the constant out was for. Verified: widening the constant now leaves the negative controls green, while reverting it, widening to any character, and widening the space to \s are each caught. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
| min_length=1, | ||
| max_length=1280, | ||
| strict=True, | ||
| pattern=rf"(?-m:^[{_identifier_chars}\*\(\), ]+\z)", |
There was a problem hiding this comment.
The canonical character-class test for this exact type was not updated, so the regression this PR fixes is not pinned where a reader would look for it.
test/openjd/model_v0/v2023_09/test_strings.py is where the CombinationExpr constraints are pinned at the type level, via a bare CombinationExprModel with no expression parser downstream. Its positive character-class case, at test_strings.py:605, is:
pytest.param(string.ascii_letters + string.digits, id="allowable identifier chars"),string.ascii_letters + string.digits does not contain _. That is precisely the character this change adds, and it is the case that would have failed before the fix. So after this PR, the file that claims to enumerate the allowable identifier characters for CombinationExpr still enumerates them without _, and its id asserts that set is the identifier characters, which is now false.
Consequence for the mutation argument made elsewhere in this PR: re-hardcoding the pattern back to [A-Za-z0-9\*\(\), ] leaves test_strings.py fully green. Only the new test_parameter_space.py class catches it, and that one goes through StepParameterSpaceDefinition, so its failure surfaces as a parameter-space problem rather than a string-constraint one. The type-level lane, the one with no parser downstream to confound the result, has no coverage of _ at all.
Cheapest fix is one character in the existing param, trivially safe because that model has no parser attached:
pytest.param(string.ascii_letters + string.digits + "_", id="allowable identifier chars"),The same file has a no newline negative case but no tab case. Adding one there would put the whitespace decision recorded in the new comment above next to the other whitespace assertion, rather than only in the parameter-space suite.
3 participants
Fixes: n/a — found while auditing
CombinationExpressionvalidation coverage in a downstream consumerWhat was the problem/requirement? (What/Why)
Template Schemas §3.4.3 constraint 1 gives a
<CombinationExpr>"Any [character] allowed in an<Identifier>plus the space,*,(, and)characters", and §7.1 lists the underscore explicitly among an<Identifier>'s characters. The character class inv2023_09/_model.pyomitted it:So a task parameter named
Frame_Rangecould be declared, but never referenced from a combination expression:The rejection landed on the pattern, before
_internal/_combination_expr.Parserran — that parser accepted underscores all along, since itsIdentTokenis^[^\d\W][\w]*$. The comment above the constraint already claimed the intended relationship to<Identifier>; only the class disagreed.The two decode paths therefore disagreed. Measured on
Frame_Range * Qualitywith both parameters declared:parse_model(model=JobTemplate_2023_09, ...)openjd._openjd_rs.decode_job_template_stropenjd-rsnever had the defect:validate_v2023_09/structure.rshas_in its allow-list and its tokenizer accepts it in names, soopenjd checkpasses the same template and reports the full 6-task space.What was the solution? (How)
Bind the identifier character class to one constant and build both patterns from it, rather than adding
_to the literal and leaving the duplication that let the two drift:The rendered
_identifier_regexis byte-identical to before, checked at runtime, soIdentifierbehaviour is unchanged.What is the impact of this change?
A combination expression may now reference a task parameter whose name contains an underscore, which is every name §7.1 permits. Templates previously rejected are now accepted; nothing previously accepted is now rejected. Consumers that validate a template through this library and had worked around the restriction by renaming parameters no longer need to.
How was this change tested?
hatch run test: 6069 passed, 24 skipped, 3 xfailed, coverage gate met.hatch run lint(ruff, black, mypy) clean.New tests, all mutation-checked:
TestCombinationExprCharacterClassintest/openjd/model_v0/v2023_09/test_parameter_space.py— six accepted cases (interior, leading and trailing underscore, a bare_, and one inside an association), four negative controls (-,.,+, and a tab, which is not the permitted space) asserting the full pattern message, and one end-to-end case decoding a template, creating the job, and assertingStepParameterSpaceIteratoryields the 3 × 2 space keyed byFrame_Range.test_combination_accepts_underscore_namesintest/openjd/model_v1/test_step_param_space_def.py— control pinning the Rust path's half of the parity, so a future tightening there cannot silently reopen the divergence.Two mutants, each applied to the source and the suite re-run:
[A-Za-z0-9\*\(\), ].+Was this change documented?
CombinationExprnow says why the class is shared rather than restating the relationship it previously only asserted.There is no conformance fixture covering this today.
2023-09/base/job_templates/carries a §3.4.3 family including3.4.3--disallowed-char-at-sign.invalid.yaml, but nothing positive on the underscore, which is why the divergence went unnoticed. Both CLIs now agree, so a fixture is warranted; I will raise it separately againstopenjd-specifications.Separately, §3.4.3 constraint 1 omits the comma from its operator list even though
<ExprList>in the grammar directly above requires one. Both implementations allow it. That reads as a spec erratum and I will raise it too.Is this a breaking change?
No. The accepted input set only grows.
Does this change impact security?
No. The change widens a character class to the set the specification defines, and the widened class remains a strict subset of the identifier and operator characters — the negative controls pin that a hyphen, dot, plus and tab are all still rejected. No file or directory handling is involved.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.