Refactor search caching to store normalized data

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')
 

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.