From 61c7b31383ca0f8502ce09ab245fb0c27dc5c423 Mon Sep 17 00:00:00 2001 From: Austin C Roos Date: Thu, 4 Nov 2021 11:37:28 +0100 Subject: [PATCH] ActiveRecord >= 6.1 compatibility (#61) * Update rubocop to be a bit more lenient with class/method length * Update gemspec to ensure ActiveRecord < 6.2 * Add compatibility for ActiveRecord >= 6.1 * Re-remove Gemfile.lock * Add tests for add_index and remove_index methods * Fix specs * Fix add_index for ruby3/AR6 * Fix test matrix since activerecord 6.0 is incompatible with ruby 3 * v0.3.0 * Add custom matcher replacing matching method * Update script to automatically test against all gemfiles in the gemfiles directory --- .github/workflows/tests.yml | 6 +- .rubocop.yml | 20 ++ CHANGELOG.md | 3 + bin/test_all_versions | 18 ++ ghost_adapter.gemspec | 4 +- .../mysql2_ghost_adapter.rb | 66 ++++++- lib/ghost_adapter/version.rb | 2 +- .../mysql2_ghost_adapter_spec.rb | 178 ++++++++++++++---- 8 files changed, 244 insertions(+), 53 deletions(-) create mode 100755 bin/test_all_versions diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 7f10881..6891627 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -25,13 +25,15 @@ jobs: - "gemfiles/activerecord-6.0.Gemfile" - "gemfiles/activerecord-6.1.Gemfile" exclude: - # Ruby 3 is not supported for ActiveRecord < 6 + # Ruby 3 is not supported for ActiveRecord < 6.1 - ruby-version: "3.0" gemfile: "gemfiles/activerecord-5.0.Gemfile" - ruby-version: "3.0" gemfile: "gemfiles/activerecord-5.1.Gemfile" - ruby-version: "3.0" gemfile: "gemfiles/activerecord-5.2.Gemfile" + - ruby-version: "3.0" + gemfile: "gemfiles/activerecord-6.0.Gemfile" steps: - uses: actions/checkout@v2 - name: Set up Ruby @@ -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 diff --git a/.rubocop.yml b/.rubocop.yml index ef86ccb..e125238 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -7,16 +7,36 @@ Style/Documentation: Style/RescueModifier: Enabled: false +Layout/LineLength: + Exclude: + - spec/**/*_spec.rb + Metrics/BlockLength: Exclude: - spec/**/*_spec.rb +Metrics/ClassLength: + CountAsOne: + - 'array' + - 'hash' + - 'heredoc' + Max: 150 + Metrics/MethodLength: + Max: 15 + CountAsOne: + - 'array' + - 'hash' + - 'heredoc' IgnoredMethods: - build_ghost_command - mysql2_ghost_connection Metrics/ModuleLength: + CountAsOne: + - 'array' + - 'hash' + - 'heredoc' Exclude: - lib/ghost_adapter/config.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f8aca8..932e283 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ # CHANGELOG +## 0.3.0 +- Fix compatibility for ActiveRecord 6.1 ([#61](https://github.com/WeTransfer/ghost_adapter/pull/61)) + ## 0.2.3 - Add comma to allowed characters when cleaning SQL queries ([#52](https://github.com/WeTransfer/ghost_adapter/pull/52)) diff --git a/bin/test_all_versions b/bin/test_all_versions new file mode 100755 index 0000000..64ca85c --- /dev/null +++ b/bin/test_all_versions @@ -0,0 +1,18 @@ +#!/bin/bash + +test_version() { + local version=$(echo $1 | sed "s/[^0-9.]*//g") + echo + echo "----------------------------------------------------------------------------------------------------------------------" + echo -e "\033[1;34mTesting against ActiveRecord ${version}\033[0m" + + local gemfile="${1}" + shift + + bundle install --gemfile "${gemfile}" --quiet + bundle exec --gemfile "${gemfile}" rspec "$@" +} + +for gemfile in $(find gemfiles -type f -regex ".*\.Gemfile$"); do + test_version "${gemfile}" +done diff --git a/ghost_adapter.gemspec b/ghost_adapter.gemspec index 5e7ab50..9a697c3 100644 --- a/ghost_adapter.gemspec +++ b/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)/}) } end spec.require_paths = ['lib'] - spec.add_dependency 'activerecord', '>= 5' + spec.add_dependency 'activerecord', '>= 5', '< 6.2' spec.add_dependency 'mysql2', '>= 0.4.0', '< 0.6.0' spec.add_development_dependency 'bump', '~> 0' diff --git a/lib/active_record/connection_adapters/mysql2_ghost_adapter.rb b/lib/active_record/connection_adapters/mysql2_ghost_adapter.rb index 99a1a59..97247f2 100644 --- a/lib/active_record/connection_adapters/mysql2_ghost_adapter.rb +++ b/lib/active_record/connection_adapters/mysql2_ghost_adapter.rb @@ -55,14 +55,47 @@ def execute(sql, name = nil) end end - def add_index(table_name, column_name, options = {}) - 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') + def add_index(table_name, column_name, **options) + index, algorithm, if_not_exists = add_index_options(table_name, column_name, **options) + return if if_not_exists && index_exists?(table_name, column_name, name: index.name) + + index_type = index.type&.to_s&.upcase || (index.unique ? 'UNIQUE' : nil) + + sql = build_add_index_sql( + table_name, quoted_columns(index), index.name, + index_type: index_type, + using: index.using, + algorithm: algorithm + ) + + execute sql + end + + def remove_index(table_name, column_name = nil, **options) + return if options[:if_exists] && !index_exists?(table_name, column_name, **options) + + index_name = index_name_for_remove(table_name, column_name, options) + execute "ALTER TABLE #{quote_table_name(table_name)} DROP INDEX #{quote_column_name(index_name)}" + end + else + def add_index(table_name, column_name, options = {}) + index_name, index_type, index_columns, _index_options = add_index_options(table_name, column_name, options) - def remove_index(table_name, options = {}) - index_name = index_name_for_remove(table_name, options) - execute "ALTER TABLE #{quote_table_name(table_name)} DROP INDEX #{quote_column_name(index_name)}" + sql = build_add_index_sql( + table_name, index_columns, index_name, + index_type: index_type&.upcase, + using: options[:using] + ) + + execute sql + end + + def remove_index(table_name, options = {}) + options = { column: options } unless options.is_a?(Hash) + index_name = index_name_for_remove(table_name, options) + execute "ALTER TABLE #{quote_table_name(table_name)} DROP INDEX #{quote_column_name(index_name)}" + end end private @@ -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 + index_type: nil, using: nil, algorithm: nil) + sql = %w[ALTER TABLE] + sql << quote_table_name(table_name) + sql << 'ADD' + sql << index_type + sql << 'INDEX' + sql << quote_column_name(index_name) + sql << "USING #{using}" if using + sql << "(#{column_names})" + sql << algorithm + + sql.compact.join(' ').gsub(/\s+/, ' ') + end + + def quoted_columns(index) + index.columns.is_a?(String) ? index.columns : quoted_columns_for_index(index.columns, index.column_options) + end end end end diff --git a/lib/ghost_adapter/version.rb b/lib/ghost_adapter/version.rb index 1240adb..e428331 100644 --- a/lib/ghost_adapter/version.rb +++ b/lib/ghost_adapter/version.rb @@ -1,3 +1,3 @@ module GhostAdapter - VERSION = '0.2.3'.freeze + VERSION = '0.3.0'.freeze end diff --git a/spec/active_record/connection_adapters/mysql2_ghost_adapter_spec.rb b/spec/active_record/connection_adapters/mysql2_ghost_adapter_spec.rb index ae63cec..0a9bd5a 100644 --- a/spec/active_record/connection_adapters/mysql2_ghost_adapter_spec.rb +++ b/spec/active_record/connection_adapters/mysql2_ghost_adapter_spec.rb @@ -2,19 +2,29 @@ require 'active_record/connection_adapters/abstract/connection_pool' RSpec.describe ActiveRecord::ConnectionAdapters::Mysql2GhostAdapter do + matcher :execute_sql do |sql| + match do |adapter| + expect(adapter).to receive(:execute).with(sql) + end + end + let(:logger) { double(:logger, puts: true) } let(:mysql_client) { double('Mysql2::Client', query_options: {}, query: nil) } + let(:table) { :foo } + let(:column) { :bar_id } subject { described_class.new(mysql_client, logger, {}, {}) } before do allow(mysql_client).to receive(:server_info).and_return({ id: 50_732, version: '5.7.32-log' }) + if Gem.loaded_specs['activerecord'].version < Gem::Version.new('6.1') + allow(mysql_client).to receive(:escape).with(table.to_s).and_return(table.to_s) + allow(mysql_client).to receive(:more_results?) + end end describe 'schema statements' do describe 'clean_query' do - let(:table_name) { 'foo' } - it 'parses query correctly' do sql = 'ADD index_type INDEX `bar_index_name` (`bar_id`), '\ @@ -25,56 +35,142 @@ 'ADD index_type INDEX baz_index_name (baz_id)' expect(GhostAdapter::Migrator).to receive(:execute) - .with(table_name, sanatized_sql, any_args) + .with(table.to_s, sanatized_sql, any_args) - subject.execute("ALTER TABLE #{table_name} #{sql}") + subject.execute("ALTER TABLE #{table} #{sql}") end end - describe '#add_index' do - let(:table_name) { :foo } - let(:column_name) { :bar_id } - let(:options) { {} } - let(:sql) { 'ADD index_type INDEX `index_name` (`bar_id`)' } - + describe 're-defined ActiveRecord methods' do before do - allow(subject).to( - receive(:add_index_options) - .with(table_name, column_name, options) - .and_return(['index_name', 'index_type', "`#{column_name}`"]) - ) + allow(subject).to receive(:execute) end - it 'passes the built SQL to #execute' do - expect(subject).to( - receive(:execute) - .with( - "ALTER TABLE `#{table_name}` ADD index_type INDEX `index_name` (`bar_id`)" - ) - ) - subject.add_index(table_name, column_name, options) - end - end + describe '#add_index' do + context 'with no options' do + it 'passes the correct SQL to #execute' do + expect(subject).to execute_sql "ALTER TABLE `#{table}` ADD INDEX `index_#{table}_on_#{column}` (`#{column}`)" + subject.add_index(table, column) + end + end - describe '#remove_index' do - let(:table_name) { :foo } - let(:options) { { column: :bar_id } } - let(:sql) { 'DROP INDEX `index_name`' } + context 'with multiple columns' do + let(:col2) { :baz_id } - before do - allow(subject).to( - receive(:index_name_for_remove) - .with(table_name, options) - .and_return('index_name') - ) + it 'passes the correct SQL to #execute' do + expect(subject).to execute_sql "ALTER TABLE `#{table}` ADD INDEX `index_#{table}_on_#{column}_and_#{col2}` (`#{column}`, `#{col2}`)" + subject.add_index(table, [column, col2]) + end + + context 'with length option' do + let(:length1) { rand(20) } + let(:length2) { rand(20) } + + it 'passes the correct SQL to #execute' do + expect(subject).to execute_sql "ALTER TABLE `#{table}` ADD INDEX `index_#{table}_on_#{column}_and_#{col2}` (`#{column}`(#{length1}), `#{col2}`(#{length2}))" + subject.add_index(table, [column, col2], length: { column.to_s => length1, col2.to_s => length2 }) + end + end + end + + context 'with unique option true' do + it 'passes the correct SQL to #execute' do + expect(subject).to execute_sql "ALTER TABLE `#{table}` ADD UNIQUE INDEX `index_#{table}_on_#{column}` (`#{column}`)" + subject.add_index(table, column, unique: true) + end + end + + context 'with name option' do + let(:index_name) { 'index_name' } + + it 'passes the correct SQL to #execute' do + expect(subject).to execute_sql "ALTER TABLE `#{table}` ADD INDEX `#{index_name}` (`#{column}`)" + subject.add_index(table, column, name: index_name) + end + end + + context 'with length option' do + let(:length) { rand(20) } + + it 'passes the correct SQL to #execute' do + expect(subject).to execute_sql "ALTER TABLE `#{table}` ADD INDEX `index_#{table}_on_#{column}` (`#{column}`(#{length}))" + subject.add_index(table, column, length: length) + end + end + + context 'with using option' do + let(:method) { 'btree' } + + it 'passes the correct SQL to #execute' do + expect(subject).to execute_sql "ALTER TABLE `#{table}` ADD INDEX `index_#{table}_on_#{column}` USING #{method} (`#{column}`)" + subject.add_index(table, column, using: method) + end + end + + context 'with type option' do + let(:type) { 'fulltext' } + + it 'passes the correct SQL to #execute' do + expect(subject).to execute_sql "ALTER TABLE `#{table}` ADD #{type.upcase} INDEX `index_#{table}_on_#{column}` (`#{column}`)" + subject.add_index(table, column, type: type) + end + end end - it 'passes the built SQL to #execute' do - expect(subject).to( - receive(:execute) - .with("ALTER TABLE `#{table_name}` DROP INDEX `index_name`") - ) - subject.remove_index(table_name, options) + describe '#remove_index' do + let(:index_name) { 'index_name' } + + before do + # This must be mocked because the method tries to query the database to get the index name + allow(subject).to receive(:index_name_for_remove).and_return(index_name) + end + + context 'with column name passed as positional argument' do + it 'passes the correct SQL to #execute' do + expect(subject).to execute_sql "ALTER TABLE `#{table}` DROP INDEX `#{index_name}`" + subject.remove_index(table, column) + end + end + + context 'with column name passed as keyword argument' do + it 'passes the correct SQL to #execute' do + expect(subject).to execute_sql "ALTER TABLE `#{table}` DROP INDEX `#{index_name}`" + subject.remove_index(table, column: column) + end + end + + context 'with index name passed as keyword argument' do + let(:index_name) { 'brand_new_index_name' } + + it 'passes the correct SQL to #execute' do + expect(subject).to execute_sql "ALTER TABLE `#{table}` DROP INDEX `#{index_name}`" + subject.remove_index(table, name: index_name) + end + end + + if Gem.loaded_specs['activerecord'].version >= Gem::Version.new('6.1') + context 'when ActiveRecord version is >= 6.1' do + context 'with if_exists property set false' do + context 'when index exists' do + before { allow(subject).to receive(:index_exists?).and_return(true) } + + it 'passes the correct SQL to #execute' do + expect(subject).to execute_sql "ALTER TABLE `#{table}` DROP INDEX `#{index_name}`" + subject.remove_index(table, column, if_exists: true) + end + end + + context 'when index does not exist' do + before { allow(subject).to receive(:index_exists?).and_return(false) } + + it 'does nothing' do + subject.remove_index(table, column, if_exists: true) + expect(subject).not_to have_received(:execute) + end + end + end + end + end end end end