pierrejeambrun commented on code in PR #62624:
URL: https://github.com/apache/airflow/pull/62624#discussion_r3275294706
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -824,12 +838,12 @@ def ti_heartbeat(
description="Store the rendered task instance fields (RTIF) for a task
instance. "
"These are the template fields after Jinja rendering has been applied. "
"Called by the worker after task execution begins.",
- responses={
- status.HTTP_404_NOT_FOUND: {"description": "Task Instance not found"},
- HTTP_422_UNPROCESSABLE_CONTENT: {
- "description": "Invalid payload for the setting rendered task
instance fields"
- },
- },
+ responses=create_openapi_http_exception_doc(
+ [
+ (status.HTTP_404_NOT_FOUND, "Task Instance not found."),
+ (HTTP_422_UNPROCESSABLE_CONTENT, "Invalid payload for the state
transition"),
Review Comment:
`ti_put_rtif`'s 422 description was `"Invalid payload for the setting
rendered task instance fields"` on `main` and it's now `"Invalid payload for
the state transition"`. This endpoint isn't a state transition — was that an
accidental copy/paste? Suggest keeping the original.
```suggestion
(HTTP_422_UNPROCESSABLE_CONTENT, "Invalid payload for setting
rendered task instance fields"),
```
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @pierrejeambrun before
posting
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -855,10 +869,15 @@ def ti_put_rtif(
@ti_id_router.patch(
"/{task_instance_id}/rendered-map-index",
status_code=status.HTTP_204_NO_CONTENT,
- responses={
- status.HTTP_404_NOT_FOUND: {"description": "Task Instance not found"},
- HTTP_422_UNPROCESSABLE_CONTENT: {"description": "Invalid
rendered_map_index value"},
- },
+ responses=create_openapi_http_exception_doc(
+ [
+ (status.HTTP_404_NOT_FOUND, "Task Instance not found."),
+ (
+ HTTP_422_UNPROCESSABLE_CONTENT,
+ "Invalid payload for the state transition",
Review Comment:
Same thing here — `ti_patch_rendered_map_index`'s 422 was `"Invalid
rendered_map_index value"` on `main`; now it's the generic `"Invalid payload
for the state transition"`. Lost the specific signal.
```suggestion
"Invalid rendered_map_index value",
```
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @pierrejeambrun before
posting
##########
airflow-core/src/airflow/api_fastapi/core_api/openapi/exceptions.py:
##########
@@ -26,16 +28,34 @@ class HTTPExceptionResponse(BaseModel):
detail: str | dict
-def create_openapi_http_exception_doc(responses_status_code: list[int]) ->
dict:
+HTTPExceptionResponseDoc = int | tuple[int, str]
Review Comment:
Nit: the type alias is only used once in the signature right below. Worth
keeping for readability, but `responses_status_code: Sequence[int | tuple[int,
str]]` would be just as clear and one fewer name to track. Up to you.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @pierrejeambrun before
posting
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -102,11 +103,13 @@
"/{task_instance_id}/run",
status_code=status.HTTP_200_OK,
dependencies=[Security(require_auth, scopes=["token:execution",
"token:workload"])],
- responses={
- status.HTTP_404_NOT_FOUND: {"description": "Task Instance not found"},
- status.HTTP_409_CONFLICT: {"description": "The TI is already in the
requested state"},
- HTTP_422_UNPROCESSABLE_CONTENT: {"description": "Invalid payload for
the state transition"},
- },
+ responses=create_openapi_http_exception_doc(
+ [
+ (status.HTTP_404_NOT_FOUND, "Task Instance not found"),
Review Comment:
Nit: half the new descriptions end with `.` and half don't (`Task Instance
not found` vs `Task Instance not found.` in this same file). Mind picking one
style and being consistent? No preference from me — just not mixed.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @pierrejeambrun before
posting
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -892,9 +911,11 @@ def ti_patch_rendered_map_index(
@ti_id_router.get(
"/{task_instance_id}/previous-successful-dagrun",
status_code=status.HTTP_200_OK,
- responses={
- status.HTTP_404_NOT_FOUND: {"description": "Task Instance or Dag Run
not found"},
- },
+ responses=create_openapi_http_exception_doc(
+ [
+ (status.HTTP_404_NOT_FOUND, "Task Instance not found."),
Review Comment:
`get_previous_successful_dagrun`'s 404 was `"Task Instance or Dag Run not
found"` and is now `"Task Instance not found."` — the endpoint really does 404
for either, so the trimmed version is misleading.
```suggestion
(status.HTTP_404_NOT_FOUND, "Task Instance or Dag Run not
found"),
```
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @pierrejeambrun before
posting
--
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]
