Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: ActiveRecord 7.2 support #397

Merged
merged 4 commits into from
Jul 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ jobs:
strategy:
fail-fast: false
matrix:
ruby: [ruby-head, '3.2', '3.1', '3.0']
pg: [11-3.0, 12-master, 13-master, 14-master, 15-master]
ruby: [ruby-head, '3.3', '3.2', '3.1']
pg: [12-master, 13-master, 14-master, 15-master, 16-master]
steps:
- name: Set Up Actions
uses: actions/checkout@v4
Expand Down
5 changes: 1 addition & 4 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ source "https://rubygems.org"
gemspec

gem "pg", "~> 1.0", platform: :ruby
gem "activerecord-jdbcpostgresql-adapter", platform: :jruby
gem "ffi-geos", platform: :jruby
gem "byebug" if ENV["BYEBUG"]

def activerecord_version
Expand All @@ -30,13 +28,12 @@ def activerecord_version

ver["number"]
end

# Need to install for tests
gem "rails", github: "rails/rails", tag: "v#{activerecord_version}"

group :development do
# Gems used by the ActiveRecord test suite
gem "bcrypt"
gem "mocha"
gem "sqlite3"
gem "msgpack"
end
16 changes: 2 additions & 14 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,7 @@ brew install postgis
#### Ubuntu/Debian

```sh
# The second package can be replaced depending on your postgresql version
# ex. postgresql-11-postgis-2 is valid as well
sudo apt-get install postgis postgresql-12-postgis-3
sudo apt-get install postgis postgresql-16-postgis-3
```

#### Windows
Expand All @@ -59,16 +57,6 @@ Gemfile:
gem 'activerecord-postgis-adapter'
```

Gemfile for JRuby\*:

```ruby
gem 'activerecord-postgis-adapter'
gem 'activerecord-jdbcpostgresql-adapter'
gem 'ffi-geos'
```

_JRuby support for Rails 4.0 and 4.1 was added in version 2.2.0_

#### Version 9.x supports ActiveRecord 7.1

```
Expand Down Expand Up @@ -257,7 +245,7 @@ rails generate migration AddPostgisExtensionToDatabase

The migration should look something like this:
```ruby
class AddPostgisExtensionToDatabase < ActiveRecord::Migration[7.0]
class AddPostgisExtensionToDatabase < ActiveRecord::Migration[7.2]
def change
enable_extension 'postgis'
end
Expand Down
6 changes: 3 additions & 3 deletions activerecord-postgis-adapter.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@ Gem::Specification.new do |spec|
spec.files = Dir["lib/**/*", "LICENSE.txt"]
spec.platform = Gem::Platform::RUBY

spec.required_ruby_version = ">= 3.0.0"
spec.required_ruby_version = ">= 3.1.0"

spec.add_dependency "activerecord", "~> 7.1.0"
spec.add_dependency "activerecord", "~> 7.2.0.beta2"
spec.add_dependency "rgeo-activerecord", "~> 7.0.0"

spec.add_development_dependency "rake", "~> 13.0"
spec.add_development_dependency "minitest", "~> 5.4"
spec.add_development_dependency "mocha", "~> 1.1"
spec.add_development_dependency "mocha", "~> 2.4"
spec.add_development_dependency "benchmark-ips", "~> 2.12"
spec.add_development_dependency "rubocop", "~> 1.50"

Expand Down
63 changes: 20 additions & 43 deletions lib/active_record/connection_adapters/postgis/create_connection.rb
Original file line number Diff line number Diff line change
@@ -1,50 +1,27 @@
# frozen_string_literal: true

if RUBY_ENGINE == "jruby"
require "active_record/connection_adapters/jdbcpostgresql_adapter"
else
require "pg"
end

module ActiveRecord # :nodoc:
module ConnectionHandling # :nodoc:
if RUBY_ENGINE == "jruby"

# modified from https://github.com/jruby/activerecord-jdbc-adapter/blob/master/lib/arjdbc/postgresql/connection_methods.rb#L3
def postgis_connection(config)
config = config.deep_dup
config = symbolize_keys_if_necessary(config)

config[:adapter_class] = ConnectionAdapters::PostGISAdapter
postgresql_connection(config)
end

alias_method :jdbcpostgis_connection, :postgis_connection

else

# Based on the default <tt>postgresql_connection</tt> definition from ActiveRecord.
# https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
# FULL REPLACEMENT because we need to create a different class.
def postgis_connection(config)
conn_params = config.symbolize_keys.compact

# Map ActiveRecords param names to PGs.
conn_params[:user] = conn_params.delete(:username) if conn_params[:username]
conn_params[:dbname] = conn_params.delete(:database) if conn_params[:database]

# Forward only valid config params to PG.connect
valid_conn_param_keys = PG::Connection.conndefaults_hash.keys + [:requiressl]
conn_params.slice!(*valid_conn_param_keys)

ConnectionAdapters::PostGISAdapter.new(
ConnectionAdapters::PostGISAdapter.new_client(conn_params),
logger,
conn_params,
config
)
end

# Based on the default <tt>postgresql_connection</tt> definition from ActiveRecord.
# https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
# FULL REPLACEMENT because we need to create a different class.
def postgis_connection(config)
seuros marked this conversation as resolved.
Show resolved Hide resolved
conn_params = config.symbolize_keys.compact

# Map ActiveRecords param names to PGs.
conn_params[:user] = conn_params.delete(:username) if conn_params[:username]
conn_params[:dbname] = conn_params.delete(:database) if conn_params[:database]

# Forward only valid config params to PG.connect
valid_conn_param_keys = PG::Connection.conndefaults_hash.keys + [:requiressl]
conn_params.slice!(*valid_conn_param_keys)

ConnectionAdapters::PostGISAdapter.new(
ConnectionAdapters::PostGISAdapter.new_client(conn_params),
logger,
conn_params,
config
)
end
end
end
30 changes: 0 additions & 30 deletions lib/active_record/connection_adapters/postgis_adapter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@

# :stopdoc:

require "rgeo/active_record"

require "active_record/connection_adapters"
require "active_record/connection_adapters/postgresql_adapter"
require_relative "postgis/version"
require_relative "postgis/column_methods"
Expand All @@ -24,15 +21,6 @@
# :startdoc:

module ActiveRecord
module ConnectionHandling # :nodoc:
def postgis_adapter_class
ConnectionAdapters::PostGISAdapter
end

def postgis_connection(config)
postgis_adapter_class.new(config)
end
end

module ConnectionAdapters
class PostGISAdapter < PostgreSQLAdapter
Expand Down Expand Up @@ -175,21 +163,3 @@ def quote_default_expression(value, column)
]
Tasks::DatabaseTasks.register_task(/postgis/, "ActiveRecord::Tasks::PostgreSQLDatabaseTasks")
end

# if using JRUBY, create ArJdbc::PostGIS module
# and prepend it to the PostgreSQL adapter since
# it is the default adapter_spec.
# see: https://github.com/jruby/activerecord-jdbc-adapter/blob/master/lib/arjdbc/postgresql/adapter.rb#27
if RUBY_ENGINE == "jruby"
module ArJdbc
module PostGIS
ADAPTER_NAME = 'PostGIS'

def adapter_name
ADAPTER_NAME
end
end
end

ArJdbc::PostgreSQL.prepend(ArJdbc::PostGIS)
end
5 changes: 4 additions & 1 deletion lib/activerecord-postgis-adapter.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
# frozen_string_literal: true

require_relative "active_record/connection_adapters/postgis_adapter"
require 'active_record'
require "active_record/connection_adapters"
require "rgeo/active_record"
ActiveRecord::ConnectionAdapters.register("postgis", "ActiveRecord::ConnectionAdapters::PostGISAdapter", "active_record/connection_adapters/postgis_adapter")
keithdoggett marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion test/cases/basic_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ def test_point_to_json
obj = SpatialModel.new
assert_match(/"latlon":null/, obj.to_json)
obj.latlon = factory.point(1.0, 2.0)
assert_match(/"latlon":"POINT\s\(1\s2\)"/, obj.to_json)
assert_match(/"latlon":"POINT\s\(1(\.0)?\s2(\.0)?\)"/, obj.to_json)
seuros marked this conversation as resolved.
Show resolved Hide resolved
end

def test_custom_column
Expand Down
33 changes: 18 additions & 15 deletions test/cases/schema_statements_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is flacky.

Copy link
Member

Choose a reason for hiding this comment

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

Probably due to how the adapter is registered now?

Copy link
Contributor Author

@seuros seuros Jun 20, 2024

Choose a reason for hiding this comment

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

No, it just flacky in CI.

I will focus on the cause later.

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

Copy link
Member

Choose a reason for hiding this comment

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

Was it flaky before this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in CI.

initialized_types = connection.send(:type_map).keys

# PostGIS types must be initialized first, so
# ActiveRecord::ConnectionAdapters::PostgreSQLAdapter#load_additional_types can use them.
# https://github.com/rails/rails/blob/8d57cb39a88787bb6cfb7e1c481556ef6d8ede7a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L593
assert_equal initialized_types.first(9), %w[
geography
geometry
geometry_collection
line_string
multi_line_string
multi_point
multi_polygon
st_point
st_polygon
]
# PostGIS types must be initialized first, so
# ActiveRecord::ConnectionAdapters::PostgreSQLAdapter#load_additional_types can use them.
# https://github.com/rails/rails/blob/8d57cb39a88787bb6cfb7e1c481556ef6d8ede7a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L593
assert_equal initialized_types.first(9), %w[
geography
geometry
geometry_collection
line_string
multi_line_string
multi_point
multi_polygon
st_point
st_polygon
]
end
end
end
end
2 changes: 1 addition & 1 deletion test/rake_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
POSTGIS_TEST_HELPER = "test/test_helper.rb"

def ar_root
File.join(Gem.loaded_specs["rails"].full_gem_path, "activerecord")
Gem.loaded_specs["activerecord"].full_gem_path
seuros marked this conversation as resolved.
Show resolved Hide resolved
end

def postgis_test_load_paths
Expand Down
Loading