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

Rails 7.2 support #31

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

denisahearn
Copy link
Contributor

Changes necessary to build the gem and get passing tests with Rails 7.2

Note: The CI build fails with Ruby 2.7. Would the maintainers of https://github.com/stadia/activerecord-mysql2rgeo-adapter be open to the idea of dropping support for Ruby 2.x?

@@ -39,7 +39,7 @@ def create_table_definition(*args, **options)
end

# override
def new_column_from_field(table_name, field)
def new_column_from_field(table_name, field, _definitions)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -22,6 +22,10 @@ def new_column_definition(name, type, **options)

column
end

def valid_column_definition_options
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -66,7 +66,7 @@ class Mysql2RgeoAdapter < Mysql2Adapter
# http://postgis.17.x6.nabble.com/Default-SRID-td5001115.html
DEFAULT_SRID = 0

def initialize(connection, logger, connection_options, config)
def initialize(...)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -137,6 +137,10 @@ def quote(value)
end
end

def with_connection
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Required by this change to ActiveRecord: https://github.com/rails/rails/pull/51162/files#diff-22750093c2231cef8c58e7e94a0a9266267d33bd990c607475e55fe285fccb8aR45

I only saw this with_connection method getting called by tests in this gem that were dumping the schema. I'm sure something more sophisticated could be created for this, however it fixed the failing tests.

@@ -1,3 +1,5 @@
# frozen_string_literal: true

require "active_record/connection_adapters/mysql2rgeo_adapter"

ActiveRecord::ConnectionAdapters.register('mysql2rgeo', 'ActiveRecord::ConnectionAdapters::Mysql2RgeoAdapter', 'active_record/connection_adapters/mysql2rgeo_adapter')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes the following error when running rake test

DEPRECATION WARNING: Database configuration specifies 'mysql2rgeo' adapter but that adapter has not been registered. Rails 7.2 has changed the way Active Record database adapters are loaded. The adapter needs to be updated to register itself rather than being loaded by convention. Ensure that the adapter in the Gemfile is at the latest version. If it is, then the adapter may need to be modified. See: https://api.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters.html#method-c-register (called from establish_test_connection at /Users/denisahearn/development/sentera/activerecord-mysql2rgeo-adapter/test/test_helper.rb:22)
/Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/activerecord-7.2.1/lib/active_record/connection_adapters.rb:85:in `resolve': Database configuration specifies 'mysql2rgeo' adapter but that adapter has not been registered. Ensure that the adapter in the Gemfile is at the latest version. If it is, then the adapter may need to be modified. (ActiveRecord::AdapterNotFound)
	from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/activerecord-7.2.1/lib/active_record/database_configurations/database_config.rb:18:in `adapter_class'
	from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/activerecord-7.2.1/lib/active_record/database_configurations/database_config.rb:30:in `validate!'
	from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/activerecord-7.2.1/lib/active_record/connection_adapters/abstract/connection_handler.rb:268:in `resolve_pool_config'
	from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/activerecord-7.2.1/lib/active_record/connection_adapters/abstract/connection_handler.rb:116:in `establish_connection'
	from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/activerecord-7.2.1/lib/active_record/connection_handling.rb:53:in `establish_connection'
	from /Users/denisahearn/development/sentera/activerecord-mysql2rgeo-adapter/test/test_helper.rb:22:in `establish_test_connection'
	from /Users/denisahearn/development/sentera/activerecord-mysql2rgeo-adapter/test/test_helper.rb:28:in `<class:SpatialModel>'
	from /Users/denisahearn/development/sentera/activerecord-mysql2rgeo-adapter/test/test_helper.rb:27:in `<top (required)>'
	from /Users/denisahearn/development/sentera/activerecord-mysql2rgeo-adapter/test/basic_test.rb:3:in `require'
	from /Users/denisahearn/development/sentera/activerecord-mysql2rgeo-adapter/test/basic_test.rb:3:in `<top (required)>'
	from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/rake-12.3.3/lib/rake/rake_test_loader.rb:12:in `require'
	from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/rake-12.3.3/lib/rake/rake_test_loader.rb:12:in `block (2 levels) in <main>'
	from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/rake-12.3.3/lib/rake/rake_test_loader.rb:11:in `each'
	from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/rake-12.3.3/lib/rake/rake_test_loader.rb:11:in `block in <main>'
	from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/rake-12.3.3/lib/rake/rake_test_loader.rb:5:in `select'
	from /Users/denisahearn/.rvm/gems/ruby-3.3.4/gems/rake-12.3.3/lib/rake/rake_test_loader.rb:5:in `<main>'

@@ -134,7 +134,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\.0\s2\.0\)"/, obj.to_json)
assert_match(/"latlon":"POINT\s\(1(\.0)?\s2(\.0)?\)"/, obj.to_json)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes this test failure:

1) Failure:
BasicTest#test_point_to_json [/Users/denisahearn/development/sentera/activerecord-mysql2rgeo-adapter/test/basic_test.rb:137]:
Expected /"latlon":"POINT\s\(1\.0\s2\.0\)"/ to match "{\"id\":null,\"latlon\":\"POINT (1 2)\",\"latlon_geo\":null}".

I didn't research what caused the change with this decimal formatting

@stadia stadia marked this pull request as draft September 4, 2024 02:09
@stadia stadia marked this pull request as ready for review September 4, 2024 06:11
@stadia stadia marked this pull request as draft September 4, 2024 06:11
@denisahearn
Copy link
Contributor Author

Merged with latest changes in main, fixed merge conflicts

@stadia stadia marked this pull request as ready for review September 8, 2024 03:59
@denisahearn
Copy link
Contributor Author

Hello. Will this PR be reviewed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant