Copilot commented on code in PR #64130:
URL: https://github.com/apache/airflow/pull/64130#discussion_r3025336523
##########
airflow-core/src/airflow/serialization/definitions/param.py:
##########
@@ -22,6 +22,7 @@
import copy
from typing import TYPE_CHECKING, Any, Literal
+from airflow.sdk.exceptions import ParamValidationError
Review Comment:
This core module now imports ParamValidationError directly from
airflow.sdk.exceptions. In configurations where airflow.sdk is intentionally
not installed (see airflow/exceptions.py’s _AIRFLOW__AS_LIBRARY fallback), this
import will raise ModuleNotFoundError at import time. Prefer importing
ParamValidationError from airflow.exceptions here so the existing fallback
mechanism can be used.
```suggestion
from airflow.exceptions import ParamValidationError
```
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_run.py:
##########
@@ -496,15 +497,15 @@ def trigger_dag_run(
partition_key=params["partition_key"],
session=session,
)
-
- dag_run_note = body.note
- if dag_run_note:
- current_user_id = user.get_id()
- dag_run.note = (dag_run_note, current_user_id)
- return dag_run
- except ValueError as e:
+ except ParamValidationError as e:
raise HTTPException(status.HTTP_400_BAD_REQUEST, str(e))
Review Comment:
Catching only ParamValidationError here means other ValueErrors raised by
SerializedDAG.create_dagrun() (e.g. invalid/forbidden run_id patterns or
data_interval/logical_date validation in
airflow/serialization/definitions/dag.py:536-567) will now propagate as 500s.
Those are client input/validation errors and previously surfaced as 400;
consider raising ParamValidationError for those validation failures inside
create_dagrun (preferred), or mapping those specific validation ValueErrors to
HTTP 400 here so clients don’t see an Internal Server Error for bad input.
```suggestion
except (ParamValidationError, ValueError) as e:
raise HTTPException(status.HTTP_400_BAD_REQUEST, str(e)) from e
```
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dag_run.py:
##########
@@ -1788,17 +1788,6 @@ def test_post_dag_runs_with_empty_payload(self,
test_client):
},
]
-
@mock.patch("airflow.serialization.definitions.dag.SerializedDAG.create_dagrun")
- def test_dagrun_creation_exception_is_handled(self, mock_create_dagrun,
test_client):
- now = timezone.utcnow().isoformat()
- error_message = "Encountered Error"
-
- mock_create_dagrun.side_effect = ValueError(error_message)
-
- response = test_client.post(f"/dags/{DAG1_ID}/dagRuns",
json={"logical_date": now})
- assert response.status_code == 400
- assert response.json() == {"detail": error_message}
-
def test_should_respond_404_if_a_dag_is_inactive(self, test_client,
session, testing_dag_bundle):
Review Comment:
The test covering “ValueError from create_dagrun() becomes a 400” was
removed, but there’s no replacement test asserting the new intended behavior
that non-validation exceptions from create_dagrun() propagate as 500s (so they
are visible in server error logs/metrics). Add a regression test that forces
create_dagrun() to raise a non-ParamValidationError (e.g., a generic
ValueError/RuntimeError) and asserts the endpoint returns 500.
##########
airflow-core/src/airflow/exceptions.py:
##########
@@ -37,6 +37,7 @@
AirflowOptionalProviderFeatureException as
AirflowOptionalProviderFeatureException,
AirflowRescheduleException as AirflowRescheduleException,
AirflowTimetableInvalid as AirflowTimetableInvalid,
+ ParamValidationError as ParamValidationError,
Review Comment:
ParamValidationError is imported from airflow.sdk.exceptions above, but the
ModuleNotFoundError fallback block doesn’t define a ParamValidationError
replacement. When airflow.sdk isn’t installed (as noted by the fallback
comment), `from airflow.exceptions import ParamValidationError` will fail; add
a fallback ParamValidationError class (mirroring SDK semantics, e.g.
subclassing AirflowException) in the except block to keep this import usable in
library mode.
--
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]
