Skip to content

Commit

Permalink
Ensure migration related cops are only running on migrations (#45)
Browse files Browse the repository at this point in the history
  • Loading branch information
AlasdairWallaceMackie authored Oct 25, 2024
1 parent 2dbf5d1 commit ff013a5
Show file tree
Hide file tree
Showing 16 changed files with 106 additions and 22 deletions.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
.tags
*.gem
Gemfile.lock
.DS_Store
4 changes: 2 additions & 2 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ Sequel/IrreversibleMigration:
Warns against using certain methods inside a migration file's `change` block that would cause the migration to become irreversible.
Reference: https://www.rubydoc.info/gems/rubocop-sequel/RuboCop/Cop/Sequel/IrreversibleMigration
Enabled: true
VersionAdded: 0.3.4
VersionChanged: 0.3.4
VersionAdded: 0.3.5
VersionChanged: 0.3.6

Sequel/JSONColumn:
Description: >-
Expand Down
2 changes: 2 additions & 0 deletions lib/rubocop-sequel.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

RuboCop::Sequel::Inject.defaults!

require_relative 'rubocop/cop/sequel/helpers/migration'

require 'rubocop/cop/sequel/concurrent_index'
require 'rubocop/cop/sequel/irreversible_migration'
require 'rubocop/cop/sequel/json_column'
Expand Down
4 changes: 4 additions & 0 deletions lib/rubocop/cop/sequel/concurrent_index.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ module Cop
module Sequel
# ConcurrentIndex looks for non-concurrent index creation.
class ConcurrentIndex < Base
include Helpers::Migration

MSG = 'Specify `concurrently` option when creating or dropping an index.'
RESTRICT_ON_SEND = %i[add_index drop_index].freeze

Expand All @@ -13,6 +15,8 @@ class ConcurrentIndex < Base
MATCHER

def on_send(node)
return unless within_sequel_migration?(node)

indexes?(node) do |args|
add_offense(node.loc.selector, message: MSG) if offensive?(args)
end
Expand Down
25 changes: 25 additions & 0 deletions lib/rubocop/cop/sequel/helpers/migration.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Sequel
module Helpers
# Migration contains helper methods for detecting if a node is inside a `Sequel.migration` block
module Migration
extend NodePattern::Macros

def_node_matcher :sequel_migration_block?, <<~MATCHER
(block
(send
(const nil? :Sequel) :migration ...)
...)
MATCHER

def within_sequel_migration?(node)
node.each_ancestor(:block).any? { |ancestor| sequel_migration_block?(ancestor) }
end
end
end
end
end
end
7 changes: 5 additions & 2 deletions lib/rubocop/cop/sequel/irreversible_migration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ module Cop
module Sequel
# IrreversibleMigration looks for methods inside a `change` block that cannot be reversed.
class IrreversibleMigration < Base
include Helpers::Migration

# https://sequel.jeremyevans.net/rdoc/files/doc/migration_rdoc.html#label-A+Basic+Migration
VALID_CHANGE_METHODS = %i[
create_table
Expand All @@ -26,11 +28,12 @@ class IrreversibleMigration < Base
set_column_allow_null
].freeze

MSG = 'Avoid using `%<name>s` inside a `change` block. Use `up` & `down` blocks instead.'
PRIMARY_KEY_MSG = 'Avoid using `add_primary_key` with an array argument inside a `change` block.'
MSG = 'Avoid using "%<name>s" inside a "change" block. Use "up" & "down" blocks instead.'
PRIMARY_KEY_MSG = 'Avoid using "add_primary_key" with an array argument inside a "change" block.'

def on_block(node)
return unless node.method_name == :change
return unless within_sequel_migration?(node)

body = node.body
return unless body
Expand Down
4 changes: 4 additions & 0 deletions lib/rubocop/cop/sequel/json_column.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ module Cop
module Sequel
# JSONColumn looks for non-JSONB columns.
class JSONColumn < Base
include Helpers::Migration

MSG = 'Use JSONB rather than JSON or hstore'
RESTRICT_ON_SEND = %i[add_column].freeze

Expand All @@ -22,12 +24,14 @@ class JSONColumn < Base

def on_send(node)
return unless json_or_hstore?(node)
return unless within_sequel_migration?(node)

add_offense(node.loc.selector, message: MSG)
end

def on_block(node)
return unless node.send_node.method_name == :create_table
return unless within_sequel_migration?(node)

node.each_node(:send) do |method|
next unless column_method?(method) || column_type?(method)
Expand Down
3 changes: 3 additions & 0 deletions lib/rubocop/cop/sequel/partial_constraint.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ module Cop
module Sequel
# PartialConstraint looks for missed usage of partial indexes.
class PartialConstraint < Base
include Helpers::Migration

MSG = "Constraint can't be partial, use where argument with index"
RESTRICT_ON_SEND = %i[add_unique_constraint].freeze

Expand All @@ -14,6 +16,7 @@ class PartialConstraint < Base

def on_send(node)
return unless add_partial_constraint?(node)
return unless within_sequel_migration?(node)

add_offense(node.loc.selector, message: MSG)
end
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/sequel/version.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ module RuboCop
module Sequel
# This module holds the RuboCop Sequel version information.
module Version
STRING = '0.3.5'
STRING = '0.3.6'
end
end
end
2 changes: 1 addition & 1 deletion rubocop-sequel.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ Gem::Specification.new do |gem|
gem.executables = gem.files.grep(%r{^bin/}).map { |f| File.basename(f) }
gem.name = 'rubocop-sequel'
gem.require_paths = ['lib']
gem.version = '0.3.5'
gem.version = '0.3.6'
gem.metadata['rubygems_mfa_required'] = 'true'

gem.required_ruby_version = '>= 2.5'
Expand Down
10 changes: 6 additions & 4 deletions spec/rubocop/cop/sequel/concurrent_index_spec.rb
Original file line number Diff line number Diff line change
@@ -1,27 +1,29 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Sequel::ConcurrentIndex do
include Spec::Helpers::Migration

subject(:cop) { described_class.new }

context 'without the concurrent option' do
it 'registers an offense without options' do
offenses = inspect_source(<<~SOURCE)
offenses = inspect_source_within_migration(<<~SOURCE)
add_index(:products, :name)
drop_index(:products, :name)
SOURCE
expect(offenses.size).to eq(2)
end

it 'registers an offense with other options' do
offenses = inspect_source(<<~SOURCE)
offenses = inspect_source_within_migration(<<~SOURCE)
add_index(:products, :name, unique: true)
drop_index(:products, :name, unique: true)
SOURCE
expect(offenses.size).to eq(2)
end

it 'registers an offense with composite index' do
offenses = inspect_source(<<~SOURCE)
offenses = inspect_source_within_migration(<<~SOURCE)
add_index(:products, [:name, :price], unique: true)
drop_index(:products, [:name, :price])
SOURCE
Expand All @@ -30,7 +32,7 @@
end

it 'does not register an offense when using concurrent option' do
offenses = inspect_source(<<~SOURCE)
offenses = inspect_source_within_migration(<<~SOURCE)
add_index(:products, :name, unique: true, concurrently: true)
drop_index(:products, :name, concurrently: true)
SOURCE
Expand Down
19 changes: 19 additions & 0 deletions spec/rubocop/cop/sequel/helpers/migration.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# frozen_string_literal: true

module Spec
module Helpers
module Migration
extend CopHelper

def inspect_source_within_migration(source, file = nil)
migration_wrapped_source = <<~SOURCE
Sequel.migration do
#{source}
end
SOURCE

inspect_source(migration_wrapped_source, file)
end
end
end
end
21 changes: 18 additions & 3 deletions spec/rubocop/cop/sequel/irreversible_migration_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Sequel::IrreversibleMigration do
include Spec::Helpers::Migration

subject(:cop) { described_class.new }

context 'when inside a change block' do
Expand Down Expand Up @@ -28,12 +30,12 @@
end

it 'registers an offense when there is an invalid method' do
offenses = inspect_source(invalid_source)
offenses = inspect_source_within_migration(invalid_source)
expect(offenses.size).to eq(2)
end

it 'does not register an offense with valid methods' do
offenses = inspect_source(valid_source)
offenses = inspect_source_within_migration(valid_source)
expect(offenses).to be_empty
end

Expand All @@ -49,7 +51,7 @@
end

it 'registers an offense' do
offenses = inspect_source(source)
offenses = inspect_source_within_migration(source)
expect(offenses.size).to eq(1)
end
end
Expand All @@ -68,6 +70,19 @@
SOURCE
end

it 'does not register an offense with any methods' do
offenses = inspect_source_within_migration(source)
expect(offenses).to be_empty
end
end

context 'when a change block is used outside of a Sequel migration' do
let(:source) do
<<~SOURCE
it { expect { subject }.to change { document_count(user_id) }.by(-1) }
SOURCE
end

it 'does not register an offense with any methods' do
offenses = inspect_source(source)
expect(offenses).to be_empty
Expand Down
18 changes: 10 additions & 8 deletions spec/rubocop/cop/sequel/json_column_spec.rb
Original file line number Diff line number Diff line change
@@ -1,48 +1,50 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Sequel::JSONColumn do
include Spec::Helpers::Migration

subject(:cop) { described_class.new }

context 'with add_column' do
it 'registers an offense when using json type' do
offenses = inspect_source('add_column(:products, :type, :json)')
offenses = inspect_source_within_migration('add_column(:products, :type, :json)')
expect(offenses.size).to eq(1)
end

it 'registers an offense when using hstore type' do
offenses = inspect_source('add_column(:products, :type, :hstore)')
offenses = inspect_source_within_migration('add_column(:products, :type, :hstore)')
expect(offenses.size).to eq(1)
end

it 'does not register an offense when using jsonb' do
offenses = inspect_source('add_column(:products, :type, :jsonb)')
offenses = inspect_source_within_migration('add_column(:products, :type, :jsonb)')
expect(offenses).to be_empty
end
end

context 'with create_table' do
it 'registers an offense when using json as a method' do
offenses = inspect_source('create_table(:products) { json :type, default: {} }')
offenses = inspect_source_within_migration('create_table(:products) { json :type, default: {} }')
expect(offenses.size).to eq(1)
end

it 'registers an offense when using the column method with hstore' do
offenses = inspect_source('create_table(:products) { column :type, :hstore }')
offenses = inspect_source_within_migration('create_table(:products) { column :type, :hstore }')
expect(offenses.size).to eq(1)
end

it 'does not register an offense when using jsonb as column type`' do
offenses = inspect_source('create_table(:products) { column :type, :jsonb }')
offenses = inspect_source_within_migration('create_table(:products) { column :type, :jsonb }')
expect(offenses).to be_empty
end

it 'does not register an offense when using jsonb' do
offenses = inspect_source('create_table(:products) { jsonb :type }')
offenses = inspect_source_within_migration('create_table(:products) { jsonb :type }')
expect(offenses).to be_empty
end

it 'does not register an offense when using a simple type' do
offenses = inspect_source('create_table(:products) { integer :type, default: 0 }')
offenses = inspect_source_within_migration('create_table(:products) { integer :type, default: 0 }')
expect(offenses).to be_empty
end
end
Expand Down
4 changes: 3 additions & 1 deletion spec/rubocop/cop/sequel/partial_constraint_spec.rb
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Sequel::PartialConstraint do
include Spec::Helpers::Migration

subject(:cop) { described_class.new }

it 'registers an offense when using where for constraint' do
offenses = inspect_source(<<~RUBY)
offenses = inspect_source_within_migration(<<~RUBY)
add_unique_constraint %i[col_1 col_2], where: "state != 'deleted'"
RUBY

Expand Down
2 changes: 2 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
require 'rubocop/rspec/support'
require 'rubocop-sequel'

require_relative 'rubocop/cop/sequel/helpers/migration'

RSpec.configure do |config|
config.include RuboCop::RSpec::ExpectOffense
config.disable_monkey_patching!
Expand Down

0 comments on commit ff013a5

Please sign in to comment.