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

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

Closed
5 tasks done
bernardojdias opened this issue Feb 20, 2024 · 9 comments · Fixed by #5264
Closed
5 tasks done

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

bernardojdias opened this issue Feb 20, 2024 · 9 comments · Fixed by #5264

Comments

@bernardojdias
Copy link

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:

  • The type number & Opt will not be outputed as varchar in the migration
  • I cannot remove the & Opt manually since I run the entity-generator everytime I need to make another update
  • I tried to mess with the entity-generator options to remove the Opt type from the Date type fields with no success

Thanks in advance!

Reproduction

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

Steps to reproduce:

  • run yarn command
  • make a copy of .env.example, rename it to .env and enter your database credentials
  • run yarn test command

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
Copy link
Member

B4nan commented Feb 20, 2024

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
Copy link
Collaborator

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
Copy link
Member

B4nan commented Feb 20, 2024

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
Copy link
Collaborator

boenrobot commented Feb 20, 2024

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
Copy link
Author

bernardojdias commented Feb 20, 2024

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
Copy link
Member

B4nan commented Feb 20, 2024

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
Copy link
Author

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
Copy link
Collaborator

boenrobot commented Feb 20, 2024

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

@Property({ length: 255, nullable: true, default: 'lol' })
foo?: string;

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
Copy link
Member

B4nan commented Feb 20, 2024

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.

boenrobot added a commit that referenced this issue Feb 20, 2024
… + string defaults

The "default" option is no longer emitted into the decorator,
in favor of it being specified in the property's declaration at the entity class.
The "defaultRaw" option is still used in cases where the SQL expression does not unambiguously map
to the "default" option's value.

The MySQL schema helper translates TINYINT(1) with a value of 0 to "false".
This ensures behavior consistent with other drivers.
And if this value accidentally finds its way back in a query, it is also OK,
as MySQL treats this literal as "0".

The "optional" option is now added early, in the DatabaseTable stage.
This has also unlocked some ManyToMany relations that were previously not detected.

Closes #5260
boenrobot added a commit that referenced this issue Feb 21, 2024
… + string defaults

The "default" option is no longer emitted into the decorator,
in favor of it being specified in the property's declaration at the entity class.
The "defaultRaw" option is still used in cases where the SQL expression does not unambiguously map
to the "default" option's value.

The MySQL schema helper translates TINYINT(1) with a value of 0 to "false".
This ensures behavior consistent with other drivers.
And if this value accidentally finds its way back in a query, it is also OK,
as MySQL treats this literal as "0".

The "optional" option is now added early, in the DatabaseTable stage.
This has also unlocked some ManyToMany relations that were previously not detected.

Closes #5260
boenrobot added a commit that referenced this issue Feb 21, 2024
… + string defaults

The "default" option is no longer emitted into the decorator,
in favor of it being specified in the property's declaration at the entity class.
The "defaultRaw" option is still used in cases where the SQL expression does not unambiguously map
to the "default" option's value.

The MySQL schema helper translates TINYINT(1) with a value of 0 to "false".
This ensures behavior consistent with other drivers.
And if this value accidentally finds its way back in a query, it is also OK,
as MySQL treats this literal as "0".

The "optional" option is now added early, in the DatabaseTable stage.
This has also unlocked some ManyToMany relations that were previously not detected.

Closes #5260
boenrobot added a commit that referenced this issue Feb 21, 2024
… + string defaults

The "default" option is no longer emitted into the decorator,
in favor of it being specified in the property's declaration at the entity class.
The "defaultRaw" option is still used in cases where the SQL expression does not unambiguously map
to the "default" option's value.

The MySQL schema helper translates TINYINT(1) with a value of 0 to "false".
This ensures behavior consistent with other drivers.
And if this value accidentally finds its way back in a query, it is also OK,
as MySQL treats this literal as "0".

The "optional" option is now added early, in the DatabaseTable stage.
This has also unlocked some ManyToMany relations that were previously not detected.

Closes #5260
boenrobot added a commit that referenced this issue Feb 21, 2024
… + string defaults

The "default" option is no longer emitted into the decorator,
in favor of it being specified in the property's declaration at the entity class.
The "defaultRaw" option is still used in cases where the SQL expression does not unambiguously map
to the "default" option's value.

The MySQL and MariaDB schema helper translates TINYINT(1) with a values of 0 or 1
to "false" or "true", respectively.
This ensures behavior consistent with other drivers.
And if this value accidentally finds its way back in a query, it is also OK,
as MySQL and MariaDB treat those literals as those number literals.

The "optional" option is now added early, in the DatabaseTable stage.
This has also unlocked some ManyToMany relations that were previously not detected.

Closes #5260
boenrobot added a commit that referenced this issue Feb 21, 2024
… + string defaults

The "default" option is no longer emitted into the decorator,
in favor of it being specified in the property's declaration at the entity class.
The "defaultRaw" option is still used in cases where the SQL expression does not unambiguously map
to the "default" option's value.

The MySQL and MariaDB schema helper translates TINYINT(1) with a values of 0 or 1
to "false" or "true", respectively.
This ensures behavior consistent with other drivers.
And if this value accidentally finds its way back in a query, it is also OK,
as MySQL and MariaDB treat those literals as those number literals.

The "optional" option is now added early, in the DatabaseTable stage.
This has also unlocked some ManyToMany relations that were previously not detected.

Closes #5260
boenrobot added a commit that referenced this issue Feb 21, 2024
… + string defaults

The "default" option is no longer emitted into the decorator,
in favor of it being specified in the property's declaration at the entity class.
The "defaultRaw" option is still used in cases where the SQL expression does not unambiguously map
to the "default" option's value.

The MySQL and MariaDB schema helper translates TINYINT(1) with a values of 0 or 1
to "false" or "true", respectively.
This ensures behavior consistent with other drivers.
And if this value accidentally finds its way back in a query, it is also OK,
as MySQL and MariaDB treat those literals as those number literals.

The "optional" option is now added early, in the DatabaseTable stage.
This has also unlocked some ManyToMany relations that were previously not detected.

Closes #5260
B4nan pushed a commit that referenced this issue Feb 21, 2024
… + string defaults (#5264)

The "default" option is no longer emitted into the decorator, in favor
of it being specified in the property's declaration at the entity class.
The "defaultRaw" option is still used in cases where the SQL expression
does not unambiguously map to the "default" option's value.

The MySQL schema helper translates TINYINT(1) with a value of 0 to
"false". This ensures behavior consistent with other drivers. And if
this value accidentally finds its way back in a query, it is also OK, as
MySQL treats this literal as "0".

The "optional" option is now added early, in the DatabaseTable stage.
This has also unlocked some ManyToMany relations that were previously
not detected.

Closes #5260
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants