Merge branch 'ab/rubocop-add-column-with-default-not-null' into 'master'

Disallow add_column_with_default with NOT NULL

See merge request gitlab-org/gitlab!21177

db/migrate/20180305144721_add_privileged_to_runner.rb

@@ -9,7 +9,7 @@ class AddPrivilegedToRunner < ActiveRecord::Migration[4.2]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default :clusters_applications_runners, :privileged, :boolean, default: true, allow_null: false
+    add_column_with_default :clusters_applications_runners, :privileged, :boolean, default: true, allow_null: false # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20180423204600_add_pages_access_level_to_project_feature.rb

@@ -5,7 +5,7 @@ class AddPagesAccessLevelToProjectFeature < ActiveRecord::Migration[4.2]
   DOWNTIME = false
 
   def up
-    add_column_with_default(:project_features, :pages_access_level, :integer, default: ProjectFeature::PUBLIC, allow_null: false)
+    add_column_with_default(:project_features, :pages_access_level, :integer, default: ProjectFeature::PUBLIC, allow_null: false) # rubocop:disable Migration/AddColumnWithDefault
 
     change_column_default(:project_features, :pages_access_level, ProjectFeature::ENABLED)
   end

db/migrate/20180515005612_add_squash_to_merge_requests.rb

@@ -10,7 +10,7 @@ class AddSquashToMergeRequests < ActiveRecord::Migration[4.2]
   def up
     unless column_exists?(:merge_requests, :squash)
       # rubocop:disable Migration/UpdateLargeTable
-      add_column_with_default :merge_requests, :squash, :boolean, default: false, allow_null: false
+      add_column_with_default :merge_requests, :squash, :boolean, default: false, allow_null: false # rubocop:disable Migration/AddColumnWithDefault
     end
   end
 

db/migrate/20180529093006_ensure_remote_mirror_columns.rb

@@ -11,7 +11,7 @@ class EnsureRemoteMirrorColumns < ActiveRecord::Migration[4.2]
     add_column :remote_mirrors, :remote_name, :string unless column_exists?(:remote_mirrors, :remote_name) # rubocop:disable Migration/AddLimitToStringColumns
 
     unless column_exists?(:remote_mirrors, :only_protected_branches)
-      add_column_with_default(:remote_mirrors,
+      add_column_with_default(:remote_mirrors, # rubocop:disable Migration/AddColumnWithDefault
                               :only_protected_branches,
                               :boolean,
                               default: false,

db/migrate/20180601213245_add_deploy_strategy_to_project_auto_devops.rb

@@ -10,7 +10,7 @@ class AddDeployStrategyToProjectAutoDevops < ActiveRecord::Migration[4.2]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default :project_auto_devops, :deploy_strategy, :integer, default: 0, allow_null: false
+    add_column_with_default :project_auto_devops, :deploy_strategy, :integer, default: 0, allow_null: false # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20181016141739_add_status_to_deployments.rb

@@ -15,7 +15,7 @@ class AddStatusToDeployments < ActiveRecord::Migration[4.2]
   # However, we have to use the default value for avoiding `NOT NULL` violation during the transition period.
   # The default value should be removed in the future release.
   def up
-    add_column_with_default(:deployments,
+    add_column_with_default(:deployments, # rubocop:disable Migration/AddColumnWithDefault
       :status,
       :integer,
       limit: 2,

db/migrate/20190218134158_add_masked_to_ci_variables.rb

@@ -12,7 +12,7 @@ class AddMaskedToCiVariables < ActiveRecord::Migration[5.0]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default :ci_variables, :masked, :boolean, default: false, allow_null: false
+    add_column_with_default :ci_variables, :masked, :boolean, default: false, allow_null: false # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190218134209_add_masked_to_ci_group_variables.rb

@@ -12,7 +12,7 @@ class AddMaskedToCiGroupVariables < ActiveRecord::Migration[5.0]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default :ci_group_variables, :masked, :boolean, default: false, allow_null: false
+    add_column_with_default :ci_group_variables, :masked, :boolean, default: false, allow_null: false # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190228192410_add_multi_line_attributes_to_suggestion.rb

@@ -8,9 +8,11 @@ class AddMultiLineAttributesToSuggestion < ActiveRecord::Migration[5.0]
   disable_ddl_transaction!
 
   def up
+    # rubocop:disable Migration/AddColumnWithDefault
     add_column_with_default :suggestions, :lines_above, :integer, default: 0, allow_null: false
     add_column_with_default :suggestions, :lines_below, :integer, default: 0, allow_null: false
     add_column_with_default :suggestions, :outdated, :boolean, default: false, allow_null: false
+    # rubocop:enable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190416185130_add_merge_train_enabled_to_ci_cd_settings.rb

@@ -8,7 +8,7 @@ class AddMergeTrainEnabledToCiCdSettings < ActiveRecord::Migration[5.1]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default :project_ci_cd_settings, :merge_trains_enabled, :boolean, default: false, allow_null: false
+    add_column_with_default :project_ci_cd_settings, :merge_trains_enabled, :boolean, default: false, allow_null: false # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190426180107_add_deployment_events_to_services.rb

@@ -8,7 +8,7 @@ class AddDeploymentEventsToServices < ActiveRecord::Migration[5.0]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default(:services, :deployment_events, :boolean, default: false, allow_null: false)
+    add_column_with_default(:services, :deployment_events, :boolean, default: false, allow_null: false) # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190709204413_add_rule_type_to_approval_project_rules.rb

@@ -8,7 +8,7 @@ class AddRuleTypeToApprovalProjectRules < ActiveRecord::Migration[5.1]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default :approval_project_rules, :rule_type, :integer, limit: 2, default: 0, allow_null: false
+    add_column_with_default :approval_project_rules, :rule_type, :integer, limit: 2, default: 0, allow_null: false # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190907184714_add_show_whitespace_in_diffs_to_user_preferences.rb

@@ -11,7 +11,7 @@ class AddShowWhitespaceInDiffsToUserPreferences < ActiveRecord::Migration[5.2]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default :user_preferences, :show_whitespace_in_diffs, :boolean, default: true, allow_null: false
+    add_column_with_default :user_preferences, :show_whitespace_in_diffs, :boolean, default: true, allow_null: false # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190918104731_add_cleanup_status_to_cluster.rb

@@ -9,7 +9,7 @@ class AddCleanupStatusToCluster < ActiveRecord::Migration[5.2]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default(:clusters, :cleanup_status,
+    add_column_with_default(:clusters, :cleanup_status, # rubocop:disable Migration/AddColumnWithDefault
       :smallint,
       default: 1,
       allow_null: false)

db/migrate/20191029191901_add_enabled_to_grafana_integrations.rb

@@ -8,7 +8,7 @@ class AddEnabledToGrafanaIntegrations < ActiveRecord::Migration[5.2]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default(
+    add_column_with_default( # rubocop:disable Migration/AddColumnWithDefault
       :grafana_integrations,
       :enabled,
       :boolean,

db/migrate/20191112090226_add_artifacts_to_ci_build_need.rb

@@ -8,7 +8,7 @@ class AddArtifactsToCiBuildNeed < ActiveRecord::Migration[5.2]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default(:ci_build_needs, :artifacts,
+    add_column_with_default(:ci_build_needs, :artifacts, # rubocop:disable Migration/AddColumnWithDefault
                             :boolean,
                             default: true,
                             allow_null: false)

doc/development/what_requires_downtime.md

@@ -34,6 +34,9 @@ blocking access to the table being modified. See ["Adding Columns With Default
 Values"](migration_style_guide.md#adding-columns-with-default-values) for more
 information on how to use this method.
 
+Note that usage of `add_column_with_default` with `allow_null: false` to also add
+a `NOT NULL` constraint is [discouraged](https://gitlab.com/gitlab-org/gitlab/issues/38060).
+
 ## Dropping Columns
 
 Removing columns is tricky because running GitLab processes may still be using

ee/db/geo/migrate/20171009162208_add_file_registry_success.rb

@@ -7,7 +7,7 @@ class AddFileRegistrySuccess < ActiveRecord::Migration[4.2]
 
   def up
     # Ensure existing rows are recorded as successes
-    add_column_with_default :file_registry, :success, :boolean, default: true, allow_null: false
+    add_column_with_default :file_registry, :success, :boolean, default: true, allow_null: false # rubocop:disable Migration/AddColumnWithDefault
 
     change_column :file_registry, :success, :boolean, default: false
   end

ee/db/geo/migrate/20180323182105_add_missing_on_primary_to_file_registry.rb

@@ -6,7 +6,7 @@ class AddMissingOnPrimaryToFileRegistry < ActiveRecord::Migration[4.2]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default :file_registry, :missing_on_primary, :boolean, default: false, allow_null: false
+    add_column_with_default :file_registry, :missing_on_primary, :boolean, default: false, allow_null: false # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

ee/db/geo/migrate/20180402170913_add_missing_on_primary_to_job_artifact_registry..rb

@@ -6,7 +6,7 @@ class AddMissingOnPrimaryToJobArtifactRegistry < ActiveRecord::Migration[4.2]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default :job_artifact_registry, :missing_on_primary, :boolean, default: false, allow_null: false
+    add_column_with_default :job_artifact_registry, :missing_on_primary, :boolean, default: false, allow_null: false # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

rubocop/cop/migration/add_column_with_default.rb

+# frozen_string_literal: true
+
+require_relative '../../migration_helpers'
+
+module RuboCop
+  module Cop
+    module Migration
+      # Cop that checks if columns are added in a way that doesn't require
+      # downtime.
+      class AddColumnWithDefault < RuboCop::Cop::Cop
+        include MigrationHelpers
+
+        WHITELISTED_TABLES = [:application_settings].freeze
+
+        MSG = '`add_column_with_default` with `allow_null: false` may cause prolonged lock situations and downtime, ' \
+          'see https://gitlab.com/gitlab-org/gitlab/issues/38060'.freeze
+
+        def on_send(node)
+          return unless in_migration?(node)
+
+          name = node.children[1]
+
+          return unless name == :add_column_with_default
+
+          # Ignore whitelisted tables.
+          return if table_whitelisted?(node.children[2])
+
+          opts = node.children.last
+
+          return unless opts && opts.type == :hash
+
+          opts.each_node(:pair) do |pair|
+            if disallows_null_values?(pair)
+              add_offense(node, location: :selector)
+            end
+          end
+        end
+
+        def table_whitelisted?(symbol)
+          symbol && symbol.type == :sym &&
+            WHITELISTED_TABLES.include?(symbol.children[0])
+        end
+
+        def disallows_null_values?(pair)
+          options = [hash_key_type(pair), hash_key_name(pair), hash_value(pair)]
+
+          options == [:sym, :allow_null, :false] # rubocop:disable Lint/BooleanSymbol
+        end
+
+        def hash_key_type(pair)
+          pair.children[0].type
+        end
+
+        def hash_key_name(pair)
+          pair.children[0].children[0]
+        end
+
+        def hash_value(pair)
+          pair.children[1].type
+        end
+      end
+    end
+  end
+end

rubocop/rubocop.rb

@@ -17,6 +17,7 @@ require_relative 'cop/prefer_class_methods_over_module'
 require_relative 'cop/put_project_routes_under_scope'
 require_relative 'cop/put_group_routes_under_scope'
 require_relative 'cop/migration/add_column'
+require_relative 'cop/migration/add_column_with_default'
 require_relative 'cop/migration/add_concurrent_foreign_key'
 require_relative 'cop/migration/add_concurrent_index'
 require_relative 'cop/migration/add_index'

spec/rubocop/cop/migration/add_column_with_default_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../../rubocop/cop/migration/add_column_with_default'
+
+describe RuboCop::Cop::Migration::AddColumnWithDefault do
+  include CopHelper
+
+  let(:cop) { described_class.new }
+
+  context 'outside of a migration' do
+    it 'does not register any offenses' do
+      expect_no_offenses(<<~RUBY)
+        def up
+          add_reference(:projects, :users)
+        end
+      RUBY
+    end
+  end
+
+  context 'in a migration' do
+    before do
+      allow(cop).to receive(:in_migration?).and_return(true)
+    end
+
+    let(:offense) { '`add_column_with_default` with `allow_null: false` may cause prolonged lock situations and downtime, see https://gitlab.com/gitlab-org/gitlab/issues/38060' }
+
+    it 'registers an offense when specifying allow_null: false' do
+      expect_offense(<<~RUBY)
+        def up
+          add_column_with_default(:ci_build_needs, :artifacts, :boolean, default: true, allow_null: false)
+          ^^^^^^^^^^^^^^^^^^^^^^^ #{offense}
+        end
+      RUBY
+    end
+
+    it 'registers no offense when specifying allow_null: true' do
+      expect_no_offenses(<<~RUBY)
+        def up
+          add_column_with_default(:ci_build_needs, :artifacts, :boolean, default: true, allow_null: true)
+        end
+      RUBY
+    end
+
+    it 'registers no offense when allow_null is not specified' do
+      expect_no_offenses(<<~RUBY)
+        def up
+          add_column_with_default(:ci_build_needs, :artifacts, :boolean, default: true)
+        end
+      RUBY
+    end
+
+    it 'registers no offense for application_settings (whitelisted table)' do
+      expect_no_offenses(<<~RUBY)
+        def up
+          add_column_with_default(:application_settings, :another_column, :boolean, default: true, allow_null: false)
+        end
+      RUBY
+    end
+  end
+end