Conversation
BuonOmo
left a comment
BuonOmo
left a comment
There was a problem hiding this comment.
Thank you for pushing these changes ! I have a few questions left, and I'd also like a second reviewer to see if that is really the direction we want to go (there might be issues related to this choice that we're not seeing..) @keithdoggett ?
| module MultiGeomSanitization | ||
| private | ||
|
|
||
| # NOTE connection and value order is swapped in Rails 8 |
There was a problem hiding this comment.
What should we do about this note?
There was a problem hiding this comment.
We could add a
if ActiveRecord::VERSION::MAJOR < 8|
|
||
| def test_multi_geom_sanitization | ||
| multi_geom = RGeo::Geos.factory.parse_wkt("MULTIPOLYGON (((0 0, 0 1, 1 1, 0 0)),((1 1, 0 0, 0 1, 1 1)))") | ||
| sql = "ST_DWithin(geom, :geom, :buffer)" |
There was a problem hiding this comment.
why do we need that st_dwithin function for the test?
There was a problem hiding this comment.
We do not need it. It could be SQL valid or not that has a geom interpolated.
There was a problem hiding this comment.
I think I'd keep it simple then :)
There was a problem hiding this comment.
To me a single ST function is pretty simple. Would you prefer ST_DWithin -> ST_Intersects or SPATIAL_FUNC (as the SQL doesn't have to be valid)?
| require "minitest/autorun" | ||
| require "rgeo-activerecord" | ||
| require "support/fake_record" | ||
| require "active_support/core_ext/date/acts_like" |
There was a problem hiding this comment.
AR expects it. Without it https://github.com/rails/rails/blob/6f57590388ca38ed2b83bc1207a8be13a9ba2aef/activerecord/lib/active_record/sanitization.rb#L228 fails.
There was a problem hiding this comment.
My concern is that a user loading our gem would have the same issue and need to load this as well. It is in the rails codebase that this should be loaded prior usage, not here in tests. So either our test suite is not initialising properly (I suspect that is the issue) or there is an issue in rails codebase. WDYT?
There was a problem hiding this comment.
I don't think Rails is initialised at all. The specs on initialise the minimal things required which I believe is active record.
There was a problem hiding this comment.
@nikolai-b I don't think this supersedes my comment. If we load active-record in our gem, then active-record code should be working, no?
There was a problem hiding this comment.
I'm not sure. This is because in the gem the FakeRecord::Base is being used. If instead ActiveRecord::Base is used then this require is not needed as then
https://github.com/rails/rails/blob/6f57590388ca38ed2b83bc1207a8be13a9ba2aef/activerecord/lib/active_record/base.rb#L6 which pulls in acts_like https://github.com/rails/rails/tree/6f57590388ca38ed2b83bc1207a8be13a9ba2aef/activesupport/lib/active_support/core_ext/time . Using active_record without using ActiveRecord::Base is very strange but it is what is done in the specs here.
| end | ||
| end | ||
|
|
||
| def cast_bound_value(value) |
There was a problem hiding this comment.
why using this fake record class rather than any record?
There was a problem hiding this comment.
I didn't see any record, what do you suggest? It looks like the tests don't require a database so my guess is the fake record was used to avoid a db being required for tests but I could be missing something.
There was a problem hiding this comment.
Oh you are right ! And could we use one method deeper in the backtrace rather than #sanitize_sql and not need this fake record while still testing properly ?
There was a problem hiding this comment.
There isn't a public method below #sanitize_sql, the rest are private
|
I'm now thinking that this is similar to rgeo/activerecord-postgis-adapter#402 |
2 participants
Resolves #79