Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce AllCops: MigratedSchemaVersion config #1383

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,20 @@ gems.locked file to find the version of Rails that has been bound to the
application. If neither of those files exist, RuboCop will use Rails 5.0
as the default.

### `AllCops: MigratedSchemaVersion`

By specifying `MigratedSchemaVersion` option, migration files that have been migrated can be ignored.
When `MigratedSchemaVersion: '20241231000000'` is set. Migration files lower than or equal to '20250101000000' will be ignored.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
When `MigratedSchemaVersion: '20241231000000'` is set. Migration files lower than or equal to '20250101000000' will be ignored.
When `MigratedSchemaVersion: '20250101000000'` is set. Migration files lower than or equal to '20250101000000' will be ignored.

For example, this is the timestamp in db/migrate/20250101000000_create_articles.rb.

```yaml
AllCops
MigratedSchemaVersion: '20250101000000'
```

This prevents inspecting schema settings for already applied migration files and avoids having different database schemas
depending on when `bin/rails db:migrate` is executed.

## Rails configuration tip

In Rails 6.1+, add the following `config.generators.after_generate` setting to
Expand Down
1 change: 1 addition & 0 deletions changelog/new_intro_migrated_schema_version.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#1383](https://github.com/rubocop/rubocop-rails/pull/1383): Introduce `AllCops: MigratedSchemaVersion` config. ([@koic][])
4 changes: 4 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ AllCops:
# application. If neither of those files exist, RuboCop will use Rails 5.0
# as the default.
TargetRailsVersion: ~
# By specifying `MigratedSchemaVersion` option, migration files that have been migrated can be ignored.
# When `MigratedSchemaVersion: '20241231000000'` is set. Migration files lower than or equal to '20250101000000' will be ignored.
# For example, this is the timestamp in db/migrate/20250101000000_create_articles.rb.
MigratedSchemaVersion: ~

Lint/NumberConversion:
# Add Rails' duration methods to the ignore list for `Lint/NumberConversion`
Expand Down
15 changes: 15 additions & 0 deletions docs/modules/ROOT/pages/usage.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,21 @@ gems.locked file to find the version of Rails that has been bound to the
application. If neither of those files exist, RuboCop will use Rails 5.0
as the default.

=== `AllCops: MigratedSchemaVersion`

By specifying `MigratedSchemaVersion` option, migration files that have been migrated can be ignored.
When `MigratedSchemaVersion: '20241231000000'` is set. Migration files lower than or equal to '20250101000000' will be ignored.
For example, this is the timestamp in db/migrate/20250101000000_create_articles.rb.

[source,yaml]
----
AllCops
MigratedSchemaVersion: '20250101000000'
----

This prevents inspecting schema settings for already applied migration files and avoids having different database schemas
depending on when `bin/rails db:migrate` is executed.
Comment on lines +57 to +68
Copy link
Contributor

@Earlopain Earlopain Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rewrote this section to add a bit more details. There is one error here where you write 20241231000000 that should be changed to 20250101000000 regardless.

Suggested change
By specifying `MigratedSchemaVersion` option, migration files that have been migrated can be ignored.
When `MigratedSchemaVersion: '20241231000000'` is set. Migration files lower than or equal to '20250101000000' will be ignored.
For example, this is the timestamp in db/migrate/20250101000000_create_articles.rb.
[source,yaml]
----
AllCops
MigratedSchemaVersion: '20250101000000'
----
This prevents inspecting schema settings for already applied migration files and avoids having different database schemas
depending on when `bin/rails db:migrate` is executed.
By specifying the `MigratedSchemaVersion` option, migration files that have already been run can be ignored.
When `MigratedSchemaVersion: '20250101000000'` is set, migration files lower than or equal to '20250101000000' will be ignored.
For example, to ignore `db/migrate/20250101000000_create_articles.rb` and earlier migrations you would configure it the following way:
[source,yaml]
----
AllCops
MigratedSchemaVersion: '20250101000000'
----
This prevents inspecting schema settings for already applied migration files. Changing already applied migrations should be avoided because it can lead to the schema getting out of sync between your local copy and what it actually is in production,
depending on when `bin/rails db:migrate` was executed.
If you want to modify your schema to comply with the cops, you should instead create new migrations.


== Rails configuration tip

In Rails 6.1+, add the following `config.generators.after_generate` setting to
Expand Down
29 changes: 29 additions & 0 deletions lib/rubocop/cop/mixin/migrations_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,35 @@ 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(/(?<timestamp>\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
1 change: 1 addition & 0 deletions lib/rubocop/cop/rails/add_column_index.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ module Rails
class AddColumnIndex < Base
extend AutoCorrector
include RangeHelp
prepend MigrationsHelper

MSG = '`add_column` does not accept an `index` key, use `add_index` instead.'
RESTRICT_ON_SEND = %i[add_column].freeze
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop/cop/rails/bulk_change_table.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ module Rails
# end
class BulkChangeTable < Base
include DatabaseTypeResolvable
prepend MigrationsHelper

MSG_FOR_CHANGE_TABLE = <<~MSG.chomp
You can combine alter queries using `bulk: true` options.
Expand Down
2 changes: 2 additions & 0 deletions lib/rubocop/cop/rails/dangerous_column_names.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ module Rails
# # good
# add_column :users, :saved
class DangerousColumnNames < Base # rubocop:disable Metrics/ClassLength
prepend MigrationsHelper

COLUMN_TYPE_METHOD_NAMES = %i[
bigint
binary
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/rails/migration_class_name.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ module Rails
#
class MigrationClassName < Base
extend AutoCorrector
include MigrationsHelper
prepend MigrationsHelper

MSG = 'Replace with `%<camelized_basename>s` that matches the file name.'

Expand Down
1 change: 1 addition & 0 deletions lib/rubocop/cop/rails/not_null_column.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ module Rails
# change_column_null :products, :category_id, false
class NotNullColumn < Base
include DatabaseTypeResolvable
prepend MigrationsHelper

MSG = 'Do not add a NOT NULL column without a default value.'
RESTRICT_ON_SEND = %i[add_column add_reference].freeze
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/rails/reversible_migration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ module Rails
# remove_index :users, column: :email
# end
class ReversibleMigration < Base
include MigrationsHelper
prepend MigrationsHelper

MSG = '%<action>s is not reversible.'

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ module Rails
# end
# end
class ReversibleMigrationMethodDefinition < Base
include MigrationsHelper
prepend MigrationsHelper

MSG = 'Migrations must contain either a `change` method, or both an `up` and a `down` method.'

Expand Down
1 change: 1 addition & 0 deletions lib/rubocop/cop/rails/schema_comment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ module Rails
#
class SchemaComment < Base
include ActiveRecordMigrationsHelper
prepend MigrationsHelper

COLUMN_MSG = 'New database column without `comment`.'
TABLE_MSG = 'New database table without `comment`.'
Expand Down
3 changes: 2 additions & 1 deletion lib/rubocop/cop/rails/three_state_boolean_column.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ module Rails
# t.boolean :active, default: true, null: false
#
class ThreeStateBooleanColumn < Base
MSG = 'Boolean columns should always have a default value and a `NOT NULL` constraint.'
prepend MigrationsHelper

MSG = 'Boolean columns should always have a default value and a `NOT NULL` constraint.'
RESTRICT_ON_SEND = %i[add_column column boolean].freeze

def_node_matcher :three_state_boolean?, <<~PATTERN
Expand Down
34 changes: 28 additions & 6 deletions spec/rubocop/cop/rails/add_column_index_spec.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Rails::AddColumnIndex, :config do
let(:config) do
RuboCop::Config.new('AllCops' => { 'MigratedSchemaVersion' => '20240101010101' })
end

it 'registers an offense and corrects when an `add_column` call has `index: true`' do
expect_offense(<<~RUBY)
expect_offense(<<~RUBY, '20250101010101_add_column_to_table.rb')
add_column :table, :column, :integer, default: 0, index: true
^^^^^^^^^^^ `add_column` does not accept an `index` key, use `add_index` instead.
RUBY
Expand All @@ -14,7 +18,7 @@
end

it 'registers an offense and corrects when an `add_column` call has `index:` with a hash' do
expect_offense(<<~RUBY)
expect_offense(<<~RUBY, '20250101010101_add_column_to_table.rb')
add_column :table, :column, :integer, default: 0, index: { unique: true, name: 'my_unique_index' }
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `add_column` does not accept an `index` key, use `add_index` instead.
RUBY
Expand All @@ -26,7 +30,7 @@
end

it 'registers an offense and corrects with there is another hash key after `index`' do
expect_offense(<<~RUBY)
expect_offense(<<~RUBY, '20250101010101_add_column_to_table.rb')
add_column :table, :column, :integer, index: true, default: 0
^^^^^^^^^^^ `add_column` does not accept an `index` key, use `add_index` instead.
RUBY
Expand All @@ -38,7 +42,7 @@
end

it 'registers an offense and corrects with string keys' do
expect_offense(<<~RUBY)
expect_offense(<<~RUBY, '20250101010101_add_column_to_table.rb')
add_column :table, :column, :integer, 'index' => true, default: 0
^^^^^^^^^^^^^^^ `add_column` does not accept an `index` key, use `add_index` instead.
RUBY
Expand All @@ -50,7 +54,7 @@
end

it 'registers an offense and corrects when on multiple lines' do
expect_offense(<<~RUBY)
expect_offense(<<~RUBY, '20250101010101_add_column_to_table.rb')
add_column :table, :column, :integer,
index: true,
^^^^^^^^^^^ `add_column` does not accept an `index` key, use `add_index` instead.
Expand All @@ -65,7 +69,7 @@
end

it 'can correct multiple `add_column` calls' do
expect_offense(<<~RUBY)
expect_offense(<<~RUBY, '20250101010101_add_column_to_table.rb')
add_column :table, :column, :integer, default: 0, index: true
^^^^^^^^^^^ `add_column` does not accept an `index` key, use `add_index` instead.
add_column :table, :column2, :integer, default: 0, index: true
Expand All @@ -85,4 +89,22 @@
add_column :table, :column, :integer, default: 0
RUBY
end

context '`MigratedSchemaVersion` is an integer' do
let(:config) do
RuboCop::Config.new('AllCops' => { 'MigratedSchemaVersion' => 20240101010101 }) # rubocop:disable Style/NumericLiterals
end

it 'registers an offense and corrects when an `add_column` call has `index: true`' do
expect_offense(<<~RUBY, '20250101010101_add_column_to_table.rb')
add_column :table, :column, :integer, default: 0, index: true
^^^^^^^^^^^ `add_column` does not accept an `index` key, use `add_index` instead.
RUBY

expect_correction(<<~RUBY)
add_column :table, :column, :integer, default: 0
add_index :table, :column
RUBY
end
end
end
16 changes: 14 additions & 2 deletions spec/rubocop/cop/rails/dangerous_column_names_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Rails::DangerousColumnNames, :config do
let(:config) do
RuboCop::Config.new('AllCops' => { 'MigratedSchemaVersion' => '20240101010101' })
end

context 'with non-dangerous column name' do
it 'does not register an offense' do
expect_no_offenses(<<~RUBY)
Expand All @@ -18,6 +22,14 @@
end
end

context 'with dangerous column name on `add_column` when migration file was migrated' do
it 'does not register an offense' do
expect_no_offenses(<<~RUBY, '20190101010101_add_save_to_users.rb')
add_column :users, :save, :string
RUBY
end
end

context 'with dangerous column name on `rename_column`' do
it 'registers an offense' do
expect_offense(<<~RUBY)
Expand All @@ -29,7 +41,7 @@

context 'with dangerous column name on `t.string`' do
it 'registers an offense' do
expect_offense(<<~RUBY)
expect_offense(<<~RUBY, '20250101010101_create_users.rb')
create_table :users do |t|
t.string :save
^^^^^ Avoid dangerous column names.
Expand All @@ -40,7 +52,7 @@

context 'with dangerous column name on `t.rename`' do
it 'registers an offense' do
expect_offense(<<~RUBY)
expect_offense(<<~RUBY, '20250101010101_create_users.rb')
create_table :users do |t|
t.rename :name, :save
^^^^^ Avoid dangerous column names.
Expand Down
7 changes: 5 additions & 2 deletions spec/rubocop/cop/rails/migration_class_name_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Rails::MigrationClassName, :config do
let(:filename) { 'db/migrate/20220101050505_create_users.rb' }
let(:config) do
RuboCop::Config.new('AllCops' => { 'MigratedSchemaVersion' => '20240101010101' })
end
let(:filename) { 'db/migrate/20250101010101_create_users.rb' }

context 'when the class name matches its file name' do
it 'does not register an offense' do
Expand Down Expand Up @@ -85,7 +88,7 @@ class AddBlobs < ActiveRecord::Migration[7.0]
# end
#
context 'when `ActiveSupport::Inflector` is applied to the class name and the case is different' do
let(:filename) { 'db/migrate/20210623095243_remove_unused_oauth_scope_grants.rb' }
let(:filename) { 'db/migrate/20250101010101_remove_unused_oauth_scope_grants.rb' }

it 'does not register an offense' do
expect_no_offenses(<<~RUBY, filename)
Expand Down
Loading