Skip to content

fix: use DateTime instead of Time for time data type #392

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.

Sign up for GitHub

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

Open
cbarton wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: use DateTime instead of Time for time data type #392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 33 additions & 3 deletions acceptance/cases/type/time_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,28 @@ def test_set_and_save_time
assert_equal expected_time, record.start_time
end

def test_date_time_string_value_with_subsecond_precision
def test_date_time_string_value_with_timezone_aware_attributes
TestTypeModel.time_zone_aware_attributes = true
TestTypeModel.reset_column_information

string_value = "2017-07-04 14:19:10.897761"
expected_time = ::Time.local 2017, 07, 4, 14, 19, 10, 897761
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Apr 1, 2026

Choose a reason for hiding this comment

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

medium

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, 897761

Copy link
Copy Markdown
Contributor Author

@cbarton cbarton Apr 1, 2026

Choose a reason for hiding this comment

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

The goal of these tests is to ensure that it gets converted to UTC since time_zone_aware_attributes is truthy.


record = TestTypeModel.new start_time: string_value
assert_equal expected_time, record.start_time.utc

record.save!
assert_equal expected_time, record.start_time.utc

assert_equal record, TestTypeModel.find_by(start_time: string_value)
ensure
TestTypeModel.time_zone_aware_attributes = false
TestTypeModel.reset_column_information
end

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
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Apr 1, 2026

Choose a reason for hiding this comment

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

medium

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


record = TestTypeModel.new start_time: string_value
assert_equal expected_time, record.start_time.utc
Expand All @@ -71,7 +89,7 @@ def test_date_time_with_string_value_with_non_iso_format
assert_equal record, TestTypeModel.find_by(start_time: string_value)
end

def test_default_year_is_correct
def test_multiparameter_time
expected_time = ::Time.utc(2000, 1, 1, 10, 30, 0)
record = TestTypeModel.new start_time: { 4 => 10, 5 => 30 }

Expand All @@ -82,6 +100,18 @@ def test_default_year_is_correct

assert_equal expected_time, record.start_time
end

def test_multiparameter_datetime
expected_time = ::Time.utc(2020, 12, 25, 10, 30, 0)
record = TestTypeModel.new start_time: { 1 => 2020, 2 => 12, 3 => 25, 4 => 10, 5 => 30 }

assert_equal expected_time, record.start_time

record.save!
record.reload

assert_equal expected_time, record.start_time
end
end
end
end
end
17 changes: 1 addition & 16 deletions lib/active_record/type/spanner/time.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
module ActiveRecord
module Type
module Spanner
class Time < ActiveRecord::Type::Time
class Time < ActiveRecord::Type::DateTime
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Apr 1, 2026

Choose a reason for hiding this comment

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

high

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.

Copy link
Copy Markdown
Contributor Author

@cbarton cbarton Apr 1, 2026

Choose a reason for hiding this comment

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

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"

def serialize_with_isolation_level value, isolation_level
if value == :commit_timestamp
return "PENDING_COMMIT_TIMESTAMP()" if isolation_level == :dml
Expand All @@ -23,21 +23,6 @@ def serialize value
val = super value
val.acts_like?(:time) ? val.utc.rfc3339(9) : val
end

def user_input_in_time_zone value
return value.in_time_zone if value.is_a? ::Time
Copy link
Copy Markdown
Contributor Author

@cbarton cbarton Apr 1, 2026

Choose a reason for hiding this comment

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

not sure this is needed given the superclass calls in_time_zone since Rails 7.0

super value
end

private

def cast_value value
if value.is_a? ::String
value = value.empty? ? nil : ::Time.parse(value)
Copy link
Copy Markdown
Contributor Author

@cbarton cbarton Apr 1, 2026

Choose a reason for hiding this comment

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

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.

end

value
end
end
end
end
Expand Down
6 changes: 3 additions & 3 deletions test/activerecord_spanner_mock_server/spanner_active_record_with_mock_server_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,7 +670,7 @@ def test_create_singer_with_last_performance_as_non_iso_string
# could represent both 4th of July and 7th of April. This test verifies that it is encoded to the
# same value as ::Time.parse(..) would encode it to.
timestamp_string = "04/07/2017 2:19pm"
timestamp = ::Time.parse(timestamp_string)
timestamp = Time.find_zone('UTC').parse(timestamp_string)

Singer.transaction do
Singer.create(first_name: "Dave", last_name: "Allison", last_performance: timestamp_string)
Expand All @@ -689,12 +689,12 @@ def test_find_singer_by_last_performance_as_non_iso_string
# could represent both 4th of July and 7th of April. This test verifies that it is encoded to the
# same value as ::Time.parse(..) would encode it to.
timestamp_string = "04/07/2017 2:19pm"
timestamp = ::Time.parse(timestamp_string)
timestamp = Time.find_zone('UTC').parse(timestamp_string)
Singer.find_by(last_performance: timestamp_string)

request = @mock.requests.select {|req| req.is_a?(Google::Cloud::Spanner::V1::ExecuteSqlRequest) && req.sql == select_sql }.first
assert_equal :TIMESTAMP, request.param_types["p1"].code
assert_equal timestamp.utc.rfc3339(9), request.params["p1"]
assert_equal timestamp.rfc3339(9), request.params["p1"]
end

def test_create_singer_with_picture
Expand Down
Loading