diff --git a/plugins/kbagent/skills/kbagent/references/commands-reference.md b/plugins/kbagent/skills/kbagent/references/commands-reference.md index 7baac186..4180012f 100644 --- a/plugins/kbagent/skills/kbagent/references/commands-reference.md +++ b/plugins/kbagent/skills/kbagent/references/commands-reference.md @@ -179,7 +179,7 @@ Requires a **super-admin** Manage API token (same kind as `org setup`). Same def - `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. *(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-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 for API errors: per-item errors are collected and reported, the batch does not abort. Non-zero exit only when at least one item failed. A malformed file (a section that is not a mapping of ID to description, a non-mapping `columns` entry, a document that is not a mapping) is rejected whole before the first write -- `INVALID_ARGUMENT`, exit 2, message naming the offending key and its actual type - `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 diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index 599b33f7..40b1562c 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -2699,7 +2699,14 @@ write descriptive metadata onto storage objects. Three behaviors are easy to mis items. The CLI exits non-zero only if `error_count > 0`, so in scripts always inspect `errors[]` (or at least `error_count`) rather than relying solely on the exit code — and when consuming `--json` output, never trust a zero-exit - as "everything applied." + as "everything applied." That tolerance covers **API** failures only: a + `--from-file` whose shape is wrong (a `tables:` / `buckets:` / `columns:` + section that is a list instead of a mapping of ID to description, a column + entry that is not a mapping, a document that is not a mapping at all) is a + usage error — the whole file is rejected before the first write with + `INVALID_ARGUMENT` and exit 2, naming the offending key and its actual type. + Nothing is half-applied. (Release step: once this ships, tag this sentence + `(since vX.Y.Z)` with the version that carried it.) - **Description-field precedence: metadata wins.** When both the native Storage API `description` field and a user-provided `KBC.description` (provider=user) metadata entry are present, `storage bucket-detail` / `storage table-detail` diff --git a/plugins/kbagent/skills/kbagent/references/storage-describe-workflow.md b/plugins/kbagent/skills/kbagent/references/storage-describe-workflow.md index e3722247..71e0e8e5 100644 --- a/plugins/kbagent/skills/kbagent/references/storage-describe-workflow.md +++ b/plugins/kbagent/skills/kbagent/references/storage-describe-workflow.md @@ -262,6 +262,14 @@ 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. +This tolerance applies to **API** failures. A malformed file is a usage error +instead: if a section is not a mapping of ID to description (a `tables:` list, +a scalar under a `columns:` table ID, a document that is not a mapping at all), +the whole file is rejected **before the first write** with +`INVALID_ARGUMENT` and exit **2**, and the message names the offending key plus +its actual type. Nothing is half-applied, so fixing the file and re-running is +always safe. + ## Migrating legacy column descriptions (since v0.88.0) A project that was documented with kbagent 0.87.0 or older still has its column diff --git a/src/keboola_agent_cli/commands/_storage_describe.py b/src/keboola_agent_cli/commands/_storage_describe.py index 9e10a259..7819f704 100644 --- a/src/keboola_agent_cli/commands/_storage_describe.py +++ b/src/keboola_agent_cli/commands/_storage_describe.py @@ -315,8 +315,14 @@ def storage_describe_batch( 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. + All sections are optional (absent or empty sections are skipped). + + A malformed file -- a section that is not a mapping of ID to + description, a null description, a non-mapping columns entry -- is + rejected before any write (INVALID_ARGUMENT, exit 2), naming the + offending key. Once application starts, a per-item API failure does + not abort the rest: those results are collected and reported, and the + command exits 1 if any item failed. """ formatter = get_formatter(ctx) service = get_service(ctx, "storage_service") diff --git a/src/keboola_agent_cli/commands/context.py b/src/keboola_agent_cli/commands/context.py index 1ddaff22..35f897af 100644 --- a/src/keboola_agent_cli/commands/context.py +++ b/src/keboola_agent_cli/commands/context.py @@ -742,9 +742,12 @@ 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. + Apply bucket/table/column descriptions from a YAML file. Sections: buckets, tables, columns (all optional; + absent or empty sections are skipped). Columns go through the same native write as describe-column. + File shape is validated BEFORE any write: a section that is not a mapping of ID to description, a null + description, or a non-mapping columns entry aborts with INVALID_ARGUMENT and exit 2, naming the offending + key -- nothing is half-applied. During application, per-item API failures are collected and reported + (one error does not abort the remaining items); the command exits 1 when error_count > 0. 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. diff --git a/src/keboola_agent_cli/services/_describe_batch_input.py b/src/keboola_agent_cli/services/_describe_batch_input.py new file mode 100644 index 00000000..ab96f0e9 --- /dev/null +++ b/src/keboola_agent_cli/services/_describe_batch_input.py @@ -0,0 +1,230 @@ +"""Shape validation for the `storage describe-batch --from-file` document. + +Lives outside ``storage_service.py`` because that module is over its +file-size budget (CONTRIBUTING.md > "File-size budgets"), and because parsing +plus validating user input carries no API orchestration -- it is exactly what +moves out. + +The documented contract is three optional top-level mappings (``buckets``, +``tables``, ``columns``), each keyed by ID. A document that got any of those +shapes wrong used to reach the write loop and die on ``.items()`` with an +``AttributeError`` -- a Rich traceback on stdout even under ``--json`` +(issue #640). Every check here runs BEFORE the first write, so a malformed +file is rejected whole rather than half-applied, and the message names the +offending key, its actual type, and a copy-pasteable example. + +Empty is not malformed +---------------------- +A section that is *absent*, ``None`` (a bare ``buckets:`` key), or an EMPTY +container (``[]``, ``''``, ``{}``) is an empty section, silently skipped. The +pre-validation code reached the same outcome through ``raw.get(key) or {}``, +and a file whose sections are generated (a templating step that emitted no +rows) must keep being a no-op rather than an exit-2 failure. Only a NON-EMPTY +wrong shape, and any other scalar (``false``, ``0``, a non-empty string), is +an error -- those carry content that would be silently dropped. The same rule +applies one level down to a table's column mapping. + +A ``None`` *description*, on the other hand, is an error: ``str(None)`` used +to write the literal text "None" onto the object. An empty mapping has nothing +to say; a described object with no description is a mistake. + +``ValueError`` is the error type the command already maps to +``ErrorCode.INVALID_ARGUMENT`` + exit 2 (see ``commands/_storage_describe.py``), +so raising it here needs no new wiring at the command layer. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from pathlib import Path +from typing import Any, NamedTuple + +# YAML-flavoured names for the types a section can wrongly be: the author is +# reading their own YAML, not Python, so "string"/"number" beat "str"/"int". +_TYPE_NAMES: dict[type, str] = { + bool: "boolean", + dict: "mapping", + float: "number", + int: "number", + list: "list", + str: "string", + type(None): "null", +} + + +class _Section(NamedTuple): + """What one top-level section must be, and how to write it correctly. + + Subject and example live together so an added or reworded section cannot + end up quoting one section's rule beside another's example. + """ + + subject: str + example: str + + +_SECTIONS: dict[str, _Section] = { + "buckets": _Section( + subject="a mapping of bucket ID to description", + example="buckets:\n in.c-sales: All sales data", + ), + "tables": _Section( + subject="a mapping of table ID to description", + example="tables:\n in.c-sales.orders: All sales orders", + ), + "columns": _Section( + subject="a mapping of table ID to a column mapping", + example="columns:\n in.c-sales.orders:\n order_id: Unique order ID", + ), +} + +_COLUMN_ENTRY_SUBJECT = "a mapping of column name to description" +_DESCRIPTION_SUBJECT = "a description string" +_TOP_LEVEL_SUBJECT = "a YAML mapping of 'buckets' / 'tables' / 'columns' sections" +_TOP_LEVEL_EXAMPLE = "\n".join(section.example for section in _SECTIONS.values()) + + +@dataclass(frozen=True) +class DescribeBatchInput: + """The three validated sections of a describe-batch document. + + Every description is already coerced to ``str`` here, so the write loop + can hand values straight to the API without re-checking their type. + """ + + buckets: dict[str, str] = field(default_factory=dict) + tables: dict[str, str] = field(default_factory=dict) + columns: dict[str, dict[str, str]] = field(default_factory=dict) + + @property + def total(self) -> int: + """Number of items across all sections (the progress-callback total).""" + return len(self.buckets) + len(self.tables) + len(self.columns) + + +def _type_name(value: Any) -> str: + """Return ``value``'s type named in YAML vocabulary.""" + return _TYPE_NAMES.get(type(value), type(value).__name__) + + +def _shape_error(key: str, expected: str, value: Any, example: str) -> ValueError: + """Build the ValueError for one wrongly-shaped key.""" + return ValueError( + f"'{key}' must be {expected}, got a {_type_name(value)}. Expected:\n{example}" + ) + + +def _is_empty(value: Any) -> bool: + """True for the values that mean "nothing here": None and empty containers. + + Deliberately not plain falsiness: ``False`` and ``0`` are content the + author typed, so they stay errors rather than being silently skipped. + """ + return value is None or (isinstance(value, dict | list | str) and len(value) == 0) + + +def _mapping(value: Any, key: str, subject: str, example: str) -> dict[Any, Any]: + """Return ``value`` as a mapping; empty stands in for an omitted section.""" + if _is_empty(value): + return {} + if not isinstance(value, dict): + raise _shape_error(key, subject, value, example) + return value + + +def _description(value: Any, key: str, example: str) -> str: + """Coerce a scalar description to ``str``, rejecting containers and null.""" + if value is None: + raise ValueError( + f"'{key}' has no description (empty value); write a non-empty string. " + f"Expected:\n{example}" + ) + if isinstance(value, dict | list): + raise _shape_error(key, _DESCRIPTION_SUBJECT, value, example) + return str(value) + + +def _coerced_key(key: Any, seen: dict[str, Any], prefix: str, example: str) -> str: + """Return ``str(key)``, refusing one that another key already claimed. + + YAML types keys, kbagent addresses objects by string: ``1:`` and ``"1":`` + are two distinct YAML keys that both coerce to ``"1"``, and the second + would silently overwrite the first -- a description the author wrote and + never saw applied. + """ + coerced = str(key) + if coerced in seen: + raise ValueError( + f"'{prefix}' has two entries that both resolve to the ID '{coerced}' " + f'(YAML keys of different types, e.g. `1:` and `"1":`); keep one. ' + f"Expected:\n{example}" + ) + return coerced + + +def _scalar_section(raw: dict[str, Any], key: str) -> dict[str, str]: + """Validate a ``{id: description}`` section.""" + section = _SECTIONS[key] + parsed: dict[str, str] = {} + for item_id, desc in _mapping(raw.get(key), key, section.subject, section.example).items(): + coerced = _coerced_key(item_id, parsed, key, section.example) + parsed[coerced] = _description(desc, f"{key}.{coerced}", section.example) + return parsed + + +def _columns_section(raw: dict[str, Any]) -> dict[str, dict[str, str]]: + """Validate the nested ``{table_id: {column: description}}`` section. + + A table whose column mapping is empty is dropped, not written: there is + nothing to say about it, and an empty write is a pointless API roundtrip. + """ + section = _SECTIONS["columns"] + example = section.example + parsed: dict[str, dict[str, str]] = {} + for table_id, col_map in _mapping( + raw.get("columns"), "columns", section.subject, example + ).items(): + table_key = _coerced_key(table_id, parsed, "columns", example) + key = f"columns.{table_key}" + columns: dict[str, str] = {} + for column, desc in _mapping(col_map, key, _COLUMN_ENTRY_SUBJECT, example).items(): + coerced = _coerced_key(column, columns, key, example) + columns[coerced] = _description(desc, f"{key}.{coerced}", example) + if columns: + parsed[table_key] = columns + return parsed + + +def parse_describe_batch_file(from_file: Path) -> DescribeBatchInput: + """Read and validate a describe-batch YAML file. + + Args: + from_file: Path to the ``--from-file`` document. + + Returns: + The validated sections, descriptions coerced to ``str``. Absent, + ``None`` and empty sections come back empty. + + Raises: + ValueError: The file is missing, is not valid YAML, is not a mapping + at the top level, or any section/entry has a wrong non-empty + shape, a null description, or a duplicate coerced key. The command + layer maps this to ``INVALID_ARGUMENT`` and exit 2. + """ + import yaml + + if not from_file.is_file(): + raise ValueError(f"Batch file not found: {from_file}") + try: + raw = yaml.safe_load(from_file.read_text(encoding="utf-8")) + except yaml.YAMLError as exc: + raise ValueError(f"Batch file is not valid YAML: {exc}") from None + if _is_empty(raw): + return DescribeBatchInput() + if not isinstance(raw, dict): + raise _shape_error(from_file.name, _TOP_LEVEL_SUBJECT, raw, _TOP_LEVEL_EXAMPLE) + return DescribeBatchInput( + buckets=_scalar_section(raw, "buckets"), + tables=_scalar_section(raw, "tables"), + columns=_columns_section(raw), + ) diff --git a/src/keboola_agent_cli/services/storage_service.py b/src/keboola_agent_cli/services/storage_service.py index a5ad0c67..80d4c5bf 100644 --- a/src/keboola_agent_cli/services/storage_service.py +++ b/src/keboola_agent_cli/services/storage_service.py @@ -2481,10 +2481,19 @@ def describe_batch( col1: "Column 1 description" col2: "Column 2 description" - All sections are optional; empty or absent sections are silently - skipped. Within each section the operations are applied in order. - A failure in one item does not skip remaining items — all results - (success and error) are collected and returned. + All sections are optional: absent, ``None`` (a bare ``buckets:`` key) + and empty (``[]``, ``''``, ``{}``) sections are silently skipped. + Within each section the operations are applied in order. + + The document's shape is validated up front (see + ``_describe_batch_input``): a section with a wrong NON-EMPTY shape, a + null description, or two keys colliding after string coercion raises + ``ValueError`` before the first write, so the file is rejected whole + rather than half-applied. + + Once application starts, a failure in one item does not skip the + remaining items — all per-item *API* results (success and error) are + collected and returned. Args: alias: Project alias. @@ -2500,69 +2509,47 @@ def describe_batch( Returns: Dict with project_alias, applied, errors, applied_count, error_count. """ - import yaml - from ..errors import KeboolaApiError + from ._describe_batch_input import parse_describe_batch_file - if not from_file.is_file(): - raise ValueError(f"Batch file not found: {from_file}") - raw = yaml.safe_load(from_file.read_text(encoding="utf-8")) or {} - if not isinstance(raw, dict): - raise ValueError("Batch file must be a YAML mapping.") + parsed = parse_describe_batch_file(from_file) applied: list[dict[str, Any]] = [] errors: list[dict[str, Any]] = [] - buckets: dict[str, str] = raw.get("buckets") or {} - tables: dict[str, str] = raw.get("tables") or {} - columns: dict[str, dict[str, str]] = raw.get("columns") or {} - - total = len(buckets) + len(tables) + len(columns) + total = parsed.total current = 0 - for bucket_id, desc in buckets.items(): + for bucket_id, desc in parsed.buckets.items(): current += 1 if progress_callback is not None: progress_callback("bucket", bucket_id, current, total) try: - self.describe_bucket(alias, bucket_id, str(desc), branch_id=branch_id) + self.describe_bucket(alias, bucket_id, desc, branch_id=branch_id) applied.append({"type": "bucket", "id": bucket_id, "description": desc}) logger.debug("describe_batch bucket %s: ok", bucket_id) except Exception as exc: msg = exc.message if isinstance(exc, KeboolaApiError) else str(exc) errors.append({"type": "bucket", "id": bucket_id, "error": msg}) - for table_id, desc in tables.items(): + for table_id, desc in parsed.tables.items(): current += 1 if progress_callback is not None: progress_callback("table", table_id, current, total) try: - self.describe_table(alias, table_id, str(desc), branch_id=branch_id) + self.describe_table(alias, table_id, desc, branch_id=branch_id) applied.append({"type": "table", "id": table_id, "description": desc}) except Exception as exc: msg = exc.message if isinstance(exc, KeboolaApiError) else str(exc) errors.append({"type": "table", "id": table_id, "error": msg}) - for table_id, col_map in columns.items(): + for table_id, col_map in parsed.columns.items(): current += 1 if progress_callback is not None: progress_callback("columns", table_id, current, total) - if not isinstance(col_map, dict): - errors.append( - {"type": "columns", "id": table_id, "error": "columns entry must be a mapping"} - ) - continue try: - self.describe_columns( - alias, table_id, {k: str(v) for k, v in col_map.items()}, branch_id=branch_id - ) - applied.append( - { - "type": "columns", - "id": table_id, - "columns": {k: str(v) for k, v in col_map.items()}, - } - ) + self.describe_columns(alias, table_id, col_map, branch_id=branch_id) + applied.append({"type": "columns", "id": table_id, "columns": col_map}) except Exception as exc: msg = exc.message if isinstance(exc, KeboolaApiError) else str(exc) errors.append({"type": "columns", "id": table_id, "error": msg}) diff --git a/tests/test_storage_describe_cli.py b/tests/test_storage_describe_cli.py index ee1bff0f..93a48f06 100644 --- a/tests/test_storage_describe_cli.py +++ b/tests/test_storage_describe_cli.py @@ -17,6 +17,7 @@ from keboola_agent_cli.services.config_service import ConfigService from keboola_agent_cli.services.job_service import JobService from keboola_agent_cli.services.project_service import ProjectService +from keboola_agent_cli.services.storage_service import StorageService TEST_TOKEN = "901-55555-fakeTestTokenDoNotUseXXXXXXXX" @@ -655,6 +656,62 @@ def test_describe_batch_file_not_found(self, tmp_path: Path) -> None: assert output["status"] == "error" assert output["error"]["code"] == "INVALID_ARGUMENT" + def test_describe_batch_malformed_shape_json_envelope(self, tmp_path: Path) -> None: + """Issue #640: a list under `tables:` answers --json, not a traceback. + + Runs the REAL StorageService so the shape check actually executes; the + client factory raises if reached, proving the rejection happens before + any API call. + """ + config_dir = tmp_path / "config" + config_dir.mkdir() + store = _setup_config(config_dir, {"prod": {"token": TEST_TOKEN}}) + + batch_file = tmp_path / "bad.yaml" + batch_file.write_text( + "tables:\n - table_id: in.c-test.orders\n columns:\n id: Surrogate key\n", + encoding="utf-8", + ) + + def _no_client(url: str, token: str) -> MagicMock: + raise AssertionError("describe-batch must not reach the API on a malformed file") + + 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 = StorageService( + config_store=store, + client_factory=_no_client, + ) + + result = runner.invoke( + app, + [ + "--json", + "storage", + "describe-batch", + "--project", + "prod", + "--from-file", + str(batch_file), + ], + ) + + 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" + assert "'tables' must be a mapping of table ID to description" in output["error"]["message"] + assert "got a list" in output["error"]["message"] + def test_describe_batch_human_mode_wires_progress_callback(self, tmp_path: Path) -> None: """Human mode must pass a progress_callback; JSON mode must not.""" config_dir = tmp_path / "config" diff --git a/tests/test_storage_describe_service.py b/tests/test_storage_describe_service.py index 0e83c579..12a2bc6d 100644 --- a/tests/test_storage_describe_service.py +++ b/tests/test_storage_describe_service.py @@ -547,6 +547,254 @@ def test_invalid_yaml_not_mapping(self, tmp_path: Path) -> None: service.describe_batch(alias="prod", from_file=batch_file) +class TestDescribeBatchShapeValidation: + """Malformed `--from-file` documents are rejected before any write (#640). + + Each case used to reach the write loop and die on ``.items()`` with an + ``AttributeError`` -- a traceback even under ``--json``. They must now + raise ``ValueError`` (mapped to INVALID_ARGUMENT / exit 2 by the command), + name the offending key and its actual type, and touch no API. + """ + + def _service(self, tmp_path: Path) -> tuple[StorageService, MagicMock]: + store = _make_store(tmp_path) + mock_client = MagicMock() + return _make_service(store, mock_client), mock_client + + def _run(self, tmp_path: Path, yaml_text: str) -> tuple[str, MagicMock]: + """Run describe_batch on ``yaml_text``, returning the message + client.""" + service, mock_client = self._service(tmp_path) + batch_file = tmp_path / "batch.yaml" + batch_file.write_text(yaml_text, encoding="utf-8") + with pytest.raises(ValueError) as excinfo: + service.describe_batch(alias="prod", from_file=batch_file) + return str(excinfo.value), mock_client + + def _assert_no_writes(self, mock_client: MagicMock) -> None: + mock_client.set_bucket_metadata.assert_not_called() + mock_client.set_table_metadata.assert_not_called() + mock_client.update_table_definition.assert_not_called() + + def test_tables_as_list(self, tmp_path: Path) -> None: + """The exact repro from issue #640: `tables:` is a list of objects.""" + message, mock_client = self._run( + tmp_path, + "tables:\n - table_id: in.c-test.part_verify\n columns:\n id: Surrogate key\n", + ) + + assert "'tables' must be a mapping of table ID to description" in message + assert "got a list" in message + # The message carries a copy-pasteable example of the right shape. + assert "in.c-sales.orders: All sales orders" in message + self._assert_no_writes(mock_client) + + def test_buckets_as_list(self, tmp_path: Path) -> None: + message, mock_client = self._run(tmp_path, "buckets:\n - in.c-sales\n") + + assert "'buckets' must be a mapping of bucket ID to description" in message + assert "got a list" in message + self._assert_no_writes(mock_client) + + def test_columns_as_list(self, tmp_path: Path) -> None: + message, mock_client = self._run(tmp_path, "columns:\n - in.c-sales.orders\n") + + assert "'columns' must be a mapping of table ID to a column mapping" in message + assert "got a list" in message + self._assert_no_writes(mock_client) + + def test_columns_entry_scalar(self, tmp_path: Path) -> None: + message, mock_client = self._run( + tmp_path, "columns:\n in.c-sales.orders: Unique order ID\n" + ) + + assert "'columns.in.c-sales.orders' must be a mapping of column name to description" in ( + message + ) + assert "got a string" in message + self._assert_no_writes(mock_client) + + def test_columns_entry_list(self, tmp_path: Path) -> None: + message, mock_client = self._run( + tmp_path, "columns:\n in.c-sales.orders:\n - order_id\n" + ) + + assert "'columns.in.c-sales.orders' must be" in message + assert "got a list" in message + self._assert_no_writes(mock_client) + + def test_column_description_is_a_container(self, tmp_path: Path) -> None: + """A nested mapping under a column name would be str()'d into garbage.""" + message, mock_client = self._run( + tmp_path, + "columns:\n in.c-sales.orders:\n order_id:\n text: Unique order ID\n", + ) + + assert "'columns.in.c-sales.orders.order_id' must be a description string" in message + assert "got a mapping" in message + self._assert_no_writes(mock_client) + + def test_table_description_is_a_container(self, tmp_path: Path) -> None: + message, mock_client = self._run( + tmp_path, "tables:\n in.c-sales.orders:\n - All sales orders\n" + ) + + assert "'tables.in.c-sales.orders' must be a description string" in message + assert "got a list" in message + self._assert_no_writes(mock_client) + + def test_bucket_description_is_null(self, tmp_path: Path) -> None: + """A bare key used to write the literal text "None" onto the bucket.""" + message, mock_client = self._run(tmp_path, "buckets:\n in.c-sales:\n") + + assert "'buckets.in.c-sales' has no description (empty value)" in message + assert "write a non-empty string" in message + self._assert_no_writes(mock_client) + + def test_table_description_is_null(self, tmp_path: Path) -> None: + message, mock_client = self._run(tmp_path, "tables:\n in.c-sales.orders:\n") + + assert "'tables.in.c-sales.orders' has no description (empty value)" in message + self._assert_no_writes(mock_client) + + def test_column_description_is_null(self, tmp_path: Path) -> None: + message, mock_client = self._run( + tmp_path, "columns:\n in.c-sales.orders:\n order_id:\n" + ) + + assert "'columns.in.c-sales.orders.order_id' has no description (empty value)" in message + self._assert_no_writes(mock_client) + + def test_duplicate_keys_colliding_after_coercion(self, tmp_path: Path) -> None: + """`1:` and `"1":` are two YAML keys; one used to silently overwrite the other.""" + message, mock_client = self._run(tmp_path, 'tables:\n 1: First\n "1": Second\n') + + assert "'tables' has two entries that both resolve to the ID '1'" in message + self._assert_no_writes(mock_client) + + def test_duplicate_column_names_colliding_after_coercion(self, tmp_path: Path) -> None: + message, mock_client = self._run( + tmp_path, + 'columns:\n in.c-sales.orders:\n 1: First\n "1": Second\n', + ) + + assert "'columns.in.c-sales.orders' has two entries" in message + assert "resolve to the ID '1'" in message + self._assert_no_writes(mock_client) + + def test_top_level_scalar(self, tmp_path: Path) -> None: + message, mock_client = self._run(tmp_path, "just a string\n") + + assert "must be a YAML mapping of 'buckets' / 'tables' / 'columns' sections" in message + assert "got a string" in message + self._assert_no_writes(mock_client) + + def test_malformed_yaml_syntax(self, tmp_path: Path) -> None: + message, mock_client = self._run(tmp_path, "tables:\n - [unclosed\n") + + assert "Batch file is not valid YAML" in message + self._assert_no_writes(mock_client) + + def test_nothing_is_applied_when_a_later_section_is_malformed(self, tmp_path: Path) -> None: + """Validation is up front: a bad `columns:` must not let buckets through.""" + message, mock_client = self._run( + tmp_path, + "buckets:\n in.c-sales: Sales data\ncolumns:\n - in.c-sales.orders\n", + ) + + assert "'columns' must be" in message + self._assert_no_writes(mock_client) + + def _apply(self, tmp_path: Path, yaml_text: str) -> tuple[dict[str, Any], MagicMock]: + """Run describe_batch on a document expected to be accepted.""" + service, mock_client = self._service(tmp_path) + batch_file = tmp_path / "batch.yaml" + batch_file.write_text(yaml_text, encoding="utf-8") + return service.describe_batch(alias="prod", from_file=batch_file), mock_client + + @pytest.mark.parametrize( + "yaml_text", + [ + "buckets:\n", # bare key -> None + "buckets: []\n", + "buckets: ''\n", + "buckets: {}\n", + ], + ids=["null", "empty-list", "empty-string", "empty-mapping"], + ) + def test_empty_section_is_a_no_op(self, tmp_path: Path, yaml_text: str) -> None: + """Back-compat: `raw.get(key) or {}` skipped these, so they must not exit 2.""" + result, mock_client = self._apply(tmp_path, yaml_text) + + assert result["applied"] == [] + assert result["errors"] == [] + self._assert_no_writes(mock_client) + + def test_empty_column_mapping_is_a_no_op(self, tmp_path: Path) -> None: + """Same rule one level down: nothing to write, so no API roundtrip.""" + result, mock_client = self._apply(tmp_path, "columns:\n in.c-sales.orders: {}\n") + + assert result["applied"] == [] + assert result["errors"] == [] + self._assert_no_writes(mock_client) + + def test_non_empty_list_section_still_errors(self, tmp_path: Path) -> None: + """Only EMPTY containers are skipped -- content must never be dropped.""" + message, mock_client = self._run(tmp_path, "buckets:\n - in.c-sales\n") + + assert "'buckets' must be a mapping of bucket ID to description" in message + self._assert_no_writes(mock_client) + + @pytest.mark.parametrize( + ("yaml_text", "expected_type"), + [("buckets: false\n", "boolean"), ("buckets: 0\n", "number")], + ids=["false", "zero"], + ) + def test_falsy_scalar_section_still_errors( + self, tmp_path: Path, yaml_text: str, expected_type: str + ) -> None: + """`false` / `0` are typed content, not an empty section.""" + message, mock_client = self._run(tmp_path, yaml_text) + + assert "'buckets' must be a mapping of bucket ID to description" in message + assert f"got a {expected_type}" in message + self._assert_no_writes(mock_client) + + def test_top_level_false_errors(self, tmp_path: Path) -> None: + message, mock_client = self._run(tmp_path, "false\n") + + assert "must be a YAML mapping of 'buckets' / 'tables' / 'columns' sections" in message + assert "got a boolean" in message + self._assert_no_writes(mock_client) + + def test_valid_file_still_applies(self, tmp_path: Path) -> None: + """Happy-path regression: the documented schema is untouched.""" + service, mock_client = self._service(tmp_path) + 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 + + batch_file = tmp_path / "good.yaml" + batch_file.write_text( + "buckets:\n" + " in.c-sales: Sales data\n" + "tables:\n" + " in.c-sales.orders: 2026\n" + "columns:\n" + " in.c-sales.orders:\n" + " order_id: Unique order ID\n", + encoding="utf-8", + ) + + result = service.describe_batch(alias="prod", from_file=batch_file) + + assert result["error_count"] == 0 + assert result["applied_count"] == 3 + # Non-string scalars stay coerced to str, as before the validation pass. + table_applied = next(a for a in result["applied"] if a["type"] == "table") + assert table_applied["description"] == "2026" + + def _token_info(project_id: int = 258) -> TokenVerifyResponse: return TokenVerifyResponse( token_id="12345",