Skip to content

fix: resolve parameter type names case-insensitively in reference validation - #372

Merged
leongdl merged 1 commit into
OpenJobDescription:mainlinefrom
mwiebe:fix/expr-type-name-case-param-references
Sep 25, 2026
Merged

leongdl merged 1 commit into
OpenJobDescription:mainlinefrom
mwiebe:fix/expr-type-name-case-param-references

Conversation

@mwiebe

@mwiebe mwiebe commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

What was the problem/requirement? (What/Why)

The EXPR extension makes job and task parameter type names case-insensitive (RFC 0007 §2), so type: string is exactly as valid a spelling as type: STRING. The pure-Python model accepted such a declaration, but then rejected every reference to the parameter:

specificationVersion: jobtemplate-2023-09
extensions: [EXPR]
name: T
parameterDefinitions:
- {name: Msg, type: string, default: hi}
steps:
- name: S
  script:
    actions:
      onRun: {command: echo, args: ['{{ Param.Msg }}']}
steps[0] -> script -> actions -> onRun -> args[0]:
    Variable Param.Msg does not exist at this location.

The same template with type: STRING passed, 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_case field validator folds the type discriminator to upper case. In _variable_reference_validation.py, _get_model_for_singleton_value resolved the type-discriminated parameter-definition union with an exact string comparison against the Literal tags (STRING, INT, …). 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 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 existing collect_types flag — already set exactly when EXPR is active.

The fold is ASCII-only, using the same str.maketrans approach as the existing field validator, so Unicode look-alikes such as ıNT (U+0131 folds to I under the Unicode-aware str.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 as Param.P / RawParam.P, plus a check that its EXPR type is still collected through the lowercase spelling ({{ Param.P + 1 }} on a string parameter is still rejected as a type error).
  • test_parameter_space.py::TestTaskParameterTypeNameCase — the same for Task.Param.F.
  • test_environment_template.py::TestEnvironmentTemplateParameterTypeNameCase — the same for an environment template's Param.P.

Each new test fails before this change with Variable ... does not exist at this location. and passes after.

  • Have you run the unit tests? Yes. hatch run test-subset test/openjd/model_v0 → 2900 passed. hatch run lint, hatch run fmt, and hatch run typing are all clean.

There are no Rust changes, so rust-bindings is untouched and no stub regeneration is needed. Note for reviewers: a full hatch run test in my working copy shows pre-existing failures under expr / model_v1 / sessions caused by a stale locally-built _openjd_rs extension. I confirmed those reproduce on unmodified mainline and are unrelated to this change.

Was this change documented?

  • Are relevant docstrings in the code base updated? Yes. _get_model_for_singleton_value gained an expr_enabled keyword 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.

@mwiebe
mwiebe requested a review from a team as a code owner September 25, 2026 18:30
…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>
@mwiebe
mwiebe force-pushed the fix/expr-type-name-case-param-references branch from 9a80cb9 to 811838f Compare September 25, 2026 18:37
@leongdl
leongdl merged commit ff0c08c into OpenJobDescription:mainline Sep 25, 2026
31 checks passed
if (
expr_enabled
and isinstance(literal_value, str)
and literal_value.translate(_ASCII_UPPERCASE) == discr_value.translate(_ASCII_UPPERCASE)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 comparison

That also makes the invariant "exact spelling always wins" explicit rather than dependent on union declaration order.

@mwiebe
mwiebe deleted the fix/expr-type-name-case-param-references branch September 25, 2026 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants