diff --git a/src/openjd/model/_merge_job_parameter.py b/src/openjd/model/_merge_job_parameter.py index 619bb15..04a7c50 100644 --- a/src/openjd/model/_merge_job_parameter.py +++ b/src/openjd/model/_merge_job_parameter.py @@ -2,7 +2,7 @@ from collections import defaultdict from decimal import Decimal -from typing import Any, NamedTuple, Optional, Union, cast +from typing import Any, Iterable, NamedTuple, Optional, Union, cast from ._errors import CompatibilityError from ._parse import parse_model @@ -117,12 +117,21 @@ def merge_job_parameter_definitions( SourcedParamDefinition(source="JobTemplate", definition=param) ) + # The merge re-validates each merged definition, so it must run under the same + # extension set decode ran under or it rejects what decode accepted -- e.g. a + # FEATURE_BUNDLE_1 parameter name between 65 and 512 characters. + supported_extensions = _declared_extensions(job_template, environment_templates) + errors = list[str]() return_value = list[JobParameterDefinition]() for name, source in collected_definitions.items(): try: - return_value.append(merge_job_parameter_definitions_for_one(source)) + return_value.append( + merge_job_parameter_definitions_for_one( + source, supported_extensions=supported_extensions + ) + ) except CompatibilityError as e: compat_errors = "\n\t".join(str(e).split("\n")) errors.append( @@ -134,13 +143,38 @@ def merge_job_parameter_definitions( return return_value +def _declared_extensions( + job_template: Optional[JobTemplate], + environment_templates: Optional[list[EnvironmentTemplate]], +) -> list[str]: + """The union of the extensions declared by all of the given templates. + + A definition was decoded under its own template's extension set, and every + definition of one job parameter carries the same name, so the union cannot admit a + value that decode did not already accept for the template that declared it. + """ + extensions: set[str] = set() + for template in (*(environment_templates or []), job_template): + if template is not None and template.extensions: + extensions.update(template.extensions) + return sorted(extensions) + + def merge_job_parameter_definitions_for_one( params: list[SourcedParamDefinition], + *, + supported_extensions: Optional[Iterable[str]] = None, ) -> JobParameterDefinition: """Given an ordered list of job parameter definitions of the *same* job parameter, this merges the definitions into a single job parameter definition. In the act of doing the merger, this performs checks to ensure that the job parameter definitions are compatible with one another. + Args: + params: The definitions of one job parameter, in merge order. + supported_extensions (optional): The extensions declared by the templates the definitions + came from. The merged definition is re-validated under this set, so omitting it applies + the base limits to a definition decode may have accepted under an extension. + Returns (JobParameterDefinition): The result of merging all of the given definitions in to a single definition. @@ -219,13 +253,16 @@ def merge_job_parameter_definitions_for_one( if errors: raise CompatibilityError("\n".join(errors)) - return parse_model(model=params[0].definition.__class__, obj=merged_properties) + return parse_model( + model=params[0].definition.__class__, + obj=merged_properties, + supported_extensions=supported_extensions, + ) # EXPR-extension types (BOOL, RANGE_EXPR, LIST[*]): not cross-merged. Return # the last-defined definition with the last-defined default applied. - # ``model_copy`` avoids re-validation, which would otherwise re-trigger the - # EXPR extension gate (the merge has no parsing context to satisfy it). But - # because ``model_copy`` skips validators, re-validate the merged default + # ``model_copy`` avoids re-validation, so this path does not re-trigger the EXPR + # extension gate at all. But because ``model_copy`` skips validators, re-validate the merged default # against the base definition's own constraints explicitly so a default # carried over from another source that violates them is still rejected # (mirrors the ``parse_model`` re-validation the legacy path performs). diff --git a/test/openjd/model_v0/v2023_09/test_feature_bundle_1.py b/test/openjd/model_v0/v2023_09/test_feature_bundle_1.py index 22d07f6..40dd6ad 100644 --- a/test/openjd/model_v0/v2023_09/test_feature_bundle_1.py +++ b/test/openjd/model_v0/v2023_09/test_feature_bundle_1.py @@ -1,11 +1,23 @@ # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +from pathlib import Path +from typing import Optional + import pytest from pydantic import TypeAdapter, ValidationError -from openjd.model import create_job, decode_job_template +from openjd.model import ( + create_job, + decode_environment_template, + decode_job_template, + preprocess_job_parameters, +) from openjd.model._errors import DecodeValidationError +from openjd.model._merge_job_parameter import ( + SourcedParamDefinition, + merge_job_parameter_definitions_for_one, +) from openjd.model._format_strings import FormatString from openjd.model._parse import _parse_model from openjd.model._types import ParameterValue, ParameterValueType @@ -1384,6 +1396,10 @@ class TestCreateJobPreservesFeatureBundle1Lengths: create_job simultaneously. G. Resolved job name (matrix): the resolved job name is checked against the extension-aware 128/512 limit after format-string substitution. + H. Job parameter name: a 512-char parameter name survives the job-parameter + merge inside preprocess_job_parameters, which create_job also reaches, for + every type the merge re-validates and for both of its branches. Includes + the base-limit and static-ceiling controls. """ @staticmethod @@ -1680,3 +1696,215 @@ def test_resolved_job_name_128_chars_no_extension_create_job_accepted(self) -> N def test_resolved_job_name_129_chars_no_extension_create_job_rejected(self) -> None: with pytest.raises(DecodeValidationError, match="128"): self._create_with_resolved_job_name(False, 129) + + # ---- Group H: job parameter name through the merge ---- + # + # Groups A-G build templates with no parameterDefinitions, so none of them reaches + # the job-parameter merge inside preprocess_job_parameters. That merge re-validates + # each merged definition through parse_model, and it ran with an empty extension + # set, enforcing the base 64-character limit on a name decode had accepted at 512. + # create_job calls preprocess_job_parameters itself, so both were affected. + + #: The four scalar types whose merged definition is re-validated through + #: ``parse_model``. The EXPR-extension types take the ``model_copy`` branch instead + #: and are covered separately at the end of the group. + _LEGACY_TYPE_DEFAULTS = { + "STRING": "v", + "PATH": "/tmp/v", + "INT": "1", + "FLOAT": "1.5", + } + + def _template_with_param( + self, + param_name: str, + *, + param_type: str = "STRING", + default: object = "v", + declare_fb1: bool = True, + ) -> dict: + """A minimal template carrying one job parameter definition.""" + template = self._template(declare_fb1=declare_fb1) + template["parameterDefinitions"] = [ + {"name": param_name, "type": param_type, "default": default} + ] + return template + + @staticmethod + def _preprocess( + template: dict, + supported: list, + environment_templates: Optional[list[EnvironmentTemplate]] = None, + ) -> dict: + """Decode the template and run it through preprocess_job_parameters. + + Paths do not matter to a name-length check, so this uses the walk-up form + rather than real directories. + """ + job_template = decode_job_template(template=template, supported_extensions=supported) + return preprocess_job_parameters( + job_template=job_template, + job_parameter_values={}, + job_template_dir=Path(), + current_working_dir=Path(), + allow_job_template_dir_walk_up=True, + environment_templates=environment_templates, + ) + + @pytest.mark.parametrize("param_type", sorted(_LEGACY_TYPE_DEFAULTS)) + def test_parameter_name_512_chars_fb1_preprocess_preserved(self, param_type: str) -> None: + """A 512-char parameter name accepted at FEATURE_BUNDLE_1 decode must survive the + merge inside preprocess_job_parameters, for every type the merge re-validates.""" + name = "a" * 512 + template = self._template_with_param( + name, param_type=param_type, default=self._LEGACY_TYPE_DEFAULTS[param_type] + ) + assert name in self._preprocess(template, _FB1_SUPPORTED) + + @pytest.mark.parametrize("param_type", sorted(_LEGACY_TYPE_DEFAULTS)) + def test_parameter_name_512_chars_fb1_create_job_preserved(self, param_type: str) -> None: + """The same name must survive create_job, which reaches the merge through its own + preprocess_job_parameters call.""" + name = "a" * 512 + job = self._create( + self._template_with_param( + name, param_type=param_type, default=self._LEGACY_TYPE_DEFAULTS[param_type] + ) + ) + assert name in job.parameters + + def test_parameter_name_512_chars_merged_from_two_sources_preserved(self) -> None: + """The merge's actual job is combining definitions from more than one source. A + 512-char name must survive when constraints are genuinely merged, not only when + a single definition is passed through.""" + name = "a" * 512 + env_template = decode_environment_template( + template={ + "specificationVersion": "environment-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "parameterDefinitions": [ + {"name": name, "type": "STRING", "minLength": 1, "default": "v"} + ], + "environment": { + "name": "Env", + "script": {"actions": {"onEnter": {"command": "echo enter"}}}, + }, + }, + supported_extensions=_FB1_SUPPORTED, + ) + template = self._template_with_param(name) + template["parameterDefinitions"][0]["maxLength"] = 8 + values = self._preprocess(template, _FB1_SUPPORTED, environment_templates=[env_template]) + assert name in values + + def test_parameter_name_512_chars_from_environment_template_preserved(self) -> None: + """A 512-char name declared by an environment template that enables + FEATURE_BUNDLE_1 survives even though the job template does not enable it, + because the merge honours every contributing template's extensions.""" + name = "a" * 512 + env_template = decode_environment_template( + template={ + "specificationVersion": "environment-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "parameterDefinitions": [{"name": name, "type": "STRING", "default": "v"}], + "environment": { + "name": "Env", + "script": {"actions": {"onEnter": {"command": "echo enter"}}}, + }, + }, + supported_extensions=_FB1_SUPPORTED, + ) + values = self._preprocess( + self._template(declare_fb1=False), [], environment_templates=[env_template] + ) + assert name in values + + def test_parameter_name_513_chars_rejected_by_static_ceiling(self) -> None: + """512 is the hard ceiling for an identifier regardless of extension: at 513 the + ``Identifier`` string constraint rejects before the extension-aware validator is + consulted, so this pins the type ceiling rather than the FEATURE_BUNDLE_1 branch. + """ + with pytest.raises( + DecodeValidationError, match=r"name:\n\tString should have at most 512 characters" + ): + decode_job_template( + template=self._template_with_param("a" * 513), + supported_extensions=_FB1_SUPPORTED, + ) + + def test_parameter_name_65_chars_no_extension_decode_rejected(self) -> None: + """Without the extension the base 64-character limit applies, and it is reached at + decode -- before the merge ever runs.""" + with pytest.raises( + DecodeValidationError, match=r"name:\n\tname must be at most 64 characters long" + ): + decode_job_template( + template=self._template_with_param("a" * 65, declare_fb1=False), + supported_extensions=[], + ) + + def test_parameter_name_64_chars_no_extension_create_job_preserved(self) -> None: + """A parameter name at the base ceiling survives create_job with no extension.""" + name = "a" * 64 + job_template = decode_job_template( + template=self._template_with_param(name, declare_fb1=False), + supported_extensions=[], + ) + job = create_job(job_template=job_template, job_parameter_values={}) + assert name in job.parameters + + def test_merge_without_extensions_enforces_base_identifier_limit(self) -> None: + """The merge must apply the base limit when no extension is in play, rather than + skipping the length check altogether. + + Decode rejects an over-length name before the merge is reachable through a + template, so this drives the merge directly with a definition decode accepted + under FEATURE_BUNDLE_1 and no extensions supplied to the merge. + """ + job_template = decode_job_template( + template=self._template_with_param("a" * 512), + supported_extensions=_FB1_SUPPORTED, + ) + sourced = [ + SourcedParamDefinition( + source="JobTemplate", definition=job_template.parameterDefinitions[0] + ) + ] + with pytest.raises(DecodeValidationError, match="name must be at most 64 characters long"): + merge_job_parameter_definitions_for_one(sourced) + + # And it accepts the same definition when the extension is supplied. + merged = merge_job_parameter_definitions_for_one( + sourced, supported_extensions=["FEATURE_BUNDLE_1"] + ) + assert len(merged.name) == 512 + + def test_all_five_fields_at_ceiling_fb1_create_job_preserved(self) -> None: + """Group F plus the parameter name: every FEATURE_BUNDLE_1-lengthened field + survives create_job at once.""" + param_name = "a" * 512 + template = self._template( + job_name="a" * 512, + env_name="a" * 512, + ef_name="a" * 512, + ef_filename="a" * 256, + ) + template["parameterDefinitions"] = [{"name": param_name, "type": "STRING", "default": "v"}] + job = self._create(template) + assert ( + len(job.name), + len(job.jobEnvironments[0].name), + len(job.steps[0].script.embeddedFiles[0].name), + len(job.steps[0].script.embeddedFiles[0].filename), + len(next(iter(job.parameters))), + ) == (512, 512, 512, 256, 512) + + def test_parameter_name_512_chars_expr_typed_parameter_preserved(self) -> None: + """The EXPR-extension parameter types take the merge's other branch, which copies + rather than re-validates. A 512-char name must survive there too, so that + unifying the two branches cannot silently reintroduce the base limit.""" + name = "a" * 512 + template = self._template_with_param(name, param_type="LIST[STRING]", default=["v"]) + template["extensions"] = ["FEATURE_BUNDLE_1", "EXPR"] + values = self._preprocess(template, [ExtensionName.FEATURE_BUNDLE_1, ExtensionName.EXPR]) + assert name in values diff --git a/test/openjd/model_v1/test_create_job.py b/test/openjd/model_v1/test_create_job.py index 83c567e..a6ba7aa 100644 --- a/test/openjd/model_v1/test_create_job.py +++ b/test/openjd/model_v1/test_create_job.py @@ -10,6 +10,7 @@ create_job, decode_environment_template, decode_job_template, + merge_job_parameter_definitions, preprocess_job_parameters, ) from openjd.model._v1.types import ( @@ -18,6 +19,7 @@ ) from openjd.model._v1.errors import ( DecodeValidationError, + ModelValidationError, ) @@ -1716,3 +1718,263 @@ def test_a_null_skipped_element_leaves_one_value(self) -> None: assert host_requirements is not None assert host_requirements.attributes is not None assert host_requirements.attributes[0].all_of == ["linux"] + + +class TestCreateJobPreservesFeatureBundle1Lengths: + """The FEATURE_BUNDLE_1 raised length ceilings survive job creation on the v1 lane. + + FEATURE_BUNDLE_1 raises four string-length ceilings: job name and environment name + to 512, embedded-file filename to 256, and the section 7.1 identifier -- an + embedded-file name or a job parameter name -- to 512. A value template validation + accepted under the extension must survive preprocess_job_parameters and create_job. + + The v0 lane lost the extension set between decode and job creation and rejected such + values, first when create_job reconstructed the target models and separately in the + job-parameter merge. v1 validates in a single pass in Rust and never re-validates a + constructed model, so it is expected to pass throughout; these tests pin that so a + re-validation pass cannot be added to the Rust implementation unnoticed. + + Groups: + A. Each of the four ceilings survives create_job. + B. The job parameter name survives the merge, preprocess_job_parameters and + create_job, for every scalar parameter type. + C. Controls: 513 and 65 are rejected, and the base ceiling is preserved. + D. Round trip: every lengthened field at once. + """ + + _FB1 = ["FEATURE_BUNDLE_1"] + + #: Defaults per scalar parameter type, so one template shape covers all four. + _TYPE_DEFAULTS = {"STRING": "v", "PATH": "/tmp/v", "INT": "1", "FLOAT": "1.5"} + + @classmethod + def _template( + cls, + *, + job_name: str = "J", + env_name: str = "Env1", + ef_name: str = "Run", + ef_filename: str = "run.sh", + param_name: str = "P", + param_type: str = "STRING", + declare_fb1: bool = True, + ) -> dict: + """A minimal template with one step, one embedded file, one job environment and + one job parameter, so all four ceilings are reachable from one shape.""" + template: dict = { + "specificationVersion": "jobtemplate-2023-09", + "name": job_name, + "parameterDefinitions": [ + { + "name": param_name, + "type": param_type, + "default": cls._TYPE_DEFAULTS[param_type], + } + ], + "steps": [ + { + "name": "S", + "script": { + "actions": {"onRun": {"command": "echo"}}, + "embeddedFiles": [ + { + "name": ef_name, + "type": "TEXT", + "data": "echo hi", + "filename": ef_filename, + } + ], + }, + } + ], + "jobEnvironments": [ + { + "name": env_name, + "script": {"actions": {"onEnter": {"command": "echo enter"}}}, + } + ], + } + if declare_fb1: + template["extensions"] = ["FEATURE_BUNDLE_1"] + return template + + @classmethod + def _preprocess(cls, template: dict, supported: list, env_templates: Any = None) -> dict: + """Decode the template and run it through preprocess_job_parameters. + + Paths do not matter to a name-length check, so this uses the walk-up form rather + than real directories. + """ + job_template = decode_job_template(template=template, supported_extensions=supported) + return preprocess_job_parameters( + job_template=job_template, + job_parameter_values={}, + job_template_dir=Path(), + current_working_dir=Path(), + allow_job_template_dir_walk_up=True, + environment_templates=env_templates, + ) + + @classmethod + def _create(cls, template: dict) -> Any: + """Decode with FEATURE_BUNDLE_1 supported, apply defaults, then create the job.""" + job_template = decode_job_template(template=template, supported_extensions=cls._FB1) + values = preprocess_job_parameters( + job_template=job_template, + job_parameter_values={}, + job_template_dir=Path(), + current_working_dir=Path(), + allow_job_template_dir_walk_up=True, + ) + return create_job(job_template=job_template, job_parameter_values=values) + + # ---- Group A: each ceiling survives create_job ---- + + def test_job_name_512_chars_fb1_create_job_preserved(self) -> None: + """A 512-char job name accepted at FEATURE_BUNDLE_1 decode survives create_job.""" + job = self._create(self._template(job_name="a" * 512)) + assert len(job.name) == 512 + + def test_environment_name_512_chars_fb1_create_job_preserved(self) -> None: + """A 512-char job-environment name survives create_job.""" + job = self._create(self._template(env_name="a" * 512)) + environments = job.jobEnvironments + assert environments is not None + assert len(environments[0].name) == 512 + + def test_embedded_file_name_512_chars_fb1_create_job_preserved(self) -> None: + """A 512-char embedded-file name survives create_job.""" + job = self._create(self._template(ef_name="a" * 512)) + embedded = job.steps[0].script.embeddedFiles + assert embedded is not None + assert len(embedded[0].name) == 512 + + def test_embedded_file_filename_256_chars_fb1_create_job_preserved(self) -> None: + """A 256-char embedded-file filename survives create_job.""" + job = self._create(self._template(ef_filename="a" * 256)) + embedded = job.steps[0].script.embeddedFiles + assert embedded is not None + assert len(embedded[0].filename) == 256 + + # ---- Group B: the job parameter name, through every stage ---- + + @pytest.mark.parametrize("param_type", sorted(_TYPE_DEFAULTS)) + def test_parameter_name_512_chars_fb1_merge_preserved(self, param_type: str) -> None: + """The job-parameter merge preserves a 512-char name. This is the stage the v0 + lane re-validated without the extension set.""" + name = "a" * 512 + job_template = decode_job_template( + template=self._template(param_name=name, param_type=param_type), + supported_extensions=self._FB1, + ) + merged = merge_job_parameter_definitions(job_template=job_template) + assert [d["name"] for d in merged] == [name] + + @pytest.mark.parametrize("param_type", sorted(_TYPE_DEFAULTS)) + def test_parameter_name_512_chars_fb1_preprocess_preserved(self, param_type: str) -> None: + """preprocess_job_parameters preserves a 512-char parameter name.""" + name = "a" * 512 + values = self._preprocess(self._template(param_name=name, param_type=param_type), self._FB1) + assert name in values + + @pytest.mark.parametrize("param_type", sorted(_TYPE_DEFAULTS)) + def test_parameter_name_512_chars_fb1_create_job_preserved(self, param_type: str) -> None: + """create_job preserves a 512-char parameter name.""" + name = "a" * 512 + job = self._create(self._template(param_name=name, param_type=param_type)) + assert name in job.parameters + + def test_parameter_name_512_chars_from_environment_template_preserved(self) -> None: + """A 512-char name declared by an environment template that enables + FEATURE_BUNDLE_1 survives even though the job template does not enable it.""" + name = "a" * 512 + env_template = decode_environment_template( + template={ + "specificationVersion": "environment-2023-09", + "extensions": ["FEATURE_BUNDLE_1"], + "parameterDefinitions": [{"name": name, "type": "STRING", "default": "v"}], + "environment": minimal_environment_2023_09, + }, + supported_extensions=self._FB1, + ) + plain_template = { + "specificationVersion": "jobtemplate-2023-09", + "name": "J", + "steps": minimal_steps_v2023_09, + } + values = self._preprocess(plain_template, [], env_templates=[env_template]) + assert name in values + + # ---- Group C: controls ---- + + def test_parameter_name_513_chars_rejected_by_static_ceiling(self) -> None: + """512 is the hard identifier ceiling regardless of extension: at 513 the + identifier length check rejects before any extension-aware limit applies.""" + with pytest.raises(DecodeValidationError) as excinfo: + decode_job_template( + template=self._template(param_name="a" * 513), + supported_extensions=self._FB1, + ) + assert "Identifier length must be 1..=512, got 513" in str(excinfo.value) + + def test_parameter_name_65_chars_no_extension_decode_rejected(self) -> None: + """Without the extension the base 64-character limit applies, and it is reached at + decode -- before the merge ever runs.""" + with pytest.raises(ModelValidationError) as excinfo: + decode_job_template( + template=self._template(param_name="a" * 65, declare_fb1=False), + supported_extensions=[], + ) + assert "parameterDefinitions[0]:\n\tname exceeds 64 characters." in str(excinfo.value) + + def test_job_name_129_chars_no_extension_decode_rejected(self) -> None: + """Without the extension the base 128-character job-name limit applies.""" + with pytest.raises(ModelValidationError) as excinfo: + decode_job_template( + template=self._template(job_name="a" * 129, declare_fb1=False), + supported_extensions=[], + ) + assert "exceeds 128 characters" in str(excinfo.value) + + def test_parameter_name_64_chars_no_extension_create_job_preserved(self) -> None: + """A parameter name at the base ceiling survives create_job with no extension.""" + name = "a" * 64 + job_template = decode_job_template( + template=self._template(param_name=name, declare_fb1=False), + supported_extensions=[], + ) + values = preprocess_job_parameters( + job_template=job_template, + job_parameter_values={}, + job_template_dir=Path(), + current_working_dir=Path(), + allow_job_template_dir_walk_up=True, + ) + job = create_job(job_template=job_template, job_parameter_values=values) + assert name in job.parameters + + # ---- Group D: round trip ---- + + def test_all_five_fields_at_ceiling_fb1_create_job_preserved(self) -> None: + """Every FEATURE_BUNDLE_1-lengthened field survives create_job at once.""" + param_name = "a" * 512 + job = self._create( + self._template( + job_name="a" * 512, + env_name="a" * 512, + ef_name="a" * 512, + ef_filename="a" * 256, + param_name=param_name, + ) + ) + environments = job.jobEnvironments + embedded = job.steps[0].script.embeddedFiles + assert environments is not None + assert embedded is not None + assert ( + len(job.name), + len(environments[0].name), + len(embedded[0].name), + len(embedded[0].filename), + param_name in job.parameters, + ) == (512, 512, 512, 256, True)