Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
leongdl merged 1 commit into from
Sep 25, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 37 additions & 9 deletions src/openjd/model/_internal/_variable_reference_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,10 @@ def _validate_model_template_variable_references(
# The errors that we're collecting for this node in the traversal, and will return from the function call.
errors: list[InitErrorDetails] = []

expr_enabled = bool(
context is not None and getattr(context, "_prevalidation_expr_enabled", False)
)

model_origin = typing.get_origin(model)

# Unwrap the Optional types
Expand Down Expand Up @@ -345,7 +349,9 @@ def _validate_model_template_variable_references(

# Unwrap a discriminated union to the selected type
if model_origin is Union and discriminator is not None:
unioned_model = _get_model_for_singleton_value(model, value, discriminator)
unioned_model = _get_model_for_singleton_value(
model, value, discriminator, expr_enabled=expr_enabled
)
if unioned_model is not None:
return _validate_model_template_variable_references(
unioned_model,
Expand Down Expand Up @@ -398,10 +404,6 @@ def _validate_model_template_variable_references(
# If the node doesn't modify the variable prefix, then symbol_prefix will be the empty string
symbol_prefix += variable_defs.symbol_prefix

expr_enabled = bool(
context is not None and getattr(context, "_prevalidation_expr_enabled", False)
)

# Recursively collect all of the variable definitions at this node and its
# child nodes. Symbol EXPR types are collected only when the EXPR
# extension is active, keeping the Rust expr surface off the non-EXPR
Expand Down Expand Up @@ -709,8 +711,17 @@ def _validate_let_bindings(
return errors


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

@github-actions github-actions Bot Sep 25, 2026

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.



def _get_model_for_singleton_value(
model: Any, value: Any, discriminator: Union[str, Discriminator, None] = None
model: Any,
value: Any,
discriminator: Union[str, Discriminator, None] = None,
*,
expr_enabled: bool = False,
) -> Optional[Type]:
"""Given a FieldInfo and the value that we're given for that field, determine
the actual Model for the value in the event that the FieldInfo may be for
Expand Down Expand Up @@ -756,7 +767,20 @@ def _get_model_for_singleton_value(
raise NotImplementedError(
"You have hit an unimplemented code path. Please report this as a bug."
)
if typing.get_args(sub_model_discr_value)[0] == discr_value:
literal_value = typing.get_args(sub_model_discr_value)[0]
if literal_value == discr_value:
return sub_model
# RFC 0007 §2: parameter type names are case-insensitive when the EXPR
# extension is active. The `_normalize_parameter_type_case` field
# validator uppercases the `type` discriminator before pydantic's
# union resolution, but this prevalidation traversal runs earlier and
# sees the raw values — so match the discriminator case-insensitively
# here, or a lowercase-typed parameter defines no Param.* symbols.
if (
expr_enabled
and isinstance(literal_value, str)
and literal_value.translate(_ASCII_UPPERCASE) == discr_value.translate(_ASCII_UPPERCASE)

@github-actions github-actions Bot Sep 25, 2026

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 ...
):

):
return sub_model

@github-actions github-actions Bot Sep 25, 2026

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.


return None
Expand Down Expand Up @@ -881,9 +905,13 @@ def _collect_variable_definitions( # noqa: C901 (suppress: too complex)
)
return {"__export__": symtab}

# Unwrap a discriminated union to the selected type
# Unwrap a discriminated union to the selected type. `collect_types` is
# set exactly when the EXPR extension is active, which also governs the
# case-insensitive `type` discriminator matching (RFC 0007 §2).
if model_origin is Union and discriminator is not None:
unioned_model = _get_model_for_singleton_value(model, value, discriminator)
unioned_model = _get_model_for_singleton_value(
model, value, discriminator, expr_enabled=collect_types
)
if unioned_model is not None:
return _collect_variable_definitions(
unioned_model,
Expand Down
15 changes: 15 additions & 0 deletions test/openjd/model_v0/v2023_09/test_environment_template.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,21 @@ def test_with_expr_miscased_accepted(self, canonical: str, miscased: str) -> Non
# which is the state an audit found untested.
self._decode(self._tmpl(miscased))

@pytest.mark.parametrize("canonical, miscased", TYPES)
def test_with_expr_miscased_param_referenceable(self, canonical: str, miscased: str) -> None:
# A non-canonically-cased type name is just as valid under EXPR, so it
# must still define Param.P for the variable-reference
# prevalidation, which resolves the raw `type` discriminator before the
# case fold runs. Regression: the definition was accepted but every
# {{ Param.P }} reference was rejected with "does not exist at this
# location".
template = self._tmpl(miscased)
template["environment"] = {
"name": "E",
"script": {"actions": {"onEnter": {"command": "echo", "args": ["{{ Param.P }}"]}}},
}
self._decode(template)

@pytest.mark.parametrize("type_name", ("\u0131NT", "\u017fTRING", "\ufb02OAT"))
def test_non_ascii_lookalike_rejected_with_expr(self, type_name: str) -> None:
# str.upper() folds U+0131 to 'I', U+017F to 'S' and U+FB02 (fl) to 'FL',
Expand Down
43 changes: 43 additions & 0 deletions test/openjd/model_v0/v2023_09/test_list_parameters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,49 @@ def test_with_expr_miscased_accepted(self, canonical, miscased, default):
# Case 4 of 4.
_decode(_tmpl(self._param(miscased, default)))

# ── Mis-cased parameters are referenceable ──

@staticmethod
def _referencing_step():
return {
"name": "S",
"script": {
"actions": {
"onRun": {"command": "echo", "args": ["{{ Param.P }}", "{{ RawParam.P }}"]}
}
},
}

@pytest.mark.parametrize("canonical, miscased, default", TYPES)
def test_with_expr_miscased_param_referenceable(self, canonical, miscased, default):
# A non-canonically-cased type name is just as valid under EXPR, so it
# must still define Param.P/RawParam.P for the
# variable-reference prevalidation, which resolves the raw `type`
# discriminator before the case fold runs. Regression: the parameter
# declaration was accepted but every {{ Param.P }} reference was
# rejected with "does not exist at this location".
template = _tmpl(self._param(miscased, default))
template["steps"] = [self._referencing_step()]
_decode(template)

def test_with_expr_miscased_param_type_checked(self):
# The reference prevalidation also collects the parameter's EXPR type
# through the lowercase spelling: string + int is a type error.
template = _tmpl(self._param("string", "hi"))
template["steps"] = [
{
"name": "S",
"script": {
"actions": {"onRun": {"command": "echo", "args": ["{{ Param.P + 1 }}"]}}
},
}
]
with pytest.raises(DecodeValidationError) as excinfo:
_decode(template)
assert "steps[0] -> script -> actions -> onRun -> args[0]" in str(excinfo.value), str(
excinfo.value
)

# ── The fold is ASCII ──

# str.upper() folds each of these wholly into the type-name alphabet: U+0131
Expand Down
16 changes: 16 additions & 0 deletions test/openjd/model_v0/v2023_09/test_parameter_space.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -900,6 +900,22 @@ def test_with_expr_miscased_accepted(
self._tmpl(miscased, range_value, chunks, self._extensions_for(canonical, expr=True))
)

@pytest.mark.parametrize("canonical, miscased, range_value, chunks", TYPES)
def test_with_expr_miscased_param_referenceable(
self, canonical: str, miscased: str, range_value: Any, chunks: Any
) -> None:
# A non-canonically-cased type name is just as valid under EXPR, so it
# must still define Task.Param.F for the
# variable-reference prevalidation, which resolves the raw `type`
# discriminator before the case fold runs. Regression: the definition
# was accepted but every {{ Task.Param.F }} reference was rejected
# with "does not exist at this location".
template = self._tmpl(
miscased, range_value, chunks, self._extensions_for(canonical, expr=True)
)
template["steps"][0]["script"]["actions"]["onRun"]["args"] = ["{{ Task.Param.F }}"]
self._decode(template)

# ── The gate is on EXPR, not on the extension that supplies the type ──

def test_chunk_int_miscased_needs_expr_not_only_task_chunking(self) -> None:
Expand Down
Loading