diff --git a/.changeset/pg-introspection-resolves-like-the-writer.md b/.changeset/pg-introspection-resolves-like-the-writer.md new file mode 100644 index 0000000000..b999530057 --- /dev/null +++ b/.changeset/pg-introspection-resolves-like-the-writer.md @@ -0,0 +1,43 @@ +--- +"@nextlyhq/adapter-drizzle": patch +"@nextlyhq/adapter-mysql": patch +"@nextlyhq/adapter-postgres": patch +"@nextlyhq/adapter-sqlite": patch +"@nextlyhq/admin": patch +"@nextlyhq/admin-css": patch +"@nextlyhq/blocks-engine": patch +"@nextlyhq/blocks-react": patch +"@nextlyhq/builder": patch +"create-nextly-app": patch +"@nextlyhq/eslint-config": patch +"@nextlyhq/eslint-plugin": patch +"@nextlyhq/module-specifiers": patch +"nextly": patch +"@nextlyhq/plugin-form-builder": patch +"@nextlyhq/plugin-page-builder": patch +"@nextlyhq/plugin-sdk": patch +"@nextlyhq/plugin-seo": patch +"@nextlyhq/prettier-config": patch +"@nextlyhq/storage-s3": patch +"@nextlyhq/storage-uploadthing": patch +"@nextlyhq/storage-vercel-blob": patch +"@nextlyhq/telemetry": patch +"@nextlyhq/tsconfig": patch +"@nextlyhq/ui": patch +--- + +On PostgreSQL, Nextly reads a database's current shape to work out what a +migration should change. Those reads looked in a schema called `public`, while +every statement Nextly writes is unqualified and lands wherever the connection's +`search_path` points. On a deployment that uses its own schema — one per tenant, +or just a house convention — the two disagreed. + +The reads now resolve a table the same way the writes do, quoting the name first +so one whose spelling carries capitals — which a custom table name may — resolves +to itself rather than to nothing. Nothing changes for a database that uses +`public`, which is the default. + +What it fixes on the others: columns that exist read as absent, so a migration +offered to add what was already there; and where a table of the same name existed +in `public`, its shape answered for the real one — comparing against a table +nothing writes to. diff --git a/packages/nextly/src/domains/schema/pipeline/diff/__tests__/introspect-search-path.integration.test.ts b/packages/nextly/src/domains/schema/pipeline/diff/__tests__/introspect-search-path.integration.test.ts new file mode 100644 index 0000000000..241247c67e --- /dev/null +++ b/packages/nextly/src/domains/schema/pipeline/diff/__tests__/introspect-search-path.integration.test.ts @@ -0,0 +1,140 @@ +/** + * Introspection must read the table the WRITES hit, not one named `public`. + * + * Every statement this package emits is unqualified — the DDL that created the + * tables and the DDL the diff generates — so PostgreSQL resolves it across the + * whole `search_path`. A read filtered on a schema NAME asks a different + * question, and the answers diverge exactly where it matters: a deployment whose + * search path is `tenant, public`. + * + * Only a real database can establish this. The predicate is SQL the server + * evaluates, and every failure mode is silent — columns that exist read as + * absent, so the diff proposes to add what is already there; or a same-named + * table in another schema answers for this one. + * + * 🔴 A DECOY, not an empty schema. Pointing the search path somewhere empty + * would prove only that something was found somewhere; a test that cannot tell + * "read the right table" from "read any table" passes on a predicate that names + * `public` outright. + * + * 🔴 ONE SESSION, held open for the whole file. `PostgresAdapter.getDrizzle()` + * wraps a `pg.Pool`, so `SET search_path` binds to whichever client served that + * statement and the next `execute()` may run on another — which would make this + * file assert against the decoy at random. A `Client` is one connection by + * construction, so the search path this sets is the one every read below uses. + * + * @module domains/schema/pipeline/diff/__tests__/introspect-search-path.integration + */ +import { sql } from "drizzle-orm"; +import { drizzle } from "drizzle-orm/node-postgres"; +import { Client } from "pg"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; + +import { queryLiveColumnTypes } from "../../live-column-types"; +import { introspectLiveSnapshot } from "../introspect-live"; + +const URL = process.env.TEST_POSTGRES_URL ?? ""; + +// Dialect gate, matching the other PostgreSQL integration tests in this package. +const describePg = describe.skipIf(!URL); + +const TENANT = "nx_search_path_probe"; +const TABLE = "nx_search_path_subject"; +// Uppercase on purpose: `resolveCollectionTableName` passes an author's `dbName` +// through verbatim, so a real table can carry capitals — and an unquoted name +// handed to `to_regclass` is folded to lower case and resolves to nothing. +const MIXED_CASE = "nx_SearchPath_Mixed"; + +describePg("introspection follows the search path (postgres)", () => { + let client: Client | undefined; + // Typed from the call rather than from `typeof drizzle`, whose default + // overload infers a Pool-backed client and does not accept this one. + let db: ReturnType, Client>>; + + beforeAll(async () => { + if (!URL) return; + client = new Client({ connectionString: URL }); + await client.connect(); + db = drizzle({ client }); + + const run = (text: string) => client!.query(text); + + await run(`DROP TABLE IF EXISTS public."${TABLE}"`); + await run(`DROP SCHEMA IF EXISTS ${TENANT} CASCADE`); + await run(`CREATE SCHEMA ${TENANT}`); + + // The decoy, in `public`: same name, a column the real one does not have. + // A read pinned to `public` finds THIS one and reports its shape. + await run(`CREATE TABLE public."${TABLE}" (decoy_only integer)`); + // The subject, in the tenant schema, carrying a different column. + await run(`CREATE TABLE ${TENANT}."${TABLE}" (subject_only text)`); + await run(`CREATE INDEX ix_subject ON ${TENANT}."${TABLE}" (subject_only)`); + // And one whose name survives only if it is quoted before it is resolved. + await run(`CREATE TABLE ${TENANT}."${MIXED_CASE}" (mixed_only text)`); + + // What a deployment on a non-default search path looks like. + await run(`SET search_path TO ${TENANT}, public`); + }); + + afterAll(async () => { + if (client === undefined) return; + await client.query(`SET search_path TO public`); + await client.query(`DROP TABLE IF EXISTS public."${TABLE}"`); + await client.query(`DROP SCHEMA IF EXISTS ${TENANT} CASCADE`); + await client.end(); + }); + + it("proves the session is the one that was configured", async () => { + // The control for every assertion below. If the search path were not the + // one set in `beforeAll` — a pooled connection, a reset — the decoy would + // answer and the failures would look like defects in the predicate. + const shown = await db.execute(sql`SHOW search_path`); + const rows = (shown as unknown as { rows: { search_path: string }[] }).rows; + expect(rows[0]?.search_path).toContain(TENANT); + }); + + it("reads the columns of the table an unqualified write would hit", async () => { + const live = await introspectLiveSnapshot(db, "postgresql", [TABLE]); + const columns = live.tables + .find(t => t.name === TABLE) + ?.columns.map(c => c.name); + + expect(columns).toEqual(["subject_only"]); + // Named separately: the decoy's column appearing IS the defect, and saying + // so makes a failure readable rather than a diff of two arrays. + expect(columns).not.toContain("decoy_only"); + }); + + it("reads the indexes of that table, not a same-named one elsewhere", async () => { + const live = await introspectLiveSnapshot(db, "postgresql", [TABLE]); + const indexes = live.tables.find(t => t.name === TABLE)?.indexes ?? []; + + // The decoy carries no index, so a read pinned to `public` reports none — + // which reads as "this table has no indexes" rather than as an error. + expect(indexes.map(i => i.name)).toContain("ix_subject"); + }); + + it("resolves a table whose name carries capitals", async () => { + // 🔴 `to_regclass` reparses its argument as an identifier reference, so an + // unquoted `nx_SearchPath_Mixed` folds to lower case and resolves to + // nothing — and the table reads as ABSENT, which makes a diff propose to + // create one that already exists. + const live = await introspectLiveSnapshot(db, "postgresql", [MIXED_CASE]); + const columns = live.tables + .find(t => t.name === MIXED_CASE) + ?.columns.map(c => c.name); + + expect(columns).toEqual(["mixed_only"]); + }); + + it("reports live column TYPES from the same table", async () => { + // A second query with the same predicate, feeding the type diff. Pinned to + // `public` it answers for the decoy: a column the subject does not have, + // and none of the ones it does. + const types = await queryLiveColumnTypes(db, "postgresql", [TABLE]); + const forTable = types.get(TABLE); + + expect(forTable?.get("subject_only")).toBe("text"); + expect(forTable?.has("decoy_only")).toBe(false); + }); +}); diff --git a/packages/nextly/src/domains/schema/pipeline/diff/introspect-live.ts b/packages/nextly/src/domains/schema/pipeline/diff/introspect-live.ts index dd7c679bda..c3d55d50df 100644 --- a/packages/nextly/src/domains/schema/pipeline/diff/introspect-live.ts +++ b/packages/nextly/src/domains/schema/pipeline/diff/introspect-live.ts @@ -17,6 +17,11 @@ import type { SupportedDialect } from "@nextlyhq/adapter-drizzle/types"; import { sql } from "drizzle-orm"; +import { + PG_CLASS_IS_THE_RELATION_THE_WRITES_HIT, + PG_RELATION_THE_WRITES_HIT, +} from "../pg-visible-relation"; + import { sizeFromDeclaration } from "./declared-size"; import type { ColumnSpec, @@ -229,6 +234,10 @@ export async function introspectLiveSnapshot( // that is not exactly a `nextval()` call yields NULL from the substring // and so falls to false, which is the safe direction: a default the diff // does not recognise is reported, never swallowed. + // Scoped by `PG_RELATION_THE_WRITES_HIT`, which carries the reasoning: the + // read has to resolve the same relation an unqualified statement does, and + // neither a schema name nor `current_schema()` does that. + // // `is_primary_key` comes from `pg_index.indisprimary` rather than // `information_schema.table_constraints`, so it needs no second round trip // and reports the same key the index query deliberately excludes. A live @@ -262,7 +271,7 @@ export async function introspectLiveSnapshot( false ) AS owned_sequence_default FROM information_schema.columns c - WHERE c.table_schema = 'public' + WHERE ${PG_RELATION_THE_WRITES_HIT} AND c.table_name IN (${tableNamesIn}) ORDER BY c.table_name, c.ordinal_position` )) as { rows: PgRow[] }; @@ -271,21 +280,20 @@ export async function introspectLiveSnapshot( // (indisprimary) and partial indexes (indpred). Expression indexes yield no // pg_attribute row and are naturally excluded. // - // Scoped to `public` like the column query above. `pg_class.relname` is - // unique per schema, not per database, so without the namespace join a - // same-named table in another schema contributes its indexes to these rows - // and `attachIndexes` groups them under the same name — reporting indexes - // that are not on the table being introspected, and masking the absence of - // ones that should be. + // Scoped through the relation like the column query above. `pg_class.relname` + // is unique per schema, not per database, so without a scope a same-named + // table in another schema contributes its indexes to these rows and + // `attachIndexes` groups them under the same name — reporting indexes that + // are not on the table being introspected, and masking the absence of ones + // that should be. const idxResult = (await dbTyped.execute( sql`SELECT t.relname AS table, i.relname AS index, ix.indisunique AS unique, a.attname AS column, array_position(ix.indkey, a.attnum) AS ord FROM pg_class t - JOIN pg_namespace n ON n.oid = t.relnamespace JOIN pg_index ix ON ix.indrelid = t.oid JOIN pg_class i ON i.oid = ix.indexrelid JOIN pg_attribute a ON a.attrelid = t.oid AND a.attnum = ANY(ix.indkey) - WHERE n.nspname = 'public' + WHERE ${PG_CLASS_IS_THE_RELATION_THE_WRITES_HIT} AND t.relname IN (${tableNamesIn}) AND ix.indisprimary = false AND ix.indpred IS NULL diff --git a/packages/nextly/src/domains/schema/pipeline/live-column-types.ts b/packages/nextly/src/domains/schema/pipeline/live-column-types.ts index 05ba35cbbf..95ba71d1f2 100644 --- a/packages/nextly/src/domains/schema/pipeline/live-column-types.ts +++ b/packages/nextly/src/domains/schema/pipeline/live-column-types.ts @@ -22,6 +22,8 @@ import type { SupportedDialect } from "@nextlyhq/adapter-drizzle/types"; import { sql } from "drizzle-orm"; +import { PG_RELATION_THE_WRITES_HIT } from "./pg-visible-relation"; + interface PgRow { table_name: string; column_name: string; @@ -81,11 +83,14 @@ export async function queryLiveColumnTypes( // object with a `.rows` array, NOT a flat row array. Verified // empirically against real PG - reading `result` directly as an // array iterates zero rows even when the query returns matches. + // Scoped by `PG_RELATION_THE_WRITES_HIT`. Here a wrong answer is a column's + // live type read from a same-named table in another schema — which is the + // input the type diff trusts. const result = (await dbTyped.execute( - sql`SELECT table_name, column_name, udt_name - FROM information_schema.columns - WHERE table_schema = 'public' - AND table_name IN (${tableNamesIn})` + sql`SELECT c.table_name, c.column_name, c.udt_name + FROM information_schema.columns c + WHERE ${PG_RELATION_THE_WRITES_HIT} + AND c.table_name IN (${tableNamesIn})` )) as { rows: PgRow[] }; for (const row of result.rows) { let cols = out.get(row.table_name); diff --git a/packages/nextly/src/domains/schema/pipeline/pg-visible-relation.ts b/packages/nextly/src/domains/schema/pipeline/pg-visible-relation.ts new file mode 100644 index 0000000000..f05c19dabf --- /dev/null +++ b/packages/nextly/src/domains/schema/pipeline/pg-visible-relation.ts @@ -0,0 +1,64 @@ +/** + * Which PostgreSQL relation a read is about, decided the way the WRITES decide. + * + * Every statement this package emits is unqualified — the DDL that created a + * table and the DDL a diff generates — so PostgreSQL resolves it across the + * whole `search_path`. A read that filters on a schema NAME asks a different + * question, and the two answers separate exactly where it matters: a deployment + * whose search path is `tenant, public`. + * + * 🔴 `current_schema()` is not the answer either. It is only the FIRST entry of + * the search path, so a table in `public` reached from `tenant, public` is + * written by the ALTER and missed by the read. `live-table-facts.ts` settled + * this for the same reason and resolves through `to_regclass`; this is that rule + * for the reads that go through `information_schema` rather than through an OID. + * + * The failures it prevents are silent and point the wrong way. Columns that + * exist read as absent, so a diff offers to add what is already there. And where + * a same-named table exists in another schema on the path, ITS shape answers for + * the real one — so the diff compares against a table nothing writes to. + * + * ## Why the name is quoted before it is resolved + * + * 🔴 `to_regclass` takes TEXT and reparses it as an identifier reference, so an + * unquoted catalog name is folded and split exactly as if it had been typed: + * `dc_LegacyPosts` resolves as `dc_legacyposts`, and a name containing a dot + * resolves as `schema.table`. Both are reachable — `resolveCollectionTableName` + * passes an author's `dbName` through verbatim, prefixing `dc_` and nothing + * else — and both would make an existing table read as ABSENT, which is the + * worst answer this module can give: a diff that proposes to create a table that + * is already there. + * + * `quote_ident` is the inverse of that parse, so the text goes back in as the + * one identifier the catalog says it is. + * + * @module domains/schema/pipeline/pg-visible-relation + */ +import { sql, type SQL } from "drizzle-orm"; + +/** + * True for the one `information_schema.columns` row set whose relation an + * unqualified statement would reach. + * + * 🔴 Written against the alias `c`, because a predicate over + * `information_schema` has to name that view's own columns and they cannot be + * passed as parameters. Every caller aliases the view `c`; there are two, both + * in this directory, and a caller that aliases it otherwise gets a SQL error + * rather than a wrong answer — which is the safe direction for a mistake that + * would otherwise change WHICH TABLE is reported. + * + * Compared by identity rather than by spelling: `format('%I.%I', …)::regclass` + * is this row's relation, `to_regclass(…)` is the one the search path resolves, + * and the equality asks whether they are the same relation. The same device the + * sequence-ownership check in `introspect-live.ts` already uses. + */ +export const PG_RELATION_THE_WRITES_HIT: SQL = sql`format('%I.%I', c.table_schema, c.table_name)::regclass = to_regclass(quote_ident(c.table_name))`; + +/** + * The same question asked of a `pg_class` row, which carries the OID directly. + * + * Spelled here rather than inline at the index query so the two reads cannot + * drift: they must agree about which relation they are describing, or the + * columns come from one table and the indexes from another. + */ +export const PG_CLASS_IS_THE_RELATION_THE_WRITES_HIT: SQL = sql`t.oid = to_regclass(quote_ident(t.relname))`;