From cedbb3366bf3dd9bafe95dde366c1e28ee70c615 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 29 Mar 2019 15:16:31 -0700 Subject: [PATCH 1/2] Fix API /project/:id/branches not returning correct merge status https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/24034 introduced a regression where only the first 20 branches were used to determine whether a branch has been merged because the pagination was applied incorrectly. Requesting the second page of branches via the API would always have the wrong merge status. We fix this by properly paginating the branches before requesting their merge status. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/56250 --- .../sh-fix-project-branches-merge-status.yml | 5 +++++ lib/api/branches.rb | 4 ++-- spec/requests/api/branches_spec.rb | 22 ++++++++++++++++++- 3 files changed, 28 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/sh-fix-project-branches-merge-status.yml diff --git a/changelogs/unreleased/sh-fix-project-branches-merge-status.yml b/changelogs/unreleased/sh-fix-project-branches-merge-status.yml new file mode 100644 index 00000000000..65f41b3faf9 --- /dev/null +++ b/changelogs/unreleased/sh-fix-project-branches-merge-status.yml @@ -0,0 +1,5 @@ +--- +title: Fix API /project/:id/branches not returning correct merge status +merge_request: 26785 +author: +type: fixed diff --git a/lib/api/branches.rb b/lib/api/branches.rb index 07f529b01bb..5c98b0ad56c 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -34,11 +34,11 @@ class Branches < Grape::API repository = user_project.repository branches = BranchesFinder.new(repository, declared_params(include_missing: false)).execute - branches = ::Kaminari.paginate_array(branches) + branches = paginate(::Kaminari.paginate_array(branches)) merged_branch_names = repository.merged_branch_names(branches.map(&:name)) present( - paginate(branches), + branches, with: Entities::Branch, current_user: current_user, project: user_project, diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb index b38cd66986f..75dcedabf96 100644 --- a/spec/requests/api/branches_spec.rb +++ b/spec/requests/api/branches_spec.rb @@ -36,10 +36,30 @@ expect(branch_names).to match_array(project.repository.branch_names) end + def check_merge_status(json_response) + merged, unmerged = json_response.partition { |branch| branch['merged'] } + merged_branches = merged.map { |branch| branch['name'] } + unmerged_branches = unmerged.map { |branch| branch['name'] } + expect(Set.new(merged_branches)).to eq(project.repository.merged_branch_names(merged_branches + unmerged_branches)) + expect(project.repository.merged_branch_names(unmerged_branches)).to be_empty + end + it 'determines only a limited number of merged branch names' do - expect(API::Entities::Branch).to receive(:represent).with(anything, has_merged_branch_names_count(2)) + expect(API::Entities::Branch).to receive(:represent).with(anything, has_merged_branch_names_count(1)).and_call_original get api(route, current_user), params: { per_page: 2 } + + expect(response).to have_gitlab_http_status(200) + + check_merge_status(json_response) + end + + it 'merge status matches reality on paginated input' do + get api(route, current_user), params: { per_page: 20, page: 2 } + + expect(response).to have_gitlab_http_status(200) + + check_merge_status(json_response) end context 'when repository is disabled' do -- GitLab From 2675581cb9dbc37c81732ac69e2166da6ced0429 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 30 Mar 2019 05:28:37 -0700 Subject: [PATCH 2/2] Revise merged branch check to green light up to N branches The main point of this check is to ensure we aren't checking all branches, not that we have an exact count. --- spec/requests/api/branches_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb index 75dcedabf96..8b503777443 100644 --- a/spec/requests/api/branches_spec.rb +++ b/spec/requests/api/branches_spec.rb @@ -20,9 +20,9 @@ let(:route) { "/projects/#{project_id}/repository/branches" } shared_examples_for 'repository branches' do - RSpec::Matchers.define :has_merged_branch_names_count do |expected| + RSpec::Matchers.define :has_up_to_merged_branch_names_count do |expected| match do |actual| - actual[:merged_branch_names].count == expected + expected >= actual[:merged_branch_names].count end end @@ -45,7 +45,7 @@ def check_merge_status(json_response) end it 'determines only a limited number of merged branch names' do - expect(API::Entities::Branch).to receive(:represent).with(anything, has_merged_branch_names_count(1)).and_call_original + expect(API::Entities::Branch).to receive(:represent).with(anything, has_up_to_merged_branch_names_count(2)).and_call_original get api(route, current_user), params: { per_page: 2 } -- GitLab