Repository navigation
fix(incidents): return a valid error when some incidents fail to merge - #6965
Open
nazarli-shabnam wants to merge 1 commit into
Open
nazarli-shabnam wants to merge 1 commit into
nazarli-shabnam wants to merge 1 commit into
Conversation
The merge route raised HTTPException with the message as its first argument, which is status_code, so FastAPI failed with 'is not a valid HTTPStatus' and the client got a broken response instead of the list of failures. Pass status_code=500 and the message as detail. Related to keephq#5173
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #5173
Problem
When some incidents fail to merge,
POST /incidents/mergerunsraise HTTPException(f"Some incidents failed to merge. {message}"). The first argument ofHTTPExceptionisstatus_code, so the message is used as a status code and Starlette fails with'Some incidents failed to merge. ...' is not a valid HTTPStatus. The client gets a broken response instead of the error, which fits the "unexpected end of JSON input" in #5173.Fix
Pass
status_code=500and the message asdetail.This does not explain why a merge of three or more incidents fails in the first place.
merge_incidents_to_idloops over every source incident, so I could not find the original cause by reading the code; this PR only makes the failure report correctly. The linked-issue check requires a closing keyword, so this PR saysCloses; if you want the issue to stay open until the cause of the failed merges is found, please reopen it.Tests
Added
test_merge_incidents_app_reports_partial_failureintests/test_incidents.py. It patchesmerge_incidents_to_idto report one failed incident and expects a 500 with the message indetail.Run in a throwaway
python:3.12container afterpoetry install --no-root --with dev. Before the fix it failed withValueError: ... is not a valid HTTPStatus; after the fix it passed.Not verified
tests/test_incidents.py. The existingtest_merge_incidents_appis inside a string literal in that file (disabled), so nothing else covers this route.AI disclosure
Claude (Anthropic) helped investigate the bug and draft the change and this description. I reviewed the diff.