From cbcdd813121f0ec7828f34c9b104a9977b6172c7 Mon Sep 17 00:00:00 2001 From: Vaclav Nosek Date: Mon, 3 Aug 2026 14:23:40 +0200 Subject: [PATCH] fix(sl-toolkit): edits use PATCH, and sql_dialect is snake_case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs, both verified against a live metastore (canary-orion). Either one is enough to make the skill fail or do damage. **1. `sqlDialect` is rejected — `/sl-build` cannot create a model today.** The API requires snake_case `sql_dialect`; camelCase returns `422 missing property 'sql_dialect'`. The skill's POST envelope, sl-build's model-create call, and the test fixture all used `sqlDialect`, so the greenfield wizard fails at its first write. Both CI guards were asserting the wrong direction — `test_sqldialect_is_camelcase` and `test_no_dialect_drift` required camelCase and *forbade* the correct key, and `schemas/semantic-model.json` did the same in its `not.anyOf`. That is why the bug shipped and stayed. Assertions inverted, and the consistency check now matches the quoted payload key rather than the bare word so the gotchas section can still name `sqlDialect` in prose. Also corrected the schema enum: only `Snowflake` and `BigQuery` are accepted. `Redshift` and `Postgres` were listed but both return `422 value must be one of 'Snowflake', 'BigQuery'`. **2. `PATCH` exists — the documented DELETE + POST edit path is destructive.** `PATCH /api/v1/repository/{type}/{id}` updates in place, keeps the object's UUID and bumps its revision (verified 1 -> 2 on both a metric and a glossary term). The skill said "the metastore has no PATCH" and built rollback machinery around delete-then-post, which mints a new UUID, resets revision history, breaks anything referencing the old UUID, and can leave the layer missing an object if the POST fails. Rewrote the edit section around `api_patch` (new helper) — send only changed fields. The rename cascade to constraint `metrics[]` is preserved and also now in-place. The rollback block is gone: nothing is deleted, so there is nothing to roll back. All 30 tests pass; confirmed the corrected guards fail when the bug is reintroduced. --- plugins/sl-toolkit/README.md | 2 +- plugins/sl-toolkit/commands/sl-build.md | 2 +- .../sl-toolkit/skills/semantic-layer/SKILL.md | 77 +++++++++++-------- .../tests/fixtures/semantic-model.json | 2 +- .../tests/schemas/semantic-model.json | 6 +- .../tests/test_skill_consistency.py | 15 +++- plugins/sl-toolkit/tests/test_smoke.py | 7 +- 7 files changed, 64 insertions(+), 47 deletions(-) diff --git a/plugins/sl-toolkit/README.md b/plugins/sl-toolkit/README.md index 4f20725..1380bcf 100644 --- a/plugins/sl-toolkit/README.md +++ b/plugins/sl-toolkit/README.md @@ -42,7 +42,7 @@ Supports GCP, AWS, and Azure stacks — metastore region derived automatically f ## Key design decisions -**No PATCH endpoint** — all edits are DELETE old id + POST new, with rollback on POST failure. +**Edits use PATCH** — `PATCH /api/v1/repository/{type}/{id}` updates in place, preserving the object's UUID and bumping its revision. Never DELETE + POST: that mints a new UUID, resets revision history, and breaks anything referencing the old one. **Constraint cascade on rename** — renaming a metric auto-updates constraint `metrics[]` references to prevent orphan FKs in `DIM_METRIC_THRESHOLD` tables. diff --git a/plugins/sl-toolkit/commands/sl-build.md b/plugins/sl-toolkit/commands/sl-build.md index 7a558f7..c995676 100644 --- a/plugins/sl-toolkit/commands/sl-build.md +++ b/plugins/sl-toolkit/commands/sl-build.md @@ -320,7 +320,7 @@ if UPDATE_ID: else: uuid = api_post('/api/v1/repository/semantic-model', { 'name': model['name'], - 'data': {'name': model['name'], 'description': model['description'], 'sqlDialect': 'Snowflake'}, + 'data': {'name': model['name'], 'description': model['description'], 'sql_dialect': 'Snowflake'}, 'branch': 'main', 'schemaVersion': '1.0.0', 'scope': 'project' })['data']['id'] print(f'✓ model created {uuid}') diff --git a/plugins/sl-toolkit/skills/semantic-layer/SKILL.md b/plugins/sl-toolkit/skills/semantic-layer/SKILL.md index a7f834c..9f87ac7 100644 --- a/plugins/sl-toolkit/skills/semantic-layer/SKILL.md +++ b/plugins/sl-toolkit/skills/semantic-layer/SKILL.md @@ -84,6 +84,12 @@ def api_post(path, body): with urllib.request.urlopen(req, timeout=30) as r: return json.loads(r.read()) +def api_patch(path, body): + req = urllib.request.Request( + f"{METASTORE}{path}", json.dumps(body).encode(), H, method='PATCH') + with urllib.request.urlopen(req, timeout=30) as r: + return json.loads(r.read()) + def api_delete(path): req = urllib.request.Request(f"{METASTORE}{path}", headers=H, method='DELETE') urllib.request.urlopen(req, timeout=15) @@ -111,6 +117,7 @@ def db_name(): ``` GET /api/v1/repository/{type} → {"data": [...]} POST /api/v1/repository/{type} → {"data": {item}} +PATCH /api/v1/repository/{type}/{id} → {"data": {item}} # in-place update DELETE /api/v1/repository/{type}/{id} ``` @@ -135,7 +142,7 @@ on the returned list — the `?modelId` query param is unreliable. ```json { "name": "", - "data": { "name": "", "description": "...", "sqlDialect": "Snowflake" }, + "data": { "name": "", "description": "...", "sql_dialect": "Snowflake" }, "branch": "main", "schemaVersion": "1.0.0", "scope": "project" @@ -312,16 +319,13 @@ or the constraint will create orphan FKs in downstream DIM_METRIC_THRESHOLD tabl ### Edit an entity -The metastore has no PATCH — editing is DELETE old + POST updated. +**Edit in place with `PATCH`.** Send only the fields that change — the object keeps its UUID and +gains a revision, so history is preserved and anything referencing it by UUID stays valid. Always show the diff to the user and get confirmation before proceeding. ```python import urllib.error, re -def envelope(name, data): - return {"name": name, "data": {**data, "modelUUID": MODEL_UUID}, - "branch": "main", "schemaVersion": "1.0.0", "scope": "project"} - # 1. Fetch and find item TYPE = 'semantic-metric' # replace with actual type all_items = api_get(f"/api/v1/repository/{TYPE}") @@ -331,15 +335,13 @@ target = next((i for i in items if not target: print("Not found. Available:", [i['attributes'].get('name') for i in items]) -# 2. Build updated attrs, save original for rollback -original_attrs = {**target['attributes']} -NEW_ATTRS = {**original_attrs} -# apply changes, e.g.: NEW_ATTRS['sql'] = '' +# 2. Decide the change (a partial patch — not the whole object) +OLD_NAME = target['attributes'].get('name', '') +CHANGES = {} # e.g. {'sql': ''} or {'name': 'Total Revenue'} +NEW_NAME = CHANGES.get('name', OLD_NAME) # 3. If renaming a metric — find constraints to cascade-update -OLD_NAME = original_attrs.get('name', '') -NEW_NAME = NEW_ATTRS.get('name', '') -is_rename = TYPE == 'semantic-metric' and OLD_NAME != NEW_NAME +is_rename = TYPE == 'semantic-metric' and NEW_NAME != OLD_NAME affected_constraints = [] if is_rename: all_c = api_get("/api/v1/repository/semantic-constraint") @@ -354,35 +356,33 @@ if is_rename: if affected_constraints: print(f"Constraints to auto-update: {[c['attributes']['name'] for c in affected_constraints]}") -# 4. Delete old + POST updated (rollback on POST failure) -api_delete(f"/api/v1/repository/{TYPE}/{target['id']}") -new_name = NEW_ATTRS.get('name') or NEW_ATTRS.get('term') +# 4. PATCH in place. Include `name` at the envelope top level only when it changed, +# so the metastore's own `meta.name` stays in sync with the payload. +body = {"data": CHANGES} +if is_rename or 'term' in CHANGES: + body["name"] = CHANGES.get('name') or CHANGES.get('term') try: - r = api_post(f"/api/v1/repository/{TYPE}", envelope(new_name, NEW_ATTRS)) - print(f"✓ Updated: {r['data']['id']}") + r = api_patch(f"/api/v1/repository/{TYPE}/{target['id']}", body) + print(f"✓ Updated {r['data']['id']} (revision {r['data'].get('meta', {}).get('revision')})") except urllib.error.HTTPError as e: - print(f"✗ POST failed ({e.code}) — attempting rollback...") - orig_name = original_attrs.get('name') or original_attrs.get('term') - try: - api_post(f"/api/v1/repository/{TYPE}", envelope(orig_name, original_attrs)) - print("✓ Rollback succeeded — original restored") - except Exception: - print("✗ Rollback also failed — check metastore manually") + # Nothing was deleted, so there is nothing to roll back — the object is untouched. + print(f"✗ PATCH failed ({e.code}): {e.read().decode()[:300]}") raise -# 5. Cascade constraint updates on rename +# 5. Cascade constraint updates on rename — also in place for c in affected_constraints: - c_attrs = {**c['attributes']} - c_attrs['metrics'] = [NEW_NAME if m == OLD_NAME else m for m in c_attrs.get('metrics', [])] - api_delete(f"/api/v1/repository/semantic-constraint/{c['id']}") + metrics = [NEW_NAME if m == OLD_NAME else m for m in (c['attributes'].get('metrics') or [])] try: - api_post("/api/v1/repository/semantic-constraint", - envelope(c_attrs['name'], c_attrs)) - print(f" ✓ Constraint updated: {c_attrs['name']}") + api_patch(f"/api/v1/repository/semantic-constraint/{c['id']}", {"data": {"metrics": metrics}}) + print(f" ✓ Constraint updated: {c['attributes']['name']}") except urllib.error.HTTPError as e: - print(f" ✗ {c_attrs['name']}: {e.code}") + print(f" ✗ {c['attributes']['name']}: {e.code}") ``` +> **Do not edit by DELETE + POST.** It destroys the object's UUID and revision history, breaks +> anything referencing it by UUID, and opens a window where the layer is missing an object if the +> POST fails. `PATCH` has none of those problems. + > **⚠ Dataset/relationship renames** are not cascaded. Renaming a dataset's *semantic name* is > safe. Changing its `tableId` breaks all metrics and relationships pointing to it — coordinate > those changes manually. @@ -421,6 +421,12 @@ joining on `CODE_METRIC` breaks silently if a metric is renamed. Prefer additive **Constraint severity has only 3 API levels** — `error`/`warning`/`info` isn't enough for 4-band health UIs. Encode real severity in the constraint name suffix instead. +**`sql_dialect` is snake_case and a closed set** — exactly `'Snowflake'` or `'BigQuery'`, +capitalized. camelCase `sqlDialect` is rejected with `422 missing property 'sql_dialect'`, +and a lowercase value with `422 value must be one of 'Snowflake', 'BigQuery'`. Both errors +surface only as a generic "Validation failed", so they are easy to misdiagnose. Take the +project's real backend from the stack rather than assuming Snowflake. + **modelUUID differs per project** — dev and prod have different UUIDs for the same logical model. When promoting, fetch the target project's model list to find its UUID, then replace `modelUUID` on each item before POSTing. @@ -433,4 +439,7 @@ for t in ['semantic-metric','semantic-dataset','semantic-glossary', open(f'/tmp/sl_backup_{t}.json','w'), indent=2) ``` -**No PATCH endpoint** — the metastore has no update operation. Editing = DELETE old id + POST new. +**Edit with PATCH, never DELETE + POST** — `PATCH /api/v1/repository/{type}/{id}` updates in place, +preserving the object's UUID and bumping its revision. Deleting and re-posting mints a new UUID, +resets revision history, breaks anything referencing the old UUID, and can leave the layer missing +an object if the POST fails. diff --git a/plugins/sl-toolkit/tests/fixtures/semantic-model.json b/plugins/sl-toolkit/tests/fixtures/semantic-model.json index 97128a2..5abb9f5 100644 --- a/plugins/sl-toolkit/tests/fixtures/semantic-model.json +++ b/plugins/sl-toolkit/tests/fixtures/semantic-model.json @@ -3,7 +3,7 @@ "data": { "name": "Revenue Analytics", "description": "Greenfield model for revenue tracking", - "sqlDialect": "Snowflake" + "sql_dialect": "Snowflake" }, "branch": "main", "schemaVersion": "1.0.0", diff --git a/plugins/sl-toolkit/tests/schemas/semantic-model.json b/plugins/sl-toolkit/tests/schemas/semantic-model.json index 68337cf..14bdcd8 100644 --- a/plugins/sl-toolkit/tests/schemas/semantic-model.json +++ b/plugins/sl-toolkit/tests/schemas/semantic-model.json @@ -2,16 +2,16 @@ "$schema": "https://json-schema.org/draft/2020-12/schema", "title": "semantic-model.data", "type": "object", - "required": ["name", "description", "sqlDialect"], + "required": ["name", "description", "sql_dialect"], "additionalProperties": true, "properties": { "name": {"type": "string", "minLength": 1}, "description": {"type": "string"}, - "sqlDialect": {"type": "string", "enum": ["Snowflake", "BigQuery", "Redshift", "Postgres"]} + "sql_dialect": {"type": "string", "enum": ["Snowflake", "BigQuery"]} }, "not": { "anyOf": [ - {"required": ["sql_dialect"]}, + {"required": ["sqlDialect"]}, {"required": ["sqldialect"]}, {"required": ["SqlDialect"]} ] diff --git a/plugins/sl-toolkit/tests/test_skill_consistency.py b/plugins/sl-toolkit/tests/test_skill_consistency.py index a85e6ab..ad0567f 100644 --- a/plugins/sl-toolkit/tests/test_skill_consistency.py +++ b/plugins/sl-toolkit/tests/test_skill_consistency.py @@ -43,11 +43,18 @@ def test_no_hardcoded_keboola_in_fqn_construction(): ) -def test_sqldialect_is_camelcase(): - """Regression: PR #72 sql_dialect bug. SKILL.md must use sqlDialect, never snake_case.""" +def test_sql_dialect_is_snake_case(): + """The metastore requires snake_case `sql_dialect` and rejects camelCase `sqlDialect` + with a 422 ("missing property 'sql_dialect'"), verified against a live stack. This + assertion was previously inverted, which is why the skill shipped a payload the API + could not accept. + + Matches the KEY as it appears in a payload (quoted), not the bare word — the gotchas + section legitimately names `sqlDialect` in prose to say it is rejected.""" text = read(SKILL_MD) - assert "sqlDialect" in text, "SKILL.md must document sqlDialect" - assert "sql_dialect" not in text, "snake_case sql_dialect is the PR #72 regression" + assert '"sql_dialect"' in text, "SKILL.md must document the sql_dialect key" + for bad in ('"sqlDialect"', "'sqlDialect'"): + assert bad not in text, f"{bad} as a payload key is rejected by the metastore (422)" def test_no_allowed_tools_wildcard_in_skill_frontmatter(): diff --git a/plugins/sl-toolkit/tests/test_smoke.py b/plugins/sl-toolkit/tests/test_smoke.py index 78042e1..e03f7e0 100644 --- a/plugins/sl-toolkit/tests/test_smoke.py +++ b/plugins/sl-toolkit/tests/test_smoke.py @@ -42,10 +42,11 @@ def test_data_shape(entity): def test_no_dialect_drift(): - """Regression test for PR #72: sqlDialect must be camelCase, never snake_case.""" + """The metastore requires snake_case `sql_dialect`; camelCase `sqlDialect` is rejected + with a 422. This assertion was previously inverted.""" fixture = load(FIXTURES / "semantic-model.json") - assert "sqlDialect" in fixture["data"], "semantic-model.data must use camelCase sqlDialect" - assert "sql_dialect" not in fixture["data"], "snake_case sql_dialect is the bug from #72" + assert "sql_dialect" in fixture["data"], "semantic-model.data must use sql_dialect" + assert "sqlDialect" not in fixture["data"], "camelCase sqlDialect is rejected (422)" def test_constraint_severity_suffix():