Merge branch 'ban-rails-default-scope' into 'master'

Ban rails default_scope usage

See merge request gitlab-org/gitlab!33847

.rubocop.yml

@@ -182,6 +182,9 @@ Rails/ApplicationRecord:
     - ee/db/**/*.rb
     - ee/spec/**/*.rb
 
+Cop/DefaultScope:
+  Enabled: true
+
 Rails/FindBy:
   Enabled: true
   Include:

app/models/badge.rb

@@ -18,7 +18,7 @@ class Badge < ApplicationRecord
   # This regex will build the new PLACEHOLDER_REGEX with the new information
   PLACEHOLDERS_REGEX = /(#{PLACEHOLDERS.keys.join('|')})/.freeze
 
-  default_scope { order_created_at_asc }
+  default_scope { order_created_at_asc } # rubocop:disable Cop/DefaultScope
 
   scope :order_created_at_asc, -> { reorder(created_at: :asc) }
 

app/models/ci/freeze_period.rb

@@ -5,7 +5,7 @@ module Ci
     include StripAttribute
     self.table_name = 'ci_freeze_periods'
 
-    default_scope { order(created_at: :asc) }
+    default_scope { order(created_at: :asc) } # rubocop:disable Cop/DefaultScope
 
     belongs_to :project, inverse_of: :freeze_periods
 

app/models/event.rb

@@ -9,7 +9,7 @@ class Event < ApplicationRecord
   include Gitlab::Utils::StrongMemoize
   include UsageStatistics
 
-  default_scope { reorder(nil) }
+  default_scope { reorder(nil) } # rubocop:disable Cop/DefaultScope
 
   ACTIONS = HashWithIndifferentAccess.new(
     created:    1,

app/models/label.rb

@@ -31,7 +31,7 @@ class Label < ApplicationRecord
   validates :title, uniqueness: { scope: [:group_id, :project_id] }
   validates :title, length: { maximum: 255 }
 
-  default_scope { order(title: :asc) }
+  default_scope { order(title: :asc) } # rubocop:disable Cop/DefaultScope
 
   scope :templates, -> { where(template: true, type: [Label.name, nil]) }
   scope :with_title, ->(title) { where(title: title) }

app/models/members/group_member.rb

@@ -13,7 +13,7 @@ class GroupMember < Member
   # Make sure group member points only to group as it source
   default_value_for :source_type, SOURCE_TYPE
   validates :source_type, format: { with: /\ANamespace\z/ }
-  default_scope { where(source_type: SOURCE_TYPE) }
+  default_scope { where(source_type: SOURCE_TYPE) } # rubocop:disable Cop/DefaultScope
 
   scope :of_groups, ->(groups) { where(source_id: groups.select(:id)) }
   scope :count_users_by_group_id, -> { joins(:user).group(:source_id).count }

app/models/members/project_member.rb

@@ -9,7 +9,7 @@ class ProjectMember < Member
   default_value_for :source_type, SOURCE_TYPE
   validates :source_type, format: { with: /\AProject\z/ }
   validates :access_level, inclusion: { in: Gitlab::Access.values }
-  default_scope { where(source_type: SOURCE_TYPE) }
+  default_scope { where(source_type: SOURCE_TYPE) } # rubocop:disable Cop/DefaultScope
 
   scope :in_project, ->(project) { where(source_id: project.id) }
   scope :in_namespaces, ->(groups) do

app/models/releases/evidence.rb

@@ -7,7 +7,7 @@ module Releases
 
     belongs_to :release, inverse_of: :evidences
 
-    default_scope { order(created_at: :asc) }
+    default_scope { order(created_at: :asc) } # rubocop:disable Cop/DefaultScope
 
     sha_attribute :summary_sha
     alias_attribute :collected_at, :created_at

app/models/repository_language.rb

@@ -4,7 +4,7 @@ class RepositoryLanguage < ApplicationRecord
   belongs_to :project
   belongs_to :programming_language
 
-  default_scope { includes(:programming_language) }
+  default_scope { includes(:programming_language) } # rubocop:disable Cop/DefaultScope
 
   validates :project, presence: true
   validates :share, inclusion: { in: 0..100, message: "The share of a language is between 0 and 100" }

db/post_migrate/20200406102120_backfill_deployment_clusters_from_deployments.rb

@@ -17,7 +17,7 @@ class BackfillDeploymentClustersFromDeployments < ActiveRecord::Migration[6.0]
   class Deployment < ActiveRecord::Base
     include EachBatch
 
-    default_scope { where('cluster_id IS NOT NULL') }
+    default_scope { where('cluster_id IS NOT NULL') } # rubocop:disable Cop/DefaultScope
 
     self.table_name = 'deployments'
   end

ee/spec/models/operations/feature_flag_scope_spec.rb

@@ -30,22 +30,22 @@ RSpec.describe Operations::FeatureFlagScope do
 
     context 'when environment scope of a default scope is updated' do
       let!(:feature_flag) { create(:operations_feature_flag) }
-      let!(:default_scope) { feature_flag.default_scope }
+      let!(:scope_default) { feature_flag.default_scope }
 
       it 'keeps default scope intact' do
-        default_scope.update(environment_scope: 'review/*')
+        scope_default.update(environment_scope: 'review/*')
 
-        expect(default_scope.errors[:environment_scope])
+        expect(scope_default.errors[:environment_scope])
           .to include("cannot be changed from default scope")
       end
     end
 
     context 'when a default scope is destroyed' do
       let!(:feature_flag) { create(:operations_feature_flag) }
-      let!(:default_scope) { feature_flag.default_scope }
+      let!(:scope_default) { feature_flag.default_scope }
 
       it 'prevents from destroying the default scope' do
-        expect { default_scope.destroy! }.to raise_error(ActiveRecord::ReadOnlyRecord)
+        expect { scope_default.destroy! }.to raise_error(ActiveRecord::ReadOnlyRecord)
       end
     end
 

ee/spec/requests/api/feature_flags_spec.rb

@@ -238,7 +238,7 @@ RSpec.describe API::FeatureFlags do
   end
 
   describe 'POST /projects/:id/feature_flags' do
-    def default_scope
+    def scope_default
       {
         environment_scope: '*',
         active: false,
@@ -253,7 +253,7 @@ RSpec.describe API::FeatureFlags do
     let(:params) do
       {
         name: 'awesome-feature',
-        scopes: [default_scope]
+        scopes: [scope_default]
       }
     end
 
@@ -328,7 +328,7 @@ RSpec.describe API::FeatureFlags do
           name: 'awesome-feature',
           description: 'this is awesome',
           scopes: [
-            default_scope,
+            scope_default,
             scope_with_user_with_id
           ]
         }

lib/gitlab/background_migration/fix_projects_without_prometheus_service.rb

@@ -62,7 +62,7 @@ module Gitlab
         class PrometheusService < ActiveRecord::Base
           self.inheritance_column = :_type_disabled
           self.table_name = 'services'
-          default_scope { where(type: type) }
+          default_scope { where(type: type) } # rubocop:disable Cop/DefaultScope
 
           def self.type
             'PrometheusService'

rubocop/cop/default_scope.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    # Cop that blacklists the use of `default_scope`.
+    class DefaultScope < RuboCop::Cop::Cop
+      MSG = <<~EOF
+        Do not use `default_scope`, as it does not follow the principle of
+        least surprise. See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/33847
+        for more details.
+      EOF
+
+      def_node_matcher :default_scope?, <<~PATTERN
+        (send {nil? (const nil? ...)} :default_scope ...)
+      PATTERN
+
+      def on_send(node)
+        return unless default_scope?(node)
+
+        add_offense(node, location: :expression)
+      end
+    end
+  end
+end

spec/rubocop/cop/default_scope_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../rubocop/cop/default_scope'
+
+describe RuboCop::Cop::DefaultScope do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  it 'does not flag the use of default_scope with a send receiver' do
+    inspect_source('foo.default_scope')
+
+    expect(cop.offenses.size).to eq(0)
+  end
+
+  it 'flags the use of default_scope with a constant receiver' do
+    inspect_source('User.default_scope')
+
+    expect(cop.offenses.size).to eq(1)
+  end
+
+  it 'flags the use of default_scope with a nil receiver' do
+    inspect_source('class Foo ; default_scope ; end')
+
+    expect(cop.offenses.size).to eq(1)
+  end
+
+  it 'flags the use of default_scope when passing arguments' do
+    inspect_source('class Foo ; default_scope(:foo) ; end')
+
+    expect(cop.offenses.size).to eq(1)
+  end
+
+  it 'flags the use of default_scope when passing a block' do
+    inspect_source('class Foo ; default_scope { :foo } ; end')
+
+    expect(cop.offenses.size).to eq(1)
+  end
+
+  it 'ignores the use of default_scope with a local variable receiver' do
+    inspect_source('users = User.all ; users.default_scope')
+
+    expect(cop.offenses.size).to eq(0)
+  end
+end