Allow the time zone to be overridden via ENV var

[ochran]

When living in a timezone that does not always match UTC, there needs to be an option to be able to set a location, as you will have difficulties during daylight saving or a season that changes the time diff.

Seems like an ENV var is the best way to achieve this and preserve current behaviour if one hasn't been supplied.

Cheers

[straight-shoota]

I don't know the specifics of time zone handling in MySQL. Maybe changing the constant might have some undesired side effects. Anyway, it needs to be tested with different timezones.

Could you explain a specific example (in code) which details the issues you experienced?

[ochran]

Let's say we have inserted a column using NOW() in MySQL, this will use your local time zone and save 2019-04-07 15:50:21

When reading this from the database using this library, rs.read(Time) it will have the value of: 2019-04-07 15:50:21 UTC - this is wrong as our database is not using UTC but our local timezone.

With this PR reading the column from MySQL with your timezone set as an ENV var, will result in the correct time object of:
2019-04-07 15:50:21 +01:00

The need for this change comes after the changes around the Time object and how it stores its location. This issue does not exist in other langs (python, go) as the Time object does not have a location built in.

@straight-shoota please let me know if you follow the above

Thanks

[straight-shoota]

MySQL has TIMESTAMP and DATETIME data types which behave differently regarding time zones. Which one are you referring to?

[straight-shoota]

Also, I can't see how this patch fixes things, because the time value returned from MySQL depends on the server time zone. Simply assigning some time zone in the client doesn't help because it can't be ensured to be in sync with the server's time zone.

[ochran]

I am using DATETIME in MySQL, the PR does fix it as Crystal will now be correctly expecting a +01:00 time object instead of a UTC.

In the example I gave above, that is happening in my local code after applying the patch. Regardless, I still think there should still be a way to tell this library what timezone to use when reading timestamps as it is causing issues.

[straight-shoota]

But time zone values will be incorrect again in case DB_TIME_ZONE differs from the server time zone.

[ochran]

This is why it should be an ENV var, so the dev/ops can add it to a .env file, currently I have no way of returning a non-UTC Time object from the DB, which is not what we want.

Maybe a better way to do this is to allow the rs.read(Time) to return a .to_local version?
EDIT: This won't work as if local is 1hour ahead of UTC, it will add an hour, thus making the timestamp incorrect.

[ochran]

But time zone values will be incorrect again in case DB_TIME_ZONE differs from the server time zone.

If a server's timezone is already UTC, there is no need to overwrite the current behaviour, so the env var need not be set. This only applies when the machine you are running the code on is not UTC (we should presume the MySQL timezone and the server's timezone match)

[RX14]

Is there no way to store the server's timezone and represent times correctly? Times coming from mysql shouldn't need configuration to be correct, is this a problem encountered by other libraries?

[bcardiff]

I agree that there needs to be an option to specify the timezone of the time values in the DB when those do not have TZ info.

That TZ configuration can also be used to indicate to which TZ translate all time if wanted although this list might be more in the responsibility of the ORM in use. I'm not sure.

But what I would like is that to be a configuration of the database connection. If there is a fallback to an env variable it's another story.

This should be handled in crystal-db directly. The current TZ support was the minimum to keep all db shards compatible with the time changes in the stdlib.

@ochran are you interested in pushing this story in crystal-db directly? If so we can discuss there the design.

[ochran]

@RX14 It isn't a problem in any other lib I've come across, but I think that may be down to the fact that most languages don't have a Time object with a built in location, or the db drivers don't return one at least.

@bcardiff yeah can do, not sure where I would make this code change as I can't see anywhere in crystal-db that handles location. I'll push to crystal-db

Cheers