-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
fixes #8747 #8748
fixes #8747 #8748
Conversation
can you please fix the conflicts and add the test case so we won't have this issue in the future again? thanks. |
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. |
why not to make it here? I need test to check what exactly this change does and what error we have without this change. |
@pleerock I added a test covering the issue. |
tests are failing |
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). |
Something weird was happening when I used @pleerock would you mind reviewing my test to see if I did something wrong? |
yes I'll do it once I get the time to checkout your branch and do manual testing |
looks like it breaks because of |
after removing So, again my question is regarding to your test - why did you use primary |
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.
Thank you! I totally forgot about that. I rewrote the test case, instead of using DEFAULT I used
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. |
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. |
# Conflicts: # src/query-builder/UpdateQueryBuilder.ts
thank you for contribution! |
…lationship (typeorm#8748) * fixes typeorm#8747 * apply fix again * added test for issue 8747 * format * remove .only * only use postgres driver * fixed test * removed createDateColumn * sqlite does not support timestamp type * fix typo Co-authored-by: Umed Khudoiberdiev <pleerock.me@gmail.com> Co-authored-by: Alex Messer <dmzt08@gmail.com>
Description of change
Fixes #8747
Handles the Date object correctly.
Pull-Request Checklist
master
branchnpm run lint
passes with this changenpm run test
passes with this changeFixes #0000