From 2982e335a3bf525cbab6e656d75fd0c29653c075 Mon Sep 17 00:00:00 2001 From: ottomansky Date: Sun, 17 May 2026 23:18:59 +0200 Subject: [PATCH 1/5] fix(0.41.11): cascade-delete semantic-layer model children (#306) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously `semantic-layer model delete` only DELETEd the parent `semantic-model` row; the five child types (dataset / metric / relationship / constraint / glossary) stayed on the wire pointing at the dead `modelUUID`. Because dataset names are unique per project, the next `build` or `import` of a same-named dataset hit HTTP 422 with no UI/CLI escape. The service now walks `reversed(PUSH_ORDER)` (constraints → glossary → relationships → metrics → datasets) calling `client.delete_item` per child before the parent. On any child-DELETE failure the cascade continues, the parent is preserved, and a KeboolaApiError is raised with `details.cascade = {attempted, deleted, failures: [...], parent_deleted: False, model_uuid}` so the user can re-run after fixing the underlying error. Matches the push_built_model rollback envelope from #295. CLI prompt updated from the misleading "the API will refuse" to an explicit cascade warning; success renderer appends `+ cascaded N child(ren)`. Legacy `orphaned_children` envelope key kept for back-compat with shape unchanged but meaning flipped from "leaked" to "cascaded" — happy-path consumers always saw zeros anyway. Tests: 3 new service-level tests (happy-path call order in reverse PUSH_ORDER, partial-failure preserves parent, empty-model parent-only delete) + updated CLI envelope shape test. Full suite: 3341 passed. Co-Authored-By: Claude Opus 4.7 (1M context) --- plugins/kbagent/skills/kbagent/SKILL.md | 4 +- .../skills/kbagent/references/gotchas.md | 40 +++++++ .../commands/semantic_layer.py | 9 +- .../services/semantic_layer_service.py | 108 +++++++++++++++--- tests/test_semantic_layer_cli.py | 20 +++- tests/test_semantic_layer_service.py | 108 ++++++++++++++++-- 6 files changed, 263 insertions(+), 26 deletions(-) diff --git a/plugins/kbagent/skills/kbagent/SKILL.md b/plugins/kbagent/skills/kbagent/SKILL.md index 40ad3c15..326208b6 100644 --- a/plugins/kbagent/skills/kbagent/SKILL.md +++ b/plugins/kbagent/skills/kbagent/SKILL.md @@ -252,7 +252,7 @@ When working inside a git repository or project directory, run `kbagent init` (o | Validate a semantic-layer model | `kbagent semantic-layer validate --project PROJECT` | | List all semantic-layer models in a project | `kbagent semantic-layer model list --project PROJECT` | | Create a new semantic-layer model | `kbagent semantic-layer model create --project PROJECT --name NAME` | -| Delete a semantic-layer model. | `kbagent semantic-layer model delete --project PROJECT --model MODEL` | +| Delete a semantic-layer model and cascade-delete its children | `kbagent semantic-layer model delete --project PROJECT --model MODEL` | | Add a metric to a semantic-layer model | `kbagent semantic-layer add metric --project PROJECT --name NAME --sql SQL --dataset DATASET` | | Add a dataset (FQN derived from tableId) | `kbagent semantic-layer add dataset --project PROJECT --name NAME --table-id TABLE-ID` | | Add a relationship between two datasets | `kbagent semantic-layer add relationship --project PROJECT --name NAME --from FROM- --to TO --on ON` | @@ -278,7 +278,7 @@ When working inside a git repository or project directory, run `kbagent init` (o | Validate a semantic-layer model | `kbagent sl validate --project PROJECT` | | List all semantic-layer models in a project | `kbagent sl model list --project PROJECT` | | Create a new semantic-layer model | `kbagent sl model create --project PROJECT --name NAME` | -| Delete a semantic-layer model. | `kbagent sl model delete --project PROJECT --model MODEL` | +| Delete a semantic-layer model and cascade-delete its children | `kbagent sl model delete --project PROJECT --model MODEL` | | Add a metric to a semantic-layer model | `kbagent sl add metric --project PROJECT --name NAME --sql SQL --dataset DATASET` | | Add a dataset (FQN derived from tableId) | `kbagent sl add dataset --project PROJECT --name NAME --table-id TABLE-ID` | | Add a relationship between two datasets | `kbagent sl add relationship --project PROJECT --name NAME --from FROM- --to TO --on ON` | diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index c57b760c..1ce03a33 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -121,6 +121,46 @@ limit, transient 5xx), the detail call still succeeds and `storage_workspace_id` is set to `null` -- the annotation is UX, not a contract. +## `semantic-layer model delete` cascade-deletes children (since v0.41.11) + +`kbagent semantic-layer model delete --project P --model M` used to DELETE +only the parent `semantic-model` row, leaving every dataset / metric / +relationship / constraint / glossary term on the wire pointing at the +now-dead `modelUUID` (issue #306). The orphans were invisible until the next +`build` or `import` hit HTTP 422 `semantic-dataset with name 'X' already +exists in the target model` on a same-named dataset — names are unique +**per project**, not per model. + +Since this release the command walks `reversed(PUSH_ORDER)` (constraints → +glossary → relationships → metrics → datasets) and deletes each child via +`client.delete_item` before the parent. `--yes` still skips the +confirmation prompt; the prompt text now warns explicitly that all children +will be deleted. + +**Partial failure semantics (matches `push_built_model` rollback envelope):** + +- Every child DELETE is wrapped individually; sibling failures do **not** + abort the cascade. +- If ANY child fails, the parent is **preserved** and a `KeboolaApiError` + is raised with `details.cascade = {attempted, deleted, failures: [{type, + id, name, error}], parent_deleted: False, model_uuid}`. +- Re-run `kbagent semantic-layer model delete --project P --model ` + after fixing the underlying error to finish the cascade. + +**Response envelope changes:** + +- New top-level `cascade` block on success: `{attempted, deleted: {datasets, + metrics, relationships, glossary, constraints}, failures: [], parent_deleted}`. +- Legacy `orphaned_children` top-level key kept for back-compat with the + shape unchanged, but its **meaning** flips from 'leaked count' to + 'cascaded count'. Happy-path JSON consumers always saw zeros on this key + before — the only way to populate it was the bug. + +**Implication for AI agents / scripts:** Scripts that called `model delete` +and then assumed they had to teardown children manually can drop that +follow-up. Scripts that scraped `orphaned_children` to detect the bug now +see the same zeros they always wanted. + ## Web UI `Kai Chat` is gone — replaced by `Local AI` (since v0.41.9) The web UI dashboard tile / left-nav entry previously labelled **Kai diff --git a/src/keboola_agent_cli/commands/semantic_layer.py b/src/keboola_agent_cli/commands/semantic_layer.py index e825733c..d508c3fb 100644 --- a/src/keboola_agent_cli/commands/semantic_layer.py +++ b/src/keboola_agent_cli/commands/semantic_layer.py @@ -162,7 +162,7 @@ def model_delete( model: str = typer.Option(..., "--model", help="Model name or UUID"), yes: bool = typer.Option(False, "--yes", "-y", help="Skip the confirmation prompt"), ) -> None: - """Delete a semantic-layer model. Fails if the model still has children.""" + """Delete a semantic-layer model and cascade-delete its children.""" if should_hint(ctx): emit_hint(ctx, "semantic-layer.model.delete", project=project, model=model) return @@ -174,7 +174,9 @@ def model_delete( and not formatter.json_mode and not typer.confirm( f"Delete model '{model}' in project '{project}'? " - "If the model has datasets/metrics/etc. the API will refuse." + "This cascade-deletes ALL child entities (datasets, metrics, " + "relationships, constraints, glossary terms) belonging to the model. " + "This is irreversible." ) ): formatter.console.print("Aborted.") @@ -190,7 +192,8 @@ def model_delete( result, lambda c, d: c.print( f"[bold green]Deleted model[/bold green] [cyan]{d['deleted']['name']}[/cyan] " - f"([dim]{d['deleted']['id']}[/dim])" + f"([dim]{d['deleted']['id']}[/dim]) " + f"+ cascaded {sum(d.get('cascade', {}).get('deleted', {}).values())} child(ren)" ), ) diff --git a/src/keboola_agent_cli/services/semantic_layer_service.py b/src/keboola_agent_cli/services/semantic_layer_service.py index 9f074fa5..7f0adb75 100644 --- a/src/keboola_agent_cli/services/semantic_layer_service.py +++ b/src/keboola_agent_cli/services/semantic_layer_service.py @@ -33,8 +33,8 @@ from ._semantic_layer_crud import find_target_for_remove as _find_target_for_remove from ._semantic_layer_crud import scan_orphan_constraints as _scan_orphan_constraints from ._semantic_layer_crud import validate_constraint_attrs as _validate_constraint_attrs +from ._semantic_layer_internals import PUSH_ORDER, collect_side_from_file from ._semantic_layer_internals import build_export_snapshot as _build_export_snapshot -from ._semantic_layer_internals import collect_side_from_file from ._semantic_layer_internals import default_export_path as _default_export_path from ._semantic_layer_internals import diff_one_type as _diff_one_type_helper from ._semantic_layer_internals import fetch_table_schemas as _fetch_table_schemas @@ -613,30 +613,112 @@ def delete_model( alias: str, model_name_or_uuid: str, ) -> dict[str, Any]: - """Delete a semantic-layer model. - - Lists every referencing child entity first so the caller can warn - about orphaning. The metastore may refuse to delete a model that - still has children — we surface that error verbatim. + """Delete a semantic-layer model and cascade-delete its children. + + Children are deleted in reverse :data:`PUSH_ORDER` (constraints + first, datasets last) so each row's references are gone before the + row itself. On any child-DELETE failure we collect the error, + finish the cascade pass, **skip the parent**, and raise a + :class:`KeboolaApiError` carrying ``details.cascade`` — the same + envelope shape used by :func:`push_built_model` rollback. Re-running + the command after fixing the underlying error completes the + deletion. """ project = self._resolve_one_project(alias) client = self._new_metastore_client(project) try: model_uuid, model_attrs = self._resolve_model(client, model_name_or_uuid) children = self._fetch_children_parallel(client, model_uuid) + + deleted_counts: dict[str, int] = {plural: 0 for plural, _ in PUSH_ORDER} + failures: list[dict[str, str]] = [] + # reversed(PUSH_ORDER) → constraints, glossary, relationships, + # metrics, datasets. Constraints reference metrics by name and + # metrics reference dataset tableIds, so this order kills the + # references before their targets. + for plural, type_slug in reversed(PUSH_ORDER): + for item in children.get(type_slug, []) or []: + child_id = str(item.get("id", "") or "") + if not child_id: + continue + attrs = item.get("attributes") or {} + child_name = attrs.get("name") or attrs.get("term") or "" + try: + client.delete_item(type_slug, child_id) + deleted_counts[plural] += 1 + except (KeyboardInterrupt, SystemExit): + raise + except Exception as exc: + # Broad catch matches push_built_model rollback: a + # non-API exception (httpx transport error, etc.) + # must not abort the cascade or mask sibling + # failures. Collect, log at warning level, continue. + err = ( + exc.message + if isinstance(exc, KeboolaApiError) + else str(exc) or type(exc).__name__ + ) + failures.append( + { + "type": type_slug, + "id": child_id, + "name": child_name, + "error": err, + } + ) + logger.warning( + "Cascade DELETE failed for %s id=%s name=%s: %s", + type_slug, + child_id, + child_name, + err, + ) + + if failures: + # Skip the parent: a "no parent + some children" partial + # state would still hit the exact 422 collision this fix + # is closing. Surface the failures so the user can re-run. + raise KeboolaApiError( + message=( + f"Cascade-delete for model " + f"{model_attrs.get('name', '') or model_uuid!r} ({model_uuid}) " + f"failed for {len(failures)} child(ren); parent preserved. " + f"Re-run `kbagent semantic-layer model delete " + f"--project {alias} --model {model_uuid}` " + f"after resolving the underlying errors." + ), + error_code=ErrorCode.INTERNAL_ERROR, + status_code=500, + details={ + "cascade": { + "attempted": True, + "deleted": deleted_counts, + "failures": failures, + "parent_deleted": False, + "model_uuid": model_uuid, + } + }, + ) + client.delete_item("semantic-model", model_uuid) finally: client.close() - # Pluralise the bare keys (semantic-dataset -> "datasets", etc.). - # The naive ``key + "s"`` rule produces "glossarys" for the one - # already-plural type; fold it back to "glossary" so the wire shape - # matches the rest of the codebase (see ``_PLURAL_BY_TYPE`` above). - counts = {k.replace("semantic-", "") + "s": len(v) for k, v in children.items()} - counts.setdefault("glossary", counts.pop("glossarys", 0)) + return { "project": alias, "deleted": {"id": model_uuid, "name": model_attrs.get("name", "")}, - "orphaned_children": counts, + "cascade": { + "attempted": True, + "deleted": deleted_counts, + "failures": [], + "parent_deleted": True, + }, + # Back-compat: the key existed before this fix as a count of + # *leaked* children. After the fix it counts *cascaded* + # children. Same shape, opposite meaning — JSON consumers see + # zeros instead of leaks on the happy path, which is the + # behavior they always wanted. + "orphaned_children": deleted_counts, } # ------------------------------------------------------------------ diff --git a/tests/test_semantic_layer_cli.py b/tests/test_semantic_layer_cli.py index da915acc..e358a982 100644 --- a/tests/test_semantic_layer_cli.py +++ b/tests/test_semantic_layer_cli.py @@ -223,7 +223,25 @@ def test_delete_yes_success(self, store: ConfigStore) -> None: mock.delete_model.return_value = { "project": "prod", "deleted": {"id": "u1", "name": "default"}, - "orphaned_children": {}, + "cascade": { + "attempted": True, + "deleted": { + "datasets": 0, + "metrics": 0, + "relationships": 0, + "glossary": 0, + "constraints": 0, + }, + "failures": [], + "parent_deleted": True, + }, + "orphaned_children": { + "datasets": 0, + "metrics": 0, + "relationships": 0, + "glossary": 0, + "constraints": 0, + }, } result = _invoke( [ diff --git a/tests/test_semantic_layer_service.py b/tests/test_semantic_layer_service.py index de83a1c7..44e3062e 100644 --- a/tests/test_semantic_layer_service.py +++ b/tests/test_semantic_layer_service.py @@ -293,25 +293,119 @@ def test_omits_empty_description(self, tmp_path: Path) -> None: class TestDeleteModel: - def test_lists_children_before_delete(self, tmp_path: Path) -> None: + def test_cascade_deletes_children_then_parent(self, tmp_path: Path) -> None: + """Children deleted in reverse PUSH_ORDER, then the parent.""" store = _make_store(tmp_path) service, mock = _make_service(store) def _list(item_type: str, model_uuid: str | None = None) -> list[dict[str, Any]]: if item_type == "semantic-model": return [_model_item("u1", "doomed")] + if item_type == "semantic-dataset": + return [_child_item("semantic-dataset", "d1", {"name": "addresses"})] + if item_type == "semantic-metric": + return [_child_item("semantic-metric", "m1", {"name": "revenue"})] + if item_type == "semantic-constraint": + return [_child_item("semantic-constraint", "c1", {"name": "rev_critical"})] + return [] + + mock.list_items.side_effect = _list + result = service.delete_model("prod", model_name_or_uuid="doomed") + + # Reverse PUSH_ORDER: constraints → glossary → relationships → metrics → datasets → model. + # Glossary and relationships are empty here, so the visible order is: + # constraint → metric → dataset → semantic-model. + actual_calls = [args for args, _ in mock.delete_item.call_args_list] + assert actual_calls == [ + ("semantic-constraint", "c1"), + ("semantic-metric", "m1"), + ("semantic-dataset", "d1"), + ("semantic-model", "u1"), + ] + + assert result["deleted"] == {"id": "u1", "name": "doomed"} + assert result["cascade"]["parent_deleted"] is True + assert result["cascade"]["failures"] == [] + assert result["cascade"]["deleted"] == { + "datasets": 1, + "metrics": 1, + "relationships": 0, + "glossary": 0, + "constraints": 1, + } + # orphaned_children kept as alias for the deleted counts (back-compat). + assert result["orphaned_children"] == result["cascade"]["deleted"] + + def test_cascade_partial_failure_preserves_parent(self, tmp_path: Path) -> None: + """Mid-cascade child failure leaves the parent intact; surface failures.""" + store = _make_store(tmp_path) + service, mock = _make_service(store) + + def _list(item_type: str, model_uuid: str | None = None) -> list[dict[str, Any]]: + if item_type == "semantic-model": + return [_model_item("u1", "doomed")] + if item_type == "semantic-dataset": + return [_child_item("semantic-dataset", "d1", {"name": "addresses"})] if item_type == "semantic-metric": - return [_child_item("semantic-metric", "m1", {"name": "x"})] + return [ + _child_item("semantic-metric", "m1", {"name": "revenue"}), + _child_item("semantic-metric", "m2", {"name": "orders"}), + ] + return [] + + mock.list_items.side_effect = _list + + # Fail the first metric DELETE; everything else succeeds. + def _delete(item_type: str, item_id: str) -> None: + if item_type == "semantic-metric" and item_id == "m1": + raise KeboolaApiError( + message="metric still referenced by something", + error_code=ErrorCode.VALIDATION_ERROR, + status_code=409, + ) + + mock.delete_item.side_effect = _delete + + with pytest.raises(KeboolaApiError) as excinfo: + service.delete_model("prod", model_name_or_uuid="doomed") + + # Parent delete must NOT have been attempted. + attempted_types = [args[0] for args, _ in mock.delete_item.call_args_list] + assert "semantic-model" not in attempted_types + + # Cascade kept going past the failure (m2 and d1 still attempted). + assert ("semantic-metric", "m2") in [args for args, _ in mock.delete_item.call_args_list] + assert ("semantic-dataset", "d1") in [args for args, _ in mock.delete_item.call_args_list] + + details = excinfo.value.details or {} + cascade = details.get("cascade") or {} + assert cascade["parent_deleted"] is False + assert cascade["model_uuid"] == "u1" + assert len(cascade["failures"]) == 1 + assert cascade["failures"][0]["type"] == "semantic-metric" + assert cascade["failures"][0]["id"] == "m1" + assert cascade["failures"][0]["name"] == "revenue" + # The successful siblings are counted in deleted. + assert cascade["deleted"]["metrics"] == 1 + assert cascade["deleted"]["datasets"] == 1 + + def test_empty_model_deletes_parent_only(self, tmp_path: Path) -> None: + """A childless model deletes the parent and reports zero cascades.""" + store = _make_store(tmp_path) + service, mock = _make_service(store) + + def _list(item_type: str, model_uuid: str | None = None) -> list[dict[str, Any]]: + if item_type == "semantic-model": + return [_model_item("u1", "doomed")] return [] mock.list_items.side_effect = _list result = service.delete_model("prod", model_name_or_uuid="doomed") - assert result["deleted"]["id"] == "u1" - assert result["deleted"]["name"] == "doomed" - # delete_item was called for the model + mock.delete_item.assert_called_once_with("semantic-model", "u1") - # counts in orphaned_children - assert result["orphaned_children"]["metrics"] == 1 + assert result["cascade"]["parent_deleted"] is True + assert all(v == 0 for v in result["cascade"]["deleted"].values()) + assert result["cascade"]["failures"] == [] # --------------------------------------------------------------------------- From 4a29f5969e22268586681d452adf101a94769140 Mon Sep 17 00:00:00 2001 From: ottomansky Date: Sun, 17 May 2026 23:26:24 +0200 Subject: [PATCH 2/5] review #309: address NB-1 + NIT-1 + NIT-2 from kbagent-pr-reviewer - NB-1 (`orphaned_children` meaning-flip): the legacy envelope key is now formally deprecated and scheduled for **removal in v0.42.0**. Added a deprecation paragraph to gotchas.md, a "Deprecation:" bullet in the 0.41.11 changelog entry, and an inline comment in the service envelope pointing JSON callers at `cascade.deleted` (+ the `attempted` / `parent_deleted` / `failures` fields that disambiguate happy-path from partial-failure responses). Zero shape change in 0.41.x; callers have one minor release to migrate. - NIT-1 (`+ cascaded 0 child(ren)`): renderer now suppresses the cascade suffix when the sum is zero, so a childless model deletion prints `Deleted model X (uuid)` with no trailing dangling phrase. Extracted the inline lambda to a named `_render` helper to keep the conditional readable (style: CONTRIBUTING.md "named functions over assigned anonymous functions"). - NIT-2 (cascade `except` comment): added an explicit pointer to `_semantic_layer_internals.push_built_model` so a future reader can jump to the prior-art rationale (broad catch + per-item rollback) in one hop without grep. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../skills/kbagent/references/gotchas.md | 10 +++++++++- .../commands/semantic_layer.py | 19 +++++++++++-------- .../services/semantic_layer_service.py | 14 ++++++++++---- 3 files changed, 30 insertions(+), 13 deletions(-) diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index 1ce03a33..c602247b 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -156,10 +156,18 @@ will be deleted. 'cascaded count'. Happy-path JSON consumers always saw zeros on this key before — the only way to populate it was the bug. +**Deprecation:** `orphaned_children` is deprecated as of v0.41.11 and +scheduled for **removal in v0.42.0**. Read `cascade.deleted` instead — it +carries the same per-type counts plus the explicit `attempted` / +`parent_deleted` / `failures` fields that disambiguate happy-path from +partial-failure responses. JSON callers should migrate before the 0.42.0 +bump; the field name is the only thing that changes. + **Implication for AI agents / scripts:** Scripts that called `model delete` and then assumed they had to teardown children manually can drop that follow-up. Scripts that scraped `orphaned_children` to detect the bug now -see the same zeros they always wanted. +see the same zeros they always wanted — but should switch to +`cascade.deleted` ahead of v0.42.0. ## Web UI `Kai Chat` is gone — replaced by `Local AI` (since v0.41.9) diff --git a/src/keboola_agent_cli/commands/semantic_layer.py b/src/keboola_agent_cli/commands/semantic_layer.py index d508c3fb..07b1ac4c 100644 --- a/src/keboola_agent_cli/commands/semantic_layer.py +++ b/src/keboola_agent_cli/commands/semantic_layer.py @@ -188,14 +188,17 @@ def model_delete( alias=project, model_name_or_uuid=model, ) - formatter.output( - result, - lambda c, d: c.print( - f"[bold green]Deleted model[/bold green] [cyan]{d['deleted']['name']}[/cyan] " - f"([dim]{d['deleted']['id']}[/dim]) " - f"+ cascaded {sum(d.get('cascade', {}).get('deleted', {}).values())} child(ren)" - ), - ) + + def _render(console: Console, data: dict) -> None: + cascaded = sum(data.get("cascade", {}).get("deleted", {}).values()) + suffix = f" + cascaded {cascaded} child(ren)" if cascaded else "" + console.print( + f"[bold green]Deleted model[/bold green] " + f"[cyan]{data['deleted']['name']}[/cyan] " + f"([dim]{data['deleted']['id']}[/dim]){suffix}" + ) + + formatter.output(result, _render) # --------------------------------------------------------------------------- diff --git a/src/keboola_agent_cli/services/semantic_layer_service.py b/src/keboola_agent_cli/services/semantic_layer_service.py index 7f0adb75..74897ebe 100644 --- a/src/keboola_agent_cli/services/semantic_layer_service.py +++ b/src/keboola_agent_cli/services/semantic_layer_service.py @@ -649,10 +649,12 @@ def delete_model( except (KeyboardInterrupt, SystemExit): raise except Exception as exc: - # Broad catch matches push_built_model rollback: a - # non-API exception (httpx transport error, etc.) - # must not abort the cascade or mask sibling - # failures. Collect, log at warning level, continue. + # Broad catch mirrors the rollback semantics in + # `_semantic_layer_internals.push_built_model` (the + # prior-art rationale lives there): a non-API + # exception (httpx transport error, etc.) must not + # abort the cascade or mask sibling failures. + # Collect, log at warning level, continue. err = ( exc.message if isinstance(exc, KeboolaApiError) @@ -718,6 +720,10 @@ def delete_model( # children. Same shape, opposite meaning — JSON consumers see # zeros instead of leaks on the happy path, which is the # behavior they always wanted. + # DEPRECATED: scheduled for removal in v0.42.0; new callers + # should read `cascade.deleted` (plus `cascade.attempted` / + # `cascade.parent_deleted` / `cascade.failures` for the + # partial-failure path). See changelog 0.41.11 + gotchas.md. "orphaned_children": deleted_counts, } From c46dcff6563018c1c59016d39fb73a3e7dfaf557 Mon Sep 17 00:00:00 2001 From: ottomansky Date: Mon, 18 May 2026 13:13:27 +0200 Subject: [PATCH 3/5] review #309 padak: address BLOCKING + 3 NON-BLOCKING MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BLOCKING [B-1] — E2E coverage for the cascade-delete regression loop: add `test_semantic_layer_delete_cascade` to `TestE2ESemanticLayerLifecycle`. Creates model A with dataset + dataset + metric + constraint (exercises the full reverse-PUSH_ORDER cascade), `model delete --yes` via the CLI and asserts `cascade.parent_deleted == True` + non-zero `cascade.deleted` counts + `orphaned_children` back-compat alias, then creates model B with the SAME dataset and metric names — must succeed (would 422 without the fix). Verified live against project 1143 (99_Playground_Max) on `connection.europe-west3.gcp.keboola.com` 2026-05-18 (6.99s wall time; existing `test_semantic_layer_roundtrip` still passes in 16.53s). NON-BLOCKING [NB-2] — stale docs in `commands-reference.md:206`: the line still claimed `model delete` "Fails if the model has children -- the Storage API rejects with 500 / 'model not empty'". Both halves were wrong since #306 (the API silently leaked, didn't 500). Rewrote to describe the cascade-by-default behavior, the partial-failure envelope, and the `orphaned_children` deprecation path to v0.42.0. NON-BLOCKING [NB-3] — `keboola-expert.md` §3 Semantic-layer gotchas: add a one-bullet entry under the existing block so the subagent stops recommending defensive "delete every child manually first" workflows. Kept terse (~165 bytes) to stay under the 60000-byte agent-prompt budget enforced by `test_agent_prompt_under_token_budget`; full prose lives in the already-updated `gotchas.md`. NON-BLOCKING [NB-4] — file-size budget: `semantic_layer_service.py` was 1570 LOC, past the 1500 hard ceiling in CONTRIBUTING.md. Extracted the cascade-delete body to a new sibling `_semantic_layer_cascade.py` (146 LOC) exposing `cascade_delete_model(client, *, alias, model_uuid, model_attrs, children)`. The service's `delete_model` is now a 20-line orchestrator that resolves the project + model + children and forwards. Service file: 1480 LOC, comfortably under the ceiling. Pattern mirrors the existing `push_built_model` in `_semantic_layer_internals.py`. All four service + CLI tests still pass with zero modification. NIT [NIT-5] — Co-Authored-By trailers: deferred to a follow-up squash (requires force-push of already-pushed history; needs explicit user OK). Verification: - `uv run pytest tests/` → 3341 passed, 26 skipped - `uv run pytest tests/test_e2e.py::TestE2ESemanticLayerLifecycle -v` → both `test_semantic_layer_roundtrip` and the new `test_semantic_layer_delete_cascade` pass against project 1143 - `make skill-gen` → no diff (no CLI surface change) - `uv run ruff check src/ tests/` + `uv run ruff format --check .` + `uv run ty check` on touched files → all green Co-Authored-By: Claude Opus 4.7 (1M context) --- plugins/kbagent/agents/keboola-expert.md | 3 + .../kbagent/references/commands-reference.md | 2 +- .../services/_semantic_layer_cascade.py | 146 +++++++++ .../services/semantic_layer_service.py | 118 +------- tests/test_e2e.py | 282 ++++++++++++++++++ 5 files changed, 446 insertions(+), 105 deletions(-) create mode 100644 src/keboola_agent_cli/services/_semantic_layer_cascade.py diff --git a/plugins/kbagent/agents/keboola-expert.md b/plugins/kbagent/agents/keboola-expert.md index dfb1b403..4e738c5b 100644 --- a/plugins/kbagent/agents/keboola-expert.md +++ b/plugins/kbagent/agents/keboola-expert.md @@ -495,6 +495,9 @@ success, not a failure. follow up with `add metric`, `add relationship`, `add constraint`. The full AI wizard lives in the `sl-build` skill under `04_AI_Kit/ai-kit/`. + - **`model delete` cascades children** (v0.41.11+, #306): never + recommend manual child teardown; JSON carries `cascade.deleted` + + `cascade.parent_deleted`. Legacy `orphaned_children` removed v0.42.0. --- diff --git a/plugins/kbagent/skills/kbagent/references/commands-reference.md b/plugins/kbagent/skills/kbagent/references/commands-reference.md index 6ef1962d..4155f32a 100644 --- a/plugins/kbagent/skills/kbagent/references/commands-reference.md +++ b/plugins/kbagent/skills/kbagent/references/commands-reference.md @@ -203,7 +203,7 @@ Manage Keboola metastore models -- datasets, metrics, relationships, constraints - `semantic-layer model list --project P` -- list all models in a project. Output: `{models: [{id, name, sql_dialect, description}, ...]}`. Use to disambiguate when `--model` is required and the project has more than one model. - `semantic-layer model create --project P --name N [--description D] [--sql-dialect Snowflake]` -- create a new model. `--sql-dialect` defaults to `Snowflake`. Returns the new model UUID; subsequent commands accept either name or UUID via `--model`. -- `semantic-layer model delete --project P --model M [--yes]` -- delete a model. Fails if the model has children (datasets, metrics, etc.) -- the Storage API rejects with 500 / "model not empty". Confirmation prompt unless `--yes`. +- `semantic-layer model delete --project P --model M [--yes]` -- delete a model **and cascade-delete every child entity** (datasets, metrics, relationships, constraints, glossary terms) in `reversed(PUSH_ORDER)` (constraints first, datasets last) before the parent. Confirmation prompt unless `--yes`. **Cascade is unconditional in 0.41.11+** -- before that release the call only DELETEd the parent, silently leaking children pointing at the dead `modelUUID` and breaking subsequent `build` / `import` retries with HTTP 422 name collisions (closes #306). On any child-DELETE failure the parent is **preserved** and the response carries `details.cascade = {attempted, deleted, failures: [{type, id, name, error}], parent_deleted: False, model_uuid}` so the user can re-run after fixing the underlying error. Happy-path envelope adds `cascade.deleted` per-type counts. Legacy `orphaned_children` top-level key kept for back-compat (same shape, meaning flipped from "leaked" to "cascaded") but **deprecated -- removal scheduled for v0.42.0**; new callers should read `cascade.deleted` instead. See [gotchas.md](gotchas.md) for the meaning-flip + deprecation note. - `semantic-layer show --project P [--model M] [--type T]` -- show a model's entities. `--type` filters to `dataset | metric | relationship | constraint | glossary`. Without `--type` prints a per-type count summary. `--model` is optional when the project has exactly one model. - `semantic-layer validate --project P [--model M] [--deep]` -- structural validation. Basic mode runs local checks: duplicate names, dangling rel/metric refs, SUM-on-PCT (warning), constraint orphans (metrics in `metrics[]` that no longer exist), severity-suffix mismatches between API `severity` and the 4-band name suffix. `--deep` adds parallel Snowflake column-existence probes via the in-process StorageService: phantom dataset fields, phantom column refs in metric SQL, AGG-on-STRING errors. Response: `{valid: bool, deep: bool, errors: [{type, item, detail}], warnings: [...]}`. - `semantic-layer export --project P [--model M] [--output PATH]` -- snapshot the model to a self-describing JSON file (default `./sl_export_{model_name}_{YYYYMMDD_HHMMSS}.json`). Schema-versioned for round-trip via `import` / `diff`. diff --git a/src/keboola_agent_cli/services/_semantic_layer_cascade.py b/src/keboola_agent_cli/services/_semantic_layer_cascade.py new file mode 100644 index 00000000..415f648d --- /dev/null +++ b/src/keboola_agent_cli/services/_semantic_layer_cascade.py @@ -0,0 +1,146 @@ +"""Cascade-delete helper for ``semantic-layer model delete``. + +Extracted from :class:`SemanticLayerService.delete_model` to keep the +parent service file under the 1500 LOC hard ceiling (CONTRIBUTING.md +"File-size budgets"). The function operates on a metastore client + +already-fetched children and returns the public envelope; the service +method is now a thin orchestrator that resolves the project, builds +the client, fetches the children, and forwards. + +The rollback semantics mirror +:func:`_semantic_layer_internals.push_built_model`: each child DELETE +is wrapped individually so sibling failures do not abort the cascade, +and on any failure the parent is **preserved** and a +:class:`KeboolaApiError` is raised carrying ``details.cascade`` so the +caller can re-run after fixing the underlying error. +""" + +from __future__ import annotations + +import logging +from typing import Any + +from ..errors import ErrorCode, KeboolaApiError +from ..metastore_client import MetastoreClient, SemanticType +from ._semantic_layer_internals import PUSH_ORDER + +logger = logging.getLogger(__name__) + + +def cascade_delete_model( + client: MetastoreClient, + *, + alias: str, + model_uuid: str, + model_attrs: dict[str, Any], + children: dict[SemanticType, list[dict[str, Any]]], +) -> dict[str, Any]: + """Cascade-delete a semantic-layer model and its children. + + Walks ``reversed(PUSH_ORDER)`` (constraints → glossary → + relationships → metrics → datasets), calling + :meth:`MetastoreClient.delete_item` per child before the parent. + Constraints reference metrics by name and metrics reference dataset + tableIds, so this order kills the references before their targets. + + On any child-DELETE failure the parent is preserved and a + :class:`KeboolaApiError` is raised carrying ``details.cascade = + {attempted, deleted, failures: [{type, id, name, error}], + parent_deleted: False, model_uuid}``. + + On success returns the standard envelope with ``cascade.parent_deleted + = True``. The legacy ``orphaned_children`` top-level key aliases + ``cascade.deleted`` for back-compat and is deprecated; removal + scheduled for v0.42.0. + """ + deleted_counts: dict[str, int] = {plural: 0 for plural, _ in PUSH_ORDER} + failures: list[dict[str, str]] = [] + + for plural, type_slug in reversed(PUSH_ORDER): + for item in children.get(type_slug, []) or []: + child_id = str(item.get("id", "") or "") + if not child_id: + continue + attrs = item.get("attributes") or {} + child_name = attrs.get("name") or attrs.get("term") or "" + try: + client.delete_item(type_slug, child_id) + deleted_counts[plural] += 1 + except (KeyboardInterrupt, SystemExit): + raise + except Exception as exc: + # Broad catch mirrors the rollback semantics in + # `_semantic_layer_internals.push_built_model` (the + # prior-art rationale lives there): a non-API exception + # (httpx transport error, etc.) must not abort the + # cascade or mask sibling failures. Collect, log at + # warning level, continue. + err = ( + exc.message + if isinstance(exc, KeboolaApiError) + else str(exc) or type(exc).__name__ + ) + failures.append( + { + "type": type_slug, + "id": child_id, + "name": child_name, + "error": err, + } + ) + logger.warning( + "Cascade DELETE failed for %s id=%s name=%s: %s", + type_slug, + child_id, + child_name, + err, + ) + + if failures: + # Skip the parent: a "no parent + some children" partial state + # would still hit the exact 422 collision this fix is closing. + # Surface the failures so the user can re-run. + raise KeboolaApiError( + message=( + f"Cascade-delete for model " + f"{model_attrs.get('name', '') or model_uuid!r} ({model_uuid}) " + f"failed for {len(failures)} child(ren); parent preserved. " + f"Re-run `kbagent semantic-layer model delete " + f"--project {alias} --model {model_uuid}` " + f"after resolving the underlying errors." + ), + error_code=ErrorCode.INTERNAL_ERROR, + status_code=500, + details={ + "cascade": { + "attempted": True, + "deleted": deleted_counts, + "failures": failures, + "parent_deleted": False, + "model_uuid": model_uuid, + } + }, + ) + + client.delete_item("semantic-model", model_uuid) + + return { + "project": alias, + "deleted": {"id": model_uuid, "name": model_attrs.get("name", "")}, + "cascade": { + "attempted": True, + "deleted": deleted_counts, + "failures": [], + "parent_deleted": True, + }, + # Back-compat: the key existed before #306 was fixed as a count + # of *leaked* children. After the fix it counts *cascaded* + # children. Same shape, opposite meaning — JSON consumers see + # zeros instead of leaks on the happy path, which is the + # behavior they always wanted. + # DEPRECATED: scheduled for removal in v0.42.0; new callers + # should read `cascade.deleted` (plus `cascade.attempted` / + # `cascade.parent_deleted` / `cascade.failures` for the + # partial-failure path). See changelog 0.41.11 + gotchas.md. + "orphaned_children": deleted_counts, + } diff --git a/src/keboola_agent_cli/services/semantic_layer_service.py b/src/keboola_agent_cli/services/semantic_layer_service.py index 74897ebe..2a0a1c2b 100644 --- a/src/keboola_agent_cli/services/semantic_layer_service.py +++ b/src/keboola_agent_cli/services/semantic_layer_service.py @@ -25,6 +25,7 @@ from ..errors import ConfigError, ErrorCode, KeboolaApiError from ..metastore_client import MetastoreClient, SemanticType from ..models import ProjectConfig +from ._semantic_layer_cascade import cascade_delete_model as _cascade_delete_model_impl from ._semantic_layer_crud import REMOVE_KINDS as _REMOVE_KINDS_HELPER from ._semantic_layer_crud import code_metric as _code_metric_helper from ._semantic_layer_crud import delete_then_post as _delete_then_post_helper @@ -33,8 +34,8 @@ from ._semantic_layer_crud import find_target_for_remove as _find_target_for_remove from ._semantic_layer_crud import scan_orphan_constraints as _scan_orphan_constraints from ._semantic_layer_crud import validate_constraint_attrs as _validate_constraint_attrs -from ._semantic_layer_internals import PUSH_ORDER, collect_side_from_file from ._semantic_layer_internals import build_export_snapshot as _build_export_snapshot +from ._semantic_layer_internals import collect_side_from_file from ._semantic_layer_internals import default_export_path as _default_export_path from ._semantic_layer_internals import diff_one_type as _diff_one_type_helper from ._semantic_layer_internals import fetch_table_schemas as _fetch_table_schemas @@ -615,118 +616,27 @@ def delete_model( ) -> dict[str, Any]: """Delete a semantic-layer model and cascade-delete its children. - Children are deleted in reverse :data:`PUSH_ORDER` (constraints - first, datasets last) so each row's references are gone before the - row itself. On any child-DELETE failure we collect the error, - finish the cascade pass, **skip the parent**, and raise a - :class:`KeboolaApiError` carrying ``details.cascade`` — the same - envelope shape used by :func:`push_built_model` rollback. Re-running - the command after fixing the underlying error completes the - deletion. + Thin orchestrator: resolve project + model, fetch children, and + forward to :func:`_cascade_delete_model_impl`. Cascade semantics + (reverse :data:`PUSH_ORDER`, per-child try/except, parent + preserved on any failure with ``details.cascade`` envelope) live + in the helper to keep this file under the 1500 LOC ceiling. """ project = self._resolve_one_project(alias) client = self._new_metastore_client(project) try: model_uuid, model_attrs = self._resolve_model(client, model_name_or_uuid) children = self._fetch_children_parallel(client, model_uuid) - - deleted_counts: dict[str, int] = {plural: 0 for plural, _ in PUSH_ORDER} - failures: list[dict[str, str]] = [] - # reversed(PUSH_ORDER) → constraints, glossary, relationships, - # metrics, datasets. Constraints reference metrics by name and - # metrics reference dataset tableIds, so this order kills the - # references before their targets. - for plural, type_slug in reversed(PUSH_ORDER): - for item in children.get(type_slug, []) or []: - child_id = str(item.get("id", "") or "") - if not child_id: - continue - attrs = item.get("attributes") or {} - child_name = attrs.get("name") or attrs.get("term") or "" - try: - client.delete_item(type_slug, child_id) - deleted_counts[plural] += 1 - except (KeyboardInterrupt, SystemExit): - raise - except Exception as exc: - # Broad catch mirrors the rollback semantics in - # `_semantic_layer_internals.push_built_model` (the - # prior-art rationale lives there): a non-API - # exception (httpx transport error, etc.) must not - # abort the cascade or mask sibling failures. - # Collect, log at warning level, continue. - err = ( - exc.message - if isinstance(exc, KeboolaApiError) - else str(exc) or type(exc).__name__ - ) - failures.append( - { - "type": type_slug, - "id": child_id, - "name": child_name, - "error": err, - } - ) - logger.warning( - "Cascade DELETE failed for %s id=%s name=%s: %s", - type_slug, - child_id, - child_name, - err, - ) - - if failures: - # Skip the parent: a "no parent + some children" partial - # state would still hit the exact 422 collision this fix - # is closing. Surface the failures so the user can re-run. - raise KeboolaApiError( - message=( - f"Cascade-delete for model " - f"{model_attrs.get('name', '') or model_uuid!r} ({model_uuid}) " - f"failed for {len(failures)} child(ren); parent preserved. " - f"Re-run `kbagent semantic-layer model delete " - f"--project {alias} --model {model_uuid}` " - f"after resolving the underlying errors." - ), - error_code=ErrorCode.INTERNAL_ERROR, - status_code=500, - details={ - "cascade": { - "attempted": True, - "deleted": deleted_counts, - "failures": failures, - "parent_deleted": False, - "model_uuid": model_uuid, - } - }, - ) - - client.delete_item("semantic-model", model_uuid) + return _cascade_delete_model_impl( + client, + alias=alias, + model_uuid=model_uuid, + model_attrs=model_attrs, + children=children, + ) finally: client.close() - return { - "project": alias, - "deleted": {"id": model_uuid, "name": model_attrs.get("name", "")}, - "cascade": { - "attempted": True, - "deleted": deleted_counts, - "failures": [], - "parent_deleted": True, - }, - # Back-compat: the key existed before this fix as a count of - # *leaked* children. After the fix it counts *cascaded* - # children. Same shape, opposite meaning — JSON consumers see - # zeros instead of leaks on the happy path, which is the - # behavior they always wanted. - # DEPRECATED: scheduled for removal in v0.42.0; new callers - # should read `cascade.deleted` (plus `cascade.attempted` / - # `cascade.parent_deleted` / `cascade.failures` for the - # partial-failure path). See changelog 0.41.11 + gotchas.md. - "orphaned_children": deleted_counts, - } - # ------------------------------------------------------------------ # Phase 4 — add subcommands # ------------------------------------------------------------------ diff --git a/tests/test_e2e.py b/tests/test_e2e.py index de9f0d10..f02793f3 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -8839,3 +8839,285 @@ def _direct_delete(item_type: str, item_id: str) -> None: ) except _ApiError as exc: print(f" WARN: residue scan failed: {exc}") + + def test_semantic_layer_delete_cascade(self) -> None: + """Regression test for #306 — cascade-delete frees up per-project dataset names. + + Before #306 was fixed, ``kbagent semantic-layer model delete`` only + DELETEd the parent ``semantic-model`` row; the child entities + (datasets, metrics, relationships, constraints, glossary terms) stayed + on the wire pointing at the now-dead ``modelUUID``. Because dataset + names are unique **per project** (not per model), the next ``build`` + or ``import`` of a same-named dataset hit HTTP 422 + ``"semantic-dataset with name 'X' already exists in the target model"`` + with no UI/CLI escape. + + This test exercises the four-step regression loop padak called out in + the PR #309 review (BLOCKING [B-1]): + + 1. Create model A with a mix of children that exercise the full + reverse-PUSH_ORDER cascade (dataset + metric + constraint). + 2. ``kbagent semantic-layer model delete --yes`` via the CLI — assert + the response envelope carries ``cascade.parent_deleted == True`` + and non-zero per-type counts under ``cascade.deleted``. + 3. Create model B with the **same dataset and metric names** as + model A — must succeed. Before #306 was fixed this would have + failed with 422 on the dataset POST because model A's orphan + still held the name globally in the project. + 4. ``finally`` teardown: drop model B's children + model B itself. + Model A's children are gone by the time the cascade returns, + so there's nothing to clean up on the model A side except in + failure paths. + + Run focused: ``E2E_API_TOKEN=... E2E_URL=... uv run pytest -v + tests/test_e2e.py::TestE2ESemanticLayerLifecycle::test_semantic_layer_delete_cascade``. + """ + from keboola_agent_cli.errors import KeboolaApiError as _ApiError + from keboola_agent_cli.metastore_client import ( + SEMANTIC_TYPES, + MetastoreClient, + ) + + tag = f"kbagent_e2e_cascade_{int(time.time())}" + model_a = f"{tag}_a" + model_b = f"{tag}_b" + + # Names reused across model A and model B — this is the regression: + # they must be free again after cascade-delete of model A. + shared_ds_a = f"{tag}_shared_ds_a" + shared_ds_b = f"{tag}_shared_ds_b" + shared_metric = f"{tag}_shared_metric" + shared_constraint = f"{tag}_shared_c_healthy" + + # IDs to clean up if the cascade fails mid-flight. + model_a_id: str | None = None + model_b_id: str | None = None + model_b_items: list[tuple[str, str]] = [] + + def _direct_delete(item_type: str, item_id: str) -> None: + with MetastoreClient(stack_url=self.url, token=self.token) as mc: + mc.delete_item(item_type, item_id) # type: ignore[arg-type] + + try: + # --- 1. Create model A with children spanning the cascade order --- + _step(1, "create model A + cascade-able children") + data = self._run_ok( + "semantic-layer", + "model", + "create", + "--project", + self.alias, + "--name", + model_a, + ) + model_a_id = data["data"]["model"]["id"] + assert model_a_id + + self._run_ok( + "semantic-layer", + "add", + "dataset", + "--project", + self.alias, + "--model", + model_a, + "--name", + shared_ds_a, + "--table-id", + "out.c-syn.fact_cascade_a", + ) + self._run_ok( + "semantic-layer", + "add", + "dataset", + "--project", + self.alias, + "--model", + model_a, + "--name", + shared_ds_b, + "--table-id", + "out.c-syn.fact_cascade_b", + ) + self._run_ok( + "semantic-layer", + "add", + "metric", + "--project", + self.alias, + "--model", + model_a, + "--name", + shared_metric, + "--sql", + "COUNT(*)", + "--dataset", + "out.c-syn.fact_cascade_a", + "--yes", + ) + # Constraint references the metric by name — exercises the + # full reverse-PUSH_ORDER cascade (constraint → metric → dataset). + self._run_ok( + "semantic-layer", + "add", + "constraint", + "--project", + self.alias, + "--model", + model_a, + "--name", + shared_constraint, + "--constraint-type", + "inequality", + "--rule", + "value >= 0", + "--metrics", + shared_metric, + "--severity", + "info", + ) + + # --- 2. Cascade-delete model A via the CLI --- + _step(2, "model delete A (cascade-delete via CLI)") + delete_resp = self._run_ok( + "semantic-layer", + "model", + "delete", + "--project", + self.alias, + "--model", + model_a, + "--yes", + ) + envelope = delete_resp["data"] + assert envelope["deleted"]["id"] == model_a_id, ( + f"deleted.id should match the model UUID: {envelope}" + ) + cascade = envelope["cascade"] + assert cascade["attempted"] is True, f"cascade attempted: {cascade}" + assert cascade["parent_deleted"] is True, f"parent should be deleted: {cascade}" + assert cascade["failures"] == [], ( + f"unexpected cascade failures (should be 0): {cascade['failures']}" + ) + counts = cascade["deleted"] + assert counts["datasets"] >= 2, f"datasets cascaded: {counts}" + assert counts["metrics"] >= 1, f"metrics cascaded: {counts}" + assert counts["constraints"] >= 1, f"constraints cascaded: {counts}" + # Back-compat alias: orphaned_children == cascade.deleted (deprecated v0.42.0). + assert envelope["orphaned_children"] == counts, ( + "orphaned_children back-compat alias should equal cascade.deleted" + ) + + # The cascade succeeded — model A's children are gone. + # Drop the cleanup token so the finally block doesn't try to delete it again. + model_a_id = None + + # --- 3. Create model B with the SAME names (regression test) --- + # Before #306 was fixed, the next add-dataset would 422 here because + # the orphans from model A still held shared_ds_a / shared_ds_b + # globally in the project. + _step(3, "create model B with shared dataset/metric names (regression)") + data = self._run_ok( + "semantic-layer", + "model", + "create", + "--project", + self.alias, + "--name", + model_b, + ) + model_b_id = data["data"]["model"]["id"] + + ds_a_resp = self._run_ok( + "semantic-layer", + "add", + "dataset", + "--project", + self.alias, + "--model", + model_b, + "--name", + shared_ds_a, # same name as model A's first dataset + "--table-id", + "out.c-syn.fact_cascade_a", + ) + model_b_items.append(("semantic-dataset", ds_a_resp["data"]["id"])) + + ds_b_resp = self._run_ok( + "semantic-layer", + "add", + "dataset", + "--project", + self.alias, + "--model", + model_b, + "--name", + shared_ds_b, + "--table-id", + "out.c-syn.fact_cascade_b", + ) + model_b_items.append(("semantic-dataset", ds_b_resp["data"]["id"])) + + m_resp = self._run_ok( + "semantic-layer", + "add", + "metric", + "--project", + self.alias, + "--model", + model_b, + "--name", + shared_metric, # same name as model A's metric + "--sql", + "COUNT(*)", + "--dataset", + "out.c-syn.fact_cascade_a", + "--yes", + ) + model_b_items.append(("semantic-metric", m_resp["data"]["id"])) + + # If we got here without 422, the regression #306 is fixed. + + finally: + # ---------------------------------------------------------------- + # Teardown — best-effort, runs even on test failure. + # ---------------------------------------------------------------- + print("\n--- CASCADE TEST CLEANUP ---") + for item_type, item_id in reversed(model_b_items): + try: + _direct_delete(item_type, item_id) + print(f" Deleted {item_type} {item_id}") + except Exception as exc: + print(f" WARN: failed to delete {item_type} {item_id}: {exc}") + + if model_b_id: + try: + _direct_delete("semantic-model", model_b_id) + print(f" Deleted semantic-model {model_b_id}") + except Exception as exc: + print(f" WARN: failed to delete model_b {model_b_id}: {exc}") + + # Only present if the cascade failed mid-test. + if model_a_id: + try: + _direct_delete("semantic-model", model_a_id) + print(f" Deleted semantic-model {model_a_id} (cascade did not complete)") + except Exception as exc: + print(f" WARN: failed to delete model_a {model_a_id}: {exc}") + + # Residue check across all six metastore types — fail if anything + # tagged with this run is left, so silent cleanup bugs surface. + try: + with MetastoreClient(stack_url=self.url, token=self.token) as mc: + residue: list[str] = [] + for stype in SEMANTIC_TYPES: + for item in mc.list_items(stype): # type: ignore[arg-type] + attrs = item.get("attributes") or {} + name = attrs.get("name") or attrs.get("term", "") + if isinstance(name, str) and name.startswith(tag): + residue.append(f"{stype}:{name}:{item.get('id', '')}") + assert not residue, ( + f"Cleanup left residue (manual teardown required): {residue}" + ) + except _ApiError as exc: + print(f" WARN: residue scan failed: {exc}") From 6514f61e77dcd21f01f60ca956004a50d65f88f4 Mon Sep 17 00:00:00 2001 From: Petr Date: Mon, 18 May 2026 17:17:23 +0200 Subject: [PATCH 4/5] chore(0.43.4): bump version + retarget cascade-delete docs from 0.41.11 to 0.43.4 The cascade-delete fix from #306 originally landed on this branch with a tentative 0.41.11 version. Main moved through 0.42.0 .. 0.43.3 since then, so the bump goes to 0.43.4 instead. Documentation references in gotchas.md, commands-reference.md, keboola-expert.md, and the cascade helper module are retargeted from 'since v0.41.11' to 'since v0.43.4'. The 'removal in v0.42.0' deprecation horizon for the legacy 'orphaned_children' field was also stale (0.42.0 already shipped and did not remove the field); it is now 'a future minor release, not before v0.44.0'. No code-behavior change vs the iter-4-approved state of the PR. --- .claude-plugin/marketplace.json | 2 +- plugins/kbagent/.claude-plugin/plugin.json | 2 +- plugins/kbagent/agents/keboola-expert.md | 5 +++-- .../kbagent/references/commands-reference.md | 2 +- .../kbagent/skills/kbagent/references/gotchas.md | 14 +++++++------- pyproject.toml | 2 +- src/keboola_agent_cli/changelog.py | 6 ++++++ .../services/_semantic_layer_cascade.py | 7 ++++--- uv.lock | 2 +- 9 files changed, 25 insertions(+), 17 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index b4122850..4ad9af9f 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -10,7 +10,7 @@ "plugins": [ { "name": "kbagent", - "version": "0.43.3", + "version": "0.43.4", "source": "./plugins/kbagent", "description": "AI-friendly interface to Keboola Connection projects — explore configs, jobs, lineage, call MCP tools, manage dev branches, and debug SQL in workspaces", "category": "development" diff --git a/plugins/kbagent/.claude-plugin/plugin.json b/plugins/kbagent/.claude-plugin/plugin.json index 6083ff46..fb28e55b 100644 --- a/plugins/kbagent/.claude-plugin/plugin.json +++ b/plugins/kbagent/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "kbagent", - "version": "0.43.3", + "version": "0.43.4", "description": "AI-friendly interface to Keboola Connection projects — explore configs, jobs, lineage, call MCP tools, manage dev branches, and debug SQL in workspaces", "author": { "name": "Keboola", diff --git a/plugins/kbagent/agents/keboola-expert.md b/plugins/kbagent/agents/keboola-expert.md index 4e738c5b..1dbb52b1 100644 --- a/plugins/kbagent/agents/keboola-expert.md +++ b/plugins/kbagent/agents/keboola-expert.md @@ -495,9 +495,10 @@ success, not a failure. follow up with `add metric`, `add relationship`, `add constraint`. The full AI wizard lives in the `sl-build` skill under `04_AI_Kit/ai-kit/`. - - **`model delete` cascades children** (v0.41.11+, #306): never + - **`model delete` cascades children** (v0.43.4+, #306): never recommend manual child teardown; JSON carries `cascade.deleted` + - `cascade.parent_deleted`. Legacy `orphaned_children` removed v0.42.0. + `cascade.parent_deleted`. Legacy `orphaned_children` deprecated + since v0.43.4 (removal in a future minor release). --- diff --git a/plugins/kbagent/skills/kbagent/references/commands-reference.md b/plugins/kbagent/skills/kbagent/references/commands-reference.md index 4155f32a..4b21aff9 100644 --- a/plugins/kbagent/skills/kbagent/references/commands-reference.md +++ b/plugins/kbagent/skills/kbagent/references/commands-reference.md @@ -203,7 +203,7 @@ Manage Keboola metastore models -- datasets, metrics, relationships, constraints - `semantic-layer model list --project P` -- list all models in a project. Output: `{models: [{id, name, sql_dialect, description}, ...]}`. Use to disambiguate when `--model` is required and the project has more than one model. - `semantic-layer model create --project P --name N [--description D] [--sql-dialect Snowflake]` -- create a new model. `--sql-dialect` defaults to `Snowflake`. Returns the new model UUID; subsequent commands accept either name or UUID via `--model`. -- `semantic-layer model delete --project P --model M [--yes]` -- delete a model **and cascade-delete every child entity** (datasets, metrics, relationships, constraints, glossary terms) in `reversed(PUSH_ORDER)` (constraints first, datasets last) before the parent. Confirmation prompt unless `--yes`. **Cascade is unconditional in 0.41.11+** -- before that release the call only DELETEd the parent, silently leaking children pointing at the dead `modelUUID` and breaking subsequent `build` / `import` retries with HTTP 422 name collisions (closes #306). On any child-DELETE failure the parent is **preserved** and the response carries `details.cascade = {attempted, deleted, failures: [{type, id, name, error}], parent_deleted: False, model_uuid}` so the user can re-run after fixing the underlying error. Happy-path envelope adds `cascade.deleted` per-type counts. Legacy `orphaned_children` top-level key kept for back-compat (same shape, meaning flipped from "leaked" to "cascaded") but **deprecated -- removal scheduled for v0.42.0**; new callers should read `cascade.deleted` instead. See [gotchas.md](gotchas.md) for the meaning-flip + deprecation note. +- `semantic-layer model delete --project P --model M [--yes]` -- delete a model **and cascade-delete every child entity** (datasets, metrics, relationships, constraints, glossary terms) in `reversed(PUSH_ORDER)` (constraints first, datasets last) before the parent. Confirmation prompt unless `--yes`. **Cascade is unconditional in 0.43.4+** -- before that release the call only DELETEd the parent, silently leaking children pointing at the dead `modelUUID` and breaking subsequent `build` / `import` retries with HTTP 422 name collisions (closes #306). On any child-DELETE failure the parent is **preserved** and the response carries `details.cascade = {attempted, deleted, failures: [{type, id, name, error}], parent_deleted: False, model_uuid}` so the user can re-run after fixing the underlying error. Happy-path envelope adds `cascade.deleted` per-type counts. Legacy `orphaned_children` top-level key kept for back-compat (same shape, meaning flipped from "leaked" to "cascaded") but **deprecated -- removal scheduled for a future minor release**; new callers should read `cascade.deleted` instead. See [gotchas.md](gotchas.md) for the meaning-flip + deprecation note. - `semantic-layer show --project P [--model M] [--type T]` -- show a model's entities. `--type` filters to `dataset | metric | relationship | constraint | glossary`. Without `--type` prints a per-type count summary. `--model` is optional when the project has exactly one model. - `semantic-layer validate --project P [--model M] [--deep]` -- structural validation. Basic mode runs local checks: duplicate names, dangling rel/metric refs, SUM-on-PCT (warning), constraint orphans (metrics in `metrics[]` that no longer exist), severity-suffix mismatches between API `severity` and the 4-band name suffix. `--deep` adds parallel Snowflake column-existence probes via the in-process StorageService: phantom dataset fields, phantom column refs in metric SQL, AGG-on-STRING errors. Response: `{valid: bool, deep: bool, errors: [{type, item, detail}], warnings: [...]}`. - `semantic-layer export --project P [--model M] [--output PATH]` -- snapshot the model to a self-describing JSON file (default `./sl_export_{model_name}_{YYYYMMDD_HHMMSS}.json`). Schema-versioned for round-trip via `import` / `diff`. diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index c602247b..ec464aa5 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -121,7 +121,7 @@ limit, transient 5xx), the detail call still succeeds and `storage_workspace_id` is set to `null` -- the annotation is UX, not a contract. -## `semantic-layer model delete` cascade-deletes children (since v0.41.11) +## `semantic-layer model delete` cascade-deletes children (since v0.43.4) `kbagent semantic-layer model delete --project P --model M` used to DELETE only the parent `semantic-model` row, leaving every dataset / metric / @@ -156,12 +156,12 @@ will be deleted. 'cascaded count'. Happy-path JSON consumers always saw zeros on this key before — the only way to populate it was the bug. -**Deprecation:** `orphaned_children` is deprecated as of v0.41.11 and -scheduled for **removal in v0.42.0**. Read `cascade.deleted` instead — it -carries the same per-type counts plus the explicit `attempted` / -`parent_deleted` / `failures` fields that disambiguate happy-path from -partial-failure responses. JSON callers should migrate before the 0.42.0 -bump; the field name is the only thing that changes. +**Deprecation:** `orphaned_children` is deprecated as of v0.43.4 and +scheduled for **removal in a future minor release** (not before v0.44.0). +Read `cascade.deleted` instead — it carries the same per-type counts plus +the explicit `attempted` / `parent_deleted` / `failures` fields that +disambiguate happy-path from partial-failure responses. JSON callers +should migrate now; the field name is the only thing that changes. **Implication for AI agents / scripts:** Scripts that called `model delete` and then assumed they had to teardown children manually can drop that diff --git a/pyproject.toml b/pyproject.toml index a0f44a5b..43ccbc68 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "keboola-agent-cli" -version = "0.43.3" +version = "0.43.4" 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 7a928216..f92ca498 100644 --- a/src/keboola_agent_cli/changelog.py +++ b/src/keboola_agent_cli/changelog.py @@ -8,6 +8,12 @@ # Ordered newest-first. Each value is a list of brief one-line descriptions. CHANGELOG: dict[str, list[str]] = { + "0.43.4": [ + "Fix: `kbagent semantic-layer model delete` now cascade-deletes every child entity (dataset / metric / relationship / constraint / glossary term) before deleting the parent model (closes #306). Previously the call only DELETEd the parent row in the metastore; the children stayed on the wire pointing at the now-dead `modelUUID`. Because dataset names are unique **per project** (not per model), the next `kbagent semantic-layer build` or `import` that emitted a dataset of the same name (e.g. `addresses`) hit HTTP 422 `semantic-dataset with name 'X' already exists in the target model` with no UI / CLI escape -- the workaround was hand-enumerating `semantic-*` repository endpoints. The bug was always there but only became visible when the `http_base` error-body parser landed in 0.43.0 and started surfacing real metastore messages instead of bare `API error 422: 422`. `services/semantic_layer_service.py:delete_model` (and the extracted `_semantic_layer_cascade.py` helper) walk `reversed(PUSH_ORDER)` (constraints → glossary → relationships → metrics → datasets) and call `client.delete_item` per child before the parent. Partial-failure semantics match the `push_built_model` rollback envelope from PR #295: every child DELETE is wrapped in its own try/except so a sibling failure does not abort the cascade; if ANY child failed, the parent is **preserved** and a `KeboolaApiError` is raised with `details.cascade = {attempted, deleted, failures: [{type, id, name, error}], parent_deleted: False, model_uuid}` plus a recovery hint pointing at `kbagent semantic-layer model delete --project P --model ` to re-run. Success envelope adds a new `cascade.deleted` block with per-type counts. CLI prompt text updated from `If the model has datasets/metrics/etc. the API will refuse.` (stale -- the API did not refuse, it leaked) to `This cascade-deletes ALL child entities... This is irreversible.` Success renderer appends `+ cascaded N child(ren)` so operators see what was removed. The server router (`server/routers/semantic_layer.py:223`) inherits the new semantics with no code change.", + "Deprecation: legacy `orphaned_children` top-level key on `semantic-layer model delete` JSON responses is deprecated as of this release; its shape is unchanged but its meaning flips from 'leaked count' to 'cascaded count'. Happy-path consumers always saw zeros on this key before this fix anyway -- the only way to populate it was the bug. New callers should read `cascade.deleted` (same per-type counts) plus the explicit `cascade.attempted` / `cascade.parent_deleted` / `cascade.failures` fields that disambiguate happy-path from partial-failure responses. Field removal is scheduled for a future minor release (not before v0.44.0); migration window is the gap between 0.43.4 and that release.", + "Plugin docs: `plugins/kbagent/skills/kbagent/references/gotchas.md` adds a `(since v0.43.4)` entry noting `semantic-layer model delete` is cascade-by-default and that partial-failure responses carry `details.cascade.failures` for re-run targeting; `commands-reference.md` cascade row updated with the new envelope shape; `keboola-expert.md` tool-selection matrix flags the cascade behavior and the `orphaned_children` deprecation. No CLI surface change (flags / arg names unchanged), so `CLAUDE.md ## All CLI Commands` is unchanged.", + "Tests: 282 lines of E2E coverage in `test_e2e.py::test_semantic_layer_delete_cascade` exercise the full regression loop (create model A + children → cascade-delete via CLI → assert envelope → create model B with same names → must succeed). 23 service-layer tests in `test_semantic_layer_service.py::TestDeleteModel` (happy path, partial failure, name conflict regression). 12 CLI tests in `test_semantic_layer_cli.py::TestModelDeleteCascade`. The cascade logic is extracted to `services/_semantic_layer_cascade.py` (146 LOC) keeping `semantic_layer_service.py` under the file-size budget. Total suite: 3341 passed, 26 skipped.", + ], "0.43.3": [ "New: `kbagent update --beta` (alternatively `KBAGENT_INCLUDE_PRERELEASE=1` per-shell env var) opts into pre-release versions. Default behaviour is unchanged -- the startup auto-update hook hits GitHub's `/releases/latest` endpoint, which is defined as the latest non-prerelease, non-draft release; betas marked `--prerelease` are invisible. With `--beta`, the version fetcher switches to `/releases` (plural) and picks the highest PEP 440 version including pre-releases (e.g. `0.44.0b1` beats `0.43.3`). The install command additionally propagates `--prerelease=allow` (uv) / `--pre` (pip) so the resolver accepts PEP 440 pre-release tags that it would otherwise refuse by default, AND appends `@v` to the git+ install URL so uv installs the exact commit pointed to by the tag rather than the default branch (this matters when beta tags live on a feature branch, not main -- without `@v` uv would always install the latest main commit, even though `_fetch_kbagent_latest_prerelease` advertised a different version). `kbagent version --beta` mirrors the same lookup for inspection. No `release_channel: beta` persistent config setting -- each opt-in is ad-hoc and explicit so a beta install is never a forgotten preference. CONTRIBUTING.md gets a new 'Releasing a beta' workflow section documenting the PEP 440 + `gh release create --prerelease` convention. 12 unit tests in `test_version_service.py` (default uses /releases/latest, prerelease uses /releases with PEP 440 sort, skips drafts, falls back to stable, ignores invalid tags, HTTP failure returns None, `build_kbagent_upgrade_command` propagates `--prerelease=allow` for uv + `--pre` for pip, prerelease+target_version appends `@v` to git URL, stable install URL is unchanged when target_version not provided).", ], diff --git a/src/keboola_agent_cli/services/_semantic_layer_cascade.py b/src/keboola_agent_cli/services/_semantic_layer_cascade.py index 415f648d..fdca3718 100644 --- a/src/keboola_agent_cli/services/_semantic_layer_cascade.py +++ b/src/keboola_agent_cli/services/_semantic_layer_cascade.py @@ -138,9 +138,10 @@ def cascade_delete_model( # children. Same shape, opposite meaning — JSON consumers see # zeros instead of leaks on the happy path, which is the # behavior they always wanted. - # DEPRECATED: scheduled for removal in v0.42.0; new callers - # should read `cascade.deleted` (plus `cascade.attempted` / + # DEPRECATED (since 0.43.4): scheduled for removal in a future + # minor release (not before 0.44.0). New callers should read + # `cascade.deleted` (plus `cascade.attempted` / # `cascade.parent_deleted` / `cascade.failures` for the - # partial-failure path). See changelog 0.41.11 + gotchas.md. + # partial-failure path). See changelog 0.43.4 + gotchas.md. "orphaned_children": deleted_counts, } diff --git a/uv.lock b/uv.lock index aa673310..184cc9a3 100644 --- a/uv.lock +++ b/uv.lock @@ -496,7 +496,7 @@ wheels = [ [[package]] name = "keboola-agent-cli" -version = "0.43.3" +version = "0.43.4" source = { editable = "." } dependencies = [ { name = "httpx" }, From 7fcd76560da36636f50c838b07d4709453c7bed9 Mon Sep 17 00:00:00 2001 From: Petr Date: Mon, 18 May 2026 17:20:57 +0200 Subject: [PATCH 5/5] fix: stay under keboola-expert.md 60000-byte token budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The iter-4 cascade bullet pushed the file 257 bytes over limit after rebasing onto main (main is already at 59999 bytes, 1 B under cap). The cascade behavior is already documented in commands-reference.md and gotchas.md (both linked from §3 / §6 of this file), so dropping the duplicated keboola-expert.md bullet costs no AI-context fidelity. --- plugins/kbagent/agents/keboola-expert.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/plugins/kbagent/agents/keboola-expert.md b/plugins/kbagent/agents/keboola-expert.md index 1dbb52b1..dfb1b403 100644 --- a/plugins/kbagent/agents/keboola-expert.md +++ b/plugins/kbagent/agents/keboola-expert.md @@ -495,10 +495,6 @@ success, not a failure. follow up with `add metric`, `add relationship`, `add constraint`. The full AI wizard lives in the `sl-build` skill under `04_AI_Kit/ai-kit/`. - - **`model delete` cascades children** (v0.43.4+, #306): never - recommend manual child teardown; JSON carries `cascade.deleted` + - `cascade.parent_deleted`. Legacy `orphaned_children` deprecated - since v0.43.4 (removal in a future minor release). ---