Uh oh!
There was an error while loading. Please reload this page.
Type asset_expression in the REST API so the UI does not cast through unknown - #67725
Conversation
bbovenzi
left a comment
There was a problem hiding this comment.
Thanks for this one!
Let's rebase and double check our tests. I assume we now need a valid asset_expression in our mocks or make sure our responses can handle None and undefined.
asset_expression was declared as `dict | None` on the REST API response
models, so the OpenAPI generator emitted an opaque `{ [k: string]: unknown }`
and the UI hand-maintained an ExpressionType union and cast the value through
it with no runtime check. A change to the server shape would not be caught by
the TypeScript build.
This adds a structured, recursive AssetExpression model in
datamodels/common.py: a discriminated union over the five shapes
BaseAsset.as_expression() produces (asset, alias, asset_ref, any, all),
following the existing Annotated[Union[...], Discriminator] pattern. It is
used on every response that serves the field (DAG details, DAGs with latest
runs, and both partitioned dag run responses), types the next_run_assets UI
endpoint with a NextRunAssetsResponse model, regenerates the OpenAPI spec and
the TypeScript client, and removes the now-redundant hand-written union and
casts in the UI.
The asset leaf carries the optional id that DagModelOperation injects on
persistence; id is optional so a row written before id-enrichment, or
migrated from the pre-3.0 dataset format, still validates. For expressions
produced by current code the emitted JSON is unchanged, so this is not a
breaking change for API consumers; only the declared schema becomes precise.
The test_get_dags fixtures previously set asset_expression to a simplified
`{"any": [{"uri": ...}]}` shape that as_expression() never produces. That was
harmless against an untyped dict but the structured model correctly rejects
the bare leaf, so the fixtures now use the real `{"any": [{"asset": {...}}]}`
shape with each asset's name and group.d88ce26 to
622dc91CompareAnuragp22
commented
Jun 2, 2026
Rebased onto main and fixed the failing tests. The The force-push is that rebase: I rebased onto current main to clear the merge conflict and squashed the work into a single commit (d88ce26 to 622dc91). No content changed beyond the test fixtures. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ExpressionType is now NonNullable<DAGDetailsResponse["asset_expression"]> instead of a hand-maintained union that could drift from the server shape. Remove AssetSummary and inline its union at the only use site (AssetNode), and drop the explanatory comment. Per review feedback.
Anuragp22
commented
Jun 10, 2026
Merged main, which now includes AIP-76 (#64571). AIP-76 rewrote the same What changed since the last review:
|
Wrap the over-length created_at/updated_at ternaries so the ruff-format pre-commit hook passes. No behaviour change.
The newly-typed asset_expression field rejects pre-3.0 dataset shapes (a bare uri string, {"any": [<strings>]}, {"alias": "<name>"}) that the 2.x->3.x column rename carried over verbatim, which would surface as an HTTP 500 on the dag-detail, dag-list and next_run_assets endpoints until the dag is re-parsed. MaybeAssetExpression wraps the union with a BeforeValidator that coerces any unrecognized stored shape to null, reproducing the blank render the UI showed while the field was an untyped dict. Adds round-trip and coercion tests.Anuragp22
commented
Jun 12, 2026
Ready for maintainer review. Two commits since the last look:
I've addressed all three of |
Resolves the airflow-ctl generated.py conflict by regenerating it from the merged OpenAPI spec (datamodel-codegen), keeping the new AssetExpression* schema classes and adopting upstream's AssetStoreWriterKind -> AssetStateStoreWriterKind rename. OpenAPI spec and TS client regenerated and confirmed drift-free.
Anuragp22
commented
Jun 15, 2026
ready |
potiuk
commented
Jun 25, 2026
@Anuragp22 — the Static checks CI job is failing here, which needs a code fix on your side (a rerun won't clear it). You can reproduce and fix locally with: Once those pass and CI is green, it'll be ready for a maintainer to pick up. Thanks! See the PR quality criteria. Automated first-pass triage note drafted by an AI-assisted tool — may get things wrong; once addressed, a real Apache Airflow maintainer takes the next look. (why automated) Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting |
Uh oh!
There was an error while loading. Please reload this page.
Per review feedback: the tolerant validator silently coerced any unrecognized shape to None, which also masked genuine server/UI shape drift this PR guards against. Emit a warning when a non-None value is dropped so the drift stays observable. Graceful degradation behaviour is unchanged.
Uh oh!
There was an error while loading. Please reload this page.
Per review feedback: pull the dict passed to .model_validate() into a model_data local in the UI routes that build responses from a dict, which reads better for the larger payloads. No behaviour change.
The dict literals were inferred with a narrow value type once pulled into a local, so mypy rejected heterogeneous entries (e.g. pending_partition_count: int | None). Declare them as dict[str, Any], which is what model_validate accepts, annotating the first occurrence per function scope to avoid no-redef.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
pierrejeambrun
left a comment
There was a problem hiding this comment.
Only 1 comments unresolved, then I believe we are good to merge.
guan404ming
commented
Jun 26, 2026
Besides the unresolved comment, lgtm! |
Per review: a pre-3.0 dataset-format asset_expression (string leaves) is served as null on /dags/{id}/details instead of 500ing the endpoint, exercising MaybeAssetExpression end-to-end.Uh oh!
There was an error while loading. Please reload this page.
Backport failed to create: airflow-ctl/v0-1-test. View the failure log Run detailsNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
You can attempt to backport this manually by running: cherry_picker 3733e62 airflow-ctl/v0-1-testThis should apply the commit to the airflow-ctl/v0-1-test branch and leave the commit in conflict state marking After you have resolved the conflicts, you can continue the backport process by running: cherry_picker --continueIf you don't have cherry-picker installed, see the installation guide. |
… unknown (apache#67725) * Type asset_expression in the REST API instead of an untyped dict asset_expression was declared as `dict | None` on the REST API response models, so the OpenAPI generator emitted an opaque `{ [k: string]: unknown }` and the UI hand-maintained an ExpressionType union and cast the value through it with no runtime check. A change to the server shape would not be caught by the TypeScript build. This adds a structured, recursive AssetExpression model in datamodels/common.py: a discriminated union over the five shapes BaseAsset.as_expression() produces (asset, alias, asset_ref, any, all), following the existing Annotated[Union[...], Discriminator] pattern. It is used on every response that serves the field (DAG details, DAGs with latest runs, and both partitioned dag run responses), types the next_run_assets UI endpoint with a NextRunAssetsResponse model, regenerates the OpenAPI spec and the TypeScript client, and removes the now-redundant hand-written union and casts in the UI. The asset leaf carries the optional id that DagModelOperation injects on persistence; id is optional so a row written before id-enrichment, or migrated from the pre-3.0 dataset format, still validates. For expressions produced by current code the emitted JSON is unchanged, so this is not a breaking change for API consumers; only the declared schema becomes precise. The test_get_dags fixtures previously set asset_expression to a simplified `{"any": [{"uri": ...}]}` shape that as_expression() never produces. That was harmless against an untyped dict but the structured model correctly rejects the bare leaf, so the fixtures now use the real `{"any": [{"asset": {...}}]}` shape with each asset's name and group. * Derive ExpressionType from generated types, drop manual aliases ExpressionType is now NonNullable<DAGDetailsResponse["asset_expression"]> instead of a hand-maintained union that could drift from the server shape. Remove AssetSummary and inline its union at the only use site (AssetNode), and drop the explanatory comment. Per review feedback. * Remove unused NextRunEvent type from AssetExpression * Apply ruff-format to partitioned_dag_runs route Wrap the over-length created_at/updated_at ternaries so the ruff-format pre-commit hook passes. No behaviour change. * Degrade legacy asset_expression shapes to null instead of 500ing the API The newly-typed asset_expression field rejects pre-3.0 dataset shapes (a bare uri string, {"any": [<strings>]}, {"alias": "<name>"}) that the 2.x->3.x column rename carried over verbatim, which would surface as an HTTP 500 on the dag-detail, dag-list and next_run_assets endpoints until the dag is re-parsed. MaybeAssetExpression wraps the union with a BeforeValidator that coerces any unrecognized stored shape to null, reproducing the blank render the UI showed while the field was an untyped dict. Adds round-trip and coercion tests. * Log a warning when an unrecognized asset_expression shape is dropped Per review feedback: the tolerant validator silently coerced any unrecognized shape to None, which also masked genuine server/UI shape drift this PR guards against. Emit a warning when a non-None value is dropped so the drift stays observable. Graceful degradation behaviour is unchanged. * Extract response dict into a named variable before model_validate Per review feedback: pull the dict passed to .model_validate() into a model_data local in the UI routes that build responses from a dict, which reads better for the larger payloads. No behaviour change. * Annotate extracted model_data dicts as dict[str, Any] The dict literals were inferred with a narrow value type once pulled into a local, so mypy rejected heterogeneous entries (e.g. pending_partition_count: int | None). Declare them as dict[str, Any], which is what model_validate accepts, annotating the first occurrence per function scope to avoid no-redef. * Add route test for legacy asset_expression served as null Per review: a pre-3.0 dataset-format asset_expression (string leaves) is served as null on /dags/{id}/details instead of 500ing the endpoint, exercising MaybeAssetExpression end-to-end.
closes: #67692
asset_expressionwas declared asdict | Noneon the REST API response models, so the OpenAPI generator emitted{ [k: string]: unknown }. The UI compensated by hand-maintaining anExpressionTypediscriminated union insrc/components/AssetExpression/and casting the API value through it with no runtime check, so a change to the server shape would not be caught by the TypeScript build. This types the expression on the API side and lets the generated client describe it.What changed:
AssetExpressionmodel indatamodels/common.py: a recursive discriminated union over the five shapesBaseAsset.as_expression()produces (asset,alias,asset_ref,any,all), following the existingAnnotated[Union[...], Discriminator]pattern in that module. Asset leaves carry theidthatDagModelOperation.update_dag_asset_expressioninjects when the expression is persisted.idis optional so a row written before id-enrichment, or migrated from the pre-3.0 dataset format, still validates instead of returning a 500.DAGDetailsResponse,DAGWithLatestDagRunsResponse, and both fields on the partitioned dag run responses.next_run_assetsUI endpoint with a newNextRunAssetsResponsemodel indatamodels/ui/assets.pyinstead of a baredict.response_model_exclude_unset=Truekeeps the response shape identical to before.ExpressionTypeunion and the casts inAssetProgressCell.tsxandAssetSchedule.tsx; the UI now consumes the generated type. The more precise types also let some now-dead defensive code inAssetExpression.tsxbe removed.For expressions produced by the current code the emitted JSON is unchanged, so this is not a breaking change for API consumers; only the declared schema becomes precise.
Tests: added
test_common.pycovering round-trip serialization of every variant, the optionalidbehaviour, and rejection of malformed shapes. Updated thetest_get_dagsfixtures, which setasset_expressionto a simplified{"any": [{"uri": ...}]}shape thatas_expression()never produces and that the new model correctly rejects, to the real{"any": [{"asset": {...}}]}shape. Thenext_run_assetsand dag-detail route tests pass unchanged.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Claude Opus 4.8), reviewed and verified by the author
Important
🛠️ Maintainer triage note for @Anuragp22 · by
@potiuk· 2026-06-12 10:47 UTCYour review threads look addressed — please confirm this PR is ready for maintainer review confirmation.
Feedback from
@bbovenziacross 3 unresolved thread(s); you've engaged with each (replies and/or pushes since).The ball is in your court — you've been assigned to this PR. Reply
yes / readyand a maintainer will pick it up from the queue.Automated triage — may be imperfect; a maintainer takes the next look.