Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugs with native postgresql enums #5108

Closed
5 tasks done
stefansundin opened this issue Jan 9, 2024 · 2 comments
Closed
5 tasks done

Bugs with native postgresql enums #5108

stefansundin opened this issue Jan 9, 2024 · 2 comments

Comments

@stefansundin
Copy link
Contributor

Describe the bug

I'm testing out the new native postgresql enums and I have encountered a few problems.

1. down migration doesn't drop type

Native enums still get non-native alter table "table_name" drop constraint if exists "nonexistent_check_constraint" instead of a drop type statement.

This also makes it hard to roll back and then roll forward since you'll get an error that the type already exists:

$ yarn mikro-orm migration:up
$ yarn mikro-orm migration:down
$ yarn mikro-orm migration:up
DriverException: create type "user_role" as enum ('Member', 'Admin'); - type "user_role" already exists

And apparently create type doesn't accept an if not exists clause so we can't work around it that way.

2. Can't convert non-native enum with a default to native enum

Trying to convert a non-native enum with a default (i.e. it is a text column with a constraint) will result in this SQL:

    this.addSql('create type "org_plan" as enum (\'Free\', \'Business\', \'Enterprise\');');
    this.addSql('alter table "org" alter column "plan" type "org_plan" using ("plan"::"org_plan");');

Which will result in this error:

DriverException: alter table "org" alter column "plan" type "org_plan" using ("plan"::"org_plan"); - default for column "plan" cannot be cast automatically to type org_plan

AFAIK, we have to use drop default and then set default:

    this.addSql('alter table "org" alter column "plan" drop default;');
    this.addSql('create type "org_plan" as enum (\'Free\', \'Business\', \'Enterprise\');');
    this.addSql('alter table "org" alter column "plan" type "org_plan" using ("plan"::"org_plan");');
    this.addSql('alter table "org" alter column "plan" set default \'Free\';');

This also has to be done in the down migration otherwise the column type will be text but the default will be the enum type (I didn't think this was possible but it is).

Take this property:

enum OrgPlan {
  Free = 'Free',
  Business = 'Business',
  Enterprise = 'Enterprise',
}

@Entity()
export class Org {
  // ... stuff omitted ...

  @Enum({
    nativeEnumName: 'org_plan', // <--- this line was added for the migration
    items: () => OrgPlan,
    default: OrgPlan.Free,
  })
  plan: OrgPlan = OrgPlan.Free;
}

Running migration:create creates the following migration:

export class Migration20240109192818_ConvertOrgPlanToNativeEnum extends Migration {

  async up(): Promise<void> {
    this.addSql('alter table "org" drop constraint if exists "org_plan_check";');

    this.addSql('create type "org_plan" as enum (\'Free\', \'Business\', \'Enterprise\');');
    this.addSql('alter table "org" alter column "plan" type "org_plan" using ("plan"::"org_plan");');
  }

  async down(): Promise<void> {
    this.addSql('alter table "org" drop constraint if exists "org_plan_check";');

    this.addSql('alter table "org" alter column "plan" type text using ("plan"::text);');
    this.addSql('alter table "org" add constraint "org_plan_check" check ("plan" in (\'Free\', \'Business\', \'Enterprise\'));');
  }

}

But this is what we need:

export class Migration20240109192818_ConvertOrgPlanToNativeEnum extends Migration {

  async up(): Promise<void> {
    this.addSql('alter table "org" drop constraint if exists "org_plan_check";');

    this.addSql('alter table "org" alter column "plan" drop default;');
    this.addSql('create type "org_plan" as enum (\'Free\', \'Business\', \'Enterprise\');');
    this.addSql('alter table "org" alter column "plan" type "org_plan" using ("plan"::"org_plan");');
    this.addSql('alter table "org" alter column "plan" set default \'Free\';');
  }

  async down(): Promise<void> {
    this.addSql('alter table "org" alter column "plan" drop default;');
    this.addSql('alter table "org" alter column "plan" type text using ("plan"::text);');
    this.addSql('alter table "org" add constraint "org_plan_check" check ("plan" in (\'Free\', \'Business\', \'Enterprise\'));');
    this.addSql('alter table "org" alter column "plan" set default \'Free\';');

    this.addSql('drop type "org_plan";');
  }

}

3. Rollback issue when adding to an enum

Take this example, adding SelfHosted to the enum above:

export class Migration20240109203600_AddSelfHostedOrgPlan extends Migration {

  async up(): Promise<void> {
    this.addSql('alter type "org_plan" add value \'SelfHosted\';');
  }

  async down(): Promise<void> {
    // this is wrong but does not break things
    this.addSql('alter table "org" drop constraint if exists "org_plan_check";');
  }

}

If you try to roll back and then migrate again you will get this error:

$ yarn mikro-orm migration:up
$ yarn mikro-orm migration:down
$ yarn mikro-orm migration:up
DriverException: alter type "org_plan" add value 'SelfHosted'; - enum label "SelfHosted" already exists

Adding if not exists to the alter type statement will fix this issue.

4. Can't remove values from the enum

Since PostgreSQL does not allow removals from types, this may be considered an intended limitation. But there are ways to work around it, and other frameworks do implement this.

By renaming the existing type and creating the new type, we will be able to remove an entry from the enum.

export class Migration20240109204535_RemoveEnterpriseOrgPlan extends Migration {

  async up(): Promise<void> {
    this.addSql(`alter type "org_plan" rename to "org_plan_old"`);
    this.addSql(`create type "org_plan" as enum('Free', 'Business')`);
    this.addSql(`alter table "org" alter column "plan" drop default;`);
    this.addSql(`alter table "org" alter column "plan" type "org_plan" using "plan"::"text"::"org_plan";`);
    this.addSql(`alter table "org" alter column "plan" set default 'Free';`);
    this.addSql('drop type "org_plan_old";');
  }

  async down(): Promise<void> {
    this.addSql(`create type "org_plan_old" as enum('Free', 'Business', 'Enterprise')`);
    this.addSql(`alter table "org" alter column "plan" drop default;`);
    this.addSql(`alter table "org" alter column "plan" type "org_plan_old" using "plan"::"text"::"org_plan_old";`);
    this.addSql(`alter table "org" alter column "plan" set default 'Free';`);
    this.addSql('drop type "org_plan";');
    this.addSql(`alter type "org_plan_old" rename to "org_plan"`);
  }

}

This should preferably only be used when removing things from the enum, because using this for adding to the enum may make the migration unnecessarily expensive if the table is very very large. I think adding to a type is super cheap the way it is now.

Not the end of the world if this wasn't supported in mikro-orm.

5. Should multiple columns be able to use the same type?

This might be overkill but wouldn't it be cool if you could use the same type for both of these columns?

enum UserRole {
  Member = 'Member',
  Admin = 'Admin',
}

@Entity()
export class Org {
  // ... stuff omitted ...

  @Enum({
    nativeEnumName: 'user_role',
    items: () => UserRole,
    default: UserRole.Member,
  })
  defaultUserRole: UserRole = UserRole.Member;
}

@Entity()
export class User {
  // ... stuff omitted ...

  @Enum({
    nativeEnumName: 'user_role',
    items: () => UserRole,
    default: UserRole.Member,
  })
  role: UserRole = UserRole.Member;
}

Right now it will try to create the type twice, which will fail.

If this was allowed then the user would run into trouble when attempting to remove one of the columns.

Reproduction

I hope the code above is enough to repro. Please let me know if any of it is unclear.

What driver are you using?

@mikro-orm/postgresql

MikroORM version

6.0.1

Node.js version

v18.19.0

Operating system

No response

Validations

@B4nan
Copy link
Member

B4nan commented Jan 9, 2024

Thanks for the write-up, I should have spent more time testing this, heh.

@B4nan B4nan closed this as completed in 49d6b4d Feb 4, 2024
@B4nan
Copy link
Member

B4nan commented Feb 4, 2024

Ok, I guess I've addressed most of this, exceptions are removing of type values and removing types, that can come later. Let me know how it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants