Merge branch '9-0-api-changes-ee' into 'master'

Port "V4 API: Remove deprecated MR and Issue endpoints and preserve V3 namespace"

See merge request !1166

changelogs/unreleased/9-0-api-changes.yml

+---
+title: Remove deprecated MR and Issue endpoints and preserve V3 namespace
+merge_request: 8967
+author:

doc/api/README.md

@@ -51,6 +51,7 @@ following locations:
 - [Todos](todos.md)
 - [Users](users.md)
 - [Validate CI configuration](ci/lint.md)
+- [V3 to V4](v3_to_v4.md)
 - [Version](version.md)
 
 ### Internal CI API

doc/api/issues.md

@@ -183,7 +183,6 @@ GET /projects/:id/issues?labels=foo,bar
 GET /projects/:id/issues?labels=foo,bar&state=opened
 GET /projects/:id/issues?milestone=1.0.0
 GET /projects/:id/issues?milestone=1.0.0&state=opened
-GET /projects/:id/issues?iid=42
 ```
 
 | Attribute | Type | Required | Description |

doc/api/merge_requests.md

@@ -10,8 +10,7 @@ The pagination parameters `page` and `per_page` can be used to restrict the list
 GET /projects/:id/merge_requests
 GET /projects/:id/merge_requests?state=opened
 GET /projects/:id/merge_requests?state=all
-GET /projects/:id/merge_requests?iid=42
-GET /projects/:id/merge_requests?iid[]=42&iid[]=43
+GET /projects/:id/merge_requests?iids[]=42&iids[]=43
 ```
 
 Parameters:

doc/api/v3_to_v4.md

+# V3 to V4 version
+
+Our V4 API version is currently available as *Beta*! It means that V3
+will still be supported and remain unchanged for now, but be aware that the following
+changes are in V4:
+
+### Changes
+
+- `iid` filter has been removed from `projects/:id/issues`
+- `projects/:id/merge_requests?iid[]=x&iid[]=y` array filter has been renamed to `iids`
+- Endpoints under `projects/merge_request/:id` have been removed (use: `projects/merge_requests/:id`)
+

lib/api/api.rb

 module API
   class API < Grape::API
     include APIGuard
-    version 'v3', using: :path
+
+    version %w(v3 v4), using: :path
+
+    version 'v3', using: :path do
+      mount ::API::V3::Issues
+      mount ::API::V3::MergeRequests
+    end
 
     before { allow_access_with_scope :api }
 

lib/api/issues.rb

@@ -15,8 +15,6 @@ module API
         labels = args.delete(:labels)
         args[:label_name] = labels if match_all_labels
 
-        args[:search] = "#{Issue.reference_prefix}#{args.delete(:iid)}" if args.key?(:iid)
-
         issues = IssuesFinder.new(current_user, args).execute.inc_notes_with_associations
 
         # TODO: Remove in 9.0  pass `label_name: args.delete(:labels)` to IssuesFinder
@@ -99,7 +97,6 @@ module API
       params do
         optional :state, type: String, values: %w[opened closed all], default: 'all',
                          desc: 'Return opened, closed, or all issues'
-        optional :iid, type: Integer, desc: 'Return the issue having the given `iid`'
         use :issues_params
       end
       get ":id/issues" do

lib/api/merge_requests.rb

@@ -2,8 +2,6 @@ module API
   class MergeRequests < Grape::API
     include PaginationParams
 
-    DEPRECATION_MESSAGE = 'This endpoint is deprecated and will be removed in GitLab 9.0.'.freeze
-
     before { authenticate! }
 
     params do
@@ -48,14 +46,14 @@ module API
                             desc: 'Return merge requests ordered by `created_at` or `updated_at` fields.'
         optional :sort, type: String, values: %w[asc desc], default: 'desc',
                         desc: 'Return merge requests sorted in `asc` or `desc` order.'
-        optional :iid, type: Array[Integer], desc: 'The IID of the merge requests'
+        optional :iids, type: Array[Integer], desc: 'The IID array of merge requests'
         use :pagination
       end
       get ":id/merge_requests" do
         authorize! :read_merge_request, user_project
 
         merge_requests = user_project.merge_requests.inc_notes_with_associations
-        merge_requests = filter_by_iid(merge_requests, params[:iid]) if params[:iid].present?
+        merge_requests = filter_by_iid(merge_requests, params[:iids]) if params[:iids].present?
 
         merge_requests =
           case params[:state]
@@ -106,20 +104,13 @@ module API
         merge_request.destroy
       end
 
-      # Routing "merge_request/:merge_request_id/..." is DEPRECATED and WILL BE REMOVED in version 9.0
-      # Use "merge_requests/:merge_request_id/..." instead.
-      #
       params do
         requires :merge_request_id, type: Integer, desc: 'The ID of a merge request'
       end
-      { ":id/merge_request/:merge_request_id" => :deprecated, ":id/merge_requests/:merge_request_id" => :ok }.each do |path, status|
       desc 'Get a single merge request' do
-          if status == :deprecated
-            detail DEPRECATION_MESSAGE
-          end
         success Entities::MergeRequest
       end
-        get path do
+      get ':id/merge_requests/:merge_request_id' do
         merge_request = find_merge_request_with_access(params[:merge_request_id])
 
         present merge_request, with: Entities::MergeRequest, current_user: current_user, project: user_project
@@ -128,7 +119,7 @@ module API
       desc 'Get the commits of a merge request' do
         success Entities::RepoCommit
       end
-        get "#{path}/commits" do
+      get ':id/merge_requests/:merge_request_id/commits' do
         merge_request = find_merge_request_with_access(params[:merge_request_id])
 
         present merge_request.commits, with: Entities::RepoCommit
@@ -137,7 +128,7 @@ module API
       desc 'Show the merge request changes' do
         success Entities::MergeRequestChanges
       end
-        get "#{path}/changes" do
+      get ':id/merge_requests/:merge_request_id/changes' do
         merge_request = find_merge_request_with_access(params[:merge_request_id])
 
         present merge_request, with: Entities::MergeRequestChanges, current_user: current_user
@@ -153,10 +144,10 @@ module API
                                desc: 'Status of the merge request'
         use :optional_params
         at_least_one_of :title, :target_branch, :description, :assignee_id,
-                          :milestone_id, :labels, :state_event, :approvals_before_merge,
+                        :milestone_id, :labels, :state_event,
                         :remove_source_branch, :squash
       end
-        put path do
+      put ':id/merge_requests/:merge_request_id' do
         merge_request = find_merge_request_with_access(params.delete(:merge_request_id), :update_merge_request)
 
         mr_params = declared_params(include_missing: false)
@@ -183,7 +174,7 @@ module API
         optional :sha, type: String, desc: 'When present, must have the HEAD SHA of the source branch'
         optional :squash, type: Boolean, desc: 'When true, the commits will be squashed into a single commit on merge'
       end
-        put "#{path}/merge" do
+      put ':id/merge_requests/:merge_request_id/merge' do
         merge_request = find_project_merge_request(params[:merge_request_id])
 
         # Merge request can not be merged
@@ -223,7 +214,7 @@ module API
       desc 'Cancel merge if "Merge When Pipeline Succeeds" is enabled' do
         success Entities::MergeRequest
       end
-        post "#{path}/cancel_merge_when_build_succeeds" do
+      post ':id/merge_requests/:merge_request_id/cancel_merge_when_build_succeeds' do
         merge_request = find_project_merge_request(params[:merge_request_id])
 
         unauthorized! unless merge_request.can_cancel_merge_when_build_succeeds?(current_user)
@@ -234,25 +225,23 @@ module API
       end
 
       desc 'Get the comments of a merge request' do
-          detail 'Duplicate. DEPRECATED and WILL BE REMOVED in 9.0'
         success Entities::MRNote
       end
       params do
         use :pagination
       end
-        get "#{path}/comments" do
+      get ':id/merge_requests/:merge_request_id/comments' do
         merge_request = find_merge_request_with_access(params[:merge_request_id])
         present paginate(merge_request.notes.fresh), with: Entities::MRNote
       end
 
       desc 'Post a comment to a merge request' do
-          detail 'Duplicate. DEPRECATED and WILL BE REMOVED in 9.0'
         success Entities::MRNote
       end
       params do
         requires :note, type: String, desc: 'The text of the comment'
       end
-        post "#{path}/comments" do
+      post ':id/merge_requests/:merge_request_id/comments' do
         merge_request = find_merge_request_with_access(params[:merge_request_id], :create_note)
 
         opts = {
@@ -276,7 +265,7 @@ module API
       params do
         use :pagination
       end
-        get "#{path}/closes_issues" do
+      get ':id/merge_requests/:merge_request_id/closes_issues' do
         merge_request = find_merge_request_with_access(params[:merge_request_id])
         issues = ::Kaminari.paginate_array(merge_request.closes_issues(current_user))
         present paginate(issues), with: issue_entity(user_project), current_user: current_user
@@ -290,7 +279,7 @@ module API
       # Examples:
       #   GET /projects/:id/merge_requests/:merge_request_id/approvals
       #
-        get "#{path}/approvals" do
+      get ':id/merge_requests/:merge_request_id/approvals' do
         merge_request = user_project.merge_requests.find(params[:merge_request_id])
 
         authorize! :read_merge_request, merge_request
@@ -305,7 +294,7 @@ module API
       # Examples:
       #   POST /projects/:id/merge_requests/:merge_request_id/approvals
       #
-        post "#{path}/approve" do
+      post ':id/merge_requests/:merge_request_id/approve' do
         merge_request = user_project.merge_requests.find(params[:merge_request_id])
 
         unauthorized! unless merge_request.can_approve?(current_user)
@@ -317,7 +306,7 @@ module API
         present merge_request, with: Entities::MergeRequestApprovals, current_user: current_user
       end
 
-        delete "#{path}/unapprove" do
+      delete ':id/merge_requests/:merge_request_id/unapprove' do
         merge_request = user_project.merge_requests.find(params[:merge_request_id])
 
         not_found! unless merge_request.has_approved?(current_user)
@@ -330,5 +319,4 @@ module API
       end
     end
   end
-  end
 end

lib/api/v3/issues.rb

+module API
+  module V3
+    class Issues < Grape::API
+      include PaginationParams
+
+      before { authenticate! }
+
+      helpers do
+        def find_issues(args = {})
+          args = params.merge(args)
+
+          args.delete(:id)
+          args[:milestone_title] = args.delete(:milestone)
+
+          match_all_labels = args.delete(:match_all_labels)
+          labels = args.delete(:labels)
+          args[:label_name] = labels if match_all_labels
+
+          args[:search] = "#{Issue.reference_prefix}#{args.delete(:iid)}" if args.key?(:iid)
+
+          issues = IssuesFinder.new(current_user, args).execute.inc_notes_with_associations
+
+          if !match_all_labels && labels.present?
+            issues = issues.includes(:labels).where('labels.title' => labels.split(','))
+          end
+
+          issues.reorder(args[:order_by] => args[:sort])
+        end
+
+        params :issues_params do
+          optional :labels, type: String, desc: 'Comma-separated list of label names'
+          optional :milestone, type: String, desc: 'Milestone title'
+          optional :order_by, type: String, values: %w[created_at updated_at], default: 'created_at',
+                              desc: 'Return issues ordered by `created_at` or `updated_at` fields.'
+          optional :sort, type: String, values: %w[asc desc], default: 'desc',
+                          desc: 'Return issues sorted in `asc` or `desc` order.'
+          optional :milestone, type: String, desc: 'Return issues for a specific milestone'
+          use :pagination
+        end
+
+        params :issue_params do
+          optional :description, type: String, desc: 'The description of an issue'
+          optional :assignee_id, type: Integer, desc: 'The ID of a user to assign issue'
+          optional :milestone_id, type: Integer, desc: 'The ID of a milestone to assign issue'
+          optional :labels, type: String, desc: 'Comma-separated list of label names'
+          optional :due_date, type: String, desc: 'Date time string in the format YEAR-MONTH-DAY'
+          optional :confidential, type: Boolean, desc: 'Boolean parameter if the issue should be confidential'
+        end
+      end
+
+      resource :issues do
+        desc "Get currently authenticated user's issues" do
+          success Entities::Issue
+        end
+        params do
+          optional :state, type: String, values: %w[opened closed all], default: 'all',
+                           desc: 'Return opened, closed, or all issues'
+          use :issues_params
+        end
+        get do
+          issues = find_issues(scope: 'authored')
+
+          present paginate(issues), with: Entities::Issue, current_user: current_user
+        end
+      end
+
+      params do
+        requires :id, type: String, desc: 'The ID of a group'
+      end
+      resource :groups do
+        desc 'Get a list of group issues' do
+          success Entities::Issue
+        end
+        params do
+          optional :state, type: String, values: %w[opened closed all], default: 'opened',
+                           desc: 'Return opened, closed, or all issues'
+          use :issues_params
+        end
+        get ":id/issues" do
+          group = find_group!(params[:id])
+
+          issues = find_issues(group_id: group.id, state: params[:state] || 'opened', match_all_labels: true)
+
+          present paginate(issues), with: Entities::Issue, current_user: current_user
+        end
+      end
+
+      params do
+        requires :id, type: String, desc: 'The ID of a project'
+      end
+      resource :projects do
+        include TimeTrackingEndpoints
+
+        desc 'Get a list of project issues' do
+          detail 'iid filter is deprecated have been removed on V4'
+          success Entities::Issue
+        end
+        params do
+          optional :state, type: String, values: %w[opened closed all], default: 'all',
+                           desc: 'Return opened, closed, or all issues'
+          optional :iid, type: Integer, desc: 'Return the issue having the given `iid`'
+          use :issues_params
+        end
+        get ":id/issues" do
+          project = find_project(params[:id])
+
+          issues = find_issues(project_id: project.id)
+
+          present paginate(issues), with: Entities::Issue, current_user: current_user, project: user_project
+        end
+
+        desc 'Get a single project issue' do
+          success Entities::Issue
+        end
+        params do
+          requires :issue_id, type: Integer, desc: 'The ID of a project issue'
+        end
+        get ":id/issues/:issue_id" do
+          issue = find_project_issue(params[:issue_id])
+          present issue, with: Entities::Issue, current_user: current_user, project: user_project
+        end
+
+        desc 'Create a new project issue' do
+          success Entities::Issue
+        end
+        params do
+          requires :title, type: String, desc: 'The title of an issue'
+          optional :created_at, type: DateTime,
+                                desc: 'Date time when the issue was created. Available only for admins and project owners.'
+          optional :merge_request_for_resolving_discussions, type: Integer,
+                                                             desc: 'The IID of a merge request for which to resolve discussions'
+          use :issue_params
+        end
+        post ':id/issues' do
+          # Setting created_at time only allowed for admins and project owners
+          unless current_user.admin? || user_project.owner == current_user
+            params.delete(:created_at)
+          end
+
+          issue_params = declared_params(include_missing: false)
+
+          if merge_request_iid = params[:merge_request_for_resolving_discussions]
+            issue_params[:merge_request_for_resolving_discussions] = MergeRequestsFinder.new(current_user, project_id: user_project.id).
+              execute.
+              find_by(iid: merge_request_iid)
+          end
+
+          issue = ::Issues::CreateService.new(user_project,
+                                              current_user,
+                                              issue_params.merge(request: request, api: true)).execute
+          if issue.spam?
+            render_api_error!({ error: 'Spam detected' }, 400)
+          end
+
+          if issue.valid?
+            present issue, with: Entities::Issue, current_user: current_user, project: user_project
+          else
+            render_validation_error!(issue)
+          end
+        end
+
+        desc 'Update an existing issue' do
+          success Entities::Issue
+        end
+        params do
+          requires :issue_id, type: Integer, desc: 'The ID of a project issue'
+          optional :title, type: String, desc: 'The title of an issue'
+          optional :updated_at, type: DateTime,
+                                desc: 'Date time when the issue was updated. Available only for admins and project owners.'
+          optional :state_event, type: String, values: %w[reopen close], desc: 'State of the issue'
+          use :issue_params
+          at_least_one_of :title, :description, :assignee_id, :milestone_id,
+                          :labels, :created_at, :due_date, :confidential, :state_event
+        end
+        put ':id/issues/:issue_id' do
+          issue = user_project.issues.find(params.delete(:issue_id))
+          authorize! :update_issue, issue
+
+          # Setting created_at time only allowed for admins and project owners
+          unless current_user.admin? || user_project.owner == current_user
+            params.delete(:updated_at)
+          end
+
+          issue = ::Issues::UpdateService.new(user_project,
+                                              current_user,
+                                              declared_params(include_missing: false)).execute(issue)
+
+          if issue.valid?
+            present issue, with: Entities::Issue, current_user: current_user, project: user_project
+          else
+            render_validation_error!(issue)
+          end
+        end
+
+        desc 'Move an existing issue' do
+          success Entities::Issue
+        end
+        params do
+          requires :issue_id, type: Integer, desc: 'The ID of a project issue'
+          requires :to_project_id, type: Integer, desc: 'The ID of the new project'
+        end
+        post ':id/issues/:issue_id/move' do
+          issue = user_project.issues.find_by(id: params[:issue_id])
+          not_found!('Issue') unless issue
+
+          new_project = Project.find_by(id: params[:to_project_id])
+          not_found!('Project') unless new_project
+
+          begin
+            issue = ::Issues::MoveService.new(user_project, current_user).execute(issue, new_project)
+            present issue, with: Entities::Issue, current_user: current_user, project: user_project
+          rescue ::Issues::MoveService::MoveError => error
+            render_api_error!(error.message, 400)
+          end
+        end
+
+        desc 'Delete a project issue'
+        params do
+          requires :issue_id, type: Integer, desc: 'The ID of a project issue'
+        end
+        delete ":id/issues/:issue_id" do
+          issue = user_project.issues.find_by(id: params[:issue_id])
+          not_found!('Issue') unless issue
+
+          authorize!(:destroy_issue, issue)
+          issue.destroy
+        end
+      end
+    end
+  end
+end

spec/models/geo_node_spec.rb

@@ -9,6 +9,7 @@ describe GeoNode, type: :model do
 
   let(:dummy_url) { 'https://localhost:3000/gitlab' }
   let(:url_helpers) { Gitlab::Application.routes.url_helpers }
+  let(:api_version) { API::API.version }
 
   context 'associations' do
     it { is_expected.to belong_to(:geo_node_key).dependent(:destroy) }
@@ -158,7 +159,7 @@ describe GeoNode, type: :model do
   end
 
   describe '#notify_projects_url' do
-    let(:refresh_url) { 'https://localhost:3000/gitlab/api/v3/geo/refresh_projects' }
+    let(:refresh_url) { "https://localhost:3000/gitlab/api/#{api_version}/geo/refresh_projects" }
 
     it 'returns api url based on node uri' do
       expect(new_node.notify_projects_url).to eq(refresh_url)
@@ -166,7 +167,7 @@ describe GeoNode, type: :model do
   end
 
   describe '#notify_wikis_url' do
-    let(:refresh_url) { 'https://localhost:3000/gitlab/api/v3/geo/refresh_wikis' }
+    let(:refresh_url) { "https://localhost:3000/gitlab/api/#{api_version}/geo/refresh_wikis" }
 
     it 'returns api url based on node uri' do
       expect(new_node.notify_wikis_url).to eq(refresh_url)
@@ -174,7 +175,7 @@ describe GeoNode, type: :model do
   end
 
   describe '#geo_events_url' do
-    let(:events_url) { 'https://localhost:3000/gitlab/api/v3/geo/receive_events' }
+    let(:events_url) { "https://localhost:3000/gitlab/api/#{api_version}/geo/receive_events" }
 
     it 'returns api url based on node uri' do
       expect(new_node.geo_events_url).to eq(events_url)

spec/requests/api/issues_spec.rb

@@ -613,23 +613,6 @@ describe API::Issues, api: true  do
       expect(json_response['iid']).to eq(issue.iid)
     end
 
-    it 'returns a project issue by iid' do
-      get api("/projects/#{project.id}/issues?iid=#{issue.iid}", user)
-
-      expect(response.status).to eq 200
-      expect(json_response.length).to eq 1
-      expect(json_response.first['title']).to eq issue.title
-      expect(json_response.first['id']).to eq issue.id
-      expect(json_response.first['iid']).to eq issue.iid
-    end
-
-    it 'returns an empty array for an unknown project issue iid' do
-      get api("/projects/#{project.id}/issues?iid=#{issue.iid + 10}", user)
-
-      expect(response.status).to eq 200
-      expect(json_response.length).to eq 0
-    end
-
     it "returns 404 if issue id not found" do
       get api("/projects/#{project.id}/issues/54321", user)
       expect(response).to have_http_status(404)

spec/requests/api/merge_requests_spec.rb

@@ -74,6 +74,16 @@ describe API::MergeRequests, api: true  do
         expect(json_response.first['title']).to eq(merge_request_merged.title)
       end
 
+      it 'returns merge_request by "iids" array' do
+        get api("/projects/#{project.id}/merge_requests", user), iids: [merge_request.iid, merge_request_closed.iid]
+
+        expect(response).to have_http_status(200)
+        expect(json_response).to be_an Array
+        expect(json_response.length).to eq(2)
+        expect(json_response.first['title']).to eq merge_request_closed.title
+        expect(json_response.first['id']).to eq merge_request_closed.id
+      end
+
       context "with ordering" do
         before do
           @mr_later = mr_with_later_created_and_updated_at_time
@@ -160,24 +170,6 @@ describe API::MergeRequests, api: true  do
       expect(json_response['force_close_merge_request']).to be_falsy
     end
 
-    it 'returns merge_request by iid' do
-      url = "/projects/#{project.id}/merge_requests?iid=#{merge_request.iid}"
-      get api(url, user)
-      expect(response.status).to eq 200
-      expect(json_response.first['title']).to eq merge_request.title
-      expect(json_response.first['id']).to eq merge_request.id
-    end
-
-    it 'returns merge_request by iid array' do
-      get api("/projects/#{project.id}/merge_requests", user), iid: [merge_request.iid, merge_request_closed.iid]
-
-      expect(response).to have_http_status(200)
-      expect(json_response).to be_an Array
-      expect(json_response.length).to eq(2)
-      expect(json_response.first['title']).to eq merge_request_closed.title
-      expect(json_response.first['id']).to eq merge_request_closed.id
-    end
-
     it "returns a 404 error if merge_request_id not found" do
       get api("/projects/#{project.id}/merge_requests/999", user)
       expect(response).to have_http_status(404)

spec/support/api_helpers.rb

@@ -17,8 +17,8 @@ module ApiHelpers
   #   => "/api/v2/issues?foo=bar&private_token=..."
   #
   # Returns the relative path to the requested API resource
-  def api(path, user = nil)
-    "/api/#{API::API.version}#{path}" +
+  def api(path, user = nil, version: API::API.version)
+    "/api/#{version}#{path}" +
 
       # Normalize query string
       (path.index('?') ? '' : '?') +
@@ -31,6 +31,11 @@ module ApiHelpers
       end
   end
 
+  # Temporary helper method for simplifying V3 exclusive API specs
+  def v3_api(path, user = nil)
+    api(path, user, version: 'v3')
+  end
+
   def ci_api(path, user = nil)
     "/ci/api/v1/#{path}" +