Skip to content

fix(driver-sql): recognise knex's canonical postgres / sqlite client spellings in the dialect getters - #11783

Merged
huangyiirene merged 1 commit into
mainfrom
claude/issue-11550-dialect-getter-client-spellings
Aug 24, 2026
Merged

fix(driver-sql): recognise knex's canonical postgres / sqlite client spellings in the dialect getters#11783
huangyiirene merged 1 commit into
mainfrom
claude/issue-11550-dialect-getter-client-spellings

Conversation

@huangyiirene

Copy link
Copy Markdown
Collaborator

Part of #11550 — the narrow half of that card. The other half is a live
support-scope decision and is out of scope here: see #11756, which remains open.

What was wrong

SqlDriverConfig is Knex.Config & {…}, so the declared surface is every
client name knex accepts. The three identity getters enforced two literals
each:

protectedgetisPostgres(): boolean {constc=(this.configasany).client;returnc==='pg'||c==='postgresql';}

knex's own table (knex/lib/constants.js, knex 3.3.0 — the version this repo
pins) registers pg and postgresql as aliases of the canonical
postgres, and sqlite as an alias of the canonical sqlite3. So
client: 'postgres' — knex's primary name for the dialect — answered
isPostgres === false, and every Postgres branch in the driver silently
stopped applying on a configuration knex considers valid.

Re-measured on the pinned knex before fixing anything: pg, postgres and
postgresql each construct a client with dialect === 'postgresql' and
driverName === 'pg'. One dialect, three spellings.

Why it is not cosmetic — reproduced

The card's own example, reproduced on origin/main @ 2a6122bd9d with the new
suite in place (full output in the reverse-verification section):

AssertionError: expected 'CURRENT_TIMESTAMP' to contain 'timezone(\'utc\', now())::date'

That is nowColumnDefault('date') under client: 'postgres' falling through to
knex.fn.now() — a bare CURRENT_TIMESTAMP default on a DATE column, which
resolves the calendar day in the server's timezone. It is the exact defect
that method's Postgres branch exists to remove ("measured: a UTC-12 server
records YESTERDAY", per its own docblock). Nothing threw; the column was just
created wrong. nowColumnDefault('time') and dialectName (which feeds the
schema-drift differ, where 'unknown' disables every dialect-aware comparison)
degraded the same way.

The change

One identity set per dialect family, and every client-keyed table in the file
now reads from it instead of carrying its own literals:

sitebeforeafter
isPostgrespg, postgresql+ postgres
isSqlitesqlite3, better-sqlite3+ sqlite
isMysqlmysql, mysql2unchanged, now reads the shared set
DIALECT_CONNECT_TIMEOUT6 hand-written keysderived; membership byte-identical
POSTGRES_WIRE_CLIENTS5 hand-written literalsPOSTGRES_EMIT_CLIENTS + cockroachdb, redshift
withUtcSessioninline client !== 'mysql' && client !== 'mysql2'reads MYSQL_EMIT_CLIENTS

What an already-recognised spelling resolves to is unchangedpg,
postgresql, sqlite3, better-sqlite3, mysql, mysql2 all answer exactly
what they answered before, pinned explicitly because sibling cards this round
select behaviour through these getters.

The direction the tables derive in is load-bearing

DIALECT_CONNECT_TIMEOUT already contained cockroachdb, and
POSTGRES_WIRE_CLIENTS already contained redshift. Merging the tables into a
union would have granted both SQL-emission identity as a silent side effect
of a refactor — and that is precisely the open question in #11756. So the
emission sets stay narrow and explicit, and the other two tables extend
them; nothing derives an emission set from a union. Three test cases pin that
asymmetry, and rewriting them is how #11756 gets answered deliberately rather
than accidentally.

Two bounded in-place repairs, named rather than left unremarked

Both are the same defect class as the card (a dialect-identity literal that
disagrees with the others), mechanical, and pinned by knex's own table:

  1. withUtcSession carried a fourth hand-written copy of the MySQL pair. It now
    reads the shared set. Behaviour is unchanged in both directions and is pinned
    by driving the real pool.afterCreate with a recording connection.
  2. POSTGRES_WIRE_CLIENTS' docblock called postgres "knex's own alias". It is
    the canon, not an alias — the exact misreading this card fixes. Corrected.

Tests

New: packages/drivers/driver-sql/src/sql-driver-11550-dialect-client-spellings.test.ts
(17 cases). The sweep loads CLIENT_ALIASES / SUPPORTED_CLIENTS out of the
installed knex rather than copying them, and asserts an alias-closure
property: if the driver recognises any spelling of a dialect it must recognise
every spelling knex resolves to the same canon. A hand-copied list would keep
passing after a knex upgrade changed the answer.

The primary pin is the DDL nowColumnDefault emits, not getter === true — a
getter assertion still passes for a refactor that breaks the branch it feeds.
It runs against a driver constructed exactly as a host would (client: 'postgres', no stand-in). Spellings this workspace cannot instantiate at all
(sqlite3, sqlite, mysql, mariadb — their npm drivers are not installed,
so knex throws in its own constructor) are probed by re-spelling
config.client, and a case asserts that those constructions really do throw, so
the stand-in stays justified rather than merely convenient.

Reverse verification — two legs, both red, both restored

Each leg proved its mutation reached the disk before any result was read
(anchored grep on the removed and injected text), and carried a
trap … EXIT INT TERM restore. No rebuild step is involved: the suite imports
./sql-driver.js relatively, so it resolves to src/, never to dist/.

  1. Targeted — drop only 'postgres' back out of the emission set:
    ON-DISK PROOF: removed-text hits before=1 after=0 | injected-text hits after=1,
    then Tests 9 failed | 8 passed (17), primary pin included.
  2. Whole PRgit checkout origin/main -- sql-driver.ts (proof:
    POSTGRES_EMIT_CLIENTS hits in reverted file = 0): Tests 9 failed | 8 passed (17), this time additionally reddening client: 'sqlite'.

One case failed on first authoring and the test was wrong, not the code:
postgres legitimately gets a pool.afterCreate (the #11389 calendar-day wire
hook), so the presence of that function proves nothing about which pin applied.
The case now drives both hooks with a recording connection, which is the only
thing that tells them apart.

Gates and suites — all on ad0ef81bfd, quoting each gate's own verdict line

Derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack
(13 path-matched families + 6 convention-triggered by the added test file), all
run, all green. Selected verdict lines:

check-driver-conformance: OK — 45 covered cell(s), 0 in the DEBT ledger, 0 exempt.
check-engine-double-contract: OK — 401 pinned, 133 in the DEBT ledger, 2 exempt.
check-type-check-coverage --re-measure: OK — 32 ledger entr(ies) re-measured in 365.9s, 1898 raw tsc error(s) total, none above its recorded number.
check-nul-bytes: OK (scanned 6570 text file(s) ... no raw ASCII control bytes).
✓ where-matcher conformance holds: 295 matcher(s) discovered ...
✓ slot-lookup ratchet holds: 107 unswept site(s) in 25 file(s), none new ...

driver-conformance before and after are identical: 45 covered, 0 DEBT, 0
exempt.
check:type-check-debt ran against the fully built workspace closure
(turbo run build, 70/70 successful) as its header requires.

Suites:

  • @objectstack/driver-sqlTest Files 125 passed | 8 skipped (133), Tests 1963 passed | 109 skipped (2072); typecheck exit 0.
  • Downstream consumers (--filter '...@objectstack/driver-sql', the dependents direction — 48 projects in scope, 39 with a typecheck script, incl. driver-turso, driver-sqlite-wasm, objectql, rest, runtime, the examples): all green.
  • The two SqlDriver subclasses, run in full because they inherit the widened getters: driver-sqlite-wasm 395/395, driver-turso 1006/1006.

Repo-wide pnpm lint is CI's run; delivered here as a measured narrowing
eslint --no-inline-config --format json over the two changed TS files: 2 files
linted, 0 errors, 0 warnings. The population is read from eslint's own
resolution (--print-config resolves 6 rules for the changed file; an ignored
file makes it fail), the count from the JSON, and the invariance holds because
this repo's eslint.config.mjs never enables type-aware linting for any
file (no parserOptions.project, no projectService — confirmed from
--print-config, and recorded with a positive control in that config's own
header), so no rule reads cross-file type information and this diff cannot move
the verdict on a file it does not contain.

Deliberately not decided here

Whether redshift and cockroachdb — separate knex dialects that speak the pg
wire protocol — should be treated as Postgres for SQL emission is a
support-scope question that decides which databases the platform claims to emit
correct DDL for. Out of scope: #11756. They keep their wire recognition and
gain no emission identity. mariadb and pgnative are the same class of
question and are noted there.

Generated by Claude Code


Generated by Claude Code

…ent spellings in the dialect getters
`SqlDriverConfig` is `Knex.Config & {…}`, so every client name knex accepts is
declared valid, while `isPostgres` / `isSqlite` / `isMysql` each enforced two
literals. `postgres` is knex's canonical name for the dialect whose registered
aliases are `pg` and `postgresql` (`sqlite` likewise for `sqlite3`), so those
canonical spellings answered false and silently dropped every dialect branch.
Measured consequence: `nowColumnDefault` emitted a bare CURRENT_TIMESTAMP
default on a DATE column, resolving the calendar day in the server's timezone.
The getters, `DIALECT_CONNECT_TIMEOUT`, `POSTGRES_WIRE_CLIENTS` and
`withUtcSession` now read one identity set per dialect family instead of four
hand-written lists. Wire recognition and the timeout table EXTEND the emission
sets; nothing derives emission identity from their union, which keeps the open
redshift/cockroachdb support-scope question unanswered by this refactor.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VK8rFDtg8eREaxBGX99Csn
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/driver-sql, touching 14 documentable anchor(s).

6 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/data-modeling/drivers.mdx(via SqlDriver (symbol), connectionString (literal), connectionTimeoutMillis (literal))
  • content/docs/data-modeling/index.mdx(via SqlDriver (symbol))
  • content/docs/plugins/packages.mdx(via SqlDriver (symbol))
  • content/docs/protocol/kernel/index.mdx(via SqlDriver (symbol))
  • content/docs/protocol/kernel/lifecycle.mdx(via SqlDriver (symbol))
  • content/docs/protocol/objectql/query-syntax.mdx(via SqlDriver (symbol))

1 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v17.mdx(via SqlDriver (symbol))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • the SDK route bridge reached 45 of 222 client-bound route-ledger rows — the other 177 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 9 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 8b41bcd85160ac1c475d26b2968d74f84954bbfapackageMentionDocs.

Which tree this was computed on

This run read content/docs from 64dd1d785ca2a3926a6e2e42f5acca9b3cc7b129 — the merge of head ad0ef81bfdd5e6ffa426693fe58c21f076ba9739 into base 8b41bcd85160ac1c475d26b2968d74f84954bbfa, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 64dd1d785ca2a3926a6e2e42f5acca9b3cc7b129 && git checkout 64dd1d785ca2a3926a6e2e42f5acca9b3cc7b129
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 8b41bcd85160ac1c475d26b2968d74f84954bbfa ad0ef81bfdd5e6ffa426693fe58c21f076ba9739 && git checkout -B drift-repro 8b41bcd85160ac1c475d26b2968d74f84954bbfa && git merge --no-ff ad0ef81bfdd5e6ffa426693fe58c21f076ba9739
node scripts/docs-audit/affected-docs.mjs --json 8b41bcd85160ac1c475d26b2968d74f84954bbfa

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 8b41bcd85160ac1c475d26b2968d74f84954bbfa → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 24, 2026
@huangyiirene
huangyiirene marked this pull request as ready for review August 24, 2026 16:47
@huangyiirene
huangyiirene added this pull request to the merge queueAug 24, 2026
Merged via the queue into main with commit f9ffd01Aug 24, 2026
32 checks passed
@huangyiirene
huangyiirene deleted the claude/issue-11550-dialect-getter-client-spellings branch August 24, 2026 17:02
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@huangyiirene@claude