Skip to content

fix: infer types for untyped query binds - #423

Open
zhaitz wants to merge 1 commit into
googleapis:mainfrom
indie-technologies:zach/upstream-untyped-bind-types
Open

zhaitz wants to merge 1 commit into
googleapis:mainfrom
indie-technologies:zach/upstream-untyped-bind-types

Conversation

@zhaitz

@zhaitz zhaitz commented Sep 14, 2026

Copy link
Copy Markdown
  • queries like 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 int64
  • so, update to_types_and_params to infer the correct Spanner types for these values
  • uses an allowlist to preserve the existing behavior for arrays and other unsupported types

@product-auto-label product-auto-label Bot added the api: spanner Issues related to the googleapis/ruby-spanner-activerecord API. label Sep 14, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment on lines +375 to +383
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

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
        end

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

@zhaitz
zhaitz marked this pull request as ready for review September 14, 2026 16:52
@zhaitz
zhaitz requested review from a team, olavloite and sakthivelmanii as code owners September 14, 2026 16:52
@zhaitz zhaitz changed the title Infer types untyped query binds fix: infer types for untyped query binds Sep 14, 2026
@zhaitz
zhaitz force-pushed the zach/upstream-untyped-bind-types branch from 5e8af50 to d31450e Compare September 14, 2026 16:54
* 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.
@zhaitz
zhaitz force-pushed the zach/upstream-untyped-bind-types branch from d31450e to a588dd0 Compare September 14, 2026 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: spanner Issues related to the googleapis/ruby-spanner-activerecord API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant