Skip to content

refactor(jdbc): route statement SQL checks through SqlTokenizer - #165

Merged
magnusp merged 2 commits into
mainfrom
issue-155-sql-tokenizer
Oct 23, 2026
Merged

magnusp merged 2 commits into
mainfrom
issue-155-sql-tokenizer

Conversation

@magnusp

@magnusp magnusp commented Oct 23, 2026

Copy link
Copy Markdown
Owner

Why

The statement and the prepared statement each carried their own SQL scanner, so the driver kept three copies of the same SQLite lexical rules. SqlTokenizer already owns those rules for result metadata. This change routes the last two call sites through it and deletes the copies.

What changed

  • LibsqlStatement.isInsertSql reads the first depth-0 statement keyword from SqlTokenizer. Generated keys now treat comments and quoted identifiers the same way the metadata parsers do.
  • LibsqlPreparedStatement decides executeQuery against executeUpdate from the same tokens. Its private Token record and scanner are gone.
  • Fifteen behavior tests cover INSERT OR REPLACE, comments before and inside a WITH clause, quoted identifiers, PRAGMA with =, WITH...INSERT RETURNING, and malformed input.

Scope

The old scanners ignored a comment before INSERT and a comment between a WITH clause and its INSERT, so those statements reported no generated keys. The shared tokenizer fixes both, and the new tests pin them. Malformed SQL now classifies as an update. That changes the local error for executeQuery, but no statement that executes changes behavior.

Tradeoffs

I sampled four reusable tokenizers and parsers. JSqlParser lexed all 38 SQLite samples but parsed only 26, so it would still need this fallback. The ANTLR SQLite grammar is the best external lexer, but the in-repo tokenizer has no dependency and already matches SQLite's rules.

Blast Radius

Every getGeneratedKeys call and every prepared-statement executeQuery and executeUpdate decision now uses SqlTokenizer. The module suite and the xerial conformance suite cover both paths.

Verification

  • ./mvnw -pl libsql-jdbc test: 351 tests, 0 failures. The 336 tests green at HEAD stayed green after the source change alone.
  • ./mvnw -pl libsql-conformance -am verify: 268 tests, 0 failures, 7 skipped.
  • The new tests against the old source: 4 fail, two on ignored comments and two on malformed input.

Cover INSERT OR REPLACE, comments, quoted identifiers, PRAGMA assignment, and malformed input.
@magnusp

magnusp commented Oct 23, 2026

Copy link
Copy Markdown
Owner Author

PASS+NOTES

Verified revisions

  • head: e652c87792bc47ab9cc33f6ee0c97906ebd9036b (branch issue-155-sql-tokenizer)
  • base: f4e0d9679eac4eac65733624f781d8321c65547f (base branch main)
  • patch-id: 1ed05db1f16b0e5764122f41f9d116516fd3bea4, recomputed with git diff f4e0d96 e652c87 | git patch-id --stable — matches the value supplied by the owner.

Static and unit level (at head)

  • mise exec -- ./mvnw -pl libsql-jdbc test → BUILD SUCCESS, 351 tests, 0 failures/errors/skips.
  • mise exec -- ./mvnw -pl libsql-conformance -am verify → BUILD SUCCESS, all modules SUCCESS, 268 tests, 0 failures/errors, 7 skipped.

User-level probe (at head)

Real sqld container (ghcr.io/tursodatabase/libsql-server:latest) reached through the JDBC driver (jdbc:libsql: + getHttpUrl()), table k(id integer primary key autoincrement, v text). Throwaway Issue155ProbeIT modeled on GeneratedKeysIT, extended SqldContainerBase, not committed; run with the exact command given (DOCKER_CONFIG=... mise exec -- ./mvnw -pl libsql-itests -am -Dit.test=Issue155ProbeIT -DfailIfNoTests=false verify). At head: 10/10 pass, BUILD SUCCESS.

  • /* seed */ insert into k (v) values ('a') via Statement.executeUpdate → getGeneratedKeys() exactly 1 row. PASS
  • -- seed\ninsert into k (v) values ('a') → 1 row. PASS
  • with cte as (select 1 as c) /* insert */ insert into k (v) select c from cte → 1 row. PASS
  • insert or replace into k (id, v) values (10, 'b') → 1 row. PASS
  • with "insert" as (select 1) select * from "insert" via executeUpdate → getGeneratedKeys() empty. PASS
  • prepared pragma table_info('k') → executeQuery() returns rows; executeUpdate() throws SQLException("Query returns results"). PASS
  • prepared pragma journal_mode = wal → executeQuery() throws SQLException("Query does not return results"); executeUpdate() is not blocked by the JDBC guard, but sqld's parser rejects the statement server-side: SQL string could not be parsed: unsupported statement: pragma journal_mode = wal. PARTIAL — see note 1.
  • prepared with cte as (select 1 as c) insert into k (v) select c from cte returning id → executeQuery() returns a row. PASS
  • prepared with cte as (select 1 as c) insert into k (v) select c from cte → executeUpdate() returns 1. PASS
  • Supplementary: prepared pragma foreign_keys = on (a pragma assignment sqld accepts) → executeUpdate() succeeds and executeQuery() throws Query does not return results, proving the pragma-assignment update path end to end. PASS

Parent against head

Checked out base f4e0d96 in the same worktree and ran the identical probe. 3 failures, all expected: 1 but was: 0 (empty getGeneratedKeys()):

  • /* seed */ insert ... (block comment before INSERT)
  • -- seed\ninsert ... (line comment before INSERT)
  • with cte as (select 1 as c) /* insert */ insert ... (comment between the CTE and the INSERT)

The other 7 probes pass at base. The worktree was returned to head e652c87 and is clean.

Note 2: the owner's claim that exactly the two comment probes fail at base is off by one. The CTE-plus-comment probe fails at base as well, so the change fixes three comment cases, not two. This does not weaken the change; it only corrects the stated base delta.

Notes

  1. The spec's pragma journal_mode = wal assertion "executeUpdate() succeeds" cannot be observed against this server: sqld rejects journal_mode writes in its own SQL parser, identically at head and base. The driver-side classification that this change controls is still pinned in both directions (executeQuery is refused locally; executeUpdate reaches the server), and the same code path is proven end-to-end with pragma foreign_keys = on, which sqld accepts. This is a server limitation, not a driver regression, and it does not block the merge.
  2. See the parent-against-head correction above.
  3. No blocking issues found. The tokenizer-backed classification holds for generated keys and for prepared-statement query/update selection on a real connection.

@magnusp
magnusp merged commit c34d08f into main Oct 23, 2026
6 checks passed
@magnusp
magnusp deleted the issue-155-sql-tokenizer branch October 23, 2026 21:44
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