From ff0cb0d363b5262745f5fae06599ffab98e0e305 Mon Sep 17 00:00:00 2001 From: kevinhongzl Date: Sat, 7 Jun 2025 01:59:06 +0800 Subject: [PATCH 1/8] feat: add a config setting to expose stacktraces --- .../src/airflow/api_fastapi/common/exceptions.py | 11 +++++++++++ airflow-core/src/airflow/config_templates/config.yml | 6 ++++++ 2 files changed, 17 insertions(+) diff --git a/airflow-core/src/airflow/api_fastapi/common/exceptions.py b/airflow-core/src/airflow/api_fastapi/common/exceptions.py index 061eec55d3d84..b5c3b1541c160 100644 --- a/airflow-core/src/airflow/api_fastapi/common/exceptions.py +++ b/airflow-core/src/airflow/api_fastapi/common/exceptions.py @@ -17,6 +17,7 @@ from __future__ import annotations +import traceback from abc import ABC, abstractmethod from enum import Enum from typing import Generic, TypeVar @@ -24,6 +25,8 @@ from fastapi import HTTPException, Request, status from sqlalchemy.exc import IntegrityError +from airflow.configuration import conf + T = TypeVar("T", bound=Exception) @@ -61,12 +64,20 @@ def __init__(self): def exception_handler(self, request: Request, exc: IntegrityError): """Handle IntegrityError exception.""" if self._is_dialect_matched(exc): + if conf.get("api", "expose_stacktrace") == "True": + stacktrace = "\n" + for tb in traceback.format_tb(exc.__traceback__): + stacktrace += tb + else: + stacktrace = "Database Integrity Error - Check logs for more details." + raise HTTPException( status_code=status.HTTP_409_CONFLICT, detail={ "reason": "Unique constraint violation", "statement": str(exc.statement), "orig_error": str(exc.orig), + "stacktrace": stacktrace, }, ) diff --git a/airflow-core/src/airflow/config_templates/config.yml b/airflow-core/src/airflow/config_templates/config.yml index bda494eed5dcc..30c6037da4a92 100644 --- a/airflow-core/src/airflow/config_templates/config.yml +++ b/airflow-core/src/airflow/config_templates/config.yml @@ -1321,6 +1321,12 @@ api: type: string example: ~ default: "False" + expose_stacktrace: + description: Expose stacktrace in the web server + version_added: ~ + type: string + example: ~ + default: "False" base_url: description: | The base url of the API server. Airflow cannot guess what domain or CNAME you are using. From 4914db05585227637086945c8495941d3ed83ab8 Mon Sep 17 00:00:00 2001 From: kevinhongzl Date: Sun, 8 Jun 2025 03:39:23 +0800 Subject: [PATCH 2/8] fix: remove the starting newline to simplify the traceback info --- airflow-core/src/airflow/api_fastapi/common/exceptions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/airflow-core/src/airflow/api_fastapi/common/exceptions.py b/airflow-core/src/airflow/api_fastapi/common/exceptions.py index b5c3b1541c160..a087bcb55b6bd 100644 --- a/airflow-core/src/airflow/api_fastapi/common/exceptions.py +++ b/airflow-core/src/airflow/api_fastapi/common/exceptions.py @@ -65,7 +65,7 @@ def exception_handler(self, request: Request, exc: IntegrityError): """Handle IntegrityError exception.""" if self._is_dialect_matched(exc): if conf.get("api", "expose_stacktrace") == "True": - stacktrace = "\n" + stacktrace = "" for tb in traceback.format_tb(exc.__traceback__): stacktrace += tb else: From 4507d5429d9c9d09f9d421affa5c41d7afd63e78 Mon Sep 17 00:00:00 2001 From: kevinhongzl Date: Sun, 8 Jun 2025 05:43:57 +0800 Subject: [PATCH 3/8] test: update tests to align with the config change --- .../tests/unit/api_fastapi/common/test_exceptions.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/airflow-core/tests/unit/api_fastapi/common/test_exceptions.py b/airflow-core/tests/unit/api_fastapi/common/test_exceptions.py index bed77a67a2723..afb7d4642973c 100644 --- a/airflow-core/tests/unit/api_fastapi/common/test_exceptions.py +++ b/airflow-core/tests/unit/api_fastapi/common/test_exceptions.py @@ -26,6 +26,7 @@ from airflow.utils.session import provide_session from airflow.utils.state import DagRunState +from tests_common.test_utils.config import conf_vars from tests_common.test_utils.db import clear_db_connections, clear_db_dags, clear_db_pools, clear_db_runs pytestmark = pytest.mark.db_test @@ -109,6 +110,7 @@ def teardown_method(self) -> None: "reason": "Unique constraint violation", "statement": "INSERT INTO slot_pool (pool, slots, description, include_deferred) VALUES (?, ?, ?, ?)", "orig_error": "UNIQUE constraint failed: slot_pool.pool", + "stacktrace": "Database Integrity Error - Check logs for more details.", }, ), HTTPException( @@ -117,6 +119,7 @@ def teardown_method(self) -> None: "reason": "Unique constraint violation", "statement": "INSERT INTO slot_pool (pool, slots, description, include_deferred) VALUES (%s, %s, %s, %s)", "orig_error": "(1062, \"Duplicate entry 'test_pool' for key 'slot_pool.slot_pool_pool_uq'\")", + "stacktrace": "Database Integrity Error - Check logs for more details.", }, ), HTTPException( @@ -125,6 +128,7 @@ def teardown_method(self) -> None: "reason": "Unique constraint violation", "statement": "INSERT INTO slot_pool (pool, slots, description, include_deferred) VALUES (%(pool)s, %(slots)s, %(description)s, %(include_deferred)s) RETURNING slot_pool.id", "orig_error": 'duplicate key value violates unique constraint "slot_pool_pool_uq"\nDETAIL: Key (pool)=(test_pool) already exists.\n', + "stacktrace": "Database Integrity Error - Check logs for more details.", }, ), ], @@ -135,6 +139,7 @@ def teardown_method(self) -> None: "reason": "Unique constraint violation", "statement": 'INSERT INTO variable ("key", val, description, is_encrypted) VALUES (?, ?, ?, ?)', "orig_error": "UNIQUE constraint failed: variable.key", + "stacktrace": "Database Integrity Error - Check logs for more details.", }, ), HTTPException( @@ -143,6 +148,7 @@ def teardown_method(self) -> None: "reason": "Unique constraint violation", "statement": "INSERT INTO variable (`key`, val, description, is_encrypted) VALUES (%s, %s, %s, %s)", "orig_error": "(1062, \"Duplicate entry 'test_key' for key 'variable.variable_key_uq'\")", + "stacktrace": "Database Integrity Error - Check logs for more details.", }, ), HTTPException( @@ -151,12 +157,14 @@ def teardown_method(self) -> None: "reason": "Unique constraint violation", "statement": "INSERT INTO variable (key, val, description, is_encrypted) VALUES (%(key)s, %(val)s, %(description)s, %(is_encrypted)s) RETURNING variable.id", "orig_error": 'duplicate key value violates unique constraint "variable_key_uq"\nDETAIL: Key (key)=(test_key) already exists.\n', + "stacktrace": "Database Integrity Error - Check logs for more details.", }, ), ], ], ), ) + @conf_vars({("api", "expose_stacktrace"): "False"}) @provide_session def test_handle_single_column_unique_constraint_error(self, session, table, expected_exception) -> None: # Take Pool and Variable tables as test cases @@ -188,6 +196,7 @@ def test_handle_single_column_unique_constraint_error(self, session, table, expe "reason": "Unique constraint violation", "statement": "INSERT INTO dag_run (dag_id, queued_at, logical_date, start_date, end_date, state, run_id, creating_job_id, run_type, triggered_by, conf, data_interval_start, data_interval_end, run_after, last_scheduling_decision, log_template_id, updated_at, clear_number, backfill_id, bundle_version, scheduled_by_job_id, context_carrier, created_dag_version_id) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, (SELECT max(log_template.id) AS max_1 \nFROM log_template), ?, ?, ?, ?, ?, ?, ?)", "orig_error": "UNIQUE constraint failed: dag_run.dag_id, dag_run.run_id", + "stacktrace": "Database Integrity Error - Check logs for more details.", }, ), HTTPException( @@ -196,6 +205,7 @@ def test_handle_single_column_unique_constraint_error(self, session, table, expe "reason": "Unique constraint violation", "statement": "INSERT INTO dag_run (dag_id, queued_at, logical_date, start_date, end_date, state, run_id, creating_job_id, run_type, triggered_by, conf, data_interval_start, data_interval_end, run_after, last_scheduling_decision, log_template_id, updated_at, clear_number, backfill_id, bundle_version, scheduled_by_job_id, context_carrier, created_dag_version_id) VALUES (%s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, (SELECT max(log_template.id) AS max_1 \nFROM log_template), %s, %s, %s, %s, %s, %s, %s)", "orig_error": "(1062, \"Duplicate entry 'test_dag_id-test_run_id' for key 'dag_run.dag_run_dag_id_run_id_key'\")", + "stacktrace": "Database Integrity Error - Check logs for more details.", }, ), HTTPException( @@ -204,12 +214,14 @@ def test_handle_single_column_unique_constraint_error(self, session, table, expe "reason": "Unique constraint violation", "statement": "INSERT INTO dag_run (dag_id, queued_at, logical_date, start_date, end_date, state, run_id, creating_job_id, run_type, triggered_by, conf, data_interval_start, data_interval_end, run_after, last_scheduling_decision, log_template_id, updated_at, clear_number, backfill_id, bundle_version, scheduled_by_job_id, context_carrier, created_dag_version_id) VALUES (%(dag_id)s, %(queued_at)s, %(logical_date)s, %(start_date)s, %(end_date)s, %(state)s, %(run_id)s, %(creating_job_id)s, %(run_type)s, %(triggered_by)s, %(conf)s, %(data_interval_start)s, %(data_interval_end)s, %(run_after)s, %(last_scheduling_decision)s, (SELECT max(log_template.id) AS max_1 \nFROM log_template), %(updated_at)s, %(clear_number)s, %(backfill_id)s, %(bundle_version)s, %(scheduled_by_job_id)s, %(context_carrier)s, %(created_dag_version_id)s) RETURNING dag_run.id", "orig_error": 'duplicate key value violates unique constraint "dag_run_dag_id_run_id_key"\nDETAIL: Key (dag_id, run_id)=(test_dag_id, test_run_id) already exists.\n', + "stacktrace": "Database Integrity Error - Check logs for more details.", }, ), ], ], ), ) + @conf_vars({("api", "expose_stacktrace"): "False"}) @provide_session def test_handle_multiple_columns_unique_constraint_error( self, session, table, expected_exception From 6966a430bbef0ec0663cf885700b1b4f74133efe Mon Sep 17 00:00:00 2001 From: kevinhongzl Date: Sun, 8 Jun 2025 09:10:36 +0800 Subject: [PATCH 4/8] feat: add exception id for better correlation between ui messages and log entries --- .../airflow/api_fastapi/common/exceptions.py | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/airflow-core/src/airflow/api_fastapi/common/exceptions.py b/airflow-core/src/airflow/api_fastapi/common/exceptions.py index a087bcb55b6bd..a99d3abb566b4 100644 --- a/airflow-core/src/airflow/api_fastapi/common/exceptions.py +++ b/airflow-core/src/airflow/api_fastapi/common/exceptions.py @@ -17,6 +17,7 @@ from __future__ import annotations +import logging import traceback from abc import ABC, abstractmethod from enum import Enum @@ -29,6 +30,8 @@ T = TypeVar("T", bound=Exception) +log = logging.getLogger(__name__) + class BaseErrorHandler(Generic[T], ABC): """Base class for error handlers.""" @@ -61,15 +64,23 @@ def __init__(self): super().__init__(IntegrityError) self.dialect: _DatabaseDialect.value | None = None - def exception_handler(self, request: Request, exc: IntegrityError): + def exception_handler(self, request: Request, exc: IntegrityError, exc_id: str | None = None): """Handle IntegrityError exception.""" if self._is_dialect_matched(exc): + exception_id = hex(id(exc)) if exc_id is None else exc_id + stacktrace = "" + for tb in traceback.format_tb(exc.__traceback__): + stacktrace += tb + + log_message = f"Error with id {exception_id}\n{stacktrace}" + log.error(log_message) if conf.get("api", "expose_stacktrace") == "True": - stacktrace = "" - for tb in traceback.format_tb(exc.__traceback__): - stacktrace += tb + message = log_message else: - stacktrace = "Database Integrity Error - Check logs for more details." + message = ( + "Serious error when handling your request. Check logs for more details - " + f"you will find it in api server when you look for ID {exception_id}" + ) raise HTTPException( status_code=status.HTTP_409_CONFLICT, @@ -77,7 +88,7 @@ def exception_handler(self, request: Request, exc: IntegrityError): "reason": "Unique constraint violation", "statement": str(exc.statement), "orig_error": str(exc.orig), - "stacktrace": stacktrace, + "message": message, }, ) From f9794464d5d4c28d12f762d0c8a5753114dde540 Mon Sep 17 00:00:00 2001 From: kevinhongzl Date: Sun, 8 Jun 2025 09:59:36 +0800 Subject: [PATCH 5/8] test: update tests --- .../api_fastapi/common/test_exceptions.py | 35 +++++++++++++------ .../routes/public/test_connections.py | 2 +- .../core_api/routes/public/test_dag_run.py | 9 ++--- .../core_api/routes/public/test_pools.py | 2 +- 4 files changed, 28 insertions(+), 20 deletions(-) diff --git a/airflow-core/tests/unit/api_fastapi/common/test_exceptions.py b/airflow-core/tests/unit/api_fastapi/common/test_exceptions.py index afb7d4642973c..09fafc5b3d4f9 100644 --- a/airflow-core/tests/unit/api_fastapi/common/test_exceptions.py +++ b/airflow-core/tests/unit/api_fastapi/common/test_exceptions.py @@ -51,6 +51,11 @@ "reason": f"Test for {_DatabaseDialect.POSTGRES.value} only", }, ] +EXC_ID = "0x123e3aade" +MESSAGE = ( + "Serious error when handling your request. Check logs for more details - " + f"you will find it in api server when you look for ID {EXC_ID}" +) def generate_test_cases_parametrize( @@ -110,7 +115,7 @@ def teardown_method(self) -> None: "reason": "Unique constraint violation", "statement": "INSERT INTO slot_pool (pool, slots, description, include_deferred) VALUES (?, ?, ?, ?)", "orig_error": "UNIQUE constraint failed: slot_pool.pool", - "stacktrace": "Database Integrity Error - Check logs for more details.", + "message": MESSAGE, }, ), HTTPException( @@ -119,7 +124,7 @@ def teardown_method(self) -> None: "reason": "Unique constraint violation", "statement": "INSERT INTO slot_pool (pool, slots, description, include_deferred) VALUES (%s, %s, %s, %s)", "orig_error": "(1062, \"Duplicate entry 'test_pool' for key 'slot_pool.slot_pool_pool_uq'\")", - "stacktrace": "Database Integrity Error - Check logs for more details.", + "message": MESSAGE, }, ), HTTPException( @@ -128,7 +133,7 @@ def teardown_method(self) -> None: "reason": "Unique constraint violation", "statement": "INSERT INTO slot_pool (pool, slots, description, include_deferred) VALUES (%(pool)s, %(slots)s, %(description)s, %(include_deferred)s) RETURNING slot_pool.id", "orig_error": 'duplicate key value violates unique constraint "slot_pool_pool_uq"\nDETAIL: Key (pool)=(test_pool) already exists.\n', - "stacktrace": "Database Integrity Error - Check logs for more details.", + "message": MESSAGE, }, ), ], @@ -139,7 +144,7 @@ def teardown_method(self) -> None: "reason": "Unique constraint violation", "statement": 'INSERT INTO variable ("key", val, description, is_encrypted) VALUES (?, ?, ?, ?)', "orig_error": "UNIQUE constraint failed: variable.key", - "stacktrace": "Database Integrity Error - Check logs for more details.", + "message": MESSAGE, }, ), HTTPException( @@ -148,7 +153,7 @@ def teardown_method(self) -> None: "reason": "Unique constraint violation", "statement": "INSERT INTO variable (`key`, val, description, is_encrypted) VALUES (%s, %s, %s, %s)", "orig_error": "(1062, \"Duplicate entry 'test_key' for key 'variable.variable_key_uq'\")", - "stacktrace": "Database Integrity Error - Check logs for more details.", + "message": MESSAGE, }, ), HTTPException( @@ -157,7 +162,7 @@ def teardown_method(self) -> None: "reason": "Unique constraint violation", "statement": "INSERT INTO variable (key, val, description, is_encrypted) VALUES (%(key)s, %(val)s, %(description)s, %(is_encrypted)s) RETURNING variable.id", "orig_error": 'duplicate key value violates unique constraint "variable_key_uq"\nDETAIL: Key (key)=(test_key) already exists.\n', - "stacktrace": "Database Integrity Error - Check logs for more details.", + "message": MESSAGE, }, ), ], @@ -179,7 +184,11 @@ def test_handle_single_column_unique_constraint_error(self, session, table, expe session.commit() with pytest.raises(HTTPException) as exeinfo_response_error: - self.unique_constraint_error_handler.exception_handler(None, exeinfo_integrity_error.value) # type: ignore + self.unique_constraint_error_handler.exception_handler( + None, # type: ignore + exeinfo_integrity_error.value, + EXC_ID, + ) assert exeinfo_response_error.value.status_code == expected_exception.status_code assert exeinfo_response_error.value.detail == expected_exception.detail @@ -196,7 +205,7 @@ def test_handle_single_column_unique_constraint_error(self, session, table, expe "reason": "Unique constraint violation", "statement": "INSERT INTO dag_run (dag_id, queued_at, logical_date, start_date, end_date, state, run_id, creating_job_id, run_type, triggered_by, conf, data_interval_start, data_interval_end, run_after, last_scheduling_decision, log_template_id, updated_at, clear_number, backfill_id, bundle_version, scheduled_by_job_id, context_carrier, created_dag_version_id) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, (SELECT max(log_template.id) AS max_1 \nFROM log_template), ?, ?, ?, ?, ?, ?, ?)", "orig_error": "UNIQUE constraint failed: dag_run.dag_id, dag_run.run_id", - "stacktrace": "Database Integrity Error - Check logs for more details.", + "message": MESSAGE, }, ), HTTPException( @@ -205,7 +214,7 @@ def test_handle_single_column_unique_constraint_error(self, session, table, expe "reason": "Unique constraint violation", "statement": "INSERT INTO dag_run (dag_id, queued_at, logical_date, start_date, end_date, state, run_id, creating_job_id, run_type, triggered_by, conf, data_interval_start, data_interval_end, run_after, last_scheduling_decision, log_template_id, updated_at, clear_number, backfill_id, bundle_version, scheduled_by_job_id, context_carrier, created_dag_version_id) VALUES (%s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, (SELECT max(log_template.id) AS max_1 \nFROM log_template), %s, %s, %s, %s, %s, %s, %s)", "orig_error": "(1062, \"Duplicate entry 'test_dag_id-test_run_id' for key 'dag_run.dag_run_dag_id_run_id_key'\")", - "stacktrace": "Database Integrity Error - Check logs for more details.", + "message": MESSAGE, }, ), HTTPException( @@ -214,7 +223,7 @@ def test_handle_single_column_unique_constraint_error(self, session, table, expe "reason": "Unique constraint violation", "statement": "INSERT INTO dag_run (dag_id, queued_at, logical_date, start_date, end_date, state, run_id, creating_job_id, run_type, triggered_by, conf, data_interval_start, data_interval_end, run_after, last_scheduling_decision, log_template_id, updated_at, clear_number, backfill_id, bundle_version, scheduled_by_job_id, context_carrier, created_dag_version_id) VALUES (%(dag_id)s, %(queued_at)s, %(logical_date)s, %(start_date)s, %(end_date)s, %(state)s, %(run_id)s, %(creating_job_id)s, %(run_type)s, %(triggered_by)s, %(conf)s, %(data_interval_start)s, %(data_interval_end)s, %(run_after)s, %(last_scheduling_decision)s, (SELECT max(log_template.id) AS max_1 \nFROM log_template), %(updated_at)s, %(clear_number)s, %(backfill_id)s, %(bundle_version)s, %(scheduled_by_job_id)s, %(context_carrier)s, %(created_dag_version_id)s) RETURNING dag_run.id", "orig_error": 'duplicate key value violates unique constraint "dag_run_dag_id_run_id_key"\nDETAIL: Key (dag_id, run_id)=(test_dag_id, test_run_id) already exists.\n', - "stacktrace": "Database Integrity Error - Check logs for more details.", + "message": MESSAGE, }, ), ], @@ -242,7 +251,11 @@ def test_handle_multiple_columns_unique_constraint_error( session.commit() with pytest.raises(HTTPException) as exeinfo_response_error: - self.unique_constraint_error_handler.exception_handler(None, exeinfo_integrity_error.value) # type: ignore + self.unique_constraint_error_handler.exception_handler( + None, # type: ignore + exeinfo_integrity_error.value, + EXC_ID, + ) assert exeinfo_response_error.value.status_code == expected_exception.status_code assert exeinfo_response_error.value.detail == expected_exception.detail diff --git a/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_connections.py b/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_connections.py index 6ddf188935249..a4184e29e8da3 100644 --- a/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_connections.py +++ b/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_connections.py @@ -306,7 +306,7 @@ def test_post_should_respond_already_exist(self, test_client, body): assert response.status_code == 409 response_json = response.json() assert "detail" in response_json - assert list(response_json["detail"].keys()) == ["reason", "statement", "orig_error"] + assert list(response_json["detail"].keys()) == ["reason", "statement", "orig_error", "message"] @pytest.mark.enable_redact @pytest.mark.parametrize( diff --git a/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dag_run.py b/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dag_run.py index baa7e503c83ed..718e76784bf88 100644 --- a/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dag_run.py +++ b/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dag_run.py @@ -38,12 +38,7 @@ from airflow.utils.types import DagRunTriggeredByType, DagRunType from tests_common.test_utils.api_fastapi import _check_dag_run_note, _check_last_log -from tests_common.test_utils.db import ( - clear_db_dags, - clear_db_logs, - clear_db_runs, - clear_db_serialized_dags, -) +from tests_common.test_utils.db import clear_db_dags, clear_db_logs, clear_db_runs, clear_db_serialized_dags from tests_common.test_utils.format_datetime import from_datetime_to_zulu, from_datetime_to_zulu_without_ms if TYPE_CHECKING: @@ -1577,7 +1572,7 @@ def test_response_409(self, test_client): assert response.status_code == 409 response_json = response.json() assert "detail" in response_json - assert list(response_json["detail"].keys()) == ["reason", "statement", "orig_error"] + assert list(response_json["detail"].keys()) == ["reason", "statement", "orig_error", "message"] @pytest.mark.usefixtures("configure_git_connection_for_dag_bundle") def test_should_respond_200_with_null_logical_date(self, test_client): diff --git a/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py b/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py index 8f6ed1a7019be..2d0700ea5d1be 100644 --- a/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py +++ b/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py @@ -417,7 +417,7 @@ def test_should_response_409( else: response_json = response.json() assert "detail" in response_json - assert list(response_json["detail"].keys()) == ["reason", "statement", "orig_error"] + assert list(response_json["detail"].keys()) == ["reason", "statement", "orig_error", "message"] assert session.query(Pool).count() == n_pools + 1 From 9e7b902e5c3f56ce6042f5cf610b118ae46ceb49 Mon Sep 17 00:00:00 2001 From: kevinhongzl Date: Mon, 9 Jun 2025 21:44:30 +0800 Subject: [PATCH 6/8] fix: use random string as exception id instead of python object id --- airflow-core/src/airflow/api_fastapi/common/exceptions.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/airflow-core/src/airflow/api_fastapi/common/exceptions.py b/airflow-core/src/airflow/api_fastapi/common/exceptions.py index a99d3abb566b4..9aaeab7ef3d26 100644 --- a/airflow-core/src/airflow/api_fastapi/common/exceptions.py +++ b/airflow-core/src/airflow/api_fastapi/common/exceptions.py @@ -27,6 +27,7 @@ from sqlalchemy.exc import IntegrityError from airflow.configuration import conf +from airflow.utils.strings import get_random_string T = TypeVar("T", bound=Exception) @@ -67,7 +68,7 @@ def __init__(self): def exception_handler(self, request: Request, exc: IntegrityError, exc_id: str | None = None): """Handle IntegrityError exception.""" if self._is_dialect_matched(exc): - exception_id = hex(id(exc)) if exc_id is None else exc_id + exception_id = get_random_string() if exc_id is None else exc_id stacktrace = "" for tb in traceback.format_tb(exc.__traceback__): stacktrace += tb From 9b0bcdb46379d944322d1f0bb554940be5e7675a Mon Sep 17 00:00:00 2001 From: kevinhongzl Date: Mon, 9 Jun 2025 23:14:43 +0800 Subject: [PATCH 7/8] fix: update tests with patched random strings --- .../airflow/api_fastapi/common/exceptions.py | 4 +-- .../api_fastapi/common/test_exceptions.py | 30 ++++++++++--------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/airflow-core/src/airflow/api_fastapi/common/exceptions.py b/airflow-core/src/airflow/api_fastapi/common/exceptions.py index 9aaeab7ef3d26..39909b7a46395 100644 --- a/airflow-core/src/airflow/api_fastapi/common/exceptions.py +++ b/airflow-core/src/airflow/api_fastapi/common/exceptions.py @@ -65,10 +65,10 @@ def __init__(self): super().__init__(IntegrityError) self.dialect: _DatabaseDialect.value | None = None - def exception_handler(self, request: Request, exc: IntegrityError, exc_id: str | None = None): + def exception_handler(self, request: Request, exc: IntegrityError): """Handle IntegrityError exception.""" if self._is_dialect_matched(exc): - exception_id = get_random_string() if exc_id is None else exc_id + exception_id = get_random_string() stacktrace = "" for tb in traceback.format_tb(exc.__traceback__): stacktrace += tb diff --git a/airflow-core/tests/unit/api_fastapi/common/test_exceptions.py b/airflow-core/tests/unit/api_fastapi/common/test_exceptions.py index 09fafc5b3d4f9..ba881c518fc14 100644 --- a/airflow-core/tests/unit/api_fastapi/common/test_exceptions.py +++ b/airflow-core/tests/unit/api_fastapi/common/test_exceptions.py @@ -51,10 +51,10 @@ "reason": f"Test for {_DatabaseDialect.POSTGRES.value} only", }, ] -EXC_ID = "0x123e3aade" +MOCKED_ID = "TgVcT3QW" MESSAGE = ( "Serious error when handling your request. Check logs for more details - " - f"you will find it in api server when you look for ID {EXC_ID}" + f"you will find it in api server when you look for ID {MOCKED_ID}" ) @@ -171,7 +171,9 @@ def teardown_method(self) -> None: ) @conf_vars({("api", "expose_stacktrace"): "False"}) @provide_session - def test_handle_single_column_unique_constraint_error(self, session, table, expected_exception) -> None: + def test_handle_single_column_unique_constraint_error( + self, session, table, expected_exception, monkeypatch + ) -> None: # Take Pool and Variable tables as test cases if table == "Pool": session.add(Pool(pool=TEST_POOL, slots=1, description="test pool", include_deferred=False)) @@ -183,12 +185,12 @@ def test_handle_single_column_unique_constraint_error(self, session, table, expe with pytest.raises(IntegrityError) as exeinfo_integrity_error: session.commit() + monkeypatch.setattr( + "airflow.api_fastapi.common.exceptions.get_random_string", + lambda length=None, choices=None: MOCKED_ID, + ) with pytest.raises(HTTPException) as exeinfo_response_error: - self.unique_constraint_error_handler.exception_handler( - None, # type: ignore - exeinfo_integrity_error.value, - EXC_ID, - ) + self.unique_constraint_error_handler.exception_handler(None, exeinfo_integrity_error.value) # type: ignore assert exeinfo_response_error.value.status_code == expected_exception.status_code assert exeinfo_response_error.value.detail == expected_exception.detail @@ -233,7 +235,7 @@ def test_handle_single_column_unique_constraint_error(self, session, table, expe @conf_vars({("api", "expose_stacktrace"): "False"}) @provide_session def test_handle_multiple_columns_unique_constraint_error( - self, session, table, expected_exception + self, session, table, expected_exception, monkeypatch ) -> None: if table == "DagRun": session.add( @@ -250,12 +252,12 @@ def test_handle_multiple_columns_unique_constraint_error( with pytest.raises(IntegrityError) as exeinfo_integrity_error: session.commit() + monkeypatch.setattr( + "airflow.api_fastapi.common.exceptions.get_random_string", + lambda length=None, choices=None: MOCKED_ID, + ) with pytest.raises(HTTPException) as exeinfo_response_error: - self.unique_constraint_error_handler.exception_handler( - None, # type: ignore - exeinfo_integrity_error.value, - EXC_ID, - ) + self.unique_constraint_error_handler.exception_handler(None, exeinfo_integrity_error.value) # type: ignore assert exeinfo_response_error.value.status_code == expected_exception.status_code assert exeinfo_response_error.value.detail == expected_exception.detail From 51d59bb84f4e2ca292b634a5d069e82c62ee99cc Mon Sep 17 00:00:00 2001 From: kevinhongzl Date: Tue, 10 Jun 2025 21:32:00 +0800 Subject: [PATCH 8/8] test: use patch fixtures in tests to prevent side effects --- .../api_fastapi/common/test_exceptions.py | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/airflow-core/tests/unit/api_fastapi/common/test_exceptions.py b/airflow-core/tests/unit/api_fastapi/common/test_exceptions.py index ba881c518fc14..b5136310611e0 100644 --- a/airflow-core/tests/unit/api_fastapi/common/test_exceptions.py +++ b/airflow-core/tests/unit/api_fastapi/common/test_exceptions.py @@ -16,6 +16,8 @@ # under the License. from __future__ import annotations +from unittest.mock import patch + import pytest from fastapi import HTTPException, status from sqlalchemy.exc import IntegrityError @@ -169,10 +171,15 @@ def teardown_method(self) -> None: ], ), ) + @patch("airflow.api_fastapi.common.exceptions.get_random_string", return_value=MOCKED_ID) @conf_vars({("api", "expose_stacktrace"): "False"}) @provide_session def test_handle_single_column_unique_constraint_error( - self, session, table, expected_exception, monkeypatch + self, + mock_get_random_string, + session, + table, + expected_exception, ) -> None: # Take Pool and Variable tables as test cases if table == "Pool": @@ -185,10 +192,6 @@ def test_handle_single_column_unique_constraint_error( with pytest.raises(IntegrityError) as exeinfo_integrity_error: session.commit() - monkeypatch.setattr( - "airflow.api_fastapi.common.exceptions.get_random_string", - lambda length=None, choices=None: MOCKED_ID, - ) with pytest.raises(HTTPException) as exeinfo_response_error: self.unique_constraint_error_handler.exception_handler(None, exeinfo_integrity_error.value) # type: ignore @@ -232,10 +235,15 @@ def test_handle_single_column_unique_constraint_error( ], ), ) + @patch("airflow.api_fastapi.common.exceptions.get_random_string", return_value=MOCKED_ID) @conf_vars({("api", "expose_stacktrace"): "False"}) @provide_session def test_handle_multiple_columns_unique_constraint_error( - self, session, table, expected_exception, monkeypatch + self, + mock_get_random_string, + session, + table, + expected_exception, ) -> None: if table == "DagRun": session.add( @@ -252,10 +260,6 @@ def test_handle_multiple_columns_unique_constraint_error( with pytest.raises(IntegrityError) as exeinfo_integrity_error: session.commit() - monkeypatch.setattr( - "airflow.api_fastapi.common.exceptions.get_random_string", - lambda length=None, choices=None: MOCKED_ID, - ) with pytest.raises(HTTPException) as exeinfo_response_error: self.unique_constraint_error_handler.exception_handler(None, exeinfo_integrity_error.value) # type: ignore