From 998afa5f74558be215a924d95aa131a69831ca43 Mon Sep 17 00:00:00 2001
From: Robert Schilling <rschilling@student.tugraz.at>
Date: Wed, 1 Mar 2017 14:35:48 +0100
Subject: [PATCH 1/7] API: Respect the 'If-Unmodified-Since' for delete
 endpoints

---
 lib/api/access_requests.rb    |  3 +++
 lib/api/award_emoji.rb        |  1 +
 lib/api/boards.rb             |  1 +
 lib/api/broadcast_messages.rb |  1 +
 lib/api/deploy_keys.rb        |  2 ++
 lib/api/environments.rb       |  1 +
 lib/api/groups.rb             |  2 ++
 lib/api/helpers.rb            |  8 ++++++++
 lib/api/issues.rb             |  2 ++
 lib/api/labels.rb             |  2 ++
 lib/api/members.rb            |  3 ++-
 lib/api/merge_requests.rb     |  2 ++
 lib/api/notes.rb              |  2 ++
 lib/api/project_hooks.rb      |  2 ++
 lib/api/project_snippets.rb   |  2 ++
 lib/api/projects.rb           |  2 ++
 lib/api/runners.rb            |  2 ++
 lib/api/services.rb           |  2 ++
 lib/api/snippets.rb           |  1 +
 lib/api/system_hooks.rb       |  2 ++
 lib/api/triggers.rb           |  2 ++
 lib/api/users.rb              | 28 ++++++++++++++++++++++++++++
 22 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb
index cdacf9839e5..0c5b8862d79 100644
--- a/lib/api/access_requests.rb
+++ b/lib/api/access_requests.rb
@@ -67,6 +67,9 @@ module API
         end
         delete ":id/access_requests/:user_id" do
           source = find_source(source_type, params[:id])
+          member = source.public_send(:requesters).find_by!(user_id: params[:user_id])
+
+          check_unmodified_since(member.updated_at)
 
           status 204
           ::Members::DestroyService.new(source, current_user, params)
diff --git a/lib/api/award_emoji.rb b/lib/api/award_emoji.rb
index 5a028fc9d0b..51a8587d26e 100644
--- a/lib/api/award_emoji.rb
+++ b/lib/api/award_emoji.rb
@@ -85,6 +85,7 @@ module API
           end
           delete "#{endpoint}/:award_id" do
             award = awardable.award_emoji.find(params[:award_id])
+            check_unmodified_since(award.updated_at)
 
             unauthorized! unless award.user == current_user || current_user.admin?
 
diff --git a/lib/api/boards.rb b/lib/api/boards.rb
index 5a2d7a681e3..d36df77dc6c 100644
--- a/lib/api/boards.rb
+++ b/lib/api/boards.rb
@@ -124,6 +124,7 @@ module API
           authorize!(:admin_list, user_project)
 
           list = board_lists.find(params[:list_id])
+          check_unmodified_since(list.updated_at)
 
           service = ::Boards::Lists::DestroyService.new(user_project, current_user)
 
diff --git a/lib/api/broadcast_messages.rb b/lib/api/broadcast_messages.rb
index 9980aec4752..352972b584a 100644
--- a/lib/api/broadcast_messages.rb
+++ b/lib/api/broadcast_messages.rb
@@ -90,6 +90,7 @@ module API
       end
       delete ':id' do
         message = find_message
+        check_unmodified_since(message.updated_at)
 
         status 204
         message.destroy
diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb
index 42e7c1486b0..971cc816454 100644
--- a/lib/api/deploy_keys.rb
+++ b/lib/api/deploy_keys.rb
@@ -125,6 +125,8 @@ module API
         key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id])
         not_found!('Deploy Key') unless key
 
+        check_unmodified_since(key.updated_at)
+
         status 204
         key.destroy
       end
diff --git a/lib/api/environments.rb b/lib/api/environments.rb
index c774a5c6685..3fc423ae79a 100644
--- a/lib/api/environments.rb
+++ b/lib/api/environments.rb
@@ -78,6 +78,7 @@ module API
         authorize! :update_environment, user_project
 
         environment = user_project.environments.find(params[:environment_id])
+        check_unmodified_since(environment.updated_at)
 
         status 204
         environment.destroy
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index e56427304a6..c9b32a85487 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -117,6 +117,8 @@ module API
       delete ":id" do
         group = find_group!(params[:id])
         authorize! :admin_group, group
+        
+        check_unmodified_since(group.updated_at)
 
         status 204
         ::Groups::DestroyService.new(group, current_user).execute
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index b56fd2388b3..1c74a14d91c 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -11,6 +11,14 @@ 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']) if headers.key?('If-Unmodified-Since') rescue nil
+
+      if if_unmodified_since && if_unmodified_since < last_modified
+        render_api_error!('412 Precondition Failed', 412)
+      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..cee9898d3a6 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -230,6 +230,8 @@ module API
         not_found!('Issue') unless issue
 
         authorize!(:destroy_issue, issue)
+        check_unmodified_since(issue.updated_at)
+
         status 204
         issue.destroy
       end
diff --git a/lib/api/labels.rb b/lib/api/labels.rb
index 4520c98d951..45fa57fdf55 100644
--- a/lib/api/labels.rb
+++ b/lib/api/labels.rb
@@ -56,6 +56,8 @@ module API
         label = user_project.labels.find_by(title: params[:name])
         not_found!('Label') unless label
 
+        check_unmodified_since(label.updated_at)
+
         status 204
         label.destroy
       end
diff --git a/lib/api/members.rb b/lib/api/members.rb
index bb970b7cd54..5634f123eca 100644
--- a/lib/api/members.rb
+++ b/lib/api/members.rb
@@ -94,7 +94,8 @@ module API
         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])
+          check_unmodified_since(member.updated_at)
 
           status 204
           ::Members::DestroyService.new(source, current_user, declared_params).execute
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index 8810d4e441d..c6fecc1aa6c 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -164,6 +164,8 @@ module API
         merge_request = find_project_merge_request(params[:merge_request_iid])
 
         authorize!(:destroy_merge_request, merge_request)
+        check_unmodified_since(merge_request.updated_at)
+
         status 204
         merge_request.destroy
       end
diff --git a/lib/api/notes.rb b/lib/api/notes.rb
index 4e4e473994b..58d71787aca 100644
--- a/lib/api/notes.rb
+++ b/lib/api/notes.rb
@@ -129,7 +129,9 @@ 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
+          check_unmodified_since(note.updated_at)
 
           status 204
           ::Notes::DestroyService.new(user_project, current_user).execute(note)
diff --git a/lib/api/project_hooks.rb b/lib/api/project_hooks.rb
index 649dd891f56..74d736fda59 100644
--- a/lib/api/project_hooks.rb
+++ b/lib/api/project_hooks.rb
@@ -96,6 +96,8 @@ module API
       delete ":id/hooks/:hook_id" do
         hook = user_project.hooks.find(params.delete(:hook_id))
 
+        check_unmodified_since(hook.updated_at)
+
         status 204
         hook.destroy
       end
diff --git a/lib/api/project_snippets.rb b/lib/api/project_snippets.rb
index f3d905b0068..645162d564d 100644
--- a/lib/api/project_snippets.rb
+++ b/lib/api/project_snippets.rb
@@ -116,6 +116,8 @@ module API
         not_found!('Snippet') unless snippet
 
         authorize! :admin_project_snippet, snippet
+        check_unmodified_since(snippet.updated_at)
+
         status 204
         snippet.destroy
       end
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index 15c3832b032..eab0ca0b3c9 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -334,6 +334,8 @@ module API
       desc 'Remove a project'
       delete ":id" do
         authorize! :remove_project, user_project
+        check_unmodified_since(user_project.updated_at)
+
         ::Projects::DestroyService.new(user_project, current_user, {}).async_execute
 
         accepted!
diff --git a/lib/api/runners.rb b/lib/api/runners.rb
index 31f940fe96b..e3b2eb904b7 100644
--- a/lib/api/runners.rb
+++ b/lib/api/runners.rb
@@ -77,7 +77,9 @@ module API
       end
       delete ':id' do
         runner = get_runner(params[:id])
+
         authenticate_delete_runner!(runner)
+        check_unmodified_since(runner.updated_at)
 
         status 204
         runner.destroy!
diff --git a/lib/api/services.rb b/lib/api/services.rb
index 843c05ae32e..4fef3383e5e 100644
--- a/lib/api/services.rb
+++ b/lib/api/services.rb
@@ -655,6 +655,8 @@ module API
       end
       delete ":id/services/:service_slug" do
         service = user_project.find_or_initialize_service(params[:service_slug].underscore)
+        # Todo, not sure
+        check_unmodified_since(service.updated_at)
 
         attrs = service_attributes(service).inject({}) do |hash, key|
           hash.merge!(key => nil)
diff --git a/lib/api/snippets.rb b/lib/api/snippets.rb
index 35ece56c65c..7107b3d669c 100644
--- a/lib/api/snippets.rb
+++ b/lib/api/snippets.rb
@@ -122,6 +122,7 @@ module API
         return not_found!('Snippet') unless snippet
 
         authorize! :destroy_personal_snippet, snippet
+        check_unmodified_since(snippet.updated_at)
 
         status 204
         snippet.destroy
diff --git a/lib/api/system_hooks.rb b/lib/api/system_hooks.rb
index c0179037440..64066a75b15 100644
--- a/lib/api/system_hooks.rb
+++ b/lib/api/system_hooks.rb
@@ -66,6 +66,8 @@ module API
         hook = SystemHook.find_by(id: params[:id])
         not_found!('System hook') unless hook
 
+        check_unmodified_since(hook.updated_at)
+
         status 204
         hook.destroy
       end
diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb
index edfdb63d183..4ae70c65759 100644
--- a/lib/api/triggers.rb
+++ b/lib/api/triggers.rb
@@ -140,6 +140,8 @@ module API
         trigger = user_project.triggers.find(params.delete(:trigger_id))
         return not_found!('Trigger') unless trigger
 
+        check_unmodified_since(trigger.updated_at)
+
         status 204
         trigger.destroy
       end
diff --git a/lib/api/users.rb b/lib/api/users.rb
index e2019d6d512..942bb72cf97 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -230,7 +230,12 @@ module API
         key = user.keys.find_by(id: params[:key_id])
         not_found!('Key') unless key
 
+<<<<<<< HEAD
         status 204
+=======
+        check_unmodified_since(key.updated_at)
+
+>>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints
         key.destroy
       end
 
@@ -287,7 +292,14 @@ module API
         email = user.emails.find_by(id: params[:email_id])
         not_found!('Email') unless email
 
+<<<<<<< HEAD
         Emails::DestroyService.new(user, email: email.email).execute
+=======
+        check_unmodified_since(email.updated_at)
+
+        email.destroy
+        user.update_secondary_emails!
+>>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints
       end
 
       desc 'Delete a user. Available only for admins.' do
@@ -299,11 +311,18 @@ module API
       end
       delete ":id" do
         authenticated_as_admin!
+
         user = User.find_by(id: params[:id])
         not_found!('User') unless user
 
+<<<<<<< HEAD
         status 204
         user.delete_async(deleted_by: current_user, params: params)
+=======
+        check_unmodified_since(user.updated_at)
+
+        ::Users::DestroyService.new(current_user).execute(user)
+>>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints
       end
 
       desc 'Block a user. Available only for admins.'
@@ -481,6 +500,8 @@ module API
         key = current_user.keys.find_by(id: params[:key_id])
         not_found!('Key') unless key
 
+        check_unmodified_since(key.updated_at)
+
         status 204
         key.destroy
       end
@@ -533,6 +554,7 @@ module API
         email = current_user.emails.find_by(id: params[:email_id])
         not_found!('Email') unless email
 
+<<<<<<< HEAD
         status 204
         Emails::DestroyService.new(current_user, email: email.email).execute
       end
@@ -550,6 +572,12 @@ module API
           .reorder(last_activity_on: :asc)
 
         present paginate(activities), with: Entities::UserActivity
+=======
+        check_unmodified_since(email.updated_at)
+
+        email.destroy
+        current_user.update_secondary_emails!
+>>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints
       end
     end
   end

From e80313f9ee5b3495a8713e6ddae111bc8106155b Mon Sep 17 00:00:00 2001
From: Robert Schilling <rschilling@student.tugraz.at>
Date: Thu, 2 Mar 2017 13:14:13 +0100
Subject: [PATCH 2/7] Conditionally destroy a ressource

---
 lib/api/access_requests.rb     | 11 ++++----
 lib/api/award_emoji.rb         |  4 +--
 lib/api/boards.rb              | 11 ++++----
 lib/api/broadcast_messages.rb  |  4 +--
 lib/api/deploy_keys.rb         |  5 +---
 lib/api/environments.rb        |  4 +--
 lib/api/groups.rb              |  7 +++--
 lib/api/helpers.rb             | 17 +++++++++---
 lib/api/issues.rb              |  4 +--
 lib/api/labels.rb              |  5 +---
 lib/api/members.rb             |  7 +++--
 lib/api/merge_requests.rb      |  4 +--
 lib/api/notes.rb               |  6 ++---
 lib/api/project_hooks.rb       |  5 +---
 lib/api/project_snippets.rb    |  4 +--
 lib/api/projects.rb            |  5 ++--
 lib/api/runner.rb              |  6 +++--
 lib/api/runners.rb             |  7 ++---
 lib/api/services.rb            |  4 +--
 lib/api/snippets.rb            |  4 +--
 lib/api/system_hooks.rb        |  5 +---
 lib/api/triggers.rb            |  5 +---
 lib/api/users.rb               | 47 ++++++++++------------------------
 spec/requests/api/tags_spec.rb |  4 +--
 24 files changed, 71 insertions(+), 114 deletions(-)

diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb
index 0c5b8862d79..4fa9b2b2494 100644
--- a/lib/api/access_requests.rb
+++ b/lib/api/access_requests.rb
@@ -67,13 +67,12 @@ module API
         end
         delete ":id/access_requests/:user_id" do
           source = find_source(source_type, params[:id])
-          member = source.public_send(:requesters).find_by!(user_id: params[:user_id])
+          member = source.requesters.find_by!(user_id: params[:user_id])
 
-          check_unmodified_since(member.updated_at)
-
-          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 51a8587d26e..8e3851640da 100644
--- a/lib/api/award_emoji.rb
+++ b/lib/api/award_emoji.rb
@@ -85,12 +85,10 @@ module API
           end
           delete "#{endpoint}/:award_id" do
             award = awardable.award_emoji.find(params[:award_id])
-            check_unmodified_since(award.updated_at)
 
             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 d36df77dc6c..0d11c5fc971 100644
--- a/lib/api/boards.rb
+++ b/lib/api/boards.rb
@@ -122,14 +122,13 @@ module API
         end
         delete "/lists/:list_id" do
           authorize!(:admin_list, user_project)
-
           list = board_lists.find(params[:list_id])
-          check_unmodified_since(list.updated_at)
 
-          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/broadcast_messages.rb b/lib/api/broadcast_messages.rb
index 352972b584a..0b45621ce7b 100644
--- a/lib/api/broadcast_messages.rb
+++ b/lib/api/broadcast_messages.rb
@@ -90,10 +90,8 @@ module API
       end
       delete ':id' do
         message = find_message
-        check_unmodified_since(message.updated_at)
 
-        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 971cc816454..f405c341398 100644
--- a/lib/api/deploy_keys.rb
+++ b/lib/api/deploy_keys.rb
@@ -125,10 +125,7 @@ module API
         key = user_project.deploy_keys_projects.find_by(deploy_key_id: params[:key_id])
         not_found!('Deploy Key') unless key
 
-        check_unmodified_since(key.updated_at)
-
-        status 204
-        key.destroy
+        destroy_conditionally!(key)
       end
     end
   end
diff --git a/lib/api/environments.rb b/lib/api/environments.rb
index 3fc423ae79a..e33269f9483 100644
--- a/lib/api/environments.rb
+++ b/lib/api/environments.rb
@@ -78,10 +78,8 @@ module API
         authorize! :update_environment, user_project
 
         environment = user_project.environments.find(params[:environment_id])
-        check_unmodified_since(environment.updated_at)
 
-        status 204
-        environment.destroy
+        destroy_conditionally!(environment)
       end
 
       desc 'Stops an existing environment' do
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index c9b32a85487..ee2ad27837b 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -117,11 +117,10 @@ module API
       delete ":id" do
         group = find_group!(params[:id])
         authorize! :admin_group, group
-        
-        check_unmodified_since(group.updated_at)
 
-        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 1c74a14d91c..8d4f8c01903 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -11,14 +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']) if headers.key?('If-Unmodified-Since') rescue nil
+    def check_unmodified_since!(last_modified)
+      if_unmodified_since = Time.parse(headers['If-Unmodified-Since']) rescue nil
 
-      if if_unmodified_since && if_unmodified_since < last_modified
+      if if_unmodified_since && 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 cee9898d3a6..6503629e2a2 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -230,10 +230,8 @@ module API
         not_found!('Issue') unless issue
 
         authorize!(:destroy_issue, issue)
-        check_unmodified_since(issue.updated_at)
 
-        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 45fa57fdf55..c0cf618ee8d 100644
--- a/lib/api/labels.rb
+++ b/lib/api/labels.rb
@@ -56,10 +56,7 @@ module API
         label = user_project.labels.find_by(title: params[:name])
         not_found!('Label') unless label
 
-        check_unmodified_since(label.updated_at)
-
-        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 5634f123eca..a5d3d7f25a0 100644
--- a/lib/api/members.rb
+++ b/lib/api/members.rb
@@ -93,12 +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])
-          check_unmodified_since(member.updated_at)
 
-          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 c6fecc1aa6c..969c6064662 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -164,10 +164,8 @@ module API
         merge_request = find_project_merge_request(params[:merge_request_iid])
 
         authorize!(:destroy_merge_request, merge_request)
-        check_unmodified_since(merge_request.updated_at)
 
-        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 58d71787aca..e116448c15b 100644
--- a/lib/api/notes.rb
+++ b/lib/api/notes.rb
@@ -131,10 +131,10 @@ module API
           note = user_project.notes.find(params[:note_id])
 
           authorize! :admin_note, note
-          check_unmodified_since(note.updated_at)
 
-          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/project_hooks.rb b/lib/api/project_hooks.rb
index 74d736fda59..5b457bbe639 100644
--- a/lib/api/project_hooks.rb
+++ b/lib/api/project_hooks.rb
@@ -96,10 +96,7 @@ module API
       delete ":id/hooks/:hook_id" do
         hook = user_project.hooks.find(params.delete(:hook_id))
 
-        check_unmodified_since(hook.updated_at)
-
-        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 645162d564d..704e8c6718d 100644
--- a/lib/api/project_snippets.rb
+++ b/lib/api/project_snippets.rb
@@ -116,10 +116,8 @@ module API
         not_found!('Snippet') unless snippet
 
         authorize! :admin_project_snippet, snippet
-        check_unmodified_since(snippet.updated_at)
 
-        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 eab0ca0b3c9..36fe3f243b9 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -334,9 +334,10 @@ module API
       desc 'Remove a project'
       delete ":id" do
         authorize! :remove_project, user_project
-        check_unmodified_since(user_project.updated_at)
 
-        ::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
diff --git a/lib/api/runner.rb b/lib/api/runner.rb
index 88fc62d33df..daa8ddbe251 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 e3b2eb904b7..68c2120cc15 100644
--- a/lib/api/runners.rb
+++ b/lib/api/runners.rb
@@ -79,10 +79,8 @@ module API
         runner = get_runner(params[:id])
 
         authenticate_delete_runner!(runner)
-        check_unmodified_since(runner.updated_at)
 
-        status 204
-        runner.destroy!
+        destroy_conditionally!(runner)
       end
     end
 
@@ -137,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 4fef3383e5e..2b5ef75b6bf 100644
--- a/lib/api/services.rb
+++ b/lib/api/services.rb
@@ -655,8 +655,8 @@ module API
       end
       delete ":id/services/:service_slug" do
         service = user_project.find_or_initialize_service(params[:service_slug].underscore)
-        # Todo, not sure
-        check_unmodified_since(service.updated_at)
+        # Todo: Check if this done the right way
+        check_unmodified_since!(service.updated_at)
 
         attrs = service_attributes(service).inject({}) do |hash, key|
           hash.merge!(key => nil)
diff --git a/lib/api/snippets.rb b/lib/api/snippets.rb
index 7107b3d669c..00eb7c60f16 100644
--- a/lib/api/snippets.rb
+++ b/lib/api/snippets.rb
@@ -122,10 +122,8 @@ module API
         return not_found!('Snippet') unless snippet
 
         authorize! :destroy_personal_snippet, snippet
-        check_unmodified_since(snippet.updated_at)
 
-        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 64066a75b15..6b6a03e3300 100644
--- a/lib/api/system_hooks.rb
+++ b/lib/api/system_hooks.rb
@@ -66,10 +66,7 @@ module API
         hook = SystemHook.find_by(id: params[:id])
         not_found!('System hook') unless hook
 
-        check_unmodified_since(hook.updated_at)
-
-        status 204
-        hook.destroy
+        destroy_conditionally!(hook)
       end
     end
   end
diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb
index 4ae70c65759..c9fee7e5193 100644
--- a/lib/api/triggers.rb
+++ b/lib/api/triggers.rb
@@ -140,10 +140,7 @@ module API
         trigger = user_project.triggers.find(params.delete(:trigger_id))
         return not_found!('Trigger') unless trigger
 
-        check_unmodified_since(trigger.updated_at)
-
-        status 204
-        trigger.destroy
+        destroy_conditionally!(trigger)
       end
     end
   end
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 942bb72cf97..d7c7b9ae9c1 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -230,13 +230,7 @@ module API
         key = user.keys.find_by(id: params[:key_id])
         not_found!('Key') unless key
 
-<<<<<<< HEAD
-        status 204
-=======
-        check_unmodified_since(key.updated_at)
-
->>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints
-        key.destroy
+        destroy_conditionally!(key)
       end
 
       desc 'Add an email address to a specified user. Available only for admins.' do
@@ -292,14 +286,11 @@ module API
         email = user.emails.find_by(id: params[:email_id])
         not_found!('Email') unless email
 
-<<<<<<< HEAD
-        Emails::DestroyService.new(user, email: email.email).execute
-=======
-        check_unmodified_since(email.updated_at)
+        destroy_conditionally!(email) do |email|
+          Emails::DestroyService.new(current_user, email: email.email).execute
+        end
 
-        email.destroy
         user.update_secondary_emails!
->>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints
       end
 
       desc 'Delete a user. Available only for admins.' do
@@ -315,14 +306,9 @@ module API
         user = User.find_by(id: params[:id])
         not_found!('User') unless user
 
-<<<<<<< HEAD
-        status 204
-        user.delete_async(deleted_by: current_user, params: params)
-=======
-        check_unmodified_since(user.updated_at)
-
-        ::Users::DestroyService.new(current_user).execute(user)
->>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints
+        destroy_conditionally!(user) do
+          user.delete_async(deleted_by: current_user, params: params)
+        end
       end
 
       desc 'Block a user. Available only for admins.'
@@ -500,10 +486,7 @@ module API
         key = current_user.keys.find_by(id: params[:key_id])
         not_found!('Key') unless key
 
-        check_unmodified_since(key.updated_at)
-
-        status 204
-        key.destroy
+        destroy_conditionally!(key)
       end
 
       desc "Get the currently authenticated user's email addresses" do
@@ -554,9 +537,11 @@ module API
         email = current_user.emails.find_by(id: params[:email_id])
         not_found!('Email') unless email
 
-<<<<<<< HEAD
-        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'
@@ -572,12 +557,6 @@ module API
           .reorder(last_activity_on: :asc)
 
         present paginate(activities), with: Entities::UserActivity
-=======
-        check_unmodified_since(email.updated_at)
-
-        email.destroy
-        current_user.update_secondary_emails!
->>>>>>> API: Respect the 'If-Unmodified-Since' for delete endpoints
       end
     end
   end
diff --git a/spec/requests/api/tags_spec.rb b/spec/requests/api/tags_spec.rb
index 9884c1ec206..54900c75eb8 100644
--- a/spec/requests/api/tags_spec.rb
+++ b/spec/requests/api/tags_spec.rb
@@ -283,7 +283,7 @@ describe API::Tags do
 
         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
 
@@ -394,7 +394,7 @@ describe API::Tags do
 
         it_behaves_like '404 response' do
           let(:request) { put api(route, current_user), description: new_description }
-          let(:message) { 'Tag does not exist' }
+          let(:message) { '404 Tag Not Found' }
         end
       end
 

From f0f3f38576c0691e6d0e751c962382beea998afb Mon Sep 17 00:00:00 2001
From: Robert Schilling <rschilling@student.tugraz.at>
Date: Thu, 2 Mar 2017 14:21:36 +0100
Subject: [PATCH 3/7] Use commit date for branches and tags

---
 lib/api/branches.rb            | 15 +++++++++++----
 lib/api/tags.rb                | 15 +++++++++++----
 spec/requests/api/tags_spec.rb |  2 +-
 3 files changed, 23 insertions(+), 9 deletions(-)

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
 
-        if result[:status] != :success
-          render_api_error!(result[:message], result[:return_code])
+        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])
+          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
 
-        if result[:status] != :success
-          render_api_error!(result[:message], result[:return_code])
+        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])
+          end
         end
       end
 
diff --git a/spec/requests/api/tags_spec.rb b/spec/requests/api/tags_spec.rb
index 54900c75eb8..6ac0caa7fe8 100644
--- a/spec/requests/api/tags_spec.rb
+++ b/spec/requests/api/tags_spec.rb
@@ -394,7 +394,7 @@ describe API::Tags do
 
         it_behaves_like '404 response' do
           let(:request) { put api(route, current_user), description: new_description }
-          let(:message) { '404 Tag Not Found' }
+          let(:message) { 'Tag does not exist' }
         end
       end
 

From dcd4ea473cab20eee05995ecaca043da98ca5a8d Mon Sep 17 00:00:00 2001
From: Robert Schilling <rschilling@student.tugraz.at>
Date: Thu, 24 Aug 2017 10:41:54 +0200
Subject: [PATCH 4/7] Update remaining endpoints

---
 lib/api/group_variables.rb                   |  3 +--
 lib/api/helpers.rb                           |  2 +-
 lib/api/pipeline_schedules.rb                |  3 +--
 lib/api/projects.rb                          |  1 -
 lib/api/protected_branches.rb                |  4 +---
 lib/api/services.rb                          | 14 +++++++-------
 lib/api/users.rb                             |  7 +++++--
 lib/api/variables.rb                         |  1 +
 spec/requests/api/pipeline_schedules_spec.rb |  3 +--
 9 files changed, 18 insertions(+), 20 deletions(-)

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/helpers.rb b/lib/api/helpers.rb
index 8d4f8c01903..84980864151 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -14,7 +14,7 @@ module API
     def check_unmodified_since!(last_modified)
       if_unmodified_since = Time.parse(headers['If-Unmodified-Since']) rescue nil
 
-      if if_unmodified_since && last_modified > if_unmodified_since
+      if if_unmodified_since && last_modified && last_modified > if_unmodified_since
         render_api_error!('412 Precondition Failed', 412)
       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/projects.rb b/lib/api/projects.rb
index 36fe3f243b9..78e25b6da03 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
 
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/services.rb b/lib/api/services.rb
index 2b5ef75b6bf..ff9ddd44439 100644
--- a/lib/api/services.rb
+++ b/lib/api/services.rb
@@ -655,15 +655,15 @@ module API
       end
       delete ":id/services/:service_slug" do
         service = user_project.find_or_initialize_service(params[:service_slug].underscore)
-        # Todo: Check if this done the right way
-        check_unmodified_since!(service.updated_at)
 
-        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/users.rb b/lib/api/users.rb
index d7c7b9ae9c1..96f47bb618a 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -408,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
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
diff --git a/spec/requests/api/pipeline_schedules_spec.rb b/spec/requests/api/pipeline_schedules_spec.rb
index 1fc0ec528b9..150a391bb8d 100644
--- a/spec/requests/api/pipeline_schedules_spec.rb
+++ b/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 match_response_schema('pipeline_schedule')
+        expect(response).to have_http_status(204)
       end
 
       it 'responds with 404 Not Found if requesting non-existing pipeline_schedule' do

From 915dd57fe26fb228f30ae08edc4905b24d4c4339 Mon Sep 17 00:00:00 2001
From: Robert Schilling <rschilling@student.tugraz.at>
Date: Thu, 24 Aug 2017 18:03:39 +0200
Subject: [PATCH 5/7] Add tests for the unmodified header

---
 spec/requests/api/award_emoji_spec.rb         | 16 +++++
 spec/requests/api/boards_spec.rb              |  4 ++
 spec/requests/api/branches_spec.rb            |  4 ++
 spec/requests/api/broadcast_messages_spec.rb  |  4 ++
 spec/requests/api/deploy_keys_spec.rb         |  4 ++
 spec/requests/api/environments_spec.rb        |  4 ++
 spec/requests/api/group_variables_spec.rb     |  4 ++
 spec/requests/api/groups_spec.rb              |  4 ++
 spec/requests/api/issues_spec.rb              |  4 ++
 spec/requests/api/labels_spec.rb              |  5 ++
 spec/requests/api/members_spec.rb             |  4 ++
 spec/requests/api/merge_requests_spec.rb      |  4 ++
 spec/requests/api/notes_spec.rb               | 12 ++++
 spec/requests/api/pipeline_schedules_spec.rb  |  4 ++
 spec/requests/api/project_hooks_spec.rb       |  4 ++
 spec/requests/api/project_snippets_spec.rb    |  7 +-
 spec/requests/api/projects_spec.rb            | 69 +++++++++++++------
 spec/requests/api/protected_branches_spec.rb  |  4 ++
 spec/requests/api/runner_spec.rb              |  5 ++
 spec/requests/api/runners_spec.rb             | 12 ++++
 spec/requests/api/snippets_spec.rb            |  4 ++
 spec/requests/api/system_hooks_spec.rb        |  4 ++
 spec/requests/api/tags_spec.rb                |  4 ++
 spec/requests/api/triggers_spec.rb            |  4 ++
 spec/requests/api/users_spec.rb               | 24 +++++++
 .../requests/api/status_shared_examples.rb    | 11 +++
 26 files changed, 206 insertions(+), 23 deletions(-)

diff --git a/spec/requests/api/award_emoji_spec.rb b/spec/requests/api/award_emoji_spec.rb
index 1dd9f3f6ddc..593068b8cd7 100644
--- a/spec/requests/api/award_emoji_spec.rb
+++ b/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
diff --git a/spec/requests/api/boards_spec.rb b/spec/requests/api/boards_spec.rb
index 43b381c2219..f698d5dddb3 100644
--- a/spec/requests/api/boards_spec.rb
+++ b/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
diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb
index 5a2e1b2cf2d..b1e011de604 100644
--- a/spec/requests/api/branches_spec.rb
+++ b/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
diff --git a/spec/requests/api/broadcast_messages_spec.rb b/spec/requests/api/broadcast_messages_spec.rb
index 67989689799..b043a333d33 100644
--- a/spec/requests/api/broadcast_messages_spec.rb
+++ b/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)
diff --git a/spec/requests/api/deploy_keys_spec.rb b/spec/requests/api/deploy_keys_spec.rb
index e497ec333a2..684877c33c0 100644
--- a/spec/requests/api/deploy_keys_spec.rb
+++ b/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
diff --git a/spec/requests/api/environments_spec.rb b/spec/requests/api/environments_spec.rb
index 87716c6fe3a..2361809e0e1 100644
--- a/spec/requests/api/environments_spec.rb
+++ b/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
diff --git a/spec/requests/api/group_variables_spec.rb b/spec/requests/api/group_variables_spec.rb
index 2179790d098..93b9cf85c1d 100644
--- a/spec/requests/api/group_variables_spec.rb
+++ b/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
diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb
index a7557c7fb22..39d76cdbc74 100644
--- a/spec/requests/api/groups_spec.rb
+++ b/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)
diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb
index 7d120e4a234..58d89451073 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/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
diff --git a/spec/requests/api/labels_spec.rb b/spec/requests/api/labels_spec.rb
index 5a4257d1009..b231fdea2a3 100644
--- a/spec/requests/api/labels_spec.rb
+++ b/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
diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb
index 06aca698c91..d3bae8d2888 100644
--- a/spec/requests/api/members_spec.rb
+++ b/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
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index 0db645863fb..9027090aabd 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/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
 
diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb
index 75e5062a99c..f5882c0c74a 100644
--- a/spec/requests/api/notes_spec.rb
+++ b/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
diff --git a/spec/requests/api/pipeline_schedules_spec.rb b/spec/requests/api/pipeline_schedules_spec.rb
index 150a391bb8d..b6a5a7ffbb5 100644
--- a/spec/requests/api/pipeline_schedules_spec.rb
+++ b/spec/requests/api/pipeline_schedules_spec.rb
@@ -275,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
diff --git a/spec/requests/api/project_hooks_spec.rb b/spec/requests/api/project_hooks_spec.rb
index 2829c243af3..ac3bab09c4c 100644
--- a/spec/requests/api/project_hooks_spec.rb
+++ b/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
diff --git a/spec/requests/api/project_snippets_spec.rb b/spec/requests/api/project_snippets_spec.rb
index 2b541f5719e..0ea95f487ac 100644
--- a/spec/requests/api/project_snippets_spec.rb
+++ b/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
diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb
index a89a58ff713..46550243d06 100644
--- a/spec/requests/api/projects_spec.rb
+++ b/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
-      group = create(:group, :public)
-      create(:project_group_link, group: group, project: project)
+    context 'for a valid group' do
+      let(:group) { create(:group, :public) }
 
-      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)
-      expect(project.project_group_links).to be_empty
+      it 'returns 204 when deleting a group share' do
+        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,10 @@ describe API::Projects do
         expect(json_response['message']).to eql('202 Accepted')
       end
 
+      it_behaves_like '412 response' do
+        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 +1574,10 @@ describe API::Projects do
         delete api('/projects/1328', admin)
         expect(response).to have_http_status(404)
       end
+
+      it_behaves_like '412 response' do
+        let(:request) { api("/projects/#{project.id}", admin) }
+      end
     end
   end
 
diff --git a/spec/requests/api/protected_branches_spec.rb b/spec/requests/api/protected_branches_spec.rb
index 1aa8a95780e..07d7f96bd70 100644
--- a/spec/requests/api/protected_branches_spec.rb
+++ b/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)
 
diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb
index e9ee3dd679d..993164aa8fe 100644
--- a/spec/requests/api/runner_spec.rb
+++ b/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
 
diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb
index c8ff25f70fa..244895a417e 100644
--- a/spec/requests/api/runners_spec.rb
+++ b/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
diff --git a/spec/requests/api/snippets_spec.rb b/spec/requests/api/snippets_spec.rb
index d09b8bc42f1..351dd3276d0 100644
--- a/spec/requests/api/snippets_spec.rb
+++ b/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
diff --git a/spec/requests/api/system_hooks_spec.rb b/spec/requests/api/system_hooks_spec.rb
index f65b475fe44..216d278ad21 100644
--- a/spec/requests/api/system_hooks_spec.rb
+++ b/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
diff --git a/spec/requests/api/tags_spec.rb b/spec/requests/api/tags_spec.rb
index 6ac0caa7fe8..0bf7863bdc8 100644
--- a/spec/requests/api/tags_spec.rb
+++ b/spec/requests/api/tags_spec.rb
@@ -278,6 +278,10 @@ 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' }
 
diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb
index 1e206fd2a9e..d1ed57b1825 100644
--- a/spec/requests/api/triggers_spec.rb
+++ b/spec/requests/api/triggers_spec.rb
@@ -293,6 +293,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
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 49739a1601a..5fef4437997 100644
--- a/spec/requests/api/users_spec.rb
+++ b/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)
 
diff --git a/spec/support/shared_examples/requests/api/status_shared_examples.rb b/spec/support/shared_examples/requests/api/status_shared_examples.rb
index 226277411d6..1bffd1e49db 100644
--- a/spec/support/shared_examples/requests/api/status_shared_examples.rb
+++ b/spec/support/shared_examples/requests/api/status_shared_examples.rb
@@ -40,3 +40,14 @@ shared_examples_for '404 response' do
     end
   end
 end
+
+shared_examples_for '412 response' do
+  let(:params) { nil }
+  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

From 8bd9fb4cc4f8689a23c68e1b7f893ee59907069f Mon Sep 17 00:00:00 2001
From: Robert Schilling <rschilling@student.tugraz.at>
Date: Fri, 25 Aug 2017 14:31:09 +0200
Subject: [PATCH 6/7] Add changelog and doc

---
 changelogs/unreleased/api-delete-respect-headers.yml | 5 +++++
 doc/api/README.md                                    | 1 +
 2 files changed, 6 insertions(+)
 create mode 100644 changelogs/unreleased/api-delete-respect-headers.yml

diff --git a/changelogs/unreleased/api-delete-respect-headers.yml b/changelogs/unreleased/api-delete-respect-headers.yml
new file mode 100644
index 00000000000..cfc8fbfdf91
--- /dev/null
+++ b/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
diff --git a/doc/api/README.md b/doc/api/README.md
index 266b5f018d9..c2a08dcff07 100644
--- a/doc/api/README.md
+++ b/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. |
 

From ee4820a5268d02fb7ed142511d1a194f06629a1e Mon Sep 17 00:00:00 2001
From: Robert Schilling <rschilling@student.tugraz.at>
Date: Mon, 28 Aug 2017 17:13:22 +0200
Subject: [PATCH 7/7] Add a spec when ressource is not modified

---
 lib/api/projects.rb                           |  6 ++---
 spec/requests/api/projects_spec.rb            |  2 ++
 .../requests/api/status_shared_examples.rb    | 22 +++++++++++++++----
 3 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index 78e25b6da03..78d900984ac 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -365,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
@@ -410,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/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb
index 46550243d06..4490e50702b 100644
--- a/spec/requests/api/projects_spec.rb
+++ b/spec/requests/api/projects_spec.rb
@@ -1541,6 +1541,7 @@ describe API::Projects do
       end
 
       it_behaves_like '412 response' do
+        let(:success_status) { 202 }
         let(:request) { api("/projects/#{project.id}", user) }
       end
 
@@ -1576,6 +1577,7 @@ describe API::Projects do
       end
 
       it_behaves_like '412 response' do
+        let(:success_status) { 202 }
         let(:request) { api("/projects/#{project.id}", admin) }
       end
     end
diff --git a/spec/support/shared_examples/requests/api/status_shared_examples.rb b/spec/support/shared_examples/requests/api/status_shared_examples.rb
index 1bffd1e49db..7d7f66adeab 100644
--- a/spec/support/shared_examples/requests/api/status_shared_examples.rb
+++ b/spec/support/shared_examples/requests/api/status_shared_examples.rb
@@ -43,11 +43,25 @@ end
 
 shared_examples_for '412 response' do
   let(:params) { nil }
-  before do
-    delete request, params, { 'HTTP_IF_UNMODIFIED_SINCE' => '1990-01-12T00:00:48-0600' }
+  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
 
-  it 'returns 412' do
-    expect(response).to have_gitlab_http_status(412)
+  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