Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the Spanner adapter to dynamically resolve types for untyped bind values based on their Ruby class, falling back to :INT64 for unsupported types, and adds corresponding unit tests. The review feedback identifies a critical issue where ActiveSupport::TimeWithZone is not handled by the new untyped_bind_type method, which would cause it to fall back to :INT64 and lead to query errors. A code suggestion is provided to explicitly handle ActiveSupport::TimeWithZone.
| def untyped_bind_type value | ||
| case value | ||
| when ::String, ::Float, ::BigDecimal, ::Time, ::Date | ||
| Google::Cloud::Spanner::Convert.field_for_object value | ||
| else | ||
| # Preserve the existing fallback for nil and unsupported values. | ||
| :INT64 | ||
| end | ||
| end |
There was a problem hiding this comment.
In Rails, timezone-aware attributes and queries (such as Time.zone.now) return ActiveSupport::TimeWithZone objects rather than standard Ruby Time objects. Since ActiveSupport::TimeWithZone does not inherit from Time or Date, it will not match the case statement in untyped_bind_type and will fall back to :INT64. This will cause Spanner type mismatch errors or serialization failures when executing queries with untyped timezone-aware time binds.
We should explicitly handle ActiveSupport::TimeWithZone if it is defined.
def untyped_bind_type value
if value.is_a?(::String) || value.is_a?(::Float) || value.is_a?(::BigDecimal) || value.is_a?(::Time) || value.is_a?(::Date) || (defined?(ActiveSupport::TimeWithZone) && value.is_a?(ActiveSupport::TimeWithZone))
Google::Cloud::Spanner::Convert.field_for_object value
else
# Preserve the existing fallback for nil and unsupported values.
:INT64
end
endThere was a problem hiding this comment.
ActiveSupport::TimeWithZone overrides is_a? so this works: https://github.com/rails/rails/blob/3989ebf3473d71e4ceca28154b0b57b5bf22db24/activesupport/lib/active_support/time_with_zone.rb#L511
but can add a test to demonstrate that
5e8af50 to
d31450e
Compare
* Queries like Singer.where("first_name = ?", "Alice") on Rails 8.1
and similar Arel.sql queries send an untyped string into the
adapter, which currently declares it as INT64.
* Update to_types_and_params to infer the correct Spanner types
for these values.
* Use an allowlist to preserve existing behavior for arrays and
other unsupported types.
d31450e to
a588dd0
Compare
Singer.where("first_name = ?", "Alice")on Rails 8.1 (and similar Arel.sql queries) send an untyped string into the adapter which gets declared as int64to_types_and_paramsto infer the correct Spanner types for these values