kaxil commented on code in PR #76007:
URL: https://github.com/apache/airflow/pull/76007#discussion_r4437512717
##########
task-sdk/src/airflow/sdk/execution_time/context.py:
##########
@@ -526,6 +535,10 @@ def _set_variable(key: str, value: Any, description: str |
None = None, serializ
except Exception as e:
log.exception(e)
+ if value is None:
+ warnings.warn(_SET_VARIABLE_NONE_DEPRECATION,
RemovedInAirflow4Warning, stacklevel=3)
Review Comment:
Will a Dag author actually see this? `stacklevel=3` attributes it to the Dag
module (loaded as `unusual_prefix_*`), and Python's default filters ignore
`DeprecationWarning` outside `__main__`. The only extra filter Airflow installs
(`airflow/configuration.py`) matches modules named `airflow*`, so this gets
dropped before the structlog `showwarning` hook ever sees it. I checked the
filter behaviour with a small script mirroring this call chain: nothing printed
at `stacklevel=3`, with or without the `airflow` filter. `pytest.warns` resets
the filters, which is why the tests pass. A `log.warning` alongside it would
put it in the task log. Same on L596.
##########
task-sdk/tests/task_sdk/execution_time/schema/test_migrator.py:
##########
@@ -127,6 +129,28 @@ 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_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")
+
+
[email protected]("version", ["2026-10-30", "2027-02-28"])
Review Comment:
This one can't fail: `"value"` is valid for both `str` and `str | None`, so
it passes with or without the version change. Same gap as the Execution API
tests on the upgrade side too, nothing upgrades a real value from 2026-10-30.
Something like `upgrade({..., "value": "x"}, PutVariable,
"2026-10-30")["value"] == "x"`, plus asserting the 2026-10-30 `PutVariable`
class accepts `value=None` while head rejects it, would pin the actual change.
##########
task-sdk/src/airflow/sdk/execution_time/schema/versions/v2027_02_28.py:
##########
@@ -0,0 +1,48 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Supervisor schema version 2027-02-28."""
+
+from __future__ import annotations
+
+from cadwyn import RequestInfo, VersionChange,
convert_request_to_next_version_for, schema
+
+from airflow.sdk.api.datamodels._generated import VariableResponse
+from airflow.sdk.execution_time.comms import PutVariable
+
+
+class MakeSupervisorVariableValueNonNullable(VersionChange):
+ """
+ Make the Variable value non-nullable in ``VariableResult`` and
``PutVariable``.
+
+ The supervisor-schema mirror of the execution API's
``MakeVariableValueNonNullable``,
+ named apart so the two migrations are not confused.
+ """
+
+ description = __doc__
+
+ instructions_to_migrate_to_previous_version = (
+ # VariableResult (and TIRunContext.variables) inherit the field from
the generated
+ # VariableResponse, and Cadwyn only changes a field on the class that
declares it.
+ schema(VariableResponse).field("value").had(type=str | None),
+ schema(PutVariable).field("value").had(type=str | None),
+ )
+
+ @convert_request_to_next_version_for(PutVariable) # type: ignore[arg-type]
+ def store_null_value_as_none_string(request: RequestInfo) -> None:
+ """Keep the old behavior of storing ``str(None)`` when an older
runtime sets null."""
+ if request.body.get("value") is None:
Review Comment:
`SchemaVersionMigrator.upgrade` runs this on the raw wire dict and only
validates afterwards at head, so `.get("value") is None` also matches a
`PutVariable` with no `value` key. That used to fail validation (the field was
required at 2026-10-30) and now silently stores `"None"`. Java always sends the
key, but a runtime whose generated models drop null fields would hit this. `if
"value" in request.body and request.body["value"] is None:` keeps it to the
null case.
##########
go-sdk/capabilities.yaml:
##########
@@ -17,11 +17,11 @@
---
sdk: go
-min_airflow_version: "3.4.0"
+min_airflow_version: "3.5.0"
Review Comment:
Why move Go and TS to 2027-02-28 here? They only ever send a string in
`PutVariable`, and Java shows 2026-10-30 keeps working. With the bump, 3.4.x
won't load their bundles (3.4.0rc2 only knows 2026-10-30 and 2026-06-16), so a
Go or TS 1.0 cut from main needs 3.5.0. Unless I'm missing something they need
from the new schema, I'd leave them on 2026-10-30 for now.
##########
airflow-core/tests/unit/api_fastapi/execution_api/versions/v2027_02_28/test_variables.py:
##########
@@ -0,0 +1,76 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import pytest
+from sqlalchemy import select
+
+from airflow.models.variable import Variable
+
+from tests_common.test_utils.db import clear_db_variables
+
+pytestmark = pytest.mark.db_test
+
+PREVIOUS_VERSION = "2026-10-30"
+
+
[email protected](autouse=True)
+def setup_method():
+ clear_db_variables()
+ yield
+ clear_db_variables()
+
+
+class TestVariableValueNonNullableVersioning:
+ """The Variable value became non-nullable in the 2027-02-28 API version."""
+
+ @pytest.mark.parametrize("body", [{"value": None}, {"val": None}])
Review Comment:
Every old-version write here sends null, so a converter that always wrote
`"None"` (dropping the `if`) would still pass, while silently clobbering every
real value an old client sets. Could you add an old-version PUT of `{"val":
"real"}` that asserts `"real"` is stored? The two GET tests below could go in
its place, they repeat `head/test_variables.py` and this version change doesn't
touch the GET path.
--
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]
