fix: use DateTime instead of Time for time data type#392
fix: use DateTime instead of Time for time data type#392cbarton wants to merge 1 commit intogoogleapis:mainfrom
Conversation
In Rails 8.1, the Time datatype will truncate the date component unless the column is excluded from conversion with `skip_time_zone_conversion_for_attributes`. This is not helpful for timestamps in a localized application. The time data type supports datetime, so it should use datetime, not time type.
cbarton
requested review from
a team,
olavloite and
sakthivelmanii
as code owners
product-auto-label
Bot
added
the
api: spanner
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request changes the base class of ActiveRecord::Type::Spanner::Time to ActiveRecord::Type::DateTime, removes redundant methods, and adds tests for time-zone aware attributes and multi-parameter time/datetime. Feedback highlights a potential ArgumentError when calling rfc3339(9) on DateTime objects, suggesting a switch to iso8601(9). Additionally, it is recommended to use ::Time.utc instead of ::Time.local in tests to ensure environment independence and to avoid leading zeros in integer literals to prevent octal interpretation issues.
| module Type | ||
| module Spanner | ||
| class Time < ActiveRecord::Type::Time | ||
| class Time < ActiveRecord::Type::DateTime |
There was a problem hiding this comment.
Changing the base class to ActiveRecord::Type::DateTime introduces a potential ArgumentError in the serialize method (line 24). While ActiveRecord::Type::Time#cast (the previous parent) always returns a Time object, ActiveRecord::Type::DateTime#cast can return a DateTime object for certain date ranges or configurations.
In Ruby's standard library, DateTime#rfc3339 does not accept any arguments, whereas Time#rfc3339 does. Calling rfc3339(9) on a DateTime instance will raise an ArgumentError: wrong number of arguments (given 1, expected 0).
Consider updating the serialize method to use iso8601(9), as ActiveSupport provides a consistent implementation for both Time and DateTime that supports the precision argument and is compatible with Spanner's TIMESTAMP format.
There was a problem hiding this comment.
This doesn't sound accurate to me, rfc3339 is an alias of iso8601 in Ruby and Rails. Double checking:
'2025-03-07T01:23:00.45678Z'.in_time_zone.class
=> ActiveSupport::TimeWithZone
'2025-03-07T01:23:00.45678Z'.in_time_zone.rfc3339(9)
=> "2025-03-07T01:23:00.456780000Z"
DateTime.parse('2025-03-07T01:23:00.45678Z').class
=> DateTime
DateTime.parse('2025-03-07T01:23:00.45678Z').rfc3339(9)
=> "2025-03-07T01:23:00.456780000+00:00"| TestTypeModel.reset_column_information | ||
|
|
||
| string_value = "2017-07-04 14:19:10.897761" | ||
| expected_time = ::Time.local 2017, 07, 4, 14, 19, 10, 897761 |
There was a problem hiding this comment.
Using ::Time.local makes the test dependent on the time zone of the environment where the tests are running. This can lead to flaky tests or failures on machines not set to UTC, especially when comparing against .utc values. It is better to use ::Time.utc to ensure consistency with how the string value is likely to be parsed in a Spanner context. Additionally, avoid leading zeros in integer literals (like 07) as they can be interpreted as octal and cause errors for values like 08 or 09.
expected_time = ::Time.utc 2017, 7, 4, 14, 19, 10, 897761There was a problem hiding this comment.
The goal of these tests is to ensure that it gets converted to UTC since time_zone_aware_attributes is truthy.
|
|
||
| def test_date_time_string_value_with_subsecond_precision | ||
| string_value = "2017-07-04 14:19:10.897761" | ||
| expected_time = ::Time.local 2017, 07, 4, 14, 19, 10, 897761 |
There was a problem hiding this comment.
Using ::Time.local makes the test dependent on the environment's time zone. Switching to ::Time.utc ensures the test is robust across different environments. Also, using decimal integers instead of octal-style literals (e.g., 7 instead of 07) is safer.
expected_time = ::Time.utc 2017, 7, 4, 14, 19, 10, 897761
| end | ||
|
|
||
| def user_input_in_time_zone value | ||
| return value.in_time_zone if value.is_a? ::Time |
There was a problem hiding this comment.
not sure this is needed given the superclass calls in_time_zone since Rails 7.0
|
|
||
| def cast_value value | ||
| if value.is_a? ::String | ||
| value = value.empty? ? nil : ::Time.parse(value) |
There was a problem hiding this comment.
The superclass has optimized versions of parsing string times, so removed in favor of that. This did cause me to update the tests, but the tests aren't exactly accurate since they do not have the time zone set on Active Record to UTC.
1 participant
In Rails 8.1, the Time datatype will truncate the date component unless the column is excluded from conversion with
skip_time_zone_conversion_for_attributes. This is not helpful for timestamps in a localized application.The time data type supports datetime, so it should use datetime, not time type.