kaxil commented on code in PR #76007:
URL: https://github.com/apache/airflow/pull/76007#discussion_r4485057441
##########
ts-sdk/src/generated/supervisor.ts:
##########
@@ -561,7 +561,7 @@ export type RegionIndex1 = number | null;
export type Duration = number | null;
export type Type60 = "PreviousTIResult";
export type Key14 = string;
-export type Value1 = string | null;
+export type Value1 = string;
Review Comment:
A plain-JS `setVariable(key, null)` used to store `"None"`. Now the
supervisor can't decode the frame, logs it and never replies, so the call sits
until the 30s request timeout and fails with a generic timeout error. Could
`setVariable` reject a non-string `value` up front, like `assertKey` does for
the state store?
##########
task-sdk/tests/task_sdk/definitions/test_variables.py:
##########
@@ -99,6 +100,25 @@ def test_var_set(self, key, value, description,
serialize_json, mock_supervisor_
),
)
+ def test_var_set_none_warns_and_stores_none_string(self,
mock_supervisor_comms):
+ with pytest.warns(
Review Comment:
Nothing checks where the warning points. With both `stacklevel=3` changed to
`2` these still pass, and the warning is attributed to
`definitions/variable.py` instead of the Dag author's line. `with
pytest.warns(...) as record:` plus `assert record[0].filename == __file__`
would pin it (same for the async test).
##########
task-sdk/tests/task_sdk/execution_time/schema/test_migrator.py:
##########
@@ -127,6 +129,46 @@ def test_loop_selectors_follow_supervisor_version(model,
extra, field):
assert migrator.downgrade(message, "2026-10-30").model_dump()[field] is
True
+def test_put_variable_null_from_older_runtime_is_stored_as_none_string():
+ body = {"type": "PutVariable", "key": "key", "value": None, "description":
None}
+
+ upgraded = get_schema_version_migrator().upgrade(body, PutVariable,
"2026-10-30")
+
+ assert upgraded["value"] == "None"
+
+
+def test_put_variable_without_value_from_older_runtime_is_rejected():
+ body = {"type": "PutVariable", "key": "key", "description": None}
+
+ with pytest.raises(ValidationError):
+ get_schema_version_migrator().upgrade(body, PutVariable, "2026-10-30")
+
+
+def test_put_variable_null_from_current_runtime_is_rejected():
+ body = {"type": "PutVariable", "key": "key", "value": None, "description":
None}
+
+ with pytest.raises(ValidationError):
+ get_schema_version_migrator().upgrade(body, PutVariable, "2027-02-28")
+
+
+def test_put_variable_value_from_older_runtime_is_kept():
+ body = {"type": "PutVariable", "key": "key", "value": "x", "description":
None}
+
+ upgraded = get_schema_version_migrator().upgrade(body, PutVariable,
"2026-10-30")
+
+ assert upgraded["value"] == "x"
+
+
+def test_put_variable_value_is_nullable_only_before_2027_02_28():
+ from airflow.sdk.execution_time.schema.versions import bundle
+
+ versioned = generate_versioned_models(bundle)
+
+ assert versioned["2026-10-30"][PutVariable](key="key", value=None,
description=None).value is None
Review Comment:
This only pins the `PutVariable` half. Deleting the
`schema(VariableResponse).field("value").had(...)` line still passes the suite,
and then the 2026-10-30 `VariableResult` (what the Java SDK receives) becomes
non-nullable. Could you parametrize this over `VariableResult` too?
##########
ts-sdk/src/coordinator/client.ts:
##########
@@ -305,7 +305,7 @@ export function createCoordinatorClient(
"GetVariable",
"VariableResult",
msg,
- (body) => (body!.value as string) ?? null,
+ (body) => body!.value as string,
Review Comment:
The `TaskClient.getVariable` doc in `ts-sdk/src/sdk/client.ts` still says it
returns `null` when the key is "missing or stored with a null value". Now that
null only means missing, can the second half go?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
