Skip to content

UI: Return 400 instead of 500 from structure_data on malformed asset_expression - #67489

Merged
pierrejeambrun merged 4 commits into
apache:mainfrom
dkranchii:fix/structure-data-malformed-asset-expression
Jun 1, 2026
Merged

UI: Return 400 instead of 500 from structure_data on malformed asset_expression#67489
pierrejeambrun merged 4 commits into
apache:mainfrom
dkranchii:fix/structure-data-malformed-asset-expression

Conversation

@dkranchii

@dkranchiidkranchii commented May 25, 2026

Copy link
Copy Markdown
Contributor

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 (anything other than any / all) or an unknown asset type (anything other than asset / asset-alias / asset-name-ref / asset-uri-ref), get_upstream_assets() raises TypeError("Unsupported type: ...") at services/ui/structure.py:69 or :102.

The exception escaped uncaught and FastAPI returned a generic {"detail": "Internal Server Error"} response body with no context about which Dag triggered it. Operators had to dig through server logs to identify the broken Dag — a frustrating debugging loop for a multi-Dag deployment.

This wraps the call in try/except TypeError and re-raises as HTTPException(400) with a detail message identifying the Dag id and version. It remains a 400 because the underlying issue is genuinely server-side stored-data corruption (not bad client input — the request is valid). The improvement is the response body: it's now controlled, documented, and debuggable.

Before

GET /structure/structure_data?dag_id=foo&external_dependencies=True → 500 Internal Server Error {"detail": "Internal Server Error"}

After

GET /structure/structure_data?dag_id=foo&external_dependencies=True → 400 Internal Server Error {"detail": "Malformed asset_expression in Dag 'foo' version 3: Unsupported type: dict_keys(['weird-op'])"}

Tests

Adds TestStructureDataEndpoint::test_should_return_400_on_malformed_asset_expression which mocks get_upstream_assets to raise TypeError and asserts the response surfaces a 400 with a detail message that names the Dag and includes the wrapped error.

Related


Was generative AI tooling used to co-author this PR?
  • Yes — (Cursor) used for review and verify.

@boring-cyborgboring-cyborgBot added the area:API Airflow's REST/HTTP API label May 25, 2026
@potiukpotiuk added the ready for maintainer review Set after triaging when all criteria pass. label May 26, 2026

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just re-raise the TypeError exception as HTTPException. It's fine to add more context to the exception, but we should correct the response code anyway.

ifexpr_keyinasset_expression:
asset_exprs: list[dict] =asset_expression[expr_key]
forexprinasset_exprs:
nested_expr_key=next(iter(expr.keys()))
ifnested_expr_keyin ("any", "all"):
nested_expression=expr
elifnested_expr_keyin ("asset", "alias", "asset-name-ref", "asset-uri-ref"):
asset_info=expr[nested_expr_key]
asset_info["type"] =nested_expr_keyifnested_expr_key!="alias"else"asset-alias"
assets_info.append(asset_info)
else:
raiseTypeError(f"Unsupported type: {expr.keys()}")

… 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.
@dkranchii
dkranchiiforce-pushed the fix/structure-data-malformed-asset-expression branch from 5581905 to 562a6f3CompareMay 26, 2026 15:44
dkranchii added a commit to dkranchii/airflow that referenced this pull request May 26, 2026
Per @jason810496 review feedback on apache#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".
@dkranchiidkranchii changed the title UI: Return clear 500 detail from structure_data on malformed asset_expressionUI: Return 400 instead of 500 from structure_data on malformed asset_expressionMay 26, 2026

@jason810496jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the static check failure, thanks.

dkranchii added a commit to dkranchii/airflow that referenced this pull request May 28, 2026
Per @jason810496 review feedback on apache#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".
@dkranchii
dkranchiiforce-pushed the fix/structure-data-malformed-asset-expression branch from 562a6f3 to e299be4CompareMay 28, 2026 06:54
dkranchii added a commit to dkranchii/airflow that referenced this pull request May 28, 2026
Per @jason810496 review feedback on apache#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".
@dkranchii
dkranchiiforce-pushed the fix/structure-data-malformed-asset-expression branch from e299be4 to 2f65c86CompareMay 28, 2026 18:18
Per @jason810496 review feedback on apache#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".
@dkranchii
dkranchiiforce-pushed the fix/structure-data-malformed-asset-expression branch from 2f65c86 to 56ff1f7CompareMay 29, 2026 00:54

@pierrejeambrunpierrejeambrun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks

@pierrejeambrun

pierrejeambrun commented May 29, 2026

Copy link
Copy Markdown
Member

CI still needs fixing. You can take a look at our contributing doc which explains how to run those locally. (pre-commit hooks etc...) if you want to reproduce locally.

@dkranchii
dkranchiiforce-pushed the fix/structure-data-malformed-asset-expression branch from e84eb2e to 889aa1fCompareMay 30, 2026 02:47
@dkranchii

Copy link
Copy Markdown
ContributorAuthor

@pierrejeambrun pls review.

@choo121600choo121600 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looks good :)
one nit

Comment threaduv.lock
@pierrejeambrun

Copy link
Copy Markdown
Member

Unrelated CI failure, merging.

@pierrejeambrun
pierrejeambrun merged commit eccbdb1 into apache:mainJun 1, 2026
141 of 143 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

Backport successfully created: v3-2-test

Note: As of Merging PRs targeted for Airflow 3.X
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.

In matter of doubt please ask in #release-management Slack channel.

StatusBranchResult
v3-2-testPR Link

github-actionsBot pushed a commit to aws-mwaa/upstream-to-airflow that referenced this pull request Jun 1, 2026
…ormed asset_expression (apache#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 apache#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>
aws-airflow-bot pushed a commit to aws-mwaa/upstream-to-airflow that referenced this pull request Jun 1, 2026
…ormed asset_expression (apache#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 apache#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>
@dkranchii
dkranchii deleted the fix/structure-data-malformed-asset-expression branch June 1, 2026 16:02
jason810496 pushed a commit that referenced this pull request Jun 2, 2026
…ormed asset_expression (#67489) (#67849)
* 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>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:APIAirflow's REST/HTTP APIready for maintainer reviewSet after triaging when all criteria pass.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@dkranchii@pierrejeambrun@choo121600@jason810496@potiuk@vatsrahul1001