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

Use local as default timezone, pass timezone option to Query #325

Closed
wants to merge 1 commit into from

Conversation

dirkmc
Copy link

@dirkmc dirkmc commented Sep 3, 2013

Fixes #303 along with dresende/node-sql-query#23

@dxg
Copy link
Collaborator

dxg commented Sep 3, 2013

Nice. This just need a test or two.
I'll work on postgres+sqlite support once there's a test case.

On a side note, the current travis test failure is a random sqlite bug.

@dirkmc
Copy link
Author

dirkmc commented Sep 3, 2013

By the way this won't affect sqlite or postgres as they ignore the timezone parameter.

@dxg
Copy link
Collaborator

dxg commented Sep 4, 2013

Yeah, I want to make those drivers timezone aware.

@dirkmc
Copy link
Author

dirkmc commented Sep 4, 2013

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:
http://stackoverflow.com/questions/7651409/mysql-datetime-insert-a-date-with-timezone-offset

However if it's possible in postgres/sqlite to insert/update a datetime using a string with a timestamp, eg
INSERT INTO test VALUES ('2012-06-12 04:23:43-08:00');
or some other mechanism, for example seconds since the epoch, eg
INSERT INTO test VALUES (2132321312);
then this won't be an issue.

@dirkmc
Copy link
Author

dirkmc commented Sep 4, 2013

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.

@dxg
Copy link
Collaborator

dxg commented Sep 4, 2013

I'm suprised MySQL doesn't support timezones. When I looked into this, postgres worked correctly when using a TIMESTAMP WITH TIMEZONE (as you said above) but it should also work correctly when no timestamp is supplied and it doesn't..

I'll work on some tests and getting this working across datastores. I hope to have it done before monday.

@dirkmc
Copy link
Author

dirkmc commented Sep 4, 2013

It's specifically the mysql datetime type that doesn't support timezones. Timestamp does support them:
http://dev.mysql.com/doc/refman/5.1/en/datetime.html

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.

@dresende
Copy link
Owner

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:

  • sqlite test config should not be memory, put a path (it's needed to have a state, to be able to reconnect and check stuff are still there)
  • mongodb seems to always store in UTC (or at least the node driver.. I'm seeing ISODate instead of Date in the database...)

}

this.query = new Query({ dialect: "mysql", timezone: config.timezone });

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

dresende added a commit that referenced this pull request Nov 8, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Time zone is double-shifted when saving
4 participants