diff --git a/.changeset/reject-empty-bulk-delete.md b/.changeset/reject-empty-bulk-delete.md new file mode 100644 index 00000000..bab15adc --- /dev/null +++ b/.changeset/reject-empty-bulk-delete.md @@ -0,0 +1,11 @@ +--- +"@upstash/box": patch +--- + +Reject an empty id list in `Box.delete` and `Box.deleteSnapshots` instead of deleting everything. + +`Box.delete({ boxIds: [] })` sent `{"ids": []}`, and the API read an empty list as "no filter", so it deleted every box on the account. A script that computed its id list and came up empty wiped the account instead of doing nothing. `Box.deleteSnapshots({ snapshotIds: [] })` had the same shape. + +Both now throw a `BoxError` before any request is made when the list is empty or contains a blank id. `EphemeralBox.delete` and `EphemeralBox.deleteSnapshots` are the same functions, so they are covered too. + +Calling `Box.deleteSnapshots()` with no `snapshotIds` still deletes every snapshot, as documented. It now says so explicitly by sending `?all=true`, so the API no longer has to infer "everything" from a missing list. diff --git a/packages/python-sdk/CHANGELOG.md b/packages/python-sdk/CHANGELOG.md index 3740dfec..bef6cd02 100644 --- a/packages/python-sdk/CHANGELOG.md +++ b/packages/python-sdk/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to `upstash-box` (Python) are documented here. ## Unreleased +- Fix `delete_boxes(box_ids=[])` deleting every box on the account. The API read + an empty id list as "no filter". `delete_boxes` and `delete_snapshots` now raise + `BoxError` before any request is made when the list is empty or contains a + blank id. +- `delete_snapshots()` with no `snapshot_ids` still deletes every snapshot, and + now says so explicitly by sending `?all=true`. - Add `git.create_issue()`, which opens a GitHub issue from a box. - Add `attach` to `git.create_pr()` and `git.create_issue()`. It takes image or video files, relative to the working directory, and uploads them to the new diff --git a/packages/python-sdk/PARITY.md b/packages/python-sdk/PARITY.md index add19391..f04fee6d 100644 --- a/packages/python-sdk/PARITY.md +++ b/packages/python-sdk/PARITY.md @@ -68,6 +68,7 @@ epoch seconds to **ms** in both SDKs. | `create`, `get`, `getByName`, `list`, `fromSnapshot` | `create`, `get`, `get_by_name`, `list`, `from_snapshot` | | `delete` (bulk) | `delete_boxes` (renamed to avoid clashing with instance `delete`) | | `deleteSnapshots` | `delete_snapshots` | +| `delete` / `deleteSnapshots` reject an empty or blank id list | `delete_boxes` / `delete_snapshots` raise `BoxError` the same way | | `setEnv`, `listEnv`, `deleteEnv`, `setAllEnv` | `set_env`, `list_env`, `delete_env`, `set_all_env` | ## `EphemeralBox` diff --git a/packages/python-sdk/tests/_async/test_box_statics.py b/packages/python-sdk/tests/_async/test_box_statics.py index b2e9df66..0f54eaf1 100644 --- a/packages/python-sdk/tests/_async/test_box_statics.py +++ b/packages/python-sdk/tests/_async/test_box_statics.py @@ -37,6 +37,26 @@ async def test_delete_boxes(): assert json.loads(route.calls.last.request.content) == {"ids": ["box-123"]} +@pytest.mark.parametrize("box_ids", [[], "", ["box-1", " "], None]) +@respx.mock +async def test_delete_boxes_rejects_an_empty_scope(box_ids): + route = respx.delete(ROOT).mock(return_value=httpx.Response(200, json={})) + with pytest.raises(BoxError, match="box_ids must contain at least one non-empty id"): + await AsyncBox.delete_boxes(box_ids=box_ids, **_opts()) + assert not route.called + + +@pytest.mark.parametrize("snapshot_ids", [[], "", ["snap-1", " "]]) +@respx.mock +async def test_delete_snapshots_rejects_an_empty_scope(snapshot_ids): + route = respx.delete(f"{ROOT}/snapshots").mock( + return_value=httpx.Response(200, json={"deleted": 0}) + ) + with pytest.raises(BoxError, match="snapshot_ids must contain at least one non-empty id"): + await AsyncBox.delete_snapshots(snapshot_ids=snapshot_ids, **_opts()) + assert not route.called + + @respx.mock async def test_delete_snapshots_all(): route = respx.delete(f"{ROOT}/snapshots").mock( @@ -45,6 +65,7 @@ async def test_delete_snapshots_all(): result = await AsyncBox.delete_snapshots(**_opts()) assert result["deleted"] == 3 assert json.loads(route.calls.last.request.content) == {} + assert route.calls.last.request.url.params["all"] == "true" @respx.mock diff --git a/packages/python-sdk/upstash_box/_async/client.py b/packages/python-sdk/upstash_box/_async/client.py index 9d640412..18388d0a 100644 --- a/packages/python-sdk/upstash_box/_async/client.py +++ b/packages/python-sdk/upstash_box/_async/client.py @@ -2124,7 +2124,7 @@ async def delete_boxes( api_key = common.resolve_api_key(options.get("api_key")) base_url = common.resolve_base_url(options.get("base_url")) headers = common.build_headers(api_key) - ids = box_ids if isinstance(box_ids, list) else [box_ids] + ids = common.require_ids(box_ids, "box_ids") async with httpx.AsyncClient() as client: response = await client.request( "DELETE", @@ -2144,14 +2144,19 @@ async def delete_snapshots( api_key = common.resolve_api_key(options.get("api_key")) base_url = common.resolve_base_url(options.get("base_url")) headers = common.build_headers(api_key) + # Deleting everything is asked for explicitly, never implied by a missing list. body: Dict[str, Any] = {} - if snapshot_ids is not None: - body["ids"] = snapshot_ids if isinstance(snapshot_ids, list) else [snapshot_ids] + params: Dict[str, str] = {} + if snapshot_ids is None: + params["all"] = "true" + else: + body["ids"] = common.require_ids(snapshot_ids, "snapshot_ids") async with httpx.AsyncClient() as client: response = await client.request( "DELETE", f"{base_url}/v2/box/snapshots", headers={**headers, "Content-Type": "application/json"}, + params=params, content=json.dumps(body), ) common.raise_for_status(response) diff --git a/packages/python-sdk/upstash_box/_common.py b/packages/python-sdk/upstash_box/_common.py index 5370972d..0881bdd5 100644 --- a/packages/python-sdk/upstash_box/_common.py +++ b/packages/python-sdk/upstash_box/_common.py @@ -63,6 +63,18 @@ def resolve_api_key(api_key: Optional[str]) -> str: return key +def require_ids(value: Any, name: str) -> List[str]: + """Normalize the ids for a bulk delete and reject a list that names nothing. + + The bulk endpoints delete whatever they are scoped to, so an empty or blank + scope must never reach them: it would read as "everything". + """ + ids = value if isinstance(value, list) else [value] + if not ids or any(not isinstance(i, str) or not i.strip() for i in ids): + raise BoxError(f"{name} must contain at least one non-empty id") + return ids + + def build_headers(api_key: str) -> Dict[str, str]: return {"X-Box-Api-Key": api_key, **telemetry_headers()} diff --git a/packages/python-sdk/upstash_box/_sync/client.py b/packages/python-sdk/upstash_box/_sync/client.py index 5f9a5878..998fb6a4 100644 --- a/packages/python-sdk/upstash_box/_sync/client.py +++ b/packages/python-sdk/upstash_box/_sync/client.py @@ -2101,7 +2101,7 @@ def delete_boxes( api_key = common.resolve_api_key(options.get("api_key")) base_url = common.resolve_base_url(options.get("base_url")) headers = common.build_headers(api_key) - ids = box_ids if isinstance(box_ids, list) else [box_ids] + ids = common.require_ids(box_ids, "box_ids") with httpx.Client() as client: response = client.request( "DELETE", @@ -2121,14 +2121,19 @@ def delete_snapshots( api_key = common.resolve_api_key(options.get("api_key")) base_url = common.resolve_base_url(options.get("base_url")) headers = common.build_headers(api_key) + # Deleting everything is asked for explicitly, never implied by a missing list. body: Dict[str, Any] = {} - if snapshot_ids is not None: - body["ids"] = snapshot_ids if isinstance(snapshot_ids, list) else [snapshot_ids] + params: Dict[str, str] = {} + if snapshot_ids is None: + params["all"] = "true" + else: + body["ids"] = common.require_ids(snapshot_ids, "snapshot_ids") with httpx.Client() as client: response = client.request( "DELETE", f"{base_url}/v2/box/snapshots", headers={**headers, "Content-Type": "application/json"}, + params=params, content=json.dumps(body), ) common.raise_for_status(response) diff --git a/packages/sdk/src/__tests__/box-delete-snapshots.test.ts b/packages/sdk/src/__tests__/box-delete-snapshots.test.ts index 6e4e6170..65f04bdf 100644 --- a/packages/sdk/src/__tests__/box-delete-snapshots.test.ts +++ b/packages/sdk/src/__tests__/box-delete-snapshots.test.ts @@ -2,6 +2,8 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { Box, BoxError } from "../client.js"; import { mockResponse, TEST_CONFIG } from "./helpers.js"; +const CONN = { apiKey: TEST_CONFIG.apiKey, baseUrl: TEST_CONFIG.baseUrl }; + describe("Box.deleteSnapshots (static)", () => { beforeEach(() => { vi.stubGlobal("fetch", vi.fn()); @@ -18,13 +20,34 @@ describe("Box.deleteSnapshots (static)", () => { }); const [url, init] = vi.mocked(fetch).mock.calls[0]!; - expect(url).toBe(`${TEST_CONFIG.baseUrl}/v2/box/snapshots`); + expect(url).toBe(`${TEST_CONFIG.baseUrl}/v2/box/snapshots?all=true`); expect(init?.method).toBe("DELETE"); const body = JSON.parse(init?.body as string); expect(body.ids).toBeUndefined(); expect(result).toEqual({ deleted: 3 }); }); + it("does not ask for everything when ids are named", async () => { + vi.mocked(fetch).mockResolvedValueOnce(mockResponse({ deleted: 1 })); + + await Box.deleteSnapshots({ ...CONN, snapshotIds: ["snap-1"] }); + + const [url, init] = vi.mocked(fetch).mock.calls[0]!; + expect(url).toBe(`${TEST_CONFIG.baseUrl}/v2/box/snapshots`); + expect(JSON.parse(init?.body as string).ids).toEqual(["snap-1"]); + }); + + it.each([ + ["an empty array", []], + ["an empty string", ""], + ["a blank id in the list", ["snap-1", " "]], + ])("rejects %s instead of deleting every snapshot", async (_label, snapshotIds) => { + await expect(Box.deleteSnapshots({ ...CONN, snapshotIds })).rejects.toThrow( + "snapshotIds must contain at least one non-empty id", + ); + expect(fetch).not.toHaveBeenCalled(); + }); + it("deletes a single snapshot by ID", async () => { vi.mocked(fetch).mockResolvedValueOnce(mockResponse({ deleted: 1 })); diff --git a/packages/sdk/src/__tests__/box-delete.test.ts b/packages/sdk/src/__tests__/box-delete.test.ts index 8fe23280..5b0c1c2b 100644 --- a/packages/sdk/src/__tests__/box-delete.test.ts +++ b/packages/sdk/src/__tests__/box-delete.test.ts @@ -34,6 +34,18 @@ describe("Box.delete (static)", () => { expect(body.ids).toEqual(["box-1", "box-2", "box-3"]); }); + it.each([ + ["an empty array", []], + ["an empty string", ""], + ["a blank id in the list", ["box-1", " "]], + ["a missing boxIds", undefined as unknown as string[]], + ])("rejects %s instead of deleting every box", async (_label, boxIds) => { + await expect( + Box.delete({ apiKey: TEST_CONFIG.apiKey, baseUrl: TEST_CONFIG.baseUrl, boxIds }), + ).rejects.toThrow("boxIds must contain at least one non-empty id"); + expect(fetch).not.toHaveBeenCalled(); + }); + it("throws when apiKey is missing", async () => { await expect(Box.delete({ boxIds: "box-1" })).rejects.toThrow("apiKey is required"); }); diff --git a/packages/sdk/src/client.ts b/packages/sdk/src/client.ts index 35a79bf8..a9bd9ab5 100644 --- a/packages/sdk/src/client.ts +++ b/packages/sdk/src/client.ts @@ -80,6 +80,19 @@ function apiHeaders(apiKey: string, enableTelemetry?: boolean): Record typeof id !== "string" || id.trim() === "")) { + throw new BoxError(`${name} must contain at least one non-empty id`); + } + return ids; +} + function base64ToBytes(b64: string): Uint8Array { if (typeof Buffer !== "undefined") return new Uint8Array(Buffer.from(b64, "base64")); if (typeof globalThis.atob !== "function") { @@ -1071,6 +1084,7 @@ export class Box { /** * Delete snapshots for the authenticated user. * Omit snapshotIds to delete all snapshots, or pass a single ID / array of IDs to delete specific ones. + * An empty snapshotIds is rejected rather than read as "all". */ static async deleteSnapshots( options?: BoxConnectionOptions & { snapshotIds?: string | string[] }, @@ -1092,12 +1106,16 @@ export class Box { "Content-Type": "application/json", }; + // Deleting everything is asked for explicitly, never implied by a missing list. const body: { ids?: string[] } = {}; - if (options?.snapshotIds !== undefined) { - body.ids = Array.isArray(options.snapshotIds) ? options.snapshotIds : [options.snapshotIds]; + let url = `${baseUrl}/v2/box/snapshots`; + if (options?.snapshotIds === undefined) { + url += "?all=true"; + } else { + body.ids = requireIds(options.snapshotIds, "snapshotIds"); } - const response = await fetch(`${baseUrl}/v2/box/snapshots`, { + const response = await fetch(url, { method: "DELETE", headers, body: JSON.stringify(body), @@ -1132,7 +1150,7 @@ export class Box { "Content-Type": "application/json", }; - const ids = Array.isArray(options.boxIds) ? options.boxIds : [options.boxIds]; + const ids = requireIds(options.boxIds, "boxIds"); const response = await fetch(`${baseUrl}/v2/box`, { method: "DELETE", headers,