Repository navigation
fix(store): never INSERT workspace_metadata on read - #45
Merged
Merged
Conversation
A workspace whose tables predate workspace_metadata (or was created outside the app) has no metadata row. get_workspace_metadata() then executed an INSERT, so the first semantic read by a non-admin failed with "Access denied" — the same class of bug as the CREATE DATABASE one, just in a narrower path. Reads now call the helper with create_missing=False and fall back to the defaults (enabled, version 1), which keeps the version check stable. The row is still created by the next admin write (toggle, commit, deploy).
The recurring "read path performs a write" failures share one root cause: mutations live in helpers that reads also call (_ensure_tables, the metadata getter), and each was fixed at the call site, so the next read path leaked. Every mutating method now starts with _require_write(), which raises for any request that may not provision storage. A future read path that reaches a write fails immediately, with a message about the programming error instead of an "Access denied" from Doris. Tests: all write methods raise for a read-only caller without touching Doris, and a sweep of the read methods asserts no write SQL is emitted.
Every write path was guarded individually, and each new read path could miss the guard — that is why this class of bug kept coming back. Move the guarantee to the one place all statements pass through: _get_conn() now returns a connection proxy whose cursors refuse INSERT/UPDATE/DELETE/DDL/GRANT/SET unless the request is allowed to provision storage. Reads (SELECT/USE/SHOW/DESCRIBE) are untouched. A forgotten guard now raises WriteNotAllowed before anything reaches Doris, instead of surfacing as the user's "Access denied". Tests: write statements are refused for a read-only caller without touching the connection; SELECT/USE pass through; an admin request can still write.
maruyue
force-pushed
the
fix/no-insert-on-read
branch
from
October 21, 2026 08:44
4e7cceb to
313f8b4
Compare
Old deployments (or workspaces created outside the app) can have semantic tables without a usable workspace_metadata table. Until now the only signal was a non-admin "Access denied" on the first read. - metadata_schema_status(): read-only probe returning fresh / ok / needs_migration / outdated, plus workspaces missing a row. - migrate_metadata_schema(): admin-only; creates the table and backfills one row per active_store_* workspace. Missing columns are reported instead of altered, since no released schema ever differed. - Writes raise SchemaMigrationRequired until the migration succeeds; fresh installs are unaffected (state "fresh"). - Web UI: the models page shows a migration banner for admins with a "Migrate metadata" button (POST /mcp/web/migrations/metadata). Non-admins see an instruction to contact an admin. - Store factory maps the error to HTTP 409. Tests cover the four states, blocking, and the migration's DDL/backfill.
Creating workspace_metadata (or migrating to it) without the database-level grant left ordinary users unable to read it, so their first semantic query still failed. Issue GRANT SELECT_PRIV ON system_mcp.* TO % after provisioning and after migration, matching what commit/deploy already did.
maruyue
force-pushed
the
fix/no-insert-on-read
branch
2 times, most recently
from
October 21, 2026 09:05
7671c04 to
3c1b7e2
Compare
A missing workspace_metadata table (or system_mcp database) blocked every semantic read: loading a workspace starts by reading its published version. get_workspace_metadata() now treats "unknown database/table" as the default state (enabled, version 1), so models still load and ordinary users can query them before an admin creates the table. Other errors still propagate. Writes remain blocked until the migration runs.
Migration was a precondition for every write: needs_migration/outdated made _require_write() and _ensure_tables() raise, so an admin could not create a workspace or commit until they clicked the banner. Admin writes now provision as before (table, row, grant); the migration stays as an optional one-shot backfill of missing rows/columns. Ordinary users are still refused by _require_write() and by the connection-level guard.
The query pool was the last unguarded place SQL reaches Doris: it trusted callers to validate first (sqlglot for execute_query, validate_readonly for compiled metric SQL). Add a keyword backstop that only admits SELECT/WITH/SHOW/DESCRIBE/EXPLAIN, so a missed validation cannot execute a write with the caller's credentials.
Detection now checks column types as well as presence, and the migration: - adds missing columns with dtype-appropriate DDL (ALTER TABLE ... ADD COLUMN); - tolerates a concurrent migration (duplicate-key inserts are skipped, not fatal); - refuses to guess when a column has the wrong type or duplicate rows exist: it reports them instead of rewriting data; - still backfills one row per active_store_* workspace and re-grants SELECT. Reads also tolerate the "unknown column" error, so an outdated table degrades to defaults instead of breaking workspace loading. Tests cover detection (fresh/needs_migration/missing columns/wrong type), ALTER-based repair, backfill+grant, duplicate reporting and admin-only access.
A duplicate row means the table is not UNIQUE KEY(workspace); altering or backfilling it would make the corruption worse. Migration now inspects first and aborts, with nothing written, when it finds duplicates, a wrong key model, or an unfixable column type. Only a table that passes verification is repaired (add missing columns, backfill rows, grant read).
Duplicates mean the table is not UNIQUE KEY(workspace), but deleting or rewriting those rows is not safe. Migration now skips duplicated workspaces, still backfills missing ones, and reports the duplicates as warnings. Wrong column types still abort before any data change.
maruyue
force-pushed
the
fix/no-insert-on-read
branch
from
October 21, 2026 09:38
d3118a3 to
7450355
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Symptom
Admin creates a workspace (or one already exists from an older deployment), then a
non-admin calls a semantic tool for it:
Access denied— the read path tried to write.Cause
get_workspace_metadata()called_get_workspace_metadata_with_cursor(), whichinserts a default row when the workspace has none. A workspace without a row can
come from:
workspace_metadataexisted (feature added in 2748afb),_ensure_tables()only logged a warning.Reads then attempted the INSERT as the caller, and non-admins have no INSERT right.
Same class of bug as the
CREATE DATABASEone fixed in #42 — a read path performinga write.
Fix
create_missing=Falseon reads: return the defaults (enabled, version 1) instead ofinserting. The row is still created by the next admin write (toggle, commit, deploy).
Tests
New case: metadata read without a row issues SELECT only, no INSERT. Write path
still inserts. Offline suite passes.