UI: Use react-query native error state for bulk action hooks#67284
Draft
pierrejeambrun wants to merge 8 commits into
Draft
UI: Use react-query native error state for bulk action hooks#67284pierrejeambrun wants to merge 8 commits into
pierrejeambrun wants to merge 8 commits into
Conversation
Restores feature parity with Airflow 2.x where DagRunModelView exposed
collective Delete on the Dag Runs list view. Adds:
- PATCH /dags/{dag_id}/dagRuns — bulk endpoint structured like the
existing bulk task-instances endpoint. Only ``delete`` is supported in
this PR; ``create`` and ``update`` are wired to return 405 in the
BulkResponse so future PRs can fill them in without changing the
route surface.
- BulkDagRunService with deletable-state enforcement (matches the
single-run delete: only QUEUED / SUCCESS / FAILED can be deleted),
per-Dag authorization caching for the wildcard path
/dags/~/dagRuns, and ``action_on_non_existence: fail | skip``
semantics.
- Row selection + a Delete bulk action on the runs list page,
mirroring how Task Instances does it.
Bulk Mark-as and Bulk Clear are intentionally out of scope and will
follow in separate PRs.
The grid view stays single-select; multi-select on the grid was not
available in 2.x either, and the runs list page is the natural target
for bulk operations on a filtered set (e.g. state=failed).
closes: apache#52439
- Switch BulkDagRunService._fetch_dag_runs to tuple_(dag_id, run_id).in_() to avoid a Cartesian over-fetch when /dags/~/dagRuns is called with pairs spanning multiple Dags. Matches BulkTaskInstanceService. - Narrow _check_dag_authorization's method type to Literal["DELETE"]; this PR only does delete, no point in exposing PUT/POST/GET on the signature. - Add a wildcard test that exercises the per-Dag authorization path (limited user accepted for one Dag, rejected with 403 in the BulkResponse for a team-restricted Dag).
Pulls in the substantive UI feedback @bbovenzi left on apache#66554 so this PR lands without re-litigating the same comments. The closed PR was broader (clear / mark / delete) but the structural feedback applies verbatim to delete-only: - Move all DagRuns-related files into ``pages/DagRuns/``, mirroring ``pages/TaskInstances/``. Adds a small ``index.ts`` re-export so ``router.tsx`` keeps the same import path. - Rename ``useBulkDagRuns`` to ``useBulkDeleteDagRuns`` so the hook name matches the single button it serves (one-hook-one-button symmetry — when we add bulk update/clear we'll add sibling hooks). - Stop hand-rolling pending/error state with ``useState<unknown>``; read ``error`` straight off ``useMutation``'s return. - Surface ALL per-entity errors from ``BulkResponse.delete.errors`` instead of just the first; render each as its own ``Alert`` row. - Only invalidate the dag-runs / task-instances queries when at least one entry actually succeeded — a 200 with all-errors should not churn the table. - Keep the dialog open when per-entity errors come back so the user can read what failed; ``reset()`` clears them on close.
Backend: - Inline ``_resolve_dag_id``, ``_result_key`` and ``_fetch_dag_runs`` — each had exactly one caller. Memory rule added so future PRs avoid premature helpers. - Drop the deletable-state restriction. Bulk task-instance delete has no such restriction; bulk Dag-run delete shouldn't either. - Emit one 404 error per missing entity when ``action_on_non_existence`` is ``fail`` instead of collating them into a single error, and stop early-returning so matched runs still get deleted. The invariant ``len(success) + len(errors) == len(requested entities)`` now holds. - Distinguish the two ways a wildcard can leak into ``dag_id`` (path vs body) in the 400 message, mirroring ``BulkTaskInstanceService._categorize_entities``. UI: - Mirror ``useBulkTaskInstances`` exactly: bring back ``useState`` for the error, the shared ``handleActionResult`` helper, single-error surfacing via ``ErrorAlert``, and the ``bulkAction(requestBody)`` shape so the consumer constructs the full ``BulkBody``. Brent's prior review of the closed twin PR pushed past this pattern, but until TI is updated we want both hooks symmetrical — a follow-up can improve both at once. - Inline the affected-runs column array into ``BulkDeleteDagRunsButton`` and delete the standalone ``bulkDagRunsColumns.tsx`` file (single caller).
Adds ``requires_access_dag_run_bulk`` in ``core_api/security.py`` following the ``requires_access_connection_bulk`` pattern. The dependency reads the parsed ``BulkBody[BulkDAGRunBody]``, resolves each entity's ``dag_id`` (body wins, falling back to the path), collects team mappings per Dag, and uses ``batch_is_authorized_dag`` to enforce auth before the route handler runs. The route now declares only this dependency plus ``action_logging``; the per-entity auth check is no longer duplicated inside ``BulkDagRunService``. Unauthorized requests fail with a single route-level 403 instead of returning 200 with per-entity 403s in the ``BulkResponse``, matching how connections / pools / variables behave.
Single ``DELETE /dags/{dag_id}/dagRuns/{dag_run_id}`` rejects RUNNING
runs with 409; bulk delete now does the same — a RUNNING entity yields
a per-entity 409 in ``BulkResponse.errors`` and the matched non-running
entities still get deleted.
Also renames ``DAGRunPatchStates`` to ``DagRunMutableStates`` since it
now gates both PATCH (mark-as) and DELETE — same set of states (QUEUED,
SUCCESS, FAILED), broader meaning. Propagated through the route handlers,
the bulk service, and the UI components that import the generated type.
Bulk delete only invalidated the top-level dag-runs and task-instances lists. Two more cache sets stay stale otherwise: - Per-attempt TI caches (log / extra links / try details), keyed by TI identity. When the user navigates back to a TI try detail via the browser back button after deleting its run, react-query serves the cached log instead of letting the request hit and return 404. - The grid view query set for each affected Dag — the grid renders one bar per Dag Run, so bulk delete literally removes bars. The per-attempt set mirrors the addition ``useBulkTaskInstances`` already gained in apache#67212. The grid invalidation is specific to this hook because deleting Dag Runs (unlike deleting TIs) changes what the grid bars themselves represent. The affected ``dag_id`` set is captured in ``bulkAction`` from the request body and read in ``onSuccess``, same lifecycle as ``useBulkClearTaskInstances``'s ``byDagRun`` grouping.
Both ``useBulkTaskInstances`` and ``useBulkDeleteDagRuns`` mirrored
the same anti-pattern: a ``useState<unknown>`` field, an ``onError``
callback that just forwarded to ``setError``, and a tiny helper that
grabbed ``errors[0]`` from the response body and re-shaped it into
``{body:{detail:...}}`` so ``ErrorAlert`` could render the first
per-entity failure. That hid every error past the first and
duplicated mutation state into React state for no reason.
Both hooks now return ``{ bulkAction, data, error, isPending, reset }``
straight from ``useMutation``:
- ``error`` covers HTTP-level failures (4xx/5xx, network).
- ``data.delete.errors`` / ``data.update.errors`` carries the
per-entity failures the backend returns on a 200 response (partial
success). Consumers render every entry, not just the first.
- ``reset`` replaces the consumer-side ``setError(undefined)`` calls.
``onSuccess`` still invalidates queries unconditionally, fires the
toaster + clears selection when ``success.length > 0``, and only
closes the dialog when ``errors.length === 0`` — partial-success
keeps the dialog open so the user can read what failed.
The three consumers (``BulkMarkTaskInstancesAsButton``,
``BulkDeleteTaskInstancesButton``, ``BulkDeleteDagRunsButton``) now
render network errors via ``ErrorAlert`` and per-entity errors as a
``Stack`` of ``Alert`` rows below it.
boring-cyborg
Bot
added
area:airflow-ctl
area:API
1 task
Member
Author
|
Only the last commit is relevant. |
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
1 participant
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.
useBulkTaskInstancesanduseBulkDeleteDagRunsboth kept the same anti-pattern @bbovenzi flagged on #67095: auseState<unknown>for the error, anonErrorcallback that just forwarded the network error tosetError, and a tiny helper that grabbederrors[0]from the response body and re-shaped it into{body:{detail:...}}soErrorAlertcould render the first per-entity failure. That hid every error past the first and duplicated mutation state into React state for no reason.Both hooks now return
{ bulkAction, data, error, isPending, reset }straight fromuseMutation:errorcovers HTTP-level failures (4xx/5xx, network).data.delete.errors/data.update.errorscarries the per-entity failures the backend returns on a 200 response (partial success). Consumers render every entry, not just the first.resetreplaces the consumer-sidesetError(undefined)calls.onSuccessstill invalidates queries unconditionally, fires the toaster + clears selection whensuccess.length > 0, and only closes the dialog whenerrors.length === 0— partial-success keeps the dialog open so the user can read what failed.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 4.7) following the guidelines