Commit 5028c305 authored by Nick Thomas's avatar Nick Thomas

Ban catch/throw with rubocop

We should use exceptions for exceptional situations, and things less
like goto for control flow.
parent 09f08640
...@@ -344,3 +344,6 @@ Style/MultilineWhenThen: ...@@ -344,3 +344,6 @@ Style/MultilineWhenThen:
Style/FloatDivision: Style/FloatDivision:
Enabled: false Enabled: false
Cop/BanCatchThrow:
Enabled: true
...@@ -97,6 +97,6 @@ class List < ApplicationRecord ...@@ -97,6 +97,6 @@ class List < ApplicationRecord
private private
def can_be_destroyed def can_be_destroyed
throw(:abort) unless destroyable? throw(:abort) unless destroyable? # rubocop:disable Cop/BanCatchThrow
end end
end end
...@@ -2430,7 +2430,7 @@ class Project < ApplicationRecord ...@@ -2430,7 +2430,7 @@ class Project < ApplicationRecord
if repository_storage.blank? || repository_with_same_path_already_exists? if repository_storage.blank? || repository_with_same_path_already_exists?
errors.add(:base, _('There is already a repository with that name on disk')) errors.add(:base, _('There is already a repository with that name on disk'))
throw :abort throw :abort # rubocop:disable Cop/BanCatchThrow
end end
end end
......
...@@ -22,6 +22,7 @@ module Metrics ...@@ -22,6 +22,7 @@ module Metrics
end end
end end
# rubocop:disable Cop/BanCatchThrow
def execute def execute
catch(:error) do catch(:error) do
throw(:error, error(_(%q(You are not allowed to push into this branch. Create another branch or open a merge request.)), :forbidden)) unless push_authorized? throw(:error, error(_(%q(You are not allowed to push into this branch. Create another branch or open a merge request.)), :forbidden)) unless push_authorized?
...@@ -33,6 +34,7 @@ module Metrics ...@@ -33,6 +34,7 @@ module Metrics
success(result.merge(http_status: :created, dashboard: dashboard_details)) success(result.merge(http_status: :created, dashboard: dashboard_details))
end end
end end
# rubocop:enable Cop/BanCatchThrow
private private
...@@ -60,6 +62,7 @@ module Metrics ...@@ -60,6 +62,7 @@ module Metrics
Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(branch) Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(branch)
end end
# rubocop:disable Cop/BanCatchThrow
def dashboard_template def dashboard_template
@dashboard_template ||= begin @dashboard_template ||= begin
throw(:error, error(_('Not found.'), :not_found)) unless self.class.allowed_dashboard_templates.include?(params[:dashboard]) throw(:error, error(_('Not found.'), :not_found)) unless self.class.allowed_dashboard_templates.include?(params[:dashboard])
...@@ -67,7 +70,9 @@ module Metrics ...@@ -67,7 +70,9 @@ module Metrics
params[:dashboard] params[:dashboard]
end end
end end
# rubocop:enable Cop/BanCatchThrow
# rubocop:disable Cop/BanCatchThrow
def branch def branch
@branch ||= begin @branch ||= begin
throw(:error, error(_('There was an error creating the dashboard, branch name is invalid.'), :bad_request)) unless valid_branch_name? throw(:error, error(_('There was an error creating the dashboard, branch name is invalid.'), :bad_request)) unless valid_branch_name?
...@@ -76,6 +81,7 @@ module Metrics ...@@ -76,6 +81,7 @@ module Metrics
params[:branch] params[:branch]
end end
end end
# rubocop:enable Cop/BanCatchThrow
def new_or_default_branch? def new_or_default_branch?
!repository.branch_exists?(params[:branch]) || project.default_branch == params[:branch] !repository.branch_exists?(params[:branch]) || project.default_branch == params[:branch]
...@@ -89,6 +95,7 @@ module Metrics ...@@ -89,6 +95,7 @@ module Metrics
@new_dashboard_path ||= File.join(USER_DASHBOARDS_DIR, file_name) @new_dashboard_path ||= File.join(USER_DASHBOARDS_DIR, file_name)
end end
# rubocop:disable Cop/BanCatchThrow
def file_name def file_name
@file_name ||= begin @file_name ||= begin
throw(:error, error(_('The file name should have a .yml extension'), :bad_request)) unless target_file_type_valid? throw(:error, error(_('The file name should have a .yml extension'), :bad_request)) unless target_file_type_valid?
...@@ -96,6 +103,7 @@ module Metrics ...@@ -96,6 +103,7 @@ module Metrics
File.basename(params[:file_name]) File.basename(params[:file_name])
end end
end end
# rubocop:enable Cop/BanCatchThrow
def target_file_type_valid? def target_file_type_valid?
File.extname(params[:file_name]) == ALLOWED_FILE_TYPE File.extname(params[:file_name]) == ALLOWED_FILE_TYPE
......
...@@ -7,6 +7,7 @@ module Metrics ...@@ -7,6 +7,7 @@ module Metrics
ALLOWED_FILE_TYPE = '.yml' ALLOWED_FILE_TYPE = '.yml'
USER_DASHBOARDS_DIR = ::Metrics::Dashboard::ProjectDashboardService::DASHBOARD_ROOT USER_DASHBOARDS_DIR = ::Metrics::Dashboard::ProjectDashboardService::DASHBOARD_ROOT
# rubocop:disable Cop/BanCatchThrow
def execute def execute
catch(:error) do catch(:error) do
throw(:error, error(_(%q(You are not allowed to push into this branch. Create another branch or open a merge request.)), :forbidden)) unless push_authorized? throw(:error, error(_(%q(You are not allowed to push into this branch. Create another branch or open a merge request.)), :forbidden)) unless push_authorized?
...@@ -17,6 +18,7 @@ module Metrics ...@@ -17,6 +18,7 @@ module Metrics
success(result.merge(http_status: :created, dashboard: dashboard_details)) success(result.merge(http_status: :created, dashboard: dashboard_details))
end end
end end
# rubocop:enable Cop/BanCatchThrow
private private
...@@ -44,6 +46,7 @@ module Metrics ...@@ -44,6 +46,7 @@ module Metrics
Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(branch) Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(branch)
end end
# rubocop:disable Cop/BanCatchThrow
def branch def branch
@branch ||= begin @branch ||= begin
throw(:error, error(_('There was an error updating the dashboard, branch name is invalid.'), :bad_request)) unless valid_branch_name? throw(:error, error(_('There was an error updating the dashboard, branch name is invalid.'), :bad_request)) unless valid_branch_name?
...@@ -52,6 +55,7 @@ module Metrics ...@@ -52,6 +55,7 @@ module Metrics
params[:branch] params[:branch]
end end
end end
# rubocop:enable Cop/BanCatchThrow
def new_or_default_branch? def new_or_default_branch?
!repository.branch_exists?(params[:branch]) || project.default_branch == params[:branch] !repository.branch_exists?(params[:branch]) || project.default_branch == params[:branch]
...@@ -69,6 +73,7 @@ module Metrics ...@@ -69,6 +73,7 @@ module Metrics
File.extname(params[:file_name]) == ALLOWED_FILE_TYPE File.extname(params[:file_name]) == ALLOWED_FILE_TYPE
end end
# rubocop:disable Cop/BanCatchThrow
def file_name def file_name
@file_name ||= begin @file_name ||= begin
throw(:error, error(_('The file name should have a .yml extension'), :bad_request)) unless target_file_type_valid? throw(:error, error(_('The file name should have a .yml extension'), :bad_request)) unless target_file_type_valid?
...@@ -76,6 +81,7 @@ module Metrics ...@@ -76,6 +81,7 @@ module Metrics
File.basename(CGI.unescape(params[:file_name])) File.basename(CGI.unescape(params[:file_name]))
end end
end end
# rubocop:enable Cop/BanCatchThrow
def update_dashboard_content def update_dashboard_content
::PerformanceMonitoring::PrometheusDashboard.from_json(params[:file_content]).to_yaml ::PerformanceMonitoring::PrometheusDashboard.from_json(params[:file_content]).to_yaml
......
...@@ -18,7 +18,7 @@ module TestHooks ...@@ -18,7 +18,7 @@ module TestHooks
return error('Testing not available for this hook') return error('Testing not available for this hook')
end end
error_message = catch(:validation_error) do error_message = catch(:validation_error) do # rubocop:disable Cop/BanCatchThrow
sample_data = self.__send__(trigger_data_method) # rubocop:disable GitlabSecurity/PublicSend sample_data = self.__send__(trigger_data_method) # rubocop:disable GitlabSecurity/PublicSend
return hook.execute(sample_data, trigger_key) # rubocop:disable Cop/AvoidReturnFromBlocks return hook.execute(sample_data, trigger_key) # rubocop:disable Cop/AvoidReturnFromBlocks
......
...@@ -11,7 +11,7 @@ module TestHooks ...@@ -11,7 +11,7 @@ module TestHooks
private private
def push_events_data def push_events_data
throw(:validation_error, s_('TestHooks|Ensure the project has at least one commit.')) if project.empty_repo? throw(:validation_error, s_('TestHooks|Ensure the project has at least one commit.')) if project.empty_repo? # rubocop:disable Cop/BanCatchThrow
Gitlab::DataBuilder::Push.build_sample(project, current_user) Gitlab::DataBuilder::Push.build_sample(project, current_user)
end end
...@@ -20,14 +20,14 @@ module TestHooks ...@@ -20,14 +20,14 @@ module TestHooks
def note_events_data def note_events_data
note = project.notes.first note = project.notes.first
throw(:validation_error, s_('TestHooks|Ensure the project has notes.')) unless note.present? throw(:validation_error, s_('TestHooks|Ensure the project has notes.')) unless note.present? # rubocop:disable Cop/BanCatchThrow
Gitlab::DataBuilder::Note.build(note, current_user) Gitlab::DataBuilder::Note.build(note, current_user)
end end
def issues_events_data def issues_events_data
issue = project.issues.first issue = project.issues.first
throw(:validation_error, s_('TestHooks|Ensure the project has issues.')) unless issue.present? throw(:validation_error, s_('TestHooks|Ensure the project has issues.')) unless issue.present? # rubocop:disable Cop/BanCatchThrow
issue.to_hook_data(current_user) issue.to_hook_data(current_user)
end end
...@@ -36,21 +36,21 @@ module TestHooks ...@@ -36,21 +36,21 @@ module TestHooks
def merge_requests_events_data def merge_requests_events_data
merge_request = project.merge_requests.first merge_request = project.merge_requests.first
throw(:validation_error, s_('TestHooks|Ensure the project has merge requests.')) unless merge_request.present? throw(:validation_error, s_('TestHooks|Ensure the project has merge requests.')) unless merge_request.present? # rubocop:disable Cop/BanCatchThrow
merge_request.to_hook_data(current_user) merge_request.to_hook_data(current_user)
end end
def job_events_data def job_events_data
build = project.builds.first build = project.builds.first
throw(:validation_error, s_('TestHooks|Ensure the project has CI jobs.')) unless build.present? throw(:validation_error, s_('TestHooks|Ensure the project has CI jobs.')) unless build.present? # rubocop:disable Cop/BanCatchThrow
Gitlab::DataBuilder::Build.build(build) Gitlab::DataBuilder::Build.build(build)
end end
def pipeline_events_data def pipeline_events_data
pipeline = project.ci_pipelines.first pipeline = project.ci_pipelines.first
throw(:validation_error, s_('TestHooks|Ensure the project has CI pipelines.')) unless pipeline.present? throw(:validation_error, s_('TestHooks|Ensure the project has CI pipelines.')) unless pipeline.present? # rubocop:disable Cop/BanCatchThrow
Gitlab::DataBuilder::Pipeline.build(pipeline) Gitlab::DataBuilder::Pipeline.build(pipeline)
end end
...@@ -58,7 +58,7 @@ module TestHooks ...@@ -58,7 +58,7 @@ module TestHooks
def wiki_page_events_data def wiki_page_events_data
page = project.wiki.list_pages(limit: 1).first page = project.wiki.list_pages(limit: 1).first
if !project.wiki_enabled? || page.blank? if !project.wiki_enabled? || page.blank?
throw(:validation_error, s_('TestHooks|Ensure the wiki is enabled and has pages.')) throw(:validation_error, s_('TestHooks|Ensure the wiki is enabled and has pages.')) # rubocop:disable Cop/BanCatchThrow
end end
Gitlab::DataBuilder::WikiPage.build(page, current_user, 'create') Gitlab::DataBuilder::WikiPage.build(page, current_user, 'create')
......
...@@ -18,7 +18,7 @@ module TestHooks ...@@ -18,7 +18,7 @@ module TestHooks
def merge_requests_events_data def merge_requests_events_data
merge_request = MergeRequest.of_projects(current_user.projects.select(:id)).first merge_request = MergeRequest.of_projects(current_user.projects.select(:id)).first
throw(:validation_error, s_('TestHooks|Ensure one of your projects has merge requests.')) unless merge_request.present? throw(:validation_error, s_('TestHooks|Ensure one of your projects has merge requests.')) unless merge_request.present? # rubocop:disable Cop/BanCatchThrow
merge_request.to_hook_data(current_user) merge_request.to_hook_data(current_user)
end end
......
...@@ -51,7 +51,7 @@ module Gitlab ...@@ -51,7 +51,7 @@ module Gitlab
def call_with_call_stack_profiling(env) def call_with_call_stack_profiling(env)
ret = nil ret = nil
report = RubyProf::Profile.profile do report = RubyProf::Profile.profile do
ret = catch(:warden) do ret = catch(:warden) do # rubocop:disable Cop/BanCatchThrow
@app.call(env) @app.call(env)
end end
end end
...@@ -67,7 +67,7 @@ module Gitlab ...@@ -67,7 +67,7 @@ module Gitlab
def call_with_memory_profiling(env) def call_with_memory_profiling(env)
ret = nil ret = nil
report = MemoryProfiler.report do report = MemoryProfiler.report do
ret = catch(:warden) do ret = catch(:warden) do # rubocop:disable Cop/BanCatchThrow
@app.call(env) @app.call(env)
end end
end end
...@@ -99,7 +99,7 @@ module Gitlab ...@@ -99,7 +99,7 @@ module Gitlab
if ret.is_a?(Array) if ret.is_a?(Array)
ret ret
else else
throw(:warden, ret) throw(:warden, ret) # rubocop:disable Cop/BanCatchThrow
end end
end end
end end
......
# frozen_string_literal: true
module RuboCop
module Cop
# Bans the use of 'catch/throw', as exceptions are better for errors and
# they are equivalent to 'goto' for flow control, with all the problems
# that implies.
#
# @example
# # bad
# catch(:error) do
# throw(:error)
# end
#
# # good
# begin
# raise StandardError
# rescue StandardError => err
# # ...
# end
#
class BanCatchThrow < RuboCop::Cop::Cop
MSG = "Do not use catch or throw unless a gem's API demands it."
def on_send(node)
receiver, method_name, _ = *node
return unless receiver.nil? && %i[catch throw].include?(method_name)
add_offense(node, location: :expression)
end
end
end
end
...@@ -26,7 +26,7 @@ module RuboCop ...@@ -26,7 +26,7 @@ module RuboCop
elsif node.descendants.first.method_name == :keys elsif node.descendants.first.method_name == :keys
'.each_key' '.each_key'
else else
throw("Expect '.values.first' or '.keys.first', but get #{node.descendants.first.method_name}.first") throw("Expect '.values.first' or '.keys.first', but get #{node.descendants.first.method_name}.first") # rubocop:disable Cop/BanCatchThrow
end end
upto_including_keys_or_values = node.descendants.first.source_range upto_including_keys_or_values = node.descendants.first.source_range
......
# frozen_string_literal: true
require 'spec_helper'
require 'rubocop'
require 'rubocop/rspec/support'
require_relative '../../../rubocop/cop/ban_catch_throw'
describe RuboCop::Cop::BanCatchThrow do
include CopHelper
subject(:cop) { described_class.new }
it 'registers an offense when `catch` or `throw` are used' do
inspect_source("catch(:foo) {\n throw(:foo)\n}")
aggregate_failures do
expect(cop.offenses.size).to eq(2)
expect(cop.offenses.map(&:line)).to eq([1, 2])
expect(cop.highlights).to eq(['catch(:foo)', 'throw(:foo)'])
end
end
it 'does not register an offense for a method called catch or throw' do
inspect_source("foo.catch(:foo) {\n foo.throw(:foo)\n}")
expect(cop.offenses).to be_empty
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment