From 5c7e55cda251d43123e8c18ed6e12940a1e94c92 Mon Sep 17 00:00:00 2001 From: Petr Date: Fri, 21 Aug 2026 23:27:09 +0200 Subject: [PATCH 1/4] feat(storage): add update_table_definition + delete_table_metadata client methods (#624) PUT /v2/storage/branch/{branch}/tables/{id}/definition (async, waits for the tableDefinitionUpdate storage job; production uses the literal branch ref "default", matching the web UI client) and the synchronous metadata entry DELETE. Fixes the set_table_metadata docstring: the flat KBC.column.{name}.description convention is legacy, invisible to the Keboola UI and MCP server, kept only for migration and read fallback. --- plugins/kbagent/skills/kbagent/SKILL.md | 9 +- .../client/storage_tables.py | 96 ++- .../commands/_storage_describe.py | 549 ++++++++++++++++++ .../services/_column_descriptions.py | 503 ++++++++++++++++ tests/test_storage_tables.py | 154 ++++- 5 files changed, 1302 insertions(+), 9 deletions(-) create mode 100644 src/keboola_agent_cli/commands/_storage_describe.py create mode 100644 src/keboola_agent_cli/services/_column_descriptions.py diff --git a/plugins/kbagent/skills/kbagent/SKILL.md b/plugins/kbagent/skills/kbagent/SKILL.md index 4c1ab0a4..120f68bd 100644 --- a/plugins/kbagent/skills/kbagent/SKILL.md +++ b/plugins/kbagent/skills/kbagent/SKILL.md @@ -167,10 +167,6 @@ When working inside a git repository or project directory, run `kbagent init` (o | Swap two storage tables (any branch, including the default/production branch) | `kbagent storage swap-tables --project PROJECT --table-id TABLE-ID --target-table-id TARGET-TABLE-ID` | | Clone (pull) a production table into a development branch | `kbagent storage clone-table --project PROJECT --table-id TABLE-ID` | | Delete one or more storage buckets | `kbagent storage delete-bucket --project PROJECT --bucket-id BUCKET-ID` | -| Set the description on a storage bucket | `kbagent storage describe-bucket --project PROJECT --bucket-id BUCKET-ID` | -| Set the description on a storage table | `kbagent storage describe-table --project PROJECT --table-id TABLE-ID` | -| Set descriptions on one or more columns of a storage table | `kbagent storage describe-column --project PROJECT --table-id TABLE-ID --column COLUMN` | -| Apply descriptions to buckets, tables, and columns from a YAML file | `kbagent storage describe-batch --project PROJECT --from-file FROM-FILE` | | List Storage Files with optional tag filtering | `kbagent storage files --project PROJECT` | | Show Storage File metadata (without downloading) | `kbagent storage file-detail --project PROJECT --file-id FILE-ID` | | Upload a local file to Storage Files | `kbagent storage file-upload --project PROJECT --file FILE` | @@ -184,6 +180,11 @@ When working inside a git repository or project directory, run `kbagent init` (o | Show one snapshot's detail (source table, creation time, description) | `kbagent storage snapshot-detail --project PROJECT --snapshot-id SNAPSHOT-ID` | | Delete one or more table snapshots (the source tables are untouched) | `kbagent storage snapshot-delete --project PROJECT --snapshot-id SNAPSHOT-ID` | | Create a NEW table from an existing snapshot (snapshot restore) | `kbagent storage table-from-snapshot --project PROJECT --snapshot-id SNAPSHOT-ID --bucket-id BUCKET-ID --name NAME` | +| Set the description on a storage bucket | `kbagent storage describe-bucket --project PROJECT --bucket-id BUCKET-ID` | +| Set the description on a storage table | `kbagent storage describe-table --project PROJECT --table-id TABLE-ID` | +| Set descriptions on one or more columns of a storage table | `kbagent storage describe-column --project PROJECT --table-id TABLE-ID --column COLUMN` | +| Apply descriptions to buckets, tables, and columns from a YAML file | `kbagent storage describe-batch --project PROJECT --from-file FROM-FILE` | +| Convert legacy KBC.column.* descriptions to the native definition endpoint | `kbagent storage describe-migrate --project PROJECT` | | List Data Streams sources in a project | `kbagent stream list --project PROJECT` | | Create an OTLP (or HTTP) source and return its endpoint | `kbagent stream create-source --project PROJECT --name NAME` | | Show a source's endpoints, protocol, and destination tables | `kbagent stream detail [SOURCE-ID] --project PROJECT` | diff --git a/src/keboola_agent_cli/client/storage_tables.py b/src/keboola_agent_cli/client/storage_tables.py index 08f6b2a2..ca2c0587 100644 --- a/src/keboola_agent_cli/client/storage_tables.py +++ b/src/keboola_agent_cli/client/storage_tables.py @@ -123,10 +123,14 @@ def set_table_metadata( POST /v2/storage/tables/{id}/metadata Provider is always ``"user"`` for CLI-originated descriptions. - Column-level descriptions use the namespaced key convention - ``KBC.column.{colname}.description`` stored at table-metadata level - (Keboola Storage API does not expose a user-writable column-metadata - endpoint; ``columnMetadata`` is populated exclusively by components). + + The flat ``KBC.column.{colname}.description`` table-metadata key this + method used to carry column descriptions is LEGACY (pre-0.88.0, issue + #624): nothing reads it except this CLI -- neither the Keboola UI nor + the MCP server -- and a metadata write never reaches the native column + description field. ``update_table_definition`` supersedes it. The key + convention survives only so ``describe_migrate`` and the table-detail + read fallback can still find entries written by older versions. Args: table_id: Full table ID (e.g. "in.c-bucket.table"). @@ -145,6 +149,90 @@ def set_table_metadata( response = self._request("POST", f"{prefix}/tables/{safe_id}/metadata", data=form) return response.json() + def update_table_definition( + self, + table_id: str, + columns: list[dict[str, Any]] | None = None, + description: str | None = None, + description_set: bool = False, + is_description_system_managed: bool | None = None, + branch_id: int | None = None, + ) -> dict[str, Any]: + """Update a table's native definition (async, waits for the storage job). + + PUT /v2/storage/branch/{branch}/tables/{id}/definition + + This is the endpoint the web UI writes column descriptions with, and + the only one whose values the backend mirrors into ``columnMetadata`` + (``KBC.description``) -- so a single write here is visible to the UI, + to the MCP server and to the underlying backend (Snowflake COMMENT / + BigQuery description). The mirroring is one-way: a metadata POST never + travels back into the native field (issue #624). + + The definition endpoint is branch-scoped only; production uses the + literal branch ref ``"default"`` (matching the web UI's client). + + Args: + table_id: Full table ID (e.g. "in.c-bucket.table"). + columns: Column entries ``{"name": str, "description": str | None}``; + a ``None`` description clears that column's description. Omitted + from the body entirely when ``None``. + description: New table-level description. Only sent when + ``description_set`` is True. + description_set: Distinguishes "clear the table description" + (``description=None`` + this flag) from "don't touch it" + (flag False), which a bare ``None`` cannot express. + is_description_system_managed: When False, marks the descriptions + as user-authored so the next component run's Output Mapping + does not overwrite them. Omitted from the body when ``None``. + branch_id: If set, target a specific dev branch. + + Returns: + Completed storage job dict. + + Raises: + KeboolaApiError: If the storage job fails or times out. + """ + branch_ref = str(branch_id) if branch_id else "default" + safe_id = quote(table_id, safe="") + body: dict[str, Any] = {} + if columns is not None: + body["columns"] = columns + if description_set: + body["description"] = description + if is_description_system_managed is not None: + body["isDescriptionSystemManaged"] = is_description_system_managed + response = self._request( + "PUT", + f"/v2/storage/branch/{branch_ref}/tables/{safe_id}/definition", + json=body, + ) + return self._wait_for_storage_job(response.json()) + + def delete_table_metadata( + self, + table_id: str, + metadata_id: int | str, + branch_id: int | None = None, + ) -> None: + """Delete a single metadata entry on a storage table by its numeric ID. + + DELETE /v2/storage/[branch/{b}/]tables/{id}/metadata/{metadataId} + + Synchronous (204). Used to retire legacy flat + ``KBC.column.{colname}.description`` entries once their value has been + written through ``update_table_definition`` -- leaving them behind + would resurrect a description the user later cleared (issue #624). + + Args: + table_id: Full table ID (e.g. "in.c-bucket.table"). + metadata_id: ID of the metadata entry (from the table detail). + branch_id: If set, target a specific dev branch. + """ + prefix = f"/v2/storage/branch/{branch_id}" if branch_id else "/v2/storage" + safe_id = quote(table_id, safe="") + self._request("DELETE", f"{prefix}/tables/{safe_id}/metadata/{metadata_id}") + def get_bucket_detail( self, bucket_id: str, diff --git a/src/keboola_agent_cli/commands/_storage_describe.py b/src/keboola_agent_cli/commands/_storage_describe.py new file mode 100644 index 00000000..9e10a259 --- /dev/null +++ b/src/keboola_agent_cli/commands/_storage_describe.py @@ -0,0 +1,549 @@ +"""Description (metadata write) commands for the ``kbagent storage`` group. + +``describe-bucket`` / ``describe-table`` / ``describe-column`` / +``describe-batch`` / ``describe-migrate`` -- thin CLI layer over +:class:`services.storage_service.StorageService`. + +Lives in a private module because ``commands/storage.py`` is already past the +commands-file ceiling (CONTRIBUTING.md). The commands are mounted flat onto +``storage_app`` via :func:`register`, so permission keys stay in the +``storage.*`` namespace and ``kbagent storage --help`` lists them together in +the "Descriptions" panel. +""" + +from pathlib import Path +from typing import Any + +import typer +from rich.markup import escape + +from ..config_store import ConfigStore +from ..errors import ConfigError, ErrorCode, KeboolaApiError +from ._helpers import ( + get_formatter, + get_service, + map_error_to_exit_code, + resolve_branch, +) + +_DESCRIBE = "Descriptions" + + +def register(app: typer.Typer) -> None: + """Mount the describe commands onto ``app`` (the ``storage`` Typer group).""" + + @app.command("describe-bucket", rich_help_panel=_DESCRIBE) + def storage_describe_bucket( + ctx: typer.Context, + project: str = typer.Option( + ..., + "--project", + help="Project alias", + ), + bucket_id: str = typer.Option( + ..., + "--bucket-id", + help="Bucket ID (e.g. 'in.c-my-bucket')", + ), + text: str | None = typer.Option( + None, + "--text", + help="Description text (inline)", + ), + file: Path | None = typer.Option( + None, + "--file", + help="Path to a file containing the description", + ), + stdin: bool = typer.Option( + False, + "--stdin", + help="Read description from standard input", + ), + branch: int | None = typer.Option( + None, + "--branch", + help="Dev branch ID (defaults to active branch if set via 'branch use')", + ), + ) -> None: + """Set the description on a storage bucket. + + Stores the description as KBC.description in bucket metadata (upsert). + Provide the text via --text, --file, or --stdin (exactly one required). + """ + formatter = get_formatter(ctx) + service = get_service(ctx, "storage_service") + config_store: ConfigStore = ctx.obj["config_store"] + _, effective_branch = resolve_branch(config_store, formatter, project, branch) + + from ._metadata_input import resolve_text_input + + try: + description = resolve_text_input(text=text, file=file, stdin=stdin) + except ConfigError as exc: + formatter.error(message=exc.message, error_code=ErrorCode.INVALID_ARGUMENT) + raise typer.Exit(code=2) from None + + try: + result = service.describe_bucket( + alias=project, + bucket_id=bucket_id, + description=description, + branch_id=effective_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, retryable=exc.retryable) + raise typer.Exit(code=map_error_to_exit_code(exc)) from None + + if formatter.json_mode: + formatter.output(result) + else: + formatter.console.print(f"[bold green]Description set:[/bold green] {bucket_id}") + formatter.console.print(f" {escape(description[:120])}") + + @app.command("describe-table", rich_help_panel=_DESCRIBE) + def storage_describe_table( + ctx: typer.Context, + project: str = typer.Option( + ..., + "--project", + help="Project alias", + ), + table_id: str = typer.Option( + ..., + "--table-id", + help="Table ID (e.g. 'in.c-my-bucket.my-table')", + ), + text: str | None = typer.Option( + None, + "--text", + help="Description text (inline)", + ), + file: Path | None = typer.Option( + None, + "--file", + help="Path to a file containing the description", + ), + stdin: bool = typer.Option( + False, + "--stdin", + help="Read description from standard input", + ), + branch: int | None = typer.Option( + None, + "--branch", + help="Dev branch ID (defaults to active branch if set via 'branch use')", + ), + ) -> None: + """Set the description on a storage table. + + Stores the description as KBC.description in table metadata (upsert). + Provide the text via --text, --file, or --stdin (exactly one required). + """ + formatter = get_formatter(ctx) + service = get_service(ctx, "storage_service") + config_store: ConfigStore = ctx.obj["config_store"] + _, effective_branch = resolve_branch(config_store, formatter, project, branch) + + from ._metadata_input import resolve_text_input + + try: + description = resolve_text_input(text=text, file=file, stdin=stdin) + except ConfigError as exc: + formatter.error(message=exc.message, error_code=ErrorCode.INVALID_ARGUMENT) + raise typer.Exit(code=2) from None + + try: + result = service.describe_table( + alias=project, + table_id=table_id, + description=description, + branch_id=effective_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, retryable=exc.retryable) + raise typer.Exit(code=map_error_to_exit_code(exc)) from None + + if formatter.json_mode: + formatter.output(result) + else: + formatter.console.print(f"[bold green]Description set:[/bold green] {table_id}") + formatter.console.print(f" {escape(description[:120])}") + + @app.command("describe-column", rich_help_panel=_DESCRIBE) + def storage_describe_column( + ctx: typer.Context, + project: str = typer.Option( + ..., + "--project", + help="Project alias", + ), + table_id: str = typer.Option( + ..., + "--table-id", + help="Table ID (e.g. 'in.c-my-bucket.my-table')", + ), + column: list[str] = typer.Option( + ..., + "--column", + help="Column description as 'NAME=DESCRIPTION' (can be repeated)", + ), + branch: int | None = typer.Option( + None, + "--branch", + help="Dev branch ID (defaults to active branch if set via 'branch use')", + ), + ) -> None: + """Set descriptions on one or more columns of a storage table. + + Writes through the native table-definition endpoint -- the same one the + Keboola UI uses -- so the text is visible in the UI, to the MCP server, + and as the backend column comment (Snowflake COMMENT / BigQuery + description). The write also marks the descriptions as user-authored so + the next component run's Output Mapping cannot overwrite them. + + Unknown column names are rejected before anything is written. Legacy + KBC.column.*.description metadata keys found on the same table (written by + kbagent before 0.88.0) are migrated in the same write and removed; see + 'storage describe-migrate' to convert a whole bucket or project. + + Example: + + kbagent storage describe-column \\ + --project myproj \\ + --table-id in.c-bucket.orders \\ + --column order_id="Unique order identifier" \\ + --column total="Order total in USD" + """ + formatter = get_formatter(ctx) + service = get_service(ctx, "storage_service") + config_store: ConfigStore = ctx.obj["config_store"] + _, effective_branch = resolve_branch(config_store, formatter, project, branch) + + parsed: dict[str, str] = {} + for entry in column: + if "=" not in entry: + formatter.error( + message=f"--column must be NAME=DESCRIPTION, got: {entry!r}", + error_code=ErrorCode.INVALID_ARGUMENT, + ) + raise typer.Exit(code=2) from None + name, _, desc = entry.partition("=") + name = name.strip() + if not name: + formatter.error( + message=f"Column name cannot be empty in: {entry!r}", + error_code=ErrorCode.INVALID_ARGUMENT, + ) + raise typer.Exit(code=2) from None + parsed[name] = desc + + try: + result = service.describe_columns( + alias=project, + table_id=table_id, + columns=parsed, + branch_id=effective_branch, + ) + except ValueError as exc: + formatter.error(message=str(exc), error_code=ErrorCode.INVALID_ARGUMENT) + raise typer.Exit(code=2) from None + 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, retryable=exc.retryable) + raise typer.Exit(code=map_error_to_exit_code(exc)) from None + + if formatter.json_mode: + formatter.output(result) + else: + formatter.console.print( + f"[bold green]Column descriptions set:[/bold green] {table_id} " + f"({len(parsed)} column(s))" + ) + for name, desc in parsed.items(): + formatter.console.print(f" {name}: {escape(desc[:80])}") + migrated = result.get("migrated") or {} + if migrated: + formatter.console.print( + f" [dim]Migrated {len(migrated)} legacy column description(s): " + f"{', '.join(sorted(migrated))}[/dim]" + ) + for item in result.get("skipped") or []: + formatter.console.print( + f" [yellow]Skipped[/yellow] {item['column']} ({item['reason']})" + ) + + @app.command("describe-batch", rich_help_panel=_DESCRIBE) + def storage_describe_batch( + ctx: typer.Context, + project: str = typer.Option( + ..., + "--project", + help="Project alias", + ), + from_file: Path = typer.Option( + ..., + "--from-file", + help="Path to a YAML file with bucket/table/column descriptions", + ), + branch: int | None = typer.Option( + None, + "--branch", + help="Dev branch ID (defaults to active branch if set via 'branch use')", + ), + ) -> None: + """Apply descriptions to buckets, tables, and columns from a YAML file. + + YAML schema: + + buckets: + in.c-my-bucket: "Bucket description" + + tables: + in.c-my-bucket.my-table: "Table description" + + columns: + in.c-my-bucket.my-table: + col1: "Column 1 description" + col2: "Column 2 description" + + All sections are optional. A failure in one item does not abort the + rest -- all results are collected and reported. + """ + formatter = get_formatter(ctx) + service = get_service(ctx, "storage_service") + config_store: ConfigStore = ctx.obj["config_store"] + _, effective_branch = resolve_branch(config_store, formatter, project, branch) + + # In human mode, show a live progress indicator so that large batches + # (100+ items) do not look frozen. JSON mode must remain silent on stderr + # so structured output is the only thing on stdout. + progress_cm: Any = None + progress_task: Any = None + progress_callback = None + if not formatter.json_mode: + from rich.progress import ( + BarColumn, + MofNCompleteColumn, + Progress, + SpinnerColumn, + TextColumn, + TimeElapsedColumn, + ) + + progress_cm = Progress( + SpinnerColumn(), + TextColumn("[progress.description]{task.description}"), + BarColumn(), + MofNCompleteColumn(), + TextColumn("•"), + TimeElapsedColumn(), + console=formatter.console, + transient=True, + ) + + def _on_item(obj_type: str, obj_id: str, current: int, total: int) -> None: + # Guard against progress_task/progress_cm not being ready yet. + if progress_task is None or progress_cm is None: + return + # total is known up-front (passed the first time), but re-setting + # is a no-op after the first call. + progress_cm.update( + progress_task, + total=total, + completed=max(current - 1, 0), + description=f"Describing {obj_type} {obj_id}", + ) + + progress_callback = _on_item + + try: + if progress_cm is not None: + progress_cm.start() + progress_task = progress_cm.add_task("Applying descriptions...", total=None) + result = service.describe_batch( + alias=project, + from_file=from_file, + branch_id=effective_branch, + progress_callback=progress_callback, + ) + if progress_cm is not None and progress_task is not None: + # Mark the task complete so the final render shows N / N. + progress_cm.update( + progress_task, + completed=result["applied_count"] + result["error_count"], + ) + except ValueError as exc: + formatter.error(message=str(exc), error_code=ErrorCode.INVALID_ARGUMENT) + raise typer.Exit(code=2) from None + 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, retryable=exc.retryable) + raise typer.Exit(code=map_error_to_exit_code(exc)) from None + finally: + if progress_cm is not None: + # .stop() is idempotent; safe for both happy and error paths. + progress_cm.stop() + + if formatter.json_mode: + formatter.output(result) + else: + applied = result["applied_count"] + errors = result["error_count"] + formatter.console.print( + f"[bold green]Batch complete:[/bold green] {applied} applied, {errors} error(s)" + ) + for item in result["applied"]: + obj_type = item["type"] + obj_id = item["id"] + if obj_type == "columns": + n = len(item.get("columns", {})) + formatter.console.print(f" [green]✓[/green] {obj_type} {obj_id} ({n} cols)") + else: + formatter.console.print(f" [green]✓[/green] {obj_type} {obj_id}") + for item in result["errors"]: + formatter.console.print( + f" [red]✗[/red] {item['type']} {item['id']}: {item['error']}" + ) + if errors: + raise typer.Exit(code=1) from None + + @app.command("describe-migrate", rich_help_panel=_DESCRIBE) + def storage_describe_migrate( + ctx: typer.Context, + project: str = typer.Option( + ..., + "--project", + help="Project alias", + ), + table_id: list[str] | None = typer.Option( + None, + "--table-id", + help="Migrate only this table (can be repeated). Excludes --bucket-id.", + ), + bucket_id: str | None = typer.Option( + None, + "--bucket-id", + help="Migrate every table of this bucket. Excludes --table-id.", + ), + prune_orphans: bool = typer.Option( + False, + "--prune-orphans", + help="Also delete legacy entries for columns that no longer exist", + ), + dry_run: bool = typer.Option( + False, + "--dry-run", + help="Show what would be migrated without writing", + ), + yes: bool = typer.Option( + False, + "--yes", + "-y", + help="Skip confirmation prompt", + ), + branch: int | None = typer.Option( + None, + "--branch", + help="Dev branch ID (defaults to active branch if set via 'branch use')", + ), + ) -> None: + """Convert legacy KBC.column.* descriptions to the native definition endpoint. + + Before 0.88.0 kbagent stored column descriptions as flat + KBC.column.{name}.description table-metadata keys, which nothing but + kbagent itself ever read. This command rewrites them through the native + endpoint (visible in the Keboola UI, to the MCP server, and as backend + column comments) and removes the flat keys afterwards. + + Scope defaults to the whole project; narrow it with --table-id (repeatable) + or --bucket-id. A column that already shows a different description keeps + it (reported as a conflict), and a key whose column no longer exists is + reported as an orphan -- deleted only with --prune-orphans. + """ + formatter = get_formatter(ctx) + service = get_service(ctx, "storage_service") + config_store: ConfigStore = ctx.obj["config_store"] + _, effective_branch = resolve_branch(config_store, formatter, project, branch) + + if table_id and bucket_id: + formatter.error( + message="--table-id and --bucket-id are mutually exclusive.", + error_code=ErrorCode.INVALID_ARGUMENT, + ) + raise typer.Exit(code=2) from None + + def _migrate(scan_only: bool) -> dict[str, Any]: + try: + return service.describe_migrate( + alias=project, + table_ids=list(table_id) if table_id else None, + bucket_id=bucket_id, + prune_orphans=prune_orphans, + dry_run=scan_only, + branch_id=effective_branch, + ) + except ValueError as exc: + formatter.error(message=str(exc), error_code=ErrorCode.INVALID_ARGUMENT) + raise typer.Exit(code=2) from None + 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, retryable=exc.retryable + ) + raise typer.Exit(code=map_error_to_exit_code(exc)) from None + + if not dry_run and not yes and not formatter.json_mode: + # Scan first so the confirmation states what is actually at stake -- + # the repo's dry-run-then-confirm pattern for bulk write operations. + scan = _migrate(True) + column_count = sum(len(item["columns"]) for item in scan["migrated"]) + formatter.console.print( + f"[bold]Scan:[/bold] {len(scan['migrated'])} table(s) with " + f"{column_count} legacy column description(s) " + f"of {scan['tables_scanned']} table(s) scanned." + ) + if not typer.confirm( + f"Migrate {len(scan['migrated'])} table(s) in project '{project}'?" + ): + formatter.console.print("Aborted.") + raise typer.Exit(code=0) + + result = _migrate(dry_run) + + if formatter.json_mode: + formatter.output(result) + else: + verb = "Would migrate" if result["dry_run"] else "Migrated" + formatter.console.print( + f"[bold green]{verb}:[/bold green] {len(result['migrated'])} table(s) of " + f"{result['tables_scanned']} scanned" + ) + for item in result["migrated"]: + cols = ", ".join(sorted(item["columns"])) + formatter.console.print(f" [green]✓[/green] {item['table_id']}: {escape(cols)}") + for item in result["skipped"]: + formatter.console.print( + f" [yellow]Skipped[/yellow] {item['table_id']}.{item['column']} ({item['reason']})" + ) + for item in result["pruned_orphans"]: + formatter.console.print( + f" [dim]Pruned orphan {item['table_id']}.{item['column']}[/dim]" + ) + for item in result["errors"]: + formatter.console.print(f" [red]✗[/red] {item['table_id']}: {item['error']}") + + if result["errors"]: + raise typer.Exit(code=1) from None diff --git a/src/keboola_agent_cli/services/_column_descriptions.py b/src/keboola_agent_cli/services/_column_descriptions.py new file mode 100644 index 00000000..652ab098 --- /dev/null +++ b/src/keboola_agent_cli/services/_column_descriptions.py @@ -0,0 +1,503 @@ +"""Column-description helpers shared by the storage service (issue #624). + +Everything here is about ONE question: where does a column description live? +Three conventions coexist, and this module is the single place that knows their +precedence -- native ``definition`` field (what the Keboola UI writes and shows) +first, then the ``KBC.description`` entry the backend mirrors it into (the only +one the MCP server reads), then the flat ``KBC.column.{name}.description`` table +metadata key kbagent wrote before 0.88.0 and nothing else ever read. + +Lives in a private module because ``services/storage_service.py`` is already +past its CONTRIBUTING.md size ceiling; the functions take an explicit client so +they stay free of service state. +""" + +import logging +from collections.abc import Callable +from typing import Any + +from ..errors import KeboolaApiError +from .base import BaseService + +logger = logging.getLogger(__name__) + +# Legacy pre-0.88.0 convention: flat table-metadata keys carrying per-column +# descriptions. Read nowhere except this CLI -- neither the Keboola UI nor the +# MCP server ever looked at them. Superseded by the native table-definition +# endpoint (#624); kept only so the read fallback and `describe-migrate` can +# still find and convert old entries. +LEGACY_COLUMN_KEY_PREFIX = "KBC.column." +LEGACY_COLUMN_KEY_SUFFIX = ".description" + +# Key the backend mirrors a native column description into. Written by the +# platform, not by us; the single place UI/MCP/kbagent all agree on for reads. +COLUMN_DESCRIPTION_METADATA_KEY = "KBC.description" + + +def collect_legacy_column_entries( + raw_metadata: list[dict[str, Any]], +) -> dict[str, dict[str, Any]]: + """Map column name -> full metadata entry for flat KBC.column.*.description keys.""" + out: dict[str, dict[str, Any]] = {} + for m in raw_metadata: + key = m.get("key", "") + if key.startswith(LEGACY_COLUMN_KEY_PREFIX) and key.endswith(LEGACY_COLUMN_KEY_SUFFIX): + col = key[len(LEGACY_COLUMN_KEY_PREFIX) : -len(LEGACY_COLUMN_KEY_SUFFIX)] + out[col] = m + return out + + +def description_from_column_meta(meta: list[dict[str, Any]]) -> str | None: + """Return the ``KBC.description`` value from one column's metadata list.""" + for m in meta: + if m.get("key") == COLUMN_DESCRIPTION_METADATA_KEY: + return m.get("value") or None + return None + + +def native_column_descriptions(table: dict[str, Any]) -> dict[str, str]: + """Map column name -> description from the native ``definition`` block. + + Alias tables carry no definition of their own; they inherit the source + table's, which is where the UI reads them from too. + """ + definition = table.get("definition") or {} + if not definition and table.get("isAlias"): + definition = (table.get("sourceTable") or {}).get("definition") or {} + out: dict[str, str] = {} + for col in definition.get("columns") or []: + name = col.get("name") + desc = (col.get("definition") or {}).get("description") + if name and desc: + out[name] = desc + return out + + +def visible_column_descriptions(table: dict[str, Any]) -> dict[str, str]: + """Resolve the description each column shows today (native, then metadata). + + Precedence mirrors the read path in ``get_table_detail`` minus the legacy + tier -- it answers "would migrating this legacy value overwrite something + a user can already see?". + """ + resolved = dict(native_column_descriptions(table)) + column_metadata: dict[str, list[dict[str, Any]]] = table.get("columnMetadata") or {} + source_column_metadata: dict[str, list[dict[str, Any]]] = ( + (table.get("sourceTable") or {}).get("columnMetadata") or {} if table.get("isAlias") else {} + ) + for col in table.get("columns") or []: + if col in resolved: + continue + meta_desc = description_from_column_meta(column_metadata.get(col, [])) + if meta_desc is None: + meta_desc = description_from_column_meta(source_column_metadata.get(col, [])) + if meta_desc: + resolved[col] = meta_desc + return resolved + + +def plan_column_migration( + table: dict[str, Any], +) -> tuple[dict[str, str], list[dict[str, Any]], list[dict[str, Any]]]: + """Compute (migratable {col: desc}, skipped entries, deletable metadata entries). + + Rules (#624): + - orphan (column no longer on the table) -> skip, reason "orphan", entry NOT deleted + - target already has a description (native definition or columnMetadata + KBC.description) that differs -> skip, reason "conflict", NOT deleted + - target has the IDENTICAL description -> nothing to write, entry IS deleted + - otherwise -> migrate, entry deleted after the write + """ + legacy_entries = collect_legacy_column_entries(table.get("metadata") or []) + table_columns = set(table.get("columns") or []) + visible = visible_column_descriptions(table) + + migratable: dict[str, str] = {} + skipped: list[dict[str, Any]] = [] + deletable: list[dict[str, Any]] = [] + for col, entry in legacy_entries.items(): + legacy_value = entry.get("value") or "" + if col not in table_columns: + skipped.append({"column": col, "reason": "orphan", "legacy": legacy_value}) + continue + current = visible.get(col) + if current == legacy_value: + # Already mirrored where everyone reads it -- drop the stale copy so + # a later "clear the description" does not get resurrected from it. + deletable.append(entry) + continue + if current: + skipped.append( + { + "column": col, + "reason": "conflict", + "legacy": legacy_value, + "current": current, + } + ) + continue + migratable[col] = legacy_value + deletable.append(entry) + return migratable, skipped, deletable + + +def delete_legacy_entries( + client: Any, + table_id: str, + entries: list[dict[str, Any]], + branch_id: int | None = None, +) -> list[dict[str, Any]]: + """Delete migrated legacy metadata entries; report failures, never raise. + + The native write is already durable by the time this runs -- a failed + cleanup leaves a stale flat key behind (harmless, re-migratable) and must + not turn a successful describe into a failed command. + """ + failures: list[dict[str, Any]] = [] + for entry in entries: + key = entry.get("key", "") + column = key[len(LEGACY_COLUMN_KEY_PREFIX) : -len(LEGACY_COLUMN_KEY_SUFFIX)] + try: + client.delete_table_metadata(table_id, entry.get("id"), branch_id=branch_id) + except Exception as exc: + msg = exc.message if isinstance(exc, KeboolaApiError) else str(exc) + logger.warning("Could not delete legacy metadata %s on %s: %s", key, table_id, msg) + failures.append({"column": column, "reason": "delete_failed", "error": msg}) + return failures + + +def write_column_descriptions( + client: Any, + table_id: str, + columns: dict[str, str], + branch_id: int | None = None, +) -> dict[str, Any]: + """Write ``columns`` through the native endpoint, migrating legacy siblings. + + Returns ``{"migrated": {col: desc}, "skipped": [...], "result": job}``. + Raises ``ValueError`` for column names the table does not have -- before + any write happens. + """ + table = client.get_table_detail(table_id, branch_id=branch_id) + + # Fail fast: the native endpoint rejects unknown columns anyway, and the old + # flat write silently accepted typos that nothing ever read. + table_columns = set(table.get("columns") or []) + unknown = [c for c in columns if c not in table_columns] + if unknown: + raise ValueError( + f"Unknown column(s) on table '{table_id}': {', '.join(sorted(unknown))}. " + f"Available: {', '.join(sorted(table_columns))}" + ) + + migratable, skipped, deletable = plan_column_migration(table) + legacy_entries = collect_legacy_column_entries(table.get("metadata") or []) + # The user's value always wins over a legacy one for the same column; the + # legacy entry is still cleaned up because the new write supersedes it. + deletable_ids = {str(entry.get("id")) for entry in deletable} + for col in columns: + migratable.pop(col, None) + skipped = [s for s in skipped if s["column"] != col] + entry = legacy_entries.get(col) + if entry is not None and str(entry.get("id")) not in deletable_ids: + deletable.append(entry) + deletable_ids.add(str(entry.get("id"))) + + payload = {**migratable, **columns} + result = client.update_table_definition( + table_id=table_id, + columns=[{"name": name, "description": desc} for name, desc in payload.items()], + is_description_system_managed=False, + branch_id=branch_id, + ) + skipped.extend(delete_legacy_entries(client, table_id, deletable, branch_id=branch_id)) + return {"migrated": migratable, "skipped": skipped, "result": result} + + +def migrate_candidates( + client: Any, + table_ids: list[str] | None, + bucket_id: str | None, + branch_id: int | None, +) -> tuple[list[str], set[str] | None]: + """Resolve the tables in scope plus (when known) those carrying legacy keys. + + The listing is fetched with ``include="metadata"`` so whole-project runs + only fetch table details for the few tables that actually have something to + migrate. The second element is ``None`` when that pre-filter is unavailable + (explicit ids, or a listing that carried no metadata). + """ + if table_ids: + return list(table_ids), None + + listing = client.list_tables(bucket_id=bucket_id, branch_id=branch_id, include="metadata") + rows = [ + row + for row in listing + if not bucket_id or str(row.get("id", "")).startswith(f"{bucket_id}.") + ] + candidates = [str(row.get("id", "")) for row in rows] + if any("metadata" not in row for row in rows): + return candidates, None + with_legacy = { + str(row.get("id", "")) + for row in rows + if collect_legacy_column_entries(row.get("metadata") or []) + } + return candidates, with_legacy + + +def migrate_one_table( + client: Any, + table_id: str, + prune_orphans: bool, + dry_run: bool, + branch_id: int | None, + migrated: list[dict[str, Any]], + skipped: list[dict[str, Any]], + pruned: list[dict[str, Any]], +) -> bool: + """Migrate one table's legacy keys; append findings to the shared lists. + + Findings are committed only once the table is through -- a table whose + write raises is reported purely as an error by the caller, never as + half-migrated. + + Returns True when a write actually happened (always False in dry-run). + """ + table = client.get_table_detail(table_id, branch_id=branch_id) + migratable, table_skipped, deletable = plan_column_migration(table) + legacy_entries = collect_legacy_column_entries(table.get("metadata") or []) + + if dry_run: + for item in table_skipped: + skipped.append({"table_id": table_id, **item}) + if migratable: + migrated.append({"table_id": table_id, "columns": migratable}) + return False + + wrote = False + if migratable: + client.update_table_definition( + table_id=table_id, + columns=[{"name": name, "description": desc} for name, desc in migratable.items()], + is_description_system_managed=False, + branch_id=branch_id, + ) + wrote = True + migrated.append({"table_id": table_id, "columns": migratable}) + for item in table_skipped: + skipped.append({"table_id": table_id, **item}) + for failure in delete_legacy_entries(client, table_id, deletable, branch_id=branch_id): + skipped.append({"table_id": table_id, **failure}) + if deletable: + wrote = True + + if prune_orphans: + orphans = [item["column"] for item in table_skipped if item["reason"] == "orphan"] + entries = [legacy_entries[col] for col in orphans if col in legacy_entries] + prune_failures = delete_legacy_entries(client, table_id, entries, branch_id=branch_id) + failed = {failure["column"] for failure in prune_failures} + for failure in prune_failures: + skipped.append({"table_id": table_id, **failure}) + for col in orphans: + if col not in failed: + pruned.append({"table_id": table_id, "column": col}) + if entries: + wrote = True + return wrote + + +def migrate_tables( + client: Any, + table_ids: list[str] | None, + bucket_id: str | None, + prune_orphans: bool, + dry_run: bool, + branch_id: int | None, + progress_callback: Callable[[str, int, int], None] | None = None, +) -> dict[str, Any]: + """Run the migration over every table in scope, accumulating per-table errors. + + Sequential on purpose -- each write is one storage job and a migration is a + one-off maintenance task. Returns the raw counters/lists; the service wraps + them in the response envelope. + """ + migrated: list[dict[str, Any]] = [] + skipped: list[dict[str, Any]] = [] + pruned: list[dict[str, Any]] = [] + errors: list[dict[str, Any]] = [] + tables_migrated = 0 + + candidates, with_legacy = migrate_candidates(client, table_ids, bucket_id, branch_id) + total = len(candidates) + for index, table_id in enumerate(candidates, start=1): + if progress_callback is not None: + progress_callback(table_id, index, total) + # Tables the listing already proved carry no legacy key need no detail fetch. + if with_legacy is not None and table_id not in with_legacy: + continue + try: + changed = migrate_one_table( + client, + table_id, + prune_orphans=prune_orphans, + dry_run=dry_run, + branch_id=branch_id, + migrated=migrated, + skipped=skipped, + pruned=pruned, + ) + except Exception as exc: + msg = exc.message if isinstance(exc, KeboolaApiError) else str(exc) + errors.append({"table_id": table_id, "error": msg}) + continue + if changed: + tables_migrated += 1 + + return { + "tables_scanned": total, + "tables_migrated": tables_migrated, + "migrated": migrated, + "skipped": skipped, + "pruned_orphans": pruned, + "errors": errors, + } + + +class ColumnDescriptionsMixin(BaseService): + """The ``describe-column`` / ``describe-migrate`` half of ``StorageService``. + + Split off so ``storage_service.py`` stays within its file-size budget; the + methods are plain service methods (project resolution + client lifecycle + + response envelope) delegating the actual rules to this module's functions. + """ + + def describe_columns( + self, + alias: str, + table_id: str, + columns: dict[str, str], + branch_id: int | None = None, + ) -> dict[str, Any]: + """Set per-column descriptions on a storage table (native endpoint). + + Writes through ``PUT /v2/storage/branch/{branch}/tables/{id}/definition`` + -- the same call the Keboola UI makes. The backend mirrors each value + into ``columnMetadata[{col}]`` ``KBC.description`` (typed and untyped + tables alike) and down to the backend column comment, so one write is + visible to the UI, the MCP server, and Snowflake/BigQuery. The write + sets ``isDescriptionSystemManaged=false``, which is what stops the next + component run's Output Mapping from overwriting the text. + + Any sibling legacy ``KBC.column.{name}.description`` metadata key found + on the table (the pre-0.88.0 convention, invisible to everything but + this CLI) is migrated in the SAME write and then deleted -- unless the + column is gone (orphan) or already shows a different description + (conflict); those are reported in ``skipped`` and left untouched. + + Args: + alias: Project alias. + table_id: Full table ID. + columns: Mapping of column name -> description text. + branch_id: If set, target a specific dev branch. + + Returns: + Dict with project_alias, table_id, columns, migrated, skipped, + result (the completed storage job), message. + """ + if not columns: + raise ValueError("At least one column description must be provided.") + projects = self.resolve_projects([alias]) + project = projects[alias] + client = self._client_factory(project.stack_url, project.token) + try: + outcome = write_column_descriptions(client, table_id, columns, branch_id=branch_id) + finally: + client.close() + + migrated = outcome["migrated"] + message = f"Described {len(columns)} column(s) on '{table_id}' in project '{alias}'." + if migrated: + message += f" Migrated {len(migrated)} legacy entry(ies)." + return { + "project_alias": alias, + "table_id": table_id, + "columns": columns, + "migrated": migrated, + "skipped": outcome["skipped"], + "result": outcome["result"], + "message": message, + } + + def describe_migrate( + self, + alias: str, + table_ids: list[str] | None = None, + bucket_id: str | None = None, + prune_orphans: bool = False, + dry_run: bool = False, + branch_id: int | None = None, + progress_callback: Callable[[str, int, int], None] | None = None, + ) -> dict[str, Any]: + """Migrate legacy flat KBC.column.* descriptions to the native endpoint. + + Scope: explicit ``table_ids``, else all tables of ``bucket_id``, else + every table in the project. Tables without legacy keys are skipped + silently (still counted in ``tables_scanned``). Per-table failures are + collected and never abort the run (error-accumulation convention #11). + + Migration rules are the ones ``describe_columns`` applies opportunistically + (see ``_plan_column_migration``): conflicts and orphans are reported, not + overwritten. ``prune_orphans`` additionally deletes the dangling legacy + entries of columns that no longer exist. + + Tables are processed sequentially -- each write is one storage job, and a + migration is a one-off maintenance task. Parallelizing per table is + possible future work if this ever runs on very large projects. + + Args: + alias: Project alias. + table_ids: Explicit tables to migrate (mutually exclusive with bucket_id). + bucket_id: Migrate every table of this bucket. + prune_orphans: Also delete legacy entries for dropped columns. + dry_run: Report what would happen; performs no writes at all. + branch_id: If set, target a specific dev branch. + progress_callback: Optional ``(table_id, current, total)`` callable + invoked **before** each table is processed (1-based ``current``). + + Returns: + Dict with project_alias, dry_run, tables_scanned, tables_migrated, + migrated, skipped, pruned_orphans, errors, message. + """ + if table_ids and bucket_id: + raise ValueError("--table-id and --bucket-id are mutually exclusive.") + + projects = self.resolve_projects([alias]) + project = projects[alias] + client = self._client_factory(project.stack_url, project.token) + try: + outcome = migrate_tables( + client, + table_ids, + bucket_id, + prune_orphans=prune_orphans, + dry_run=dry_run, + branch_id=branch_id, + progress_callback=progress_callback, + ) + finally: + client.close() + + verb = "Would migrate" if dry_run else "Migrated" + return { + "project_alias": alias, + "dry_run": dry_run, + **outcome, + "message": ( + f"{verb} {len(outcome['migrated'])} table(s) of " + f"{outcome['tables_scanned']} scanned in project '{alias}'; " + f"{len(outcome['skipped'])} column(s) skipped, " + f"{len(outcome['errors'])} error(s)." + ), + } diff --git a/tests/test_storage_tables.py b/tests/test_storage_tables.py index 798d7004..cdf0f612 100644 --- a/tests/test_storage_tables.py +++ b/tests/test_storage_tables.py @@ -1,6 +1,7 @@ """Tests for storage tables multi-project listing (issue #198). Covers: +- KeboolaClient.update_table_definition() / delete_table_metadata() (issue #624) - StorageService.list_tables() multi-project parallel execution - CLI storage tables without --project (all projects) - CLI storage tables with multiple --project flags @@ -17,8 +18,9 @@ from typer.testing import CliRunner from keboola_agent_cli.cli import app +from keboola_agent_cli.client import KeboolaClient from keboola_agent_cli.config_store import ConfigStore -from keboola_agent_cli.errors import ConfigError, KeboolaApiError +from keboola_agent_cli.errors import ConfigError, ErrorCode, KeboolaApiError from keboola_agent_cli.models import AppConfig, ProjectConfig from keboola_agent_cli.services.storage_service import StorageService @@ -460,3 +462,153 @@ def test_unknown_project_exits_with_config_error(self, tmp_path: Path) -> None: ) assert result.exit_code == 5 + + +# ------------------------------------------------------------------ +# Client-layer tests: native column descriptions (issue #624) +# ------------------------------------------------------------------ + + +def _make_client() -> KeboolaClient: + return KeboolaClient(stack_url="https://connection.keboola.com", token=TEST_TOKEN) + + +class TestUpdateTableDefinitionClient: + """KeboolaClient.update_table_definition() - HTTP layer.""" + + def test_sends_native_payload(self, httpx_mock) -> None: + """PUTs the native definition body to the production 'default' branch ref.""" + httpx_mock.add_response( + url="https://connection.keboola.com/v2/storage/branch/default/tables/in.c-b.t/definition", + method="PUT", + json={"id": 1, "status": "success", "operationName": "tableDefinitionUpdate"}, + status_code=200, + ) + + client = _make_client() + result = client.update_table_definition( + table_id="in.c-b.t", + columns=[{"name": "c1", "description": "d"}], + is_description_system_managed=False, + ) + + assert result["status"] == "success" + body = json.loads(httpx_mock.get_request().content.decode("utf-8")) + assert body == { + "columns": [{"name": "c1", "description": "d"}], + "isDescriptionSystemManaged": False, + } + client.close() + + def test_branch_scoped_url(self, httpx_mock) -> None: + """A numeric branch_id replaces the literal 'default' branch ref.""" + httpx_mock.add_response( + url="https://connection.keboola.com/v2/storage/branch/123/tables/in.c-b.t/definition", + method="PUT", + json={"id": 2, "status": "success"}, + status_code=200, + ) + + client = _make_client() + client.update_table_definition( + table_id="in.c-b.t", + columns=[{"name": "c1", "description": "d"}], + branch_id=123, + ) + + assert "/v2/storage/branch/123/tables/in.c-b.t/definition" in str( + httpx_mock.get_request().url + ) + client.close() + + def test_description_set_none_clears(self, httpx_mock) -> None: + """description_set=True sends an explicit null; the flag is what clears.""" + httpx_mock.add_response( + url="https://connection.keboola.com/v2/storage/branch/default/tables/in.c-b.t/definition", + method="PUT", + json={"id": 3, "status": "success"}, + status_code=200, + ) + + client = _make_client() + client.update_table_definition( + table_id="in.c-b.t", + description=None, + description_set=True, + ) + + body = json.loads(httpx_mock.get_request().content.decode("utf-8")) + assert body == {"description": None} + client.close() + + def test_description_omitted_without_flag(self, httpx_mock) -> None: + """description_set=False leaves the table description untouched (key absent).""" + httpx_mock.add_response( + url="https://connection.keboola.com/v2/storage/branch/default/tables/in.c-b.t/definition", + method="PUT", + json={"id": 4, "status": "success"}, + status_code=200, + ) + + client = _make_client() + client.update_table_definition( + table_id="in.c-b.t", + columns=[{"name": "c1", "description": None}], + ) + + body = json.loads(httpx_mock.get_request().content.decode("utf-8")) + assert body == {"columns": [{"name": "c1", "description": None}]} + assert "description" not in body + assert "isDescriptionSystemManaged" not in body + client.close() + + def test_job_error_raises(self, httpx_mock) -> None: + """A failed storage job surfaces as STORAGE_JOB_FAILED, not a silent success.""" + httpx_mock.add_response( + url="https://connection.keboola.com/v2/storage/branch/default/tables/in.c-b.t/definition", + method="PUT", + json={"id": 5, "status": "error", "error": {"message": "boom"}}, + status_code=200, + ) + + client = _make_client() + with pytest.raises(KeboolaApiError) as exc_info: + client.update_table_definition( + table_id="in.c-b.t", + columns=[{"name": "c1", "description": "d"}], + ) + + assert exc_info.value.error_code == ErrorCode.STORAGE_JOB_FAILED + client.close() + + +class TestDeleteTableMetadataClient: + """KeboolaClient.delete_table_metadata() - HTTP layer.""" + + def test_production_url(self, httpx_mock) -> None: + """Production path has no branch segment and the call returns None.""" + httpx_mock.add_response( + url="https://connection.keboola.com/v2/storage/tables/in.c-b.t/metadata/42", + method="DELETE", + status_code=204, + ) + + client = _make_client() + assert client.delete_table_metadata(table_id="in.c-b.t", metadata_id=42) is None + client.close() + + def test_branch_url(self, httpx_mock) -> None: + """branch_id switches to the branch-prefixed metadata path.""" + httpx_mock.add_response( + url="https://connection.keboola.com/v2/storage/branch/5/tables/in.c-b.t/metadata/42", + method="DELETE", + status_code=204, + ) + + client = _make_client() + client.delete_table_metadata(table_id="in.c-b.t", metadata_id=42, branch_id=5) + + assert "/v2/storage/branch/5/tables/in.c-b.t/metadata/42" in str( + httpx_mock.get_request().url + ) + client.close() From d1c8a5f45a755b3d700d924a20134f8784575b33 Mon Sep 17 00:00:00 2001 From: Petr Date: Fri, 21 Aug 2026 23:27:10 +0200 Subject: [PATCH 2/4] feat(storage): native column descriptions, describe-migrate, table-detail fallback (#624) describe-column/describe-batch write through the native definition endpoint with isDescriptionSystemManaged=false, fail fast on unknown columns, and auto-migrate remaining legacy flat entries on the touched table (conflict -> skip, orphan -> skip, migrated entries deleted so a later clear cannot be resurrected by the read fallback). New storage describe-migrate command converts legacy entries in bulk (dry-run-then-confirm, per-table error accumulation, --prune-orphans). table-detail resolves descriptions native definition -> columnMetadata KBC.description (alias source fallback) -> legacy flat key and reports legacy_column_descriptions with a human-mode warning; reads never write. Descriptions panel moved to commands/_storage_describe.py and the service logic to services/_column_descriptions.py to hold the file-size budgets (snapshot-module precedent). --- src/keboola_agent_cli/commands/storage.py | 398 +------------ src/keboola_agent_cli/permissions.py | 1 + .../services/_table_detail.py | 72 ++- .../services/storage_service.py | 54 +- tests/test_e2e.py | 69 +++ tests/test_storage_describe_cli.py | 302 ++++++++++ tests/test_storage_describe_service.py | 535 +++++++++++++++++- 7 files changed, 957 insertions(+), 474 deletions(-) diff --git a/src/keboola_agent_cli/commands/storage.py b/src/keboola_agent_cli/commands/storage.py index 1859263e..0176abb7 100644 --- a/src/keboola_agent_cli/commands/storage.py +++ b/src/keboola_agent_cli/commands/storage.py @@ -5,7 +5,6 @@ """ from pathlib import Path -from typing import Any import typer from rich.markup import escape @@ -423,6 +422,17 @@ def storage_table_detail( else: render_table_detail(formatter, result) + # Legacy flat KBC.column.*.description keys are readable here but + # invisible everywhere else (#624) -- point at the migration command. + # JSON mode needs no warning: the key itself is the signal. + legacy_columns = result.get("legacy_column_descriptions") or [] + if legacy_columns: + formatter.console.print( + f"\n[yellow]Warning:[/yellow] {len(legacy_columns)} column description(s) " + "use the legacy KBC.column.* convention -- invisible to the Keboola UI " + "and MCP server. Run 'kbagent storage describe-migrate' to convert." + ) + @storage_app.command("create-bucket", rich_help_panel=_BUCKETS) def storage_create_bucket( @@ -1664,385 +1674,6 @@ def storage_delete_bucket( raise typer.Exit(code=1) -# ------------------------------------------------------------------ -# Describe (metadata write) commands -# ------------------------------------------------------------------ - -_DESCRIBE = "Descriptions" - - -@storage_app.command("describe-bucket", rich_help_panel=_DESCRIBE) -def storage_describe_bucket( - ctx: typer.Context, - project: str = typer.Option( - ..., - "--project", - help="Project alias", - ), - bucket_id: str = typer.Option( - ..., - "--bucket-id", - help="Bucket ID (e.g. 'in.c-my-bucket')", - ), - text: str | None = typer.Option( - None, - "--text", - help="Description text (inline)", - ), - file: Path | None = typer.Option( - None, - "--file", - help="Path to a file containing the description", - ), - stdin: bool = typer.Option( - False, - "--stdin", - help="Read description from standard input", - ), - branch: int | None = typer.Option( - None, - "--branch", - help="Dev branch ID (defaults to active branch if set via 'branch use')", - ), -) -> None: - """Set the description on a storage bucket. - - Stores the description as KBC.description in bucket metadata (upsert). - Provide the text via --text, --file, or --stdin (exactly one required). - """ - formatter = get_formatter(ctx) - service = get_service(ctx, "storage_service") - config_store: ConfigStore = ctx.obj["config_store"] - _, effective_branch = resolve_branch(config_store, formatter, project, branch) - - from ._metadata_input import resolve_text_input - - try: - description = resolve_text_input(text=text, file=file, stdin=stdin) - except ConfigError as exc: - formatter.error(message=exc.message, error_code=ErrorCode.INVALID_ARGUMENT) - raise typer.Exit(code=2) from None - - try: - result = service.describe_bucket( - alias=project, - bucket_id=bucket_id, - description=description, - branch_id=effective_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, retryable=exc.retryable) - raise typer.Exit(code=map_error_to_exit_code(exc)) from None - - if formatter.json_mode: - formatter.output(result) - else: - formatter.console.print(f"[bold green]Description set:[/bold green] {bucket_id}") - formatter.console.print(f" {escape(description[:120])}") - - -@storage_app.command("describe-table", rich_help_panel=_DESCRIBE) -def storage_describe_table( - ctx: typer.Context, - project: str = typer.Option( - ..., - "--project", - help="Project alias", - ), - table_id: str = typer.Option( - ..., - "--table-id", - help="Table ID (e.g. 'in.c-my-bucket.my-table')", - ), - text: str | None = typer.Option( - None, - "--text", - help="Description text (inline)", - ), - file: Path | None = typer.Option( - None, - "--file", - help="Path to a file containing the description", - ), - stdin: bool = typer.Option( - False, - "--stdin", - help="Read description from standard input", - ), - branch: int | None = typer.Option( - None, - "--branch", - help="Dev branch ID (defaults to active branch if set via 'branch use')", - ), -) -> None: - """Set the description on a storage table. - - Stores the description as KBC.description in table metadata (upsert). - Provide the text via --text, --file, or --stdin (exactly one required). - """ - formatter = get_formatter(ctx) - service = get_service(ctx, "storage_service") - config_store: ConfigStore = ctx.obj["config_store"] - _, effective_branch = resolve_branch(config_store, formatter, project, branch) - - from ._metadata_input import resolve_text_input - - try: - description = resolve_text_input(text=text, file=file, stdin=stdin) - except ConfigError as exc: - formatter.error(message=exc.message, error_code=ErrorCode.INVALID_ARGUMENT) - raise typer.Exit(code=2) from None - - try: - result = service.describe_table( - alias=project, - table_id=table_id, - description=description, - branch_id=effective_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, retryable=exc.retryable) - raise typer.Exit(code=map_error_to_exit_code(exc)) from None - - if formatter.json_mode: - formatter.output(result) - else: - formatter.console.print(f"[bold green]Description set:[/bold green] {table_id}") - formatter.console.print(f" {escape(description[:120])}") - - -@storage_app.command("describe-column", rich_help_panel=_DESCRIBE) -def storage_describe_column( - ctx: typer.Context, - project: str = typer.Option( - ..., - "--project", - help="Project alias", - ), - table_id: str = typer.Option( - ..., - "--table-id", - help="Table ID (e.g. 'in.c-my-bucket.my-table')", - ), - column: list[str] = typer.Option( - ..., - "--column", - help="Column description as 'NAME=DESCRIPTION' (can be repeated)", - ), - branch: int | None = typer.Option( - None, - "--branch", - help="Dev branch ID (defaults to active branch if set via 'branch use')", - ), -) -> None: - """Set descriptions on one or more columns of a storage table. - - Descriptions are stored as KBC.column.{name}.description keys in table - metadata (upsert). Keboola Storage does not expose a user-writable - column-level metadata endpoint; this convention lets you annotate columns - and read them back via 'storage table-detail'. - - Example: - - kbagent storage describe-column \\ - --project myproj \\ - --table-id in.c-bucket.orders \\ - --column order_id="Unique order identifier" \\ - --column total="Order total in USD" - """ - formatter = get_formatter(ctx) - service = get_service(ctx, "storage_service") - config_store: ConfigStore = ctx.obj["config_store"] - _, effective_branch = resolve_branch(config_store, formatter, project, branch) - - parsed: dict[str, str] = {} - for entry in column: - if "=" not in entry: - formatter.error( - message=f"--column must be NAME=DESCRIPTION, got: {entry!r}", - error_code=ErrorCode.INVALID_ARGUMENT, - ) - raise typer.Exit(code=2) from None - name, _, desc = entry.partition("=") - name = name.strip() - if not name: - formatter.error( - message=f"Column name cannot be empty in: {entry!r}", - error_code=ErrorCode.INVALID_ARGUMENT, - ) - raise typer.Exit(code=2) from None - parsed[name] = desc - - try: - result = service.describe_columns( - alias=project, - table_id=table_id, - columns=parsed, - branch_id=effective_branch, - ) - except ValueError as exc: - formatter.error(message=str(exc), error_code=ErrorCode.INVALID_ARGUMENT) - raise typer.Exit(code=2) from None - 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, retryable=exc.retryable) - raise typer.Exit(code=map_error_to_exit_code(exc)) from None - - if formatter.json_mode: - formatter.output(result) - else: - formatter.console.print( - f"[bold green]Column descriptions set:[/bold green] {table_id} " - f"({len(parsed)} column(s))" - ) - for name, desc in parsed.items(): - formatter.console.print(f" {name}: {escape(desc[:80])}") - - -@storage_app.command("describe-batch", rich_help_panel=_DESCRIBE) -def storage_describe_batch( - ctx: typer.Context, - project: str = typer.Option( - ..., - "--project", - help="Project alias", - ), - from_file: Path = typer.Option( - ..., - "--from-file", - help="Path to a YAML file with bucket/table/column descriptions", - ), - branch: int | None = typer.Option( - None, - "--branch", - help="Dev branch ID (defaults to active branch if set via 'branch use')", - ), -) -> None: - """Apply descriptions to buckets, tables, and columns from a YAML file. - - YAML schema: - - buckets: - in.c-my-bucket: "Bucket description" - - tables: - in.c-my-bucket.my-table: "Table description" - - columns: - in.c-my-bucket.my-table: - col1: "Column 1 description" - col2: "Column 2 description" - - All sections are optional. A failure in one item does not abort the - rest -- all results are collected and reported. - """ - formatter = get_formatter(ctx) - service = get_service(ctx, "storage_service") - config_store: ConfigStore = ctx.obj["config_store"] - _, effective_branch = resolve_branch(config_store, formatter, project, branch) - - # In human mode, show a live progress indicator so that large batches - # (100+ items) do not look frozen. JSON mode must remain silent on stderr - # so structured output is the only thing on stdout. - progress_cm: Any = None - progress_task: Any = None - progress_callback = None - if not formatter.json_mode: - from rich.progress import ( - BarColumn, - MofNCompleteColumn, - Progress, - SpinnerColumn, - TextColumn, - TimeElapsedColumn, - ) - - progress_cm = Progress( - SpinnerColumn(), - TextColumn("[progress.description]{task.description}"), - BarColumn(), - MofNCompleteColumn(), - TextColumn("•"), - TimeElapsedColumn(), - console=formatter.console, - transient=True, - ) - - def _on_item(obj_type: str, obj_id: str, current: int, total: int) -> None: - # Guard against progress_task/progress_cm not being ready yet. - if progress_task is None or progress_cm is None: - return - # total is known up-front (passed the first time), but re-setting - # is a no-op after the first call. - progress_cm.update( - progress_task, - total=total, - completed=max(current - 1, 0), - description=f"Describing {obj_type} {obj_id}", - ) - - progress_callback = _on_item - - try: - if progress_cm is not None: - progress_cm.start() - progress_task = progress_cm.add_task("Applying descriptions...", total=None) - result = service.describe_batch( - alias=project, - from_file=from_file, - branch_id=effective_branch, - progress_callback=progress_callback, - ) - if progress_cm is not None and progress_task is not None: - # Mark the task complete so the final render shows N / N. - progress_cm.update( - progress_task, - completed=result["applied_count"] + result["error_count"], - ) - except ValueError as exc: - formatter.error(message=str(exc), error_code=ErrorCode.INVALID_ARGUMENT) - raise typer.Exit(code=2) from None - 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, retryable=exc.retryable) - raise typer.Exit(code=map_error_to_exit_code(exc)) from None - finally: - if progress_cm is not None: - # .stop() is idempotent; safe for both happy and error paths. - progress_cm.stop() - - if formatter.json_mode: - formatter.output(result) - else: - applied = result["applied_count"] - errors = result["error_count"] - formatter.console.print( - f"[bold green]Batch complete:[/bold green] {applied} applied, {errors} error(s)" - ) - for item in result["applied"]: - obj_type = item["type"] - obj_id = item["id"] - if obj_type == "columns": - n = len(item.get("columns", {})) - formatter.console.print(f" [green]✓[/green] {obj_type} {obj_id} ({n} cols)") - else: - formatter.console.print(f" [green]✓[/green] {obj_type} {obj_id}") - for item in result["errors"]: - formatter.console.print(f" [red]✗[/red] {item['type']} {item['id']}: {item['error']}") - if errors: - raise typer.Exit(code=1) from None - - # ------------------------------------------------------------------ # File operations # ------------------------------------------------------------------ @@ -2712,3 +2343,10 @@ def storage_unload_table( from ._storage_snapshots import register as _register_snapshot_commands # noqa: E402 _register_snapshot_commands(storage_app) + + +# Description commands (issue #624 grew them past the file's size ceiling) live +# in a private module for the same reason, mounted the same way. +from ._storage_describe import register as _register_describe_commands # noqa: E402 + +_register_describe_commands(storage_app) diff --git a/src/keboola_agent_cli/permissions.py b/src/keboola_agent_cli/permissions.py index bc2a9171..e53f31c5 100644 --- a/src/keboola_agent_cli/permissions.py +++ b/src/keboola_agent_cli/permissions.py @@ -250,6 +250,7 @@ "storage.describe-table": "write", "storage.describe-column": "write", "storage.describe-batch": "write", + "storage.describe-migrate": "write", # Encryption "encrypt.values": "write", # Semantic layer (metastore) — new in 0.41.0 diff --git a/src/keboola_agent_cli/services/_table_detail.py b/src/keboola_agent_cli/services/_table_detail.py index 6e389a87..ed778dfa 100644 --- a/src/keboola_agent_cli/services/_table_detail.py +++ b/src/keboola_agent_cli/services/_table_detail.py @@ -20,48 +20,69 @@ from dataclasses import dataclass, field from typing import Any +from ._column_descriptions import ( + collect_legacy_column_entries, + description_from_column_meta, + native_column_descriptions, +) + @dataclass(frozen=True) class Descriptions: """The user-authored descriptions carried in a table's metadata list.""" table: str = "" + # Column name -> value of its LEGACY flat `KBC.column.*.description` key. + # Only the legacy tier lives in the table's metadata list; the two tiers + # that outrank it are read per column in `_column_details`. columns: dict[str, str] = field(default_factory=dict) def _split_descriptions(raw_metadata: list[dict[str, Any]]) -> Descriptions: - """Pull the user table description and per-column descriptions out of metadata. - - Keboola has no user-writable column-metadata endpoint, so column descriptions - are stored as `KBC.column.{name}.description` rows in the *table's* metadata - list rather than alongside the column. + """Pull the user table description and legacy column descriptions out of metadata. + + Before 0.88.0 kbagent stored column descriptions as + `KBC.column.{name}.description` rows in the *table's* metadata list, on the + belief that Keboola had no user-writable column-level endpoint. It does -- + see `_column_descriptions.write_column_descriptions` (#624) -- so that shape + is legacy: nothing but this CLI ever read it. It is still parsed here as the + last-resort tier so already-documented projects keep rendering. """ table_description = "" - col_descriptions: dict[str, str] = {} for entry in raw_metadata: - key = entry.get("key", "") - if key == "KBC.description" and entry.get("provider") == "user": + if entry.get("key") == "KBC.description" and entry.get("provider") == "user": table_description = entry.get("value", "") or "" - elif key.startswith("KBC.column.") and key.endswith(".description"): - col_name = key[len("KBC.column.") : -len(".description")] - col_descriptions[col_name] = entry.get("value", "") or "" - return Descriptions(table=table_description, columns=col_descriptions) + legacy = collect_legacy_column_entries(raw_metadata) + return Descriptions( + table=table_description, + columns={col: (entry.get("value") or "") for col, entry in legacy.items()}, + ) def _column_details( columns: list[str], column_metadata: dict[str, list[dict[str, Any]]], - col_descriptions: dict[str, str], + legacy_descriptions: dict[str, str], + native_descriptions: dict[str, str], + source_column_metadata: dict[str, list[dict[str, Any]]], ) -> list[dict[str, Any]]: """Build the per-column view from `columnMetadata`'s KBC.datatype.* rows. Types are read from `columnMetadata`, never from `definition` -- the latter has really been served as `[]` (see tests/test_storage_empty_definition.py). + + Descriptions resolve in three tiers (#624): the native `definition` field + the Keboola UI writes and shows, then `KBC.description` in `columnMetadata` + (where the backend mirrors that write, and the only place the MCP server + looks), then the legacy flat key. An alias carries no `columnMetadata` of + its own, so it falls back to the source table's -- the same lookup the MCP + server performs. """ details: list[dict[str, Any]] = [] for col in columns: col_info: dict[str, Any] = {"name": col} - for entry in column_metadata.get(col, []): + entries = column_metadata.get(col, []) + for entry in entries: key = entry.get("key", "") value = entry.get("value", "") if key == "KBC.datatype.basetype": @@ -77,8 +98,12 @@ def _column_details( col_info["nullable"] = value == "1" elif key == "KBC.datatype.default": col_info["default"] = value - if col in col_descriptions: - col_info["description"] = col_descriptions[col] + meta_desc = description_from_column_meta(entries) + if meta_desc is None: + meta_desc = description_from_column_meta(source_column_metadata.get(col, [])) + description = native_descriptions.get(col) or meta_desc or legacy_descriptions.get(col) + if description: + col_info["description"] = description details.append(col_info) return details @@ -97,6 +122,9 @@ def build_table_detail(alias: str, table_id: str, table: dict[str, Any]) -> dict columns = table.get("columns", []) raw_metadata: list[dict[str, Any]] = table.get("metadata", []) descriptions = _split_descriptions(raw_metadata) + source_column_metadata: dict[str, list[dict[str, Any]]] = ( + (table.get("sourceTable") or {}).get("columnMetadata") or {} if table.get("isAlias") else {} + ) return { "project_alias": alias, @@ -112,7 +140,11 @@ def build_table_detail(alias: str, table_id: str, table: dict[str, Any]) -> dict "description": descriptions.table, "columns": columns, "column_details": _column_details( - columns, table.get("columnMetadata", {}), descriptions.columns + columns, + table.get("columnMetadata", {}), + descriptions.columns, + native_column_descriptions(table), + source_column_metadata, ), "primary_key": table.get("primaryKey", []), # API may return null on empty tables; coerce to 0. @@ -131,4 +163,10 @@ def build_table_detail(alias: str, table_id: str, table: dict[str, Any]) -> dict # `requirePartitionFilter` and an unbounded `partitions[]` list. Not # re-shaped: trimming an API field is the bug this key exists to fix. "definition": table.get("definition"), + # Columns still carrying a legacy flat KBC.column.*.description key + # (invisible to the Keboola UI and the MCP server). Always present, + # empty when there are none; the CLI turns a non-empty list into a hint + # to run `kbagent storage describe-migrate` (#624). Reporting only -- + # a read never rewrites what it finds. + "legacy_column_descriptions": sorted(descriptions.columns), } diff --git a/src/keboola_agent_cli/services/storage_service.py b/src/keboola_agent_cli/services/storage_service.py index 97d1946e..48cec4df 100644 --- a/src/keboola_agent_cli/services/storage_service.py +++ b/src/keboola_agent_cli/services/storage_service.py @@ -14,8 +14,8 @@ from ..constants import STORAGE_BRANCHES_FEATURE from ..errors import ConfigError, ErrorCode, KeboolaApiError from ..models import ProjectConfig +from ._column_descriptions import ColumnDescriptionsMixin from ._table_detail import build_table_detail -from .base import BaseService logger = logging.getLogger(__name__) @@ -395,7 +395,7 @@ def _prepend_csv_header(file_path: str, columns: list[str]) -> None: tmp_path.replace(p) -class StorageService(BaseService): +class StorageService(ColumnDescriptionsMixin): """Business logic for storage bucket and table operations. Supports multi-project parallel queries for listing operations. @@ -2461,56 +2461,6 @@ def describe_table( "message": f"Description set on table '{table_id}' in project '{alias}'.", } - def describe_columns( - self, - alias: str, - table_id: str, - columns: dict[str, str], - branch_id: int | None = None, - ) -> dict[str, Any]: - """Set per-column descriptions on a storage table. - - Column descriptions are stored as namespaced table metadata using the - key convention ``KBC.column.{colname}.description``. Keboola's - Storage API does not provide a user-writable column-level metadata - endpoint (``columnMetadata`` is populated exclusively by processing - components); this convention is the supported alternative for - annotating columns from the CLI. - - Args: - alias: Project alias. - table_id: Full table ID. - columns: Mapping of column name -> description text. - branch_id: If set, target a specific dev branch. - - Returns: - Dict with project_alias, table_id, columns dict, result, message. - """ - if not columns: - raise ValueError("At least one column description must be provided.") - projects = self.resolve_projects([alias]) - project = projects[alias] - entries = [(f"KBC.column.{name}.description", desc) for name, desc in columns.items()] - client = self._client_factory(project.stack_url, project.token) - try: - result = client.set_table_metadata( - table_id=table_id, - entries=entries, - branch_id=branch_id, - ) - finally: - client.close() - return { - "project_alias": alias, - "table_id": table_id, - "columns": columns, - "result": result, - "message": ( - f"Descriptions set for {len(columns)} column(s) on table '{table_id}' " - f"in project '{alias}'." - ), - } - def describe_batch( self, alias: str, diff --git a/tests/test_e2e.py b/tests/test_e2e.py index ffc35d02..d641b974 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -4007,6 +4007,75 @@ def _test_storage_describe(self, bucket_id: str, table_id: str) -> None: assert data["data"]["description"] == "Batch table desc" col_descs = {c["name"]: c.get("description", "") for c in data["data"]["column_details"]} assert col_descs.get("id") == "Batch column id desc" + # The native write leaves no legacy flat keys behind (#624). + assert data["data"]["legacy_column_descriptions"] == [] + + self._test_storage_describe_migrate(table_id) + + def _test_storage_describe_migrate(self, table_id: str) -> None: + """Seed a pre-0.88.0 flat metadata key and migrate it (#624). + + The flat ``KBC.column.{name}.description`` convention is what kbagent + wrote before the native definition endpoint; nothing but kbagent ever + read it. Seeding goes through the raw client on purpose -- no CLI + command writes that shape any more. + """ + self.api.set_table_metadata( + table_id=table_id, + entries=[("KBC.column.name.description", "Legacy column description")], + ) + + data = self._run_ok( + "storage", "table-detail", "--project", self.alias, "--table-id", table_id + ) + assert data["data"]["legacy_column_descriptions"] == ["name"] + + # Dry run reports the table and writes nothing. + data = self._run_ok( + "storage", + "describe-migrate", + "--project", + self.alias, + "--table-id", + table_id, + "--dry-run", + ) + assert data["data"]["dry_run"] is True + assert data["data"]["tables_migrated"] == 0 + migrated = {item["table_id"]: item["columns"] for item in data["data"]["migrated"]} + assert migrated[table_id]["name"] == "Legacy column description" + + data = self._run_ok( + "storage", + "describe-migrate", + "--project", + self.alias, + "--table-id", + table_id, + "--yes", + ) + assert data["data"]["tables_migrated"] == 1 + assert data["data"]["errors"] == [] + + # The description survives where everyone reads it, the legacy key is gone. + data = self._run_ok( + "storage", "table-detail", "--project", self.alias, "--table-id", table_id + ) + assert data["data"]["legacy_column_descriptions"] == [] + col_descs = {c["name"]: c.get("description", "") for c in data["data"]["column_details"]} + assert col_descs.get("name") == "Legacy column description" + + # Re-running is a no-op: nothing left to migrate. + data = self._run_ok( + "storage", + "describe-migrate", + "--project", + self.alias, + "--table-id", + table_id, + "--yes", + ) + assert data["data"]["migrated"] == [] def _test_semantic_layer_roundtrip(self) -> None: """Live roundtrip of the semantic-layer command group against ``self.alias``. diff --git a/tests/test_storage_describe_cli.py b/tests/test_storage_describe_cli.py index a82b6067..ee1bff0f 100644 --- a/tests/test_storage_describe_cli.py +++ b/tests/test_storage_describe_cli.py @@ -944,3 +944,305 @@ def test_bigquery_human_render_with_project_omits_helper_hint(self, tmp_path: Pa # Helper hint is for the empty-project case only -- must not appear # when project IS populated. assert "not exposed by Storage API" not in output + + +_MIGRATE_RESULT = { + "project_alias": "prod", + "dry_run": True, + "tables_scanned": 2, + "tables_migrated": 0, + "migrated": [{"table_id": "in.c-bucket.orders", "columns": {"order_id": "Unique id"}}], + "skipped": [], + "pruned_orphans": [], + "errors": [], + "message": "Would migrate 1 table(s) of 2 scanned in project 'prod'.", +} + + +class TestStorageDescribeMigrate: + """Tests for `kbagent storage describe-migrate`.""" + + def _invoke( + self, + tmp_path: Path, + args: list[str], + mock_storage: MagicMock, + stdin: str | None = None, + ): + config_dir = tmp_path / "config" + config_dir.mkdir(exist_ok=True) + store = _setup_config(config_dir, {"prod": {"token": TEST_TOKEN}}) + + with ( + patch("keboola_agent_cli.cli.ConfigStore") as MockStore, + patch("keboola_agent_cli.cli.ProjectService") as MockProjService, + patch("keboola_agent_cli.cli.ConfigService") as MockCfgService, + patch("keboola_agent_cli.cli.JobService") as MockJobService, + patch("keboola_agent_cli.cli.StorageService") as MockStorageService, + ): + MockStore.return_value = store + MockProjService.return_value = ProjectService(config_store=store) + MockCfgService.return_value = ConfigService(config_store=store) + MockJobService.return_value = JobService(config_store=store) + MockStorageService.return_value = mock_storage + return runner.invoke(app, args, input=stdin) + + def test_describe_migrate_dry_run_json(self, tmp_path: Path) -> None: + """--dry-run --json emits the scan report and performs no write.""" + mock_storage = MagicMock() + mock_storage.describe_migrate.return_value = _MIGRATE_RESULT + + result = self._invoke( + tmp_path, + [ + "--json", + "storage", + "describe-migrate", + "--project", + "prod", + "--dry-run", + ], + mock_storage, + ) + + assert result.exit_code == 0, f"Exit code {result.exit_code}: {result.output}" + output = json.loads(result.output) + assert output["status"] == "ok" + assert output["data"]["dry_run"] is True + assert output["data"]["migrated"][0]["table_id"] == "in.c-bucket.orders" + mock_storage.describe_migrate.assert_called_once_with( + alias="prod", + table_ids=None, + bucket_id=None, + prune_orphans=False, + dry_run=True, + branch_id=None, + ) + + def test_describe_migrate_scope_conflict_exits_2(self, tmp_path: Path) -> None: + """--table-id together with --bucket-id is a usage error.""" + mock_storage = MagicMock() + + result = self._invoke( + tmp_path, + [ + "--json", + "storage", + "describe-migrate", + "--project", + "prod", + "--table-id", + "in.c-bucket.orders", + "--bucket-id", + "in.c-bucket", + ], + mock_storage, + ) + + assert result.exit_code == 2, f"Expected 2, got {result.exit_code}: {result.output}" + output = json.loads(result.output) + assert output["status"] == "error" + assert output["error"]["code"] == "INVALID_ARGUMENT" + mock_storage.describe_migrate.assert_not_called() + + def test_describe_migrate_confirm_abort_makes_no_writes(self, tmp_path: Path) -> None: + """Answering 'n' at the confirm prompt leaves the scan as the only call.""" + mock_storage = MagicMock() + mock_storage.describe_migrate.return_value = _MIGRATE_RESULT + + result = self._invoke( + tmp_path, + ["storage", "describe-migrate", "--project", "prod"], + mock_storage, + stdin="n\n", + ) + + assert result.exit_code == 0, f"Exit code {result.exit_code}: {result.output}" + assert "Aborted" in result.output + assert mock_storage.describe_migrate.call_count == 1 + assert mock_storage.describe_migrate.call_args.kwargs["dry_run"] is True + + def test_describe_migrate_yes_skips_confirm(self, tmp_path: Path) -> None: + """--yes applies straight away (single, non-dry-run call).""" + mock_storage = MagicMock() + applied = dict(_MIGRATE_RESULT, dry_run=False, tables_migrated=1) + mock_storage.describe_migrate.return_value = applied + + result = self._invoke( + tmp_path, + [ + "storage", + "describe-migrate", + "--project", + "prod", + "--bucket-id", + "in.c-bucket", + "--prune-orphans", + "--yes", + ], + mock_storage, + ) + + assert result.exit_code == 0, f"Exit code {result.exit_code}: {result.output}" + mock_storage.describe_migrate.assert_called_once_with( + alias="prod", + table_ids=None, + bucket_id="in.c-bucket", + prune_orphans=True, + dry_run=False, + branch_id=None, + ) + assert "in.c-bucket.orders" in result.output + + def test_describe_migrate_table_ids_repeatable(self, tmp_path: Path) -> None: + mock_storage = MagicMock() + mock_storage.describe_migrate.return_value = dict(_MIGRATE_RESULT, dry_run=False) + + result = self._invoke( + tmp_path, + [ + "--json", + "storage", + "describe-migrate", + "--project", + "prod", + "--table-id", + "in.c-bucket.a", + "--table-id", + "in.c-bucket.b", + "--yes", + ], + mock_storage, + ) + + assert result.exit_code == 0, f"Exit code {result.exit_code}: {result.output}" + mock_storage.describe_migrate.assert_called_once_with( + alias="prod", + table_ids=["in.c-bucket.a", "in.c-bucket.b"], + bucket_id=None, + prune_orphans=False, + dry_run=False, + branch_id=None, + ) + + def test_describe_migrate_api_error_maps_exit_code(self, tmp_path: Path) -> None: + mock_storage = MagicMock() + mock_storage.describe_migrate.side_effect = KeboolaApiError( + message="Bucket not found", + status_code=404, + error_code="NOT_FOUND", + retryable=False, + ) + + result = self._invoke( + tmp_path, + [ + "--json", + "storage", + "describe-migrate", + "--project", + "prod", + "--dry-run", + ], + mock_storage, + ) + + assert result.exit_code == 1, f"Exit code {result.exit_code}: {result.output}" + output = json.loads(result.output) + assert output["status"] == "error" + assert output["error"]["code"] == "NOT_FOUND" + + def test_describe_migrate_is_a_write_operation(self) -> None: + """The permission firewall must classify the command as a write.""" + from keboola_agent_cli.permissions import OPERATION_REGISTRY + + assert OPERATION_REGISTRY["storage.describe-migrate"] == "write" + + +class TestStorageTableDetailLegacyWarning: + """table-detail surfaces legacy column-description keys (#624).""" + + def _invoke(self, tmp_path: Path, args: list[str], payload: dict): + config_dir = tmp_path / "config" + config_dir.mkdir(exist_ok=True) + store = _setup_config(config_dir, {"prod": {"token": TEST_TOKEN}}) + + mock_storage = MagicMock() + mock_storage.get_table_detail.return_value = payload + + with ( + patch("keboola_agent_cli.cli.ConfigStore") as MockStore, + patch("keboola_agent_cli.cli.ProjectService") as MockProjService, + patch("keboola_agent_cli.cli.ConfigService") as MockCfgService, + patch("keboola_agent_cli.cli.JobService") as MockJobService, + patch("keboola_agent_cli.cli.StorageService") as MockStorageService, + ): + MockStore.return_value = store + MockProjService.return_value = ProjectService(config_store=store) + MockCfgService.return_value = ConfigService(config_store=store) + MockJobService.return_value = JobService(config_store=store) + MockStorageService.return_value = mock_storage + return runner.invoke(app, args) + + @staticmethod + def _payload(legacy: list[str]) -> dict: + return { + "project_alias": "prod", + "table_id": "in.c-bucket.orders", + "name": "orders", + "display_name": "orders", + "bucket_id": "in.c-bucket", + "backend": "snowflake", + "description": "", + "columns": ["order_id"], + "column_details": [{"name": "order_id", "type": "STRING"}], + "primary_key": [], + "rows_count": 0, + "data_size_bytes": 0, + "is_alias": False, + "last_import_date": "", + "last_change_date": "", + "created": "", + "metadata": [], + "legacy_column_descriptions": legacy, + } + + def test_table_detail_human_warns_on_legacy(self, tmp_path: Path) -> None: + result = self._invoke( + tmp_path, + ["storage", "table-detail", "--project", "prod", "--table-id", "in.c-bucket.orders"], + self._payload(["order_id"]), + ) + + assert result.exit_code == 0, f"Exit code {result.exit_code}: {result.output}" + assert "legacy" in result.output.lower() + assert "describe-migrate" in result.output + + def test_table_detail_human_silent_without_legacy(self, tmp_path: Path) -> None: + result = self._invoke( + tmp_path, + ["storage", "table-detail", "--project", "prod", "--table-id", "in.c-bucket.orders"], + self._payload([]), + ) + + assert result.exit_code == 0, f"Exit code {result.exit_code}: {result.output}" + assert "describe-migrate" not in result.output + + def test_table_detail_json_exposes_legacy_key(self, tmp_path: Path) -> None: + result = self._invoke( + tmp_path, + [ + "--json", + "storage", + "table-detail", + "--project", + "prod", + "--table-id", + "in.c-bucket.orders", + ], + self._payload(["order_id"]), + ) + + assert result.exit_code == 0, f"Exit code {result.exit_code}: {result.output}" + output = json.loads(result.output) + assert output["data"]["legacy_column_descriptions"] == ["order_id"] diff --git a/tests/test_storage_describe_service.py b/tests/test_storage_describe_service.py index ca9fa7a2..0e83c579 100644 --- a/tests/test_storage_describe_service.py +++ b/tests/test_storage_describe_service.py @@ -7,6 +7,7 @@ """ from pathlib import Path +from typing import Any from unittest.mock import MagicMock import pytest @@ -178,35 +179,94 @@ def test_api_error_propagates(self, tmp_path: Path) -> None: mock_client.close.assert_called_once() +def _table_detail( + columns: list[str] | None = None, + metadata: list[dict[str, Any]] | None = None, + column_metadata: dict[str, list[dict[str, Any]]] | None = None, + **extra: Any, +) -> dict[str, Any]: + """Build a Storage API table-detail payload for describe/migrate tests.""" + table: dict[str, Any] = { + "id": "in.c-sales.orders", + "name": "orders", + "displayName": "orders", + "bucket": {"id": "in.c-sales"}, + "columns": columns if columns is not None else ["c1", "c2"], + "primaryKey": [], + "columnMetadata": column_metadata or {}, + "metadata": metadata or [], + } + table.update(extra) + return table + + +_JOB_OK = {"id": 4242, "status": "success", "operationName": "tableDefinitionUpdate"} + + +def _legacy_entry(entry_id: int, column: str, value: str) -> dict[str, Any]: + return { + "id": entry_id, + "key": f"KBC.column.{column}.description", + "value": value, + "provider": "user", + "timestamp": "2026-08-21T10:00:00Z", + } + + class TestDescribeColumnsService: - """Tests for StorageService.describe_columns().""" + """Tests for StorageService.describe_columns() (native definition endpoint).""" - def test_success_namespaced_keys(self, tmp_path: Path) -> None: + def test_describe_columns_uses_native_endpoint(self, tmp_path: Path) -> None: + """The write goes through PUT .../definition, never the flat metadata key.""" store = _make_store(tmp_path) mock_client = MagicMock() - mock_client.set_table_metadata.return_value = [] + mock_client.get_table_detail.return_value = _table_detail(columns=["c1"]) + mock_client.update_table_definition.return_value = _JOB_OK service = _make_service(store, mock_client) result = service.describe_columns( alias="prod", table_id="in.c-sales.orders", - columns={"order_id": "Unique order identifier", "total": "Order total in USD"}, + columns={"c1": "d1"}, ) - assert result["project_alias"] == "prod" - assert result["table_id"] == "in.c-sales.orders" - assert result["columns"]["order_id"] == "Unique order identifier" - assert result["columns"]["total"] == "Order total in USD" - mock_client.set_table_metadata.assert_called_once_with( + mock_client.update_table_definition.assert_called_once_with( table_id="in.c-sales.orders", - entries=[ - ("KBC.column.order_id.description", "Unique order identifier"), - ("KBC.column.total.description", "Order total in USD"), - ], + columns=[{"name": "c1", "description": "d1"}], + is_description_system_managed=False, branch_id=None, ) + mock_client.set_table_metadata.assert_not_called() + assert result["project_alias"] == "prod" + assert result["table_id"] == "in.c-sales.orders" + assert result["columns"] == {"c1": "d1"} + assert result["migrated"] == {} + assert result["skipped"] == [] + assert result["result"] == _JOB_OK mock_client.close.assert_called_once() + def test_describe_columns_with_branch(self, tmp_path: Path) -> None: + store = _make_store(tmp_path) + mock_client = MagicMock() + mock_client.get_table_detail.return_value = _table_detail(columns=["c1"]) + mock_client.update_table_definition.return_value = _JOB_OK + service = _make_service(store, mock_client) + + service.describe_columns( + alias="prod", + table_id="in.c-sales.orders", + columns={"c1": "First column"}, + branch_id=77, + ) + + mock_client.get_table_detail.assert_called_once_with("in.c-sales.orders", branch_id=77) + mock_client.update_table_definition.assert_called_once_with( + table_id="in.c-sales.orders", + columns=[{"name": "c1", "description": "First column"}], + is_description_system_managed=False, + branch_id=77, + ) + def test_empty_columns_raises_value_error(self, tmp_path: Path) -> None: store = _make_store(tmp_path) mock_client = MagicMock() @@ -215,26 +275,173 @@ def test_empty_columns_raises_value_error(self, tmp_path: Path) -> None: with pytest.raises(ValueError, match="At least one column"): service.describe_columns(alias="prod", table_id="in.c-sales.orders", columns={}) - mock_client.set_table_metadata.assert_not_called() + mock_client.update_table_definition.assert_not_called() - def test_with_branch(self, tmp_path: Path) -> None: + def test_describe_columns_unknown_column_fails_fast(self, tmp_path: Path) -> None: + """Unknown column names abort BEFORE any write (old flat write accepted anything).""" store = _make_store(tmp_path) mock_client = MagicMock() - mock_client.set_table_metadata.return_value = [] + mock_client.get_table_detail.return_value = _table_detail(columns=["c1", "c2"]) service = _make_service(store, mock_client) - service.describe_columns( + with pytest.raises(ValueError, match="nope"): + service.describe_columns( + alias="prod", + table_id="in.c-sales.orders", + columns={"c1": "ok", "nope": "bad"}, + ) + + mock_client.update_table_definition.assert_not_called() + + def test_describe_columns_migrates_legacy_sibling(self, tmp_path: Path) -> None: + """A sibling legacy flat key rides along in the same native write, then is deleted.""" + store = _make_store(tmp_path) + mock_client = MagicMock() + mock_client.get_table_detail.return_value = _table_detail( + columns=["c1", "c2"], + metadata=[_legacy_entry(7, "c2", "old")], + ) + mock_client.update_table_definition.return_value = _JOB_OK + service = _make_service(store, mock_client) + + result = service.describe_columns( alias="prod", table_id="in.c-sales.orders", - columns={"col1": "First column"}, - branch_id=77, + columns={"c1": "new"}, ) - mock_client.set_table_metadata.assert_called_once_with( + payload = mock_client.update_table_definition.call_args.kwargs["columns"] + assert {"name": "c1", "description": "new"} in payload + assert {"name": "c2", "description": "old"} in payload + mock_client.delete_table_metadata.assert_called_once_with( + "in.c-sales.orders", 7, branch_id=None + ) + assert result["migrated"] == {"c2": "old"} + assert result["skipped"] == [] + + def test_describe_columns_migration_conflict_skipped(self, tmp_path: Path) -> None: + """A legacy value that clashes with the visible description is skipped, not deleted.""" + store = _make_store(tmp_path) + mock_client = MagicMock() + mock_client.get_table_detail.return_value = _table_detail( + columns=["c1", "c2"], + metadata=[_legacy_entry(7, "c2", "old")], + column_metadata={"c2": [{"key": "KBC.description", "value": "newer"}]}, + ) + mock_client.update_table_definition.return_value = _JOB_OK + service = _make_service(store, mock_client) + + result = service.describe_columns( + alias="prod", table_id="in.c-sales.orders", - entries=[("KBC.column.col1.description", "First column")], - branch_id=77, + columns={"c1": "new"}, + ) + + payload = mock_client.update_table_definition.call_args.kwargs["columns"] + assert payload == [{"name": "c1", "description": "new"}] + mock_client.delete_table_metadata.assert_not_called() + assert result["migrated"] == {} + assert result["skipped"] == [ + {"column": "c2", "reason": "conflict", "legacy": "old", "current": "newer"} + ] + + def test_describe_columns_migration_identical_deletes_only(self, tmp_path: Path) -> None: + """Identical legacy + visible value: nothing to write, the stale entry still goes.""" + store = _make_store(tmp_path) + mock_client = MagicMock() + mock_client.get_table_detail.return_value = _table_detail( + columns=["c1", "c2"], + metadata=[_legacy_entry(7, "c2", "same")], + column_metadata={"c2": [{"key": "KBC.description", "value": "same"}]}, ) + mock_client.update_table_definition.return_value = _JOB_OK + service = _make_service(store, mock_client) + + result = service.describe_columns( + alias="prod", + table_id="in.c-sales.orders", + columns={"c1": "new"}, + ) + + payload = mock_client.update_table_definition.call_args.kwargs["columns"] + assert payload == [{"name": "c1", "description": "new"}] + mock_client.delete_table_metadata.assert_called_once_with( + "in.c-sales.orders", 7, branch_id=None + ) + assert result["migrated"] == {} + + def test_describe_columns_orphan_skipped(self, tmp_path: Path) -> None: + """A legacy key for a dropped column is reported, never deleted implicitly.""" + store = _make_store(tmp_path) + mock_client = MagicMock() + mock_client.get_table_detail.return_value = _table_detail( + columns=["c1"], + metadata=[_legacy_entry(9, "ghost", "gone")], + ) + mock_client.update_table_definition.return_value = _JOB_OK + service = _make_service(store, mock_client) + + result = service.describe_columns( + alias="prod", + table_id="in.c-sales.orders", + columns={"c1": "new"}, + ) + + mock_client.delete_table_metadata.assert_not_called() + assert result["skipped"] == [{"column": "ghost", "reason": "orphan", "legacy": "gone"}] + + def test_describe_columns_user_value_wins_over_legacy(self, tmp_path: Path) -> None: + """A legacy key on a column the user is describing loses -- and is cleaned up.""" + store = _make_store(tmp_path) + mock_client = MagicMock() + mock_client.get_table_detail.return_value = _table_detail( + columns=["c1"], + metadata=[_legacy_entry(5, "c1", "old")], + ) + mock_client.update_table_definition.return_value = _JOB_OK + service = _make_service(store, mock_client) + + result = service.describe_columns( + alias="prod", + table_id="in.c-sales.orders", + columns={"c1": "new"}, + ) + + payload = mock_client.update_table_definition.call_args.kwargs["columns"] + assert payload == [{"name": "c1", "description": "new"}] + mock_client.delete_table_metadata.assert_called_once_with( + "in.c-sales.orders", 5, branch_id=None + ) + assert result["migrated"] == {} + assert result["skipped"] == [] + + def test_describe_columns_delete_failure_does_not_fail(self, tmp_path: Path) -> None: + """The native write is durable; a failed legacy cleanup is reported, not raised.""" + store = _make_store(tmp_path) + mock_client = MagicMock() + mock_client.get_table_detail.return_value = _table_detail( + columns=["c1", "c2"], + metadata=[_legacy_entry(7, "c2", "old")], + ) + mock_client.update_table_definition.return_value = _JOB_OK + mock_client.delete_table_metadata.side_effect = KeboolaApiError( + message="boom", + status_code=500, + error_code="SERVER_ERROR", + retryable=False, + ) + service = _make_service(store, mock_client) + + result = service.describe_columns( + alias="prod", + table_id="in.c-sales.orders", + columns={"c1": "new"}, + ) + + assert result["result"] == _JOB_OK + assert result["migrated"] == {"c2": "old"} + assert [s["reason"] for s in result["skipped"]] == ["delete_failed"] + assert result["skipped"][0]["column"] == "c2" class TestDescribeBatchService: @@ -245,6 +452,8 @@ def test_success_all_sections(self, tmp_path: Path) -> None: mock_client = MagicMock() mock_client.set_bucket_metadata.return_value = [] mock_client.set_table_metadata.return_value = [] + mock_client.get_table_detail.return_value = _table_detail(columns=["order_id"]) + mock_client.update_table_definition.return_value = _JOB_OK service = _make_service(store, mock_client) batch_file = tmp_path / "batch.yaml" @@ -268,10 +477,12 @@ def test_success_all_sections(self, tmp_path: Path) -> None: assert "bucket" in applied_types assert "table" in applied_types assert "columns" in applied_types - # Bucket metadata called once (for the bucket), table metadata called twice - # (once for table description, once for column descriptions) + # Bucket metadata called once (for the bucket); the table description + # still goes through table metadata, column descriptions now go through + # the native definition endpoint. assert mock_client.set_bucket_metadata.call_count == 1 - assert mock_client.set_table_metadata.call_count == 2 + assert mock_client.set_table_metadata.call_count == 1 + assert mock_client.update_table_definition.call_count == 1 def test_file_not_found(self, tmp_path: Path) -> None: store = _make_store(tmp_path) @@ -812,3 +1023,277 @@ def test_backend_defaults_to_empty_when_absent(self, tmp_path: Path) -> None: result = service.get_table_detail(alias="prod", table_id="in.c-sales.orders") assert result["backend"] == "" + + +def _listing_row(table_id: str, metadata: list[dict[str, Any]] | None = None) -> dict[str, Any]: + """Row as returned by ``list_tables(include="metadata")``.""" + return {"id": table_id, "name": table_id.split(".")[-1], "metadata": metadata or []} + + +class TestDescribeMigrateService: + """Tests for StorageService.describe_migrate() (bulk legacy conversion).""" + + def _client( + self, listing: list[dict[str, Any]], details: dict[str, dict[str, Any]] + ) -> MagicMock: + mock_client = MagicMock() + mock_client.list_tables.return_value = listing + mock_client.get_table_detail.side_effect = lambda tid, branch_id=None: details[tid] + mock_client.update_table_definition.return_value = _JOB_OK + return mock_client + + def test_describe_migrate_dry_run_reports_no_writes(self, tmp_path: Path) -> None: + store = _make_store(tmp_path) + legacy = [_legacy_entry(7, "c2", "old")] + mock_client = self._client( + [_listing_row("in.c-x.t1", legacy), _listing_row("in.c-x.t2")], + { + "in.c-x.t1": _table_detail(columns=["c1", "c2"], metadata=legacy), + "in.c-x.t2": _table_detail(columns=["c1"]), + }, + ) + service = _make_service(store, mock_client) + + result = service.describe_migrate(alias="prod", dry_run=True) + + assert result["dry_run"] is True + assert result["tables_scanned"] == 2 + assert result["tables_migrated"] == 0 + assert result["migrated"] == [{"table_id": "in.c-x.t1", "columns": {"c2": "old"}}] + mock_client.update_table_definition.assert_not_called() + mock_client.delete_table_metadata.assert_not_called() + + def test_describe_migrate_applies_and_deletes(self, tmp_path: Path) -> None: + store = _make_store(tmp_path) + legacy = [_legacy_entry(7, "c2", "old")] + mock_client = self._client( + [_listing_row("in.c-x.t1", legacy), _listing_row("in.c-x.t2")], + { + "in.c-x.t1": _table_detail(columns=["c1", "c2"], metadata=legacy), + "in.c-x.t2": _table_detail(columns=["c1"]), + }, + ) + service = _make_service(store, mock_client) + + result = service.describe_migrate(alias="prod") + + mock_client.update_table_definition.assert_called_once_with( + table_id="in.c-x.t1", + columns=[{"name": "c2", "description": "old"}], + is_description_system_managed=False, + branch_id=None, + ) + mock_client.delete_table_metadata.assert_called_once_with("in.c-x.t1", 7, branch_id=None) + assert result["tables_migrated"] == 1 + assert result["errors"] == [] + + def test_describe_migrate_scope_bucket(self, tmp_path: Path) -> None: + store = _make_store(tmp_path) + legacy = [_legacy_entry(7, "c2", "old")] + mock_client = self._client( + [_listing_row("in.c-x.t1", legacy), _listing_row("in.c-y.t9", legacy)], + { + "in.c-x.t1": _table_detail(columns=["c1", "c2"], metadata=legacy), + "in.c-y.t9": _table_detail(columns=["c1", "c2"], metadata=legacy), + }, + ) + service = _make_service(store, mock_client) + + result = service.describe_migrate(alias="prod", bucket_id="in.c-x", dry_run=True) + + assert result["tables_scanned"] == 1 + assert [m["table_id"] for m in result["migrated"]] == ["in.c-x.t1"] + + def test_describe_migrate_scope_table_ids(self, tmp_path: Path) -> None: + store = _make_store(tmp_path) + legacy = [_legacy_entry(7, "c2", "old")] + mock_client = self._client( + [], {"in.c-x.t1": _table_detail(columns=["c1", "c2"], metadata=legacy)} + ) + service = _make_service(store, mock_client) + + result = service.describe_migrate(alias="prod", table_ids=["in.c-x.t1"], dry_run=True) + + mock_client.list_tables.assert_not_called() + assert result["tables_scanned"] == 1 + assert result["migrated"] == [{"table_id": "in.c-x.t1", "columns": {"c2": "old"}}] + + def test_describe_migrate_both_scopes_raises(self, tmp_path: Path) -> None: + store = _make_store(tmp_path) + mock_client = MagicMock() + service = _make_service(store, mock_client) + + with pytest.raises(ValueError, match="mutually exclusive"): + service.describe_migrate(alias="prod", table_ids=["in.c-x.t1"], bucket_id="in.c-x") + + def test_describe_migrate_prune_orphans(self, tmp_path: Path) -> None: + store = _make_store(tmp_path) + legacy = [_legacy_entry(11, "ghost", "gone")] + details = {"in.c-x.t1": _table_detail(columns=["c1"], metadata=legacy)} + + store_client = self._client([_listing_row("in.c-x.t1", legacy)], details) + service = _make_service(store, store_client) + result = service.describe_migrate(alias="prod") + store_client.delete_table_metadata.assert_not_called() + assert result["skipped"] == [ + {"table_id": "in.c-x.t1", "column": "ghost", "reason": "orphan", "legacy": "gone"} + ] + assert result["pruned_orphans"] == [] + + prune_client = self._client([_listing_row("in.c-x.t1", legacy)], details) + service = _make_service(store, prune_client) + result = service.describe_migrate(alias="prod", prune_orphans=True) + prune_client.delete_table_metadata.assert_called_once_with("in.c-x.t1", 11, branch_id=None) + assert result["pruned_orphans"] == [{"table_id": "in.c-x.t1", "column": "ghost"}] + + def test_describe_migrate_error_accumulation(self, tmp_path: Path) -> None: + store = _make_store(tmp_path) + legacy = [_legacy_entry(7, "c2", "old")] + mock_client = self._client( + [_listing_row("in.c-x.a", legacy), _listing_row("in.c-x.b", legacy)], + { + "in.c-x.a": _table_detail(columns=["c1", "c2"], metadata=legacy), + "in.c-x.b": _table_detail(columns=["c1", "c2"], metadata=legacy), + }, + ) + mock_client.update_table_definition.side_effect = [ + KeboolaApiError( + message="boom", status_code=500, error_code="SERVER_ERROR", retryable=False + ), + _JOB_OK, + ] + service = _make_service(store, mock_client) + + result = service.describe_migrate(alias="prod") + + assert result["tables_migrated"] == 1 + assert len(result["errors"]) == 1 + assert result["errors"][0]["table_id"] == "in.c-x.a" + assert "boom" in result["errors"][0]["error"] + assert [m["table_id"] for m in result["migrated"]] == ["in.c-x.b"] + + def test_describe_migrate_progress_callback(self, tmp_path: Path) -> None: + store = _make_store(tmp_path) + legacy = [_legacy_entry(7, "c2", "old")] + mock_client = self._client( + [_listing_row("in.c-x.t1", legacy), _listing_row("in.c-x.t2")], + { + "in.c-x.t1": _table_detail(columns=["c1", "c2"], metadata=legacy), + "in.c-x.t2": _table_detail(columns=["c1"]), + }, + ) + service = _make_service(store, mock_client) + seen: list[tuple[str, int, int]] = [] + + service.describe_migrate( + alias="prod", + dry_run=True, + progress_callback=lambda tid, cur, total: seen.append((tid, cur, total)), + ) + + assert seen == [("in.c-x.t1", 1, 2), ("in.c-x.t2", 2, 2)] + + +class TestGetTableDetailDescriptionPrecedence: + """Read path: native definition -> columnMetadata KBC.description -> legacy flat key.""" + + def test_table_detail_native_definition_wins(self, tmp_path: Path) -> None: + store = _make_store(tmp_path) + mock_client = MagicMock() + mock_client.get_table_detail.return_value = _table_detail( + columns=["c1"], + metadata=[_legacy_entry(1, "c1", "legacy")], + column_metadata={"c1": [{"key": "KBC.description", "value": "meta"}]}, + definition={"columns": [{"name": "c1", "definition": {"description": "native"}}]}, + ) + service = _make_service(store, mock_client) + + result = service.get_table_detail(alias="prod", table_id="in.c-sales.orders") + + col_map = {c["name"]: c for c in result["column_details"]} + assert col_map["c1"]["description"] == "native" + # The stale flat key is still present on the table -> flagged for migration. + assert result["legacy_column_descriptions"] == ["c1"] + + def test_table_detail_column_metadata_fallback(self, tmp_path: Path) -> None: + store = _make_store(tmp_path) + mock_client = MagicMock() + mock_client.get_table_detail.return_value = _table_detail( + columns=["c1"], + column_metadata={"c1": [{"key": "KBC.description", "value": "meta"}]}, + ) + service = _make_service(store, mock_client) + + result = service.get_table_detail(alias="prod", table_id="in.c-sales.orders") + + col_map = {c["name"]: c for c in result["column_details"]} + assert col_map["c1"]["description"] == "meta" + assert result["legacy_column_descriptions"] == [] + + def test_table_detail_legacy_fallback_and_warning_key(self, tmp_path: Path) -> None: + store = _make_store(tmp_path) + mock_client = MagicMock() + mock_client.get_table_detail.return_value = _table_detail( + columns=["c1"], + metadata=[_legacy_entry(1, "c1", "legacy only")], + ) + service = _make_service(store, mock_client) + + result = service.get_table_detail(alias="prod", table_id="in.c-sales.orders") + + col_map = {c["name"]: c for c in result["column_details"]} + assert col_map["c1"]["description"] == "legacy only" + assert result["legacy_column_descriptions"] == ["c1"] + + def test_table_detail_alias_source_metadata(self, tmp_path: Path) -> None: + """Alias tables inherit the source table's columnMetadata (MCP-server parity).""" + store = _make_store(tmp_path) + mock_client = MagicMock() + mock_client.get_table_detail.return_value = _table_detail( + columns=["c1"], + isAlias=True, + sourceTable={ + "id": "in.c-src.orders", + "columnMetadata": {"c1": [{"key": "KBC.description", "value": "from source"}]}, + }, + ) + service = _make_service(store, mock_client) + + result = service.get_table_detail(alias="prod", table_id="in.c-sales.orders") + + col_map = {c["name"]: c for c in result["column_details"]} + assert col_map["c1"]["description"] == "from source" + assert result["legacy_column_descriptions"] == [] + + def test_table_detail_alias_source_definition(self, tmp_path: Path) -> None: + """Alias tables without their own definition read the source table's.""" + store = _make_store(tmp_path) + mock_client = MagicMock() + mock_client.get_table_detail.return_value = _table_detail( + columns=["c1"], + isAlias=True, + sourceTable={ + "id": "in.c-src.orders", + "definition": { + "columns": [{"name": "c1", "definition": {"description": "src native"}}] + }, + }, + ) + service = _make_service(store, mock_client) + + result = service.get_table_detail(alias="prod", table_id="in.c-sales.orders") + + col_map = {c["name"]: c for c in result["column_details"]} + assert col_map["c1"]["description"] == "src native" + + def test_table_detail_no_descriptions_empty_legacy_list(self, tmp_path: Path) -> None: + """The key is always present so callers never need a .get() guard.""" + store = _make_store(tmp_path) + mock_client = MagicMock() + mock_client.get_table_detail.return_value = _table_detail(columns=["c1"]) + service = _make_service(store, mock_client) + + result = service.get_table_detail(alias="prod", table_id="in.c-sales.orders") + + assert result["legacy_column_descriptions"] == [] + assert "description" not in result["column_details"][0] From ab443c40b455b8abbebe4cb49da0bc29dfb5af70 Mon Sep 17 00:00:00 2001 From: Petr Date: Fri, 21 Aug 2026 23:27:11 +0200 Subject: [PATCH 3/4] docs: sync all surfaces for native column descriptions + 0.88.0 (#624) Version 0.88.0 + changelog; AGENT_CONTEXT, CLAUDE.md command list, commands-reference, gotchas (since v0.88.0), storage-describe-workflow, regenerated SKILL.md decision table, keboola-expert one-line trigger (61991/62000 bytes). --- CLAUDE.md | 17 +++ plugins/kbagent/agents/keboola-expert.md | 2 + .../kbagent/references/commands-reference.md | 5 +- .../skills/kbagent/references/gotchas.md | 37 +++-- .../references/storage-describe-workflow.md | 135 ++++++++++++++++-- src/keboola_agent_cli/changelog.py | 44 ++++++ src/keboola_agent_cli/commands/context.py | 15 +- 7 files changed, 228 insertions(+), 27 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 7c93533d..3df718d1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -469,6 +469,23 @@ kbagent storage describe-bucket --project NAME --bucket-id ID [--text STR | --fi kbagent storage describe-table --project NAME --table-id ID [--text STR | --file PATH | --stdin] [--branch ID] kbagent storage describe-column --project NAME --table-id ID --column NAME=DESC [--column ...] [--branch ID] kbagent storage describe-batch --project NAME --from-file YAML [--branch ID] +kbagent storage describe-migrate --project ALIAS [--table-id ID ...] [--bucket-id ID] [--prune-orphans] [--dry-run] [--yes] [--branch ID] +# Column descriptions (0.88.0+, #624): describe-column/describe-batch write through the native +# `PUT .../tables/{id}/definition` endpoint (async job) with `isDescriptionSystemManaged: false` +# (stops the next Output Mapping run from overwriting the text). The backend mirrors the value into +# `columnMetadata` `KBC.description`, so the UI, the MCP server and the Snowflake COMMENT / +# BigQuery description all see it. Before 0.88.0 kbagent wrote a flat `KBC.column.{name}.description` +# key on the TABLE's metadata -- read by nothing but kbagent, so documented columns looked blank +# everywhere else. Unknown column names now FAIL FAST before any write (behavior change; the flat +# write accepted typos silently). `table-detail` reads with precedence native definition -> +# columnMetadata KBC.description -> legacy flat key, always returns `legacy_column_descriptions` +# and warns in human mode when legacy keys remain; it never writes. `describe-migrate` converts +# legacy keys in bulk (scope: --table-id / --bucket-id / whole project; scan-then-confirm, +# --dry-run reports only; per-table errors accumulate). describe-column/describe-batch also migrate +# leftovers on the table they touch. Rules: a column whose visible description already differs is +# skipped as `conflict` (newer value wins), an entry for a dropped column is skipped as `orphan` +# unless --prune-orphans. Migrated flat entries are DELETED so a later clear cannot be resurrected +# by the read fallback. kbagent storage files --project NAME [--tag TAG ...] [--limit N] [--offset N] [--query Q] [--branch ID] kbagent storage file-upload --project NAME --file PATH [--name NAME] [--tag TAG ...] [--permanent] [--branch ID] kbagent storage file-download --project NAME [--file-id ID | --tag TAG ...] [--output FILE] diff --git a/plugins/kbagent/agents/keboola-expert.md b/plugins/kbagent/agents/keboola-expert.md index 53668d4c..3f4f0558 100644 --- a/plugins/kbagent/agents/keboola-expert.md +++ b/plugins/kbagent/agents/keboola-expert.md @@ -250,6 +250,8 @@ its absence is NOT a promise the entry is version-independent (see §1 Rule 6). `--name` (the API rejects empty) and fails on an existing table name -- restore under a new name, verify, then `swap-tables`. `snapshot-delete` only forecloses restores; the source table is untouched. +- **Column descriptions** (0.88.0+, #624): native endpoint; legacy + `KBC.column.*` invisible to UI/MCP; `describe-migrate`. gotchas.md. - **`bucket-detail` is dialect-aware**: read `sql_dialect` + per-table `sql_path` (already correctly quoted) -- don't branch on the backend yourself. diff --git a/plugins/kbagent/skills/kbagent/references/commands-reference.md b/plugins/kbagent/skills/kbagent/references/commands-reference.md index c74d1b51..13b1a787 100644 --- a/plugins/kbagent/skills/kbagent/references/commands-reference.md +++ b/plugins/kbagent/skills/kbagent/references/commands-reference.md @@ -178,8 +178,9 @@ Requires a **super-admin** Manage API token (same kind as `org setup`). Same def - `storage snapshot-delete --project NAME --snapshot-id ID [--snapshot-id ...] [--dry-run] [--yes]` (since v0.75.0) -- delete snapshots (destructive: forecloses restores; source tables untouched). Batch-tolerant, exit 1 when any ID failed - `storage describe-bucket --project NAME --bucket-id ID [--text STR | --file PATH | --stdin] [--branch ID]` -- set a bucket description (stored as `KBC.description` in bucket metadata, upsert). Provide exactly one of `--text`, `--file`, `--stdin`. Read back via `storage bucket-detail` - `storage describe-table --project NAME --table-id ID [--text STR | --file PATH | --stdin] [--branch ID]` -- set a table description (stored as `KBC.description` in table metadata, upsert). Provide exactly one of `--text`, `--file`, `--stdin`. Read back via `storage table-detail` -- `storage describe-column --project NAME --table-id ID --column NAME=DESCRIPTION [--column ...] [--branch ID]` -- set one or more column descriptions. Stored as `KBC.column.{name}.description` keys in the table's metadata (Keboola has no user-writable column-metadata endpoint). Read back in `storage table-detail` under `column_details[].description` -- `storage describe-batch --project NAME --from-file PATH [--branch ID]` -- apply bucket/table/column descriptions from a YAML file (top-level `buckets`, `tables`, `columns` sections, all optional). Partial-failure tolerant: per-item errors are collected and reported, the batch does not abort. Non-zero exit only when at least one item failed +- `storage describe-column --project NAME --table-id ID --column NAME=DESCRIPTION [--column ...] [--branch ID]` -- set one or more column descriptions. *(since v0.88.0)* Writes through the native `PUT /v2/storage/branch/{branch}/tables/{id}/definition` endpoint (the one the web UI uses; async `tableDefinitionUpdate` storage job) with `isDescriptionSystemManaged: false`, so the next component run's Output Mapping cannot overwrite the text. The backend mirrors the value into `columnMetadata` `KBC.description`, so the Keboola UI, the MCP server (`get_tables`) and the Snowflake `COMMENT` / BigQuery column description all see it. Unknown column names are rejected BEFORE any write (behavior change -- the pre-0.88.0 flat-metadata write accepted typos silently). Legacy flat `KBC.column.{name}.description` entries on the same table are migrated in the same write and then deleted. Read back in `storage table-detail` under `column_details[].description` +- `storage describe-batch --project NAME --from-file PATH [--branch ID]` -- apply bucket/table/column descriptions from a YAML file (top-level `buckets`, `tables`, `columns` sections, all optional). Column items go through the same native write (and same fail-fast + auto-migration) as `describe-column`. Partial-failure tolerant: per-item errors are collected and reported, the batch does not abort. Non-zero exit only when at least one item failed +- `storage describe-migrate --project ALIAS [--table-id ID ...] [--bucket-id ID] [--prune-orphans] [--dry-run] [--yes] [--branch ID]` *(since v0.88.0)* -- bulk-convert legacy pre-0.88.0 flat `KBC.column.*.description` metadata to the native definition endpoint. Scope is explicit `--table-id` (repeatable), a single `--bucket-id`, or every table in the project; the two scope flags are mutually exclusive (exit 2). Scans first and prints the summary, then asks for confirmation -- `--dry-run` reports without writing, `--yes` skips the prompt. A column whose currently visible description already differs is skipped as `conflict` (the newer value wins); an entry for a column that no longer exists is skipped as `orphan` unless `--prune-orphans` deletes it. Migrated flat entries are deleted after a successful write, so a later `describe-column` clearing the text cannot be resurrected by the read fallback. Per-table failures are accumulated into `errors[]` and never abort the run. Permission class `write` ## Storage Files - `storage files --project NAME [--tag TAG ...] [--limit N] [--offset N] [--query Q] [--branch ID]` -- list Storage Files, optionally filtered by tag/query diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index fdc40340..8e192246 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -2648,15 +2648,34 @@ so setting a branch's description will **not** update the dashboard. `kbagent storage describe-bucket / describe-table / describe-column / describe-batch` write descriptive metadata onto storage objects. Three behaviors are easy to miss: -- **Column descriptions use a metadata-key convention, not a column endpoint.** - The Keboola Storage API has no user-writable column-level metadata endpoint, - so `describe-column` stores each description as a `KBC.column.{name}.description` - entry on the **table's** metadata (upsert). `storage table-detail` reads them - back via the same key and surfaces them under `column_details[].description`. - Renaming or deleting a column does NOT automatically clean these entries up - (they remain on the table's metadata under the old name). Same convention for - table and bucket descriptions: stored as `KBC.description` (provider=user) on - the object's metadata. +- **Column descriptions go through the native definition endpoint** *(since + v0.88.0)*. `describe-column` / `describe-batch` write via + `PUT /v2/storage/branch/{branch}/tables/{id}/definition` -- the endpoint the + web UI uses (async `tableDefinitionUpdate` storage job) -- with + `isDescriptionSystemManaged: false`, which is what stops the next component + run's Output Mapping from overwriting a hand-authored description. The backend + mirrors the written value into `columnMetadata` `KBC.description` for typed AND + untyped tables, so one write is visible to the Keboola UI, to the MCP server + (`get_tables`), and in the Snowflake `COMMENT` / BigQuery column description. + *Pre-0.88.0 behaviour:* kbagent stored each description as a flat + `KBC.column.{name}.description` entry on the **table's** metadata. Nothing but + kbagent itself ever read that key -- columns documented that way look blank in + the UI, are invisible to the MCP server, and never reach the warehouse. That + mirroring is one-way: a `POST .../metadata` write never reaches the native + field. Convert leftovers with `kbagent storage describe-migrate` (bulk, + scan-then-confirm, `--dry-run` first); `describe-column` / `describe-batch` + also migrate remaining legacy entries on whatever table they touch. A column + whose visible description already differs is skipped as `conflict` (newer value + wins), an entry for a dropped column is skipped as `orphan` unless + `--prune-orphans`. **Migrated flat entries are DELETED** -- that is deliberate: + leaving them would let the read fallback resurrect an old description after + someone clears the column's text. `table-detail` reads with the precedence + native definition -> `columnMetadata` `KBC.description` -> legacy flat key, + always returns `legacy_column_descriptions`, and warns in human mode when + legacy keys remain (it never writes -- safe under a read-only token or + `--deny-writes`). Unknown column names now fail fast BEFORE any write; the old + flat write accepted typos silently. Table and bucket descriptions are + unaffected: still `KBC.description` (provider=user) on the object's metadata. - **`describe-batch` is partial-failure-tolerant.** Item-level errors are collected into `result.errors[]` but the batch keeps processing the remaining items. The CLI exits non-zero only if `error_count > 0`, so in scripts always diff --git a/plugins/kbagent/skills/kbagent/references/storage-describe-workflow.md b/plugins/kbagent/skills/kbagent/references/storage-describe-workflow.md index de12e37e..e3722247 100644 --- a/plugins/kbagent/skills/kbagent/references/storage-describe-workflow.md +++ b/plugins/kbagent/skills/kbagent/references/storage-describe-workflow.md @@ -3,9 +3,10 @@ `kbagent storage describe-*` attaches human-readable descriptions to storage buckets, tables, and columns so that downstream consumers (dashboards, the Keboola UI, `kbagent storage buckets`/`tables`, AI agents) can surface -meaningful documentation rather than raw IDs. Descriptions are stored as metadata on -the storage object and round-trip via `storage bucket-detail` / `storage -table-detail`. +meaningful documentation rather than raw IDs. Bucket and table descriptions are +stored as metadata on the storage object; column descriptions are written to the +table's native definition *(since v0.88.0)*. All of them round-trip via +`storage bucket-detail` / `storage table-detail`. ## Quick reference @@ -15,6 +16,7 @@ table-detail`. | `storage describe-table` | Set a table description | | `storage describe-column` | Set descriptions on one or more columns | | `storage describe-batch` | Apply bucket/table/column descriptions from a YAML file | +| `storage describe-migrate` | Convert legacy pre-0.88.0 `KBC.column.*` metadata to the native endpoint | | `storage bucket-detail` | Read back the bucket description | | `storage table-detail` | Read back the table description and `column_details[].description` | @@ -31,18 +33,58 @@ table-detail`. ## Storage model (what actually gets written) -Descriptions are stored as metadata entries on the object: +Bucket and table descriptions are metadata entries on the object; column +descriptions are a native field on the table definition: - **Bucket description** -- `KBC.description` (provider=user) on bucket metadata - **Table description** -- `KBC.description` (provider=user) on table metadata -- **Column description** -- `KBC.column.{column_name}.description` on the - **table's** metadata. Keboola has no user-writable column-metadata endpoint, - so this key convention is the storage layer for column descriptions. Read - them back via `storage table-detail` (`column_details[].description`). +- **Column description** *(since v0.88.0)* -- written through + `PUT /v2/storage/branch/{branch}/tables/{table_id}/definition`, the same + endpoint the Keboola web UI uses. The call is asynchronous (a + `tableDefinitionUpdate` storage job; kbagent polls it to completion) and always + carries `isDescriptionSystemManaged: false`, which is what prevents the next + component run's Output Mapping from overwriting a hand-authored description. + The backend mirrors the value into `columnMetadata[{column}]` + `KBC.description` for typed AND untyped tables, so a single write is visible + to the UI, to the MCP server, and in the Snowflake `COMMENT` / BigQuery column + description. Read them back via `storage table-detail` + (`column_details[].description`). Descriptions are `upsert`: calling `describe-*` with a new text replaces whatever was there before. There is no append mode. +### Legacy convention (pre-0.88.0) and how to get rid of it + +Before 0.88.0 kbagent stored each column description as a flat +`KBC.column.{column_name}.description` entry on the **table's** metadata. That +key was read by nothing except kbagent itself -- columns documented that way +appear blank in the Keboola UI, are invisible to the MCP server, and never reach +the warehouse. The mirroring is one-way, so a metadata write can never reach the +native field. + +Existing entries are not lost. `storage table-detail` still reads them (last in +the precedence chain below) and reports them in `legacy_column_descriptions`; +`storage describe-migrate` converts them in bulk, and `describe-column` / +`describe-batch` convert whatever is left on the table they touch as part of the +same write. Migrated flat entries are **deleted** after a successful write -- +otherwise clearing a column description later would be silently undone by the +read fallback resurrecting the old value. + +### Read precedence (`storage table-detail`) + +For each column, the description is resolved in this order: + +1. the native `definition.columns[].definition.description` field +2. `columnMetadata[{column}]` entry with key `KBC.description` (for an alias + table, the source table's `columnMetadata` is consulted when the alias itself + has none -- parity with the MCP server) +3. the legacy flat `KBC.column.{column}.description` table-metadata entry + +`table-detail` always returns `legacy_column_descriptions` (the columns still +backed by convention 3; an empty list when there are none) and prints a warning +in human mode when it is non-empty. Reading never writes, so it is safe with a +read-only token or under `--deny-writes`. + ## Single-item: bucket ```bash @@ -106,10 +148,16 @@ kbagent --json storage describe-column \ --column "created_at=Server-side creation timestamp (UTC)" ``` -Column descriptions live under `KBC.column.{name}.description` on the -**table's** metadata -- they are NOT attached to the column record itself. -If you rename or delete a column, the old key lingers until you manually -clean it up (there is no `--delete-column-description` command today). +All requested column names are validated against the table BEFORE anything is +written *(since v0.88.0)*: a name that is not on the table aborts the command +with a usage error naming it. Pre-0.88.0 a typo was accepted and produced a +metadata entry nothing could ever read, which looked like a success. + +The write is one asynchronous storage job per call, so a `describe-column` with +several `--column` flags is still a single roundtrip. If the table still carries +legacy `KBC.column.*` entries for OTHER columns, they are folded into the same +write and their flat keys deleted afterwards (conflicting and orphaned entries +are reported as skipped instead -- see `describe-migrate` below). Read back via `storage table-detail`: @@ -214,6 +262,53 @@ The CLI exits **1** when `error_count > 0`. In scripts, always inspect the without issues (it means there were no partial failures). A non-zero exit means *some* items failed; the successful items still landed. +## Migrating legacy column descriptions (since v0.88.0) + +A project that was documented with kbagent 0.87.0 or older still has its column +descriptions in the invisible flat convention. Find them with `table-detail`: + +```bash +kbagent --json storage table-detail --project ALIAS --table-id in.c-sales.orders \ + | jq '.data.legacy_column_descriptions' +``` + +Convert them with `storage describe-migrate`. Always scan first: + +```bash +# 1. What would change? (no writes at all) +kbagent --json storage describe-migrate --project ALIAS --dry-run + +# 2. Narrow the scope if you want to go table by table or bucket by bucket +kbagent --json storage describe-migrate --project ALIAS --bucket-id in.c-sales --dry-run +kbagent --json storage describe-migrate --project ALIAS \ + --table-id in.c-sales.orders --table-id in.c-sales.customers --dry-run + +# 3. Apply (interactive confirm unless --yes) +kbagent --json storage describe-migrate --project ALIAS --bucket-id in.c-sales --yes +``` + +`--table-id` (repeatable) and `--bucket-id` are mutually exclusive; with neither, +every table in the project is scanned. Tables without legacy keys are skipped +silently and only counted. + +Per-column rules: + +- **conflict** -- the column already has a *different* visible description + (native field or `columnMetadata`). The legacy value is NOT written and its + flat key is NOT deleted; the entry is reported in `skipped[]` with both values + so you can decide. The newer, visible value wins by default. +- **orphan** -- the flat key names a column that no longer exists on the table. + Skipped and left in place unless you pass `--prune-orphans`, which deletes it. +- **identical** -- the visible description already matches the legacy value. + Nothing is written; the redundant flat key is deleted. +- otherwise the value is migrated, and the flat key is deleted after the write + succeeds. A failed cleanup is reported (`reason: "delete_failed"`) but never + fails the command -- the description is already durable. + +Per-table failures are collected into `errors[]` and never abort the run +(convention #11), so one inaccessible table does not stop a project-wide sweep. +Re-running is safe: a table with no legacy keys left is a no-op. + ## End-to-end example: onboarding a new bucket ```bash @@ -232,7 +327,10 @@ kbagent --json storage table-detail --project ALIAS --table-id in.c-sales.orders | jq '{description: .data.description, columns: .data.column_details}' ``` -## Precedence vs the native description field +## Precedence vs the native description field (buckets and tables) + +Columns follow their own chain -- see "Read precedence" above; this section is +about the bucket-level and table-level description only. The Storage API has a native `description` field on buckets and tables, but it is only settable at creation time. Anything you set with `describe-*` @@ -246,8 +344,15 @@ entries with `provider="user"` are considered the canonical description. ## Key behaviors - `describe-*` is **upsert** -- no append mode; re-running replaces the value. -- Column descriptions piggy-back on table metadata via the - `KBC.column.{name}.description` key convention. +- Column descriptions go through the native `.../tables/{id}/definition` + endpoint *(since v0.88.0)* with `isDescriptionSystemManaged: false`; the + backend mirrors them into `columnMetadata` `KBC.description`, so the UI, the + MCP server and the warehouse all see them. Legacy flat + `KBC.column.{name}.description` keys are read as a last-resort fallback, + reported in `legacy_column_descriptions`, and converted by + `storage describe-migrate`. +- Unknown column names abort `describe-column` / `describe-batch` before any + write *(since v0.88.0)*. - `describe-batch` is **partial-failure-tolerant** -- check `errors[]` even on exit code 0. - All commands support `--branch ID` to target a dev branch. diff --git a/src/keboola_agent_cli/changelog.py b/src/keboola_agent_cli/changelog.py index c131ba3c..14ebd132 100644 --- a/src/keboola_agent_cli/changelog.py +++ b/src/keboola_agent_cli/changelog.py @@ -52,6 +52,50 @@ "touching the prompt pay for an unrelated trim first. `AGENT_CONTEXT` remains the " "place for exhaustive per-command detail, and splitting the prompt into per-domain " "specialists remains the answer to sustained growth.", + "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` / " + "`describe-batch` stored each description as a flat " + "`KBC.column.{name}.description` entry on the TABLE's metadata -- a convention " + "nothing but kbagent itself ever read, so a documented column looked blank in " + "the UI, was invisible to `get_tables` in the MCP server, and never reached the " + "Snowflake `COMMENT` / BigQuery column description. The write now goes through " + "the native `PUT /v2/storage/branch/{branch}/tables/{id}/definition` endpoint " + "(the same one the web UI uses; async, waits for the `tableDefinitionUpdate` " + "storage job), and the backend mirrors the value into `columnMetadata` " + "`KBC.description` for typed AND untyped tables -- one write, visible " + "everywhere. `description: null` clears a column description.", + "New (#624): every native write sets `isDescriptionSystemManaged: false`. " + "That flag is what stops the next component run's Output Mapping from " + "overwriting a hand-authored description (DMD-1662). The old metadata-key " + "write had no such protection.", + "New (#624): `kbagent storage describe-migrate` converts legacy flat " + "`KBC.column.*` entries to the native endpoint in bulk. Full usage: " + "`describe-migrate --project ALIAS [--table-id ID ...] [--bucket-id ID] " + "[--prune-orphans] [--dry-run] [--yes] [--branch ID]`. Scope is " + "explicit tables, one bucket, or the whole project; it scans first and asks for " + "confirmation before writing (`--dry-run` reports without touching anything), and " + "per-table failures are accumulated instead of aborting the run. A column that " + "already carries a different visible description is skipped as a `conflict` " + "(newer value wins), and an entry for a column that no longer exists is skipped " + "as an `orphan` unless `--prune-orphans` is passed. `describe-column` / " + "`describe-batch` also migrate any remaining legacy entries on the table they " + "touch, as part of the same write. Migrated flat entries are DELETED afterwards, " + "so clearing a description later cannot be silently resurrected by the read " + "fallback.", + "New (#624): `storage table-detail` now reads column descriptions written by " + "the UI or by a component, not only kbagent's own. Resolution precedence is " + "native definition -> `columnMetadata` `KBC.description` -> legacy " + "flat `KBC.column.*` (alias tables fall back to the source table's " + "`columnMetadata`, matching the MCP server). The response always carries a " + "`legacy_column_descriptions` list naming the columns still backed by the legacy " + "convention; human output prints a warning pointing at `describe-migrate`. " + "Reading never writes -- a read-only token or a `--deny-writes` session is " + "unaffected.", + "Change (#624): `describe-column` / `describe-batch` now REJECT a column name " + "that does not exist on the table, before any write. The old flat-metadata write " + "accepted anything and silently created an entry nothing could ever read, so a " + "typo looked like a success. Scripts that relied on that behaviour will now get a " + "usage error naming the unknown columns.", ], "0.87.0": [ "New (#626): `data-app create` gains `--workspace / --no-workspace` and grants " diff --git a/src/keboola_agent_cli/commands/context.py b/src/keboola_agent_cli/commands/context.py index b69e7df2..e693275d 100644 --- a/src/keboola_agent_cli/commands/context.py +++ b/src/keboola_agent_cli/commands/context.py @@ -733,13 +733,26 @@ Set the KBC.description metadata on a table (upsert). Readable via table-detail --json .data.description. kbagent storage describe-column --project NAME --table-id ID --column NAME=DESC [--column ...] [--branch ID] - Set per-column descriptions stored as KBC.column.{{name}}.description in table metadata (upsert). + Set per-column descriptions via the native PUT .../tables/{{id}}/definition endpoint (0.88.0+, upsert, + async storage job). Writes isDescriptionSystemManaged=false so the next component run's Output Mapping + cannot overwrite the text; the backend mirrors the value into columnMetadata KBC.description, so the + Keboola UI, the MCP server and the warehouse COMMENT all see it. Unknown column names fail fast before + any write. Legacy flat KBC.column.*.description entries on the same table are migrated in the same write + (conflict/orphan entries skipped) and the migrated flat keys deleted. Readable via table-detail --json .data.column_details[].description. kbagent storage describe-batch --project NAME --from-file YAML [--branch ID] Apply bucket/table/column descriptions from a YAML file. Sections: buckets, tables, columns (all optional). + Columns go through the same native write as describe-column. Failures collected; one error does not abort remaining items. + kbagent storage describe-migrate --project ALIAS [--table-id ID ...] [--bucket-id ID] [--prune-orphans] [--dry-run] [--yes] [--branch ID] + Bulk-convert legacy pre-0.88.0 flat KBC.column.*.description metadata to the native endpoint. + Scope: explicit --table-id (repeatable), one --bucket-id, or the whole project. Scans and prints a + summary, then asks for confirmation (--dry-run reports only; --yes skips the prompt). A column whose + visible description already differs is skipped as "conflict"; an entry for a dropped column is skipped + as "orphan" unless --prune-orphans. Per-table errors are accumulated, never abort the run. + ### Storage Files kbagent storage files --project NAME [--tag TAG ...] [--limit N] [--offset N] [--query Q] [--branch ID] From 64c915d1c4fb021d086e8a084e3d329ee65a9106 Mon Sep 17 00:00:00 2001 From: Petr Date: Fri, 21 Aug 2026 23:45:57 +0200 Subject: [PATCH 4/4] fix(storage): mirror describe-migrate on serve; guard positive-only column patch (#624) Review follow-ups on PR #631. - serve: add POST /storage/columns/{project}/describe-migrate, the 1:1 mirror CONTRIBUTING.md requires (its three describe-* siblings already had routes, so external callers got a 404). No `yes` field -- a REST caller opting into the write IS the confirmation; `dry_run` is the shared preview. Two router tests cover scope/flag forwarding and the empty-body default. - e2e: assert a sibling column keeps its description across a partial write. describe-batch sends `id` alone, so `name` (described two steps earlier) must survive; the PUT .../definition `columns` payload is a positive-only patch and nothing asserted that before. - e2e: seed the legacy migration key on `value`, the one undescribed column -- the real pre-0.88.0 shape. Seeding it on `name` made the live run report `conflict` (correctly: the column already had a native description), which the old assertion mistook for a migration failure. The conflict rule now has its own explicit assertions. --- .../server/routers/storage.py | 30 ++++++++++++ tests/test_e2e.py | 47 +++++++++++++++++-- tests/test_server_router_calls.py | 47 +++++++++++++++++++ 3 files changed, 120 insertions(+), 4 deletions(-) diff --git a/src/keboola_agent_cli/server/routers/storage.py b/src/keboola_agent_cli/server/routers/storage.py index 63a4bbad..65323c9d 100644 --- a/src/keboola_agent_cli/server/routers/storage.py +++ b/src/keboola_agent_cli/server/routers/storage.py @@ -80,6 +80,19 @@ class DescribeColumns(BaseModel): branch_id: int | None = None +class DescribeMigrate(BaseModel): + # Scope: explicit tables, one bucket, or (both unset) the whole project. + # Mutually exclusive -- the service raises on both being set. + table_ids: list[str] | None = None + bucket_id: str | None = None + prune_orphans: bool = False + # No `yes` field: the CLI's confirm prompt is a terminal affordance, and a + # REST caller opting into the write IS the confirmation. `dry_run` is the + # preview both surfaces share. + dry_run: bool = False + branch_id: int | None = None + + class TagFile(BaseModel): add: list[str] | None = None remove: list[str] | None = None @@ -557,6 +570,23 @@ def describe_columns( ) +@router.post("/columns/{project}/describe-migrate", summary="Migrate legacy column descriptions") +def describe_migrate( + project: str, + body: DescribeMigrate, + registry: ServiceRegistry = Depends(get_registry), +) -> dict[str, Any]: + """Convert legacy `KBC.column.*` descriptions. Mirrors `kbagent storage describe-migrate`.""" + return registry.storage.describe_migrate( + alias=project, + table_ids=body.table_ids, + bucket_id=body.bucket_id, + prune_orphans=body.prune_orphans, + dry_run=body.dry_run, + branch_id=body.branch_id, + ) + + # Registered AFTER the more specific /columns/.../describe route above: the # greedy {table_id:path} would otherwise shadow that POST and swallow a # ".../describe" suffix as part of the table id. diff --git a/tests/test_e2e.py b/tests/test_e2e.py index d641b974..635f845f 100644 --- a/tests/test_e2e.py +++ b/tests/test_e2e.py @@ -4007,6 +4007,12 @@ def _test_storage_describe(self, bucket_id: str, table_id: str) -> None: assert data["data"]["description"] == "Batch table desc" col_descs = {c["name"]: c.get("description", "") for c in data["data"]["column_details"]} assert col_descs.get("id") == "Batch column id desc" + # The `columns` payload of PUT .../definition is a POSITIVE-ONLY patch: + # the batch above sent `id` alone, so `name` -- described two steps + # earlier -- must survive untouched. The whole write path assumes this; + # a full-replace endpoint would silently wipe every column left out of + # the payload, and the assertion on `id` alone would never notice. + assert col_descs.get("name") == "Human-readable name" # The native write leaves no legacy flat keys behind (#624). assert data["data"]["legacy_column_descriptions"] == [] @@ -4019,16 +4025,22 @@ def _test_storage_describe_migrate(self, table_id: str) -> None: wrote before the native definition endpoint; nothing but kbagent ever read it. Seeding goes through the raw client on purpose -- no CLI command writes that shape any more. + + Seeded on ``value``, the one column the describe steps above leave + undescribed: that is the real pre-0.88.0 shape (a legacy key and + nothing else). ``id`` / ``name`` already carry native descriptions, + so a legacy key there is a *conflict*, which the second half of this + test covers separately. """ self.api.set_table_metadata( table_id=table_id, - entries=[("KBC.column.name.description", "Legacy column description")], + entries=[("KBC.column.value.description", "Legacy column description")], ) data = self._run_ok( "storage", "table-detail", "--project", self.alias, "--table-id", table_id ) - assert data["data"]["legacy_column_descriptions"] == ["name"] + assert data["data"]["legacy_column_descriptions"] == ["value"] # Dry run reports the table and writes nothing. data = self._run_ok( @@ -4043,7 +4055,7 @@ def _test_storage_describe_migrate(self, table_id: str) -> None: assert data["data"]["dry_run"] is True assert data["data"]["tables_migrated"] == 0 migrated = {item["table_id"]: item["columns"] for item in data["data"]["migrated"]} - assert migrated[table_id]["name"] == "Legacy column description" + assert migrated[table_id]["value"] == "Legacy column description" data = self._run_ok( "storage", @@ -4063,7 +4075,7 @@ def _test_storage_describe_migrate(self, table_id: str) -> None: ) assert data["data"]["legacy_column_descriptions"] == [] col_descs = {c["name"]: c.get("description", "") for c in data["data"]["column_details"]} - assert col_descs.get("name") == "Legacy column description" + assert col_descs.get("value") == "Legacy column description" # Re-running is a no-op: nothing left to migrate. data = self._run_ok( @@ -4077,6 +4089,33 @@ def _test_storage_describe_migrate(self, table_id: str) -> None: ) assert data["data"]["migrated"] == [] + # A legacy key on a column that ALREADY has a native description is a + # conflict: the newer (visible) value wins, the stale key is reported + # and left alone rather than overwriting what the UI shows. + self.api.set_table_metadata( + table_id=table_id, + entries=[("KBC.column.id.description", "Stale legacy id description")], + ) + data = self._run_ok( + "storage", + "describe-migrate", + "--project", + self.alias, + "--table-id", + table_id, + "--yes", + ) + assert data["data"]["migrated"] == [] + conflicts = [s for s in data["data"]["skipped"] if s["reason"] == "conflict"] + assert [s["column"] for s in conflicts] == ["id"] + + data = self._run_ok( + "storage", "table-detail", "--project", self.alias, "--table-id", table_id + ) + col_descs = {c["name"]: c.get("description", "") for c in data["data"]["column_details"]} + assert col_descs.get("id") == "Batch column id desc" + assert data["data"]["legacy_column_descriptions"] == ["id"] + def _test_semantic_layer_roundtrip(self) -> None: """Live roundtrip of the semantic-layer command group against ``self.alias``. diff --git a/tests/test_server_router_calls.py b/tests/test_server_router_calls.py index 74479042..7adc3d61 100644 --- a/tests/test_server_router_calls.py +++ b/tests/test_server_router_calls.py @@ -289,6 +289,53 @@ def test_storage_describe_columns_passes_columns_kwarg(tmp_path: Path) -> None: ) +# --------------------------------------------------------------------------- +# storage.py POST /columns/{p}/describe-migrate +# Service: storage.describe_migrate(...) -- the 1:1 mirror of the CLI command +# --------------------------------------------------------------------------- + + +def test_storage_describe_migrate_forwards_scope_and_flags(tmp_path: Path) -> None: + """Router must forward every scope/flag kwarg to StorageService.describe_migrate.""" + storage_svc = MagicMock() + storage_svc.describe_migrate.return_value = {"tables_migrated": 1} + registry = _mock_registry(storage=storage_svc) + app = _make_app_with_registry(tmp_path, registry) + + with TestClient(app) as client: + res = client.post( + f"/storage/columns/{PROJECT}/describe-migrate", + headers=AUTH, + json={"table_ids": [TABLE_ID], "prune_orphans": True, "dry_run": True}, + ) + + assert res.status_code == 200, res.text + kwargs = storage_svc.describe_migrate.call_args.kwargs + assert kwargs["alias"] == PROJECT + assert kwargs["table_ids"] == [TABLE_ID] + assert kwargs["bucket_id"] is None + assert kwargs["prune_orphans"] is True + assert kwargs["dry_run"] is True + + +def test_storage_describe_migrate_defaults_to_whole_project_write(tmp_path: Path) -> None: + """An empty body means whole-project scope and a real (non-dry-run) write.""" + storage_svc = MagicMock() + storage_svc.describe_migrate.return_value = {"tables_migrated": 0} + registry = _mock_registry(storage=storage_svc) + app = _make_app_with_registry(tmp_path, registry) + + with TestClient(app) as client: + res = client.post(f"/storage/columns/{PROJECT}/describe-migrate", headers=AUTH, json={}) + + assert res.status_code == 200, res.text + kwargs = storage_svc.describe_migrate.call_args.kwargs + assert kwargs["table_ids"] is None + assert kwargs["bucket_id"] is None + assert kwargs["dry_run"] is False + assert kwargs["prune_orphans"] is False + + # --------------------------------------------------------------------------- # storage.py POST /{p} (create table) # Service: storage.create_table(source_table_id=..., time_partitioning_*=...,