From 6e24d89cc9eee32aac4032cbccbad7cb9381259f Mon Sep 17 00:00:00 2001 From: rahulmahadik Date: Fri, 14 Aug 2026 09:38:05 +0800 Subject: [PATCH 1/2] Reattach a doc comment to the function it describes --- .../kotlin/com/rahulmahadik/asksql/ide/engine/IdentifierCase.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/jetbrains/src/main/kotlin/com/rahulmahadik/asksql/ide/engine/IdentifierCase.kt b/packages/jetbrains/src/main/kotlin/com/rahulmahadik/asksql/ide/engine/IdentifierCase.kt index c0b0f0b..9f81fca 100644 --- a/packages/jetbrains/src/main/kotlin/com/rahulmahadik/asksql/ide/engine/IdentifierCase.kt +++ b/packages/jetbrains/src/main/kotlin/com/rahulmahadik/asksql/ide/engine/IdentifierCase.kt @@ -24,10 +24,10 @@ object IdentifierCase { return "$quoteChar$name$close" } - /** Where a literal or comment ends, or -1 when the position starts neither. */ /** A dollar-quoted body is a literal in Postgres and DuckDB, and may contain anything. */ private val DOLLAR_OPEN = Regex("""\$[A-Za-z_]\w*\$|\$\$""") + /** Where a literal or comment ends, or -1 when the position starts neither. */ private fun skipTo(sql: String, i: Int, doubleQuoteIsLiteral: Boolean, backslashEscapes: Boolean = false): Int { val ch = sql[i] val next = if (i + 1 < sql.length) sql[i + 1] else ' ' From 57ee4016a685698253446a7106a584b876238ca0 Mon Sep 17 00:00:00 2001 From: rahulmahadik Date: Fri, 14 Aug 2026 09:48:29 +0800 Subject: [PATCH 2/2] Test identifier handling against real engines and hostile schemas A mixed-case Postgres schema failed every query and shipped that way for weeks. Nothing in CI had ever pointed at a schema we did not write ourselves, so every database test shared the same blind spots. This runs the normalise-then-guard path against Postgres, MySQL and SQLite over schemas built to break the rules: mixed case, reserved words, spaces, unicode, names the parser treats as keywords. No model is involved; it is not needed to surface this class of defect and would only make the result non-deterministic. Two rules keep it from going quiet. On a folding engine each fixture first asserts the bare form genuinely fails, so a fixture that would pass without the fix is reported as testing nothing. An engine that cannot be reached is skipped rather than passed, and CI names the engines it requires so the job cannot go green on the embedded one alone. Removing the normalisation makes three fixtures fail with the error users saw: relation "customers" does not exist. --- .github/workflows/ci.yml | 41 ++++ CONTRIBUTING.md | 20 ++ package.json | 1 + tools/schema-regression.mjs | 359 ++++++++++++++++++++++++++++++++++++ 4 files changed, 421 insertions(+) create mode 100644 tools/schema-regression.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bdf7df8..19249d8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -63,3 +63,44 @@ jobs: chrome_path="$(npx --yes @puppeteer/browsers@latest install chrome@151.0.7922.76 | awk '{ $1=""; sub(/^ /,""); print }')" node test/e2e-smoke.mjs "$chrome_path" working-directory: packages/browser-extension + + # Real engines, hostile schemas, no model. A mixed-case Postgres schema once failed every query + # and shipped that way, because every database test used tables we had written ourselves. + schema-regression: + runs-on: ubuntu-latest + services: + postgres: + image: postgres:16 + env: + POSTGRES_PASSWORD: pg + POSTGRES_DB: schema_regression + ports: + - 5433:5432 + options: >- + --health-cmd pg_isready --health-interval 5s --health-timeout 5s --health-retries 12 + mysql: + image: mysql:8 + env: + MYSQL_ALLOW_EMPTY_PASSWORD: 'yes' + MYSQL_DATABASE: schema_regression + ports: + - 3307:3306 + options: >- + --health-cmd "mysqladmin ping -h127.0.0.1" --health-interval 5s --health-timeout 5s --health-retries 20 + steps: + - uses: actions/checkout@v5 + - uses: pnpm/action-setup@v5 + - uses: actions/setup-node@v5 + with: + node-version: 22 + cache: pnpm + - run: pnpm install --frozen-lockfile + - run: pnpm build + # The harness exits non-zero if an engine is unreachable, so a silent skip cannot pass here. + - run: pnpm test:schemas -- --require=postgres,mysql,sqlite + env: + ASKSQL_PG_URL: postgres://postgres:pg@127.0.0.1:5433/schema_regression + ASKSQL_MYSQL_HOST: 127.0.0.1 + ASKSQL_MYSQL_PORT: '3307' + ASKSQL_MYSQL_USER: root + ASKSQL_MYSQL_PASSWORD: '' diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2ed254b..cb91c08 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -22,6 +22,7 @@ pnpm test # the full suite (see gating below) pnpm format:check # prettier, as CI runs it pnpm coverage # what CI runs instead of `test`; the coverage floor applies only with --coverage pnpm test:packaged # installs the packed tarballs outside the workspace and exercises every export +pnpm test:schemas # identifier handling against real engines and hostile schemas; needs Postgres and MySQL pnpm verify # all of the above in the order CI runs them; use this before pushing ``` @@ -71,6 +72,25 @@ To exercise them, provide: model. Per-provider model overrides use `ASKSQL__MODEL`. - **Browser E2E** - a Chrome install; the tests drive it via `puppeteer-core`. +### Schema regression + +`pnpm test:schemas` runs the engine's identifier handling against real databases using schemas +built to break the rules: mixed case, reserved words, spaces, unicode, names the SQL parser treats +as keywords. It needs no model, because this class of defect does not need one to surface and a +model would only make the result non-deterministic. + +It exists because a mixed-case Postgres schema once failed **every** query and shipped that way for +weeks. Every database test we owned used tables we had written ourselves, so they shared our blind +spots by construction. + +Two rules keep it honest, and both matter more than the pass count: + +- On an engine that folds unquoted names, each fixture first asserts the **bare form genuinely + fails**. A fixture that passes before the fix is testing nothing. +- An unreachable engine is reported as **skipped, never as a pass**. CI passes + `--require=postgres,mysql,sqlite` so the job cannot go green on the embedded engine alone when the + service containers fail to start. + The security boundary is developed **test-first**: add or extend a case in the `guard-security` / `guard-fuzz` suites before changing the guard. diff --git a/package.json b/package.json index 0b0b04b..5f7edfd 100644 --- a/package.json +++ b/package.json @@ -17,6 +17,7 @@ "test:watch": "vitest", "coverage": "vitest run --coverage", "test:packaged": "node tools/packaged-consumer-test.mjs", + "test:schemas": "node tools/schema-regression.mjs", "test:real-db:load": "node tools/real-db-load.mjs", "test:real-db": "node tools/real-db-e2e.mjs", "test:schema-sweep": "node tools/schema-sweep.mjs", diff --git a/tools/schema-regression.mjs b/tools/schema-regression.mjs new file mode 100644 index 0000000..cd671fa --- /dev/null +++ b/tools/schema-regression.mjs @@ -0,0 +1,359 @@ +/** + * Runs the engine's identifier handling against real databases and hostile schemas. + * + * A mixed-case Postgres schema once failed every query and shipped that way for weeks, because + * every database test we owned used tables we had written ourselves and so shared our blind spots. + * This uses schemas built to break the rules instead: mixed case, reserved words, spaces, unicode, + * names that are parser keywords. + * + * No model is involved. A model is not needed to catch this class of defect and would make the + * result non-deterministic; the harness writes the bare, unquoted SQL a model produces, puts it + * through the same normalise-then-guard path the engine uses, and executes it. + * + * Each fixture also asserts the naive form genuinely fails on a folding engine, so the suite cannot + * pass by doing nothing. + * + * Usage: + * node tools/schema-regression.mjs # every engine it can reach + * node tools/schema-regression.mjs --engine=postgres + * + * Connection details come from the environment, with local defaults: + * ASKSQL_PG_URL, ASKSQL_MYSQL_HOST/PORT/USER/PASSWORD + * An engine that cannot be reached is reported as skipped, never as a pass. + */ +import { mkdtempSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { DatabaseSync } from 'node:sqlite'; + +// needsQuoting and quoteCatalogIdentifiers are internal to the engine; this reads the built files +// rather than widening the package's public surface for a test tool. +const { guardSql } = await import('../packages/core/dist/guard.js'); +const { needsQuoting } = await import('../packages/core/dist/catalog.js'); +const { quoteCatalogIdentifiers } = await import('../packages/core/dist/identifier-case.js'); +const { reservedWordsFor } = await import('../packages/core/dist/sql-keywords.js'); + +const PG_URL = process.env['ASKSQL_PG_URL'] ?? 'postgres://postgres:pg@127.0.0.1:5433/schema_regression'; +const MYSQL = { + host: process.env['ASKSQL_MYSQL_HOST'] ?? '127.0.0.1', + port: Number(process.env['ASKSQL_MYSQL_PORT'] ?? 3307), + user: process.env['ASKSQL_MYSQL_USER'] ?? 'root', + password: process.env['ASKSQL_MYSQL_PASSWORD'] ?? '', +}; + +/** + * Each fixture is the same schema written the way a real database would hold it, per dialect. + * `probe` is the question a model would answer with bare names. + */ +const FIXTURES = [ + { + name: 'mixed case', + postgres: [ + `CREATE TABLE "Customers" ("CustomerId" INT, "FirstName" TEXT, "Country" TEXT)`, + `INSERT INTO "Customers" VALUES (1, 'Ada', 'UK')`, + ], + mysql: [ + 'CREATE TABLE `Customers` (`CustomerId` INT, `FirstName` VARCHAR(50), `Country` VARCHAR(50))', + "INSERT INTO `Customers` VALUES (1, 'Ada', 'UK')", + ], + sqlite: [ + `CREATE TABLE "Customers" ("CustomerId" INTEGER, "FirstName" TEXT, "Country" TEXT)`, + `INSERT INTO "Customers" VALUES (1, 'Ada', 'UK')`, + ], + }, + { + name: 'reserved words as names', + postgres: [`CREATE TABLE "order" ("select" INT, "group" TEXT)`, `INSERT INTO "order" VALUES (1, 'a')`], + mysql: ['CREATE TABLE `order` (`select` INT, `group` VARCHAR(20))', "INSERT INTO `order` VALUES (1, 'a')"], + sqlite: [`CREATE TABLE "order" ("select" INTEGER, "group" TEXT)`, `INSERT INTO "order" VALUES (1, 'a')`], + }, + { + name: 'a name the parser treats as a keyword', + postgres: [`CREATE TABLE "Nulls" ("Id" INT, "Val" TEXT)`, `INSERT INTO "Nulls" VALUES (1, NULL)`], + mysql: ['CREATE TABLE `Nulls` (`Id` INT, `Val` VARCHAR(20))', 'INSERT INTO `Nulls` VALUES (1, NULL)'], + sqlite: [`CREATE TABLE "Nulls" ("Id" INTEGER, "Val" TEXT)`, `INSERT INTO "Nulls" VALUES (1, NULL)`], + }, + { + name: 'spaces and symbols', + postgres: [ + `CREATE TABLE "Order Items" ("item id" INT, "unit-price" NUMERIC)`, + `INSERT INTO "Order Items" VALUES (1, 9.99)`, + ], + mysql: [ + 'CREATE TABLE `Order Items` (`item id` INT, `unit-price` DECIMAL(10,2))', + 'INSERT INTO `Order Items` VALUES (1, 9.99)', + ], + sqlite: [ + `CREATE TABLE "Order Items" ("item id" INTEGER, "unit-price" REAL)`, + `INSERT INTO "Order Items" VALUES (1, 9.99)`, + ], + }, + { + name: 'unicode', + postgres: [`CREATE TABLE "Ünïcødé" ("naïve_col" TEXT)`, `INSERT INTO "Ünïcødé" VALUES ('x')`], + mysql: [ + 'CREATE TABLE `Ünïcødé` (`naïve_col` VARCHAR(20)) DEFAULT CHARSET=utf8mb4', + "INSERT INTO `Ünïcødé` VALUES ('x')", + ], + sqlite: [`CREATE TABLE "Ünïcødé" ("naïve_col" TEXT)`, `INSERT INTO "Ünïcødé" VALUES ('x')`], + }, + { + name: 'a column named like its table', + postgres: [`CREATE TABLE "Country" ("Country" TEXT, "Customers" INT)`, `INSERT INTO "Country" VALUES ('UK', 3)`], + mysql: [ + 'CREATE TABLE `Country` (`Country` VARCHAR(20), `Customers` INT)', + "INSERT INTO `Country` VALUES ('UK', 3)", + ], + sqlite: [`CREATE TABLE "Country" ("Country" TEXT, "Customers" INTEGER)`, `INSERT INTO "Country" VALUES ('UK', 3)`], + }, +]; + +/** + * A name can only be written without quotes when it is a plain word that is not a keyword. A name + * with a space has no bare form at all, so asking for one would test an input no model produces. + */ +function canBeBare(name, engine) { + return /^[A-Za-z_]\w*$/.test(name) && !reservedWordsFor(engine).has(name.toLowerCase()); +} + +const quote = (name, quoteChar) => `${quoteChar}${name}${quoteChar === '[' ? ']' : quoteChar}`; + +/** + * The two shapes a model actually emits. `bare` is the one that broke: correct spelling, no quotes, + * which a folding engine then resolves to something else. `quoted` must survive untouched. + */ +function probes(table, engine, quoteChar) { + const name = (n) => (canBeBare(n, engine) ? n : quote(n, quoteChar)); + const out = [ + { + shape: 'quoted', + sql: `SELECT ${table.columns.map((c) => quote(c.name, quoteChar)).join(', ')} FROM ${quote(table.name, quoteChar)}`, + }, + ]; + const bareable = canBeBare(table.name, engine) && table.columns.every((c) => canBeBare(c.name, engine)); + if (bareable) { + out.unshift({ + shape: 'bare', + sql: `SELECT ${table.columns.map((c) => name(c.name)).join(', ')} FROM ${name(table.name)}`, + }); + } + return out; +} + +/** The engine's own path: quote what the database would not read back, then guard, then run. */ +function normalise(sql, catalog, engine, quoteChar) { + const all = catalog.tables.flatMap((t) => [t.name, ...t.columns.map((c) => c.name)]); + const spellings = new Map(); + for (const n of all) { + const set = spellings.get(n.toLowerCase()) ?? new Set(); + set.add(n); + spellings.set(n.toLowerCase(), set); + } + const quotable = all.filter((n) => needsQuoting(n, engine) && spellings.get(n.toLowerCase())?.size === 1); + const tables = catalog.tables.map((t) => t.name).filter((n) => quotable.includes(n)); + return quoteCatalogIdentifiers(sql, quotable, quoteChar, tables) ?? sql; +} + +const results = []; +const record = (engine, fixture, status, detail = '') => { + results.push({ engine, fixture, status, detail }); + const mark = status === 'pass' ? 'ok ' : status === 'skip' ? 'skip' : 'FAIL'; + console.log(` ${mark} ${fixture}${detail ? ` - ${detail}` : ''}`); +}; + +/** + * Runs every fixture against one engine. + * `run` executes a statement; `introspect` returns the catalog; both come from the real connector. + */ +async function checkEngine({ engine, quoteChar, connect, folds }) { + console.log(`\n${engine}`); + let session; + try { + session = await connect(); + } catch (err) { + record(engine, 'connection', 'skip', String(err).split('\n')[0].slice(0, 80)); + return; + } + try { + for (const fixture of FIXTURES) { + const ddl = fixture[engine === 'duckdb' ? 'sqlite' : engine]; + if (!ddl) continue; + try { + await session.reset(); + for (const statement of ddl) await session.run(statement); + } catch (err) { + record(engine, fixture.name, 'skip', `fixture rejected: ${String(err).split('\n')[0].slice(0, 60)}`); + continue; + } + + let catalog; + try { + catalog = await session.introspect(); + } catch (err) { + record(engine, fixture.name, 'fail', `introspection failed: ${String(err).split('\n')[0].slice(0, 70)}`); + continue; + } + const table = catalog.tables.at(-1); + if (!table) { + record(engine, fixture.name, 'fail', 'introspection returned no table'); + continue; + } + + for (const probe of probes(table, engine, quoteChar)) { + const label = `${fixture.name} [${probe.shape}]`; + + // A bare name on a folding engine must genuinely fail, or the fixture proves nothing. + if (folds && probe.shape === 'bare') { + try { + await session.run(probe.sql); + record(engine, label, 'fail', 'the bare form was accepted, so this fixture tests nothing'); + continue; + } catch { + // expected: this is the defect the normalisation exists to fix + } + } + + const fixed = normalise(probe.sql, catalog, engine, quoteChar); + const verdict = guardSql({ sql: fixed, dialect: session.dialect }); + if (!verdict.allowed) { + record(engine, label, 'fail', `guard rejected it: ${(verdict.reason ?? verdict.ruleId ?? '').slice(0, 60)}`); + continue; + } + try { + await session.run(verdict.sql); + record(engine, label, 'pass'); + } catch (err) { + record(engine, label, 'fail', String(err).split('\n')[0].slice(0, 90)); + } + } + } + } finally { + // Closing must not mask whatever the fixture was reporting. + await session.close().catch(() => {}); + } +} + +async function postgresSession() { + // pg is the connector's own dependency, not the repo root's. + const { default: pg } = await import('../packages/postgres/node_modules/pg/lib/index.js'); + const { POSTGRES_DIALECT } = await import('../packages/core/dist/dialects.js'); + const { PostgresConnector } = await import('@asksql/postgres'); + const client = new pg.Client(PG_URL); + await client.connect(); + return { + dialect: POSTGRES_DIALECT, + run: (sql) => client.query(sql), + reset: () => client.query('DROP SCHEMA IF EXISTS public CASCADE; CREATE SCHEMA public;'), + // A fresh connector each time, so the catalog is never a cached view of an older fixture. + introspect: async () => { + const connector = new PostgresConnector({ id: 'pg', name: 'pg', connectionString: PG_URL }); + try { + await connector.connect(); + return await connector.introspect(); + } finally { + await connector.close().catch(() => {}); + } + }, + close: () => client.end(), + }; +} + +async function mysqlSession() { + // mysql2 is the connector's own dependency, not the repo root's. + const mysql = (await import('../packages/mysql/node_modules/mysql2/promise.js')).default; + const { MYSQL_DIALECT } = await import('../packages/core/dist/dialects.js'); + const { MysqlConnector } = await import('@asksql/mysql'); + const admin = await mysql.createConnection(MYSQL); + await admin.query('CREATE DATABASE IF NOT EXISTS schema_regression'); + await admin.changeUser({ database: 'schema_regression' }); + return { + dialect: MYSQL_DIALECT, + run: (sql) => admin.query(sql), + reset: async () => { + const [rows] = await admin.query('SHOW TABLES'); + for (const row of rows) await admin.query(`DROP TABLE IF EXISTS \`${Object.values(row)[0]}\``); + }, + // A fresh connector each time, so the catalog is never a cached view of an older fixture. + introspect: async () => { + const connector = new MysqlConnector({ id: 'my', name: 'my', ...MYSQL, database: 'schema_regression' }); + try { + await connector.connect(); + return await connector.introspect(); + } finally { + await connector.close().catch(() => {}); + } + }, + close: () => admin.end(), + }; +} + +async function sqliteSession() { + const { SQLITE_DIALECT } = await import('../packages/core/dist/dialects.js'); + const { SqliteConnector } = await import('@asksql/sqlite'); + const file = join(mkdtempSync(join(tmpdir(), 'asksql-schema-')), 'x.db'); + let db = new DatabaseSync(file); + return { + dialect: SQLITE_DIALECT, + run: async (sql) => db.exec(sql), + reset: async () => { + for (const row of db.prepare("SELECT name FROM sqlite_master WHERE type='table'").all()) { + db.exec(`DROP TABLE IF EXISTS "${row.name}"`); + } + }, + introspect: async () => { + // The connector opens the file read-only, so the writable handle is closed for the duration + // and always reopened, even when introspection fails. + db.close(); + const connector = new SqliteConnector({ id: 'sq', name: 'sq', file }); + try { + await connector.connect(); + return await connector.introspect(); + } finally { + await connector.close().catch(() => {}); + db = new DatabaseSync(file); + } + }, + close: async () => db.close(), + }; +} + +const only = process.argv.find((a) => a.startsWith('--engine='))?.split('=')[1]; +/** + * Engines that must actually run. Without this, CI would go green on SQLite alone whenever the + * database services failed to start, which is the shape of a suite that quietly stops testing. + */ +const required = (process.argv.find((a) => a.startsWith('--require='))?.split('=')[1] ?? '').split(',').filter(Boolean); +const ENGINES = [ + { engine: 'postgres', quoteChar: '"', folds: true, connect: postgresSession }, + { engine: 'mysql', quoteChar: '`', folds: false, connect: mysqlSession }, + { engine: 'sqlite', quoteChar: '"', folds: false, connect: sqliteSession }, +]; + +for (const target of ENGINES) { + if (only && target.engine !== only) continue; + await checkEngine(target); +} + +const failed = results.filter((r) => r.status === 'fail'); +const skipped = results.filter((r) => r.status === 'skip'); +const passed = results.filter((r) => r.status === 'pass'); +console.log(`\n${passed.length} passed, ${failed.length} failed, ${skipped.length} skipped`); +if (skipped.length > 0) { + console.log('Skipped engines were unreachable; they are not a pass.'); +} +if (failed.length > 0) { + for (const f of failed) console.log(` FAILED ${f.engine} / ${f.fixture}: ${f.detail}`); + process.exit(1); +} + +const ranEngines = new Set(passed.map((r) => r.engine)); +const missing = required.filter((e) => !ranEngines.has(e)); +if (missing.length > 0) { + console.log(`Required engines never ran: ${missing.join(', ')}. A skip is not a pass.`); + process.exit(1); +} + +if (passed.length === 0) { + console.log('Nothing ran. Start a database, or pass --engine= for one you have.'); + process.exit(1); +}