From 5b854229f19b8cbf6388bb38090d827c0d442aba Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 22:31:03 +0000 Subject: [PATCH 1/2] fix(metadata-protocol): project the duplicate-probe query's folded columns through their own COALESCE (#6772) --- .../view-definition-active-index.test.ts | 66 ++++++++++++++++++- .../view-definition-active-index.ts | 31 +++++++-- 2 files changed, 90 insertions(+), 7 deletions(-) diff --git a/packages/metadata-protocol/src/migrations/view-definition-active-index.test.ts b/packages/metadata-protocol/src/migrations/view-definition-active-index.test.ts index 4a19ceb584..863f8a3fd1 100644 --- a/packages/metadata-protocol/src/migrations/view-definition-active-index.test.ts +++ b/packages/metadata-protocol/src/migrations/view-definition-active-index.test.ts @@ -13,6 +13,7 @@ import { viewActiveIndexKeyParts, VIEW_ACTIVE_INDEX_NAME, VIEW_ACTIVE_PROBE_INDEX_NAME, + VIEW_ACTIVE_INDEX_COLUMNS, VIEW_ACTIVE_NULL_SENTINELS, type IndexExec, } from './view-definition-active-index.js'; @@ -387,6 +388,20 @@ describe('sys_view_definition active-row uniqueness (#5839) on a NULL-safe key ( expect(offenders).toHaveLength(1); expect(offenders[0]!.name).toBe('lead.team'); expect(offenders[0]!.duplicate_rows).toBe(2); + // The folded columns come back under their bucket-key aliases (#6772), + // and the operator loses nothing by reading them: the offending pair is + // `owner IS NULL`, and `''` is the only way that can be spelled here + // because an owner is a user id and never the empty string. + expect(offenders[0]!.organization_id_key).toBe('org1'); + expect(offenders[0]!.owner_key).toBe(''); + // ⚠️ What this test can and cannot see: the pre-#6772 bare projection + // EXECUTED here without error and returned the same one offender row + // with `duplicate_rows: 2` — SQLite grouped it happily, so the three + // assertions above the alias pair were green on the broken query too. + // Only the alias names (and the dialect pin below) move. Running the + // query on a real database therefore proves it lists the rows; it can + // never prove the query is legal on PostgreSQL, because the engine + // this test has is precisely the lenient one. }); /** @@ -484,13 +499,58 @@ describe('sys_view_definition active-row uniqueness (#5839) on a NULL-safe key ( // Same key parts as the index, so what it reports and what the index // rejects cannot diverge. expect(probe).toContain(`GROUP BY ${viewActiveIndexKeyParts().join(', ')}`); - // Selected columns are the RAW ones — an operator needs the stored - // values (NULLs included), not the folded bucket keys. - expect(probe).toContain('SELECT name, organization_id, owner, COUNT(*) AS duplicate_rows'); + // The projection is those same key parts — each folded column under + // its bucket-key alias, never bare (#6772; see the dialect pin below). + expect(probe).toContain( + "SELECT name, COALESCE(organization_id, '__global__') AS organization_id_key, " + + "COALESCE(owner, '') AS owner_key, COUNT(*) AS duplicate_rows", + ); expect(probe).toContain("WHERE state = 'active'"); expect(probe).toContain('HAVING COUNT(*) > 1'); }); + /** + * The query is shipped to an operator inside an `error`-level degradation + * report, on both dialects that can build the index it explains. It has to + * RUN on both. PostgreSQL requires every non-aggregated projection to + * appear verbatim in `GROUP BY`; a bare `organization_id` projected against + * `GROUP BY COALESCE(organization_id, '__global__')` is rejected with + * `must appear in the GROUP BY clause` — which is exactly what shipped + * until #6772, invisible because the only engine the sibling test above can + * run is the lenient one. + * + * Mirrors `overlay-index.test.ts`'s + * `the duplicate-listing query is groupable on PostgreSQL, not only SQLite`. + */ + it('the duplicate-listing query is groupable on PostgreSQL, not only SQLite', () => { + const sql = buildDuplicateProbeSql(); + expect(sql).toEqual( + "SELECT name, COALESCE(organization_id, '__global__') AS organization_id_key, " + + "COALESCE(owner, '') AS owner_key, COUNT(*) AS duplicate_rows " + + "FROM sys_view_definition WHERE state = 'active' " + + "GROUP BY name, COALESCE(organization_id, '__global__'), COALESCE(owner, '') " + + 'HAVING COUNT(*) > 1', + ); + + // PG's rule, applied term by term rather than only to the whole string: + // every BARE projection must be a bare GROUP BY term, and every folded + // column must reach the select list only through its own expression. + const selectList = sql.slice('SELECT '.length, sql.indexOf(' FROM ')); + const groupBy = sql.slice(sql.indexOf('GROUP BY ') + 'GROUP BY '.length, sql.indexOf(' HAVING')); + for (const column of VIEW_ACTIVE_INDEX_COLUMNS) { + const bare = new RegExp(`(^|, )${column}(,|$)`); + const sentinel = VIEW_ACTIVE_NULL_SENTINELS[column]; + if (sentinel === undefined) { + expect(selectList).toMatch(bare); + expect(groupBy).toMatch(bare); + } else { + expect(selectList).not.toMatch(bare); + expect(selectList).toContain(`COALESCE(${column}, '${sentinel}') AS ${column}_key`); + expect(groupBy).toContain(`COALESCE(${column}, '${sentinel}')`); + } + } + }); + it('resolveIndexExec prefers raw(), falls back to execute(), else undefined', async () => { const raw = vi.fn(async () => undefined); const execute = vi.fn(async () => undefined); diff --git a/packages/metadata-protocol/src/migrations/view-definition-active-index.ts b/packages/metadata-protocol/src/migrations/view-definition-active-index.ts index 18a5a31193..e3faf54283 100644 --- a/packages/metadata-protocol/src/migrations/view-definition-active-index.ts +++ b/packages/metadata-protocol/src/migrations/view-definition-active-index.ts @@ -225,14 +225,37 @@ export function buildActiveIndexSql(indexName: string): string { * has it without waiting for `os migrate plan`. * * It GROUPs by exactly the index's own key parts, so what it reports and what - * the index rejects cannot diverge. Dialect-neutral: `COALESCE`, `GROUP BY` - * and `HAVING` are ANSI on every engine this platform runs on. + * the index rejects cannot diverge — the projection and the `GROUP BY` are + * built from the SAME array below, so they cannot drift apart either. + * + * ⚠️ Each folded column is projected through its OWN `COALESCE` (aliased + * `_key`), never bare. The three constructs are ANSI, but a query that + * projects a bare column while grouping by that column only INSIDE an + * expression is not: PostgreSQL requires every non-aggregated projection to + * appear verbatim in `GROUP BY` and rejects the bare form with + * `column "sys_view_definition.organization_id" must appear in the GROUP BY + * clause`. SQLite accepts it, which is why this shipped broken (#6772) — and + * PostgreSQL is one of exactly TWO dialects that can build the index this + * query explains, so it must be legal on both, not on the lenient one. + * + * Folding costs the operator nothing here: neither sentinel can occur in real + * data, so `organization_id_key = '__global__'` reads as "organization_id IS + * NULL" and `owner_key = ''` as "owner IS NULL" (see + * {@link VIEW_ACTIVE_NULL_SENTINELS}). + * + * Same shape as `overlay-index.ts`'s `buildOverlayDuplicateProbeSql()`, the + * sibling migration's query for the same report (#6770). */ export function buildDuplicateProbeSql(): string { + const keyParts = viewActiveIndexKeyParts(); + const projected = VIEW_ACTIVE_INDEX_COLUMNS.map((column, i) => { + const keyPart = keyParts[i]!; + return keyPart === column ? column : `${keyPart} AS ${column}_key`; + }); return ( - `SELECT ${VIEW_ACTIVE_INDEX_COLUMNS.join(', ')}, COUNT(*) AS duplicate_rows ` + + `SELECT ${projected.join(', ')}, COUNT(*) AS duplicate_rows ` + `FROM ${VIEW_DEFINITION_TABLE} WHERE state = 'active' ` + - `GROUP BY ${viewActiveIndexKeyParts().join(', ')} HAVING COUNT(*) > 1` + `GROUP BY ${keyParts.join(', ')} HAVING COUNT(*) > 1` ); } From 4843d5fdba89512733b63faa4d4a115ab6614f33 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 22:51:21 +0000 Subject: [PATCH 2/2] chore: changeset for the PG-groupable duplicate-probe projection (#6772) --- ...view-definition-probe-sql-pg-projection.md | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 .changeset/view-definition-probe-sql-pg-projection.md diff --git a/.changeset/view-definition-probe-sql-pg-projection.md b/.changeset/view-definition-probe-sql-pg-projection.md new file mode 100644 index 0000000000..77b6628aa5 --- /dev/null +++ b/.changeset/view-definition-probe-sql-pg-projection.md @@ -0,0 +1,59 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): the view-definition conflict report's remedy query now runs on PostgreSQL, not only SQLite (#6772) + +`buildDuplicateProbeSql()` — the query `ensureViewDefinitionActiveIndex` ships +**inside** its `error`-level degradation report, as ADR-0120 D4's "name the +offending rows" — projected two bare columns while grouping by only their +`COALESCE` forms: + +```sql +SELECT name, organization_id, owner, COUNT(*) AS duplicate_rows +FROM sys_view_definition WHERE state = 'active' +GROUP BY name, COALESCE(organization_id, '__global__'), COALESCE(owner, '') +HAVING COUNT(*) > 1 +``` + +PostgreSQL requires every non-aggregated projection to appear **verbatim** in +`GROUP BY`; wrapped in an expression does not count. So the query an operator is +handed fails with + +```text +ERROR: column "sys_view_definition.organization_id" must appear in the GROUP BY + clause or be used in an aggregate function +``` + +on one of exactly **two** dialects that can build the partial index the report is +explaining. The operator copy-pastes the remedy out of an error message and gets +a second error instead of the conflicting rows. SQLite accepts the bare form, +which is why the existing real-SQLite test stayed green and the defect shipped; +MySQL/MariaDB reaches the same string through the `unsupported` arm. + +Each folded column is now projected through its own `COALESCE` under a bucket-key +alias — the shape `overlay-index.ts`'s `buildOverlayDuplicateProbeSql()` already +uses for the sibling migration (#6770): + +```sql +SELECT name, COALESCE(organization_id, '__global__') AS organization_id_key, + COALESCE(owner, '') AS owner_key, COUNT(*) AS duplicate_rows +FROM sys_view_definition WHERE state = 'active' +GROUP BY name, COALESCE(organization_id, '__global__'), COALESCE(owner, '') +HAVING COUNT(*) > 1 +``` + +Every bare projection is now a bare `GROUP BY` term, so the query is legal on +both dialects. The projection and the `GROUP BY` are built from the same array, +so they cannot drift apart again. Nothing is lost by reading bucket keys instead +of stored values: neither sentinel can occur in real data, so +`organization_id_key = '__global__'` means `organization_id IS NULL` and +`owner_key = ''` means `owner IS NULL`. + +The function's "Dialect-neutral: `COALESCE`, `GROUP BY` and `HAVING` are ANSI on +every engine this platform runs on" comment was true about the three constructs +and false about the query built from them; it now states the projection rule the +query has to satisfy, and why the real-SQLite test cannot see it. + +No behaviour change to any index, write path or status: only the text of the +remedy query inside the two degradation reports.