diff --git a/Cargo.lock b/Cargo.lock index 57679de1..d4389f56 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -724,9 +724,9 @@ checksum = "9f7c3e4beb33f85d45ae3e3a1792185706c8e16d043238c593331cc7cd313b50" [[package]] name = "openjd-expr" -version = "0.9.0" +version = "0.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2b016bd852c294b6a55a632e794bb150a305cf04529ba98c0255f57edefbda9c" +checksum = "098bbd32e8479e2ac55ee8094a3909b7cc9a8e73a68ee3ba48d36524ab35a003" dependencies = [ "regex", "regex-syntax", @@ -741,9 +741,9 @@ dependencies = [ [[package]] name = "openjd-model" -version = "0.9.0" +version = "0.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "655c4a740ee46f502c993fe0dcd334f442721dfbc2aa52803b576f5370ea69d4" +checksum = "c25b419619695aa093312bec01a8f6969fc207a4b976883f4b3a465243d0a595" dependencies = [ "indexmap", "openjd-expr", @@ -773,9 +773,9 @@ dependencies = [ [[package]] name = "openjd-sessions" -version = "0.7.0" +version = "0.7.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1c4f7afee55eaf0cd1b7dafa55203386283e614088a94379aca213a6efa68e25" +checksum = "b8578d15f0d54cc1cde9a85f6babe30a0c4b419d824c05f27aaa200cbcc79d1f" dependencies = [ "bitflags", "futures-util", diff --git a/THIRD-PARTY-LICENSES.txt b/THIRD-PARTY-LICENSES.txt index 8e53a0ca..2f16c76c 100644 --- a/THIRD-PARTY-LICENSES.txt +++ b/THIRD-PARTY-LICENSES.txt @@ -2741,9 +2741,9 @@ limitations under the License. ** itoa; version 1.0.18 -- https://crates.io/crates/itoa ** libc; version 0.2.189 -- https://crates.io/crates/libc ** manyhow-macros; version 0.11.4 -- https://crates.io/crates/manyhow-macros -** openjd-expr; version 0.9.0 -- https://crates.io/crates/openjd-expr -** openjd-model; version 0.9.0 -- https://crates.io/crates/openjd-model -** openjd-sessions; version 0.7.0 -- https://crates.io/crates/openjd-sessions +** openjd-expr; version 0.10.0 -- https://crates.io/crates/openjd-expr +** openjd-model; version 0.10.0 -- https://crates.io/crates/openjd-model +** openjd-sessions; version 0.7.1 -- https://crates.io/crates/openjd-sessions ** pin-project-lite; version 0.2.17 -- https://crates.io/crates/pin-project-lite ** portable-atomic; version 1.15.0 -- https://crates.io/crates/portable-atomic ** proc-macro2; version 1.0.107 -- https://crates.io/crates/proc-macro2 diff --git a/rust-bindings/Cargo.toml b/rust-bindings/Cargo.toml index cb094f5f..bf5bc60c 100644 --- a/rust-bindings/Cargo.toml +++ b/rust-bindings/Cargo.toml @@ -12,9 +12,9 @@ name = "_openjd_rs" crate-type = ["cdylib", "rlib"] [dependencies] -openjd-expr = "0.9.0" -openjd-model = "0.9.0" -openjd-sessions = "0.7.0" +openjd-expr = "0.10.0" +openjd-model = "0.10.0" +openjd-sessions = "0.7.1" tokio = { version = "1", features = ["rt-multi-thread"] } uuid = { version = "1", features = ["v4"] } serde_json = "1" diff --git a/rust-bindings/src/model/template_types.rs b/rust-bindings/src/model/template_types.rs index bd8ccd74..4f3f551d 100644 --- a/rust-bindings/src/model/template_types.rs +++ b/rust-bindings/src/model/template_types.rs @@ -979,19 +979,25 @@ pub(crate) struct PyAmountRequirement { impl PyAmountRequirement { #[new] #[pyo3(signature = (*, name, min=None, max=None))] - fn new(name: String, min: Option, max: Option) -> Self { + fn new(name: PyFormatString, min: Option, max: Option) -> Self { PyAmountRequirement { inner: AmountRequirement { - name, + name: name.inner, min: min.map(|fs| fs.inner), max: max.map(|fs| fs.inner), }, } } + /// `@fmtstring` as of openjd-rs#409: a name may carry expressions, and its + /// §3.3.1.1 constraints are checked on the resolved value. Use `.raw()` for + /// the template text. The job-side `AmountRequirement.name` is the resolved + /// `str`. #[getter] - fn name(&self) -> &str { - &self.inner.name + fn name(&self) -> PyFormatString { + PyFormatString { + inner: self.inner.name.clone(), + } } #[getter] @@ -1011,7 +1017,7 @@ impl PyAmountRequirement { } fn __repr__(&self) -> String { - format!("AmountRequirement(name={:?})", self.inner.name) + format!("AmountRequirement(name={:?})", self.inner.name.raw()) } #[allow(clippy::type_complexity)] @@ -1054,22 +1060,28 @@ impl PyAttributeRequirement { #[new] #[pyo3(signature = (*, name, any_of=None, all_of=None))] fn new( - name: String, + name: PyFormatString, any_of: Option>, all_of: Option>, ) -> Self { PyAttributeRequirement { inner: AttributeRequirement { - name, + name: name.inner, any_of: any_of.map(|v| v.into_iter().map(|fs| fs.inner).collect()), all_of: all_of.map(|v| v.into_iter().map(|fs| fs.inner).collect()), }, } } + /// `@fmtstring` as of openjd-rs#409: a name may carry expressions, and its + /// §3.3.2.1 constraints are checked on the resolved value. Use `.raw()` for + /// the template text. The job-side `AttributeRequirement.name` is the + /// resolved `str`. #[getter] - fn name(&self) -> &str { - &self.inner.name + fn name(&self) -> PyFormatString { + PyFormatString { + inner: self.inner.name.clone(), + } } #[getter] @@ -1103,7 +1115,7 @@ impl PyAttributeRequirement { } fn __repr__(&self) -> String { - format!("AttributeRequirement(name={:?})", self.inner.name) + format!("AttributeRequirement(name={:?})", self.inner.name.raw()) } #[allow(clippy::type_complexity)] diff --git a/specs/python-expr-interface.md b/specs/python-expr-interface.md index b0726fa6..63f0a72e 100644 --- a/specs/python-expr-interface.md +++ b/specs/python-expr-interface.md @@ -255,6 +255,27 @@ never raises (Python convention). This makes unresolved values safe to inspect in debuggers and tracebacks while still failing loudly anywhere a real value is expected. +**How an unresolved operand propagates.** Three rules are worth stating, +because each decides whether an expression errors or concludes unresolved, +and all three changed in openjd-expr 0.10.0 (openjd-rs#407): + +* A conditional whose test is unresolved evaluates both branches. A *value* + error in one branch is absorbed — run time may select the healthy one — but + a memory or operation *budget* exceedance propagates, because the budget was + spent in this evaluation whichever branch run time takes. +* `and` / `or` behave the same way for operands after the first unresolved + one: value errors are suppressed for the same short-circuit reason, budget + exceedances are not. +* A list comprehension whose filter evaluates unresolved concludes + `unresolved[list[T]]` rather than erroring, whether the iterable is + unresolved or concrete. Per-element inclusion is undecidable, so the + elements accumulated so far are abandoned. `T` is the body's type derived + under an *unresolved* loop variable, so evaluating the body on an element the + run-time filter may exclude cannot raise a spurious value error: + `[10 // x for x in [0, 2] if x > N]` with `N` unresolved is + `unresolved[list[int]]`, not a division-by-zero. A filter whose type can + never be a boolean is still an error. + ### `SymbolTable` Hierarchical key-value store providing variable bindings for expression diff --git a/specs/python-model-interface.md b/specs/python-model-interface.md index f20726dd..4f06d738 100644 --- a/specs/python-model-interface.md +++ b/specs/python-model-interface.md @@ -795,9 +795,10 @@ cancel.notify_period_in_seconds # Optional[int] Job-time host requirements. Distinct from the template-time ``HostRequirements`` / ``AmountRequirement`` / ``AttributeRequirement`` (see `openjd.model._v1.template`): the template-time variants carry -unresolved ``FormatString`` values for ``min`` / ``max`` / ``anyOf`` / -``allOf``, while these job-time variants carry the post-``create_job`` -resolved ``f64`` (amounts) and ``str`` (attributes) values. +unresolved ``FormatString`` values for ``name``, ``min`` / ``max`` / ``anyOf`` +/ ``allOf``, while these job-time variants carry the post-``create_job`` +resolved ``str`` name and the resolved ``f64`` (amounts) and ``str`` +(attributes) values. ```python hr = step.host_requirements # alias: step.hostRequirements @@ -955,16 +956,38 @@ hr.amounts # Optional[list[AmountRequirement]] hr.attributes # Optional[list[AttributeRequirement]] amt = hr.amounts[0] -amt.name # str +amt.name # FormatString — raw, may hold expressions amt.min # Optional[FormatString] amt.max # Optional[FormatString] attr = hr.attributes[0] -attr.name # str +attr.name # FormatString — raw, may hold expressions attr.any_of # Optional[list[FormatString]] (alias: anyOf) attr.all_of # Optional[list[FormatString]] (alias: allOf) ``` +`name` is `@fmtstring` (§3.3.1, §3.3.2): `"{{Param.FleetAttribute}}"` is a +valid template-time name, and the §3.3.1.1 / §3.3.2.1 capability-name +constraints apply to the *resolved* name. Which stage applies them depends on +when the name's value becomes known: + +| Name | Checked at | Applied | +|---|---|---| +| literal | decode | pattern, length, reserved scopes, uniqueness, standard-capability values | +| fully static — `{{ 'attr.custom.x' }}`, or built only from `let` bindings with literal values | decode | the same set, against the resolved text | +| partly static — `"amount.custom.<95 chars>{{Param.X}}"` | decode | the length *lower bound*: `resolves to at least 109 characters, exceeding the maximum of 100.` | +| parameter-dependent | `create_job` | pattern, length, reserved scopes, case-insensitive uniqueness within `amounts` and within `attributes`, and the standard-capability value rules the resolved name selects | + +A name is resolved at job creation, so only symbols available there are in +scope for it: `Task.Param.*` in a name is rejected at decode as an undefined +variable even where the same symbol is valid elsewhere in the step. + +`name` is an `openjd.expr.FormatString`, like every other FormatString-typed +field on the template types, so reading the template text needs `.raw()` and +constructing one needs `FormatString(...)` rather than a bare `str`. On 0.9.0 +it was a `str` in both directions. The job-side `AmountRequirement.name` / +`AttributeRequirement.name` are still `str`: they hold the resolved name. + ### `StepDependency` ```python @@ -1372,25 +1395,49 @@ template = decode_job_template(template={...}, supported_extensions=["EXPR"]) # 2. Read the template's declared profile back out. profile = template.profile # ModelProfile(revision=V2023_09, extensions=[EXPR]) -profile.revision # SpecificationRevision.V2023_09 +profile.revision # SpecificationRevision.v2023_09 profile.extensions # [ModelExtension.EXPR] profile.has_extension(ModelExtension.EXPR) # True -# 3. Build it manually if needed (e.g. when validating against a different -# policy than the template declared). +# 3. Build it manually if needed (e.g. to enable an extension the template +# does not declare, or to carry caller limits). manual = ModelProfile(extensions=[ModelExtension.EXPR, ModelExtension.TASK_CHUNKING]) -ModelProfile.from_strings(SpecificationRevision.V2023_09, ["EXPR"]) +ModelProfile.from_strings(SpecificationRevision.v2023_09, ["EXPR"]) -# 4. Pass to create_job through a ValidationContext if you want to -# override the template's default validation context. +# 4. Pass to create_job through a ValidationContext, usually to attach +# caller limits. The context must COVER the template: same revision, and +# every extension the template declares (enabling more is fine). limits = CallerLimits(max_step_count=100, max_task_count=10_000) -ctx = ValidationContext(profile, caller_limits=limits) +ctx = ValidationContext(template.profile, caller_limits=limits) job = create_job( job_template=template, job_parameter_values={...}, validation_context=ctx, # optional; defaults to template.default_validation_context() ) +``` +A context that strips an extension the template declares raises +`ModelValidationError`: +`create_job requires a context enabling every extension the template declares: +missing EXPR.` An application that does not support an extension rejects the +template at decode, via `supported_extensions`, rather than at job creation. +Deriving the context from `template.profile` — or omitting it — satisfies the +contract by construction. + +Because the context contract makes every evaluation error at job creation a +real defect, `create_job` reports them all. A value-dependent failure — +`args: ["{{ 10 // Param.N }}"]` with `N = 0` — fails `create_job` rather than +every session that runs the task. Lowered `max_eval_memory_bytes` / +`max_eval_operations` are enforced inside a conditional whose test only a +worker can resolve, which is the idiomatic construction for one. + +Validation outcomes do not depend on the host operating system. Every stage +that evaluates outside host context — template validation and every resolution +`create_job` performs, including its resolved-value re-checks — evaluates +under the POSIX path format, so a PATH value flowing through a `let` binding +into an argument validates identically on Windows and POSIX. + +```python # 5. Bridge to the expression engine. from openjd.expr import HostContext expr_profile = profile.to_expr_profile(HostContext.unresolved()) diff --git a/src/openjd/_openjd_rs.pyi b/src/openjd/_openjd_rs.pyi index ea583441..d99ad304 100644 --- a/src/openjd/_openjd_rs.pyi +++ b/src/openjd/_openjd_rs.pyi @@ -2772,7 +2772,14 @@ class TemplateAction: @typing.final class TemplateAmountRequirement: @property - def name(self) -> builtins.str: ... + def name(self) -> FormatString: + r""" + `@fmtstring` as of openjd-rs#409: a name may carry expressions, and its + §3.3.1.1 constraints are checked on the resolved value. Use `.raw()` for + the template text. The job-side `AmountRequirement.name` is the resolved + `str`. + """ + @property def min(self) -> typing.Optional[FormatString]: ... @property @@ -2780,7 +2787,7 @@ class TemplateAmountRequirement: def __new__( cls, *, - name: builtins.str, + name: FormatString, min: typing.Optional[FormatString] = None, max: typing.Optional[FormatString] = None, ) -> TemplateAmountRequirement: ... @@ -2790,7 +2797,14 @@ class TemplateAmountRequirement: @typing.final class TemplateAttributeRequirement: @property - def name(self) -> builtins.str: ... + def name(self) -> FormatString: + r""" + `@fmtstring` as of openjd-rs#409: a name may carry expressions, and its + §3.3.2.1 constraints are checked on the resolved value. Use `.raw()` for + the template text. The job-side `AttributeRequirement.name` is the + resolved `str`. + """ + @property def any_of(self) -> typing.Optional[builtins.list[FormatString]]: ... @property @@ -2802,7 +2816,7 @@ class TemplateAttributeRequirement: def __new__( cls, *, - name: builtins.str, + name: FormatString, any_of: typing.Optional[typing.Sequence[FormatString]] = None, all_of: typing.Optional[typing.Sequence[FormatString]] = None, ) -> TemplateAttributeRequirement: ... diff --git a/test/openjd/expr/test_unresolved_eval.py b/test/openjd/expr/test_unresolved_eval.py index 8e8e7937..71bbd402 100644 --- a/test/openjd/expr/test_unresolved_eval.py +++ b/test/openjd/expr/test_unresolved_eval.py @@ -723,3 +723,188 @@ def test_type_error_before_unknown_not_suppressed(self) -> None: ] ) assert str(exc_info.value) == expected + + +class TestConcreteIterableUnresolvedFilter: + """openjd-expr 0.10.0 (landed with openjd-rs#407) made ``eval_listcomp``'s two + paths agree: when a filter condition evaluates unresolved on a *concrete* + element, per-element inclusion is undecidable, so the comprehension as a whole + concludes ``unresolved[list[T]]``. + + On 0.9.0 every concrete-iterable case below raised + ``List comprehension filter must be a boolean, got unresolved[bool]``; the + unresolved-iterable control already behaved this way. That made a template like + ``args: ["{{ [f for f in Param.Files.split(',') if f != Task.Param.Skip] }}"]`` + pass ``openjd check`` (where the iterable is unresolved, the tolerant path), run + cleanly on a worker (where everything is bound), and fail only at job creation — + the one stage where the iterable is concrete and the filter is not. + + Extends ``TestUnknownListComprehensions`` above, which covers the + unresolved-iterable path. + """ + + def test_concrete_list_iterable(self) -> None: + values = SymbolTable({"Skip": ExprValue.unresolved(ExprType("string"))}) + result = evaluate_expression("[f for f in 'a,b,c'.split(',') if f != Skip]", values=values) + assert result.type == ExprType("unresolved[list[string]]") + + def test_concrete_range_iterable(self) -> None: + values = SymbolTable({"N": ExprValue.unresolved(ExprType("int"))}) + result = evaluate_expression("[x * 2 for x in range(3) if x > N]", values=values) + assert result.type == ExprType("unresolved[list[int]]") + + def test_the_body_type_is_derived_under_an_unresolved_loop_variable(self) -> None: + """``10 // 0`` would raise if the body were evaluated on the concrete element + ``0``, which the run-time filter may exclude. Deriving the body type with the + loop variable unresolved avoids that spurious error, so this case is the one + that distinguishes "type the body once, abstractly" from "evaluate the body on + each element".""" + values = SymbolTable({"N": ExprValue.unresolved(ExprType("int"))}) + result = evaluate_expression("[10 // x for x in [0, 2] if x > N]", values=values) + assert result.type == ExprType("unresolved[list[int]]") + + def test_a_filter_short_circuiting_before_the_unresolved_term(self) -> None: + """For ``x = 0`` the filter is decided concretely (``0 > 0`` is false); for + ``x = 1`` it is unresolved. The elements already accumulated are abandoned + rather than returned as a partial list.""" + values = SymbolTable({"B": ExprValue.unresolved(ExprType("bool"))}) + result = evaluate_expression("[x for x in [0, 1, 2] if x > 0 and B]", values=values) + assert result.type == ExprType("unresolved[list[int]]") + + def test_a_filter_that_can_never_be_a_boolean_is_still_rejected(self) -> None: + """Negative control. The change is not "stop checking the filter type" — a + filter whose type can never be a boolean errors on both paths. A mutation that + widened the fix to swallow this would pass every case above.""" + values = SymbolTable({"Skip": ExprValue.unresolved(ExprType("string"))}) + with pytest.raises(ExpressionError) as exc_info: + evaluate_expression("[x for x in [1, 2] if Skip]", values=values) + assert "List comprehension filter must be a boolean, got string" in str(exc_info.value) + + def test_the_unresolved_iterable_path_still_concludes_unresolved(self) -> None: + """Negative control for the shared helper: the path that already behaved this + way on 0.9.0 still does, so the two paths agree rather than having swapped.""" + values = SymbolTable( + {"X": ExprValue.unresolved("list[int]"), "N": ExprValue.unresolved("int")} + ) + result = evaluate_expression("[x for x in X if x > N]", values=values) + assert result.type == ExprType("unresolved[list[int]]") + + +class TestBoolOpBudgetErrorsPropagate: + """``eval_boolop`` suppressed *every* error in an operand after an unresolved one, + including budget exceedances — the same bypass ``eval_ifexp`` had. openjd-expr + 0.10.0 (with openjd-rs#407) propagates budget errors and keeps suppressing value + errors, since a run-time short-circuit may skip them. + + On 0.9.0 both budget cases returned ``unresolved[bool]``, so a caller who lowered + a budget had it silently stop applying. Not named in the openjd-rs 0.10.0 + changelog. + """ + + _OPERATORS = [pytest.param("and", id="and"), pytest.param("or", id="or")] + + @staticmethod + def _values() -> SymbolTable: + return SymbolTable({"Flag": ExprValue.unresolved(ExprType("bool"))}) + + @staticmethod + def _expr(operator: str) -> str: + return f"Flag {operator} len('A' * 100000) > 0" + + @pytest.mark.parametrize("operator", _OPERATORS) + def test_an_operation_budget_exceedance_propagates(self, operator: str) -> None: + with pytest.raises(ExpressionError) as exc_info: + evaluate_expression(self._expr(operator), values=self._values(), operation_limit=5) + assert "operation count" in str(exc_info.value) + assert "exceeded limit (5)" in str(exc_info.value) + + @pytest.mark.parametrize("operator", _OPERATORS) + def test_a_memory_budget_exceedance_propagates(self, operator: str) -> None: + with pytest.raises(ExpressionError) as exc_info: + evaluate_expression(self._expr(operator), values=self._values(), memory_limit=1024) + assert "memory usage" in str(exc_info.value) + assert "exceeded limit (1024 bytes)" in str(exc_info.value) + + @pytest.mark.parametrize("operator", _OPERATORS) + def test_a_budget_error_inside_a_nested_conditional_still_propagates( + self, operator: str + ) -> None: + """Both exemptions have to compose. The budget error is raised inside an + ``ifexp`` whose test is unresolved, which is itself an operand after an + unresolved ``and`` / ``or`` operand, so it passes through two absorption + points.""" + with pytest.raises(ExpressionError) as exc_info: + evaluate_expression( + f"Flag {operator} ('A' * 100000 if Flag else 'B') != ''", + values=self._values(), + operation_limit=5, + ) + assert "exceeded limit (5)" in str(exc_info.value) + + @pytest.mark.parametrize("operator", _OPERATORS) + def test_a_value_error_after_the_unresolved_operand_is_still_suppressed( + self, operator: str + ) -> None: + """Negative control: the change is budget-specific. A value error stays + absorbed because a run-time short-circuit may never reach it — the behaviour + ``TestUnknownBoolOpErrorSuppression`` above covers in general.""" + result = evaluate_expression(f"Flag {operator} (10 // 0) > 0", values=self._values()) + assert result.type == ExprType("unresolved[bool]") + + @pytest.mark.parametrize("operator", _OPERATORS) + def test_the_expression_fits_within_a_generous_budget(self, operator: str) -> None: + """Negative control for the budget cases: the same expression under the default + budgets does not raise, so the rejections above are the budget and not the + construction.""" + assert evaluate_expression(self._expr(operator), values=self._values()) is not None + + +class TestIfExpBudgetErrorsPropagate: + """The ``eval_ifexp`` half of the same openjd-rs#407 change, and the half its + description does name. ``{{ 'A' * Param.N if Session.HasPathMappingRules else 'B' }}`` + with a large ``N`` was accepted on 0.9.0 under any budget. + + Extends ``TestUnknownIfElse`` above, which covers the value-error absorption this + leaves in place. + """ + + _BRANCHES = [ + pytest.param("'A' * 100000 if Flag else 'B'", id="if branch"), + pytest.param("'B' if Flag else 'A' * 100000", id="else branch"), + ] + + @staticmethod + def _values() -> SymbolTable: + return SymbolTable({"Flag": ExprValue.unresolved(ExprType("bool"))}) + + @pytest.mark.parametrize("expression", _BRANCHES) + def test_an_operation_budget_exceedance_propagates(self, expression: str) -> None: + with pytest.raises(ExpressionError) as exc_info: + evaluate_expression(expression, values=self._values(), operation_limit=5) + assert "operation count" in str(exc_info.value) + assert "exceeded limit (5)" in str(exc_info.value) + + @pytest.mark.parametrize("expression", _BRANCHES) + def test_a_memory_budget_exceedance_propagates(self, expression: str) -> None: + with pytest.raises(ExpressionError) as exc_info: + evaluate_expression(expression, values=self._values(), memory_limit=1024) + assert "memory usage" in str(exc_info.value) + assert "exceeded limit (1024 bytes)" in str(exc_info.value) + + @pytest.mark.parametrize( + "expression", + [ + pytest.param("10 // 0 if Flag else 1", id="if branch"), + pytest.param("1 if Flag else 10 // 0", id="else branch"), + ], + ) + def test_a_value_error_in_a_branch_is_still_absorbed(self, expression: str) -> None: + """Negative control: run time may select the healthy branch, so a value error + in one branch does not fail the evaluation.""" + result = evaluate_expression(expression, values=self._values()) + assert result.type == ExprType("unresolved[int]") + + @pytest.mark.parametrize("expression", _BRANCHES) + def test_the_expression_fits_within_a_generous_budget(self, expression: str) -> None: + result = evaluate_expression(expression, values=self._values()) + assert result.type == ExprType("unresolved[string]") diff --git a/test/openjd/model_v1/test_create_job.py b/test/openjd/model_v1/test_create_job.py index b98888af..6ee742a6 100644 --- a/test/openjd/model_v1/test_create_job.py +++ b/test/openjd/model_v1/test_create_job.py @@ -4,10 +4,11 @@ import tempfile import pytest from pathlib import Path -from typing import Any +from typing import Any, Optional from openjd.model._v1 import ( CallerLimits, + ModelProfile, create_job, decode_environment_template, decode_job_template, @@ -17,6 +18,7 @@ from openjd.model._v1.types import ( JobParameterType, JobParameterValue, + ModelExtension, ValidationContext, ) from openjd.model._v1.errors import ( @@ -2134,3 +2136,419 @@ def test_job_environment_argument_under_the_cap_is_accepted(self) -> None: args = on_enter.args assert args is not None assert [str(a) for a in args] == ["{{Param.P}}"] + + +class TestFormatStringCapabilityNamesAtJobCreation: + """Companion to ``test_parse.py::TestFormatStringCapabilityNames``. A capability + ``name`` that depends on a job parameter is only fully known at job creation, so + that is where openjd-model 0.10.0 (openjd-rs#409) applies the §3.3.1.1 / + §3.3.2.1 constraints and the case-insensitive uniqueness rule. + + Every rejection here was unreachable on 0.9.0: the template did not decode. Each + rule is exercised on both ``amounts`` and ``attributes``, which upstream checks + through separate call sites with separate messages. + """ + + _KINDS = [pytest.param("amounts", id="amounts"), pytest.param("attributes", id="attributes")] + + @staticmethod + def _entry(kind: str, name: str) -> dict[str, Any]: + if kind == "amounts": + return {"name": name, "min": "1"} + return {"name": name, "anyOf": ["linux"]} + + @classmethod + def _template(cls, kind: str, *names: str) -> dict[str, Any]: + return { + "specificationVersion": "jobtemplate-2023-09", + "name": "T", + "parameterDefinitions": [{"name": "Attr", "type": "STRING"}], + "steps": [ + { + "name": "S", + "hostRequirements": {kind: [cls._entry(kind, n) for n in names]}, + "script": {"actions": {"onRun": {"command": "echo"}}}, + } + ], + } + + @classmethod + def _created_names(cls, kind: str, value: str, *names: str) -> list[str]: + decoded = decode_job_template(template=cls._template(kind, *names)) + job = create_job(job_template=decoded, job_parameter_values={"Attr": value}) + requirements = job.steps[0].host_requirements + assert requirements is not None + entries = requirements.amounts if kind == "amounts" else requirements.attributes + assert entries is not None + return [e.name for e in entries] + + @classmethod + def _created_name(cls, kind: str, value: str) -> str: + return cls._created_names(kind, value, "{{Param.Attr}}")[0] + + @pytest.mark.parametrize("kind", _KINDS) + @pytest.mark.parametrize( + "value", + [ + pytest.param("custom.x", id="customer-defined"), + pytest.param("vendor:custom.x", id="vendor-prefixed"), + ], + ) + def test_the_resolved_name_is_written_onto_the_job(self, kind: str, value: str) -> None: + prefix = "amount" if kind == "amounts" else "attr" + name = value.replace("custom.x", f"{prefix}.custom.x") + assert self._created_name(kind, name) == name + + @pytest.mark.parametrize( + "kind,name", + [ + pytest.param("amounts", "amount.worker.vcpu", id="amounts"), + pytest.param("attributes", "attr.worker.os.family", id="attributes"), + ], + ) + def test_a_resolved_standard_capability_name_is_accepted(self, kind: str, name: str) -> None: + assert self._created_name(kind, name) == name + + @pytest.mark.parametrize("kind", _KINDS) + @pytest.mark.parametrize( + "value,expected_message", + [ + pytest.param( + "not a name", + "name 'not a name' does not match capability name pattern.", + id="pattern", + ), + pytest.param("PREFIX.worker.made_up", "reserved scope 'worker'", id="reserved scope"), + ], + ) + def test_a_resolved_name_violating_its_constraints_is_rejected( + self, kind: str, value: str, expected_message: str + ) -> None: + prefix = "amount" if kind == "amounts" else "attr" + with pytest.raises(ModelValidationError) as excinfo: + self._created_name(kind, value.replace("PREFIX", prefix)) + assert expected_message in str(excinfo.value) + + @pytest.mark.parametrize("kind", _KINDS) + def test_a_resolved_name_over_100_characters_is_rejected(self, kind: str) -> None: + prefix = "amount.custom." if kind == "amounts" else "attr.custom." + name = prefix + "a" * (101 - len(prefix)) + assert len(name) == 101 + with pytest.raises(ModelValidationError) as excinfo: + self._created_name(kind, name) + assert "exceeds 100 characters." in str(excinfo.value) + + @pytest.mark.parametrize("kind", _KINDS) + def test_the_100_character_boundary_is_accepted(self, kind: str) -> None: + """Negative control for the length case above: exactly 100 characters passes, + so the check is a boundary and not an unconditional rejection.""" + prefix = "amount.custom." if kind == "amounts" else "attr.custom." + name = prefix + "a" * (100 - len(prefix)) + assert len(name) == 100 + assert self._created_name(kind, name) == name + + @pytest.mark.parametrize( + "kind,literal,value", + [ + pytest.param("amounts", "amount.custom.x", "AMOUNT.CUSTOM.X", id="amounts"), + pytest.param("attributes", "attr.custom.x", "ATTR.CUSTOM.X", id="attributes"), + ], + ) + def test_a_resolved_name_colliding_with_a_literal_is_rejected( + self, kind: str, literal: str, value: str + ) -> None: + """Uniqueness is case-insensitive and spans both spellings, so a format string + cannot smuggle in a duplicate of a literal sibling.""" + singular = "amount" if kind == "amounts" else "attribute" + with pytest.raises(ModelValidationError) as excinfo: + self._created_names(kind, value, literal, "{{Param.Attr}}") + assert f"duplicate {singular} name '{value}'." in str(excinfo.value) + + @pytest.mark.parametrize( + "kind,literal,value", + [ + pytest.param("amounts", "amount.custom.x", "amount.custom.y", id="amounts"), + pytest.param("attributes", "attr.custom.x", "attr.custom.y", id="attributes"), + ], + ) + def test_two_distinct_resolved_names_are_accepted( + self, kind: str, literal: str, value: str + ) -> None: + """Negative control for the collision case: the uniqueness check compares the + resolved names, so a distinct resolution is fine.""" + assert self._created_names(kind, value, literal, "{{Param.Attr}}") == [literal, value] + + @pytest.mark.parametrize( + "any_of,expected_message", + [ + pytest.param( + ["plan9"], "value 'plan9' is not valid for attr.worker.os.family.", id="rejected" + ), + pytest.param(["linux"], None, id="accepted"), + ], + ) + def test_the_resolved_name_identifies_a_standard_attribute_for_its_value_checks( + self, any_of: list[str], expected_message: Optional[str] + ) -> None: + """Whether a capability is standard — and therefore which values are legal — + is decided by the *resolved* name. At decode the name was unknown, so no value + check could attach; this is the stage that supplies one.""" + template = self._template("attributes", "{{Param.Attr}}") + template["steps"][0]["hostRequirements"]["attributes"][0]["anyOf"] = any_of + decoded = decode_job_template(template=template) + if expected_message is None: + job = create_job( + job_template=decoded, job_parameter_values={"Attr": "attr.worker.os.family"} + ) + assert job.name == "T" + return + with pytest.raises(ModelValidationError) as excinfo: + create_job(job_template=decoded, job_parameter_values={"Attr": "attr.worker.os.family"}) + assert expected_message in str(excinfo.value) + + +class TestCreateJobContextExtensionContract: + """openjd-model 0.10.0 (openjd-rs#407) requires ``create_job``'s context to cover + every extension the template declares. On 0.9.0 passing a context that stripped + ``EXPR`` created the job, and the resolved-value checks silently skipped every + evaluation error as possibly-a-context-artifact. + + ``create_job`` without ``validation_context`` derives the context from the + template, so the default path cannot violate the contract. + """ + + _TEMPLATE: dict[str, Any] = { + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["EXPR"], + "name": "T", + "steps": [{"name": "S", "script": {"actions": {"onRun": {"command": "echo"}}}}], + } + + @classmethod + def _decoded(cls) -> Any: + return decode_job_template(template=cls._TEMPLATE, supported_extensions=["EXPR"]) + + def test_the_default_context_satisfies_the_contract(self) -> None: + assert create_job(job_template=self._decoded(), job_parameter_values={}).name == "T" + + def test_a_context_derived_from_the_template_satisfies_the_contract(self) -> None: + decoded = self._decoded() + job = create_job( + job_template=decoded, + job_parameter_values={}, + validation_context=ValidationContext(decoded.profile), + ) + assert job.name == "T" + + def test_a_context_that_strips_a_declared_extension_is_rejected(self) -> None: + with pytest.raises(ModelValidationError) as excinfo: + create_job( + job_template=self._decoded(), + job_parameter_values={}, + validation_context=ValidationContext(ModelProfile(extensions=[])), + ) + message = str(excinfo.value) + assert "every extension the template declares" in message + assert "missing EXPR" in message + + def test_a_context_enabling_more_than_the_template_declares_is_accepted(self) -> None: + """Cover, not equality: the contract is that the context is a superset.""" + job = create_job( + job_template=self._decoded(), + job_parameter_values={}, + validation_context=ValidationContext( + ModelProfile(extensions=[ModelExtension.EXPR, ModelExtension.TASK_CHUNKING]) + ), + ) + assert job.name == "T" + + +class TestValueDependentEvaluationErrorAtJobCreation: + """With the context contract enforced (openjd-rs#407), every evaluation error at + job creation is a real defect, so the lenient policy that skipped them is gone. + On 0.9.0 the failing case below created a job and the failure surfaced on every + worker that ran the task instead. + """ + + _TEMPLATE: dict[str, Any] = { + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["EXPR"], + "name": "T", + "parameterDefinitions": [{"name": "N", "type": "INT"}], + "steps": [ + { + "name": "S", + "script": { + "actions": {"onRun": {"command": "echo", "args": ["{{ 10 // Param.N }}"]}} + }, + } + ], + } + + @classmethod + def _create(cls, divisor: int) -> Any: + return create_job( + job_template=decode_job_template(template=cls._TEMPLATE, supported_extensions=["EXPR"]), + job_parameter_values={"N": divisor}, + ) + + def test_a_healthy_value_creates_the_job(self) -> None: + """Negative control, and it pins that the check reads the resolved value + without rewriting the field: the argument stays a format string on the job, + because task parameters resolve in the session.""" + job = self._create(2) + args = job.steps[0].script.actions.onRun.args + assert args is not None + assert [str(a) for a in args] == ["{{ 10 // Param.N }}"] + + def test_a_value_dependent_error_fails_job_creation(self) -> None: + with pytest.raises(ModelValidationError) as excinfo: + self._create(0) + message = str(excinfo.value) + assert "Division by zero" in message + assert "steps[0] -> script -> actions -> onRun -> args[0]" in message + + +class TestEvaluationBudgetsInsideUnresolvedConditionals: + """openjd-rs#407 stopped the evaluator absorbing *budget* errors raised inside a + branch of a conditional whose test is unresolved. The budget is spent in this + evaluation whichever branch run time takes, so a caller who lowered + ``max_eval_operations`` had it silently stop applying — this is the idiomatic + construction for a worker-resolved test, so the bypass was reachable. + + The expression-level pins live in + ``test/openjd/expr/test_unresolved_eval.py``; this class covers the path through + ``create_job``, where the budget arrives on a ``CallerLimits``. + """ + + _TEMPLATE: dict[str, Any] = { + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["EXPR"], + "name": "T", + "parameterDefinitions": [{"name": "N", "type": "INT"}], + "steps": [ + { + "name": "S", + "script": { + "actions": { + "onRun": { + "command": "echo", + "args": ["{{ 'A' * Param.N if Session.HasPathMappingRules else 'B' }}"], + } + } + }, + } + ], + } + + @classmethod + def _create(cls, max_eval_operations: int) -> Any: + decoded = decode_job_template(template=cls._TEMPLATE, supported_extensions=["EXPR"]) + return create_job( + job_template=decoded, + job_parameter_values={"N": 100_000}, + validation_context=ValidationContext( + decoded.profile, + caller_limits=CallerLimits(max_eval_operations=max_eval_operations), + ), + ) + + def test_a_lowered_operation_budget_is_enforced_inside_the_conditional(self) -> None: + with pytest.raises(ModelValidationError) as excinfo: + self._create(5) + message = str(excinfo.value) + assert "operation count" in message + assert "exceeded limit (5)" in message + + def test_a_budget_the_expression_fits_within_creates_the_job(self) -> None: + """Negative control: the same template and the same parameter value, so the + rejection above is the budget and not the construction.""" + assert self._create(1_000_000).name == "T" + + +class TestUnresolvedFilterComprehensionAtJobCreation: + """The reviewer's repro from openjd-rs#407. Job creation evaluates under a symbol + state no other stage sees — ``Param.*`` concrete, ``Task.*`` / ``Session.*`` + unresolved — so it is the only stage where this comprehension has a concrete + iterable and an unresolved filter. + + This test does **not** discriminate 0.9.0 from 0.10.0: it creates a job on both, + for different reasons. On 0.9.0 the comprehension raised and the lenient error + policy silently skipped it; on 0.10.0 it raises nothing. It is here because the two + halves of that release have to hold *together* — the strict policy of openjd-rs#407 + without its companion listcomp fix rejects this template, which is what upstream + found in review. The discriminating pins are in + ``test/openjd/expr/test_unresolved_eval.py::TestConcreteIterableUnresolvedFilter``. + """ + + _TEMPLATE: dict[str, Any] = { + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["EXPR"], + "name": "T", + "parameterDefinitions": [{"name": "Files", "type": "STRING"}], + "steps": [ + { + "name": "S", + "parameterSpace": { + "taskParameterDefinitions": [{"name": "Skip", "type": "STRING", "range": ["b"]}] + }, + "script": { + "actions": { + "onRun": { + "command": "echo", + "args": [ + "{{ [f for f in Param.Files.split(',') if f != Task.Param.Skip] }}" + ], + } + } + }, + } + ], + } + + def test_the_template_creates_a_job(self) -> None: + decoded = decode_job_template(template=self._TEMPLATE, supported_extensions=["EXPR"]) + job = create_job(job_template=decoded, job_parameter_values={"Files": "a,b,c"}) + args = job.steps[0].script.actions.onRun.args + assert args is not None + assert [str(a) for a in args] == [ + "{{ [f for f in Param.Files.split(',') if f != Task.Param.Skip] }}" + ] + + +class TestValidationIsIndependentOfTheHostPathFormat: + """openjd-rs#407 also made every stage that evaluates outside host context do so + under ``PathFormat::Posix``. ``create_job`` already built its symbol tables that + way, but its resolved-value re-checks and template validation's pass 8 evaluated + under the *host* format — so a PATH value flowing from a ``let`` binding into an + argument drew ``Path format mismatch`` on Windows, masked until the strict error + policy exposed it as 11 conformance failures. + + On a POSIX host the host format *is* POSIX, so this assertion is a no-op here and + carries its weight only on the Windows CI lane. It is the only change in this bump + whose effect is platform-dependent. + """ + + def test_a_path_from_a_let_binding_reaches_an_argument(self) -> None: + decoded = decode_job_template( + template={ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["EXPR"], + "name": "T", + "parameterDefinitions": [{"name": "P", "type": "PATH"}], + "steps": [ + { + "name": "S", + "let": ["p = RawParam.P"], + "script": { + "actions": {"onRun": {"command": "echo", "args": ["{{ string(p) }}"]}} + }, + } + ], + }, + supported_extensions=["EXPR"], + ) + job = create_job(job_template=decoded, job_parameter_values={"P": "/tmp/x"}) + args = job.steps[0].script.actions.onRun.args + assert args is not None + assert [str(a) for a in args] == ["{{ string(p) }}"] diff --git a/test/openjd/model_v1/test_parse.py b/test/openjd/model_v1/test_parse.py index 9d1b34ab..dd601419 100644 --- a/test/openjd/model_v1/test_parse.py +++ b/test/openjd/model_v1/test_parse.py @@ -943,3 +943,282 @@ def test_the_dict_entry_points_have_nothing_to_measure(self) -> None: supported_extensions=[], caller_limits=CallerLimits(max_template_size=10), ) + + +class TestFormatStringCapabilityNames: + """A host requirement capability ``name`` is ``@fmtstring`` as of openjd-model + 0.10.0 (openjd-rs#409, tracking openjd-specifications#189). On 0.9.0 every case + below that decodes now was rejected with + ``name '{{Param.Attr}}' does not match capability name pattern.`` — the pattern + was applied to the raw template text. + + Template validation checks a name whose value it already knows: a literal, or a + format string that is fully static. A name that depends on a job parameter is + validated as a format string (symbol availability, the 100-character lower + bound) and has its capability-name constraints checked at job creation instead; + see ``test_create_job.py::TestFormatStringCapabilityNamesAtJobCreation``. + """ + + _PARAMS = [{"name": "Attr", "type": "STRING"}] + + @staticmethod + def _template( + host_requirements: dict[str, Any], + *, + parameter_definitions: Optional[list[dict[str, Any]]] = None, + ) -> dict[str, Any]: + template: dict[str, Any] = { + "specificationVersion": "jobtemplate-2023-09", + "name": "T", + "steps": [ + { + "name": "S", + "hostRequirements": host_requirements, + "script": {"actions": {"onRun": {"command": "echo"}}}, + } + ], + } + if parameter_definitions is not None: + template["parameterDefinitions"] = parameter_definitions + return template + + @pytest.mark.parametrize( + "host_requirements,expected", + [ + pytest.param( + {"amounts": [{"name": "{{Param.Attr}}", "min": "1"}]}, + "{{Param.Attr}}", + id="amount name", + ), + pytest.param( + {"attributes": [{"name": "{{Param.Attr}}", "anyOf": ["linux"]}]}, + "{{Param.Attr}}", + id="attribute name", + ), + ], + ) + def test_a_parameter_dependent_name_decodes( + self, host_requirements: dict[str, Any], expected: str + ) -> None: + """The name reaches the decoded template as a ``FormatString`` carrying the raw + text, matching its ``min`` / ``max`` / ``anyOf`` / ``allOf`` siblings. On 0.9.0 + it was a ``str``.""" + decoded = decode_job_template( + template=self._template(host_requirements, parameter_definitions=self._PARAMS) + ) + requirements = decoded.steps[0].host_requirements + assert requirements is not None + entries = ( + requirements.amounts if "amounts" in host_requirements else requirements.attributes + ) + assert entries is not None + assert entries[0].name.raw() == expected + + @pytest.mark.parametrize( + "name,expected_message", + [ + pytest.param( + "bogus.name", + "name 'bogus.name' does not match capability name pattern.", + id="literal, unchanged from 0.9.0", + ), + pytest.param( + "{{ 'bogus.static' }}", + "name 'bogus.static' does not match capability name pattern.", + id="fully static, checked on the resolved text", + ), + pytest.param( + "{{ 'amount.worker.made_up' }}", + "reserved", + id="fully static, reserved scope", + ), + ], + ) + def test_a_name_whose_value_is_known_is_checked_at_validation( + self, name: str, expected_message: str + ) -> None: + with pytest.raises(ModelValidationError) as excinfo: + decode_job_template(template=self._template({"amounts": [{"name": name, "min": "1"}]})) + assert expected_message in str(excinfo.value) + + def test_an_undefined_parameter_in_a_name_is_reported_as_an_expression_error(self) -> None: + """0.9.0 reported this as a capability-name pattern violation, because it never + parsed the name. It is now an expression error naming the symbol.""" + with pytest.raises(ModelValidationError) as excinfo: + decode_job_template( + template=self._template({"amounts": [{"name": "{{Param.Nope}}", "min": "1"}]}) + ) + message = str(excinfo.value) + assert "Undefined variable: 'Param.Nope'" in message + assert "amounts[0] -> name" in message + + +class TestFormatStringCapabilityNameChecksAtValidation: + """The checks openjd-rs#409 added at template validation for a capability name + whose value is fully known. None existed on 0.9.0: the pattern was applied to the + raw text and nothing else about a format-string name was examined. + + Companion to ``TestFormatStringCapabilityNames`` above, which covers the decode + acceptance and the pattern check, and to + ``test_create_job.py::TestFormatStringCapabilityNamesAtJobCreation``, which covers + the same rules on a resolved name. + """ + + @staticmethod + def _step(host_requirements: dict[str, Any], **extras: Any) -> dict[str, Any]: + step: dict[str, Any] = { + "name": "S", + "hostRequirements": host_requirements, + "script": {"actions": {"onRun": {"command": "echo"}}}, + } + step.update(extras) + return step + + @classmethod + def _decode( + cls, + host_requirements: dict[str, Any], + *, + parameter_definitions: Optional[list[dict[str, Any]]] = None, + extensions: Optional[list[str]] = None, + **step_extras: Any, + ) -> Any: + template: dict[str, Any] = { + "specificationVersion": "jobtemplate-2023-09", + "name": "T", + "steps": [cls._step(host_requirements, **step_extras)], + } + if parameter_definitions is not None: + template["parameterDefinitions"] = parameter_definitions + if extensions is not None: + template["extensions"] = extensions + return decode_job_template( + template=template, supported_extensions=extensions if extensions else [] + ) + + @pytest.mark.parametrize( + "name,expected_message", + [ + pytest.param( + "{{ '' }}", + "name '' does not match capability name pattern.", + id="empty", + ), + pytest.param( + "{{ 'amount.custom.' + 'a' * 87 }}", + "exceeds 100 characters.", + id="101 characters", + ), + ], + ) + def test_a_fully_static_name_is_length_and_pattern_checked( + self, name: str, expected_message: str + ) -> None: + with pytest.raises(ModelValidationError) as excinfo: + self._decode({"amounts": [{"name": name, "min": "1"}]}) + assert expected_message in str(excinfo.value) + + def test_a_name_built_only_from_let_bindings_with_literal_values_is_static(self) -> None: + """A ``let`` binding with a literal value makes the name fully known, so it is + checked at validation rather than deferred. This one is valid, so it decodes and + keeps its raw text.""" + decoded = self._decode( + {"amounts": [{"name": "{{ n }}", "min": "1"}]}, + extensions=["EXPR"], + let=["n = 'amount.custom.x'"], + ) + requirements = decoded.steps[0].host_requirements + assert requirements is not None + amounts = requirements.amounts + assert amounts is not None + assert amounts[0].name.raw() == "{{ n }}" + + def test_a_partly_static_name_gets_a_length_lower_bound(self) -> None: + """The literal runs of a name give a guaranteed minimum resolved length, so a + name that cannot possibly fit in 100 characters is rejected before its + parameter is bound. A distinct message from the exact-length one above.""" + with pytest.raises(ModelValidationError) as excinfo: + self._decode( + {"amounts": [{"name": "amount.custom." + "a" * 95 + "{{Param.Attr}}", "min": "1"}]}, + parameter_definitions=[{"name": "Attr", "type": "STRING"}], + ) + assert "resolves to at least 109 characters, exceeding the maximum of 100." in str( + excinfo.value + ) + + def test_a_static_name_duplicating_a_literal_sibling_is_rejected(self) -> None: + with pytest.raises(ModelValidationError) as excinfo: + self._decode( + { + "amounts": [ + {"name": "amount.custom.x", "min": "1"}, + {"name": "{{ 'amount.custom.x' }}", "min": "1"}, + ] + } + ) + assert "duplicate amount name 'amount.custom.x'." in str(excinfo.value) + + def test_a_duplicate_is_reported_once(self) -> None: + """Uniqueness is now checked in a pass that also sees literal names, so a + literal duplicate must not be reported by both the old and the new check.""" + with pytest.raises(ModelValidationError) as excinfo: + self._decode( + { + "amounts": [ + {"name": "amount.custom.x", "min": "1"}, + {"name": "amount.custom.x", "min": "1"}, + ] + } + ) + assert "1 validation error for JobTemplate" in str(excinfo.value) + + @pytest.mark.parametrize( + "entry,expected_message", + [ + pytest.param( + {"name": "{{ 'attr.worker.os.family' }}", "anyOf": ["plan9"]}, + "value 'plan9' is not valid for attr.worker.os.family.", + id="standard-capability value", + ), + pytest.param( + {"name": "{{ 'attr.worker.os.family' }}", "allOf": ["linux", "macos"]}, + "single-valued attribute cannot have more than 1 element.", + id="single-valued allOf", + ), + ], + ) + def test_a_static_standard_attribute_name_drives_its_value_checks( + self, entry: dict[str, Any], expected_message: str + ) -> None: + """Resolving the name is what identifies the capability as standard, so the + value rules attach to the resolved name, not the raw text.""" + with pytest.raises(ModelValidationError) as excinfo: + self._decode({"attributes": [entry]}) + assert expected_message in str(excinfo.value) + + def test_a_name_using_a_symbol_unavailable_at_job_creation_is_rejected(self) -> None: + """A name is resolved at job creation, where task parameters are not yet bound, + so ``Task.Param.*`` is not in scope for it however valid it is elsewhere in the + step.""" + with pytest.raises(ModelValidationError) as excinfo: + self._decode( + {"amounts": [{"name": "{{Task.Param.F}}", "min": "1"}]}, + parameterSpace={ + "taskParameterDefinitions": [{"name": "F", "type": "STRING", "range": ["a"]}] + }, + ) + assert "Undefined variable: 'Task.Param.F'" in str(excinfo.value) + + def test_a_parameter_dependent_duplicate_is_deferred_not_rejected(self) -> None: + """Negative control for the uniqueness check, and the deferral it rests on: two + names that *may* collide once resolved must still decode, because validation + cannot know. ``test_create_job.py`` pins that the collision is caught there.""" + assert self._decode( + { + "amounts": [ + {"name": "amount.custom.x", "min": "1"}, + {"name": "{{Param.Attr}}", "min": "1"}, + ] + }, + parameter_definitions=[{"name": "Attr", "type": "STRING"}], + ) diff --git a/test/openjd/model_v1/test_template_types.py b/test/openjd/model_v1/test_template_types.py index 02245aca..38af17e4 100644 --- a/test/openjd/model_v1/test_template_types.py +++ b/test/openjd/model_v1/test_template_types.py @@ -17,7 +17,7 @@ import pytest -from openjd.expr import FormatString +from openjd.expr import ExpressionError, FormatString from openjd.model._v1 import decode_environment_template, decode_job_template from openjd.model._v1.template import ( Action, @@ -210,7 +210,8 @@ def test_host_requirements(self): amts = hr.amounts assert amts is not None and len(amts) == 1 assert isinstance(amts[0], AmountRequirement) - assert amts[0].name == "amount.worker.vcpu" + assert isinstance(amts[0].name, FormatString) + assert amts[0].name.raw() == "amount.worker.vcpu" assert isinstance(amts[0].min, FormatString) assert amts[0].min.raw() == "4" assert amts[0].max.raw() == "8" @@ -218,7 +219,8 @@ def test_host_requirements(self): attrs = hr.attributes assert attrs is not None and len(attrs) == 2 assert isinstance(attrs[0], AttributeRequirement) - assert attrs[0].name == "attr.worker.os.family" + assert isinstance(attrs[0].name, FormatString) + assert attrs[0].name.raw() == "attr.worker.os.family" assert attrs[0].any_of[0].raw() == "linux" assert attrs[0].anyOf[0].raw() == "linux" # camelCase alias assert attrs[1].all_of[0].raw() == "x86_64" @@ -417,7 +419,7 @@ def test_step_dependency(self): def test_amount_requirement(self): ar = AmountRequirement( - name="amount.worker.vcpu", + name=FormatString("amount.worker.vcpu"), min=FormatString("4"), max=FormatString("8"), ) @@ -428,7 +430,7 @@ def test_amount_requirement(self): def test_attribute_requirement(self): ar = AttributeRequirement( - name="attr.worker.os.family", + name=FormatString("attr.worker.os.family"), any_of=[FormatString("linux")], ) loaded = pickle.loads(pickle.dumps(ar)) @@ -482,3 +484,77 @@ def test_pickle_uses_template_module_path(self): # `TemplateStepDependency` (with `module = ...template`). assert b"openjd.model._v1.template" in data assert b"TemplateStepDependency" in data + + +class TestCapabilityNameIsAFormatString: + """``template::AmountRequirement::name`` and + ``template::AttributeRequirement::name`` became ``FormatString`` in openjd-model + 0.10.0 (openjd-rs#409), and the Python-facing ``name`` follows, matching its + ``min`` / ``max`` / ``anyOf`` / ``allOf`` siblings and every other + FormatString-typed field on the template types. + + On 0.9.0 ``name`` was a ``str`` in both directions. The job-side + ``AmountRequirement.name`` / ``AttributeRequirement.name`` are still ``str``: they + hold the resolved name. + """ + + @pytest.mark.parametrize("cls", [AmountRequirement, AttributeRequirement]) + @pytest.mark.parametrize( + "name", + [ + pytest.param("amount.worker.vcpu", id="literal"), + pytest.param("{{Param.Attr}}", id="parameter reference"), + pytest.param("{{ 'amount.custom.x' }}", id="static expression"), + pytest.param("prefix.{{Param.Attr}}.suffix", id="interpolated"), + ], + ) + def test_the_name_is_a_format_string_carrying_the_raw_text(self, cls: type, name: str) -> None: + got = cls(name=FormatString(name)).name + assert isinstance(got, FormatString) + assert got.raw() == name + + @pytest.mark.parametrize("cls", [AmountRequirement, AttributeRequirement]) + def test_a_plain_str_name_is_refused(self, cls: type) -> None: + """The reason this is worth pinning: ``name`` used to accept a ``str`` on 0.9.0, + so a caller has to wrap it. Refusing it is what makes the field consistent with + its siblings, which have always refused one.""" + with pytest.raises(TypeError) as exc_info: + cls(name="amount.worker.vcpu") + assert "not an instance of 'FormatString'" in str(exc_info.value) + + @pytest.mark.parametrize("cls", [AmountRequirement, AttributeRequirement]) + def test_a_malformed_name_cannot_be_constructed_at_all(self, cls: type) -> None: + """Validation now sits in ``FormatString`` rather than in the requirement, so a + malformed name is rejected one step earlier than it would have been by a + ``str``-typed field that parsed on the way in.""" + with pytest.raises(ExpressionError) as exc_info: + cls(name=FormatString("{{")) + assert "Braces mismatch" in str(exc_info.value) + + @pytest.mark.parametrize( + "cls,expected", + [ + pytest.param( + AmountRequirement, + 'AmountRequirement(name="{{Param.Attr}}")', + id="TemplateAmountRequirement", + ), + pytest.param( + AttributeRequirement, + 'AttributeRequirement(name="{{Param.Attr}}")', + id="TemplateAttributeRequirement", + ), + ], + ) + def test_repr_shows_the_raw_text(self, cls: type, expected: str) -> None: + """``__repr__`` goes through ``.raw()``, matching ``Action``'s treatment of its + ``command``. A mutation that formatted the ``FormatString`` itself would print + its Debug shape instead.""" + assert repr(cls(name=FormatString("{{Param.Attr}}"))) == expected + + @pytest.mark.parametrize("cls", [AmountRequirement, AttributeRequirement]) + def test_pickle_round_trips_a_format_string_name(self, cls: type) -> None: + """``__reduce__`` passes ``name`` back through the constructor, so it has to hand + back a ``FormatString`` rather than the raw text.""" + loaded = pickle.loads(pickle.dumps(cls(name=FormatString("{{Param.Attr}}")))) + assert loaded.name.raw() == "{{Param.Attr}}"