Merge branch '62322-add-optional-id-to-label-api-put-delete-pd' into 'master'

Add label_id parameter to label API for PUT and DELETE

Closes #62322

See merge request gitlab-org/gitlab-ce!31804

changelogs/unreleased/62322-add-optional-id-to-label-api-put-delete-pd.yml

+---
+title: Add optional label_id parameter to label API for PUT and DELETE
+merge_request: 31804
+author:
+type: changed

doc/api/labels.md

@@ -137,8 +137,9 @@ DELETE /projects/:id/labels
 | Attribute | Type    | Required | Description           |
 | --------- | ------- | -------- | --------------------- |
-| `id`      | integer/string    | yes      | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user |
-| `name`    | string  | yes      | The name of the label |
+| `id`            | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user |
+| `label_id`      | integer        | yes (or `name`)                   | The id of the existing label     |
+| `name`          | string         | yes (or `label_id`)               | The name of the existing label   |
 ```bash
 curl --request DELETE --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/projects/1/labels?name=bug"
@@ -156,7 +157,8 @@ PUT /projects/:id/labels
 | Attribute       | Type    | Required                          | Description                      |
 | --------------- | ------- | --------------------------------- | -------------------------------  |
 | `id`      | integer/string    | yes      | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user |
-| `name`          | string  | yes                               | The name of the existing label   |
+| `label_id`      | integer | yes (or `name`)                   | The id of the existing label     |
+| `name`          | string  | yes (or `label_id`)               | The name of the existing label   |
 | `new_name`      | string  | yes if `color` is not provided    | The new name of the label        |
 | `color`         | string  | yes if `new_name` is not provided | The color of the label given in 6-digit hex notation with leading '#' sign (e.g. #FFAABB) or one of the [CSS color names](https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#Color_keywords) |
 | `description`   | string  | no                                | The new description of the label |

lib/api/helpers/label_helpers.rb

@@ -11,9 +11,9 @@ module API
         optional :description, type: String, desc: 'The description of label to be created'
       end
-      def find_label(parent, id, include_ancestor_groups: true)
+      def find_label(parent, id_or_title, include_ancestor_groups: true)
         labels = available_labels_for(parent, include_ancestor_groups: include_ancestor_groups)
-        label = labels.find_by_id(id) || labels.find_by_title(id)
+        label = labels.find_by_id(id_or_title) || labels.find_by_title(id_or_title)
         label || not_found!('Label')
       end
@@ -35,12 +35,7 @@ module API
         priority = params.delete(:priority)
         label_params = declared_params(include_missing: false)
-        label =
-          if parent.is_a?(Project)
-            ::Labels::CreateService.new(label_params).execute(project: parent)
-          else
-            ::Labels::CreateService.new(label_params).execute(group: parent)
-          end
+        label = ::Labels::CreateService.new(label_params).execute(create_service_params(parent))
         if label.persisted?
           if parent.is_a?(Project)
@@ -56,10 +51,13 @@ module API
       def update_label(parent, entity)
         authorize! :admin_label, parent
-        label = find_label(parent, params[:name], include_ancestor_groups: false)
+        label = find_label(parent, params_id_or_title, include_ancestor_groups: false)
         update_priority = params.key?(:priority)
         priority = params.delete(:priority)
+        # params is used to update the label so we need to remove this field here
+        params.delete(:label_id)
+
         label = ::Labels::UpdateService.new(declared_params(include_missing: false)).execute(label)
         render_validation_error!(label) unless label.valid?
@@ -77,10 +75,24 @@ module API
       def delete_label(parent)
         authorize! :admin_label, parent
-        label = find_label(parent, params[:name], include_ancestor_groups: false)
+        label = find_label(parent, params_id_or_title, include_ancestor_groups: false)
         destroy_conditionally!(label)
       end
+
+      def params_id_or_title
+        @params_id_or_title ||= params[:label_id] || params[:name]
+      end
+
+      def create_service_params(parent)
+        if parent.is_a?(Project)
+          { project: parent }
+        elsif parent.is_a?(Group)
+          { group: parent }
+        else
+          raise TypeError, 'Parent type is not supported'
+        end
+      end
     end
   end
 end

lib/api/labels.rb

@@ -38,11 +38,13 @@ module API
         success Entities::ProjectLabel
       end
       params do
-        requires :name, type: String, desc: 'The name of the label to be updated'
+        optional :label_id, type: Integer, desc: 'The id of the label to be updated'
+        optional :name, type: String, desc: 'The name of the label to be updated'
         optional :new_name, type: String, desc: 'The new name of the label'
         optional :color, type: String, desc: "The new color of the label given in 6-digit hex notation with leading '#' sign (e.g. #FFAABB) or one of the allowed CSS color names"
         optional :description, type: String, desc: 'The new description of label'
         optional :priority, type: Integer, desc: 'The priority of the label', allow_blank: true
+        exactly_one_of :label_id, :name
         at_least_one_of :new_name, :color, :description, :priority
       end
       put ':id/labels' do
@@ -53,7 +55,9 @@ module API
         success Entities::ProjectLabel
       end
       params do
-        requires :name, type: String, desc: 'The name of the label to be deleted'
+        optional :label_id, type: Integer, desc: 'The id of the label to be deleted'
+        optional :name, type: String, desc: 'The name of the label to be deleted'
+        exactly_one_of :label_id, :name
       end
       delete ':id/labels' do
         delete_label(user_project)

spec/lib/api/helpers/label_helpers_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::Helpers::LabelHelpers do
+  describe 'create_service_params' do
+    let(:label_helper) do
+      Class.new do
+        include API::Helpers::LabelHelpers
+      end.new
+    end
+
+    context 'when a project is given' do
+      it 'returns the expected params' do
+        project = create(:project)
+        expect(label_helper.create_service_params(project)).to eq({ project: project })
+      end
+    end
+
+    context 'when a group is given' do
+      it 'returns the expected params' do
+        group = create(:group)
+        expect(label_helper.create_service_params(group)).to eq({ group: group })
+      end
+    end
+
+    context 'when something else is given' do
+      it 'raises a type error' do
+        expect { label_helper.create_service_params(Class.new) }.to raise_error(TypeError)
+      end
+    end
+  end
+end

spec/requests/api/labels_spec.rb

@@ -6,6 +6,180 @@ describe API::Labels do
   let!(:label1) { create(:label, title: 'label1', project: project) }
   let!(:priority_label) { create(:label, title: 'bug', project: project, priority: 3) }
+  shared_examples 'label update API' do
+    it 'returns 200 if name is changed' do
+      request_params = {
+        new_name: 'New Label'
+      }.merge(spec_params)
+
+      put api("/projects/#{project.id}/labels", user),
+          params: request_params
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['name']).to eq('New Label')
+      expect(json_response['color']).to eq(label1.color)
+    end
+
+    it 'returns 200 if colors is changed' do
+      request_params = {
+        color: '#FFFFFF'
+      }.merge(spec_params)
+
+      put api("/projects/#{project.id}/labels", user),
+          params: request_params
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['name']).to eq(label1.name)
+      expect(json_response['color']).to eq('#FFFFFF')
+    end
+
+    it 'returns 200 if a priority is added' do
+      request_params = {
+        priority: 3
+      }.merge(spec_params)
+
+      put api("/projects/#{project.id}/labels", user),
+          params: request_params
+
+      expect(response.status).to eq(200)
+      expect(json_response['name']).to eq(label1.name)
+      expect(json_response['priority']).to eq(3)
+    end
+
+    it 'returns 400 if no new parameters given' do
+      put api("/projects/#{project.id}/labels", user), params: spec_params
+
+      expect(response).to have_gitlab_http_status(400)
+      expect(json_response['error']).to eq('new_name, color, description, priority are missing, '\
+                                           'at least one parameter must be provided')
+    end
+
+    it 'returns 400 when color code is too short' do
+      request_params = {
+        color: '#FF'
+      }.merge(spec_params)
+
+      put api("/projects/#{project.id}/labels", user),
+          params: request_params
+
+      expect(response).to have_gitlab_http_status(400)
+      expect(json_response['message']['color']).to eq(['must be a valid color code'])
+    end
+
+    it 'returns 400 for too long color code' do
+      request_params = {
+        color: '#FFAAFFFF'
+      }.merge(spec_params)
+
+      put api("/projects/#{project.id}/labels", user),
+          params: request_params
+
+      expect(response).to have_gitlab_http_status(400)
+      expect(json_response['message']['color']).to eq(['must be a valid color code'])
+    end
+
+    it 'returns 400 for invalid priority' do
+      request_params = {
+        priority: 'foo'
+      }.merge(spec_params)
+
+      put api("/projects/#{project.id}/labels", user),
+          params: request_params
+
+      expect(response).to have_gitlab_http_status(400)
+    end
+
+    it 'returns 200 if name and colors and description are changed' do
+      request_params = {
+        new_name: 'New Label',
+        color: '#FFFFFF',
+        description: 'test'
+      }.merge(spec_params)
+
+      put api("/projects/#{project.id}/labels", user),
+          params: request_params
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['name']).to eq('New Label')
+      expect(json_response['color']).to eq('#FFFFFF')
+      expect(json_response['description']).to eq('test')
+    end
+
+    it 'returns 400 for invalid name' do
+      request_params = {
+        new_name: ',',
+        color: '#FFFFFF'
+      }.merge(spec_params)
+
+      put api("/projects/#{project.id}/labels", user),
+          params: request_params
+
+      expect(response).to have_gitlab_http_status(400)
+      expect(json_response['message']['title']).to eq(['is invalid'])
+    end
+
+    it 'returns 200 if description is changed' do
+      request_params = {
+        description: 'test'
+      }.merge(spec_params)
+
+      put api("/projects/#{project.id}/labels", user),
+          params: request_params
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['id']).to eq(expected_response_label_id)
+      expect(json_response['description']).to eq('test')
+    end
+
+    it 'returns 200 if priority is changed' do
+      request_params = {
+        priority: 10
+      }.merge(spec_params)
+
+      put api("/projects/#{project.id}/labels", user),
+          params: request_params
+
+      expect(response.status).to eq(200)
+      expect(json_response['id']).to eq(expected_response_label_id)
+      expect(json_response['priority']).to eq(10)
+    end
+
+    it 'returns 200 if a priority is removed' do
+      label = find_by_spec_params(spec_params)
+      expect(label).not_to be_nil
+
+      label.priorities.create(project: label.project, priority: 1)
+      label.save!
+
+      request_params = {
+        priority: nil
+      }.merge(spec_params)
+
+      put api("/projects/#{project.id}/labels", user),
+          params: request_params
+
+      expect(response.status).to eq(200)
+      expect(json_response['id']).to eq(expected_response_label_id)
+      expect(json_response['priority']).to be_nil
+    end
+
+    def find_by_spec_params(params)
+      if params.key?(:label_id)
+        Label.find(params[:label_id])
+      else
+        Label.find_by(name: params[:name])
+      end
+    end
+  end
+
+  shared_examples 'label delete API' do
+    it 'returns 204 for existing label' do
+      delete api("/projects/#{project.id}/labels", user), params: spec_params
+
+      expect(response).to have_gitlab_http_status(204)
+    end
+  end
+
   before do
     project.add_maintainer(user)
   end
@@ -208,174 +382,92 @@ describe API::Labels do
   end
   describe 'DELETE /projects/:id/labels' do
-    it 'returns 204 for existing label' do
-      delete api("/projects/#{project.id}/labels", user), params: { name: 'label1' }
-      expect(response).to have_gitlab_http_status(204)
+    it_behaves_like 'label delete API' do
+      let(:spec_params) { { name: 'label1' } }
+    end
+    it_behaves_like 'label delete API' do
+      let(:spec_params) { { label_id: label1.id } }
     end
     it 'returns 404 for non existing label' do
       delete api("/projects/#{project.id}/labels", user), params: { name: 'label2' }
+
       expect(response).to have_gitlab_http_status(404)
       expect(json_response['message']).to eq('404 Label Not Found')
     end
     it 'returns 400 for wrong parameters' do
       delete api("/projects/#{project.id}/labels", user)
-      expect(response).to have_gitlab_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
-    it 'returns 200 if name and colors and description are changed' do
-      put api("/projects/#{project.id}/labels", user),
-          params: {
-            name: 'label1',
-            new_name: 'New Label',
-            color: '#FFFFFF',
-            description: 'test'
-          }
-      expect(response).to have_gitlab_http_status(200)
-      expect(json_response['name']).to eq('New Label')
-      expect(json_response['color']).to eq('#FFFFFF')
-      expect(json_response['description']).to eq('test')
+      expect(response).to have_gitlab_http_status(400)
     end
-    it 'returns 200 if name is changed' do
-      put api("/projects/#{project.id}/labels", user),
+    it 'fails if label_id and name are given in params' do
+      delete api("/projects/#{project.id}/labels", user),
           params: {
-            name: 'label1',
-            new_name: 'New Label'
+            label_id: label1.id,
+            name: priority_label.name
           }
-      expect(response).to have_gitlab_http_status(200)
-      expect(json_response['name']).to eq('New Label')
-      expect(json_response['color']).to eq(label1.color)
-    end
-    it 'returns 200 if colors is changed' do
-      put api("/projects/#{project.id}/labels", user),
-          params: {
-            name: 'label1',
-            color: '#FFFFFF'
-          }
-      expect(response).to have_gitlab_http_status(200)
-      expect(json_response['name']).to eq(label1.name)
-      expect(json_response['color']).to eq('#FFFFFF')
+      expect(response).to have_gitlab_http_status(400)
     end
-    it 'returns 200 if description is changed' do
-      put api("/projects/#{project.id}/labels", user),
-          params: {
-            name: 'bug',
-            description: 'test'
-          }
-
-      expect(response).to have_gitlab_http_status(200)
-      expect(json_response['name']).to eq(priority_label.name)
-      expect(json_response['description']).to eq('test')
-      expect(json_response['priority']).to eq(3)
+    it_behaves_like '412 response' do
+      let(:request) { api("/projects/#{project.id}/labels", user) }
+      let(:params) { { name: 'label1' } }
     end
-    it 'returns 200 if priority is changed' do
-      put api("/projects/#{project.id}/labels", user),
-           params: {
-             name: 'bug',
-             priority: 10
-           }
-
-      expect(response.status).to eq(200)
-      expect(json_response['name']).to eq(priority_label.name)
-      expect(json_response['priority']).to eq(10)
+  end
+  describe 'PUT /projects/:id/labels' do
+    context 'when using name' do
+      it_behaves_like 'label update API' do
+        let(:spec_params) { { name: 'label1' } }
+        let(:expected_response_label_id) { label1.id }
+      end
     end
-    it 'returns 200 if a priority is added' do
-      put api("/projects/#{project.id}/labels", user),
-           params: {
-             name: 'label1',
-             priority: 3
-           }
-
-      expect(response.status).to eq(200)
-      expect(json_response['name']).to eq(label1.name)
-      expect(json_response['priority']).to eq(3)
+    context 'when using label_id' do
+      it_behaves_like 'label update API' do
+        let(:spec_params) { { label_id: label1.id } }
+        let(:expected_response_label_id) { label1.id }
+      end
     end
-    it 'returns 200 if the priority is removed' do
+    it 'returns 404 if label does not exist' do
       put api("/projects/#{project.id}/labels", user),
           params: {
-            name: priority_label.name,
-            priority: nil
+            name: 'label2',
+            new_name: 'label3'
           }
-      expect(response.status).to eq(200)
-      expect(json_response['name']).to eq(priority_label.name)
-      expect(json_response['priority']).to be_nil
+      expect(response).to have_gitlab_http_status(404)
     end
-    it 'returns 404 if label does not exist' do
+    it 'returns 404 if label by id does not exist' do
       put api("/projects/#{project.id}/labels", user),
           params: {
-            name: 'label2',
+            label_id: 0,
             new_name: 'label3'
           }
+
       expect(response).to have_gitlab_http_status(404)
     end
-    it 'returns 400 if no label name given' do
+    it 'returns 400 if no label name and id is given' do
       put api("/projects/#{project.id}/labels", user), params: { new_name: 'label2' }
-      expect(response).to have_gitlab_http_status(400)
-      expect(json_response['error']).to eq('name is missing')
-    end
-
-    it 'returns 400 if no new parameters given' do
-      put api("/projects/#{project.id}/labels", user), params: { name: 'label1' }
-      expect(response).to have_gitlab_http_status(400)
-      expect(json_response['error']).to eq('new_name, color, description, priority are missing, '\
-                                           'at least one parameter must be provided')
-    end
-    it 'returns 400 for invalid name' do
-      put api("/projects/#{project.id}/labels", user),
-          params: {
-            name: 'label1',
-            new_name: ',',
-            color: '#FFFFFF'
-          }
       expect(response).to have_gitlab_http_status(400)
-      expect(json_response['message']['title']).to eq(['is invalid'])
+      expect(json_response['error']).to eq('label_id, name are missing, exactly one parameter must be provided')
     end
-    it 'returns 400 when color code is too short' do
+    it 'fails if label_id and name are given in params' do
       put api("/projects/#{project.id}/labels", user),
           params: {
-            name: 'label1',
-            color: '#FF'
+            label_id: label1.id,
+            name: priority_label.name,
+            new_name: 'New Label'
           }
-      expect(response).to have_gitlab_http_status(400)
-      expect(json_response['message']['color']).to eq(['must be a valid color code'])
-    end
-
-    it 'returns 400 for too long color code' do
-      put api("/projects/#{project.id}/labels", user),
-           params: {
-             name: 'label1',
-             color: '#FFAAFFFF'
-           }
-      expect(response).to have_gitlab_http_status(400)
-      expect(json_response['message']['color']).to eq(['must be a valid color code'])
-    end
-
-    it 'returns 400 for invalid priority' do
-      put api("/projects/#{project.id}/labels", user),
-           params: {
-             name: 'label1',
-             priority: 'foo'
-           }
       expect(response).to have_gitlab_http_status(400)
     end