From d265ad206137b383d46eb8a7c00fb4c893d8eac6 Mon Sep 17 00:00:00 2001 From: Alasdair Wallace Mackie <73504675+AlasdairWallaceMackie@users.noreply.github.com> Date: Tue, 10 Dec 2024 16:30:42 -0500 Subject: [PATCH 1/3] Ignore node if part of method call --- .../cop/sequel/irreversible_migration.rb | 8 ++++++- .../cop/sequel/irreversible_migration_spec.rb | 24 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/lib/rubocop/cop/sequel/irreversible_migration.rb b/lib/rubocop/cop/sequel/irreversible_migration.rb index ca76215..a2db268 100644 --- a/lib/rubocop/cop/sequel/irreversible_migration.rb +++ b/lib/rubocop/cop/sequel/irreversible_migration.rb @@ -28,7 +28,7 @@ class IrreversibleMigration < Base set_column_allow_null ].freeze - MSG = 'Avoid using "%s" inside a "change" block. Use "up" & "down" blocks instead.' + MSG = 'Using "%s" inside a "change" block may cause an irreversible migration. Use "up" & "down" instead.' PRIMARY_KEY_MSG = 'Avoid using "add_primary_key" with an array argument inside a "change" block.' def on_block(node) @@ -46,6 +46,8 @@ def on_block(node) def validate_node(node) return if within_create_table_block?(node) + return if part_of_method_call?(node) + add_offense(node.loc.selector, message: format(MSG, name: node.method_name)) unless valid_change_method?(node) add_offense(node.loc.selector, message: PRIMARY_KEY_MSG) if invalid_primary_key_method?(node) @@ -68,6 +70,10 @@ def within_create_table_block?(node) ancestor.method_name == :create_table end end + + def part_of_method_call?(node) + node.each_ancestor(:send).count.positive? + end end end end diff --git a/spec/rubocop/cop/sequel/irreversible_migration_spec.rb b/spec/rubocop/cop/sequel/irreversible_migration_spec.rb index 12188aa..65daa46 100644 --- a/spec/rubocop/cop/sequel/irreversible_migration_spec.rb +++ b/spec/rubocop/cop/sequel/irreversible_migration_spec.rb @@ -103,6 +103,30 @@ expect(offenses.size).to eq(1) end end + + describe 'and a method is used within an argument' do + let(:source) do + <<~SOURCE + change do + alter_table(:stores) do + add_column(:products, JSON, null: false, default: Sequel.pg_json({})) + add_constraint( + :only_one_user, + ( + Sequel.cast(Sequel.~(user_id: nil), Integer) + + Sequel.cast(Sequel.~(owner_id: nil), Integer) + ) => 1, + ) + end + end + SOURCE + end + + it 'does not register an offense with valid methods' do + offenses = inspect_source_within_migration(source) + expect(offenses).to be_empty + end + end end context 'when inside an up block' do From 1887980a94970db0a355fdb76d76d58e2184bf58 Mon Sep 17 00:00:00 2001 From: Alasdair Wallace Mackie <73504675+AlasdairWallaceMackie@users.noreply.github.com> Date: Tue, 10 Dec 2024 16:32:06 -0500 Subject: [PATCH 2/3] Bump version --- config/default.yml | 2 +- lib/rubocop/sequel/version.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/config/default.yml b/config/default.yml index 4dec344..01cb81f 100644 --- a/config/default.yml +++ b/config/default.yml @@ -17,7 +17,7 @@ Sequel/IrreversibleMigration: Reference: https://www.rubydoc.info/gems/rubocop-sequel/RuboCop/Cop/Sequel/IrreversibleMigration Enabled: true VersionAdded: 0.3.5 - VersionChanged: 0.3.7 + VersionChanged: 0.3.8 Sequel/JSONColumn: Description: >- diff --git a/lib/rubocop/sequel/version.rb b/lib/rubocop/sequel/version.rb index e209a6c..187a426 100644 --- a/lib/rubocop/sequel/version.rb +++ b/lib/rubocop/sequel/version.rb @@ -4,7 +4,7 @@ module RuboCop module Sequel # This module holds the RuboCop Sequel version information. module Version - STRING = '0.3.7' + STRING = '0.3.8' end end end From 1b54e819ce2ae16f9f22eb4049b4075bc0562fd8 Mon Sep 17 00:00:00 2001 From: Alasdair Wallace Mackie <73504675+AlasdairWallaceMackie@users.noreply.github.com> Date: Tue, 10 Dec 2024 16:52:36 -0500 Subject: [PATCH 3/3] Edge case spec --- .../cop/sequel/irreversible_migration_spec.rb | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/spec/rubocop/cop/sequel/irreversible_migration_spec.rb b/spec/rubocop/cop/sequel/irreversible_migration_spec.rb index 65daa46..2f33421 100644 --- a/spec/rubocop/cop/sequel/irreversible_migration_spec.rb +++ b/spec/rubocop/cop/sequel/irreversible_migration_spec.rb @@ -127,6 +127,28 @@ expect(offenses).to be_empty end end + + describe 'and an invalid change method contains another invalid change method as an argument' do + let(:source) do + <<~SOURCE + change do + alter_table(:stores) do + drop_column(:products, JSON, null: false, default: Sequel.pg_json({})) + end + end + SOURCE + end + + it 'only registers 1 offense' do + offenses = inspect_source_within_migration(source) + expect(offenses.size).to eq(1) + end + + it 'only registers an offense for the parent method' do + offenses = inspect_source_within_migration(source) + expect(offenses.first.message).to include('drop_column') + end + end end context 'when inside an up block' do