From 3b5e29c6f0e5f9ee7fda7ea9322984edf947a21a Mon Sep 17 00:00:00 2001 From: Petr Date: Sat, 22 Aug 2026 22:25:27 +0200 Subject: [PATCH 1/3] fix(config): never purge on a repeated delete; add restore + trash-list (0.89.0) The Storage API overloads DELETE .../configs/{id}: on a live configuration it soft-deletes into the trash, but on a configuration ALREADY in the trash the same call purges it permanently -- versions, rows and metadata included. A timed-out delete followed by a retry is exactly that second call, and retrying on timeout is what every agent and CI script does. `config delete` now locates the configuration before deleting: * live -> DELETE, status "deleted" * already in the trash -> NO second DELETE, status "already_in_trash", exit 0 (the retry stays idempotent for scripts) * absent from both -> NOT_FOUND (a plain GET 404s for trashed AND missing configs alike, so the trash listing is what separates the two) * --dry-run reports the located state without writing New commands complete the loop: * `config restore` -- POST .../configs/{id}/restore, the undo (versions, rows and metadata come back); permission class write * `config trash-list` -- what restore can bring back, project-wide or per component; permission class read All three are mirrored on `kbagent serve` (DELETE gains dry_run; POST .../restore and GET /configs/trash/{project} are new). Docs: CLAUDE.md's All CLI Commands had NEVER listed `config delete` -- silent drift that made the command look nonexistent to AI agents reading the file (it misled one today). Added, along with the double-delete gotcha in gotchas.md and entries in commands-reference.md and AGENT_CONTEXT. Endpoint semantics verified against keboola/connection source: the official PHP client's purgeConfiguration docblock states the repeated-DELETE purge behaviour and offers POST .../purge (400 when not trashed) as the safe explicit path; restoreComponentConfiguration confirms the restore route. Trash lookup and result shaping live in services/_config_trash.py because config_service.py is over its size budget; the service methods stay thin. Commands live in commands/_config_trash_cmd.py for the same reason (config.py), mounted via register() like _config_clone_cmd.py. 15 unit tests (the assert_not_called() checks on client.delete_config are the point: they prove the purge call cannot happen) + 3 router tests. The E2E delete step now runs the full round trip live: delete -> repeated delete answers already_in_trash -> trash-list finds it -> restore -> detail confirms live -> final delete. Version 0.89.0 (pyproject + version-sync), changelog block added -- which is also what satisfies check_version_gates for the new (since v0.89.0) markers; the gate caught this PR's own docs before the changelog existed. --- .claude-plugin/marketplace.json | 2 +- CLAUDE.md | 11 + plugins/kbagent/.claude-plugin/plugin.json | 2 +- plugins/kbagent/skills/kbagent/SKILL.md | 4 +- .../kbagent/references/commands-reference.md | 4 +- .../skills/kbagent/references/gotchas.md | 24 ++ pyproject.toml | 2 +- src/keboola_agent_cli/changelog.py | 22 ++ src/keboola_agent_cli/client/configs.py | 58 +++ .../commands/_config_trash_cmd.py | 119 +++++++ src/keboola_agent_cli/commands/config.py | 33 +- src/keboola_agent_cli/commands/context.py | 15 +- src/keboola_agent_cli/permissions.py | 2 + .../server/routers/configs.py | 43 ++- .../services/_config_trash.py | 91 +++++ .../services/config_service.py | 103 +++++- tests/test_config_trash.py | 334 ++++++++++++++++++ tests/test_e2e.py | 47 ++- tests/test_server_router_calls.py | 48 +++ uv.lock | 2 +- 20 files changed, 947 insertions(+), 19 deletions(-) create mode 100644 src/keboola_agent_cli/commands/_config_trash_cmd.py create mode 100644 src/keboola_agent_cli/services/_config_trash.py create mode 100644 tests/test_config_trash.py diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index e744f3ac..d18a104f 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -10,7 +10,7 @@ "plugins": [ { "name": "kbagent", - "version": "0.88.0", + "version": "0.89.0", "source": "./plugins/kbagent", "description": "AI-friendly interface to Keboola Connection projects — explore configs, jobs, lineage, sync configs as files, manage dev branches, and debug SQL in workspaces", "category": "development" diff --git a/CLAUDE.md b/CLAUDE.md index 43df74f2..e4615ed5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -377,6 +377,17 @@ kbagent config search --query PATTERN [--project NAME] [--component-type TYPE] [ kbagent config update --project NAME --component-id ID --config-id ID [--name N] [--description D] [--configuration JSON|@file|-] [--configuration-file PATH] [--set PATH=VALUE ...] [--merge] [--change-description TEXT] [--dry-run] [--branch ID] [--allow-plaintext-on-encrypt-failure] kbagent config set-default-bucket --project NAME --component-id ID --config-id ID (--bucket BUCKET_ID | --clear) [--dry-run] [--branch ID] kbagent config rename --project NAME --component-id ID --config-id ID --name "New Name" [--branch ID] [--directory DIR] +kbagent config delete --project NAME --component-id ID --config-id ID [--branch ID] [--dry-run] +kbagent config restore --project NAME --component-id ID --config-id ID [--branch ID] +kbagent config trash-list --project NAME [--component-id ID] [--branch ID] +# config delete (0.89.0+ safety): SOFT delete into the Storage trash, with a locate-first guard. +# The raw API purges PERMANENTLY when DELETE hits a config already in the trash -- the classic +# agent retry after a timeout. kbagent now looks the config up first: live -> trash it; +# already trashed -> status "already_in_trash", exit 0, NO second DELETE ever sent; absent +# from both -> NOT_FOUND. Undo with `config restore`; browse candidates with `config +# trash-list`. Before 0.89.0 the second delete destroyed the config permanently. CLAUDE.md +# did not list `config delete` at all until 0.89.0 (silent drift) -- the command itself has +# existed for a long time. kbagent config variables-set --project NAME --component-id ID --config-id ID --var KEY=VALUE [--var ...] [--replace] [--variables-id ID] [--values-id ID] [--branch ID] [--dry-run] kbagent config variables-get --project NAME --component-id ID --config-id ID [--branch ID] kbagent config variables-clear --project NAME --component-id ID --config-id ID [--branch ID] [--yes] diff --git a/plugins/kbagent/.claude-plugin/plugin.json b/plugins/kbagent/.claude-plugin/plugin.json index 4c98bbc3..915f12fe 100644 --- a/plugins/kbagent/.claude-plugin/plugin.json +++ b/plugins/kbagent/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "kbagent", - "version": "0.88.0", + "version": "0.89.0", "description": "AI-friendly interface to Keboola Connection projects — explore configs, jobs, lineage, sync configs as files, manage dev branches, and debug SQL in workspaces", "author": { "name": "Keboola", diff --git a/plugins/kbagent/skills/kbagent/SKILL.md b/plugins/kbagent/skills/kbagent/SKILL.md index 120f68bd..168b26e9 100644 --- a/plugins/kbagent/skills/kbagent/SKILL.md +++ b/plugins/kbagent/skills/kbagent/SKILL.md @@ -113,7 +113,7 @@ When working inside a git repository or project directory, run `kbagent init` (o | Update a configuration's metadata and/or content | `kbagent config update --project PROJECT --component-id COMPONENT-ID --config-id CONFIG-ID` | | Set or clear ``storage.output.default_bucket`` on a configuration | `kbagent config set-default-bucket --project PROJECT --component-id COMPONENT-ID --config-id CONFIG-ID` | | Rename a configuration (update name via API + rename local sync directory) | `kbagent config rename --project PROJECT --component-id COMPONENT-ID --config-id CONFIG-ID --name NAME` | -| Delete a configuration from a project | `kbagent config delete --project PROJECT --component-id COMPONENT-ID --config-id CONFIG-ID` | +| Soft-delete a configuration into the trash (restorable) | `kbagent config delete --project PROJECT --component-id COMPONENT-ID --config-id CONFIG-ID` | | Generate boilerplate configuration files for a Keboola component, optionally creating the config remotely in one shot | `kbagent config new --component-id COMPONENT-ID` | | List all metadata entries on a configuration | `kbagent config metadata-list --project PROJECT --component-id COMPONENT-ID --config-id CONFIG-ID` | | Read a single metadata value by key | `kbagent config get-metadata --project PROJECT --component-id COMPONENT-ID --config-id CONFIG-ID --key KEY` | @@ -130,6 +130,8 @@ When working inside a git repository or project directory, run `kbagent init` (o | Overwrite the runtime ``state`` dict of a configuration or one of its rows | `kbagent config state-set --project PROJECT --component-id COMPONENT-ID --config-id CONFIG-ID --state STATE` | | Duplicate a configuration, whole -- including runtime, storage and authorization | `kbagent config clone --project PROJECT --component-id COMPONENT-ID --config-id CONFIG-ID --name NAME` | | Requires master token. | `kbagent config oauth-url --project PROJECT --component-id COMPONENT-ID --config-id CONFIG-ID` | +| Restore a configuration from the trash (undo of 'config delete') | `kbagent config restore --project PROJECT --component-id COMPONENT-ID --config-id CONFIG-ID` | +| List configurations in the trash (restorable via 'config restore') | `kbagent config trash-list --project PROJECT` | | List data apps across one or more registered projects | `kbagent data-app list` | | Show merged Data Science + Storage detail for one data app | `kbagent data-app detail --project PROJECT --app-id APP-ID` | | Create a Keboola data app end-to-end (POST + encrypt + PUT + deploy) | `kbagent data-app create --project PROJECT --name NAME --slug SLUG` | diff --git a/plugins/kbagent/skills/kbagent/references/commands-reference.md b/plugins/kbagent/skills/kbagent/references/commands-reference.md index 7baac186..35a7c14a 100644 --- a/plugins/kbagent/skills/kbagent/references/commands-reference.md +++ b/plugins/kbagent/skills/kbagent/references/commands-reference.md @@ -128,7 +128,9 @@ Requires a **super-admin** Manage API token (same kind as `org setup`). Same def - `config update --project NAME --component-id ID --config-id ID [--name N] [--description D] [--configuration JSON|@file|-] [--configuration-file PATH] [--set PATH=VALUE ...] [--merge] [--change-description TEXT] [--dry-run] [--branch ID] [--allow-plaintext-on-encrypt-failure]` -- update metadata and/or configuration content. `--change-description` sets the new config version's `changeDescription` (the version-history audit line); omit it to keep the auto-generated default (e.g. `Updated configuration via kbagent config update`). Distinct from `--description`, which sets the config's display description. `--dry-run` echoes the `change_description` that would be sent. **`#`-prefixed secrets auto-encrypt via the Encryption API before write (fail-closed; since 0.54.0, #378)** -- `--allow-plaintext-on-encrypt-failure` overrides, `--dry-run` keeps plaintext in the diff (ciphertext is non-deterministic). Note `--set '#password=...'` sets a *top-level* key; for a nested secret use `--set 'parameters.#password=...'`. `--set` targets a nested key (e.g. `parameters.db.host=new-host`). `--merge` deep-merges into existing config (preserves sibling keys). `--dry-run` previews changes without applying. Paths are relative to the configuration root. **Auto-normalize (0.28.0+; #245 / 0.31.0+; #274)**: `parameters.blocks[].codes[].script` is fixed before pushing to Storage API. **String -> array** (0.28.0+; #245): SQL transformations get statement-level split (respects `'...'` / `"..."` / `$$..$$` / `--` / `#` / `//` / `/* ... */`); Python / R / `kds-team.app-custom-python` get `[script]` wrap. **List-element re-split** (0.31.0+; #274): when `script` is already a list but an element packs multiple `;`-separated statements, each SQL element is re-run through `split_statements()` and replaced inline. Closes the ODBC `Actual statement count N did not match the desired statement count 1` (SQL state 0A000) runtime crash that survives the 0.28.0 string fix. The result envelope's `normalizations: [{path, action: "sql_split"|"wrap_array"|"sql_resplit", before_type, after_type, after_length, before_length?}]` records every change (empty when nothing was malformed; `sql_resplit` adds `before_length` and a `[E]` suffix on `path` pointing at the original element index). Bypassing kbagent (raw REST) does NOT inherit either pass -- prefer `kbagent config update` for SQL transformation body changes. - `config set-default-bucket --project NAME --component-id ID --config-id ID (--bucket BUCKET_ID | --clear) [--dry-run] [--branch ID]` -- set or clear `configuration.storage.output.default_bucket` on a configuration. Discoverable shortcut for the raw-mode workaround at https://keboola.atlassian.net/wiki/spaces/SUP/pages/3770155030/. Read-modify-write that preserves sibling keys; returns `{"changed": false}` when the value already matches the requested state. Honored by output tables that don't pin their own `destination`. - `config rename --project NAME --component-id ID --config-id ID --name "New Name" [--branch ID] [--directory DIR]` -- rename a configuration (API update + local sync directory rename with git mv support) -- `config delete --project NAME --component-id ID --config-id ID [--branch ID]` -- delete a configuration +- `config delete --project NAME --component-id ID --config-id ID [--branch ID] [--dry-run]` -- SOFT-delete a configuration into the Storage trash (restorable). Since 0.89.0 it locates the config first and a config already in the trash is NOT deleted again -- the raw API purges permanently on a second DELETE (the retry-after-timeout trap); kbagent reports `already_in_trash` and exits 0 instead. `--dry-run` reports the located state without writing. Undo with `config restore`. Permission class `destructive` +- `config restore --project NAME --component-id ID --config-id ID [--branch ID]` *(since v0.89.0)* -- restore a trashed configuration (versions, rows and metadata come back). Only works on a config currently in the trash. Permission class `write` +- `config trash-list --project NAME [--component-id ID] [--branch ID]` *(since v0.89.0)* -- list configurations in the trash; each row carries `component_id`, `config_id`, `name`, `version` and `deleted_at`, which is exactly what `config restore` needs. Permission class `read` - `config new --component-id ID [--project NAME] [--name NAME] [--output-dir DIR] [--push --no-files --description D --configuration JSON|@file|- --configuration-file PATH --no-validate --branch ID --dry-run --allow-plaintext-on-encrypt-failure]` -- **two modes**. **Default (no `--push`)**: scaffold new config from component schema; writes files to `--output-dir` or prints to stdout. **Zero API calls.** **With `--push`** (0.33.0+, requires `--project` + non-empty `--name`): also POSTs to `/v2/storage/components/{cid}/configs` for a one-shot remote create. `#`-prefixed secrets in the pushed body auto-encrypt via the Encryption API first (fail-closed; since 0.54.0, #378; `--allow-plaintext-on-encrypt-failure` overrides). `--no-files` skips the filesystem step entirely (FIIA-style empty-shell pattern). `--configuration` / `--configuration-file` override the POSTed body (default is `{}`, with validation auto-skipped for the default empty shell). `--dry-run` previews the planned POST + validation result without creating. Schema validation runs by default when an explicit body is given (fail-closed: `ConfigError` exit 5 on mismatch) but skips silently if the AI Service has no schema for the component or returns an error; `--no-validate` opts out. Works for ALL component types including `keboola.snowflake-transformation`. - `config clone --project P --component-id ID --config-id ID --name NAME [--target-project P2] [--description D] [--set PATH=VALUE ...] [--secret PATH=VALUE ...] [--branch ID] [--target-branch ID] [--dry-run] [--allow-plaintext-on-encrypt-failure]` (0.84.2+, #587) -- duplicate a configuration **whole**. Reach for this instead of reading `config detail` and rebuilding a body: copying only `configuration["parameters"]` silently drops its siblings (`runtime`, `storage`, `authorization`), and a lost `runtime.parallelism` makes Keboola fall back to `parallelism: 1` -- the reporter's 65-row writer went sequential, 140 min instead of ~60-90, with nothing in any output pointing at it. **Same project** (default): server-side copy via `POST .../configs/{id}/versions/{v}/create`; rows and `KBC::` encrypted values travel with it (verified live). `--set PATH=VALUE` is applied as a follow-up update on the copy, so an override can never be the reason a key went missing. **Cross project** (`--target-project`): reassembled client-side and rows recreated one by one, because encrypted values **cannot** travel -- a Keboola ciphertext is scoped to the project it was encrypted in. Any `KBC::` value makes the clone **fail with exit 5**, listing every path, until re-supplied via `--secret PATH=VALUE` (encrypted in the TARGET project on write). `--dry-run` reports those paths instead of refusing -- run it first to learn what to gather. Storage bucket/table IDs are copied **verbatim, never remapped**; `sync clone` is the command that remaps. - `config variables-set --project NAME --component-id ID --config-id ID --var KEY=VALUE [--var ...] [--replace] [--variables-id ID] [--values-id ID] [--branch ID] [--dry-run] [--allow-plaintext-on-encrypt-failure] [--yes]` -- attach variable values to a config. Auto-creates a sibling `keboola.variables` config + default row on first use and links it via the parent's `runtime.variables_id` / `variables_values_id`. Defaults to merge; `--replace` drops keys not in `--var`. `#`-prefixed values encrypt via the Encryption API (fail-closed; exit non-zero on `ENCRYPTION_FAILED`). See `variables-workflow.md` diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index 599b33f7..e67d2c4c 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -1327,6 +1327,30 @@ events and emits a final `done` SSE frame mirroring the same record. error names the created configuration id and how many rows landed, so you can delete it and re-run. +## `config delete` twice = permanent purge -- kbagent now refuses the second one (since v0.89.0) + +The Storage API overloads `DELETE .../configs/{id}`: on a live configuration +it soft-deletes into the trash (restorable), but on a configuration ALREADY in +the trash the same call **purges it permanently** -- versions, rows and +metadata gone, no restore. The classic way to hit this is an agent retry: the +first DELETE times out client-side after the server already trashed the +config, the retry fires, and the retry destroys it for good. + +- Since 0.89.0 `config delete` locates the configuration first and never + sends a DELETE at anything that is not live. A config already in the trash + answers `status: "already_in_trash"` with **exit 0** (the retry stays + idempotent for scripts) and a pointer to `config restore`. A config in + neither place is NOT_FOUND. +- `config restore --project P --component-id C --config-id ID` is the undo; + `config trash-list` shows what is restorable. Restore brings back versions, + rows and metadata. +- **On kbagent <= 0.88.x the guard does not exist** -- a repeated + `config delete` there purges permanently. When driving an older kbagent, + never blind-retry a delete; check `config list` first. +- Direct API callers: the purge-safe alternative is the dedicated + `POST .../configs/{id}/purge` endpoint (fails with 400 when the config is + not in the trash), never a second DELETE. + ## `data-app` JSON output: key for the app's own id is `app_id` (since v0.33.0) - Every `kbagent --json data-app ` envelope emits the diff --git a/pyproject.toml b/pyproject.toml index f833d98e..6e0626ce 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "keboola-cli" -version = "0.88.0" +version = "0.89.0" description = "AI-friendly CLI for managing Keboola projects" readme = "README.md" requires-python = ">=3.12" diff --git a/src/keboola_agent_cli/changelog.py b/src/keboola_agent_cli/changelog.py index 505abbd1..80f3aeb6 100644 --- a/src/keboola_agent_cli/changelog.py +++ b/src/keboola_agent_cli/changelog.py @@ -24,6 +24,28 @@ # Ordered newest-first. Each value is a list of brief one-line descriptions. CHANGELOG: dict[str, list[str]] = { + "0.89.0": [ + "Fix: `config delete` can no longer permanently purge a configuration by being run " + "twice. The Storage API overloads DELETE -- on a live configuration it soft-deletes " + "into the trash, but on a configuration ALREADY in the trash the same call purges it " + "permanently, versions, rows and metadata included. A timed-out delete followed by a " + "retry is exactly that second call, and retrying on timeout is what every agent and CI " + "script does. `config delete` now locates the configuration first and never sends a " + "DELETE at anything that is not live: already trashed answers `already_in_trash` with " + "exit 0 (the retry stays idempotent), absent from both answers NOT_FOUND. `--dry-run` " + "reports the located state without writing.", + "New: `kbagent config restore` -- the undo for `config delete`. Restores a trashed " + "configuration with its versions, rows and metadata (`POST .../configs/{id}/restore`). " + "Only works on a configuration currently in the trash.", + "New: `kbagent config trash-list` lists configurations in the trash, project-wide or " + "narrowed by `--component-id`. Each row carries component_id, config_id, name, version " + "and deleted_at -- exactly what `config restore` needs.", + "Note: all three are mirrored on `kbagent serve`. `DELETE /configs/...` gains " + "`dry_run`; `POST /configs/{p}/{c}/{id}/restore` and `GET /configs/trash/{p}` are new.", + "Plugin docs: `CLAUDE.md`'s command list had never included `config delete` at all. " + "That silent drift made the command look nonexistent to AI agents reading it. " + "Added alongside the new commands, with the double-delete trap recorded in gotchas.md.", + ], "0.88.0": [ "Fix (#624): column descriptions are now written where the Keboola UI and the " "MCP server actually read them. Until 0.87.0 `storage describe-column` / " diff --git a/src/keboola_agent_cli/client/configs.py b/src/keboola_agent_cli/client/configs.py index 4e784a77..c4a67e5e 100644 --- a/src/keboola_agent_cli/client/configs.py +++ b/src/keboola_agent_cli/client/configs.py @@ -789,6 +789,64 @@ def rebase_config_delete( """ return self._rebase_request(component_id, config_id, branch_id, version, diff={}) + def list_deleted_configs( + self, + component_id: str | None = None, + branch_id: int | None = None, + ) -> list[dict[str, Any]]: + """List configurations sitting in the trash (``isDeleted=true``). + + With ``component_id`` this hits the per-component listing and returns + the trashed configuration dicts directly. Without it, it walks + ``GET /components?isDeleted=true`` -- which groups configurations + under their component -- and flattens the result, stamping each + configuration with its ``component_id`` so callers get one uniform + shape either way. + + A trashed configuration is invisible to the normal listings and a + direct ``GET .../configs/{id}`` answers 404 for it, so this endpoint + is the only way to tell "in the trash" apart from "never existed" -- + the distinction :meth:`restore_config` and the double-delete guard in + the service layer both depend on. + """ + prefix = f"/v2/storage/branch/{branch_id}" if branch_id else "/v2/storage" + if component_id is not None: + resp = self._request( + "GET", + f"{prefix}/components/{quote(component_id, safe='')}/configs", + params={"isDeleted": "true"}, + ) + return resp.json() + resp = self._request( + "GET", + f"{prefix}/components", + params={"isDeleted": "true"}, + ) + flat: list[dict[str, Any]] = [] + for component in resp.json(): + for config in component.get("configurations", []): + config["component_id"] = component.get("id") + flat.append(config) + return flat + + def restore_config( + self, component_id: str, config_id: str, branch_id: int | None = None + ) -> dict[str, Any]: + """Restore a trashed configuration (``POST .../configs/{id}/restore``). + + Only works on a configuration that is currently in the trash; the API + rejects a restore of a live configuration. Returns the restored + configuration body. + """ + safe_component = quote(component_id, safe="") + safe_config = quote(config_id, safe="") + prefix = f"/v2/storage/branch/{branch_id}" if branch_id else "/v2/storage" + resp = self._request( + "POST", + f"{prefix}/components/{safe_component}/configs/{safe_config}/restore", + ) + return resp.json() + def delete_config( self, component_id: str, config_id: str, branch_id: int | None = None ) -> None: diff --git a/src/keboola_agent_cli/commands/_config_trash_cmd.py b/src/keboola_agent_cli/commands/_config_trash_cmd.py new file mode 100644 index 00000000..1d6cbf68 --- /dev/null +++ b/src/keboola_agent_cli/commands/_config_trash_cmd.py @@ -0,0 +1,119 @@ +"""``kbagent config restore`` + ``config trash-list`` -- the undo side of delete. + +``config delete`` is a soft delete into the Storage trash; these two commands +make that reversible from the CLI. Lives in a private module because +``commands/config.py`` is over its size ceiling (``make loc-check``); mounted +onto ``config_app`` via :func:`register`, so the permission keys stay +``config.restore`` / ``config.trash-list``. +""" + +from __future__ import annotations + +import typer +from rich.markup import escape + +from ..errors import ConfigError, ErrorCode, KeboolaApiError +from ._helpers import get_formatter, get_service, map_error_to_exit_code + + +def register(app: typer.Typer) -> None: + """Mount restore + trash-list onto ``app`` (the ``config`` Typer group).""" + + @app.command("restore", rich_help_panel="Lifecycle") + def config_restore( + ctx: typer.Context, + project: str = typer.Option(..., "--project", help="Project alias"), + component_id: str = typer.Option( + ..., "--component-id", help="Component ID (e.g. keboola.snowflake-transformation)" + ), + config_id: str = typer.Option(..., "--config-id", help="Trashed configuration ID"), + branch: int | None = typer.Option( + None, "--branch", help="Restore in a specific dev branch ID (defaults to active branch)" + ), + ) -> None: + """Restore a configuration from the trash (undo of 'config delete'). + + Only a trashed configuration can be restored; find candidates with + 'config trash-list'. Restoring brings back the configuration with its + versions, rows and metadata. + """ + formatter = get_formatter(ctx) + service = get_service(ctx, "config_service") + try: + result = service.restore_config( + alias=project, + component_id=component_id, + config_id=config_id, + branch_id=branch, + ) + except ConfigError as exc: + formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR) + raise typer.Exit(code=5) from None + except KeboolaApiError as exc: + formatter.error(message=exc.message, error_code=exc.error_code) + raise typer.Exit(code=map_error_to_exit_code(exc)) from None + + if formatter.json_mode: + formatter.output(result) + else: + formatter.success( + f"Restored config {result['component_id']}/{result['config_id']} " + f"('{result.get('name')}', version {result.get('version')}) " + f"in project '{result['project_alias']}'" + ) + + @app.command("trash-list", rich_help_panel="Lifecycle") + def config_trash_list( + ctx: typer.Context, + project: str = typer.Option(..., "--project", help="Project alias"), + component_id: str | None = typer.Option( + None, "--component-id", help="Limit to one component's trashed configurations" + ), + branch: int | None = typer.Option( + None, "--branch", help="List a specific dev branch's trash (defaults to active branch)" + ), + ) -> None: + """List configurations in the trash (restorable via 'config restore').""" + formatter = get_formatter(ctx) + service = get_service(ctx, "config_service") + try: + result = service.list_config_trash( + alias=project, + component_id=component_id, + branch_id=branch, + ) + except ConfigError as exc: + formatter.error(message=exc.message, error_code=ErrorCode.CONFIG_ERROR) + raise typer.Exit(code=5) from None + except KeboolaApiError as exc: + formatter.error(message=exc.message, error_code=exc.error_code) + raise typer.Exit(code=map_error_to_exit_code(exc)) from None + + if formatter.json_mode: + formatter.output(result) + return + entries = result["trash"] + if not entries: + formatter.console.print("Trash is empty.") + return + from rich.table import Table + + table = Table(title=f"Trashed configurations -- {escape(project)} ({len(entries)})") + table.add_column("Component", style="dim") + table.add_column("Config ID", style="bold cyan") + table.add_column("Name") + table.add_column("Version", justify="right", style="dim") + table.add_column("Deleted at", style="dim") + for e in entries: + table.add_row( + str(e.get("component_id") or ""), + str(e.get("config_id") or ""), + escape(str(e.get("name") or "")), + str(e.get("version") or ""), + str(e.get("deleted_at") or ""), + ) + formatter.console.print(table) + formatter.console.print( + "[dim]Restore with: kbagent config restore --project " + f"{escape(project)} --component-id --config-id [/dim]" + ) diff --git a/src/keboola_agent_cli/commands/config.py b/src/keboola_agent_cli/commands/config.py index 6f923fca..17fb0212 100644 --- a/src/keboola_agent_cli/commands/config.py +++ b/src/keboola_agent_cli/commands/config.py @@ -1115,12 +1115,21 @@ def config_delete( "--branch", help="Delete from a specific dev branch ID (defaults to active branch)", ), + dry_run: bool = typer.Option( + False, + "--dry-run", + help="Report what would happen (live / already in trash) without deleting", + ), ) -> None: - """Delete a configuration from a project. + """Soft-delete a configuration into the trash (restorable). If a dev branch is active (via 'branch use'), the deletion targets that branch. Use --branch to override. Deleting in a branch marks the config as removed without affecting Main. + + A configuration already in the trash is NOT deleted again -- the API + would purge it permanently -- the command reports 'already_in_trash' + and exits 0. Undo with 'config restore'; browse with 'config trash-list'. """ formatter = get_formatter(ctx) service = get_service(ctx, "config_service") @@ -1131,6 +1140,7 @@ def config_delete( component_id=component_id, config_id=config_id, branch_id=branch, + dry_run=dry_run, ) except (ConfigError, KeboolaApiError) as exc: _handle_config_service_error(formatter, exc) @@ -1141,10 +1151,21 @@ def config_delete( branch_info = "" if result.get("branch_id"): branch_info = f" (branch {result['branch_id']})" - formatter.success( - f"Deleted config {result['component_id']}/{result['config_id']} " - f"from project '{result['project_alias']}'{branch_info}" - ) + status = result.get("status") + if status == "already_in_trash": + formatter.console.print(f"[yellow]{result['message']}[/yellow]") + elif status == "would_delete": + formatter.console.print( + f"[bold yellow]DRY RUN[/bold yellow] -- would move config " + f"{result['component_id']}/{result['config_id']} to the trash" + f"{branch_info} (restorable via 'config restore')" + ) + else: + formatter.success( + f"Moved config {result['component_id']}/{result['config_id']} " + f"to the trash in project '{result['project_alias']}'{branch_info} " + f"(undo: kbagent config restore)" + ) # --- File extension to Rich Syntax lexer mapping --- @@ -2470,6 +2491,8 @@ def config_row_delete( # the `config.*` permission namespace and appears in `kbagent config --help`. from ._config_clone_cmd import register as _register_clone_command # noqa: E402 from ._config_oauth import register as _register_oauth_command # noqa: E402 +from ._config_trash_cmd import register as _register_trash_commands # noqa: E402 _register_clone_command(config_app) _register_oauth_command(config_app) +_register_trash_commands(config_app) diff --git a/src/keboola_agent_cli/commands/context.py b/src/keboola_agent_cli/commands/context.py index 1ddaff22..9c924cc2 100644 --- a/src/keboola_agent_cli/commands/context.py +++ b/src/keboola_agent_cli/commands/context.py @@ -405,8 +405,19 @@ exists (.keboola/manifest.json), renames the directory and updates the manifest path. Uses git mv when inside a git repo for cleaner history. - kbagent config delete --project NAME --component-id ID --config-id ID [--branch ID] - Delete a configuration. Branch-aware. + kbagent config delete --project NAME --component-id ID --config-id ID [--branch ID] [--dry-run] + Soft-delete a configuration into the Storage trash (restorable). Branch-aware. + Locates the config first: one already in the trash is NOT deleted again + (the raw API would purge it permanently on the second DELETE -- the classic + retry-after-timeout trap); it reports status "already_in_trash" and exits 0. + --dry-run reports the located state without writing. + + kbagent config restore --project NAME --component-id ID --config-id ID [--branch ID] + Restore a trashed configuration -- the undo for `config delete`. Only works + on a config currently in the trash; brings back versions, rows and metadata. + + kbagent config trash-list --project NAME [--component-id ID] [--branch ID] + List configurations in the trash (what `config restore` can bring back). kbagent config new --component-id ID [--name NAME] [--project NAME] [--output-dir DIR] [--push --no-files --description D --configuration JSON|@file|- --configuration-file PATH --no-validate --branch ID --dry-run --allow-plaintext-on-encrypt-failure] diff --git a/src/keboola_agent_cli/permissions.py b/src/keboola_agent_cli/permissions.py index e53f31c5..0c08e0cc 100644 --- a/src/keboola_agent_cli/permissions.py +++ b/src/keboola_agent_cli/permissions.py @@ -74,6 +74,8 @@ "config.set-default-bucket": "write", "config.rename": "write", "config.delete": "destructive", + "config.restore": "write", + "config.trash-list": "read", "config.new": "write", # Creates a new configuration; never modifies or deletes the source. "config.clone": "write", diff --git a/src/keboola_agent_cli/server/routers/configs.py b/src/keboola_agent_cli/server/routers/configs.py index 442698ea..1bb2a73f 100644 --- a/src/keboola_agent_cli/server/routers/configs.py +++ b/src/keboola_agent_cli/server/routers/configs.py @@ -205,14 +205,55 @@ def config_delete( component_id: str, config_id: str, branch_id: int | None = None, + dry_run: bool = False, registry: ServiceRegistry = Depends(get_registry), ) -> dict[str, Any]: - """Delete a component configuration.""" + """Soft-delete a component configuration into the trash. + + An already-trashed configuration is never deleted again (the API would + purge it permanently); the response reports ``already_in_trash`` instead. + """ return registry.config.delete_config( alias=project, component_id=component_id, config_id=config_id, branch_id=branch_id, + dry_run=dry_run, + ) + + +@router.post( + "/{project}/{component_id}/{config_id}/restore", + summary="Restore a configuration from the trash", +) +def config_restore( + project: str, + component_id: str, + config_id: str, + branch_id: int | None = None, + registry: ServiceRegistry = Depends(get_registry), +) -> dict[str, Any]: + """Restore a trashed configuration. Mirrors `kbagent config restore`.""" + return registry.config.restore_config( + alias=project, + component_id=component_id, + config_id=config_id, + branch_id=branch_id, + ) + + +@router.get("/trash/{project}", summary="List trashed configurations") +def config_trash_list( + project: str, + component_id: str | None = None, + branch_id: int | None = None, + registry: ServiceRegistry = Depends(get_registry), +) -> dict[str, Any]: + """List configurations in the trash. Mirrors `kbagent config trash-list`.""" + return registry.config.list_config_trash( + alias=project, + component_id=component_id, + branch_id=branch_id, ) diff --git a/src/keboola_agent_cli/services/_config_trash.py b/src/keboola_agent_cli/services/_config_trash.py new file mode 100644 index 00000000..26dc31cb --- /dev/null +++ b/src/keboola_agent_cli/services/_config_trash.py @@ -0,0 +1,91 @@ +"""Trash-safety helpers for configuration delete / restore (issue: double-delete purge). + +The Storage API gives ``DELETE .../configs/{id}`` two different meanings +depending on state: on a LIVE configuration it is a soft delete into the +trash, but on a configuration ALREADY in the trash it is a permanent purge -- +versions, rows and metadata gone, no restore. That second meaning is exactly +what a retrying agent triggers: request times out after the server already +trashed the config, the client retries, and the retry destroys it for good. + +These helpers give the service layer a way to never issue that second DELETE: +look the configuration up first, and when it is not live, consult the trash +listing to answer "already trashed" or "does not exist" explicitly. + +Kept out of ``config_service.py`` because that module is over its size budget +(``make loc-check``): the service methods stay thin and delegate here. +""" + +from __future__ import annotations + +from typing import Any + +from ..errors import KeboolaApiError + +# States a delete/restore attempt can find the configuration in. +STATE_LIVE = "live" +STATE_TRASHED = "trashed" +STATE_MISSING = "missing" + + +def locate_config( + client: Any, + component_id: str, + config_id: str, + branch_id: int | None, +) -> str: + """Answer whether a configuration is live, in the trash, or absent. + + A direct ``GET .../configs/{id}`` answers 404 for BOTH a trashed and a + never-existed configuration, so a 404 alone cannot drive the delete + decision -- the trash listing is what separates the two. + """ + try: + client.get_config_detail(component_id, config_id, branch_id=branch_id) + return STATE_LIVE + except KeboolaApiError as exc: + if exc.status_code != 404: + raise + trashed = client.list_deleted_configs(component_id=component_id, branch_id=branch_id) + if any(str(cfg.get("id")) == str(config_id) for cfg in trashed): + return STATE_TRASHED + return STATE_MISSING + + +def already_trashed_result( + alias: str, + component_id: str, + config_id: str, + branch_id: int | None, +) -> dict[str, Any]: + """The refusal envelope for a delete aimed at an already-trashed config. + + Status ``already_in_trash`` (not ``deleted``) so a caller inspecting the + result sees that THIS invocation changed nothing -- while an agent + blindly checking the exit code still gets the idempotent success it + expects from a retry. + """ + return { + "status": "already_in_trash", + "project_alias": alias, + "component_id": component_id, + "config_id": config_id, + "branch_id": branch_id, + "message": ( + f"Configuration '{component_id}/{config_id}' is already in the trash; " + "not deleting again (a second DELETE would purge it permanently). " + "Use 'kbagent config restore' to bring it back." + ), + } + + +def shape_trash_entry(config: dict[str, Any], component_id: str | None) -> dict[str, Any]: + """One uniform row for ``config trash-list`` output.""" + current = config.get("currentVersion") or {} + return { + "component_id": config.get("component_id") or component_id, + "config_id": config.get("id"), + "name": config.get("name"), + "version": config.get("version"), + "deleted_change_description": current.get("changeDescription"), + "deleted_at": current.get("created"), + } diff --git a/src/keboola_agent_cli/services/config_service.py b/src/keboola_agent_cli/services/config_service.py index adaf2775..2aac5043 100644 --- a/src/keboola_agent_cli/services/config_service.py +++ b/src/keboola_agent_cli/services/config_service.py @@ -1220,8 +1220,21 @@ def delete_config( component_id: str, config_id: str, branch_id: int | None = None, + dry_run: bool = False, ) -> dict[str, Any]: - """Delete a configuration from a project. + """Soft-delete a configuration into the trash, never past it. + + The Storage API overloads DELETE: on a live configuration it moves it + to the trash (restorable), but on a configuration ALREADY in the + trash the same call purges it permanently -- versions, rows and + metadata included. A timed-out delete followed by a retry is exactly + that second call, so this method locates the configuration first and + refuses to issue a DELETE at anything that is not live: + + * live -> DELETE, status ``deleted`` + * already in the trash -> no API write, status ``already_in_trash`` + (exit stays 0 -- the retry is idempotent, the purge never happens) + * absent from both -> NOT_FOUND Args: alias: Project alias. @@ -1229,14 +1242,18 @@ def delete_config( config_id: The configuration ID to delete. branch_id: If set, delete from a specific dev branch. If None, uses the project's active branch (if any). + dry_run: Report the located state without deleting anything. Returns: Dict with deletion confirmation details. Raises: ConfigError: If the alias is not found. - KeboolaApiError: If the API call fails. + KeboolaApiError: If the API call fails or the configuration does + not exist in either the live listing or the trash. """ + from . import _config_trash as trash + projects = self.resolve_projects([alias]) project = projects[alias] @@ -1245,6 +1262,31 @@ def delete_config( client = self._client_factory(project.stack_url, project.token) try: + state = trash.locate_config(client, component_id, config_id, effective_branch_id) + if state == trash.STATE_MISSING: + raise KeboolaApiError( + message=( + f"Configuration '{component_id}/{config_id}' not found -- " + "neither live nor in the trash." + ), + error_code=ErrorCode.NOT_FOUND, + status_code=404, + ) + if state == trash.STATE_TRASHED: + result = trash.already_trashed_result( + alias, component_id, config_id, effective_branch_id + ) + result["dry_run"] = dry_run + return result + if dry_run: + return { + "status": "would_delete", + "dry_run": True, + "project_alias": alias, + "component_id": component_id, + "config_id": config_id, + "branch_id": effective_branch_id, + } client.delete_config( component_id=component_id, config_id=config_id, @@ -1255,10 +1297,67 @@ def delete_config( return { "status": "deleted", + "dry_run": False, + "project_alias": alias, + "component_id": component_id, + "config_id": config_id, + "branch_id": effective_branch_id, + } + + def restore_config( + self, + alias: str, + component_id: str, + config_id: str, + branch_id: int | None = None, + ) -> dict[str, Any]: + """Restore a trashed configuration (the undo for :meth:`delete_config`).""" + projects = self.resolve_projects([alias]) + project = projects[alias] + effective_branch_id = branch_id or project.active_branch_id + client = self._client_factory(project.stack_url, project.token) + try: + restored = client.restore_config( + component_id=component_id, + config_id=config_id, + branch_id=effective_branch_id, + ) + finally: + client.close() + return { + "status": "restored", "project_alias": alias, "component_id": component_id, "config_id": config_id, "branch_id": effective_branch_id, + "name": restored.get("name"), + "version": restored.get("version"), + } + + def list_config_trash( + self, + alias: str, + component_id: str | None = None, + branch_id: int | None = None, + ) -> dict[str, Any]: + """List trashed configurations (what :meth:`restore_config` can bring back).""" + from . import _config_trash as trash + + projects = self.resolve_projects([alias]) + project = projects[alias] + effective_branch_id = branch_id or project.active_branch_id + client = self._client_factory(project.stack_url, project.token) + try: + raw = client.list_deleted_configs( + component_id=component_id, branch_id=effective_branch_id + ) + finally: + client.close() + return { + "project_alias": alias, + "branch_id": effective_branch_id, + "component_id": component_id, + "trash": [trash.shape_trash_entry(cfg, component_id) for cfg in raw], } def rename_config( diff --git a/tests/test_config_trash.py b/tests/test_config_trash.py new file mode 100644 index 00000000..c649eaf9 --- /dev/null +++ b/tests/test_config_trash.py @@ -0,0 +1,334 @@ +"""Tests for the config trash safety net: delete preflight, restore, trash-list. + +The Storage API overloads ``DELETE .../configs/{id}``: on a live configuration +it soft-deletes into the trash, but on a configuration ALREADY in the trash it +purges permanently -- versions, rows and metadata gone. A timed-out delete +followed by a retry is exactly that second call, so ``delete_config`` must +locate the configuration first and never issue a DELETE at anything but a +live config. The assertions on ``client.delete_config.assert_not_called()`` +are the point of this file: they prove the purge call cannot happen. +""" + +import json +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest +from typer.testing import CliRunner, Result + +from helpers import setup_single_project +from keboola_agent_cli.cli import app +from keboola_agent_cli.config_store import ConfigStore +from keboola_agent_cli.errors import ErrorCode, KeboolaApiError +from keboola_agent_cli.models import ProjectConfig +from keboola_agent_cli.services.config_service import ConfigService +from keboola_agent_cli.services.project_service import ProjectService + +TEST_TOKEN = "901-55555-fakeTestTokenDoNotUseXXXXXXXX" + +runner = CliRunner() + + +def _not_found() -> KeboolaApiError: + return KeboolaApiError( + message="Configuration not found", + error_code=ErrorCode.NOT_FOUND, + status_code=404, + ) + + +def _make_service( + tmp_config_dir: Path, + *, + live: bool, + in_trash: bool, +) -> tuple[ConfigService, MagicMock]: + """ConfigService over a mock client representing one config state.""" + store = setup_single_project(tmp_config_dir) + client = MagicMock() + if live: + client.get_config_detail.return_value = {"id": "cfg-1", "name": "Probe"} + else: + client.get_config_detail.side_effect = _not_found() + client.list_deleted_configs.return_value = ( + [{"id": "cfg-1", "name": "Probe", "version": 3}] if in_trash else [] + ) + client.restore_config.return_value = {"id": "cfg-1", "name": "Probe", "version": 3} + service = ConfigService(config_store=store, client_factory=lambda url, token: client) + return service, client + + +class TestDeletePreflight: + """delete_config never sends the DELETE that would purge a trashed config.""" + + def test_live_config_is_deleted(self, tmp_config_dir: Path) -> None: + service, client = _make_service(tmp_config_dir, live=True, in_trash=False) + result = service.delete_config("prod", "keboola.comp", "cfg-1") + assert result["status"] == "deleted" + client.delete_config.assert_called_once() + + def test_trashed_config_is_never_deleted_again(self, tmp_config_dir: Path) -> None: + """The retry scenario: config already in trash -> refuse the second DELETE.""" + service, client = _make_service(tmp_config_dir, live=False, in_trash=True) + result = service.delete_config("prod", "keboola.comp", "cfg-1") + assert result["status"] == "already_in_trash" + assert "restore" in result["message"] + client.delete_config.assert_not_called() + + def test_missing_config_raises_not_found(self, tmp_config_dir: Path) -> None: + """Absent from live AND trash -> NOT_FOUND, and no blind DELETE either.""" + service, client = _make_service(tmp_config_dir, live=False, in_trash=False) + with pytest.raises(KeboolaApiError) as exc_info: + service.delete_config("prod", "keboola.comp", "cfg-1") + assert exc_info.value.error_code == ErrorCode.NOT_FOUND + client.delete_config.assert_not_called() + + def test_non_404_lookup_error_propagates(self, tmp_config_dir: Path) -> None: + """A 500 on the preflight must not be misread as 'not live' -- it aborts.""" + service, client = _make_service(tmp_config_dir, live=True, in_trash=False) + client.get_config_detail.side_effect = KeboolaApiError( + message="boom", error_code=ErrorCode.API_ERROR, status_code=500 + ) + with pytest.raises(KeboolaApiError): + service.delete_config("prod", "keboola.comp", "cfg-1") + client.delete_config.assert_not_called() + + def test_dry_run_on_live_config_writes_nothing(self, tmp_config_dir: Path) -> None: + service, client = _make_service(tmp_config_dir, live=True, in_trash=False) + result = service.delete_config("prod", "keboola.comp", "cfg-1", dry_run=True) + assert result["status"] == "would_delete" + assert result["dry_run"] is True + client.delete_config.assert_not_called() + + def test_dry_run_on_trashed_config_reports_state(self, tmp_config_dir: Path) -> None: + service, client = _make_service(tmp_config_dir, live=False, in_trash=True) + result = service.delete_config("prod", "keboola.comp", "cfg-1", dry_run=True) + assert result["status"] == "already_in_trash" + client.delete_config.assert_not_called() + + +class TestRestore: + def test_restore_returns_name_and_version(self, tmp_config_dir: Path) -> None: + service, client = _make_service(tmp_config_dir, live=False, in_trash=True) + result = service.restore_config("prod", "keboola.comp", "cfg-1") + assert result["status"] == "restored" + assert result["name"] == "Probe" + assert result["version"] == 3 + client.restore_config.assert_called_once_with( + component_id="keboola.comp", config_id="cfg-1", branch_id=None + ) + + +class TestTrashList: + def test_component_scope_passes_through(self, tmp_config_dir: Path) -> None: + service, client = _make_service(tmp_config_dir, live=False, in_trash=True) + result = service.list_config_trash("prod", component_id="keboola.comp") + assert result["trash"][0]["config_id"] == "cfg-1" + assert result["trash"][0]["component_id"] == "keboola.comp" + client.list_deleted_configs.assert_called_once_with( + component_id="keboola.comp", branch_id=None + ) + + def test_project_wide_uses_flattened_component_id(self, tmp_config_dir: Path) -> None: + """Without --component-id the client stamps component_id per entry.""" + service, client = _make_service(tmp_config_dir, live=False, in_trash=False) + client.list_deleted_configs.return_value = [ + {"id": "a", "name": "A", "version": 1, "component_id": "keboola.x"}, + ] + result = service.list_config_trash("prod") + assert result["trash"][0]["component_id"] == "keboola.x" + + +# --- CLI layer --------------------------------------------------------------- + + +def _setup_store(config_dir: Path) -> ConfigStore: + store = ConfigStore(config_dir=config_dir) + store.add_project( + "prod", + ProjectConfig( + stack_url="https://connection.keboola.com", + token=TEST_TOKEN, + project_name="Production", + project_id=1234, + ), + ) + return store + + +def _invoke(args: list[str], *, config_dir: Path, svc: MagicMock) -> Result: + store = _setup_store(config_dir) + with ( + patch("keboola_agent_cli.cli.ConfigStore") as MockStore, + patch("keboola_agent_cli.cli.ProjectService") as MockProjService, + patch("keboola_agent_cli.cli.ConfigService") as MockConfigService, + ): + MockStore.return_value = store + MockProjService.return_value = ProjectService(config_store=store) + MockConfigService.return_value = svc + return runner.invoke(app, args) + + +class TestRestoreCli: + def test_json_envelope(self, tmp_path: Path) -> None: + svc = MagicMock() + svc.restore_config.return_value = { + "status": "restored", + "project_alias": "prod", + "component_id": "keboola.comp", + "config_id": "cfg-1", + "branch_id": None, + "name": "Probe", + "version": 3, + } + result = _invoke( + [ + "--json", + "config", + "restore", + "--project", + "prod", + "--component-id", + "keboola.comp", + "--config-id", + "cfg-1", + ], + config_dir=tmp_path, + svc=svc, + ) + assert result.exit_code == 0 + payload = json.loads(result.stdout) + assert payload["data"]["status"] == "restored" + + def test_human_mode_renders(self, tmp_path: Path) -> None: + svc = MagicMock() + svc.restore_config.return_value = { + "status": "restored", + "project_alias": "prod", + "component_id": "keboola.comp", + "config_id": "cfg-1", + "branch_id": None, + "name": "Probe", + "version": 3, + } + result = _invoke( + [ + "config", + "restore", + "--project", + "prod", + "--component-id", + "keboola.comp", + "--config-id", + "cfg-1", + ], + config_dir=tmp_path, + svc=svc, + ) + assert result.exit_code == 0 + assert "Restored" in result.stdout + + +class TestTrashListCli: + def test_json_envelope(self, tmp_path: Path) -> None: + svc = MagicMock() + svc.list_config_trash.return_value = { + "project_alias": "prod", + "branch_id": None, + "component_id": None, + "trash": [ + { + "component_id": "keboola.comp", + "config_id": "cfg-1", + "name": "Probe", + "version": 3, + "deleted_change_description": None, + "deleted_at": None, + } + ], + } + result = _invoke( + ["--json", "config", "trash-list", "--project", "prod"], + config_dir=tmp_path, + svc=svc, + ) + assert result.exit_code == 0 + payload = json.loads(result.stdout) + assert payload["data"]["trash"][0]["config_id"] == "cfg-1" + + def test_human_mode_empty_trash(self, tmp_path: Path) -> None: + svc = MagicMock() + svc.list_config_trash.return_value = { + "project_alias": "prod", + "branch_id": None, + "component_id": None, + "trash": [], + } + result = _invoke( + ["config", "trash-list", "--project", "prod"], + config_dir=tmp_path, + svc=svc, + ) + assert result.exit_code == 0 + assert "empty" in result.stdout + + +class TestDeleteCliDryRun: + def test_dry_run_flag_reaches_service(self, tmp_path: Path) -> None: + svc = MagicMock() + svc.delete_config.return_value = { + "status": "would_delete", + "dry_run": True, + "project_alias": "prod", + "component_id": "keboola.comp", + "config_id": "cfg-1", + "branch_id": None, + } + result = _invoke( + [ + "config", + "delete", + "--project", + "prod", + "--component-id", + "keboola.comp", + "--config-id", + "cfg-1", + "--dry-run", + ], + config_dir=tmp_path, + svc=svc, + ) + assert result.exit_code == 0 + assert "DRY RUN" in result.stdout + assert svc.delete_config.call_args.kwargs["dry_run"] is True + + def test_already_in_trash_is_exit_zero(self, tmp_path: Path) -> None: + """The retry path must stay a success for scripts.""" + svc = MagicMock() + svc.delete_config.return_value = { + "status": "already_in_trash", + "dry_run": False, + "project_alias": "prod", + "component_id": "keboola.comp", + "config_id": "cfg-1", + "branch_id": None, + "message": "Configuration 'keboola.comp/cfg-1' is already in the trash; " + "not deleting again. Use 'kbagent config restore' to bring it back.", + } + result = _invoke( + [ + "config", + "delete", + "--project", + "prod", + "--component-id", + "keboola.comp", + "--config-id", + "cfg-1", + ], + config_dir=tmp_path, + svc=svc, + ) + assert result.exit_code == 0 + assert "already in the trash" in result.stdout diff --git a/tests/test_e2e.py b/tests/test_e2e.py index 6b9214d3..cb14b5db 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -3626,10 +3626,48 @@ def _test_job_commands(self) -> None: assert detail_data["data"]["id"] def _test_config_delete(self, config_id: str) -> None: - """Delete the test config via CLI.""" + """Delete the test config via CLI, exercising the 0.89.0 trash round trip. + + delete -> repeated delete answers ``already_in_trash`` (the retry that + used to PURGE permanently) -> trash-list finds it -> restore brings it + back -> final delete. Every leg runs against the real Storage API, so + the double-delete guard is proven against the endpoint that actually + overloads DELETE, not against a mock. + """ + common = ( + "--project", + self.alias, + "--component-id", + TEST_COMPONENT_ID, + "--config-id", + config_id, + ) + data = self._run_ok("config", "delete", *common) + assert data["data"]["config_id"] == config_id + assert data["data"]["status"] == "deleted" + + # The retry: MUST be a no-op success, never a permanent purge. + data = self._run_ok("config", "delete", *common) + assert data["data"]["status"] == "already_in_trash" + data = self._run_ok( "config", - "delete", + "trash-list", + "--project", + self.alias, + "--component-id", + TEST_COMPONENT_ID, + ) + trashed_ids = [e["config_id"] for e in data["data"]["trash"]] + assert config_id in trashed_ids, f"{config_id} not in trash listing: {trashed_ids}" + + data = self._run_ok("config", "restore", *common) + assert data["data"]["status"] == "restored" + + # Restored config is live again -- detail must answer. + data = self._run_ok( + "config", + "detail", "--project", self.alias, "--component-id", @@ -3637,7 +3675,10 @@ def _test_config_delete(self, config_id: str) -> None: "--config-id", config_id, ) - assert data["data"]["config_id"] == config_id + + # Final cleanup: back into the trash. + data = self._run_ok("config", "delete", *common) + assert data["data"]["status"] == "deleted" # Remove from cleanup since we deleted via CLI self._created_config_ids.remove((TEST_COMPONENT_ID, config_id)) diff --git a/tests/test_server_router_calls.py b/tests/test_server_router_calls.py index bebb6a01..ac282e57 100644 --- a/tests/test_server_router_calls.py +++ b/tests/test_server_router_calls.py @@ -2259,3 +2259,51 @@ def test_storage_tables_forwards_include_usage(tmp_path: Path) -> None: assert res.status_code == 200, res.text assert storage_svc.list_tables.call_args.kwargs["include_usage"] is True + + +# --------------------------------------------------------------------------- +# configs.py trash safety: DELETE dry_run + restore + trash listing (0.89.0) +# --------------------------------------------------------------------------- + + +def test_config_delete_forwards_dry_run(tmp_path: Path) -> None: + """DELETE /configs/... must be able to preview without deleting.""" + cfg_svc = MagicMock() + cfg_svc.delete_config.return_value = {"status": "would_delete"} + app = _make_app_with_registry(tmp_path, _mock_registry(config=cfg_svc)) + + with TestClient(app) as client: + res = client.delete( + f"/configs/{PROJECT}/{COMPONENT}/{CONFIG_ID}?dry_run=true", headers=AUTH + ) + + assert res.status_code == 200, res.text + assert cfg_svc.delete_config.call_args.kwargs["dry_run"] is True + + +def test_config_restore_route(tmp_path: Path) -> None: + """POST .../restore mirrors `kbagent config restore`.""" + cfg_svc = MagicMock() + cfg_svc.restore_config.return_value = {"status": "restored"} + app = _make_app_with_registry(tmp_path, _mock_registry(config=cfg_svc)) + + with TestClient(app) as client: + res = client.post(f"/configs/{PROJECT}/{COMPONENT}/{CONFIG_ID}/restore", headers=AUTH) + + assert res.status_code == 200, res.text + kwargs = cfg_svc.restore_config.call_args.kwargs + assert kwargs["component_id"] == COMPONENT + assert kwargs["config_id"] == CONFIG_ID + + +def test_config_trash_list_route(tmp_path: Path) -> None: + """GET /configs/trash/{project} mirrors `kbagent config trash-list`.""" + cfg_svc = MagicMock() + cfg_svc.list_config_trash.return_value = {"trash": []} + app = _make_app_with_registry(tmp_path, _mock_registry(config=cfg_svc)) + + with TestClient(app) as client: + res = client.get(f"/configs/trash/{PROJECT}?component_id={COMPONENT}", headers=AUTH) + + assert res.status_code == 200, res.text + assert cfg_svc.list_config_trash.call_args.kwargs["component_id"] == COMPONENT diff --git a/uv.lock b/uv.lock index 6f789a5f..29a82995 100644 --- a/uv.lock +++ b/uv.lock @@ -581,7 +581,7 @@ wheels = [ [[package]] name = "keboola-cli" -version = "0.88.0" +version = "0.89.0" source = { editable = "." } dependencies = [ { name = "croniter" }, From 01b3b2dde7434e1e39a3119df1ee207be7b2dd2d Mon Sep 17 00:00:00 2001 From: Petr Date: Sat, 22 Aug 2026 22:31:20 +0200 Subject: [PATCH 2/3] refactor(config): keep config_service under the hard size ceiling The first commit pushed services/config_service.py from 1487 to 1566 code lines, over the 1500 HARD ceiling -- make loc-check rightly blocked it. - the delete/restore/trash-list bodies move fully into services/_config_trash.py (execute_delete / execute_restore / execute_trash_list); the ConfigService methods are resolve-and-delegate only - _find_matches_in_json moves to json_utils.py as find_matches_in_json -- it is a pure JSON-walking helper and json_utils is where those live (deep_merge, set_nested_value, compute_diff); the one call site and the test import follow config_service.py lands at 1493 code lines: under the hard ceiling, still carrying the pre-existing soft warning that a real split is due. --- src/keboola_agent_cli/json_utils.py | 25 ++++ .../services/_config_trash.py | 90 +++++++++++- .../services/config_service.py | 130 +++--------------- tests/test_config_search.py | 15 +- 4 files changed, 139 insertions(+), 121 deletions(-) diff --git a/src/keboola_agent_cli/json_utils.py b/src/keboola_agent_cli/json_utils.py index 1b4b39cf..5829ee82 100644 --- a/src/keboola_agent_cli/json_utils.py +++ b/src/keboola_agent_cli/json_utils.py @@ -142,3 +142,28 @@ def _fmt(value: Any) -> str: s = repr(value) max_len = 80 return s if len(s) <= max_len else s[: max_len - 3] + "..." + + +def find_matches_in_json( + obj: Any, + match_fn: Any, + path: str = "", +) -> list[str]: + """Recursively walk a JSON-like object and return paths where match_fn(str_value) is True.""" + paths: list[str] = [] + if isinstance(obj, dict): + for key, value in obj.items(): + child_path = f"{path}.{key}" if path else key + paths.extend(find_matches_in_json(value, match_fn, child_path)) + elif isinstance(obj, list): + for i, item in enumerate(obj): + child_path = f"{path}[{i}]" + paths.extend(find_matches_in_json(item, match_fn, child_path)) + elif isinstance(obj, str): + if match_fn(obj): + paths.append(path) + else: + # Numbers, booleans -- convert to string for matching + if obj is not None and match_fn(str(obj)): + paths.append(path) + return paths diff --git a/src/keboola_agent_cli/services/_config_trash.py b/src/keboola_agent_cli/services/_config_trash.py index 26dc31cb..c6acaa33 100644 --- a/src/keboola_agent_cli/services/_config_trash.py +++ b/src/keboola_agent_cli/services/_config_trash.py @@ -19,7 +19,7 @@ from typing import Any -from ..errors import KeboolaApiError +from ..errors import ErrorCode, KeboolaApiError # States a delete/restore attempt can find the configuration in. STATE_LIVE = "live" @@ -78,6 +78,94 @@ def already_trashed_result( } +def execute_delete( + client: Any, + alias: str, + component_id: str, + config_id: str, + branch_id: int | None, + dry_run: bool, +) -> dict[str, Any]: + """The whole delete state machine, on an already-open client. + + * live -> DELETE, status ``deleted`` + * already in the trash -> NO second DELETE (it would purge permanently), + status ``already_in_trash`` + * absent from both -> NOT_FOUND + * dry_run -> report the located state, write nothing + """ + state = locate_config(client, component_id, config_id, branch_id) + if state == STATE_MISSING: + raise KeboolaApiError( + message=( + f"Configuration '{component_id}/{config_id}' not found -- " + "neither live nor in the trash." + ), + error_code=ErrorCode.NOT_FOUND, + status_code=404, + ) + if state == STATE_TRASHED: + result = already_trashed_result(alias, component_id, config_id, branch_id) + result["dry_run"] = dry_run + return result + if dry_run: + return { + "status": "would_delete", + "dry_run": True, + "project_alias": alias, + "component_id": component_id, + "config_id": config_id, + "branch_id": branch_id, + } + client.delete_config(component_id=component_id, config_id=config_id, branch_id=branch_id) + return { + "status": "deleted", + "dry_run": False, + "project_alias": alias, + "component_id": component_id, + "config_id": config_id, + "branch_id": branch_id, + } + + +def execute_restore( + client: Any, + alias: str, + component_id: str, + config_id: str, + branch_id: int | None, +) -> dict[str, Any]: + """Restore a trashed configuration and shape the result envelope.""" + restored = client.restore_config( + component_id=component_id, config_id=config_id, branch_id=branch_id + ) + return { + "status": "restored", + "project_alias": alias, + "component_id": component_id, + "config_id": config_id, + "branch_id": branch_id, + "name": restored.get("name"), + "version": restored.get("version"), + } + + +def execute_trash_list( + client: Any, + alias: str, + component_id: str | None, + branch_id: int | None, +) -> dict[str, Any]: + """List the trash and shape one uniform row per configuration.""" + raw = client.list_deleted_configs(component_id=component_id, branch_id=branch_id) + return { + "project_alias": alias, + "branch_id": branch_id, + "component_id": component_id, + "trash": [shape_trash_entry(cfg, component_id) for cfg in raw], + } + + def shape_trash_entry(config: dict[str, Any], component_id: str | None) -> dict[str, Any]: """One uniform row for ``config trash-list`` output.""" current = config.get("currentVersion") or {} diff --git a/src/keboola_agent_cli/services/config_service.py b/src/keboola_agent_cli/services/config_service.py index 2aac5043..7f441672 100644 --- a/src/keboola_agent_cli/services/config_service.py +++ b/src/keboola_agent_cli/services/config_service.py @@ -21,7 +21,7 @@ ROOT_LEVEL_CONFIG_COMPONENTS, ) from ..errors import ConfigError, ErrorCode, KeboolaApiError -from ..json_utils import compute_diff, deep_merge, set_nested_value +from ..json_utils import compute_diff, deep_merge, find_matches_in_json, set_nested_value from ..models import ComponentDetail, ProjectConfig from ..sync.code_extraction import normalize_blocks_codes_script from ..sync.manifest import Manifest, load_manifest, save_manifest @@ -93,31 +93,6 @@ def _default_change_description(command: str, *, has_metadata: bool, has_content return f"Updated {' + '.join(parts)} via kbagent {command}" -def _find_matches_in_json( - obj: Any, - match_fn: Any, - path: str = "", -) -> list[str]: - """Recursively walk a JSON-like object and return paths where match_fn(str_value) is True.""" - paths: list[str] = [] - if isinstance(obj, dict): - for key, value in obj.items(): - child_path = f"{path}.{key}" if path else key - paths.extend(_find_matches_in_json(value, match_fn, child_path)) - elif isinstance(obj, list): - for i, item in enumerate(obj): - child_path = f"{path}[{i}]" - paths.extend(_find_matches_in_json(item, match_fn, child_path)) - elif isinstance(obj, str): - if match_fn(obj): - paths.append(path) - else: - # Numbers, booleans -- convert to string for matching - if obj is not None and match_fn(str(obj)): - paths.append(path) - return paths - - def _not_found(message: str) -> KeboolaApiError: """Build a 404 NOT_FOUND error (issue #593 -- shared by ConfigService._extract_state).""" return KeboolaApiError(status_code=404, error_code=ErrorCode.NOT_FOUND, message=message) @@ -1228,82 +1203,28 @@ def delete_config( to the trash (restorable), but on a configuration ALREADY in the trash the same call purges it permanently -- versions, rows and metadata included. A timed-out delete followed by a retry is exactly - that second call, so this method locates the configuration first and - refuses to issue a DELETE at anything that is not live: - - * live -> DELETE, status ``deleted`` - * already in the trash -> no API write, status ``already_in_trash`` - (exit stays 0 -- the retry is idempotent, the purge never happens) - * absent from both -> NOT_FOUND - - Args: - alias: Project alias. - component_id: The component ID (e.g. keboola.python-transformation-v2). - config_id: The configuration ID to delete. - branch_id: If set, delete from a specific dev branch. - If None, uses the project's active branch (if any). - dry_run: Report the located state without deleting anything. - - Returns: - Dict with deletion confirmation details. - - Raises: - ConfigError: If the alias is not found. - KeboolaApiError: If the API call fails or the configuration does - not exist in either the live listing or the trash. + that second call, so the executor locates the configuration first and + refuses to issue a DELETE at anything that is not live. See + :mod:`._config_trash` for the state machine; it lives there because + this module is over its size budget. + + Returns a dict whose ``status`` is ``deleted`` / ``already_in_trash`` + / ``would_delete`` (dry run); raises NOT_FOUND when the configuration + exists neither live nor in the trash. """ from . import _config_trash as trash projects = self.resolve_projects([alias]) project = projects[alias] - - # Use active branch if no explicit branch_id given effective_branch_id = branch_id or project.active_branch_id - client = self._client_factory(project.stack_url, project.token) try: - state = trash.locate_config(client, component_id, config_id, effective_branch_id) - if state == trash.STATE_MISSING: - raise KeboolaApiError( - message=( - f"Configuration '{component_id}/{config_id}' not found -- " - "neither live nor in the trash." - ), - error_code=ErrorCode.NOT_FOUND, - status_code=404, - ) - if state == trash.STATE_TRASHED: - result = trash.already_trashed_result( - alias, component_id, config_id, effective_branch_id - ) - result["dry_run"] = dry_run - return result - if dry_run: - return { - "status": "would_delete", - "dry_run": True, - "project_alias": alias, - "component_id": component_id, - "config_id": config_id, - "branch_id": effective_branch_id, - } - client.delete_config( - component_id=component_id, - config_id=config_id, - branch_id=effective_branch_id, + return trash.execute_delete( + client, alias, component_id, config_id, effective_branch_id, dry_run ) finally: client.close() - return { - "status": "deleted", - "dry_run": False, - "project_alias": alias, - "component_id": component_id, - "config_id": config_id, - "branch_id": effective_branch_id, - } - def restore_config( self, alias: str, @@ -1312,27 +1233,18 @@ def restore_config( branch_id: int | None = None, ) -> dict[str, Any]: """Restore a trashed configuration (the undo for :meth:`delete_config`).""" + from . import _config_trash as trash + projects = self.resolve_projects([alias]) project = projects[alias] effective_branch_id = branch_id or project.active_branch_id client = self._client_factory(project.stack_url, project.token) try: - restored = client.restore_config( - component_id=component_id, - config_id=config_id, - branch_id=effective_branch_id, + return trash.execute_restore( + client, alias, component_id, config_id, effective_branch_id ) finally: client.close() - return { - "status": "restored", - "project_alias": alias, - "component_id": component_id, - "config_id": config_id, - "branch_id": effective_branch_id, - "name": restored.get("name"), - "version": restored.get("version"), - } def list_config_trash( self, @@ -1348,17 +1260,9 @@ def list_config_trash( effective_branch_id = branch_id or project.active_branch_id client = self._client_factory(project.stack_url, project.token) try: - raw = client.list_deleted_configs( - component_id=component_id, branch_id=effective_branch_id - ) + return trash.execute_trash_list(client, alias, component_id, effective_branch_id) finally: client.close() - return { - "project_alias": alias, - "branch_id": effective_branch_id, - "component_id": component_id, - "trash": [trash.shape_trash_entry(cfg, component_id) for cfg in raw], - } def rename_config( self, @@ -1847,7 +1751,7 @@ def _search_project_configs( for cfg in component.get("configurations", []): configs_searched += 1 - match_locations = _find_matches_in_json(cfg, match_fn) + match_locations = find_matches_in_json(cfg, match_fn) if match_locations: matches.append( diff --git a/tests/test_config_search.py b/tests/test_config_search.py index 3c8dfa72..563f7c08 100644 --- a/tests/test_config_search.py +++ b/tests/test_config_search.py @@ -4,7 +4,8 @@ from unittest.mock import MagicMock from helpers import setup_single_project, setup_two_projects -from keboola_agent_cli.services.config_service import ConfigService, _find_matches_in_json +from keboola_agent_cli.json_utils import find_matches_in_json +from keboola_agent_cli.services.config_service import ConfigService # --------------------------------------------------------------------------- # Helpers @@ -125,7 +126,7 @@ def test_find_match_in_string_value(self) -> None: obj = {"name": "My Extractor", "type": "extractor"} match_fn = lambda s: "Extractor" in s # noqa: E731 - paths = _find_matches_in_json(obj, match_fn) + paths = find_matches_in_json(obj, match_fn) assert "name" in paths assert len(paths) == 1 @@ -143,7 +144,7 @@ def test_find_match_in_nested_dict(self) -> None: } match_fn = lambda s: "snowflakecomputing" in s # noqa: E731 - paths = _find_matches_in_json(obj, match_fn) + paths = find_matches_in_json(obj, match_fn) assert paths == ["configuration.parameters.db.host"] @@ -157,7 +158,7 @@ def test_find_match_in_list(self) -> None: } match_fn = lambda s: "ad_groups" in s # noqa: E731 - paths = _find_matches_in_json(obj, match_fn) + paths = find_matches_in_json(obj, match_fn) assert paths == ["tables[1].tableName"] @@ -166,7 +167,7 @@ def test_find_match_in_number(self) -> None: obj = {"port": 443, "name": "test"} match_fn = lambda s: "443" in s # noqa: E731 - paths = _find_matches_in_json(obj, match_fn) + paths = find_matches_in_json(obj, match_fn) assert "port" in paths @@ -178,7 +179,7 @@ def test_no_match_returns_empty(self) -> None: } match_fn = lambda s: "nonexistent_string_xyz" in s # noqa: E731 - paths = _find_matches_in_json(obj, match_fn) + paths = find_matches_in_json(obj, match_fn) assert paths == [] @@ -197,7 +198,7 @@ def test_find_multiple_matches(self) -> None: } match_fn = lambda s: "snowflake" in s.lower() # noqa: E731 - paths = _find_matches_in_json(obj, match_fn) + paths = find_matches_in_json(obj, match_fn) assert len(paths) == 3 assert "name" in paths From 01a364e63cf7c5928fcc6003410bc04d5d7e1643 Mon Sep 17 00:00:00 2001 From: Petr Date: Sat, 22 Aug 2026 22:51:42 +0200 Subject: [PATCH 3/3] fix(config): stop the HTTP layer from retrying the config DELETE into a purge Devin Review caught a hole in the first commit's central claim. The locate-first guard runs ONCE per delete_config call, so it protects against a retry across separate command invocations -- but not against the retry the HTTP client performs inside that single call. DELETE is in RETRY_SAFE_METHODS, so a read timeout or a 5xx made _do_request repeat it automatically. On this endpoint the repeat IS the purge: the server trashes the config, the response is lost, the retry lands on the now-trashed config and destroys it before any caller sees a result. That is the MOST likely form of the very scenario the PR set out to close, and it was still open. Reproduced before fixing: one delete_config call put two DELETEs on the wire. - http_base._do_request gains a per-call `retry_safe` override; None keeps the method-based rule. _server_error_hint honours it too, so a 5xx on an opted-out call gets the "may already have taken effect" note. - KeboolaClient._request passes it through; client.delete_config sets retry_safe=False. A lost response now surfaces as TIMEOUT for the caller to decide about, and re-running the command is safe because the service guard catches the trashed state -- which a transport-level retry never can. - Every other DELETE keeps its retry. The opt-out is per endpoint because idempotency is a property of the endpoint, not of the method. Second Devin point, also addressed: locate_config inferred "live" from the absence of a 404. GET on a trashed config answers 404 on connection.keboola.com and on the GCP stack (verified live), but a stack returning the tombstone body would have made a 200 mean "trashed" -- and the DELETE that followed would purge. It now reads `isDeleted` from the body rather than trusting the status code, so the behaviour no longer depends on a cross-stack convention. 5 tests added (20 in the file): both purge paths register exactly ONE mocked outcome, so a passing test proves a single DELETE left the client; plus the default-still-retries case and the override in isolation. --- .../skills/kbagent/references/gotchas.md | 8 ++ src/keboola_agent_cli/changelog.py | 2 + src/keboola_agent_cli/client/_core.py | 12 +- src/keboola_agent_cli/client/configs.py | 18 ++- src/keboola_agent_cli/http_base.py | 27 ++++- .../services/_config_trash.py | 23 +++- tests/test_config_trash.py | 109 ++++++++++++++++++ 7 files changed, 187 insertions(+), 12 deletions(-) diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index e67d2c4c..53d952ef 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -1350,6 +1350,14 @@ config, the retry fires, and the retry destroys it for good. - Direct API callers: the purge-safe alternative is the dedicated `POST .../configs/{id}/purge` endpoint (fails with 400 when the config is not in the trash), never a second DELETE. +- **The likeliest second DELETE is not a human retry -- it is the HTTP + client's own.** `DELETE` is conventionally idempotent, so most transports + (kbagent's included, before 0.89.0) repeat it after a read timeout or a + 5xx. On this endpoint that automatic repeat IS the purge, and it happens + before any caller sees a result. If you are writing a script or another + client against the Storage API, disable transport-level retry for a config + DELETE specifically; idempotency here is a property of the endpoint, not + of the method. ## `data-app` JSON output: key for the app's own id is `app_id` (since v0.33.0) diff --git a/src/keboola_agent_cli/changelog.py b/src/keboola_agent_cli/changelog.py index 80f3aeb6..dda183fc 100644 --- a/src/keboola_agent_cli/changelog.py +++ b/src/keboola_agent_cli/changelog.py @@ -34,6 +34,8 @@ "DELETE at anything that is not live: already trashed answers `already_in_trash` with " "exit 0 (the retry stays idempotent), absent from both answers NOT_FOUND. `--dry-run` " "reports the located state without writing.", + "Fix: the config DELETE is no longer retried by the HTTP layer itself. `DELETE` sits in `RETRY_SAFE_METHODS`, so a read timeout or a 5xx made the transport repeat it automatically -- and for THIS endpoint the repeat is the purge. The server trashes the config, the response is lost, the retry lands on the now-trashed config and destroys it before any caller sees a result. The service-level locate-first guard runs once per call and cannot see inside that loop, so `client.delete_config` now passes a new per-call `retry_safe=False` override and a lost response surfaces as a TIMEOUT the caller decides about; re-running the command is safe because the guard catches the trashed state. Every other DELETE keeps the retry -- the opt-out is per endpoint, because idempotency is a property of the endpoint, not of the method.", + "Note: `locate_config` no longer infers a live state from the absence of a 404 either. Every stack checked answers 404 for a trashed configuration (verified live on connection.keboola.com and the GCP stack), but a body carrying `isDeleted: true` is now read as trashed regardless of status code -- that flag decides whether a purge-capable DELETE goes out, so it is read rather than inferred.", "New: `kbagent config restore` -- the undo for `config delete`. Restores a trashed " "configuration with its versions, rows and metadata (`POST .../configs/{id}/restore`). " "Only works on a configuration currently in the trash.", diff --git a/src/keboola_agent_cli/client/_core.py b/src/keboola_agent_cli/client/_core.py index 838696d0..172b85ad 100644 --- a/src/keboola_agent_cli/client/_core.py +++ b/src/keboola_agent_cli/client/_core.py @@ -131,9 +131,15 @@ def __enter__(self) -> Self: def __exit__(self, *args: Any) -> None: self.close() - def _request(self, method: str, path: str, **kwargs: Any) -> httpx.Response: - """Execute a Storage API request with retry.""" - return self._do_request(method, path, **kwargs) + def _request( + self, method: str, path: str, retry_safe: bool | None = None, **kwargs: Any + ) -> httpx.Response: + """Execute a Storage API request with retry. + + ``retry_safe=False`` opts a single call out of the method-based retry + rule -- see :meth:`BaseHttpClient._do_request`. + """ + return self._do_request(method, path, retry_safe=retry_safe, **kwargs) def _get_or_create_sub_client( self, diff --git a/src/keboola_agent_cli/client/configs.py b/src/keboola_agent_cli/client/configs.py index c4a67e5e..c3a199ad 100644 --- a/src/keboola_agent_cli/client/configs.py +++ b/src/keboola_agent_cli/client/configs.py @@ -850,7 +850,22 @@ def restore_config( def delete_config( self, component_id: str, config_id: str, branch_id: int | None = None ) -> None: - """Delete a component configuration. + """Delete a component configuration -- NEVER retried at the HTTP layer. + + DELETE is normally treated as idempotent (``RETRY_SAFE_METHODS``), and + for most endpoints it is: repeating converges on the same state. This + endpoint is the exception, because its meaning CHANGES once it has + succeeded. The first DELETE moves a live configuration to the trash; + a second one lands on the now-trashed configuration and purges it + permanently -- versions, rows and metadata gone. + + That makes the transport's own retry the most likely way to trigger + the purge: the server trashes the config, the response is lost to a + read timeout or a 5xx, and the retry destroys it before any caller + sees a result. So this call passes ``retry_safe=False`` and a lost + response surfaces as a TIMEOUT the caller must decide about. + ``ConfigService.delete_config``'s locate-first guard then makes the + re-run safe, which a transport-level retry can never be. Args: component_id: Component ID. @@ -863,4 +878,5 @@ def delete_config( self._request( "DELETE", f"{prefix}/components/{safe_component}/configs/{safe_config}", + retry_safe=False, ) diff --git a/src/keboola_agent_cli/http_base.py b/src/keboola_agent_cli/http_base.py index e49d3aa0..6bf97432 100644 --- a/src/keboola_agent_cli/http_base.py +++ b/src/keboola_agent_cli/http_base.py @@ -153,6 +153,7 @@ def _do_request( *, client: httpx.Client | None = None, base_url: str | None = None, + retry_safe: bool | None = None, **kwargs: Any, ) -> httpx.Response: """Execute an HTTP request with retry and exponential backoff. @@ -173,6 +174,12 @@ def _do_request( client: Optional httpx.Client to use (defaults to self._client). Useful for subclasses that maintain multiple clients (e.g. queue client). base_url: Optional base URL for error messages (defaults to self._base_url). + retry_safe: Override the method-based idempotency verdict. Pass + ``False`` for a request whose method looks idempotent but whose + SERVER-SIDE MEANING changes once it has succeeded -- the + canonical case is ``DELETE`` on a component configuration, + where a repeat lands on the now-trashed config and purges it + permanently. ``None`` (default) keeps the method-based rule. **kwargs: Additional arguments passed to httpx.Client.request(). Returns: @@ -184,7 +191,9 @@ def _do_request( http_client = client or self._client url_label = base_url or self._base_url last_response: httpx.Response | None = None - retry_safe = method.upper() in RETRY_SAFE_METHODS + # An explicit override wins: RETRY_SAFE_METHODS reasons about the METHOD, + # but idempotency is a property of the endpoint. See the `retry_safe` arg. + retry_safe = method.upper() in RETRY_SAFE_METHODS if retry_safe is None else retry_safe # Counted separately from `attempt`: a 429 burns an attempt without # being a server error, so using the attempt index would report "the # same 5xx came back on N attempts" after seeing exactly one. @@ -230,7 +239,9 @@ def _do_request( last_response = response continue - hint = self._server_error_hint(method, response.status_code, server_error_attempts) + hint = self._server_error_hint( + method, response.status_code, server_error_attempts, retry_safe=retry_safe + ) self._raise_api_error( response, url_label, @@ -331,7 +342,14 @@ def _non_idempotent_note(method: str) -> str: ) @classmethod - def _server_error_hint(cls, method: str, status: int, server_error_attempts: int) -> str | None: + def _server_error_hint( + cls, + method: str, + status: int, + server_error_attempts: int, + *, + retry_safe: bool | None = None, + ) -> str | None: """Return the actionable next step for a 5xx, or None if there isn't one. Two situations need two different answers (issue #599). A 5xx on a @@ -349,7 +367,8 @@ def _server_error_hint(cls, method: str, status: int, server_error_attempts: int """ if status < 500: return None - if method.upper() not in RETRY_SAFE_METHODS: + effective_safe = method.upper() in RETRY_SAFE_METHODS if retry_safe is None else retry_safe + if not effective_safe: return cls._non_idempotent_note(method) if server_error_attempts > 1: return ( diff --git a/src/keboola_agent_cli/services/_config_trash.py b/src/keboola_agent_cli/services/_config_trash.py index c6acaa33..ffbd1094 100644 --- a/src/keboola_agent_cli/services/_config_trash.py +++ b/src/keboola_agent_cli/services/_config_trash.py @@ -36,15 +36,30 @@ def locate_config( """Answer whether a configuration is live, in the trash, or absent. A direct ``GET .../configs/{id}`` answers 404 for BOTH a trashed and a - never-existed configuration, so a 404 alone cannot drive the delete - decision -- the trash listing is what separates the two. + never-existed configuration (verified live on connection.keboola.com and + the GCP stack), so a 404 alone cannot drive the delete decision -- the + trash listing is what separates the two. + + The 200 path does not trust the status code either: a body carrying + ``isDeleted: true`` is reported as trashed. Nothing observed returns a + tombstone from that endpoint, but "not live" is the answer that decides + whether a purge-capable DELETE goes out, so it is read from the flag + rather than inferred from the absence of a 404. """ try: - client.get_config_detail(component_id, config_id, branch_id=branch_id) - return STATE_LIVE + detail = client.get_config_detail(component_id, config_id, branch_id=branch_id) except KeboolaApiError as exc: if exc.status_code != 404: raise + else: + # Do not infer "live" from a 200 alone. Every stack checked so far + # answers 404 for a trashed configuration, but a stack that returned + # the tombstone body instead would make a 200 mean "trashed" -- and + # the DELETE that followed would purge it. The flag is authoritative + # where the status code is only conventional. + if isinstance(detail, dict) and detail.get("isDeleted"): + return STATE_TRASHED + return STATE_LIVE trashed = client.list_deleted_configs(component_id=component_id, branch_id=branch_id) if any(str(cfg.get("id")) == str(config_id) for cfg in trashed): return STATE_TRASHED diff --git a/tests/test_config_trash.py b/tests/test_config_trash.py index c649eaf9..6df09f1e 100644 --- a/tests/test_config_trash.py +++ b/tests/test_config_trash.py @@ -332,3 +332,112 @@ def test_already_in_trash_is_exit_zero(self, tmp_path: Path) -> None: ) assert result.exit_code == 0 assert "already in the trash" in result.stdout + + +class TestDeleteIsNeverRetriedAtTheTransport: + """The purge path the locate-first guard alone cannot close. + + ``DELETE`` sits in ``RETRY_SAFE_METHODS``, so before this fix the HTTP + layer repeated a timed-out or 5xx'd config delete on its own -- the server + trashed the config, the response was lost, and the retry purged it + permanently before any caller saw a result. The service guard runs ONCE + per call and cannot see inside that loop, so the opt-out has to live at + the transport. + """ + + URL = "https://connection.keboola.com/v2/storage/components/keboola.comp/configs/cfg-1" + + def _client(self): + from keboola_agent_cli.client import KeboolaClient + + return KeboolaClient("https://connection.keboola.com", TEST_TOKEN) + + def test_read_timeout_sends_exactly_one_delete(self, httpx_mock) -> None: + """Server already trashed it; the lost response must NOT trigger a purge.""" + import httpx + + import keboola_agent_cli.http_base as hb + + # Exactly ONE outcome is registered. If the transport retried, the + # second attempt would find no mock at all -- so a passing test proves + # a single DELETE left the client. + httpx_mock.add_exception(httpx.ReadTimeout("lost"), url=self.URL) + + client = self._client() + original_sleep = hb.time.sleep + hb.time.sleep = lambda *a, **k: None # ty: ignore[invalid-assignment] + try: + with pytest.raises(KeboolaApiError) as exc_info: + client.delete_config("keboola.comp", "cfg-1") + assert exc_info.value.error_code == "TIMEOUT" + sent = [r for r in httpx_mock.get_requests() if r.method == "DELETE"] + assert len(sent) == 1, f"a second DELETE would purge; sent {len(sent)}" + finally: + hb.time.sleep = original_sleep + client.close() + + def test_server_error_sends_exactly_one_delete(self, httpx_mock) -> None: + """A 5xx after a successful soft delete is the same trap as a timeout.""" + import keboola_agent_cli.http_base as hb + + httpx_mock.add_response(url=self.URL, status_code=503, text="unavailable") + + client = self._client() + original_sleep = hb.time.sleep + hb.time.sleep = lambda *a, **k: None # ty: ignore[invalid-assignment] + try: + with pytest.raises(KeboolaApiError): + client.delete_config("keboola.comp", "cfg-1") + sent = [r for r in httpx_mock.get_requests() if r.method == "DELETE"] + assert len(sent) == 1, f"a second DELETE would purge; sent {len(sent)}" + finally: + hb.time.sleep = original_sleep + client.close() + + def test_default_delete_still_retries(self, httpx_mock) -> None: + """The opt-out is per call, not a blanket policy change. + + A DELETE without the override keeps the resilience RETRY_SAFE_METHODS + exists to provide -- deleting a table converges on repeat, so losing + that would trade one endpoint's safety for every other endpoint's. + """ + import keboola_agent_cli.http_base as hb + + url = "https://connection.keboola.com/plain-delete" + httpx_mock.add_response(url=url, status_code=503, text="unavailable") + httpx_mock.add_response(url=url, status_code=204) + + client = self._client() + original_sleep = hb.time.sleep + hb.time.sleep = lambda *a, **k: None # ty: ignore[invalid-assignment] + try: + response = client._do_request("DELETE", "/plain-delete") + assert response.status_code == 204 + assert len(httpx_mock.get_requests()) == 2 + finally: + hb.time.sleep = original_sleep + client.close() + + def test_override_is_what_stops_it(self, httpx_mock) -> None: + """Same request, retry_safe=False -> exactly one attempt.""" + url = "https://connection.keboola.com/plain-delete" + httpx_mock.add_response(url=url, status_code=503, text="unavailable") + + client = self._client() + try: + with pytest.raises(KeboolaApiError): + client._do_request("DELETE", "/plain-delete", retry_safe=False) + assert len(httpx_mock.get_requests()) == 1 + finally: + client.close() + + +class TestLocateDoesNotTrustTheStatusCode: + """A 200 carrying isDeleted must not be read as 'live' and then DELETEd.""" + + def test_tombstone_body_reports_trashed(self, tmp_config_dir: Path) -> None: + service, client = _make_service(tmp_config_dir, live=True, in_trash=False) + client.get_config_detail.return_value = {"id": "cfg-1", "isDeleted": True} + result = service.delete_config("prod", "keboola.comp", "cfg-1") + assert result["status"] == "already_in_trash" + client.delete_config.assert_not_called()