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

breaking change on uuid column #8002

Closed
Diluka opened this issue Aug 2, 2021 · 8 comments · Fixed by #8343
Closed

breaking change on uuid column #8002

Diluka opened this issue Aug 2, 2021 · 8 comments · Fixed by #8343
Labels
bug regression worked in previous version and doesn't work in current requires triage

Comments

@Diluka
Copy link
Contributor

Diluka commented Aug 2, 2021

Issue Description

update to 0.2.35, the uuid column force to char(36) #7853 , even if set type to varchar in options

Expected Behavior

if set type to varchar, it should be varchar

Actual Behavior

always char, no matter what type is set

export const IDColumnOpts = { type: 'varchar', charset: 'ascii', length: 36 } as const;

export abstract class BaseEntity extends TypeORMBaseEntity {
  @Generated('uuid')
  @PrimaryColumn({ comment: 'ID', ...IDColumnOpts })
  id: string;
}

Steps to Reproduce

// insert code here

My Environment

Dependency Version
Operating System
Node.js version x.y.zzz
Typescript version x.y.zzz
TypeORM version x.y.zzz

Additional Context

Relevant Database Driver(s)

DB Type Reproducible
aurora-data-api no
aurora-data-api-pg no
better-sqlite3 no
cockroachdb no
cordova no
expo no
mongodb no
mysql no
nativescript no
oracle no
postgres no
react-native no
sap no
sqlite no
sqlite-abstract no
sqljs no
sqlserver no

Are you willing to resolve this issue by submitting a Pull Request?

  • ✖️ Yes, I have the time, and I know how to start.
  • ✖️ Yes, I have the time, but I don't know how to start. I would need guidance.
  • ✖️ No, I don’t have the time, but I can support (using donations) development.
  • ✖️ No, I don’t have the time and I’m okay to wait for the community / maintainers to resolve this issue.
@pleerock pleerock added regression worked in previous version and doesn't work in current and removed breaking change labels Aug 10, 2021
@Diluka
Copy link
Contributor Author

Diluka commented Sep 6, 2021

what's the update? I need varchar type.

@Diluka
Copy link
Contributor Author

Diluka commented Sep 6, 2021

why just mysql changed into char however other dbs(which not native support uuid) using varchar

@Diluka
Copy link
Contributor Author

Diluka commented Sep 24, 2021

for mysql, this is like before

import { randomUUID } from 'crypto';
import { BaseEntity as TypeORMBaseEntity, BeforeInsert, Column, PrimaryColumn } from 'typeorm';

export abstract class BaseEntity extends TypeORMBaseEntity {
  @PrimaryColumn({ charset: 'ascii', length: 36 })
  id: string;

  @BeforeInsert()
  generateId() {
    if (this.id == null) {
      this.id = randomUUID();
    }
  }
}

@maitrungduc1410
Copy link

maitrungduc1410 commented Sep 24, 2021

I'm having same issue, after syncing, our production database columns get cleared because of this. Thanks for new release !

@nallenscott
Copy link

Ghosted our nightly DB. No biggie, that's what nightly's for. @pleerock Where does this rank priority-wise? We'd like to take advantage of the latest release.

@knsg16
Copy link

knsg16 commented Nov 4, 2021

The same issue happened to me!

This #7853 is more than breaking change to me because I have a lot of uuid columns which is the primary key in my product.
The migration problem is huge to me.

Please consider the implementation which we can choose varchar or char

@jusfeel
Copy link

jusfeel commented Nov 4, 2021

Just to share some experience.

Basically, the generated migration script will DROP/ADD COLUMN on the keys which can be replaced with MODIFY COLUMN. That's pretty much it for me.

It's indeed troublesome if you have many tables.

@ph1p
Copy link

ph1p commented Nov 9, 2021

Hi! My colleague @jsproede and I had the same problem and since we didn't find any other solution, we wrote a migration. Maybe this helps one or the other.

The migration reads all your entities, finds their primary/foreign keys and modifies the column type.
First we need to disable the foreign key checks, after everything has gone through we need to enable them again.

The good thing about this solution is that no data is lost!

Caution: Use at your own risk!

More: #8167 #7853
Docs: https://github.com/typeorm/typeorm/blob/master/docs/migrations.md

import { MigrationInterface, QueryRunner } from 'typeorm';
import { ColumnMetadata } from 'typeorm/metadata/ColumnMetadata';

export class UUIDMigration implements MigrationInterface {
  name = 'UUIDMigration';

  public async up(queryRunner: QueryRunner): Promise<void> {
    await this.runMigration(queryRunner, 'CHAR');
  }

  public async down(queryRunner: QueryRunner): Promise<void> {
    await this.runMigration(queryRunner, 'VARCHAR');
  }

  private async runMigration(queryRunner: QueryRunner, type: 'VARCHAR' | 'CHAR') {
    await this.disableForeignKeyChecks(queryRunner);

    for (const { tableName, primaryColumns, foreignKeys } of queryRunner.connection.entityMetadatas) {
      for (const columnName of this.getRelevantPrimaryColumnNames(primaryColumns)) {
        await queryRunner.query(`ALTER TABLE ${tableName} MODIFY ${columnName} ${type}(36);`);
      }

      for (const foreignKey of foreignKeys) {
        for (const columnName of this.getRelevantForeignKeyColumnNames(foreignKey.columns)) {
          await queryRunner.query(`ALTER TABLE ${tableName} MODIFY ${columnName} ${type}(36);`);
        }
      }
    }

    await this.enableForeignKeyChecks(queryRunner);
  }

  private getRelevantForeignKeyColumnNames(foreignKeyColumns: ColumnMetadata[]) {
    return foreignKeyColumns.filter(column => column.type === 'uuid').map(column => column.databaseName);
  }

  private getRelevantPrimaryColumnNames(primaryColumns: ColumnMetadata[]) {
    return primaryColumns
      .filter(({ type, length }) => type === 'uuid' || (type === 'varchar' && length === '36'))
      .map(column => column.databaseName);
  }

  private async disableForeignKeyChecks(queryRunner: QueryRunner) {
    await queryRunner.query('SET foreign_key_checks = 0;');
  }

  private async enableForeignKeyChecks(queryRunner: QueryRunner) {
    await queryRunner.query('SET foreign_key_checks = 1;');
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug regression worked in previous version and doesn't work in current requires triage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants