Postgres migration down function doesn't drop fk relations

stephaniesyed commented 10 months ago

Link to reproduction

No response

Describe the Bug

When adding a new table, the down function doesn't drop any foreign key constraints.

For example, this migration file:

import { MigrateUpArgs, MigrateDownArgs } from '@payloadcms/db-postgres'
import { sql } from 'drizzle-orm'

export async function up({ payload }: MigrateUpArgs): Promise<void> {
await payload.db.drizzle.execute(sql`

DO $$ BEGIN
 CREATE TYPE "enum_users_roles" AS ENUM('super-admin', 'admin');
EXCEPTION
 WHEN duplicate_object THEN null;
END $$;

CREATE TABLE IF NOT EXISTS "users_roles" (
    "order" integer NOT NULL,
    "parent_id" integer NOT NULL,
    "value" "enum_users_roles",
    "id" serial PRIMARY KEY NOT NULL
);

CREATE TABLE IF NOT EXISTS "users" (
    "id" serial PRIMARY KEY NOT NULL,
    "full_name" varchar,
    "updated_at" timestamp(3) with time zone DEFAULT now() NOT NULL,
    "created_at" timestamp(3) with time zone DEFAULT now() NOT NULL,
    "email" varchar NOT NULL,
    "reset_password_token" varchar,
    "reset_password_expiration" timestamp(3) with time zone,
    "salt" varchar,
    "hash" varchar,
    "login_attempts" numeric,
    "lock_until" timestamp(3) with time zone
);

CREATE TABLE IF NOT EXISTS "payload_preferences" (
    "id" serial PRIMARY KEY NOT NULL,
    "key" varchar,
    "value" jsonb,
    "updated_at" timestamp(3) with time zone DEFAULT now() NOT NULL,
    "created_at" timestamp(3) with time zone DEFAULT now() NOT NULL
);

CREATE TABLE IF NOT EXISTS "payload_preferences_rels" (
    "id" serial PRIMARY KEY NOT NULL,
    "order" integer,
    "parent_id" integer NOT NULL,
    "path" varchar NOT NULL,
    "users_id" integer
);

CREATE TABLE IF NOT EXISTS "payload_migrations" (
    "id" serial PRIMARY KEY NOT NULL,
    "name" varchar,
    "batch" numeric,
    "updated_at" timestamp(3) with time zone DEFAULT now() NOT NULL,
    "created_at" timestamp(3) with time zone DEFAULT now() NOT NULL
);

CREATE INDEX IF NOT EXISTS "order_idx" ON "users_roles" ("order");
CREATE INDEX IF NOT EXISTS "parent_idx" ON "users_roles" ("parent_id");
CREATE INDEX IF NOT EXISTS "created_at_idx" ON "users" ("created_at");
CREATE UNIQUE INDEX IF NOT EXISTS "email_idx" ON "users" ("email");
CREATE INDEX IF NOT EXISTS "created_at_idx" ON "payload_preferences" ("created_at");
CREATE INDEX IF NOT EXISTS "order_idx" ON "payload_preferences_rels" ("order");
CREATE INDEX IF NOT EXISTS "parent_idx" ON "payload_preferences_rels" ("parent_id");
CREATE INDEX IF NOT EXISTS "path_idx" ON "payload_preferences_rels" ("path");
CREATE INDEX IF NOT EXISTS "created_at_idx" ON "payload_migrations" ("created_at");
DO $$ BEGIN
 ALTER TABLE "users_roles" ADD CONSTRAINT "users_roles_parent_id_users_id_fk" FOREIGN KEY ("parent_id") REFERENCES "users"("id") ON DELETE cascade ON UPDATE no action;
EXCEPTION
 WHEN duplicate_object THEN null;
END $$;

DO $$ BEGIN
 ALTER TABLE "payload_preferences_rels" ADD CONSTRAINT "payload_preferences_rels_parent_id_payload_preferences_id_fk" FOREIGN KEY ("parent_id") REFERENCES "payload_preferences"("id") ON DELETE cascade ON UPDATE no action;
EXCEPTION
 WHEN duplicate_object THEN null;
END $$;

DO $$ BEGIN
 ALTER TABLE "payload_preferences_rels" ADD CONSTRAINT "payload_preferences_rels_users_id_users_id_fk" FOREIGN KEY ("users_id") REFERENCES "users"("id") ON DELETE cascade ON UPDATE no action;
EXCEPTION
 WHEN duplicate_object THEN null;
END $$;
`);

};

export async function down({ payload }: MigrateDownArgs): Promise<void> {
await payload.db.drizzle.execute(sql`

DROP TABLE "users_roles";
DROP TABLE "users";
DROP TABLE "payload_preferences";
DROP TABLE "payload_preferences_rels";
DROP TABLE "payload_migrations";`);

};

Can migrate fine, but throws an error when trying to run migrate:down Terminal output:

stephanie.syed@LXYWG0WJ6C partner-manager % make db-fresh                            
Running in docker: yarn run payload migrate:fresh
yarn run v1.22.19
$ cross-env PAYLOAD_CONFIG_PATH=src/payload.config.ts payload migrate:fresh
[16:40:31] INFO (payload): Starting Payload...
✔ WARNING: This will drop your database and run all migrations. Are you sure you want to proceed? … yes
[16:40:33] INFO (payload): Dropping database.
[16:40:33] INFO (payload): Reading migration files from /home/node/app/src/migrations
[16:40:33] INFO (payload): Migrating: 20231222_164027_inital_migration
[16:40:33] INFO (payload): Migrated:  20231222_164027_inital_migration (73ms)
[16:40:33] INFO (payload): Done.
Done in 2.47s.
stephanie.syed@LXYWG0WJ6C partner-manager % 
stephanie.syed@LXYWG0WJ6C partner-manager % make db-down                             
Running in docker: yarn run payload migrate:down
yarn run v1.22.19
$ cross-env PAYLOAD_CONFIG_PATH=src/payload.config.ts payload migrate:down
[16:40:38] INFO (payload): Starting Payload...
[16:40:38] INFO (payload): Reading migration files from /home/node/app/src/migrations
[16:40:38] INFO (payload): Rolling back batch 1 consisting of 1 migration(s).
[16:40:38] INFO (payload): Migrating down: 20231222_164027_inital_migration
[16:40:38] ERROR (payload): Error migrating down 20231222_164027_inital_migration. Rolling back. cannot drop table users because other objects depend on it.
    err: {
      "type": "DatabaseError",
      "message": "cannot drop table users because other objects depend on it",
      "stack":
          error: cannot drop table users because other objects depend on it
              at /home/node/app/node_modules/pg-pool/index.js:45:11
              at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
      "length": 301,
      "name": "error",
      "severity": "ERROR",
      "code": "2BP01",
      "detail": "constraint payload_preferences_rels_users_id_users_id_fk on table payload_preferences_rels depends on table users",
      "hint": "Use DROP ... CASCADE to drop the dependent objects too.",
      "file": "dependency.c",
      "line": "997",
      "routine": "reportDependentObjects"
    }
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
make: *** [db-down] Error 1

I can manually edit the down function to either cascade on table drop or to drop constraints first before trying to drop the tables. This could be generalized to 1) adding fk drops and 2) ordering the down function in the opposite order of the up function .

While adding cascade would work in this case, I think more edge cases would be covered if the down sql statements were the opposite of the migration statements in reverse order (since the current migration file does table creation then foreign key creation).

Alternatively, it could be worth investigating the scope of using Drizzle's built in migration generation for the postgres adapter (there could be more edge cases they already handle gracefully)

To Reproduce

create a migration with migrate:create 'inital_migration' that creates a table with foreign keys (the payload metadata does this, so shouldn't be necessary to create models with relationships)
apply that migration with migrate
attempt to downgrade with migrate:down (see the terminal output below for the error output)

In the terminal output below, I actually used migrate:fresh since my local db was in a bad state.

stephanie.syed@LXYWG0WJ6C partner-manager % make db-fresh                            
Running in docker: yarn run payload migrate:fresh
yarn run v1.22.19
$ cross-env PAYLOAD_CONFIG_PATH=src/payload.config.ts payload migrate:fresh
[16:40:31] INFO (payload): Starting Payload...
✔ WARNING: This will drop your database and run all migrations. Are you sure you want to proceed? … yes
[16:40:33] INFO (payload): Dropping database.
[16:40:33] INFO (payload): Reading migration files from /home/node/app/src/migrations
[16:40:33] INFO (payload): Migrating: 20231222_164027_inital_migration
[16:40:33] INFO (payload): Migrated:  20231222_164027_inital_migration (73ms)
[16:40:33] INFO (payload): Done.
Done in 2.47s.
stephanie.syed@LXYWG0WJ6C partner-manager % 
stephanie.syed@LXYWG0WJ6C partner-manager % make db-down                             
Running in docker: yarn run payload migrate:down
yarn run v1.22.19
$ cross-env PAYLOAD_CONFIG_PATH=src/payload.config.ts payload migrate:down
[16:40:38] INFO (payload): Starting Payload...
[16:40:38] INFO (payload): Reading migration files from /home/node/app/src/migrations
[16:40:38] INFO (payload): Rolling back batch 1 consisting of 1 migration(s).
[16:40:38] INFO (payload): Migrating down: 20231222_164027_inital_migration
[16:40:38] ERROR (payload): Error migrating down 20231222_164027_inital_migration. Rolling back. cannot drop table users because other objects depend on it.
    err: {
      "type": "DatabaseError",
      "message": "cannot drop table users because other objects depend on it",
      "stack":
          error: cannot drop table users because other objects depend on it
              at /home/node/app/node_modules/pg-pool/index.js:45:11
              at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
      "length": 301,
      "name": "error",
      "severity": "ERROR",
      "code": "2BP01",
      "detail": "constraint payload_preferences_rels_users_id_users_id_fk on table payload_preferences_rels depends on table users",
      "hint": "Use DROP ... CASCADE to drop the dependent objects too.",
      "file": "dependency.c",
      "line": "997",
      "routine": "reportDependentObjects"
    }
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
make: *** [db-down] Error 1

The only model defined was User, as shown below (simplified for this example):

import type { CollectionConfig } from 'payload/types'

export const Users: CollectionConfig = {
  slug: 'users',
  auth: true,
  admin: {
    useAsTitle: 'email',
  },
  fields: [
    {
      name: 'fullName',
      type: 'text',
    },
    {
      name: 'roles',
      type: 'select',
      hasMany: true,
      required: true,
      options: [
        { //users with cross-partner access
          label: 'Super Admin', 
          value: 'super-admin',
        },
        { // partner's employees
          label: 'Admin',
          value: 'admin',
        },
      ],
      defaultValue: ['admin'],
    },
  ],
}

export default Users

Payload Version

2.4.0

Adapters and Plugins

none

DanRibbens commented 9 months ago

Alternatively, it could be worth investigating the scope of using Drizzle's built in migration generation for the postgres adapter (there could be more edge cases they already handle gracefully)

We are simply relying on drizzle-kit and how it generates these migrations. I was hopeful that our recent version upgrade might improve this.

Since the DROP TABLEs comes straight from Drizzle, I don't know that this is going to be handled by Payload.

I do think it is okay to use the generated migrations as a first pass and modify according to your own needs.

What do you think?

stephaniesyed commented 9 months ago

Hi Dan, sorry for the confusion. On a previous bug ticket, something I would have expected to be handled by Drizzle (since they do provide migration support) was implemented by payload instead, so I wasn't sure what was Drizzle and what was Payload.

You're right, to mitigate the issue we can modify the file after it is generated, so this isn't a blocking/high priority bug. However, I do think it is still a bug since I expect modifying foreign keys to be a basic SQL migration pattern. I am also biased in this regard as I have used other ORMs for different languages that had fewer issues, and because of that I haven't had to write a manual migration in a while 😅

From my perspective, Payload provides postgres support, so bugs in that functionality are bugs in Payload, even if the root cause is in a dependency. I also think that Drizzle might be more inclined to address bugs reported by other companies using their code rather than individuals. They have a bit of a backlog at the moment so a ticket coming from Payload (and therefore affecting all payload postgres users) might have a bit more weight, especially since Payload is listed as a gold sponsor on their ORM repo's readme.

However, I know the Payload team is small and postgres support isn't part of the monetized product. If the team isn't able to support reporting bugs to dependencies I can submit bugs in migration file generation directly to Drizzle instead.

DanRibbens commented 9 months ago

@stephaniesyed I appreciate the understanding here!

We will keep this issue open while we monitor the Drizzle issue. That gives the best visibility for anyone looking for the problem: https://github.com/drizzle-team/drizzle-orm/issues/1144

AndriiSherman commented 9 months ago

Working on resolving this issue on the Drizzle side

Snailedlt commented 6 months ago

@AndriiSherman Great! Let us know if you need us to test anything :)

payloadcms / payload