feat(api): Enhance database error messages for better user experience #49929
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AkhilaNenavath123
requested review from
amoghrajesh,
ashb,
bugraoz93,
ephraimbuddy,
jason810496,
kaxil,
pierrejeambrun,
rawwar and
shubhamraj-git
as code owners
|
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)
|
2 tasks
bugraoz93
reviewed
May 1, 2025
There was a problem hiding this comment.
Thanks for the PR! Some tests are failing because of the change, pre-commit needs to be run and some comments need to be addressed :)
FAILED airflow-core/tests/unit/api_fastapi/common/test_exceptions.py::TestUniqueConstraintErrorHandler::test_handle_multiple_columns_unique_constraint_error[Test for postgres only - DagRun] - assert equals failed
'This operation would create a {
duplicate entry. Please ensure
all unique fields have unique v
alues.\nType: unique_constraint
_violation'
'orig_error': 'duplicate key
value violates unique constrain
t "dag_run_dag_id_run_id_key"\n
DETAIL: Key (dag_id, run_id)=(
test_dag_id, test_run_id) alrea
dy exists.\n',
'reason': 'Unique constraint
violation',
'statement': 'INSERT INTO dag
_run (dag_id, queued_at, logica
l_date, start_date, end_date, s
tate, run_id, creating_job_id,
run_type, triggered_by, conf, d
ata_interval_start, data_interv
al_end, run_after, last_schedul
ing_decision, log_template_id,
updated_at, clear_number, backf
ill_id, bundle_version, schedul
ed_by_job_id, context_carrier,
created_dag_version_id) VALUES
(%(dag_id)s, %(queued_at)s, %(l
ogical_date)s, %(start_date)s,
%(end_date)s, %(state)s, %(run_
id)s, %(creating_job_id)s, %(ru
n_type)s, %(triggered_by)s, %(c
onf)s, %(data_interval_start)s,
%(data_interval_end)s, %(run_a
fter)s, %(last_scheduling_decis
ion)s, (SELECT max(log_template
.id) AS max_1 \nFROM log_templa
te), %(updated_at)s, %(clear_nu
mber)s, %(backfill_id)s, %(bund
le_version)s, %(scheduled_by_jo
b_id)s, %(context_carrier)s, %(
created_dag_version_id)s) RETUR
NING dag_run.id',
}
| error_message = self._get_user_friendly_message(exc) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_409_CONFLICT, | ||
| detail={ |
There was a problem hiding this comment.
I don't think we should change this with a string. Some tests and logic depend on this is expected to be a dictionary, not a string, in the API and API tests. Is there a reason for this change?
| error_message = self._get_user_friendly_message(exc) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail=f"{error_message}\nType: database_error" |
There was a problem hiding this comment.
Same here, I think enhancing the text message according to cases and making it user-friendly is a good idea, but the other two things are also useful for debugging.
"statement": str(exc.statement),
"orig_error": str(exc.orig),
pierrejeambrun
requested changes
May 5, 2025
Comment on lines
+79
to
+96
| def _get_user_friendly_message(self, exc: IntegrityError) -> str: | ||
| """Convert database error to user-friendly message.""" | ||
| exc_orig_str = str(exc.orig) | ||
|
|
||
| # Handle DAG run unique constraint | ||
| if "dag_run.dag_id" in exc_orig_str and "dag_run.run_id" in exc_orig_str: | ||
| return "A DAG run with this ID already exists. Please use a different run ID." | ||
|
|
||
| # Handle task instance unique constrain | ||
| if "task_instance.dag_id" in exc_orig_str and "task_instance.task_id" in exc_orig_str: | ||
| return "A task instance with this ID already exists. Please use a different task ID." | ||
|
|
||
| # Handle DAG run logical date unique constraint | ||
| if "dag_run.dag_id" in exc_orig_str and "dag_run.logical_date" in exc_orig_str: | ||
| return "A DAG run with this logical date already exists. Please use a different logical date." | ||
|
|
||
| # Generic unique constraint message | ||
| return "This operation would create a duplicate entry. Please ensure all unique fields have unique values." |
There was a problem hiding this comment.
This seem to handle only duplicated entry errors (unique constraint), while there are many other IntegrityError possible.
Comment on lines
+112
to
+119
| def _get_user_friendly_message(self, exc: SQLAlchemyError) -> str: | ||
| """Convert database error to user-friendly message.""" | ||
| if isinstance(exc, OperationalError): | ||
| return "A database operation failed. Please try again later." | ||
| elif isinstance(exc, ProgrammingError): | ||
| return "An error occurred while processing your request. Please check your input and try again." | ||
| else: | ||
| return "An unexpected database error occurred. Please try again later." |
There was a problem hiding this comment.
This obfuscate the error cause, there's no way to tell what's happening from a user perspective receiving such messages.
There was a problem hiding this comment.
Apologies, I’ve recently been assigned to another project and will be unable to work on this.
|
closing for now, @rawwar will pick it up |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR improves the user experience by making database error messages in the Airflow Rest API more user-friendly and less technical. Specifically, it addresses issues related to unique constraint violations and general database errors by providing clear, actionable error messages without exposing technical SQL details to the user.
Problem:
When attempting to trigger a second DAG run with the same logical date as the first one, the UI displayed a generic unique_constraint error, which included SQL query details. This error was difficult for users to interpret, as it exposed database internals rather than providing a clear action for resolution.
Solution:
This PR enhances the _UniqueConstraintErrorHandler and adds a new _DatabaseErrorHandler to handle database errors in a more user-friendly manner. The changes ensure that:
Key Changes:
Improved Error Messages:
New Database Error Handler:
Refactor Exception Handling:
Issue Link: #49034