diff --git a/src/api/app/models/workflow/step.rb b/src/api/app/models/workflow/step.rb index 07f2f4d7be9..f777a4f9064 100644 --- a/src/api/app/models/workflow/step.rb +++ b/src/api/app/models/workflow/step.rb @@ -4,7 +4,9 @@ class Workflow::Step SHORT_COMMIT_SHA_LENGTH = 7 - validate :validate_step_instructions + validate :validate_required_keys_in_step_instructions + validate :validate_project_names_in_step_instructions + validate :validate_package_names_in_step_instructions attr_accessor :scm_webhook, :step_instructions, :token, :workflow_run @@ -60,7 +62,7 @@ def target_package_name(short_commit_sha: false) protected - def validate_step_instructions + def validate_required_keys_in_step_instructions self.class::REQUIRED_KEYS.each do |required_key| unless step_instructions.key?(required_key) errors.add(:base, "The '#{required_key}' key is missing") @@ -77,15 +79,21 @@ def target_project_base_name raise AbstractMethodCalled end - # Only used in LinkPackageStep and BranchPackageStep. - def validate_source_project_and_package_name - errors.add(:base, "invalid source project '#{source_project_name}'") if step_instructions[:source_project] && !Project.valid_name?(source_project_name) - errors.add(:base, "invalid source package '#{source_package_name}'") if step_instructions[:source_package] && !Package.valid_name?(source_package_name) - errors.add(:base, "invalid target project '#{step_instructions[:target_project]}'") if step_instructions[:target_project] && !Project.valid_name?(step_instructions[:target_project]) + def validate_project_names_in_step_instructions + %i[project source_project target_project].each do |key_name| + next unless step_instructions[key_name] + next if Project.valid_name?(step_instructions[key_name]) + + errors.add(:base, "invalid #{key_name}: '#{step_instructions[key_name]}'") + end end - def validate_project_and_package_name - errors.add(:base, "invalid project '#{step_instructions[:project]}'") if step_instructions[:project] && !Project.valid_name?(step_instructions[:project]) - errors.add(:base, "invalid package '#{step_instructions[:package]}'") if step_instructions[:package] && !Package.valid_name?(step_instructions[:package]) + def validate_package_names_in_step_instructions + %i[package source_package target_package].each do |key_name| + next unless step_instructions[key_name] + next if Package.valid_name?(step_instructions[key_name]) + + errors.add(:base, "invalid #{key_name}: '#{step_instructions[key_name]}'") + end end end diff --git a/src/api/app/models/workflow/step/branch_package_step.rb b/src/api/app/models/workflow/step/branch_package_step.rb index e98c1003f0c..dcba9ecb2ea 100644 --- a/src/api/app/models/workflow/step/branch_package_step.rb +++ b/src/api/app/models/workflow/step/branch_package_step.rb @@ -5,8 +5,6 @@ class Workflow::Step::BranchPackageStep < Workflow::Step REQUIRED_KEYS = %i[source_project source_package target_project].freeze BRANCH_REQUEST_COMMIT_MESSAGE = 'Updated _branch_request file via SCM/CI Workflow run'.freeze - validate :validate_source_project_and_package_name - def call return unless valid? diff --git a/src/api/app/models/workflow/step/configure_repositories.rb b/src/api/app/models/workflow/step/configure_repositories.rb index e87d979259f..657b55a6d9f 100644 --- a/src/api/app/models/workflow/step/configure_repositories.rb +++ b/src/api/app/models/workflow/step/configure_repositories.rb @@ -6,7 +6,6 @@ class Workflow::Step::ConfigureRepositories < Workflow::Step validate :validate_repositories validate :validate_repository_paths validate :validate_architectures - validate :validate_project_name def call return if scm_webhook.closed_merged_pull_request? || scm_webhook.reopened_pull_request? @@ -83,10 +82,4 @@ def validate_architectures inexistent_architectures_sentence ||= inexistent_architectures.map { |key| "'#{key}'" }.to_sentence errors.add(:base, "configure_repositories step: Architectures #{inexistent_architectures_sentence} do not exist") end - - def validate_project_name - return if step_instructions[:project].blank? || Project.valid_name?(target_project_base_name) - - errors.add(:base, "Invalid project '#{target_project_base_name}'") - end end diff --git a/src/api/app/models/workflow/step/link_package_step.rb b/src/api/app/models/workflow/step/link_package_step.rb index 83612878384..3d02c4cc56f 100644 --- a/src/api/app/models/workflow/step/link_package_step.rb +++ b/src/api/app/models/workflow/step/link_package_step.rb @@ -4,7 +4,6 @@ class Workflow::Step::LinkPackageStep < Workflow::Step REQUIRED_KEYS = %i[source_project source_package target_project].freeze - validate :validate_source_project_and_package_name validate :validate_source_project_or_package_are_not_scmsynced def call diff --git a/src/api/app/models/workflow/step/rebuild_package.rb b/src/api/app/models/workflow/step/rebuild_package.rb index 1ebacb5ca9e..cd75d070011 100644 --- a/src/api/app/models/workflow/step/rebuild_package.rb +++ b/src/api/app/models/workflow/step/rebuild_package.rb @@ -5,8 +5,6 @@ class Workflow::Step::RebuildPackage < Workflow::Step attr_reader :project_name, :package_name - validate :validate_project_and_package_name - def call return if scm_webhook.closed_merged_pull_request? || scm_webhook.reopened_pull_request? return unless valid? diff --git a/src/api/app/models/workflow/step/submit_request.rb b/src/api/app/models/workflow/step/submit_request.rb index 501b17493e1..84914c1093c 100644 --- a/src/api/app/models/workflow/step/submit_request.rb +++ b/src/api/app/models/workflow/step/submit_request.rb @@ -1,6 +1,5 @@ class Workflow::Step::SubmitRequest < Workflow::Step REQUIRED_KEYS = %i[source_project source_package target_project].freeze - validate :validate_source_project_and_package_name def call return unless valid? diff --git a/src/api/app/models/workflow/step/trigger_services.rb b/src/api/app/models/workflow/step/trigger_services.rb index a506603ac2c..ffb97846053 100644 --- a/src/api/app/models/workflow/step/trigger_services.rb +++ b/src/api/app/models/workflow/step/trigger_services.rb @@ -4,8 +4,6 @@ class Workflow::Step::TriggerServices < Workflow::Step REQUIRED_KEYS = %i[project package].freeze - validate :validate_project_and_package_name - def call return if scm_webhook.closed_merged_pull_request? || scm_webhook.reopened_pull_request? diff --git a/src/api/spec/models/workflow/step/branch_package_step_spec.rb b/src/api/spec/models/workflow/step/branch_package_step_spec.rb index c092eaedce5..3b98b8799e6 100644 --- a/src/api/spec/models/workflow/step/branch_package_step_spec.rb +++ b/src/api/spec/models/workflow/step/branch_package_step_spec.rb @@ -573,44 +573,6 @@ end end - describe '#validate_source_project_and_package_name' do - let(:project) { create(:project, name: 'foo_project', maintainer: user) } - let(:package) { create(:package_with_file, name: 'bar_package', project: project) } - let(:scm_webhook) do - SCMWebhook.new(payload: { - scm: 'github', - event: 'pull_request', - action: 'opened', - pr_number: 1, - source_repository_full_name: 'reponame', - commit_sha: long_commit_sha, - target_repository_full_name: 'openSUSE/open-build-service' - }) - end - - context 'when the source project is invalid' do - let(:step_instructions) { { source_project: 'Invalid/format', source_package: package.name, target_project: target_project_name } } - - it 'gives an error for invalid name' do - subject.valid? - - expect { subject.call }.not_to change(Package, :count) - expect(subject.errors.full_messages.to_sentence).to eq("invalid source project 'Invalid/format'") - end - end - - context 'when the source package is invalid' do - let(:step_instructions) { { source_project: package.project.name, source_package: 'Invalid/format', target_project: target_project_name } } - - it 'gives an error for invalid name' do - subject.valid? - - expect { subject.call }.not_to change(Package, :count) - expect(subject.errors.full_messages.to_sentence).to eq("invalid source package 'Invalid/format'") - end - end - end - describe '.add_repositories?' do let(:project) { create(:project, name: 'foo_project', maintainer: user) } let(:package) { create(:package_with_file, name: 'bar_package', project: project) } diff --git a/src/api/spec/models/workflow/step/configure_repositories_spec.rb b/src/api/spec/models/workflow/step/configure_repositories_spec.rb index 0fd0377ad99..dbce524d3b8 100644 --- a/src/api/spec/models/workflow/step/configure_repositories_spec.rb +++ b/src/api/spec/models/workflow/step/configure_repositories_spec.rb @@ -326,41 +326,4 @@ end end end - - describe '#validate_project_name' do - let(:step_instructions) do - { - project: 'Invalid/format', - repositories: - [ - { - name: 'openSUSE_Tumbleweed', - paths: [{ target_project: 'openSUSE:Factory', target_repository: 'snapshot' }], - architectures: %w[ - x86_64 - ppc - ] - } - ] - } - end - let(:scm_webhook) { SCMWebhook.new(payload: payload) } - - subject do - described_class.new(step_instructions: step_instructions, - scm_webhook: scm_webhook, - token: token) - end - - context 'when the source project is invalid' do - let(:payload) { { scm: 'gitlab', event: 'Push Hook' } } - - it 'adds a validation error' do - subject.valid? - - expect { subject.call }.not_to change(Package, :count) - expect(subject.errors.full_messages.to_sentence).to eq("Invalid project 'Invalid/format'") - end - end - end end diff --git a/src/api/spec/models/workflow/step/rebuild_package_step_spec.rb b/src/api/spec/models/workflow/step/rebuild_package_step_spec.rb index 697ae5506f3..871e83533dc 100644 --- a/src/api/spec/models/workflow/step/rebuild_package_step_spec.rb +++ b/src/api/spec/models/workflow/step/rebuild_package_step_spec.rb @@ -37,28 +37,4 @@ it { expect { subject.call }.to raise_error(Pundit::NotAuthorizedError) } end - - describe '#validate_project_and_package_name' do - context 'when the project is invalid' do - let(:step_instructions) { { package: package.name, project: 'Invalid/format' } } - - it 'gives an error for invalid name' do - subject.valid? - - expect { subject.call }.not_to change(Package, :count) - expect(subject.errors.full_messages.to_sentence).to eq("invalid project 'Invalid/format'") - end - end - - context 'when the package is invalid' do - let(:step_instructions) { { package: 'Invalid/format', project: project.name } } - - it 'gives an error for invalid name' do - subject.valid? - - expect { subject.call }.not_to change(Package, :count) - expect(subject.errors.full_messages.to_sentence).to eq("invalid package 'Invalid/format'") - end - end - end end diff --git a/src/api/spec/models/workflow/step_spec.rb b/src/api/spec/models/workflow/step_spec.rb index a4f4102e838..29286073f07 100644 --- a/src/api/spec/models/workflow/step_spec.rb +++ b/src/api/spec/models/workflow/step_spec.rb @@ -161,16 +161,46 @@ def target_project_base_name end end - describe '#validate_source_project_and_package_name' do + describe '#validate_project_names_in_step_instructions' do before do subject.valid? end + context 'when the project is invalid' do + subject { Workflow::Step::RebuildPackage.new(step_instructions: { project: 'Invalid/format', package: 'franz' }) } + + it 'gives an error for invalid name' do + expect(subject.errors.full_messages.to_sentence).to eq("invalid project: 'Invalid/format'") + end + end + context 'when the source project is invalid' do subject { Workflow::Step::BranchPackageStep.new(step_instructions: { source_project: 'Invalid/format', source_package: 'hans', target_project: 'franz' }) } it 'gives an error for invalid name' do - expect(subject.errors.full_messages.to_sentence).to eq("invalid source project 'Invalid/format'") + expect(subject.errors.full_messages.to_sentence).to eq("invalid source_project: 'Invalid/format'") + end + end + + context 'when the target project is invalid' do + subject { Workflow::Step::BranchPackageStep.new(step_instructions: { source_project: 'hans', source_package: 'franz', target_project: 'Invalid/format' }) } + + it 'gives an error for invalid name' do + expect(subject.errors.full_messages.to_sentence).to eq("invalid target_project: 'Invalid/format'") + end + end + end + + describe '#validate_package_names_in_step_instructions' do + before do + subject.valid? + end + + context 'when the package is invalid' do + subject { Workflow::Step::RebuildPackage.new(step_instructions: { project: 'hans', package: 'Invalid/format' }) } + + it 'gives an error for invalid name' do + expect(subject.errors.full_messages.to_sentence).to eq("invalid package: 'Invalid/format'") end end @@ -178,15 +208,15 @@ def target_project_base_name subject { Workflow::Step::BranchPackageStep.new(step_instructions: { source_project: 'hans', source_package: 'Invalid/format', target_project: 'franz' }) } it 'gives an error for invalid name' do - expect(subject.errors.full_messages.to_sentence).to eq("invalid source package 'Invalid/format'") + expect(subject.errors.full_messages.to_sentence).to eq("invalid source_package: 'Invalid/format'") end end - context 'when the target project is invalid' do - subject { Workflow::Step::BranchPackageStep.new(step_instructions: { source_project: 'hans', source_package: 'franz', target_project: 'Invalid/format' }) } + context 'when the target package is invalid' do + subject { Workflow::Step::BranchPackageStep.new(step_instructions: { source_project: 'hans', source_package: 'franz', target_project: 'peter', target_package: 'Invalid/format' }) } it 'gives an error for invalid name' do - expect(subject.errors.full_messages.to_sentence).to eq("invalid target project 'Invalid/format'") + expect(subject.errors.full_messages.to_sentence).to eq("invalid target_package: 'Invalid/format'") end end end