diff --git a/apps/dev-playground/config/database/schema.ts b/apps/dev-playground/config/database/schema.ts index 2bc9a0126..1d779f597 100644 --- a/apps/dev-playground/config/database/schema.ts +++ b/apps/dev-playground/config/database/schema.ts @@ -1,6 +1,7 @@ // Annotations a reviewer leaves on a saved dashboard, plus the audit trail the // plugin keeps for them. Three tables is enough to exercise a two-edge include; -// the DatabasePlugin does not create them, so the app expects them to exist. +// the DatabasePlugin does not create them, so the app expects them to exist in +// public, where they were created before the default schema became app. import { defineSchema, @@ -11,34 +12,37 @@ import { varchar, } from "@databricks/appkit/beta"; -export const schema = defineSchema(({ table }) => { - const boards = table("boards", { - id: id(), - slug: varchar(64).notNull().unique(), - title: text().notNull(), - created_at: timestamp({ withTimezone: true }).defaultNow().notNull(), - }); +export const schema = defineSchema( + ({ table }) => { + const boards = table("boards", { + id: id(), + slug: varchar(64).notNull().unique(), + title: text().notNull(), + created_at: timestamp({ withTimezone: true }).defaultNow().notNull(), + }); - const notes = table("notes", { - id: id(), - board_id: fk(() => boards.id) - .notNull() - .onDelete("cascade"), - author: text().notNull(), - // Server code needs it to notify the reviewer; no client should see it. - author_email: text().private(), - body: text().notNull(), - created_at: timestamp({ withTimezone: true }).defaultNow().notNull(), - }); + const notes = table("notes", { + id: id(), + board_id: fk(() => boards.id) + .notNull() + .onDelete("cascade"), + author: text().notNull(), + // Server code needs it to notify the reviewer; no client should see it. + author_email: text().private(), + body: text().notNull(), + created_at: timestamp({ withTimezone: true }).defaultNow().notNull(), + }); - const note_events = table("note_events", { - id: id(), - note_id: fk(() => notes.id) - .notNull() - .onDelete("cascade"), - action: varchar(32).notNull(), - created_at: timestamp({ withTimezone: true }).defaultNow().notNull(), - }); + const note_events = table("note_events", { + id: id(), + note_id: fk(() => notes.id) + .notNull() + .onDelete("cascade"), + action: varchar(32).notNull(), + created_at: timestamp({ withTimezone: true }).defaultNow().notNull(), + }); - return { boards, notes, note_events }; -}); + return { boards, notes, note_events }; + }, + { schemaName: "public" }, +); diff --git a/docs/docs/plugins/database.md b/docs/docs/plugins/database.md index 7f8aa77e1..1467dd10b 100644 --- a/docs/docs/plugins/database.md +++ b/docs/docs/plugins/database.md @@ -84,6 +84,34 @@ public column, list with an explicit non-empty `order`. A keyless table with no sortable columns has no list route. `upsert` is available to server code but has no generated HTTP route. +Tables live in the Postgres schema `app` unless the second argument of +`defineSchema()` names another one, for example `{ schemaName: "public" }`. The +default is not `public` because the deployed app's service principal creates and +owns `app`, but has no `CREATE` privilege on `public`. + +Give each app its own schema. Two apps on one database and one schema share the +migration history, and the second app's service principal cannot change the +tables the first one owns. + +### Apps with tables in `public` + +Earlier releases put tables in `public` by default. An app that created its +tables there fails setup after the upgrade, and the error says the tables are in +`public`. Keep them there with: + +```ts +export const schema = defineSchema( + ({ table }) => ({ + notes: table("notes", { id: id(), body: text().notNull() }), + }), + { schemaName: "public" }, +); +``` + +Migrations applied on start into `public` also need +`GRANT CREATE ON SCHEMA public` to the app's service principal, because it does +not own `public`. + ## Schema discovery and overrides `database()` and `database({})` use the same defaults. During setup, the plugin diff --git a/packages/appkit/src/database/migrations/ownership.ts b/packages/appkit/src/database/migrations/ownership.ts index ec2dd75ac..3136b0daa 100644 --- a/packages/appkit/src/database/migrations/ownership.ts +++ b/packages/appkit/src/database/migrations/ownership.ts @@ -50,6 +50,15 @@ export function ownershipHint( "On the deploy branch that role is the app's service principal: bind the database to the app with CAN_CONNECT_AND_CREATE." ); } + if ( + ownership?.schemaName === "public" && + ownership.owner !== ownership.currentUser + ) { + return ( + `Schema public belongs to "${ownership.owner}", and the app's role "${ownership.currentUser}" has no CREATE on it. ` + + `Grant it once with GRANT CREATE ON SCHEMA public TO "${ownership.currentUser}", or drop schemaName from defineSchema() to use the default schema app, which the app creates and owns.` + ); + } if (ownership && ownership.owner !== ownership.currentUser) { return ( `Schema "${ownership.schemaName}" is owned by "${ownership.owner}", but the app connects as "${ownership.currentUser}". ` + diff --git a/packages/appkit/src/database/migrations/tests/ownership.test.ts b/packages/appkit/src/database/migrations/tests/ownership.test.ts index 4f93e59d7..27acc56b5 100644 --- a/packages/appkit/src/database/migrations/tests/ownership.test.ts +++ b/packages/appkit/src/database/migrations/tests/ownership.test.ts @@ -40,6 +40,19 @@ describe("ownership: one owner per branch", () => { ); }); + test("public, which the app never owns, gets the grant or the default schema", () => { + expect( + ownershipHint("42501", { + schemaName: "public", + owner: "pg_database_owner", + currentUser: "app-sp", + }), + ).toBe( + `Schema public belongs to "pg_database_owner", and the app's role "app-sp" has no CREATE on it. ` + + `Grant it once with GRANT CREATE ON SCHEMA public TO "app-sp", or drop schemaName from defineSchema() to use the default schema app, which the app creates and owns.`, + ); + }); + test("any other failure gets no ownership hint", () => { expect(ownershipHint("42P07")).toBeUndefined(); expect(ownershipHint(undefined)).toBeUndefined(); diff --git a/packages/appkit/src/database/runtime/tests/drizzle-data-path.test.ts b/packages/appkit/src/database/runtime/tests/drizzle-data-path.test.ts index b9d35409b..7c9bc7241 100644 --- a/packages/appkit/src/database/runtime/tests/drizzle-data-path.test.ts +++ b/packages/appkit/src/database/runtime/tests/drizzle-data-path.test.ts @@ -23,22 +23,26 @@ import { } from "../engine/drizzle-data-path"; import { returningColumns } from "../engine/translate"; -const schema = defineSchema((builder) => { - const users = builder.table("users", { - id: id(), - email: text().unique(), - name: text(), - active: boolean(), - secret: text().private(), - }); - const posts = builder.table("posts", { - id: id(), - authorId: fk(() => users.id), - title: text(), - secret: text().private(), - }); - return { users, posts }; -}); +// Explicitly public: the order-by assertion expects unqualified SQL ("users"."secret"). +const schema = defineSchema( + (builder) => { + const users = builder.table("users", { + id: id(), + email: text().unique(), + name: text(), + active: boolean(), + secret: text().private(), + }); + const posts = builder.table("posts", { + id: id(), + authorId: fk(() => users.id), + title: text(), + secret: text().private(), + }); + return { users, posts }; + }, + { schemaName: "public" }, +); const users = schema.$tables.users; const naturalKeySchema = defineSchema((builder) => ({ diff --git a/packages/appkit/src/database/runtime/tests/translate.test.ts b/packages/appkit/src/database/runtime/tests/translate.test.ts index cf2c11fa9..cd90de104 100644 --- a/packages/appkit/src/database/runtime/tests/translate.test.ts +++ b/packages/appkit/src/database/runtime/tests/translate.test.ts @@ -35,27 +35,31 @@ import { translateWhere, } from "../engine/translate"; -const schema = defineSchema((builder) => { - const users = builder.table("users", { - id: id(), - name: text(), - age: integer(), - active: boolean().notNull(), - large: bigint().notNull(), - createdAt: timestamp().notNull(), - externalId: uuid().notNull(), - status: enumColumn("user_status", ["active", "disabled"]).notNull(), - metadata: jsonb(), - secret: text().private(), - }); - const posts = builder.table("posts", { - id: id(), - authorId: fk(() => users.id), - title: text(), - secret: text().private(), - }); - return { users, posts }; -}); +// Explicitly public: the assertions expect unqualified column SQL ("users"."name"). +const schema = defineSchema( + (builder) => { + const users = builder.table("users", { + id: id(), + name: text(), + age: integer(), + active: boolean().notNull(), + large: bigint().notNull(), + createdAt: timestamp().notNull(), + externalId: uuid().notNull(), + status: enumColumn("user_status", ["active", "disabled"]).notNull(), + metadata: jsonb(), + secret: text().private(), + }); + const posts = builder.table("posts", { + id: id(), + authorId: fk(() => users.id), + title: text(), + secret: text().private(), + }); + return { users, posts }; + }, + { schemaName: "public" }, +); const users = schema.$tables.users; const dialect = new PgDialect(); diff --git a/packages/appkit/src/database/schema-builder/define-schema.ts b/packages/appkit/src/database/schema-builder/define-schema.ts index 64ed5ff2d..5d8cf3d91 100644 --- a/packages/appkit/src/database/schema-builder/define-schema.ts +++ b/packages/appkit/src/database/schema-builder/define-schema.ts @@ -339,6 +339,13 @@ export function assertFinalizedSchema(value: unknown): asserts value is Schema { } } +/** + * Postgres schema of every table when `schemaName` is omitted. Not `public`: + * the deployed app's service principal can create and own a schema of its + * own, but has no CREATE on `public`. + */ +const DEFAULT_SCHEMA_NAME = "app"; + /** * Compile one declared schema. The returned type keeps the table names the * builder returned, so `api.tables` and `hooks` can name only real tables. @@ -347,7 +354,7 @@ export function defineSchema>( builder: (context: SchemaBuilderContext) => TTables, options?: DefineSchemaOptions, ): Schema> { - const schemaName = options?.schemaName ?? "public"; + const schemaName = options?.schemaName ?? DEFAULT_SCHEMA_NAME; if (!schemaName) throw new SchemaBuildError("Schema name cannot be empty"); const state: DeclarationState = { diff --git a/packages/appkit/src/database/schema-builder/tests/define-schema.test.ts b/packages/appkit/src/database/schema-builder/tests/define-schema.test.ts index 6afa07afe..a8fe2ac0b 100644 --- a/packages/appkit/src/database/schema-builder/tests/define-schema.test.ts +++ b/packages/appkit/src/database/schema-builder/tests/define-schema.test.ts @@ -79,8 +79,10 @@ describe("defineSchema finalization", () => { expect(Object.getPrototypeOf(schema.$tables.users.$columns)).toBeNull(); }); - it("defaults to public and supports a safe custom schema", () => { - expect(schema.$schemaName).toBe("public"); + it("defaults to app and supports a safe custom schema", () => { + // Not public: the deployed app's service principal has no CREATE there. + expect(schema.$schemaName).toBe("app"); + expect(schema.$tables.users.$schemaName).toBe("app"); const options: DefineSchemaOptions = { schemaName: "application" }; const custom = defineSchema( (builder) => ({ widgets: builder.table("widgets", { id: id() }) }), @@ -90,6 +92,26 @@ describe("defineSchema finalization", () => { expect(custom.$tables.widgets.$schemaName).toBe("application"); }); + it.each([ + ["by default, in app", undefined, "app"], + ['with schemaName "public", unqualified', "public", undefined], + ] as const)("creates tables and enums %s", (_label, schemaName, expected) => { + const schema = defineSchema( + (builder) => ({ + tickets: builder.table("tickets", { + status: builder.enum("ticket_status", ["open", "closed"]), + }), + }), + schemaName ? { schemaName } : undefined, + ); + const config = getTableConfig(pgOf(schema.$tables.tickets.$engine)); + expect(config.schema).toBe(expected); + expect( + (config.columns[0] as unknown as { enum?: { schema?: string } }).enum + ?.schema, + ).toBe(expected); + }); + it("accepts the explicit builder context type", () => { const build = (builder: SchemaBuilderContext) => { const users: TableHandle<{ id: ColumnBuilder; email: ColumnBuilder }> = diff --git a/packages/appkit/src/database/schema-builder/types.ts b/packages/appkit/src/database/schema-builder/types.ts index d49a14b02..133cbb77a 100644 --- a/packages/appkit/src/database/schema-builder/types.ts +++ b/packages/appkit/src/database/schema-builder/types.ts @@ -157,6 +157,7 @@ export type TableHandle> = AppKitTable & { }; export interface DefineSchemaOptions { + /** Postgres schema of every table. Default: `"app"`. */ readonly schemaName?: string; } diff --git a/packages/appkit/src/plugins/database/schema-check.ts b/packages/appkit/src/plugins/database/schema-check.ts index bf206c427..69a6d575e 100644 --- a/packages/appkit/src/plugins/database/schema-check.ts +++ b/packages/appkit/src/plugins/database/schema-check.ts @@ -75,8 +75,36 @@ export async function assertSchemaMatchesDatabase( } } if (problems.length > 0) { + const absent = tableNames.filter((name) => !found.has(name)); + const advice = + (await inPublicInstead(dataPath, schemaName, absent)) ?? + "Create them, or commit migrations under config/database/migrations and set APPKIT_DB_MIGRATE_ON_START=true in app.yaml so the deployed app applies them on start."; throw databaseSetupFailed( - `the declared schema does not match the database: ${problems.join("; ")}. Create them, or commit migrations under config/database/migrations and set APPKIT_DB_MIGRATE_ON_START=true in app.yaml so the deployed app applies them on start.`, + `the declared schema does not match the database: ${problems.join("; ")}. ${advice}`, ); } } + +/** + * Apps from before the default schema moved to `app` keep their tables in + * `public`. Name that case; migrating would create a second, empty copy. + */ +async function inPublicInstead( + dataPath: DataPath, + schemaName: string, + absent: readonly string[], +): Promise { + if (schemaName === "public" || absent.length === 0) return undefined; + const rows = await dataPath.raw<{ table_name: string }>` + select c.relname::text as table_name + from pg_catalog.pg_class c + join pg_catalog.pg_namespace n on n.oid = c.relnamespace + where n.nspname = 'public' + and c.relname::text = any(${absent}::text[]) + and c.relkind in ('r', 'p')`; + if (rows.length < absent.length) return undefined; + return ( + `Schema public has ${absent.length === 1 ? "that table" : "those tables"}, but defineSchema() puts tables in schema "${schemaName}" unless told otherwise. ` + + 'To keep them in public, pass { schemaName: "public" } as the second argument of defineSchema() in config/database/schema.ts.' + ); +} diff --git a/packages/appkit/src/plugins/database/tests/lifecycle.test.ts b/packages/appkit/src/plugins/database/tests/lifecycle.test.ts index 0401912f6..fe7695dc7 100644 --- a/packages/appkit/src/plugins/database/tests/lifecycle.test.ts +++ b/packages/appkit/src/plugins/database/tests/lifecycle.test.ts @@ -209,7 +209,7 @@ describe("createDatabaseState", () => { const drift = new DatabasePluginError( "SETUP_FAILED", "setup", - "Database setup failed: table public.tags does not exist", + "Database setup failed: table app.tags does not exist", ); mocks.assertSchemaMatchesDatabase.mockRejectedValueOnce(drift); @@ -223,7 +223,7 @@ describe("createDatabaseState", () => { schema, ); expect(error).toBe(drift); - expect(error.message).toContain("table public.tags does not exist"); + expect(error.message).toContain("table app.tags does not exist"); expect(pool.end).toHaveBeenCalledTimes(1); }); diff --git a/packages/appkit/src/plugins/database/tests/migrate-on-start.test.ts b/packages/appkit/src/plugins/database/tests/migrate-on-start.test.ts index 6d53bc5c4..a1aaa09a3 100644 --- a/packages/appkit/src/plugins/database/tests/migrate-on-start.test.ts +++ b/packages/appkit/src/plugins/database/tests/migrate-on-start.test.ts @@ -20,10 +20,10 @@ import { MigrationFilesError } from "../../../database/migrations/paths"; import { defineSchema, id } from "../../../database/schema-builder"; import { migrateOnStart, shouldMigrateOnStart } from "../migrate-on-start"; -const schema = defineSchema( - ({ table }) => ({ notes: table("notes", { id: id() }) }), - { schemaName: "app" }, -); +// The default schema, app, holds the journal too. +const schema = defineSchema(({ table }) => ({ + notes: table("notes", { id: id() }), +})); const roots: string[] = []; const pool = { connect: vi.fn() }; const deployEnv = { diff --git a/packages/appkit/src/plugins/database/tests/schema-check.test.ts b/packages/appkit/src/plugins/database/tests/schema-check.test.ts index 6f10836a8..2abb3d26c 100644 --- a/packages/appkit/src/plugins/database/tests/schema-check.test.ts +++ b/packages/appkit/src/plugins/database/tests/schema-check.test.ts @@ -4,16 +4,20 @@ import type { DataPath } from "../../../database/runtime"; import { defineSchema, fk, id, text } from "../../../database/schema-builder"; import { assertSchemaMatchesDatabase } from "../schema-check"; -const schema = defineSchema(({ table }) => { - const boards = table("boards", { id: id(), title: text().notNull() }); - const notes = table("notes", { - id: id(), - board_id: fk(() => boards.id).notNull(), - author_email: text().private(), - body: text().notNull(), - }); - return { boards, notes }; -}); +// Explicitly public: the assertions name the tables public.boards and public.notes. +const schema = defineSchema( + ({ table }) => { + const boards = table("boards", { id: id(), title: text().notNull() }); + const notes = table("notes", { + id: id(), + board_id: fk(() => boards.id).notNull(), + author_email: text().private(), + body: text().notNull(), + }); + return { boards, notes }; + }, + { schemaName: "public" }, +); type CatalogRow = { table_name: string; @@ -21,10 +25,11 @@ type CatalogRow = { relation_kind: string; }; -/** Answer the catalog query with the given tables and columns. */ +/** Answer the catalog query with the given tables and columns, and the lookup in public with `inPublic`. */ function catalog( tables: Record, kinds: Record = {}, + inPublic: string[] = [], ) { const rows: CatalogRow[] = Object.entries(tables).flatMap( ([name, columns]): CatalogRow[] => @@ -42,10 +47,53 @@ function catalog( relation_kind: kinds[name] ?? "r", })), ); - const raw = vi.fn(async () => rows); + const raw = vi.fn(async (strings: TemplateStringsArray) => + strings.join("?").includes("n.nspname = 'public'") + ? inPublic.map((table_name) => ({ table_name })) + : rows, + ); return { raw, path: { raw } as unknown as DataPath }; } +describe("assertSchemaMatchesDatabase — tables left in public", () => { + const appSchema = defineSchema(({ table }) => ({ + boards: table("boards", { id: id() }), + notes: table("notes", { id: id() }), + })); + + test("every missing table is in public: says so and how to keep them there, not to migrate", async () => { + const { path } = catalog({}, {}, ["boards", "notes"]); + + const error = await assertSchemaMatchesDatabase(path, appSchema).catch( + (caught) => caught, + ); + + expect(error.message).toContain("table app.boards does not exist"); + expect(error.message).toContain( + `Schema public has those tables, but defineSchema() puts tables in schema "app" unless told otherwise. ` + + 'To keep them in public, pass { schemaName: "public" } as the second argument of defineSchema()', + ); + expect(error.message).not.toContain("commit migrations"); + }); + + test("only some missing tables are in public: the usual advice", async () => { + const { path } = catalog({}, {}, ["boards"]); + + await expect(assertSchemaMatchesDatabase(path, appSchema)).rejects.toThrow( + "commit migrations under config/database/migrations", + ); + }); + + test("a schema declared as public never looks in public twice", async () => { + const { raw, path } = catalog({ boards: ["id", "title"] }); + + await expect(assertSchemaMatchesDatabase(path, schema)).rejects.toThrow( + "commit migrations", + ); + expect(raw).toHaveBeenCalledTimes(1); + }); +}); + describe("assertSchemaMatchesDatabase", () => { test("accepts a database with every declared column, and extra ones", async () => { const { path } = catalog({