Skip to content

fix: data-modifying WITH statements misrouted across SQLKit (classifier, adapters, JDBC bridge) - #147

Open
Blankll wants to merge 4 commits into
masterfrom
fix/with-dml-classification
Open

fix: data-modifying WITH statements misrouted across SQLKit (classifier, adapters, JDBC bridge)#147
Blankll wants to merge 4 commits into
masterfrom
fix/with-dml-classification

Conversation

@Blankll

@Blankll Blankll commented Sep 3, 2026

Copy link
Copy Markdown
Member

Problem

WITH … INSERT/UPDATE/DELETE/MERGE (data-modifying CTEs) was misrouted across SQLKit in four places:

  1. MCP read-only guard misclassified writes as reads. sqlparser parsed WITH … INSERT/UPDATE as Statement::Query with the DML wrapped inside the query body (SetExpr::Insert/Update) and the CTE definitions. The top-level AST classifier only looked at the outer variant, so:

    • sqlkit__execute_query silently accepted and ran these write statements — the "read-only" promise didn't hold
    • sqlkit__execute_write rejected them with a confusing "does not accept Read statements" error
  2. Postgres adapter swallowed the result. Any SQL starting with WITH was treated as a row-returning query. A WITH … INSERT without RETURNING exposes zero columns, so the writes committed but the UI showed a silently empty grid — no rows_affected, no feedback.

  3. MySQL/SQLite adapters lacked WITH in their read classification. Both engines only allow WITH before read-only SELECT, yet such statements fell into the write branch — rusqlite's execute() rejects row-returning statements (ExecuteReturnedResults), so CTE reads on SQLite hard-errored.

  4. JDBC bridge keyword sniffing. QueryExecutor chose executeQuery() vs executeUpdate() by SQL prefix, so data-modifying WITH statements hit executeQuery() — throwing on drivers that reject result-less statements (SQL Server, Oracle).

  5. sqlparser 0.55 couldn't parse WITH … DELETE/MERGE at all (or DELETE/MERGE inside CTEs), leaving sqlkit__execute_delete unusable for them.

Fix

  • capabilities/sql_write.rs — recursive classifier: classify_query walks the query body and every CTE; combine_kind merges kinds by severity (Read < Write < Ddl < Delete) so a destructive statement never rides inside a tree classified as Read.
  • database/postgres.rs — prepared statements exposing zero result columns (data-modifying WITH chain without RETURNING) run through client.execute() and return rows_affected; WITH … INSERT … RETURNING still returns its rows.
  • database/mysql.rs, database/sqlite.rsWITH added to the read-keyword list.
  • jdbc-bridge/QueryExecutor.java — prefix sniffing removed; Statement.execute() + getResultSet()/getUpdateCount()/getMoreResults() drain decides routing from the driver's actual protocol response.
  • sqlparser 0.55 → 0.62WITH … DELETE/MERGE (body and CTE positions) now parse as SetExpr::Delete/Merge, and the classifier routes them to Delete / Write. Migrated breaking AST changes: 0.55 SET-family Statement variants consolidated into Statement::Set; SelectItem::ExprWithAliases (Spark) handled in column extraction (sql_service.rs).

Tests

  • 8 new unit tests in sql_write.rs (16 classifier tests total; full lib suite 367/367): WITH … INSERT chain (the exact org/membership query from the bug report), DML-in-CTE, WITH … UPDATE, WITH … DELETE (top-level + DELETE-in-CTE), WITH … MERGE, read-guard rejection, existing WITH … SELECT read cases
  • cargo fmt --check clean on changed files; no new clippy warnings
  • mvn compile clean for the JDBC bridge (local check against JDK 21; pom targets 25 for release builds)

Commits

  1. 22f3e34 fix(mcp): detect data-modifying WITH statements in read-only guard
  2. fb26a78 fix(queries): route WITH-prefixed statements through the query path on MySQL/SQLite
  3. 6a8659e fix(jdbc): execute statements via Statement.execute() instead of keyword sniffing
  4. 2146ecf chore(deps): upgrade sqlparser 0.55 to 0.62

Notes

  • Runtime smoke tests against live Oracle/SQL Server instances for the JDBC change were not possible in this environment — recommend a manual pass with a JDBC-routed connection before release.
  • Turso (HTTP executor, no keyword classification) and the SQL Server native adapter (tiberius simple_query + result inspection) were investigated and need no change.

Blankll and others added 2 commits September 4, 2026 00:20
sqlparser 0.55 represents `WITH ... INSERT/UPDATE` as a Statement::Query
whose body and CTEs carry the DML, so the top-level AST classifier let
write statements pass sqlkit__execute_query's read-only gate and rejected
them from sqlkit__execute_write. classify_query now walks the query body
and every CTE (combining kinds by severity) so data-modifying statements
are never classified as Read.

The Postgres adapter also treated any WITH-prefixed statement as a
row-returning query: a `WITH ... INSERT/UPDATE/DELETE` without RETURNING
exposes zero columns, so the writes ran but the result was silently empty.
Column-less prepared statements are now routed through execute() and
report rows_affected.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…n MySQL/SQLite

MySQL and SQLite only allow WITH before read-only SELECT, yet both adapters
classified statements by prefix without WITH, so `WITH ... SELECT` fell into
the write branch: rusqlite's execute() rejects row-returning statements with
ExecuteReturnedResults, making CTE reads on SQLite hard-error.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Blankll and others added 2 commits September 4, 2026 00:35
…ord sniffing

QueryExecutor chose executeQuery vs executeUpdate by uppercasing the SQL
and checking a prefix list. Any statement starting with WITH was treated as
a row-returning query, so data-modifying WITH statements (or plain DML with
a leading CTE) hit executeQuery() — drivers that reject statements without a
result set (SQL Server, Oracle) threw, and others returned rows_affected=0.

Delegate to Statement.execute() and drain results with getResultSet()/
getUpdateCount()/getMoreResults(): row-returning statements (SELECT,
WITH ... SELECT, INSERT ... RETURNING) yield columns+rows; everything else
yields rows_affected. The heuristic is removed entirely.

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
0.62 parses statement shapes 0.55 could not: `WITH ... DELETE`,
`WITH ... MERGE` and DELETE/MERGE inside CTE bodies are now valid ASTs
(Query body carries SetExpr::Delete/SetExpr::Merge), so the MCP classifier
no longer fails closed on them. The recursive classifier gains the
Delete/Merge arms (both the statement body and nested CTE positions) and
routes them to execute_delete / execute_write respectively.

Migration for breaking AST changes:
- sql_write.rs: the 0.55 SET-family variants (SetVariable, SetNames,
  SetNamesDefault, SetRole, SetSessionParam, SetTimeZone, SetTransaction)
  are consolidated into Statement::Set
- sql_service.rs: SelectItem gained ExprWithAliases (Spark) - projected
  names now use its first alias, falling back to the expression name

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
@Blankll Blankll changed the title fix(mcp): detect data-modifying WITH statements in read-only guard fix: data-modifying WITH statements misrouted across SQLKit (classifier, adapters, JDBC bridge) Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant