Merge branch '29130-add-a-rubocop-rule-to-warn-developers-to-put-new-routes-under' into 'master'

Add a rubocop rule to put new project routes under /-/

Closes #29130

See merge request gitlab-org/gitlab!20460

config/routes/project.rb

 # frozen_string_literal: true
 
+# rubocop: disable Cop/PutProjectRoutesUnderScope
 resources :projects, only: [:index, :new, :create]
 
 draw :git_http
 
 get '/projects/:id' => 'projects#resolve'
+# rubocop: enable Cop/PutProjectRoutesUnderScope
 
 constraints(::Constraints::ProjectUrlConstrainer.new) do
   # If the route has a wildcard segment, the segment has a regex constraint,
@@ -210,6 +212,10 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
       end
       # End of the /-/ scope.
 
+      # All new routes should go under /-/ scope.
+      # Look for scope '-' at the top of the file.
+      # rubocop: disable Cop/PutProjectRoutesUnderScope
+
       #
       # Templates
       #
@@ -522,6 +528,10 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
       draw :wiki
       draw :repository
 
+      # All new routes should go under /-/ scope.
+      # Look for scope '-' at the top of the file.
+      # rubocop: enable Cop/PutProjectRoutesUnderScope
+
       # Legacy routes.
       # Introduced in 12.0.
       # Should be removed with https://gitlab.com/gitlab-org/gitlab/issues/28848.
@@ -533,6 +543,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
                                             :cycle_analytics, :mattermost, :variables, :triggers)
     end
 
+    # rubocop: disable Cop/PutProjectRoutesUnderScope
     resources(:projects,
               path: '/',
               constraints: { id: Gitlab::PathRegex.project_route_regex },
@@ -554,5 +565,6 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
         put :new_issuable_address
       end
     end
+    # rubocop: enable Cop/PutProjectRoutesUnderScope
   end
 end

ee/config/routes/project.rb

@@ -64,6 +64,10 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
       end
       # End of the /-/ scope.
 
+      # All new routes should go under /-/ scope.
+      # Look for scope '-' at the top of the file.
+      # rubocop: disable Cop/PutProjectRoutesUnderScope
+
       resources :path_locks, only: [:index, :destroy] do
         collection do
           post :toggle
@@ -206,6 +210,10 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
       end
 
       resources :audit_events, only: [:index]
+
+      # All new routes should go under /-/ scope.
+      # Look for scope '-' at the top of the file.
+      # rubocop: enable Cop/PutProjectRoutesUnderScope
     end
   end
 end

rubocop/cop/put_project_routes_under_scope.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    # Checks for a project routes outside '/-/' scope.
+    # For more information see: https://gitlab.com/gitlab-org/gitlab/issues/29572
+    class PutProjectRoutesUnderScope < RuboCop::Cop::Cop
+      MSG = 'Put new project routes under /-/ scope'
+
+      def_node_matcher :dash_scope?, <<~PATTERN
+        (:send nil? :scope (:str "-"))
+      PATTERN
+
+      def on_send(node)
+        return unless in_project_routes?(node)
+        return unless resource?(node)
+        return unless outside_scope?(node)
+
+        add_offense(node)
+      end
+
+      def outside_scope?(node)
+        node.each_ancestor(:block).none? do |parent|
+          dash_scope?(parent.to_a.first)
+        end
+      end
+
+      def in_project_routes?(node)
+        path = node.location.expression.source_buffer.name
+        dirname = File.dirname(path)
+        filename = File.basename(path)
+
+        dirname.end_with?('config/routes') &&
+          filename.end_with?('project.rb')
+      end
+
+      def resource?(node)
+        node.method_name == :resource ||
+          node.method_name == :resources
+      end
+    end
+  end
+end

rubocop/rubocop.rb

@@ -14,6 +14,7 @@ require_relative 'cop/avoid_break_from_strong_memoize'
 require_relative 'cop/avoid_route_redirect_leading_slash'
 require_relative 'cop/line_break_around_conditional_block'
 require_relative 'cop/prefer_class_methods_over_module'
+require_relative 'cop/put_project_routes_under_scope'
 require_relative 'cop/migration/add_column'
 require_relative 'cop/migration/add_concurrent_foreign_key'
 require_relative 'cop/migration/add_concurrent_index'

spec/rubocop/cop/put_project_routes_under_scope_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'rubocop'
+require_relative '../../../rubocop/cop/put_project_routes_under_scope'
+
+describe RuboCop::Cop::PutProjectRoutesUnderScope do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  before do
+    allow(cop).to receive(:in_project_routes?).and_return(true)
+  end
+
+  it 'registers an offense when route is outside scope' do
+    expect_offense(<<~PATTERN.strip_indent)
+      scope '-' do
+        resource :issues
+      end
+
+      resource :notes
+      ^^^^^^^^^^^^^^^ Put new project routes under /-/ scope
+    PATTERN
+  end
+
+  it 'does not register an offense when resource inside the scope' do
+    expect_no_offenses(<<~PATTERN.strip_indent)
+      scope '-' do
+        resource :issues
+        resource :notes
+      end
+    PATTERN
+  end
+
+  it 'does not register an offense when resource is deep inside the scope' do
+    expect_no_offenses(<<~PATTERN.strip_indent)
+      scope '-' do
+        resource :issues
+        resource :projects do
+          resource :issues do
+            resource :notes
+          end
+        end
+      end
+    PATTERN
+  end
+end