feat!: ActiveRecord 7.2 support #397

seuros · 2024-06-18T15:07:52Z

Here is a working implementation for Rails 7.2

ping @theonlyriddle @anthony0030 @ugisozols @keithdoggett

JRuby code was removed until it start supporting Rails 7.1 and 7.2

closes #395

seuros · 2024-06-19T20:11:55Z

@keithdoggett : The errors are flacky and happen when the adapter is not connected yet in CI.
rerunning the test will make them pass.

lib/activerecord-postgis-adapter.rb

test/cases/basic_test.rb

seuros · 2024-06-19T20:15:38Z

test/cases/schema_statements_test.rb

@@ -5,22 +5,25 @@
 module PostGIS
  class SchemaStatementsTest < ActiveSupport::TestCase
    def test_initialize_type_map
-      initialized_types = SpatialModel.connection.send(:type_map).keys
+      SpatialModel.with_connection do |connection|
+        assert connection.connected?


This is flacky.

Probably due to how the adapter is registered now?

No, it just flacky in CI.

I will focus on the cause later.

If you rerun the failed builds, they will become green.

Was it flaky before this PR?

Yes, in CI.

lib/active_record/connection_adapters/postgis/create_connection.rb

.github/workflows/tests.yml

keithdoggett · 2024-06-19T22:05:48Z

@seuros thank you for submitting this I'll review this in the morning. Thanks for all the help.

anthony0030 · 2024-06-20T07:05:25Z

@seuros thank you for your continued effort. I and @theonlyriddle will be unable to work on the project that we need this for, for some time.

We appreciate your effort very much!

keithdoggett

@seuros looks good. Just a few questions here. Thanks again

Gemfile

lib/active_record/connection_adapters/postgis/create_connection.rb

test/cases/basic_test.rb

keithdoggett · 2024-06-20T20:23:06Z

test/cases/schema_statements_test.rb

@@ -5,22 +5,25 @@
 module PostGIS
  class SchemaStatementsTest < ActiveSupport::TestCase
    def test_initialize_type_map
-      initialized_types = SpatialModel.connection.send(:type_map).keys
+      SpatialModel.with_connection do |connection|
+        assert connection.connected?


Probably due to how the adapter is registered now?

test/rake_helper.rb

BuonOmo

A few reviews, to me the Gemfile part is important. I also have doubts about removing the JRUBY code, is it really bothering us? My opinion is that support will come at some point and that we will just have to write the same code again...

Besides that I think we're pretty good. Thank you for the dedication !

.github/workflows/tests.yml

Gemfile

lib/active_record/connection_adapters/postgis/create_connection.rb

BuonOmo · 2024-06-23T10:03:10Z

test/cases/schema_statements_test.rb

@@ -5,22 +5,25 @@
 module PostGIS
  class SchemaStatementsTest < ActiveSupport::TestCase
    def test_initialize_type_map
-      initialized_types = SpatialModel.connection.send(:type_map).keys
+      SpatialModel.with_connection do |connection|
+        assert connection.connected?


Was it flaky before this PR?

BuonOmo · 2024-06-23T14:01:24Z

I think there is still some misunderstanding in the Gemfile part, to me it is important that we test on the latest AR patch.

seuros · 2024-07-01T12:48:33Z

@BuonOmo , I get it.
I want to lock it to ˜> 7.2.0 when we release it .

BuonOmo · 2024-07-01T14:50:56Z

So the script you removed allowed to verify that we were properly testing against the latest patch. What you are doing isn't, unless I missed something which you could explain to me ?

seuros · 2024-07-01T15:00:05Z

No the script that i removed, pull in each run the list from rubygems , select the last one and use it as a tag.

You can bypass the network calls by setting AR_VERSION

BuonOmo · 2024-07-02T05:38:31Z

No the script that i removed, pull in each run the list from rubygems , select the last one and use it as a tag.

Which means it forces usage of the very last patch, which is what want IMHO!

seuros · 2024-07-02T08:29:30Z

fair! I restored the file.

theonlyriddle · 2024-07-02T13:58:51Z

@seuros I updated my gem to use your current master branch to see how things are running. When I started my application, it crashed at my initializer file config/initializers/rgeo.rb with the error uninitialized constant RGeo::ActiveRecord (NameError).

The contents of the initializer file are below.

RGeo::ActiveRecord::SpatialFactoryStore.instance.tap do |config|
  # By default, use the GEOS implementation for spatial columns.
  # config.default = RGeo::Geos.factory_generator

  # # But use a geographic implementation for point columns.
  # config.register(RGeo::Geographic.spherical_factory(srid: 4326), geo_type: "point")
end

class RGeo::GeoJSON::Feature
  def properties
    @properties
  end
end

I only have this problem on your branch. Just thought to bring this to your attention.

seuros · 2024-07-02T14:46:07Z

@theonlyriddle

RGeo::ActiveRecord::SpatialFactoryStore.instance.tap do |config|
  # By default, use the GEOS implementation for spatial columns.
  # config.default = RGeo::Geos.factory_generator

  # # But use a geographic implementation for point columns.
  # config.register(RGeo::Geographic.spherical_factory(srid: 4326), geo_type: "point")
end

Is doing nothing in your case.

theonlyriddle · 2024-07-04T06:41:21Z

@seuros I agree with you, the code I posted does nothing as it's commented out.

However, this code from the documentation (essentially the same as what I posted) results in the same error that I described above.

RGeo::ActiveRecord::SpatialFactoryStore.instance.tap do |config|
  # By default, use the GEOS implementation for spatial columns.
  config.default = RGeo::Geos.factory_generator

  # But use a geographic implementation for point columns.
  config.register(RGeo::Geographic.spherical_factory(srid: 4326), geo_type: "point")
end

Gemfile

seuros · 2024-07-04T07:57:44Z

@theonlyriddle, Right!
we need to upgrade the documentation to configure it after initialization.
Now the adapters are lazy loaded and available only after first usage.

BuonOmo

LGTM, I think once @theonlyriddle's issue is sorted out we should be good to go!

seuros · 2024-07-04T09:36:48Z

@BuonOmo

seuros force-pushed the master branch 2 times, most recently from 4cb4959 to b1485ec Compare June 18, 2024 15:16

feat!: ActiveRecord 7.2 support

ab0c104

seuros force-pushed the master branch from b1485ec to ab0c104 Compare June 18, 2024 15:19

seuros marked this pull request as ready for review June 19, 2024 20:07

seuros commented Jun 19, 2024

View reviewed changes

keithdoggett approved these changes Jun 20, 2024

View reviewed changes

keithdoggett assigned BuonOmo Jun 20, 2024

BuonOmo reviewed Jun 23, 2024

View reviewed changes

remove version pg 11

d2cf602

seuros requested review from BuonOmo and keithdoggett June 23, 2024 11:00

Update Gemfile

d8db26f

BuonOmo reviewed Jul 4, 2024

View reviewed changes

Gemfile Outdated Show resolved Hide resolved

BuonOmo approved these changes Jul 4, 2024

View reviewed changes

fix PR concerns

dbb3ac3

BuonOmo merged commit 9ed6cb5 into rgeo:master Jul 5, 2024
18 of 20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: ActiveRecord 7.2 support #397

feat!: ActiveRecord 7.2 support #397

seuros commented Jun 18, 2024

seuros commented Jun 19, 2024

seuros Jun 19, 2024

keithdoggett Jun 20, 2024

seuros Jun 20, 2024 •

edited

Loading

BuonOmo Jun 23, 2024

seuros Jun 23, 2024

keithdoggett commented Jun 19, 2024

anthony0030 commented Jun 20, 2024

keithdoggett left a comment

keithdoggett Jun 20, 2024

BuonOmo left a comment

BuonOmo Jun 23, 2024

BuonOmo commented Jun 23, 2024

seuros commented Jul 1, 2024

BuonOmo commented Jul 1, 2024

seuros commented Jul 1, 2024 •

edited

Loading

BuonOmo commented Jul 2, 2024

seuros commented Jul 2, 2024

theonlyriddle commented Jul 2, 2024

seuros commented Jul 2, 2024

theonlyriddle commented Jul 4, 2024

seuros commented Jul 4, 2024

BuonOmo left a comment

seuros commented Jul 4, 2024

feat!: ActiveRecord 7.2 support #397

feat!: ActiveRecord 7.2 support #397

Conversation

seuros commented Jun 18, 2024

seuros commented Jun 19, 2024

seuros Jun 19, 2024

Choose a reason for hiding this comment

keithdoggett Jun 20, 2024

Choose a reason for hiding this comment

seuros Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

BuonOmo Jun 23, 2024

Choose a reason for hiding this comment

seuros Jun 23, 2024

Choose a reason for hiding this comment

keithdoggett commented Jun 19, 2024

anthony0030 commented Jun 20, 2024

keithdoggett left a comment

Choose a reason for hiding this comment

keithdoggett Jun 20, 2024

Choose a reason for hiding this comment

BuonOmo left a comment

Choose a reason for hiding this comment

BuonOmo Jun 23, 2024

Choose a reason for hiding this comment

BuonOmo commented Jun 23, 2024

seuros commented Jul 1, 2024

BuonOmo commented Jul 1, 2024

seuros commented Jul 1, 2024 • edited Loading

BuonOmo commented Jul 2, 2024

seuros commented Jul 2, 2024

theonlyriddle commented Jul 2, 2024

seuros commented Jul 2, 2024

theonlyriddle commented Jul 4, 2024

seuros commented Jul 4, 2024

BuonOmo left a comment

Choose a reason for hiding this comment

seuros commented Jul 4, 2024

seuros Jun 20, 2024 •

edited

Loading

seuros commented Jul 1, 2024 •

edited

Loading