Error `TypeError: can't cast RGeo::Geographic::SphericalPolygonImpl` on Rails 7.2 and HEAD `activerecord-postgis-adapter`

Question

Error `TypeError: can't cast RGeo::Geographic::SphericalPolygonImpl` on Rails 7.2 and HEAD `activerecord-postgis-adapter`

stevegeek opened this issue 4 months ago · 22 comments

The following reproduction script shows the error.

It works on Rails 7.0/7.1 with the latest release of activerecord-postgis-adapter but fails when run with Rails 7.2 and master branch of activerecord-postgis-adapter

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "rails", "~> 7.2"
  # If you want to test against edge Rails replace the previous line with this:
  # gem "rails", github: "rails/rails", branch: "main"

  gem "pg"
  gem "activerecord-postgis-adapter", github: "rgeo/activerecord-postgis-adapter", branch: "master"
  gem "rgeo-activerecord", "~> 7"
end

require "active_record"
require "minitest/autorun"
require "logger"


# `createdb postgis_test_db`

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(
  "postgis://localhost/postgis_test_db"
)
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  enable_extension 'postgis'

  create_table :posts, force: true do |t|
    t.st_point :geo
  end
end

class Post < ActiveRecord::Base
end

class BugTest < Minitest::Test
  def test_geo_where
    post = Post.create!(geo: RGeo::Geos.factory(srid: 4326).point(-122, 47))
    point = post.geo
    geo_factory = RGeo::Geographic.spherical_factory(srid: 4326, buffer_resolution: 4)
    area_center = geo_factory.point(*point.coordinates)
    circle = area_center.buffer(10_000)
    polygon = geo_factory.multi_polygon([circle])
    assert_equal 1, Post.where("ST_DWithin(?, posts.geo, ?)", polygon, 1).count
  end
end

Exception:

TypeError: can't cast RGeo::Geographic::SphericalPolygonImpl
    /Users/stephen/.asdf/installs/ruby/3.3.4/lib/ruby/gems/3.3.0/gems/activerecord-7.2.0/lib/active_record/connection_adapters/abstract/quoting.rb:105:in `type_cast'
    /Users/stephen/.asdf/installs/ruby/3.3.4/lib/ruby/gems/3.3.0/gems/activerecord-7.2.0/lib/active_record/connection_adapters/postgresql/quoting.rb:185:in `type_cast'
    /Users/stephen/.asdf/installs/ruby/3.3.4/lib/ruby/gems/3.3.0/gems/activerecord-7.2.0/lib/active_record/connection_adapters/abstract/quoting.rb:229:in `block in type_casted_binds'
    /Users/stephen/.asdf/installs/ruby/3.3.4/lib/ruby/gems/3.3.0/gems/activerecord-7.2.0/lib/active_record/connection_adapters/abstract/quoting.rb:225:in `map'
    /Users/stephen/.asdf/installs/ruby/3.3.4/lib/ruby/gems/3.3.0/gems/activerecord-7.2.0/lib/active_record/connection_adapters/abstract/quoting.rb:225:in `type_casted_binds'
...

Thanks for all the work on the postgis adapter for Rails!

Answer 1 · 2024-08-23T11:46:02.000Z

Same error here. Downgrading to Rails 7.1 in the meantime!

Thank you guys for all the help, and let me know if I can chip in with any money to help you free up time for this.

Answer 2 · 2024-08-23T16:47:00.000Z

I'm getting a slightly different can't cast RGeo::Geographic::SphericalLineStringImpl error in my test suite when running it against Rails 7.2.1. I looked into it a little bit, and was able to get a little more detail - hopefully it's helpful.

The query it's running when it's hitting this error is:

Node.where("ST_DWITHIN(geog, ?, 25)", geog)

If I adjust that query to this unsafe version, it does work:

Node.where("ST_DWITHIN(geog, '#{geog}', 25)")

Answer 3 · 2024-08-26T18:56:17.000Z

I have not this problem on rails 7.2.1

test.log

Answer 4 · 2024-08-28T17:07:12.000Z

@JamesChevalier I have a similar experience in terms of an unsafe version of query working, but throwing an error when using the safe version. Also Rails 7.2.1.

Works:

details.common.where("ST_CONTAINS(way, st_transform(GeomFromEWKT('srid=4326;POINT(#{lng.to_f}  #{lat.to_f})'), 3857))")

Errors:

details.common.where("ST_CONTAINS(way, st_transform(GeomFromEWKT('srid=4326;POINT(:lng  :lat)'), 3857))", {lng: lng.to_f, lat: lat.to_f})

Throws: ActiveRecord::StatementInvalid: PG::IndeterminateDatatype: ERROR: could not determine data type of parameter $1

Answer 5 · 2024-09-18T16:06:36.000Z

Some additional details that 🤷 may or may not be helpful...

My initializer:

RGEO_FACTORY = RGeo::Geographic.spherical_factory(srid: 4326, uses_lenient_assertions: true)

RGeo::ActiveRecord::SpatialFactoryStore.instance.tap do |config|
  config.default = RGEO_FACTORY
end

I tested with simple_mercator_factory and that also produced the same error.
I tested against the bump-7-2 branch from #405 and that also produced the same error.

Answer 6 · 2024-09-18T16:53:49.000Z

@JamesChevalier do you have a full one file example tested against bump-7-2? That would be pretty useful! (here's the activerecord template)

Answer 7 · 2024-09-18T18:36:56.000Z

Thanks for the suggestion, @BuonOmo

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"
  git_source(:github) { |repo| "https://github.com/#{repo}.git" }
  gem "rails", "~> 7.2"
  gem "pg"
  gem "activerecord-postgis-adapter", github: "rgeo/activerecord-postgis-adapter", branch: "bump-7-2"
end

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.logger = Logger.new(STDOUT)
ActiveRecord::Base.establish_connection("postgis://localhost/postgis_test_db")
ActiveRecord::Schema.define do
  enable_extension "postgis"

  create_table :posts, force: true do |t|
    t.geometry :geo, geographic: true
  end
end

class Post < ActiveRecord::Base
end

class BugTest < Minitest::Test
  def test_geo_where
    post = Post.create!(geo: RGeo::Geographic.spherical_factory(srid: 4326, uses_lenient_assertions: true).point(-122, 47))
    assert_equal 1, Post.where("ST_DWITHIN(geo, ?, 25)", post.geo).count
  end
end

Answer 8 · 2024-09-18T19:20:42.000Z

Thank you! It might actually be related to the failing tests that we currently have and are blocking us from releasing this branch. I'll hopefully have a look within the next two weeks.

Answer 9 · 2024-09-18T20:26:31.000Z

@BuonOmo If helpful, here's another failing one file example. This is throwing ActiveRecord::StatementInvalid: PG::IndeterminateDatatype: ERROR: could not determine data type of parameter $1

Using an unsafe query instead -- commented out after the failing assertion -- passes.

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"
  git_source(:github) { |repo| "https://github.com/#{repo}.git" }
  gem "rails", "~> 7.2"
  gem "pg"
  gem "activerecord-postgis-adapter", github: "rgeo/activerecord-postgis-adapter", branch: "bump-7-2"
end

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.logger = Logger.new(STDOUT)
ActiveRecord::Base.establish_connection("postgis://localhost/postgis_test_db")
ActiveRecord::Schema.define do
  enable_extension "postgis"

  create_table :posts, force: true do |t|
    t.geometry :geo, limit: {srid: 3857, type: "geometry"}
  end
end

class Post < ActiveRecord::Base
end

class BugTest < Minitest::Test
  def test_geo_where
    post = Post.create!(geo: "POLYGON ((-9953167.702116467 5323537.15351723, -9953167.702116467 5323635.125782488, -9953041.625807118 5323635.125782488, -9953041.625807118 5323537.15351723, -9953167.702116467 5323537.15351723))")
		post_with_center = Post.select("ST_Y(ST_centroid(ST_transform(geo,4326))) as lat, ST_X(ST_centroid(ST_transform(geo,4326))) as lng").first
		lng = post_with_center.lng
		lat = post_with_center.lat
    assert_equal 1, Post.where("ST_CONTAINS(geo, st_transform(GeomFromEWKT('srid=4326;POINT(?  ?)'), 3857))", lng, lat).count
    # assert_equal 1, Post.where("ST_CONTAINS(geo, st_transform(GeomFromEWKT('srid=4326;POINT(#{lng} #{lat})'), 3857))").count
  end
end

Answer 10 · 2024-09-24T15:56:27.000Z

So it looks like this was already failing before 7.2. @jnweaver do you have a timing in history where it was not failing ? As of now the failure makes a lot of sense, there is not any overriding of type casting at all, so the code Post.where(query, bindings) never goes through this codebase!

@seuros @keithdoggett any idea of history related to this issue ?

Here's a diff that would fix the issue:

diff --git a/lib/active_record/connection_adapters/postgis/quoting.rb b/lib/active_record/connection_adapters/postgis/quoting.rb
new file mode 100644
index 0000000..1d7c8b9
--- /dev/null
+++ b/lib/active_record/connection_adapters/postgis/quoting.rb
@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+module ActiveRecord
+  module ConnectionAdapters
+    module PostGIS
+      module Quoting
+        def type_cast(value)
+          case value
+          when RGeo::Feature::Instance
+            value.to_s
+          else
+            super
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/active_record/connection_adapters/postgis_adapter.rb b/lib/active_record/connection_adapters/postgis_adapter.rb
index 9b890d5..7b80558 100644
--- a/lib/active_record/connection_adapters/postgis_adapter.rb
+++ b/lib/active_record/connection_adapters/postgis_adapter.rb
@@ -16,6 +16,7 @@ require_relative "postgis/spatial_column"
 require_relative "postgis/arel_tosql"
 require_relative "postgis/oid/spatial"
 require_relative "postgis/oid/date_time"
+require_relative "postgis/quoting"
 require_relative "postgis/type" # has to be after oid/*
 # :startdoc:
 
@@ -42,6 +43,7 @@ module ActiveRecord
       # http://postgis.17.x6.nabble.com/Default-SRID-td5001115.html
       DEFAULT_SRID = 0
 
+      include PostGIS::Quoting
       include PostGIS::SchemaStatements
       include PostGIS::DatabaseStatements

But I don't see time to test this now, and it does not change anything with the actual PR so I would not prioritize it. In the mean time, you can just #to_s and still be safe :)

Answer 11 · 2024-09-24T19:11:56.000Z

Thanks @BuonOmo

I may not understand what you are asking, but this was not failing with Rails 7.1 and activerecord-postgis-adapter 9.0.2

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"
  git_source(:github) { |repo| "https://github.com/#{repo}.git" }
  gem "rails", "7.1.3.4"
  gem "pg"
  gem "activerecord-postgis-adapter", "9.0.2"
end

require "active_record"
require "minitest/autorun"
require "logger"

ActiveRecord::Base.logger = Logger.new(STDOUT)
ActiveRecord::Base.establish_connection("postgis://localhost/postgis_test_db")
ActiveRecord::Schema.define do
  enable_extension "postgis"

  create_table :posts, force: true do |t|
    t.geometry :geo, limit: {srid: 3857, type: "geometry"}
  end
end

class Post < ActiveRecord::Base
end

class BugTest < Minitest::Test
  def test_geo_where
    post = Post.create!(geo: "POLYGON ((-9953167.702116467 5323537.15351723, -9953167.702116467 5323635.125782488, -9953041.625807118 5323635.125782488, -9953041.625807118 5323537.15351723, -9953167.702116467 5323537.15351723))")
		post_with_center = Post.select("ST_Y(ST_centroid(ST_transform(geo,4326))) as lat, ST_X(ST_centroid(ST_transform(geo,4326))) as lng").first
		lng = post_with_center.lng
		lat = post_with_center.lat
    assert_equal 1, Post.where("ST_CONTAINS(geo, st_transform(GeomFromEWKT('srid=4326;POINT(?  ?)'), 3857))", lng, lat).count
    # assert_equal 1, Post.where("ST_CONTAINS(geo, st_transform(GeomFromEWKT('srid=4326;POINT(#{lng} #{lat})'), 3857))").count
  end
end

At any rate, I can definitely work around this when a version supporting Rails 7.2 is available.

Answer 12 · 2024-09-26T14:26:36.000Z

@jnweaver if this induces a bug in 7.2 (which we see it does thanks to your last comment!), then a version supporting rails 7.2 depends on this!

It is taking a lot of my personnal time, when I'll be able to make some money out of my OSS projects I might be able to contribute faster! In the mean time, could you help now ?

I've been working on the difference between the two versions (basically running a debugger and checking the stacktrace after #build_where_clause using the irb command u /build_where_clause/). And it seems that is where things change.

I think the culprit commit in rails is rails/rails@8e6a5de. Would you (or anyone) have time to investigate further?

Answer 13 · 2024-09-26T17:18:39.000Z

I had a few minutes to dig into things - here are some details, in case they're helpful (apologies if I'm sharing information you already know).

activerecord v7.2 introduced parts = [build_bound_sql_literal(opts, rest)]
Previously, the query would use parts = [klass.sanitize_sql(rest.empty? ? opts : [opts, *rest])]

So, for a query in the format of:

YourModel.where("ST_DWITHIN(geog, ?, 25)", geog)

The old version of this code would produce output like:

["ST_DWITHIN(geog, 'STRING_VERSION_OF_GEO_DATA', 25)"]

Whereas the new version of this code produces output like:

[#<Arel::Nodes::BoundSqlLiteral "(ST_DWITHIN(geog, ?, 25))" [#<RGeo::Geographic::SphericalLineStringImpl:0x3d554 "LINESTRING (ALL, THE, COORDINATES)">]>]

I don't think it's super relevant, but when the code flows through the build_bound_sql_literal method, it goes into the else branch where value is passed along without modification because value.respond_to?(:id_for_database) is false.

Answer 14 · 2024-09-26T20:04:57.000Z

Your PostGIS::Quoting diff, above, does resolve the issue. It makes sense, as well, since the thing that's missing in Rails 7.2+ is the sql sanitization that was once done via parts = [klass.sanitize_sql(rest.empty? ? opts : [opts, *rest])]

The sanitize_sql code flows like:

sanitize_sql calls sanitize_sql_for_conditions
which in turn calls sanitize_sql_array
and since the statement includes ? it calls replace_bind_variables
which then flows through replace_bind_variable and into quote_bound_value (again because of the ?)
finally calling connection.quote(connection.cast_bound_value(value))

That last step is where we land on the logic of the commit that you linked to. That commit was specifically (if I'm understanding correctly) removing the use of connection in too many places. Your use of to_s falls in line with the spirit of the commit, in that it's also not relying on connection.

One thing I don't know yet is whether when RGeo::Feature::Instance catches all possible cases of to_s being needed. Based on what I've seen so far (if I'm understanding things correctly), I'd answer the question of "In which cases does activerecord-postgis-adapter need to self-sanitize?" with "All of them."

This is the pull request that goes with that commit: rails/rails#51139

UPDATE: I notice that quote is already defined at https://github.com/rgeo/activerecord-postgis-adapter/blob/master/lib/active_record/connection_adapters/postgis_adapter.rb#L121-L129 and since I don't have experience at this low level I'm unsure if that is/will be an issue

Answer 15 · 2024-10-02T12:49:30.000Z

Confirmed that #402 (comment) resolves the issue for me too.

If anyone needs it while @BuonOmo takes the necessary time to work out a permanent and tested fix, I've put it in a branch:

gem "activerecord-postgis-adapter", "~> 9.0", github: 'fishpercolator/activerecord-postgis-adapter', branch: 'bump-7-2-quoting-fix'

Answer 16 · 2024-10-06T13:54:08.000Z

Does someone tried to run it against the new HEAD rails 8.0.0.beta1 ?

Answer 17 · 2024-10-07T11:08:22.000Z

@pedantic-git you could open a PR :)

Answer 18 · 2024-10-07T11:10:18.000Z

@BuonOmo I certainly could, but my change is built on top of your PR branch in #405 so I thought you might like to cherry-pick the commit into that branch rather than having two PRs open with largely the same diff. Happy to open a PR if you disagree!

Answer 19 · 2024-10-07T11:50:30.000Z

@pedantic-git if your branch fixes some of the blocking failing tests, I'll cherry-pick it. Otherwise lets wait for the merging of #405 before opening a new PR indeed :)

Answer 20 · 2024-10-07T12:28:21.000Z

Thanks! Very happy to wait and offer any assistance I can in the completion of #405