From cca26c15c6f08b2afccf46ca2309114361c89da2 Mon Sep 17 00:00:00 2001 From: ghinfey Date: Fri, 12 Dec 2025 13:12:25 +0000 Subject: [PATCH] Add default branch to top results for branch rules pagination Updates the branch rules finder to include correct sorting of default branch for branch rules pagination. Changelog: changed MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/216283 --- app/finders/projects/branch_rules_finder.rb | 29 +++++++++++++++++-- app/models/protected_branch.rb | 2 ++ .../projects/branch_rules_finder_spec.rb | 27 +++++++++++++++++ spec/models/protected_branch_spec.rb | 25 ++++++++++++++++ 4 files changed, 80 insertions(+), 3 deletions(-) diff --git a/app/finders/projects/branch_rules_finder.rb b/app/finders/projects/branch_rules_finder.rb index b8afddbbfd658f..77908c660fdde8 100644 --- a/app/finders/projects/branch_rules_finder.rb +++ b/app/finders/projects/branch_rules_finder.rb @@ -16,6 +16,8 @@ def initialize(project, custom_rules:, protected_branches:) @project = project @custom_rules = custom_rules @protected_branches = protected_branches + @default_branch = project.default_branch + @default_protected_branch = ProtectedBranch.default_branch_for(project) if @default_branch end def execute(cursor: nil, limit: DEFAULT_LIMIT) @@ -30,7 +32,7 @@ def execute(cursor: nil, limit: DEFAULT_LIMIT) private - attr_reader :project, :custom_rules, :protected_branches + attr_reader :project, :custom_rules, :protected_branches, :default_branch, :default_protected_branch def paginate_from_start(limit) return custom_rules_page(limit) if custom_rules.size > limit @@ -52,13 +54,18 @@ def paginate_after_protected_branch_rule(after_cursor, limit) # Only apply cursor if we have both name and id query = query.after_name_and_id(after_cursor['name'], after_cursor['id']) if after_cursor && after_cursor['id'] - # Add one extra to check if a next page exists - branches = query.limit(limit + 1).to_a + # Exclude the default branch from the query if we're not on the first page + query = query.excluding_name(@default_branch) if after_cursor && @default_branch + + fetch_limit = calculate_fetch_limit(after_cursor, limit) + branches = query.limit(fetch_limit).to_a return Page.new if branches.empty? has_next = branches.size > limit page_branches = branches.first(limit) + page_branches = prioritise_default_branch(page_branches) if first_page_with_default_branch?(after_cursor) + cursor = has_next ? encode_cursor(page_branches.last.name, page_branches.last.id) : nil Page.new( @@ -68,6 +75,22 @@ def paginate_after_protected_branch_rule(after_cursor, limit) ) end + def calculate_fetch_limit(after_cursor, limit) + # On first page, we'll prepend the default branch, so fetch one fewer + return limit if first_page_with_default_branch?(after_cursor) + + limit + 1 + end + + def first_page_with_default_branch?(after_cursor) + @default_protected_branch && !after_cursor + end + + def prioritise_default_branch(page_branches) + filtered = page_branches - [@default_protected_branch] + [@default_protected_branch] + filtered + end + def custom_rules_page(limit) rules = custom_rules.first(limit) diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index 200f1537e3400b..927abc87d7bd5e 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -29,6 +29,8 @@ class ProtectedBranch < ApplicationRecord where('(name > ?) OR (name = ? AND id > ?)', name, name, id).order(:name, :id) } + scope :excluding_name, ->(name) { where.not(name: name) if name.present? } + protected_ref_access_levels :merge, :push def self.get_ids_by_name(name) diff --git a/spec/finders/projects/branch_rules_finder_spec.rb b/spec/finders/projects/branch_rules_finder_spec.rb index bd93effe868f43..76c595008d0ce6 100644 --- a/spec/finders/projects/branch_rules_finder_spec.rb +++ b/spec/finders/projects/branch_rules_finder_spec.rb @@ -209,6 +209,33 @@ end end + context 'with default branch' do + before do + create(:protected_branch, project: project, name: project.default_branch) + create(:protected_branch, project: project, name: 'zzz-branch-a') + create(:protected_branch, project: project, name: 'zzz-branch-b') + end + + describe 'first page' do + it 'prioritises default branch' do + page = finder.execute(cursor: nil, limit: 3) + + expect(page.rules.first).to eq(all_branches_rule) + expect(page.rules.second.protected_branch.name).to eq(project.default_branch) + end + end + + describe 'second page' do + it 'excludes the default branch' do + first_page = finder.execute(cursor: nil, limit: 3) + second_page = finder.execute(cursor: first_page.end_cursor, limit: 3) + + branch_names = second_page.rules.filter_map { |rule| rule.protected_branch&.name } + expect(branch_names).not_to include(project.default_branch) + end + end + end + context 'for cursor edge cases' do let!(:protected_branch) { create(:protected_branch, project: project, name: 'main') } diff --git a/spec/models/protected_branch_spec.rb b/spec/models/protected_branch_spec.rb index b4e4265882cb2d..af6b87427a2e95 100644 --- a/spec/models/protected_branch_spec.rb +++ b/spec/models/protected_branch_spec.rb @@ -316,6 +316,31 @@ end end + describe '.excluding_name' do + let_it_be(:project) { create(:project) } + let_it_be(:branch_a) { create(:protected_branch, project: project, name: 'a') } + let_it_be(:branch_b) { create(:protected_branch, project: project, name: 'b') } + let_it_be(:branch_c) { create(:protected_branch, project: project, name: 'c') } + + it 'excludes the branch with the given name' do + result = described_class.where(project: project).excluding_name('b') + + expect(result).to contain_exactly(branch_a, branch_c) + end + + it 'returns all branches when name is nil' do + result = described_class.where(project: project).excluding_name(nil) + + expect(result).to contain_exactly(branch_a, branch_b, branch_c) + end + + it 'returns all branches when name is empty string' do + result = described_class.where(project: project).excluding_name('') + + expect(result).to contain_exactly(branch_a, branch_b, branch_c) + end + end + describe '#default_branch?' do context 'when group level' do subject { build_stubbed(:protected_branch, project: nil, group: build(:group)) } -- GitLab