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

Added simple-enum column type #3700

Merged
merged 2 commits into from
Feb 25, 2019
Merged

Added simple-enum column type #3700

merged 2 commits into from
Feb 25, 2019

Conversation

borremosch
Copy link
Contributor

I have added the simple-enum column type discussed in #1414. It works like this:

  • For MySQL, MariaDB and Postgres, the native ENUM type is used
  • For SQLite and SQL Server, a CHECK() constraint is used, like so:
      colname CHECK (colname in ('A','B','C')) NOT NULL DEFAULT ('A')
    
    The CHECK is also reverse engineered back into an enum when code is being generated from a DB schema
  • For Oracle, I found that enum is currently not supported in TypeORM. Since Oracle does have a native ENUM type, I believe simple-enum should be based on it. I have not added simple-enum for Oracle.

@@ -288,6 +290,10 @@ export abstract class AbstractSqliteDriver implements Driver {

} else if (columnMetadata.type === "simple-json") {
value = DateUtils.stringToSimpleJson(value);

} else if ( columnMetadata.type === "simple-enum" ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since sqlite supports enum now, it should have enum type as well

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, don't forget to list new types in the docs (website)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or using CHECK constraint in sqlite can't be called enum ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot find native enum support on the SQLite website: https://www.sqlite.org/datatype3.html.

I would say it makes sense to have enum for native enum types, and simple-enum for the emulated enum type. That way it will be more clear when you are using a derived/emulated type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is currently no section on either enums or simple- types on the website. Do you want me to create one in a pull request?

Also, are you aware that the website certificate is invalid when accessing it from https://typeorm.io/?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, are you aware that the website certificate is invalid when accessing it from https://typeorm.io/?

yes =(

There is currently no section on either enums or simple- types on the website. Do you want me to create one in a pull request?

yes feel free please

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 other simple- types are now mentioned, but simple-enum is not:
https://typeorm.io/entities#simple-array-column-type
#10077

@pleerock
Copy link
Member

Great work! Asking @AlexMesser for review

@pleerock pleerock merged commit d9f5581 into typeorm:master Feb 25, 2019
@pleerock
Copy link
Member

Thank you so much!

@toonvanstrijp
Copy link

@borremosch thanks! :)

@JVMartin
Copy link
Contributor

JVMartin commented Feb 14, 2020

Found a bug in this PR, that prevents me from using a protected word like "from" as the column name. For example:

export enum FromType {
  NO_REPLY = 'no-reply',
  RANDOM = 'random',
}

@Entity('broadcast')
class BroadcastEntity {
  @PrimaryGeneratedColumn()
  id: number;

  @Column({
    type: 'simple-enum',
    enum: FromType,
    nullable: false,
  })
  from: FromType;
}

Here's the error:

  console.log ../node_modules/typeorm/platform/PlatformTools.js:200
    query failed: CREATE TABLE "broadcast" ("id" integer PRIMARY KEY AUTOINCREMENT NOT NULL, "from" varchar CHECK( from IN ('no-reply','random') ) NOT NULL)

  console.log ../node_modules/typeorm/platform/PlatformTools.js:200
    error: [Error: SQLITE_ERROR: near "from": syntax error] {
      errno: 1,
      code: 'SQLITE_ERROR'
    }

As you can see, the generated column:
"from" varchar CHECK( from IN ('no-reply','random') ) NOT NULL
is missing the double quotes that should be surrounding the column name, and so a protected keyword like from cannot be used.

It should look like this:
"from" varchar CHECK( "from" IN ('no-reply','random') ) NOT NULL

(The column name must be wrapped in double-quotes.)

This was in sqlite, if that matters.

bobbywang000 added a commit to bobbywang000/nemosyne that referenced this pull request Jun 5, 2020
Note that we use `simple-enum` instead of `enum`
because sqlite doesn't support enum, but this PR
allows a workaround by adding a `simple-enum` type.

typeorm/typeorm#3700
bobbywang000 added a commit to bobbywang000/nemosyne that referenced this pull request Jun 5, 2020
Note that we use `simple-enum` instead of `enum`
because sqlite doesn't support enum, but this PR
allows a workaround by adding a `simple-enum` type.

typeorm/typeorm#3700
@opgbaudouin
Copy link

opgbaudouin commented Jun 23, 2020

When using simple-enum and using 'synchronise' in the connection options i get a continuous "column has changed errors" (i.e. it keeps ALTER table).

   //simple-enum causes TypeORM to think columns have always changed...
   @Column({name: 'type'/*, type: 'simple-enum', enum: ProjectType*/}) 
   @IsEnum(ProjectType) //class-validator
    type: ProjectType;

Removing it naturally removes the CHECK IN feature but there is some error in detecting if a column schema is the same and should be altered.

This is in TypeORM 0.2.25 and SQLite 3 adapter 4.2.0.

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

Successfully merging this pull request may close these issues.

6 participants