mikro-orm / mikro-orm

TypeScript ORM for Node.js based on Data Mapper, Unit of Work and Identity Map patterns. Supports MongoDB, MySQL, MariaDB, MS SQL Server, PostgreSQL and SQLite/libSQL databases.
https://mikro-orm.io
MIT License
7.79k stars 543 forks source link

property: Date & Opt; translates to varchar in migration #5260

Closed bernardojdias closed 8 months ago

bernardojdias commented 8 months ago

Describe the bug

Hello.

I am using @mikro-orm/entity-generator with @mikro-orm/migrations to create a small program to generate the migrations at runtime and update the database according to the output of the entity-generator schemas.

The databases are not all equal and this is a way that I found to update them without having to do it manually since we have a lot of changes.

When I run the entity-generator it creates the date properties as expected but it also adds a lenght: 0 option and the Opt type. Check both nullable and not nullable property expressions bellow: @Property({ length: 0, defaultRaw: 'CURRENT_TIMESTAMP' }) dateCreate!: Date & Opt;

@Property({ length: 0, nullable: true, defaultRaw: 'CURRENT_TIMESTAMP' }) dateCreate?: Date;

With the expressions above, the schema generated converts the Date type properties to varchar when I generate the migrations as follows bellow: modify 'date_create' varchar(0) not null default CURRENT_TIMESTAMP

As it says in the documentation ...we use the Opt type to intersect with the property type to tell the ORM (on type level)... so it should not change the database type at all.

After some investigations, I am convinced that there are 3 issues:

  1. The @mikro-orm/entity-generator is adding the length: 0 to the Date types
  2. It's also making non nullable fields in the database with a default value as Optional in the generated schema (not 100% sure yet)
  3. When the @mikro-orm/migrations generates the query, translates Date & Opt to varchar

I tested the migration generation with both MySql and Postrgresql drivers, and both had the same issue.

Notes:

Thanks in advance!

Reproduction

https://github.com/bernardojdias/mikro-orm-reproduction

Steps to reproduce:

If all the tests run with success, it means that you've reproduced the issue

Note: Due to SQLite column data types not including datetime, I had to use MySql driver in the reproduction example

What driver are you using?

@mikro-orm/mysql

MikroORM version

6.1.4

Node.js version

Node.js: 18.17.1 | Typescript: 5.2.2

Operating system

Windows

Validations

B4nan commented 8 months ago

The @mikro-orm/entity-generator is adding the length: 0 to the Date types

That sounds expected to me, 0 is in fact the default length for dates in all drivers except postgres.

It's also making non nullable fields in the database with a default value as Optional in the generated schema (not 100% sure yet)

That's correct behavior as well, the Opt type is exactly for such properties.

When the @mikro-orm/migrations generates the query, translates Date & Opt to varchar

It works only if you have the runtime default, as the ORM will use that to infer the type. Otherwise, reflect metadata can't provide this type, anything more than a simple scalar won't work with it (similarly, using union type breaks it too, e.g. with Date | null.

So the metadata reflection part is a wontfix, you need to provide the type option explicitly (or use ts-morph). But surely, the entity generator shouldn't suffer from this problem, if that's the case, we need to print the type there explicitly.

boenrobot commented 8 months ago

But surely, the entity generator shouldn't suffer from this problem, if that's the case, we need to print the type there explicitly.

There exists the scalarTypeInDecorator option, exactly for this purpose.

B4nan commented 8 months ago

Actually, you should be able to enforce the scalar types in the decorator options already via scalarTypeInDecorator option

edit: damn you were faster :D but we should really make this implicit for the properties where we use Opt type (or other intersection types)

boenrobot commented 8 months ago

I am using @mikro-orm/entity-generator with @mikro-orm/migrations to create a small program to generate the migrations at runtime and update the database according to the output of the entity-generator schemas.

btw, I've been meaning to write a test fixture that does just that, so @bernardojdias, if there's any chance you could make a PR with exactly that, and maybe include one or two passing tests, we can then greatly accelerate the compatibility between the entity generator and the migrations generator.

That is, we need a test that gets a schema definition as a string, imports it, generates entities out of it... similar to entity generator tests so far... and then from a different connection creates a new schema from an initial migration using the newly generated entities... and finally compares the two schemas (the resulting information_schema rows that is), expecting everything to be equal.

bernardojdias commented 8 months ago

There exists the scalarTypeInDecorator option, exactly for this purpose.

That did the trick, thanks! While reading the documentation I overlooked that option, sorry!

That sounds expected to me, 0 is in fact the default length for dates in all drivers except postgres.

For the 0 length I made a workaround to prevent the property to be inserted into the decorator by adding a check in onInitialMetadata option that checks the property type and assigning the prop.length to undefined.

I know it's a bit sketchy but it's the only way that I found to prevent the length property to the decorator and subsequently making the migrator "confused".

With the scalarTypeInDecorator option this workaround is no longer needed for the migrator but it can be used to prevent the length property to be added to the decorator in types like the Date as it, technically, doesn't have length.

B4nan commented 8 months ago

assigning the prop.length to undefined.

But that is exactly the same as having it set to 0, or not?

If omitted, the default precision is 0.

https://dev.mysql.com/doc/refman/8.0/en/date-and-time-type-syntax.html

bernardojdias commented 8 months ago

so @bernardojdias, if there's any chance you could make a PR

I've been a bit low on time but I will try to make one.

But that is exactly the same as having it set to 0, or not?

It is actually. I could swear that if I left the length in the decorator the migrator would translate it to varchar(0), but I was wrong. Sorry about that!

boenrobot commented 8 months ago

we should really make this implicit for the properties where we use Opt type (or other intersection types)

@B4nan On a related note, should cases like

https://github.com/mikro-orm/mikro-orm/blob/9158f515e513310f593aade04e6bedaa29cd6459/tests/features/entity-generator/__snapshots__/EntityGenerator.mysql.test.ts.snap#L1554-L1555

add Opt and a "=" too? This is done for numbers and booleans, and yet not for strings for some reason. Not sure if that's accurate. I mean, I'd think that as long as "default" is not undefined, Opt and an equals should be added.

B4nan commented 8 months ago

Yeah, that sounds good to me, I think it was done like that for historical reasons, as before the default could contain SQL functions too, now you need defaultRaw, so it should be safe.