[v3-2-test] UI: Return 400 instead of 500 from structure_data on malformed asset_expression (#67489)#67849
Merged
Merged
Conversation
…ormed asset_expression (#67489) * UI: Return clear 500 detail from structure_data when asset_expression is malformed The /structure/structure_data endpoint calls get_upstream_assets() to walk the serialized Dag's asset_expression. If the stored expression contains an unknown key or asset type, get_upstream_assets() raises TypeError("Unsupported type: ..."). The exception escaped uncaught and FastAPI returned a generic {"detail": "Internal Server Error"} body with no context about which Dag triggered it, forcing operators to dig through server logs to identify the broken Dag. Wrap the call in try/except TypeError and re-raise as HTTPException(500) with a detail message identifying the Dag id and version. Still a 500 (the underlying data corruption is genuinely server-side, not bad client input), but now with a controlled, debuggable response body. Regression test mocks get_upstream_assets to raise TypeError and asserts the response is 500 with a detail message that includes the Dag id. * Use 400 BAD_REQUEST for malformed asset_expression per review feedback Per @jason810496 review feedback on #67489: the malformed asset_expression ultimately originates from user-authored Dag code (via the Task SDK), so the appropriate response is 400 BAD_REQUEST rather than 500 INTERNAL_SERVER_ERROR. - Change status code from 500 to 400 in structure_data. - Add HTTP_400_BAD_REQUEST to create_openapi_http_exception_doc so the OpenAPI spec advertises the new error response. - Update regression test to assert 400 and rename accordingly. Detail message is unchanged per reviewer: "It's fine to add more context". * Revert uv.lock diff --------- (cherry picked from commit eccbdb1) Co-authored-by: Deepak kumar <deepakkumar@meta.com> Co-authored-by: pierrejeambrun <pierrejbrun@gmail.com>
1 task
jason810496
approved these changes
Jun 2, 2026
Member
jason810496
left a comment
There was a problem hiding this comment.
Unrelated CI failure. I will merge right away.
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.
The /structure/structure_data endpoint calls get_upstream_assets() to walk the
serialized Dag's asset_expression. If the stored expression contains an unknown
key or asset type, get_upstream_assets() raises TypeError("Unsupported type: ...").
The exception escaped uncaught and FastAPI returned a generic
{"detail": "Internal Server Error"} body with no context about which Dag
triggered it, forcing operators to dig through server logs to identify the
broken Dag.
Wrap the call in try/except TypeError and re-raise as HTTPException(500) with a
detail message identifying the Dag id and version. Still a 500 (the underlying
data corruption is genuinely server-side, not bad client input), but now with a
controlled, debuggable response body.
Regression test mocks get_upstream_assets to raise TypeError and asserts the
response is 500 with a detail message that includes the Dag id.
Per @jason810496 review feedback on #67489: the malformed asset_expression
ultimately originates from user-authored Dag code (via the Task SDK), so the
appropriate response is 400 BAD_REQUEST rather than 500 INTERNAL_SERVER_ERROR.
spec advertises the new error response.
Detail message is unchanged per reviewer: "It's fine to add more context".
(cherry picked from commit eccbdb1)
Co-authored-by: Deepak kumar deepakkumar@meta.com
Co-authored-by: pierrejeambrun pierrejbrun@gmail.com