From 5ba4be10fcb2e7ba815deda58331ff0cb8288ec1 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 28 Sep 2026 13:37:58 -0700 Subject: [PATCH 1/2] chore(deps): Bump openjd-* Rust crates to the 0.10.0 release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit openjd-rs released on 2026-09-28: openjd-expr 0.10.0, openjd-model 0.10.0, openjd-sessions 0.7.1 (OpenJobDescription/openjd-rs#408). This package pinned 0.9.0 / 0.9.0 / 0.7.0. openjd-sessions 0.7.1 is a transitive re-pin with no source change. Two breaking changes, both features: - #409 makes template::AmountRequirement::name and template::AttributeRequirement::name a FormatString instead of a String, so a capability name may contain expressions (openjd-specifications#189). The §3.3.1.1 / §3.3.2.1 constraints now apply to the resolved name. - #407 requires create_job's context to cover the template's declared extensions. With mismatch impossible, the job-creation resolved-value checks report every evaluation error instead of silently skipping it. Only #409 broke compilation, in four places: the two template-type constructors and the two name getters. The Python-facing `name` stays a `str` holding the raw template text. v0 models these as AmountCapabilityName / AttributeCapabilityName, both subclasses of v0's FormatString, which subclasses str -- so exposing an openjd.expr .FormatString here would make v0 and v1 diverge where they agree, and .raw() is what #409 prescribes for reading these fields. One behaviour change falls out of the adaptation rather than upstream: the constructor parses, so a malformed format string now raises ExpressionError. create_job needed no code change. The binding derives its context from job_template.default_validation_context() when the caller passes none, which covers the template's extensions by construction; a caller-supplied mismatched context is what #407 now rejects, and the binding surfaces it. #407 is a squash of five commits whose message documents three behaviour changes the release changelog and the PR body do not name: - eval_boolop no longer suppresses budget errors in operands after an unresolved one, the same bypass eval_ifexp had. - A list comprehension over a *concrete* iterable whose filter evaluates unresolved concludes unresolved[list[T]] instead of hard-erroring. This was a defect that predated the release, masked by the lenient policy #407 deleted, and reachable only at job creation -- the one stage where the iterable is concrete and the filter is not. - Template validation and create_job's re-checks evaluate under PathFormat::Posix, so validation outcomes no longer depend on the host OS. Windows-only in effect; unverified locally. Verified: 6243 passed / 24 skipped / 3 xfailed, coverage 94.22%. The three xfails are the pre-existing openjd.expr known gaps; none flipped. ruff, black, mypy, cargo fmt and clippy clean. Every behaviour change above was measured through this package's public API against both 0.9.0 and 0.10.0, and the openjd.expr expectations are taken from the upstream assertions in test_unresolved_eval.rs and test_memory.rs. Four mutants, each rebuilt and each caught: reverting the three pins kills 53 of the new cases, the two __repr__ sites kill 2, the two name getters kill 11, and swallowing the name parse error kills 2. Each was restored byte-for-byte with the bytecode cache cleared between runs. THIRD-PARTY-LICENSES.txt moves only the three version lines; the release pulled in no new transitive crates. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- Cargo.lock | 12 +- THIRD-PARTY-LICENSES.txt | 6 +- rust-bindings/Cargo.toml | 6 +- rust-bindings/src/model/template_types.rs | 47 ++- specs/python-expr-interface.md | 21 + specs/python-model-interface.md | 71 +++- test/openjd/expr/test_unresolved_eval.py | 185 +++++++++ test/openjd/model_v1/test_create_job.py | 420 +++++++++++++++++++- test/openjd/model_v1/test_parse.py | 279 +++++++++++++ test/openjd/model_v1/test_template_types.py | 67 +++- 10 files changed, 1076 insertions(+), 38 deletions(-) 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..4aa66cb9 100644 --- a/rust-bindings/src/model/template_types.rs +++ b/rust-bindings/src/model/template_types.rs @@ -30,6 +30,8 @@ use openjd_model::template::{ }; use openjd_model::types::{EndOfLine, FileType}; +use openjd_expr::format_string::FormatString; + use crate::expr::PyFormatString; // ── Action ── @@ -963,6 +965,25 @@ impl PyEnvironment { // ── HostRequirements / AmountRequirement / AttributeRequirement ── +/// Parse a capability `name` into the `FormatString` the template type +/// now holds (openjd-rs#409). +/// +/// The Python-facing `name` stays a `str` holding the raw template text, +/// rather than becoming an `openjd.expr.FormatString` like its `min` / `max` +/// / `anyOf` / `allOf` siblings. v0 models these as `AmountCapabilityName` / +/// `AttributeCapabilityName`, both subclasses of v0's `FormatString`, which +/// subclasses `str` — so exposing a `FormatString` pyclass here would make v0 +/// and v1 diverge where they agree, and `.raw()` is what openjd-rs#409 +/// prescribes for reading these fields. Nothing is lost: the crate's +/// `Deserialize` parses through `FormatString::new` too. +/// +/// The capability-name constraints are not applied here. Upstream checks a +/// name whose value it already knows at template validation, and a resolved +/// one at job creation. +fn parse_capability_name(name: &str) -> PyResult { + FormatString::new(name).map_err(crate::expr::errors::expr_err_to_py) +} + #[cfg_attr(feature = "stub-gen", gen_stub_pyclass(module = "openjd._openjd_rs"))] #[pyclass( module = "openjd.model._v1.template", @@ -979,19 +1000,19 @@ pub(crate) struct PyAmountRequirement { impl PyAmountRequirement { #[new] #[pyo3(signature = (*, name, min=None, max=None))] - fn new(name: String, min: Option, max: Option) -> Self { - PyAmountRequirement { + fn new(name: &str, min: Option, max: Option) -> PyResult { + Ok(PyAmountRequirement { inner: AmountRequirement { - name, + name: parse_capability_name(name)?, min: min.map(|fs| fs.inner), max: max.map(|fs| fs.inner), }, - } + }) } #[getter] fn name(&self) -> &str { - &self.inner.name + self.inner.name.raw() } #[getter] @@ -1011,7 +1032,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 +1075,22 @@ impl PyAttributeRequirement { #[new] #[pyo3(signature = (*, name, any_of=None, all_of=None))] fn new( - name: String, + name: &str, any_of: Option>, all_of: Option>, - ) -> Self { - PyAttributeRequirement { + ) -> PyResult { + Ok(PyAttributeRequirement { inner: AttributeRequirement { - name, + name: parse_capability_name(name)?, 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()), }, - } + }) } #[getter] fn name(&self) -> &str { - &self.inner.name + self.inner.name.raw() } #[getter] @@ -1103,7 +1124,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..726395f0 100644 --- a/specs/python-model-interface.md +++ b/specs/python-model-interface.md @@ -796,7 +796,8 @@ 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`` +``allOf`` and a raw, possibly-expression-bearing ``name``, while these +job-time variants carry the post-``create_job`` resolved ``str`` name and the resolved ``f64`` (amounts) and ``str`` (attributes) values. ```python @@ -955,16 +956,40 @@ hr.amounts # Optional[list[AmountRequirement]] hr.attributes # Optional[list[AttributeRequirement]] amt = hr.amounts[0] -amt.name # str +amt.name # str — raw template text, may hold expressions amt.min # Optional[FormatString] amt.max # Optional[FormatString] attr = hr.attributes[0] -attr.name # str +attr.name # str — raw template text, 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. The +job-time `AmountRequirement.name` / `AttributeRequirement.name` are the +resolved `str`. + +The Python-facing `name` is a `str` holding the raw template text, not an +`openjd.expr.FormatString`, so it mirrors v0 where `AmountCapabilityName` and +`AttributeCapabilityName` subclass v0's `FormatString`, itself a `str` +subclass. Constructing one parses the name, so a malformed format string +raises `ExpressionError`. + ### `StepDependency` ```python @@ -1371,26 +1396,50 @@ from openjd.model._v1.types import ModelExtension, ValidationContext 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 = template.profile # ModelProfile(revision=v2023_09, extensions=[EXPR]) +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/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..a5d622a6 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 its raw text. ``name`` stays a + ``str`` on the Python side even though the Rust field is a ``FormatString``, + matching v0 where ``AmountCapabilityName`` subclasses 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 == 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 == "{{ 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..06fe98e2 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, @@ -482,3 +482,68 @@ 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 TestCapabilityNameIsAFormatStringUnderneath: + """``template::AmountRequirement::name`` and + ``template::AttributeRequirement::name`` became ``FormatString`` in openjd-model + 0.10.0 (openjd-rs#409). The Python-facing ``name`` stays a ``str`` holding the raw + template text, mirroring v0; see ``parse_capability_name`` in + ``rust-bindings/src/model/template_types.rs`` for why. + + One behaviour change falls out of that adaptation rather than out of upstream: the + constructor parses the name, so a malformed format string now raises where 0.9.0 + stored the text unexamined. + """ + + @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_round_trips_as_its_raw_text(self, cls: type, name: str) -> None: + assert cls(name=name).name == name + assert isinstance(cls(name=name).name, str) + + @pytest.mark.parametrize("cls", [AmountRequirement, AttributeRequirement]) + def test_a_malformed_format_string_is_rejected(self, cls: type) -> None: + """``ExpressionError`` specifically, not merely a ``ValueError`` — the binding + routes the parse failure through ``expr_err_to_py`` so it matches every other + format-string parse error in the package.""" + with pytest.raises(ExpressionError) as exc_info: + cls(name="{{") + 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_still_shows_the_raw_text(self, cls: type, expected: str) -> None: + """The repr went through ``{:?}`` on a ``String`` before and goes through + ``{:?}`` on ``.raw()`` now, so it must be unchanged. A mutation that formatted + the ``FormatString`` itself would print its Debug shape instead.""" + assert repr(cls(name="{{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, which now + parses. Pins that a format-string name survives the round trip rather than + raising on reconstruction.""" + loaded = pickle.loads(pickle.dumps(cls(name="{{Param.Attr}}"))) + assert loaded.name == "{{Param.Attr}}" From 89a866203c2ee2013c4e960104a702f005629a5d Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Mon, 28 Sep 2026 14:57:34 -0700 Subject: [PATCH 2/2] fix: Expose the capability name as a FormatString, not a str Review feedback on #373: consistency with the other v1 fields beats consistency with v0 here. openjd-rs#409 made template::AmountRequirement::name and template::AttributeRequirement::name a FormatString. The first pass kept the Python-facing `name` a `str` and parsed on the way in, on the grounds that v0 models these as AmountCapabilityName / AttributeCapabilityName, both subclasses of v0's FormatString, which subclasses str. That argument looks at the wrong neighbour: every other FormatString-typed field on the v1 template types -- `min`, `max`, `anyOf`, `allOf`, `Action.command` -- is an openjd.expr.FormatString and refuses a bare str. `name` now matches them. This deletes more than it adds. parse_capability_name and its doc comment go away, because the FormatString arrives already parsed, and both constructors go back to being infallible. Validation moves to where it belongs: a malformed name is rejected by FormatString's own constructor rather than by a str-typed field that parses behind the caller's back. __repr__ keeps .raw(), matching Action's treatment of its command. This is a breaking change to the v1 Python API, and two pre-existing tests prove it: TestPickle::test_amount_requirement and test_attribute_requirement both passed `name=` a str, and TestStepTemplate::test_host_requirements compared `.name` to one. All three are updated, so this is a behaviour change and not a refactor. Also from review: the spec edit that changed the documented ModelProfile repr to `revision=v2023_09` was wrong -- PyModelProfile::__repr__ still emits the uppercase-V form (rust-bindings/src/model/profile.rs:397), measured as `ModelProfile(revision=V2023_09, extensions=[EXPR])`. Reverted. The two `SpecificationRevision.v2023_09` corrections in the same block stand: that repr is lowercase (profile.rs:63) and `V2023_09` is not an attribute at all. The divergence between the two reprs is pre-existing and left alone here. Verified: 6245 passed / 24 skipped / 3 xfailed, coverage 94.32%. ruff, black, mypy, cargo fmt and clippy clean. Four mutants, each rebuilt and each caught: reverting the pins to 0.9.0 kills 68 cases, formatting the FormatString in both __repr__ sites kills 2, both getters returning a constant kills 12, and both constructors discarding the supplied name kills 10. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- rust-bindings/src/model/template_types.rs | 59 ++++++++---------- specs/python-model-interface.md | 28 ++++----- src/openjd/_openjd_rs.pyi | 22 +++++-- test/openjd/model_v1/test_parse.py | 10 +-- test/openjd/model_v1/test_template_types.py | 69 ++++++++++++--------- 5 files changed, 101 insertions(+), 87 deletions(-) diff --git a/rust-bindings/src/model/template_types.rs b/rust-bindings/src/model/template_types.rs index 4aa66cb9..4f3f551d 100644 --- a/rust-bindings/src/model/template_types.rs +++ b/rust-bindings/src/model/template_types.rs @@ -30,8 +30,6 @@ use openjd_model::template::{ }; use openjd_model::types::{EndOfLine, FileType}; -use openjd_expr::format_string::FormatString; - use crate::expr::PyFormatString; // ── Action ── @@ -965,25 +963,6 @@ impl PyEnvironment { // ── HostRequirements / AmountRequirement / AttributeRequirement ── -/// Parse a capability `name` into the `FormatString` the template type -/// now holds (openjd-rs#409). -/// -/// The Python-facing `name` stays a `str` holding the raw template text, -/// rather than becoming an `openjd.expr.FormatString` like its `min` / `max` -/// / `anyOf` / `allOf` siblings. v0 models these as `AmountCapabilityName` / -/// `AttributeCapabilityName`, both subclasses of v0's `FormatString`, which -/// subclasses `str` — so exposing a `FormatString` pyclass here would make v0 -/// and v1 diverge where they agree, and `.raw()` is what openjd-rs#409 -/// prescribes for reading these fields. Nothing is lost: the crate's -/// `Deserialize` parses through `FormatString::new` too. -/// -/// The capability-name constraints are not applied here. Upstream checks a -/// name whose value it already knows at template validation, and a resolved -/// one at job creation. -fn parse_capability_name(name: &str) -> PyResult { - FormatString::new(name).map_err(crate::expr::errors::expr_err_to_py) -} - #[cfg_attr(feature = "stub-gen", gen_stub_pyclass(module = "openjd._openjd_rs"))] #[pyclass( module = "openjd.model._v1.template", @@ -1000,19 +979,25 @@ pub(crate) struct PyAmountRequirement { impl PyAmountRequirement { #[new] #[pyo3(signature = (*, name, min=None, max=None))] - fn new(name: &str, min: Option, max: Option) -> PyResult { - Ok(PyAmountRequirement { + fn new(name: PyFormatString, min: Option, max: Option) -> Self { + PyAmountRequirement { inner: AmountRequirement { - name: parse_capability_name(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.raw() + fn name(&self) -> PyFormatString { + PyFormatString { + inner: self.inner.name.clone(), + } } #[getter] @@ -1075,22 +1060,28 @@ impl PyAttributeRequirement { #[new] #[pyo3(signature = (*, name, any_of=None, all_of=None))] fn new( - name: &str, + name: PyFormatString, any_of: Option>, all_of: Option>, - ) -> PyResult { - Ok(PyAttributeRequirement { + ) -> Self { + PyAttributeRequirement { inner: AttributeRequirement { - name: parse_capability_name(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.raw() + fn name(&self) -> PyFormatString { + PyFormatString { + inner: self.inner.name.clone(), + } } #[getter] diff --git a/specs/python-model-interface.md b/specs/python-model-interface.md index 726395f0..4f06d738 100644 --- a/specs/python-model-interface.md +++ b/specs/python-model-interface.md @@ -795,10 +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`` and a raw, possibly-expression-bearing ``name``, while these -job-time variants carry the post-``create_job`` resolved ``str`` name and the -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 @@ -956,12 +956,12 @@ hr.amounts # Optional[list[AmountRequirement]] hr.attributes # Optional[list[AttributeRequirement]] amt = hr.amounts[0] -amt.name # str — raw template text, may hold expressions +amt.name # FormatString — raw, may hold expressions amt.min # Optional[FormatString] amt.max # Optional[FormatString] attr = hr.attributes[0] -attr.name # str — raw template text, may hold expressions +attr.name # FormatString — raw, may hold expressions attr.any_of # Optional[list[FormatString]] (alias: anyOf) attr.all_of # Optional[list[FormatString]] (alias: allOf) ``` @@ -980,15 +980,13 @@ when the name's value becomes known: 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. The -job-time `AmountRequirement.name` / `AttributeRequirement.name` are the -resolved `str`. +variable even where the same symbol is valid elsewhere in the step. -The Python-facing `name` is a `str` holding the raw template text, not an -`openjd.expr.FormatString`, so it mirrors v0 where `AmountCapabilityName` and -`AttributeCapabilityName` subclass v0's `FormatString`, itself a `str` -subclass. Constructing one parses the name, so a malformed format string -raises `ExpressionError`. +`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` @@ -1396,7 +1394,7 @@ from openjd.model._v1.types import ModelExtension, ValidationContext 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 = template.profile # ModelProfile(revision=V2023_09, extensions=[EXPR]) profile.revision # SpecificationRevision.v2023_09 profile.extensions # [ModelExtension.EXPR] profile.has_extension(ModelExtension.EXPR) # True 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/model_v1/test_parse.py b/test/openjd/model_v1/test_parse.py index a5d622a6..dd601419 100644 --- a/test/openjd/model_v1/test_parse.py +++ b/test/openjd/model_v1/test_parse.py @@ -1000,9 +1000,9 @@ def _template( def test_a_parameter_dependent_name_decodes( self, host_requirements: dict[str, Any], expected: str ) -> None: - """The name reaches the decoded template as its raw text. ``name`` stays a - ``str`` on the Python side even though the Rust field is a ``FormatString``, - matching v0 where ``AmountCapabilityName`` subclasses a ``str``.""" + """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) ) @@ -1012,7 +1012,7 @@ def test_a_parameter_dependent_name_decodes( requirements.amounts if "amounts" in host_requirements else requirements.attributes ) assert entries is not None - assert entries[0].name == expected + assert entries[0].name.raw() == expected @pytest.mark.parametrize( "name,expected_message", @@ -1131,7 +1131,7 @@ def test_a_name_built_only_from_let_bindings_with_literal_values_is_static(self) assert requirements is not None amounts = requirements.amounts assert amounts is not None - assert amounts[0].name == "{{ n }}" + 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 diff --git a/test/openjd/model_v1/test_template_types.py b/test/openjd/model_v1/test_template_types.py index 06fe98e2..38af17e4 100644 --- a/test/openjd/model_v1/test_template_types.py +++ b/test/openjd/model_v1/test_template_types.py @@ -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)) @@ -484,16 +486,16 @@ def test_pickle_uses_template_module_path(self): assert b"TemplateStepDependency" in data -class TestCapabilityNameIsAFormatStringUnderneath: +class TestCapabilityNameIsAFormatString: """``template::AmountRequirement::name`` and ``template::AttributeRequirement::name`` became ``FormatString`` in openjd-model - 0.10.0 (openjd-rs#409). The Python-facing ``name`` stays a ``str`` holding the raw - template text, mirroring v0; see ``parse_capability_name`` in - ``rust-bindings/src/model/template_types.rs`` for why. + 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. - One behaviour change falls out of that adaptation rather than out of upstream: the - constructor parses the name, so a malformed format string now raises where 0.9.0 - stored the text unexamined. + 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]) @@ -506,17 +508,27 @@ class TestCapabilityNameIsAFormatStringUnderneath: pytest.param("prefix.{{Param.Attr}}.suffix", id="interpolated"), ], ) - def test_the_name_round_trips_as_its_raw_text(self, cls: type, name: str) -> None: - assert cls(name=name).name == name - assert isinstance(cls(name=name).name, str) + 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_malformed_format_string_is_rejected(self, cls: type) -> None: - """``ExpressionError`` specifically, not merely a ``ValueError`` — the binding - routes the parse failure through ``expr_err_to_py`` so it matches every other - format-string parse error in the package.""" + 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="{{") + cls(name=FormatString("{{")) assert "Braces mismatch" in str(exc_info.value) @pytest.mark.parametrize( @@ -534,16 +546,15 @@ def test_a_malformed_format_string_is_rejected(self, cls: type) -> None: ), ], ) - def test_repr_still_shows_the_raw_text(self, cls: type, expected: str) -> None: - """The repr went through ``{:?}`` on a ``String`` before and goes through - ``{:?}`` on ``.raw()`` now, so it must be unchanged. A mutation that formatted - the ``FormatString`` itself would print its Debug shape instead.""" - assert repr(cls(name="{{Param.Attr}}")) == expected + 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, which now - parses. Pins that a format-string name survives the round trip rather than - raising on reconstruction.""" - loaded = pickle.loads(pickle.dumps(cls(name="{{Param.Attr}}"))) - assert loaded.name == "{{Param.Attr}}" + """``__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}}"