Add upsert to database review guidelines

- similar to update, destroy, etc, this method should require
  database review

danger/bulk_database_actions/Dangerfile

 # frozen_string_literal: true
 
-bulk_database_actions.add_comment_for_bulk_database_action_method_usage
+helper.all_changed_files.each do |filename|
+  next unless filename.end_with?('.rb')
+  next if filename.start_with?('spec/', 'ee/spec/', 'jh/spec/')
+
+  bulk_database_actions.add_suggestions_for(filename)
+end

danger/plugins/bulk_database_actions.rb

@@ -4,6 +4,8 @@
 
 module Danger
   class BulkDatabaseActions < ::Danger::Plugin
-    include Tooling::Danger::BulkDatabaseActions
+    def add_suggestions_for(filename)
+      Tooling::Danger::BulkDatabaseActions.new(filename, context: self).suggest
+    end
   end
 end

doc/development/database_review.md

@@ -29,7 +29,7 @@ A database review is required for:
   These metrics could have complex queries over large tables.
   See the [Analytics Instrumentation Guide](https://about.gitlab.com/handbook/product/analytics-instrumentation-guide/)
   for implementation details.
-- Changes that use [`update`, `delete`, `update_all`, `delete_all` or `destroy_all`](#preparation-when-using-update-delete-update_all-delete_all-or-destroy_all)
+- Changes that use [`update`, `upsert`, `delete`, `update_all`, `upsert_all`, `delete_all` or `destroy_all`](#preparation-when-using-bulk-update-operations)
   methods on an ActiveRecord object.
 
 A database reviewer is expected to look out for overly complex
@@ -227,9 +227,10 @@ Include in the MR description:
 - If you're adding a composite index, another index might become redundant, so remove that in the same migration.
   For example adding `index(column_A, column_B, column_C)` makes the indexes `index(column_A, column_B)` and `index(column_A)` redundant.
 
-#### Preparation when using `update`, `delete`, `update_all`, `delete_all` or `destroy_all`
+#### Preparation when using bulk update operations
 
-Using these ActiveRecord methods requires extra care because they modify data and can perform poorly, or they
+Using  `update`, `upsert`, `delete`, `update_all`, `upsert_all`, `delete_all` or `destroy_all`
+ActiveRecord methods requires extra care because they modify data and can perform poorly, or they
 can destroy data if improperly scoped. These methods are also
 [incompatible with Common Table Expression (CTE) statements](sql.md#when-to-use-common-table-expressions).
 Danger will comment on a Merge Request Diff when these methods are used.

spec/tooling/danger/bulk_database_actions_spec.rb

@@ -8,115 +8,87 @@
 require_relative '../../../tooling/danger/project_helper'
 
 RSpec.describe Tooling::Danger::BulkDatabaseActions, feature_category: :tooling do
-  include_context "with dangerfile"
+  include_context 'with dangerfile'
 
-  let(:fake_danger) { DangerSpecHelper.fake_danger.include(described_class) }
+  let(:fake_danger) { DangerSpecHelper.fake_danger }
   let(:fake_project_helper) { instance_double(Tooling::Danger::ProjectHelper) }
-
-  let(:mr_url) { 'https://gitlab.com/gitlab-org/gitlab/-/merge_requests/1' }
-  let(:doc_link) { described_class::DOCUMENTATION_LINK }
-
-  let(:comment_text) { "\n#{described_class::COMMENT_TEXT}" }
-
-  let(:file_lines) do
-    file_diff.map { |line| line.delete_prefix('+') }
-  end
+  let(:comment_text) { "\n#{described_class::SUGGESTION}" }
+  let(:file_lines) { file_diff.map { |line| line.delete_prefix('+') } }
 
   before do
     allow(bulk_database_actions).to receive(:project_helper).and_return(fake_project_helper)
     allow(bulk_database_actions.project_helper).to receive(:file_lines).and_return(file_lines)
     allow(bulk_database_actions.helper).to receive(:added_files).and_return([filename])
     allow(bulk_database_actions.helper).to receive(:changed_lines).with(filename).and_return(file_diff)
-    allow(bulk_database_actions.helper).to receive(:mr_web_url).and_return(mr_url)
+
+    bulk_database_actions.define_singleton_method(:add_suggestions_for) do |filename|
+      Tooling::Danger::BulkDatabaseActions.new(filename, context: self).suggest
+    end
   end
 
   subject(:bulk_database_actions) { fake_danger.new(helper: fake_helper) }
 
-  shared_examples 'no Danger comment' do
-    it 'does not comment on the bulk update action usage' do
-      expect(bulk_database_actions).not_to receive(:markdown)
-
-      bulk_database_actions.add_comment_for_bulk_database_action_method_usage
+  context 'for single line method call' do
+    let(:file_diff) do
+      <<~DIFF.split("\n")
+        +    def execute
+        +      #{code}
+        +
+        +      ServiceResponse.success
+        +    end
+      DIFF
     end
-  end
 
-  describe '#add_comment_for_bulk_database_action_method_usage' do
-    context 'for single line method call' do
-      let(:file_diff) do
-        [
-          "+    def execute",
-          "+      pat_family.active.#{method_call}",
-          "+",
-          "+      ServiceResponse.success",
-          "+    end"
-        ]
-      end
+    context 'when file is a non-spec Ruby file' do
+      let(:filename) { 'app/services/personal_access_tokens/revoke_token_family_service.rb' }
 
-      context 'when file is a non-spec Ruby file' do
-        let(:filename) { 'app/services/personal_access_tokens/revoke_token_family_service.rb' }
-
-        using RSpec::Parameterized::TableSyntax
-
-        where(:method_call, :expect_comment?) do
-          'update_all(revoked: true)' | true
-          'destroy_all'               | true
-          'delete_all'                | true
-          'update(revoked: true)'     | true
-          'delete'                    | true
-          'update_two_factor'         | false
-          'delete_keys(key)'          | false
-          'destroy_hook(hook)'        | false
-          'destroy_all_merged'        | false
-          'update_all_mirrors'        | false
+      using RSpec::Parameterized::TableSyntax
+
+      context 'when comment is expected' do
+        where(:code) do
+          [
+            'update_all(revoked: true)',
+            'destroy_all',
+            'delete_all',
+            'update(revoked: true)',
+            'delete',
+            'upsert',
+            'upsert_all',
+            'User.upsert',
+            'User.last.destroy',
+            ' .destroy'
+          ]
         end
 
         with_them do
-          it "correctly handles potential bulk database action" do
-            if expect_comment?
-              expect(bulk_database_actions).to receive(:markdown).with(comment_text, file: filename, line: 2)
-            else
-              expect(bulk_database_actions).not_to receive(:markdown)
-            end
-
-            bulk_database_actions.add_comment_for_bulk_database_action_method_usage
+          specify do
+            expect(bulk_database_actions).to receive(:markdown).with(comment_text.chomp, file: filename, line: 2)
+
+            bulk_database_actions.add_suggestions_for(filename)
           end
         end
       end
 
-      context 'for spec directories' do
-        let(:method_call) { 'update_all(revoked: true)' }
-
-        context 'for FOSS spec file' do
-          let(:filename) { 'spec/services/personal_access_tokens/revoke_token_family_service_spec.rb' }
-
-          it_behaves_like 'no Danger comment'
-        end
-
-        context 'for EE spec file' do
-          let(:filename) { 'ee/spec/services/personal_access_tokens/revoke_token_family_service_spec.rb' }
-
-          it_behaves_like 'no Danger comment'
+      context 'when no comment is expected' do
+        where(:code) do
+          [
+            'we update bob',
+            'update_two_factor',
+            'delete_keys(key)',
+            'destroy_hook(hook)',
+            'destroy_all_merged',
+            'update_all_mirrors'
+          ]
         end
 
-        context 'for JiHu spec file' do
-          let(:filename) { 'jh/spec/services/personal_access_tokens/revoke_token_family_service_spec.rb' }
+        with_them do
+          specify do
+            expect(bulk_database_actions).not_to receive(:markdown)
 
-          it_behaves_like 'no Danger comment'
+            bulk_database_actions.add_suggestions_for(filename)
+          end
         end
       end
     end
-
-    context 'for strings' do
-      let(:filename) { 'app/services/personal_access_tokens/revoke_token_family_service.rb' }
-      let(:file_diff) do
-        [
-          '+    expect { subject }.to output(',
-          '+      "ERROR: Could not update tag"',
-          '+    ).to_stderr'
-        ]
-      end
-
-      it_behaves_like 'no Danger comment'
-    end
   end
 end

spec/tooling/danger/project_helper_spec.rb

@@ -251,6 +251,11 @@
         [:backend, :analytics_instrumentation]            | '+ count(User.active)'                         | ['lib/gitlab/usage_data/topology.rb']
         [:backend, :analytics_instrumentation]            | '+ foo_count(User.active)'                     | ['lib/gitlab/usage_data.rb']
         [:backend]                                        | '+ count(User.active)'                         | ['user.rb']
+        [:database, :backend]                             | '+ User.upsert({ name: "blah" })'              | ['app/foo/bar.rb']
+        [:database, :backend]                             | '+ upsert({ name: "blah" })'                   | ['app/foo/bar.rb']
+        [:database, :backend]                             | '+ .upsert({ name: "blah" })'                  | ['app/foo/bar.rb']
+        [:database, :backend]                             | '+ .delete_all'                                | ['app/foo/bar.rb']
+        [:database, :backend]                             | '+ .destroy_all'                               | ['app/foo/bar.rb']
         [:import_integrate_be, :database]                 | '+ add_column :integrations, :foo, :text'      | ['db/migrate/foo.rb']
         [:import_integrate_be, :database]                 | '+ create_table :zentao_tracker_data do |t|'   | ['ee/db/post_migrate/foo.rb']
         [:import_integrate_be, :backend]                  | '+ Integrations::Foo'                          | ['app/foo/bar.rb']

tooling/danger/bulk_database_actions.rb

 # frozen_string_literal: true
 
-require_relative 'suggestor'
+require_relative 'suggestion'
 
 module Tooling
   module Danger
-    module BulkDatabaseActions
-      include ::Tooling::Danger::Suggestor
+    class BulkDatabaseActions < Suggestion
+      MATCH = %r{\A\+\s+(\S*\.)?((update|upsert|delete|destroy)(_all)?)\b}
+      REPLACEMENT = nil
+      DOCUMENTATION_LINK = 'https://docs.gitlab.com/ee/development/database_review.html#preparation-when-using-bulk-update-operations'
 
-      BULK_UPDATE_METHODS_REGEX = /\.((update|delete|destroy)(_all)?)\b/
+      SUGGESTION = <<~MESSAGE_MARKDOWN.freeze
+        When using `update`, `upsert`, `delete`, `update_all`, `upsert_all`, `delete_all` or `destroy_all`
+        you must include the full database query and query execution plan in the merge request description,
+        and request a ~database review.
 
-      DOCUMENTATION_LINK = 'https://docs.gitlab.com/ee/development/database_review.html#preparation-when-using-update-delete-update_all-and-destroy_all'
-      COMMENT_TEXT =
-        "When using `update`, `delete`, `update_all`, `delete_all` or `destroy_all` you must include the full " \
-        "database query and query execution plan in the merge request description, and request a ~database review. " \
-        "This comment can be ignored if the object is not an ActiveRecord class, since no database query " \
-        "would be generated. For more information, see [Database Review documentation](#{DOCUMENTATION_LINK}).".freeze
+        This comment can be ignored if the object is not an ActiveRecord class, since no database query would be generated.
 
-      def add_comment_for_bulk_database_action_method_usage
-        changed_ruby_files.each do |filename|
-          add_suggestion(
-            filename: filename,
-            regex: BULK_UPDATE_METHODS_REGEX,
-            comment_text: COMMENT_TEXT
-          )
-        end
-      end
+        ----
 
-      def changed_ruby_files
-        helper.added_files.select { |f| f.end_with?('.rb') && !f.start_with?('spec/', 'ee/spec/', 'jh/spec/') }
-      end
+        For more information, see [Database Review documentation](#{DOCUMENTATION_LINK}).
+      MESSAGE_MARKDOWN
     end
   end
 end

tooling/danger/project_helper.rb

@@ -105,6 +105,7 @@ module ProjectHelper
         %r{\A(app/models/project_authorization|app/services/users/refresh_authorized_projects_service)(/|\.rb)} => [:database, :backend],
         %r{\A((ee|jh)/)?app/finders/} => [:database, :backend],
         %r{\Arubocop/cop/migration(/|\.rb)} => :database,
+        [%r{\A((ee|jh)/)?(app|lib)/.+\.rb}, %r{\A\+\s+(\w*\.)?(update_all|upsert|upsert_all|delete_all|destroy_all)(\(.*\))?\s*\z}] => [:database, :backend],
 
         %r{\Alib/gitlab/ci/templates} => :ci_template,