Prevent errors when synchronizing Sqlite or MSSql DBs with simple-enum types

[louisremi]

Issue type:

[ ] question
[x] bug report
[ ] feature request
[ ] documentation issue

Database system/driver:

[ ] cordova
[ ] mongodb
[ ] mssql
[ ] mysql / mariadb
[ ] oracle
[ ] postgres
[ ] cockroachdb
[x] sqlite
[ ] sqljs
[ ] react-native
[ ] expo

TypeORM version:

[x] latest
[ ] @next
[ ] 0.x.x (or put your version here)

Steps to reproduce:

I tried to use the new (and undocumented) simple-enum type with sqlite. I'm surprised to see that Typeorm is able to synchronize only once when the table hasn't been created yet. Once the table has been created, synchronizing throws with the following error message:

{ QueryFailedError: SQLITE_ERROR: near "-": syntax error
    at new QueryFailedError (/home/louisremi/Workspace/monbelappart/src/error/QueryFailedError.ts:9:9)
    at handler (/home/louisremi/Workspace/monbelappart/src/driver/sqlite/SqliteQueryRunner.ts:53:26)
    at replacement (/home/louisremi/Workspace/monbelappart/node_modules/sqlite3/lib/trace.js:19:31)
    at Statement.errBack (/home/louisremi/Workspace/monbelappart/node_modules/sqlite3/lib/sqlite3.js:16:21)
  message: 'SQLITE_ERROR: near "-": syntax error',
  errno: 1,
  code: 'SQLITE_ERROR',
  name: 'QueryFailedError',
  query: 'CREATE TABLE "temporary_room" ("id" varchar PRIMARY KEY NOT NULL, "reference" varchar NOT NULL, "name" varchar NOT NULL, "floorArea" integer NOT NULL, "status" simple-enum CHECK( status IN (\'draft\',\'active\',\'maintenance\',\'archived\',\'deleted\') ) NOT NULL DEFAULT (\'draft\'), "createdAt" datetime NOT NULL DEFAULT (datetime(\'now\')), "updatedAt" datetime NOT NULL DEFAULT (datetime(\'now\')), CONSTRAINT "UQ_8c1de74045cb8a30adbb6614f7c" UNIQUE ("reference"))',
  parameters: [] }

I am a complete noob with Typeorm, but from poking around the source code, here's what I'm guessing is going wrong:

• at some point loadTables is executed to detect a difference between the entity and the actual Table layout.
• for some reason, the person who added simple-enum support wanted loadTables() to return simple-enum instead of the actual varchar type that was used when creating the table in sqlite, see

  typeorm/src/driver/sqlite-abstract/AbstractSqliteQueryRunner.ts

  Line 818 in 63d993b

  if (tableColumn.type === "varchar") {

  This code block is troubling because there is no such "inverse conversion" for other simple-* types such as simple-json and simple-array
• because of this, two problems happen: Typeorm incorrectly detects a difference between the entity and the table layout, so it tries to recreate the table… and this time uses simple-enum as the column type in the query, which causes the above error.

I removed the "inverse conversion" code blocks in both Sqlite and MSSql query runners, and I removed the tests that were expecting findColumnByName to return the entity definition instead of the actual table layout. Again, the reason I did this is because the implementation and tests of simple-array and simple-json don't have this behavior and assertions… and also because using simple-enum only appears to work in sqlite when I remove this code.

I'm willing to add documentation for simple-enum to this PR if you are OK with the proposed changes.

[pleerock]

We can't simply remove exist line of code.. I appreciate research you did and possible bugs you found, but please ping author of those changes and ask him why he added them here.

[louisremi]

Hello @borremosch, you are the author of #3700 and your input here would be very much appreciated :-)

Do you remember the rationale for adding the "inverse transformation" of "varchar + CHECK()" back to simple-enum? This appears to cause errors when synchronizing with SQLite. Such behavior wasn't implemented for simple-json and simple-array, so I'm assuming you have been overzealous here, and I suggest this behavior should be removed for simple-enum. What do you think?

[louisremi]

Hello again @borremosch, your input on this issue ↑ is more than welcome :-)

[borremosch]

Hey @louisremi, apologies for the delay, I have been quite busy lately. I have taken a quick look and it looks like you might be right. I thought it would be nice if Typeorm would infer certain check statements back to a simple enum so that's probably why I added the code for the inverse transformation, but if this is causing issues then it is probably a good idea to remove that part of the code.

[pleerock]

can you tell why did you remove those checks for data equality?

[louisremi]

It isn't data equality that is checked here. Data equality is still tested on line 118. These tests were written to ensure that findByColumnName("simpleEnum") returned the original definition of the simple-enum, (instead of the actual nvarchar used by Sqlite). This is the behavior I am proposing to fix. So these tests are no longer relevant.

[imnotjames]

Please add a test that confirms that this change allows the intended behavior to work as expected. I think it's that when you do a second sync it will work correctly?

And that changes to the enum are still reflected - unless there's already a test for that.