diff --git a/CLAUDE.md b/CLAUDE.md index 508593a8..5474f41c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -838,6 +838,18 @@ kbagent sync pull --project ALIAS [--all-projects] [--force] [--theirs] [--dry-r kbagent sync status [--directory DIR] kbagent sync diff --project ALIAS [--all-projects] [--directory DIR] [--branch ID] kbagent sync push --project ALIAS [--all-projects] [--dry-run] [--force] [--allow-plaintext-on-encrypt-failure] [--branch ID] [--no-name-drift-warnings] +# sync push (since vNEXT, #686): the manifest baseline `pull_config_hash` is stamped from the API +# response (or a read-back), never from disk -- push-deployed multi-statement SQL transformations +# (and anything disabled in the UI whose local YAML lacks `is_disabled`) no longer show permanent +# phantom `~ REMOTE MODIFIED` drift in `sync diff`. Unreadable-after-write leaves the baseline +# UNTOUCHED + a `warnings[]` entry (never a disk-derived fallback). One canonical script[] shape now +# (one element = one statement) and `transform.sql` gains `/* ===== STATEMENT ===== */` markers when +# semicolons cannot recover the boundaries -- which also closes a SILENT pre-vNEXT rewrite that +# collapsed such scripts to one statement (MULTI_STATEMENT_COUNT=1) while diff said "in sync". +# Migration: entries carry `metadata.config_hash_version`; unversioned ones match leniently (pre-vNEXT +# hash of the SAME remote counts as in sync, nothing else), and ONE `sync pull` per project migrates. +# A pre-markers tree whose only difference from the remote is the lost boundaries is REFUSED per-change +# with SYNC_LEGACY_BOUNDARY telling you to pull first; genuine edits push normally. # sync diff/push (0.89.0+, #649): local side read from exactly ONE tree (target branch subtree, else main/); entries tracked on another branch's tree are excluded from the changeset and reported under orphaned[] + summary.orphaned (reasons + reconcile hints); fix with sync pull. Adopt-by-id is branch-aware. # Ignored components (since vNEXT, #689): keboola.mcp-server-tool joins keboola.sandboxes on ALWAYS_IGNORED_COMPONENTS (the MCP server's auto-created empty mcp-workspace- configs); the manifest field ignoredComponents (.keboola/manifest.json) is now LIVE and unions with the hardcoded set, honored by pull/diff/push. pull drops manifest entries + local dirs for a newly-ignored component, reported with action "ignored" (distinct from "removed" = deleted on remote); diff filters the local side too, so a stale dir for an ignored component can never classify as DELETED -- closes the delete-dir-then-push trap that used to destroy production keboola.mcp-server-tool configs. kbagent sync clone --source DIR --target ALIAS --target-dir DIR [--bucket-map FILE] [--variable-values FILE] [--instance-rename FILE] [--dry-run] [--branch ID] diff --git a/docs/error-codes.md b/docs/error-codes.md index 45df8897..a3aba6e7 100644 --- a/docs/error-codes.md +++ b/docs/error-codes.md @@ -130,6 +130,7 @@ of `ErrorCode` in `src/keboola_agent_cli/errors.py`. | `PARENT_CONFIG_NOT_TRACKED` | Row operation references a parent config not in the manifest | | `VARIABLE_LINK_UNRESOLVED` | `sync push` could not resolve a transformation's variables link to a tracked config | | `SYNC_CONFLICT` | `sync pull --force` aborted: local and remote both changed since the last pull (`details.conflicts` lists them) | +| `SYNC_LEGACY_BOUNDARY` | `sync push` refused one config: the working tree predates statement-boundary tracking, so pushing it would merge separate SQL statements into one. Run `sync pull` first | ### Encryption diff --git a/plugins/kbagent/agents/keboola-expert.md b/plugins/kbagent/agents/keboola-expert.md index 694caa8d..d4052c2f 100644 --- a/plugins/kbagent/agents/keboola-expert.md +++ b/plugins/kbagent/agents/keboola-expert.md @@ -257,7 +257,11 @@ its absence is NOT a promise the entry is version-independent (see ยง1 Rule 6). `never_fetched` warning on diff/push = run `sync pull` first; a non-zero `summary.orphaned` (0.89.0+, #649) = the manifest is targeted at another branch's tree -- `sync pull` to re-target, never push. `sync status` - is local-only -- audit real drift with `sync diff`. + is local-only -- audit real drift with `sync diff`. On <= 0.90.1 a + `~ REMOTE MODIFIED ... codes changed` on a config nobody touched is usually + PHANTOM (issue #686: push stamped the baseline from disk); fixed in vNEXT -- + one `sync pull` per project migrates a tree pulled by an older version, and + a `SYNC_LEGACY_BOUNDARY` push error means exactly that: pull first. - **Ignored components** (vNEXT, #689): `keboola.sandboxes` + `keboola.mcp-server-tool` are always excluded from `pull`/`diff`/`push`, unioned with the manifest's `ignoredComponents` list. A component newly diff --git a/plugins/kbagent/skills/kbagent/references/commands-reference.md b/plugins/kbagent/skills/kbagent/references/commands-reference.md index fbb4e474..77ebeb39 100644 --- a/plugins/kbagent/skills/kbagent/references/commands-reference.md +++ b/plugins/kbagent/skills/kbagent/references/commands-reference.md @@ -326,9 +326,9 @@ Requires the project to be added with its **master ('owner') Storage API token** ## Sync (GitOps) - `sync init --project ALIAS [--directory DIR] [--git-branching] [--adopt-existing]` -- initialize sync working directory; `--adopt-existing` (since v0.22.0) adopts a `.keboola/manifest.json` already written by the kbc Go CLI without overwriting (idempotent; validates `project_id` against the alias token) - `sync pull --project ALIAS [--all-projects] [--force] [--theirs] [--dry-run] [--with-samples] [--no-storage] [--no-jobs] [--job-limit N] [--branch ID]` -- download configs to local files. For large projects (>100 configs), automatically fetches jobs per-config when the grouped API limit is insufficient. `--force` is conflict-aware (since 0.53.0): a locally-modified config whose remote is unchanged is **preserved** (pending delta stays pushable, never silently re-stamped); a true merge conflict (local AND remote both changed since last pull) **aborts** the pull (exit 1, `SYNC_CONFLICT`; `--json` lists `details.conflicts`); local-untouched + remote-changed takes remote. `--theirs` (since v0.72.0) is the supported "discard local, take production" reconcile path: overwrites locally-modified configs/rows, restores deleted/missing files, resolves conflicts by taking remote (no abort, no manifest surgery). Since v0.72.0 plain pull also re-materializes a tracked config whose local dir was deleted (manifest<->disk invariant), so delete-dir-then-pull refetches. Config-level `isDisabled` round-trips (since v0.72.0) as sparse `is_disabled: true` in `_config.yml` -- absent key = enabled. `--branch` (0.47.0+) per-invocation dev-branch override, beats every other branch source. Ignored components (since vNEXT): `keboola.sandboxes` + `keboola.mcp-server-tool` are always excluded, unioned with the manifest's `ignoredComponents` list; a component newly ignored has its manifest entry dropped and local directory removed, reported with pull action `"ignored"` (distinct from `"removed"` = genuinely deleted on remote). -- `sync push --project ALIAS [--all-projects] [--dry-run] [--force] [--allow-plaintext-on-encrypt-failure] [--branch ID] [--no-name-drift-warnings]` -- push local changes (auto-encrypts secrets, fails if encryption fails). Fresh-CREATE writeback updates placeholder manifest entries in place (since 0.47.0) and propagates any `KBC.configuration.*` metadata via `set_config_metadata`. Fresh-CREATE variable binding (since 0.47.2): when a `keboola.variables` config + its values row are created alongside a transformation in the same push, the transformation's `variables_id` / `variables_values_id` placeholders are rebound to the assigned ULIDs and the row's `values` are hoisted even without a `_keboola` block, so `job run` succeeds with no post-push `config variables-set` step (unresolvable/ambiguous links surface a `variable_link` entry in `errors[]`, never a broken link). Never-fetched guard (since v0.72.0): a manifest entry with an empty `pull_hash` and no local files (pre-0.72 name-collision phantom) is **never** planned as a remote DELETE -- diff/push exclude it and report it under `never_fetched` with a warning (run `sync pull` to materialize); local deletion of a properly-pulled config still deletes on push. Adopted-by-id writeback (since v0.72.0): pushing an untracked file whose `_keboola.config_id` resolves on the branch also writes the manifest entry, so follow-up diffs are stable. `--branch` (0.47.0+) per-invocation override; when no `/` subtree exists on disk (since 0.47.2) the local default tree (`main/`) is promoted to the target branch (API writes still target the branch id); `--no-name-drift-warnings` (0.47.0+) drops the cosmetic warnings array. Branch-scoped since v0.89.0 (issue #649): push consumes the diff's changeset, so configs tracked on another branch's tree are never planned as creates -- they ride along on the result envelope under `orphaned` instead (see `sync diff`). Ignored components (since vNEXT) are filtered out on both sides of the diff push builds on, so a stale local directory for an ignored component (e.g. `keboola.mcp-server-tool`) is never classified as `DELETED` and can never be pushed as a remote deletion. +- `sync push --project ALIAS [--all-projects] [--dry-run] [--force] [--allow-plaintext-on-encrypt-failure] [--branch ID] [--no-name-drift-warnings]` -- push local changes (auto-encrypts secrets, fails if encryption fails). Fresh-CREATE writeback updates placeholder manifest entries in place (since 0.47.0) and propagates any `KBC.configuration.*` metadata via `set_config_metadata`. Fresh-CREATE variable binding (since 0.47.2): when a `keboola.variables` config + its values row are created alongside a transformation in the same push, the transformation's `variables_id` / `variables_values_id` placeholders are rebound to the assigned ULIDs and the row's `values` are hoisted even without a `_keboola` block, so `job run` succeeds with no post-push `config variables-set` step (unresolvable/ambiguous links surface a `variable_link` entry in `errors[]`, never a broken link). Never-fetched guard (since v0.72.0): a manifest entry with an empty `pull_hash` and no local files (pre-0.72 name-collision phantom) is **never** planned as a remote DELETE -- diff/push exclude it and report it under `never_fetched` with a warning (run `sync pull` to materialize); local deletion of a properly-pulled config still deletes on push. Adopted-by-id writeback (since v0.72.0): pushing an untracked file whose `_keboola.config_id` resolves on the branch also writes the manifest entry, so follow-up diffs are stable. `--branch` (0.47.0+) per-invocation override; when no `/` subtree exists on disk (since 0.47.2) the local default tree (`main/`) is promoted to the target branch (API writes still target the branch id); `--no-name-drift-warnings` (0.47.0+) drops the cosmetic warnings array. Branch-scoped since v0.89.0 (issue #649): push consumes the diff's changeset, so configs tracked on another branch's tree are never planned as creates -- they ride along on the result envelope under `orphaned` instead (see `sync diff`). **Since vNEXT (#686)** the manifest baseline `pull_config_hash` is stamped from the API response (or a read-back), not from the files on disk, so a pushed multi-statement SQL transformation -- or anything disabled in the UI whose local YAML lacks `is_disabled` -- no longer shows permanent phantom `REMOTE MODIFIED` drift; if the config cannot be read back after the write the baseline is left UNTOUCHED and a `warnings[]` entry says to run `sync pull` (never a disk-derived fallback). One legacy change is refused per-change with `SYNC_LEGACY_BOUNDARY`: a tree pulled before statement-boundary markers existed whose only difference from the remote is the lost boundaries (pushing it would collapse separate SQL statements into one) -- run `sync pull` for that project first. Ignored components (since vNEXT) are filtered out on both sides of the diff push builds on, so a stale local directory for an ignored component (e.g. `keboola.mcp-server-tool`) is never classified as `DELETED` and can never be pushed as a remote deletion. - `sync clone --source DIR --target ALIAS --target-dir DIR [--bucket-map FILE] [--variable-values FILE] [--instance-rename FILE] [--dry-run] [--branch ID]` -- clone a reference synced project into a **fresh** target project and parameterize it (since v0.63.0). Copies the reference tree at `--source` into `--target-dir`, applies declarative overrides from JSON/YAML files (`--bucket-map` `{old_bucket_id: new_bucket_id}` rewrites storage input/output table refs; `--variable-values` `{var_name: value}` overrides `keboola.variables` rows; `--instance-rename` `{old_path_prefix: new_path_prefix}` renames config dirs + manifest paths), re-points the manifest at the target project, and pushes. Because the reference's config ids do not exist in the fresh target, every config is CREATEd fresh and **keboola.flow task `configId`s + transformation variable links are remapped reference->ULID** by push Phase C/D (the push result carries `flow_task_remaps`). **Idempotent**: re-running with an existing `--target-dir` skips copy/overrides and just pushes, reporting `no_changes` / `created: 0`. Fails fast (`CONFIG_ERROR`) if the target already contains the reference's configs -- clone requires a fresh/empty target. `SyncService.clone_project(...)` returns a typed `CloneResult` for in-process SDK callers. Override files must be flat `{id: scalar}` mappings *(since v0.89.0)* -- a nested mapping, list, or null value is rejected with `CONFIG_ERROR` (exit 5) naming the key and its actual type. -- `sync diff --project ALIAS [--all-projects] [--branch ID]` -- 3-way diff (local vs base vs remote), detects conflicts. `--branch` (0.47.0+) per-invocation dev-branch override. Branch-scoped since v0.89.0 (issue #649): the local side is read from exactly ONE tree (the target branch's subtree, or `main/` when the target has none). Manifest entries belonging to another branch's tree -- what `sync pull --branch ` leaves behind when it re-targets the manifest -- are excluded from the changeset and reported under `orphaned` (`summary.orphaned` + details with `component_id`, `config_id`, `path`, `branch_id`, `branch_path`, `exists_on_target`, `reason`, `hint`); human mode previews the first 10. An orphaned FILE whose `_keboola.config_id` still resolves on the target is adopted (diffed as `unchanged`/`modified`), never re-created; same-tree id claims keep the #482/#497 fork-by-copy CREATE. Fix a non-zero `summary.orphaned` with `sync pull`. Ignored components (since vNEXT) -- `keboola.sandboxes`, `keboola.mcp-server-tool`, and anything listed in the manifest's `ignoredComponents` -- are excluded from BOTH sides of the comparison, so a stale local directory for one of them never shows up as `DELETED`. +- `sync diff --project ALIAS [--all-projects] [--branch ID]` -- 3-way diff (local vs base vs remote), detects conflicts. `--branch` (0.47.0+) per-invocation dev-branch override. Branch-scoped since v0.89.0 (issue #649): the local side is read from exactly ONE tree (the target branch's subtree, or `main/` when the target has none). Manifest entries belonging to another branch's tree -- what `sync pull --branch ` leaves behind when it re-targets the manifest -- are excluded from the changeset and reported under `orphaned` (`summary.orphaned` + details with `component_id`, `config_id`, `path`, `branch_id`, `branch_path`, `exists_on_target`, `reason`, `hint`); human mode previews the first 10. An orphaned FILE whose `_keboola.config_id` still resolves on the target is adopted (diffed as `unchanged`/`modified`), never re-created; same-tree id claims keep the #482/#497 fork-by-copy CREATE. Fix a non-zero `summary.orphaned` with `sync pull`. **Since vNEXT (#686)** a manifest entry without `metadata.config_hash_version` (written by a pre-vNEXT kbagent) is compared leniently: a stored hash equal to the pre-vNEXT hash of the SAME remote config counts as in sync, so the phantom `codes changed` entries disappear immediately; every other field is still pinned by that hash, so real remote drift is unaffected. One `sync pull` per project stamps the version and ends the leniency. Ignored components (since vNEXT) -- `keboola.sandboxes`, `keboola.mcp-server-tool`, and anything listed in the manifest's `ignoredComponents` -- are excluded from BOTH sides of the comparison, so a stale local directory for one of them never shows up as `DELETED`. - `sync status [--directory DIR]` -- show locally modified/added/deleted configs. Also surfaces `plaintext_secret_warnings` (since 0.55.0): in-sync configs/rows whose `#`-secrets are still plaintext on the remote (a leftover from pre-0.54.0 writes; #378). Pending (un-pushed) edits are not flagged. Fix = re-push on >=0.54.0 + rotate (version history keeps the plaintext). - `sync branch-link --project ALIAS [--branch-id ID] [--branch-name NAME]` -- link git branch to Keboola dev branch - `sync branch-unlink [--directory DIR]` -- remove git-to-Keboola branch mapping diff --git a/plugins/kbagent/skills/kbagent/references/gotchas.md b/plugins/kbagent/skills/kbagent/references/gotchas.md index 847dd2b1..8bf0818b 100644 --- a/plugins/kbagent/skills/kbagent/references/gotchas.md +++ b/plugins/kbagent/skills/kbagent/references/gotchas.md @@ -458,6 +458,75 @@ Versioning convention: `--allow-plaintext-on-encrypt-failure`, which would write the PAT in plaintext into Storage. +## `sync push` no longer leaves phantom `REMOTE MODIFIED` drift; `transform.sql` carries statement boundaries (since vNEXT, #686) + +`pull_config_hash` in `.keboola/manifest.json` is the 3-way diff's base, and it +means "the hash of this config **as the API returns it**". `sync pull` and the +remote side of `sync diff` always computed it that way. `sync push` computed it +from the **files on disk** instead -- and the two producers disagreed about +`parameters.blocks[].codes[].script`, so every multi-statement SQL +transformation deployed through `sync push` was reported +`~ REMOTE MODIFIED ... codes changed` by every later `sync diff`, forever, with +the working tree byte-identical to the remote. Only a real `sync pull` cleared +it, and the next deploy re-created it (field report: 18 phantom configs hiding +2 real UI changes across 21 projects). + +- **What changed.** Push now stamps the baseline from the API's own view of + what it just wrote -- at all six sites (config create/update, row + create/update, and the Phase C/D link backfills). `pull_hash` and + `pull_extra_hashes` stay disk-derived; they describe local files. +- **The same fix closes the `is_disabled` phantom.** A config (or row) disabled + in the UI whose local YAML has no `is_disabled` key drifted permanently after + every push for the same reason. No workaround needed any more. +- **If the config cannot be read back after the write** (a partial mutation + response AND a failed re-read), `pull_config_hash` is left EXACTLY as it was + and the push envelope carries a `warnings[]` entry telling you to run + `sync pull`. Visibly stale beats confidently wrong -- push never falls back + to a disk-derived baseline. +- **One `script[]` shape now, everywhere: one element = one executable + statement.** That is what the Keboola runtime means by the array + (#119/#120/#274), and `sync push` has produced it since 0.30.x. The API side + used to collapse each code into ONE joined string, which no multi-statement + SQL config could ever match. Non-SQL components (Python, R, custom apps) are + unchanged -- there both sides always agreed on the single joined string. +- **`transform.sql` can now contain `/* ===== STATEMENT ===== */` marker + lines.** They are written only when semicolons alone cannot recover the + statement array -- i.e. when the API's elements carry no trailing `;`. A + `;`-terminated file is byte-identical to what earlier versions wrote, so + existing trees produce no spurious diff. This closes a SILENT failure: before + vNEXT, a script like `["SELECT 1", "SELECT 2"]` lost its boundary on pull and + push rewrote production as ONE statement (the `MULTI_STATEMENT_COUNT=1` crash + shape) while `sync diff` reported "in sync". + - Markers are guaranteed boundaries, but `;` splitting still runs INSIDE each + marked segment -- typing `; SELECT ...` in a marked segment splits + correctly. + - Do not hand-write a line identical to the marker inside a statement: when + extraction sees one it suppresses markers for that code entirely (logging a + warning) rather than writing an ambiguous file. + +**Migration -- one `sync pull` per project, then the noise is gone:** + +- Each manifest entry now carries `metadata.config_hash_version` (currently + `2`) next to a hash the current producer computed. +- An entry WITHOUT that key predates the fix, so it is compared **leniently**: + a stored hash equal to the pre-fix hash of the SAME remote config counts as + in sync. Every other field is pinned by that same hash, so the leniency + cannot hide real drift -- a renamed or edited remote still reports + `REMOTE MODIFIED`. +- `sync pull` re-runs extraction (writing the markers where needed) and stamps + the version, which ends the leniency for that entry. `sync diff` stays + read-only and migrates nothing. +- A migration pull will NOT overwrite an edited `transform.sql` / + `_description.md`: unlike the ordinary overwrite-guard (which only ever + checked `_config.yml`), the migration path checks the companion files too, + and preserves the entry unstamped when any of them changed. +- **`sync push` refuses ONE specific legacy change** with + `SYNC_LEGACY_BOUNDARY` (per-change error, exit 1, the rest of the push + proceeds): a tree pulled before markers existed whose only difference from + the remote is the lost statement boundaries. Pushing it would merge separate + statements into one. Run `sync pull` for that project, then push again. + Genuine SQL edits are never blocked by this guard. + ## `sync push` fresh-CREATE writeback now updates placeholders in place (since v0.47.0) Before v0.47.0, `kbagent sync push` always **appended** new `ManifestConfiguration` diff --git a/plugins/kbagent/skills/kbagent/references/sync-workflow.md b/plugins/kbagent/skills/kbagent/references/sync-workflow.md index c4c7af35..26d40c61 100644 --- a/plugins/kbagent/skills/kbagent/references/sync-workflow.md +++ b/plugins/kbagent/skills/kbagent/references/sync-workflow.md @@ -416,6 +416,11 @@ Stored in `.keboola/branch-mapping.json`: - **Pull protects local edits**: locally-modified files are skipped by default - **`--force` is conflict-aware (since 0.53.0)**: see below -- it no longer blindly overwrites - **Push only sends local changes**: remote_modified and conflict changes are skipped +- **Push records the API's own view of what it wrote (since vNEXT, #686)**: the + manifest baseline (`pull_config_hash`) comes from the API response (or a + read-back), never from the files on disk. Before vNEXT the two producers + disagreed and every pushed multi-statement SQL transformation showed + permanent phantom `REMOTE MODIFIED` drift - **Encrypted values**: nonce differences are ignored in diff (no false positives) - **New configs**: push auto-assigns IDs from the API, updates manifest - **Storage metadata is read-only**: not tracked in manifest, excluded from diff/push @@ -477,6 +482,39 @@ the 3-way diff state per config (and per row): > stops you loudly instead of losing work. To intentionally drop a local edit, > delete the file (or the config directory) and pull. +## Migrating a tree pulled before vNEXT (#686) + +`sync push` used to stamp the manifest baseline from the files on disk while +`sync pull` / `sync diff` computed it from the API. Any config the two +producers disagreed about -- in practice every SQL transformation with two or +more statements, plus anything disabled in the UI whose local YAML has no +`is_disabled` key -- came back as `~ REMOTE MODIFIED` after every deploy, with +the tree byte-identical to the remote. + +**What to do once per project: `kbagent sync pull --project `.** + +That single pull re-extracts the code files (writing +`/* ===== STATEMENT ===== */` boundary markers where semicolons cannot recover +the statement array) and stamps `metadata.config_hash_version` on each manifest +entry. Commit the resulting `manifest.json` (plus any `transform.sql` that +gained markers) and the phantom entries are gone for good -- no more +content-free "refresh the baseline" PR after each deploy. + +Until that pull happens, kbagent is lenient with unversioned entries: a stored +hash that matches the pre-vNEXT hash of the same remote config is treated as in +sync. The leniency covers ONLY that difference -- a genuinely changed remote +still reports `REMOTE MODIFIED`. + +One case is refused rather than pushed: if a pre-markers tree's only difference +from the remote is the lost statement boundaries, `sync push` aborts THAT change +with `SYNC_LEGACY_BOUNDARY` (other changes in the same push still go through) +because pushing it would merge separate SQL statements into one -- the +`MULTI_STATEMENT_COUNT=1` runtime failure. Pull the project, then push. + +If a config cannot be read back after a successful write, push leaves its +baseline untouched and reports it under `warnings[]` in the result envelope; +run `sync pull` to refresh it. + ## Cloning a reference project (`sync clone`, since v0.63.0) `sync clone` builds a new customer/instance project by **copying a golden diff --git a/src/keboola_agent_cli/commands/sync.py b/src/keboola_agent_cli/commands/sync.py index 5b1a8046..900d7046 100644 --- a/src/keboola_agent_cli/commands/sync.py +++ b/src/keboola_agent_cli/commands/sync.py @@ -1111,6 +1111,8 @@ def sync_push( f" Error: {err['change_type']} {err['component_id']}/{err['config_id']}: " f"{err['message']}" ) + for warn in result.get("warnings", []): + formatter.warning(f" {warn['message']}") @sync_app.command("clone") diff --git a/src/keboola_agent_cli/constants.py b/src/keboola_agent_cli/constants.py index 256c7ca5..ffc809af 100644 --- a/src/keboola_agent_cli/constants.py +++ b/src/keboola_agent_cli/constants.py @@ -645,6 +645,13 @@ def _resolve_app_name() -> str: # Manifest v3 introduces ManifestConfigRow.metadata (row-level pull hashes) but does # not change the on-disk YAML shape, so CONFIG_YML_VERSION stays at 2. CONFIG_YML_VERSION: int = 2 +# Shape version of the manifest's ``pull_config_hash``, stored per entry under +# ``metadata.config_hash_version`` (issue #686). Version 2 = SQL transformation +# ``script[]`` normalized to one element per statement. An entry WITHOUT the key +# predates the fix: its stored hash may be the legacy collapsed shape, which +# `sync diff` / `sync pull` accept leniently until the next pull re-stamps it. +CONFIG_HASH_VERSION: int = 2 +CONFIG_HASH_VERSION_KEY: str = "config_hash_version" SANITIZE_NAME_MAX_LENGTH: int = 100 # How many `orphaned` rows `sync diff` / `sync push` print in human mode before # collapsing the rest into a count. A manifest re-targeted by diff --git a/src/keboola_agent_cli/errors.py b/src/keboola_agent_cli/errors.py index 38267efb..6e3d4993 100644 --- a/src/keboola_agent_cli/errors.py +++ b/src/keboola_agent_cli/errors.py @@ -96,6 +96,7 @@ class ErrorCode(StrEnum): PARENT_CONFIG_NOT_TRACKED = "PARENT_CONFIG_NOT_TRACKED" VARIABLE_LINK_UNRESOLVED = "VARIABLE_LINK_UNRESOLVED" SYNC_CONFLICT = "SYNC_CONFLICT" + SYNC_LEGACY_BOUNDARY = "SYNC_LEGACY_BOUNDARY" # Encryption ENCRYPTION_FAILED = "ENCRYPTION_FAILED" @@ -307,6 +308,7 @@ def __init__(self, feature: str, *, remedy: str = "") -> None: ErrorCode.CONFIG_ERROR: "configuration", ErrorCode.VALIDATION_ERROR: "validation", ErrorCode.SYNC_CONFLICT: "conflict", + ErrorCode.SYNC_LEGACY_BOUNDARY: "conflict", ErrorCode.PERMISSION_DENIED: "authorization", ErrorCode.DP_LOGIN_FAILED: "authentication", ErrorCode.DP_MFA_REQUIRED: "authentication", diff --git a/src/keboola_agent_cli/services/_sync_baseline.py b/src/keboola_agent_cli/services/_sync_baseline.py new file mode 100644 index 00000000..616460e7 --- /dev/null +++ b/src/keboola_agent_cli/services/_sync_baseline.py @@ -0,0 +1,547 @@ +"""API-derived manifest baselines and legacy-hash compatibility (issue #686). + +``pull_config_hash`` is the 3-way diff's base: the normalized hash of the +config *as the API returns it*. ``sync pull`` and the remote side of +``sync diff`` always computed it that way; ``sync push`` instead recomputed it +from the files on disk, so any config whose local<->API round-trip is not +hash-stable was reported ``~ REMOTE MODIFIED`` by every subsequent diff -- +forever, with the working tree byte-identical to the remote. + +This module holds the one producer push now uses (:func:`config_baseline` / +:func:`row_baseline`) plus the migration helpers that keep manifests written by +a pre-#686 kbagent readable: + +- :func:`effective_stored_hash` -- lenient comparison for entries without + ``metadata.config_hash_version``. +- :func:`raise_on_legacy_boundary` -- refuses to push a legacy tree whose + ``transform.sql`` cannot represent the remote's statement boundaries, which + would silently collapse several statements into one. + +:func:`detect_force_pull_conflicts` lives here for the same reason: it is the +third place a stored baseline is weighed against a fresh API hash, and it must +apply the same leniency or a ``--force`` pull would abort on a shape-only +difference. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass +from pathlib import Path +from typing import TYPE_CHECKING, Any + +from ..constants import ( + CONFIG_FILENAME, + CONFIG_HASH_VERSION, + CONFIG_HASH_VERSION_KEY, +) +from ..errors import ErrorCode, KeboolaApiError +from ..sync.code_extraction import ( + is_sql_transformation_component, + marker_less_roundtrip, + merge_code_files, +) +from ..sync.config_format import api_config_to_local, api_row_to_local +from ..sync.diff_engine import config_hash +from ..sync.manifest import Manifest + +if TYPE_CHECKING: + from .sync_service import SyncService + +logger = logging.getLogger(__name__) + + +@dataclass +class BaselineStamp: + """The ``pull_config_hash`` a push should record, or the reason it cannot. + + ``stamped=False`` means the API state could not be established (the + mutation response was partial AND the follow-up detail fetch failed). The + caller must then leave the previous ``pull_config_hash`` untouched -- never + fall back to a hash computed from disk, which is the very asymmetry #686 + is about, and which would read a response missing ``isDisabled`` as + "enabled". + """ + + cfg_hash: str = "" + stamped: bool = False + warning: dict[str, str] | None = None + + @property + def version(self) -> int | None: + """The hash-shape version to store, or ``None`` when nothing is stamped. + + The version is only ever written next to a hash actually computed with + the current producer -- never next to a preserved legacy hash. + """ + return CONFIG_HASH_VERSION if self.stamped else None + + +def _usable_payload(response: Any) -> dict[str, Any] | None: + """Return *response* when it is a full configuration/row object. + + The Storage API makes no ``include`` promise on PUT/POST, and mocked + clients in tests routinely answer ``{"id": "..."}``. A payload counts as + full only when it carries a ``configuration`` key -- an id-only response + would hash as an empty, enabled config and stamp a baseline that never + matches. The value may be a list: PHP serializes an empty configuration as + ``[]`` and ``api_config_to_local`` already normalizes that to ``{}``. + """ + if not isinstance(response, dict): + return None + if not isinstance(response.get("configuration"), (dict, list)): + return None + return response + + +def _fetch_warning(component_id: str, config_id: str, exc: Exception) -> dict[str, str]: + """Build the push-envelope warning for an unstampable baseline.""" + message = ( + f"Could not read back {component_id}/{config_id} after the write, so the " + f"manifest baseline was left unchanged; run 'kbagent sync pull' to refresh " + f"it. Cause: {exc}" + ) + logger.warning("%s", message) + return { + "change_type": "baseline_stamp", + "component_id": component_id, + "config_id": config_id, + "message": message, + } + + +def config_baseline( + client: Any, + *, + component_id: str, + config_id: str, + branch_id: int | None, + response: Any, +) -> BaselineStamp: + """Compute the post-write baseline hash for a configuration. + + Prefers the mutation response; falls back to ``get_config_detail`` when it + is partial. A failed fetch yields an unstamped result plus a warning. + """ + payload = _usable_payload(response) + if payload is None: + try: + payload = _usable_payload( + client.get_config_detail( + component_id=component_id, + config_id=config_id, + branch_id=branch_id, + ) + ) + except Exception as exc: + return BaselineStamp(warning=_fetch_warning(component_id, config_id, exc)) + if payload is None: + return BaselineStamp( + warning=_fetch_warning( + component_id, config_id, ValueError("configuration missing from the response") + ) + ) + return BaselineStamp( + cfg_hash=config_hash(api_config_to_local(component_id, payload, config_id)), + stamped=True, + ) + + +def row_baseline( + client: Any, + *, + component_id: str, + config_id: str, + row_id: str, + branch_id: int | None, + response: Any, +) -> BaselineStamp: + """Compute the post-write baseline hash for a configuration row. + + Same contract as :func:`config_baseline`, via ``get_config_row``. Row + hashes are NOT script-normalized (``api_row_to_local`` never was), so no + shape migration applies to them -- but a partial response would still drop + ``isDisabled`` and strand a permanent phantom diff, which is why rows take + the identical read-back path. + """ + payload = _usable_payload(response) + if payload is None: + try: + payload = _usable_payload( + client.get_config_row( + component_id=component_id, + config_id=config_id, + row_id=row_id, + branch_id=branch_id, + ) + ) + except Exception as exc: + return BaselineStamp(warning=_fetch_warning(component_id, f"{config_id}/{row_id}", exc)) + if payload is None: + return BaselineStamp( + warning=_fetch_warning( + component_id, + f"{config_id}/{row_id}", + ValueError("configuration missing from the response"), + ) + ) + return BaselineStamp( + cfg_hash=config_hash(api_row_to_local(payload, component_id)), + stamped=True, + ) + + +def apply_stamp(metadata: dict[str, Any], stamp: BaselineStamp) -> None: + """Record a baseline on a manifest entry's metadata dict. + + A no-op when the stamp failed: the previous ``pull_config_hash`` (and its + version marker, if any) survives untouched, so the state stays *visibly* + stale rather than confidently wrong. + """ + if not stamp.stamped: + return + metadata["pull_config_hash"] = stamp.cfg_hash + metadata[CONFIG_HASH_VERSION_KEY] = CONFIG_HASH_VERSION + + +# --------------------------------------------------------------------------- +# Migration: manifests written before the shape change +# --------------------------------------------------------------------------- + + +def is_legacy_hash( + stored: str, + *, + component_id: str, + config_id: str, + raw_remote: dict[str, Any], +) -> bool: + """True iff *stored* is the pre-#686 hash of this very remote config. + + Computed from the RAW API config through the old collapse normalization -- + never by re-collapsing already-split data, which would not reproduce the + same bytes. A match proves the entry differs from the current producer in + the script shape ALONE: every other field is pinned by the same hash, so + the leniency cannot mask real drift. + """ + if not stored: + return False + legacy = api_config_to_local(component_id, raw_remote, config_id, legacy_scripts=True) + return stored == config_hash(legacy) + + +def effective_stored_hash( + metadata: dict[str, Any], + *, + component_id: str, + config_id: str, + raw_remote: dict[str, Any] | None, + remote_local: dict[str, Any] | None, +) -> str: + """Return the entry's baseline hash, upgraded when it is legacy-shaped. + + Versioned entries (``config_hash_version`` present) are compared strictly. + An unversioned entry whose stored hash is the legacy-shape hash of the + CURRENT remote is treated as if it held the new-shape hash -- that is the + whole migration: one ``sync pull`` re-stamps it and the leniency stops + applying. Anything else (a real remote edit, a real local edit) is + returned untouched, so the leniency cannot mask drift. + """ + stored = str(metadata.get("pull_config_hash", "") or "") + if not stored or metadata.get(CONFIG_HASH_VERSION_KEY): + return stored + if raw_remote is None or remote_local is None: + return stored + remote_hash = config_hash(remote_local) + if stored == remote_hash: + return stored + if is_legacy_hash( + stored, component_id=component_id, config_id=config_id, raw_remote=raw_remote + ): + return remote_hash + return stored + + +def needs_shape_migration( + metadata: dict[str, Any], + *, + component_id: str, + config_id: str, + raw_remote: dict[str, Any], + api_cfg_hash: str, +) -> bool: + """True iff this entry's baseline is a pre-#686 hash of the same remote. + + The pull-side counterpart of :func:`effective_stored_hash`: the remote is + unchanged, only the recorded shape is old, so the pull must re-run + extraction (to write the boundary markers) and re-stamp -- unless the + local files were edited, in which case they are preserved untouched. + """ + stored = str(metadata.get("pull_config_hash", "") or "") + if not stored or metadata.get(CONFIG_HASH_VERSION_KEY) or stored == api_cfg_hash: + return False + return is_legacy_hash( + stored, component_id=component_id, config_id=config_id, raw_remote=raw_remote + ) + + +def extras_modified(service: SyncService, config_dir: Path, extra_hashes: dict[str, str]) -> bool: + """True iff any companion file recorded at pull time changed on disk. + + The pull overwrite-guard has only ever compared ``_config.yml``, so an + edited ``transform.sql`` beside an untouched ``_config.yml`` was + overwritten. That is pre-existing behaviour everywhere EXCEPT the shape + migration, which rewrites code files for a remote that did not change -- + there, silently discarding a local edit would be new damage, so the + migration checks the companions too. + """ + for fname, stored_hash in (extra_hashes or {}).items(): + fpath = config_dir / fname + if not fpath.exists() or service._file_hash(fpath) != stored_hash: + return True + return False + + +def _scripts_by_code(config_data: dict[str, Any]) -> list[list[Any]] | None: + """Collect every ``blocks[].codes[].script`` array, in document order.""" + parameters = config_data.get("parameters") + if not isinstance(parameters, dict): + return None + blocks = parameters.get("blocks") + if not isinstance(blocks, list): + return None + scripts: list[list[Any]] = [] + for block in blocks: + if not isinstance(block, dict): + return None + for code in block.get("codes") or []: + if not isinstance(code, dict): + return None + script = code.get("script") + scripts.append(list(script) if isinstance(script, list) else []) + return scripts + + +def _boundary_only_difference(local: dict[str, Any], remote: dict[str, Any]) -> bool: + """True iff local is exactly the marker-less rendering of the remote. + + Any other difference -- a real SQL edit, a reindent, a different block + layout -- is a genuine local change and must push normally. + """ + local_scripts = _scripts_by_code(local) + remote_scripts = _scripts_by_code(remote) + if local_scripts is None or remote_scripts is None: + return False + if len(local_scripts) != len(remote_scripts): + return False + found = False + for local_script, remote_script in zip(local_scripts, remote_scripts, strict=True): + if local_script == remote_script: + continue + if marker_less_roundtrip(remote_script) != local_script: + return False # a genuine edit -- let it push + found = True + return found + + +def raise_on_legacy_boundary( + service: SyncService, + client: Any, + *, + component_id: str, + config_id: str, + config_dir: Path, + manifest: Manifest, + branch_id: int | None, +) -> None: + """Refuse a push that would collapse the remote's statement boundaries. + + A tree pulled before #686 has a ``transform.sql`` with no boundary markers. + When the remote's ``script[]`` elements carry no trailing semicolons, that + file cannot express where one statement ends -- so ``merge_code_files`` + returns ONE element holding everything and the push silently rewrites + production into the ``MULTI_STATEMENT_COUNT=1`` crash shape of issues + #119/#120/#274, while ``sync diff`` reports "in sync". + + Scope is deliberately narrow: SQL transformations only, only entries with + no ``config_hash_version`` (a pulled-since-#686 tree carries markers and + cannot hit this), and only when the statement TEXT is identical while the + element counts differ. A genuine edit proceeds. A failed remote read + proceeds too -- this is a safety net, not a gate. + + Raises: + KeboolaApiError: with :data:`ErrorCode.SYNC_LEGACY_BOUNDARY`, caught by + the push loop and accumulated as a per-change error. + """ + if not is_sql_transformation_component(component_id): + return + entry = next( + ( + c + for c in manifest.configurations + if c.component_id == component_id and c.id == config_id + ), + None, + ) + if entry is None or entry.metadata.get(CONFIG_HASH_VERSION_KEY): + return + local_data = service._read_config_file(config_dir) + if local_data is None: + return + try: + remote_raw = client.get_config_detail( + component_id=component_id, config_id=config_id, branch_id=branch_id + ) + except Exception: + logger.debug("Legacy boundary guard skipped: %s/%s unreadable", component_id, config_id) + return + if _usable_payload(remote_raw) is None: + return + merge_code_files(component_id, local_data, config_dir) + if not _boundary_only_difference( + local_data, api_config_to_local(component_id, remote_raw, config_id) + ): + return + raise KeboolaApiError( + message=( + f"Refusing to push {component_id}/{config_id}: this working tree predates " + f"statement-boundary tracking, so pushing it would merge separate SQL " + f"statements into one (the MULTI_STATEMENT_COUNT=1 failure). The content " + f"is otherwise identical to the remote. Run 'kbagent sync pull' for this " + f"project first, then push again." + ), + status_code=0, + error_code=ErrorCode.SYNC_LEGACY_BOUNDARY, + ) + + +# --------------------------------------------------------------------------- +# Force-pull conflict detection +# --------------------------------------------------------------------------- + + +def _is_conflict( + service: SyncService, + config_file: Path, + old_pull_hash: str, + old_cfg_hash: str, + api_cfg_hash: str, +) -> bool: + """True iff the file is locally modified AND the remote also changed. + + A 3-way conflict needs both a stored ``pull_hash`` (the synced file + state) and a stored ``pull_config_hash`` (the synced remote state); + without either we cannot prove a conflict, so return False -- be + conservative, ``--force`` must not abort on incomplete bookkeeping. + A missing local file is not a content conflict (nothing to lose). + """ + if not old_pull_hash or not old_cfg_hash: + return False + if not config_file.exists(): + return False + locally_modified = service._file_hash(config_file) != old_pull_hash + remote_changed = api_cfg_hash != old_cfg_hash + return locally_modified and remote_changed + + +def detect_force_pull_conflicts( + service: SyncService, + components: list[dict[str, Any]], + branch_dir: Path, + *, + ignored_components: frozenset[str], + existing_keys: set[str], + existing_paths: dict[str, str], + existing_file_hashes: dict[str, str], + existing_metadata: dict[str, dict[str, Any]], + existing_rows: dict[str, dict[str, str]], +) -> list[dict[str, str]]: + """Return configs/rows a ``--force`` pull would clobber as conflicts. + + A *conflict* is a config (or row) that is BOTH locally modified (its + on-disk ``_config.yml`` hash differs from the manifest ``pull_hash``) + AND changed on the remote since the last pull (the freshly fetched + config hash differs from ``pull_config_hash``). That is the only case + where ``--force`` must stop: local and remote have diverged, so neither + "take remote" nor "keep local" is safe without the user deciding. + + Configs only locally modified (remote unchanged) are NOT conflicts -- + ``--force`` preserves them so their pending delta stays pushable. + Brand-new remote configs and configs whose local file is missing are + skipped (nothing local to lose). Read-only: hashes but writes nothing. + + The stored config hash goes through :func:`effective_stored_hash`, so a + baseline written before the script-shape change (issue #686) is not + mistaken for a remote edit and does not abort an otherwise clean + ``--force`` pull. Rows are compared strictly -- their hash producer never + changed. + + ``ignored_components`` is the caller's pre-computed effective set + (``SyncService._effective_ignored_components``: the hardcoded always-ignored + components plus the manifest's ``ignoredComponents``, issue #689) so this + guard can never disagree with pull/diff about what is ignored. + """ + conflicts: list[dict[str, str]] = [] + for component in components: + component_id = component.get("id", "") + if component_id in ignored_components: + continue + for cfg in component.get("configurations", []): + config_id = str(cfg.get("id", "")) + lookup_key = f"{component_id}/{config_id}" + if lookup_key not in existing_keys: + continue # brand-new remote config -- nothing local to lose + + rel_path = existing_paths.get(lookup_key, "") + remote_local = api_config_to_local(component_id, cfg, config_id) + if _is_conflict( + service, + branch_dir / rel_path / CONFIG_FILENAME, + existing_file_hashes.get(lookup_key, ""), + effective_stored_hash( + existing_metadata.get(lookup_key, {}), + component_id=component_id, + config_id=config_id, + raw_remote=cfg, + remote_local=remote_local, + ), + config_hash(remote_local), + ): + conflicts.append( + { + "scope": "config", + "component_id": component_id, + "config_id": config_id, + "config_name": str(cfg.get("name", "untitled")), + "path": rel_path, + } + ) + + # Row-level conflicts (same 3-way rule, per row). + config_dir = branch_dir / rel_path + for row in cfg.get("rows", []): + row_id = str(row.get("id", "")) + existing_row = existing_rows.get(f"{component_id}/{config_id}/{row_id}") + if not existing_row: + continue + row_rel_path = existing_row.get("path", "") + if _is_conflict( + service, + config_dir / row_rel_path / CONFIG_FILENAME, + existing_row.get("pull_hash", ""), + existing_row.get("pull_config_hash", ""), + config_hash(api_row_to_local(row, component_id)), + ): + conflicts.append( + { + "scope": "row", + "component_id": component_id, + "config_id": config_id, + "config_name": ( + f"{cfg.get('name', 'untitled')}/{row.get('name', 'untitled')}" + ), + "path": f"{rel_path}/{row_rel_path}", + "row_id": row_id, + } + ) + return conflicts diff --git a/src/keboola_agent_cli/services/_sync_bindings.py b/src/keboola_agent_cli/services/_sync_bindings.py index bcb34139..e5417972 100644 --- a/src/keboola_agent_cli/services/_sync_bindings.py +++ b/src/keboola_agent_cli/services/_sync_bindings.py @@ -26,6 +26,7 @@ from ..sync.code_extraction import merge_code_files from ..sync.config_format import local_config_to_api from ..sync.manifest import Manifest +from ._sync_baseline import apply_stamp, config_baseline from ._sync_models import ( FLOW_COMPONENT_ID, VARIABLES_COMPONENT_ID, @@ -128,6 +129,7 @@ def resolve_variable_bindings( row_ulid=row_ulid, manifest=manifest, branch_id=branch_id, + warnings=result.warnings, ) except KeboolaApiError as exc: result.errors.append( @@ -255,6 +257,7 @@ def _apply_variable_binding( row_ulid: str | None, manifest: Manifest, branch_id: int | None, + warnings: list[dict[str, str]], ) -> None: """PUT the resolved variables link, rewrite local, refresh manifest hashes. @@ -270,7 +273,7 @@ def _apply_variable_binding( if row_ulid: configuration["variables_values_id"] = row_ulid - client.update_config( + response = client.update_config( component_id=created.component_id, config_id=created.config_id, configuration=configuration, @@ -295,7 +298,44 @@ def _apply_variable_binding( # config_hash includes _configuration_extra, so refresh the stored # hashes from the post-rewrite disk state or sync diff sees a conflict. + _refresh_binding_hashes( + service, + client, + created=created, + manifest=manifest, + branch_id=branch_id, + response=response, + warnings=warnings, + ) + + +def _refresh_binding_hashes( + service: SyncService, + client: Any, + *, + created: CreatedConfig, + manifest: Manifest, + branch_id: int | None, + response: Any, + warnings: list[dict[str, str]], +) -> None: + """Re-stamp a rebound config's manifest bookkeeping after the backfill PUT. + + ``config_hash`` includes ``_configuration_extra``, which both backfills + rewrite, so the stored hashes must be refreshed or ``sync diff`` reports a + conflict. The config hash comes from the API's view of the config it just + wrote (issue #686); the file hashes describe the local files. + """ hashes = service._compute_config_hashes(created.config_dir, created.component_id) + stamp = config_baseline( + client, + component_id=created.component_id, + config_id=created.config_id, + branch_id=branch_id, + response=response, + ) + if stamp.warning is not None: + warnings.append(stamp.warning) target_branch = branch_id or 0 for cfg in manifest.configurations: if ( @@ -304,8 +344,8 @@ def _apply_variable_binding( and cfg.id == created.config_id ): cfg.metadata["pull_hash"] = hashes.file_hash - cfg.metadata["pull_config_hash"] = hashes.cfg_hash cfg.metadata["pull_extra_hashes"] = hashes.extra_hashes + apply_stamp(cfg.metadata, stamp) break @@ -365,6 +405,7 @@ def resolve_flow_task_bindings( local_data=local_data, manifest=manifest, branch_id=branch_id, + warnings=result.warnings, ) except KeboolaApiError as exc: result.errors.append( @@ -416,6 +457,7 @@ def _apply_flow_task_binding( local_data: dict[str, Any], manifest: Manifest, branch_id: int | None, + warnings: list[dict[str, str]], ) -> None: """PUT a remapped flow, rewrite local ``_config.yml``, refresh hashes. @@ -428,7 +470,7 @@ def _apply_flow_task_binding( merge_code_files(created.component_id, merged, created.config_dir) _name, _description, configuration = local_config_to_api(merged) - client.update_config( + response = client.update_config( component_id=created.component_id, config_id=created.config_id, configuration=configuration, @@ -442,15 +484,12 @@ def _apply_flow_task_binding( ) service._write_config_file(created.config_dir, local_data) - hashes = service._compute_config_hashes(created.config_dir, created.component_id) - target_branch = branch_id or 0 - for cfg in manifest.configurations: - if ( - cfg.branch_id == target_branch - and cfg.component_id == created.component_id - and cfg.id == created.config_id - ): - cfg.metadata["pull_hash"] = hashes.file_hash - cfg.metadata["pull_config_hash"] = hashes.cfg_hash - cfg.metadata["pull_extra_hashes"] = hashes.extra_hashes - break + _refresh_binding_hashes( + service, + client, + created=created, + manifest=manifest, + branch_id=branch_id, + response=response, + warnings=warnings, + ) diff --git a/src/keboola_agent_cli/services/_sync_models.py b/src/keboola_agent_cli/services/_sync_models.py index aae3c66a..2307d322 100644 --- a/src/keboola_agent_cli/services/_sync_models.py +++ b/src/keboola_agent_cli/services/_sync_models.py @@ -61,9 +61,11 @@ class VariableBindingResult: local ``_configuration_extra`` were rebound to ULIDs (drives the manifest-dirty flag). ``errors`` accumulates unresolved links so the push envelope surfaces them instead of leaving a broken link silently. + ``warnings`` carries non-fatal baseline-stamping notices (issue #686). """ errors: list[dict[str, str]] = field(default_factory=list) + warnings: list[dict[str, str]] = field(default_factory=list) configs_rewritten: int = 0 @@ -74,10 +76,12 @@ class FlowBindingResult: ``configs_rewritten`` counts flows whose task ``configId``s were remapped to ULIDs (drives the manifest-dirty flag); ``tasks_remapped`` is the total task references rewritten; ``errors`` accumulates PUT failures so the push - envelope surfaces them. + envelope surfaces them. ``warnings`` carries non-fatal baseline-stamping + notices (issue #686). """ errors: list[dict[str, str]] = field(default_factory=list) + warnings: list[dict[str, str]] = field(default_factory=list) configs_rewritten: int = 0 tasks_remapped: int = 0 diff --git a/src/keboola_agent_cli/services/_sync_push_ops.py b/src/keboola_agent_cli/services/_sync_push_ops.py index 4bf104fd..4ffa8d9e 100644 --- a/src/keboola_agent_cli/services/_sync_push_ops.py +++ b/src/keboola_agent_cli/services/_sync_push_ops.py @@ -20,9 +20,9 @@ from ..errors import ErrorCode, KeboolaApiError from ..sync.code_extraction import merge_code_files from ..sync.config_format import local_config_to_api, local_row_to_api -from ..sync.diff_engine import config_hash from ..sync.manifest import Manifest, ManifestConfiguration from ._encryption import encrypt_secrets_in_config +from ._sync_baseline import apply_stamp, row_baseline from ._sync_writeback import writeback_after_push, writeback_create_row_in_manifest if TYPE_CHECKING: @@ -44,6 +44,7 @@ def push_row_change( manifest: Manifest, branch_id: int | None, allow_plaintext_fallback: bool = False, + warnings: list[dict[str, str]] | None = None, ) -> str | None: """Dispatch a single row-level change (added/modified/deleted) to the API. @@ -57,6 +58,11 @@ def push_row_change( before dispatch, so both the manifest parent lookup and ``create_config_row(config_id=...)`` hit the real config (KFR-05). + ``warnings`` accumulates non-fatal baseline-stamping warnings (issue #686) + for the push envelope; when the API state cannot be read back after the + write, the row's ``pull_config_hash`` is left untouched rather than + recomputed from disk. + Returns the API-assigned row id on ``added`` (so the caller can map placeholder -> ULID for variable-link backfill), else ``None``. """ @@ -108,6 +114,7 @@ def push_row_change( branch_id=branch_id, project_id=project_id, allow_plaintext_fallback=allow_plaintext_fallback, + warnings=warnings, ) if change_type == "modified": @@ -122,6 +129,7 @@ def push_row_change( branch_id=branch_id, project_id=project_id, allow_plaintext_fallback=allow_plaintext_fallback, + warnings=warnings, ) return None @@ -140,6 +148,7 @@ def _push_create_row( branch_id: int | None, project_id: int | None, allow_plaintext_fallback: bool, + warnings: list[dict[str, str]] | None = None, ) -> str: """POST a new row; record API-assigned id + hashes in the parent's row list. @@ -178,14 +187,24 @@ def _push_create_row( row_file = row_dir / CONFIG_FILENAME new_file_hash = service._file_hash(row_file) if row_file.exists() else "" - cfg_hash_value = config_hash(pristine_data) - writeback_create_row_in_manifest( + stamp = row_baseline( + client, + component_id=component_id, + config_id=parent_config_id, + row_id=new_row_id, + branch_id=branch_id, + response=result, + ) + row_entry = writeback_create_row_in_manifest( parent=parent, row_path_str=row_path_str, new_row_id=new_row_id, file_hash=new_file_hash, - cfg_hash=cfg_hash_value, + cfg_hash=stamp.cfg_hash, ) + apply_stamp(row_entry.metadata, stamp) + if stamp.warning is not None and warnings is not None: + warnings.append(stamp.warning) return new_row_id @@ -201,8 +220,14 @@ def push_update_row( branch_id: int | None, project_id: int | None, allow_plaintext_fallback: bool, + warnings: list[dict[str, str]] | None = None, ) -> None: - """PUT an existing row; refresh its hashes in the parent's row list.""" + """PUT an existing row; refresh its hashes in the parent's row list. + + The baseline comes from the API's own view of the row (issue #686), so a + row disabled remotely whose local file carries no ``is_disabled`` key does + not leave a permanent phantom diff behind. + """ local_data = service._read_config_file(row_dir) if local_data is None: raise FileNotFoundError(f"Row file not found: {row_dir / CONFIG_FILENAME}") @@ -217,7 +242,7 @@ def push_update_row( allow_plaintext_fallback=allow_plaintext_fallback, ) - client.update_config_row( + result = client.update_config_row( component_id=component_id, config_id=parent_config_id, row_id=row_id, @@ -236,11 +261,20 @@ def push_update_row( row_file = row_dir / CONFIG_FILENAME new_file_hash = service._file_hash(row_file) if row_file.exists() else "" - cfg_hash_value = config_hash(pristine_data) + stamp = row_baseline( + client, + component_id=component_id, + config_id=parent_config_id, + row_id=row_id, + branch_id=branch_id, + response=result, + ) + if stamp.warning is not None and warnings is not None: + warnings.append(stamp.warning) for r in parent.rows: if r.id == row_id: r.metadata["pull_hash"] = new_file_hash - r.metadata["pull_config_hash"] = cfg_hash_value + apply_stamp(r.metadata, stamp) break @@ -331,8 +365,12 @@ def push_update( branch_id: int | None, *, allow_plaintext_fallback: bool = False, -) -> None: - """Update an existing config from a local _config.yml file.""" +) -> dict[str, Any]: + """Update an existing config from a local _config.yml file. + + Returns the API response so the caller can stamp the manifest baseline + from the remote's own view of the config (issue #686). + """ branch_path = service._resolve_source_branch_path(manifest, project_root, branch_id) config_dir = project_root / branch_path / config_path_str local_data = service._read_config_file(config_dir) @@ -358,7 +396,7 @@ def push_update( allow_plaintext_fallback=allow_plaintext_fallback, ) - client.update_config( + result = client.update_config( component_id=component_id, config_id=config_id, name=name, @@ -375,3 +413,5 @@ def push_update( # Write back: update local file with encrypted secrets. Use pristine_data so # blocks/code stay only in their code files. writeback_after_push(service, pristine_data, config_dir, config_id, configuration) + + return result if isinstance(result, dict) else {} diff --git a/src/keboola_agent_cli/services/_sync_writeback.py b/src/keboola_agent_cli/services/_sync_writeback.py index 2524fc3b..d3e737a3 100644 --- a/src/keboola_agent_cli/services/_sync_writeback.py +++ b/src/keboola_agent_cli/services/_sync_writeback.py @@ -15,7 +15,8 @@ from ..errors import KeboolaApiError from ..sync.manifest import ManifestConfigRow, ManifestConfiguration from ._encryption import apply_encrypted_to_local -from ._sync_models import WritebackResult +from ._sync_baseline import apply_stamp, config_baseline +from ._sync_models import LocalConfigHashes, WritebackResult if TYPE_CHECKING: from ..sync.manifest import Manifest @@ -24,6 +25,96 @@ logger = logging.getLogger(__name__) +def stamp_created_config( + client: Any, + *, + manifest: Manifest, + component_id: str, + branch_id: int | None, + config_path_str: str, + new_id: str, + hashes: LocalConfigHashes, + response: Any, + warnings: list[dict[str, str]], +) -> WritebackResult: + """Record a created config with an API-derived ``pull_config_hash`` (#686). + + ``pull_hash`` describes the local file and stays disk-derived; the config + hash is the API's own view of what was just written, so the next + ``sync diff`` compares like with like. + """ + stamp = config_baseline( + client, + component_id=component_id, + config_id=new_id, + branch_id=branch_id, + response=response, + ) + if stamp.warning is not None: + warnings.append(stamp.warning) + writeback = writeback_create_config_in_manifest( + manifest=manifest, + component_id=component_id, + branch_id=branch_id, + config_path_str=config_path_str, + new_id=new_id, + file_hash=hashes.file_hash, + cfg_hash=stamp.cfg_hash, + ) + apply_stamp(writeback.entry.metadata, stamp) + return writeback + + +def stamp_updated_config( + client: Any, + *, + manifest: Manifest, + component_id: str, + config_id: str, + branch_id: int | None, + config_path_str: str, + hashes: LocalConfigHashes, + response: Any, + warnings: list[dict[str, str]], +) -> None: + """Refresh a pushed config's manifest bookkeeping from the API state (#686). + + When the API state cannot be established (partial response AND a failed + read-back), ``pull_config_hash`` is left exactly as it was: visibly stale + beats confidently wrong, and a disk-derived value is what created the + phantom drift in the first place. + + An update that finds no manifest entry is an adopted-by-id config (issue + #497) -- an untracked local file whose ``_keboola.config_id`` resolved on + the branch. It is registered here so later diffs read a stable entry. + """ + stamp = config_baseline( + client, + component_id=component_id, + config_id=config_id, + branch_id=branch_id, + response=response, + ) + if stamp.warning is not None: + warnings.append(stamp.warning) + for cfg in manifest.configurations: + if cfg.component_id == component_id and cfg.id == config_id: + cfg.metadata["pull_hash"] = hashes.file_hash + cfg.metadata["pull_extra_hashes"] = hashes.extra_hashes + apply_stamp(cfg.metadata, stamp) + return + entry = writeback_create_config_in_manifest( + manifest=manifest, + component_id=component_id, + branch_id=branch_id, + config_path_str=config_path_str, + new_id=config_id, + file_hash=hashes.file_hash, + cfg_hash=stamp.cfg_hash, + ).entry + apply_stamp(entry.metadata, stamp) + + def writeback_create_config_in_manifest( *, manifest: Manifest, diff --git a/src/keboola_agent_cli/services/sync_service.py b/src/keboola_agent_cli/services/sync_service.py index d2950cd1..f54ed01e 100644 --- a/src/keboola_agent_cli/services/sync_service.py +++ b/src/keboola_agent_cli/services/sync_service.py @@ -18,6 +18,8 @@ ALWAYS_IGNORED_COMPONENTS, BRANCH_MAPPING_FILENAME, CONFIG_FILENAME, + CONFIG_HASH_VERSION, + CONFIG_HASH_VERSION_KEY, DEFAULT_JOBS_PER_CONFIG, DEFAULT_MAX_SAMPLES, DEFAULT_SAMPLE_LIMIT, @@ -68,6 +70,13 @@ encrypt_secrets_in_config, find_plaintext_secret_keys, ) +from ._sync_baseline import ( + detect_force_pull_conflicts, + effective_stored_hash, + extras_modified, + needs_shape_migration, + raise_on_legacy_boundary, +) from ._sync_bindings import resolve_flow_task_bindings, resolve_variable_bindings from ._sync_branch import ( branch_link as _branch_link, @@ -98,7 +107,8 @@ ) from ._sync_writeback import ( propagate_kbc_metadata, - writeback_create_config_in_manifest, + stamp_created_config, + stamp_updated_config, ) from .base import BaseService @@ -438,29 +448,6 @@ def _local_files_match_pull_state( return False return True - def _is_conflict( - self, - config_file: Path, - old_pull_hash: str, - old_cfg_hash: str, - api_cfg_hash: str, - ) -> bool: - """True iff the file is locally modified AND the remote also changed. - - A 3-way conflict needs both a stored ``pull_hash`` (the synced file - state) and a stored ``pull_config_hash`` (the synced remote state); - without either we cannot prove a conflict, so return False -- be - conservative, ``--force`` must not abort on incomplete bookkeeping. - A missing local file is not a content conflict (nothing to lose). - """ - if not old_pull_hash or not old_cfg_hash: - return False - if not config_file.exists(): - return False - locally_modified = self._file_hash(config_file) != old_pull_hash - remote_changed = api_cfg_hash != old_cfg_hash - return locally_modified and remote_changed - @staticmethod def _effective_ignored_components(manifest: Manifest) -> frozenset[str]: """Components excluded from this working tree's sync operations. @@ -476,89 +463,6 @@ def _effective_ignored_components(manifest: Manifest) -> frozenset[str]: """ return ALWAYS_IGNORED_COMPONENTS | frozenset(manifest.ignored_components) - def _detect_force_pull_conflicts( - self, - components: list[dict[str, Any]], - branch_dir: Path, - *, - ignored_components: frozenset[str], - existing_keys: set[str], - existing_paths: dict[str, str], - existing_file_hashes: dict[str, str], - existing_config_hashes: dict[str, str], - existing_rows: dict[str, dict[str, str]], - ) -> list[dict[str, str]]: - """Return configs/rows a ``--force`` pull would clobber as conflicts. - - A *conflict* is a config (or row) that is BOTH locally modified (its - on-disk ``_config.yml`` hash differs from the manifest ``pull_hash``) - AND changed on the remote since the last pull (the freshly fetched - config hash differs from ``pull_config_hash``). That is the only case - where ``--force`` must stop: local and remote have diverged, so neither - "take remote" nor "keep local" is safe without the user deciding. - - Configs only locally modified (remote unchanged) are NOT conflicts -- - ``--force`` preserves them so their pending delta stays pushable. - Brand-new remote configs and configs whose local file is missing are - skipped (nothing local to lose). Read-only: hashes but writes nothing. - """ - conflicts: list[dict[str, str]] = [] - for component in components: - component_id = component.get("id", "") - if component_id in ignored_components: - continue - for cfg in component.get("configurations", []): - config_id = str(cfg.get("id", "")) - lookup_key = f"{component_id}/{config_id}" - if lookup_key not in existing_keys: - continue # brand-new remote config -- nothing local to lose - - rel_path = existing_paths.get(lookup_key, "") - api_cfg_hash = config_hash(api_config_to_local(component_id, cfg, config_id)) - if self._is_conflict( - branch_dir / rel_path / CONFIG_FILENAME, - existing_file_hashes.get(lookup_key, ""), - existing_config_hashes.get(lookup_key, ""), - api_cfg_hash, - ): - conflicts.append( - { - "scope": "config", - "component_id": component_id, - "config_id": config_id, - "config_name": str(cfg.get("name", "untitled")), - "path": rel_path, - } - ) - - # Row-level conflicts (same 3-way rule, per row). - config_dir = branch_dir / rel_path - for row in cfg.get("rows", []): - row_id = str(row.get("id", "")) - existing_row = existing_rows.get(f"{component_id}/{config_id}/{row_id}") - if not existing_row: - continue - row_rel_path = existing_row.get("path", "") - if self._is_conflict( - config_dir / row_rel_path / CONFIG_FILENAME, - existing_row.get("pull_hash", ""), - existing_row.get("pull_config_hash", ""), - config_hash(api_row_to_local(row, component_id)), - ): - conflicts.append( - { - "scope": "row", - "component_id": component_id, - "config_id": config_id, - "config_name": ( - f"{cfg.get('name', 'untitled')}/{row.get('name', 'untitled')}" - ), - "path": f"{rel_path}/{row_rel_path}", - "row_id": row_id, - } - ) - return conflicts - def pull( self, alias: str, @@ -673,6 +577,11 @@ def pull( f"{c.component_id}/{c.id}": c.path for c in manifest.configurations } existing_keys: set[str] = set(existing_paths.keys()) + # Full metadata per entry -- the shape-migration checks need the + # ``config_hash_version`` marker alongside the hashes (issue #686). + existing_metadata: dict[str, dict[str, Any]] = { + f"{c.component_id}/{c.id}": c.metadata for c in manifest.configurations + } existing_config_hashes: dict[str, str] = { f"{c.component_id}/{c.id}": c.metadata.get("pull_config_hash", "") for c in manifest.configurations @@ -721,14 +630,15 @@ def pull( # ``--theirs`` skips the guard entirely: the user explicitly asked for # remote to win, so conflicts are resolved by overwriting, not aborting. if force and not theirs: - conflicts = self._detect_force_pull_conflicts( + conflicts = detect_force_pull_conflicts( + self, components, branch_dir, ignored_components=ignored_components, existing_keys=existing_keys, existing_paths=existing_paths, existing_file_hashes=existing_file_hashes, - existing_config_hashes=existing_config_hashes, + existing_metadata=existing_metadata, existing_rows=existing_rows, ) if conflicts: @@ -830,6 +740,22 @@ def pull( if config_file.exists(): current_file_hash = self._file_hash(config_file) locally_modified = current_file_hash != old_file_hash + # Shape migration (issue #686): the remote is unchanged, only + # the recorded hash shape is old, so this pull re-extracts + # (writing the boundary markers) and re-stamps. Because the + # rewrite is not driven by a remote change, an edited + # companion file must be preserved too -- the ordinary + # overwrite-guard above only ever looks at ``_config.yml``. + if not locally_modified and needs_shape_migration( + existing_metadata.get(lookup_key, {}), + component_id=component_id, + config_id=config_id, + raw_remote=cfg, + api_cfg_hash=api_cfg_hash, + ): + locally_modified = extras_modified( + self, config_dir, existing_extra_hashes.get(lookup_key, {}) + ) remote_unchanged = False # set in else branch; default for locally_modified path if locally_modified and not dry_run: @@ -1029,6 +955,12 @@ def pull( "pull_hash": old_pull_hash, "pull_config_hash": old_cfg_hash, } + # The preserved hash was NOT produced by the current + # producer, so its version marker is carried over verbatim + # (absent stays absent) -- never stamped onto a legacy hash. + old_version = existing_metadata.get(lookup_key, {}).get(CONFIG_HASH_VERSION_KEY) + if old_version: + cfg_metadata[CONFIG_HASH_VERSION_KEY] = old_version else: # Compute hashes for all extracted files extra_hashes: dict[str, str] = {} @@ -1047,6 +979,9 @@ def pull( "pull_hash": file_hash, "pull_config_hash": pull_cfg_hash, "pull_extra_hashes": extra_hashes, + # Freshly computed with the current producer, so the + # shape version is stamped alongside it (issue #686). + CONFIG_HASH_VERSION_KEY: CONFIG_HASH_VERSION, } new_configurations.append( ManifestConfiguration( @@ -1277,6 +1212,10 @@ def diff( # remote_rows: "{component_id}/{parent_config_id}/rows/{row_id}" -> row data remote_configs: dict[str, dict[str, Any]] = {} remote_rows: dict[str, dict[str, Any]] = {} + # Raw (unconverted) API configs, kept so a manifest entry without + # ``config_hash_version`` can be checked against the pre-#686 hash of + # this very config -- the migration leniency in ``effective_stored_hash``. + remote_raw: dict[str, dict[str, Any]] = {} for component in components: component_id = component.get("id", "") if component_id in ignored_components: @@ -1285,6 +1224,7 @@ def diff( config_id = str(cfg.get("id", "")) key = f"{component_id}/{config_id}" remote_configs[key] = api_config_to_local(component_id, cfg, config_id) + remote_raw[key] = cfg for row in cfg.get("rows", []): row_id = str(row.get("id", "")) row_key = f"{component_id}/{config_id}/rows/{row_id}" @@ -1321,6 +1261,7 @@ def diff( local_configs: list[dict[str, Any]] = [] file_unchanged: dict[str, bool] = {} local_override_hashes: dict[str, str] = {} + stored_hashes: dict[str, str] = {} for cfg in scope.in_tree: config_dir = project_root / source_branch_path / cfg.path local_data = self._read_config_file(config_dir) @@ -1351,11 +1292,20 @@ def diff( is_unchanged = config_unchanged and extras_unchanged file_unchanged[key] = is_unchanged - if is_unchanged: + # Baseline hash, leniently upgraded for entries written before the + # script-shape change (issue #686). Strict for versioned entries. + stored_cfg_hash = effective_stored_hash( + cfg.metadata, + component_id=cfg.component_id, + config_id=cfg.id, + raw_remote=remote_raw.get(key), + remote_local=remote_configs.get(key), + ) + stored_hashes[key] = stored_cfg_hash + + if is_unchanged and stored_cfg_hash: # All files match pull state -- use stored API hash - stored_cfg_hash = cfg.metadata.get("pull_config_hash", "") - if stored_cfg_hash: - local_override_hashes[key] = stored_cfg_hash + local_override_hashes[key] = stored_cfg_hash # Always merge for local_data (needed for deep_diff details) merge_code_files(cfg.component_id, local_data, config_dir) @@ -1431,7 +1381,7 @@ def diff( base_hashes: dict[str, str] = {} for cfg in scope.in_tree: key = f"{cfg.component_id}/{cfg.id}" - pch = cfg.metadata.get("pull_config_hash") + pch = stored_hashes.get(key) if pch: base_hashes[key] = pch elif file_unchanged.get(key): @@ -1658,6 +1608,9 @@ def push( updated = 0 deleted = 0 errors: list[dict[str, str]] = [] + # Non-fatal push warnings: today only unstampable manifest baselines + # (issue #686), i.e. the API state could not be read back after a write. + warnings: list[dict[str, str]] = [] pushed_details: list[dict[str, str]] = [] manifest_dirty = False @@ -1701,15 +1654,16 @@ def push( if result: new_id = str(result.get("id", "")) config_dir = project_root / branch_path / config_path_str - hashes = self._compute_config_hashes(config_dir, component_id) - writeback = writeback_create_config_in_manifest( + writeback = stamp_created_config( + client, manifest=manifest, component_id=component_id, branch_id=branch_id, config_path_str=config_path_str, new_id=new_id, - file_hash=hashes.file_hash, - cfg_hash=hashes.cfg_hash, + hashes=self._compute_config_hashes(config_dir, component_id), + response=result, + warnings=warnings, ) # Record placeholder -> ULID so child rows and # transformation variable links can be remapped. @@ -1744,7 +1698,17 @@ def push( pushed_details.append(change) elif change_type == "modified": - push_update( + config_dir = project_root / branch_path / config_path_str + raise_on_legacy_boundary( + self, + client, + component_id=component_id, + config_id=config_id, + config_dir=config_dir, + manifest=manifest, + branch_id=branch_id, + ) + response = push_update( self, client, component_id, @@ -1756,34 +1720,18 @@ def push( allow_plaintext_fallback=allow_plaintext_fallback, ) # Update hashes so pull knows local == remote - config_dir = project_root / branch_path / config_path_str - config_file = config_dir / CONFIG_FILENAME - if config_file.exists(): - hashes = self._compute_config_hashes(config_dir, component_id) - entry_found = False - for cfg in manifest.configurations: - if cfg.component_id == component_id and cfg.id == config_id: - cfg.metadata["pull_hash"] = hashes.file_hash - cfg.metadata["pull_config_hash"] = hashes.cfg_hash - cfg.metadata["pull_extra_hashes"] = hashes.extra_hashes - entry_found = True - break - if not entry_found: - # Adopted-by-id config (issue #497): the update - # targeted an existing remote config that had no - # manifest entry (untracked local file whose - # _keboola.config_id resolved on the branch). - # Register it now so subsequent diffs read a - # stable entry and a local deletion is detected. - writeback_create_config_in_manifest( - manifest=manifest, - component_id=component_id, - branch_id=branch_id, - config_path_str=config_path_str, - new_id=config_id, - file_hash=hashes.file_hash, - cfg_hash=hashes.cfg_hash, - ) + if (config_dir / CONFIG_FILENAME).exists(): + stamp_updated_config( + client, + manifest=manifest, + component_id=component_id, + config_id=config_id, + branch_id=branch_id, + config_path_str=config_path_str, + hashes=self._compute_config_hashes(config_dir, component_id), + response=response, + warnings=warnings, + ) manifest_dirty = True updated += 1 pushed_details.append(change) @@ -1848,6 +1796,7 @@ def push( manifest=manifest, branch_id=branch_id, allow_plaintext_fallback=allow_plaintext_fallback, + warnings=warnings, ) manifest_dirty = True if change_type == "added": @@ -1884,6 +1833,7 @@ def push( branch_id=branch_id, ) errors.extend(binding.errors) + warnings.extend(binding.warnings) if binding.configs_rewritten: manifest_dirty = True @@ -1900,6 +1850,7 @@ def push( branch_id=branch_id, ) errors.extend(flow_binding.errors) + warnings.extend(flow_binding.warnings) if flow_binding.configs_rewritten: manifest_dirty = True @@ -1915,6 +1866,8 @@ def push( "errors": errors, "pushed_details": pushed_details, } + if warnings: + result_data["warnings"] = warnings if flow_binding.tasks_remapped: result_data["flow_task_remaps"] = flow_binding.tasks_remapped if name_drift_warnings and not no_name_drift_warnings: @@ -2001,14 +1954,15 @@ def _record_push_error( config_id, exc, ) - errors.append( - { - "change_type": change_type, - "component_id": component_id, - "config_id": config_id, - "message": str(exc), - } - ) + record: dict[str, str] = { + "change_type": change_type, + "component_id": component_id, + "config_id": config_id, + "message": str(exc), + } + if isinstance(exc, KeboolaApiError) and exc.error_code: + record["error_code"] = str(exc.error_code) + errors.append(record) # ------------------------------------------------------------------ # bulk operations (all projects) diff --git a/src/keboola_agent_cli/sync/code_extraction.py b/src/keboola_agent_cli/sync/code_extraction.py index 33742a1a..c0d630cd 100644 --- a/src/keboola_agent_cli/sync/code_extraction.py +++ b/src/keboola_agent_cli/sync/code_extraction.py @@ -6,11 +6,29 @@ from __future__ import annotations +import logging from pathlib import Path from typing import Any, cast from keboola_agent_cli.sync.sql_split import split_statements +logger = logging.getLogger(__name__) + +# Explicit statement boundary written into ``transform.sql`` when semicolons +# alone cannot recover the canonical ``script[]`` array -- i.e. when the API's +# elements carry no trailing ``;`` (issue #686 part 3). Without it, +# ``merge_code_files`` collapses several statements into one element and +# ``sync push`` silently rewrites production into the +# ``MULTI_STATEMENT_COUNT=1`` crash shape of issues #119/#120/#274. +# +# Emission is CONDITIONAL: a ``;``-terminated script round-trips on its own, so +# existing trees stay byte-identical. Recognition is an exact full-line match +# after stripping. Auto-appending the missing ``;`` was rejected as the +# alternative: it changes content, and Oracle (ODBC) rejects a trailing +# semicolon outright (ORA-00911). The marker never changes content and is +# backend-neutral. +SQL_STATEMENT_MARKER = "/* ===== STATEMENT ===== */" + def _strip_trailing_empty(lines: list[str]) -> list[str]: """Remove trailing empty lines but preserve leading whitespace.""" @@ -20,16 +38,59 @@ def _strip_trailing_empty(lines: list[str]) -> list[str]: return result +def canonical_sql_script(script: list[Any]) -> list[str]: + """Return the canonical ``script[]`` array for a SQL transformation code. + + ONE array element = ONE executable statement (the Keboola runtime's own + semantics -- the premise of issues #119/#120/#274). Each existing element + is split independently and the results are flattened; the array is NEVER + joined first, so two elements without trailing semicolons stay two + statements instead of silently merging into one (issue #686). + + This is the single producer of that shape: the API side + (``config_format._normalize_scripts``) and the file side + (:func:`_lines_to_script`) both agree with it, which is what makes the + stored ``pull_config_hash`` comparable across pull, push and diff. + """ + canonical: list[str] = [] + for element in script: + if isinstance(element, str): + canonical.extend(split_statements(element)) + elif element is not None: + canonical.append(element) + return canonical + + +def _split_on_statement_markers(lines: list[str]) -> list[list[str]]: + """Split collected code lines into segments on marker lines.""" + segments: list[list[str]] = [[]] + for line in lines: + if line.strip() == SQL_STATEMENT_MARKER: + segments.append([]) + continue + segments[-1].append(line) + return segments + + def _lines_to_script(lines: list[str], *, is_sql: bool = False) -> list[str]: """Convert collected lines back into the ``script[]`` array. For SQL transformations: splits on semicolons using a state machine, producing one element per statement (matching Keboola runtime semantics). + :data:`SQL_STATEMENT_MARKER` lines, when present, are GUARANTEED + boundaries -- but :func:`split_statements` still runs *within* each + segment, so a user who types ``; SELECT ...`` inside a marked segment + still gets it split correctly. For Python/other: joins all lines into a single element. """ stripped = _strip_trailing_empty(lines) if not stripped: return [] + if is_sql and any(line.strip() == SQL_STATEMENT_MARKER for line in stripped): + script: list[str] = [] + for segment in _split_on_statement_markers(stripped): + script.extend(split_statements("\n".join(segment))) + return script content = "\n".join(stripped) if is_sql: return split_statements(content) @@ -311,6 +372,65 @@ def merge_code_files( # ---- SQL Transformations ---- +def _render_sql_script_lines(scripts: list[Any], *, with_markers: bool) -> list[str]: + """Render one code's ``script[]`` as ``transform.sql`` lines.""" + lines: list[str] = [] + for si, script in enumerate(scripts): + if si > 0: + lines.append("") # blank line between statements + if with_markers: + lines.append(SQL_STATEMENT_MARKER) + lines.append("") + if isinstance(script, str) and "\n" in script: + lines.extend(script.split("\n")) + else: + lines.append(script) + return lines + + +def marker_less_roundtrip(script: list[Any]) -> list[str]: + """What a ``transform.sql`` written WITHOUT markers merges back into. + + The pre-#686 rendering, kept as a predicate: a working tree pulled before + boundary markers existed holds exactly this array, so comparing against it + identifies a config whose only difference from the remote is the lost + statement boundaries (see ``_sync_baseline.raise_on_legacy_boundary``). + """ + return _lines_to_script(_render_sql_script_lines(script, with_markers=False), is_sql=True) + + +def _render_sql_code(scripts: list[Any], code_name: str) -> list[str]: + """Render one code block, adding statement markers only when needed. + + Markers are emitted only when the plain rendering cannot be parsed back + into the canonical statement array (:func:`canonical_sql_script`) -- i.e. + when the elements carry no trailing semicolons. The ``;``-terminated case + (the overwhelming majority) renders exactly as before, so existing trees + are byte-stable and produce no spurious diff. + + Collision guard: if a statement's own text already contains a + marker-identical line, emitting boundaries would make the file ambiguous. + Markers are then suppressed for that code and a warning is logged -- the + round-trip degrades to the pre-#686 semicolon-only behaviour for it. + """ + plain = _render_sql_script_lines(scripts, with_markers=False) + canonical = canonical_sql_script(scripts) + if _lines_to_script(plain, is_sql=True) == canonical: + return plain + if any( + isinstance(s, str) and any(line.strip() == SQL_STATEMENT_MARKER for line in s.split("\n")) + for s in scripts + ): + logger.warning( + "Code %r contains a line identical to the statement-boundary marker; " + "writing transform.sql without boundary markers. Statements without a " + "trailing semicolon may be merged on the next push.", + code_name, + ) + return plain + return _render_sql_script_lines(scripts, with_markers=True) + + def _extract_sql_transformation(config_data: dict[str, Any], config_dir: Path) -> dict[str, Any]: """Extract SQL blocks from parameters.blocks into transform.sql.""" parameters = config_data.get("parameters") or {} @@ -330,15 +450,7 @@ def _extract_sql_transformation(config_data: dict[str, Any], config_dir: Path) - for code in block.get("codes", []): code_name = code.get("name", "unnamed") lines.append(SQL_CODE_MARKER.format(name=code_name)) - - scripts = code.get("script") or [] - for si, script in enumerate(scripts): - if si > 0: - lines.append("") # blank line between statements - if isinstance(script, str) and "\n" in script: - lines.extend(script.split("\n")) - else: - lines.append(script) + lines.extend(_render_sql_code(code.get("script") or [], code_name)) lines.append("") sql_content = "\n".join(lines).rstrip() + "\n" diff --git a/src/keboola_agent_cli/sync/config_format.py b/src/keboola_agent_cli/sync/config_format.py index e71c36fc..c44c95a3 100644 --- a/src/keboola_agent_cli/sync/config_format.py +++ b/src/keboola_agent_cli/sync/config_format.py @@ -13,46 +13,91 @@ import yaml from ..constants import CONFIG_YML_VERSION +from .code_extraction import canonical_sql_script, is_sql_transformation_component -def _normalize_scripts(parameters: Any) -> Any: +def _iter_codes(parameters: dict[str, Any]) -> Any: + """Yield every ``blocks[].codes[]`` dict in a transformation's parameters.""" + for block in parameters.get("blocks", []): + if not isinstance(block, dict): + continue + for code in block.get("codes", []): + if isinstance(code, dict): + yield code + + +def _join_script(scripts: list[Any]) -> list[str]: + """Collapse a code's ``script[]`` into a single joined string. + + The non-SQL shape: Python / R / custom-app components carry ONE code + body per code block, and the push side (``_lines_to_script`` without + ``is_sql``) produces exactly one element, so both sides agree. + """ + all_lines: list[str] = [] + for s in scripts: + if isinstance(s, str) and "\n" in s: + all_lines.extend(s.split("\n")) + else: + all_lines.append(s) + # Strip trailing whitespace per line (YAML roundtrip strips it) and + # remove trailing empty lines. + all_lines = [line.rstrip() for line in all_lines] + while all_lines and all_lines[-1] == "": + all_lines.pop() + return ["\n".join(all_lines)] if all_lines else [] + + +def _normalize_scripts(parameters: Any, component_id: str) -> Any: """Normalize script arrays in transformation parameters. The Keboola API inconsistently returns code scripts as either: - ``["line1", "line2", ...]`` (per-line array) - ``["full\\ncode\\nwith\\nnewlines"]`` (single multiline string) - This normalizes to single-string-per-code-block format so that local - merge (which produces single-string via ``_lines_to_script``) and - remote data compare identically. The Keboola transformation runner - treats each array element as a separate executable statement, so each - CODE block must be a single joined string. + The normalization is COMPONENT-AWARE (issue #686). Both sides of every + hash comparison must agree on what ``script[]`` looks like, and the push + side (``code_extraction._lines_to_script``) has split SQL on statement + boundaries since PR #120: + + - **SQL transformations** (:func:`is_sql_transformation_component`): + one element per executable statement, via + :func:`canonical_sql_script` -- each existing element split + independently and flattened, never joined first. + - **Everything else**: joined into a single string per code block, + matching ``_lines_to_script``'s non-SQL branch. + + Until #686 this function always collapsed to one element, which no SQL + transformation with two or more statements could ever match -- the + permanent ``~ REMOTE MODIFIED`` phantom drift after every ``sync push``. """ if not isinstance(parameters, dict): return parameters params = copy.deepcopy(parameters) - for block in params.get("blocks", []): - if not isinstance(block, dict): - continue - for code in block.get("codes", []): - if not isinstance(code, dict): - continue - scripts = code.get("script") - if isinstance(scripts, list) and scripts: - # Flatten everything into individual lines first. - all_lines: list[str] = [] - for s in scripts: - if isinstance(s, str) and "\n" in s: - all_lines.extend(s.split("\n")) - else: - all_lines.append(s) - # Strip trailing whitespace per line (YAML roundtrip - # strips it) and remove trailing empty lines. - all_lines = [line.rstrip() for line in all_lines] - while all_lines and all_lines[-1] == "": - all_lines.pop() - # Join into a single string per code block. - code["script"] = ["\n".join(all_lines)] if all_lines else [] + is_sql = is_sql_transformation_component(component_id) + for code in _iter_codes(params): + scripts = code.get("script") + if isinstance(scripts, list) and scripts: + code["script"] = canonical_sql_script(scripts) if is_sql else _join_script(scripts) + return params + + +def _normalize_scripts_legacy(parameters: Any) -> Any: + """Pre-#686 normalization: collapse every code's ``script[]`` into one. + + Kept for ONE purpose: recomputing the hash a pre-#686 kbagent would have + stored, so a manifest entry without ``config_hash_version`` can be + recognised as "in sync apart from the script shape" instead of being + re-classified as drift on the first run after the upgrade. It is never + used to produce data that is written anywhere -- only to compare against + an already-stored hash, and always from the RAW API config. + """ + if not isinstance(parameters, dict): + return parameters + params = copy.deepcopy(parameters) + for code in _iter_codes(params): + scripts = code.get("script") + if isinstance(scripts, list) and scripts: + code["script"] = _join_script(scripts) return params @@ -109,7 +154,11 @@ def classify_component_type(api_type: str) -> str: def api_config_to_local( - component_id: str, config_data: dict[str, Any], config_id: str + component_id: str, + config_data: dict[str, Any], + config_id: str, + *, + legacy_scripts: bool = False, ) -> dict[str, Any]: """Convert an API configuration response to the local ``_config.yml`` structure. @@ -128,6 +177,13 @@ def api_config_to_local( Any remaining keys inside ``configuration`` that are not explicitly promoted are preserved under a ``_configuration_extra`` key so that round-tripping does not lose data. + + Args: + legacy_scripts: Migration compatibility only (issue #686). When True, + ``parameters`` is normalized with the pre-#686 collapse + (:func:`_normalize_scripts_legacy`) so the caller can recompute + the hash an older kbagent would have stored for this same remote + config. Never pass it on a path that WRITES the result. """ configuration: dict[str, Any] = config_data.get("configuration") or {} @@ -141,7 +197,11 @@ def api_config_to_local( # Promote well-known nested keys if "parameters" in configuration: - local["parameters"] = _normalize_scripts(configuration["parameters"]) + local["parameters"] = ( + _normalize_scripts_legacy(configuration["parameters"]) + if legacy_scripts + else _normalize_scripts(configuration["parameters"], component_id) + ) storage: dict[str, Any] = configuration.get("storage") or {} if "input" in storage: diff --git a/tests/test_sync_baseline_stamping.py b/tests/test_sync_baseline_stamping.py new file mode 100644 index 00000000..92884946 --- /dev/null +++ b/tests/test_sync_baseline_stamping.py @@ -0,0 +1,705 @@ +"""Regression tests for issue #686 -- push stamps API-derived baselines. + +``pull_config_hash`` is the 3-way diff's base and is defined as the hash of +the config *as the API returns it*. ``sync push`` used to recompute it from +the files on disk instead, so any config whose local<->API round-trip is not +hash-stable was reported ``~ REMOTE MODIFIED`` by every later ``sync diff``, +forever, with the tree byte-identical to the remote (18 configs across a +21-project production repo in the field report). + +Covered here: + +* push -> diff is in sync for a multi-statement SQL transformation; +* push sends the same statement COUNT it pulled, semicolons or not; +* the ``is_disabled`` instance of the same class (remote disabled, local file + without the key); +* create / row create / row update / Phase C all stamp API-derived hashes; +* a partial (id-only) mutation response triggers a detail fetch, and a failed + fetch leaves the baseline unstamped with a warning -- never a disk hash; +* the ``config_hash_version`` migration: unversioned entries match leniently, + versioned ones strictly; +* the legacy push guard that refuses a boundary-only rewrite. +""" + +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock + +import yaml + +from helpers import setup_single_project +from keboola_agent_cli.constants import ( + CONFIG_FILENAME, + CONFIG_HASH_VERSION, + CONFIG_HASH_VERSION_KEY, +) +from keboola_agent_cli.models import TokenVerifyResponse +from keboola_agent_cli.services.sync_service import SyncService +from keboola_agent_cli.sync.manifest import load_manifest + +SQL_COMPONENT = "keboola.snowflake-transformation" + +SAMPLE_VERIFY_TOKEN = TokenVerifyResponse( + token_id="tok-001", + token_description="kbagent-cli", + project_id=258, + project_name="Production", + owner_name="My Org", +) + +SAMPLE_BRANCHES = [{"id": 12345, "name": "Main", "isDefault": True}] + + +# --------------------------------------------------------------------------- +# Remote fixtures +# --------------------------------------------------------------------------- + + +def _sql_config(script: list[str], config_id: str = "cfg-sql") -> dict[str, Any]: + """One SQL transformation config whose single code holds *script*.""" + return { + "id": config_id, + "name": "Raw data processing", + "description": "", + "configuration": { + "parameters": { + "blocks": [{"name": "Block 1", "codes": [{"name": "Code 1", "script": script}]}] + } + }, + "rows": [], + } + + +def _sql_components(script: list[str]) -> list[dict[str, Any]]: + return [ + { + "id": SQL_COMPONENT, + "type": "transformation", + "configurations": [_sql_config(script)], + } + ] + + +def _http_components( + *, is_disabled: bool = False, rows: list | None = None +) -> list[dict[str, Any]]: + config: dict[str, Any] = { + "id": "cfg-001", + "name": "My HTTP Extractor", + "description": "Fetches data", + "configuration": {"parameters": {"baseUrl": "https://api.example.com"}}, + "rows": rows or [], + } + if is_disabled: + config["isDisabled"] = True + return [{"id": "keboola.ex-http", "type": "extractor", "configurations": [config]}] + + +# --------------------------------------------------------------------------- +# A mock client that behaves like the real API: writes update the state that +# subsequent reads (list / detail) return. +# --------------------------------------------------------------------------- + + +class FakeApi: + """Minimal stateful Storage API double for config + row writes.""" + + def __init__(self, components: list[dict[str, Any]]): + self.components = components + self.update_calls: list[dict[str, Any]] = [] + self.row_update_calls: list[dict[str, Any]] = [] + self.detail_calls = 0 + self.detail_fails = False + self.partial_responses = False + + # -- lookup helpers ------------------------------------------------- + def _find(self, component_id: str, config_id: str) -> dict[str, Any]: + for component in self.components: + if component["id"] != component_id: + continue + for config in component["configurations"]: + if str(config["id"]) == str(config_id): + return config + raise KeyError(f"{component_id}/{config_id}") + + def _response(self, config: dict[str, Any]) -> dict[str, Any]: + return {"id": config["id"]} if self.partial_responses else dict(config) + + # -- API surface ---------------------------------------------------- + def list_components_with_configs(self, branch_id: int | None = None) -> list[dict[str, Any]]: + return self.components + + def list_dev_branches(self) -> list[dict[str, Any]]: + return SAMPLE_BRANCHES + + def verify_token(self) -> TokenVerifyResponse: + return SAMPLE_VERIFY_TOKEN + + def get_config_detail( + self, component_id: str, config_id: str, branch_id: int | None = None + ) -> dict[str, Any]: + self.detail_calls += 1 + if self.detail_fails: + raise RuntimeError("boom") + return dict(self._find(component_id, config_id)) + + def get_config_row( + self, + component_id: str, + config_id: str, + row_id: str, + branch_id: int | None = None, + ) -> dict[str, Any]: + parent = self._find(component_id, config_id) + for row in parent.get("rows", []): + if str(row["id"]) == str(row_id): + return dict(row) + raise KeyError(row_id) + + def update_config( + self, + component_id: str, + config_id: str, + name: str | None = None, + configuration: dict[str, Any] | None = None, + description: str | None = None, + change_description: str = "", + branch_id: int | None = None, + is_disabled: bool | None = None, + ) -> dict[str, Any]: + config = self._find(component_id, config_id) + self.update_calls.append({"component_id": component_id, "configuration": configuration}) + if configuration is not None: + config["configuration"] = configuration + if name is not None: + config["name"] = name + if description is not None: + config["description"] = description + if is_disabled is not None: + config["isDisabled"] = is_disabled + return self._response(config) + + def create_config( + self, + component_id: str, + name: str, + configuration: dict[str, Any], + description: str = "", + branch_id: int | None = None, + is_disabled: bool = False, + ) -> dict[str, Any]: + config: dict[str, Any] = { + "id": "cfg-new", + "name": name, + "description": description, + "configuration": configuration, + "rows": [], + } + if is_disabled: + config["isDisabled"] = True + for component in self.components: + if component["id"] == component_id: + component["configurations"].append(config) + break + else: + self.components.append( + {"id": component_id, "type": "transformation", "configurations": [config]} + ) + return self._response(config) + + def create_config_row( + self, + component_id: str, + config_id: str, + name: str, + configuration: dict[str, Any], + description: str = "", + is_disabled: bool = False, + branch_id: int | None = None, + ) -> dict[str, Any]: + parent = self._find(component_id, config_id) + row: dict[str, Any] = { + "id": "row-new", + "name": name, + "description": description, + "configuration": configuration, + } + if is_disabled: + row["isDisabled"] = True + parent.setdefault("rows", []).append(row) + return {"id": row["id"]} if self.partial_responses else dict(row) + + def update_config_row( + self, + component_id: str, + config_id: str, + row_id: str, + name: str | None = None, + configuration: dict[str, Any] | None = None, + description: str | None = None, + is_disabled: bool | None = None, + change_description: str = "", + branch_id: int | None = None, + ) -> dict[str, Any]: + parent = self._find(component_id, config_id) + for row in parent.get("rows", []): + if str(row["id"]) != str(row_id): + continue + self.row_update_calls.append({"row_id": row_id, "configuration": configuration}) + if configuration is not None: + row["configuration"] = configuration + if name is not None: + row["name"] = name + if is_disabled is not None: + row["isDisabled"] = is_disabled + return {"id": row["id"]} if self.partial_responses else dict(row) + raise KeyError(row_id) + + +def _client_for(api: FakeApi) -> MagicMock: + """Wrap a :class:`FakeApi` in a context-manager mock client.""" + client = MagicMock(wraps=api) + client.__enter__ = MagicMock(return_value=client) + client.__exit__ = MagicMock(return_value=False) + client.encrypt_values = MagicMock(side_effect=lambda *a, **k: {}) + return client + + +def _service(store: Any, api: FakeApi) -> SyncService: + return SyncService(config_store=store, client_factory=lambda url, token: _client_for(api)) + + +def _init_and_pull(tmp_config_dir: Path, project_root: Path, api: FakeApi) -> Any: + project_root.mkdir(exist_ok=True) + store = setup_single_project(tmp_config_dir) + _service(store, api).init_sync(alias="prod", project_root=project_root) + _service(store, api).pull( + alias="prod", project_root=project_root, no_storage=True, no_jobs=True + ) + return store + + +def _sql_file(project_root: Path) -> Path: + matches = list(project_root.rglob("transform.sql")) + assert len(matches) == 1 + return matches[0] + + +def _config_file(project_root: Path, under_rows: bool = False) -> Path: + matches = [f for f in project_root.rglob(CONFIG_FILENAME) if ("rows" in f.parts) == under_rows] + assert len(matches) == 1 + return matches[0] + + +def _entry(project_root: Path, config_id: str = "cfg-sql") -> Any: + manifest = load_manifest(project_root) + return next(c for c in manifest.configurations if c.id == config_id) + + +def _remote_script(api: FakeApi) -> list[str]: + config = api._find(SQL_COMPONENT, "cfg-sql") + return config["configuration"]["parameters"]["blocks"][0]["codes"][0]["script"] + + +# =================================================================== +# The reporter's regression test: push -> diff must be in sync +# =================================================================== + + +def test_push_then_diff_in_sync_for_multi_statement_sql( + tmp_config_dir: Path, tmp_path: Path +) -> None: + """The #686 headline: no phantom drift after pushing a multi-statement SQL.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;", "SELECT 3;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + + sql_file = _sql_file(project_root) + sql_file.write_text( + sql_file.read_text(encoding="utf-8").replace("SELECT 3;", "SELECT 4;"), encoding="utf-8" + ) + + push_result = _service(store, api).push(alias="prod", project_root=project_root) + assert push_result["status"] == "pushed" + assert push_result["errors"] == [] + + diff_result = _service(store, api).diff(alias="prod", project_root=project_root) + assert diff_result["summary"]["remote_modified"] == 0 + assert diff_result["summary"]["modified"] == 0 + assert diff_result["summary"]["conflict"] == 0 + + +def test_push_preserves_statement_count_with_semicolons( + tmp_config_dir: Path, tmp_path: Path +) -> None: + """Mirror test: push sends the same number of statements it pulled.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + + sql_file = _sql_file(project_root) + sql_file.write_text( + sql_file.read_text(encoding="utf-8").replace("SELECT 2;", "SELECT 22;"), encoding="utf-8" + ) + _service(store, api).push(alias="prod", project_root=project_root) + + assert _remote_script(api) == ["SELECT 1;", "SELECT 22;"] + + +def test_push_preserves_statement_count_without_semicolons( + tmp_config_dir: Path, tmp_path: Path +) -> None: + """The silent-rewrite half: no trailing ``;`` must not collapse to one.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1", "SELECT 2"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + + sql_file = _sql_file(project_root) + sql_file.write_text( + sql_file.read_text(encoding="utf-8").replace("SELECT 2", "SELECT 22"), encoding="utf-8" + ) + _service(store, api).push(alias="prod", project_root=project_root) + + assert _remote_script(api) == ["SELECT 1", "SELECT 22"] + + diff_result = _service(store, api).diff(alias="prod", project_root=project_root) + assert diff_result["summary"]["remote_modified"] == 0 + + +def test_push_of_disabled_config_without_local_key_leaves_no_drift( + tmp_config_dir: Path, tmp_path: Path +) -> None: + """is_disabled instance of the same class (issue #467 semantics).""" + project_root = tmp_path / "project" + api = FakeApi(_http_components(is_disabled=True)) + store = _init_and_pull(tmp_config_dir, project_root, api) + + # Drop the sparse is_disabled key, as a hand-written/legacy tree has it, + # and change something so the config is pushable. + config_file = _config_file(project_root) + data = yaml.safe_load(config_file.read_text(encoding="utf-8")) + data.pop("is_disabled", None) + data["parameters"]["baseUrl"] = "https://changed.example.com" + config_file.write_text(yaml.dump(data, default_flow_style=False), encoding="utf-8") + + _service(store, api).push(alias="prod", project_root=project_root) + + diff_result = _service(store, api).diff(alias="prod", project_root=project_root) + assert diff_result["summary"]["remote_modified"] == 0 + assert diff_result["summary"]["conflict"] == 0 + + +# =================================================================== +# Stamping mechanics (create / rows / partial responses) +# =================================================================== + + +def test_update_stamps_api_derived_hash_and_version(tmp_config_dir: Path, tmp_path: Path) -> None: + """A pushed config records the API's hash plus the shape version.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + + before = _entry(project_root).metadata["pull_config_hash"] + sql_file = _sql_file(project_root) + sql_file.write_text( + sql_file.read_text(encoding="utf-8").replace("SELECT 2;", "SELECT 9;"), encoding="utf-8" + ) + _service(store, api).push(alias="prod", project_root=project_root) + + entry = _entry(project_root) + assert entry.metadata["pull_config_hash"] != before + assert entry.metadata[CONFIG_HASH_VERSION_KEY] == CONFIG_HASH_VERSION + + +def test_create_stamps_api_derived_hash(tmp_config_dir: Path, tmp_path: Path) -> None: + """A freshly created config gets an API-derived baseline, not a disk one.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + + new_dir = project_root / "main" / "transformation" / SQL_COMPONENT / "new-transformation" + new_dir.mkdir(parents=True) + (new_dir / CONFIG_FILENAME).write_text( + yaml.dump({"version": 2, "name": "New", "description": "", "parameters": {}}), + encoding="utf-8", + ) + (new_dir / "transform.sql").write_text("SELECT 100;\n\nSELECT 200;\n", encoding="utf-8") + + result = _service(store, api).push(alias="prod", project_root=project_root) + assert result["created"] == 1 + + entry = _entry(project_root, "cfg-new") + assert entry.metadata["pull_config_hash"] + assert entry.metadata[CONFIG_HASH_VERSION_KEY] == CONFIG_HASH_VERSION + assert ( + _service(store, api).diff(alias="prod", project_root=project_root)["summary"][ + "remote_modified" + ] + == 0 + ) + + +def test_row_push_stamps_api_derived_hash(tmp_config_dir: Path, tmp_path: Path) -> None: + """A disabled remote row whose local file lacks the key leaves no drift.""" + project_root = tmp_path / "project" + row = { + "id": "row-001", + "name": "Users", + "description": "", + "configuration": {"parameters": {"path": "/users"}}, + "isDisabled": True, + } + api = FakeApi(_http_components(rows=[row])) + store = _init_and_pull(tmp_config_dir, project_root, api) + + row_file = _config_file(project_root, under_rows=True) + data = yaml.safe_load(row_file.read_text(encoding="utf-8")) + data.pop("is_disabled", None) + data["parameters"]["path"] = "/people" + row_file.write_text(yaml.dump(data, default_flow_style=False), encoding="utf-8") + + _service(store, api).push(alias="prod", project_root=project_root) + + diff_result = _service(store, api).diff(alias="prod", project_root=project_root) + assert diff_result["summary"]["remote_modified"] == 0 + assert diff_result["summary"]["conflict"] == 0 + + +def test_partial_mutation_response_triggers_detail_fetch( + tmp_config_dir: Path, tmp_path: Path +) -> None: + """An id-only PUT response is not trusted -- the config is re-read.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + + api.partial_responses = True + api.detail_calls = 0 + sql_file = _sql_file(project_root) + sql_file.write_text( + sql_file.read_text(encoding="utf-8").replace("SELECT 2;", "SELECT 9;"), encoding="utf-8" + ) + result = _service(store, api).push(alias="prod", project_root=project_root) + + assert api.detail_calls >= 1 + assert result["errors"] == [] + assert not result.get("warnings") + assert _entry(project_root).metadata[CONFIG_HASH_VERSION_KEY] == CONFIG_HASH_VERSION + + +def test_detail_fetch_failure_leaves_baseline_unstamped( + tmp_config_dir: Path, tmp_path: Path +) -> None: + """No API state, no stamp: the old baseline survives and a warning is raised.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + + before = _entry(project_root).metadata["pull_config_hash"] + api.partial_responses = True + api.detail_fails = True + sql_file = _sql_file(project_root) + sql_file.write_text( + sql_file.read_text(encoding="utf-8").replace("SELECT 2;", "SELECT 9;"), encoding="utf-8" + ) + result = _service(store, api).push(alias="prod", project_root=project_root) + + assert result["errors"] == [] + assert len(result["warnings"]) == 1 + assert "sync pull" in result["warnings"][0]["message"] + # The baseline is left exactly as the pull wrote it -- never recomputed + # from disk, which is the asymmetry #686 is about. + assert _entry(project_root).metadata["pull_config_hash"] == before + + +# =================================================================== +# Migration: manifests written before the shape change +# =================================================================== + + +SQL_HEADER = "/* ===== BLOCK: Block 1 ===== */\n\n/* ===== CODE: Code 1 ===== */\n" + + +def _downgrade_to_legacy( + project_root: Path, api: FakeApi, marker_less_body: str | None = None +) -> None: + """Rewrite the tree the way a pre-#686 kbagent would have left it. + + The manifest entry loses its ``config_hash_version`` and carries the old + collapsed-shape hash. When *marker_less_body* is given, ``transform.sql`` is + rewritten without boundary markers (the pre-#686 rendering) and its recorded + companion hash is refreshed, so the file still counts as untouched. + """ + import hashlib + + from keboola_agent_cli.sync.config_format import api_config_to_local + from keboola_agent_cli.sync.diff_engine import config_hash + from keboola_agent_cli.sync.manifest import save_manifest + + sql_hash = "" + if marker_less_body is not None: + sql_file = _sql_file(project_root) + sql_file.write_text(SQL_HEADER + marker_less_body, encoding="utf-8") + sql_hash = hashlib.sha256(sql_file.read_bytes()).hexdigest() + + manifest = load_manifest(project_root) + raw = api._find(SQL_COMPONENT, "cfg-sql") + legacy = config_hash(api_config_to_local(SQL_COMPONENT, raw, "cfg-sql", legacy_scripts=True)) + for cfg in manifest.configurations: + if cfg.id == "cfg-sql": + cfg.metadata["pull_config_hash"] = legacy + cfg.metadata.pop(CONFIG_HASH_VERSION_KEY, None) + if sql_hash: + cfg.metadata.setdefault("pull_extra_hashes", {})["transform.sql"] = sql_hash + save_manifest(project_root, manifest) + + +def test_unversioned_legacy_hash_diffs_as_in_sync(tmp_config_dir: Path, tmp_path: Path) -> None: + """A pre-#686 baseline is accepted leniently instead of showing drift.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + _downgrade_to_legacy(project_root, api) + + diff_result = _service(store, api).diff(alias="prod", project_root=project_root) + assert diff_result["summary"]["remote_modified"] == 0 + assert diff_result["summary"]["conflict"] == 0 + + +def test_versioned_entry_is_compared_strictly(tmp_config_dir: Path, tmp_path: Path) -> None: + """With the version key present, a legacy-shaped hash is real drift.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + _downgrade_to_legacy(project_root, api) + + from keboola_agent_cli.sync.manifest import save_manifest + + manifest = load_manifest(project_root) + for cfg in manifest.configurations: + cfg.metadata[CONFIG_HASH_VERSION_KEY] = CONFIG_HASH_VERSION + save_manifest(project_root, manifest) + + diff_result = _service(store, api).diff(alias="prod", project_root=project_root) + assert diff_result["summary"]["remote_modified"] == 1 + + +def test_real_remote_drift_is_still_reported_on_unversioned_entry( + tmp_config_dir: Path, tmp_path: Path +) -> None: + """Leniency covers the script shape only -- genuine remote edits still show.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + _downgrade_to_legacy(project_root, api) + + api._find(SQL_COMPONENT, "cfg-sql")["name"] = "Renamed in the UI" + + diff_result = _service(store, api).diff(alias="prod", project_root=project_root) + assert diff_result["summary"]["remote_modified"] == 1 + + +def test_pull_migrates_unversioned_entry(tmp_config_dir: Path, tmp_path: Path) -> None: + """One ``sync pull`` re-stamps the entry with the new shape + version.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1", "SELECT 2"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + # A tree pulled before boundary markers existed. + _downgrade_to_legacy(project_root, api, "SELECT 1\n\nSELECT 2\n") + + _service(store, api).pull( + alias="prod", project_root=project_root, no_storage=True, no_jobs=True + ) + + entry = _entry(project_root) + assert entry.metadata[CONFIG_HASH_VERSION_KEY] == CONFIG_HASH_VERSION + # Extraction re-ran, so the boundary markers are now on disk. + assert "STATEMENT" in _sql_file(project_root).read_text(encoding="utf-8") + + +def test_pull_migration_preserves_locally_edited_code_file( + tmp_config_dir: Path, tmp_path: Path +) -> None: + """A migration pull must not clobber an edited transform.sql (R4).""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1", "SELECT 2"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + _downgrade_to_legacy(project_root, api, "SELECT 1\n\nSELECT 2\n") + + sql_file = _sql_file(project_root) + edited = SQL_HEADER + "SELECT 1\n\nSELECT 999\n" + sql_file.write_text(edited, encoding="utf-8") + + _service(store, api).pull( + alias="prod", project_root=project_root, no_storage=True, no_jobs=True + ) + + assert sql_file.read_text(encoding="utf-8") == edited + assert CONFIG_HASH_VERSION_KEY not in _entry(project_root).metadata + + +def test_legacy_boundary_push_is_refused(tmp_config_dir: Path, tmp_path: Path) -> None: + """A pre-markers tree that would collapse statements is aborted, not pushed.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1", "SELECT 2"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + # Pre-markers rendering: the boundary between the two statements is lost. + _downgrade_to_legacy(project_root, api, "SELECT 1\n\nSELECT 2\n") + # Touch _config.yml so the config is classified as locally modified. + config_file = _config_file(project_root) + data = yaml.safe_load(config_file.read_text(encoding="utf-8")) + data["description"] = "edited" + config_file.write_text(yaml.dump(data, default_flow_style=False), encoding="utf-8") + + result = _service(store, api).push(alias="prod", project_root=project_root) + + assert result["updated"] == 0 + assert len(result["errors"]) == 1 + assert result["errors"][0]["error_code"] == "SYNC_LEGACY_BOUNDARY" + assert "sync pull" in result["errors"][0]["message"] + # The remote statement array is untouched. + assert _remote_script(api) == ["SELECT 1", "SELECT 2"] + + +def test_genuine_edit_on_legacy_tree_still_pushes(tmp_config_dir: Path, tmp_path: Path) -> None: + """The guard is boundary-specific: a real SQL edit is not blocked.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + _downgrade_to_legacy(project_root, api) + + sql_file = _sql_file(project_root) + sql_file.write_text( + sql_file.read_text(encoding="utf-8").replace("SELECT 2;", "SELECT 42;"), encoding="utf-8" + ) + result = _service(store, api).push(alias="prod", project_root=project_root) + + assert result["errors"] == [] + assert result["updated"] == 1 + assert _remote_script(api) == ["SELECT 1;", "SELECT 42;"] + + +def test_force_pull_does_not_abort_on_legacy_shape(tmp_config_dir: Path, tmp_path: Path) -> None: + """A shape-only baseline is not a remote change, so --force must not abort.""" + project_root = tmp_path / "project" + api = FakeApi(_sql_components(["SELECT 1;", "SELECT 2;"])) + store = _init_and_pull(tmp_config_dir, project_root, api) + _downgrade_to_legacy(project_root, api) + + # Local edit + a legacy baseline: strict comparison would read the remote + # as "changed" and raise SyncConflictError. + config_file = _config_file(project_root) + data = yaml.safe_load(config_file.read_text(encoding="utf-8")) + data["description"] = "edited locally" + config_file.write_text(yaml.dump(data, default_flow_style=False), encoding="utf-8") + + _service(store, api).pull( + alias="prod", project_root=project_root, force=True, no_storage=True, no_jobs=True + ) + + # The un-pushed edit survives (force preserves a locally-modified config + # whose remote did not move). + assert ( + yaml.safe_load(config_file.read_text(encoding="utf-8"))["description"] == "edited locally" + ) diff --git a/tests/test_sync_code_extraction.py b/tests/test_sync_code_extraction.py index e127aa87..128f82b0 100644 --- a/tests/test_sync_code_extraction.py +++ b/tests/test_sync_code_extraction.py @@ -7,6 +7,8 @@ import pytest from keboola_agent_cli.sync.code_extraction import ( + SQL_STATEMENT_MARKER, + canonical_sql_script, extract_code_files, merge_code_files, ) @@ -693,3 +695,203 @@ def test_all_sql_components_recognized(self, component_id: str, tmp_path: Path) extract_code_files(component_id, config_data, config_dir) assert (config_dir / "transform.sql").exists() + + +# =================================================================== +# Statement-boundary markers (issue #686, part 3) +# =================================================================== + + +SQL_COMPONENT = "keboola.snowflake-transformation" + + +def _sql_config(script: list[str]) -> dict[str, Any]: + """Build a minimal SQL-transformation config carrying one code block.""" + return { + "parameters": { + "blocks": [{"name": "Block 1", "codes": [{"name": "Code 1", "script": list(script)}]}] + } + } + + +def _script_of(config_data: dict[str, Any]) -> list[str]: + """Return the first code's script array.""" + return config_data["parameters"]["blocks"][0]["codes"][0]["script"] + + +class TestStatementMarkers: + """``transform.sql`` carries explicit statement boundaries when needed.""" + + def test_marker_written_when_semicolons_cannot_recover_boundaries(self, tmp_path: Path) -> None: + """No trailing semicolons -> an explicit marker separates the statements.""" + config_data = _sql_config(["SELECT 1", "SELECT 2"]) + config_dir = tmp_path / "no-semicolons" + + extract_code_files(SQL_COMPONENT, config_data, config_dir) + + content = (config_dir / "transform.sql").read_text(encoding="utf-8") + assert SQL_STATEMENT_MARKER in content + + def test_marker_round_trip_preserves_statement_count(self, tmp_path: Path) -> None: + """The marked file merges back to the exact original array.""" + original = ["SELECT 1", "SELECT 2", "SELECT 3"] + config_data = _sql_config(original) + config_dir = tmp_path / "no-semicolons-rt" + + extract_code_files(SQL_COMPONENT, config_data, config_dir) + merge_code_files(SQL_COMPONENT, config_data, config_dir) + + assert _script_of(config_data) == original + + def test_no_marker_for_semicolon_terminated_scripts(self, tmp_path: Path) -> None: + """The common ``;``-terminated case stays byte-identical to before.""" + config_data = _sql_config(["SELECT 1;", "SELECT 2;"]) + config_dir = tmp_path / "semicolons" + + extract_code_files(SQL_COMPONENT, config_data, config_dir) + + content = (config_dir / "transform.sql").read_text(encoding="utf-8") + assert SQL_STATEMENT_MARKER not in content + assert "STATEMENT" not in content + + def test_marker_segment_is_still_split_on_semicolons(self, tmp_path: Path) -> None: + """A user adding ``; SELECT ...`` inside a marked segment gets split (R3).""" + config_data = _sql_config(["SELECT 1", "SELECT 2"]) + config_dir = tmp_path / "resplit-segment" + extract_code_files(SQL_COMPONENT, config_data, config_dir) + + sql_file = config_dir / "transform.sql" + content = sql_file.read_text(encoding="utf-8") + sql_file.write_text(content.replace("SELECT 1", "SELECT 1; SELECT 9;"), encoding="utf-8") + + merged: dict[str, Any] = {"parameters": {}} + merge_code_files(SQL_COMPONENT, merged, config_dir) + + assert _script_of(merged) == ["SELECT 1;", "SELECT 9;", "SELECT 2"] + + def test_marker_collision_falls_back_to_no_markers(self, tmp_path: Path) -> None: + """A statement whose own text holds a marker line disables marker emission.""" + config_data = _sql_config([f"SELECT 1\n{SQL_STATEMENT_MARKER}", "SELECT 2"]) + config_dir = tmp_path / "collision" + + extract_code_files(SQL_COMPONENT, config_data, config_dir) + + content = (config_dir / "transform.sql").read_text(encoding="utf-8") + # The marker text appears only as part of the statement itself -- exactly + # once -- never as an emitted boundary. + assert content.count(SQL_STATEMENT_MARKER) == 1 + + def test_canonical_sql_script_splits_each_element(self) -> None: + """Each element is split independently and the results flattened.""" + assert canonical_sql_script(["SELECT 1; SELECT 2;", "SELECT 3"]) == [ + "SELECT 1;", + "SELECT 2;", + "SELECT 3", + ] + + def test_canonical_sql_script_drops_blank_elements(self) -> None: + """Whitespace-only elements vanish, matching the file round-trip.""" + assert canonical_sql_script(["SELECT 1;", " ", ""]) == ["SELECT 1;"] + + +class TestPullPushDiffShapeParity: + """The issue #686 repro table, at the hash level. + + Drives the real pull -> push -> diff producers over one code block and + asserts the two sides agree (no phantom drift) and that the statement + array survives the file round-trip. + """ + + @staticmethod + def _api(script: list[str]) -> dict[str, Any]: + return { + "id": "1", + "name": "c", + "description": "", + "configuration": { + "parameters": { + "blocks": [{"name": "B", "codes": [{"name": "C", "script": list(script)}]}] + } + }, + } + + @pytest.mark.parametrize( + ("api_script", "expected_sent"), + [ + # ;-terminated, several elements: unchanged, was PHANTOM before. + (["SELECT 1;", "SELECT 2;"], ["SELECT 1;", "SELECT 2;"]), + # No semicolons: boundaries survive via the marker. Before the fix + # push silently sent ONE element (MULTI_STATEMENT_COUNT=1). + (["SELECT 1", "SELECT 2"], ["SELECT 1", "SELECT 2"]), + # One element packing two statements is NOT a canonical array: the + # runtime wants one statement per element, so it is split. That is + # the #274 normalization, deliberate -- and now no longer phantom. + (["SELECT 1;\nSELECT 2;"], ["SELECT 1;", "SELECT 2;"]), + (["SELECT 1"], ["SELECT 1"]), + ], + ) + def test_push_matches_remote_and_diff_is_in_sync( + self, api_script: list[str], expected_sent: list[str], tmp_path: Path + ) -> None: + from keboola_agent_cli.sync.config_format import api_config_to_local + from keboola_agent_cli.sync.diff_engine import config_hash + + component = "keboola.snowflake-transformation" + config_dir = tmp_path / "cfg" + + local = api_config_to_local(component, self._api(api_script), "1") # pull + extract_code_files(component, local, config_dir) + pushed = copy.deepcopy(local) + merge_code_files(component, pushed, config_dir) # push + sent = pushed["parameters"]["blocks"][0]["codes"][0]["script"] + + assert sent == expected_sent + # diff: remote side of what push just wrote vs the pushed baseline + remote_hash = config_hash(api_config_to_local(component, self._api(sent), "1")) + assert remote_hash == config_hash(pushed) + + +class TestBroadPredicateSqlComponent: + """SQL backends matched only by fragment keep both sides consistent (R1). + + ``keboola.exasol-transformation`` is SQL for normalization purposes but is + not in the exact extraction set, so its blocks stay inside ``_config.yml`` + and the merge is an identity. The split shape must therefore survive + untouched -- the local data IS the normalized data. + """ + + COMPONENT = "keboola.exasol-transformation" + + def test_yaml_identity_round_trip_preserves_split_shape(self, tmp_path: Path) -> None: + from keboola_agent_cli.sync.config_format import api_config_to_local + from keboola_agent_cli.sync.diff_engine import config_hash + + api_config = { + "id": "1", + "name": "c", + "description": "", + "configuration": { + "parameters": { + "blocks": [ + { + "name": "B", + "codes": [{"name": "C", "script": ["SELECT 1;", "SELECT 2;"]}], + } + ] + } + }, + } + config_dir = tmp_path / "exasol" + + local = api_config_to_local(self.COMPONENT, api_config, "1") + extract_code_files(self.COMPONENT, local, config_dir) + # No code file is written -- the blocks stay in the YAML body. + assert not (config_dir / "transform.sql").exists() + assert local["parameters"]["blocks"][0]["codes"][0]["script"] == [ + "SELECT 1;", + "SELECT 2;", + ] + + pushed = copy.deepcopy(local) + merge_code_files(self.COMPONENT, pushed, config_dir) + assert config_hash(pushed) == config_hash(local) diff --git a/tests/test_sync_config_format.py b/tests/test_sync_config_format.py index 58d7a1a1..c90191e0 100644 --- a/tests/test_sync_config_format.py +++ b/tests/test_sync_config_format.py @@ -6,6 +6,7 @@ from keboola_agent_cli.sync.config_format import ( _normalize_scripts, + _normalize_scripts_legacy, api_config_to_local, api_row_to_local, classify_component_type, @@ -39,6 +40,11 @@ SAMPLE_COMPONENT_ID = "keboola.ex-http" SAMPLE_CONFIG_ID = "cfg-123" +# Script normalization is component-aware (issue #686): SQL transformations +# split one element per statement, everything else joins into one string. +SQL_COMPONENT = "keboola.snowflake-transformation" +NON_SQL_COMPONENT = "keboola.python-transformation-v2" + class TestClassifyComponentType: """Tests for classify_component_type().""" @@ -171,16 +177,22 @@ def test_local_config_to_api_extras_merged_back(self) -> None: class TestNormalizeScripts: - """Tests for _normalize_scripts() -- script array normalization.""" + """Tests for _normalize_scripts() -- script array normalization. + + The normalization is component-aware since issue #686: SQL + transformations split into one element per statement (the runtime's + own semantics, matching ``_lines_to_script`` on the push side), every + other component keeps the historical join-into-one-string behaviour. + """ def test_per_line_array_joined_to_single_string(self) -> None: - """Per-line script array is joined into a single string.""" + """Per-line script array is joined into a single string (non-SQL).""" params = { "blocks": [ {"codes": [{"script": ["CREATE TABLE foo AS", " SELECT col1", " FROM bar;"]}]} ] } - result = _normalize_scripts(params) + result = _normalize_scripts(params, NON_SQL_COMPONENT) script = result["blocks"][0]["codes"][0]["script"] assert len(script) == 1 assert script[0] == "CREATE TABLE foo AS\n SELECT col1\n FROM bar;" @@ -192,7 +204,7 @@ def test_single_multiline_string_preserved(self) -> None: {"codes": [{"script": ["CREATE TABLE foo AS\n SELECT col1\n FROM bar;"]}]} ] } - result = _normalize_scripts(params) + result = _normalize_scripts(params, NON_SQL_COMPONENT) script = result["blocks"][0]["codes"][0]["script"] assert len(script) == 1 assert script[0] == "CREATE TABLE foo AS\n SELECT col1\n FROM bar;" @@ -200,7 +212,7 @@ def test_single_multiline_string_preserved(self) -> None: def test_trailing_whitespace_stripped(self) -> None: """Trailing whitespace per line is stripped during normalization.""" params = {"blocks": [{"codes": [{"script": ["SELECT 1 ", "FROM bar "]}]}]} - result = _normalize_scripts(params) + result = _normalize_scripts(params, NON_SQL_COMPONENT) script = result["blocks"][0]["codes"][0]["script"] assert len(script) == 1 assert script[0] == "SELECT 1\nFROM bar" @@ -208,19 +220,19 @@ def test_trailing_whitespace_stripped(self) -> None: def test_empty_script_preserved(self) -> None: """Empty script array stays empty.""" params = {"blocks": [{"codes": [{"script": []}]}]} - result = _normalize_scripts(params) + result = _normalize_scripts(params, NON_SQL_COMPONENT) assert result["blocks"][0]["codes"][0]["script"] == [] def test_no_blocks_passthrough(self) -> None: """Parameters without blocks are returned unchanged.""" params = {"key": "value"} - result = _normalize_scripts(params) + result = _normalize_scripts(params, NON_SQL_COMPONENT) assert result == {"key": "value"} def test_non_dict_passthrough(self) -> None: """Non-dict input is returned as-is.""" - assert _normalize_scripts("not a dict") == "not a dict" - assert _normalize_scripts(42) == 42 + assert _normalize_scripts("not a dict", NON_SQL_COMPONENT) == "not a dict" + assert _normalize_scripts(42, NON_SQL_COMPONENT) == 42 def test_does_not_mutate_input(self) -> None: """Original parameters are not mutated.""" @@ -228,9 +240,71 @@ def test_does_not_mutate_input(self) -> None: import copy original = copy.deepcopy(params) - _normalize_scripts(params) + _normalize_scripts(params, NON_SQL_COMPONENT) assert params == original + # -- SQL transformations: one element per statement (issue #686) -------- + + def test_sql_splits_each_element_into_statements(self) -> None: + """Each element is split on statement boundaries and flattened.""" + params = {"blocks": [{"codes": [{"script": ["SELECT 1;\nSELECT 2;", "SELECT 3;"]}]}]} + result = _normalize_scripts(params, SQL_COMPONENT) + assert result["blocks"][0]["codes"][0]["script"] == [ + "SELECT 1;", + "SELECT 2;", + "SELECT 3;", + ] + + def test_sql_never_joins_elements_before_splitting(self) -> None: + """Elements without trailing semicolons stay separate statements (R1).""" + params = {"blocks": [{"codes": [{"script": ["SELECT 1", "SELECT 2"]}]}]} + result = _normalize_scripts(params, SQL_COMPONENT) + assert result["blocks"][0]["codes"][0]["script"] == ["SELECT 1", "SELECT 2"] + + def test_sql_multiline_single_statement_stays_one_element(self) -> None: + """A multi-line single statement is not exploded per line.""" + params = { + "blocks": [ + {"codes": [{"script": ["CREATE TABLE foo AS", " SELECT col1", " FROM bar;"]}]} + ] + } + result = _normalize_scripts(params, SQL_COMPONENT) + assert result["blocks"][0]["codes"][0]["script"] == [ + "CREATE TABLE foo AS", + "SELECT col1", + "FROM bar;", + ] + + def test_legacy_normalization_still_collapses(self) -> None: + """The legacy producer (migration compat only) keeps collapsing to one.""" + params = {"blocks": [{"codes": [{"script": ["SELECT 1;", "SELECT 2;"]}]}]} + result = _normalize_scripts_legacy(params) + assert result["blocks"][0]["codes"][0]["script"] == ["SELECT 1;\nSELECT 2;"] + + def test_api_config_to_local_splits_for_sql_component(self) -> None: + """The pull-side converter emits the split shape for SQL components.""" + api_config = { + "id": "1", + "name": "t", + "description": "", + "configuration": { + "parameters": { + "blocks": [{"name": "B", "codes": [{"name": "C", "script": ["S1;", "S2;"]}]}] + } + }, + } + local = api_config_to_local(SQL_COMPONENT, api_config, "1") + assert local["parameters"]["blocks"][0]["codes"][0]["script"] == ["S1;", "S2;"] + + legacy = api_config_to_local(SQL_COMPONENT, api_config, "1", legacy_scripts=True) + assert legacy["parameters"]["blocks"][0]["codes"][0]["script"] == ["S1;\nS2;"] + + def test_broad_predicate_sql_variant_also_splits(self) -> None: + """A SQL backend matched only by fragment (exasol) splits too (R1).""" + params = {"blocks": [{"codes": [{"script": ["SELECT 1;", "SELECT 2;"]}]}]} + result = _normalize_scripts(params, "keboola.exasol-transformation") + assert result["blocks"][0]["codes"][0]["script"] == ["SELECT 1;", "SELECT 2;"] + class TestRowConversion: """Tests for api_row_to_local() and local_row_to_api().""" diff --git a/tests/test_sync_reconcile.py b/tests/test_sync_reconcile.py index c28fc223..dd4ed284 100644 --- a/tests/test_sync_reconcile.py +++ b/tests/test_sync_reconcile.py @@ -18,6 +18,7 @@ import shutil from pathlib import Path +from typing import Any from unittest.mock import MagicMock import yaml @@ -87,11 +88,29 @@ def _make_mock_client( client.verify_token.return_value = verify_token_response if components_response is not None: client.list_components_with_configs.return_value = components_response + # ``sync push`` reads the config back to stamp an API-derived manifest + # baseline (issue #686); serve it from the same remote fixture. + client.get_config_detail.side_effect = _detail_from(components_response) if branches_response is not None: client.list_dev_branches.return_value = branches_response return client +def _detail_from(components: list) -> Any: + """Build a ``get_config_detail`` side effect backed by *components*.""" + + def _detail(component_id: str, config_id: str, branch_id: int | None = None) -> dict: + for component in components: + if component.get("id") != component_id: + continue + for config in component.get("configurations", []): + if str(config.get("id")) == str(config_id): + return dict(config) + raise KeyError(f"{component_id}/{config_id}") + + return _detail + + def _svc(store: ConfigStore, components: list | None = None) -> SyncService: """SyncService whose factory mints a fresh mock client per call.""" return SyncService( diff --git a/tests/test_sync_service.py b/tests/test_sync_service.py index 7dc14a59..acc3f9c7 100644 --- a/tests/test_sync_service.py +++ b/tests/test_sync_service.py @@ -3540,6 +3540,32 @@ def fake_create_config(**kwargs: Any) -> dict[str, str]: client.create_config.side_effect = fake_create_config client.create_config_row.return_value = {"id": "VALS-9"} client.update_config.return_value = {"id": "TX-9"} + + # ``sync push`` reads each written config/row back to stamp an + # API-derived manifest baseline (issue #686). The mutation responses + # above are id-only, so serve the read-back from the post-push remote. + remote = self._remote_after_create() + + def fake_detail(component_id: str, config_id: str, branch_id: Any = None) -> dict[str, Any]: + for component in remote: + if component["id"] != component_id: + continue + for config in component["configurations"]: + if config["id"] == config_id: + return dict(config) + raise KeyError(config_id) + + def fake_row( + component_id: str, config_id: str, row_id: str, branch_id: Any = None + ) -> dict[str, Any]: + parent = fake_detail(component_id, config_id) + for row in parent.get("rows", []): + if row["id"] == row_id: + return dict(row) + raise KeyError(row_id) + + client.get_config_detail.side_effect = fake_detail + client.get_config_row.side_effect = fake_row return client def _remote_after_create(self) -> list[dict[str, Any]]: