ActiveRecord >= 6.1 compatibility #61

acroos · 2021-11-03T13:29:28Z

Description

Some method signatures changed (somewhat recently?) that started causing errors with the methods we've redefined around adding and removing indexes. We need to use the new method signatures so we're compatible with all versions.

Fixes #60
Fixes #59

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Manually run add_index and remove_index migrations on test ActiveRecord app
Add more specs

Checklist:

My code follows the style guidelines set by rubocop
I have performed a self-review of my own code
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules
I have checked my code and corrected any misspellings

acroos · 2021-11-03T21:51:13Z

ok @grdw this is finally ready!

acroos · 2021-11-03T21:55:32Z

.github/workflows/tests.yml

@@ -43,4 +45,4 @@ jobs:
      - name: Run rubocop
        run: bundle exec rake rubocop
      - name: Run tests
-        run: bundle exec rake spec
+        run: bundle exec --gemfile ${{ matrix.gemfile }} rspec


this makes 100% sure that the gems used are the ones in the specified Gemfile (even though they were the ones installed in a previous step)

acroos · 2021-11-03T21:56:05Z

.rubocop.yml

 Metrics/BlockLength:
  Exclude:
    - spec/**/*_spec.rb

+Metrics/ClassLength:
+  CountAsOne:


copying some common styles I use, I tend to find the default strictness to be a bit of a headache

acroos · 2021-11-03T21:57:00Z

CHANGELOG.md

@@ -1,5 +1,8 @@
 # CHANGELOG

+## 0.3.0


went with a minor bump because (1) we're still at a 0. release so no rules! and (2) this fixes core functionality for a number of significant usages

acroos · 2021-11-03T21:57:47Z

bin/test_all_versions

@@ -0,0 +1,19 @@
+#!/bin/bash


this is just for testing locally. it won't try all the ruby versions, but at least it'll go through each ActiveRecord version. it's much more likely that we have a bug specific to an AR version than a ruby version

acroos · 2021-11-03T21:58:06Z

ghost_adapter.gemspec

@@ -18,11 +18,11 @@ Gem::Specification.new do |spec|
  # Specify which files should be added to the gem when it is released.
  # The `git ls-files -z` loads the files in the RubyGem that have been added into git.
  spec.files = Dir.chdir(File.expand_path(__dir__)) do
-    `git ls-files -z`.split("\x0").reject { |f| f.match(%r{^(test|spec|features|doc)/}) }
+    `git ls-files -z`.split("\x0").reject { |f| f.match(%r{^(test|spec|features|doc|bin)/}) }


no reason to have the test script and console included in the packaged gem

acroos · 2021-11-03T21:58:33Z

ghost_adapter.gemspec

  end
  spec.require_paths = ['lib']

-  spec.add_dependency 'activerecord', '>= 5'
+  spec.add_dependency 'activerecord', '>= 5', '< 6.2'


starting to test on 7.0 alpha already, but I didn't want to include it in here yet

acroos · 2021-11-03T21:59:13Z

lib/active_record/connection_adapters/mysql2_ghost_adapter.rb

-        index_name, index_type, index_columns, index_options = add_index_options(table_name, column_name, options)
-        execute "ALTER TABLE #{quote_table_name(table_name)} ADD #{index_type} INDEX #{quote_column_name(index_name)} (#{index_columns})#{index_options}" # rubocop:disable Layout/LineLength
-      end
+      if Gem.loaded_specs['activerecord'].version >= Gem::Version.new('6.1')


if there's a prettier way of achieving this, I'm all ears. this was an easy solution that seems pretty simple to follow

There are multiple ways of making this prettier:

Splitting into modules:

# in lib/active_record/connection_adapters/mysql2_ghost_adapter/indexes_six.rb module IndexesSix def add_index(....) end end # in lib/active_record/connection_adapters/mysql2_ghost_adapter/indexes_five.rb module IndexesFive def add_index(....) end end # in lib/active_record/connection_adapters/mysql2_ghost_adapter.rb module ConnectionAdapters class Mysql2GhostAdapter < Mysql2Adapter if Gem.loaded_specs['activerecord'].version >= Gem::Version.new('6.1') include Mysql2GhostAdapter::IndexesSix else include Mysql2GhostAdapter::IndexesFive end end end

Another way would be dynamically including a module based on the version:

version = if Gem.loaded_specs['activerecord'].version >= Gem::Version.new('6.1') "Five" else "Six" end include const_get("SomeLib::#{version}")

since the number of methods is relatively low, I somewhat prefer the existing way to including modules as its easier to navigate. the second approach is nice, though, if we end up needing another conditional. I will make an issue for it as a pattern to consider if we also need 7.0 compatibility (somewhat likely)

acroos · 2021-11-03T21:59:54Z

lib/active_record/connection_adapters/mysql2_ghost_adapter.rb

@@ -111,6 +144,25 @@ def schema_migration_update?(sql)
        INSERT_SCHEMA_MIGRATION_PATTERN =~ sql ||
          DROP_SCHEMA_MIGRATION_PATTERN =~ sql
      end
+
+      def build_add_index_sql(table_name, column_names, index_name, # rubocop:disable Metrics/ParameterLists


inspired by the ActiveRecord code used to build a CREATE INDEX query (we can't use that because gh-ost only triggers on ALTER TABLE queries

acroos · 2021-11-03T22:00:53Z

spec/active_record/connection_adapters/mysql2_ghost_adapter_spec.rb

+      allow(mysql_client).to receive(:escape).with(table.to_s).and_return(table.to_s)
+      allow(mysql_client).to receive(:more_results?)


just mocked these because tests started failing on them. I didn't do too much digging into how ActiveRecord is using these return values (other than obviously using the escape method to... escape a string)

spec/active_record/connection_adapters/mysql2_ghost_adapter_spec.rb

larryfox

I don't know enough about ActiveRecord internals to feel confident in my approval but it looks like good ruby 😅

bin/test_all_versions

…les directory

acroos added the bug Something isn't working label Nov 3, 2021

acroos self-assigned this Nov 3, 2021

acroos requested a review from grdw November 3, 2021 13:29

acroos force-pushed the fix/activerecord-6-1-compatibility branch 2 times, most recently from d874d66 to a9d7925 Compare November 3, 2021 13:33

acroos added 4 commits November 3, 2021 19:35

Update rubocop to be a bit more lenient with class/method length

68c1794

Update gemspec to ensure ActiveRecord < 6.2

c373441

Add compatibility for ActiveRecord >= 6.1

264dc4e

Re-remove Gemfile.lock

c303ea7

acroos force-pushed the fix/activerecord-6-1-compatibility branch from a9d7925 to c303ea7 Compare November 3, 2021 18:36

acroos added 5 commits November 3, 2021 21:51

Add tests for add_index and remove_index methods

95683e7

Fix specs

2f2397a

Fix add_index for ruby3/AR6

6eff6bb

Fix test matrix since activerecord 6.0 is incompatible with ruby 3

f9fe585

v0.3.0

239cab9

acroos commented Nov 3, 2021

View reviewed changes

spec/active_record/connection_adapters/mysql2_ghost_adapter_spec.rb Outdated Show resolved Hide resolved

larryfox approved these changes Nov 3, 2021

View reviewed changes

grdw reviewed Nov 4, 2021

View reviewed changes

bin/test_all_versions Outdated Show resolved Hide resolved

Add custom matcher replacing matching method

8c42832

acroos mentioned this pull request Nov 4, 2021

Refactor compatibility code #63

Open

grdw approved these changes Nov 4, 2021

View reviewed changes

Update script to automatically test against all gemfiles in the gemfi…

6b9f9f4

…les directory

acroos merged commit 61c7b31 into main Nov 4, 2021

acroos deleted the fix/activerecord-6-1-compatibility branch November 4, 2021 10:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ActiveRecord >= 6.1 compatibility #61

ActiveRecord >= 6.1 compatibility #61

acroos commented Nov 3, 2021 •

edited

acroos commented Nov 3, 2021

acroos Nov 3, 2021

acroos Nov 3, 2021

acroos Nov 3, 2021

acroos Nov 3, 2021

acroos Nov 3, 2021

acroos Nov 3, 2021

acroos Nov 3, 2021

grdw Nov 4, 2021 •

edited

acroos Nov 4, 2021

acroos Nov 3, 2021

acroos Nov 3, 2021

larryfox left a comment

		allow(mysql_client).to receive(:escape).with(table.to_s).and_return(table.to_s)
		allow(mysql_client).to receive(:more_results?)

ActiveRecord >= 6.1 compatibility #61

ActiveRecord >= 6.1 compatibility #61

Conversation

acroos commented Nov 3, 2021 • edited

Description

Type of change

How Has This Been Tested?

Checklist:

acroos commented Nov 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grdw Nov 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larryfox left a comment

Choose a reason for hiding this comment

acroos commented Nov 3, 2021 •

edited

grdw Nov 4, 2021 •

edited