Merge pull request #12570 from adrianschroeter/conflicting_action_det…

…ection

[api] protect us against broken requests due to multiple entries

src/api/app/models/bs_request.rb

@@ -412,6 +412,7 @@ def render_xml(opts = {})
           builder.comment!(description) if description.present?
         end
       end
+
       if opts[:withfullhistory]
         history_elements.each do |history|
           # we do ignore the review history here on purpose to stay compatible
@@ -922,6 +923,7 @@ def sanitize!
     expand_targets
 
     check_bs_request_actions!
+    check_uniq_actions!
 
     # Autoapproval? Is the creator allowed to accept it?
     permission_check_change_state!(newstate: 'accepted') if accept_at
@@ -1154,6 +1156,14 @@ def change_priorities?(new_priority)
       (new_priority == 'moderate' && priority == 'low')
   end
 
+  def check_uniq_actions!
+    uniq_keys = []
+    bs_request_actions.each do |action|
+      uniq_keys << action.uniq_key
+    end
+    raise ConflictingActions, 'Conflicting Actions' if uniq_keys.length > uniq_keys.uniq.length
+  end
+
   def check_bs_request_actions!(opts = {})
     bs_request_actions.each do |action|
       action.check_action_permission!(opts[:skip_source])

src/api/app/models/bs_request/errors.rb

@@ -59,6 +59,9 @@ class ReviewChangeStateNoPermission < APIError
   class ReviewNotSpecified < APIError
   end
 
+  class ConflictingActions < APIError
+  end
+
   class CreatorCannotAcceptOwnRequests < APIError
     setup 403, "the creator of the request cannot approve it's own requests"
   end

src/api/app/models/bs_request_action.rb

@@ -30,12 +30,6 @@ class BsRequestAction < ApplicationRecord
   validates :sourceupdate, inclusion: { in: VALID_SOURCEUPDATE_OPTIONS, allow_nil: true }
   validate :check_sanity
   validates :type, presence: true
-  validates :type, uniqueness: {
-    scope: [:target_project, :target_package, :target_repository, :bs_request_id],
-    case_sensitive: false,
-    conditions: -> { where.not(type: ['add_role', 'maintenance_incident']) }
-  }
-
   before_validation :set_target_associations
   after_create :cache_diffs
 
@@ -824,6 +818,10 @@ def set_sourceupdate_default(user)
     update(sourceupdate: 'cleanup') if target_project && user.branch_project_name(target_project) == source_project
   end
 
+  def uniq_key
+    ' '
+  end
+
   private
 
   def cache_diffs

src/api/app/models/bs_request_action_add_role.rb

@@ -25,6 +25,10 @@ def check_sanity
     errors.add(:person_name, 'Either person or group needs to be set')
   end
 
+  def uniq_key
+    "add_role/#{target_project}/#{target_package}/#{person_name}/#{role}"
+  end
+
   def execute_accept(_opts)
     object = Project.find_by_name(target_project)
     object = object.packages.find_by_name(target_package) if target_package

src/api/app/models/bs_request_action_change_devel.rb

@@ -26,6 +26,10 @@ def execute_accept(_opts)
     target_package.store(comment: "change_devel request #{bs_request.number}", request: bs_request)
   end
 
+  def uniq_key
+    "changedevel/#{target_project}/#{target_package}"
+  end
+
   #### Alias of methods
 end
 

src/api/app/models/bs_request_action_delete.rb

@@ -23,6 +23,10 @@ def check_sanity
     errors.add(:target_project, 'must not target package and target repository') if target_repository && target_package
   end
 
+  def uniq_key
+    "#{target_project}/#{target_package}"
+  end
+
   def render_xml_attributes(node)
     attributes = xml_package_attributes('target')
     attributes[:repository] = target_repository if target_repository.present?

src/api/app/models/bs_request_action_maintenance_incident.rb

@@ -27,6 +27,11 @@ def is_maintenance_incident?
     true
   end
 
+  def uniq_key
+    # source_package should be actually release_name, but this would be a speed burden here atm.
+    "#{target_project}/#{source_package}/#{target_releaseproject}"
+  end
+
   def get_releaseproject(pkg, tprj)
     return if pkg.is_patchinfo?
 

src/api/app/models/bs_request_action_maintenance_release.rb

@@ -26,6 +26,10 @@ def is_maintenance_release?
     true
   end
 
+  def uniq_key
+    "#{target_project}/#{target_package}"
+  end
+
   def execute_accept(opts)
     pkg = Package.get_by_project_and_name(source_project, source_package)
 

src/api/app/models/bs_request_action_release.rb

@@ -30,6 +30,10 @@ def is_release?
   end
   # rubocop:enable Naming/PredicateName
 
+  def uniq_key
+    "#{target_project}/#{target_package}"
+  end
+
   def execute_accept(opts)
     pkg = Package.get_by_project_and_name(source_project, source_package)
 

src/api/app/models/bs_request_action_set_bugowner.rb

@@ -24,6 +24,10 @@ def check_sanity
     errors.add(:person_name, 'Either person or group needs to be set')
   end
 
+  def uniq_key
+    "setbugowner/#{target_project}/#{target_package}"
+  end
+
   def execute_accept(_opts)
     object = Project.find_by_name!(target_project)
     bugowner = Role.find_by_title!('bugowner')

src/api/app/models/bs_request_action_submit.rb

@@ -23,6 +23,10 @@ def is_submit?
     true
   end
 
+  def uniq_key
+    "#{target_project}/#{target_package}"
+  end
+
   def execute_accept(opts)
     # create package unless it exists already
     target_project = Project.get_by_name(self.target_project)

src/api/spec/models/bs_request_action_spec.rb

@@ -46,25 +46,15 @@
 
     it { expect(bs_request_action).to be_valid }
 
-    it 'validates uniqueness of type among bs requests, target_project and target_package' do
-      duplicated_bs_request_action = build(:bs_request_action, action_attributes.merge(bs_request: bs_request))
-      expect(duplicated_bs_request_action).not_to be_valid
-      expect(duplicated_bs_request_action.errors.full_messages.to_sentence).to eq('Type has already been taken')
-    end
-
-    RSpec.shared_examples 'it skips validation for type' do |type|
-      context "type '#{type}'" do
-        it 'allows multiple bs request actions' do
-          expect(build(:bs_request_action, action_attributes.merge(type: type,
-                                                                   person_name: user.login,
-                                                                   role: Role.find_by_title!('maintainer'),
-                                                                   bs_request: bs_request))).to be_valid
-        end
-      end
+    it 'validates uniqueness for same type' do
+      bs_request.bs_request_actions << bs_request.bs_request_actions.first.dup
+      expect { bs_request.send(:check_uniq_actions!) }.to raise_error(BsRequest::Errors::ConflictingActions)
     end
 
-    it_behaves_like 'it skips validation for type', 'add_role'
-    it_behaves_like 'it skips validation for type', 'maintenance_incident'
+    it 'does not validate uniqueness for different types' do
+      bs_request.bs_request_actions << build(:bs_request_action_add_bugowner_role)
+      expect { bs_request.send(:check_uniq_actions!) }.not_to raise_error(BsRequest::Errors::ConflictingActions)
+    end
   end
 
   it { is_expected.to belong_to(:bs_request).touch(true).optional }

src/api/test/functional/maintenance_test.rb

@@ -1937,6 +1937,22 @@ def test_create_invalid_incident_request
                                  </request>'
     assert_response 404
     assert_xml_tag tag: 'status', attributes: { code: 'unknown_project' }
+
+    # double entries
+    post '/request?cmd=create&addrevision=1', params: '<request>
+                                   <action type="maintenance_incident">
+                                     <source project="RemoteInstance:kde4" package="kdelibs" />
+                                     <target project="My:Maintenance" releaseproject="BaseDistro2.0:LinkedUpdateProject" />
+                                   </action>
+                                   <action type="maintenance_incident">
+                                     <source project="RemoteInstance:kde4" package="kdelibs" />
+                                     <target project="My:Maintenance" releaseproject="BaseDistro2.0:LinkedUpdateProject" />
+                                   </action>
+                                   <description>To fix my &lt;bug</description>
+                                   <state name="new" />
+                                 </request>'
+    assert_response 400
+    assert_xml_tag tag: 'status', attributes: { code: 'conflicting_actions' }
   end
 
   def test_create_invalid_release_request

src/api/test/functional/request_events_test.rb

@@ -44,11 +44,11 @@ def test_very_large_request_event
     Timecop.travel(2013, 8, 20, 12, 0, 0)
     myid = 0
     SendEventEmailsJob.new.perform
-    assert_difference('ActionMailer::Base.deliveries.size', +1) do
+    assert_difference('ActionMailer::Base.deliveries.size', +2) do
       body = "<request>\n"
       actions = 1000
-      actions.times do
-        body += "<action type='add_role'><target project='home:tom'/><person name='Iggy' role='reviewer'/></action>\n"
+      actions.times do |number|
+        body += "<action type='submit'><source project='Apache' package='apache2'/><target project='home:tom' package='package_#{number}'/></action>\n"
       end
       body += '</request>'
       post '/request?cmd=create', params: body
@@ -61,8 +61,8 @@ def test_very_large_request_event
 
     email = ActionMailer::Base.deliveries.last
 
-    assert_match(/^Request #{myid} created by Iggy \(add_role home:tom, /, email.subject)
-    assert_equal ['tschmidt@example.com'], email.to # tom is maintainer
+    assert_match(%r{^Request #{myid} requires review \(submit home:tom/package_0, }, email.subject)
+    assert_equal ['fred@feuerstein.de', 'fred@feuerstein.de'], email.to
   end
 
   def test_set_bugowner_event