Merge branch '225874-rubocop' into 'master'

Rubocop check for direct manipulation of ActiveModel errors hash

See merge request gitlab-org/gitlab!59971

.rubocop.yml

@@ -307,6 +307,9 @@ Cop/ActiveRecordAssociationReload:
     - 'spec/**/*'
     - 'ee/spec/**/*'
 
+Cop/ActiveModelErrorsDirectManipulation:
+  Enabled: true
+
 Gitlab/AvoidFeatureGet:
   Enabled: true
 

app/services/users/destroy_service.rb

@@ -31,7 +31,7 @@ module Users
       end
 
       if !delete_solo_owned_groups && user.solo_owned_groups.present?
-        user.errors[:base] << 'You must transfer ownership or delete groups before you can remove user'
+        user.errors.add(:base, 'You must transfer ownership or delete groups before you can remove user')
         return user
       end
 

app/validators/branch_filter_validator.rb

@@ -20,11 +20,11 @@ class BranchFilterValidator < ActiveModel::EachValidator
       value_without_wildcards = value.tr('*', 'x')
 
       unless Gitlab::GitRefValidator.validate(value_without_wildcards)
-        record.errors[attribute] << "is not a valid branch name"
+        record.errors.add(attribute, "is not a valid branch name")
       end
 
       unless value.length <= 4000
-        record.errors[attribute] << "is longer than the allowed length of 4000 characters."
+        record.errors.add(attribute, "is longer than the allowed length of 4000 characters.")
       end
     end
   end

app/validators/same_project_association_validator.rb

@@ -16,6 +16,6 @@ class SameProjectAssociationValidator < ActiveModel::EachValidator
   def validate_each(record, attribute, value)
     return if record.project == value&.project
 
-    record.errors[attribute] << 'must associate the same project'
+    record.errors.add(attribute, 'must associate the same project')
   end
 end

ee/app/services/vulnerability_feedback/create_service.rb

@@ -68,7 +68,7 @@ module VulnerabilityFeedback
             .execute
 
           if result[:status] == :error
-            vulnerability_feedback.errors[:issue] << result[:message]
+            vulnerability_feedback.errors.add(:issue, result[:message])
             raise ActiveRecord::Rollback
           end
 
@@ -78,7 +78,7 @@ module VulnerabilityFeedback
         issue_link_result = create_vulnerability_issue_link(vulnerability_feedback.vulnerability_data[:vulnerability_id], issue)
 
         if issue_link_result&.error?
-          vulnerability_feedback.errors[:issue_link] << issue_link_result.message
+          vulnerability_feedback.errors.add(:issue_link, issue_link_result.message)
           raise ActiveRecord::Rollback
         end
 
@@ -100,7 +100,7 @@ module VulnerabilityFeedback
 
         vulnerability_feedback.save
       else
-        vulnerability_feedback.errors[:merge_request] << result[:message]
+        vulnerability_feedback.errors.add(:merge_request, result[:message])
       end
     end
 

rubocop/cop/active_model_errors_direct_manipulation.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    # Cop that avoid direct manipulation of ActiveModel#errors hash,
+    # in preparation to upgrade to Rails 6.1
+    #
+    # See https://gitlab.com/gitlab-org/gitlab/-/issues/225874
+    class ActiveModelErrorsDirectManipulation < RuboCop::Cop::Cop
+      MSG = 'Avoid manipulating errors hash directly. For more details check https://gitlab.com/gitlab-org/gitlab/-/issues/225874'
+
+      MANIPULATIVE_METHODS = ":<< :append :clear :collect! :compact! :concat :delete :delete_at :delete_if :drop :drop_while :fill :filter! :keep_if :flatten! :insert :map! :pop :prepend :push :reject! :replace :reverse! :rotate! :select! :shift :shuffle! :slice! :sort! :sort_by! :uniq! :unshift"
+
+      def_node_matcher :active_model_errors_root_manipulation?, <<~PATTERN
+        (send
+          (send
+            (send {send ivar lvar} :errors)
+            :[]
+            ...)
+          {#{MANIPULATIVE_METHODS}}
+          ...)
+      PATTERN
+
+      def_node_matcher :active_model_errors_root_assignment?, <<~PATTERN
+        (send
+          (send {send ivar lvar} :errors)
+          :[]=
+          ...)
+      PATTERN
+
+      def_node_matcher :active_model_errors_manipulation?, <<~PATTERN
+        (send
+          (send
+            (send
+              (send {send ivar lvar} :errors)
+              {:messages :details})
+            :[]
+            ...)
+          {#{MANIPULATIVE_METHODS}}
+          ...)
+      PATTERN
+
+      def_node_matcher :active_model_errors_assignment?, <<~PATTERN
+        (send
+          (send
+            (send {send ivar lvar} :errors)
+            {:messages :details})
+          :[]=
+          ...)
+      PATTERN
+
+      def on_send(node)
+        add_offense(node, location: :expression) if active_model_errors_root_assignment?(node)
+        add_offense(node, location: :expression) if active_model_errors_root_manipulation?(node)
+        add_offense(node, location: :expression) if active_model_errors_manipulation?(node)
+        add_offense(node, location: :expression) if active_model_errors_assignment?(node)
+      end
+    end
+  end
+end

spec/rubocop/cop/active_model_errors_direct_manipulation_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require_relative '../../../rubocop/cop/active_model_errors_direct_manipulation'
+
+RSpec.describe RuboCop::Cop::ActiveModelErrorsDirectManipulation do
+  subject(:cop) { described_class.new }
+
+  context 'when modifying errors' do
+    it 'registers an offense' do
+      expect_offense(<<~PATTERN)
+        user.errors[:name] << 'msg'
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating errors hash directly. [...]
+      PATTERN
+    end
+
+    context 'when assigning' do
+      it 'registers an offense' do
+        expect_offense(<<~PATTERN)
+        user.errors[:name] = []
+        ^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating errors hash directly. [...]
+        PATTERN
+      end
+    end
+  end
+
+  context 'when modifying errors.messages' do
+    it 'registers an offense' do
+      expect_offense(<<~PATTERN)
+        user.errors.messages[:name] << 'msg'
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating errors hash directly. [...]
+      PATTERN
+    end
+
+    context 'when assigning' do
+      it 'registers an offense' do
+        expect_offense(<<~PATTERN)
+        user.errors.messages[:name] = []
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating errors hash directly. [...]
+        PATTERN
+      end
+    end
+  end
+
+  context 'when modifying errors.details' do
+    it 'registers an offense' do
+      expect_offense(<<~PATTERN)
+        user.errors.details[:name] << {}
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating errors hash directly. [...]
+      PATTERN
+    end
+
+    context 'when assigning' do
+      it 'registers an offense' do
+        expect_offense(<<~PATTERN)
+        user.errors.details[:name] = []
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid manipulating errors hash directly. [...]
+        PATTERN
+      end
+    end
+  end
+end