Merge branch 'return-empty-body-on-delete' into 'master'

Return empty body for 204 responses in API

See merge request gitlab-org/gitlab!22086

changelogs/unreleased/return-empty-body-for-204-responses-in-api.yml

+---
+title: Return empty body for 204 responses in API
+merge_request: 22086
+author:
+type: fixed

doc/development/api_styleguide.md

@@ -92,6 +92,12 @@ For instance:
 Model.create(foo: params[:foo])
 ```
 
+## Using HTTP status helpers
+
+For non-200 HTTP responses, use the provided helpers in `lib/api/helpers.rb` to ensure correct behaviour (`not_found!`, `no_content!` etc.). These will `throw` inside Grape and abort the execution of your endpoint.
+
+For `DELETE` requests, you should also generally use the `destroy_conditionally!` helper which by default returns a `204 No Content` response on success, or a `412 Precondition Failed` response if the given `If-Unmodified-Since` header is out of range. This helper calls `#destroy` on the passed resource, but you can also implement a custom deletion method by passing a block.
+
 ## Using API path helpers in GitLab Rails codebase
 
 Because we support [installing GitLab under a relative URL], one must take this

ee/lib/api/geo_nodes.rb

@@ -203,7 +203,8 @@ module API
           not_found!('GeoNode') unless geo_node
 
           geo_node.destroy!
-          status 204
+
+          no_content!
         end
       end
     end

ee/lib/api/ldap_group_links.rb

@@ -41,7 +41,7 @@ module API
         ldap_group_link = group.ldap_group_links.find_by(cn: params[:cn])
         if ldap_group_link
           ldap_group_link.destroy
-          status 204
+          no_content!
         else
           render_api_error!('Linked LDAP group not found', 404)
         end
@@ -61,7 +61,7 @@ module API
         ldap_group_link = group.ldap_group_links.find_by(cn: params[:cn], provider: params[:provider])
         if ldap_group_link
           ldap_group_link.destroy
-          status 204
+          no_content!
         else
           render_api_error!('Linked LDAP group not found', 404)
         end

ee/lib/api/managed_licenses.rb

@@ -113,8 +113,9 @@ module API
         authorize_can_admin!
         not_found!('SoftwareLicensePolicy') unless software_license_policy
 
-        status 204
         software_license_policy.destroy!
+
+        no_content!
       end
     end
   end

ee/lib/api/project_aliases.rb

@@ -58,7 +58,7 @@ module API
       delete ':name' do
         project_alias.destroy
 
-        status 204
+        no_content!
       end
     end
   end

ee/lib/api/project_push_rule.rb

@@ -78,7 +78,8 @@ module API
         not_found!('Push Rule') unless push_rule
 
         push_rule.destroy
-        status 204
+
+        no_content!
       end
     end
   end

ee/lib/api/scim.rb

@@ -157,9 +157,7 @@ module API
           updated = update_scim_user(identity)
 
           if updated
-            status 204
-
-            {}
+            no_content!
           else
             scim_error!(message: "Error updating #{identity.user.name} with #{params.inspect}")
           end
@@ -180,9 +178,7 @@ module API
 
           scim_not_found!(message: "Resource #{params[:id]} not found") unless destroyed
 
-          status 204
-
-          {}
+          no_content!
         end
       end
     end

ee/spec/requests/api/scim_spec.rb

@@ -249,7 +249,7 @@ describe API::Scim do
           end
 
           it 'responds with an empty response' do
-            expect(json_response).to eq({})
+            expect(response.body).to eq('')
           end
         end
 
@@ -322,7 +322,7 @@ describe API::Scim do
         end
 
         it 'responds with an empty response' do
-          expect(json_response).to eq({})
+          expect(response.body).to eq('')
         end
       end
 

lib/api/applications.rb

@@ -38,7 +38,7 @@ module API
         application = ApplicationsFinder.new(params).execute
         application.destroy
 
-        status 204
+        no_content!
       end
     end
   end

lib/api/badges.rb

@@ -135,7 +135,6 @@ module API
           end
 
           destroy_conditionally!(badge)
-          body false
         end
       end
     end

lib/api/branches.rb

@@ -57,7 +57,7 @@ module API
           requires :branch, type: String, desc: 'The name of the branch'
         end
         head do
-          user_project.repository.branch_exists?(params[:branch]) ? status(204) : status(404)
+          user_project.repository.branch_exists?(params[:branch]) ? no_content! : not_found!
         end
         get do
           branch = find_branch!(params[:branch])

lib/api/custom_attributes_endpoints.rb

@@ -77,7 +77,7 @@ module API
 
         resource.custom_attributes.find_by!(key: params[:key]).destroy
 
-        status 204
+        no_content!
       end
       # rubocop: enable CodeReuse/ActiveRecord
     end

lib/api/features.rb

@@ -74,7 +74,7 @@ module API
       delete ':name' do
         Feature.get(params[:name]).remove
 
-        status 204
+        no_content!
       end
     end
   end

lib/api/group_milestones.rb

@@ -67,7 +67,7 @@ module API
         milestone = user_group.milestones.find(params[:milestone_id])
         Milestones::DestroyService.new(user_group, current_user).execute(milestone)
 
-        status(204)
+        no_content!
       end
 
       desc 'Get all issues for a single group milestone' do

lib/api/helpers.rb

@@ -31,6 +31,7 @@ module API
       check_unmodified_since!(last_updated)
 
       status 204
+      body false
 
       if block_given?
         yield resource

lib/api/pages.rb

@@ -17,9 +17,9 @@ module API
       delete ':id/pages' do
         authorize! :remove_pages, user_project
 
-        status 204
-
         ::Pages::DeleteService.new(user_project, current_user).execute
+
+        no_content!
       end
     end
   end

lib/api/pages_domains.rb

@@ -148,8 +148,9 @@ module API
       delete ":id/pages/domains/:domain", requirements: PAGES_DOMAINS_ENDPOINT_REQUIREMENTS do
         authorize! :update_pages, user_project
 
-        status 204
         pages_domain.destroy
+
+        no_content!
       end
     end
   end

lib/api/project_milestones.rb

@@ -69,7 +69,7 @@ module API
         milestone = user_project.milestones.find(params[:milestone_id])
         Milestones::DestroyService.new(user_project, current_user).execute(milestone)
 
-        status(204)
+        no_content!
       end
 
       desc 'Get all issues for a single project milestone' do

lib/api/projects.rb

@@ -447,7 +447,7 @@ module API
           ::Projects::UnlinkForkService.new(user_project, current_user).execute
         end
 
-        result ? status(204) : not_modified!
+        not_modified! unless result
       end
 
       desc 'Share the project with a group' do

lib/api/users.rb

@@ -346,8 +346,9 @@ module API
         key = user.gpg_keys.find_by(id: params[:key_id])
         not_found!('GPG Key') unless key
 
-        status 204
         key.destroy
+
+        no_content!
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
@@ -760,8 +761,9 @@ module API
         key = current_user.gpg_keys.find_by(id: params[:key_id])
         not_found!('GPG Key') unless key
 
-        status 204
         key.destroy
+
+        no_content!
       end
       # rubocop: enable CodeReuse/ActiveRecord
 

lib/api/variables.rb

@@ -111,9 +111,10 @@ 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
+        # Variables don't have a timestamp. Therefore, destroy unconditionally.
         variable.destroy
+
+        no_content!
       end
       # rubocop: enable CodeReuse/ActiveRecord
     end

lib/api/wikis.rb

@@ -107,8 +107,9 @@ module API
       delete ':id/wikis/:slug' do
         authorize! :admin_wiki, user_project
 
-        status 204
         WikiPages::DestroyService.new(user_project, current_user).execute(wiki_page)
+
+        no_content!
       end
 
       desc 'Upload an attachment to the wiki repository' do

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

@@ -59,8 +59,9 @@ shared_examples_for '412 response' do
       delete request, params: params, headers: { 'HTTP_IF_UNMODIFIED_SINCE' => '1990-01-12T00:00:48-0600' }
     end
 
-    it 'returns 412' do
+    it 'returns 412 with a JSON error' do
       expect(response).to have_gitlab_http_status(412)
+      expect(json_response).to eq('message' => '412 Precondition Failed')
     end
   end
 
@@ -69,8 +70,9 @@ shared_examples_for '412 response' do
       delete request, params: params, headers: { 'HTTP_IF_UNMODIFIED_SINCE' => Time.now }
     end
 
-    it 'returns accepted' do
+    it 'returns 204 with an empty body' do
       expect(response).to have_gitlab_http_status(success_status)
+      expect(response.body).to eq('') if success_status == 204
     end
   end
 end