From 7d97840cea6e9739ebc57bbc5a40d19dca744d8a Mon Sep 17 00:00:00 2001 From: Javiera Tapia Date: Mon, 4 Aug 2025 22:11:56 -0400 Subject: [PATCH 1/2] Use strong_memoize for tag_names and branch_names Use strong_memoize for tag_names and branch_names in projects/settings/repository_controller#show. This optimization reduces external dependencies and improves response times by keeping frequently accessed data available throughout the request lifecycle. This step will allow us to stop relying on the Redis cache to store tag_names and branch_names. Changelog: changed --- .../settings/repository_controller.rb | 17 ++++++++-- app/models/protectable_dropdown.rb | 9 +++-- .../shared/_protected_tag.html.haml | 2 +- .../shared/_protected_branch.html.haml | 2 +- spec/models/protectable_dropdown_spec.rb | 4 +-- .../settings/repository_controller_spec.rb | 33 +++++++++++++++++++ 6 files changed, 57 insertions(+), 10 deletions(-) create mode 100644 spec/requests/projects/settings/repository_controller_spec.rb diff --git a/app/controllers/projects/settings/repository_controller.rb b/app/controllers/projects/settings/repository_controller.rb index 9bb71de9f83d60..d94642007df104 100644 --- a/app/controllers/projects/settings/repository_controller.rb +++ b/app/controllers/projects/settings/repository_controller.rb @@ -87,18 +87,29 @@ def define_variables @deploy_keys = DeployKeysPresenter.new(@project, current_user: current_user) define_deploy_token_variables + define_ref_names define_protected_refs remote_mirror end + def define_ref_names + @tag_names = Gitlab::SafeRequestStore.fetch("tag_names:#{@project.id}") do + @project.repository.tag_names + end + + @branch_names = Gitlab::SafeRequestStore.fetch("branch_names:#{@project.id}") do + @project.repository.branch_names + end + end + # rubocop: disable CodeReuse/ActiveRecord def define_protected_refs @protected_branches = fetch_protected_branches(@project).preload_access_levels @protected_tags = @project.protected_tags.preload_access_levels.order(:name).page(pagination_params[:page]) @protected_branch = @project.protected_branches.new @protected_tag = @project.protected_tags.new + @protected_tags_count = @protected_tags.reduce(0) { |sum, tag| sum + tag.matching(@tag_names).size } - @protected_tags_count = @protected_tags.reduce(0) { |sum, tag| sum + tag.matching(@project.repository.tag_names).size } load_gon_index end # rubocop: enable CodeReuse/ActiveRecord @@ -139,11 +150,11 @@ def project_params_attributes end def protectable_tags_for_dropdown - { open_tags: ProtectableDropdown.new(@project, :tags).hash } + { open_tags: ProtectableDropdown.new(@project, :tags, ref_names: @tag_names).array } end def protectable_branches_for_dropdown - { open_branches: ProtectableDropdown.new(@project, :branches).hash } + { open_branches: ProtectableDropdown.new(@project, :branches, ref_names: @branch_names).array } end def define_deploy_token_variables diff --git a/app/models/protectable_dropdown.rb b/app/models/protectable_dropdown.rb index 855876f2ec9058..7e5c760bbbc684 100644 --- a/app/models/protectable_dropdown.rb +++ b/app/models/protectable_dropdown.rb @@ -7,11 +7,12 @@ class ProtectableDropdown tags: :tag_names }.freeze - def initialize(project, ref_type) + def initialize(project, ref_type, ref_names: nil) raise ArgumentError, "invalid ref type `#{ref_type}`" unless ref_type.in?(REF_TYPES) @project = project @ref_type = ref_type + @ref_names = ref_names.presence || get_ref_names end # Tags/branches which are yet to be individually protected @@ -21,13 +22,15 @@ def protectable_ref_names @protectable_ref_names ||= ref_names - non_wildcard_protected_ref_names end - def hash + def array protectable_ref_names.map { |ref_name| { text: ref_name, id: ref_name, title: ref_name } } end private - def ref_names + attr_reader :ref_names + + def get_ref_names @project.repository.public_send(ref_name_method) # rubocop:disable GitlabSecurity/PublicSend end diff --git a/app/views/projects/protected_tags/shared/_protected_tag.html.haml b/app/views/projects/protected_tags/shared/_protected_tag.html.haml index a801dc1d65d11f..f855ca5424aaa8 100644 --- a/app/views/projects/protected_tags/shared/_protected_tag.html.haml +++ b/app/views/projects/protected_tags/shared/_protected_tag.html.haml @@ -6,7 +6,7 @@ = gl_badge_tag s_('ProtectedTags|default'), variant: :info, class: 'gl-ml-2' %td{ data: { label: s_('ProtectedBranch|Last commit') }, class: '!gl-align-middle' } - if protected_tag.wildcard? - - matching_tags = protected_tag.matching(repository.tag_names) + - matching_tags = protected_tag.matching(@tag_names) = link_to pluralize(matching_tags.count, "matching tag"), project_protected_tag_path(@project, protected_tag) - else - if commit = protected_tag.commit diff --git a/app/views/protected_branches/shared/_protected_branch.html.haml b/app/views/protected_branches/shared/_protected_branch.html.haml index 06ee3c26811abb..7e213fe152b4b8 100644 --- a/app/views/protected_branches/shared/_protected_branch.html.haml +++ b/app/views/protected_branches/shared/_protected_branch.html.haml @@ -12,7 +12,7 @@ = gl_badge_tag s_('ProtectedBranch|default'), variant: :info - if protected_branch.wildcard? - - matching_branches = protected_branch.matching(repository.branch_names) + - matching_branches = protected_branch.matching(@branch_names) = link_to pluralize(matching_branches.count, "matching branch"), namespace_project_protected_branch_path(@project.namespace, @project, protected_branch) - elsif !protected_branch.commit %span.gl-text-subtle= s_('ProtectedBranch|Branch does not exist.') diff --git a/spec/models/protectable_dropdown_spec.rb b/spec/models/protectable_dropdown_spec.rb index a256f9e0ab16ca..ed26c27e0cb3c6 100644 --- a/spec/models/protectable_dropdown_spec.rb +++ b/spec/models/protectable_dropdown_spec.rb @@ -70,8 +70,8 @@ end end - describe '#hash' do - subject { dropdown.hash } + describe '#array' do + subject { dropdown.array } context 'for branches' do let(:ref_type) { :branches } diff --git a/spec/requests/projects/settings/repository_controller_spec.rb b/spec/requests/projects/settings/repository_controller_spec.rb new file mode 100644 index 00000000000000..0a223b3ff7f006 --- /dev/null +++ b/spec/requests/projects/settings/repository_controller_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::Settings::RepositoryController, feature_category: :source_code_management do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user, maintainer_of: project) } + + before do + sign_in(user) + end + + describe 'GET #show' do + context "with defined ref names", :request_store do + let(:tag_names) { %w[v1.0 v2.0] } + let(:branch_names) { %w[main develop] } + let(:tag_names_key) { "tag_names:#{project.id}" } + let(:branch_names_key) { "branch_names:#{project.id}" } + + it 'caches tag_names and branch_names in SafeRequestStore' do + allow(Gitlab::SafeRequestStore).to receive(:fetch).and_call_original + allow(project.repository).to receive_messages(tag_names: tag_names, branch_names: branch_names) + + expect(Gitlab::SafeRequestStore).to receive(:fetch).once.with(tag_names_key).and_call_original + expect(Gitlab::SafeRequestStore).to receive(:fetch).once.with(branch_names_key).and_call_original + + get project_settings_repository_path(project) + + expect(response).to have_gitlab_http_status(:ok) + end + end + end +end -- GitLab From 488107a7e63acf95da21234483f388b8155ae585 Mon Sep 17 00:00:00 2001 From: Javiera Tapia Date: Wed, 3 Sep 2025 23:58:04 -0400 Subject: [PATCH 2/2] Replace Gitlab::SafeRequestStore for strong_memoize --- .../settings/repository_controller.rb | 25 +++++++++------- .../settings/repository_controller_spec.rb | 30 +++++++++++-------- 2 files changed, 31 insertions(+), 24 deletions(-) diff --git a/app/controllers/projects/settings/repository_controller.rb b/app/controllers/projects/settings/repository_controller.rb index d94642007df104..5410b56b45770e 100644 --- a/app/controllers/projects/settings/repository_controller.rb +++ b/app/controllers/projects/settings/repository_controller.rb @@ -3,6 +3,8 @@ module Projects module Settings class RepositoryController < Projects::ApplicationController + include Gitlab::Utils::StrongMemoize + layout 'project_settings' before_action :authorize_admin_project! before_action :define_variables, only: [:create_deploy_token] @@ -87,20 +89,21 @@ def define_variables @deploy_keys = DeployKeysPresenter.new(@project, current_user: current_user) define_deploy_token_variables - define_ref_names + tag_names + branch_names define_protected_refs remote_mirror end - def define_ref_names - @tag_names = Gitlab::SafeRequestStore.fetch("tag_names:#{@project.id}") do - @project.repository.tag_names - end + def tag_names + @project.repository.tag_names + end + strong_memoize_attr :tag_names - @branch_names = Gitlab::SafeRequestStore.fetch("branch_names:#{@project.id}") do - @project.repository.branch_names - end + def branch_names + @project.repository.branch_names end + strong_memoize_attr :branch_names # rubocop: disable CodeReuse/ActiveRecord def define_protected_refs @@ -108,7 +111,7 @@ def define_protected_refs @protected_tags = @project.protected_tags.preload_access_levels.order(:name).page(pagination_params[:page]) @protected_branch = @project.protected_branches.new @protected_tag = @project.protected_tags.new - @protected_tags_count = @protected_tags.reduce(0) { |sum, tag| sum + tag.matching(@tag_names).size } + @protected_tags_count = @protected_tags.reduce(0) { |sum, tag| sum + tag.matching(tag_names).size } load_gon_index end @@ -150,11 +153,11 @@ def project_params_attributes end def protectable_tags_for_dropdown - { open_tags: ProtectableDropdown.new(@project, :tags, ref_names: @tag_names).array } + { open_tags: ProtectableDropdown.new(@project, :tags, ref_names: tag_names).array } end def protectable_branches_for_dropdown - { open_branches: ProtectableDropdown.new(@project, :branches, ref_names: @branch_names).array } + { open_branches: ProtectableDropdown.new(@project, :branches, ref_names: branch_names).array } end def define_deploy_token_variables diff --git a/spec/requests/projects/settings/repository_controller_spec.rb b/spec/requests/projects/settings/repository_controller_spec.rb index 0a223b3ff7f006..ff04e43c4959ee 100644 --- a/spec/requests/projects/settings/repository_controller_spec.rb +++ b/spec/requests/projects/settings/repository_controller_spec.rb @@ -11,23 +11,27 @@ end describe 'GET #show' do - context "with defined ref names", :request_store do - let(:tag_names) { %w[v1.0 v2.0] } - let(:branch_names) { %w[main develop] } - let(:tag_names_key) { "tag_names:#{project.id}" } - let(:branch_names_key) { "branch_names:#{project.id}" } + before do + allow(Project).to receive(:find_by_full_path).and_return(project) + end + + it 'renders the show template successfully' do + get project_settings_repository_path(project) - it 'caches tag_names and branch_names in SafeRequestStore' do - allow(Gitlab::SafeRequestStore).to receive(:fetch).and_call_original - allow(project.repository).to receive_messages(tag_names: tag_names, branch_names: branch_names) + expect(response).to have_gitlab_http_status(:ok) + expect(response).to render_template(:show) + end - expect(Gitlab::SafeRequestStore).to receive(:fetch).once.with(tag_names_key).and_call_original - expect(Gitlab::SafeRequestStore).to receive(:fetch).once.with(branch_names_key).and_call_original + it 'memoizes tag_names call' do + expect(project.repository).to receive(:tag_names).once.and_call_original + + get project_settings_repository_path(project) + end - get project_settings_repository_path(project) + it 'memoizes branch_names call' do + expect(project.repository).to receive(:branch_names).once.and_call_original - expect(response).to have_gitlab_http_status(:ok) - end + get project_settings_repository_path(project) end end end -- GitLab