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

Introduce custom timezone configuration support for ActiveRecord enumerators #512

Merged
merged 7 commits into from
Oct 11, 2024

Conversation

yigitozkavci
Copy link
Contributor

@yigitozkavci yigitozkavci commented Oct 9, 2024

Background

Current implementation of column_value is prone to faulty serialisation if ActiveRecord's timezone differs from the timezone in the database.

In our case, we are using Eastern Time (US & Canada) in our Rails application but our database stores datas in UTC.

Following console log demonstrates the issue:

[2] pry(#<>)> Time.use_zone("UTC"){ OrderTransaction.select(:created_at).order(created_at: :desc).take(1).first.created_at.strftime("%Y-%m-%d %H:%M:%S.%N") }
D, [2024-10-09T16:54:28.648833 #17479] DEBUG -- :   CACHE OrderTransaction Load (0.2ms)  SELECT `order_transactions`.`created_at` FROM `order_transactions` ORDER BY `order_transactions`.`created_at` DESC LIMIT 1
D, [2024-10-09T16:54:28.649738 #17479] DEBUG -- :   ↳ (pry):2:in `block in build_enumerator'
=> "2024-10-09 08:02:49.000000000"
[3] pry(#<>)> OrderTransaction.select(:created_at).order(created_at: :desc).take(1).first.created_at.strftime("%Y-%m-%d %H:%M:%S.%N")
D, [2024-10-09T16:54:37.011976 #17479] DEBUG -- :   CACHE OrderTransaction Load (0.3ms)  SELECT `order_transactions`.`created_at` FROM `order_transactions` ORDER BY `order_transactions`.`created_at` DESC LIMIT 1
D, [2024-10-09T16:54:37.012498 #17479] DEBUG -- :   ↳ (pry):3:in `build_enumerator'
=> "2024-10-09 04:02:49.000000000"

Fix

Current implementation of column_value assumes that the database & the application are using the same timezone regardless of it being UTC or not. However, it is generally accepted that database column values that are of type date are assumed to be UTC unless explicitly stated otherwise. Therefore, any serialisation may assume as such instead of assuming them both having the same timezone.

To make this change not potentially a breaking change we've made it configurable with a default value of nil. This will allow users of this library to configure a custom timezone if need be.

@yigitozkavci yigitozkavci marked this pull request as ready for review October 9, 2024 15:59
@yigitozkavci yigitozkavci force-pushed the fix-utc-timezone-in-cursor-resume branch from 8b98e4b to 14f522a Compare October 10, 2024 09:58
@yigitozkavci yigitozkavci changed the title Fix the UTC timezone issue with cursor resume Introduce custom timezone configuration for ActiveRecord enumerators Oct 10, 2024
@yigitozkavci yigitozkavci changed the title Introduce custom timezone configuration for ActiveRecord enumerators Introduce custom timezone configuration support for ActiveRecord enumerators Oct 10, 2024
Copy link
Contributor

@Mangara Mangara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just missing a change to enumerator_builder to pass through the timezone, LGTM otherwise.

@Mangara
Copy link
Contributor

Mangara commented Oct 11, 2024

Just missing a change to enumerator_builder to pass through the timezone

Never mind, the splats take care of this. Good to merge!

@Mangara Mangara merged commit 46ec901 into Shopify:main Oct 11, 2024
25 checks passed
@punkstar punkstar self-assigned this Oct 11, 2024
@@ -61,6 +62,7 @@ def column_value(record, attribute)
value = record.read_attribute(attribute.to_sym)
case record.class.columns_hash.fetch(attribute).type
when :datetime
value = value.in_time_zone(@timezone) unless @timezone.nil?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this has to be configured explicitly. Active Job serializes and deserializes the timezone on the job, and sets it in an around_perform hook. Could this simply be:

value.in_time_zone(::Time.zone)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that the default behaviour? I think the original issue is with a job enqueued (and running) in the local timezone, but reading data that was stored in a different time zone (UTC).

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.

5 participants