From 9d552b19633bd46110d84c4bd4df7e0d60483d70 Mon Sep 17 00:00:00 2001 From: Yongzhi Wei <276409147+wyongzhi@users.noreply.github.com> Date: Thu, 3 Sep 2026 13:27:43 -0700 Subject: [PATCH 1/4] =?UTF-8?q?fix:=20coerce=20LIST[BOOL]=20items=20per=20?= =?UTF-8?q?RFC=200007=20=C2=A72.15?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each LIST[BOOL] parameter item independently accepts the same values as scalar BOOL (bool literals, int/float 0/1, case-insensitive strings true/yes/on/1 and false/no/off/0). Python previously stored values verbatim, so heterogeneous lists failed downstream expression evaluation with 'List contains incompatible types', and homogeneous non-bool spellings silently produced wrongly-typed lists. Coerce each item to a canonical bool at the job-creation boundary: template defaults in _collect_defaults_2023_09 and submitted values (native list and JSON string) in _coerce_expr_param_value, gated exactly on LIST_BOOL. Invalid items keep 'Parameter :' context. The boundary also covers environment-template merged defaults, which bypass model validators via model_copy. Behavior change: created-Job LIST[BOOL] values are now canonical booleans; previously-working homogeneous string lists (e.g. ["yes","no"]) now store and interpolate as true/false, matching the Rust reference implementation. LIST[FLOAT] integer items are intentionally left as-is (no spec-enumerated alternate spellings; the expression engine promotes int to float). Signed-off-by: Yongzhi Wei <276409147+wyongzhi@users.noreply.github.com> --- src/openjd/model/_create_job.py | 70 +++++- .../model_v0/test_expr_param_coercion.py | 216 +++++++++++++++++- .../v2023_09/test_create_job_expr_params.py | 62 +++++ 3 files changed, 336 insertions(+), 12 deletions(-) diff --git a/src/openjd/model/_create_job.py b/src/openjd/model/_create_job.py index ede9d53d..671f8be7 100644 --- a/src/openjd/model/_create_job.py +++ b/src/openjd/model/_create_job.py @@ -68,12 +68,16 @@ def _coerce_expr_param_value(param_type_name: str, value: Any) -> Any: native form, mirroring openjd-rs's ``coerce_from_str`` (job/create_job/parameters.rs): BOOL accepts the spec's boolean strings, and LIST[*] values may be supplied as JSON — the public input type is - ``dict[str, str]``, so string forms must be accepted. Native values - (bool, list) pass through unchanged. + ``dict[str, str]``, so string forms must be accepted. LIST[BOOL] values + are additionally normalized per item (RFC 0007 §2.15): each item accepts + the same values as a scalar BOOL parameter, whether the value arrives as + a native list or as a JSON string. Other native values (bool, list) pass + through unchanged. Raises: - ValueError: If a string value cannot be coerced (message shapes match - the Rust implementation). + ValueError: If a string value cannot be coerced, or a LIST[BOOL] item + is not a valid boolean (message shapes match the Rust + implementation). """ if param_type_name == "BOOL" and isinstance(value, str): lowered = value.lower() @@ -91,7 +95,25 @@ def _coerce_expr_param_value(param_type_name: str, value: Any) -> Any: raise ValueError(f"Value '{value}' is not valid JSON for a list parameter.") if not isinstance(parsed, list): raise ValueError(f"Value '{value}' is not valid JSON for a list parameter.") + if param_type_name == "LIST_BOOL": + # RFC 0007 §2.15: each LIST[BOOL] item accepts the same values as a + # scalar BOOL parameter (JobBoolParameterDefinition). Normalize the + # freshly parsed items; other LIST[*] types pass the parsed list + # through unchanged. + # Inline import matches the file's deferred v2023_09 import pattern + # (avoids a module-level dependency on the version package). + from .v2023_09._model import _coerce_bool_value + + return [_coerce_bool_value(item) for item in parsed] return parsed + if param_type_name == "LIST_BOOL" and isinstance(value, list): + # RFC 0007 §2.15: a LIST[BOOL] value submitted as a native list is + # normalized per item, same as its JSON-string form above. Build a new + # list — never mutate the caller's input. + # Inline import matches the file's deferred v2023_09 import pattern. + from .v2023_09._model import _coerce_bool_value + + return [_coerce_bool_value(item) for item in value] return value @@ -208,8 +230,23 @@ def _collect_defaults_2023_09( # default through so the typed symbol-table builder can # coerce it. The PATH-relative-default handling below only # applies to the scalar PATH type. + default_value: Any = param.default + if param.type.name == "LIST_BOOL" and isinstance(param.default, list): + # RFC 0007 §2.15: each LIST[BOOL] item accepts the same + # values as a scalar BOOL parameter. Normalize the + # template default per item (build a new list — never + # mutate param.default), matching the submitted-value + # path so mixed spellings store as canonical booleans. + # Inline import matches the file's deferred v2023_09 + # import pattern. + from .v2023_09._model import _coerce_bool_value + + # Defaults are pre-validated at decode time by + # _check_item (and re-validated on any merge via + # _check_constraints), so this coercion cannot fail. + default_value = [_coerce_bool_value(item) for item in param.default] return_value[param.name] = ParameterValue( - type=ParameterValueType(param.type), value=param.default + type=ParameterValueType(param.type), value=default_value ) continue default = str(param.default) @@ -233,7 +270,18 @@ def _collect_defaults_2023_09( # their native values, then carry through; mirrors # openjd-rs's coerce_from_str. # Raises ValueError (collected by the caller) on bad input. - value = _coerce_expr_param_value(param.type.name, value) + try: + value = _coerce_expr_param_value(param.type.name, value) + except ValueError as exc: + if param.type.name == "LIST_BOOL": + # RFC 0007 §2.15: per-item coercion runs here during + # value collection, before _check_2023_09/_check_constraints, + # so the error would surface name-free unless named at this + # call site. + # Other EXPR errors keep their verbatim (name-free) + # message, as the scalar BOOL branch does. + raise ValueError(f"Parameter {param.name}: {exc}") from exc + raise return_value[param.name] = ParameterValue( type=ParameterValueType(param.type), value=value ) @@ -263,11 +311,11 @@ def _check_2023_09( for param in job_parameter_definitions: if param.name in job_parameter_values: param_value = job_parameter_values[param.name] - # The EXPR-extension LIST[*]/RANGE_EXPR definitions don't implement - # _check_constraints (BOOL and the original scalars do). Their - # template defaults are validated at decode time, and their values - # are type-checked when coerced into the typed EXPR symbol table, so - # skip the create-time constraint check when it isn't available. + # Every 2023_09 job-parameter definition now implements + # _check_constraints: the scalars (STRING/PATH/INT/FLOAT/BOOL), the + # LIST[*] types via _JobListParameterDefinitionBase, and RANGE_EXPR. + # The getattr fallback is retained as defense in case a definition + # type without one is ever added; it currently matches none. check_constraints = getattr(param, "_check_constraints", None) if check_constraints is None: continue diff --git a/test/openjd/model_v0/test_expr_param_coercion.py b/test/openjd/model_v0/test_expr_param_coercion.py index 99fc6cbc..b4b38c0d 100644 --- a/test/openjd/model_v0/test_expr_param_coercion.py +++ b/test/openjd/model_v0/test_expr_param_coercion.py @@ -10,7 +10,12 @@ import pytest -from openjd.model import create_job, decode_job_template, preprocess_job_parameters +from openjd.model import ( + create_job, + decode_environment_template, + decode_job_template, + preprocess_job_parameters, +) _TEMPLATE = { "specificationVersion": "jobtemplate-2023-09", @@ -97,3 +102,212 @@ def test_invalid_bool_rejected_with_rust_message(self, template, tmp_path): def test_invalid_list_json_rejected_with_rust_message(self, template, tmp_path, bad): with pytest.raises(ValueError, match=r"not valid JSON for a list parameter"): _preprocess(template, {"Flag": "true", "Values": bad, "Nested": [[1]]}, tmp_path) + + +_LIST_BOOL_TEMPLATE = { + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["EXPR"], + "name": "T", + "parameterDefinitions": [{"name": "Flags", "type": "LIST[BOOL]"}], + "steps": [ + { + "name": "S", + "script": {"actions": {"onRun": {"command": "echo", "args": ["{{ Param.Flags[0] }}"]}}}, + } + ], +} + + +@pytest.fixture +def list_bool_template(): + return decode_job_template(template=_LIST_BOOL_TEMPLATE, supported_extensions=["EXPR"]) + + +class TestListBoolValueCoercion: + """RFC 0007 §2.15: each LIST[BOOL] item accepts the same spellings as scalar + BOOL and is coerced per item. Heterogeneous native lists and JSON-string forms both + normalize to a list[bool]; an unrecognized item is rejected with the + offending parameter named. Previously items passed through unchanged and a + heterogeneous list only failed later with an opaque Rust type error. + """ + + def test_native_list_coerced_per_item(self, list_bool_template, tmp_path) -> None: + pv = _preprocess(list_bool_template, {"Flags": ["yes", 0, True]}, tmp_path) + assert pv["Flags"].value == [True, False, True] + # equality alone passes for ints ([1,0,1] == [True,False,True]). + assert all(type(x) is bool for x in pv["Flags"].value) + + def test_json_string_list_coerced_per_item(self, list_bool_template, tmp_path) -> None: + pv = _preprocess(list_bool_template, {"Flags": '[true, "off", 1]'}, tmp_path) + assert pv["Flags"].value == [True, False, True] + # equality alone passes for ints ([1,0,1] == [True,False,True]). + assert all(type(x) is bool for x in pv["Flags"].value) + + def test_invalid_item_rejected_with_parameter_name(self, list_bool_template, tmp_path) -> None: + with pytest.raises(ValueError, match=r"Parameter Flags"): + _preprocess(list_bool_template, {"Flags": ["maybe"]}, tmp_path) + + def test_json_string_invalid_item_rejected_with_parameter_name( + self, list_bool_template, tmp_path + ) -> None: + # The JSON-string form must ALSO name the parameter on a bad item, + # exercising the parse-then-coerce error branch (the native-list form + # is covered by test_invalid_item_rejected_with_parameter_name). + with pytest.raises(ValueError, match=r"Parameter Flags"): + _preprocess(list_bool_template, {"Flags": '["maybe"]'}, tmp_path) + + @pytest.mark.parametrize( + "submitted", + [ + pytest.param("[[true]]", id="nested-list-item"), + pytest.param("[null]", id="null-item"), + pytest.param("[2]", id="int-out-of-range-item"), + pytest.param("[2.0]", id="float-out-of-range-item"), + ], + ) + def test_json_string_invalid_items_rejected_with_parameter_name( + self, list_bool_template, tmp_path, submitted + ) -> None: + # Each parsed item fails _coerce_bool_value (a list, null, an int + # other than 0/1, or a float other than 0.0/1.0), so the JSON-string + # form must name the parameter on the offending item. + with pytest.raises(ValueError, match=r"Parameter Flags"): + _preprocess(list_bool_template, {"Flags": submitted}, tmp_path) + + def test_json_object_not_a_list_rejected(self, list_bool_template, tmp_path) -> None: + # A JSON object (not an array) hits the non-list JSON guard before any + # per-item coercion runs. + with pytest.raises(ValueError, match=r"not valid JSON for a list parameter"): + _preprocess(list_bool_template, {"Flags": '{"a": 1}'}, tmp_path) + + def test_empty_native_list_passes_through(self, list_bool_template, tmp_path) -> None: + # The LIST[BOOL] definition declares no minLength, so an empty list is + # accepted and stored unchanged (per-item coercion of [] yields []). + pv = _preprocess(list_bool_template, {"Flags": []}, tmp_path) + assert pv["Flags"].value == [] + + @pytest.mark.parametrize( + "submitted", + [ + pytest.param([1, 0], id="ints"), + pytest.param([1.0, 0.0], id="floats"), + pytest.param(["yes", "off"], id="strings"), + ], + ) + def test_homogeneous_list_coerced_to_bools( + self, list_bool_template, tmp_path, submitted + ) -> None: + # Homogeneous rows are the silent-failure case: [1, 0] and [1.0, 0.0] + # each compare equal to [True, False] in Python (bool is an int + # subclass, 1.0 == True), so an equality-only assertion would pass even + # if coercion never ran. The type check is what proves per-item + # coercion actually happened; the all-strings row would store verbatim + # (and later fail with an opaque type error) without the fix. + pv = _preprocess(list_bool_template, {"Flags": submitted}, tmp_path) + assert pv["Flags"].value == [True, False] + assert all(type(x) is bool for x in pv["Flags"].value) + + +_NONBOOL_LIST_TEMPLATE = { + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["EXPR"], + "name": "T", + "parameterDefinitions": [ + {"name": "Ints", "type": "LIST[INT]"}, + {"name": "Strs", "type": "LIST[STRING]"}, + ], + "steps": [ + { + "name": "S", + "script": {"actions": {"onRun": {"command": "echo", "args": ["{{ Param.Ints[0] }}"]}}}, + } + ], +} + + +@pytest.fixture +def nonbool_list_template(): + return decode_job_template(template=_NONBOOL_LIST_TEMPLATE, supported_extensions=["EXPR"]) + + +class TestNonBoolListCoercionGuards: + """Per-item BOOL coercion must apply ONLY to LIST[BOOL]. Other LIST[*] types + keep their prior behavior: LIST[INT] still parses a JSON-string form, and a + native LIST[STRING] value passes through untouched (no per-item coercion). + """ + + def test_list_int_json_string_still_parsed(self, nonbool_list_template, tmp_path) -> None: + pv = _preprocess(nonbool_list_template, {"Ints": "[1, 2, 3]", "Strs": ["a", "b"]}, tmp_path) + assert pv["Ints"].value == [1, 2, 3] + + def test_list_string_native_passthrough(self, nonbool_list_template, tmp_path) -> None: + pv = _preprocess(nonbool_list_template, {"Ints": [1], "Strs": ["a", "b"]}, tmp_path) + assert pv["Strs"].value == ["a", "b"] + + +# A job template that declares the EXPR extension and a step but defines no +# job parameters of its own — the LIST[BOOL] parameter is contributed solely by +# an environment template, so its default flows through the merge path. +_JOB_TEMPLATE_NO_PARAMS = { + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["EXPR"], + "name": "T", + "steps": [ + { + "name": "S", + "script": {"actions": {"onRun": {"command": "echo", "args": ["hi"]}}}, + } + ], +} + + +def _env_template(default): + return { + "specificationVersion": "environment-2023-09", + "extensions": ["EXPR"], + "parameterDefinitions": [{"name": "Flags", "type": "LIST[BOOL]", "default": default}], + "environment": { + "name": "Env1", + "script": {"actions": {"onEnter": {"command": "bar"}}}, + }, + } + + +class TestEnvironmentTemplateListBoolDefaultCoercion: + """RFC 0007 §2.15: a LIST[BOOL] default supplied by an environment template + (not the job template) is normalized per item at the create boundary, just + like a job-template default. The merged definition reaches + ``_collect_defaults_2023_09`` via ``merge_job_parameter_definitions``, whose + ``model_copy`` carry-over deliberately skips validators, so the coercion + must happen at collection time rather than being assumed to have run during + decode/merge. + """ + + def test_env_template_mixed_spelling_default_coerced(self, tmp_path) -> None: + jt = decode_job_template(template=_JOB_TEMPLATE_NO_PARAMS, supported_extensions=["EXPR"]) + env = decode_environment_template( + template=_env_template([True, "yes", 0]), supported_extensions=["EXPR"] + ) + pv = preprocess_job_parameters( + job_template=jt, + job_parameter_values={}, + job_template_dir=tmp_path, + current_working_dir=tmp_path, + environment_templates=[env], + ) + assert pv["Flags"].value == [True, True, False] + # equality alone passes for ints ([1,1,0] == [True,True,False]); the + # type check proves the mixed-spelling items were coerced to bools. + assert all(type(x) is bool for x in pv["Flags"].value) + + def test_env_template_empty_list_default_passes_through(self, tmp_path) -> None: + jt = decode_job_template(template=_JOB_TEMPLATE_NO_PARAMS, supported_extensions=["EXPR"]) + env = decode_environment_template(template=_env_template([]), supported_extensions=["EXPR"]) + pv = preprocess_job_parameters( + job_template=jt, + job_parameter_values={}, + job_template_dir=tmp_path, + current_working_dir=tmp_path, + environment_templates=[env], + ) + assert pv["Flags"].value == [] diff --git a/test/openjd/model_v0/v2023_09/test_create_job_expr_params.py b/test/openjd/model_v0/v2023_09/test_create_job_expr_params.py index b2c88271..cdb80d86 100644 --- a/test/openjd/model_v0/v2023_09/test_create_job_expr_params.py +++ b/test/openjd/model_v0/v2023_09/test_create_job_expr_params.py @@ -72,6 +72,68 @@ def test_create_job_scalar_types_unchanged(self): job = _create({"name": "N", "type": "INT", "default": 7}) assert _stored_value(job, "N") == "7" + def test_list_bool_default_mixed_spellings_normalized(self) -> None: + # RFC 0007 §2.15: each LIST[BOOL] item accepts the same spellings as a + # scalar BOOL and is coerced per item into canonical booleans, not + # stored verbatim as a heterogeneous list. + job = _create( + {"name": "Bs", "type": "LIST[BOOL]", "default": [True, "false", "yes", "off", "1", 0]} + ) + assert _stored_value(job, "Bs") == [True, False, True, False, True, False] + # equality alone passes for ints ([1,0,1] == [True,False,True]). + assert all(type(x) is bool for x in _stored_value(job, "Bs")) + + def test_list_bool_default_mixed_case_strings_normalized(self) -> None: + # Per-item coercion is case-insensitive, matching the scalar BOOL forms. + job = _create( + { + "name": "Bs", + "type": "LIST[BOOL]", + "default": ["TRUE", "False", "YES", "no", "On", "OFF"], + } + ) + assert _stored_value(job, "Bs") == [True, False, True, False, True, False] + # equality alone passes for ints ([1,0,1] == [True,False,True]). + assert all(type(x) is bool for x in _stored_value(job, "Bs")) + + @pytest.mark.parametrize( + "default", + [ + pytest.param([1, 0], id="ints"), + pytest.param([1.0, 0.0], id="floats"), + pytest.param(["yes", "off"], id="strings"), + ], + ) + def test_list_bool_homogeneous_default_normalized(self, default) -> None: + # Homogeneous defaults are the silent-failure case: [1, 0] and + # [1.0, 0.0] each compare equal to [True, False] in Python, so an + # equality-only assertion would pass even if coercion never ran. The + # type check is what proves the template default was coerced per item. + job = _create({"name": "Bs", "type": "LIST[BOOL]", "default": default}) + assert _stored_value(job, "Bs") == [True, False] + assert all(type(x) is bool for x in _stored_value(job, "Bs")) + + def test_list_bool_empty_default_passes_through(self) -> None: + # The LIST[BOOL] definition declares no minLength, so an empty default + # is accepted and reaches create_job unchanged (coercion of [] is []). + job = _create({"name": "Bs", "type": "LIST[BOOL]", "default": []}) + assert _stored_value(job, "Bs") == [] + + @pytest.mark.parametrize( + "param_def,expected", + [ + ({"name": "Ps", "type": "LIST[PATH]", "default": ["/a", "/b"]}, ["/a", "/b"]), + ({"name": "Ss", "type": "LIST[STRING]", "default": ["a", "b"]}, ["a", "b"]), + ({"name": "Ms", "type": "LIST[LIST[INT]]", "default": [[1, 2], [3]]}, [[1, 2], [3]]), + ], + ) + def test_non_bool_list_default_unchanged(self, param_def, expected) -> None: + # Per-item BOOL coercion applies ONLY to LIST[BOOL] defaults; other + # LIST[*] defaults must reach create_job untouched (no cross-type + # effect from the LIST[BOOL] normalization added for RFC 0007 §2.15). + job = _create(param_def) + assert _stored_value(job, param_def["name"]) == expected + class TestRangeExprTypedValidation: """A RANGE_EXPR parameter now carries a typed (``range_expr``) EXPR symbol, From e446d9145d7343d226376e32462955ba1adbcbdc Mon Sep 17 00:00:00 2001 From: Yongzhi Wei <276409147+wyongzhi@users.noreply.github.com> Date: Thu, 3 Sep 2026 14:38:39 -0700 Subject: [PATCH 2/4] fix: add parameter context to LIST[BOOL] default coercion errors Wrap template-default per-item coercion failures with the same Parameter : prefix the submitted-value path uses. Add native-list adversarial coverage and a float-spelling case. Signed-off-by: Yongzhi Wei <276409147+wyongzhi@users.noreply.github.com> --- src/openjd/model/_create_job.py | 11 +++++-- .../model_v0/test_expr_param_coercion.py | 29 +++++++++++++++++++ .../v2023_09/test_create_job_expr_params.py | 24 +++++++++++++++ 3 files changed, 62 insertions(+), 2 deletions(-) diff --git a/src/openjd/model/_create_job.py b/src/openjd/model/_create_job.py index 671f8be7..250c5cdd 100644 --- a/src/openjd/model/_create_job.py +++ b/src/openjd/model/_create_job.py @@ -243,8 +243,15 @@ def _collect_defaults_2023_09( # Defaults are pre-validated at decode time by # _check_item (and re-validated on any merge via - # _check_constraints), so this coercion cannot fail. - default_value = [_coerce_bool_value(item) for item in param.default] + # _check_constraints), so this coercion normally cannot + # fail. A definition that bypasses those validators + # (e.g. a model_copy carry-over) could still reach here, + # so name the parameter on failure, matching the + # submitted-value path's error context below. + try: + default_value = [_coerce_bool_value(item) for item in param.default] + except ValueError as exc: + raise ValueError(f"Parameter {param.name}: {exc}") from exc return_value[param.name] = ParameterValue( type=ParameterValueType(param.type), value=default_value ) diff --git a/test/openjd/model_v0/test_expr_param_coercion.py b/test/openjd/model_v0/test_expr_param_coercion.py index b4b38c0d..e09a1cd3 100644 --- a/test/openjd/model_v0/test_expr_param_coercion.py +++ b/test/openjd/model_v0/test_expr_param_coercion.py @@ -143,6 +143,16 @@ def test_json_string_list_coerced_per_item(self, list_bool_template, tmp_path) - # equality alone passes for ints ([1,0,1] == [True,False,True]). assert all(type(x) is bool for x in pv["Flags"].value) + def test_json_string_float_spelling_coerced_per_item( + self, list_bool_template, tmp_path + ) -> None: + # The valid float spelling 1.0/0.0 is accepted per item and stored as + # canonical booleans; equality alone would pass ([1.0, 0.0] == + # [True, False]), so the type check proves per-item coercion ran. + pv = _preprocess(list_bool_template, {"Flags": "[1.0, 0.0]"}, tmp_path) + assert pv["Flags"].value == [True, False] + assert all(type(x) is bool for x in pv["Flags"].value) + def test_invalid_item_rejected_with_parameter_name(self, list_bool_template, tmp_path) -> None: with pytest.raises(ValueError, match=r"Parameter Flags"): _preprocess(list_bool_template, {"Flags": ["maybe"]}, tmp_path) @@ -174,6 +184,25 @@ def test_json_string_invalid_items_rejected_with_parameter_name( with pytest.raises(ValueError, match=r"Parameter Flags"): _preprocess(list_bool_template, {"Flags": submitted}, tmp_path) + @pytest.mark.parametrize( + "submitted", + [ + pytest.param([None], id="null-item"), + pytest.param([2], id="int-out-of-range-item"), + pytest.param([2.0], id="float-out-of-range-item"), + pytest.param([[True]], id="nested-list-item"), + ], + ) + def test_native_list_invalid_items_rejected_with_parameter_name( + self, list_bool_template, tmp_path, submitted + ) -> None: + # The native-list submitted branch (not the JSON-string parse-then- + # coerce branch) must also name the parameter when an item fails + # _coerce_bool_value: null, an int other than 0/1, a float other than + # 0.0/1.0, or a nested list each pin the existing per-item guard. + with pytest.raises(ValueError, match=r"Parameter Flags"): + _preprocess(list_bool_template, {"Flags": submitted}, tmp_path) + def test_json_object_not_a_list_rejected(self, list_bool_template, tmp_path) -> None: # A JSON object (not an array) hits the non-list JSON guard before any # per-item coercion runs. diff --git a/test/openjd/model_v0/v2023_09/test_create_job_expr_params.py b/test/openjd/model_v0/v2023_09/test_create_job_expr_params.py index cdb80d86..ff077efa 100644 --- a/test/openjd/model_v0/v2023_09/test_create_job_expr_params.py +++ b/test/openjd/model_v0/v2023_09/test_create_job_expr_params.py @@ -9,6 +9,8 @@ symbol table can coerce them. """ +from pathlib import Path + import pytest from openjd.model import ( @@ -16,6 +18,7 @@ create_job, decode_job_template, model_to_object, + preprocess_job_parameters, ) @@ -119,6 +122,27 @@ def test_list_bool_empty_default_passes_through(self) -> None: job = _create({"name": "Bs", "type": "LIST[BOOL]", "default": []}) assert _stored_value(job, "Bs") == [] + def test_list_bool_invalid_default_error_names_parameter(self) -> None: + # The template-default coercion path must name the offending parameter, + # matching the submitted-value path. Defaults are normally pre-validated + # at decode, so bypass decode validation with model_copy to place an + # invalid item on the default (mirroring the merge path's model_copy + # carry-over, which skips validators) and reach collection-time coercion. + jt = decode_job_template( + template=_template({"name": "Flags", "type": "LIST[BOOL]", "default": [True]}), + supported_extensions=["EXPR"], + ) + bad_param = jt.parameterDefinitions[0].model_copy(update={"default": ["maybe"]}) + bad_jt = jt.model_copy(update={"parameterDefinitions": [bad_param]}) + with pytest.raises(ValueError, match=r"Parameter Flags"): + preprocess_job_parameters( + job_template=bad_jt, + job_parameter_values={}, + job_template_dir=Path(), + current_working_dir=Path(), + allow_job_template_dir_walk_up=True, + ) + @pytest.mark.parametrize( "param_def,expected", [ From d2e40bb220b2bf345d30de1fa40a8920a6a7d694 Mon Sep 17 00:00:00 2001 From: Yongzhi Wei <276409147+wyongzhi@users.noreply.github.com> Date: Thu, 3 Sep 2026 14:43:29 -0700 Subject: [PATCH 3/4] refactor: dedupe LIST[BOOL] coercion comments Signed-off-by: Yongzhi Wei <276409147+wyongzhi@users.noreply.github.com> --- src/openjd/model/_create_job.py | 34 ++++++++++----------------------- 1 file changed, 10 insertions(+), 24 deletions(-) diff --git a/src/openjd/model/_create_job.py b/src/openjd/model/_create_job.py index 250c5cdd..a7eeb669 100644 --- a/src/openjd/model/_create_job.py +++ b/src/openjd/model/_create_job.py @@ -96,21 +96,17 @@ def _coerce_expr_param_value(param_type_name: str, value: Any) -> Any: if not isinstance(parsed, list): raise ValueError(f"Value '{value}' is not valid JSON for a list parameter.") if param_type_name == "LIST_BOOL": - # RFC 0007 §2.15: each LIST[BOOL] item accepts the same values as a - # scalar BOOL parameter (JobBoolParameterDefinition). Normalize the - # freshly parsed items; other LIST[*] types pass the parsed list - # through unchanged. - # Inline import matches the file's deferred v2023_09 import pattern - # (avoids a module-level dependency on the version package). + # §2.15: LIST[BOOL] items accept the same spellings as scalar BOOL; + # reuse the scalar's coercion so the two can't drift. Deferred import: + # this file is version-agnostic and v2023_09._model imports from this + # package, so a top-level import would risk a cycle. from .v2023_09._model import _coerce_bool_value return [_coerce_bool_value(item) for item in parsed] return parsed if param_type_name == "LIST_BOOL" and isinstance(value, list): - # RFC 0007 §2.15: a LIST[BOOL] value submitted as a native list is - # normalized per item, same as its JSON-string form above. Build a new - # list — never mutate the caller's input. - # Inline import matches the file's deferred v2023_09 import pattern. + # Same §2.15 normalization as the JSON branch above; build a new list, + # never mutate the caller's input. from .v2023_09._model import _coerce_bool_value return [_coerce_bool_value(item) for item in value] @@ -232,22 +228,12 @@ def _collect_defaults_2023_09( # applies to the scalar PATH type. default_value: Any = param.default if param.type.name == "LIST_BOOL" and isinstance(param.default, list): - # RFC 0007 §2.15: each LIST[BOOL] item accepts the same - # values as a scalar BOOL parameter. Normalize the - # template default per item (build a new list — never - # mutate param.default), matching the submitted-value - # path so mixed spellings store as canonical booleans. - # Inline import matches the file's deferred v2023_09 - # import pattern. + # Same §2.15 normalization for template defaults. Decode-time + # validation normally guarantees success, but validator-bypassing + # definitions (e.g. model_copy) can still reach here, hence the + # Parameter-name context on failure. from .v2023_09._model import _coerce_bool_value - # Defaults are pre-validated at decode time by - # _check_item (and re-validated on any merge via - # _check_constraints), so this coercion normally cannot - # fail. A definition that bypasses those validators - # (e.g. a model_copy carry-over) could still reach here, - # so name the parameter on failure, matching the - # submitted-value path's error context below. try: default_value = [_coerce_bool_value(item) for item in param.default] except ValueError as exc: From 441cd303dd18e511f670b8195618a0f2dde3bcba Mon Sep 17 00:00:00 2001 From: Yongzhi Wei <276409147+wyongzhi@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:23:06 -0700 Subject: [PATCH 4/4] fix: leave LIST[BOOL] JSON parse errors unprefixed Route only per-item coercion failures to the Parameter-name prefix. Move _coerce_bool_value to a version-agnostic module, dropping the inline imports. Pin template non-mutation and the None-default guard. Signed-off-by: Yongzhi Wei <276409147+wyongzhi@users.noreply.github.com> --- src/openjd/model/_bool_coercion.py | 36 ++++++++++++++ src/openjd/model/_create_job.py | 48 ++++++++++--------- src/openjd/model/v2023_09/_model.py | 35 +------------- .../model_v0/test_expr_param_coercion.py | 24 ++++++++++ .../v2023_09/test_create_job_expr_params.py | 36 ++++++++++++++ 5 files changed, 123 insertions(+), 56 deletions(-) create mode 100644 src/openjd/model/_bool_coercion.py diff --git a/src/openjd/model/_bool_coercion.py b/src/openjd/model/_bool_coercion.py new file mode 100644 index 00000000..fc4d81b0 --- /dev/null +++ b/src/openjd/model/_bool_coercion.py @@ -0,0 +1,36 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + +from typing import Any + +# Accepted string spellings for boolean defaults/values (case-insensitive), +# per RFC 0007 (BOOL parameter type). +_BOOL_TRUE_STRINGS = frozenset({"true", "yes", "on", "1"}) +_BOOL_FALSE_STRINGS = frozenset({"false", "no", "off", "0"}) + + +def _coerce_bool_value(value: Any) -> bool: + """Coerce an RFC 0007 BOOL value to a Python bool, raising ValueError for + anything outside the accepted set (bool, int 0/1, float 0.0/1.0, or a + case-insensitive true/false/yes/no/on/off/1/0 string). + """ + if isinstance(value, bool): + return value + if isinstance(value, int): # bool already handled above + if value in (0, 1): + return bool(value) + raise ValueError("BOOL value as an integer must be 0 or 1.") + if isinstance(value, float): + if value in (0.0, 1.0): + return bool(value) + raise ValueError("BOOL value as a float must be 0.0 or 1.0.") + if isinstance(value, str): + low = value.lower() + if low in _BOOL_TRUE_STRINGS: + return True + if low in _BOOL_FALSE_STRINGS: + return False + raise ValueError( + "BOOL value as a string must be one of (case-insensitive): " + "true, false, yes, no, on, off, 1, 0." + ) + raise ValueError("BOOL value must be a boolean, 0/1, 0.0/1.0, or a boolean string.") diff --git a/src/openjd/model/_create_job.py b/src/openjd/model/_create_job.py index a7eeb669..2e2a0ad9 100644 --- a/src/openjd/model/_create_job.py +++ b/src/openjd/model/_create_job.py @@ -8,6 +8,7 @@ from pydantic import ValidationError +from ._bool_coercion import _coerce_bool_value from ._errors import CompatibilityError, DecodeValidationError from ._format_strings import FormatStringError from ._symbol_table import SymbolTable @@ -63,6 +64,14 @@ class JobWithSymbolTables: _LEGACY_SCALAR_TYPE_NAMES = frozenset({"STRING", "INT", "FLOAT", "PATH"}) +class _ListBoolItemError(ValueError): + """A LIST[BOOL] per-item coercion failure, distinct from the JSON-level + parse errors shared by all LIST[*] types. The value-collection call site + prefixes only these with the parameter name; JSON/scalar errors stay + verbatim, matching the other list types. + """ + + def _coerce_expr_param_value(param_type_name: str, value: Any) -> Any: """Coerce a SUBMITTED string value for an EXPR-typed job parameter to its native form, mirroring openjd-rs's ``coerce_from_str`` @@ -96,20 +105,19 @@ def _coerce_expr_param_value(param_type_name: str, value: Any) -> Any: if not isinstance(parsed, list): raise ValueError(f"Value '{value}' is not valid JSON for a list parameter.") if param_type_name == "LIST_BOOL": - # §2.15: LIST[BOOL] items accept the same spellings as scalar BOOL; - # reuse the scalar's coercion so the two can't drift. Deferred import: - # this file is version-agnostic and v2023_09._model imports from this - # package, so a top-level import would risk a cycle. - from .v2023_09._model import _coerce_bool_value - - return [_coerce_bool_value(item) for item in parsed] + # §2.15: LIST[BOOL] items accept the same spellings as scalar BOOL; reuse the scalar's coercion so the two can't drift. + try: + return [_coerce_bool_value(item) for item in parsed] + except ValueError as exc: + raise _ListBoolItemError(str(exc)) from exc return parsed if param_type_name == "LIST_BOOL" and isinstance(value, list): # Same §2.15 normalization as the JSON branch above; build a new list, # never mutate the caller's input. - from .v2023_09._model import _coerce_bool_value - - return [_coerce_bool_value(item) for item in value] + try: + return [_coerce_bool_value(item) for item in value] + except ValueError as exc: + raise _ListBoolItemError(str(exc)) from exc return value @@ -232,8 +240,6 @@ def _collect_defaults_2023_09( # validation normally guarantees success, but validator-bypassing # definitions (e.g. model_copy) can still reach here, hence the # Parameter-name context on failure. - from .v2023_09._model import _coerce_bool_value - try: default_value = [_coerce_bool_value(item) for item in param.default] except ValueError as exc: @@ -265,16 +271,14 @@ def _collect_defaults_2023_09( # Raises ValueError (collected by the caller) on bad input. try: value = _coerce_expr_param_value(param.type.name, value) - except ValueError as exc: - if param.type.name == "LIST_BOOL": - # RFC 0007 §2.15: per-item coercion runs here during - # value collection, before _check_2023_09/_check_constraints, - # so the error would surface name-free unless named at this - # call site. - # Other EXPR errors keep their verbatim (name-free) - # message, as the scalar BOOL branch does. - raise ValueError(f"Parameter {param.name}: {exc}") from exc - raise + except _ListBoolItemError as exc: + # RFC 0007 §2.15: per-item coercion runs here during value + # collection, before _check_2023_09/_check_constraints, so + # a per-item failure would surface name-free unless named + # at this call site. JSON-level and scalar errors are plain + # ValueErrors and keep their verbatim (name-free) message, + # matching the other list types. + raise ValueError(f"Parameter {param.name}: {exc}") from exc return_value[param.name] = ParameterValue( type=ParameterValueType(param.type), value=value ) diff --git a/src/openjd/model/v2023_09/_model.py b/src/openjd/model/v2023_09/_model.py index f27499be..373d2e03 100644 --- a/src/openjd/model/v2023_09/_model.py +++ b/src/openjd/model/v2023_09/_model.py @@ -31,6 +31,7 @@ from .._format_strings import FormatString from .._errors import ExpressionError, TokenError +from .._bool_coercion import _coerce_bool_value from .._capabilities import ( validate_amount_capability_name, validate_attribute_capability_name, @@ -3755,40 +3756,6 @@ class JobBoolParameterDefinitionUserInterface(OpenJDModel_v2023_09): groupLabel: Optional[UserInterfaceLabelStringValue] = None # noqa: N815 -# Accepted string spellings for boolean defaults/values (case-insensitive), -# per RFC 0007 (BOOL parameter type). -_BOOL_TRUE_STRINGS = frozenset({"true", "yes", "on", "1"}) -_BOOL_FALSE_STRINGS = frozenset({"false", "no", "off", "0"}) - - -def _coerce_bool_value(value: Any) -> bool: - """Coerce an RFC 0007 BOOL value to a Python bool, raising ValueError for - anything outside the accepted set (bool, int 0/1, float 0.0/1.0, or a - case-insensitive true/false/yes/no/on/off/1/0 string). - """ - if isinstance(value, bool): - return value - if isinstance(value, int): # bool already handled above - if value in (0, 1): - return bool(value) - raise ValueError("BOOL value as an integer must be 0 or 1.") - if isinstance(value, float): - if value in (0.0, 1.0): - return bool(value) - raise ValueError("BOOL value as a float must be 0.0 or 1.0.") - if isinstance(value, str): - low = value.lower() - if low in _BOOL_TRUE_STRINGS: - return True - if low in _BOOL_FALSE_STRINGS: - return False - raise ValueError( - "BOOL value as a string must be one of (case-insensitive): " - "true, false, yes, no, on, off, 1, 0." - ) - raise ValueError("BOOL value must be a boolean, 0/1, 0.0/1.0, or a boolean string.") - - class JobBoolParameterDefinition(NameIdentifierLengthMixin, OpenJDModel_v2023_09): """A Job Parameter of type bool (EXPR extension, RFC 0007). diff --git a/test/openjd/model_v0/test_expr_param_coercion.py b/test/openjd/model_v0/test_expr_param_coercion.py index e09a1cd3..c49da766 100644 --- a/test/openjd/model_v0/test_expr_param_coercion.py +++ b/test/openjd/model_v0/test_expr_param_coercion.py @@ -209,6 +209,30 @@ def test_json_object_not_a_list_rejected(self, list_bool_template, tmp_path) -> with pytest.raises(ValueError, match=r"not valid JSON for a list parameter"): _preprocess(list_bool_template, {"Flags": '{"a": 1}'}, tmp_path) + @pytest.mark.parametrize( + "bad", + [ + pytest.param("[1,2", id="malformed-json"), + pytest.param('{"a": 1}', id="json-but-not-a-list"), + ], + ) + def test_json_parse_error_not_prefixed_matches_other_list_types( + self, list_bool_template, template, tmp_path, bad + ) -> None: + # The JSON-level parse error is shared by all LIST[*] types and is not a + # per-item coercion failure, so LIST[BOOL] must NOT prefix it with the + # parameter name; the message must be byte-identical to the one a + # LIST[INT] parameter produces for the same bad input. + with pytest.raises(ValueError) as bool_exc: + _preprocess(list_bool_template, {"Flags": bad}, tmp_path) + with pytest.raises(ValueError) as int_exc: + _preprocess(template, {"Flag": "true", "Values": bad, "Nested": [[1]]}, tmp_path) + # The parse error is the first collected error; the trailing + # missing-value line names each template's own list parameter, so + # compare the parse-error line itself. + assert not str(bool_exc.value).startswith("Parameter") + assert str(bool_exc.value).splitlines()[0] == str(int_exc.value).splitlines()[0] + def test_empty_native_list_passes_through(self, list_bool_template, tmp_path) -> None: # The LIST[BOOL] definition declares no minLength, so an empty list is # accepted and stored unchanged (per-item coercion of [] yields []). diff --git a/test/openjd/model_v0/v2023_09/test_create_job_expr_params.py b/test/openjd/model_v0/v2023_09/test_create_job_expr_params.py index ff077efa..6fbb9113 100644 --- a/test/openjd/model_v0/v2023_09/test_create_job_expr_params.py +++ b/test/openjd/model_v0/v2023_09/test_create_job_expr_params.py @@ -122,6 +122,42 @@ def test_list_bool_empty_default_passes_through(self) -> None: job = _create({"name": "Bs", "type": "LIST[BOOL]", "default": []}) assert _stored_value(job, "Bs") == [] + def test_list_bool_default_template_not_mutated(self) -> None: + # create_job coerces a LIST[BOOL] template default to canonical booleans + # for the created Job, but must build a NEW list and leave the template's + # own default (and its serialized form) with the raw submitted spellings. + jt = decode_job_template( + template=_template({"name": "Bs", "type": "LIST[BOOL]", "default": ["yes", 0, True]}), + supported_extensions=["EXPR"], + ) + job = create_job(job_template=jt, job_parameter_values={}) + created = _stored_value(job, "Bs") + assert created == [True, False, True] + # equality alone passes for ints ([1,0,1] == [True,False,True]). + assert all(type(x) is bool for x in created) + # The template object's default is untouched, with its original item types. + default = jt.parameterDefinitions[0].default + assert default == ["yes", 0, True] + assert [type(x) for x in default] == [str, int, bool] + # The serialized template still carries the raw spellings, not the coerced booleans. + dumped = model_to_object(model=jt)["parameterDefinitions"][0]["default"] + assert dumped == ["yes", 0, True] + assert [type(x) for x in dumped] == [str, int, bool] + + def test_list_bool_none_default_flows_without_type_error(self) -> None: + # A LIST[BOOL] definition with no default (default None) must not reach + # the per-item coercion comprehension: the outer `is not None` check plus + # the `isinstance(param.default, list)` guard keep None from being + # iterated. Job creation must surface the normal missing-required-value + # error, never a TypeError from iterating None. + jt = decode_job_template( + template=_template({"name": "Bs", "type": "LIST[BOOL]"}), + supported_extensions=["EXPR"], + ) + assert jt.parameterDefinitions[0].default is None + with pytest.raises(DecodeValidationError, match=r"missing for required job parameters"): + create_job(job_template=jt, job_parameter_values={}) + def test_list_bool_invalid_default_error_names_parameter(self) -> None: # The template-default coercion path must name the offending parameter, # matching the submitted-value path. Defaults are normally pre-validated