Improve dag error handling #49164

Yusin0903 · 2025-04-12T18:23:42Z

Improve error handling for DAG access in API endpoints

This PR improves error handling when accessing DAGs via API endpoints:

Add consistent error handling for DAG access across API endpoints
Handle ImportError and SyntaxError with 422 status code and clear error messages
Handle generic exceptions with 500 status code
Provide clear error messages for different failure scenarios
Update API documentation to include new error response codes
Add comprehensive tests for error handling scenarios

This change ensures users get clear feedback when DAGs fail to load
due to import errors, syntax errors, or other unexpected issues.

closes: #48960

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

boring-cyborg · 2025-04-12T18:23:46Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:

Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected]
Slack: https://s.apache.org/airflow-slack

airflow-core/src/airflow/api_fastapi/core_api/routes/public/assets.py

airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_error_handling.py

jason810496

Thanks for the PR!
I agree with @rawwar, and also leave nits for modularizing.

airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_run.py

pierrejeambrun

Thanks for the PR. A few adjustments needed before we can merge :)

airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_run.py

airflow-core/src/airflow/api_fastapi/core_api/routes/public/assets.py

airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_run.py

pierrejeambrun

The main problem of this is it will really handle only a precise deserialization errror => dag_id is missing from the encoded dag.

Any other undexpected error in the deserialization process, will just crash and not provide any specific feedback.

I think we shoul catch everything in SerializedDAG.deserialize_dag and raise based on the exception a new custom DeserializationError.

Then all those deserialization errors can be catch on the API side and provide a relevant feedback.

airflow-core/src/airflow/api/common/utils.py

airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_run.py

airflow-core/src/airflow/api/common/utils.py

airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_assets.py

pierrejeambrun · 2025-05-05T09:35:22Z

Don't hesitate to directly resolve comments you've addressed so we know easily what's left to focus on.

Yusin0903 · 2025-05-13T18:51:10Z

The main problem of this is it will really handle only a precise deserialization errror => dag_id is missing from the encoded dag.

Any other undexpected error in the deserialization process, will just crash and not provide any specific feedback.

I think we shoul catch everything in SerializedDAG.deserialize_dag and raise based on the exception a new custom DeserializationError.

Then all those deserialization errors can be catch on the API side and provide a relevant feedback.

Hi @pierrejeambrun,
Thanks for the review.
I’ve raised a custom DeserializationError and handled it in SerializedDAG.deserialize_dag.
I also noticed the new dependency DagBagDep, and I’ve merged it into get_dag_from_dag_bag.

pierrejeambrun

Nice, looking good. A few comments. Thanks.

airflow-core/src/airflow/api/common/utils.py

airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_run.py

airflow-core/src/airflow/api_fastapi/core_api/routes/public/assets.py

airflow-core/src/airflow/exceptions.py

airflow-core/src/airflow/serialization/serialized_objects.py

airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_assets.py

Yusin0903 · 2025-05-25T12:23:35Z

Hi @pierrejeambrun,
Thanks for the review and suggestion.
I have catched all the Runtime error in the DeserializationError and simplify the dag_bag.get_dag handle.

bugraoz93

Overall looks good, thanks! Small comments here and there :)

airflow-core/src/airflow/api_fastapi/core_api/routes/public/assets.py

airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dags.py

airflow-core/src/airflow/api_fastapi/core_api/routes/ui/assets.py

airflow-core/src/airflow/api_fastapi/core_api/routes/public/task_instances.py

airflow-core/src/airflow/serialization/serialized_objects.py

…8960)

uranusjr · 2025-05-27T01:51:40Z

airflow-core/src/airflow/api_fastapi/common/exceptions.py

 DatabaseErrorHandlers = [
    _UniqueConstraintErrorHandler(),
 ]
+
+DAGErrorHandlers = [
+    DAGErrorHandler(),
+]


Do we really need these to be separate? How about just have one list called ERROR_HANDLERS instead.

Or, do we really need the list at all? Why not just import the classes directly in app.py?

+1 unique list of ErrorHandlers seems more appropriate for now as there is really just one db handler.

uranusjr · 2025-05-27T01:54:46Z

airflow-core/src/airflow/serialization/serialized_objects.py

-
-        return dag
+            return dag
+        except (RuntimeError, ValueError, KeyError) as err:


Does RuntimeError here only intend to catch the case we raise ourselves? I’d rather avoid it if possible; it’s used too often for built-in errors that we may not want to mask.

I don't know, basically any unexpected error there is a deserialization error, and we prefer to wrap that into a DeserializationError (with the original stack trace), to show that to the users of the API, instead of having a plain ValueError/KeyError with a 500 (Default ValueError and KeyError cannot be handled globally by the api server exception handler).

Raising anything else not handled by the server will end up in 500 and the error buried in the stack trace.

uranusjr · 2025-05-27T01:55:38Z

airflow-core/src/airflow/serialization/serialized_objects.py

+        except:
+            raise


Suggested change

except:

raise

uranusjr · 2025-05-27T01:58:04Z

airflow-core/src/airflow/api_fastapi/common/exceptions.py

+    def exception_handler(self, request: Request, exc: DeserializationError):
+        """Handle DAG deserialization exceptions."""
+        raise HTTPException(
+            status_code=status.HTTP_400_BAD_REQUEST,


400 does not sound right to me. This is not something the client can resolve; a 500 would be more appropriate IMO.

+1. 400 'could work' maybe if we consider that a dag authorizing mistake causing the deserialization error and the user needs to operate on a the dag itself ?

@pierrejeambrun Just to confirm, should we still raise a 400 status code in this case?

lets try with a 500 as TP suggested.

I feel Airflow should catch authoring mistakes and prevent un-deserialisable content enter the database; in that sense 500 is more appropriate. 400 still does not feel appropriate even if the input is fixable since the code implies the user can get a successful result simply by tweaking the request parameters.

Yusin0903 requested review from ephraimbuddy, pierrejeambrun and rawwar as code owners April 12, 2025 18:23

boring-cyborg bot added the area:API Airflow's REST/HTTP API label Apr 12, 2025

Yusin0903 force-pushed the improve-dag-error-handling branch 3 times, most recently from 49162b4 to 49f4b74 Compare April 12, 2025 18:33

rawwar reviewed Apr 13, 2025

View reviewed changes

airflow-core/src/airflow/api_fastapi/core_api/routes/public/assets.py Outdated Show resolved Hide resolved

rawwar reviewed Apr 13, 2025

View reviewed changes

airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_error_handling.py Outdated Show resolved Hide resolved

jason810496 requested changes Apr 13, 2025

View reviewed changes

airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_run.py Outdated Show resolved Hide resolved

pierrejeambrun reviewed Apr 14, 2025

View reviewed changes

Yusin0903 force-pushed the improve-dag-error-handling branch from 49f4b74 to e296a5d Compare May 1, 2025 14:53

Yusin0903 requested review from bugraoz93 and shubhamraj-git as code owners May 1, 2025 14:53

pierrejeambrun added this to the Airflow 3.1.0 milestone May 5, 2025

pierrejeambrun reviewed May 5, 2025

View reviewed changes

Yusin0903 force-pushed the improve-dag-error-handling branch from e296a5d to a62f542 Compare May 13, 2025 18:33

Yusin0903 requested review from ashb and bolkedebruin as code owners May 13, 2025 18:33

Yusin0903 force-pushed the improve-dag-error-handling branch 2 times, most recently from df76944 to f1e2875 Compare May 13, 2025 18:47

pierrejeambrun reviewed May 14, 2025

View reviewed changes

Yusin0903 force-pushed the improve-dag-error-handling branch 3 times, most recently from c933083 to 73dad08 Compare May 25, 2025 12:18

bugraoz93 reviewed May 25, 2025

View reviewed changes

airflow-core/src/airflow/api_fastapi/core_api/routes/public/assets.py Outdated Show resolved Hide resolved

airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dags.py Outdated Show resolved Hide resolved

pierrejeambrun reviewed May 26, 2025

View reviewed changes

Yusin0903 added 2 commits May 27, 2025 01:56

fix: Improve error handling for DAG access in API endpoints (apache#4…

73f6a64

…8960)

Add tests for DAG access error handling (apache#48960)

a66859d

Yusin0903 force-pushed the improve-dag-error-handling branch from 73dad08 to a66859d Compare May 26, 2025 17:57

uranusjr reviewed May 27, 2025

View reviewed changes

Improve dag error handling #49164

Are you sure you want to change the base?

Improve dag error handling #49164

Uh oh!

Conversation

Yusin0903 commented Apr 12, 2025

Uh oh!

boring-cyborg bot commented Apr 12, 2025

Uh oh!

Uh oh!

Uh oh!

jason810496 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pierrejeambrun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pierrejeambrun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pierrejeambrun commented May 5, 2025

Uh oh!

Yusin0903 commented May 13, 2025

Uh oh!

pierrejeambrun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Yusin0903 commented May 25, 2025

Uh oh!

bugraoz93 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uranusjr May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pierrejeambrun left a comment •

edited

Loading

uranusjr May 27, 2025 •

edited

Loading