diff --git a/THIRD-PARTY-LICENSES.txt b/THIRD-PARTY-LICENSES.txt index 18d4d438..aa8694fd 100644 --- a/THIRD-PARTY-LICENSES.txt +++ b/THIRD-PARTY-LICENSES.txt @@ -119,7 +119,7 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. ------ -** typing_extensions; version 4.15.0 -- https://pypi.org/project/typing_extensions/ +** typing_extensions; version 4.16.0 -- https://pypi.org/project/typing_extensions/ A. HISTORY OF THE SOFTWARE ========================== diff --git a/reports/model-bindings-quality-evaluation-report.md b/reports/model-bindings-quality-evaluation-report.md index 0720d827..35df400e 100644 --- a/reports/model-bindings-quality-evaluation-report.md +++ b/reports/model-bindings-quality-evaluation-report.md @@ -450,16 +450,28 @@ established v0 idiom or a spec-promised behavior). Numbers are stable so future fix commits can resolve them with the `~~ ... ~~ **Resolved.**` strikethrough convention. -1. **Empty-steps validation should raise `DecodeValidationError`, not +1. ~~**Empty-steps validation should raise `DecodeValidationError`, not `ModelValidationError`** — for v0 parity. Either remap the empty-steps case in `rust-bindings/src/model/errors.rs::model_err_to_py` (or in the upstream Rust validator) so it surfaces under `PyDecodeValidationError`, or update the spec to call out the exception-class change explicitly. Pinned in - `test/openjd/model_v1/test_known_gaps.py::test_empty_steps_raises_decode_validation_error_like_v0`. - -2. **`JobTemplate.specification_version` should return a value + `test/openjd/model_v1/test_known_gaps.py::test_empty_steps_raises_decode_validation_error_like_v0`.~~ + **Resolved** — documented in `specs/python-model-interface.md` + under "Exceptions" as a deliberate v0 divergence: + `DecodeValidationError` is reserved for schema-level (parse-stage) + failures the binding can detect before constructing the typed + model, and `ModelValidationError` is raised for everything caught + by the model validator (the at-least-one-step rule, parameter- + definition invariants, step-name uniqueness, etc.). Both classes + inherit `ValueError`, so callers catching `ValueError` are + unaffected; callers that distinguish should catch the tuple. + Regression test (with full message-body assertion per AGENTS.md + test quality standard) lives at + `test/openjd/model_v1/test_parse.py::TestDecodeJobTemplate::test_empty_steps_raises_model_validation_error`. + +2. ~~**`JobTemplate.specification_version` should return a value comparable to `v1.TemplateSpecificationVersion`** — today it returns the Rust pyo3 enum (`openjd._openjd_rs.TemplateSpecificationVersion`, variants @@ -472,9 +484,26 @@ so future fix commits can resolve them with the shim); (b) drop the Python str-Enum shim and re-export the Rust pyclass under `v1.TemplateSpecificationVersion` (requires renaming variants for v0 parity — `JOBTEMPLATE_v2023_09` style). Pinned in - `test/openjd/model_v1/test_known_gaps.py::test_template_specification_version_comparable_to_python_str_enum`. - -3. **`merge_job_parameter_definitions` should emit `default` as the + `test/openjd/model_v1/test_known_gaps.py::test_template_specification_version_comparable_to_python_str_enum`.~~ + **Resolved** — chose path (b). The Rust pyclass + `PyTemplateSpecificationVersion` in + `rust-bindings/src/model/types.rs` was reshaped to expose the v0 + `str`-Enum surface directly: `#[pyo3(name = "JOBTEMPLATE_v2023_09")]` + / `ENVIRONMENT_v2023_09` variant names, a `.value` getter that + returns the spec-form string, a `__new__` that accepts either form, + `__eq__` / `__hash__` that compare equal (and hash equal) to the + spec-form string, and `module = "openjd._openjd_rs"` so its + canonical Python identity lives in the Rust module. The Python + wrapper `_v1/__init__.py` now imports `TemplateSpecificationVersion` + directly from `openjd._openjd_rs` (no shim). There is exactly one + class, identity-preserving across re-exports. + The same treatment was applied to `SpecificationRevision`. + The stale "str-Enum shim" paragraph in the spec's "Pickle Support" + section was also corrected to match. Regression test moved from + `test_known_gaps.py` to + `test/openjd/model_v1/test_version_enums.py::TestTemplateSpecificationVersion::test_template_specification_version_returned_from_decode`. + +3. ~~**`merge_job_parameter_definitions` should emit `default` as the parameter's native Python type, not as a string** — `int` for `INT`, `float` for `FLOAT`, `bool` for `BOOL`, `list[T]` for the `LIST[*]` variants, and `str` for `STRING` / `PATH` / @@ -484,9 +513,30 @@ so future fix commits can resolve them with the `param_type` dispatch that converts the underlying typed value into the right Python type. Pinned in `test/openjd/model_v1/test_known_gaps.py::test_merge_default_int_returned_as_int` - and `test_merge_default_float_returned_as_float`. - -4. **`merge_job_parameter_definitions` should include the `description` + and `test_merge_default_float_returned_as_float`.~~ + **Resolved** — added a `default_to_native` helper in + `rust-bindings/src/model/create_job_fns.rs` that dispatches on + `JobParameterType` and parses the upstream-stringified default + back into the native Python type. The upstream + `MergedParameterDefinition::default` is `Option` (every + variant is round-tripped through `default_value()`'s stringifier), + so the binding now parses it back: `i64` for `INT`, `f64` for + `FLOAT`, `bool` for `BOOL`, `serde_json::from_str::>` for + the `LIST[*]` variants, pass-through for `STRING` / `PATH` / + `RANGE_EXPR`. Parsing failures fall back to the raw string as a + defensive guard against future upstream serialization changes. + Existing parametrized tests in + `test/openjd/model_v1/test_merge_job_parameters.py::TestMergeTemplates_v2023_09` + updated to reflect native-type defaults (`"default": 8` instead of + `"default": "8"`, etc.). The two int/float gap tests were moved + out of `test_known_gaps.py` and expanded into a full per-type + coverage class `TestMergeDefaultNativeTypes` covering all 12 type + variants (INT, FLOAT, STRING, PATH, BOOL, LIST_INT, LIST_FLOAT, + LIST_STRING, LIST_BOOL, LIST_LIST_INT). Each assertion pins type + identity (`isinstance(..., int)`) as well as equality, since + `5 == 5.0` would otherwise let a regression slip through. + +4. ~~**`merge_job_parameter_definitions` should include the `description` field on each merged dict** — v0's typed defs carried it, and the upstream `openjd_model::merge_job_parameter_definitions` produces it on each input definition. The fix at the binding layer is to @@ -495,7 +545,26 @@ so future fix commits can resolve them with the template or the relevant environment template, and add it to the merged dict when present. Update the spec's "Return shape" key list to include `description`. Pinned in - `test/openjd/model_v1/test_known_gaps.py::test_merge_includes_description`. + `test/openjd/model_v1/test_known_gaps.py::test_merge_includes_description`.~~ + **Resolved** — chose path (a). The binding's + `py_merge_job_parameter_definitions` in + `rust-bindings/src/model/create_job_fns.rs` now builds a + `HashMap<&str, &str>` from `name → description` by walking the + same template sources the upstream merge walked (environment + templates in order, then the job template), with later + descriptions overwriting earlier ones — matching how the upstream + merge tracks `default`. The merged dict carries the `description` + key when at least one contributing template provided one, and + omits the key otherwise (consistent with how `default` / + `objectType` / `dataFlow` are conditionally emitted). The spec's + "Return shape" key list now lists `description` with its + semantics. Test moved out of `test_known_gaps.py` and expanded + into a full `TestMergeDescriptionPropagation` class covering five + cases: description from job template, description from + environment template, job-template-wins ordering, later-env-wins + ordering, and key-absent-when-no-description. `test_known_gaps.py` + is now empty of merge gaps (only the typing-imports leak + regression test remains). 5. **Internal typing imports in `openjd.model._v1.__init__.py` should be prefixed with `_`** — `from typing import Any, Optional, diff --git a/rust-bindings/src/model/create_job_fns.rs b/rust-bindings/src/model/create_job_fns.rs index 1903649c..64a63e32 100644 --- a/rust-bindings/src/model/create_job_fns.rs +++ b/rust-bindings/src/model/create_job_fns.rs @@ -8,6 +8,7 @@ use pyo3_stub_gen::derive::*; use openjd_model::template::EnvironmentTemplate; use openjd_model::types::JobParameterInputValues; +use openjd_model::JobParameterType; use openjd_model::PathParameterOptions; use crate::expr::expr_value::py_to_expr_value; @@ -188,13 +189,41 @@ pub(crate) fn py_merge_job_parameter_definitions( let merged = openjd_model::merge_job_parameter_definitions(&job_template.inner, &env_templates) .map_err(model_err_to_py)?; + // Collect descriptions by parameter name. The upstream + // `MergedParameterDefinition` struct does not carry a description + // field (only `name`, `param_type`, `default`, `object_type`, + // `data_flow`, `source`, and the merged constraint fields), so we + // recover it here by walking the same template sources the merge + // walked. To match how `default` is tracked (later template wins), + // we walk environment templates first (in order) then the job + // template last, overwriting on each set description we encounter. + let mut descriptions: std::collections::HashMap<&str, &str> = std::collections::HashMap::new(); + for et in &env_templates { + if let Some(params) = &et.parameter_definitions { + for p in params { + if let Some(desc) = p.description() { + descriptions.insert(p.name(), desc); + } + } + } + } + for p in job_template.inner.parameter_definitions_list() { + if let Some(desc) = p.description() { + descriptions.insert(p.name(), desc); + } + } + let mut out = Vec::new(); for m in &merged { let d = PyDict::new(py); d.set_item("name", &m.name)?; d.set_item("type", m.param_type.as_spec_str())?; if let Some(ref default) = m.default { - d.set_item("default", default)?; + let py_default = default_to_native(py, m.param_type, default)?; + d.set_item("default", py_default)?; + } + if let Some(desc) = descriptions.get(m.name.as_str()) { + d.set_item("description", *desc)?; } if let Some(ref ot) = m.object_type { d.set_item("objectType", ot.to_string())?; @@ -208,6 +237,81 @@ pub(crate) fn py_merge_job_parameter_definitions( Ok(out) } +/// Convert a stringified default value back into the native Python +/// type for its parameter type. +/// +/// The upstream Rust crate's `MergedParameterDefinition::default` is +/// `Option` — every variant is stringified through +/// `JobParameterDefinition::default_value()` regardless of the +/// underlying typed payload. The binding contract (per +/// `specs/python-model-interface.md`) is that callers receive the +/// default in its native Python type (`int` for `INT`, `float` for +/// `FLOAT`, `list[T]` for `LIST[T]`, etc.). This helper parses the +/// stringified form back into the right Python type. +/// +/// `STRING` / `PATH` / `RANGE_EXPR` defaults pass through unchanged +/// (they are already strings). `LIST[*]` defaults are JSON-serialised +/// by the upstream `default_value()` implementation, so they round-trip +/// through `serde_json`. +/// +/// If parsing fails for any reason (defensive fallback against future +/// upstream serialization changes), the original string is returned +/// so the caller never sees a `None` where v0 would have produced a +/// default. +fn default_to_native( + py: Python<'_>, + param_type: JobParameterType, + raw: &str, +) -> PyResult> { + use pyo3::IntoPyObjectExt; + match param_type { + JobParameterType::String | JobParameterType::Path | JobParameterType::RangeExpr => { + raw.into_py_any(py) + } + JobParameterType::Int => raw + .parse::() + .ok() + .map(|v| v.into_py_any(py)) + .unwrap_or_else(|| raw.into_py_any(py)), + JobParameterType::Float => raw + .parse::() + .ok() + .map(|v| v.into_py_any(py)) + .unwrap_or_else(|| raw.into_py_any(py)), + JobParameterType::Bool => match raw { + "true" => true.into_py_any(py), + "false" => false.into_py_any(py), + _ => raw.into_py_any(py), + }, + JobParameterType::ListString | JobParameterType::ListPath => { + serde_json::from_str::>(raw) + .ok() + .map(|v| v.into_py_any(py)) + .unwrap_or_else(|| raw.into_py_any(py)) + } + JobParameterType::ListInt => serde_json::from_str::>(raw) + .ok() + .map(|v| v.into_py_any(py)) + .unwrap_or_else(|| raw.into_py_any(py)), + JobParameterType::ListFloat => serde_json::from_str::>(raw) + .ok() + .map(|v| v.into_py_any(py)) + .unwrap_or_else(|| raw.into_py_any(py)), + JobParameterType::ListBool => serde_json::from_str::>(raw) + .ok() + .map(|v| v.into_py_any(py)) + .unwrap_or_else(|| raw.into_py_any(py)), + JobParameterType::ListListInt => serde_json::from_str::>>(raw) + .ok() + .map(|v| v.into_py_any(py)) + .unwrap_or_else(|| raw.into_py_any(py)), + // ``JobParameterType`` is ``#[non_exhaustive]`` — any future + // variant we don't know about falls back to the raw string so + // callers see *some* value rather than nothing. + _ => raw.into_py_any(py), + } +} + #[cfg_attr( feature = "stub-gen", gen_stub_pyfunction(module = "openjd._openjd_rs") diff --git a/specs/python-model-interface.md b/specs/python-model-interface.md index 5f3e3ed4..aa8097c7 100644 --- a/specs/python-model-interface.md +++ b/specs/python-model-interface.md @@ -324,6 +324,13 @@ following keys: default) — the default value, in its native Python type (``int`` / ``float`` / ``str`` / ``list`` / …) per the parameter's type. +* ``description`` (``str``, present only if at least one + contributing template provided one) — the human-readable + description copied from the originating template. When more + than one contributing template defines a description for the + same parameter, the description from the template walked last + wins (environment templates are walked in order, then the job + template), mirroring how ``default`` is tracked. * ``objectType`` (``str``, present only for ``PATH`` parameters with an ``objectType`` declared) — ``"FILE"`` or ``"DIRECTORY"``. @@ -342,7 +349,9 @@ values" pass. The underlying Rust crate's ``openjd_model::merge_job_parameter_definitions`` returns a struct with ``source`` / ``name`` / ``param_type`` / ``default`` / ``object_type`` / ``data_flow`` fields; the binding flattens -that struct into the dict above. +that struct into the dict above and additionally recovers +``description`` by walking the same template sources the merge +walked (the upstream struct does not carry a description field). #### `evaluate_let_bindings` @@ -1386,23 +1395,31 @@ Practical consequences: ```python from openjd.model._v1 import decode_job_template -from openjd.model._v1.errors import DecodeValidationError +from openjd.model._v1.errors import DecodeValidationError, ModelValidationError -# Invalid template +# Invalid template — schema-level failure (missing required top-level +# key) raises DecodeValidationError. These are the failures the +# binding can detect before the dict is reshaped into the typed +# model, so they don't need to run through the model validator. try: decode_job_template(template={"bad": "template"}) except DecodeValidationError as e: str(e) # "Template is missing Open Job Description schema version key: specificationVersion" -# Empty steps +# Empty steps — model-level structural failure raises +# ModelValidationError. The shape parses successfully but the +# decoded JobTemplate fails the "at least one step" invariant when +# the model validator runs, so the failure surfaces under +# ModelValidationError rather than DecodeValidationError. See the +# divergence note below. try: decode_job_template(template={ "specificationVersion": "jobtemplate-2023-09", "name": "Test", "steps": [], }) -except DecodeValidationError as e: - str(e) # validation error about empty steps +except ModelValidationError as e: + str(e) # "1 validation error for JobTemplate\nJobTemplate: must have at least one step." ``` | Exception | Base | @@ -1412,6 +1429,28 @@ except DecodeValidationError as e: | `UnsupportedSchema` | `ValueError` | | `ExpressionError` | `ValueError` | +**`DecodeValidationError` vs `ModelValidationError` divergence from v0.** +The v0 (Pydantic) reference raised ``DecodeValidationError`` for *every* +template-decode failure — including model-level structural invariants +like "must have at least one step" — because pydantic's discriminated- +union dispatch ran the field validators inside the decode call. The v1 +binding splits the two phases: ``DecodeValidationError`` is raised +strictly for schema-level failures the binding can detect before +constructing the typed model (missing/unknown ``specificationVersion``, +unknown fields under strict mode, unparseable JSON/YAML, etc.), and +``ModelValidationError`` is raised for everything caught by the model +validator that runs once the decoded shape is in hand (the at-least-one- +step rule, parameter-definition invariants, step-name uniqueness, etc.). +Both classes inherit ``ValueError``, so callers that catch ``ValueError`` +are unaffected — only callers that distinguish between the two classes +need to be aware of the split. The v1 binding **deliberately** does not +remap empty-steps to ``DecodeValidationError`` for v0 byte-parity: the +v1 model validator is the right home for the check (it surfaces with the +same path-prefixed message shape as every other model-level invariant), +and callers wanting to catch every decode-time failure should catch +``ValueError`` or both classes by tuple (``except (DecodeValidationError, +ModelValidationError)``). + **`UnsupportedSchema` constructor divergence from v0.** The v0 reference defines ``UnsupportedSchema(version_str)`` such that ``str(e) == "Unsupported schema version: {version_str}"`` and the @@ -1472,12 +1511,15 @@ original. | ``PathTaskParameter`` | constructor argument (``range``) | | ``ChunkIntTaskParameter`` | constructor arguments (``range``, ``chunks``) | | ``TaskChunksDefinition`` | constructor arguments (three fields) | +| ``SpecificationRevision`` | variant name (``v2023_09``) | +| ``TemplateSpecificationVersion`` | variant name (``JOBTEMPLATE_v2023_09``, ``ENVIRONMENT_v2023_09``) | | ``DecodeValidationError``, ``ModelValidationError``, ``UnsupportedSchema`` | standard exception pickle, under their canonical ``openjd.model._v1`` module path | -``SpecificationRevision`` and ``TemplateSpecificationVersion`` pickle -through the Python ``str``-Enum shims provided by ``openjd.model._v1`` -(the underlying Rust pyclasses live at ``openjd._openjd_rs`` and pickle -correctly there too). +``SpecificationRevision`` and ``TemplateSpecificationVersion`` are Rust +pyclass enums whose canonical home is ``openjd._openjd_rs``; they are +re-exported from ``openjd.model._v1`` (identity-preserving — there is +exactly one class in each case). Pickled instances round-trip through +the variant name via the module-level ``_reconstruct_enum`` helper. The decoded model containers (``JobTemplate``, ``EnvironmentTemplate``, ``Job``, ``Step``, etc.) and the live ``StepParameterSpaceIterator`` / diff --git a/test/openjd/model_v1/test_known_gaps.py b/test/openjd/model_v1/test_known_gaps.py index 227b123c..70b27c65 100644 --- a/test/openjd/model_v1/test_known_gaps.py +++ b/test/openjd/model_v1/test_known_gaps.py @@ -19,149 +19,6 @@ from __future__ import annotations -import pytest - -from openjd.model._v1 import ( - TemplateSpecificationVersion, - decode_job_template, - merge_job_parameter_definitions, -) - - -# ── JobTemplate.specification_version is comparable to TemplateSpecificationVersion ── -# -# Resolved: ``rust-bindings/src/model/types.rs`` (PyTemplateSpecificationVersion) -# was reshaped so the Rust pyclass exposes the v0 ``str``-Enum surface -# directly: lowercase variant names (``JOBTEMPLATE_v2023_09`` / -# ``ENVIRONMENT_v2023_09``), a ``.value`` getter, equality with the -# spec-form string, and a constructor that accepts the spec form or the -# variant name. There is now exactly one -# ``TemplateSpecificationVersion`` class — the one in ``openjd._openjd_rs`` -# is also re-exported at ``openjd.model._v1`` and ``openjd.model._v1.types`` -# (identity-preserving). ``JobTemplate.specification_version`` returns -# instances of the same class. - - -def test_template_specification_version_comparable_to_python_str_enum() -> None: - t = decode_job_template( - template={ - "specificationVersion": "jobtemplate-2023-09", - "name": "T", - "steps": [{"name": "S", "script": {"actions": {"onRun": {"command": "echo"}}}}], - } - ) - assert t.specification_version == TemplateSpecificationVersion.JOBTEMPLATE_v2023_09 - - -# ── merge_job_parameter_definitions: default is always a string ── -# -# The spec for ``merge_job_parameter_definitions`` ("Return shape — -# list[dict], not typed pyclasses") says: -# -# ``default`` (present only if the source template provided a -# default) — the default value, in its native Python type -# (``int`` / ``float`` / ``str`` / ``list`` / …) per the -# parameter's type. -# -# In practice the binding emits the default as a ``str`` for every -# variant — including ``INT`` (where v0 carried the int) and ``FLOAT`` -# (where v0 carried the float). The to-display-string coercion lives -# in ``rust-bindings/src/model/create_job_fns.rs::py_merge_job_parameter_definitions`` -# (the ``default`` extraction routes through ``param_value.to_display_string()``). -# -# Fix path: at the v1 binding boundary, convert the default to its -# native Python type for ``INT`` (``int``), ``FLOAT`` (``float``), -# ``BOOL`` (``bool``), and the ``LIST[*]`` variants (``list[T]``). -# Strings stay as ``str``; ``PATH`` stays as ``str`` (matching the -# spec's "STRING / PATH" handling elsewhere). The Rust crate's -# ``MergedParameterDefinition::default`` already carries the typed -# value — the binding just needs to dispatch on ``param_type`` when -# inserting into the dict. -# -# Cross-reference: report Recommendation #3. - - -@pytest.mark.xfail( - strict=True, reason="v1 emits default as str for every type; spec promises native Python type" -) -def test_merge_default_int_returned_as_int() -> None: - t = decode_job_template( - template={ - "specificationVersion": "jobtemplate-2023-09", - "name": "T", - "parameterDefinitions": [ - {"name": "Count", "type": "INT", "default": 5}, - ], - "steps": [{"name": "S", "script": {"actions": {"onRun": {"command": "echo"}}}}], - } - ) - merged = merge_job_parameter_definitions(job_template=t) - [count_def] = merged - assert count_def["default"] == 5 - assert isinstance(count_def["default"], int) - assert not isinstance(count_def["default"], str) - - -@pytest.mark.xfail( - strict=True, reason="v1 emits default as str for every type; spec promises native Python type" -) -def test_merge_default_float_returned_as_float() -> None: - t = decode_job_template( - template={ - "specificationVersion": "jobtemplate-2023-09", - "name": "T", - "parameterDefinitions": [ - {"name": "Pi", "type": "FLOAT", "default": 3.14}, - ], - "steps": [{"name": "S", "script": {"actions": {"onRun": {"command": "echo"}}}}], - } - ) - merged = merge_job_parameter_definitions(job_template=t) - [pi_def] = merged - assert pi_def["default"] == 3.14 - assert isinstance(pi_def["default"], float) - - -# ── merge_job_parameter_definitions drops the description field ── -# -# v0's ``merge_job_parameter_definitions`` returned a list of typed -# ``Job*ParameterDefinition`` pyclasses; each carried the per-template -# ``description: Optional[str]`` field copied straight from the -# template. The v1 binding flattens every variant to a dict, but the -# spec-listed keys (``name``, ``type``, ``source``, ``default``, -# ``objectType``, ``dataFlow``) **do not include** ``description`` — -# and the binding does not emit one. Downstream tooling that builds -# UI prompts ("show the user every parameter and ask for values") -# loses access to the per-parameter human-readable description. -# -# The underlying Rust crate's -# ``openjd_model::merge_job_parameter_definitions`` does not surface -# a description on the merged struct (only on the original -# ``JobParameterDefinition``). The v1 binding could either: -# (a) add ``description`` to the merged dict by reading it back off -# the originating template's ``parameter_definitions`` list, or -# (b) document the omission in the spec and steer callers to read -# the description off ``JobTemplate.parameter_definitions``. -# -# Cross-reference: report Recommendation #4. - - -@pytest.mark.xfail(strict=True, reason="v1 drops description; spec lists no description key") -def test_merge_includes_description() -> None: - t = decode_job_template( - template={ - "specificationVersion": "jobtemplate-2023-09", - "name": "T", - "parameterDefinitions": [ - {"name": "Count", "type": "INT", "default": 5, "description": "Number of frames"}, - ], - "steps": [{"name": "S", "script": {"actions": {"onRun": {"command": "echo"}}}}], - } - ) - merged = merge_job_parameter_definitions(job_template=t) - [count_def] = merged - assert count_def.get("description") == "Number of frames" - # ── Top-level package no longer leaks typing imports ── # @@ -177,6 +34,9 @@ def test_merge_includes_description() -> None: # none of them have been re-introduced. +import pytest + + @pytest.mark.parametrize("name", ["Any", "Optional", "Sequence", "Union", "re", "Enum"]) def test_no_internal_imports_leak_at_top_level(name: str) -> None: import openjd.model._v1 as v1 diff --git a/test/openjd/model_v1/test_merge_job_parameters.py b/test/openjd/model_v1/test_merge_job_parameters.py index f0fd085b..91f37964 100644 --- a/test/openjd/model_v1/test_merge_job_parameters.py +++ b/test/openjd/model_v1/test_merge_job_parameters.py @@ -98,7 +98,7 @@ class TestMergeTemplates_v2023_09: ), ], [ - {"name": "Foo", "type": "INT", "default": "8", "source": "JobTemplate"}, + {"name": "Foo", "type": "INT", "default": 8, "source": "JobTemplate"}, { "name": "Bar", "type": "STRING", @@ -134,7 +134,7 @@ class TestMergeTemplates_v2023_09: ), ], [ - {"name": "Foo", "type": "INT", "default": "42", "source": "JobTemplate"}, + {"name": "Foo", "type": "INT", "default": 42, "source": "JobTemplate"}, ], id="environment default propagates when job template has none", ), @@ -157,3 +157,236 @@ def _to_comparable(param_list): return {tuple(sorted(p.items())) for p in param_list} assert _to_comparable(result) == _to_comparable(expected) + + +class TestMergeDefaultNativeTypes: + """``merge_job_parameter_definitions`` emits ``default`` in the + parameter's native Python type — ``int`` for ``INT``, ``float`` for + ``FLOAT``, ``bool`` for ``BOOL``, ``list[T]`` for the ``LIST[*]`` + variants, and ``str`` for ``STRING`` / ``PATH`` / ``RANGE_EXPR``. + + Pinned per `specs/python-model-interface.md`'s + ``merge_job_parameter_definitions`` "Return shape" key list and the + AGENTS.md "Test Quality Standard" guidance (assertions on type + identity, not just equality, since ``5 == 5.0`` would otherwise + let a regression slip through).""" + + @staticmethod + def _merge_one(*, param_type: str, default: Any, **extra: Any) -> dict[str, Any]: + # The EXPR-extension parameter types (BOOL, RANGE_EXPR, + # LIST[*]) require the template to opt into the EXPR extension + # both in its body and via supported_extensions. The base + # STRING/INT/FLOAT/PATH variants are unaffected. + t = decode_job_template( + template={ + "specificationVersion": "jobtemplate-2023-09", + "name": "T", + "extensions": ["EXPR"], + "parameterDefinitions": [ + {"name": "P", "type": param_type, "default": default, **extra}, + ], + "steps": [BASIC_JOB_TEMPLATE_STEP_2023_09], + }, + supported_extensions=["EXPR"], + ) + merged = merge_job_parameter_definitions(job_template=t) + assert len(merged) == 1 + return merged[0] + + def test_int(self) -> None: + m = self._merge_one(param_type="INT", default=5) + assert m["default"] == 5 + assert isinstance(m["default"], int) + + def test_float(self) -> None: + m = self._merge_one(param_type="FLOAT", default=3.14) + assert m["default"] == 3.14 + assert isinstance(m["default"], float) + + def test_string(self) -> None: + m = self._merge_one(param_type="STRING", default="hello") + assert m["default"] == "hello" + assert isinstance(m["default"], str) + + def test_path(self) -> None: + m = self._merge_one(param_type="PATH", default="/tmp/out") + assert m["default"] == "/tmp/out" + assert isinstance(m["default"], str) + + def test_bool(self) -> None: + m = self._merge_one(param_type="BOOL", default=True) + assert m["default"] is True + + def test_list_int(self) -> None: + m = self._merge_one(param_type="LIST[INT]", default=[1, 2, 3]) + assert m["default"] == [1, 2, 3] + assert isinstance(m["default"], list) + assert all(isinstance(v, int) and not isinstance(v, bool) for v in m["default"]) + + def test_list_float(self) -> None: + m = self._merge_one(param_type="LIST[FLOAT]", default=[1.5, 2.5]) + assert m["default"] == [1.5, 2.5] + assert all(isinstance(v, float) for v in m["default"]) + + def test_list_string(self) -> None: + m = self._merge_one(param_type="LIST[STRING]", default=["a", "b"]) + assert m["default"] == ["a", "b"] + assert all(isinstance(v, str) for v in m["default"]) + + def test_list_bool(self) -> None: + m = self._merge_one(param_type="LIST[BOOL]", default=[True, False]) + assert m["default"] == [True, False] + assert all(isinstance(v, bool) for v in m["default"]) + + def test_list_list_int(self) -> None: + m = self._merge_one(param_type="LIST[LIST[INT]]", default=[[1, 2], [3, 4]]) + assert m["default"] == [[1, 2], [3, 4]] + assert isinstance(m["default"], list) + assert all( + isinstance(inner, list) and all(isinstance(v, int) for v in inner) + for inner in m["default"] + ) + + +class TestMergeDescriptionPropagation: + """``merge_job_parameter_definitions`` propagates the per-parameter + ``description`` from the originating template onto each merged + dict, mirroring how ``default`` is carried. + + The upstream ``MergedParameterDefinition`` struct does not surface + a description field — only ``name`` / ``param_type`` / ``default`` + / ``object_type`` / ``data_flow`` / ``source`` / merged + constraints. The binding recovers it by walking the same template + sources the merge walked (environment templates in order, then the + job template), with later-defined descriptions overwriting earlier + ones (matching how ``default`` is tracked upstream). + + The ``description`` key is **only** present when at least one + contributing template provided one; templates without a + ``description`` produce a dict that does not carry the key (vs. + carrying ``None``).""" + + def test_description_from_job_template(self) -> None: + t = decode_job_template( + template={ + "specificationVersion": "jobtemplate-2023-09", + "name": "T", + "parameterDefinitions": [ + { + "name": "Count", + "type": "INT", + "default": 5, + "description": "Number of frames", + }, + ], + "steps": [BASIC_JOB_TEMPLATE_STEP_2023_09], + } + ) + [m] = merge_job_parameter_definitions(job_template=t) + assert m["description"] == "Number of frames" + + def test_description_from_environment_template(self) -> None: + t = decode_job_template( + template={ + "specificationVersion": "jobtemplate-2023-09", + "name": "T", + "parameterDefinitions": [ + {"name": "Count", "type": "INT", "default": 5}, + ], + "steps": [BASIC_JOB_TEMPLATE_STEP_2023_09], + } + ) + env = decode_environment_template( + template={ + "specificationVersion": "environment-2023-09", + "parameterDefinitions": [ + {"name": "Count", "type": "INT", "description": "env description"}, + ], + "environment": {"name": "E", **BASIC_ENVIRONMENT_TEMPLATE_ACTION_2023_09}, + } + ) + [m] = merge_job_parameter_definitions(job_template=t, environment_templates=[env]) + assert m["description"] == "env description" + + def test_job_template_description_overrides_environment(self) -> None: + # When both an env template and the job template provide a + # description, the job template's wins (it's walked last; + # matches how ``default`` is tracked). + t = decode_job_template( + template={ + "specificationVersion": "jobtemplate-2023-09", + "name": "T", + "parameterDefinitions": [ + { + "name": "Count", + "type": "INT", + "default": 5, + "description": "from job template", + }, + ], + "steps": [BASIC_JOB_TEMPLATE_STEP_2023_09], + } + ) + env = decode_environment_template( + template={ + "specificationVersion": "environment-2023-09", + "parameterDefinitions": [ + {"name": "Count", "type": "INT", "description": "from env template"}, + ], + "environment": {"name": "E", **BASIC_ENVIRONMENT_TEMPLATE_ACTION_2023_09}, + } + ) + [m] = merge_job_parameter_definitions(job_template=t, environment_templates=[env]) + assert m["description"] == "from job template" + + def test_later_environment_description_overrides_earlier(self) -> None: + # When two env templates both provide a description (and the + # job template doesn't), the later one wins. + t = decode_job_template( + template={ + "specificationVersion": "jobtemplate-2023-09", + "name": "T", + "parameterDefinitions": [ + {"name": "Count", "type": "INT", "default": 5}, + ], + "steps": [BASIC_JOB_TEMPLATE_STEP_2023_09], + } + ) + env1 = decode_environment_template( + template={ + "specificationVersion": "environment-2023-09", + "parameterDefinitions": [ + {"name": "Count", "type": "INT", "description": "from env1"}, + ], + "environment": {"name": "E1", **BASIC_ENVIRONMENT_TEMPLATE_ACTION_2023_09}, + } + ) + env2 = decode_environment_template( + template={ + "specificationVersion": "environment-2023-09", + "parameterDefinitions": [ + {"name": "Count", "type": "INT", "description": "from env2"}, + ], + "environment": {"name": "E2", **BASIC_ENVIRONMENT_TEMPLATE_ACTION_2023_09}, + } + ) + [m] = merge_job_parameter_definitions(job_template=t, environment_templates=[env1, env2]) + assert m["description"] == "from env2" + + def test_no_description_means_key_absent(self) -> None: + # When no contributing template provides a description, the + # ``description`` key is omitted from the merged dict (not + # carried as ``None``). Consistent with how ``default`` / + # ``objectType`` / ``dataFlow`` are conditionally emitted. + t = decode_job_template( + template={ + "specificationVersion": "jobtemplate-2023-09", + "name": "T", + "parameterDefinitions": [ + {"name": "Count", "type": "INT", "default": 5}, + ], + "steps": [BASIC_JOB_TEMPLATE_STEP_2023_09], + } + ) + [m] = merge_job_parameter_definitions(job_template=t) + assert "description" not in m diff --git a/test/openjd/model_v1/test_parse.py b/test/openjd/model_v1/test_parse.py index 3f48ace3..482a9bca 100644 --- a/test/openjd/model_v1/test_parse.py +++ b/test/openjd/model_v1/test_parse.py @@ -74,13 +74,20 @@ def test_empty_steps_raises_model_validation_error(self) -> None: # (v0 raised the latter, which v1 deliberately corrects — # ``DecodeValidationError`` is reserved for parse-stage failures # like unknown specificationVersion or malformed YAML/JSON). + # The divergence is documented in + # ``specs/python-model-interface.md`` under "Exceptions". template = { "specificationVersion": "jobtemplate-2023-09", "name": "T", "steps": [], } - with pytest.raises(ModelValidationError): + with pytest.raises(ModelValidationError) as exc_info: decode_job_template(template=template) + # Pin the message body (per the AGENTS.md "Test Quality Standard": + # exception class + message body, not just class). + assert str(exc_info.value) == ( + "1 validation error for JobTemplate\n" "JobTemplate: must have at least one step." + ) class TestDecodeEnvironmentTemplate: diff --git a/test/openjd/model_v1/test_version_enums.py b/test/openjd/model_v1/test_version_enums.py index b29cfdb2..efbfaf49 100644 --- a/test/openjd/model_v1/test_version_enums.py +++ b/test/openjd/model_v1/test_version_enums.py @@ -2,7 +2,7 @@ import pytest -from openjd.model._v1 import TemplateSpecificationVersion +from openjd.model._v1 import TemplateSpecificationVersion, decode_job_template # All known variants. Add new ones here as the spec evolves; the test @@ -30,3 +30,30 @@ def test_job_template_versions(self, version: TemplateSpecificationVersion) -> N def test_environment_template_versions(self, version: TemplateSpecificationVersion) -> None: assert version.is_environment_template() assert not version.is_job_template() + + def test_template_specification_version_returned_from_decode(self) -> None: + # ``JobTemplate.specification_version`` returns an instance of + # the same ``TemplateSpecificationVersion`` class that + # ``openjd.model._v1`` re-exports — i.e. there is exactly one + # class, the Rust pyclass at ``openjd._openjd_rs``, and the + # comparison ``template.specification_version == + # TemplateSpecificationVersion.JOBTEMPLATE_v2023_09`` works as + # written without any str-Enum shim. Regression test for the + # historical gap where ``JobTemplate.specification_version`` + # returned the Rust pyclass but the public-name + # ``TemplateSpecificationVersion`` was a separate Python + # ``str``-Enum, so the natural-looking equality silently + # returned ``False``. + t = decode_job_template( + template={ + "specificationVersion": "jobtemplate-2023-09", + "name": "T", + "steps": [{"name": "S", "script": {"actions": {"onRun": {"command": "echo"}}}}], + } + ) + assert t.specification_version == TemplateSpecificationVersion.JOBTEMPLATE_v2023_09 + assert type(t.specification_version) is TemplateSpecificationVersion + # Equality is symmetric with the spec-form string (str-Enum- + # like behaviour without being a str subclass). + assert t.specification_version == "jobtemplate-2023-09" + assert t.specification_version.value == "jobtemplate-2023-09"