From 5fcfda42823b61fec70a5077636ad496f91bc7c7 Mon Sep 17 00:00:00 2001 From: Cody Cutrer Date: Wed, 3 Apr 2024 14:46:42 -0600 Subject: [PATCH] Rails/NotNullColumn: Inspect change_table calls for offenses --- ...nspect_change_table_for_not_null_column.md | 1 + lib/rubocop/cop/rails/not_null_column.rb | 78 +++++++++++- .../rubocop/cop/rails/not_null_column_spec.rb | 120 ++++++++++++++++++ 3 files changed, 193 insertions(+), 6 deletions(-) create mode 100644 changelog/change_inspect_change_table_for_not_null_column.md diff --git a/changelog/change_inspect_change_table_for_not_null_column.md b/changelog/change_inspect_change_table_for_not_null_column.md new file mode 100644 index 0000000000..a39eef2161 --- /dev/null +++ b/changelog/change_inspect_change_table_for_not_null_column.md @@ -0,0 +1 @@ +* [#1266](https://github.com/rubocop/rubocop-rails/pull/1266): Check `change_table` calls for offenses. ([@ccutrer][]) diff --git a/lib/rubocop/cop/rails/not_null_column.rb b/lib/rubocop/cop/rails/not_null_column.rb index 2abd0927c8..1649fb1c22 100644 --- a/lib/rubocop/cop/rails/not_null_column.rb +++ b/lib/rubocop/cop/rails/not_null_column.rb @@ -15,10 +15,16 @@ module Rails # # bad # add_column :users, :name, :string, null: false # add_reference :products, :category, null: false + # change_table :users do |t| + # t.string :name, null: false + # end # # # good # add_column :users, :name, :string, null: true # add_column :users, :name, :string, null: false, default: '' + # change_table :users do |t| + # t.string :name, null: false, default: '' + # end # add_reference :products, :category # add_reference :products, :category, null: false, default: 1 class NotNullColumn < Base @@ -35,6 +41,22 @@ class NotNullColumn < Base (send nil? :add_reference _ _ (hash $...)) PATTERN + def_node_matcher :change_table?, <<~PATTERN + (block (send nil? :change_table ...) (args (arg $_)) _) + PATTERN + + def_node_matcher :add_not_null_column_in_change_table?, <<~PATTERN + (send (lvar $_) :column _ $_ (hash $...)) + PATTERN + + def_node_matcher :add_not_null_column_via_shortcut_in_change_table?, <<~PATTERN + (send (lvar $_) $_ _ (hash $...)) + PATTERN + + def_node_matcher :add_not_null_reference_in_change_table?, <<~PATTERN + (send (lvar $_) :add_reference _ _ (hash $...)) + PATTERN + def_node_matcher :null_false?, <<~PATTERN (pair (sym :null) (false)) PATTERN @@ -48,16 +70,25 @@ def on_send(node) check_add_reference(node) end + def on_block(node) + check_change_table(node) + end + alias on_numblock on_block + private + def check_column(type, pairs) + if type.respond_to?(:value) + return if type.value == :virtual || type.value == 'virtual' + return if (type.value == :text || type.value == 'text') && database == MYSQL + end + + check_pairs(pairs) + end + def check_add_column(node) add_not_null_column?(node) do |type, pairs| - if type.respond_to?(:value) - return if type.value == :virtual || type.value == 'virtual' - return if (type.value == :text || type.value == 'text') && database == MYSQL - end - - check_pairs(pairs) + check_column(type, pairs) end end @@ -67,6 +98,41 @@ def check_add_reference(node) end end + def check_add_column_in_change_table(node, table) + add_not_null_column_in_change_table?(node) do |receiver, type, pairs| + next unless receiver == table + + check_column(type, pairs) + end + end + + def check_add_column_via_shortcut_in_change_table(node, table) + add_not_null_column_via_shortcut_in_change_table?(node) do |receiver, type, pairs| + next unless receiver == table + + check_column(type, pairs) + end + end + + def check_add_reference_in_change_table(node, table) + add_not_null_reference_in_change_table?(node) do |receiver, pairs| + next unless receiver == table + + check_pairs(pairs) + end + end + + def check_change_table(node) + change_table?(node) do |table| + children = node.body.begin_type? ? node.body.children : [node.body] + children.each do |child| + check_add_column_in_change_table(child, table) + check_add_column_via_shortcut_in_change_table(child, table) + check_add_reference_in_change_table(child, table) + end + end + end + def check_pairs(pairs) return if pairs.any? { |pair| default_option?(pair) } diff --git a/spec/rubocop/cop/rails/not_null_column_spec.rb b/spec/rubocop/cop/rails/not_null_column_spec.rb index 15d8a310c0..a5c9b3dda2 100644 --- a/spec/rubocop/cop/rails/not_null_column_spec.rb +++ b/spec/rubocop/cop/rails/not_null_column_spec.rb @@ -87,6 +87,126 @@ def change end end + context 'with change_table call' do + context 'with shortcut column call' do + context 'with null: false' do + it 'reports an offense' do + expect_offense(<<~RUBY) + def change + change_table :users do |t| + t.string :name, null: false + ^^^^^^^^^^^ Do not add a NOT NULL column without a default value. + end + end + RUBY + end + + it 'reports multiple offenses' do + expect_offense(<<~RUBY) + def change + change_table :users do |t| + t.string :name, null: false + ^^^^^^^^^^^ Do not add a NOT NULL column without a default value. + t.string :address, null: false + ^^^^^^^^^^^ Do not add a NOT NULL column without a default value. + end + end + RUBY + end + end + + context 'with default option' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + def change + change_table :users do |t| + t.string :name, null: false, default: "" + end + end + RUBY + end + end + + context 'without any options' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + def change + change_table :users do |t| + t.string :name + end + end + RUBY + end + end + end + + context 'with column call' do + context 'with null: false' do + it 'reports an offense' do + expect_offense(<<~RUBY) + def change + change_table :users do |t| + t.column :name, :string, null: false + ^^^^^^^^^^^ Do not add a NOT NULL column without a default value. + end + end + RUBY + end + end + + context 'with default option' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + def change + change_table :users do |t| + t.column :name, :string, null: false, default: "" + end + end + RUBY + end + end + + context 'without any options' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + def change + change_table :users do |t| + t.column :name, :string + end + end + RUBY + end + end + end + + context 'with reference call' do + context 'with null: false' do + it 'reports an offense' do + expect_offense(<<~RUBY) + def change + change_table :users do |t| + t.references :address, null: false + ^^^^^^^^^^^ Do not add a NOT NULL column without a default value. + end + end + RUBY + end + end + + context 'without any options' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + def change + change_table :users do |t| + t.references :address + end + end + RUBY + end + end + end + end + context 'with add_reference call' do context 'with null: false' do it 'reports an offense' do