Copilot commented on code in PR #63801:
URL: https://github.com/apache/airflow/pull/63801#discussion_r3025334063
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/hitl.py:
##########
@@ -121,7 +122,7 @@ def update_hitl_detail(
).scalar()
hitl_detail_model = _check_hitl_detail_exists(hitl_detail_model_result)
if hitl_detail_model.response_received:
- raise HTTPException(
+ raise ExecutionHTTPException(
status.HTTP_409_CONFLICT,
f"Human-in-the-loop detail for Task Instance with id
{task_instance_id} already exists.",
Review Comment:
Passing a string `detail` here will be normalized by
`ExecutionHTTPException`, but it will default `reason` to `"error"`. For
consistency with the PR goal (“structured detail payloads with reason and
message”), provide an explicit dict with a specific `reason` (e.g.,
`already_exists`) and the string as `message`.
```suggestion
detail={
"reason": "already_exists",
"message": (
f"Human-in-the-loop detail for Task Instance with id
{task_instance_id} already exists."
),
},
```
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/variables.py:
##########
@@ -95,6 +96,15 @@ def put_variable(
team_name: Annotated[str | None, Depends(get_team_name_dep)],
):
"""Set an Airflow Variable."""
+ if not variable_key:
+ raise ExecutionHTTPException(
+ status.HTTP_404_NOT_FOUND,
+ detail={
+ "reason": "not_found",
+ "message": "Not Found",
+ },
+ )
Review Comment:
`variable_key` is a path parameter, so an “empty string” value won’t match
the route (the segment can’t be empty). These branches are effectively
unreachable and also return `404 Not Found`, which is not an appropriate status
for an invalid/missing path parameter. Consider removing these checks; if you
want explicit validation, enforce it at the parameter level (e.g.,
`Path(min_length=1)`) so FastAPI returns a 422 validation error with the
normalized detail shape.
##########
airflow-core/src/airflow/api_fastapi/common/exceptions.py:
##########
@@ -122,4 +122,35 @@ def exception_handler(self, request: Request, exc:
DeserializationError):
)
+class ExecutionHTTPException(HTTPException):
+ """
+ HTTPException subclass used by Execution API.
+
+ Enforces consistent error response format containing `reason` and
`message` keys.
+ """
+
+ def __init__(
+ self,
+ status_code: int,
+ detail: dict[str, Any] | str | None = None,
+ headers: dict[str, str] | None = None,
+ ) -> None:
+ """Initialize with enforced dict format."""
+ if isinstance(detail, str) or detail is None:
+ detail = {
+ "reason": "error",
+ "message": detail or "An error occurred",
+ }
+ elif isinstance(detail, dict):
+ detail = dict(detail) # copy
+ if "reason" not in detail:
+ detail["reason"] = "error"
+ if "message" not in detail:
+ detail["message"] = "An error occurred"
+ else:
+ detail = {"reason": "error", "message": str(detail)}
+
+ super().__init__(status_code=status_code, detail=detail,
headers=headers)
Review Comment:
Introducing `ExecutionHTTPException` only normalizes errors for call sites
that explicitly use it. In this same module, some existing handlers (e.g.,
`DagErrorHandler`) still raise `HTTPException` with string `detail`, which will
continue producing mixed formats. To fully enforce consistency, either (1)
update Execution API error handlers to raise `ExecutionHTTPException`, or (2)
register a FastAPI exception handler for `HTTPException` under the Execution
API app/router that wraps/normalizes `exc.detail` into the `{reason, message}`
shape.
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/variables.py:
##########
@@ -112,4 +122,13 @@ def delete_variable(
team_name: Annotated[str | None, Depends(get_team_name_dep)],
):
"""Delete an Airflow Variable."""
+ if not variable_key:
+ raise ExecutionHTTPException(
+ status.HTTP_404_NOT_FOUND,
+ detail={
+ "reason": "not_found",
+ "message": "Not Found",
+ },
+ )
Review Comment:
`variable_key` is a path parameter, so an “empty string” value won’t match
the route (the segment can’t be empty). These branches are effectively
unreachable and also return `404 Not Found`, which is not an appropriate status
for an invalid/missing path parameter. Consider removing these checks; if you
want explicit validation, enforce it at the parameter level (e.g.,
`Path(min_length=1)`) so FastAPI returns a 422 validation error with the
normalized detail shape.
--
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]
