fix(deps): Accept a stepEnvironment name reused across steps on the v1 path - #363
Conversation
ed27a76 to
cb5342a
Compare
…1 path Bump openjd-model to 0.7.1 and openjd-sessions to 0.5.8. openjd-rs#381 scopes stepEnvironment name uniqueness to its step, matching the Python implementation and Template Schemas section 3: a template in which two steps each declare a stepEnvironment named the same was rejected by the v1 decoder and now decodes. The per-step rule, the step-vs-job rule, and jobEnvironments uniqueness are unchanged. max_env_count now counts environments rather than distinct names, which only differ once names may repeat. openjd-sessions 0.5.8 is a release-train bump with no source change. Signed-off-by: wyongzhi <276409147+wyongzhi@users.noreply.github.com>
c15bdbc to
a93a3f4
Compare
| """Four Steps each declare ``StepEnv``. Only one Step's environments are ever | ||
| active in a Session, so these names never collide.""" | ||
| template = self._template([self._step(f"Step{i}", ["StepEnv"]) for i in range(4)]) | ||
| job_template = decode_job_template(template=template, supported_extensions=[]) |
There was a problem hiding this comment.
The relaxation is only exercised at decode_job_template. Nothing covers create_job on the same template, which is where a name collision would actually bite.
decode_job_template validates the JobTemplate/StepTemplate shapes. But the environment names that matter operationally are the ones on the instantiated Job — Step.step_environments (src/openjd/_openjd_rs.pyi:2226) and Job.job_environments (:1166), reached via create_job, and then handed to Session.enter_environment (rust-bindings/src/sessions/session.rs:443) which tracks them by name in environments_entered (:117, :407).
That instantiation path is a second place a name set could be held. openjd-rs#381 split one template-wide set into a job set plus a per-Step set on the decode side; if create_job (or preprocess_job_parameters, which py_create_job routes through at rust-bindings/src/model/create_job_fns.rs:106) holds its own collection keyed by environment name, decode_job_template would now accept the four-StepEnv template while create_job on that same template still fails — or worse, silently collapses four distinct Environment objects into one. test_same_name_across_steps_is_accepted returns before ever finding out; it reads job_template.steps, not a created Job.
test/openjd/model_v1/test_create_job.py has no stepEnvironments at all (grep is empty across all 1583 lines), so there is no existing case that would catch this incidentally. Extending test_same_name_across_steps_is_accepted to feed its decoded template through create_job and assert the four Steps still each carry their own StepEnv would close the gap cheaply — it reuses the template already built by _template, and it is the assertion that actually demonstrates the reused name is usable rather than merely parseable.
There was a problem hiding this comment.
Checked the instantiation path in openjd-rs 0.7.1 for the hypothesized name-keyed collection; there is none. create_job handles step environments positionally: instantiate.rs maps each step's stepEnvironments Vec element-by-element through convert_environment (no name lookup), and the only pass over all environments in create_job/mod.rs is the max_environment_size check, where env.name appears solely in the error message. The only HashSet in that module collects accessed symbol names, not environment names. On the session side, environments_entered is name-keyed but a session only ever enters a single step's environments, so cross-step reuse cannot coexist there — which is the spec rationale for allowing it. Leaving this PR scoped to the decode-side relaxation it ships; a create_job-level pin would be guarding a structure that does not exist today.
What changed
Bumps two
openjd-*Rust crate pins inrust-bindings/Cargo.toml, refreshesCargo.lock, and regeneratesTHIRD-PARTY-LICENSES.txt(scripts/check_third_party_licenses.sh --update):openjd-modelopenjd-sessionsopenjd-exprstays at 0.7.0.cargo update -p openjd-model -p openjd-sessionsmoved only these two packages; no new transitive dependency.Upstream changes
openjd-rs#381 (
openjd-model0.7.1) — stepEnvironment name uniqueness is scoped to its step. The 2023-09 spec requires environment names to be unique withinjobEnvironments, within each step'sstepEnvironments, and requires a stepEnvironment name to differ from every jobEnvironment name. It does not forbid two different steps from reusing a name, and the v0 (Python) path has always accepted that. The v1 (Rust) path held every environment name in one set and rejected the second step to declare the same name. The same fix changedmax_env_countto count environments rather than distinct names; the two were equal only while duplicates were always rejected. The pin brings both; no binding code change was needed (patch release, no API change).openjd-sessions0.5.7 → 0.5.8 carries no source change to the crate (release-train version bump only).Tests
TestStepEnvironmentNameScopeintest/openjd/model_v1/test_parse.pypins the behaviour on the v1 path:StepEnvdecode successfullyjobEnvironments(the rule most exposed by splitting the single set, previously untested on v1)max_env_count: 4 xStepEnv+JobEnvis 5 environments under 2 names; a limit of 4 rejects withtotal environments (5) exceeds caller limit of 4The class docstring records that the asserted v1 error text for the step-vs-job rule differs from v0 (
Name X must differ from ...atstep[i] -> stepEnvironments[j] -> name); aligning them is an openjd-rs concern, not introduced here.Mutation-checked by rebuilding the extension against the previous pins (
openjd-model0.7.0): 2 failed, 3 passed — the positive case fails withduplicate environment name: 'StepEnv', themax_env_countcase fails because 0.7.0 never reaches the count, and all three controls pass. Rebuilt on 0.7.1: 5 passed.Full run:
hatch run test5969 passed, 24 skipped, 3 xfailed, coverage 94.25%;hatch run lintandhatch run typingclean.Changelog
Subject is
fix(deps):rather thanchore(deps):on purpose.choreis inpatch_tagsbut.semantic_release/CHANGELOG.md.j2renders nochoregroup, so a chore-typed bump ships with no release note (as #349 did). This bump changes which templates decode, so it should appear in the notes.Breaking change / security
No public interface changes. No security impact.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.