Merge pull request #1266 from ccutrer/inspect_change_table_for_not_nu…

…ll_column

Rails/NotNullColumn: Inspect change_table calls for offenses

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][])

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) }
 

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