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

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

See merge request !9621

changelogs/unreleased/api-delete-respect-headers.yml

@@ -0,0 +1,5 @@
+---
+title: 'API: Respect the "If-Unmodified-Since" header when delting a resource'
+merge_request: 9621
+author: Robert Schilling
+type: added

doc/api/README.md

@@ -263,6 +263,7 @@ The following table shows the possible return codes for API requests.
 | `404 Not Found` | A resource could not be accessed, e.g., an ID for a resource could not be found. |
 | `405 Method Not Allowed` | The request is not supported. |
 | `409 Conflict` | A conflicting resource already exists, e.g., creating a project with a name that already exists. |
+| `412` | Indicates the request was denied. May happen if the `If-Unmodified-Since` header is provided when trying to delete a resource, which was modified in between. |
 | `422 Unprocessable` | The entity could not be processed. |
 | `500 Server Error` | While handling the request something went wrong server-side. |
 

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
+          destroy_conditionally!(member) do
-          ::Members::DestroyService.new(source, current_user, params)
+            ::Members::DestroyService.new(source, current_user, params)
-            .execute(:requesters)
+              .execute(:requesters)
+          end
         end
       end
     end

lib/api/award_emoji.rb

@@ -88,8 +88,7 @@ module API
 
             unauthorized! unless award.user == current_user || current_user.admin?
 
-            status 204
+            destroy_conditionally!(award)
-            award.destroy
           end
         end
       end

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)
+          destroy_conditionally!(list) do |list|
-
+            service = ::Boards::Lists::DestroyService.new(user_project, current_user)
-          unless service.execute(list)
+            unless service.execute(list)
-            render_api_error!({ error: 'List could not be deleted!' }, 400)
+              render_api_error!({ error: 'List could not be deleted!' }, 400)
+            end
           end
         end
       end

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)
+        branch = user_project.repository.find_branch(params[:branch])
-                 .execute(params[:branch])
+        not_found!('Branch') unless branch
 
-        if result[:status] != :success
+        commit = user_project.repository.commit(branch.dereferenced_target)
-          render_api_error!(result[:message], result[:return_code])
+
+        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])
+          end
         end
       end
 

lib/api/broadcast_messages.rb

@@ -91,8 +91,7 @@ module API
       delete ':id' do
         message = find_message
 
-        status 204
+        destroy_conditionally!(message)
-        message.destroy
       end
     end
   end

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
+        destroy_conditionally!(key)
-        key.destroy
       end
     end
   end

lib/api/environments.rb

@@ -79,8 +79,7 @@ module API
 
         environment = user_project.environments.find(params[:environment_id])
 
-        status 204
+        destroy_conditionally!(environment)
-        environment.destroy
       end
 
       desc 'Stops an existing environment' do

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
+        destroy_conditionally!(variable)
-        variable.destroy
       end
     end
   end

lib/api/groups.rb

@@ -118,8 +118,9 @@ module API
         group = find_group!(params[:id])
         authorize! :admin_group, group
 
-        status 204
+        destroy_conditionally!(group) do |group|
-        ::Groups::DestroyService.new(group, current_user).execute
+          ::Groups::DestroyService.new(group, current_user).execute
+        end
       end
 
       desc 'Get a list of projects in this group.' do

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)
 

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

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
+        destroy_conditionally!(label)
-        label.destroy
       end
 
       desc 'Update an existing label. At least one optional parameter is required.' do

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
+          member = source.members.find_by!(user_id: params[:user_id])
-          source.members.find_by!(user_id: params[:user_id])
 
-          status 204
+          destroy_conditionally!(member) do
-          ::Members::DestroyService.new(source, current_user, declared_params).execute
+            ::Members::DestroyService.new(source, current_user, declared_params).execute
+          end
         end
       end
     end

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

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
+          destroy_conditionally!(note) do |note|
-          ::Notes::DestroyService.new(user_project, current_user).execute(note)
+            ::Notes::DestroyService.new(user_project, current_user).execute(note)
+          end
         end
       end
     end

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
+        destroy_conditionally!(pipeline_schedule)
-        present pipeline_schedule.destroy, with: Entities::PipelineScheduleDetails
       end
     end
 

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
+        destroy_conditionally!(hook)
-        hook.destroy
       end
     end
   end

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'

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
+          destroy_conditionally!(user_project.forked_project_link)
-          user_project.forked_project_link.destroy
         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
+        destroy_conditionally!(link)
-        link.destroy
       end
 
       desc 'Upload a file'

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
+        destroy_conditionally!(protected_branch)
-
-        status 204
       end
     end
   end

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

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
+        destroy_conditionally!(runner)
-        runner.destroy!
       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
+        destroy_conditionally!(runner_project)
-        runner_project.destroy
       end
     end
 

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|
+        destroy_conditionally!(service) do
-          hash.merge!(key => nil)
+          attrs = service_attributes(service).inject({}) do |hash, key|
-        end
+            hash.merge!(key => nil)
+          end
 
-        unless service.update_attributes(attrs.merge(active: false))
+          unless service.update_attributes(attrs.merge(active: false))
-          render_api_error!('400 Bad Request', 400)
+            render_api_error!('400 Bad Request', 400)
+          end
         end
       end
 

lib/api/snippets.rb

@@ -123,8 +123,7 @@ module API
 
         authorize! :destroy_personal_snippet, snippet
 
-        status 204
+        destroy_conditionally!(snippet)
-        snippet.destroy
       end
 
       desc 'Get a raw snippet' do

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
+        destroy_conditionally!(hook)
-        hook.destroy
       end
     end
   end

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)
+        tag = user_project.repository.find_tag(params[:tag_name])
-          .execute(params[:tag_name])
+        not_found!('Tag') unless tag
 
-        if result[:status] != :success
+        commit = user_project.repository.commit(tag.dereferenced_target)
-          render_api_error!(result[:message], result[:return_code])
+
+        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])
+          end
         end
       end
 

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
+        destroy_conditionally!(trigger)
-        trigger.destroy
       end
     end
   end

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
+        destroy_conditionally!(key)
-        key.destroy
       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
+        destroy_conditionally!(user) do
-        user.delete_async(deleted_by: current_user, params: params)
+          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
+            token = find_impersonation_token
-            find_impersonation_token.revoke!
+
+            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
+        destroy_conditionally!(key)
-        key.destroy
       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
+        destroy_conditionally!(email) do |email|
-        Emails::DestroyService.new(current_user, email: email.email).execute
+          Emails::DestroyService.new(current_user, email: email.email).execute
+        end
+
+        current_user.update_secondary_emails!
       end
 
       desc 'Get a list of user activities'

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

spec/requests/api/award_emoji_spec.rb

@@ -253,6 +253,10 @@ describe API::AwardEmoji do
 
         expect(response).to have_http_status(404)
       end
+
+      it_behaves_like '412 response' do
+        let(:request) { api("/projects/#{project.id}/issues/#{issue.iid}/award_emoji/#{award_emoji.id}", user) }
+      end
     end
 
     context 'when the awardable is a Merge Request' do
@@ -269,6 +273,10 @@ describe API::AwardEmoji do
 
         expect(response).to have_http_status(404)
       end
+
+      it_behaves_like '412 response' do
+        let(:request) { api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/award_emoji/#{downvote.id}", user) }
+      end
     end
 
     context 'when the awardable is a Snippet' do
@@ -282,6 +290,10 @@ describe API::AwardEmoji do
           expect(response).to have_http_status(204)
         end.to change { snippet.award_emoji.count }.from(1).to(0)
       end
+
+      it_behaves_like '412 response' do
+        let(:request) { api("/projects/#{project.id}/snippets/#{snippet.id}/award_emoji/#{award.id}", user) }
+      end
     end
   end
 
@@ -295,5 +307,9 @@ describe API::AwardEmoji do
         expect(response).to have_http_status(204)
       end.to change { note.award_emoji.count }.from(1).to(0)
     end
+
+    it_behaves_like '412 response' do
+      let(:request) { api("/projects/#{project.id}/issues/#{issue.iid}/notes/#{note.id}/award_emoji/#{rocket.id}", user) }
+    end
   end
 end

spec/requests/api/boards_spec.rb

@@ -195,6 +195,10 @@ describe API::Boards do
 
         expect(response).to have_http_status(204)
       end
+
+      it_behaves_like '412 response' do
+        let(:request) { api("#{base_url}/#{dev_list.id}", owner) }
+      end
     end
   end
 end

spec/requests/api/branches_spec.rb

@@ -499,6 +499,10 @@ describe API::Branches do
 
       expect(response).to have_gitlab_http_status(404)
     end
+
+    it_behaves_like '412 response' do
+      let(:request) { api("/projects/#{project.id}/repository/branches/#{branch_name}", user) }
+    end
   end
 
   describe 'DELETE /projects/:id/repository/merged_branches' do

spec/requests/api/broadcast_messages_spec.rb

@@ -171,6 +171,10 @@ describe API::BroadcastMessages do
       expect(response).to have_http_status(403)
     end
 
+    it_behaves_like '412 response' do
+      let(:request) { api("/broadcast_messages/#{message.id}", admin) }
+    end
+
     it 'deletes the broadcast message for admins' do
       expect do
         delete api("/broadcast_messages/#{message.id}", admin)

spec/requests/api/deploy_keys_spec.rb

@@ -190,6 +190,10 @@ describe API::DeployKeys do
 
       expect(response).to have_http_status(404)
     end
+
+    it_behaves_like '412 response' do
+      let(:request) { api("/projects/#{project.id}/deploy_keys/#{deploy_key.id}", admin) }
+    end
   end
 
   describe 'POST /projects/:id/deploy_keys/:key_id/enable' do

spec/requests/api/environments_spec.rb

@@ -138,6 +138,10 @@ describe API::Environments do
         expect(response).to have_http_status(404)
         expect(json_response['message']).to eq('404 Not found')
       end
+
+      it_behaves_like '412 response' do
+        let(:request) { api("/projects/#{project.id}/environments/#{environment.id}", user) }
+      end
     end
 
     context 'a non member' do

spec/requests/api/group_variables_spec.rb

@@ -200,6 +200,10 @@ describe API::GroupVariables do
 
         expect(response).to have_http_status(404)
       end
+
+      it_behaves_like '412 response' do
+        let(:request) { api("/groups/#{group.id}/variables/#{variable.key}", user) }
+      end
     end
 
     context 'authorized user with invalid permissions' do

spec/requests/api/groups_spec.rb

@@ -488,6 +488,10 @@ describe API::Groups do
         expect(response).to have_http_status(204)
       end
 
+      it_behaves_like '412 response' do
+        let(:request) { api("/groups/#{group1.id}", user1) }
+      end
+
       it "does not remove a group if not an owner" do
         user4 = create(:user)
         group1.add_master(user4)

spec/requests/api/issues_spec.rb

@@ -1304,6 +1304,10 @@ describe API::Issues, :mailer do
 
         expect(response).to have_http_status(204)
       end
+
+      it_behaves_like '412 response' do
+        let(:request) { api("/projects/#{project.id}/issues/#{issue.iid}", owner) }
+      end
     end
 
     context 'when issue does not exist' do

spec/requests/api/labels_spec.rb

@@ -189,6 +189,11 @@ describe API::Labels do
       delete api("/projects/#{project.id}/labels", user)
       expect(response).to have_http_status(400)
     end
+
+    it_behaves_like '412 response' do
+      let(:request) { api("/projects/#{project.id}/labels", user) }
+      let(:params) { { name: 'label1' } }
+    end
   end
 
   describe 'PUT /projects/:id/labels' do

spec/requests/api/members_spec.rb

@@ -284,6 +284,10 @@ describe API::Members do
             expect(response).to have_http_status(204)
           end.to change { source.members.count }.by(-1)
         end
+
+        it_behaves_like '412 response' do
+          let(:request) { api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", master) }
+        end
       end
 
       it 'returns 404 if member does not exist' do

spec/requests/api/merge_requests_spec.rb

@@ -698,6 +698,10 @@ describe API::MergeRequests do
 
         expect(response).to have_gitlab_http_status(404)
       end
+
+      it_behaves_like '412 response' do
+        let(:request) { api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user) }
+      end
     end
   end
 

spec/requests/api/notes_spec.rb

@@ -390,6 +390,10 @@ describe API::Notes do
 
         expect(response).to have_http_status(404)
       end
+
+      it_behaves_like '412 response' do
+        let(:request) { api("/projects/#{project.id}/issues/#{issue.iid}/notes/#{issue_note.id}", user) }
+      end
     end
 
     context 'when noteable is a Snippet' do
@@ -410,6 +414,10 @@ describe API::Notes do
 
         expect(response).to have_http_status(404)
       end
+
+      it_behaves_like '412 response' do
+        let(:request) { api("/projects/#{project.id}/snippets/#{snippet.id}/notes/#{snippet_note.id}", user) }
+      end
     end
 
     context 'when noteable is a Merge Request' do
@@ -430,6 +438,10 @@ describe API::Notes do
 
         expect(response).to have_http_status(404)
       end
+
+      it_behaves_like '412 response' do
+        let(:request) { api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/notes/#{merge_request_note.id}", user) }
+      end
     end
   end
 end

spec/requests/api/pipeline_schedules_spec.rb

@@ -267,8 +267,7 @@ describe API::PipelineSchedules do
           delete api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", master)
         end.to change { project.pipeline_schedules.count }.by(-1)
 
-        expect(response).to have_http_status(:accepted)
+        expect(response).to have_http_status(204)
-        expect(response).to match_response_schema('pipeline_schedule')
       end
 
       it 'responds with 404 Not Found if requesting non-existing pipeline_schedule' do
@@ -276,6 +275,10 @@ describe API::PipelineSchedules do
 
         expect(response).to have_http_status(:not_found)
       end
+
+      it_behaves_like '412 response' do
+        let(:request) { api("/projects/#{project.id}/pipeline_schedules/#{pipeline_schedule.id}", master) }
+      end
     end
 
     context 'authenticated user with invalid permissions' do

spec/requests/api/project_hooks_spec.rb

@@ -212,5 +212,9 @@ describe API::ProjectHooks, 'ProjectHooks' do
       expect(response).to have_http_status(404)
       expect(WebHook.exists?(hook.id)).to be_truthy
     end
+
+    it_behaves_like '412 response' do
+      let(:request) { api("/projects/#{project.id}/hooks/#{hook.id}", user) }
+    end
   end
 end

spec/requests/api/project_snippets_spec.rb

@@ -228,9 +228,6 @@ describe API::ProjectSnippets do
     let(:snippet) { create(:project_snippet, author: admin) }
 
     it 'deletes snippet' do
-      admin = create(:admin)
-      snippet = create(:project_snippet, author: admin)
-
       delete api("/projects/#{snippet.project.id}/snippets/#{snippet.id}/", admin)
 
       expect(response).to have_http_status(204)
@@ -242,6 +239,10 @@ describe API::ProjectSnippets do
       expect(response).to have_http_status(404)
       expect(json_response['message']).to eq('404 Snippet Not Found')
     end
+
+    it_behaves_like '412 response' do
+      let(:request) { api("/projects/#{snippet.project.id}/snippets/#{snippet.id}/", admin) }
+    end
   end
 
   describe 'GET /projects/:project_id/snippets/:id/raw' do

spec/requests/api/projects_spec.rb

@@ -1029,6 +1029,10 @@ describe API::Projects do
       delete api("/projects/#{project.id}/snippets/1234", user)
       expect(response).to have_http_status(404)
     end
+
+    it_behaves_like '412 response' do
+      let(:request) { api("/projects/#{project.id}/snippets/#{snippet.id}", user) }
+    end
   end
 
   describe 'GET /projects/:id/snippets/:snippet_id/raw' do
@@ -1104,25 +1108,33 @@ describe API::Projects do
           project_fork_target.group.add_developer user2
         end
 
+        context 'for a forked project' do
+          before do
+            post api("/projects/#{project_fork_target.id}/fork/#{project_fork_source.id}", admin)
+            project_fork_target.reload
+            expect(project_fork_target.forked_from_project).not_to be_nil
+            expect(project_fork_target.forked?).to be_truthy
+          end
+
+          it 'makes forked project unforked' do
+            delete api("/projects/#{project_fork_target.id}/fork", admin)
+
+            expect(response).to have_http_status(204)
+            project_fork_target.reload
+            expect(project_fork_target.forked_from_project).to be_nil
+            expect(project_fork_target.forked?).not_to be_truthy
+          end
+
+          it_behaves_like '412 response' do
+            let(:request) { api("/projects/#{project_fork_target.id}/fork", admin) }
+          end
+        end
+
         it 'is forbidden to non-owner users' do
           delete api("/projects/#{project_fork_target.id}/fork", user2)
           expect(response).to have_http_status(403)
         end
 
-        it 'makes forked project unforked' do
-          post api("/projects/#{project_fork_target.id}/fork/#{project_fork_source.id}", admin)
-          project_fork_target.reload
-          expect(project_fork_target.forked_from_project).not_to be_nil
-          expect(project_fork_target.forked?).to be_truthy
-
-          delete api("/projects/#{project_fork_target.id}/fork", admin)
-
-          expect(response).to have_http_status(204)
-          project_fork_target.reload
-          expect(project_fork_target.forked_from_project).to be_nil
-          expect(project_fork_target.forked?).not_to be_truthy
-        end
-
         it 'is idempotent if not forked' do
           expect(project_fork_target.forked_from_project).to be_nil
           delete api("/projects/#{project_fork_target.id}/fork", admin)
@@ -1188,14 +1200,23 @@ describe API::Projects do
   end
 
   describe 'DELETE /projects/:id/share/:group_id' do
-    it 'returns 204 when deleting a group share' do
+    context 'for a valid group' do
-      group = create(:group, :public)
+      let(:group) { create(:group, :public) }
-      create(:project_group_link, group: group, project: project)
 
-      delete api("/projects/#{project.id}/share/#{group.id}", user)
+      before do
+        create(:project_group_link, group: group, project: project)
+      end
 
-      expect(response).to have_http_status(204)
+      it 'returns 204 when deleting a group share' do
-      expect(project.project_group_links).to be_empty
+        delete api("/projects/#{project.id}/share/#{group.id}", user)
+
+        expect(response).to have_http_status(204)
+        expect(project.project_group_links).to be_empty
+      end
+
+      it_behaves_like '412 response' do
+        let(:request) { api("/projects/#{project.id}/share/#{group.id}", user) }
+      end
     end
 
     it 'returns a 400 when group id is not an integer' do
@@ -1519,6 +1540,11 @@ describe API::Projects do
         expect(json_response['message']).to eql('202 Accepted')
       end
 
+      it_behaves_like '412 response' do
+        let(:success_status) { 202 }
+        let(:request) { api("/projects/#{project.id}", user) }
+      end
+
       it 'does not remove a project if not an owner' do
         user3 = create(:user)
         project.team << [user3, :developer]
@@ -1549,6 +1575,11 @@ describe API::Projects do
         delete api('/projects/1328', admin)
         expect(response).to have_http_status(404)
       end
+
+      it_behaves_like '412 response' do
+        let(:success_status) { 202 }
+        let(:request) { api("/projects/#{project.id}", admin) }
+      end
     end
   end
 

spec/requests/api/protected_branches_spec.rb

@@ -213,6 +213,10 @@ describe API::ProtectedBranches do
       expect(response).to have_gitlab_http_status(204)
     end
 
+    it_behaves_like '412 response' do
+      let(:request) { api("/projects/#{project.id}/protected_branches/#{branch_name}", user) }
+    end
+
     it "returns 404 if branch does not exist" do
       delete api("/projects/#{project.id}/protected_branches/barfoo", user)
 

spec/requests/api/runner_spec.rb

@@ -149,6 +149,11 @@ describe API::Runner do
           expect(response).to have_http_status 204
           expect(Ci::Runner.count).to eq(0)
         end
+
+        it_behaves_like '412 response' do
+          let(:request) { api('/runners') }
+          let(:params) { { token: runner.token } }
+        end
       end
     end
 

spec/requests/api/runners_spec.rb

@@ -279,6 +279,10 @@ describe API::Runners do
             expect(response).to have_http_status(204)
           end.to change { Ci::Runner.shared.count }.by(-1)
         end
+
+        it_behaves_like '412 response' do
+          let(:request) { api("/runners/#{shared_runner.id}", admin) }
+        end
       end
 
       context 'when runner is not shared' do
@@ -332,6 +336,10 @@ describe API::Runners do
             expect(response).to have_http_status(204)
           end.to change { Ci::Runner.specific.count }.by(-1)
         end
+
+        it_behaves_like '412 response' do
+          let(:request) { api("/runners/#{specific_runner.id}", user) }
+        end
       end
     end
 
@@ -463,6 +471,10 @@ describe API::Runners do
             expect(response).to have_http_status(204)
           end.to change { project.runners.count }.by(-1)
         end
+
+        it_behaves_like '412 response' do
+          let(:request) { api("/projects/#{project.id}/runners/#{two_projects_runner.id}", user) }
+        end
       end
 
       context 'when runner have one associated projects' do

spec/requests/api/snippets_spec.rb

@@ -270,6 +270,10 @@ describe API::Snippets do
       expect(response).to have_http_status(404)
       expect(json_response['message']).to eq('404 Snippet Not Found')
     end
+
+    it_behaves_like '412 response' do
+      let(:request) { api("/snippets/#{public_snippet.id}", user) }
+    end
   end
 
   describe "GET /snippets/:id/user_agent_detail" do

spec/requests/api/system_hooks_spec.rb

@@ -102,5 +102,9 @@ describe API::SystemHooks do
 
       expect(response).to have_http_status(404)
     end
+
+    it_behaves_like '412 response' do
+      let(:request) { api("/hooks/#{hook.id}", admin) }
+    end
   end
 end

spec/requests/api/tags_spec.rb

@@ -278,12 +278,16 @@ describe API::Tags do
         expect(response).to have_gitlab_http_status(204)
       end
 
+      it_behaves_like '412 response' do
+        let(:request) { api(route, current_user) }
+      end
+
       context 'when tag does not exist' do
         let(:tag_name) { 'unknown' }
 
         it_behaves_like '404 response' do
           let(:request) { delete api(route, current_user) }
-          let(:message) { 'No such tag' }
+          let(:message) { '404 Tag Not Found' }
         end
       end
 

spec/requests/api/triggers_spec.rb

@@ -309,6 +309,10 @@ describe API::Triggers do
 
         expect(response).to have_http_status(404)
       end
+
+      it_behaves_like '412 response' do
+        let(:request) { api("/projects/#{project.id}/triggers/#{trigger.id}", user) }
+      end
     end
 
     context 'authenticated user with invalid permissions' do

spec/requests/api/users_spec.rb

@@ -733,6 +733,10 @@ describe API::Users do
         end.to change { user.keys.count }.by(-1)
       end
 
+      it_behaves_like '412 response' do
+        let(:request) { api("/users/#{user.id}/keys/#{key.id}", admin) }
+      end
+
       it 'returns 404 error if user not found' do
         user.keys << key
         user.save
@@ -838,6 +842,10 @@ describe API::Users do
         end.to change { user.emails.count }.by(-1)
       end
 
+      it_behaves_like '412 response' do
+        let(:request) { api("/users/#{user.id}/emails/#{email.id}", admin) }
+      end
+
       it 'returns 404 error if user not found' do
         user.emails << email
         user.save
@@ -876,6 +884,10 @@ describe API::Users do
       expect { Namespace.find(namespace.id) }.to raise_error ActiveRecord::RecordNotFound
     end
 
+    it_behaves_like '412 response' do
+      let(:request) { api("/users/#{user.id}", admin) }
+    end
+
     it "does not delete for unauthenticated user" do
       Sidekiq::Testing.inline! { delete api("/users/#{user.id}") }
       expect(response).to have_http_status(401)
@@ -1116,6 +1128,10 @@ describe API::Users do
       end.to change { user.keys.count}.by(-1)
     end
 
+    it_behaves_like '412 response' do
+      let(:request) { api("/user/keys/#{key.id}", user) }
+    end
+
     it "returns 404 if key ID not found" do
       delete api("/user/keys/42", user)
 
@@ -1239,6 +1255,10 @@ describe API::Users do
       end.to change { user.emails.count}.by(-1)
     end
 
+    it_behaves_like '412 response' do
+      let(:request) { api("/user/emails/#{email.id}", user) }
+    end
+
     it "returns 404 if email ID not found" do
       delete api("/user/emails/42", user)
 
@@ -1551,6 +1571,10 @@ describe API::Users do
       expect(json_response['message']).to eq('403 Forbidden')
     end
 
+    it_behaves_like '412 response' do
+      let(:request) { api("/users/#{user.id}/impersonation_tokens/#{impersonation_token.id}", admin) }
+    end
+
     it 'revokes a impersonation token' do
       delete api("/users/#{user.id}/impersonation_tokens/#{impersonation_token.id}", admin)
 

spec/support/shared_examples/requests/api/status_shared_examples.rb

@@ -40,3 +40,28 @@ shared_examples_for '404 response' do
     end
   end
 end
+
+shared_examples_for '412 response' do
+  let(:params) { nil }
+  let(:success_status) { 204 }
+
+  context 'for a modified ressource' do
+    before do
+      delete request, params, { 'HTTP_IF_UNMODIFIED_SINCE' => '1990-01-12T00:00:48-0600' }
+    end
+
+    it 'returns 412' do
+      expect(response).to have_gitlab_http_status(412)
+    end
+  end
+
+  context 'for an unmodified ressource' do
+    before do
+      delete request, params, { 'HTTP_IF_UNMODIFIED_SINCE' => Time.now }
+    end
+
+    it 'returns accepted' do
+      expect(response).to have_gitlab_http_status(success_status)
+    end
+  end
+end