From 8a06cfd9391e56bca88f0e611df6cd00de1b319b Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Tue, 14 Jan 2025 20:33:15 +0900 Subject: [PATCH] [Fix #1410] Make registered cops aware of `AllCops: MigratedSchemaVersion` When implementing #1383, the detection of `AllCops: MigratedSchemaVersion` was initially considered only for cops related to migrations. However, feedback received later, such as in #1410, indicated that it is also expected to apply to `Style`, `Lint`, and other categories. This suggestion is reasonable, as warnings for migrated files may not be limited to database columns but could also include Ruby programming logic. Excluding `Style` and `Lint` from this consideration would not align with this feedback. This PR modifies the behavior so that all registered cops can detect the value of `AllCops: MigratedSchemaVersion`. Fixes #1410. --- ...are_of_all_cops_migrated_schema_version.md | 1 + lib/rubocop-rails.rb | 3 + lib/rubocop/cop/mixin/migrations_helper.rb | 29 ---------- lib/rubocop/cop/rails/add_column_index.rb | 2 +- lib/rubocop/cop/rails/bulk_change_table.rb | 2 +- .../cop/rails/dangerous_column_names.rb | 2 +- lib/rubocop/cop/rails/migration_class_name.rb | 2 +- lib/rubocop/cop/rails/not_null_column.rb | 2 +- lib/rubocop/cop/rails/reversible_migration.rb | 2 +- .../reversible_migration_method_definition.rb | 2 +- lib/rubocop/cop/rails/schema_comment.rb | 2 +- .../cop/rails/three_state_boolean_column.rb | 2 +- lib/rubocop/rails/migration_file_skippable.rb | 56 +++++++++++++++++++ spec/rubocop/cop/rails/presence_spec.rb | 12 ++++ 14 files changed, 81 insertions(+), 38 deletions(-) create mode 100644 changelog/change_make_registered_cops_aware_of_all_cops_migrated_schema_version.md create mode 100644 lib/rubocop/rails/migration_file_skippable.rb diff --git a/changelog/change_make_registered_cops_aware_of_all_cops_migrated_schema_version.md b/changelog/change_make_registered_cops_aware_of_all_cops_migrated_schema_version.md new file mode 100644 index 0000000000..e8368e8d9d --- /dev/null +++ b/changelog/change_make_registered_cops_aware_of_all_cops_migrated_schema_version.md @@ -0,0 +1 @@ +* [#1410](https://github.com/rubocop/rubocop-rails/issues/1410): Make registered cops aware of `AllCops: MigratedSchemaVersion`. ([@koic][]) diff --git a/lib/rubocop-rails.rb b/lib/rubocop-rails.rb index 45c869c079..65c443c91a 100644 --- a/lib/rubocop-rails.rb +++ b/lib/rubocop-rails.rb @@ -15,6 +15,9 @@ require_relative 'rubocop/cop/rails_cops' +require_relative 'rubocop/rails/migration_file_skippable' +RuboCop::Rails::MigrationFileSkippable.apply_to_cops! + RuboCop::Cop::Style::HashExcept.minimum_target_ruby_version(2.0) RuboCop::Cop::Style::InverseMethods.singleton_class.prepend( diff --git a/lib/rubocop/cop/mixin/migrations_helper.rb b/lib/rubocop/cop/mixin/migrations_helper.rb index ecdb3e146a..76dedda41f 100644 --- a/lib/rubocop/cop/mixin/migrations_helper.rb +++ b/lib/rubocop/cop/mixin/migrations_helper.rb @@ -21,35 +21,6 @@ def in_migration?(node) migration_class?(class_node) end end - - # rubocop:disable Style/DocumentDynamicEvalDefinition - %i[on_send on_csend on_block on_numblock on_class].each do |method| - class_eval(<<~RUBY, __FILE__, __LINE__ + 1) - def #{method}(node) - return if already_migrated_file? - - super if method(__method__).super_method - end - RUBY - end - # rubocop:enable Style/DocumentDynamicEvalDefinition - - private - - def already_migrated_file? - return false unless migrated_schema_version - - match_data = File.basename(processed_source.file_path).match(/(?\d{14})/) - schema_version = match_data['timestamp'] if match_data - - return false unless schema_version - - schema_version <= migrated_schema_version.to_s # Ignore applied migration files. - end - - def migrated_schema_version - config.for_all_cops.fetch('MigratedSchemaVersion', nil) - end end end end diff --git a/lib/rubocop/cop/rails/add_column_index.rb b/lib/rubocop/cop/rails/add_column_index.rb index 480a1e84c9..dc6334b311 100644 --- a/lib/rubocop/cop/rails/add_column_index.rb +++ b/lib/rubocop/cop/rails/add_column_index.rb @@ -19,7 +19,7 @@ module Rails class AddColumnIndex < Base extend AutoCorrector include RangeHelp - prepend MigrationsHelper + include MigrationsHelper MSG = '`add_column` does not accept an `index` key, use `add_index` instead.' RESTRICT_ON_SEND = %i[add_column].freeze diff --git a/lib/rubocop/cop/rails/bulk_change_table.rb b/lib/rubocop/cop/rails/bulk_change_table.rb index 62ec19c5df..f4948ff38b 100644 --- a/lib/rubocop/cop/rails/bulk_change_table.rb +++ b/lib/rubocop/cop/rails/bulk_change_table.rb @@ -65,7 +65,7 @@ module Rails # end class BulkChangeTable < Base include DatabaseTypeResolvable - prepend MigrationsHelper + include MigrationsHelper MSG_FOR_CHANGE_TABLE = <<~MSG.chomp You can combine alter queries using `bulk: true` options. diff --git a/lib/rubocop/cop/rails/dangerous_column_names.rb b/lib/rubocop/cop/rails/dangerous_column_names.rb index 1d24e8c526..621f73cda9 100644 --- a/lib/rubocop/cop/rails/dangerous_column_names.rb +++ b/lib/rubocop/cop/rails/dangerous_column_names.rb @@ -14,7 +14,7 @@ module Rails # # good # add_column :users, :saved class DangerousColumnNames < Base # rubocop:disable Metrics/ClassLength - prepend MigrationsHelper + include MigrationsHelper COLUMN_TYPE_METHOD_NAMES = %i[ bigint diff --git a/lib/rubocop/cop/rails/migration_class_name.rb b/lib/rubocop/cop/rails/migration_class_name.rb index ecb05a8f00..1e895accde 100644 --- a/lib/rubocop/cop/rails/migration_class_name.rb +++ b/lib/rubocop/cop/rails/migration_class_name.rb @@ -20,7 +20,7 @@ module Rails # class MigrationClassName < Base extend AutoCorrector - prepend MigrationsHelper + include MigrationsHelper MSG = 'Replace with `%s` that matches the file name.' diff --git a/lib/rubocop/cop/rails/not_null_column.rb b/lib/rubocop/cop/rails/not_null_column.rb index 6fe0df1db9..0ff380b6d0 100644 --- a/lib/rubocop/cop/rails/not_null_column.rb +++ b/lib/rubocop/cop/rails/not_null_column.rb @@ -41,7 +41,7 @@ module Rails # change_column_null :products, :category_id, false class NotNullColumn < Base include DatabaseTypeResolvable - prepend MigrationsHelper + include MigrationsHelper MSG = 'Do not add a NOT NULL column without a default value.' RESTRICT_ON_SEND = %i[add_column add_reference].freeze diff --git a/lib/rubocop/cop/rails/reversible_migration.rb b/lib/rubocop/cop/rails/reversible_migration.rb index 9c5b8af071..1cfb2288f1 100644 --- a/lib/rubocop/cop/rails/reversible_migration.rb +++ b/lib/rubocop/cop/rails/reversible_migration.rb @@ -151,7 +151,7 @@ module Rails # remove_index :users, column: :email # end class ReversibleMigration < Base - prepend MigrationsHelper + include MigrationsHelper MSG = '%s is not reversible.' diff --git a/lib/rubocop/cop/rails/reversible_migration_method_definition.rb b/lib/rubocop/cop/rails/reversible_migration_method_definition.rb index 979f066a6b..fbf2b1d87d 100644 --- a/lib/rubocop/cop/rails/reversible_migration_method_definition.rb +++ b/lib/rubocop/cop/rails/reversible_migration_method_definition.rb @@ -43,7 +43,7 @@ module Rails # end # end class ReversibleMigrationMethodDefinition < Base - prepend MigrationsHelper + include MigrationsHelper MSG = 'Migrations must contain either a `change` method, or both an `up` and a `down` method.' diff --git a/lib/rubocop/cop/rails/schema_comment.rb b/lib/rubocop/cop/rails/schema_comment.rb index bf40899dc8..bd707d68af 100644 --- a/lib/rubocop/cop/rails/schema_comment.rb +++ b/lib/rubocop/cop/rails/schema_comment.rb @@ -23,7 +23,7 @@ module Rails # class SchemaComment < Base include ActiveRecordMigrationsHelper - prepend MigrationsHelper + include MigrationsHelper COLUMN_MSG = 'New database column without `comment`.' TABLE_MSG = 'New database table without `comment`.' diff --git a/lib/rubocop/cop/rails/three_state_boolean_column.rb b/lib/rubocop/cop/rails/three_state_boolean_column.rb index 1bdbebe248..26940e0fad 100644 --- a/lib/rubocop/cop/rails/three_state_boolean_column.rb +++ b/lib/rubocop/cop/rails/three_state_boolean_column.rb @@ -18,7 +18,7 @@ module Rails # t.boolean :active, default: true, null: false # class ThreeStateBooleanColumn < Base - prepend MigrationsHelper + include MigrationsHelper MSG = 'Boolean columns should always have a default value and a `NOT NULL` constraint.' RESTRICT_ON_SEND = %i[add_column column boolean].freeze diff --git a/lib/rubocop/rails/migration_file_skippable.rb b/lib/rubocop/rails/migration_file_skippable.rb new file mode 100644 index 0000000000..2cb5048453 --- /dev/null +++ b/lib/rubocop/rails/migration_file_skippable.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module RuboCop + module Rails + # This module allows cops to detect and ignore files that have already been migrated + # by leveraging the `AllCops: MigratedSchemaVersion` configuration. + # + # [source,yaml] + # ----- + # AllCops: + # MigratedSchemaVersion: '20241225000000' + # ----- + # + # When applied to cops, it dynamically overrides the `on_*` methods for all supported + # node types, ensuring that cops skip processing if the file is determined to be a + # migrated file based on the schema version. + # + # @api private + module MigrationFileSkippable + Parser::Meta::NODE_TYPES.each do |node_type| + method_name = :"on_#{node_type}" + + class_eval(<<~RUBY, __FILE__, __LINE__ + 1) + def #{method_name}(node, *args) # def on_if(node, *args) + return if already_migrated_file? # return if already_migrated_file? + # + super if method(__method__).super_method # super if method(__method__).super_method + rescue NameError # rescue NameError + # noop # # noop + end # end + RUBY + end + + def self.apply_to_cops! + RuboCop::Cop::Registry.all.each { |cop| cop.prepend(MigrationFileSkippable) } + end + + private + + def already_migrated_file? + return false unless migrated_schema_version + + match_data = File.basename(processed_source.file_path).match(/(?\d{14})/) + schema_version = match_data['timestamp'] if match_data + + return false unless schema_version + + schema_version <= migrated_schema_version.to_s # Ignore applied migration files. + end + + def migrated_schema_version + config.for_all_cops.fetch('MigratedSchemaVersion', nil) + end + end + end +end diff --git a/spec/rubocop/cop/rails/presence_spec.rb b/spec/rubocop/cop/rails/presence_spec.rb index 93cf6ab707..657039d1a5 100644 --- a/spec/rubocop/cop/rails/presence_spec.rb +++ b/spec/rubocop/cop/rails/presence_spec.rb @@ -441,4 +441,16 @@ end RUBY end + + context 'when inspection file that have already been migrated' do + let(:config) do + RuboCop::Config.new('AllCops' => { 'MigratedSchemaVersion' => '20240101010101' }) + end + + it 'does not register an offense and corrects when `a.present? ? a : nil`' do + expect_no_offenses(<<~RUBY, '20190101010101_add_column_to_table.rb') + a.present? ? a : nil + RUBY + end + end end