Skip to content

G
gitlab-ce
Commit ef7523d9 authored by Igor Drozdov's avatar Igor Drozdov
Browse files
Options

Merge branch... 

Merge branch '8883-api-endpoint-put-projects-id-merge_requests-merge_request_iid-approvers-executes-more-than-100' into 'master'

Resolve "API endpoint PUT /projects/:id/merge_requests/:merge_request_iid/approvers executes more than 100 SQL queries"

See merge request gitlab-org/gitlab!57344
parents e482597a 07060a67
Hide whitespace changes
Inline Side-by-side
Showing with 5 additions and 204 deletions
doc/api/merge_request_approvals.md
View file @ ef7523d9
...@@ -703,81 +703,6 @@ POST /projects/:id/merge_requests/:merge_request_iid/approvals ...@@ -703,81 +703,6 @@ POST /projects/:id/merge_requests/:merge_request_iid/approvals
} }
``` ```
### Change allowed approvers for Merge Request
> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/183) in GitLab 10.6.
> - Moved to GitLab Premium in 13.9.
NOTE:
This API endpoint has been deprecated. Please use Approval Rule API instead.
If you are allowed to, you can change approvers and approver groups using
the following endpoint:
```plaintext
PUT /projects/:id/merge_requests/:merge_request_iid/approvers
```
**Important:** Approvers and groups not in the request are **removed**
**Parameters:**
| Attribute | Type | Required | Description |
|----------------------|---------|----------|-----------------------------------------------------------|
| `id` | integer | yes | The ID of a project |
| `merge_request_iid` | integer | yes | The IID of MR |
| `approver_ids` | Array | yes | An array of User IDs that can approve the MR |
| `approver_group_ids` | Array | yes | An array of Group IDs whose members can approve the MR |
```json
{
"id": 5,
"iid": 5,
"project_id": 1,
"title": "Approvals API",
"description": "Test",
"state": "opened",
"created_at": "2016-06-08T00:19:52.638Z",
"updated_at": "2016-06-08T21:20:42.470Z",
"merge_status": "cannot_be_merged",
"approvals_required": 2,
"approvals_left": 2,
"approved_by": [],
"approvers": [
{
"user": {
"name": "Administrator",
"username": "root",
"id": 1,
"state": "active",
"avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80\u0026d=identicon",
"web_url": "http://localhost:3000/root"
}
}
],
"approver_groups": [
{
"group": {
"id": 5,
"name": "group1",
"path": "group1",
"description": "",
"visibility": "public",
"lfs_enabled": false,
"avatar_url": null,
"web_url": "http://localhost/groups/group1",
"request_access_enabled": false,
"full_name": "group1",
"full_path": "group1",
"parent_id": null,
"ldap_cn": null,
"ldap_access": null
}
}
]
}
```
### Get the approval state of merge requests ### Get the approval state of merge requests
> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/13712) in GitLab 12.3. > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/13712) in GitLab 12.3.
......
ee/changelogs/unreleased/mgill-deprecate-mr-approvers.yml 0 → 100644
View file @ ef7523d9
---
title: Remove /projects/:id/merge_requests/:merge_request_iid/approvers endpoint
merge_request: 57344
author:
type: removed
ee/lib/ee/api/merge_request_approvals.rb
View file @ ef7523d9
...@@ -77,30 +77,6 @@ module EE ...@@ -77,30 +77,6 @@ module EE
present_approval(merge_request) present_approval(merge_request)
end end
# Deprecated in favor of approval rules API
desc 'Update approvers and approver groups' do
detail 'This feature was introduced in 10.6'
success ::EE::API::Entities::ApprovalState
end
params do
requires :approver_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce,
desc: 'Array of User IDs to set as approvers.'
requires :approver_group_ids, type: Array[Integer], coerce_with: ::API::Validations::Types::CommaSeparatedToIntegerArray.coerce,
desc: 'Array of Group IDs to set as approvers.'
end
put 'approvers' do
::Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/issues/8883')
merge_request = find_merge_request_with_access(params[:merge_request_iid], :update_approvers)
merge_request = ::MergeRequests::UpdateService.new(user_project, current_user, declared(params, include_parent_namespaces: false).merge(remove_old_approvers: true)).execute(merge_request)
# Merge request shouldn't be in an invalid state after the changes, but handling errors to be safe
handle_merge_request_errors!(merge_request)
present_approval(merge_request)
end
end end
end end
end end
......
ee/spec/requests/api/merge_request_approvals_spec.rb
View file @ ef7523d9
...@@ -333,111 +333,6 @@ RSpec.describe API::MergeRequestApprovals do ...@@ -333,111 +333,6 @@ RSpec.describe API::MergeRequestApprovals do
end end
end end
describe 'PUT :id/merge_requests/:merge_request_iid/approvers' do
shared_examples_for 'user allowed to change approvers' do
context 'when disable_overriding_approvers_per_merge_request is true on the project' do
before do
project.update(disable_overriding_approvers_per_merge_request: true)
end
it 'does not allow overriding approvers' do
expect do
put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", current_user),
params: { approver_ids: approver.id.to_s, approver_group_ids: group.id.to_s }
end.to not_change { merge_request.approvers.count }.and not_change { merge_request.approver_groups.count }
end
end
context 'when disable_overriding_approvers_per_merge_request is false on the project' do
before do
project.update(disable_overriding_approvers_per_merge_request: false)
end
it 'allows overriding approvers' do
expect do
put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", current_user),
params: { approver_ids: "#{approver.id},#{user2.id}", approver_group_ids: "#{group.id}" }
end.to change { merge_request.approvers.count }.from(0).to(2)
.and change { merge_request.approver_groups.count }.from(0).to(1)
expect(response).to have_gitlab_http_status(:ok)
expect(json_response['approvers'].map { |approver| approver['user'] }.map { |user| user['username'] }).to contain_exactly(approver.username, user2.username)
expect(json_response['approver_groups'][0]['group']['name']).to eq(group.name)
end
it 'removes approvers not in the payload' do
merge_request.approvers.create(user: approver)
merge_request.approver_groups.create(group: group)
expect do
put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", current_user),
params: { approver_ids: '', approver_group_ids: '' }.to_json, headers: { CONTENT_TYPE: 'application/json' }
end.to change { merge_request.approvers.count }.from(1).to(0)
.and change { merge_request.approver_groups.count }.from(1).to(0)
expect(response).to have_gitlab_http_status(:ok)
expect(json_response['approvers']).to eq([])
end
context 'when sending form-encoded data' do
it 'removes approvers not in the payload' do
merge_request.approvers.create(user: approver)
merge_request.approver_groups.create(group: group)
expect do
put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", current_user),
params: { approver_ids: '', approver_group_ids: '' }
end.to change { merge_request.approvers.count }.from(1).to(0)
.and change { merge_request.approver_groups.count }.from(1).to(0)
expect(response).to have_gitlab_http_status(:ok)
expect(json_response['approvers']).to eq([])
end
end
end
it 'only shows approver groups that are visible to current user' do
private_group = create(:group, :private)
merge_request.approver_groups.create(group: private_group)
put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", current_user),
params: { approver_ids: [approver.id], approver_group_ids: [private_group.id, group.id] }
expect(response).to have_gitlab_http_status(:ok)
expect(json_response['approver_groups'].size).to eq(expected_group_size)
end
end
context 'as a project admin' do
it_behaves_like 'user allowed to change approvers' do
let(:current_user) { user }
let(:expected_group_size) { 1 }
end
end
context 'as a global admin' do
it_behaves_like 'user allowed to change approvers' do
let(:current_user) { admin }
let(:expected_group_size) { 2 }
end
end
context 'as a random user' do
before do
project.update(disable_overriding_approvers_per_merge_request: false)
end
it 'does not allow overriding approvers' do
expect do
put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", user2),
params: { approver_ids: [approver.id], approver_group_ids: [group.id] }
end.to not_change { merge_request.approvers.count }.and not_change { merge_request.approver_groups.count }
expect(response).to have_gitlab_http_status(:forbidden)
end
end
end
describe 'POST :id/merge_requests/:merge_request_iid/approve' do describe 'POST :id/merge_requests/:merge_request_iid/approve' do
let!(:rule) { create(:approval_merge_request_rule, merge_request: merge_request, approvals_required: 2) } let!(:rule) { create(:approval_merge_request_rule, merge_request: merge_request, approvals_required: 2) }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7