chore(aci milestone 3): handle exceptions in migration helpers #85079
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #85079 +/- ##
=======================================
Coverage 87.94% 87.94%
=======================================
Files 9634 9634
Lines 544128 544138 +10
Branches 21124 21124
=======================================
+ Hits 478528 478537 +9
- Misses 65295 65296 +1
Partials 305 305 |
| extra={"details": str(e)}, | ||
| ) | ||
| return Response( | ||
| "Error when dual deleting alert rule", status=status.HTTP_400_BAD_REQUEST |
There was a problem hiding this comment.
We may want to look at the kind of exception to decide the status code here -- 400 errors are mean that they're caused by bad user input.
Since there are a lot of possible errors that can arise when we are deleting these rules, we should check the exception to see if it was caused by user input (4xx errors) or something on our end (5xx errors).
side note for the best of the http status codes, 418
| logger.exception( | ||
| "Error when dual writing alert rule", extra={"details": str(e)} | ||
| ) | ||
| raise BadRequest |
There was a problem hiding this comment.
it might be nice if the exception included why it failed, then proxy that through the BadRequest so the API can have more specific errors.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| migrate_metric_action(action) | ||
| except ValidationError as e: | ||
| # invalid action type | ||
| raise serializers.ValidationError(str(e)) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 5 days ago
To fix the problem, we need to ensure that the detailed error message is logged on the server, and a generic error message is returned to the user. This can be achieved by logging the exception details and raising a generic serializers.ValidationError with a non-specific message.
- Modify the
createmethod to log the detailed exception message and raise a generic error message. - Modify the
updatemethod similarly to ensure consistency.
| @@ -1,2 +1,5 @@ | ||
| from django.forms import ValidationError | ||
| import logging | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| from django.utils.encoding import force_str | ||
| @@ -220,3 +223,4 @@ | ||
| # invalid action type | ||
| raise serializers.ValidationError(str(e)) | ||
| logger.error("Validation error during metric action migration: %s", str(e)) | ||
| raise serializers.ValidationError("An internal error has occurred.") | ||
|
|
||
| @@ -231,3 +235,4 @@ | ||
| except (ApiRateLimitedError, InvalidTriggerActionError) as e: | ||
| raise serializers.ValidationError(force_str(e)) | ||
| logger.error("Error updating alert rule trigger action: %s", force_str(e)) | ||
| raise serializers.ValidationError("An internal error has occurred.") | ||
|
|
Changes to exception handling for migration helpers.
Wrote some TODOs to wrap the dual write/update/delete calls in transactions, but this is outside the scope of this PR.