From 819b1060bc96f400aaba9e91c8ffa848da961c15 Mon Sep 17 00:00:00 2001 From: rkumar555 Date: Wed, 22 Oct 2025 19:24:27 +0200 Subject: [PATCH] Fix the zoekt filters caching --- ee/lib/search/zoekt/cache.rb | 30 ++-- ee/lib/search/zoekt/search_results.rb | 8 +- ee/spec/lib/search/zoekt/cache_spec.rb | 128 ++++-------------- .../lib/search/zoekt/search_results_spec.rb | 6 +- 4 files changed, 49 insertions(+), 123 deletions(-) diff --git a/ee/lib/search/zoekt/cache.rb b/ee/lib/search/zoekt/cache.rb index 0a687df875dbe8..378ec98fed5235 100644 --- a/ee/lib/search/zoekt/cache.rb +++ b/ee/lib/search/zoekt/cache.rb @@ -8,9 +8,6 @@ class Cache MAX_PAGES = 10 EXPIRES_IN = 5.minutes - attr_reader :current_user, :query, :project_id, :group_id, :per_page, :current_page, :max_per_page, :search_mode, - :multi_match - def self.humanize_expires_in parts = EXPIRES_IN.parts unit = parts.each_key.first @@ -28,12 +25,7 @@ def initialize(query, **options) @max_per_page = options.fetch(:max_per_page) @search_mode = options.fetch(:search_mode) @multi_match = options.fetch(:multi_match, false) - end - - def enabled? - return false unless Gitlab::CurrentSettings.zoekt_cache_response? - - (project_id.present? || group_id.present?) && per_page <= max_per_page + @filters = options.fetch(:filters) end def fetch @@ -50,14 +42,23 @@ def fetch [search_results, total_count, file_count] end + private + + attr_reader :current_user, :query, :project_id, :group_id, :per_page, :current_page, :max_per_page, :search_mode, + :multi_match, :filters + + def enabled? + return false unless Gitlab::CurrentSettings.zoekt_cache_response? + + (project_id.present? || group_id.present?) && per_page <= max_per_page + end + def cache_key(page: current_page) user_id = current_user&.id || 0 # We need to use {user_id} as part of the key for Redis Cluster support "cache:zoekt:{#{user_id}}/#{search_fingerprint}/#{per_page}/#{page}" end - private - def with_redis(&block) Gitlab::Redis::Cache.with(&block) # rubocop:disable CodeReuse/ActiveRecord -- this has nothing to do with AR end @@ -70,10 +71,9 @@ def page_limit def search_fingerprint scope_key = "g#{group_id}-p#{project_id}" - multi_match_key = multi_match.present? ? multi_match.max_chunks_size : 'false' - - OpenSSL::Digest.hexdigest('SHA256', - "#{query}-#{scope_key}-#{search_mode}-#{multi_match_key}") + multi_match_key = multi_match.present? ? multi_match.max_chunks_size : 'f' + filters_key = filters.sort.join + OpenSSL::Digest.hexdigest('SHA256', "#{query}-#{scope_key}-#{search_mode}-#{multi_match_key}-#{filters_key}") end def read_cache diff --git a/ee/lib/search/zoekt/search_results.rb b/ee/lib/search/zoekt/search_results.rb index 297ed944851081..c0ed2a9a1fc9ee 100644 --- a/ee/lib/search/zoekt/search_results.rb +++ b/ee/lib/search/zoekt/search_results.rb @@ -230,6 +230,7 @@ def zoekt_search_and_wrap(query, per_page:, page: 1, preload_method: nil, &blk) group_id: options[:group_id], max_per_page: DEFAULT_PER_PAGE * 2, search_mode: search_mode, + filters: filters, multi_match: multi_match ) @@ -298,12 +299,7 @@ def filtered_projects(projects) def zoekt_targets sha = OpenSSL::Digest.hexdigest('SHA256', limit_project_ids.sort.join(',')) - cache_key = [ - self.class.name, - :zoekt_targets, - current_user&.id, - sha - ] + cache_key = [self.class.name, :zoekt_targets, current_user&.id, sha] Rails.cache.fetch(cache_key, expires_in: ZOEKT_TARGETS_CACHE_EXPIRES_IN) do ::Search::Zoekt::RoutingService.execute(projects) diff --git a/ee/spec/lib/search/zoekt/cache_spec.rb b/ee/spec/lib/search/zoekt/cache_spec.rb index e8264f131abfa3..6604bda81981f6 100644 --- a/ee/spec/lib/search/zoekt/cache_spec.rb +++ b/ee/spec/lib/search/zoekt/cache_spec.rb @@ -4,7 +4,7 @@ RSpec.describe Search::Zoekt::Cache, :clean_gitlab_redis_cache, feature_category: :global_search do let(:default_options) do - { per_page: 20, max_per_page: 40, search_mode: :regex } + { per_page: 20, max_per_page: 40, search_mode: :regex, filters: {} } end let(:query) { 'foo' } @@ -28,119 +28,47 @@ stub_const("#{described_class.name}::MAX_PAGES", 2) end - describe '#cache_key' do - let(:multi_match_1) { instance_double(::Search::Zoekt::MultiMatch, max_chunks_size: 1) } - let(:multi_match_2) { instance_double(::Search::Zoekt::MultiMatch, max_chunks_size: 5) } - let(:uniq_examples) do - [ - { current_user: build(:user, id: 1), query: 'foo', project_id: 3, group_id: 2, page: 1 }, - { current_user: build(:user, id: 1), query: 'foo', project_id: 3, group_id: 2, page: 1, per_page: 10 }, - { current_user: build(:user, id: 1), query: 'bar', project_id: 3, group_id: 2, page: 1 }, - { current_user: build(:user, id: 1), query: 'foo', project_id: nil, group_id: 2, page: 1 }, - { current_user: build(:user, id: 1), query: 'foo', project_id: 3, group_id: 2, page: 2 }, - { current_user: build(:user, id: 2), query: 'foo', project_id: 3, group_id: 2, page: 1 }, - { current_user: build(:user, id: nil), query: 'foo', project_id: 3, group_id: 2, page: 1 }, - { current_user: build(:user, id: 1), query: 'foo', project_id: 3, group_id: 2, page: 1, - multi_match: multi_match_1 }, - { current_user: build(:user, id: 1), query: 'foo', project_id: 3, group_id: 2, page: 1, - multi_match: multi_match_2 } - ] - end - - let(:duplicate_examples) do - [ - { current_user: build(:user, id: 1), query: 'foo', project_id: 1, group_id: 2, page: 1, per_page: 20 }, - { current_user: build(:user, id: 1), query: 'foo', project_id: 1, group_id: 2, page: 1, per_page: 20 } - ] - end - - it 'returns unique cache keys for different queries' do - result = {} - - uniq_examples.each do |e| - cache_key = described_class.new(e[:query], **default_options.merge(e.except(:query))).cache_key - - result[cache_key] ||= [] - result[cache_key] << e - end + describe '#fetch' do + context 'when application setting zoekt_cache_response is disabled', :zoekt_cache_disabled do + it 'does not read or update cache' do + expect(cache).not_to receive(:read_cache) + expect(cache).not_to receive(:update_cache!) - result.each do |key, examples| - expect(examples.size).to eq(1), "#{key} has duplicate examples: #{examples}" - end - end + data = cache.fetch do |page_limit| + expect(page_limit).to eq(page) + response + end - it 'returns the same cache key for duplicate queries' do - cache_keys = duplicate_examples.map do |e| - described_class.new(e[:query], **default_options.merge(e.except(:query))).cache_key + expect(data).to eq(response) end - - expect(cache_keys.uniq.size).to eq(1) end - end - describe '#enabled?' do - it 'returns true' do - expect(cache.enabled?).to be true - end - - context 'when application setting zoekt_cache_response is disabled' do - before do - stub_ee_application_setting(zoekt_cache_response: false) - end - - it 'returns false' do - expect(cache.enabled?).to be false - end - end + context 'when read_cache returns nothing' do + it 'updates cache' do + expect(cache).to receive(:read_cache) + expect(cache).to receive(:update_cache!) - context 'when per_page is more than max_per_page' do - let(:default_options) do - { per_page: 40, max_per_page: 20, search_mode: :regex } - end + data = cache.fetch do |page_limit| + expect(page_limit).to eq(described_class::MAX_PAGES) + response + end - it 'returns false' do - expect(cache.enabled?).to be false + expect(data).to eq(response) end end - context 'when project_id is not present' do - let(:project_id) { nil } + context 'when read_cache returns data' do + it 'does not update cache' do + expect(cache).to receive(:read_cache).and_return([search_results, total_count, file_count]) + expect(cache).not_to receive(:update_cache!) - it 'returns true if group_id is present' do - expect(group_id).to be_present - expect(cache.enabled?).to be true - end - - context 'and group_id is not present' do - let(:group_id) { nil } - - it 'returns false' do - expect(cache.enabled?).to be false + data = cache.fetch do |page_limit| + expect(page_limit).to eq(described_class::MAX_PAGES) + response end - end - end - - context 'when group_id is not present' do - let(:group_id) { nil } - it 'returns true if project_id is present' do - expect(project_id).to be_present - expect(cache.enabled?).to be true - end - end - end - - describe '#fetch' do - it 'reads and updates cache' do - expect(cache).to receive(:read_cache) - expect(cache).to receive(:update_cache!) - - data = cache.fetch do |page_limit| - expect(page_limit).to eq(described_class::MAX_PAGES) - response + expect(data).to eq(response) end - - expect(data).to eq(response) end context 'when page is higher than the limit' do diff --git a/ee/spec/lib/search/zoekt/search_results_spec.rb b/ee/spec/lib/search/zoekt/search_results_spec.rb index cbdc93153fdafb..9fa77ff137144b 100644 --- a/ee/spec/lib/search/zoekt/search_results_spec.rb +++ b/ee/spec/lib/search/zoekt/search_results_spec.rb @@ -69,7 +69,8 @@ project_id: nil, group_id: nil, search_mode: :regex, - multi_match: nil + multi_match: nil, + filters: {} ).and_call_original objects @@ -89,7 +90,8 @@ project_id: nil, group_id: nil, search_mode: :regex, - multi_match: an_instance_of(Search::Zoekt::MultiMatch) + multi_match: an_instance_of(Search::Zoekt::MultiMatch), + filters: {} ).and_call_original objects -- GitLab