Add bulk-delete endpoints for remaining grid entities - #2569
Conversation
![peqy[bot]](https://avatars.githubusercontent.com/in/1930020?s=60&v=4){kind=small}
There was a problem hiding this comment.
[Improvement] bulk_delete_by_ids(owner_attr=...) should probably guard against missing user_id (and optionally validate owner_attr exists) to avoid accidental weakening of owner-only semantics.
[Improvement] /test_runs/bulk fetches task ids using visibility filtering; consider aligning that query with the same ownership filter used for deletion to avoid extra work / subtle coupling.
[Question] /tasks/bulk doesn’t emit telemetry like the single-item delete—intentional?
Found 3 issues (0 critical, 2 improvements, 1 question).
| not_found_ids = [i for i in item_ids if i not in existing_ids] | ||
| not_found_ids = [i for i in item_ids if i not in visible_ids] | ||
|
|
||
| forbidden_ids = None |
There was a problem hiding this comment.
[Improvement] owner_attr is meant to enforce an owner-only delete rule, but if a caller ever passes owner_attr without a user_id (it’s still optional in the signature), the query will silently treat everything as not-owned.
Fix: consider an explicit guard like
if owner_attr and not user_id: raise ValueError(...)(and optionally validatehasattr(model, owner_attr)) so misuse can’t weaken the authorization semantics.
There was a problem hiding this comment.
I checked with_visibility_filter(None): passing owner_attr without user_id won’t delete anything (owned_ids ends up empty), so it’s not an auth bypass. Still think it’s worth an explicit guard (if owner_attr and not user_id: raise ...) + hasattr(model, owner_attr) to prevent silent misuse/confusing forbidden_ids results.
| """ | ||
| from rhesis.backend.celery.core import app as celery_app | ||
|
|
||
| organization_id, user_id = tenant_context |
There was a problem hiding this comment.
[Improvement] Bulk delete gets task ids via get_test_run_task_ids(..., user_id=user_id) which applies .with_visibility_filter(user_id). For TestRun you’re also enforcing creator-only delete (owner_attr="user_id").
If with_visibility_filter for TestRun is broader than “creator-only” (e.g., org-visible), you’ll fetch task IDs for runs you won’t delete (forbidden). That’s not a security issue, but it’s extra work and makes the revoke logic depend on visibility semantics.
Fix: consider aligning the task-id query with the same ownership filter (either pass an
owner_attr-like filter intoget_test_run_task_ids, or compute task ids only fromresult["deleted_ids"]after the delete by re-querying those ids).
There was a problem hiding this comment.
Confirmed fixed in e1d8d4e: get_test_run_task_ids() now filters to TestRun.user_id == user_id, so we only fetch task ids for runs that can actually land in deleted_ids.
| raise HTTPException(status_code=500, detail="Internal server error") | ||
|
|
||
|
|
||
| @router.delete("/bulk", response_model=schemas.TaskBulkDeleteResponse) |
There was a problem hiding this comment.
[Question] The bulk delete path enforces creator-only via owner_attr, but (unlike the single-item delete) it doesn’t record telemetry (track_feature_usage) or return a success message.
Is that intentional for bulk endpoints? If you want parity, you could track a single action="bulk_deleted" with count + ids (or just count), and keep the response shape consistent with other bulk deletes.
There was a problem hiding this comment.
Confirmed fixed in e1d8d4e: bulk task delete now tracks telemetry via track_feature_usage(..., action="bulk_deleted", count=...).
bulk_delete_by_ids only understood organization scope and the
visibility column, so it couldn't back an owner-only (":own") delete
rule the way single-item delete does via authorize_object. Ids that
exist but fail the new owner_attr check land in "forbidden_ids"
instead of being silently skipped or deleted.
Single-item delete already enforces that only the creator may delete their own test run (DELETE_OWN). Wires bulk_delete_by_ids's new owner_attr check into a DELETE /test_runs/bulk endpoint so bulk delete enforces the same rule instead of silently dropping it, and revokes the Celery task for any active run among the ones actually deleted.
TestSet's visibility column is the only delete gate (no owner-only rule on top, unlike TestRun), so this ports Tests' existing bulk_delete_by_ids pattern as-is behind a new DELETE /test_sets/bulk endpoint, replacing the per-row delete loop the frontend used before.
No owner-only rule on endpoint delete, so this ports the existing bulk_delete_by_ids pattern as-is behind a new DELETE /endpoints/bulk endpoint.
No owner-only rule on source delete, so this ports the existing bulk_delete_by_ids pattern as-is behind a new DELETE /sources/bulk endpoint.
No owner-only rule on token delete, so this ports the existing bulk_delete_by_ids pattern as-is behind a new DELETE /tokens/bulk endpoint, matching the existing revoke_token naming internally.
Single-item delete already enforces that only the creator may delete their own task (DELETE_OWN). Wires bulk_delete_by_ids's owner_attr check into a DELETE /tasks/bulk endpoint, same treatment as test runs, so bulk delete enforces the same rule instead of silently dropping it.
get_test_run_task_ids only filtered by visibility (a no-op for TestRun), so it fetched task ids for runs that would land in forbidden_ids too -- wasted work, since only deleted_ids is ever consulted for revocation. Scopes it to the caller's own runs to match. Also adds the track_feature_usage call to the task bulk-delete route, matching every other action in this router.
e1d8d4e to
af5e669
Compare
|
Overall looks solid: consistent bulk endpoints + good route-ordering notes/tests, and the [Improvement]
Found 1 issue (0 critical, 1 improvement). |
|
Re-reviewed latest commits: the TestRun task-id query now aligns with the owner-only delete rule, and bulk Task delete now tracks telemetry — both look good. [Improvement]
|
1 participant
Purpose
Add bulk-select delete support to every backend entity that has a grid in the frontend. This covers the backend half of #2261 (Test Runs and Test Sets) and extends the same pattern to Endpoints, Sources, Tokens, and Tasks. Two of these entities (Test Run, Task) have an owner-only delete rule that the existing generic bulk-delete helper didn't understand, so this also extends that helper to support it safely instead of letting any org member bulk-delete rows they don't own.
What Changed
bulk_delete_by_ids()gained an optionalowner_attrparameter: when set, only rows owned by the caller are deleted, and ids that exist but belong to someone else are reported in a newforbidden_idsfield instead of being silently skipped or deleted.DELETE /test_runs/bulk, respecting the existing creator-only delete rule (also revokes the Celery task for any active run among the ones actually deleted).DELETE /test_sets/bulk, replacing the one-row-at-a-time delete loop the frontend used before.DELETE /endpoints/bulk,DELETE /sources/bulk, andDELETE /tokens/bulk— none of these have an owner-only rule, so each is a direct wrapper around the existing bulk-delete helper.DELETE /tasks/bulk, respecting the existing creator-only delete rule, same treatment as test runs.Additional Context
Closes the backend half of #2261. Team Members and account-level User deletion were intentionally left out of this batch — Team Members isn't backed by a real data grid today, and deleting another user's account carries different risk than deleting a resource like a test or endpoint. The frontend wiring (selection UI, bulk-action bars, API client methods) is a separate PR.
Testing
Added a route-level test file per entity under
tests/backend/routes/, covering the deleted/not-found split for every entity and the deleted/forbidden/not-found split for Test Run and Task specifically. Ran the full set of new tests together with the existing Tests bulk-delete suite and the object-level authorization suite to confirm no regressions; all 36 tests pass.