From 250c53477451f603834a879c09b63c3e13ff8e34 Mon Sep 17 00:00:00 2001 From: Joe Woodward Date: Tue, 20 Feb 2024 18:05:31 +0000 Subject: [PATCH] Refactor BranchRules::BaseService to handle common pattern In the EE version there are custom branch rules which need to be treated differently. The branch rule service classes have a common pattern which detects which type of branch rule is currently being passed. This change extracts that logic into the base service and then calls 4 methods 1. `authorized?` -> handle authorization for the service 2. `execute_on_branch_rule` -> logic for `Projects::BranchRule` 3. `execute_on_all_branches_rule` -> logic for `Projects::AllBranchesRule` 4. `execute_on_all_protected_branches_rule` -> logic for `Projects::AllProtectedBranchesRule` This should simplify the logic for each child service. --- app/services/branch_rules/base_service.rb | 30 +++++++-- app/services/branch_rules/destroy_service.rb | 14 +--- app/services/branch_rules/update_service.rb | 36 +++------- .../services/ee/branch_rules/base_service.rb | 22 +++++++ .../ee/branch_rules/destroy_service.rb | 23 +------ .../ee/branch_rules/update_service.rb | 19 ++---- .../ee/branch_rules/base_service_spec.rb | 66 +++++++++++++++++++ .../branch_rules/destroy_service_spec.rb | 0 .../branch_rules/update_service_spec.rb | 0 .../branch_rules/base_service_spec.rb | 54 +++++++++++++++ .../branch_rules/update_service_spec.rb | 61 ++++++++++------- 11 files changed, 220 insertions(+), 105 deletions(-) create mode 100644 ee/spec/services/ee/branch_rules/base_service_spec.rb rename ee/spec/services/{ => ee}/branch_rules/destroy_service_spec.rb (100%) rename ee/spec/services/{ => ee}/branch_rules/update_service_spec.rb (100%) create mode 100644 spec/services/branch_rules/base_service_spec.rb diff --git a/app/services/branch_rules/base_service.rb b/app/services/branch_rules/base_service.rb index 5ed8ea3a23bec6..99a6419531461f 100644 --- a/app/services/branch_rules/base_service.rb +++ b/app/services/branch_rules/base_service.rb @@ -4,19 +4,39 @@ module BranchRules class BaseService include Gitlab::Allowable - attr_reader :project, :branch_rule, :current_user, :params + PERMITTED_PARAMS = [].freeze + MISSING_METHOD_ERROR = Class.new(StandardError) + + attr_reader :branch_rule, :current_user, :params + + delegate :project, to: :branch_rule, allow_nil: true def initialize(branch_rule, user = nil, params = {}) @branch_rule = branch_rule - @project = branch_rule.project @current_user = user - @params = params.slice(*permitted_params) + @params = params.slice(*self.class::PERMITTED_PARAMS) + end + + def execute(skip_authorization: false) + raise Gitlab::Access::AccessDeniedError unless skip_authorization || authorized? + + return execute_on_branch_rule if branch_rule.instance_of?(Projects::BranchRule) + + ServiceResponse.error(message: 'Unknown branch rule type.') end private - def permitted_params - [] + def execute_on_branch_rule + missing_method_error('execute_on_branch_rule') + end + + def authorized? + missing_method_error('authorized?') + end + + def missing_method_error(method_name) + raise MISSING_METHOD_ERROR, "Please define an `#{method_name}` method in #{self.class.name}" end end end diff --git a/app/services/branch_rules/destroy_service.rb b/app/services/branch_rules/destroy_service.rb index 8ff1fd43e2d1e4..8e639e49c9235b 100644 --- a/app/services/branch_rules/destroy_service.rb +++ b/app/services/branch_rules/destroy_service.rb @@ -2,23 +2,13 @@ module BranchRules class DestroyService < BaseService - def execute - raise Gitlab::Access::AccessDeniedError unless can_destroy_branch_rule? - - return destroy_protected_branch if branch_rule.instance_of?(Projects::BranchRule) - - yield if block_given? - - ServiceResponse.error(message: 'Unknown branch rule type.') - end - private - def can_destroy_branch_rule? + def authorized? can?(current_user, :destroy_protected_branch, branch_rule) end - def destroy_protected_branch + def execute_on_branch_rule service = ProtectedBranches::DestroyService.new(project, current_user) return ServiceResponse.success if service.execute(branch_rule.protected_branch) diff --git a/app/services/branch_rules/update_service.rb b/app/services/branch_rules/update_service.rb index 6b79376663e91a..53162056c54378 100644 --- a/app/services/branch_rules/update_service.rb +++ b/app/services/branch_rules/update_service.rb @@ -4,40 +4,20 @@ module BranchRules class UpdateService < BaseService PERMITTED_PARAMS = %i[name].freeze - attr_reader :skip_authorization - - def execute(skip_authorization: false) - @skip_authorization = skip_authorization - - raise Gitlab::Access::AccessDeniedError unless can_update_branch_rule? - - return update_protected_branch if branch_rule.instance_of?(Projects::BranchRule) - - yield if block_given? - - ServiceResponse.error(message: 'Unknown branch rule type.') - end - private - def permitted_params - PERMITTED_PARAMS + def authorized? + can?(current_user, :update_branch_rule, branch_rule) end - def can_update_branch_rule? - return true if skip_authorization - - can?(current_user, :update_protected_branch, branch_rule) - end - - def update_protected_branch - service = ProtectedBranches::UpdateService.new(project, current_user, params) - - service_response = service.execute(branch_rule.protected_branch, skip_authorization: skip_authorization) + def execute_on_branch_rule + protected_branch = ProtectedBranches::UpdateService + .new(project, current_user, params) + .execute(branch_rule.protected_branch, skip_authorization: true) - return ServiceResponse.success unless service_response.errors.any? + return ServiceResponse.success unless protected_branch.errors.any? - ServiceResponse.error(message: service_response.errors.full_messages) + ServiceResponse.error(message: protected_branch.errors.full_messages) end end end diff --git a/ee/app/services/ee/branch_rules/base_service.rb b/ee/app/services/ee/branch_rules/base_service.rb index beb4ad6a5e71af..064542503fed67 100644 --- a/ee/app/services/ee/branch_rules/base_service.rb +++ b/ee/app/services/ee/branch_rules/base_service.rb @@ -4,12 +4,34 @@ module EE module BranchRules module BaseService extend ActiveSupport::Concern + extend ::Gitlab::Utils::Override prepended do extend Forwardable def_delegators(:branch_rule, :approval_project_rules, :external_status_checks) end + + override :execute + def execute(skip_authorization: false) + raise ::Gitlab::Access::AccessDeniedError unless skip_authorization || authorized? + + case branch_rule + when ::Projects::AllBranchesRule then return execute_on_all_branches_rule + when ::Projects::AllProtectedBranchesRule then return execute_on_all_protected_branches_rule + when ::Projects::BranchRule then return execute_on_branch_rule + end + + ServiceResponse.error(message: 'Unknown branch rule type.') + end + + def execute_on_all_branches_rule + missing_method_error('execute_on_all_branches_rule') + end + + def execute_on_all_protected_branches_rule + missing_method_error('execute_on_all_protected_branches_rule') + end end end end diff --git a/ee/app/services/ee/branch_rules/destroy_service.rb b/ee/app/services/ee/branch_rules/destroy_service.rb index 830c9a843e18ab..4cb89eb8ce4fb0 100644 --- a/ee/app/services/ee/branch_rules/destroy_service.rb +++ b/ee/app/services/ee/branch_rules/destroy_service.rb @@ -3,28 +3,9 @@ module EE module BranchRules module DestroyService - extend ::Gitlab::Utils::Override - - # rubocop:disable Cop/AvoidReturnFromBlocks -- The overriden `execute` - # method yields and then raises an error. We use `return` here to exit - # the super method. If we used break it would break out of the block and - # continue execution of the super method causing the error to be raised. - override :execute - def execute - super do - case branch_rule - when ::Projects::AllBranchesRule - return destroy_all_branches_rule - when ::Projects::AllProtectedBranchesRule - return destroy_all_protected_branches_rule - end - end - end - # rubocop:enable Cop/AvoidReturnFromBlocks - private - def destroy_all_branches_rule + def execute_on_all_branches_rule response = destroy_approval_project_rules return response if response.error? @@ -32,7 +13,7 @@ def destroy_all_branches_rule destroy_external_status_checks end - def destroy_all_protected_branches_rule + def execute_on_all_protected_branches_rule destroy_approval_project_rules end diff --git a/ee/app/services/ee/branch_rules/update_service.rb b/ee/app/services/ee/branch_rules/update_service.rb index 61fdf9733e04a6..959740db1e3a2f 100644 --- a/ee/app/services/ee/branch_rules/update_service.rb +++ b/ee/app/services/ee/branch_rules/update_service.rb @@ -3,22 +3,13 @@ module EE module BranchRules module UpdateService - extend ::Gitlab::Utils::Override + def execute_on_all_branches_rule + ServiceResponse.error(message: 'All branch rules cannot be updated') + end - # rubocop:disable Cop/AvoidReturnFromBlocks -- see similar - # implementation and explanation in BranchRules::DestroyService - override :execute - def execute(skip_authorization: false) - super do - case branch_rule - when ::Projects::AllBranchesRule - return ServiceResponse.error(message: 'All branch rules cannot be updated') - when ::Projects::AllProtectedBranchesRule - return ServiceResponse.error(message: 'All protected branch rules cannot be updated') - end - end + def execute_on_all_protected_branches_rule + ServiceResponse.error(message: 'All protected branch rules cannot be updated') end - # rubocop:enable Cop/AvoidReturnFromBlocks end end end diff --git a/ee/spec/services/ee/branch_rules/base_service_spec.rb b/ee/spec/services/ee/branch_rules/base_service_spec.rb new file mode 100644 index 00000000000000..cca557c3a7ed4d --- /dev/null +++ b/ee/spec/services/ee/branch_rules/base_service_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe BranchRules::BaseService, feature_category: :source_code_management do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + let_it_be(:protected_branch) { create(:protected_branch) } + + describe '#execute' do + subject(:execute) { described_class.new(branch_rule, user).execute(skip_authorization: skip_authorization) } + + let(:branch_rule) { Projects::BranchRule.new(project, protected_branch) } + + shared_examples 'missing_method_error' do |method_name| + it 'raises a missing method error' do + expect { execute }.to raise_error( + described_class::MISSING_METHOD_ERROR, + "Please define an `#{method_name}` method in #{described_class.name}" + ) + end + end + + context 'with skip_authorization: false' do + let(:skip_authorization) { false } + + it_behaves_like 'missing_method_error', 'authorized?' + end + + context 'with skip_authorization: true' do + let(:skip_authorization) { true } + + context 'when branch_rule is an instance of Projects::BranchRule' do + it_behaves_like 'missing_method_error', 'execute_on_branch_rule' + end + + context 'when branch_rule is an instance of Projects::AllBranchesRule' do + let(:branch_rule) { Projects::AllBranchesRule.new(project) } + + it_behaves_like 'missing_method_error', 'execute_on_all_branches_rule' + end + + context 'when branch_rule is an instance of Projects::AllProtectedBranchesRule' do + let(:branch_rule) { Projects::AllProtectedBranchesRule.new(project) } + + it_behaves_like 'missing_method_error', 'execute_on_all_protected_branches_rule' + end + + context 'when branch_rule is not an instance of Projects::BranchRule' do + let(:branch_rule) { Project.new } + + it 'returns an unknown branch rule type error' do + expect(execute.message).to eq('Unknown branch rule type.') + end + end + + context 'when branch_rule is nil' do + let(:branch_rule) { nil } + + it 'returns an unknown branch rule type error' do + expect(execute.message).to eq('Unknown branch rule type.') + end + end + end + end +end diff --git a/ee/spec/services/branch_rules/destroy_service_spec.rb b/ee/spec/services/ee/branch_rules/destroy_service_spec.rb similarity index 100% rename from ee/spec/services/branch_rules/destroy_service_spec.rb rename to ee/spec/services/ee/branch_rules/destroy_service_spec.rb diff --git a/ee/spec/services/branch_rules/update_service_spec.rb b/ee/spec/services/ee/branch_rules/update_service_spec.rb similarity index 100% rename from ee/spec/services/branch_rules/update_service_spec.rb rename to ee/spec/services/ee/branch_rules/update_service_spec.rb diff --git a/spec/services/branch_rules/base_service_spec.rb b/spec/services/branch_rules/base_service_spec.rb new file mode 100644 index 00000000000000..fcd6dede1debd8 --- /dev/null +++ b/spec/services/branch_rules/base_service_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe BranchRules::BaseService, feature_category: :source_code_management do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } + let_it_be(:protected_branch) { create(:protected_branch) } + + describe '#execute' do + subject(:execute) { described_class.new(branch_rule, user).execute(skip_authorization: skip_authorization) } + + let(:branch_rule) { Projects::BranchRule.new(project, protected_branch) } + + shared_examples 'missing_method_error' do |method_name| + it 'raises a missing method error' do + expect { execute }.to raise_error( + described_class::MISSING_METHOD_ERROR, + "Please define an `#{method_name}` method in #{described_class.name}" + ) + end + end + + context 'with skip_authorization: false' do + let(:skip_authorization) { false } + + it_behaves_like 'missing_method_error', 'authorized?' + end + + context 'with skip_authorization: true' do + let(:skip_authorization) { true } + + context 'when branch_rule is an instance of Projects::BranchRule' do + it_behaves_like 'missing_method_error', 'execute_on_branch_rule' + end + + context 'when branch_rule is not an instance of Projects::BranchRule' do + let(:branch_rule) { Project.new } + + it 'returns an unknown branch rule type error' do + expect(execute.message).to eq('Unknown branch rule type.') + end + end + + context 'when branch_rule is nil' do + let(:branch_rule) { nil } + + it 'returns an unknown branch rule type error' do + expect(execute.message).to eq('Unknown branch rule type.') + end + end + end + end +end diff --git a/spec/services/branch_rules/update_service_spec.rb b/spec/services/branch_rules/update_service_spec.rb index a1cd12f2cb3c68..45547f5cc06be5 100644 --- a/spec/services/branch_rules/update_service_spec.rb +++ b/spec/services/branch_rules/update_service_spec.rb @@ -5,41 +5,54 @@ RSpec.describe BranchRules::UpdateService, feature_category: :source_code_management do let_it_be(:project) { create(:project, :repository) } let_it_be(:user) { create(:user) } - let_it_be(:protected_branch) { create(:protected_branch) } + let_it_be(:protected_branch, reload: true) { create(:protected_branch) } describe '#execute' do let(:branch_rule) { Projects::BranchRule.new(project, protected_branch) } - let(:action_allowed) { true } - let(:update_service) { ProtectedBranches::UpdateService } - let(:update_service_instance) { instance_double(update_service) } + let(:ability_allowed) { true } let(:new_name) { 'new_name' } - let(:errors) { ["Error 1", "Error 2"] } let(:params) { { name: new_name } } + let(:skip_authorization) { false } - subject(:execute) { described_class.new(branch_rule, user, params).execute } + subject(:execute) do + described_class.new(branch_rule, user, params).execute(skip_authorization: skip_authorization) + end before do allow(Ability).to receive(:allowed?).and_return(true) - allow(Ability) - .to receive(:allowed?).with(user, :update_protected_branch, branch_rule) - .and_return(action_allowed) + allow(Ability).to receive(:allowed?) + .with(user, :update_branch_rule, branch_rule) + .and_return(ability_allowed) end context 'when the current_user cannot update the branch rule' do - let(:action_allowed) { false } + let(:ability_allowed) { false } it 'raises an access denied error' do expect { execute }.to raise_error(Gitlab::Access::AccessDeniedError) end + + context 'and skip_authorization is true' do + let(:skip_authorization) { true } + + it 'raises an access denied error' do + expect { execute }.not_to raise_error + end + end end context 'when branch_rule is a Projects::BranchRule' do - it 'updates the ProtectedBranch and returns a success execute' do - expect(execute[:status]).to eq(:success) + let(:update_service) { ProtectedBranches::UpdateService } + let(:update_service_instance) { instance_double(update_service) } + + it 'updates the ProtectedBranch and returns a success response' do + expect(execute).to be_success expect(protected_branch.reload.name).to eq(new_name) end context 'if the update fails' do + let(:errors) { ["Error 1", "Error 2"] } + before do allow(update_service).to receive(:new).and_return(update_service_instance) allow(update_service_instance).to receive(:execute).and_return(protected_branch) @@ -48,9 +61,17 @@ end it 'returns an error' do - response = execute + expect(response = execute).to be_error expect(response[:message]).to eq(errors) - expect(response[:status]).to eq(:error) + end + end + + context 'when unpermitted params are provided' do + let(:params) { { name: new_name, not_permitted: 'not_permitted' } } + + it 'removes them' do + expect(update_service).to receive(:new).with(project, user, { name: new_name }).and_call_original + execute end end end @@ -59,18 +80,8 @@ let(:branch_rule) { protected_branch } it 'returns an error' do - response = execute + expect(response = execute).to be_error expect(response[:message]).to eq('Unknown branch rule type.') - expect(response[:status]).to eq(:error) - end - end - - context 'when unpermitted params are provided' do - let(:params) { { name: new_name, not_permitted: 'not_permitted' } } - - it 'removes them' do - expect(update_service).to receive(:new).with(project, user, { name: new_name }).and_call_original - execute end end end -- GitLab