diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 58799bc6..801c8fac 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'.freeze + 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] @@ -83,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 @@ -202,11 +204,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(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) + 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,37 +256,46 @@ 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, include_filters: false) + prepare_timdex_query(query) + cache_query = include_filters ? query.merge(active_filters: active_filters) : query + cache_key = CacheKeyGenerator.call(cache_query) - response = query_timdex(query) - errors = extract_errors(response) + 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 - 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 } + if errors.nil? + hits = response.dig(:data, 'search', 'hits') || 0 + raw_results = extract_results(response) + results = NormalizeTimdexResults.new(raw_results, @enhanced_query[:q]).normalize + payload = { results: results, errors: nil, hits: hits } + payload[:filters] = extract_filters(response) if include_filters + payload + else + { results: [], errors: errors, hits: 0 } + end end 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) + Rails.logger.debug do + "External Primo search request: tab=#{@active_tab}, offset=#{offset}, per_page=#{per_page}" + end + + primo_search = PrimoSearch.new(@active_tab) + primo_search.search(@enhanced_query[:q], per_page, offset) end - def query_timdex(query) + 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 +305,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_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', diff --git a/test/controllers/search_controller_test.rb b/test/controllers/search_controller_test.rb index 95b19c82..75201a3c 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', @@ -870,6 +882,98 @@ 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 '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 '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', + 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