fix: resolve parameter type names case-insensitively in reference validation - #372
Conversation
…idation
The EXPR extension makes job and task parameter type names
case-insensitive (RFC 0007 §2), so `type: string` is exactly as valid as
`type: STRING`. The model accepted such a declaration but then rejected
every reference to the parameter:
steps[0] -> script -> actions -> onRun -> args[0]:
Variable Param.Msg does not exist at this location.
The variable-reference prevalidation pass runs on raw template values,
before the field validator that folds the `type` discriminator to upper
case, and it resolved the type-discriminated parameter-definition union
with an exact string comparison. A lowercase or mixed-case type name
matched no member of the union, so the definition contributed none of its
`Param.*` / `RawParam.*` / `Task.Param.*` symbols and none of its EXPR
type information.
Compare the discriminator with an ASCII-only case fold when EXPR is
active, in both the reference-validation and the definition-collection
traversal. Job, task, and environment-template parameters now define
their symbols and types whatever the case of the type name. The fold is
ASCII-only so Unicode look-alikes such as `ıNT` stay rejected, matching
the field validator.
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
9a80cb9 to
811838f
Compare
| if ( | ||
| expr_enabled | ||
| and isinstance(literal_value, str) | ||
| and literal_value.translate(_ASCII_UPPERCASE) == discr_value.translate(_ASCII_UPPERCASE) |
There was a problem hiding this comment.
The case-insensitive fallback is applied to every string-Literal discriminated union reached during the traversal, but RFC 0007 §2 only makes parameter type names case-insensitive. Today the only str-discriminated unions in the model are JobParameterDefinitionList and TaskParameterList (both keyed on type), and _normalize_parameter_type_case is registered only for those three fields — so there is no behavioural difference right now.
The risk is forward-looking, and it is a silent divergence rather than an error: if any future union is discriminated on some other Literal[str] field, this traversal would resolve {"mode": "terminate"} vs "TERMINATE" case-insensitively while pydantic (which has no normalizer for that field) resolves it case-sensitively. The prevalidation pass would then collect symbols from a different sub-model than the one actually constructed, which is the kind of mismatch that produces confusing "does not exist at this location" / missing-error behaviour rather than a clean failure.
Scoping it to the field the normalizer actually covers keeps the two resolutions in lockstep:
if (
expr_enabled
and discriminator == "type"
and isinstance(literal_value, str)
and ...
):|
|
||
| # Parameter type names are ASCII (RFC 0007 §2). str.upper() is Unicode-aware | ||
| # and folds U+0131 to 'I', which would make 'ıNT' a spelling of 'INT'. | ||
| _ASCII_UPPERCASE = str.maketrans("abcdefghijklmnopqrstuvwxyz", "ABCDEFGHIJKLMNOPQRSTUVWXYZ") |
There was a problem hiding this comment.
This _ASCII_UPPERCASE table (plus its comment) is a byte-for-byte duplicate of src/openjd/model/v2023_09/_model.py:4012, and the two now have to agree for correctness — this PR exists precisely because the prevalidation fold and the pydantic-side fold disagreed. Leaving two independent copies re-creates that hazard: if someone later widens/narrows the fold in one module (say, to handle a new bracketed type name or to switch to casefold), prevalidation and union resolution silently diverge again and the failure mode is the same confusing "does not exist at this location" rather than a test that obviously breaks.
Since v2023_09/_model.py already imports from _internal, the dependency direction allows the constant to live here (or in a small shared helper under _internal) and be imported by _model.py, so there is exactly one definition of what "the fold" means.
Worth noting there is a third fold in play: expr_type_for_openjd_type / the Rust job_parameter_type_expr_spec is documented as case-insensitive and does its own normalization. That one is fine to keep separate since it lives behind an API boundary, but it does mean the invariant "all three folds agree on which spellings are equivalent" is currently unpinned by any test.
| and isinstance(literal_value, str) | ||
| and literal_value.translate(_ASCII_UPPERCASE) == discr_value.translate(_ASCII_UPPERCASE) | ||
| ): | ||
| return sub_model |
There was a problem hiding this comment.
Ordering issue: the case-insensitive fallback is evaluated inside the same loop iteration as the exact-match check, so a loose match on an earlier union member wins over an exact match on a later one.
Concretely, for a union ordered [A: Literal["int"], B: Literal["INT"]] and discr_value == "INT", iteration 1 fails the literal_value == discr_value test for A, then immediately succeeds on the folded comparison and returns A — B, the exact match, is never reached. Pydantics own resolution (which sees the normalized "INT") would pick B, so prevalidation would collect symbols from a different sub-model than the one actually constructed.
No current union has two members whose type literals differ only in ASCII case, so this is latent rather than live. But it is cheap to make order-independent by running the exact pass to completion first:
for sub_model in typing.get_args(model):
...
if literal_value == discr_value:
return sub_model
if not expr_enabled:
return None
folded = discr_value.translate(_ASCII_UPPERCASE)
for sub_model in typing.get_args(model):
... # second pass, folded comparisonThat also makes the invariant "exact spelling always wins" explicit rather than dependent on union declaration order.
What was the problem/requirement? (What/Why)
The EXPR extension makes job and task parameter type names case-insensitive (RFC 0007 §2), so
type: stringis exactly as valid a spelling astype: STRING. The pure-Python model accepted such a declaration, but then rejected every reference to the parameter:The same template with
type: STRINGpassed, and the Rust implementation accepted both spellings.The cause is an ordering issue. The variable-reference prevalidation pass runs on raw template values, before the
_normalize_parameter_type_casefield validator folds thetypediscriminator to upper case. In_variable_reference_validation.py,_get_model_for_singleton_valueresolved thetype-discriminated parameter-definition union with an exact string comparison against theLiteraltags (STRING,INT, …). A lowercase or mixed-case type name matched no member of the union, so the definition contributed none of itsParam.*/RawParam.*/Task.Param.*symbols, and none of its EXPR type information either.What was the solution? (How)
Compare the discriminator with an ASCII-only case fold when EXPR is active, in both traversals in
_variable_reference_validation.py:_validate_model_template_variable_references, which reads the EXPR flag off the parsing context (_prevalidation_expr_enabled); and_collect_variable_definitions, which reuses the existingcollect_typesflag — already set exactly when EXPR is active.The fold is ASCII-only, using the same
str.maketransapproach as the existing field validator, so Unicode look-alikes such asıNT(U+0131 folds toIunder the Unicode-awarestr.upper()) stay rejected.What is the impact of this change?
Templates that declare parameter types in lower or mixed case under EXPR can now reference those parameters, matching the Rust implementation and the specification. Tools that validate templates with the Python model no longer reject otherwise-valid templates. Parameters also correctly contribute their EXPR types, so expressions over them are type-checked rather than falling back to name-only checking.
Behaviour is unchanged when EXPR is not active: type names remain case-sensitive there. No public API changes.
How was this change tested?
Unit tests were added to the three existing type-case test classes, each parameterised over the type names those classes already cover:
test_list_parameters.py::TestJobParameterTypeNameCase— a job parameter declared with any casing is referenceable asParam.P/RawParam.P, plus a check that its EXPR type is still collected through the lowercase spelling ({{ Param.P + 1 }}on astringparameter is still rejected as a type error).test_parameter_space.py::TestTaskParameterTypeNameCase— the same forTask.Param.F.test_environment_template.py::TestEnvironmentTemplateParameterTypeNameCase— the same for an environment template'sParam.P.Each new test fails before this change with
Variable ... does not exist at this location.and passes after.hatch run test-subset test/openjd/model_v0→ 2900 passed.hatch run lint,hatch run fmt, andhatch run typingare all clean.There are no Rust changes, so
rust-bindingsis untouched and no stub regeneration is needed. Note for reviewers: a fullhatch run testin my working copy shows pre-existing failures underexpr/model_v1/sessionscaused by a stale locally-built_openjd_rsextension. I confirmed those reproduce on unmodifiedmainlineand are unrelated to this change.Was this change documented?
_get_model_for_singleton_valuegained anexpr_enabledkeyword argument, documented with a comment explaining the prevalidation ordering that makes the case-insensitive match necessary, and the new tests carry comments describing the regression they pin. No user-facing documentation needed updating: this restores the behaviour the specification already describes, rather than changing a documented contract.Is this a breaking change?
No.
Does this change impact security?
No.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.