From c8b2f201c51dedc6450ac6316c393384e32e9894 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Fri, 4 Sep 2026 13:32:22 -0400 Subject: [PATCH 1/6] Refactor search caching to store normalized data Why are these changes being introduced: * We currently store raw data from Primo and Timdex, which is larger than necessary. * Load More functionality started to introduce a new caching mechanishm, but it still fell back on the raw data caches and was only in place for the All tab. * Our cache is regular hitting the max data storage size for our redis tier, and rather than expanding it further (more money), it felt worth considering options to store data more efficiently. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/USE-701 How does this address that need: * Refactors flow to compute and check for normalized search data prior to running external queries. * Includes ADR documenting this change. --- app/controllers/search_controller.rb | 144 +++++++++++------ .../0003-cache-normalized-search-results.md | 152 ++++++++++++++++++ test/controllers/search_controller_test.rb | 50 ++++++ 3 files changed, 297 insertions(+), 49 deletions(-) create mode 100644 docs/architecture-decisions/0003-cache-normalized-search-results.md diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 58799bc6..a97f2fe2 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -1,4 +1,10 @@ class SearchController < ApplicationController + # Increment this namespace when the normalized search result payload changes in a way that + # makes existing cached values incomplete or incompatible, such as adding required fields, + # removing/renaming fields, changing key types, or changing error/continuation metadata shape. + SEARCH_RESULTS_CACHE_NAMESPACE = 'normalized-search-results/v1' + SEARCH_RESULTS_CACHE_TTL = 12.hours + before_action :validate_q!, only: %i[results] before_action :authorized_request?, only: %i[results] before_action :set_active_tab, only: %i[results] @@ -202,11 +208,40 @@ def fetch_primo_data(offset: nil, per_page: nil) return { results: [], pagination: {}, errors: nil, show_continuation: true, hits: 0 } end - primo_response = query_primo(per_page, offset) - hits = primo_response.dig('info', 'total') || 0 - results = NormalizePrimoResults.new(primo_response, @enhanced_query[:q]).normalize - pagination = Analyzer.new(@enhanced_query, hits, :primo).pagination + cached = cached_primo_data(per_page, offset) + pagination = Analyzer.new(@enhanced_query, cached[:hits], :primo).pagination + + cached.merge(pagination: pagination) + rescue StandardError => e + { results: [], pagination: {}, errors: handle_primo_errors(e), show_continuation: false, hits: 0 } + end + + def fetch_timdex_data(offset: nil, per_page: nil) + query = QueryBuilder.new(@enhanced_query).query + query['from'] = offset.to_s if offset + query['perPage'] = per_page || ENV.fetch('RESULTS_PER_PAGE', '20').to_i + query['fulltext'] = true if Feature.enabled?(:timdex_fulltext) + + cached = cached_timdex_data(query) + return cached.merge(pagination: {}) if cached[:errors] + + pagination = Analyzer.new(@enhanced_query, cached[:hits], :timdex).pagination + cached.merge(pagination: pagination) + end + + def cached_primo_data(per_page, offset) + cache_key = CacheKeyGenerator.call(@enhanced_query.merge(per_page: per_page, offset: offset)) + + Rails.cache.fetch("#{SEARCH_RESULTS_CACHE_NAMESPACE}/#{cache_key}/primo", expires_in: SEARCH_RESULTS_CACHE_TTL) do + primo_response = query_primo(per_page, offset) + hits = primo_response.dig('info', 'total') || 0 + results = NormalizePrimoResults.new(primo_response, @enhanced_query[:q]).normalize + + build_primo_cache_payload(primo_response, results, hits, offset) + end + end + def build_primo_cache_payload(primo_response, results, hits, offset) # Handle empty results from Primo API. Sometimes Primo will return no results at a given offset, # despite claiming in the initial query that more are available. This happens randomly and # seemingly for no reason (well below the recommended offset of 2,000). While the bug also @@ -225,29 +260,26 @@ def fetch_primo_data(offset: nil, per_page: nil) end end - { results: results, pagination: pagination, errors: errors, show_continuation: show_continuation, - hits: hits } - rescue StandardError => e - { results: [], pagination: {}, errors: handle_primo_errors(e), show_continuation: false, hits: 0 } + { results: results, errors: errors, show_continuation: show_continuation, hits: hits } end - def fetch_timdex_data(offset: nil, per_page: nil) - query = QueryBuilder.new(@enhanced_query).query - query['from'] = offset.to_s if offset - query['perPage'] = per_page || ENV.fetch('RESULTS_PER_PAGE', '20').to_i - query['fulltext'] = true if Feature.enabled?(:timdex_fulltext) + def cached_timdex_data(query) + prepare_timdex_query(query) + cache_key = CacheKeyGenerator.call(query) - response = query_timdex(query) - errors = extract_errors(response) - - if errors.nil? - hits = response.dig(:data, 'search', 'hits') || 0 - pagination = Analyzer.new(@enhanced_query, hits, :timdex).pagination - raw_results = extract_results(response) - results = NormalizeTimdexResults.new(raw_results, @enhanced_query[:q]).normalize - { results: results, pagination: pagination, errors: nil, hits: hits } - else - { results: [], pagination: {}, errors: errors, hits: 0 } + Rails.cache.fetch("#{SEARCH_RESULTS_CACHE_NAMESPACE}/#{cache_key}/#{@active_tab}", + expires_in: SEARCH_RESULTS_CACHE_TTL) do + response = serialize_timdex_response(execute_timdex_query(query)) + errors = extract_errors(response) + + if errors.nil? + hits = response.dig(:data, 'search', 'hits') || 0 + raw_results = extract_results(response) + results = NormalizeTimdexResults.new(raw_results, @enhanced_query[:q]).normalize + { results: results, errors: nil, hits: hits } + else + { results: [], errors: errors, hits: 0 } + end end end @@ -256,6 +288,27 @@ def active_filters end def query_timdex(query) + prepare_timdex_query(query) + + # We generate unique cache keys to avoid naming collisions. + cache_key = CacheKeyGenerator.call(query) + + # Builder hands off to wrapper which returns raw results here. + Rails.cache.fetch("#{cache_key}/#{@active_tab}", expires_in: 12.hours) do + serialize_timdex_response(execute_timdex_query(query)) + end + end + + def query_primo(per_page, offset) + Rails.logger.debug do + "External Primo search request: tab=#{@enhanced_query[:tab]}, offset=#{offset}, per_page=#{per_page}" + end + + primo_search = PrimoSearch.new(@enhanced_query[:tab]) + primo_search.search(@enhanced_query[:q], per_page, offset) + end + + def prepare_timdex_query(query) query[:sourceFilter] = ['MIT Libraries Website', 'LibGuides'] if @active_tab == 'website' query[:sourceFilter] = ['MIT ArchivesSpace'] if @active_tab == 'aspace' query[:sourceFilter] = ['MIT Alma'] if @active_tab == 'timdex_alma' @@ -265,37 +318,30 @@ def query_timdex(query) query[:sourceFilter] = ['Digital Collections'] if @active_tab == 'digital_collections' query[:useGlobalScoring] = Feature.enabled?(:global_scoring) - # We generate unique cache keys to avoid naming collisions. - cache_key = CacheKeyGenerator.call(query) - - # Builder hands off to wrapper which returns raw results here. - Rails.cache.fetch("#{cache_key}/#{@active_tab}", expires_in: 12.hours) do - raw = if Feature.enabled?(:geodata) - execute_geospatial_query(query) - else - TimdexBase::Client.query(TimdexSearch::BaseQuery, variables: query) - end - - # The response type is a GraphQL::Client::Response, which is not directly serializable, so we - # convert it to a hash. - { - data: raw.data.to_h, - errors: raw.errors.details.to_h - } - end + query end - def query_primo(per_page, offset) - # We generate unique cache keys to avoid naming collisions. - # Include per_page and offset in the cache key to ensure pagination works correctly. - cache_key = CacheKeyGenerator.call(@enhanced_query.merge(per_page: per_page, offset: offset)) + def execute_timdex_query(query) + Rails.logger.debug do + "External TIMDEX search request: tab=#{@active_tab}, from=#{query['from'] || 0}, per_page=#{query['perPage']}" + end - Rails.cache.fetch("#{cache_key}/primo", expires_in: 12.hours) do - primo_search = PrimoSearch.new(@enhanced_query[:tab]) - primo_search.search(@enhanced_query[:q], per_page, offset) + if Feature.enabled?(:geodata) + execute_geospatial_query(query) + else + TimdexBase::Client.query(TimdexSearch::BaseQuery, variables: query) end end + def serialize_timdex_response(raw) + # The response type is a GraphQL::Client::Response, which is not directly serializable, so we + # convert it to a hash. + { + data: raw.data.to_h, + errors: raw.errors.details.to_h + } + end + def execute_geospatial_query(query) query = query.except('queryMode') diff --git a/docs/architecture-decisions/0003-cache-normalized-search-results.md b/docs/architecture-decisions/0003-cache-normalized-search-results.md new file mode 100644 index 00000000..c9dc4ee1 --- /dev/null +++ b/docs/architecture-decisions/0003-cache-normalized-search-results.md @@ -0,0 +1,152 @@ +# 3. Cache normalized search results + +Date: 2026-09-04 + +## Status + +Accepted + +## Context + +TIMDEX UI fetches search results from both Primo and TIMDEX. These provider responses are larger than the normalized +result hashes the application uses to render search results. + +`SearchController#query_timdex` currently caches raw TIMDEX GraphQL response hashes after converting them from +`GraphQL::Client::Response` objects. `SearchController#query_primo` currently caches raw Primo API responses. In both +cases, `SearchController#fetch_timdex_data` and `SearchController#fetch_primo_data` normalize after cache retrieval, so +raw cache hits avoid external provider calls but still rerun normalization. + +The all-tab load-more path has an additional cache layer in `MergedSearchService#fetch_load_more`. That state cache +stores normalized `primo_results` and `timdex_results`, along with ordered result keys, hit counts, exhaustion flags, +and errors. However, the all-tab service is populated by controller fetchers that currently call the raw single-source +caches. A single all-tab request can therefore leave both raw source cache entries and normalized all-tab state entries +in Redis. + +This creates avoidable duplication and repeated normalization work. It also means the cache strategy differs by path: +single-source Primo and TIMDEX searches cache provider-shaped data, while all-tab load-more caches application-shaped +data. + +## Decision + +We will cache normalized search result payloads for single-source Primo and TIMDEX result requests instead of caching +raw provider responses. + +For these requests, cache lookup will happen before any external Primo or TIMDEX API call and before any normalizer is +instantiated. On cache hit, the application will return the cached normalized payload directly. Cache hits must not call +Primo, TIMDEX, `NormalizePrimoResults`, or `NormalizeTimdexResults`. + +Single-source Primo and TIMDEX result requests will use this flow: + +```text +Single-source search request + │ + ▼ +Build source-specific cache key + │ + ▼ +Check normalized search result cache + │ + ├── Cache hit + │ │ + │ ├── Read normalized payload + │ │ └── includes results, hits, errors, and continuation metadata + │ │ + │ └── Build request-specific pagination and view data + │ └── no Primo/TIMDEX API call and no normalization + │ + └── Cache miss + │ + ├── Call Primo or TIMDEX + │ + ├── Normalize provider response once + │ + ├── Write normalized payload to cache + │ └── use explicit cache schema versioning or namespacing + │ + └── Build request-specific pagination and view data + │ + ▼ +Render results +``` + +On cache miss, the application will call the external provider, normalize the response once, write the normalized +payload to cache, and return it. Cached payloads must include enough metadata to preserve current behavior, including +normalized `results`, `hits`, `errors`, and Primo `show_continuation` information where applicable. + +We will use explicit cache schema versioning or namespacing so existing raw cached values are not read as normalized +payloads. We will preserve the current query-, tab-, offset-, and per-page-sensitive cache key behavior unless +implementation work identifies a specific reason to change it. + +The all-tab load-more cache will remain application-shaped. This decision brings the single-source Primo and TIMDEX +cache strategy into alignment with that path and avoids storing raw source payloads for records that are already stored +in normalized form. + +We will accept that all-tab load-more requests may store normalized source results twice: once in the source result +cache and once in the all-tab state cache. The all-tab state is not just a copy of source records. It also stores the +reranked result pool, stable display order, hit counts, source exhaustion flags, and errors needed to preserve the +load-more experience across requests. + +All-tab load-more result requests will use this flow: + +```text +All-tab load-more request + │ + ▼ +Build all-tab state cache key + │ + ▼ +Check reranked all-tab state cache + │ + ├── Enough cached state exists + │ │ + │ ├── Read reranked all-tab state + │ │ └── includes normalized source results, ordered keys, hits, exhaustion flags, and errors + │ │ + │ └── Return requested stable display slice + │ └── no Primo/TIMDEX API call and no normalization + │ + └── More source candidates needed + │ + ├── Fetch next Primo and/or TIMDEX source chunk + │ │ + │ ├── Check single source normalized source result cache (see above diagram) + │ │ ├── hit → read unsorted normalized source payload + │ │ └── miss → call provider, normalize once, write source cache + │ │ + │ └── Return normalized source results + │ + ├── Add normalized source results to all-tab state + │ + ├── Rerank candidate pool while preserving already-visible order + │ + ├── Write reranked all-tab state to cache + │ + └── Return requested stable display slice + │ + ▼ +Render results +``` + +## Consequences + +Redis should store smaller, application-shaped payloads for single-source search results. Cache hits should avoid both +external API calls and normalization work. + +All-tab load-more may continue to use more cache space than a single-source request because it stores state for the +reranked result set. This is intentional. The additional state lets the application preserve already-visible result +order while adding newly fetched and reranked candidates on later load-more requests. + +The application will become more dependent on the normalized record contract. Changes to normalized record shape are +also cache-shape changes, so cache versioning or namespacing will be necessary when that contract changes. For example, +if we add a new normalized `availability` field that search result views expect to be present, existing cached +normalized payloads would not include that field. The implementation should use a new cache namespace or schema version +for that change so requests do not read older cached payloads that no longer match the current normalized record +contract. + +The raw provider response will no longer be available from the search result cache. If future behavior requires fields +that are not present in normalized results, those fields should be added deliberately to the normalized payload rather +than relying on provider-specific raw data. + +Tests should verify that cached single-source Primo and TIMDEX results preserve user-visible behavior, including hit +counts, pagination behavior, errors, and Primo continuation behavior. Tests should also verify that cache hits avoid +external provider calls and normalizer instantiation. diff --git a/test/controllers/search_controller_test.rb b/test/controllers/search_controller_test.rb index 95b19c82..6097c3b7 100644 --- a/test/controllers/search_controller_test.rb +++ b/test/controllers/search_controller_test.rb @@ -870,6 +870,56 @@ def source_filter_count(controller) end end + test 'primo cache hit avoids external search and normalization' do + sample_doc = { + api: 'primo', + title: 'Cached Primo Document Title', + format: 'Article', + year: '2025', + creators: [{ value: 'Foo Barston', link: nil }], + identifier: 'primo-record-123', + links: [{ 'kind' => 'full record', 'url' => 'https://example.com/primo-record' }] + } + + mock_primo = mock('primo_search') + mock_primo.expects(:search).once.returns({ 'docs' => [sample_doc], 'info' => { 'total' => 1 } }) + PrimoSearch.expects(:new).once.returns(mock_primo) + + mock_normalizer = mock('normalizer') + mock_normalizer.expects(:normalize).once.returns([sample_doc]) + NormalizePrimoResults.expects(:new).once.returns(mock_normalizer) + + 2.times do + get '/results?q=test&tab=primo' + assert_response :success + assert_select '.record-title', text: /Cached Primo Document Title/ + end + end + + test 'timdex cache hit avoids external search and normalization' do + normalized_result = { + api: 'timdex', + title: 'Cached TIMDEX Document Title', + format: 'Article', + year: '2025', + creators: [{ value: 'Foo Barston', link: nil }], + identifier: 'timdex-record-123', + links: [{ 'kind' => 'full record', 'url' => 'https://example.com/timdex-record' }] + } + + TimdexBase::Client.expects(:query).once.returns(build_timdex_mock_response) + + mock_normalizer = mock('normalizer') + mock_normalizer.expects(:normalize).once.returns([normalized_result]) + NormalizeTimdexResults.expects(:new).once.returns(mock_normalizer) + + 2.times do + get '/results?q=test&tab=timdex' + assert_response :success + assert_select '.record-title', text: /Cached TIMDEX Document Title/ + end + end + test 'results shows tab navigation when GeoData is disabled' do mock_primo_search_success From c1aa0a30ec08a1cbb971e500bc61c802a646d236 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Fri, 4 Sep 2026 15:02:54 -0400 Subject: [PATCH 2/6] Move geodata to new caching strategy Note: geodata and non-geodata timdex queries follow a slightly different process due to geodata loading aggregations and filters. When we refactor the view logic to bring geodata up to date visually, we may want to also consider normalizing how all timdex queries (geo or otherwise) flow through the controller. --- app/controllers/search_controller.rb | 35 ++++------- .../controllers/search_controller_geo_test.rb | 61 +++++++++++++++++++ 2 files changed, 72 insertions(+), 24 deletions(-) diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index a97f2fe2..f9ae397d 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -89,18 +89,14 @@ def sleep_if_too_fast def load_geodata_results query = QueryBuilder.new(@enhanced_query).query + cached = cached_timdex_data(query, include_filters: true) - response = query_timdex(query) - - # Handle errors - @errors = extract_errors(response) + @errors = cached[:errors] return unless @errors.nil? - hits = response.dig(:data, 'search', 'hits') || 0 - @pagination = Analyzer.new(@enhanced_query, hits, :timdex).pagination - raw_results = extract_results(response) - @results = NormalizeTimdexResults.new(raw_results, @enhanced_query[:q]).normalize - @filters = extract_filters(response) + @pagination = Analyzer.new(@enhanced_query, cached[:hits], :timdex).pagination + @results = cached[:results] + @filters = cached[:filters] return unless @pagination_load_more_enabled @append_results = @results @@ -263,9 +259,10 @@ def build_primo_cache_payload(primo_response, results, hits, offset) { results: results, errors: errors, show_continuation: show_continuation, hits: hits } end - def cached_timdex_data(query) + def cached_timdex_data(query, include_filters: false) prepare_timdex_query(query) - cache_key = CacheKeyGenerator.call(query) + cache_query = include_filters ? query.merge(active_filters: active_filters) : query + cache_key = CacheKeyGenerator.call(cache_query) Rails.cache.fetch("#{SEARCH_RESULTS_CACHE_NAMESPACE}/#{cache_key}/#{@active_tab}", expires_in: SEARCH_RESULTS_CACHE_TTL) do @@ -276,7 +273,9 @@ def cached_timdex_data(query) hits = response.dig(:data, 'search', 'hits') || 0 raw_results = extract_results(response) results = NormalizeTimdexResults.new(raw_results, @enhanced_query[:q]).normalize - { results: results, errors: nil, hits: hits } + payload = { results: results, errors: nil, hits: hits } + payload[:filters] = extract_filters(response) if include_filters + payload else { results: [], errors: errors, hits: 0 } end @@ -287,18 +286,6 @@ def active_filters ENV.fetch('ACTIVE_FILTERS', '').split(',').map(&:strip) end - def query_timdex(query) - prepare_timdex_query(query) - - # We generate unique cache keys to avoid naming collisions. - cache_key = CacheKeyGenerator.call(query) - - # Builder hands off to wrapper which returns raw results here. - Rails.cache.fetch("#{cache_key}/#{@active_tab}", expires_in: 12.hours) do - serialize_timdex_response(execute_timdex_query(query)) - end - end - def query_primo(per_page, offset) Rails.logger.debug do "External Primo search request: tab=#{@enhanced_query[:tab]}, offset=#{offset}, per_page=#{per_page}" diff --git a/test/controllers/search_controller_geo_test.rb b/test/controllers/search_controller_geo_test.rb index a1813af6..eff75483 100644 --- a/test/controllers/search_controller_geo_test.rb +++ b/test/controllers/search_controller_geo_test.rb @@ -2,6 +2,33 @@ # Geospatial search behavior class SearchControllerGeoTest < ActionDispatch::IntegrationTest + def build_geodata_timdex_mock_response + sample_result = { + 'title' => 'Raw GeoData Document Title', + 'timdexRecordId' => 'geodata-record-123', + 'contentType' => ['Dataset'], + 'dates' => [{ 'kind' => 'Publication date', 'value' => '2026' }], + 'contributors' => [{ 'value' => 'Foo Barston', 'kind' => 'Creator' }], + 'sourceLink' => 'https://example.com/geodata-record' + } + + mock_response = mock('timdex_response') + mock_errors = mock('timdex_errors') + mock_errors.stubs(:details).returns({}) + mock_response.stubs(:errors).returns(mock_errors) + + mock_data = mock('timdex_data') + mock_data.stubs(:to_h).returns({ + 'search' => { + 'hits' => 1, + 'aggregations' => {}, + 'records' => [sample_result] + } + }) + mock_response.stubs(:data).returns(mock_data) + mock_response + end + test 'GeoData has specific advanced search fields' do ClimateControl.modify FEATURE_GEODATA: 'true' do get '/' @@ -184,6 +211,40 @@ class SearchControllerGeoTest < ActionDispatch::IntegrationTest end end + test 'geodata cache hit avoids external search and normalization' do + normalized_result = { + api: 'timdex', + title: 'Cached GeoData Document Title', + identifier: 'geodata-record-123', + content_type: ['Dataset'], + dates: [{ 'kind' => 'Publication date', 'value' => '2026' }], + creators: [{ value: 'Foo Barston', link: nil }], + links: [{ 'kind' => 'full record', 'url' => 'https://example.com/geodata-record' }] + } + + TimdexBase::Client.expects(:query).once.returns(build_geodata_timdex_mock_response) + + mock_normalizer = mock('normalizer') + mock_normalizer.expects(:normalize).once.returns([normalized_result]) + NormalizeTimdexResults.expects(:new).once.returns(mock_normalizer) + + ClimateControl.modify FEATURE_GEODATA: 'true' do + query = { + geobox: 'true', + geoboxMinLongitude: 40.5, + geoboxMinLatitude: 60.0, + geoboxMaxLongitude: 78.2, + geoboxMaxLatitude: 80.0 + }.to_query + + 2.times do + get "/results?#{query}" + assert_response :success + assert_select '.record-title', text: /Cached GeoData Document Title/ + end + end + end + test 'can query geodistance' do ClimateControl.modify FEATURE_GEODATA: 'true' do VCR.use_cassette('geodistance', From eed7fdec80918a3305cd87ff0a354e1f053e80e5 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Fri, 4 Sep 2026 15:05:04 -0400 Subject: [PATCH 3/6] Freeze string constant --- app/controllers/search_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index f9ae397d..1710c18e 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -2,7 +2,7 @@ class SearchController < ApplicationController # Increment this namespace when the normalized search result payload changes in a way that # makes existing cached values incomplete or incompatible, such as adding required fields, # removing/renaming fields, changing key types, or changing error/continuation metadata shape. - SEARCH_RESULTS_CACHE_NAMESPACE = 'normalized-search-results/v1' + SEARCH_RESULTS_CACHE_NAMESPACE = 'normalized-search-results/v1'.freeze SEARCH_RESULTS_CACHE_TTL = 12.hours before_action :validate_q!, only: %i[results] From 6a052eed6644095b9545dbdff1ef7bc0caabd60e Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Fri, 4 Sep 2026 15:38:12 -0400 Subject: [PATCH 4/6] Ensure primo cache key includes tab --- app/controllers/search_controller.rb | 2 +- test/controllers/search_controller_test.rb | 30 ++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 1710c18e..b2be7d1a 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -226,7 +226,7 @@ def fetch_timdex_data(offset: nil, per_page: nil) end def cached_primo_data(per_page, offset) - cache_key = CacheKeyGenerator.call(@enhanced_query.merge(per_page: per_page, offset: offset)) + cache_key = CacheKeyGenerator.call(@enhanced_query.merge(tab: @active_tab, per_page: per_page, offset: offset)) Rails.cache.fetch("#{SEARCH_RESULTS_CACHE_NAMESPACE}/#{cache_key}/primo", expires_in: SEARCH_RESULTS_CACHE_TTL) do primo_response = query_primo(per_page, offset) diff --git a/test/controllers/search_controller_test.rb b/test/controllers/search_controller_test.rb index 6097c3b7..5b0fe5ac 100644 --- a/test/controllers/search_controller_test.rb +++ b/test/controllers/search_controller_test.rb @@ -896,6 +896,36 @@ def source_filter_count(controller) end end + test 'primo cache key uses resolved active tab for default all requests' do + sample_doc = { + api: 'primo', + title: 'Cached Default All Primo Document Title', + format: 'Article', + year: '2025', + creators: [{ value: 'Foo Barston', link: nil }], + identifier: 'default-all-primo-record-123', + links: [{ 'kind' => 'full record', 'url' => 'https://example.com/default-all-primo-record' }] + } + + mock_primo = mock('primo_search') + mock_primo.expects(:search).once.returns({ 'docs' => [sample_doc], 'info' => { 'total' => 1 } }) + PrimoSearch.expects(:new).once.returns(mock_primo) + + mock_normalizer = mock('normalizer') + mock_normalizer.expects(:normalize).once.returns([sample_doc]) + NormalizePrimoResults.expects(:new).once.returns(mock_normalizer) + + controller = SearchController.new + controller.instance_variable_set(:@active_tab, 'all') + controller.instance_variable_set(:@enhanced_query, { q: 'test' }) + controller.send(:cached_primo_data, 20, 0) + + controller.instance_variable_set(:@enhanced_query, { q: 'test', tab: 'all' }) + cached = controller.send(:cached_primo_data, 20, 0) + + assert_equal [sample_doc], cached[:results] + end + test 'timdex cache hit avoids external search and normalization' do normalized_result = { api: 'timdex', From 5387341923f3f5830fc934ed98bdb44ccb2d5857 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Fri, 4 Sep 2026 15:40:59 -0400 Subject: [PATCH 5/6] Filter out blank ACTIVE_FILTERS Filtering out blank entries makes the default behavior (no reordering) work correctly. --- app/controllers/search_controller.rb | 2 +- test/controllers/search_controller_test.rb | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index b2be7d1a..62616c6d 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -283,7 +283,7 @@ def cached_timdex_data(query, include_filters: false) end def active_filters - ENV.fetch('ACTIVE_FILTERS', '').split(',').map(&:strip) + ENV.fetch('ACTIVE_FILTERS', '').split(',').map(&:strip).reject(&:blank?) end def query_primo(per_page, offset) diff --git a/test/controllers/search_controller_test.rb b/test/controllers/search_controller_test.rb index 5b0fe5ac..87545264 100644 --- a/test/controllers/search_controller_test.rb +++ b/test/controllers/search_controller_test.rb @@ -748,6 +748,18 @@ def source_filter_count(controller) end end + test 'active filters ignores blank env entries' do + controller = SearchController.new + + ClimateControl.modify ACTIVE_FILTERS: 'contentType, , source, ' do + assert_equal %w[contentType source], controller.send(:active_filters) + end + + ClimateControl.modify ACTIVE_FILTERS: '' do + assert_empty controller.send(:active_filters) + end + end + test 'applications can customize the displayed filters via ENV' do skip('Filters not implemented in USE UI') VCR.use_cassette('data basic controller', From 9deed3625dd65a59de91bce85c1f8a06aecce884 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Fri, 4 Sep 2026 16:08:52 -0400 Subject: [PATCH 6/6] Align Primo search tab usage with cache key --- app/controllers/search_controller.rb | 4 ++-- test/controllers/search_controller_test.rb | 12 ++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 62616c6d..801c8fac 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -288,10 +288,10 @@ def active_filters def query_primo(per_page, offset) Rails.logger.debug do - "External Primo search request: tab=#{@enhanced_query[:tab]}, offset=#{offset}, per_page=#{per_page}" + "External Primo search request: tab=#{@active_tab}, offset=#{offset}, per_page=#{per_page}" end - primo_search = PrimoSearch.new(@enhanced_query[:tab]) + primo_search = PrimoSearch.new(@active_tab) primo_search.search(@enhanced_query[:q], per_page, offset) end diff --git a/test/controllers/search_controller_test.rb b/test/controllers/search_controller_test.rb index 87545264..75201a3c 100644 --- a/test/controllers/search_controller_test.rb +++ b/test/controllers/search_controller_test.rb @@ -938,6 +938,18 @@ def source_filter_count(controller) assert_equal [sample_doc], cached[:results] end + test 'primo query uses resolved active tab' do + controller = SearchController.new + controller.instance_variable_set(:@active_tab, 'all') + controller.instance_variable_set(:@enhanced_query, { q: 'test', tab: 'invalid_tab' }) + + mock_primo = mock('primo_search') + mock_primo.expects(:search).with('test', 20, 0).returns({ 'docs' => [], 'info' => { 'total' => 0 } }) + PrimoSearch.expects(:new).with('all').returns(mock_primo) + + controller.send(:query_primo, 20, 0) + end + test 'timdex cache hit avoids external search and normalization' do normalized_result = { api: 'timdex',