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

Support for Rails 7.2 #5

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Support for Rails 7.2 #5

wants to merge 9 commits into from

Conversation

denisahearn
Copy link

@denisahearn denisahearn commented Aug 28, 2024

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

@@ -19,9 +19,9 @@ jobs:
BUNDLE_GEMFILE: ${{matrix.gemfile}}
strategy:
matrix:
ruby: ['3.0', '2.7']
ruby: ['3.1', '2.7']
Copy link
Author

Choose a reason for hiding this comment

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

@@ -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
Author

@denisahearn denisahearn Aug 28, 2024

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
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
Author

Choose a reason for hiding this comment

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

Comment on lines +140 to +142
def with_connection
yield self
end
Copy link
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
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
Author

@denisahearn denisahearn Aug 28, 2024

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

@denisahearn
Copy link
Author

Note that the CI build fails with Ruby 2.7. I wonder if the maintainers of https://github.com/stadia/activerecord-mysql2rgeo-adapter would entertain the idea of dropping support for Ruby 2.x?

jeff.dean and others added 5 commits September 4, 2024 11:08
The version number of Mysql2Rgeo was increased from 7.0.0 to 7.0.1, to reflect the updated dependencies like mocha and to mark the introduction of associated enhancements.
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.

3 participants