Copilot commented on code in PR #63995:
URL: https://github.com/apache/airflow/pull/63995#discussion_r3025336428
##########
airflow-core/src/airflow/api_fastapi/execution_api/versions/v2026_03_31.py:
##########
@@ -95,3 +95,11 @@ def ensure_start_date_in_dag_run(response: ResponseInfo) ->
None: # type: ignor
"""Ensure start_date is never None in direct DagRun responses for
previous API versions."""
if response.body.get("start_date") is None:
response.body["start_date"] = response.body.get("run_after")
+
+
+class AddMessageResponseModel(VersionChange):
+ """Add MessageResponse schema to common datamodels."""
+
+ description = __doc__
+
+ instructions_to_migrate_to_previous_version = ()
Review Comment:
`AddMessageResponseModel` is defined but not wired into the execution API
version bundle, so it has no effect. Either add it to
`airflow/api_fastapi/execution_api/versions/__init__.py` for the appropriate
Version(...) entry, or remove the unused VersionChange class to avoid dead code.
```suggestion
```
##########
airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_variables.py:
##########
@@ -274,7 +274,7 @@ def test_should_not_delete_variable(self, client, session):
response = client.delete("/execution/variables/non_existent_key")
- assert response.status_code == 204
+ assert response.status_code == 200
vars = session.scalars(select(Variable)).all()
assert len(vars) == 1
Review Comment:
The delete-variable tests now assert `200` for both existing and
non-existent keys. If the endpoint is intended to return a message body (as
this PR does), please also assert the response payload (e.g.
`response.json()["message"] ...`) to ensure the new schema is actually returned
and prevent regressions where the handler returns an empty body/incorrect shape.
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/xcoms.py:
##########
@@ -348,7 +349,7 @@ def set_xcom(
mapped_length: Annotated[
int | None, Query(description="Number of mapped tasks this value
expands into")
] = None,
-):
+) -> MessageResponse:
"""Set an Airflow XCom."""
from airflow.configuration import conf
Review Comment:
There is a `from airflow.configuration import conf` import inside
`set_xcom()`. Airflow guidelines require imports at the module level unless
needed for circular-import avoidance or isolation/lazy-loading; please move
this import to the top of the file or add a short comment justifying why it
must remain inside the function.
##########
task-sdk/src/airflow/sdk/api/datamodels/_generated.py:
##########
@@ -177,6 +177,14 @@ class IntermediateTIState(str, Enum):
DEFERRED = "deferred"
+class MessageResponse(BaseModel):
+ """
+ Message response schema.
+ """
+
+ message: Annotated[str, Field(title="Message")]
+
+
Review Comment:
`_generated.py` is a generated file (datamodel-codegen header). Adding
`MessageResponse` here suggests a manual edit that will be lost on
regeneration. Please ensure the OpenAPI schema is updated and then regenerate
the Task SDK models so this class is produced automatically, rather than
maintaining it by hand.
```suggestion
```
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/xcoms.py:
##########
@@ -436,4 +437,4 @@ def delete_xcom(
)
session.execute(query)
session.commit()
- return {"message": f"XCom with key: {key} successfully deleted."}
+ return MessageResponse(message=f"XCom with key: {key} successfully
deleted.")
Review Comment:
`delete_xcom()` calls `session.commit()` even though it receives a session
dependency. Per Airflow DB/session lifecycle guidelines, functions that accept
an injected session should not commit; the caller/dependency should manage the
transaction. Please remove the explicit commit and rely on the request/session
lifecycle (or refactor to use a helper that manages its own session if commit
is required).
##########
airflow-core/src/airflow/api_fastapi/execution_api/datamodels/common.py:
##########
@@ -0,0 +1,25 @@
+# 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
+
+from airflow.api_fastapi.core_api.base import BaseModel
+
+
+class MessageResponse(BaseModel):
+ """Message response schema."""
+
+ message: str
Review Comment:
To keep the OpenAPI schema consistent with the Task SDK-generated
`MessageResponse` (which uses `Field(title="Message")`), consider adding an
explicit `Field`/metadata here as well (e.g. title/description). This helps
ensure the generated OpenAPI and SDK models stay aligned.
```suggestion
from pydantic import Field
class MessageResponse(BaseModel):
"""Message response schema."""
message: str = Field(title="Message")
```
--
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]
