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

Migration - unecessary sql statements #1096

Closed
ml1nk opened this issue Nov 19, 2020 · 6 comments
Closed

Migration - unecessary sql statements #1096

ml1nk opened this issue Nov 19, 2020 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@ml1nk
Copy link
Contributor

ml1nk commented Nov 19, 2020

Describe the bug
Every migration in my env generates the following statements:

    this.addSql('alter table "core_task" drop constraint if exists "core_task_state_check";');
    this.addSql('alter table "core_task" alter column "state" type int2 using ("state"::int2);');
    this.addSql('alter table "core_task" drop constraint if exists "core_task_origin_check";');
    this.addSql('alter table "core_task" alter column "origin" type int2 using ("origin"::int2);');

    this.addSql('alter table "directory_contact" drop constraint if exists "directory_contact_book_object_id_check";');
    this.addSql('alter table "directory_contact" alter column "book_object_id" type int4 using ("book_object_id"::int4);');
    this.addSql('alter table "directory_contact" alter column "book_object_id" set not null;');

    this.addSql('alter table "pbx_sip" drop constraint if exists "pbx_sip_status_check";');
    this.addSql('alter table "pbx_sip" alter column "status" type int2 using ("status"::int2);');

    this.addSql('alter table "pbx_tenant" drop constraint if exists "pbx_tenant_status_check";');
    this.addSql('alter table "pbx_tenant" alter column "status" type int2 using ("status"::int2);');

These are clearly unecessary, for example core_task.state is already int2 and executing does not change anything. The bug persists since i'm using mikro-orm (4.x.x) and until now only occurs when using enums.

To Reproduce

  @Enum({ items: () => CoreTaskStateEnum, default: CoreTaskStateEnum.Queued })
  state!: CoreTaskStateEnum
export enum CoreTaskStateEnum {
  Queued,
  Running,
  Waiting,
  Continue,
  Finished,
  Failed,
  Cancelled
}

Generate a migration after applying initial migration (metadataProvider: TsMorphMetadataProvider).

Expected behavior
No unecessary/duplicate sql statement.

Versions

| Dependency | Version |
| node | 15.2.1 |
| typescript | 4.0.5 |
| mikro-orm | 4.3.0 |
| pg | 8.5.1 | (server 12)

@B4nan B4nan added the bug Something isn't working label Nov 19, 2020
@visurel
Copy link

visurel commented Dec 3, 2020

Experiencing the same. It also generates these unnecessary migrations when specifying the columnType explicitly. e.g.

@Enum({
  items: () => CoreTaskStateEnum, 
  columnType: 'int2' // also tried 'smallint'
})
state!: CoreTaskStateEnum

Using ReflectMetadataProvider
@mikro-orm/core: 4.3.2
@mikro-orm/postgresql: 4.3.2
@mikro-orm/migrations: 4.3.2

@B4nan
Copy link
Member

B4nan commented Dec 4, 2020

This is most probably because you are passing the items there. The ORM tries to diff the enum items, but it is a numeric enum, you don't really need to pass items as long as you specify the column type.

This will work fine:

@Enum({ columnType: 'int2' })
state!: CoreTaskStateEnum

@ml1nk ml1nk closed this as completed Dec 4, 2020
@B4nan
Copy link
Member

B4nan commented Dec 4, 2020

Let's keep this open, it should work like this automatically.

@B4nan B4nan reopened this Dec 4, 2020
@visurel
Copy link

visurel commented Dec 4, 2020

Just had another occurence of this issue:
When using

@Enum(() => StringEnum)
type!: StringEnum

with

export enum StringEnum {
  TEST= 'TEST'
}

and the String Based Enum only has 1 value, it emits:

    this.addSql('alter table "test_table" drop constraint if exists "test_table_type_check";');
    this.addSql('alter table "test_table" alter column "type" type text using ("type"::text);');
    this.addSql('alter table "test_table" add constraint "test_table_type_check" check ("type" in (\'TEST\'));');

when you add a second value it stops emitting the Migration.

If you change the enum to be like this, it stops emitting the unnecessary Migrations:

export enum StringEnum {
  TEST= 'TEST',
  ANOTHER = 'ANOTHER'
}

@B4nan
Copy link
Member

B4nan commented Dec 4, 2020

@visurel this issue is about numeric enums, string based enums should be well tested:

https://github.com/mikro-orm/mikro-orm/blob/master/tests/SchemaGenerator.test.ts#L289
https://github.com/mikro-orm/mikro-orm/blob/master/tests/__snapshots__/SchemaGenerator.test.ts.snap#L1495

So if you see similar issue there, it would be great to create separate reproduction for that.

@B4nan B4nan closed this as completed in e02ffea Dec 4, 2020
@B4nan
Copy link
Member

B4nan commented Dec 4, 2020

Fixed in 4.3.3-dev.19, the issue was with const enums - those without explicit values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants