fix: use DateTime instead of Time for time data type

[sakthivelmanii]

In Rails 8.1 (specifically following rails/rails@8eeaa38), 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.

[gemini-code-assist]

Code Review

This pull request updates ActiveRecord::Type::Spanner::Time to inherit from ActiveRecord::Type::DateTime instead of ActiveRecord::Type::Time, removing custom casting logic in favor of standard Rails behavior. It also updates and adds corresponding tests for timezone-aware attributes, subsecond precision, and multiparameter datetimes. The feedback suggests reloading the records from the database before asserting their values in several test cases to ensure the persisted data is correctly verified.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the Spanner Time type in ActiveRecord to inherit from ActiveRecord::Type::DateTime instead of ActiveRecord::Type::Time, aligning its behavior with datetime handling and improving support for multiparameter assignments and timezone-aware attributes. Associated tests have been updated and expanded to verify these changes. However, a critical syntax error was introduced in schema_dumper_test.rb due to a misplaced closing parenthesis in an assertion, which needs to be corrected.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request updates ActiveRecord::Type::Spanner::Time to inherit from ActiveRecord::Type::DateTime instead of ActiveRecord::Type::Time, modifying how multiparameter assignments are handled. It also updates the test suite to cover timezone-aware attributes, subsecond precision, and multiparameter datetime parsing. The review feedback suggests using Time.now.utc.floor for a more idiomatic way to strip subseconds, and explicitly setting and restoring Time.zone in tests to avoid potential NoMethodError flakiness.

[sakthivelmanii]

/gemini review

[gemini-code-assist]

Code Review

This pull request updates the ActiveRecord::Type::Spanner::Time class to inherit from ActiveRecord::Type::DateTime instead of ActiveRecord::Type::Time, enabling better support for timezone-aware attributes, nanosecond precision, and multiparameter assignments. It also adds comprehensive test coverage for these scenarios and introduces a helper in the Rakefile to wait for the Spanner emulator to be ready. However, a critical issue was identified in the new cast_value implementation: the is_utc? method is no longer inherited from the new parent class, which will cause a NoMethodError at runtime. This should be replaced with a direct check of ActiveRecord.default_timezone == :utc.

[gemini-code-assist]

Since ActiveRecord::Type::Spanner::Time now inherits from ActiveRecord::Type::DateTime instead of ActiveRecord::Type::Time, the private helper method is_utc? is no longer inherited and will raise a NoMethodError at runtime when casting string values.

You should replace is_utc? with ActiveRecord.default_timezone == :utc directly to check the default timezone configuration.

              if ActiveRecord.default_timezone == :utc

[sakthivelmanii]

module ActiveRecord
  module Type
    class DateTime < ActiveModel::Type::DateTime
      include Internal::Timezone # <--- Included here
    end
  end
end

module ActiveRecord
  module Type
    module Internal
      module Timezone
        def is_utc?
          ActiveRecord.default_timezone == :utc
        end
        # ...
      end
    end
  end
end

This is false positive

[sakthivelmanii]

To support nanoseconds precision in GoogleSQL dialect. By default cast_value is stripping the precision to 6.

[sakthivelmanii]

To support nanoseconds precision in GoogleSQL dialect. By default cast_value is stripping the precision to 6.

[olavloite]

This changes the behavior of the provider, and I think that we should document that:

• If the application had set ActiveRecord.default_timezone = :utc (this is the default)
• Then prior to this change, if you assigned a timestamp string without a timezone (e.g. "2017-07-04 14:19:10") to a timestamp property, then it would be interpreted in the local timezone. This is not in line with the intended behavior of ActiveRecord, and also not in line with other providers, and as such a bug.
• After this change, it will correctly be interpreted in UTC. While this is a bug fix, it is changing the default behavior, and we should call this out clearly in the release notes.
• The workaround for customers who want to retain the original behavior would be to set ActiveRecord.default_timezone = :local

I actually also needed to apply these changes locally to get the tests to pass (locally):

diff --git a/acceptance/cases/type/time_test.rb b/acceptance/cases/type/time_test.rb
--- a/acceptance/cases/type/time_test.rb
+++ b/acceptance/cases/type/time_test.rb
@@ -47,7 +47,11 @@ module ActiveRecord
       def test_date_time_string_value_with_subsecond_precision
-        expected_time = ::Time.local 2017, 07, 4, 14, 19, 10, 897761
+        expected_time = if ActiveRecord.default_timezone == :utc
+                          ::Time.utc 2017, 7, 4, 14, 19, 10, 897761
+                        else
+                          ::Time.local 2017, 7, 4, 14, 19, 10, 897761
+                        end
 
         string_value = "2017-07-04 14:19:10.897761"
 
@@ -60,7 +60,11 @@ module ActiveRecord
       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 = if ActiveRecord.default_timezone == :utc
+                          ::Time.utc 2017, 7, 4, 14, 19
+                        else
+                          ::Time.local 2017, 7, 4, 14, 19
+                        end
 
         record = TestTypeModel.new start_time: string_value
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
--- 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
@@ -672,3 +672,7 @@ module MockServerTests
       timestamp_string = "04/07/2017 2:19pm"
-      timestamp = ::Time.parse(timestamp_string)
+      timestamp = if ActiveRecord.default_timezone == :utc
+                    ::DateTime.parse(timestamp_string).to_time.getutc
+                  else
+                    ::Time.parse(timestamp_string).getlocal
+                  end
 
@@ -695,3 +695,7 @@ module MockServerTests
       timestamp_string = "04/07/2017 2:19pm"
-      timestamp = ::Time.parse(timestamp_string)
+      timestamp = if ActiveRecord.default_timezone == :utc
+                    ::DateTime.parse(timestamp_string).to_time.getutc
+                  else
+                    ::Time.parse(timestamp_string).getlocal
+                  end
       Singer.find_by(last_performance: timestamp_string)