UI: Add multi-select bulk actions for Dag Runs#66554
Conversation
|
@yuseok89 — Your unresolved review thread(s) from @bbovenzi appear to have been addressed (post-review commits and/or in-thread replies on every thread, with the latest commit pushed after the most recent thread). I've added the @bbovenzi — could you take another look when you have a chance? If you agree the feedback was addressed, please mark the threads as resolved so the queue signal stays accurate. If a thread still needs work, please reply in-line — @yuseok89 will follow up. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting |
|
@bbovenzi |
Yeah go for it. Sorry, I deleted it to do a better review but then got distracted. I was looking at our Bulk Task Instance logic. So as long as we follow that precedent we should be good. But if we could do more on the backend than BulkTis did then even better. |
| ) -> None: | ||
| """Apply ``new_state`` to ``dag_run`` and fire the matching listener hook.""" | ||
| if new_state == DAGRunPatchStates.SUCCESS: | ||
| set_dag_run_state_to_success(dag=dag, run_id=dag_run.run_id, commit=True, session=session) |
There was a problem hiding this comment.
What was the reasoning for commit=True on all of these?
There was a problem hiding this comment.
Without commit=True these helpers don't apply the change, and using it across all three states keeps them consistent.
Only QUEUED could go lighter with a direct state set.
Would that be better?
…dag-runs-actions # Conflicts: # airflow-core/src/airflow/ui/openapi-gen/requests/services.gen.ts
bbovenzi
left a comment
There was a problem hiding this comment.
Big change that we need:
- Don't use DagRunService directly, instead use the generated use mutation hooks. We can still have a custom wrapper hook around it to invalidate queries and do dry runs
…dag-runs-actions # Conflicts: # airflow-core/src/airflow/ui/src/pages/DagRuns/DagRuns.tsx
|
@yuseok89 — There are 1 unresolved review thread on this PR from @bbovenzi. Could you either push a fix or reply in each thread explaining why the feedback doesn't apply? Once you believe the feedback is addressed, mark the thread as resolved so the reviewer isn't re-pinged needlessly. Thanks! Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
pierrejeambrun
left a comment
There was a problem hiding this comment.
Thanks for your PR @yuseok89.
This has come to our attention that feature parity on bulk actions has become critical and need to land in 3.3.0.
Therefore I'll work on it so we can make this happen as fast as possible.
Closing in favor of #67095 and other PRs I'm about to make because this will need a bunch of iteration before it's ready.
Sorry for the late notice.
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.
|
Thanks @pierrejeambrun and @bbovenzi for the reviews and for carrying this forward in #67095. |
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.
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.
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.
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.
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.
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.
* Add bulk delete endpoint for Dag Runs
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: #52439
* Address self-review on bulk delete Dag Runs
- 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).
* Apply Brent's review feedback from the closed twin PR
Pulls in the substantive UI feedback @bbovenzi left on #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.
* Address inline review: mirror task-instance bulk delete patterns
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).
* Move bulk Dag Run authorization to a dedicated route dependency
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.
* Mirror single-run DELETE state restriction on bulk delete
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.
* Invalidate dependent queries after bulk delete Dag Run
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 #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.
* Restore resolve_run_on_latest_version import dropped during rebase
Rebasing the branch onto the latest ``apache/main`` with the
``-X theirs`` strategy used the incoming side for every conflict in
``routes/public/dag_run.py``. That dropped the
``resolve_run_on_latest_version`` import that #63884 added — the
function call at line 336 was preserved but the import line wasn't,
so the module failed to load and every test in ``test_dag_run.py``
crashed at collection (Static checks, mypy, and all DB-core test
matrices on CI).
This commit only adds the missing import back; no other change.
closes: #63854
Promise.allSettled; partial failures keep the confirmation dialog open with anErrorAlert, while the successful subset is reported via toaster.POST /dagRuns/bulkendpoint (analogous tobulkTaskInstances) is left as a follow-up, for the same reasons it was deferred for Task Instances.After
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.