Merge branch '218706-change-to-use-the-gitlab-styles-updatelargetable-cop' into 'master'

Change to use the gitlab-styles UpdateLargeTable cop

See merge request gitlab-org/gitlab!33507

.rubocop.yml

@@ -2,11 +2,14 @@ inherit_gem:
   gitlab-styles:
     - rubocop-default.yml
 
-inherit_from: .rubocop_todo.yml
 require:
   - ./rubocop/rubocop
   - rubocop-rspec
 
+inherit_from:
+  - .rubocop_todo.yml
+  - ./rubocop/rubocop-migrations.yml
+
 inherit_mode:
   merge:
     - Include

rubocop/cop/migration/update_large_table.rb

-require_relative '../../migration_helpers'
-
-module RuboCop
-  module Cop
-    module Migration
-      # This cop checks for `add_column_with_default` on a table that's been
-      # explicitly blacklisted because of its size.
-      #
-      # Even though this helper performs the update in batches to avoid
-      # downtime, using it with tables with millions of rows still causes a
-      # significant delay in the deploy process and is best avoided.
-      #
-      # See https://gitlab.com/gitlab-com/infrastructure/issues/1602 for more
-      # information.
-      class UpdateLargeTable < RuboCop::Cop::Cop
-        include MigrationHelpers
-
-        MSG = 'Using `%s` on the `%s` table will take a long time to ' \
-              'complete, and should be avoided unless absolutely ' \
-              'necessary'.freeze
-
-        BATCH_UPDATE_METHODS = %w[
-          :add_column_with_default
-          :change_column_type_concurrently
-          :rename_column_concurrently
-          :update_column_in_batches
-        ].join(' ').freeze
-
-        def_node_matcher :batch_update?, <<~PATTERN
-          (send nil? ${#{BATCH_UPDATE_METHODS}} $(sym ...) ...)
-        PATTERN
-
-        def on_send(node)
-          return unless in_migration?(node)
-
-          matches = batch_update?(node)
-          return unless matches
-
-          update_method = matches.first
-          table = matches.last.to_a.first
-
-          return unless BLACKLISTED_TABLES.include?(table)
-
-          add_offense(node, location: :expression, message: format(MSG, update_method, table))
-        end
-      end
-    end
-  end
-end

rubocop/migration_helpers.rb

@@ -6,45 +6,6 @@ module RuboCop
       plan_limits
     ].freeze
 
-    # Blacklisted tables due to:
-    #   - size in GB (>= 10 GB on GitLab.com as of 02/2020)
-    #   - number of records
-    BLACKLISTED_TABLES = %i[
-       audit_events
-       ci_build_trace_sections
-       ci_builds
-       ci_builds_metadata
-       ci_job_artifacts
-       ci_pipeline_variables
-       ci_pipelines
-       ci_stages
-       deployments
-       events
-       issues
-       merge_request_diff_commits
-       merge_request_diff_files
-       merge_request_diffs
-       merge_request_metrics
-       merge_requests
-       namespaces
-       note_diff_files
-       notes
-       project_authorizations
-       projects
-       project_ci_cd_settings
-       project_features
-       push_event_payloads
-       resource_label_events
-       routes
-       sent_notifications
-       services
-       system_note_metadata
-       taggings
-       todos
-       users
-       web_hook_logs
-    ].freeze
-
     # Blacklisted tables due to:
     #   - number of columns (> 50 on GitLab.com as of 03/2020)
     #   - number of records

rubocop/rubocop-migrations.yml

+Migration/UpdateLargeTable:
+  Enabled: true
+  DeniedTables: &denied_tables # size in GB (>= 10 GB on GitLab.com as of 02/2020) and/or number of records
+    - :audit_events
+    - :ci_build_trace_sections
+    - :ci_builds
+    - :ci_builds_metadata
+    - :ci_job_artifacts
+    - :ci_pipeline_variables
+    - :ci_pipelines
+    - :ci_stages
+    - :deployments
+    - :events
+    - :issues
+    - :merge_request_diff_commits
+    - :merge_request_diff_files
+    - :merge_request_diffs
+    - :merge_request_metrics
+    - :merge_requests
+    - :namespaces
+    - :note_diff_files
+    - :notes
+    - :project_authorizations
+    - :projects
+    - :project_ci_cd_settings
+    - :project_features
+    - :push_event_payloads
+    - :resource_label_events
+    - :routes
+    - :sent_notifications
+    - :services
+    - :system_note_metadata
+    - :taggings
+    - :todos
+    - :users
+    - :web_hook_logs
+  DeniedMethods:
+    - :change_column_type_concurrently
+    - :rename_column_concurrently
+    - :update_column_in_batches

spec/rubocop/cop/migration/update_large_table_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-require 'rubocop'
-require 'rubocop/rspec/support'
-
-require_relative '../../../../rubocop/cop/migration/update_large_table'
-
-describe RuboCop::Cop::Migration::UpdateLargeTable do
-  include CopHelper
-
-  subject(:cop) { described_class.new }
-
-  context 'in migration' do
-    before do
-      allow(cop).to receive(:in_migration?).and_return(true)
-    end
-
-    shared_examples 'large tables' do |update_method|
-      described_class::BLACKLISTED_TABLES.each do |table|
-        it "registers an offense for the #{table} table" do
-          inspect_source("#{update_method} :#{table}, :column, default: true")
-
-          aggregate_failures do
-            expect(cop.offenses.size).to eq(1)
-            expect(cop.offenses.map(&:line)).to eq([1])
-          end
-        end
-      end
-    end
-
-    context 'for the add_column_with_default method' do
-      include_examples 'large tables', 'add_column_with_default'
-    end
-
-    context 'for the change_column_type_concurrently method' do
-      include_examples 'large tables', 'change_column_type_concurrently'
-    end
-
-    context 'for the rename_column_concurrently method' do
-      include_examples 'large tables', 'rename_column_concurrently'
-    end
-
-    context 'for the update_column_in_batches method' do
-      include_examples 'large tables', 'update_column_in_batches'
-    end
-
-    it 'registers no offense for non-blacklisted tables' do
-      inspect_source("add_column_with_default :table, :column, default: true")
-
-      expect(cop.offenses).to be_empty
-    end
-
-    it 'registers no offense for non-blacklisted methods' do
-      table = described_class::BLACKLISTED_TABLES.sample
-
-      inspect_source("some_other_method :#{table}, :column, default: true")
-
-      expect(cop.offenses).to be_empty
-    end
-  end
-
-  context 'outside of migration' do
-    let(:table) { described_class::BLACKLISTED_TABLES.sample }
-
-    it 'registers no offense for add_column_with_default' do
-      inspect_source("add_column_with_default :#{table}, :column, default: true")
-
-      expect(cop.offenses).to be_empty
-    end
-
-    it 'registers no offense for change_column_type_concurrently' do
-      inspect_source("change_column_type_concurrently :#{table}, :column, default: true")
-
-      expect(cop.offenses).to be_empty
-    end
-
-    it 'registers no offense for rename_column_concurrently' do
-      inspect_source("rename_column_concurrently :#{table}, :column, default: true")
-
-      expect(cop.offenses).to be_empty
-    end
-
-    it 'registers no offense for update_column_concurrently' do
-      inspect_source("update_column_concurrently :#{table}, :column, default: true")
-
-      expect(cop.offenses).to be_empty
-    end
-  end
-end