Conversation
Extending ActiveRecord::Type::Time means that string values get incorrectly coerced to have a date of 2000-01-01. See https://github.com/rails/rails/blob/main/activemodel/lib/active_model/type/time.rb#L14. This is because Time in activerecord is intended to store just the time value (and maps to a TIME type in other databases), unlike Time generally in ruby that represents a date and time.x
…Rails 8.1 Rails 8.1 moved the protected-environment guard onto the per-adapter tasks object (Tasks::AbstractTasks#check_current_protected_environment!), and DatabaseTasks#check_protected_environments! now calls it on each adapter's tasks object. SpannerDatabaseTasks predates AbstractTasks and doesn't inherit it, so db:check_protected_environments (run during db:test:prepare / load_schema / purge) raised NoMethodError. Mirror the AbstractTasks implementation plus its with_temporary_pool helper. Upstream v2.5.0/main lack this despite advertising 8.1 support.
Binds that come from raw SQL placeholders (`Arel.sql("name = ?", "abc")` on
7.1+, or `where("name = ?", "abc")` on Rails 8.1+) arrive as bare Ruby values
with no attached ActiveModel type. `to_types` only recognised query attributes,
Symbols and booleans, and declared everything else INT64, so a String bind was
rejected by Spanner with "Expected INT64". `to_params` always serialised such
binds through the Integer type as well.
Add `untyped_bind_type`, which maps String, true/false, Float, BigDecimal,
Time/DateTime and Date to the matching ActiveModel type, and use it from both
`to_types` and `to_params` so the declared type and serialised value agree.
Unrecognised values keep the previous behaviour (INT64, value sent as-is).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
fix: type untyped binds by Ruby class instead of defaulting to INT64
product-auto-label
Bot
added
the
api: spanner
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![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 updates the Spanner adapter for Rails 8.1 compatibility, including implementing the protected-environment guard, mapping untyped binds to Spanner types based on their Ruby class, and changing the Spanner Time type to inherit from DateTime. It also adds retry logic for emulator startup and pins the json gem to version < 3. Review feedback identifies a critical issue where timezone-aware ActiveSupport::TimeWithZone objects are not recognized by untyped_bind_type, defaulting to INT64, and suggests using acts_like? to correctly identify them.
| def untyped_bind_type value | ||
| case value | ||
| when ::String then ActiveModel::Type::String.new | ||
| when true, false then ActiveModel::Type::Boolean.new | ||
| when ::Float then ActiveModel::Type::Float.new | ||
| when ::BigDecimal then ActiveModel::Type::Decimal.new | ||
| # DateTime is a subclass of Date, so it must be matched before Date. | ||
| when ::Time, ::DateTime then ActiveRecord::Type::Spanner::Time.new | ||
| when ::Date then ActiveModel::Type::Date.new | ||
| end | ||
| end |
There was a problem hiding this comment.
The untyped_bind_type method currently checks if the value is an instance of ::Time or ::DateTime. However, in Rails applications, timezone-aware time values are typically instances of ActiveSupport::TimeWithZone, which does not inherit from ::Time or ::DateTime. As a result, any untyped bind containing an ActiveSupport::TimeWithZone object (such as Time.zone.now or Time.current) will not be recognized, defaulting to :INT64 and causing serialization or type mismatch errors at runtime.
To fix this, we should use Rails' idiomatic acts_like?(:time) and acts_like?(:date) helpers to correctly identify time-like and date-like objects, including ActiveSupport::TimeWithZone.
def untyped_bind_type value
if value.respond_to?(:acts_like?)
if value.acts_like?(:time)
return ActiveRecord::Type::Spanner::Time.new
elsif value.acts_like?(:date)
return ActiveModel::Type::Date.new
end
end
case value
when ::String then ActiveModel::Type::String.new
when true, false then ActiveModel::Type::Boolean.new
when ::Float then ActiveModel::Type::Float.new
when ::BigDecimal then ActiveModel::Type::Decimal.new
# DateTime is a subclass of Date, so it must be matched before Date.
when ::Time, ::DateTime then ActiveRecord::Type::Spanner::Time.new
when ::Date then ActiveModel::Type::Date.new
end
end3 participants
Merge our pinned branch into main so we can continue to work from there