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.
This commit is contained in:
ZdenekSrotyr
GitHub
parent
79a958ec26
commit
56db622e36
4 changed files with 281 additions and 10 deletions
19
CHANGELOG.md
19
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."<ds>"."<tbl>"` 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
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
// 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 =
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
217
tests/test_issue_266_bq_edit_modal_destruction.py
Normal file
217
tests/test_issue_266_bq_edit_modal_destruction.py
Normal file
|
|
@ -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."
|
||||
)
|
||||
Loading…
Reference in a new issue