resolve the dialect in the repository factory instead of assuming sqlite (#1143)

createCurrentRepositoryContext() hardcoded `dialect: "sqlite"` while the
runtime already carried all three engines. That field is not decoration:
returning.ts reads it to decide whether it can ask for RETURNING, and
whether an upsert spells itself onConflictDoUpdate or onDuplicateKeyUpdate.
Reporting sqlite while connected to MySQL means the first upsert calls
onConflictDoUpdate on a mysql2 insert builder, which does not have it --
a TypeError, not a rejected query, as the note in returning.ts warned.

So MySQL never worked outside the tests, and Postgres worked only because
it also supports RETURNING and shares the conflict syntax.

Three things were supposed to catch this and none could. The repository
suite builds its own DatabaseContext in test-support.ts, verify-dialects.mjs
builds its own, and the CI matrix runs both against real Postgres and MySQL
containers -- all of them bypassing the one function the application calls.
Green on three engines, broken on two.

Resolve it from the environment, and test the factory itself rather than a
hand-built context: the default, each configured dialect, the write hook it
installs only for sqlite, and that an unsupported value throws rather than
falling back. Reverting the fix fails two of them.

Fixes Termix-SSH/Support#282
This commit is contained in:
ZacharyZcR
2026-07-31 16:42:57 +08:00
committed by GitHub
parent 2a66775fc3
commit 975b271a11
3 changed files with 92 additions and 1 deletions
+13
View File
@@ -189,6 +189,19 @@ Tested against PostgreSQL 16 and MySQL 8. **MariaDB is not a substitute for
MySQL when testing** — it accepts DDL that MySQL 8 rejects, which has hidden a MySQL when testing** — it accepts DDL that MySQL 8 rejects, which has hidden a
real defect here more than once. real defect here more than once.
### What neither of them covers
Both harnesses build a `DatabaseContext` of their own, so neither runs
`createCurrentRepositoryContext()` — the one the application actually uses.
That gap hid a hardcoded `dialect: "sqlite"` in it: every engine reported
itself as SQLite at runtime while all three test passes stayed green, which on
MySQL meant `upsert` reached for `onConflictDoUpdate` and died with a
TypeError on the first write.
Anything the factory decides from the dialect needs its own test against the
factory. Asserting it through a hand-built context proves nothing about what
runs in production.
## Known limits ## Known limits
- The desktop app always uses SQLite. It embeds its own backend and cannot ship - The desktop app always uses SQLite. It embeds its own backend and cannot ship
+13 -1
View File
@@ -47,9 +47,21 @@ import { UserRepository } from "./user-repository.js";
import { VaultProfileRepository } from "./vault-profile-repository.js"; import { VaultProfileRepository } from "./vault-profile-repository.js";
import { VaultTokenRepository } from "./vault-token-repository.js"; import { VaultTokenRepository } from "./vault-token-repository.js";
/**
* The context every repository runs against.
*
* The dialect has to be resolved, not assumed: it is what `returning.ts` reads
* to decide whether it can ask for RETURNING, and whether an upsert spells
* itself `onConflictDoUpdate` or `onDuplicateKeyUpdate`. Reporting "sqlite"
* while connected to MySQL makes the second of those a TypeError on the first
* write.
*
* Both cross-dialect harnesses build a DatabaseContext themselves, so neither
* exercises this function — see tests/database/repositories/factory-context.
*/
export function createCurrentRepositoryContext(): DatabaseContext { export function createCurrentRepositoryContext(): DatabaseContext {
return { return {
dialect: "sqlite", dialect: resolveDatabaseDialect(),
drizzle: getDb(), drizzle: getDb(),
}; };
} }
@@ -0,0 +1,66 @@
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import { DATABASE_DIALECT_ENV } from "../../../database/db/dialect.js";
// getDb() throws unless a database was initialized; the context's handle is
// not what this file is about.
vi.mock("../../../database/db/index.js", () => ({
getDb: () => ({}),
getSqlite: () => ({}),
DatabaseSaveTrigger: { forceSave: vi.fn() },
}));
const { createCurrentRepositoryContext, createCurrentRepositoryWriteHook } =
await import("../../../database/repositories/factory.js");
// Neither cross-dialect harness reaches this function: both
// tests/database/repositories/test-support.ts and scripts/verify-dialects.mjs
// construct a DatabaseContext of their own. That is why the production path
// could report "sqlite" while connected to MySQL with CI green on all three
// engines, and why this asserts on the real factory rather than a fixture.
describe("createCurrentRepositoryContext", () => {
const saved = process.env[DATABASE_DIALECT_ENV];
beforeEach(() => {
delete process.env[DATABASE_DIALECT_ENV];
});
afterEach(() => {
if (saved === undefined) delete process.env[DATABASE_DIALECT_ENV];
else process.env[DATABASE_DIALECT_ENV] = saved;
});
it("defaults to sqlite when nothing is configured", () => {
expect(createCurrentRepositoryContext().dialect).toBe("sqlite");
});
it("reports the configured dialect", () => {
for (const dialect of ["sqlite", "postgres", "mysql"]) {
process.env[DATABASE_DIALECT_ENV] = dialect;
expect(createCurrentRepositoryContext().dialect).toBe(dialect);
}
});
it("rejects an unsupported dialect rather than falling back to sqlite", () => {
process.env[DATABASE_DIALECT_ENV] = "oracle";
expect(() => createCurrentRepositoryContext()).toThrow(/oracle/);
});
});
describe("createCurrentRepositoryWriteHook", () => {
const saved = process.env[DATABASE_DIALECT_ENV];
afterEach(() => {
if (saved === undefined) delete process.env[DATABASE_DIALECT_ENV];
else process.env[DATABASE_DIALECT_ENV] = saved;
});
it("installs a persist hook only for sqlite", () => {
process.env[DATABASE_DIALECT_ENV] = "sqlite";
expect(createCurrentRepositoryWriteHook("test")).toBeTypeOf("function");
for (const dialect of ["postgres", "mysql"]) {
process.env[DATABASE_DIALECT_ENV] = dialect;
expect(createCurrentRepositoryWriteHook("test")).toBeUndefined();
}
});
});