Merge branch '38385-disallow-add-column-with-default' into 'master'

Consider absence of explicit `allow_null: true` to be an offense too

Closes #38385

See merge request gitlab-org/gitlab!22234

db/migrate/20180831164905_add_common_to_prometheus_metrics.rb

@@ -8,7 +8,7 @@ class AddCommonToPrometheusMetrics < ActiveRecord::Migration[4.2]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default(:prometheus_metrics, :common, :boolean, default: false)
+    add_column_with_default(:prometheus_metrics, :common, :boolean, default: false) # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20180907015926_add_legacy_abac_to_cluster_providers_gcp.rb

@@ -8,7 +8,7 @@ class AddLegacyAbacToClusterProvidersGcp < ActiveRecord::Migration[4.2]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default(:cluster_providers_gcp, :legacy_abac, :boolean, default: true)
+    add_column_with_default(:cluster_providers_gcp, :legacy_abac, :boolean, default: true) # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20181017001059_add_cluster_type_to_clusters.rb

@@ -9,7 +9,7 @@ class AddClusterTypeToClusters < ActiveRecord::Migration[4.2]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default(:clusters, :cluster_type, :smallint, default: PROJECT_CLUSTER_TYPE)
+    add_column_with_default(:clusters, :cluster_type, :smallint, default: PROJECT_CLUSTER_TYPE) # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190220142344_add_email_header_and_footer_enabled_flag_to_appearances_table.rb

@@ -8,7 +8,7 @@ class AddEmailHeaderAndFooterEnabledFlagToAppearancesTable < ActiveRecord::Migra
   DOWNTIME = false
 
   def up
-    add_column_with_default(:appearances, :email_header_and_footer_enabled, :boolean, default: false)
+    add_column_with_default(:appearances, :email_header_and_footer_enabled, :boolean, default: false) # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190322164830_add_auto_ssl_enabled_to_pages_domain.rb

@@ -8,7 +8,7 @@ class AddAutoSslEnabledToPagesDomain < ActiveRecord::Migration[5.0]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default :pages_domains, :auto_ssl_enabled, :boolean, default: false
+    add_column_with_default :pages_domains, :auto_ssl_enabled, :boolean, default: false # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190325165127_add_managed_to_cluster.rb

@@ -8,7 +8,7 @@ class AddManagedToCluster < ActiveRecord::Migration[5.0]
   DOWNTIME = false
 
   def up
-    add_column_with_default(:clusters, :managed, :boolean, default: true)
+    add_column_with_default(:clusters, :managed, :boolean, default: true) # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190415030217_add_variable_type_to_ci_variables.rb

@@ -8,7 +8,7 @@ class AddVariableTypeToCiVariables < ActiveRecord::Migration[5.0]
   ENV_VAR_VARIABLE_TYPE = 1
 
   def up
-    add_column_with_default(:ci_variables, :variable_type, :smallint, default: ENV_VAR_VARIABLE_TYPE)
+    add_column_with_default(:ci_variables, :variable_type, :smallint, default: ENV_VAR_VARIABLE_TYPE) # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190416213556_add_variable_type_to_ci_group_variables.rb

@@ -8,7 +8,7 @@ class AddVariableTypeToCiGroupVariables < ActiveRecord::Migration[5.0]
   ENV_VAR_VARIABLE_TYPE = 1
 
   def up
-    add_column_with_default(:ci_group_variables, :variable_type, :smallint, default: ENV_VAR_VARIABLE_TYPE)
+    add_column_with_default(:ci_group_variables, :variable_type, :smallint, default: ENV_VAR_VARIABLE_TYPE) # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190416213615_add_variable_type_to_ci_pipeline_variables.rb

@@ -8,7 +8,7 @@ class AddVariableTypeToCiPipelineVariables < ActiveRecord::Migration[5.0]
   ENV_VAR_VARIABLE_TYPE = 1
 
   def up
-    add_column_with_default(:ci_pipeline_variables, :variable_type, :smallint, default: ENV_VAR_VARIABLE_TYPE)
+    add_column_with_default(:ci_pipeline_variables, :variable_type, :smallint, default: ENV_VAR_VARIABLE_TYPE) # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190416213631_add_variable_type_to_ci_pipeline_schedule_variables.rb

@@ -8,7 +8,7 @@ class AddVariableTypeToCiPipelineScheduleVariables < ActiveRecord::Migration[5.0
   ENV_VAR_VARIABLE_TYPE = 1
 
   def up
-    add_column_with_default(:ci_pipeline_schedule_variables, :variable_type, :smallint, default: ENV_VAR_VARIABLE_TYPE)
+    add_column_with_default(:ci_pipeline_schedule_variables, :variable_type, :smallint, default: ENV_VAR_VARIABLE_TYPE) # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190520200123_add_rule_type_to_approval_merge_request_approval_rules.rb

@@ -12,7 +12,7 @@ class AddRuleTypeToApprovalMergeRequestApprovalRules < ActiveRecord::Migration[5
   disable_ddl_transaction!
 
   def up
-    add_column_with_default(:approval_merge_request_rules, :rule_type, :integer, limit: 2, default: 1)
+    add_column_with_default(:approval_merge_request_rules, :rule_type, :integer, limit: 2, default: 1) # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190607085356_add_source_to_pages_domains.rb

@@ -12,7 +12,7 @@ class AddSourceToPagesDomains < ActiveRecord::Migration[5.1]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default(:pages_domains, :certificate_source, :smallint, default: 0)
+    add_column_with_default(:pages_domains, :certificate_source, :smallint, default: 0) # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190628145246_add_strategies_to_operations_feature_flag_scopes.rb

@@ -8,7 +8,7 @@ class AddStrategiesToOperationsFeatureFlagScopes < ActiveRecord::Migration[5.1]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default :operations_feature_flag_scopes, :strategies, :jsonb, default: [{ name: "default", parameters: {} }]
+    add_column_with_default :operations_feature_flag_scopes, :strategies, :jsonb, default: [{ name: "default", parameters: {} }] # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190712064021_add_namespace_per_environment_flag_to_clusters.rb

@@ -11,7 +11,7 @@ class AddNamespacePerEnvironmentFlagToClusters < ActiveRecord::Migration[5.1]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default :clusters, :namespace_per_environment, :boolean, default: false
+    add_column_with_default :clusters, :namespace_per_environment, :boolean, default: false # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190715173819_add_object_storage_flag_to_geo_node.rb

@@ -12,7 +12,7 @@ class AddObjectStorageFlagToGeoNode < ActiveRecord::Migration[5.2]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default :geo_nodes, :sync_object_storage, :boolean, default: false
+    add_column_with_default :geo_nodes, :sync_object_storage, :boolean, default: false # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190729180447_add_merge_requests_require_code_owner_approval_to_protected_branches.rb

@@ -9,7 +9,7 @@ class AddMergeRequestsRequireCodeOwnerApprovalToProtectedBranches < ActiveRecord
   disable_ddl_transaction!
 
   def up
-    add_column_with_default(
+    add_column_with_default( # rubocop:disable Migration/AddColumnWithDefault
       :protected_branches,
       :code_owner_approval_required,
       :boolean,

db/migrate/20190816151221_add_active_jobs_limit_to_plans.rb

@@ -8,7 +8,7 @@ class AddActiveJobsLimitToPlans < ActiveRecord::Migration[5.2]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default :plans, :active_jobs_limit, :integer, default: 0
+    add_column_with_default :plans, :active_jobs_limit, :integer, default: 0 # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190901174200_add_max_issue_count_to_list.rb

@@ -7,7 +7,7 @@ class AddMaxIssueCountToList < ActiveRecord::Migration[4.2]
   DOWNTIME = false
 
   def up
-    add_column_with_default :lists, :max_issue_count, :integer, default: 0
+    add_column_with_default :lists, :max_issue_count, :integer, default: 0 # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20190905140605_add_cloud_run_to_clusters_providers_gcp.rb

@@ -8,7 +8,7 @@ class AddCloudRunToClustersProvidersGcp < ActiveRecord::Migration[5.2]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default(:cluster_providers_gcp, :cloud_run, :boolean, default: false)
+    add_column_with_default(:cluster_providers_gcp, :cloud_run, :boolean, default: false) # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20191014123159_add_expire_notification_delivered_to_personal_access_tokens.rb

@@ -8,7 +8,7 @@ class AddExpireNotificationDeliveredToPersonalAccessTokens < ActiveRecord::Migra
   disable_ddl_transaction!
 
   def up
-    add_column_with_default :personal_access_tokens, :expire_notification_delivered, :boolean, default: false
+    add_column_with_default :personal_access_tokens, :expire_notification_delivered, :boolean, default: false # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20191023093207_add_comment_actions_to_services.rb

@@ -8,7 +8,7 @@ class AddCommentActionsToServices < ActiveRecord::Migration[5.2]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default(:services, :comment_on_event_enabled, :boolean, default: true)
+    add_column_with_default(:services, :comment_on_event_enabled, :boolean, default: true) # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20191028130054_add_max_issue_weight_to_list.rb

@@ -8,7 +8,7 @@ class AddMaxIssueWeightToList < ActiveRecord::Migration[5.2]
   DOWNTIME = false
 
   def up
-    add_column_with_default :lists, :max_issue_weight, :integer, default: 0
+    add_column_with_default :lists, :max_issue_weight, :integer, default: 0 # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20191105155113_add_secret_to_snippet.rb

@@ -9,7 +9,7 @@ class AddSecretToSnippet < ActiveRecord::Migration[5.2]
 
   def up
     unless column_exists?(:snippets, :secret)
-      add_column_with_default :snippets, :secret, :boolean, default: false
+      add_column_with_default :snippets, :secret, :boolean, default: false # rubocop:disable Migration/AddColumnWithDefault
     end
 
     add_concurrent_index :snippets, [:visibility_level, :secret]

db/migrate/20191106144901_add_state_to_merge_trains.rb

@@ -9,7 +9,7 @@ class AddStateToMergeTrains < ActiveRecord::Migration[5.2]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default :merge_trains, :status, :integer, limit: 2, default: MERGE_TRAIN_STATUS_CREATED
+    add_column_with_default :merge_trains, :status, :integer, limit: 2, default: MERGE_TRAIN_STATUS_CREATED # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20191121193110_add_issue_links_type.rb

@@ -8,7 +8,7 @@ class AddIssueLinksType < ActiveRecord::Migration[5.1]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default :issue_links, :link_type, :integer, default: 0, limit: 2
+    add_column_with_default :issue_links, :link_type, :integer, default: 0, limit: 2 # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20191127221608_add_wildcard_and_domain_type_to_pages_domains.rb

@@ -9,8 +9,10 @@ class AddWildcardAndDomainTypeToPagesDomains < ActiveRecord::Migration[5.2]
   disable_ddl_transaction!
 
   def up
+    # rubocop:disable Migration/AddColumnWithDefault
     add_column_with_default :pages_domains, :wildcard, :boolean, default: false
     add_column_with_default :pages_domains, :domain_type, :integer, limit: 2, default: PROJECT_TYPE
+    # rubocop:enable Migration/AddColumnWithDefault
   end
 
   def down

db/migrate/20191129134844_add_broadcast_type_to_broadcast_message.rb

@@ -10,7 +10,7 @@ class AddBroadcastTypeToBroadcastMessage < ActiveRecord::Migration[5.2]
   disable_ddl_transaction!
 
   def up
-    add_column_with_default(:broadcast_messages, :broadcast_type, :smallint, default: BROADCAST_MESSAGE_BANNER_TYPE)
+    add_column_with_default(:broadcast_messages, :broadcast_type, :smallint, default: BROADCAST_MESSAGE_BANNER_TYPE) # rubocop:disable Migration/AddColumnWithDefault
   end
 
   def down

ee/db/geo/migrate/20170606155045_add_needs_resync_to_project_registry.rb

@@ -6,8 +6,10 @@ class AddNeedsResyncToProjectRegistry < ActiveRecord::Migration[4.2]
   disable_ddl_transaction!
 
   def up
+    # rubocop:disable Migration/AddColumnWithDefault
     add_column_with_default(:project_registry, :resync_repository, :boolean, default: true)
     add_column_with_default(:project_registry, :resync_wiki, :boolean, default: true)
+    # rubocop:enable Migration/AddColumnWithDefault
   end
 
   def down

rubocop/cop/migration/add_column_with_default.rb

@@ -12,52 +12,33 @@ module RuboCop
 
         WHITELISTED_TABLES = [:application_settings].freeze
 
-        MSG = '`add_column_with_default` with `allow_null: false` may cause prolonged lock situations and downtime, ' \
+        MSG = '`add_column_with_default` without `allow_null: true` may cause prolonged lock situations and downtime, ' \
           'see https://gitlab.com/gitlab-org/gitlab/issues/38060'.freeze
 
+        def_node_matcher :add_column_with_default?, <<~PATTERN
+          (send _ :add_column_with_default $_ ... (hash $...))
+        PATTERN
+
         def on_send(node)
           return unless in_migration?(node)
 
-          name = node.children[1]
-
-          return unless name == :add_column_with_default
+          add_column_with_default?(node) do |table, options|
+            break if table_whitelisted?(table) || nulls_allowed?(options)
 
-          # Ignore whitelisted tables.
-          return if table_whitelisted?(node.children[2])
-
-          opts = node.children.last
+            add_offense(node, location: :selector)
+          end
+        end
 
-          return unless opts && opts.type == :hash
+        private
 
-          opts.each_node(:pair) do |pair|
-            if disallows_null_values?(pair)
-              add_offense(node, location: :selector)
-            end
-          end
+        def nulls_allowed?(options)
+          options.find { |opt| opt.key.value == :allow_null && opt.value.true_type? }
         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

spec/rubocop/cop/migration/add_column_with_default_spec.rb

@@ -27,7 +27,7 @@ describe RuboCop::Cop::Migration::AddColumnWithDefault 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' }
+    let(:offense) { '`add_column_with_default` without `allow_null: true` 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)
@@ -46,10 +46,11 @@ describe RuboCop::Cop::Migration::AddColumnWithDefault do
       RUBY
     end
 
-    it 'registers no offense when allow_null is not specified' do
-      expect_no_offenses(<<~RUBY)
+    it 'registers an offense when allow_null is not specified' do
+      expect_offense(<<~RUBY)
         def up
           add_column_with_default(:ci_build_needs, :artifacts, :boolean, default: true)
+          ^^^^^^^^^^^^^^^^^^^^^^^ #{offense}
         end
       RUBY
     end