From 56db622e36257277652e9568d71a9d2b99e24c14 Mon Sep 17 00:00:00 2001 From: ZdenekSrotyr <139972147+ZdenekSrotyr@users.noreply.github.com> Date: Tue, 12 May 2026 17:29:56 +0200 Subject: [PATCH] =?UTF-8?q?release:=200.53.1=20=E2=80=94=20fix=20#266=20BQ?= =?UTF-8?q?=20Edit=20modal=20destroying=20bucket/source=5Ftable=20(#269)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three client-side fixes in admin_tables.html plus a regression test file pinning the server-side PUT contract the new JS relies on. Bug 1 — saveBqTabEdit (synced/custom) nulled bucket/source_table on every save; the null was supposed to clear stale state on a true remote→materialized mode flip but fired on every save, silently wiping persisted bucket/source_table when admin edited only the description on an already-materialized row. Now gated by _editOriginalQueryMode !== 'materialized'. Bug 2/3 — _buildBigQueryPayload (synced/whole) at register time did not send bucket/source_table — only source_query — so whole-table materialized rows persisted with bucket=NULL. Edit modal then loaded empty Dataset/Table inputs over a SELECT * SQL. Register now sends both fields; _openEditBqModal additionally parses source_query as a fallback for rows that registered pre-0.53.1. Closes #266. --- CHANGELOG.md | 19 ++ app/web/templates/admin_tables.html | 53 ++++- pyproject.toml | 2 +- ...est_issue_266_bq_edit_modal_destruction.py | 217 ++++++++++++++++++ 4 files changed, 281 insertions(+), 10 deletions(-) create mode 100644 tests/test_issue_266_bq_edit_modal_destruction.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 41aade4..20f8e8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,25 @@ CalVer image tags (`stable-YYYY.MM.N`, `dev-YYYY.MM.N`) are produced for every C - **Asana connector reverted from hosted Remote MCP back to PAT + raw REST against `app.asana.com/api/1.0`.** The MCP path (introduced in commit `adee8ea`, 2026-05-11) used ~5× the tokens per call because Claude Code reads the entire MCP response envelope; the PAT + REST path lets the agent read only the fields it needs from a flat JSON response. The new Asana prompt stores the PAT in the OS keychain under `agnes-asana-pat`, verifies against `/users/me` before writing, and prints the unified `✅`/`❌` line. Re-running setup on an instance still holding the leftover MCP registration detects it and asks the user to run `claude mcp remove asana` first so the two surfaces don't compete. - **Atlassian connector instructs picking the longest API-token expiry (today: "1 year").** The Atlassian Cloud token-create dropdown defaults to a short-lived expiry; the prompt now tells Claude to direct the user to choose the longest option in the "Expires" dropdown. There's no public query-parameter hook on `id.atlassian.com/manage-profile/security/api-tokens` to pre-select the expiry (verified — `?expiry=1y` returns identical HTML); the prompt acknowledges that limitation so a future contributor doesn't re-investigate. +## [0.53.1] — 2026-05-12 + +Follow-up to 0.53.0 closing #266 — `/admin/tables` Edit modal on BQ +materialized rows silently destroyed `bucket` / `source_table` on every +save, and the prior whole-table register path never persisted them in +the first place. Three small client-side fixes in `admin_tables.html`, +plus regression tests pinning the server-side PUT contract the new JS +relies on. + +### Fixed + +- **Edit modal on custom-SQL materialized rows no longer wipes `bucket` / `source_table`** (#266). `saveBqTabEdit` nulled both fields on every save in the `synced/custom` branch, originally to clear stale values on a remote→materialized mode flip. The null fired even when the operator was just editing description / folder / sync_schedule on an already-materialized row — an unrelated change destroyed the row's `bucket=` and `source_table=` columns. Guarded by `_editOriginalQueryMode !== 'materialized'` so the null only fires on a genuine mode flip; otherwise the keys are omitted from the JSON and the server's `exclude_unset=True` semantics preserve the existing values. +- **Register modal whole-table branch now persists `bucket` + `source_table`** (#266). `_buildBigQueryPayload` previously sent only `source_query` for `synced/whole` registers, leaving `bucket=NULL` on the row even though the dataset+table were the source of truth in the SQL. Edit modal then loaded empty Dataset/Table inputs over a `SELECT *` SQL, and a save with the empty inputs would synthesize a broken `SELECT * FROM bq."".""` SQL. Register now sends both fields alongside the SQL — consistent with the live-mode branch. +- **Edit modal pre-fills Dataset/Table from `source_query` when bucket is null** (#266). Back-compat for whole-table materialized rows that were registered pre-0.53.1 (`bucket=NULL` in the registry). `_openEditBqModal` now parses the `SELECT * FROM bq."".""` form with the same regex it already uses to set the whole/custom radio, and falls back to the captured groups when `table.bucket` is empty. + +### Internal + +- 4 regression tests in `tests/test_issue_266_bq_edit_modal_destruction.py` pin the server-side PUT contract (omitted fields preserved, explicit null clears) and template-grep the three JS-side fixes. + ## [0.53.0] — 2026-05-12 Second hygiene round closing the Tier B trackers opened during the diff --git a/app/web/templates/admin_tables.html b/app/web/templates/admin_tables.html index 0e5350b..e82a2d2 100644 --- a/app/web/templates/admin_tables.html +++ b/app/web/templates/admin_tables.html @@ -2510,10 +2510,19 @@ // fail-open with a warning — operator who needs the cap to engage // picks Custom query and writes backtick-quoted native BQ // identifiers. + // + // Issue #266: also persist bucket+source_table on the row so the + // Edit modal can pre-fill those inputs on a subsequent open. Pre- + // #266 the register flow only sent source_query, leaving bucket= + // NULL in the registry; the Edit modal then loaded empty inputs + // and an admin saving with the visible empty fields would + // synthesize the broken `SELECT * FROM bq."".""` SQL. return { name: viewName || sourceTable, source_type: 'bigquery', query_mode: 'materialized', + bucket: dataset, + source_table: sourceTable, source_query: 'SELECT * FROM bq."' + dataset + '"."' + sourceTable + '"', profile_after_sync: false, description: description, @@ -2849,17 +2858,35 @@ var qmode = (table.query_mode || 'remote'); var sq = (table.source_query || ''); + // Issue #266: register-time the whole-table branch omits + // bucket/source_table from the JSON payload (only source_query is + // sent), so existing whole-table materialized rows persist with + // bucket=NULL in the registry. When the Edit modal then reads + // table.bucket on those rows it gets null → empty input → admin + // saves a broken `SELECT * FROM bq."".""` SQL. Parse the source_query + // to recover the dataset+table for the pre-fill. Same regex shape + // we already use to detect whole-table mode below. + var SELECT_STAR_RE = /^\s*SELECT\s+\*\s+FROM\s+bq\."([^"]+)"\."([^"]+)"\s*$/i; + var isAutoSelectStar = SELECT_STAR_RE.test(sq); if (qmode === 'materialized') { _setEditBqRadio('editBqAccessMode', 'synced'); - var isAutoSelectStar = /^\s*SELECT\s+\*\s+FROM\s+bq\.".+"\.".+"\s*$/i.test(sq); _setEditBqRadio('editBqSyncMode', isAutoSelectStar ? 'whole' : 'custom'); } else { _setEditBqRadio('editBqAccessMode', 'live'); _setEditBqRadio('editBqSyncMode', 'whole'); } - document.getElementById('editBqDataset').value = table.bucket || ''; - document.getElementById('editBqSourceTable').value = table.source_table || ''; + // Pre-fill dataset/table inputs. For whole-table materialized rows + // without persisted bucket/source_table (pre-#266 register state), + // recover them from the SQL. + var preDataset = table.bucket || ''; + var preSourceTable = table.source_table || ''; + if (!preDataset && !preSourceTable && isAutoSelectStar) { + var m = sq.match(SELECT_STAR_RE); + if (m) { preDataset = m[1]; preSourceTable = m[2]; } + } + document.getElementById('editBqDataset').value = preDataset; + document.getElementById('editBqSourceTable').value = preSourceTable; document.getElementById('editBqSourceQuery').value = sq; document.getElementById('editBqSyncSchedule').value = table.sync_schedule || ''; document.getElementById('editBqDescription').value = table.description || ''; @@ -3025,12 +3052,20 @@ payload.query_mode = 'materialized'; payload.source_query = document.getElementById('editBqSourceQuery').value.trim(); - // PUT semantics: explicit null clears prior value so a - // remote→materialized switch doesn't carry stale - // bucket/source_table forward through the synthetic - // RegisterTableRequest validator. - payload.bucket = null; - payload.source_table = null; + // Issue #266: only clear bucket/source_table on a genuine mode + // flip (was non-materialized → now custom-SQL materialized). The + // original pre-#266 code nulled unconditionally on every save — + // so an admin editing only the description on an already- + // materialized custom-SQL row would silently wipe bucket/ + // source_table. Destructive for rows that had them persisted + // (whole-table-then-switched-to-custom, or curl-set via API). + // On no-op saves (mode didn't change since modal load), omit + // the keys entirely so the PUT handler's `exclude_unset=True` + // preserves the existing values in the registry. + if (_editOriginalQueryMode !== 'materialized') { + payload.bucket = null; + payload.source_table = null; + } } else if (accessMode === 'synced' && syncMode === 'whole') { payload.query_mode = 'materialized'; payload.source_query = diff --git a/pyproject.toml b/pyproject.toml index 0a9742f..06d418c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "agnes-the-ai-analyst" -version = "0.53.0" +version = "0.53.1" description = "Agnes — AI Data Analyst platform for AI analytical systems" requires-python = ">=3.11,<3.14" license = "MIT" diff --git a/tests/test_issue_266_bq_edit_modal_destruction.py b/tests/test_issue_266_bq_edit_modal_destruction.py new file mode 100644 index 0000000..e2e8c1d --- /dev/null +++ b/tests/test_issue_266_bq_edit_modal_destruction.py @@ -0,0 +1,217 @@ +"""Regression coverage for #266 — Edit modal on BQ materialized rows +silently nulled bucket / source_table or showed empty inputs. + +Three bugs traced from the Save chain (admin_tables.html JS → PUT +/api/admin/registry/{id} → table_registry upsert): + +1. `saveBqTabEdit` (synced/custom) sent `bucket: null, source_table: + null` on every save — not just on a true remote→materialized mode + flip. An admin editing only the description of an already- + materialized custom-SQL row wiped persisted bucket/source_table. + +2. `_buildBigQueryPayload` (synced/whole) at register time DID NOT + send bucket/source_table — only source_query. So whole-table + materialized rows persisted with bucket=NULL from day one. The + Edit modal then read empty `table.bucket` and rendered empty + Dataset/Table inputs over the SELECT * SQL. + +3. `_openEditBqModal` populated the Dataset/Table inputs from + `table.bucket || ''` only — for whole-table rows registered pre-#266 + (bucket=NULL), the inputs stayed empty. Saving with the empty + inputs would synthesize a broken `SELECT * FROM bq."".""` SQL. + +These tests pin the server-side contract that the client now relies +on: PUTs that OMIT bucket/source_table keys preserve existing values +(thanks to `exclude_unset=True` in `app/api/admin.update_table`). +The template-grep test pins the JS-side fixes themselves. +""" +from __future__ import annotations + + +def _auth(token: str) -> dict: + return {"Authorization": f"Bearer {token}"} + + +def test_put_omitting_bucket_preserves_existing_value_on_materialized_row( + seeded_app, bq_instance, stub_bq_extractor, +): + """Bug 1 fix contract: when the new Edit-modal save path omits + bucket/source_table from the JSON body on a no-op-mode save, the + server preserves the existing values. + + Pre-#266 the JS sent `bucket: null, source_table: null` on every + save in the synced/custom branch — this test pins that an OMITTED + key on a custom-SQL materialized row preserves bucket. Same + invariant the v26 PUT-preservation tests pin for primary_key / + sync_strategy, but specific to bucket/source_table on a + materialized row (which the older tests didn't cover).""" + client = seeded_app["client"] + headers = _auth(seeded_app["admin_token"]) + + # Register a custom-SQL materialized BQ row that ALSO has bucket+ + # source_table set. Both can coexist — bucket is documentary, the + # SQL is the source of truth. Pre-#266 a curl PUT could set this + # state; post-#266 the whole-table register flow also produces it. + custom_sql = ( + "SELECT order_id, customer_id, total_usd " + "FROM `myproj.finance.orders` " + "WHERE event_date >= DATE_SUB(CURRENT_DATE(), INTERVAL 30 DAY)" + ) + resp = client.post( + "/api/admin/register-table", + json={ + "name": "preserve_bucket_test", + "source_type": "bigquery", + "query_mode": "materialized", + "bucket": "finance", + "source_table": "orders", + "source_query": custom_sql, + }, + headers=headers, + ) + assert resp.status_code in (200, 201, 202), resp.text + + # PUT a description-only change — body OMITS bucket/source_table, + # mirroring the post-#266 JS payload-builder behavior on a no-op + # mode save. + resp = client.put( + "/api/admin/registry/preserve_bucket_test", + json={"description": "Customer orders, last 30 days"}, + headers=headers, + ) + assert resp.status_code == 200, resp.text + + reg = client.get("/api/admin/registry", headers=headers).json() + row = next(t for t in reg["tables"] if t["id"] == "preserve_bucket_test") + assert row["bucket"] == "finance" + assert row["source_table"] == "orders" + assert row["description"] == "Customer orders, last 30 days" + assert row["source_query"] == custom_sql + assert row["query_mode"] == "materialized" + + +def test_put_explicit_null_clears_bucket_on_mode_flip( + seeded_app, bq_instance, stub_bq_extractor, +): + """Bug 1 fix contract: a TRUE mode-flip save (remote → materialized + custom) still wants to clear stale bucket/source_table from the + old mode. The JS sends explicit null in that case; the server + persists NULL. This pins the existing exclude_unset semantics + that distinguish 'omitted' (preserve) from 'explicit null' (clear) + — see admin.py:2636-2654 inline comment.""" + client = seeded_app["client"] + headers = _auth(seeded_app["admin_token"]) + + # Register remote BQ row with bucket+source_table. + resp = client.post( + "/api/admin/register-table", + json={ + "name": "flip_remote_to_custom", + "source_type": "bigquery", + "bucket": "finance", + "source_table": "orders", + }, + headers=headers, + ) + assert resp.status_code in (200, 201, 202), resp.text + + # Mode flip: remote → materialized custom-SQL. JS sends explicit + # null in this branch (its `_editOriginalQueryMode !== 'materialized'` + # condition fires). + new_sql = "SELECT 1 AS placeholder" + resp = client.put( + "/api/admin/registry/flip_remote_to_custom", + json={ + "query_mode": "materialized", + "source_query": new_sql, + "bucket": None, + "source_table": None, + }, + headers=headers, + ) + assert resp.status_code == 200, resp.text + + reg = client.get("/api/admin/registry", headers=headers).json() + row = next(t for t in reg["tables"] if t["id"] == "flip_remote_to_custom") + assert row["query_mode"] == "materialized" + assert row["bucket"] is None + assert row["source_table"] is None + assert row["source_query"] == new_sql + + +def test_register_whole_table_materialized_persists_bucket( + seeded_app, bq_instance, stub_bq_extractor, +): + """Bug 2/3 fix contract: post-#266 the JS whole-table register + branch sends bucket+source_table alongside source_query so a + subsequent Edit modal can pre-fill those inputs from the + persisted values (instead of leaving them empty / re-parsing + SQL). + + The server already accepts this shape — `_validate_bigquery_register_payload` + treats bucket/source_table as optional when source_query is + provided. This test pins that the persisted row carries both.""" + client = seeded_app["client"] + headers = _auth(seeded_app["admin_token"]) + + # Mirror the post-#266 JS payload for synced/whole register. + resp = client.post( + "/api/admin/register-table", + json={ + "name": "whole_table_with_bucket", + "source_type": "bigquery", + "query_mode": "materialized", + "bucket": "analytics", + "source_table": "page_views", + "source_query": 'SELECT * FROM bq."analytics"."page_views"', + }, + headers=headers, + ) + assert resp.status_code in (200, 201, 202), resp.text + + reg = client.get("/api/admin/registry", headers=headers).json() + row = next(t for t in reg["tables"] if t["id"] == "whole_table_with_bucket") + assert row["bucket"] == "analytics" + assert row["source_table"] == "page_views" + assert row["query_mode"] == "materialized" + # source_query persisted verbatim (DuckDB three-part-alias form; + # the materialize wrapper translates this to BQ-native at exec). + assert 'bq."analytics"."page_views"' in row["source_query"] + + +def test_admin_tables_template_has_266_fixes(): + """Pin the three JS-side fixes in the rendered template. These + are tested by string presence rather than headless browser since + Agnes has no JS test harness. A future maintainer who reverts + one of them will trip an obvious failure here.""" + from pathlib import Path + tpl = Path(__file__).parent.parent / "app" / "web" / "templates" / "admin_tables.html" + text = tpl.read_text(encoding="utf-8") + + # Bug 1: saveBqTabEdit synced/custom branch must guard the null + # writes with a mode-flip check, not null unconditionally. + assert "_editOriginalQueryMode !== 'materialized'" in text, ( + "Bug 1 regression: saveBqTabEdit nulls bucket/source_table " + "unconditionally — must guard on a real mode flip." + ) + # The unconditional null pattern from pre-#266 must be GONE in + # the custom branch. We grep for the surrounding comment-tail. + assert "payload.bucket = null;\n payload.source_table = null;\n } else" not in text, ( + "Bug 1 regression: the unconditional `payload.bucket = null` " + "block is back in the synced/custom branch." + ) + + # Bug 2/3: _buildBigQueryPayload synced/whole branch must include + # bucket+source_table in the JSON. + assert "bucket: dataset," in text, ( + "Bug 2/3 regression: _buildBigQueryPayload whole-table branch " + "must send bucket alongside source_query so Edit can pre-fill." + ) + + # _openEditBqModal must parse dataset+source_table out of the + # source_query when bucket is empty (back-compat for rows + # registered pre-#266 with bucket=NULL). + assert "if (!preDataset && !preSourceTable && isAutoSelectStar)" in text, ( + "Bug 2 regression: _openEditBqModal must fall back to parsing " + "source_query for pre-#266 whole-table rows with bucket=NULL." + )