diff --git a/.changeset/analytics-sqldialect-declared-vocabulary.md b/.changeset/analytics-sqldialect-declared-vocabulary.md new file mode 100644 index 00000000000..4911b281a36 --- /dev/null +++ b/.changeset/analytics-sqldialect-declared-vocabulary.md @@ -0,0 +1,63 @@ +--- +"@objectstack/service-analytics": minor +--- + +fix(analytics)!: `AnalyticsServiceConfig.sqlDialect` declares its three-name accept set, and a host that answers outside it is told once (#16206) + + + +**BREAKING** for a TypeScript host that declares its `sqlDialect` hook as returning +`string`: the hook's declared return is now the three canonical dialect names or +`undefined`, so such a composition stops compiling until the host's own annotation +says which names it can answer. Shipped as `minor` under the repo's launch-window +convention, in which breaking-ness is carried by this banner and the disposition +above rather than by the bump level. Runtime behaviour for every host is unchanged: +the same three names were the only ones that ever did anything. + +## What was wrong + +`AnalyticsServiceConfig.sqlDialect` — the hook a host answers to say which SQL +dialect backs an object — was typed as free `string`, while `normalizeSqlDialect` +has only ever recognised `sqlite`, `postgres` and `mysql`. Nothing said so, and +nothing told a host that answered otherwise. + +So a host that owns a SQLite datasource and answers the spelling its own stack uses +— knex's canonical `sqlite3`, or `better-sqlite3`, both of which `driver-sql` itself +lists in `SQLITE_EMIT_CLIENTS` — was read as `unknown`. And because `sqlDialectFor` +is tiered "cannot answer, do not block", **a wrong answer and no answer were the +same answer**: the host that tried hardest to help got the residue arm, silently. + +## What it does now + +- **The vocabulary is declared**, on the type and in the docblock, as + `AcceptedSqlDialect` — `sqlite` | `postgres` | `mysql` — so a host reading the + config learns the accept set without running anything. The type and the runtime + membership set are generated from one `const` tuple, so a future widening cannot + land in one and miss the other. +- **A non-empty answer outside the set is diagnosed**: one `warn` naming the object, + the answer and the accepted set. It is emitted **once per distinct unrecognised + spelling** — the failure's identity — so the line count is bounded by the host's + own hook and never grows with query volume. +- **`undefined` stays silent and legal.** The hook is optional and "cannot answer, + do not block" is a supported composition, not a misconfiguration. A pin holds both + halves, because a diagnostic that also shouted at hosts who wired nothing would be + a worse defect than the one being fixed. +- **The accept set is NOT widened.** Teaching this package `driver-sql`'s knex + aliases would be a second copy of that driver's table, and an unrecognised + spelling is sometimes deliberate (`mariadb`, #11756). The answer is still read as + `unknown`; only the silence changed. +- **The plugin bridge translates the driver's own residue.** `SqlDriver.dialectName` + carries a fourth name, `unknown`, meaning "I cannot say"; handed on verbatim it + would have presented a correctly-behaving driver as a host answering out of + contract. It now arrives as `undefined`, this hook's own spelling for the same + thing. The dialect the compilers end up with is unchanged either way. + +## Measured, and worth reading before relying on the residue arm + +Driven on sql.js through a host answering `sqlite3`, against the shared +`FILTER_TEXT_CASES` fixture, with a host answering `sqlite` as the control: **five of +the six case-EXACT cases come back with the wrong rows** — every case that +discriminates on ASCII case. `{ name: { $contains: 'acme' } }` answers `['1','2']` +where the table says `['2']`, and the negated form DROPS a row that belongs in the +result. That is #15684's fold, live on the arm this population lands on, and it is +reported rather than fixed here: closing it is that card's business, not this one's. diff --git a/packages/services/service-analytics/src/__tests__/sql-dialect-vocabulary.test.ts b/packages/services/service-analytics/src/__tests__/sql-dialect-vocabulary.test.ts new file mode 100644 index 00000000000..f2b0414f197 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/sql-dialect-vocabulary.test.ts @@ -0,0 +1,422 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#16206] `AnalyticsServiceConfig.sqlDialect` — a PUBLIC host hook that was + * typed as free `string` while only three spellings ever did anything. + * + * ## The defect + * + * `normalizeSqlDialect` accepts `'sqlite'` / `'postgres'` / `'mysql'` and reads + * everything else as `'unknown'`. That set is `driver-sql`'s `SqlDialectName` + * vocabulary and is the right one when the answer comes from + * `SqlDriver.dialectName` — but the reader is `sqlDialectFor`, and its channel + * is a HOST-supplied hook. A host that owns a SQLite datasource and answers the + * spelling its own stack uses — knex's canonical `'sqlite3'`, which + * `driver-sql` itself lists in `SQLITE_EMIT_CLIENTS` alongside + * `'better-sqlite3'` — landed on the residue arm. + * + * ⭐ And nothing told it. `sqlDialectFor` is tiered "cannot answer, do not + * block" by design, so **a wrong answer and no answer were the same answer**. + * These pins exist to break exactly that identity, and no more than that. + * + * ## What was ruled, and what is therefore pinned here + * + * Option A: the declared return vocabulary is the three canonical names or + * `undefined`, stated on the type and in the docblock; a NON-EMPTY answer + * outside them is diagnosed once; `undefined` stays silent and legal. ⛔ The + * accept set was NOT widened to knex's aliases (that was refused by name — a + * second copy of `driver-sql`'s table is the drift this repo keeps paying for, + * and #11756 shows an unrecognised spelling is sometimes deliberate), and ⛔ the + * diagnostic was not skipped (leaving the host uninformed is the silent- + * tolerance shape). + * + * ⇒ Both halves of the ruling's named pin are below, and the second is the + * CONTROL that keeps the first honest: a suite that only asserted "a warning + * appeared" would pass just as well for an implementation that shouts at every + * host who wired nothing, which is the failure mode the tiering exists to + * prevent. + * + * ## The `unknown` arm's rows, EXECUTED — not inherited + * + * The last block drives the case-EXACT family (`$contains` and its three + * siblings) through a host answering `'sqlite3'`, on sql.js, and prints the row + * ids it gets. The card carried that consequence as NOT MEASURED, read off + * #15684's own measurement of the same arm rather than re-driven for this + * population. It is measured here, with the canonical-spelling host as the + * discriminating control — the two hosts differ in ONE character of one string + * and in nothing else. + * + * ⚠️ Those assertions pin a WRONG row set on purpose. They are the finding, not + * the contract: #15684's fold is live on this arm, and the day it is fixed + * there this block must go red and be re-read, exactly as + * `text-operator-case-exactness.test.ts` intends for its own "the defect, still + * reachable" pin. ⛔ Do not "repair" them by loosening the expectation. + */ + +import { describe, it, expect, vi, beforeAll, afterAll } from 'vitest'; +import type { Cube } from '@objectstack/spec/data'; +import { FILTER_TEXT_CASES, FILTER_TEXT_ROWS } from '@objectstack/spec/data'; +import type { AnalyticsQuery } from '@objectstack/spec/contracts'; + +import { AnalyticsService, type AnalyticsServiceConfig } from '../analytics-service.js'; +import { + ACCEPTED_SQL_DIALECTS, + isUnrecognisedSqlDialectAnswer, + normalizeSqlDialect, + type AcceptedSqlDialect, +} from '../text-match-sql.js'; + +/** The four operators #4706 Q2 = A rules case-SENSITIVE. */ +const CASE_EXACT_OPS = new Set(['$contains', '$notContains', '$startsWith', '$endsWith']); + +/** + * The shared table's rows that aim a case-EXACT operator at the TEXT column — + * selected from `FILTER_TEXT_CASES` rather than restated, so this file drives + * the same family the five drivers answer. + */ +const NAME_CASE_EXACT = FILTER_TEXT_CASES.filter( + (c): c is Extract => { + if (c.expectRejection === true) return false; + const entries = Object.entries(c.filter as Record); + if (entries.length !== 1) return false; + const [field, predicate] = entries[0]; + if (field !== 'name' || typeof predicate !== 'object' || predicate === null) return false; + const op = Object.keys(predicate as Record)[0]; + return CASE_EXACT_OPS.has(op); + }, +); + +const CUBE: Cube = { + name: 'texts', + title: 'Texts', + sql: 'rows', + measures: { total: { name: 'total', label: 'Total', type: 'count', sql: '*' } }, + dimensions: { + id: { name: 'id', label: 'Id', type: 'string', sql: 'id' }, + name: { name: 'name', label: 'Name', type: 'string', sql: 'name' }, + }, + public: false, +} as unknown as Cube; + +/** + * A second cube over a DIFFERENT object, so "one answer, many objects" is + * drivable. ⚠️ The hook is asked about the OBJECT the cube reads (`sql`), not + * about the cube — which is why the line below names `rows`, and why this + * twin has to point somewhere else to be a second object at all. + */ +const OTHER_CUBE: Cube = { + ...(CUBE as unknown as Record), + name: 'other_texts', + sql: 'other_rows', +} as unknown as Cube; + +const query = (where: unknown, cube = 'texts'): AnalyticsQuery => + ({ cube, measures: ['total'], dimensions: ['id'], timezone: 'UTC', where }) as AnalyticsQuery; + +const makeLogger = () => ({ + info: vi.fn(), + debug: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + child: vi.fn().mockReturnThis(), +}); + +type TestLogger = ReturnType; + +/** + * A service composed the way a direct embedder composes one — the population + * this card is about. `answer` is deliberately typed `string | undefined`, not + * {@link AcceptedSqlDialect}: the declaration says what a host is ASKED for, and + * every interesting case here is a host answering something else. + */ +const serviceAnswering = ( + answer: string | undefined | (() => string | undefined), +): { service: AnalyticsService; logger: TestLogger } => { + const logger = makeLogger(); + const hook = typeof answer === 'function' ? answer : () => answer; + const config = { + logger: logger as unknown as AnalyticsServiceConfig['logger'], + cubes: [CUBE, OTHER_CUBE], + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: true, inMemory: false }), + // `NativeSQLStrategy.canHandle` requires a raw-SQL door to exist. Nothing + // below EXECUTES through it — the SQL is minted by `generateSql` and run on + // sql.js directly — so it only has to be present and typed. + executeRawSql: async () => [] as Record[], + sqlDialect: hook as unknown as AnalyticsServiceConfig['sqlDialect'], + } satisfies AnalyticsServiceConfig; + return { service: new AnalyticsService(config), logger }; +}; + +/** A service that wired NO hook at all — the legal, silent composition. */ +const serviceAnsweringNothing = (): { service: AnalyticsService; logger: TestLogger } => { + const logger = makeLogger(); + return { + service: new AnalyticsService({ + logger: logger as unknown as AnalyticsServiceConfig['logger'], + cubes: [CUBE, OTHER_CUBE], + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: true, inMemory: false }), + executeRawSql: async () => [] as Record[], + }), + logger, + }; +}; + +/** Only the dialect diagnostic — the constructor's own `info` is not it. */ +const dialectWarnings = (logger: TestLogger): string[] => + logger.warn.mock.calls.map((c) => String(c[0])).filter((m) => m.includes('sqlDialect hook answered')); + +describe('[#16206] the declared vocabulary', () => { + it('is the three canonical names, and the type and the runtime set are ONE source', () => { + expect([...ACCEPTED_SQL_DIALECTS]).toEqual(['sqlite', 'postgres', 'mysql']); + // Every declared name is accepted at runtime… + for (const d of ACCEPTED_SQL_DIALECTS) expect(normalizeSqlDialect(d), d).toBe(d); + // …and the type is the same list, checked by the compiler rather than by eye. + const declared: readonly AcceptedSqlDialect[] = ACCEPTED_SQL_DIALECTS; + expect(declared.length).toBe(3); + // ⛔ `'unknown'` is the residue arm, never something a host may answer. + expect((ACCEPTED_SQL_DIALECTS as readonly string[]).includes('unknown')).toBe(false); + expect(normalizeSqlDialect('unknown')).toBe('unknown'); + }); + + it('⛔ was NOT widened to driver-sql\'s knex aliases — option B, refused by name', () => { + // These are exactly the spellings `SqlDriver.SQLITE_EMIT_CLIENTS`, + // `POSTGRES_EMIT_CLIENTS` and `MYSQL_EMIT_CLIENTS` recognise and this file + // deliberately does not. A widening here is the refused option, not a fix. + for (const alias of ['sqlite3', 'better-sqlite3', 'pg', 'postgresql', 'pgnative', 'mysql2', 'mariadb']) { + expect(normalizeSqlDialect(alias), alias).toBe('unknown'); + } + }); + + it('separates a non-answer from a wrong answer — the identity the defect rested on', () => { + // A wrong answer: something was said, and it is outside the accept set. + for (const wrong of ['sqlite3', 'better-sqlite3', 'SQLite', 'mssql', ' sqlite']) { + expect(isUnrecognisedSqlDialectAnswer(wrong), wrong).toBe(true); + } + // ⛔ A non-answer is NOT a wrong answer. The hook is optional. + for (const silent of [undefined, null, '']) { + expect(isUnrecognisedSqlDialectAnswer(silent), String(silent)).toBe(false); + } + // …and neither is a name that IS accepted. + for (const ok of ACCEPTED_SQL_DIALECTS) expect(isUnrecognisedSqlDialectAnswer(ok), ok).toBe(false); + }); +}); + +describe('[#16206] the ruling\'s named pin — both halves', () => { + it('a host answering knex\'s `sqlite3` is read as `unknown` AND is told so, once', async () => { + const { service, logger } = serviceAnswering('sqlite3'); + const out = await service.generateSql(query({ name: { $contains: 'acme' } })); + + // Half one, the behaviour: still the residue arm. ⛔ The answer is NOT + // accepted — the diagnostic informs, it does not widen. + expect(out.sql).toContain('LIKE'); + expect(out.sql).not.toMatch(/GLOB/); + + // Half one, the diagnostic: exactly one line, naming all three things the + // ruling requires — the object, the answer, and the accepted set. + const warnings = dialectWarnings(logger); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('"sqlite3"'); + // The OBJECT the hook was asked about — `texts` reads the object `rows`. + expect(warnings[0]).toContain('"rows"'); + for (const accepted of ACCEPTED_SQL_DIALECTS) expect(warnings[0], accepted).toContain(accepted); + }); + + it('⛔ `undefined` says nothing — the optional hook stays optional', async () => { + // The control that keeps the pin above honest. An implementation that made + // "no answer" loud would pass the first test and fail this one. + const { service: wired, logger: wiredLog } = serviceAnswering(undefined); + await wired.generateSql(query({ name: { $contains: 'acme' } })); + expect(dialectWarnings(wiredLog)).toEqual([]); + + // …and so does a host that wired no hook at all. + const { service: bare, logger: bareLog } = serviceAnsweringNothing(); + await bare.generateSql(query({ name: { $contains: 'acme' } })); + expect(dialectWarnings(bareLog)).toEqual([]); + + // An empty string is a non-answer too: "non-empty" is the ruled trigger. + const { service: empty, logger: emptyLog } = serviceAnswering(''); + await empty.generateSql(query({ name: { $contains: 'acme' } })); + expect(dialectWarnings(emptyLog)).toEqual([]); + }); + + it('says nothing to a host that answers correctly', async () => { + for (const accepted of ACCEPTED_SQL_DIALECTS) { + const { service, logger } = serviceAnswering(accepted); + await service.generateSql(query({ name: { $contains: 'acme' } })); + expect(dialectWarnings(logger), accepted).toEqual([]); + } + }); +}); + +describe('[#16206] "once" is keyed on the failure\'s identity, and is bounded', () => { + it('one misspelling reaching many objects and many queries is ONE line', async () => { + const { service, logger } = serviceAnswering('sqlite3'); + for (let i = 0; i < 25; i++) { + await service.generateSql(query({ name: { $contains: 'acme' } }, 'texts')); + await service.generateSql(query({ name: { $startsWith: 'ACME' } }, 'other_texts')); + } + const warnings = dialectWarnings(logger); + expect(warnings).toHaveLength(1); + // The FIRST object to elicit it is the one named — a concrete place to look, + // not a count that grows with the object registry. Two DIFFERENT objects + // were asked about (`rows` and `other_rows`); one line came out. + expect(warnings[0]).toContain('"rows"'); + expect(warnings[0]).not.toContain('"other_rows"'); + }); + + it('a SECOND, DIFFERENT wrong answer is a second failure and gets its own line', async () => { + let answer = 'sqlite3'; + const { service, logger } = serviceAnswering(() => answer); + await service.generateSql(query({ name: { $contains: 'acme' } })); + answer = 'better-sqlite3'; + await service.generateSql(query({ name: { $contains: 'acme' } })); + answer = 'sqlite3'; + await service.generateSql(query({ name: { $contains: 'acme' } })); + + const warnings = dialectWarnings(logger); + expect(warnings).toHaveLength(2); + expect(warnings[0]).toContain('"sqlite3"'); + expect(warnings[1]).toContain('"better-sqlite3"'); + }); + + it('the line count does not move with TRAFFIC — 4x the queries, the same one line', async () => { + const drive = async (laps: number): Promise => { + const { service, logger } = serviceAnswering('sqlite3'); + for (let i = 0; i < laps; i++) { + await service.generateSql(query({ name: { $contains: 'acme' } }, 'texts')); + await service.generateSql(query({ name: { $notContains: 'acme' } }, 'other_texts')); + } + return dialectWarnings(logger).length; + }; + const oneX = await drive(50); + const fourX = await drive(200); + expect(oneX).toBe(1); + expect(fourX).toBe(oneX); + }); + + it('each service instance carries its OWN key set — no module-global residue', async () => { + // A process-wide key would make the second host's identical + // misconfiguration invisible, which is the shape #15166 was filed against. + const first = serviceAnswering('sqlite3'); + await first.service.generateSql(query({ name: { $contains: 'acme' } })); + const second = serviceAnswering('sqlite3'); + await second.service.generateSql(query({ name: { $contains: 'acme' } })); + expect(dialectWarnings(first.logger)).toHaveLength(1); + expect(dialectWarnings(second.logger)).toHaveLength(1); + }); +}); + +/** + * ⚠️ The measurement the ruling made a precondition of landing: the case-EXACT + * family, EXECUTED on SQLite, through a host answering `'sqlite3'`. + */ +describe('[#16206] the `unknown` arm\'s ROWS for a `sqlite3`-answering host, on a real SQLite engine', () => { + let db: any; + let sqlite3Host: AnalyticsService; + let sqliteHost: AnalyticsService; + + /** Point sql.js at the `.wasm` shipped inside its own package (Node-safe). */ + const locateWasm = async (): Promise<((file: string) => string) | undefined> => { + try { + const { createRequire } = await import('node:module'); + const require = createRequire(import.meta.url); + const pkgJsonPath = require.resolve('sql.js/package.json'); + const { dirname, join } = await import('node:path'); + return (file: string) => join(dirname(pkgJsonPath), 'dist', file); + } catch { + return undefined; + } + }; + + const run = (sql: string, params: unknown[]): string[] => { + const stmt = db.prepare(sql.replace(/\$\d+/g, '?')); + stmt.bind(params as any[]); + const rows: Record[] = []; + while (stmt.step()) rows.push(stmt.getAsObject()); + stmt.free(); + return rows.map((r) => String(r.id)).sort((a, b) => a.localeCompare(b)); + }; + + beforeAll(async () => { + const mod: any = await import('sql.js'); + const initSqlJs = mod.default ?? mod; + const locateFile = await locateWasm(); + const SQL = await initSqlJs(locateFile ? { locateFile } : undefined); + db = new SQL.Database(); + db.run(`CREATE TABLE "rows" ("id" TEXT PRIMARY KEY, "name" TEXT);`); + const insert = db.prepare(`INSERT INTO "rows" ("id","name") VALUES (?,?)`); + for (const r of FILTER_TEXT_ROWS) insert.run([r.id, r.name]); + insert.free(); + + // The two hosts differ in ONE character of ONE string. Everything else — + // cubes, capabilities, engine, fixture — is identical, which is what makes + // the row difference below attributable to the spelling. + sqlite3Host = serviceAnswering('sqlite3').service; + sqliteHost = serviceAnswering('sqlite').service; + }); + + afterAll(() => { + db?.close(); + }); + + const executedIds = async (where: unknown, service: AnalyticsService): Promise => { + const { sql, params } = await service.generateSql(query(where)); + return run(sql, params); + }; + + it('the rig discriminates: the canonical-spelling CONTROL answers the shared table exactly', async () => { + // Same query, same engine, same rows — the only host that is heard. + expect(run('SELECT "id" FROM "rows"', [])).toEqual(['1', '2', '3', '4', '5', '6', '7', '8', '9']); + expect(NAME_CASE_EXACT.length).toBeGreaterThan(0); + for (const c of NAME_CASE_EXACT) { + expect(await executedIds(c.filter, sqliteHost), `control · ${c.name}`).toEqual([...c.expected]); + } + }); + + it('⭐ the `sqlite3`-answering host gets WRONG ROWS — not merely slower ones', async () => { + // ⚠️ This is the finding, not the contract. #15684's ASCII fold is live on + // the `unknown` arm, and this host reaches that arm. + const wrong: { case: string; expected: string[]; measured: string[] }[] = []; + for (const c of NAME_CASE_EXACT) { + const measured = await executedIds(c.filter, sqlite3Host); + if (JSON.stringify(measured) !== JSON.stringify([...c.expected])) { + wrong.push({ case: c.name, expected: [...c.expected], measured }); + } + } + // FIVE of the shared table's SIX case-exact cases come back wrong — every + // one that discriminates on ASCII case. The sixth (`$contains 'a_b'`) is + // the LIKE-metacharacter row, which carries no cased letter to fold, and is + // the reason this is a count and not "all of them". + expect(wrong.map((w) => w.case)).toEqual([ + '$contains is case-SENSITIVE — a lower-case comparand misses the upper-case row', + '$contains is case-SENSITIVE — an upper-case comparand misses the lower-case row', + '$startsWith is case-SENSITIVE', + '$endsWith is case-SENSITIVE', + '$notContains is case-SENSITIVE, and negation does not widen it', + ]); + + // Named, so the record reads the rows rather than a count. + expect(await executedIds({ name: { $contains: 'acme' } }, sqlite3Host)).toEqual(['1', '2']); + expect(await executedIds({ name: { $contains: 'acme' } }, sqliteHost)).toEqual(['2']); + expect(await executedIds({ name: { $contains: 'ACME' } }, sqlite3Host)).toEqual(['1', '2']); + expect(await executedIds({ name: { $contains: 'ACME' } }, sqliteHost)).toEqual(['1']); + expect(await executedIds({ name: { $startsWith: 'ACME' } }, sqlite3Host)).toEqual(['1', '2']); + expect(await executedIds({ name: { $startsWith: 'ACME' } }, sqliteHost)).toEqual(['1']); + expect(await executedIds({ name: { $endsWith: 'corp' } }, sqlite3Host)).toEqual(['1', '2']); + expect(await executedIds({ name: { $endsWith: 'corp' } }, sqliteHost)).toEqual(['2']); + // ⭐ Negation turns the over-match into an UNDER-match: row 1 is DROPPED + // from a result set that should contain it. On a read scope that direction + // hides rows; on the query's own `where` it is a wrong chart. + expect(await executedIds({ name: { $notContains: 'acme' } }, sqlite3Host)) + .toEqual(['3', '4', '5', '6', '7', '8', '9']); + expect(await executedIds({ name: { $notContains: 'acme' } }, sqliteHost)) + .toEqual(['1', '3', '4', '5', '6', '7', '8', '9']); + + // The construct that causes it, so the finding names a mechanism: the + // residue arm's plain `LIKE`, which SQLite folds ASCII case on. + const viaSqlite3 = await sqlite3Host.generateSql(query({ name: { $contains: 'acme' } })); + const viaSqlite = await sqliteHost.generateSql(query({ name: { $contains: 'acme' } })); + expect(viaSqlite3.sql).toContain('LIKE'); + expect(viaSqlite.sql).toContain('GLOB'); + }); +}); diff --git a/packages/services/service-analytics/src/analytics-service.ts b/packages/services/service-analytics/src/analytics-service.ts index 4414634eefa..4f3350d9c27 100644 --- a/packages/services/service-analytics/src/analytics-service.ts +++ b/packages/services/service-analytics/src/analytics-service.ts @@ -76,6 +76,11 @@ import { evaluateAnalyticsQueryOverRows } from './preview-evaluator.js'; // member as the request spelled it (see `dataset-refusal.ts`'s header for why // that code and not `DATASET_INVALID`). import { invalidMemberError } from './dataset-refusal.js'; +// [#16206] The `sqlDialect` hook's DECLARED accept set, and the predicate that +// says whether a host answered outside it. Both live next to the membership set +// the compilers read, so the contract has one definition and this file states +// it rather than restating it. +import { ACCEPTED_SQL_DIALECTS, isUnrecognisedSqlDialectAnswer, type AcceptedSqlDialect } from './text-match-sql.js'; /** * Analytics result augmented with drill-through metadata (ADR-0021 D2; see @@ -691,6 +696,26 @@ export interface AnalyticsServiceConfig { * [#15684] The SQL dialect of the datasource backing `object` — `'sqlite'`, * `'postgres'`, `'mysql'`, or `undefined` when the host cannot answer. * + * [#16206] ⭐ Those three names are the WHOLE accepted vocabulary + * ({@link AcceptedSqlDialect}), declared here rather than left to be + * discovered: this used to be typed as free `string`, so a host that owned a + * SQLite datasource and answered the spelling its own stack uses — knex's + * `'sqlite3'`, or `'better-sqlite3'`, both of which `driver-sql` itself lists + * in `SQLITE_EMIT_CLIENTS` — was read as `'unknown'` and told nothing. ⛔ The + * knex aliases are deliberately NOT accepted: a second copy of that driver's + * table is the drift this repo keeps paying for, and an unrecognised spelling + * is sometimes deliberate (#11756's `'mariadb'`). + * + * ⇒ Two behaviours follow, and they are opposite on purpose: + * + * - **A non-empty answer outside the three is DIAGNOSED** — one `warn` naming + * the object, the answer and the accepted set, so a wrong answer stops + * reading as "no answer". It is emitted once per distinct unrecognised + * spelling; the dialect still resolves to `'unknown'`, so nothing about the + * query's behaviour changes. + * - **`undefined` stays silent and legal.** The hook is OPTIONAL; "cannot + * answer, do not block" is a supported composition, not a misconfiguration. + * * The three SQL compilers need it for ONE thing: the case-EXACT text family * (`$contains` / `$notContains` / `$startsWith` / `$endsWith`, #4706 Q2 = A) * has no construct that is case-exact and parses on every dialect. A plain @@ -704,7 +729,7 @@ export interface AnalyticsServiceConfig { * that wires nothing keeps the `LIKE` the compilers always emitted — * "cannot answer, do not block". */ - sqlDialect?: (object: string) => string | undefined; + sqlDialect?: (object: string) => AcceptedSqlDialect | undefined; /** Pre-defined datasets to compile + register at construction (ADR-0021). */ datasets?: Dataset[]; /** @@ -825,6 +850,18 @@ export class AnalyticsService implements IAnalyticsService { private readonly isExternalObject?: AnalyticsServiceConfig['isExternalObject']; /** [#3867] One-shot flag for the {@link assertInferableCube} stand-down warning. */ private warnedNoObjectRegistry = false; + /** + * [#16206] The out-of-contract `sqlDialect` answers this service has already + * diagnosed — the dedupe key for {@link diagnoseSqlDialectAnswer}. + * + * ⭐ Keyed on the ANSWER, which is the failure's identity: "your hook says a + * word this contract does not accept" is one defect with one fix, whether it + * reaches one object or every object. ⛔ Never keyed on anything that grows + * with traffic — the set's size is bounded by the number of DISTINCT strings + * the host's own hook can return, a property of the host's code fixed before + * any query runs, not by how many queries ask. + */ + private readonly diagnosedDialectAnswers = new Set(); /** * [#8286] Does the executed statement travel back to the caller? * See {@link AnalyticsServiceConfig.debugSql} for the switch and its default. @@ -903,7 +940,21 @@ export class AnalyticsService implements IAnalyticsService { // [#15684] The dialect that will run the compiled statement, so the // case-EXACT text family picks a construct that IS case-exact there. // Same tiering as the hook above: `undefined` keeps today's `LIKE`. - sqlDialect: (object: string) => config.sqlDialect?.(object), + // + // [#16206] This is also the seam where the host's answer ARRIVES, and so + // the only place that can tell a wrong answer from no answer — the + // compilers downstream see one `'unknown'` for both. The answer is passed + // through untouched either way: the diagnostic informs, it does not + // correct, and it does not accept a wider vocabulary. + sqlDialect: (object: string) => { + // ⚠️ Read back as `string`, not as the declared `AcceptedSqlDialect`. + // The declaration says what the host is ASKED for; this seam exists + // precisely because a host can answer something else at runtime — a + // plain JS embedder, or a TS one whose hook is typed `string`. + const answered: string | undefined = config.sqlDialect?.(object); + this.diagnoseSqlDialectAnswer(object, answered); + return answered; + }, }; // Build strategy chain (built-in + custom, sorted by priority) @@ -929,6 +980,56 @@ export class AnalyticsService implements IAnalyticsService { ); } + /** + * [#16206] Tell a host that ANSWERED the `sqlDialect` hook out of contract. + * + * ## The defect this closes + * + * `sqlDialectFor` is tiered "cannot answer, do not block", and that tiering + * had a cost nobody was paying attention to: a WRONG answer and NO answer + * were the same answer. A host that owned a SQLite datasource and answered + * `'sqlite3'` — knex's own canonical spelling, and one `driver-sql` lists in + * `SQLITE_EMIT_CLIENTS` — was read as `'unknown'`, silently, and the host that + * tried hardest to help got the residue arm with no way to find out. This is + * the one line that breaks that identity. + * + * ## `warn`, not `error` + * + * Functional degradation, by AGENTS.md's one question: after it, the system is + * VISIBLY smaller — a dialect-specific construct is not enabled — and nothing + * that claims to be persisted has failed to land. ⛔ Escalating it to `error` + * would train everyone to skim `error`. + * + * ## Why "once" is keyed on the ANSWER, and why that is bounded + * + * The key is the failure's identity — the out-of-contract spelling — which is + * exactly the ruling's own granularity ("a non-empty answer outside them is + * diagnosed once"). One misspelling reaching a thousand objects is ONE defect + * with ONE fix; the object is named in the line so the host can find the + * wiring, but it is not part of the key. + * + * ⛔ Nothing in the key grows with traffic. Its cardinality is the number of + * DISTINCT strings the host's own hook can return — a property of the host's + * code, fixed before the first query runs. Ten thousand queries over the same + * misconfiguration print one line; the shipped bridge in `plugin.ts` answers + * from `SqlDriver.dialectName`, whose return type IS the accept set, so it + * cannot reach this path at all. + */ + private diagnoseSqlDialectAnswer(object: string, answered: string | undefined): void { + // `undefined`, `null` and `''` are legal, silent non-answers — the optional + // hook stays optional. Only a host that said something is told anything. + if (!isUnrecognisedSqlDialectAnswer(answered)) return; + if (this.diagnosedDialectAnswers.has(answered)) return; + this.diagnosedDialectAnswers.add(answered); + this.logger.warn( + `[Analytics] The sqlDialect hook answered "${answered}" for object "${object}", which is not one of ` + + `the accepted dialect names (${ACCEPTED_SQL_DIALECTS.join(', ')}). The answer is read as "unknown", so the ` + + `text operators compile the dialect-blind construct instead of this dialect's — same rows a host that wired ` + + `no hook at all would get. Answer one of the accepted names, or undefined if this host cannot say. ` + + `Reported once per distinct unrecognised answer.`, + ); + } + /** * Build a per-call StrategyContext that binds the read-scope provider to the * current request's ExecutionContext (ADR-0021 D-C). The strategy then sees a diff --git a/packages/services/service-analytics/src/plugin.ts b/packages/services/service-analytics/src/plugin.ts index 4fdf453de3a..7287059bcbd 100644 --- a/packages/services/service-analytics/src/plugin.ts +++ b/packages/services/service-analytics/src/plugin.ts @@ -12,6 +12,9 @@ import type { AnalyticsDriverCapabilities } from './strategies/types.js'; import { pickDisplayField, type DimensionLabelDeps } from './dimension-labels.js'; import { assertReadScopeCannotVacate } from './read-scope-sql.js'; import { readScopeUnresolvedError } from './read-scope-refusal.js'; +// [#16206] The narrowing from a driver's FOUR-name `dialectName` to the THREE +// this package's config hook declares — see the bridge below. +import { asAcceptedSqlDialect, type AcceptedSqlDialect } from './text-match-sql.js'; /** * The slice of the DECLARED engine contracts this plugin's auto-bridges @@ -998,13 +1001,24 @@ export class AnalyticsServicePlugin implements Plugin { * `undefined` on every tier that cannot answer — no data engine, a driver * that names no dialect (memory, mongo), a throw — and `undefined` keeps * the plain `LIKE`, which is exactly the pre-#15684 behaviour. + * + * [#16206] ⭐ A FIFTH tier that cannot answer, and the reason this is not a + * verbatim pass-through: `SqlDriver.dialectName` is a FOUR-name vocabulary + * whose fourth name is `'unknown'` — that driver's own "I cannot say", which + * is what it returns for a client it does not model (`'mariadb'`, left + * unrecognised on purpose by #11756). The config hook's accept set is the + * other THREE, so handing `'unknown'` on verbatim would present a driver + * behaving correctly as a host answering out of contract, and every such + * deployment would carry a warning about itself. ⇒ The residue is + * translated to this hook's own spelling for the same thing, `undefined`. + * The dialect the compilers end up with is unchanged either way. */ - const sqlDialect = (objectName: string): string | undefined => { + const sqlDialect = (objectName: string): AcceptedSqlDialect | undefined => { try { const svc = ctx.getService('data'); const driver = svc?.getDriverForObject?.(objectName) as DialectNamingDriver | undefined; const named = driver?.dialectName; - return typeof named === 'string' ? named : undefined; + return asAcceptedSqlDialect(typeof named === 'string' ? named : undefined); } catch { // Same tiering as the temporal hooks: an unresolvable driver keeps the // dialect-blind construct, which is today's behaviour. diff --git a/packages/services/service-analytics/src/text-match-sql.ts b/packages/services/service-analytics/src/text-match-sql.ts index 054316a8865..4c7bcafa969 100644 --- a/packages/services/service-analytics/src/text-match-sql.ts +++ b/packages/services/service-analytics/src/text-match-sql.ts @@ -184,16 +184,41 @@ import { import type { StrategyContext } from '@objectstack/spec/contracts'; import type { DatasetScopedStrategyContext } from './strategies/types.js'; +/** + * [#16206] The dialect names a HOST may answer — the declared accept set of + * `AnalyticsServiceConfig.sqlDialect`, and the same three a `SqlDriver` answers + * from its own `dialectName`. + * + * ⛔ `'unknown'` is deliberately NOT in here. It is this file's RESIDUE arm — + * what a non-answer normalises to — never a name a host is asked to say, and + * putting it in the accept set would make "I don't know" indistinguishable from + * a considered answer at the one seam that exists to tell them apart. + * + * Spelled as a `const` tuple so {@link AcceptedSqlDialect} (what the compiler + * checks a host against) and {@link KNOWN_DIALECTS} (what the runtime checks it + * against) are ONE source: a widening cannot land in the type and miss the set, + * which is how a declared vocabulary drifts back into free text. + */ +export const ACCEPTED_SQL_DIALECTS = ['sqlite', 'postgres', 'mysql'] as const; + +/** + * [#16206] The vocabulary `AnalyticsServiceConfig.sqlDialect` accepts, stated + * on the type so a host reading the config learns the accept set without + * running anything — the ruled remedy for a hook that was declared as free + * text while only three spellings ever did anything. + */ +export type AcceptedSqlDialect = (typeof ACCEPTED_SQL_DIALECTS)[number]; + /** * The dialects this package's compilers distinguish — deliberately the same * four names `driver-sql`'s `SqlDialectName` carries, including `'unknown'`, * so a driver's own answer can be handed straight through with no second * mapping table to drift. */ -export type AnalyticsSqlDialect = 'sqlite' | 'postgres' | 'mysql' | 'unknown'; +export type AnalyticsSqlDialect = AcceptedSqlDialect | 'unknown'; /** Every dialect this file has an arm for; anything else is `'unknown'`. */ -const KNOWN_DIALECTS = new Set(['sqlite', 'postgres', 'mysql']); +const KNOWN_DIALECTS = new Set(ACCEPTED_SQL_DIALECTS); /** * Read a host's / driver's dialect answer as one of {@link AnalyticsSqlDialect}. @@ -201,11 +226,54 @@ const KNOWN_DIALECTS = new Set(['sqlite', 'postgres', 'mysql']); * Anything unrecognised — including `undefined` from a host that wired no hook * — is `'unknown'`, which compiles the pre-#15684 `LIKE`. "Cannot answer, do * not block": a name this file does not model must not silently pick an arm. + * + * [#16206] ⛔ The accept set stays these three: widening it to `driver-sql`'s + * knex-client aliases was refused by name (a second copy of that table is the + * drift this repo keeps paying for, and #11756 shows an unrecognised spelling is + * sometimes deliberate). What changed instead is that a WRONG answer no longer + * reads as NO answer — see {@link isUnrecognisedSqlDialectAnswer}. */ export function normalizeSqlDialect(name: string | undefined | null): AnalyticsSqlDialect { return typeof name === 'string' && KNOWN_DIALECTS.has(name) ? (name as AnalyticsSqlDialect) : 'unknown'; } +/** + * [#16206] Did the host answer something, and is that something outside + * {@link ACCEPTED_SQL_DIALECTS}? + * + * The predicate behind the one diagnostic the hook owes its host. It lives HERE, + * beside the set it asks about, so "what counts as an out-of-contract answer" + * has a single definition rather than a second one re-derived at the logging + * site — the same reason {@link KNOWN_DIALECTS} is not spelled twice. + * + * ⛔ It is deliberately FALSE for `undefined`, `null` and `''`. The hook is + * OPTIONAL and "cannot answer" is a legal, silent answer that + * {@link sqlDialectFor} is tiered on; making a NON-answer loud would punish the + * hosts the tiering exists for. The diagnostic's whole subject is the host that + * DID answer and was not heard. + */ +export function isUnrecognisedSqlDialectAnswer(name: string | undefined | null): name is string { + return typeof name === 'string' && name.length > 0 && !KNOWN_DIALECTS.has(name); +} + +/** + * [#16206] `name` if it is one of {@link ACCEPTED_SQL_DIALECTS}, else + * `undefined` — the narrowing a caller needs when it must hand an answer on to + * something that declares the accept set. + * + * ⚠️ It exists for the bridge in `plugin.ts`, which answers from a `SqlDriver`'s + * `dialectName` — a FOUR-name vocabulary whose fourth name is `'unknown'`, that + * driver's own "I cannot say". Passed through verbatim, that residue would + * arrive at the config hook looking like a considered answer outside the accept + * set, and the host would be warned about a driver doing exactly the right + * thing (and about `driver-sql`'s deliberately unrecognised spellings, #11756). + * ⇒ Translate the residue to the hook's own spelling for "cannot answer" — + * `undefined` — rather than teaching the diagnostic a list of exceptions. + */ +export function asAcceptedSqlDialect(name: string | undefined | null): AcceptedSqlDialect | undefined { + return typeof name === 'string' && KNOWN_DIALECTS.has(name) ? (name as AcceptedSqlDialect) : undefined; +} + /** * The dialect of the datasource backing `objectName`, read off the context's * `sqlDialect` hook — `'unknown'` when the host wired none. @@ -214,6 +282,14 @@ export function normalizeSqlDialect(name: string | undefined | null): AnalyticsS * the compilers cannot see this from the filter, the host can answer it from * the driver that will execute the statement, and a host that cannot answer * keeps the behaviour it had. + * + * [#16206] ⚠️ This function still answers `'unknown'` for a wrong answer and for + * no answer alike, and that is correct HERE: by the time a compiler asks, the + * only honest reading of an unmodelled name is "no arm for this". Telling the + * two apart is a job for the seam that OWNS the contract — `AnalyticsService`, + * where the host's config hook arrives and where the one `warn` is emitted + * ({@link isUnrecognisedSqlDialectAnswer}) — not for a per-predicate call the + * compilers make once per filter node. */ export function sqlDialectFor(ctx: StrategyContext, objectName: string): AnalyticsSqlDialect { const hook = (ctx as DatasetScopedStrategyContext).sqlDialect;