deserialze time with zone to provide consistent accessor interface

[eprothro]

My understanding is that an Active Record date_time accessors are time zone aware, returning the object configured with the current time zone (e.g. not the system time). If I'm wrong there, just let me know.

Migrating to this gem with existing data (e.g replacing field foo_at that was an Active Record date_time type field with an hstore_accessor attribute with data_type: :time resulted in unexpected behavior, where the returned object is now initialized with the zone of the system clock.

Before migration to hstore

some_model.foo_at.iso8601 => 2015-05-11T21:28:00Z After migration to hstore

some_model.foo_at.iso8601 => 2015-05-11T16:28:00-05:00 The change in this PR fixes this, such that the ActiveSupport timezone is respected.

If you think this is the right direction, I'd love your thoughts about the robustness of this change, and how to best integrate that with the test suite.

[eprothro]

New PR that is rebased to master from a branch on my fork (since i have two PRs open, I wanted to keep them independent)

[crismali]

@eprothro I've played around a bit with this and I was seeing some inconsistent behavior with this change (the previous PR). I'm going to poke around again with this one and see what I can figure out.

Regardless, we're definitely going to want some tests around this.

[eprothro]

@crismali Agreed on testing. Previously, I was using a different conversion that was less robust (and broke some specs). I had nerves on that to begin with, and didn't see an obvious way to add the tests I wanted.

The current method is a much more robust. I've added a couple specs, let me know what you think and if you see any weirdness.

I squashed, so you might need to delete your local branch and update/pull again.

[crismali]

ActiveRecord appears to use whatever the default timezone is set to (here's the full method). I'd like to see that behavior tested in here before merging this in.

So we would either want to reproduce that ternary on line 11 or perhaps us that method itself to deserialize the time.

[eprothro]

By default, Active Record makes datetime attributes time zone aware, which I believe means the TimeZoneConversion decorator is used. This calls in_time_zone here to determine the value returned from the attribute getter.

I think the Value methods you link to above handles conversion to and from :db format, and handles respecting the configuration of ::Base.default_timezone.

In my mind, ::Base.default_timezone is primarily for legacy support (e.g. you imported a bunch of data from a legacy database that was stored in local instead of UTC. I would say that using ::Base.default_timezone to determine how hstore_accessor stores / loads the datetimevalue is a separate issue/feature outside the scope of this issue.

I would say this issue/PR is to make the attribute interface (e.g. value returned when calling the getter for the attribute/key) conform to the Active Record default behavior.

Once this is done, a next step could then be respecting the configuration of ActiveRecord::Base.time_zone_aware_attributes and ActiveRecord::Base.skip_time_zone_conversion_for_attributes to determine if this behavior applies to a particular attribute/key or not. But again, to me that is outside the scope of this issue.

[crismali]

I think the easiest approach that would handle this PR and the other issues you outlined would be to use ActiveRecord's serialization and deserialization as much as possible. I don't want this gem to essentially reimplement all of that logic and it would probably be much easier. Then the testing is easier too (just making sure the appropriate ActiveRecord serialization methods are used).

This weekend I'll probably look at this a bit more and merge it as long as it's a step closer to making the gem 1 to 1 with ActiveRecord, but right now I'm a little concerned that this might create some weird edge cases.

[eprothro]

Sounds like an appropriate approach to me. Any particular edge cases in mind?

[crismali]

I suppose my main concern is what users expect with different combinations of the different time/timezone configurations. Is the in_time_zone implementation's behavior more or less expected if you set config.active_record.time_zone_aware_attributes = false? That may depend on whether or not they've set a non utc default timezone. Does it make sense with self.skip_time_zone_conversion_for_attributes = [:written_on] or is that something we need to workaround (ie, if skip_time_zone_conversion_for_attributes= raises errors if given hstore accessor defined attributes? I'd just like to dive a little deeper into how all of these different pieces fit together.

Right now my feeling is that this is a step in the right direction though.

[eprothro]

Agree.

The more we've discussed it here, the kicker for me has become that this implementation works as expected with the default (aware on, zone utc, no skipped attributes), and the most common non-default configurations (aware on, zone non-default, no skipped attributes).

Frankly, I didn't even know about the existence of skip_time_zone_conversion_for_attributes until this issue :flushed: , and I've never turned time_zone_aware_attributes off.

[crismali]

I think this definitely gets us closer to being 1 to 1 with ActiveRecord. There are probably some edge cases out there, but I suppose will wait for those to come up in real life.