[api][webui] fix breakage on project destroy

Avoid the double write of meta data. It leads to a broken project
after undelete and it is causing double load in the backend.

Testcase for correct undelete behaviour added.

method in project model which was only used in one place got dropped,
since it wasn't usable this way anyway.

src/api/app/controllers/source_controller.rb

@@ -150,7 +150,11 @@ def delete_project
       return
     end
     project.check_weak_dependencies!
-    check_and_remove_repositories!(project.repositories, !params[:remove_linking_repositories].blank?, !params[:force].blank?)
+    opts = { no_write_to_backend: true,
+             force:               params[:force].present?,
+             recursive_remove:    params[:remove_linking_repositories].present?
+           }
+    check_and_remove_repositories!(project.repositories, opts)
 
     logger.info "destroying project object #{project.name}"
     project.commit_opts = { comment: params[:comment] }
@@ -504,7 +508,11 @@ def update_project_meta
 
     if project
       remove_repositories = project.get_removed_repositories(request_data)
-      check_and_remove_repositories!(remove_repositories, !params[:remove_linking_repositories].blank?, !params[:force].blank?)
+      opts = { no_write_to_backend: true,
+               force:               params[:force].present?,
+               recursive_remove:    params[:remove_linking_repositories].present?
+             }
+      check_and_remove_repositories!(remove_repositories, opts)
     end
 
     Project.transaction do
@@ -522,13 +530,14 @@ def update_project_meta
     render_ok
   end
 
-  def check_and_remove_repositories!(repositories, full_remove, force = false)
-    error = Project.check_repositories(repositories) unless force
-    if !force && error[:error]
+  def check_and_remove_repositories!(repositories, opts)
+    error = Project.check_repositories(repositories) unless opts[:force]
+    if !opts[:force] && error[:error]
       raise RepoDependency, error[:error]
     else
-      error = Project.remove_repositories(repositories, full_remove)
-      if !force && error[:error]
+      error = Project.remove_repositories(repositories, opts)
+
+      if !opts[:force] && error[:error]
         raise ChangeProjectNoPermission, error[:error]
       end
     end

src/api/app/controllers/webui/project_controller.rb

@@ -708,12 +708,14 @@ def save_meta
       Suse::Validator.validate('project', params[:meta])
       request_data = Xmlhash.parse(params[:meta])
 
+      remove_repositories = @project.get_removed_repositories(request_data)
+      error = Project.check_repositories(remove_repositories)
+      errors << error[:error] if error[:error]
       errors << Project.validate_remote_permissions(request_data)[:error]
       errors << Project.validate_link_xml_attribute(request_data, @project.name)[:error]
       errors << Project.validate_maintenance_xml_attribute(request_data)[:error]
       errors << Project.validate_repository_xml_attribute(request_data, @project.name)[:error]
 
-      errors << @project.check_and_remove_repositories(request_data, !params[:remove_linking_repositories].blank?)[:error]
       errors = errors.compact
 
       if errors.empty?
@@ -724,7 +726,11 @@ def save_meta
         end
       end
 
-    rescue Suse::ValidationError  => exception
+      opts = {no_write_to_backend: true}
+      opts[:recursive_remove] = params[:remove_linking_repositories].present?
+      Project.remove_repositories(remove_repositories, opts)
+
+    rescue Suse::ValidationError => exception
       errors << exception.message
     rescue Project::UnknownObjectError  => exception
       errors << "Project with name '#{exception.message}' not found"

src/api/app/models/project.rb

@@ -1728,16 +1728,6 @@ def self.validate_repository_xml_attribute(request_data, project_name)
     {}
   end
 
-  def check_and_remove_repositories(request_data, full_remove = false)
-    remove_repositories = get_removed_repositories(request_data)
-    error = Project.check_repositories(remove_repositories)
-
-    return error if error[:error]
-
-    error = Project.remove_repositories(remove_repositories, full_remove)
-    error[:error] ? error : {}
-  end
-
   def get_removed_repositories(request_data)
     new_repositories = request_data.elements('repository').map(&:values).flatten
     old_repositories = repositories.all.map(&:name)
@@ -1773,18 +1763,22 @@ def self.check_repositories(repositories)
     {}
   end
 
-  def self.remove_repositories(repositories, full_remove = false)
+  # opts: recursive_remove no_write_to_backend
+  def self.remove_repositories(repositories, opts = {})
     deleted_repository = Repository.deleted_instance
 
     repositories.each do |repo|
       linking_repositories = repo.linking_repositories
       project = repo.project
 
       # full remove, otherwise the model will take care of the cleanup
-      if full_remove
+      if opts[:recursive_remove]
         # recursive for INDIRECT linked repositories
         unless linking_repositories.length < 1
-          Project.remove_repositories(linking_repositories, true)
+          # FIXME: we would actually need to check for :no_write_to_backend here as well
+          #        but the calling code is currently broken and would need the starting
+          #        project different
+          Project.remove_repositories(linking_repositories, {recursive_remove: true})
         end
 
         # try to remove the repository
@@ -1802,7 +1796,7 @@ def self.remove_repositories(repositories, full_remove = false)
       if Repository.exists?(repo.id) && repository
         logger.info "destroy repo #{repository.name} in '#{project.name}'"
         repository.destroy
-        project.store({ lowprio: true }) # low prio storage
+        project.store({ lowprio: true }) unless opts[:no_write_to_backend]
       end
     end
     {}

src/api/test/fixtures/path_elements.yml

@@ -30,3 +30,7 @@ UseRemoteInstance_pop_path:
   parent_id: BaseDistro_repo
   repository_id: UseRemoteInstance_pop
   position: 1
+kde4_repo:
+  parent_id: 98
+  repository_id: 86
+  position: 1

src/api/test/fixtures/repositories.yml

@@ -84,3 +84,7 @@ repositories_97:
   id: 97
   name: BrokenPublishing_repo
   project: BrokenPublishing
+kde4_standard:
+  id: 98
+  name: kde4_standard
+  project: kde4

src/api/test/fixtures/repository_architectures.yml

@@ -122,3 +122,8 @@ repository_architectures_broken_publishing:
   position: 0
   id: 940713216
   architecture: i586
+kde4_repo_arch:
+  repository_id: 98
+  position: 0
+  id: 940713217
+  architecture: s390

src/api/test/functional/maintenance_test.rb

@@ -609,7 +609,10 @@ def test_mbranch_and_maintenance_entire_project_request
     assert_no_xml_tag :parent => { tag: 'issue' }, :tag => 'issue', :attributes => { change: '' }
     assert_xml_tag :parent => { tag: 'issue', attributes: { change: 'added' } }, :tag => 'name', :content => '1042'
 
-    post '/source', :cmd => 'branch', :package => 'kdelibs', :target_project => 'home:tom:branches:OBS_Maintained:pack2'
+    get '/source/home:tom:branches:OBS_Maintained:pack2/_meta'
+    assert_response :success
+    oldmeta = @response.body
+    post '/source', cmd: 'branch', package: 'kdelibs', target_project: 'home:tom:branches:OBS_Maintained:pack2'
     assert_response :success
     get '/source/home:tom:branches:OBS_Maintained:pack2/kdelibs.kde4/_link'
     assert_response :success
@@ -668,6 +671,8 @@ def test_mbranch_and_maintenance_entire_project_request
     # delete kdelibs package again or incident creation will fail since it does not point to a maintained project.
     delete '/source/home:tom:branches:OBS_Maintained:pack2/kdelibs.kde4'
     assert_response :success
+    put '/source/home:tom:branches:OBS_Maintained:pack2/_meta', oldmeta
+    assert_response :success
 
     # create maintenance request
     # without specifing target, the default target must get found via attribute
@@ -699,8 +704,8 @@ def test_mbranch_and_maintenance_entire_project_request
 
     # store data for later checks
     get '/source/home:tom:branches:OBS_Maintained:pack2/_meta'
-    oprojectmeta = ActiveXML::Node.new(@response.body)
     assert_response :success
+    oprojectmeta = ActiveXML::Node.new(@response.body)
 
     get "/source/home:tom:branches:OBS_Maintained:pack2/_meta"
     assert_response :success

src/api/test/functional/request_controller_test.rb

@@ -1535,10 +1535,10 @@ def test_auto_revoke_when_source_gets_removed_submit
     # id2 = node.value(:id)
 
     # delete projects
-    delete '/source/home:tom:branches:kde4'
-    assert_response :success
     delete '/source/home:tom:branches:home:tom:branches:kde4'
     assert_response :success
+    delete '/source/home:tom:branches:kde4'
+    assert_response :success
 
     # request got automatically revoked
     get "/request/#{id1}"

src/api/test/functional/source_controller_test.rb

@@ -333,7 +333,7 @@ def test_put_project_meta_with_invalid_permissions
 
     # Change description
     xml = @response.body
-    new_desc = 'Changed description'
+    new_desc = 'Changed description 1'
     doc = ActiveXML::Node.new(xml)
     d = doc.find_first('description')
     d.text = new_desc
@@ -583,7 +583,7 @@ def do_change_project_meta_test (project, response1, response2, tag2, doesmatch)
 
     # Change description
     xml = @response.body
-    new_desc = 'Changed description'
+    new_desc = 'Changed description 2'
     doc = REXML::Document.new(xml)
     d = doc.elements['//description']
     d.text = new_desc
@@ -848,13 +848,12 @@ def test_lock_package
   def test_put_package_meta_with_invalid_permissions
     login_tom
     # The user is valid, but has weak permissions
-
-    get url_for(:controller => :source, :action => :show_package_meta, :project => 'kde4', :package => 'kdelibs')
+    get url_for(controller: :source, action: :show_package_meta, project: 'kde4', package: 'kdelibs')
     assert_response :success
 
     # Change description
     xml = @response.body
-    new_desc = 'Changed description'
+    new_desc = 'Changed description 4'
     olddoc = REXML::Document.new(xml)
     doc = REXML::Document.new(xml)
     d = doc.elements['//description']
@@ -907,7 +906,7 @@ def do_change_package_meta_test (project, package, response1, response2, tag2, m
     end
     # Change description
     xml = @response.body
-    new_desc = 'Changed description'
+    new_desc = 'Changed description 3'
     doc = REXML::Document.new(xml)
     d = doc.elements['//description']
     d.text = new_desc
@@ -1786,6 +1785,18 @@ def test_remove_and_undelete_operations
     assert_response :success
 
     # delete entire project
+    prepare_request_with_user 'fredlibs', 'buildservice'
+    get '/source/kde4/_meta'
+    assert_response :success
+    meta_before_delete = @response.body
+    # kind of stupid, but do_change_project_meta_test modifies backend data and does not clean up
+    put '/source/kde4/_meta', meta_before_delete
+    assert_response :success
+    get '/source/kde4/_project/_history?meta=1'
+    assert_response :success
+    node = ActiveXML::Node.new(@response.body)
+    revision = node.each(:revision).last.value :rev
+    expected_revision = revision.to_i + 1
     delete '/source/kde4?user=illegal&comment=drop%20project'
     assert_response :success
 
@@ -1816,9 +1827,13 @@ def test_remove_and_undelete_operations
     assert_xml_tag(:parent => { :tag => 'revision' }, :tag => 'user', :content => 'fredlibs')
     assert_xml_tag(:parent => { :tag => 'revision' }, :tag => 'comment', :content => 'drop project')
     get '/source/kde4/_project/_history?meta=1&deleted=1'
-    assert_xml_tag(:parent => { :tag => 'revision' }, :tag => 'user', :content => 'fredlibs')
-    assert_xml_tag(:parent => { :tag => 'revision' }, :tag => 'comment', :content => 'drop project')
     assert_response :success
+    assert_xml_tag(parent: { tag: 'revision' }, tag: 'user', content: 'fredlibs')
+    assert_xml_tag(parent: { tag: 'revision' }, tag: 'comment', content: 'drop project')
+    # there must be only one change
+    node = ActiveXML::Node.new(@response.body)
+    revision = node.each(:revision).last.value :rev
+    assert_equal expected_revision, revision.to_i
 
     prepare_request_with_user 'fredlibs', 'buildservice'
     # undelete project
@@ -1833,10 +1848,10 @@ def test_remove_and_undelete_operations
     get '/source/kde4'
     assert_response :success
     get '/source/kde4/_project'
-
     assert_response :success
     get '/source/kde4/_meta'
     assert_response :success
+    assert_equal meta_before_delete, @response.body
     get '/source/kde4/kdelibs'
     assert_response :success
     get '/source/kde4/kdelibs/_meta'

src/api/test/unit/code_quality_test.rb

@@ -101,6 +101,8 @@ def setup
       'Webui::PackageController#submit_request'                                 => 101.98,
       'Webui::PatchinfoController#save'                                         => 240.1,
       'Webui::ProjectController#check_devel_package_status'                     => 81.95,
+      'Webui::ProjectController#save_meta'                                      => 83.61,
+      'Webui::RequestController#show'                                           => 91.96,
       'Webui::SearchController#set_parameters'                                  => 98.04,
       'WizardController#package_wizard'                                         => 97.46
   }