From 9cd6b77676b3fb6787ef7971b2ec2e0610c3b650 Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Thu, 3 Sep 2026 14:19:45 -0700 Subject: [PATCH 1/2] fix: Render a PATH parameter in the host's path format A session is host scope, so a `path` there takes the host operating system's semantics -- separators included (Expression Language 1.2.1: "In host contexts (SESSION and TASK scopes), the semantics match the host's operating system"). This package applied the host's separators only *inside* a matching path mapping rule, in `PathMappingRule.apply`, so that was the only thing that ever chose one. A PATH parameter that no rule matched reached the task in whatever form the submitter wrote it. Measured on a Windows host with no rules, `Task.Param.InputFile` for `/path/a.exr` was `/path/a.exr`, where conformance fixture 2023-09/base/jobs/3.4--path-parameter.test.yaml requires `\path\a.exr`; with a rule present but not matching, the same. That fixture has been failing on the Python conformance job's windows-latest leg continuously since 2026-05-22. openjd-rs applies the format unconditionally, wrapping both the mapped and the unmapped case in `ExprValue::new_path(mapped, PathFormat::host())` for `Param.`, `Task.Param.` and each element of the LIST[PATH] forms, while leaving `RawParam.*` raw. `processed_parameter_value` now mirrors that: `to_host_path_separators` wraps `apply_mapping` on both PATH branches, which is idempotent for a value a rule did match and so keeps the two paths agreeing. `to_host_path_separators` duplicates Rust's `normalize_path_separators` rather than calling it through `openjd.expr`. The fixture that pins this is a base-spec template, so the fix sits on the non-EXPR path that test/openjd/ test_import_purity.py protects, and the native extension must not become a load-time requirement there. The URI arm reuses this module's existing `_URI_SOURCE_RE`, so only Rust's three-way branch is restated. It lives in `_path_mapping` and reads that module's `os_name` deliberately: `apply` reads the same name, and deriving the host twice would let the two disagree. Separators and nothing else. Rendering through PureWindowsPath would satisfy the headline assertion and still be wrong -- measured, it turns `s3://bucket/key` into `s3:\bucket\key`, collapses `/a//b` to `\a\b`, drops the trailing separator that `apply` deliberately preserves, and renders `""` as `.`. 17 tests, written first and failing 7/17 against the unmodified source. Full suite unchanged apart from those 7 (15 failed/996 passed -> 8 failed/1003 passed, the 8 pre-existing and environmental). All 7 mutants caught, including one that formats `RawParam.*` too and one that swaps in PureWindowsPath. Windows is exercised through a patched seam rather than on Windows, so the end-to-end confirmation is 3.4--path-parameter's next windows-latest result. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_path_mapping.py | 35 +++ src/openjd/sessions/_session.py | 15 +- .../test_path_parameter_host_format.py | 278 ++++++++++++++++++ 3 files changed, 325 insertions(+), 3 deletions(-) create mode 100644 test/openjd/sessions_v0/test_path_parameter_host_format.py diff --git a/src/openjd/sessions/_path_mapping.py b/src/openjd/sessions/_path_mapping.py index 48e1cad0..2abc6f35 100644 --- a/src/openjd/sessions/_path_mapping.py +++ b/src/openjd/sessions/_path_mapping.py @@ -26,6 +26,41 @@ def _ascii_lower(value: str) -> str: return value.translate(_ASCII_LOWER_TABLE) +def to_host_path_separators(value: str) -> str: + """Render ``value`` with this host's path separators. + + A session is host scope, so a ``path`` there takes the host operating + system's semantics. openjd-rs applies that to every host-scope path value by + constructing it through ``ExprValue::new_path(.., PathFormat::host())``, + which calls ``normalize_path_separators`` + (``crates/openjd-expr/src/value.rs``). This is that function for the host's + format, and the three cases are its three arms: + + - a URI keeps forward slashes on every host, because its path portion is a + set of opaque identifiers rather than a filesystem path; + - a POSIX host changes nothing, because a backslash is a legal character in + a POSIX filename and rewriting one would corrupt the path; + - a Windows host replaces ``/`` with ``\\``. + + Separators and nothing else. Rendering through ``PureWindowsPath`` would + also collapse ``//`` to ``\\``, drop a trailing separator, and turn ``""`` + into ``"."`` -- and :meth:`PathMappingRule.apply` deliberately preserves a + trailing separator, so that one is a behaviour this must not undo. + + Duplicated from Rust rather than called through ``openjd.expr``: the native + extension must not become a load-time requirement of a non-EXPR session (see + ``test/openjd/test_import_purity.py``), and a PATH parameter reaches this on + the non-EXPR path. The URI test reuses :data:`_URI_SOURCE_RE`, which is this + module's existing spelling of the same ``://`` rule, so the + duplication is of Rust's three-way branch only. + """ + if _URI_SOURCE_RE.match(value) is not None: + return value + if os_name == "posix": + return value + return value.replace("/", "\\") + + _URI_SOURCE_RE = re.compile(r"[A-Za-z][A-Za-z0-9+.\-]*://") """A URI-format rule's ``source_path`` must start with ``://``. diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py index 77c55b60..e98a3147 100644 --- a/src/openjd/sessions/_session.py +++ b/src/openjd/sessions/_session.py @@ -41,7 +41,7 @@ from ._embedded_files import EmbeddedFiles, EmbeddedFilesScope, _FileRecord, write_file_for_user from ._logging import LOG, log_section_banner, LoggerAdapter, LogExtraInfo, LogContent from ._os_checker import is_posix, is_windows -from ._path_mapping import PathMappingRule +from ._path_mapping import PathMappingRule, to_host_path_separators from ._runner_base import ( ScriptRunnerBase, apply_let_bindings, @@ -1670,12 +1670,21 @@ def apply_mapping(path: str) -> str: return path def processed_parameter_value(param: ParameterValue) -> Any: + # The host format is applied to every PATH value, not only to a + # mapped one. `apply_mapping` chooses the host's separator for a + # rule's output, so before this it was the *only* thing that did: + # a value no rule matched reached the task in whatever form the + # submitter wrote it, and on a Windows host a POSIX-spelled + # parameter stayed POSIX-spelled. openjd-rs wraps both the mapped + # and the unmapped case in `ExprValue::new_path(.., host())`. + # Applying it after `apply_mapping` is idempotent for a value a rule + # did match, which is what keeps the two paths agreeing. if param.type == ParameterValueType.PATH: - return apply_mapping(param.value) + return to_host_path_separators(apply_mapping(param.value)) if param.type == ParameterValueType.LIST_PATH and isinstance(param.value, list): # openjd-rs maps each element of a LIST[PATH] parameter at # session scope; mirror that element-wise mapping. - return [apply_mapping(p) for p in param.value] + return [to_host_path_separators(apply_mapping(p)) for p in param.value] return param.value def record_expr_types( diff --git a/test/openjd/sessions_v0/test_path_parameter_host_format.py b/test/openjd/sessions_v0/test_path_parameter_host_format.py new file mode 100644 index 00000000..f240c274 --- /dev/null +++ b/test/openjd/sessions_v0/test_path_parameter_host_format.py @@ -0,0 +1,278 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""A ``PATH`` parameter's ``Param.*``/``Task.Param.*`` value must render in the +host's path format. + +The session is host scope, and the Expression Language spec says a ``path`` there +takes the host operating system's semantics -- separators included. openjd-rs +does that unconditionally when it builds the session symbol table: every +``Param.`` and ``Task.Param.`` is wrapped in +``ExprValue::new_path(mapped, PathFormat::host())`` (``crates/openjd-sessions/ +src/session.rs``), so the format is applied whether or not a path mapping rule +matched. + +This package used to apply the host's separators only *inside* +``PathMappingRule.apply``, so a parameter with no matching rule reached the task +in whatever form the submitter wrote it. On a Windows host a POSIX-spelled +parameter stayed POSIX-spelled, which is what conformance fixture +``2023-09/base/jobs/3.4--path-parameter.test.yaml`` catches: + + output_windows: + - TASK:InputFile=\\path\\a.exr + +On simulating a Windows host. A POSIX host renders both readings identically, so +comparing values on this machine proves nothing -- the assertions would pass +whatever the code did. ``_windows_host`` forces the other format. + +It patches exactly one seam, ``openjd.sessions._path_mapping.os_name``, and that +is deliberate rather than a simplification: that module-level name is the single +place this package decides which separator a host-scope path uses. It is what +``PathMappingRule.apply`` reads for the separator of a rule's output, and it is +what ``host_path_format`` reads for the format of an unmapped value. Patching one +and not the other would produce a self-inconsistent host -- mapped values +rendering Windows while unmapped ones rendered POSIX -- and a test built on that +would assert an arrangement that cannot occur in production. Keeping both readers +on one seam is the reason the fix put ``host_path_format`` in ``_path_mapping`` +rather than deriving ``os.name`` a second time in ``_session``. +""" + +from __future__ import annotations + +from contextlib import contextmanager +from pathlib import PurePosixPath, PureWindowsPath +from typing import Generator +from unittest.mock import patch + +import pytest + +from openjd.model import ( + ParameterValue, + ParameterValueType, + SpecificationRevision, + SymbolTable, +) +from openjd.sessions import PathFormat, PathMappingRule, Session + +import openjd.sessions._path_mapping as path_mapping_impl_mod + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +_POSIX_TEXT = "/path/a.exr" +"""The parameter as a submitter wrote it.""" + +_WINDOWS_TEXT = r"\path\a.exr" +"""The same parameter as a Windows host must render it. Distinct from +``_POSIX_TEXT``, which is what an unformatted value shows.""" + + +@contextmanager +def _host(os_name: str) -> Generator[None, None, None]: + """Force the host path format. See this module's docstring for why one seam + is enough, and why it is this one.""" + with patch.object(path_mapping_impl_mod, "os_name", os_name): + yield + + +def _symtab( + params: dict[str, ParameterValue], + *, + os_name: str, + rules: list[PathMappingRule] | None = None, +) -> SymbolTable: + """Build a session symbol table the way a running session does.""" + with Session( + session_id="test-path-format", + job_parameter_values=params, + path_mapping_rules=rules or [], + ) as session: + with _host(os_name): + return session._symbol_table(SpecificationRevision.v2023_09, params) + + +def _path_param(value: str) -> dict[str, ParameterValue]: + return {"InputFile": ParameterValue(type=ParameterValueType.PATH, value=value)} + + +# --------------------------------------------------------------------------- +# The defect +# --------------------------------------------------------------------------- + + +class TestPathParameterTakesTheHostFormat: + """``Param.*``/``Task.Param.*`` for a PATH parameter render in the host's + format, with no path mapping rule involved.""" + + @pytest.mark.parametrize("key", ["Param.InputFile", "Task.Param.InputFile"]) + def test_windows_host_renders_backslashes(self, key: str) -> None: + # GIVEN a POSIX-spelled PATH parameter and no path mapping rules + # WHEN the session symbol table is built on a Windows host + symtab = _symtab(_path_param(_POSIX_TEXT), os_name="nt") + + # THEN the value carries the host's separators + assert str(symtab[key]) == _WINDOWS_TEXT + + @pytest.mark.parametrize("key", ["Param.InputFile", "Task.Param.InputFile"]) + def test_posix_host_leaves_the_value_alone(self, key: str) -> None: + """The negative control for the test above: a POSIX host must not + rewrite anything, so a fix that always converts is caught here.""" + # GIVEN the same parameter + # WHEN the table is built on a POSIX host + symtab = _symtab(_path_param(_POSIX_TEXT), os_name="posix") + + # THEN it is unchanged + assert str(symtab[key]) == _POSIX_TEXT + + @pytest.mark.parametrize("key", ["RawParam.InputFile", "Task.RawParam.InputFile"]) + def test_the_raw_form_is_not_reformatted(self, key: str) -> None: + """openjd-rs passes a PATH parameter's raw value through untouched + (``JobParameterType::Path | ListPath => param.value.clone()``), so the + fix must not reach ``RawParam.*``. Without this, a fix that normalizes + both forms looks correct on every other assertion here.""" + # GIVEN a POSIX-spelled PATH parameter + # WHEN the table is built on a Windows host + symtab = _symtab(_path_param(_POSIX_TEXT), os_name="nt") + + # THEN the raw form is still what the submitter wrote + assert str(symtab[key]) == _POSIX_TEXT + + +class TestPathParameterFormatAndPathMappingAgree: + """The host format is applied whether or not a rule matched. This is the + property that ``PathMappingRule.apply`` alone cannot provide.""" + + def test_a_non_matching_rule_still_leaves_a_host_format_value(self) -> None: + """The defect in its narrowest form: a rule exists, so the mapping code + runs, but it does not match, so its separator choice never applies.""" + # GIVEN a rule that cannot match the parameter + rules = [ + PathMappingRule( + source_path_format=PathFormat.POSIX, + source_path=PurePosixPath("/nowhere"), + destination_path=PurePosixPath("/elsewhere"), + ) + ] + + # WHEN the table is built on a Windows host + symtab = _symtab(_path_param(_POSIX_TEXT), os_name="nt", rules=rules) + + # THEN the unmapped value still took the host's format + assert str(symtab["Task.Param.InputFile"]) == _WINDOWS_TEXT + + def test_a_matching_rule_is_unchanged_by_the_fix(self) -> None: + """A matching rule already emitted host separators, so normalizing after + it must be a no-op rather than a second transformation.""" + # GIVEN a rule that matches + rules = [ + PathMappingRule( + source_path_format=PathFormat.POSIX, + source_path=PurePosixPath("/path"), + destination_path=PureWindowsPath(r"C:\dest"), + ) + ] + + # WHEN the table is built on a Windows host + symtab = _symtab(_path_param(_POSIX_TEXT), os_name="nt", rules=rules) + + # THEN the mapped value is what path mapping produced, unaltered + assert str(symtab["Task.Param.InputFile"]) == r"C:\dest\a.exr" + + +class TestPathParameterFormatIsSeparatorsOnly: + """Guards the *shape* of the conversion. Rendering through + ``PureWindowsPath``/``PurePosixPath`` would satisfy the assertions above and + still be wrong: it collapses duplicate separators, drops a trailing + separator, rewrites ``s3://`` to ``s3:/``, and turns an empty value into + ``'.'``. openjd-rs replaces separators and nothing else + (``normalize_path_separators``), so these pin that. + """ + + @pytest.mark.parametrize( + "given,expected", + [ + pytest.param("/a//b", r"\a\\b", id="duplicate separators survive"), + pytest.param("/a/", "\\a\\", id="trailing separator survives"), + pytest.param("relative/path", r"relative\path", id="relative path"), + pytest.param("", "", id="empty value stays empty"), + pytest.param(r"C:\already\windows", r"C:\already\windows", id="already windows"), + ], + ) + def test_windows_host_replaces_separators_only(self, given: str, expected: str) -> None: + # GIVEN a PATH parameter whose text a PurePath would rewrite + # WHEN the table is built on a Windows host + symtab = _symtab(_path_param(given), os_name="nt") + + # THEN only the separators changed + assert str(symtab["Task.Param.InputFile"]) == expected + + def test_a_uri_is_left_alone(self) -> None: + """A ``path`` holding a URI keeps forward slashes on every host. The + Expression Language spec makes URI paths exempt from the host's + separator, and openjd-rs implements that in the same function the fix + reaches (``is_uri`` short-circuits ``normalize_path_separators``).""" + # GIVEN a PATH parameter holding a URI + given = "s3://bucket/key/with/slashes" + + # WHEN the table is built on a Windows host + symtab = _symtab(_path_param(given), os_name="nt") + + # THEN it is untouched + assert str(symtab["Task.Param.InputFile"]) == given + + +class TestListPathParameterTakesTheHostFormat: + """openjd-rs formats a ``LIST[PATH]`` element-wise. So must this.""" + + def test_windows_host_formats_every_element(self) -> None: + # GIVEN a LIST[PATH] parameter of POSIX-spelled elements + params = { + "Inputs": ParameterValue( + type=ParameterValueType.LIST_PATH, + value=["/path/a.exr", "/other/b.exr"], + ) + } + + # WHEN the table is built on a Windows host + symtab = _symtab(params, os_name="nt") + + # THEN every element carries the host's separators + assert [str(element) for element in symtab["Task.Param.Inputs"]] == [ + r"\path\a.exr", + r"\other\b.exr", + ] + + def test_posix_host_leaves_every_element_alone(self) -> None: + # GIVEN the same parameter + params = { + "Inputs": ParameterValue( + type=ParameterValueType.LIST_PATH, + value=["/path/a.exr", "/other/b.exr"], + ) + } + + # WHEN the table is built on a POSIX host + symtab = _symtab(params, os_name="posix") + + # THEN nothing changed + assert [str(element) for element in symtab["Task.Param.Inputs"]] == [ + "/path/a.exr", + "/other/b.exr", + ] + + +class TestOtherParameterTypesAreUntouched: + """The fix is scoped to PATH and LIST[PATH]. A STRING that happens to look + like a path must not be reformatted -- openjd-rs coerces it as a string.""" + + def test_a_string_parameter_keeps_its_slashes_on_a_windows_host(self) -> None: + # GIVEN a STRING parameter whose value looks like a POSIX path + params = {"Text": ParameterValue(type=ParameterValueType.STRING, value=_POSIX_TEXT)} + + # WHEN the table is built on a Windows host + symtab = _symtab(params, os_name="nt") + + # THEN it is still the string the submitter wrote + assert str(symtab["Param.Text"]) == _POSIX_TEXT + assert str(symtab["Task.Param.Text"]) == _POSIX_TEXT From c7bbc2259a43949f1c078d6b40e5bbd2c288e6ed Mon Sep 17 00:00:00 2001 From: David Leong <116610336+leongdl@users.noreply.github.com> Date: Thu, 3 Sep 2026 16:03:06 -0700 Subject: [PATCH 2/2] fix: Address review on the PATH host-format change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from the automated review. Three were correct, one was not. **Correct.** The comment claiming this is "idempotent for a value a rule did match" overclaimed. It holds for a POSIX- or WINDOWS-format rule, whose output `apply` rebuilds through `PureWindowsPath`, but not for a URI-format rule: `_apply_uri` copies `destination_path` verbatim and uses the host separator only for the appended child parts. Measured, a URI rule with a POSIX-spelled destination on a Windows host gives `/tmp/openjd\scene\out` from `apply` alone, which this then completes to `\tmp\openjd\scene\out`. That is a real behaviour change on a matched rule and no test covered it. It is also exactly what openjd-rs produces for the same mapped string, measured through `new_path(mapped, host())`, so completing it is the intent rather than a side effect. The comment now states both cases and `test_a_matching_uri_rule_is_completed_not_left_mixed` pins them, asserting the `apply`-only value first so a future change to `_apply_uri` says which half moved. **Correct.** Two maintainability points: `to_host_path_separators` was defined above the `_URI_SOURCE_RE` it reads, which worked only because the global is resolved at call time; it now follows the regex. And the test module's docstring named a `host_path_format` that never shipped -- an earlier name for this function -- so a reader following it found nothing. **Not an error.** The review flagged `_URI_SOURCE_RE`'s one-character scheme as misclassifying a Windows drive letter, since `C://Users/foo` is treated as a URI and returned unchanged while `C:/Users/foo` is converted, and asked whether openjd-rs guards against a one-character scheme. It does not. Measured through the engine, openjd-rs renders `C://Users/foo`, `C:/Users/foo`, `x://y/z` and four other cases identically to this function -- 7/7 agreement. RFC 3986 §3.1 admits a single-character scheme and the Expression Language states the pattern as `^[a-zA-Z][a-zA-Z0-9+.-]*://`, so this is the specified behaviour, and tightening the regex would make this package diverge from the implementation it is being aligned with. Pinned deliberately by `test_a_one_character_scheme_matches_the_engine` so it is not later "fixed" into a divergence; if the behaviour is wrong it is wrong in the specification. Also, per review feedback, the three branches now carry the openjd-expr source they were transcribed from -- `normalize_path_separators` in `crates/openjd-expr/src/value.rs` -- quoted inline so the copy can be diffed against the original without leaving the file. `normalize_path_separators` and the `uri_path::parse` behind its `is_uri` are byte identical in openjd-expr 0.5.0 and 0.6.0, checked against both crates.io sources rather than assumed. No production logic changed: the diff is one moved function, comments, and tests. 21 tests now (was 17), all 7 mutants still caught, and the full suite is unchanged at the same 8 pre-existing environmental failures with 1007 passing (was 1003). Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com> --- src/openjd/sessions/_path_mapping.py | 48 +++++++++-- src/openjd/sessions/_session.py | 14 ++- .../test_path_parameter_host_format.py | 86 +++++++++++++++++-- 3 files changed, 131 insertions(+), 17 deletions(-) diff --git a/src/openjd/sessions/_path_mapping.py b/src/openjd/sessions/_path_mapping.py index 2abc6f35..c9da0b7a 100644 --- a/src/openjd/sessions/_path_mapping.py +++ b/src/openjd/sessions/_path_mapping.py @@ -26,6 +26,14 @@ def _ascii_lower(value: str) -> str: return value.translate(_ASCII_LOWER_TABLE) +_URI_SOURCE_RE = re.compile(r"[A-Za-z][A-Za-z0-9+.\-]*://") +"""A URI-format rule's ``source_path`` must start with ``://``. + +Mirrors the scheme grammar of RFC 3986 §3.1, which is what the EXPR engine's +URI parser accepts. Validated in the constructor so a malformed rule is rejected +where the value can be named.""" + + def to_host_path_separators(value: str) -> str: """Render ``value`` with this host's path separators. @@ -53,7 +61,39 @@ def to_host_path_separators(value: str) -> str: the non-EXPR path. The URI test reuses :data:`_URI_SOURCE_RE`, which is this module's existing spelling of the same ``://`` rule, so the duplication is of Rust's three-way branch only. + + Note that a one-character scheme is a URI to both implementations, so + ``C://Users/foo`` keeps its forward slashes while ``C:/Users/foo`` does not. + That looks like a drive-letter misclassification and is deliberate: RFC 3986 + §3.1 admits a single-character scheme, ``openjd-rs``'s ``uri_path::is_uri`` + accepts one, and the Expression Language's stated pattern + (``^[a-zA-Z][a-zA-Z0-9+.-]*://``) does too. Measured against the engine, the + two agree on this input and on ``x://y/z``. Tightening the regex here would + make this package diverge from the oracle it is being aligned with, so if the + behaviour is wrong it is wrong in the specification. """ + # The three branches below are a transcription of openjd-expr's + # `normalize_path_separators`, in `crates/openjd-expr/src/value.rs`. Kept in + # the same order as the original so the two can be diffed by eye. Quoted here + # from openjd-expr 0.6.0; `openjd-model`'s `rust-bindings` links 0.5.0, and + # this function and the `uri_path::parse` behind its `is_uri` are byte + # identical in both (checked against the two crates.io sources, not assumed): + # + # pub fn normalize_path_separators(value: &str, format: PathFormat) -> String { + # if crate::uri_path::is_uri(value) { + # return value.to_string(); + # } + # match format { + # PathFormat::Windows => value.replace('/', "\\"), + # PathFormat::Posix | PathFormat::Uri => value.to_string(), + # } + # } + # + # `format` is always the host's here, so the `Uri` arm of that `match` is + # unreachable from this caller and is folded into the POSIX one. If you change + # anything below, check it against that function first -- the whole reason this + # is a copy rather than a call is in the docstring, and a silent divergence is + # the cost that buys. if _URI_SOURCE_RE.match(value) is not None: return value if os_name == "posix": @@ -61,14 +101,6 @@ def to_host_path_separators(value: str) -> str: return value.replace("/", "\\") -_URI_SOURCE_RE = re.compile(r"[A-Za-z][A-Za-z0-9+.\-]*://") -"""A URI-format rule's ``source_path`` must start with ``://``. - -Mirrors the scheme grammar of RFC 3986 §3.1, which is what the EXPR engine's -URI parser accepts. Validated in the constructor so a malformed rule is rejected -where the value can be named.""" - - class PathFormat(str, Enum): POSIX = "POSIX" WINDOWS = "WINDOWS" diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py index e98a3147..71bcba70 100644 --- a/src/openjd/sessions/_session.py +++ b/src/openjd/sessions/_session.py @@ -1677,8 +1677,18 @@ def processed_parameter_value(param: ParameterValue) -> Any: # submitter wrote it, and on a Windows host a POSIX-spelled # parameter stayed POSIX-spelled. openjd-rs wraps both the mapped # and the unmapped case in `ExprValue::new_path(.., host())`. - # Applying it after `apply_mapping` is idempotent for a value a rule - # did match, which is what keeps the two paths agreeing. + # + # For a POSIX- or WINDOWS-format rule this is idempotent, because + # `apply` rebuilds the result through `PureWindowsPath` and leaves no + # forward slash to replace. For a URI-format rule it is not: + # `_apply_uri` copies `destination_path` verbatim and uses the host + # separator only for the appended child parts, so a POSIX-spelled + # destination on a Windows host yields a mixed result + # ("/tmp/openjd\\scene\\out") that this then completes + # ("\\tmp\\openjd\\scene\\out"). That is the openjd-rs answer for the + # same input -- measured -- so completing it is the point rather than + # a side effect. `TestPathParameterFormatAndPathMappingAgree` pins + # both cases. if param.type == ParameterValueType.PATH: return to_host_path_separators(apply_mapping(param.value)) if param.type == ParameterValueType.LIST_PATH and isinstance(param.value, list): diff --git a/test/openjd/sessions_v0/test_path_parameter_host_format.py b/test/openjd/sessions_v0/test_path_parameter_host_format.py index f240c274..4a4321ca 100644 --- a/test/openjd/sessions_v0/test_path_parameter_host_format.py +++ b/test/openjd/sessions_v0/test_path_parameter_host_format.py @@ -29,12 +29,12 @@ is deliberate rather than a simplification: that module-level name is the single place this package decides which separator a host-scope path uses. It is what ``PathMappingRule.apply`` reads for the separator of a rule's output, and it is -what ``host_path_format`` reads for the format of an unmapped value. Patching one -and not the other would produce a self-inconsistent host -- mapped values -rendering Windows while unmapped ones rendered POSIX -- and a test built on that -would assert an arrangement that cannot occur in production. Keeping both readers -on one seam is the reason the fix put ``host_path_format`` in ``_path_mapping`` -rather than deriving ``os.name`` a second time in ``_session``. +what ``to_host_path_separators`` reads for the format of an unmapped value. +Patching one and not the other would produce a self-inconsistent host -- mapped +values rendering Windows while unmapped ones rendered POSIX -- and a test built on +that would assert an arrangement that cannot occur in production. Keeping both +readers on one seam is the reason the fix put ``to_host_path_separators`` in +``_path_mapping`` rather than deriving ``os.name`` a second time in ``_session``. """ from __future__ import annotations @@ -93,7 +93,11 @@ def _symtab( def _path_param(value: str) -> dict[str, ParameterValue]: - return {"InputFile": ParameterValue(type=ParameterValueType.PATH, value=value)} + return _path_param_named("InputFile", value) + + +def _path_param_named(name: str, value: str) -> dict[str, ParameterValue]: + return {name: ParameterValue(type=ParameterValueType.PATH, value=value)} # --------------------------------------------------------------------------- @@ -179,6 +183,45 @@ def test_a_matching_rule_is_unchanged_by_the_fix(self) -> None: # THEN the mapped value is what path mapping produced, unaltered assert str(symtab["Task.Param.InputFile"]) == r"C:\dest\a.exr" + def test_a_matching_uri_rule_is_completed_not_left_mixed(self) -> None: + """A URI-format rule is the one case where this is *not* a no-op, and + that is deliberate. + + ``PathMappingRule._apply_uri`` copies ``destination_path`` verbatim and + uses the host separator only for the appended child parts, so a + POSIX-spelled destination on a Windows host comes out of ``apply`` + half-converted -- ``/tmp/openjd\\scene\\out``. openjd-rs wraps the mapped + value in ``new_path(mapped, host())`` regardless of the rule's format, and + measured against the engine that yields the fully converted form. So + completing it here is agreement with the oracle, not a side effect. + + The `apply`-only value is asserted first, so if `_apply_uri` ever starts + emitting host separators for the whole path this test says which half + changed instead of just failing. + """ + # GIVEN a URI-format rule with a POSIX-spelled destination, which is how + # the existing suite spells cross-format rules + rules = [ + PathMappingRule( + source_path_format=PathFormat.URI, + source_path="s3://bucket/prefix", + destination_path=PurePosixPath("/tmp/openjd"), + ) + ] + given = "s3://bucket/prefix/scene/out" + + # WHEN the rule is applied on a Windows host, and again through the table + with _host("nt"): + matched, mapped_only = rules[0].apply(path=given) + symtab = _symtab(_path_param_named("InputFile", given), os_name="nt", rules=rules) + + # THEN path mapping alone leaves a mixed-separator result + assert matched is True + assert mapped_only == "/tmp/openjd\\scene\\out" + + # AND the symbol table completes it to the host's format + assert str(symtab["Task.Param.InputFile"]) == r"\tmp\openjd\scene\out" + class TestPathParameterFormatIsSeparatorsOnly: """Guards the *shape* of the conversion. Rendering through @@ -207,6 +250,35 @@ def test_windows_host_replaces_separators_only(self, given: str, expected: str) # THEN only the separators changed assert str(symtab["Task.Param.InputFile"]) == expected + @pytest.mark.parametrize( + "given,expected", + [ + pytest.param("C://Users/foo", "C://Users/foo", id="one-char scheme is a URI"), + pytest.param("C:/Users/foo", r"C:\Users\foo", id="drive letter is not a URI"), + pytest.param("x://y/z", "x://y/z", id="one-char scheme, non-drive letter"), + ], + ) + def test_a_one_character_scheme_matches_the_engine(self, given: str, expected: str) -> None: + """``C://Users/foo`` keeps its forward slashes and ``C:/Users/foo`` does + not. That reads like a drive-letter misclassification; it is what the + oracle does. + + RFC 3986 §3.1 admits a single-character scheme, the Expression Language's + stated pattern is ``^[a-zA-Z][a-zA-Z0-9+.-]*://``, and openjd-rs's + ``uri_path::is_uri`` accepts one -- measured through the engine, it renders + all three of these identically to the assertions below. So this is pinned + deliberately: tightening the regex to require two scheme characters would + make this package diverge from the implementation it is being aligned + with. If the behaviour is wrong, it is wrong in the specification, and + this test should change when the specification does. + """ + # GIVEN a value whose scheme is one character + # WHEN the table is built on a Windows host + symtab = _symtab(_path_param(given), os_name="nt") + + # THEN it is treated exactly as the engine treats it + assert str(symtab["Task.Param.InputFile"]) == expected + def test_a_uri_is_left_alone(self) -> None: """A ``path`` holding a URI keeps forward slashes on every host. The Expression Language spec makes URI paths exempt from the host's