fix(metadata-protocol): the view-definition conflict report's remedy query now runs on PostgreSQL, not only SQLite (#6772)

.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.

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);

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
+ * `<column>_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`
  );
 }