MULTI geoms are bound to sql separately

[nikolai-b]

I'd expect multi geoms to be bound / cast (?) as a multi geoms in active record:

multi_geom = RGeo::Geos.factory.parse_wkt("MULTIPOLYGON (((0 0, 0 1, 1 1, 0 0)),((1 1, 0 0, 0 1, 1 1)))")
ApplicationRecord.send(:quote_bound_value, multi_geom)
=> "'00200000030000000000000001000000040000000000000000000000000000000000000000000000003ff00000000000003ff00000000000003ff000000000000000000000000000000000000000000000','00200000030000000000000001000000043ff00000000000003ff00000000000000000000000000000000000000000000000000000000000003ff00000000000003ff00000000000003ff0000000000000'"

To get a proper multi-geom I have to wrap it in an array

ApplicationRecord.send(:quote_bound_value, [multi_geom])
 => "'00200000060000000000000002000000000300000001000000040000000000000000000000000000000000000000000000003ff00000000000003ff00000000000003ff000000000000000000000000000000000000000000000000000000300000001000000043ff00000000000003ff00000000000000000000000000000000000000000000000000000000000003ff00000000000003ff00000000000003ff0000000000000'" 

This is because AR checks if a method responds to map here

Not sure how best to solve this without some nasty monkey patch

[BuonOmo]

Maybe fixing this upstream by first checking value.respond_to?(:id_for_database) before checking that it responds to map?

Otherwise how do we end up sending quote_bound_value with a multi_geom? And could it be cast to string or wrapped in an array at some point?

[nikolai-b]

I wasn't sure the implications of setting id_for_database as it is related to the PK. How about adding an acts_like_string? method?

multi_geom = RGeo::Geos.factory.parse_wkt("MULTIPOLYGON (((0 0, 0 1, 1 1, 0 0)),((1 1, 0 0, 0 1, 1 1)))")
multi_geom.define_singleton_method(:acts_like_string?) { }
ApplicationRecord.send(:quote_bound_value, multi_geom)
=> "'00200000060000000000000002000000000300000001000000040000000000000000000000000000000000000000000000003ff00000000000003ff00000000000003ff000000000000000000000000000000000000000000000000000000300000001000000043ff00000000000003ff00000000000000000000000000000000000000000000000000000000000003ff00000000000003ff00000000000003ff0000000000000'"

I can make a PR.

[BuonOmo]

I wasn't sure the implications of setting id_for_database as it is related to the PK

Sorry I don't get you. My point was to change the logic of the method you mentioned:

        def quote_bound_value(value, c = connection)
          value = value.id_for_database if value.respond_to?(:id_for_database) # Add this
          if value.respond_to?(:map) && !value.acts_like?(:string)
            values = value.map { |v| v.respond_to?(:id_for_database) ? v.id_for_database : v }
            if values.empty?
              c.quote(c.cast_bound_value(nil))
            else
              values.map! { |v| c.quote(c.cast_bound_value(v)) }.join(",")
            end
          else
            value = value.id_for_database if value.respond_to?(:id_for_database) # Remove this
            c.quote(c.cast_bound_value(value))
          end
        end

How about adding an acts_like_string? method?

I don't think a geometry could really be considered as acting like a string. You'd have to look into rails codebase for a more strict definition of what is expected by something that has the acts_like_string? method. I don't think it is just the same as respond_to?(:to_s).

[nikolai-b]

Sure you could move the value = value.id_for_database if value.respond_to?(:id_for_database) but the RGeo::Geos::CAPIMultiPolygonImpl doesn't have the method id_for_database so it would mean adding it also. I was expressing reservations about adding it as it seems to be related to a PK and it requires an upstream change.

The current Rails codebase only seems to use acts_like_string? which comes from acts_like in places related to SQL queries:

https://github.com/search?q=repo%3Arails%2Frails+acts_like%3F%28%3Astring%29&type=code

There is no guarantee that it wouldn't be used in other places in future so I understand if you don't like this suggestion. Currently acts_like?(:string) does seem to be used in cases like this, as the MULTI* geoms do act as one unit rather than an array.

[BuonOmo]

Oh got you, so it is the #quote method that is making the conversion to string. I agree that we should not go further in that direction.

Regarding #acts_like? I still feel pretty worried about misinterpretation of this duck recognition system without a clear duck definition anywhere (sometimes I do love types and interfaces!). Also, if you search for repos outside of rails there are many others.

Could we come up with another solution? Maybe somewhere else in the backtrace, before calling #quote_bound_value there is a way of wrapping the geometry in an array? What is the public method that you are calling usually to end up with this issue?

[nikolai-b]

Yes, sorry I should have told you my entrance point, it is sanitize_sql as you don't seem to support St_DWithin in this library and I wasn't sure how to cast a column to geography in a query.

sql = "ST_DWithin(geography(buildings.geom), geography(:geom), :buffer)"
Building.where(Building.sanitize_sql([ sql, geom: multi_geom, buffer: buffer ])

[BuonOmo]

So maybe in between sanitize_sql and quote_bound_value there is a method in the backtrace in which we could add some logic related to our gem.

Here we'll go in sanitize_sql_array, which uses one of replace_named_bind_variables or replace_bind_variables which both use replace_bind_variable. So we could also override replace_bind_variable with some of our logic, we'd have to find something that is not breaking anything though!