Merge branch...

Merge branch '217009-exclude-text-arrays-from-the-rubocop-rules-for-texts-and-strings' into 'master'

Exclude string and text arrays from Rubocop migration checks

See merge request gitlab-org/gitlab!34056

db/migrate/20200507221434_add_container_registry_features_to_application_settings.rb

@@ -4,7 +4,7 @@ class AddContainerRegistryFeaturesToApplicationSettings < ActiveRecord::Migratio
   DOWNTIME = false
 
   def up
-    add_column :application_settings, :container_registry_features, :text, array: true, default: [], null: false # rubocop:disable Migration/AddLimitToTextColumns
+    add_column :application_settings, :container_registry_features, :text, array: true, default: [], null: false
   end
 
   def down

rubocop/cop/migration/add_limit_to_text_columns.rb

@@ -39,6 +39,9 @@ module RuboCop
         private
 
         def text_operation?(node)
+          # Don't complain about text arrays
+          return false if array_column?(node)
+
           modifier = node.children[0]
           migration_method = node.children[1]
 

rubocop/cop/migration/prevent_strings.rb

@@ -33,6 +33,9 @@ module RuboCop
         private
 
         def string_operation?(node)
+          # Don't complain about string arrays
+          return false if array_column?(node)
+
           modifier = node.children[0]
           migration_method = node.children[1]
 

rubocop/migration_helpers.rb

@@ -34,6 +34,16 @@ module RuboCop
       File.basename(node.location.expression.source_buffer.name).split('_').first.to_i
     end
 
+    # Returns true if a column definition is for an array
+    # rubocop:disable Lint/BooleanSymbol
+    def array_column?(node)
+      node.each_descendant(:pair).any? do |pair_node|
+        pair_node.child_nodes[0].value == :array && # Searching for a (pair (sym :array) (true)) node
+        pair_node.child_nodes[1].type == :true # RuboCop::AST::Node uses symbols for types, even when that is a :true
+      end
+    end
+    # rubocop:enable Lint/BooleanSymbol
+
     private
 
     def dirname(node)

spec/rubocop/cop/migration/add_limit_to_text_columns_spec.rb

@@ -73,6 +73,27 @@ describe RuboCop::Cop::Migration::AddLimitToTextColumns do
       end
     end
 
+    context 'when text array columns are defined without a limit' do
+      it 'registers no offense' do
+        expect_no_offenses(<<~RUBY)
+          class TestTextLimits < ActiveRecord::Migration[6.0]
+            DOWNTIME = false
+
+            def up
+              create_table :test_text_limits, id: false do |t|
+                t.integer :test_id, null: false
+                t.text :name, array: true, default: [], null: false
+              end
+
+              add_column :test_text_limits, :email, :text, array: true
+              add_column_with_default :test_text_limits, :role, :text, default: [], array: true
+              change_column_type_concurrently :test_text_limits, :test_id, :text, array: true
+            end
+          end
+        RUBY
+      end
+    end
+
     # Make sure that the cop is properly checking for an `add_text_limit`
     # over the same {table, attribute} as the one that triggered the offence
     context 'when the limit is defined for a same name attribute but different table' do

spec/rubocop/cop/migration/prevent_strings_spec.rb

@@ -90,6 +90,27 @@ describe RuboCop::Cop::Migration::PreventStrings do
       end
     end
 
+    context 'when the string data type is used for arrays' do
+      it 'registers no offense' do
+        expect_no_offenses(<<~RUBY)
+          class TestStringArrays < ActiveRecord::Migration[6.0]
+            DOWNTIME = false
+
+            def up
+              create_table :test_string_arrays, id: false do |t|
+                t.integer :test_id, null: false
+                t.string :name, array: true, default: [], null: false
+              end
+
+              add_column :test_string_arrays, :email, :string, array: true
+              add_column_with_default :test_string_arrays, :role, :string, default: [], array: true
+              change_column_type_concurrently :test_string_arrays, :test_id, :string, array: true
+            end
+          end
+        RUBY
+      end
+    end
+
     context 'on down' do
       it 'registers no offense' do
         expect_no_offenses(<<~RUBY)