Merge branch 'custom-empty-exception-class-cop' into 'master'

Add RuboCop cop for custom error classes

Closes #28770

See merge request !9573

app/controllers/projects/blob_controller.rb

@@ -5,7 +5,7 @@ class Projects::BlobController < Projects::ApplicationController
   include ActionView::Helpers::SanitizeHelper
 
   # Raised when given an invalid file path
-  class InvalidPathError < StandardError; end
+  InvalidPathError = Class.new(StandardError)
 
   before_action :require_non_empty_project, except: [:new, :create]
   before_action :authorize_download_code!

app/models/project.rb

@@ -19,7 +19,7 @@ class Project < ActiveRecord::Base
 
   extend Gitlab::ConfigHelper
 
-  class BoardLimitExceeded < StandardError; end
+  BoardLimitExceeded = Class.new(StandardError)
 
   NUMBER_OF_PERMITTED_BOARDS = 1
   UNKNOWN_IMPORT_URL = 'http://unknown.git'.freeze

app/models/project_wiki.rb

@@ -7,7 +7,7 @@ class ProjectWiki
     'AsciiDoc' => :asciidoc
   }.freeze unless defined?(MARKUPS)
 
-  class CouldNotCreateWikiError < StandardError; end
+  CouldNotCreateWikiError = Class.new(StandardError)
 
   # Returns a string describing what went wrong after
   # an operation fails.

app/serializers/pipeline_serializer.rb

@@ -1,5 +1,5 @@
 class PipelineSerializer < BaseSerializer
-  class InvalidResourceError < StandardError; end
+  InvalidResourceError = Class.new(StandardError)
 
   entity PipelineEntity
 

app/services/commits/change_service.rb

@@ -1,7 +1,7 @@
 module Commits
   class ChangeService < ::BaseService
-    class ValidationError < StandardError; end
+    ValidationError = Class.new(StandardError)
-    class ChangeError < StandardError; end
+    ChangeError = Class.new(StandardError)
 
     def execute
       @start_project = params[:start_project] || @project

app/services/files/base_service.rb

@@ -1,6 +1,6 @@
 module Files
   class BaseService < ::BaseService
-    class ValidationError < StandardError; end
+    ValidationError = Class.new(StandardError)
 
     def execute
       @start_project = params[:start_project] || @project

app/services/files/multi_service.rb

@@ -1,6 +1,6 @@
 module Files
   class MultiService < Files::BaseService
-    class FileChangedError < StandardError; end
+    FileChangedError = Class.new(StandardError)
 
     ACTIONS = %w[create update delete move].freeze
 

app/services/files/update_service.rb

@@ -1,6 +1,6 @@
 module Files
   class UpdateService < Files::BaseService
-    class FileChangedError < StandardError; end
+    FileChangedError = Class.new(StandardError)
 
     def commit
       repository.update_file(current_user, @file_path, @file_content,

app/services/issues/move_service.rb

@@ -1,6 +1,6 @@
 module Issues
   class MoveService < Issues::BaseService
-    class MoveError < StandardError; end
+    MoveError = Class.new(StandardError)
 
     def execute(issue, new_project)
       @old_issue = issue

app/services/merge_requests/resolve_service.rb

@@ -1,7 +1,6 @@
 module MergeRequests
   class ResolveService < MergeRequests::BaseService
-    class MissingFiles < Gitlab::Conflict::ResolutionError
+    MissingFiles = Class.new(Gitlab::Conflict::ResolutionError)
-    end
 
     attr_accessor :conflicts, :rugged, :merge_index, :merge_request
 

app/services/projects/destroy_service.rb

@@ -2,7 +2,7 @@ module Projects
   class DestroyService < BaseService
     include Gitlab::ShellAdapter
 
-    class DestroyError < StandardError; end
+    DestroyError = Class.new(StandardError)
 
     DELETED_FLAG = '+deleted'.freeze
 

app/services/projects/import_service.rb

@@ -2,7 +2,7 @@ module Projects
   class ImportService < BaseService
     include Gitlab::ShellAdapter
 
-    class Error < StandardError; end
+    Error = Class.new(StandardError)
 
     def execute
       add_repository_to_project unless project.gitlab_project_import?

app/services/projects/transfer_service.rb

@@ -9,7 +9,7 @@
 module Projects
   class TransferService < BaseService
     include Gitlab::ShellAdapter
-    class TransferError < StandardError; end
+    TransferError = Class.new(StandardError)
 
     def execute(new_namespace)
       if allowed_transfer?(current_user, project, new_namespace)

lib/api/api_guard.rb

@@ -160,13 +160,10 @@ module API
     # Exceptions
     #
 
-    class MissingTokenError < StandardError; end
+    MissingTokenError = Class.new(StandardError)
-
+    TokenNotFoundError = Class.new(StandardError)
-    class TokenNotFoundError < StandardError; end
+    ExpiredError = Class.new(StandardError)
-
+    RevokedError = Class.new(StandardError)
-    class ExpiredError < StandardError; end
-
-    class RevokedError < StandardError; end
 
     class InsufficientScopeError < StandardError
       attr_reader :scopes

lib/bitbucket/error/unauthorized.rb

@@ -1,6 +1,5 @@
 module Bitbucket
   module Error
-    class Unauthorized < StandardError
+    Unauthorized = Class.new(StandardError)
-    end
   end
 end

lib/ci/gitlab_ci_yaml_processor.rb

@@ -1,6 +1,6 @@
 module Ci
   class GitlabCiYamlProcessor
-    class ValidationError < StandardError; end
+    ValidationError = Class.new(StandardError)
 
     include Gitlab::Ci::Config::Entry::LegacyValidationHelpers
 

lib/extracts_path.rb

@@ -2,7 +2,7 @@
 # file path string when combined in a request parameter
 module ExtractsPath
   # Raised when given an invalid file path
-  class InvalidPathError < StandardError; end
+  InvalidPathError = Class.new(StandardError)
 
   # Given a string containing both a Git tree-ish, such as a branch or tag, and
   # a filesystem path joined by forward slashes, attempts to separate the two.

lib/gitlab/access.rb

@@ -5,7 +5,7 @@
 #
 module Gitlab
   module Access
-    class AccessDeniedError < StandardError; end
+    AccessDeniedError = Class.new(StandardError)
 
     NO_ACCESS = 0
     GUEST     = 10

lib/gitlab/auth.rb

@@ -1,6 +1,6 @@
 module Gitlab
   module Auth
-    class MissingPersonalTokenError < StandardError; end
+    MissingPersonalTokenError = Class.new(StandardError)
 
     SCOPES = [:api, :read_user].freeze
     DEFAULT_SCOPES = [:api].freeze

lib/gitlab/ci/build/artifacts/metadata.rb

@@ -6,7 +6,7 @@ module Gitlab
     module Build
       module Artifacts
         class Metadata
-          class ParserError < StandardError; end
+          ParserError = Class.new(StandardError)
 
           VERSION_PATTERN = /^[\w\s]+(\d+\.\d+\.\d+)/
           INVALID_PATH_PATTERN = %r{(^\.?\.?/)|(/\.?\.?/)}

lib/gitlab/ci/config/entry/factory.rb

@@ -6,7 +6,7 @@ module Gitlab
         # Factory class responsible for fabricating entry objects.
         #
         class Factory
-          class InvalidFactory < StandardError; end
+          InvalidFactory = Class.new(StandardError)
 
           def initialize(entry)
             @entry = entry

lib/gitlab/ci/config/entry/node.rb

@@ -6,7 +6,7 @@ module Gitlab
         # Base abstract class for each configuration entry node.
         #
         class Node
-          class InvalidError < StandardError; end
+          InvalidError = Class.new(StandardError)
 
           attr_reader :config, :metadata
           attr_accessor :key, :parent, :description

lib/gitlab/ci/config/loader.rb

@@ -2,7 +2,7 @@ module Gitlab
   module Ci
     class Config
       class Loader
-        class FormatError < StandardError; end
+        FormatError = Class.new(StandardError)
 
         def initialize(config)
           @config = YAML.safe_load(config, [Symbol], [], true)

lib/gitlab/conflict/file.rb

@@ -4,8 +4,7 @@ module Gitlab
       include Gitlab::Routing.url_helpers
       include IconsHelper
 
-      class MissingResolution < ResolutionError
+      MissingResolution = Class.new(ResolutionError)
-      end
 
       CONTEXT_LINES = 3
 

lib/gitlab/conflict/file_collection.rb

@@ -1,8 +1,7 @@
 module Gitlab
   module Conflict
     class FileCollection
-      class ConflictSideMissing < StandardError
+      ConflictSideMissing = Class.new(StandardError)
-      end
 
       attr_reader :merge_request, :our_commit, :their_commit
 

lib/gitlab/conflict/parser.rb

@@ -1,25 +1,15 @@
 module Gitlab
   module Conflict
     class Parser
-      class UnresolvableError < StandardError
+      UnresolvableError = Class.new(StandardError)
-      end
+      UnmergeableFile = Class.new(UnresolvableError)
-
+      UnsupportedEncoding = Class.new(UnresolvableError)
-      class UnmergeableFile < UnresolvableError
-      end
-
-      class UnsupportedEncoding < UnresolvableError
-      end
 
       # Recoverable errors - the conflict can be resolved in an editor, but not with
       # sections.
-      class ParserError < StandardError
+      ParserError = Class.new(StandardError)
-      end
+      UnexpectedDelimiter = Class.new(ParserError)
-
+      MissingEndDelimiter = Class.new(ParserError)
-      class UnexpectedDelimiter < ParserError
-      end
-
-      class MissingEndDelimiter < ParserError
-      end
 
       def parse(text, our_path:, their_path:, parent_file: nil)
         raise UnmergeableFile if text.blank? # Typically a binary file

lib/gitlab/conflict/resolution_error.rb

@@ -1,6 +1,5 @@
 module Gitlab
   module Conflict
-    class ResolutionError < StandardError
+    ResolutionError = Class.new(StandardError)
-    end
   end
 end

lib/gitlab/email/receiver.rb

@@ -4,19 +4,19 @@ require_dependency 'gitlab/email/handler'
 # Inspired in great part by Discourse's Email::Receiver
 module Gitlab
   module Email
-    class ProcessingError < StandardError; end
+    ProcessingError = Class.new(StandardError)
-    class EmailUnparsableError < ProcessingError; end
+    EmailUnparsableError = Class.new(ProcessingError)
-    class SentNotificationNotFoundError < ProcessingError; end
+    SentNotificationNotFoundError = Class.new(ProcessingError)
-    class ProjectNotFound < ProcessingError; end
+    ProjectNotFound = Class.new(ProcessingError)
-    class EmptyEmailError < ProcessingError; end
+    EmptyEmailError = Class.new(ProcessingError)
-    class AutoGeneratedEmailError < ProcessingError; end
+    AutoGeneratedEmailError = Class.new(ProcessingError)
-    class UserNotFoundError < ProcessingError; end
+    UserNotFoundError = Class.new(ProcessingError)
-    class UserBlockedError < ProcessingError; end
+    UserBlockedError = Class.new(ProcessingError)
-    class UserNotAuthorizedError < ProcessingError; end
+    UserNotAuthorizedError = Class.new(ProcessingError)
-    class NoteableNotFoundError < ProcessingError; end
+    NoteableNotFoundError = Class.new(ProcessingError)
-    class InvalidNoteError < ProcessingError; end
+    InvalidNoteError = Class.new(ProcessingError)
-    class InvalidIssueError < ProcessingError; end
+    InvalidIssueError = Class.new(ProcessingError)
-    class UnknownIncomingEmail < ProcessingError; end
+    UnknownIncomingEmail = Class.new(ProcessingError)
 
     class Receiver
       def initialize(raw)

lib/gitlab/git/diff.rb

@@ -2,7 +2,7 @@
 module Gitlab
   module Git
     class Diff
-      class TimeoutError < StandardError; end
+      TimeoutError = Class.new(StandardError)
       include Gitlab::Git::EncodingHelper
 
       # Diff properties

lib/gitlab/git/repository.rb

@@ -10,9 +10,9 @@ module Gitlab
 
       SEARCH_CONTEXT_LINES = 3
 
-      class NoRepository < StandardError; end
+      NoRepository = Class.new(StandardError)
-      class InvalidBlobName < StandardError; end
+      InvalidBlobName = Class.new(StandardError)
-      class InvalidRef < StandardError; end
+      InvalidRef = Class.new(StandardError)
 
       # Full path to repo
       attr_reader :path

lib/gitlab/import_export/error.rb

@@ -1,5 +1,5 @@
 module Gitlab
   module ImportExport
-    class Error < StandardError; end
+    Error = Class.new(StandardError)
   end
 end

lib/gitlab/o_auth/user.rb

@@ -5,7 +5,7 @@
 #
 module Gitlab
   module OAuth
-    class SignupDisabledError < StandardError; end
+    SignupDisabledError = Class.new(StandardError)
 
     class User
       attr_accessor :auth_hash, :gl_user

lib/gitlab/route_map.rb

@@ -1,6 +1,6 @@
 module Gitlab
   class RouteMap
-    class FormatError < StandardError; end
+    FormatError = Class.new(StandardError)
 
     def initialize(data)
       begin

lib/gitlab/serializer/pagination.rb

@@ -1,7 +1,7 @@
 module Gitlab
   module Serializer
     class Pagination
-      class InvalidResourceError < StandardError; end
+      InvalidResourceError = Class.new(StandardError)
       include ::API::Helpers::Pagination
 
       def initialize(request, response)

lib/gitlab/shell.rb

@@ -2,7 +2,7 @@ require 'securerandom'
 
 module Gitlab
   class Shell
-    class Error < StandardError; end
+    Error = Class.new(StandardError)
 
     KeyAdder = Struct.new(:io) do
       def add_key(id, key)

lib/gitlab/template/finders/repo_template_finder.rb

@@ -4,7 +4,7 @@ module Gitlab
     module Finders
       class RepoTemplateFinder < BaseTemplateFinder
         # Raised when file is not found
-        class FileNotFoundError < StandardError; end
+        FileNotFoundError = Class.new(StandardError)
 
         def initialize(project, base_dir, extension, categories = {})
           @categories     = categories

lib/gitlab/update_path_error.rb

@@ -1,3 +1,3 @@
 module Gitlab
-  class UpdatePathError < StandardError; end
+  UpdatePathError = Class.new(StandardError)
 end

lib/mattermost/client.rb

@@ -1,5 +1,5 @@
 module Mattermost
-  class ClientError < Mattermost::Error; end
+  ClientError = Class.new(Mattermost::Error)
 
   class Client
     attr_reader :user

lib/mattermost/error.rb

@@ -1,3 +1,3 @@
 module Mattermost
-  class Error < StandardError; end
+  Error = Class.new(StandardError)
 end

lib/mattermost/session.rb

@@ -5,7 +5,7 @@ module Mattermost
     end
   end
 
-  class ConnectionError < Mattermost::Error; end
+  ConnectionError = Class.new(Mattermost::Error)
 
   # This class' prime objective is to obtain a session token on a Mattermost
   # instance with SSO configured where this GitLab instance is the provider.

rubocop/cop/custom_error_class.rb

@@ -0,0 +1,64 @@
+module RuboCop
+  module Cop
+    # This cop makes sure that custom error classes, when empty, are declared
+    # with Class.new.
+    #
+    # @example
+    #   # bad
+    #   class FooError < StandardError
+    #   end
+    #
+    #   # okish
+    #   class FooError < StandardError; end
+    #
+    #   # good
+    #   FooError = Class.new(StandardError)
+    class CustomErrorClass < RuboCop::Cop::Cop
+      MSG = 'Use `Class.new(SuperClass)` to define an empty custom error class.'.freeze
+
+      def on_class(node)
+        _klass, parent, body = node.children
+
+        return if body
+
+        parent_klass = class_name_from_node(parent)
+
+        return unless parent_klass && parent_klass.to_s.end_with?('Error')
+
+        add_offense(node, :expression)
+      end
+
+      def autocorrect(node)
+        klass, parent, _body = node.children
+        replacement = "#{class_name_from_node(klass)} = Class.new(#{class_name_from_node(parent)})"
+
+        lambda do |corrector|
+          corrector.replace(node.source_range, replacement)
+        end
+      end
+
+      private
+
+      # The nested constant `Foo::Bar::Baz` looks like:
+      #
+      #   s(:const,
+      #     s(:const,
+      #       s(:const, nil, :Foo), :Bar), :Baz)
+      #
+      # So recurse through that to get the name as written in the source.
+      #
+      def class_name_from_node(node, suffix = nil)
+        return unless node&.type == :const
+
+        name = node.children[1].to_s
+        name = "#{name}::#{suffix}" if suffix
+
+        if node.children[0]
+          class_name_from_node(node.children[0], name)
+        else
+          name
+        end
+      end
+    end
+  end
+end

rubocop/rubocop.rb

@@ -1,3 +1,4 @@
+require_relative 'cop/custom_error_class'
 require_relative 'cop/gem_fetcher'
 require_relative 'cop/migration/add_column'
 require_relative 'cop/migration/add_column_with_default'

spec/rubocop/cop/custom_error_class_spec.rb

@@ -0,0 +1,111 @@
+require 'spec_helper'
+
+require 'rubocop'
+require 'rubocop/rspec/support'
+
+require_relative '../../../rubocop/cop/custom_error_class'
+
+describe RuboCop::Cop::CustomErrorClass do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  context 'when a class has a body' do
+    it 'does nothing' do
+      inspect_source(cop, 'class CustomError < StandardError; def foo; end; end')
+
+      expect(cop.offenses).to be_empty
+    end
+  end
+
+  context 'when a class has no explicit superclass' do
+    it 'does nothing' do
+      inspect_source(cop, 'class CustomError; end')
+
+      expect(cop.offenses).to be_empty
+    end
+  end
+
+  context 'when a class has a superclass that does not end in Error' do
+    it 'does nothing' do
+      inspect_source(cop, 'class CustomError < BasicObject; end')
+
+      expect(cop.offenses).to be_empty
+    end
+  end
+
+  context 'when a class is empty and inherits from a class ending in Error' do
+    context 'when the class is on a single line' do
+      let(:source) do
+        <<-SOURCE
+          module Foo
+            class CustomError < Bar::Baz::BaseError; end
+          end
+        SOURCE
+      end
+
+      let(:expected) do
+        <<-EXPECTED
+          module Foo
+            CustomError = Class.new(Bar::Baz::BaseError)
+          end
+        EXPECTED
+      end
+
+      it 'registers an offense' do
+        expected_highlights = source.split("\n")[1].strip
+
+        inspect_source(cop, source)
+
+        aggregate_failures do
+          expect(cop.offenses.size).to eq(1)
+          expect(cop.offenses.map(&:line)).to eq([2])
+          expect(cop.highlights).to contain_exactly(expected_highlights)
+        end
+      end
+
+      it 'autocorrects to the right version' do
+        autocorrected = autocorrect_source(cop, source, 'foo/custom_error.rb')
+
+        expect(autocorrected).to eq(expected)
+      end
+    end
+
+    context 'when the class is on multiple lines' do
+      let(:source) do
+        <<-SOURCE
+          module Foo
+            class CustomError < Bar::Baz::BaseError
+            end
+          end
+        SOURCE
+      end
+
+      let(:expected) do
+        <<-EXPECTED
+          module Foo
+            CustomError = Class.new(Bar::Baz::BaseError)
+          end
+        EXPECTED
+      end
+
+      it 'registers an offense' do
+        expected_highlights = source.split("\n")[1..2].join("\n").strip
+
+        inspect_source(cop, source)
+
+        aggregate_failures do
+          expect(cop.offenses.size).to eq(1)
+          expect(cop.offenses.map(&:line)).to eq([2])
+          expect(cop.highlights).to contain_exactly(expected_highlights)
+        end
+      end
+
+      it 'autocorrects to the right version' do
+        autocorrected = autocorrect_source(cop, source, 'foo/custom_error.rb')
+
+        expect(autocorrected).to eq(expected)
+      end
+    end
+  end
+end