Repository navigation
feat: accept string-encoded boolean job parameters - #1098
Conversation
| param_value = ParameterValue(type=ParameterValueType.BOOL, value=value["bool"]) | ||
| param_value = ParameterValue( | ||
| type=ParameterValueType.BOOL, | ||
| value=_bool_from_api_response(name, value["bool"]), |
There was a problem hiding this comment.
_validate_job_parameters still rejects string booleans, so this coercion is unreachable for job parameters.
JobDetails.validate_entity_data runs before JobDetails.from_boto (see job_entities.py:328-329), and its shape checks at job_details.py:555 / job_details.py:561 are still:
"bool": lambda v: isinstance(v, bool),
...
"boolList": lambda v: isinstance(v, list) and all(isinstance(item, bool) for item in v),So a BatchGetJobEntity response carrying the new wire form {"bool": "true"} raises
Value of parameters["x"] does not match the expected shape for its type "bool"
during validation and never reaches _bool_from_api_response. Note the sibling entries for int/float/chunkInt are already isinstance(v, str), which is consistent with the service sending these as strings — bool looks like it was simply missed.
Net effect: for the job-details path the PR does not fix the problem it targets (only the task-parameter path in scheduler/session_queue.py:535, which bypasses this validator, benefits). Suggest widening both predicates to accept str as well, e.g.:
"bool": lambda v: isinstance(v, (str, bool)),
"boolList": lambda v: isinstance(v, list) and all(isinstance(item, (str, bool)) for item in v),and adding a validate_entity_data-level test with a string-valued bool parameter, since the new tests only exercise parameters_from_api_response directly and therefore pass despite this gap.
There was a problem hiding this comment.
looking at this, im not sure if this is correct
|
|
||
| # WHEN | ||
| result = parameters_from_api_response(cast("dict[str, Any]", params)) | ||
|
|
There was a problem hiding this comment.
Existing test test_job_details.py:412-429 still asserts the opposite of this PRs premise.
That parametrized case is labelled "nonvalid parameters - bool value is a string, not a boolean." and feeds {"param1": {"bool": "true"}} through JobDetails.validate_entity_data expecting a ValueError. It is untouched by this PR and still passes — which is direct confirmation that the string wire form is rejected before ever reaching the new _bool_from_api_response on the job-details path (see the separate comment on job_details.py:129).
If string booleans are now valid on the wire, that test case should move from the invalid set to the valid set, otherwise the suite is codifying the pre-change behaviour as correct.
| param_value = ParameterValue(type=ParameterValueType.LIST_BOOL, value=value["boolList"]) | ||
| param_value = ParameterValue( | ||
| type=ParameterValueType.LIST_BOOL, | ||
| value=[_bool_from_api_response(name, item) for item in value["boolList"]], |
There was a problem hiding this comment.
Worth considering: on the task-parameter path this new ValueError is not caught and will take down the session thread.
parameters_from_api_response is also called from session_queue.py:535 for TASK_RUN parameters, and that call sits outside the surrounding try/except (ValueError, RuntimeError) that wraps step_details(...) at lines 516-533. So a ValueError from here escapes dequeue(), and Session._start_action only catches SessionActionError (session.py:658) — it propagates through _run() into run(), which sets _stop_fail_message and re-raises.
That means an unrecognized boolean token in a task parameter fails the whole session rather than reporting the single action FAILED. Task parameters also get no _validate_job_parameters-style pre-check, so this is the actual live path.
There is a pre-existing raise ValueError in the else branch at line 164, so the escape route is not brand new — but this PR widens it from "unknown parameter form" (a protocol-level impossibility) to "bool token we do not recognise" (something a service-side vocabulary addition could trigger). Wrapping the parameters_from_api_response call at session_queue.py:535 in the same StepDetailsError handling as its neighbours would keep the failure scoped to the action.
Also, minor: {"boolList": None} reaches the comprehension here and raises TypeError, not ValueError, which would not be caught even by a (ValueError, RuntimeError) handler.
The service is changing boolean job parameters from native JSON booleans to constrained strings drawn from the OpenJD case-insensitive boolean vocabulary (true/false/yes/no/on/off/1/0/1.0/0.0). Jobs created before that change are persisted with native booleans and echoed back verbatim, so either form may arrive on the wire. Teach the single decode choke point (parameters_from_api_response) to coerce both forms into native Python bools at the boundary where OpenJD expects them. A private helper _bool_from_api_response handles the vocabulary lookup and rejects anything outside it. A non-list boolList value is rejected with a ValueError naming the parameter rather than surfacing as a TypeError. The api_models TypedDicts are widened to str|bool to reflect the dual wire format. JobDetails.validate_entity_data runs before from_boto, so its shape predicates for bool and boolList are widened to accept str as well; without this the string wire form was rejected during validation and never reached the decoder. The validate_entity_data tests that asserted string booleans were invalid are moved to the valid set, with an added end-to-end check that a string bool survives validation and decodes to a native bool. Task parameters (TaskParameterValue / UpdateWorkerSchedule) use a separate map that is not changing, and native bools pass through the helper unchanged, so the task path is unaffected. No change is needed for the parameter-list max widening from 64 to 512: the agent has no list-length assumptions, and botocore does not enforce @Length constraints on response parsing. Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
11576e0
631e382 to
11576e0
Compare
What was the problem/requirement? (What/Why)
The service is changing boolean job parameters from native JSON booleans to constrained strings drawn from the OpenJD case-insensitive boolean vocabulary (
true/false/yes/no/on/off/1/0/1.0/0.0). Jobs created before that change are persisted with native booleans and echoed back verbatim, so either form may arrive on the wire.What was the solution? (How)
Teach the single decode choke point (
parameters_from_api_response) to coerce both string and native-bool forms into native Python bools at the boundary where OpenJD expects them. A private helper_bool_from_api_responsehandles the vocabulary lookup and rejects anything outside it. Theapi_modelsTypedDicts are widened tostr | boolto reflect the dual wire format.Task parameters (
TaskParameterValue/UpdateWorkerSchedule) use a separate map that is not changing, and native bools pass through the helper unchanged, so the task path is unaffected.No change is needed for the parameter-list max widening from 64 to 512: the agent has no list-length assumptions, and botocore does not enforce
@lengthconstraints on response parsing.What is the impact of this change?
Boolean job parameters arriving as strings are correctly decoded to native Python bools. Pre-existing jobs arriving as native booleans continue to work. No behavioral change for any non-boolean parameter type.
How was this change tested?
boolListdecoding.hatch run all:test: 3212 passed on Python 3.9, 3.10, 3.11.hatch run lint: ruff check, ruff format, mypy all pass.Was this change documented?
Inline code comments and docstring explain the dual wire format and the bool-subclass-of-int guard. No user-facing documentation change needed.
Is this a breaking change?
No.