Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .changeset/reject-empty-bulk-delete.md
Original file line number Diff line number Diff line change
@@ -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.
6 changes: 6 additions & 0 deletions packages/python-sdk/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions packages/python-sdk/PARITY.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
21 changes: 21 additions & 0 deletions packages/python-sdk/tests/_async/test_box_statics.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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
Expand Down
11 changes: 8 additions & 3 deletions packages/python-sdk/upstash_box/_async/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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)
Expand Down
12 changes: 12 additions & 0 deletions packages/python-sdk/upstash_box/_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()}

Expand Down
11 changes: 8 additions & 3 deletions packages/python-sdk/upstash_box/_sync/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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)
Expand Down
25 changes: 24 additions & 1 deletion packages/sdk/src/__tests__/box-delete-snapshots.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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 }));

Expand Down
12 changes: 12 additions & 0 deletions packages/sdk/src/__tests__/box-delete.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
});
Expand Down
26 changes: 22 additions & 4 deletions packages/sdk/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,19 @@ function apiHeaders(apiKey: string, enableTelemetry?: boolean): Record<string, s
}

/** Decode base64 to bytes in both Node and edge runtimes. */
/**
* Normalizes the ids for a bulk delete and rejects 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".
*/
function requireIds(value: string | string[], name: string): string[] {
const ids = Array.isArray(value) ? value : [value];
if (ids.length === 0 || ids.some((id) => 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") {
Expand Down Expand Up @@ -1071,6 +1084,7 @@ export class Box<TProvider = unknown> {
/**
* 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[] },
Expand All @@ -1092,12 +1106,16 @@ export class Box<TProvider = unknown> {
"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),
Expand Down Expand Up @@ -1132,7 +1150,7 @@ export class Box<TProvider = unknown> {
"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,
Expand Down
Loading