Refactor search caching to store normalized data - #465
Conversation
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.
Coverage Report for CI Build 33914782201Coverage increased (+0.02%) to 98.324%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
❌ 8 blocking issues (12 total)
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
|
|
||
| cached.merge(pagination: pagination) | ||
| rescue StandardError => e | ||
| { results: [], pagination: {}, errors: handle_primo_errors(e), show_continuation: false, hits: 0 } |
| return cached.merge(pagination: {}) if cached[:errors] | ||
|
|
||
| pagination = Analyzer.new(@enhanced_query, cached[:hits], :timdex).pagination | ||
| cached.merge(pagination: pagination) |
| end | ||
| end | ||
|
|
||
| def build_primo_cache_payload(primo_response, results, hits, offset) |
| 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 } |
| errors: raw.errors.details.to_h | ||
| } | ||
| end | ||
| query |
There was a problem hiding this comment.
Found 3 issues:
1. Function with high complexity (count = 7): prepare_timdex_query [qlty:function-complexity]
2. Assignment Branch Condition size for prepare_timdex_query is too high. [<8, 9, 14> 18.47/17] [rubocop:Metrics/AbcSize]
3. Cyclomatic complexity for prepare_timdex_query is too high. [8/7] [rubocop:Metrics/CyclomaticComplexity]
There was a problem hiding this comment.
🟡 Changes recommended
A remaining raw TIMDEX cache path (query_timdex) conflicts with the documented normalized caching strategy and can still populate un-namespaced provider-shaped cache entries.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors SearchController caching so single-source Primo and TIMDEX result requests cache application-normalized payloads (rather than raw provider responses), reducing Redis storage pressure and avoiding repeated normalization work on cache hits. It also adds an ADR documenting the new caching approach and tests asserting that cache hits bypass external calls and normalization.
Changes:
- Cache normalized payloads for single-source Primo/TIMDEX searches with an explicit cache namespace and TTL.
- Update controller flow so cache lookup occurs before provider calls/normalization, and pagination is computed after cache retrieval.
- Add controller tests to ensure cache hits avoid external provider calls and normalizer instantiation; add ADR documenting the decision.
File summaries
| File | Description |
|---|---|
| app/controllers/search_controller.rb | Introduces normalized-results cache namespace/TTL and refactors Primo/TIMDEX fetch paths to cache normalized payloads before external calls/normalization. |
| test/controllers/search_controller_test.rb | Adds tests asserting that repeated requests hit cache and avoid re-calling external search/normalizers for Primo and TIMDEX. |
| docs/architecture-decisions/0003-cache-normalized-search-results.md | Documents the decision and expected caching flows for normalized single-source results and all-tab load-more state. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
| else | ||
| { results: [], errors: errors, hits: 0 } | ||
| end | ||
| end |
There was a problem hiding this comment.
There was a problem hiding this comment.
🟡 Changes recommended
The refactor introduces a couple of correctness/efficiency issues in the caching/keying and filter configuration handling that should be addressed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
app/controllers/search_controller.rb:232
cached_primo_databuilds its cache key from@enhanced_query, butEnhanceronly sets:tabwhen the request includes atabparam. That means logically identical all-tab requests (/results?q=...vs/results?q=...&tab=all) can generate different cache keys and store duplicate normalized payloads, reducing cache efficiency (the primary goal of this PR). Consider always keying Primo caches with the resolved@active_tab(or defaulting to'all') so equivalent requests share the same entry.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
Filtering out blank entries makes the default behavior (no reordering) work correctly.
There was a problem hiding this comment.
🟡 Changes recommended
A confirmed geospatial query branching bug can prevent combined geobox+geodistance searches from using the intended query path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
app/controllers/search_controller.rb:334
- In
execute_geospatial_query, the combined geobox+geodistance branch checksquery[:geodistance], butQueryBuildersets geospatial flags on string keys (e.g.,query['geodistance']). This makes the “both enabled” branch effectively unreachable and can route combined requests to the geobox-only query.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core search caching behavior across Primo/TIMDEX and GeoData paths, so a final human review is warranted to validate runtime/cache-shape impacts beyond the included tests.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
Why are these changes being introduced:
Relevant ticket(s):
How does this address that need:
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing