diff --git a/.changeset/analytics-missing-column-hard-failure.md b/.changeset/analytics-missing-column-hard-failure.md new file mode 100644 index 0000000000..babdab2066 --- /dev/null +++ b/.changeset/analytics-missing-column-hard-failure.md @@ -0,0 +1,41 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(service-analytics): postgres 的「缺列」措辞不再被判为「缺源」(#6035) + +数据集查询的降级路径靠驱动措辞判断「后端表没挂载」,从而把控件渲染成空网格而不是 500。 +它的判据 `isMissingSourceError` 自己的文档写明范围**只含缺表/缺对象,不含列/语法错误—— +后者要保持硬失败,好让真正的查询 bug 浮上来**。有一条 postgres 措辞按构造违反了这条承诺: + +``` +column "label" of relation "acct" does not exist (SQLSTATE 42703) +``` + +它内部**逐字包含**一整段合法的缺表措辞 `relation "acct" does not exist`。#5717 把 postgres +那一支从「同时含两个词的任意句子」收紧为锚定真实缺表措辞后,这条依然命中——它必然命中,因为它 +字面上**就是**那段措辞。所以任何对「这句话是不是在说某个 relation 不存在」的收紧都排除不掉它, +只有**先问更具体的问题**才可以:修法是一个**判定顺序**(先摘掉缺列措辞,再做缺源判定),而不是 +一个更好的正则。 + +两种后果都是错的,而具体触发哪一种只取决于措辞里那个关系名是否恰好是数据集自己的对象: + +- 名字是**被 JOIN 的表** → 报出一条响亮但**虚假**的跨数据源拓扑错误,把一个拼写错误说成数据源 + 布局问题; +- 名字是**数据集自己的对象** → 控件降级成空网格,只留一条 warn,拼错的列名不会告诉任何人。 + +两半现在都作为回归钉住。判定顺序抄 `rest-server.ts` 的 `mapDataError` 自 #5352 起就在用的先例 +(它同样先摘出这条措辞,于是 REST 面回答 `400 INVALID_FIELD` 而不是 `404`),用的是同一条正则 +而不是它的第二种方言——两个面不该对「postgres 什么时候在说 column」给出不同答案。兄弟函数 +`missingSourceRelation` 做同样的前置摘除:实测在修改前它对这条措辞回答 `sys_team`,只修其一会让 +「是不是缺了什么」与「缺的是什么」相互矛盾,而那正是 #5717 在这一支上刚消除的分歧。 + +**这不修线上事故,而是让判据与它自己的文档一致。** analytics 是只读面,而 postgres 在 SELECT +下的未知列措辞是 `column "bogus" does not exist`(不含 `relation`,本来就不命中); +`column … of relation …` 是 INSERT/UPDATE/ALTER 措辞。价值在于:这条分歧不再依赖「读路径不产生该 +措辞」这个假设活着——哪天有任何写形状语句、驱动改措辞、或多包一层 `cause` 把它送到这个 catch +面前,它会被正确分类,而不是被静默吞掉。 + +#5717 量过的 13 条仓内真实措辞全部重新钉住,并且是**按调用方可观测的结果**(空网格 / 拓扑拒收 / +原样上抛)钉的,而不是按私有判据的布尔值——实测 **13 条里只有 1 条改判**,就是缺列那条,其余 12 +条(三个驱动家族的措辞、框架的 not-registered 信号、本包自己的拒收)逐条不变。 diff --git a/packages/services/service-analytics/src/__tests__/missing-column-phrase-hard-failure.test.ts b/packages/services/service-analytics/src/__tests__/missing-column-phrase-hard-failure.test.ts new file mode 100644 index 0000000000..c3e9431ae6 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/missing-column-phrase-hard-failure.test.ts @@ -0,0 +1,270 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#6035] Postgres's missing-COLUMN wording stays a hard failure, because a + * mistyped column name is a query bug and not an absent table. + * + * ## What was wrong + * + * `isMissingSourceError`'s own docblock promises the degradation path is + * "Deliberately scoped to MISSING SOURCE (table/object/relation) — not + * column/syntax errors, which stay hard failures so real query bugs still + * surface". One driver wording broke that promise by construction: + * + * `column "label" of relation "acct" does not exist` (SQLSTATE 42703) + * + * carries `relation "acct" does not exist` inside it VERBATIM. #5717 anchored + * the postgres limb to postgres's real missing-table wording, and this string + * matched anyway — it had to, because it literally contains that wording. No + * tightening of "does this say a relation is missing" can exclude it; only + * asking the more specific question FIRST can, which is why the fix is an + * ORDERING (subtract the column phrase, then classify) and not a better regex. + * + * Both consequences were wrong, and which one fired depended only on whether + * the named relation happened to be the dataset's own object: + * + * - a JOINED table's name → a loud but FALSE cross-datasource topology + * error, blaming the datasource layout for a typo; + * - the dataset's OWN name → the widget degrades to an empty grid, one + * `warn`, and the mistyped column is never mentioned to anyone. + * + * Both are pinned below, because "which half fires" is an accident of the + * fixture and a fix that only closed one would look green from either side. + * + * ## Why this was dormant, stated as a limit rather than a boast + * + * Analytics is a READ face, and postgres does not use this wording on reads: a + * SELECT naming an unknown column says `column "bogus" does not exist`, with no + * `relation` in it (row 06 of the corpus below — a miss before and after). + * `column … of relation …` is INSERT/UPDATE/ALTER phrasing. So this repairs a + * predicate that disagreed with its own documentation, and does NOT repair a + * production incident. The value is that the disagreement cannot outlive the + * assumption that keeps it harmless — the day any write-shaped statement, any + * driver re-wording, or any newly wrapped `cause` puts that phrase in front of + * this catch, it is classified correctly instead of silently. + * + * ## The corpus, and why it is asserted through the public API + * + * PR #6045 (#5717) measured 13 real in-repo wordings and moved exactly one + * verdict. This file re-pins all 13 — but as `queryDataset` OUTCOMES rather + * than as booleans out of a private predicate, so what is guarded is the + * behaviour a caller can actually observe (empty grid / topology refusal / + * propagated verbatim), and so the pair `isMissingSourceError` + + * `missingSourceRelation` is exercised JOINTLY. Each wording is thrown BARE, on + * purpose: several of these producers carry an ADR-0112 envelope in real life + * and would propagate via #5717's defence B no matter what the sniffer said, + * which would make them vacuous as pins of the sniffer (#5046's lesson — a case + * that passes because nothing is produced). Stripping the envelope is the same + * isolation `dataset-degradation-envelope.test.ts` uses for its defence-C case. + * + * ## Reverse verification — direction predicted BEFORE running + * + * Ordinary direction (red), and deliberately NARROW: this change subtracts one + * phrase, so restoring the deleted guard must turn red exactly the cases about + * that phrase and nothing else. Predicted, with the guard removed from both + * functions: + * + * - `column … of relation "sys_team" …` (a joined name) → RED, becomes the + * false topology refusal; + * - `column … of relation "opportunity" …` (own object) → RED, becomes an + * empty grid; + * - the 13-row corpus table → RED on row 05 ONLY; + * - the other 12 rows, and every case in + * `dataset-degradation-envelope.test.ts` → GREEN throughout. + * + * MEASURED, both guards disabled: **3 red / 15 green** in this file — the three + * predicted cases and no others, row 05 the only corpus row to move, and + * `dataset-degradation-envelope.test.ts` green at 11/11 in both states, which is + * what makes "narrowing, not a behaviour change" a measurement rather than a + * claim. (The green count is corrected from the 12 first written here — 18 + * cases, not 15; the RED SET was predicted exactly and is the part that carries + * the argument, so the miscount is fixed in place rather than quietly.) + * + * The two reverted failures are worth quoting, because they are the defect + * itself rather than a red mark: the joined-name case came back as + * `[Analytics] dataset "sales" cannot be executed as one statement: table + * "sys_team" is not on the default datasource …` — a datasource-topology story + * told about a typo — and the own-object case failed as `expected undefined to + * be an instance of Error`, i.e. nothing was thrown at all and the widget got + * its empty grid. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { DatasetSchema } from '@objectstack/spec/ui'; +import type { ExecutionContext } from '@objectstack/spec/kernel'; +import { AnalyticsService } from '../analytics-service.js'; + +const EMPTY = { rows: [], fields: [], totals: [] }; + +const dataset = DatasetSchema.parse({ + name: 'sales', + label: 'Sales', + object: 'opportunity', + include: ['account'], + dimensions: [{ name: 'region', field: 'account.region', type: 'string' }], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], +}); + +const SELECTION = { dimensions: ['region'], measures: ['revenue'] }; +const CTX = { tenantId: 'org_A' } as ExecutionContext; + +function logger() { + return { info: vi.fn(), debug: vi.fn(), warn: vi.fn(), error: vi.fn(), child: vi.fn() } as any; +} + +/** A service whose execution throws `thrown` — the only way into the catch under test. */ +function serviceThatThrows(thrown: unknown, log = logger()) { + return new AnalyticsService({ + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + executeRawSql: async () => { throw thrown; }, + isRegisteredObject: () => true, + logger: log, + }); +} + +/** + * The three outcomes `queryDataset`'s catch can produce, named so a corpus row + * reads as a behaviour rather than as an internal boolean. + */ +type Outcome = 'empty' | 'topology' | 'propagated'; + +async function outcomeOf(wording: string, log = logger()): Promise { + try { + const result = await serviceThatThrows(new Error(wording), log).queryDataset(dataset, SELECTION, CTX); + expect(result).toEqual(EMPTY); + return 'empty'; + } catch (e) { + const message = String((e as Error)?.message); + // The #5033 cross-datasource refusal is this layer's OWN wording; anything + // else reaching the caller is the driver's error, untouched. + return /cannot be executed as one statement/.test(message) ? 'topology' : 'propagated'; + } +} + +// ── the corpus PR #6045 measured, transcribed from its real producers ───────── + +/** Postgres's missing-COLUMN wording, matching `rest.test.ts`'s pinned case. */ +const MISSING_COLUMN_JOINED = 'column "label" of relation "sys_team" does not exist'; + +const CORPUS: Array<[row: string, wording: string, outcome: Outcome]> = [ + ['01 sqlite/libsql bare', 'no such table: opportunity', 'empty'], + ['02 sqlite via knex (sql-prefixed)', 'SELECT COUNT(*) FROM "opportunity" - no such table: opportunity', 'empty'], + ['03 postgres missing table', 'select "region" from "opportunity" - relation "opportunity" does not exist', 'empty'], + ['04 postgres schema-qualified', 'relation "public.crm_account" does not exist', 'topology'], + // The one verdict this change moves. Before: `topology` — a false + // cross-datasource refusal for a mistyped column. + ['05 postgres MISSING COLUMN (write path)', MISSING_COLUMN_JOINED, 'propagated'], + ['06 postgres missing column (read path)', 'column "bogus_dim" does not exist', 'propagated'], + ['07 mysql', "Table 'app.opportunity' doesn't exist", 'empty'], + [ + '08 objectql datasource', + "[ObjectQL] Datasource 'warehouse' configured for object 'crm_account' is not registered.", + 'topology', + ], + ['09 rest unknown object', "Object 'ghost' is not registered", 'topology'], + [ + '10 analytics CUBE_NOT_FOUND (#3867)', + "Cube 'ghost' not found: no cube is registered under that name, and it is not a " + + 'registered object either (a cube can only be auto-inferred from a registered object). ' + + "Define a Cube in your stack, or check the object name.", + 'empty', + ], + [ + '11 dataset-compiler relationship refusal', + '[dataset-compiler] dataset "sales" includes relationship "bogus" which does not exist on object "opportunity".', + 'propagated', + ], + [ + '12 read-scope nested/relation value', + '[read-scope-sql] "owner" has a nested/relation value which is not supported in a read scope (fail-closed).', + 'propagated', + ], + [ + '13 analytics measure gate (#4437)', + "Measure 'ghost_sum' on cube 'opportunity' aggregates field 'ghost', which object " + + "'opportunity' does not have. Valid measures: (none).", + 'propagated', + ], +]; + +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#6035] postgres’s missing-COLUMN wording is a hard failure, not a missing source', () => { + it('the wording is the one this repo already pins on the REST face', () => { + // Guards the fixture against drifting away from the phrase the precedent + // handles: `rest-server.ts`'s `mapDataError` extracts this same string to + // answer `400 INVALID_FIELD` instead of a `404`, pinned in `rest.test.ts`. + // If these two stop being the same sentence, the two faces have started + // disagreeing about what postgres says — which is what this fix removed. + expect(MISSING_COLUMN_JOINED).toMatch( + /column\s+["'`]([a-z0-9_]+)["'`]\s+of relation\s+\S+\s+does not exist/i, + ); + // …and it does contain a well-formed missing-TABLE wording, which is the + // whole reason a subtraction is needed rather than a tighter anchor. + expect(MISSING_COLUMN_JOINED).toMatch(/relation\s+["'`]?[A-Za-z0-9_$.]+["'`]?\s+does not exist/i); + }); + + it('naming a JOINED table: propagates verbatim instead of a false topology refusal', async () => { + const log = logger(); + let caught: Error | undefined; + try { + await serviceThatThrows(new Error(MISSING_COLUMN_JOINED), log).queryDataset(dataset, SELECTION, CTX); + } catch (e) { + caught = e as Error; + } + expect(caught, 'a mistyped column was swallowed by the degradation path').toBeInstanceOf(Error); + // The driver's own sentence, unedited — the caller can read the column name. + expect(String(caught?.message)).toBe(MISSING_COLUMN_JOINED); + // Specifically NOT the #5033 cross-datasource story, which blames the + // datasource layout for what is a spelling mistake. + expect(String(caught?.message)).not.toMatch(/cannot be executed as one statement/); + expect(String(caught?.message)).not.toMatch(/A dataset JOIN cannot cross datasources/); + // Bare in ⇒ bare out; this layer must not invent an envelope for it. + expect((caught as { code?: unknown })?.code).toBeUndefined(); + expect((caught as { status?: unknown })?.status).toBeUndefined(); + }); + + it('naming the dataset’s OWN object: propagates instead of degrading to an empty grid', async () => { + // The other half of the same defect, and the quieter one: here the named + // relation IS `opportunity`, so the old code took the degradation branch — + // `{rows: []}` plus one warn, with the mistyped column mentioned to nobody. + const own = 'column "reginn" of relation "opportunity" does not exist'; + const log = logger(); + let caught: Error | undefined; + try { + await serviceThatThrows(new Error(own), log).queryDataset(dataset, SELECTION, CTX); + } catch (e) { + caught = e as Error; + } + expect(caught, 'a mistyped column became a confident empty chart').toBeInstanceOf(Error); + expect(String(caught?.message)).toBe(own); + expect(log.warn).not.toHaveBeenCalledWith(expect.stringContaining('returning an empty result')); + expect(log.warn).not.toHaveBeenCalledWith(expect.stringContaining('is unavailable')); + }); + + it('an unquoted look-alike is left alone — the subtraction does not widen past postgres', async () => { + // Direction-of-error guard. Postgres always quotes both names here, so the + // subtraction requires quotes; a sentence that merely talks ABOUT a relation + // must keep whatever verdict it had rather than newly becoming a hard + // failure, since over-matching would regress #5033's deliberate leniency. + expect(await outcomeOf('relation "opportunity" does not exist')).toBe('empty'); + }); +}); + +describe('[#6035] the other 12 in-repo wordings keep the verdict #5717 measured', () => { + it.each(CORPUS)('%s → %s', async (_row, wording, expected) => { + expect(await outcomeOf(wording)).toBe(expected); + }); + + it('exactly one corpus row is a hard failure for the column reason', () => { + // Counts the shape rather than restating the table: the corpus must keep + // containing the moved row and its read-path neighbour, so a future edit + // that quietly drops row 05 cannot leave this file passing. + const columnRows = CORPUS.filter(([, wording]) => /column\s+["'`]/i.test(wording)); + expect(columnRows.map(([row]) => row)).toEqual([ + '05 postgres MISSING COLUMN (write path)', + '06 postgres missing column (read path)', + ]); + expect(columnRows.every(([, , outcome]) => outcome === 'propagated')).toBe(true); + }); +}); diff --git a/packages/services/service-analytics/src/analytics-service.ts b/packages/services/service-analytics/src/analytics-service.ts index 74b8052c3c..172adadece 100644 --- a/packages/services/service-analytics/src/analytics-service.ts +++ b/packages/services/service-analytics/src/analytics-service.ts @@ -110,6 +110,36 @@ function hasDeclaredErrorEnvelope(err: unknown): boolean { return typeof e?.status === 'number' && typeof e?.code === 'string' && e.code.length > 0; } +/** + * [#6035] Postgres's MISSING COLUMN wording — the one driver phrase that is a + * missing-SOURCE phrase by substring while meaning the opposite. + * + * `column "label" of relation "acct" does not exist` (SQLSTATE 42703) + * + * `relation "acct" does not exist` sits inside it verbatim, so both + * {@link isMissingSourceError} and {@link missingSourceRelation} read it as + * "the table `acct` is gone" — which would degrade the widget to an empty grid + * (when `acct` is the dataset's own object) or report a cross-datasource + * topology error (when it is a joined one). Neither is true: `acct` is right + * there and a COLUMN NAME IS MISPELLED — precisely the class both docblocks + * promise to leave as a hard failure. + * + * Subtracting it first is the shape `rest-server.ts`'s `mapDataError` has used + * since #5352 (its `unknownColumn` probe extracts this same phrase ahead of the + * unknown-object branch, so the REST face answers `400 INVALID_FIELD` rather + * than `404`; the case is pinned in `rest.test.ts`). This is deliberately that + * regex rather than a second dialect of it — the two faces must not disagree + * about what counts as postgres saying "column". + * + * Both quotes are required because postgres always emits them here (its errmsg + * template is `column "%s" of relation "%s" does not exist`), and requiring + * them is the safe direction of error: a wording this misses merely keeps + * today's verdict, while one it over-matches would turn a genuinely missing + * table into a hard failure and regress #5033's deliberate leniency. + */ +const MISSING_COLUMN_OF_RELATION = + /column\s+["'`]([a-z0-9_]+)["'`]\s+of relation\s+\S+\s+does not exist/i; + /** * Detect the "backing object/table isn't present in this kernel" class of * error so a dataset query can degrade to an empty result instead of failing @@ -142,15 +172,19 @@ function hasDeclaredErrorEnvelope(err: unknown): boolean { * wording changes, which is what makes this a narrowing rather than a * behaviour change for #5033's leniency. * - * Known residue, filed as #6035 rather than widened into here: postgres spells - * a missing COLUMN on the write path as `column "c" of relation "t" does not - * exist`, which carries a whole missing-relation phrase inside it and so is a - * hit both before and after this anchor. Dormant on a read-only face (a SELECT - * says `column "c" does not exist`, no `relation`), but it is the one case - * where this predicate is still wider than the paragraph above it. + * [#6035] The residue #5717 left and named here is now closed by + * {@link MISSING_COLUMN_OF_RELATION}, subtracted BEFORE any limb below runs. + * The anchor above cannot do it alone, for a reason worth stating plainly: the + * missing-COLUMN wording literally CONTAINS a well-formed missing-relation + * wording, so no tightening of "does this say a relation is missing" can ever + * exclude it — only asking the more specific question FIRST can. That makes the + * ORDER the fix, not the pattern. */ function isMissingSourceError(err: unknown): boolean { const raw = String((err as { message?: unknown })?.message ?? err ?? ''); + // [#6035] Missing COLUMN is not missing SOURCE — the paragraph above promises + // column errors stay hard failures, and this is where that promise is kept. + if (MISSING_COLUMN_OF_RELATION.test(raw)) return false; const msg = raw.toLowerCase(); return ( msg.includes('no such table') || // sqlite / libsql @@ -178,9 +212,22 @@ function isMissingSourceError(err: unknown): boolean { * table name, so the result is comparable to a dataset's `object`), or * `undefined` when the driver's phrasing carries no name. Unparseable ⇒ the * caller keeps today's degradation, never a louder guess. + * + * [#6035] It subtracts {@link MISSING_COLUMN_OF_RELATION} for the same reason + * its sibling does, and the reason is CONSISTENCY rather than a second bug: + * measured on `origin/main`, the column wording made this function answer + * `sys_team`, so fixing only "is something missing" would leave the pair + * DISAGREEING — one saying nothing is missing, the other naming a table. That + * disagreement is the exact defect #5717 closed on the postgres limb, and + * re-opening it here would re-arm the same mine one edit away: today this + * function is only ever called behind a true `isMissingSourceError`, so the + * guard is unreachable, but "unreachable" is a property of the CALL ORDER at + * one call site, not of this function. Guarding both keeps the two answers + * derivable from the wording alone. */ function missingSourceRelation(err: unknown): string | undefined { const msg = String((err as { message?: unknown })?.message ?? err ?? ''); + if (MISSING_COLUMN_OF_RELATION.test(msg)) return undefined; const patterns = [ /no such table:\s*[`"'[]?([A-Za-z0-9_$.]+)/i, // sqlite / libsql /relation\s+[`"']?([A-Za-z0-9_$.]+)[`"']?\s+does not exist/i, // postgres