Validate project/package names in Workflow::Step instructions

Any project/package names in the YAML file should be valid names. No need to
make complicated logic around this. Also no need to test this as this is a
simple loop with a bunch of Package.valid_name
which is tested elsewhere...

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

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?
 

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

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

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?

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?

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?
 

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) }

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

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

src/api/spec/models/workflow/step_spec.rb

@@ -161,32 +161,62 @@ 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
 
     context 'when the source package is invalid' do
       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