Merge branch 'bvl-feature-category-not-owned-cop' into 'master'

Add a RuboCop to prevent new not_owned endpoints

See merge request gitlab-org/gitlab!83958

app/controllers/admin/application_settings_controller.rb

@@ -13,7 +13,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
 
   before_action :disable_query_limiting, only: [:usage_data]
 
-  feature_category :not_owned, [
+  feature_category :not_owned, [ # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
                      :general, :reporting, :metrics_and_profiling, :network,
                      :preferences, :update, :reset_health_check_token
                    ]

app/controllers/admin/background_jobs_controller.rb

 # frozen_string_literal: true
 
 class Admin::BackgroundJobsController < Admin::ApplicationController
-  feature_category :not_owned
+  feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 end

app/controllers/admin/dashboard_controller.rb

@@ -5,7 +5,7 @@ class Admin::DashboardController < Admin::ApplicationController
 
   COUNTED_ITEMS = [Project, User, Group].freeze
 
-  feature_category :not_owned
+  feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
   # rubocop: disable CodeReuse/ActiveRecord
   def index

app/controllers/admin/health_check_controller.rb

 # frozen_string_literal: true
 
 class Admin::HealthCheckController < Admin::ApplicationController
-  feature_category :not_owned
+  feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
   def show
     @errors = HealthCheck::Utils.process_checks(checks)

app/controllers/admin/plan_limits_controller.rb

@@ -5,7 +5,7 @@ class Admin::PlanLimitsController < Admin::ApplicationController
 
   before_action :set_plan_limits
 
-  feature_category :not_owned
+  feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
   def create
     redirect_path = referer_path(request) || general_admin_application_settings_path

app/controllers/admin/requests_profiles_controller.rb

 # frozen_string_literal: true
 
 class Admin::RequestsProfilesController < Admin::ApplicationController
-  feature_category :not_owned
+  feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
   def index
     @profile_token = Gitlab::RequestProfiler.profile_token

app/controllers/admin/spam_logs_controller.rb

 # frozen_string_literal: true
 
 class Admin::SpamLogsController < Admin::ApplicationController
-  feature_category :not_owned
+  feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
   # rubocop: disable CodeReuse/ActiveRecord
   def index

app/controllers/admin/system_info_controller.rb

 # frozen_string_literal: true
 
 class Admin::SystemInfoController < Admin::ApplicationController
-  feature_category :not_owned
+  feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
   EXCLUDED_MOUNT_OPTIONS = %w[
     nobrowse

app/controllers/admin/version_check_controller.rb

 # frozen_string_literal: true
 
 class Admin::VersionCheckController < Admin::ApplicationController
-  feature_category :not_owned
+  feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
   def version_check
     response = VersionCheck.new.response

app/controllers/graphql_controller.rb

@@ -44,7 +44,7 @@ class GraphqlController < ApplicationController
   around_action :sessionless_bypass_admin_mode!, if: :sessionless_user?
 
   # The default feature category is overridden to read from request
-  feature_category :not_owned
+  feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
   # We don't know what the query is going to be, so we can't set a high urgency
   # See https://gitlab.com/groups/gitlab-org/-/epics/5841 for the work that will

app/controllers/help_controller.rb

@@ -3,7 +3,7 @@
 class HelpController < ApplicationController
   skip_before_action :authenticate_user!, unless: :public_visibility_restricted?
   skip_before_action :check_two_factor_requirement
-  feature_category :not_owned
+  feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
   layout 'help'
 

app/controllers/projects/serverless/functions_controller.rb

@@ -6,7 +6,7 @@ module Projects
       before_action :ensure_feature_enabled!
       before_action :authorize_read_cluster!
 
-      feature_category :not_owned
+      feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
       def index
         respond_to do |format|

app/controllers/projects/uploads_controller.rb

@@ -11,7 +11,7 @@ class Projects::UploadsController < Projects::ApplicationController
   before_action :authorize_upload_file!, only: [:create, :authorize]
   before_action :verify_workhorse_api!, only: [:authorize]
 
-  feature_category :not_owned
+  feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
   private
 

app/controllers/projects/work_items_controller.rb

@@ -5,7 +5,7 @@ class Projects::WorkItemsController < Projects::ApplicationController
     push_force_frontend_feature_flag(:work_items, project&.work_items_feature_flag_enabled?)
   end
 
-  feature_category :not_owned
+  feature_category :team_planning
 
   def index
     render_404 unless project&.work_items_feature_flag_enabled?

app/controllers/sandbox_controller.rb

@@ -3,7 +3,7 @@
 class SandboxController < ApplicationController # rubocop:disable Gitlab/NamespacedClass
   skip_before_action :authenticate_user!
 
-  feature_category :not_owned
+  feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
   def mermaid
     render layout: false

app/controllers/uploads_controller.rb

@@ -26,7 +26,7 @@ class UploadsController < ApplicationController
   before_action :authorize_create_access!, only: [:create, :authorize]
   before_action :verify_workhorse_api!, only: [:authorize]
 
-  feature_category :not_owned
+  feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
   def self.model_classes
     MODEL_CLASSES

app/workers/concerns/chaos_queue.rb

@@ -5,6 +5,6 @@ module ChaosQueue
 
   included do
     queue_namespace :chaos
-    feature_category_not_owned!
+    feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
   end
 end

app/workers/concerns/reactive_cacheable_worker.rb

@@ -8,7 +8,10 @@ module ReactiveCacheableWorker
 
     sidekiq_options retry: 3
 
-    feature_category_not_owned!
+    # Feature category is different depending on the model that is using the
+    # reactive cache. Identified by the `related_class` attribute.
+    feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
+
     loggable_arguments 0
 
     def self.context_for_arguments(arguments)

app/workers/concerns/worker_attributes.rb

@@ -35,17 +35,9 @@ module WorkerAttributes
 
   class_methods do
     def feature_category(value, *extras)
-      raise "Invalid category. Use `feature_category_not_owned!` to mark a worker as not owned" if value == :not_owned
-
       set_class_attribute(:feature_category, value)
     end
 
-    # Special case: mark this work as not associated with a feature category
-    # this should be used for cross-cutting concerns, such as mailer workers.
-    def feature_category_not_owned!
-      set_class_attribute(:feature_category, :not_owned)
-    end
-
     # Special case: if a worker is not owned, get the feature category
     # (if present) from the calling context.
     def get_feature_category

app/workers/delete_stored_files_worker.rb

@@ -7,7 +7,7 @@ class DeleteStoredFilesWorker # rubocop:disable Scalability/IdempotentWorker
 
   sidekiq_options retry: 3
 
-  feature_category_not_owned!
+  feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
   loggable_arguments 0
 
   def perform(class_name, keys)

app/workers/flush_counter_increments_worker.rb

@@ -12,7 +12,10 @@ class FlushCounterIncrementsWorker
 
   sidekiq_options retry: 3
 
-  feature_category_not_owned!
+  # The increments in `ProjectStatistics` are owned by several teams depending
+  # on the counter
+  feature_category :not_owned # rubocop:disable Gitlab/AvoidFeatureCategoryNotOwned
+
   urgency :low
   deduplicate :until_executing, including_scheduled: true
 

app/workers/object_storage/background_move_worker.rb

@@ -8,7 +8,7 @@ module ObjectStorage
     include ObjectStorageQueue
 
     sidekiq_options retry: 5
-    feature_category_not_owned!
+    feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
     loggable_arguments 0, 1, 2, 3
 
     def perform(uploader_class_name, subject_class_name, file_field, subject_id)

app/workers/object_storage/migrate_uploads_worker.rb

@@ -10,7 +10,7 @@ module ObjectStorage
     sidekiq_options retry: 3
     include ObjectStorageQueue
 
-    feature_category_not_owned!
+    feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
     loggable_arguments 0, 1, 2, 3
 
     SanityCheckError = Class.new(StandardError)

doc/development/feature_categorization/index.md

@@ -58,7 +58,8 @@ not, the specs will fail.
 ### Excluding Sidekiq workers from feature categorization
 
 A few Sidekiq workers, that are used across all features, cannot be mapped to a
-single category. These should be declared as such using the `feature_category_not_owned!`
+single category. These should be declared as such using the
+`feature_category :not_owned`
 declaration, as shown below:
 
 ```ruby
@@ -66,7 +67,7 @@ class SomeCrossCuttingConcernWorker
   include ApplicationWorker
 
   # Declares that this worker does not map to a feature category
-  feature_category_not_owned!
+  feature_category :not_owned # rubocop:disable Gitlab/AvoidFeatureCategoryNotOwned
 
   # ...
 end

ee/app/controllers/admin/emails_controller.rb

@@ -6,7 +6,7 @@ class Admin::EmailsController < Admin::ApplicationController
   before_action :check_license_send_emails_from_admin_area_available!
   before_action :check_emails_rate_limit!, only: [:create]
 
-  feature_category :not_owned
+  feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
   def show
   end

ee/app/controllers/projects/threat_monitoring_controller.rb

@@ -6,7 +6,7 @@ module Projects
 
     before_action :authorize_read_threat_monitoring!
 
-    feature_category :not_owned
+    feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
     # rubocop: disable CodeReuse/ActiveRecord
     def alert_details

ee/app/controllers/sitemap_controller.rb

@@ -3,7 +3,7 @@
 class SitemapController < ApplicationController
   skip_before_action :authenticate_user!
 
-  feature_category :not_owned
+  feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
   def show
     return render_404 unless Gitlab.com?

lib/api/admin/plan_limits.rb

@@ -5,7 +5,7 @@ module API
     class PlanLimits < ::API::Base
       before { authenticated_as_admin! }
 
-      feature_category :not_owned
+      feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
       helpers do
         def current_plan(name)

lib/api/admin/sidekiq.rb

@@ -5,7 +5,7 @@ module API
     class Sidekiq < ::API::Base
       before { authenticated_as_admin! }
 
-      feature_category :not_owned
+      feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
       namespace 'admin' do
         namespace 'sidekiq' do

lib/api/api.rb

@@ -321,7 +321,7 @@ module API
       end
     end
 
-    route :any, '*path', feature_category: :not_owned do
+    route :any, '*path', feature_category: :not_owned do # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
       error!('404 Not Found', 404)
     end
   end

lib/api/internal/base.rb

@@ -189,7 +189,7 @@ module API
           present actor.user, with: Entities::UserSafe
         end
 
-        get '/check', feature_category: :not_owned do
+        get '/check', feature_category: :not_owned do # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
           {
             api_version: API.version,
             gitlab_version: Gitlab::VERSION,

lib/api/markdown.rb

@@ -2,7 +2,7 @@
 
 module API
   class Markdown < ::API::Base
-    feature_category :not_owned
+    feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
     params do
       requires :text, type: String, desc: "The markdown text to render"

lib/api/projects.rb

@@ -631,7 +631,7 @@ module API
       desc 'Workhorse authorize the file upload' do
         detail 'This feature was introduced in GitLab 13.11'
       end
-      post ':id/uploads/authorize', feature_category: :not_owned do
+      post ':id/uploads/authorize', feature_category: :not_owned do # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
         require_gitlab_workhorse!
 
         status 200
@@ -643,7 +643,7 @@ module API
       params do
         requires :file, types: [Rack::Multipart::UploadedFile, ::API::Validations::Types::WorkhorseFile], desc: 'The attachment file to be uploaded'
       end
-      post ":id/uploads", feature_category: :not_owned do
+      post ":id/uploads", feature_category: :not_owned do # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
         log_if_upload_exceed_max_size(user_project, params[:file])
 
         service = UploadService.new(user_project, params[:file])

lib/api/settings.rb

@@ -4,7 +4,7 @@ module API
   class Settings < ::API::Base
     before { authenticated_as_admin! }
 
-    feature_category :not_owned
+    feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
     helpers Helpers::SettingsHelpers
 

lib/api/sidekiq_metrics.rb

@@ -6,7 +6,7 @@ module API
   class SidekiqMetrics < ::API::Base
     before { authenticated_as_admin! }
 
-    feature_category :not_owned
+    feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
     helpers do
       def queue_metrics

lib/api/version.rb

@@ -9,7 +9,7 @@ module API
 
     before { authenticate! }
 
-    feature_category :not_owned
+    feature_category :not_owned # rubocop:todo Gitlab/AvoidFeatureCategoryNotOwned
 
     METADATA_QUERY = <<~EOF
       {

rubocop/cop/gitlab/avoid_feature_category_not_owned.rb

+# frozen_string_literal: true
+
+require_relative '../../code_reuse_helpers'
+
+module RuboCop
+  module Cop
+    module Gitlab
+      class AvoidFeatureCategoryNotOwned < RuboCop::Cop::Cop
+        include ::RuboCop::CodeReuseHelpers
+
+        MSG = 'Avoid adding new endpoints with `feature_category :not_owned`. See https://docs.gitlab.com/ee/development/feature_categorization'
+        RESTRICT_ON_SEND = %i[feature_category get post put patch delete].freeze
+
+        def_node_matcher :feature_category_not_owned?, <<~PATTERN
+          (send _ :feature_category (sym :not_owned) ...)
+        PATTERN
+
+        def_node_matcher :feature_category_not_owned_api?, <<~PATTERN
+          (send nil? {:get :post :put :patch :delete} _
+            (hash <(pair (sym :feature_category) (sym :not_owned)) ...>)
+          )
+        PATTERN
+
+        def on_send(node)
+          return unless file_needs_feature_category?(node)
+          return unless setting_not_owned?(node)
+
+          add_offense(node, location: :expression)
+        end
+
+        private
+
+        def file_needs_feature_category?(node)
+          in_controller?(node) || in_worker?(node) || in_api?(node)
+        end
+
+        def setting_not_owned?(node)
+          feature_category_not_owned?(node) || feature_category_not_owned_api?(node)
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/sidekiq_middleware/server_metrics_spec.rb

@@ -292,7 +292,7 @@ RSpec.describe Gitlab::SidekiqMiddleware::ServerMetrics do
         if category
           feature_category category
         else
-          feature_category_not_owned!
+          feature_category :not_owned
         end
 
         def perform

spec/lib/gitlab/sidekiq_middleware/worker_context/client_spec.rb

@@ -28,7 +28,7 @@ RSpec.describe Gitlab::SidekiqMiddleware::WorkerContext::Client do
         'TestNotOwnedWithContextWorker'
       end
 
-      feature_category_not_owned!
+      feature_category :not_owned
     end
   end
 

spec/lib/gitlab/sidekiq_middleware/worker_context/server_spec.rb

@@ -29,7 +29,7 @@ RSpec.describe Gitlab::SidekiqMiddleware::WorkerContext::Server do
         "NotOwnedWorker"
       end
 
-      feature_category_not_owned!
+      feature_category :not_owned
     end
   end
 

spec/rubocop/cop/gitlab/avoid_feature_category_not_owned_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require_relative '../../../../rubocop/cop/gitlab/avoid_feature_category_not_owned'
+
+RSpec.describe RuboCop::Cop::Gitlab::AvoidFeatureCategoryNotOwned do
+  subject(:cop) { described_class.new }
+
+  shared_examples 'defining feature category on a class' do
+    it 'flags a method call on a class' do
+      expect_offense(<<~SOURCE)
+        feature_category :not_owned
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid adding new endpoints with `feature_category :not_owned`. See https://docs.gitlab.com/ee/development/feature_categorization
+      SOURCE
+    end
+
+    it 'flags a method call on a class with an array passed' do
+      expect_offense(<<~SOURCE)
+        feature_category :not_owned, [:index, :edit]
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid adding new endpoints with `feature_category :not_owned`. See https://docs.gitlab.com/ee/development/feature_categorization
+      SOURCE
+    end
+
+    it 'flags a method call on a class with an array passed' do
+      expect_offense(<<~SOURCE)
+        worker.feature_category :not_owned, [:index, :edit]
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid adding new endpoints with `feature_category :not_owned`. See https://docs.gitlab.com/ee/development/feature_categorization
+      SOURCE
+    end
+  end
+
+  context 'in controllers' do
+    before do
+      allow(subject).to receive(:in_controller?).and_return(true)
+    end
+
+    it_behaves_like 'defining feature category on a class'
+  end
+
+  context 'in workers' do
+    before do
+      allow(subject).to receive(:in_worker?).and_return(true)
+    end
+
+    it_behaves_like 'defining feature category on a class'
+  end
+
+  context 'for grape endpoints' do
+    before do
+      allow(subject).to receive(:in_api?).and_return(true)
+    end
+
+    it_behaves_like 'defining feature category on a class'
+
+    it 'flags when passed as a hash for a Grape endpoint as keyword args' do
+      expect_offense(<<~SOURCE)
+        get :hello, feature_category: :not_owned
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid adding new endpoints with `feature_category :not_owned`. See https://docs.gitlab.com/ee/development/feature_categorization
+      SOURCE
+    end
+
+    it 'flags when passed as a hash for a Grape endpoint in a hash' do
+      expect_offense(<<~SOURCE)
+        get :hello, { feature_category: :not_owned, urgency: :low}
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Avoid adding new endpoints with `feature_category :not_owned`. See https://docs.gitlab.com/ee/development/feature_categorization
+      SOURCE
+    end
+  end
+end

spec/workers/concerns/application_worker_spec.rb

@@ -49,7 +49,7 @@ RSpec.describe ApplicationWorker do
         worker.feature_category :pages
         expect(worker.sidekiq_options['queue']).to eq('queue_2')
 
-        worker.feature_category_not_owned!
+        worker.feature_category :not_owned
         expect(worker.sidekiq_options['queue']).to eq('queue_3')
 
         worker.urgency :high

spec/workers/every_sidekiq_worker_spec.rb

@@ -54,7 +54,7 @@ RSpec.describe 'Every Sidekiq worker' do
     # All Sidekiq worker classes should declare a valid `feature_category`
     # or explicitly be excluded with the `feature_category_not_owned!` annotation.
     # Please see doc/development/sidekiq_style_guide.md#feature-categorization for more details.
-    it 'has a feature_category or feature_category_not_owned! attribute', :aggregate_failures do
+    it 'has a feature_category attribute', :aggregate_failures do
       workers_without_defaults.each do |worker|
         expect(worker.get_feature_category).to be_a(Symbol), "expected #{worker.inspect} to declare a feature_category or feature_category_not_owned!"
       end