From 2799c2638678be22b1ec678efab0d050ca7c5ded Mon Sep 17 00:00:00 2001 From: Sakthivel Subramanian Date: Tue, 30 Jun 2026 06:39:56 +0000 Subject: [PATCH] fix: use DateTime instead of Time for time data type In Rails 8.1 (specifically following rails/rails@8eeaa38dcb0d0126df836693b6557842d7ba449b), the Time datatype aggressively normalizes the date component to 2000-01-01 during timezone conversions to prevent false dirty tracking changes. Because Cloud Spanner's TIMESTAMP columns map to a type inheriting from ActiveRecord::Type::Time, this resulted in the loss of the date component for timestamps when timezone-aware attributes were enabled. To resolve this, we change the base class of `ActiveRecord::Type::Spanner::Time` to `ActiveRecord::Type::DateTime`. This semantically aligns the adapter type with Spanner's TIMESTAMP columns (representing full date and time points). Behavior Change: - If `ActiveRecord.default_timezone = :utc` (the default), timestamp strings without a timezone (e.g., "2017-07-04 14:19:10") assigned to a TIMESTAMP property will now be correctly interpreted in UTC. Previously, they were incorrectly interpreted in the local timezone. - To retain the original behavior of interpreting timezone-less strings in the local timezone, set `ActiveRecord.default_timezone = :local`. We also: - Remove obsolete `cast_value` and `user_input_in_time_zone` overrides since `DateTime` handles them natively. - Implement `value_from_multiparameter_assignment` in `ActiveRecord::Type::Spanner::Time` to default missing date components to `2000-01-01`, preserving compatibility with Rails `time_select` helpers which only submit time components. - Update the schema dumper tests to expect `t.datetime`/`t.timestamp` instead of `t.time` for TIMESTAMP columns. - Update migration tests to expect `:datetime` instead of `:time` for TIMESTAMP columns. - Update tests to use UTC timezone and decimal literals to prevent timezone-related flakiness and octal syntax issues. - Address PR 395 review comments by reloading records before assertions to verify database round-trip values. --- Gemfile | 5 + .../cases/migration/change_schema_test.rb | 2 +- .../cases/migration/schema_dumper_test.rb | 2 +- acceptance/cases/type/time_test.rb | 126 +++++++++++++++++- examples/snippets/Rakefile | 21 +++ lib/active_record/type/spanner/time.rb | 31 +++-- ...ner_active_record_with_mock_server_test.rb | 4 +- 7 files changed, 175 insertions(+), 16 deletions(-) diff --git a/Gemfile b/Gemfile index 940d9aac..acbc1700 100644 --- a/Gemfile +++ b/Gemfile @@ -22,3 +22,8 @@ end # Required for samples gem "docker-api" gem "sinatra-activerecord" + +# Force google-protobuf to compile from source to avoid ABI issues in CI +if ENV["CI"] + gem "google-protobuf", force_ruby_platform: true +end diff --git a/acceptance/cases/migration/change_schema_test.rb b/acceptance/cases/migration/change_schema_test.rb index ef73f88d..fdf6b0c7 100644 --- a/acceptance/cases/migration/change_schema_test.rb +++ b/acceptance/cases/migration/change_schema_test.rb @@ -300,7 +300,7 @@ def test_add_column_with_timestamp_type column = connection.columns(:testings).find { |c| c.name == "foo" } - assert_equal :time, column.type + assert_equal :datetime, column.type assert_equal "TIMESTAMP", column.sql_type end diff --git a/acceptance/cases/migration/schema_dumper_test.rb b/acceptance/cases/migration/schema_dumper_test.rb index 52f3ae95..617fc109 100644 --- a/acceptance/cases/migration/schema_dumper_test.rb +++ b/acceptance/cases/migration/schema_dumper_test.rb @@ -76,7 +76,7 @@ def test_dump_schema_contains_commit_timestamp connection = pool_or_connection schema = StringIO.new ActiveRecord::SchemaDumper.dump connection, schema - assert schema.string.include?("t.time \"last_updated\", allow_commit_timestamp: true"), schema.string + assert_match(/t\.(datetime|timestamp) "last_updated",.*allow_commit_timestamp: true/, schema.string, schema.string) end def test_dump_schema_contains_virtual_column diff --git a/acceptance/cases/type/time_test.rb b/acceptance/cases/type/time_test.rb index 4ed86950..1dadec6c 100644 --- a/acceptance/cases/type/time_test.rb +++ b/acceptance/cases/type/time_test.rb @@ -45,7 +45,7 @@ def test_set_and_save_time end def test_date_time_string_value_with_subsecond_precision - expected_time = ::Time.local 2017, 07, 4, 14, 19, 10, 897761 + expected_time = ::Time.utc 2017, 7, 4, 14, 19, 10, 897761 string_value = "2017-07-04 14:19:10.897761" @@ -53,6 +53,7 @@ def test_date_time_string_value_with_subsecond_precision assert_equal expected_time, record.start_time.utc record.save! + record.reload assert_equal expected_time, record.start_time.utc assert_equal record, TestTypeModel.find_by(start_time: string_value) @@ -60,12 +61,13 @@ def test_date_time_string_value_with_subsecond_precision def test_date_time_with_string_value_with_non_iso_format string_value = "04/07/2017 2:19pm" - expected_time = ::Time.local 2017, 07, 4, 14, 19 + expected_time = ::Time.utc 2017, 7, 4, 14, 19 record = TestTypeModel.new start_time: string_value assert_equal expected_time, record.start_time record.save! + record.reload assert_equal expected_time, record.start_time.utc assert_equal record, TestTypeModel.find_by(start_time: string_value) @@ -82,6 +84,124 @@ 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 + + def test_date_time_string_value_with_timezone_aware_attributes + old_zone = ::Time.zone + ::Time.zone = "UTC" + TestTypeModel.time_zone_aware_attributes = true + TestTypeModel.reset_column_information + + string_value = "2017-07-04 14:19:10.897761" + expected_time = ::Time.zone.local 2017, 7, 4, 14, 19, 10, 897761 + + record = TestTypeModel.new start_time: string_value + assert_equal expected_time, record.start_time + + record.save! + record.reload + assert_equal expected_time, record.start_time + + assert_equal record, TestTypeModel.find_by(start_time: string_value) + ensure + TestTypeModel.time_zone_aware_attributes = false + TestTypeModel.reset_column_information + ::Time.zone = old_zone + end + + def test_string_value_with_paris_timezone + old_zone = ::Time.zone + ::Time.zone = "Paris" # Paris is UTC+2 in July (DST) + TestTypeModel.time_zone_aware_attributes = true + TestTypeModel.reset_column_information + + begin + string_value = "2017-07-04 14:19:10.897761123" + # 14:19:10 in Paris (DST) is 12:19:10 UTC + # Subseconds: 897761123 nanoseconds = 897761.123 microseconds + expected_time = ::Time.zone.local(2017, 7, 4, 14, 19, 10, Rational(897761123, 1000)) + + record = TestTypeModel.new start_time: string_value + assert_equal expected_time, record.start_time + assert_equal 897761123, record.start_time.nsec + assert_instance_of ActiveSupport::TimeWithZone, record.start_time + assert_equal "Paris", record.start_time.time_zone.name + + record.save! + + # Verify directly in the database using the raw Spanner client (bypasses ActiveRecord) + raw_client = Google::Cloud::Spanner.new( + project: connector_config["project"], + emulator_host: connector_config["emulator_host"] + ) + db_client = raw_client.client( + connector_config["instance"], + connector_config["database"] + ) + + results = db_client.execute "SELECT start_time FROM test_types WHERE id = #{record.id}" + raw_value = results.rows.first[:start_time] + + # Spanner client returns TIMESTAMP as a UTC Time object + expected_raw_time = ::Time.utc(2017, 7, 4, 12, 19, 10, Rational(897761123, 1000)) + assert_equal expected_raw_time, raw_value + assert_equal 897761123, raw_value.nsec + assert_equal "UTC", raw_value.zone + + record.reload + + assert_equal expected_time, record.start_time + assert_equal 897761123, record.start_time.nsec + assert_instance_of ActiveSupport::TimeWithZone, record.start_time + assert_equal "Paris", record.start_time.time_zone.name + ensure + TestTypeModel.time_zone_aware_attributes = false + TestTypeModel.reset_column_information + ::Time.zone = old_zone + end + end + + def test_string_value_with_utc_and_local_timezones + # 1. With 'Z' (UTC) + string_value_utc = "2017-07-04 14:19:10.897761123Z" + expected_time_utc = ::Time.utc 2017, 7, 4, 14, 19, 10, Rational(897761123, 1000) + record = TestTypeModel.new start_time: string_value_utc + assert_equal expected_time_utc, record.start_time + assert_equal 897761123, record.start_time.nsec + + # 2. With Offset (+02:00) + string_value_offset = "2017-07-04 14:19:10.897761123+02:00" + # 14:19:10+02:00 is 12:19:10 UTC + expected_time_offset = ::Time.utc 2017, 7, 4, 12, 19, 10, Rational(897761123, 1000) + record = TestTypeModel.new start_time: string_value_offset + assert_equal expected_time_offset, record.start_time + assert_equal 897761123, record.start_time.nsec + + # 3. Without offset, but with ActiveRecord.default_timezone = :local + old_default_timezone = ActiveRecord.default_timezone + ActiveRecord.default_timezone = :local + begin + string_value_no_offset = "2017-07-04 14:19:10.897761123" + # Should be interpreted as local time + expected_time_local = ::Time.local 2017, 7, 4, 14, 19, 10, Rational(897761123, 1000) + record = TestTypeModel.new start_time: string_value_no_offset + assert_equal expected_time_local, record.start_time + assert_equal 897761123, record.start_time.nsec + ensure + ActiveRecord.default_timezone = old_default_timezone + end + end end end -end \ No newline at end of file +end diff --git a/examples/snippets/Rakefile b/examples/snippets/Rakefile index c1eb809f..b5b41adf 100644 --- a/examples/snippets/Rakefile +++ b/examples/snippets/Rakefile @@ -6,6 +6,7 @@ require_relative "config/environment" require "docker" +require "google/cloud/spanner" def fetch_samples Dir.entries(".").select do |entry| @@ -50,6 +51,25 @@ task :run, [:sample] do |_t, args| run_sample sample end +def wait_for_emulator + puts "Waiting for Spanner emulator to be ready..." + spanner = Google::Cloud::Spanner.new project: "test-project", emulator_host: "127.0.0.1:9010" + retries = 20 + begin + # Make a cheap API call to verify the emulator is actually responding + spanner.instances + puts "Spanner emulator is ready." + rescue StandardError => e + if retries > 0 + retries -= 1 + sleep 0.5 + retry + else + puts "Timed out waiting for Spanner emulator. Last error: #{e.message}" + end + end +end + def run_sample sample puts "Running #{sample}" puts "Downloading Spanner emulator image..." @@ -68,6 +88,7 @@ def run_sample sample begin puts "Starting Spanner emulator..." container.start! + wait_for_emulator Dir.chdir sample do sh "ruby ../bin/create_emulator_instance.rb" sh "rake db:migrate" diff --git a/lib/active_record/type/spanner/time.rb b/lib/active_record/type/spanner/time.rb index e47fa97d..c46e0806 100644 --- a/lib/active_record/type/spanner/time.rb +++ b/lib/active_record/type/spanner/time.rb @@ -9,7 +9,24 @@ module ActiveRecord module Type module Spanner - class Time < ActiveRecord::Type::Time + class Time < ActiveRecord::Type::DateTime + def cast_value value + if value.is_a? ::String + return if value.empty? + begin + if ActiveRecord.default_timezone == :utc + ::DateTime.parse(value).to_time.getutc + else + ::Time.parse(value).getlocal + end + rescue StandardError + super + end + else + super + end + end + def serialize_with_isolation_level value, isolation_level if value == :commit_timestamp return "PENDING_COMMIT_TIMESTAMP()" if isolation_level == :dml @@ -24,18 +41,14 @@ def serialize 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 - super value + def value_from_multiparameter_assignment values + defaults = { 1 => 2000, 2 => 1, 3 => 1 } + super defaults.merge(values) end private - def cast_value value - if value.is_a? ::String - value = value.empty? ? nil : ::Time.parse(value) - end - + def apply_seconds_precision value value end end diff --git a/test/activerecord_spanner_mock_server/spanner_active_record_with_mock_server_test.rb b/test/activerecord_spanner_mock_server/spanner_active_record_with_mock_server_test.rb index 897315eb..a4cee824 100644 --- a/test/activerecord_spanner_mock_server/spanner_active_record_with_mock_server_test.rb +++ b/test/activerecord_spanner_mock_server/spanner_active_record_with_mock_server_test.rb @@ -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.utc(2017, 7, 4, 14, 19, 0) Singer.transaction do Singer.create(first_name: "Dave", last_name: "Allison", last_performance: timestamp_string) @@ -689,7 +689,7 @@ 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.utc(2017, 7, 4, 14, 19, 0) 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