Merge branch 'api-delete-respect-headers' into 'master'

API: Respect the 'If-Unmodified-Since' for delete endpoints

See merge request !9621

29 files changed, 122 insertions, 83 deletions

diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb
index cdacf9839e5..4fa9b2b2494 100644
--- a/lib/api/access_requests.rb
+++ b/lib/api/access_requests.rb
@@ -67,10 +67,12 @@ module API
         end
         delete ":id/access_requests/:user_id" do
           source = find_source(source_type, params[:id])
+          member = source.requesters.find_by!(user_id: params[:user_id])
 
-          status 204
-          ::Members::DestroyService.new(source, current_user, params)
-            .execute(:requesters)
+          destroy_conditionally!(member) do
+            ::Members::DestroyService.new(source, current_user, params)
+              .execute(:requesters)
+          end
         end
       end
     end
diff --git a/lib/api/award_emoji.rb b/lib/api/award_emoji.rb
index 5a028fc9d0b..8e3851640da 100644
--- a/lib/api/award_emoji.rb
+++ b/lib/api/award_emoji.rb
@@ -88,8 +88,7 @@ module API
 
             unauthorized! unless award.user == current_user || current_user.admin?
 
-            status 204
-            award.destroy
+            destroy_conditionally!(award)
           end
         end
       end
diff --git a/lib/api/boards.rb b/lib/api/boards.rb
index 5a2d7a681e3..0d11c5fc971 100644
--- a/lib/api/boards.rb
+++ b/lib/api/boards.rb
@@ -122,13 +122,13 @@ module API
         end
         delete "/lists/:list_id" do
           authorize!(:admin_list, user_project)
-
           list = board_lists.find(params[:list_id])
 
-          service = ::Boards::Lists::DestroyService.new(user_project, current_user)
-
-          unless service.execute(list)
-            render_api_error!({ error: 'List could not be deleted!' }, 400)
+          destroy_conditionally!(list) do |list|
+            service = ::Boards::Lists::DestroyService.new(user_project, current_user)
+            unless service.execute(list)
+              render_api_error!({ error: 'List could not be deleted!' }, 400)
+            end
           end
         end
       end
diff --git a/lib/api/branches.rb b/lib/api/branches.rb
index d3dbf941298..b87f7cdbad1 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -125,11 +125,18 @@ module API
       delete ':id/repository/branches/:branch', requirements: BRANCH_ENDPOINT_REQUIREMENTS do
         authorize_push_project
 
-        result = DeleteBranchService.new(user_project, current_user)
-                 .execute(params[:branch])
+        branch = user_project.repository.find_branch(params[:branch])
+        not_found!('Branch') unless branch
+
+        commit = user_project.repository.commit(branch.dereferenced_target)
+
+        destroy_conditionally!(commit, last_update_field: :authored_date) do
+          result = DeleteBranchService.new(user_project, current_user)
+                    .execute(params[:branch])
 
-        if result[:status] != :success
-          render_api_error!(result[:message], result[:return_code])
+          if result[:status] != :success
+            render_api_error!(result[:message], result[:return_code])
+          end
         end
       end
 
diff --git a/lib/api/broadcast_messages.rb b/lib/api/broadcast_messages.rb
index 9980aec4752..0b45621ce7b 100644
--- a/lib/api/broadcast_messages.rb
+++ b/lib/api/broadcast_messages.rb
@@ -91,8 +91,7 @@ module API
       delete ':id' do
         message = find_message
 
-        status 204
-        message.destroy
+        destroy_conditionally!(message)
       end
     end
   end
diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb
index 42e7c1486b0..f405c341398 100644
--- a/lib/api/deploy_keys.rb
+++ b/lib/api/deploy_keys.rb
@@ -125,8 +125,7 @@ module API
         key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id])
         not_found!('Deploy Key') unless key
 
-        status 204
-        key.destroy
+        destroy_conditionally!(key)
       end
     end
   end
diff --git a/lib/api/environments.rb b/lib/api/environments.rb
index c774a5c6685..e33269f9483 100644
--- a/lib/api/environments.rb
+++ b/lib/api/environments.rb
@@ -79,8 +79,7 @@ module API
 
         environment = user_project.environments.find(params[:environment_id])
 
-        status 204
-        environment.destroy
+        destroy_conditionally!(environment)
       end
 
       desc 'Stops an existing environment' do
diff --git a/lib/api/group_variables.rb b/lib/api/group_variables.rb
index f64da4ab77b..25152f30998 100644
--- a/lib/api/group_variables.rb
+++ b/lib/api/group_variables.rb
@@ -88,8 +88,7 @@ module API
         variable = user_group.variables.find_by(key: params[:key])
         not_found!('GroupVariable') unless variable
 
-        status 204
-        variable.destroy
+        destroy_conditionally!(variable)
       end
     end
   end
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index e56427304a6..ee2ad27837b 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -118,8 +118,9 @@ module API
         group = find_group!(params[:id])
         authorize! :admin_group, group
 
-        status 204
-        ::Groups::DestroyService.new(group, current_user).execute
+        destroy_conditionally!(group) do |group|
+          ::Groups::DestroyService.new(group, current_user).execute
+        end
       end
 
       desc 'Get a list of projects in this group.' do
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index b56fd2388b3..84980864151 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -11,6 +11,25 @@ module API
       declared(params, options).to_h.symbolize_keys
     end
 
+    def check_unmodified_since!(last_modified)
+      if_unmodified_since = Time.parse(headers['If-Unmodified-Since']) rescue nil
+
+      if if_unmodified_since && last_modified && last_modified > if_unmodified_since
+        render_api_error!('412 Precondition Failed', 412)
+      end
+    end
+
+    def destroy_conditionally!(resource, last_update_field: :updated_at)
+      check_unmodified_since!(resource.public_send(last_update_field))
+
+      status 204
+      if block_given?
+        yield resource
+      else
+        resource.destroy
+      end
+    end
+
     def current_user
       return @current_user if defined?(@current_user)
 
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 4cec1145f3a..6503629e2a2 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -230,8 +230,8 @@ module API
         not_found!('Issue') unless issue
 
         authorize!(:destroy_issue, issue)
-        status 204
-        issue.destroy
+
+        destroy_conditionally!(issue)
       end
 
       desc 'List merge requests closing issue'  do
diff --git a/lib/api/labels.rb b/lib/api/labels.rb
index 4520c98d951..c0cf618ee8d 100644
--- a/lib/api/labels.rb
+++ b/lib/api/labels.rb
@@ -56,8 +56,7 @@ module API
         label = user_project.labels.find_by(title: params[:name])
         not_found!('Label') unless label
 
-        status 204
-        label.destroy
+        destroy_conditionally!(label)
       end
 
       desc 'Update an existing label. At least one optional parameter is required.' do
diff --git a/lib/api/members.rb b/lib/api/members.rb
index bb970b7cd54..a5d3d7f25a0 100644
--- a/lib/api/members.rb
+++ b/lib/api/members.rb
@@ -93,11 +93,11 @@ module API
         end
         delete ":id/members/:user_id" do
           source = find_source(source_type, params[:id])
-          # Ensure that memeber exists
-          source.members.find_by!(user_id: params[:user_id])
+          member = source.members.find_by!(user_id: params[:user_id])
 
-          status 204
-          ::Members::DestroyService.new(source, current_user, declared_params).execute
+          destroy_conditionally!(member) do
+            ::Members::DestroyService.new(source, current_user, declared_params).execute
+          end
         end
       end
     end
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index 8810d4e441d..969c6064662 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -164,8 +164,8 @@ module API
         merge_request = find_project_merge_request(params[:merge_request_iid])
 
         authorize!(:destroy_merge_request, merge_request)
-        status 204
-        merge_request.destroy
+
+        destroy_conditionally!(merge_request)
       end
 
       params do
diff --git a/lib/api/notes.rb b/lib/api/notes.rb
index 4e4e473994b..e116448c15b 100644
--- a/lib/api/notes.rb
+++ b/lib/api/notes.rb
@@ -129,10 +129,12 @@ module API
         end
         delete ":id/#{noteables_str}/:noteable_id/notes/:note_id" do
           note = user_project.notes.find(params[:note_id])
+
           authorize! :admin_note, note
 
-          status 204
-          ::Notes::DestroyService.new(user_project, current_user).execute(note)
+          destroy_conditionally!(note) do |note|
+            ::Notes::DestroyService.new(user_project, current_user).execute(note)
+          end
         end
       end
     end
diff --git a/lib/api/pipeline_schedules.rb b/lib/api/pipeline_schedules.rb
index dbeaf9e17ef..e3123ef4e2d 100644
--- a/lib/api/pipeline_schedules.rb
+++ b/lib/api/pipeline_schedules.rb
@@ -117,8 +117,7 @@ module API
         not_found!('PipelineSchedule') unless pipeline_schedule
         authorize! :admin_pipeline_schedule, pipeline_schedule
 
-        status :accepted
-        present pipeline_schedule.destroy, with: Entities::PipelineScheduleDetails
+        destroy_conditionally!(pipeline_schedule)
       end
     end
 
diff --git a/lib/api/project_hooks.rb b/lib/api/project_hooks.rb
index 649dd891f56..5b457bbe639 100644
--- a/lib/api/project_hooks.rb
+++ b/lib/api/project_hooks.rb
@@ -96,8 +96,7 @@ module API
       delete ":id/hooks/:hook_id" do
         hook = user_project.hooks.find(params.delete(:hook_id))
 
-        status 204
-        hook.destroy
+        destroy_conditionally!(hook)
       end
     end
   end
diff --git a/lib/api/project_snippets.rb b/lib/api/project_snippets.rb
index f3d905b0068..704e8c6718d 100644
--- a/lib/api/project_snippets.rb
+++ b/lib/api/project_snippets.rb
@@ -116,8 +116,8 @@ module API
         not_found!('Snippet') unless snippet
 
         authorize! :admin_project_snippet, snippet
-        status 204
-        snippet.destroy
+
+        destroy_conditionally!(snippet)
       end
 
       desc 'Get a raw project snippet'
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index 15c3832b032..78d900984ac 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -1,7 +1,6 @@
 require_dependency 'declarative_policy'
 
 module API
-  # Projects API
   class Projects < Grape::API
     include PaginationParams
 
@@ -334,7 +333,10 @@ module API
       desc 'Remove a project'
       delete ":id" do
         authorize! :remove_project, user_project
-        ::Projects::DestroyService.new(user_project, current_user, {}).async_execute
+
+        destroy_conditionally!(user_project) do
+          ::Projects::DestroyService.new(user_project, current_user, {}).async_execute
+        end
 
         accepted!
       end
@@ -363,8 +365,7 @@ module API
         authorize! :remove_fork_project, user_project
 
         if user_project.forked?
-          status 204
-          user_project.forked_project_link.destroy
+          destroy_conditionally!(user_project.forked_project_link)
         else
           not_modified!
         end
@@ -408,8 +409,7 @@ module API
         link = user_project.project_group_links.find_by(group_id: params[:group_id])
         not_found!('Group Link') unless link
 
-        status 204
-        link.destroy
+        destroy_conditionally!(link)
       end
 
       desc 'Upload a file'
diff --git a/lib/api/protected_branches.rb b/lib/api/protected_branches.rb
index dccf4fa27a7..15fcb9e8e27 100644
--- a/lib/api/protected_branches.rb
+++ b/lib/api/protected_branches.rb
@@ -76,9 +76,7 @@ module API
       delete ':id/protected_branches/:name', requirements: BRANCH_ENDPOINT_REQUIREMENTS do
         protected_branch = user_project.protected_branches.find_by!(name: params[:name])
 
-        protected_branch.destroy
-
-        status 204
+        destroy_conditionally!(protected_branch)
       end
     end
   end
diff --git a/lib/api/runner.rb b/lib/api/runner.rb
index 1a7ded31c91..11999354594 100644
--- a/lib/api/runner.rb
+++ b/lib/api/runner.rb
@@ -45,8 +45,10 @@ module API
       end
       delete '/' do
         authenticate_runner!
-        status 204
-        Ci::Runner.find_by_token(params[:token]).destroy
+
+        runner = Ci::Runner.find_by_token(params[:token])
+
+        destroy_conditionally!(runner)
       end
 
       desc 'Validates authentication credentials' do
diff --git a/lib/api/runners.rb b/lib/api/runners.rb
index 31f940fe96b..68c2120cc15 100644
--- a/lib/api/runners.rb
+++ b/lib/api/runners.rb
@@ -77,10 +77,10 @@ module API
       end
       delete ':id' do
         runner = get_runner(params[:id])
+
         authenticate_delete_runner!(runner)
 
-        status 204
-        runner.destroy!
+        destroy_conditionally!(runner)
       end
     end
 
@@ -135,8 +135,7 @@ module API
         runner = runner_project.runner
         forbidden!("Only one project associated with the runner. Please remove the runner instead") if runner.projects.count == 1
 
-        status 204
-        runner_project.destroy
+        destroy_conditionally!(runner_project)
       end
     end
 
diff --git a/lib/api/services.rb b/lib/api/services.rb
index 843c05ae32e..ff9ddd44439 100644
--- a/lib/api/services.rb
+++ b/lib/api/services.rb
@@ -656,12 +656,14 @@ module API
       delete ":id/services/:service_slug" do
         service = user_project.find_or_initialize_service(params[:service_slug].underscore)
 
-        attrs = service_attributes(service).inject({}) do |hash, key|
-          hash.merge!(key => nil)
-        end
+        destroy_conditionally!(service) do
+          attrs = service_attributes(service).inject({}) do |hash, key|
+            hash.merge!(key => nil)
+          end
 
-        unless service.update_attributes(attrs.merge(active: false))
-          render_api_error!('400 Bad Request', 400)
+          unless service.update_attributes(attrs.merge(active: false))
+            render_api_error!('400 Bad Request', 400)
+          end
         end
       end
 
diff --git a/lib/api/snippets.rb b/lib/api/snippets.rb
index 35ece56c65c..00eb7c60f16 100644
--- a/lib/api/snippets.rb
+++ b/lib/api/snippets.rb
@@ -123,8 +123,7 @@ module API
 
         authorize! :destroy_personal_snippet, snippet
 
-        status 204
-        snippet.destroy
+        destroy_conditionally!(snippet)
       end
 
       desc 'Get a raw snippet' do
diff --git a/lib/api/system_hooks.rb b/lib/api/system_hooks.rb
index c0179037440..6b6a03e3300 100644
--- a/lib/api/system_hooks.rb
+++ b/lib/api/system_hooks.rb
@@ -66,8 +66,7 @@ module API
         hook = SystemHook.find_by(id: params[:id])
         not_found!('System hook') unless hook
 
-        status 204
-        hook.destroy
+        destroy_conditionally!(hook)
       end
     end
   end
diff --git a/lib/api/tags.rb b/lib/api/tags.rb
index 1333747cced..81b17935b81 100644
--- a/lib/api/tags.rb
+++ b/lib/api/tags.rb
@@ -65,11 +65,18 @@ module API
       delete ':id/repository/tags/:tag_name', requirements: TAG_ENDPOINT_REQUIREMENTS do
         authorize_push_project
 
-        result = ::Tags::DestroyService.new(user_project, current_user)
-          .execute(params[:tag_name])
+        tag = user_project.repository.find_tag(params[:tag_name])
+        not_found!('Tag') unless tag
+
+        commit = user_project.repository.commit(tag.dereferenced_target)
+
+        destroy_conditionally!(commit, last_update_field: :authored_date) do
+          result = ::Tags::DestroyService.new(user_project, current_user)
+                    .execute(params[:tag_name])
 
-        if result[:status] != :success
-          render_api_error!(result[:message], result[:return_code])
+          if result[:status] != :success
+            render_api_error!(result[:message], result[:return_code])
+          end
         end
       end
 
diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb
index edfdb63d183..c9fee7e5193 100644
--- a/lib/api/triggers.rb
+++ b/lib/api/triggers.rb
@@ -140,8 +140,7 @@ module API
         trigger = user_project.triggers.find(params.delete(:trigger_id))
         return not_found!('Trigger') unless trigger
 
-        status 204
-        trigger.destroy
+        destroy_conditionally!(trigger)
       end
     end
   end
diff --git a/lib/api/users.rb b/lib/api/users.rb
index e2019d6d512..96f47bb618a 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -230,8 +230,7 @@ module API
         key = user.keys.find_by(id: params[:key_id])
         not_found!('Key') unless key
 
-        status 204
-        key.destroy
+        destroy_conditionally!(key)
       end
 
       desc 'Add an email address to a specified user. Available only for admins.' do
@@ -287,7 +286,11 @@ module API
         email = user.emails.find_by(id: params[:email_id])
         not_found!('Email') unless email
 
-        Emails::DestroyService.new(user, email: email.email).execute
+        destroy_conditionally!(email) do |email|
+          Emails::DestroyService.new(current_user, email: email.email).execute
+        end
+
+        user.update_secondary_emails!
       end
 
       desc 'Delete a user. Available only for admins.' do
@@ -299,11 +302,13 @@ module API
       end
       delete ":id" do
         authenticated_as_admin!
+
         user = User.find_by(id: params[:id])
         not_found!('User') unless user
 
-        status 204
-        user.delete_async(deleted_by: current_user, params: params)
+        destroy_conditionally!(user) do
+          user.delete_async(deleted_by: current_user, params: params)
+        end
       end
 
       desc 'Block a user. Available only for admins.'
@@ -403,8 +408,11 @@ module API
             requires :impersonation_token_id, type: Integer, desc: 'The ID of the impersonation token'
           end
           delete ':impersonation_token_id' do
-            status 204
-            find_impersonation_token.revoke!
+            token = find_impersonation_token
+
+            destroy_conditionally!(token) do
+              token.revoke!
+            end
           end
         end
       end
@@ -481,8 +489,7 @@ module API
         key = current_user.keys.find_by(id: params[:key_id])
         not_found!('Key') unless key
 
-        status 204
-        key.destroy
+        destroy_conditionally!(key)
       end
 
       desc "Get the currently authenticated user's email addresses" do
@@ -533,8 +540,11 @@ module API
         email = current_user.emails.find_by(id: params[:email_id])
         not_found!('Email') unless email
 
-        status 204
-        Emails::DestroyService.new(current_user, email: email.email).execute
+        destroy_conditionally!(email) do |email|
+          Emails::DestroyService.new(current_user, email: email.email).execute
+        end
+
+        current_user.update_secondary_emails!
       end
 
       desc 'Get a list of user activities'
diff --git a/lib/api/variables.rb b/lib/api/variables.rb
index 7c0fdd3d1be..da71787abab 100644
--- a/lib/api/variables.rb
+++ b/lib/api/variables.rb
@@ -88,6 +88,7 @@ module API
         variable = user_project.variables.find_by(key: params[:key])
         not_found!('Variable') unless variable
 
+        # Variables don't have any timestamp. Therfore, destroy unconditionally.
         status 204
         variable.destroy
       end