fixes #8747

[jorenvandeweyer]

Description of change

Fixes #8747

Handles the Date object correctly.

Pull-Request Checklist

• Code is up-to-date with the master branch
• npm run lint passes with this change
• npm run test passes with this change
• This pull request links relevant issues as Fixes #0000
• There are new or updated unit tests validating the change
• Documentation has been updated to reflect this change (NA)
• The new commits follow conventions explained in CONTRIBUTING.md

[pleerock]

can you please fix the conflicts and add the test case so we won't have this issue in the future again? thanks.

[jorenvandeweyer]

I agree a test case should be added, I think it would be better in a different issue/pr. Testing this would require a good timeseries database setup imo.

@pleerock if you agree I'll create a new issue and create a separate pull request. This way we can already merge/close this one.

[pleerock]

why not to make it here? I need test to check what exactly this change does and what error we have without this change.

[jorenvandeweyer]

@pleerock I added a test covering the issue.

[pleerock]

tests are failing

[pleerock]

I didn't really get it - why it didn't work in mysql? If test has something mysql-specific it's better to simplify it (test should be simple as possible anyway and contain the only related to the problem).

[jorenvandeweyer]

Something weird was happening when I used createDateColumn as the primary key. Something to do with the default value of the column.

@pleerock would you mind reviewing my test to see if I did something wrong?

[pleerock]

yes I'll do it once I get the time to checkout your branch and do manual testing

[pleerock]

looks like it breaks because of precision: 3. Maybe mysql doesn't play well with this value. Do you really need precision in this test?

[pleerock]

after removing precision I found another problem - mysql tests would not be able to pass because you rely on primary column not explicitly set in your code, instead it set by database's DEFAULT (since you are using CreateDateColumn). After TypeORM saves your entity it needs entity id-s to perform mapping operations, it doesn't work if it doesn't have entity ids. So, this scenario is not supported. It works in postgres because it supports RETURNING statement which is not supported by mysql and some other drivers.

So, again my question is regarding to your test - why did you use primary CreateDateColumn ? Is it required in order to fix what you are trying to fix?

[jorenvandeweyer]

looks like it breaks because of precision: 3. Maybe mysql doesn't play well with this value. Do you really need precision in this test?

Not sure if it is needed in mysql but it is needed in PostgreSQL. JavaScript timestamps only have a precision of 3 decimals on a second, postgres has 6 by default. So it would be impossible to set the foreign key of the relationship since the timestamps would differ.

after removing precision I found another problem - mysql tests would not be able to pass because you rely on primary column not explicitly set in your code, instead it set by database's DEFAULT (since you are using CreateDateColumn). After TypeORM saves your entity it needs entity id-s to perform mapping operations, it doesn't work if it doesn't have entity ids. So, this scenario is not supported. It works in postgres because it supports RETURNING statement which is not supported by mysql and some other drivers.

Thank you! I totally forgot about that. I rewrote the test case, instead of using DEFAULT I used new Date() in the code instead.

So, again my question is regarding to your test - why did you use primary CreateDateColumn ? Is it required in order to fix what you are trying to fix?

CreateDateColumn is not required for the test, the composite primary key only requires a timestamp. I removed the createDateColumn in the latest commit.

Now it's working for both postgres & mysql.

[pleerock]

That's why I ask to keep tests as simple as possible and contain the only code test need. As you can see both of us waste lot of time.

[AlexMesser]

thank you for contribution!