-
Notifications
You must be signed in to change notification settings - Fork 376
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
Use local as default timezone, pass timezone option to Query #325
Conversation
Nice. This just need a test or two. On a side note, the current travis test failure is a random sqlite bug. |
By the way this won't affect sqlite or postgres as they ignore the timezone parameter. |
Yeah, I want to make those drivers timezone aware. |
I'm not certain, but I think it may not be an issue with postgres/sqlite. It depends on the database's syntax for inserting or updating a row with a datetime in it. As far as I know, in mysql there's no way of specifying the time zone of the datetime: However if it's possible in postgres/sqlite to insert/update a datetime using a string with a timestamp, eg |
I've added a test case in the sql-lite project for mysql. Not much code has changed in the orm project so I'm not sure how I would build code that tests it. |
I'm suprised MySQL doesn't support timezones. When I looked into this, postgres worked correctly when using a I'll work on some tests and getting this working across datastores. I hope to have it done before monday. |
It's specifically the mysql datetime type that doesn't support timezones. Timestamp does support them: However when I discovered this bug I also tried with timestamp and it still didn't work. I think the assumption that inserts/updates should be in UTC was in some places in the code, but not others, leading to inconsistencies. This pull request should fix those inconsistencies because it assumes system time zone unless otherwise configured. |
I think I've fixed some stuff, but since I'm in UTC right now I can't be sure. Can someone please test the latest commits? Some notes:
|
} | ||
|
||
this.query = new Query({ dialect: "mysql", timezone: config.timezone }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
config.timezone
if the config is null Cannot read property 'timezone' of null
line 16, you need to use your protected variable
this.query = new Query({ dialect: "mysql", timezone: this.config.timezone });
thx
Fixes #303 along with dresende/node-sql-query#23