-
Notifications
You must be signed in to change notification settings - Fork 0
Graceful degredation when one API times out #388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
b89765d
6365a9b
4e346f0
cdc7f57
effe17b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,7 +37,10 @@ def results | |
|
|
||
| # Render the response in HTML or JSON format | ||
| respond_to do |format| | ||
| format.json { render json: { results: @results, pagination: @pagination, errors: @errors } } | ||
| format.json do | ||
| render json: { results: @results, pagination: @pagination, errors: @errors, | ||
| incomplete_results: @incomplete_results } | ||
| end | ||
| format.html { render :results } | ||
| end | ||
| end | ||
|
|
@@ -70,6 +73,7 @@ def load_geodata_results | |
|
|
||
| # Handle errors | ||
| @errors = extract_errors(response) | ||
| @incomplete_results = nil | ||
| return unless @errors.nil? | ||
|
|
||
| hits = response.dig(:data, 'search', 'hits') || 0 | ||
|
|
@@ -85,13 +89,15 @@ def load_primo_results | |
| @pagination = data[:pagination] | ||
| @errors = data[:errors] | ||
| @show_primo_continuation = data[:show_continuation] | ||
| @incomplete_results = nil | ||
| end | ||
|
|
||
| def load_timdex_results | ||
| data = fetch_timdex_data | ||
| @results = data[:results] | ||
| @pagination = data[:pagination] | ||
| @errors = data[:errors] | ||
| @incomplete_results = nil | ||
| end | ||
|
|
||
| def load_all_results | ||
|
|
@@ -114,6 +120,7 @@ def load_all_results | |
| @errors = data[:errors] | ||
| @pagination = data[:pagination] | ||
| @show_primo_continuation = data[:show_primo_continuation] | ||
| @incomplete_results = data[:incomplete_results] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| end | ||
|
|
||
| def fetch_primo_data(offset: nil, per_page: nil) | ||
|
|
@@ -152,6 +159,8 @@ def fetch_primo_data(offset: nil, per_page: nil) | |
|
|
||
| { results: results, pagination: pagination, errors: errors, show_continuation: show_continuation, | ||
| hits: hits } | ||
| rescue HTTP::TimeoutError | ||
| { results: [], pagination: {}, errors: nil, show_continuation: false, hits: 0, timed_out: true } | ||
| rescue StandardError => e | ||
| { results: [], pagination: {}, errors: handle_primo_errors(e), show_continuation: false, hits: 0 } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Found 4 issues: |
||
| end | ||
|
|
@@ -174,6 +183,8 @@ def fetch_timdex_data(offset: nil, per_page: nil) | |
| else | ||
| { results: [], pagination: {}, errors: errors, hits: 0 } | ||
| end | ||
| rescue Net::ReadTimeout | ||
| { results: [], pagination: {}, errors: nil, hits: 0, timed_out: true } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| end | ||
|
|
||
| def active_filters | ||
|
|
@@ -391,7 +402,8 @@ def handle_primo_errors(error) | |
| 'message' => 'Hmm, we seem to be having difficulties...', | ||
| 'description' => 'In the meantime, try searching these tools directly.', | ||
| 'links' => [ | ||
| { 'label' => "MIT's WorldCat", 'description' => 'Books and media', 'url' => 'https://libraries.mit.edu/worldcat' }, | ||
| { 'label' => "MIT's WorldCat", 'description' => 'Books and media', | ||
| 'url' => 'https://libraries.mit.edu/worldcat' }, | ||
| { 'label' => 'Google Scholar', 'description' => 'Articles', 'url' => 'https://scholar.google.com/' }, | ||
| { 'label' => 'ArchivesSpace', 'description' => 'MIT archives', 'url' => 'https://archivesspace.mit.edu/' }, | ||
| { 'label' => 'DSpace@MIT', 'description' => 'MIT research', 'url' => 'https://dspace.mit.edu/' } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -115,6 +115,10 @@ def write_cached_totals(totals) | |
| # fetchers. Each fetcher should return the usual response hash including | ||
| # `:results` and `:hits`. | ||
| # | ||
| # Timeouts are handled at the HTTP level by each fetcher (PRIMO_TIMEOUT and | ||
| # TIMDEX_TIMEOUT), which raise catchable exceptions that the fetchers convert | ||
| # to `timed_out: true` responses for graceful partial-results display. | ||
| # | ||
| # WARNING: exceptions raised inside these threads will not automatically | ||
| # propagate to the caller; callers/tests should account for this. | ||
| # | ||
|
|
@@ -124,6 +128,7 @@ def write_cached_totals(totals) | |
| def parallel_fetch(offset:, per_page:) | ||
| primo = nil | ||
| timdex = nil | ||
|
|
||
|
Comment on lines
129
to
+131
|
||
| threads = [] | ||
| threads << Thread.new { primo = @primo_fetcher.call(offset: offset, per_page: per_page, query: @enhanced_query) } | ||
| threads << Thread.new { timdex = @timdex_fetcher.call(offset: offset, per_page: per_page, query: @enhanced_query) } | ||
|
|
@@ -175,7 +180,7 @@ def fetch_all_tab_page_chunks(paginator) | |
| # @param current_page [Integer] | ||
| # @param per_page [Integer] | ||
| # @param deeper [Boolean] whether this was a deeper-page flow | ||
| # @return [Hash] response with :results, :errors, :pagination, :show_primo_continuation | ||
| # @return [Hash] response with :results, :errors, :pagination, :show_primo_continuation, :incomplete_results | ||
| def assemble_all_tab_result(paginator, primo_data, timdex_data, current_page, per_page, deeper: false) | ||
| primo_total = primo_data[:hits] || 0 | ||
| timdex_total = timdex_data[:hits] || 0 | ||
|
|
@@ -184,6 +189,9 @@ def assemble_all_tab_result(paginator, primo_data, timdex_data, current_page, pe | |
| errors = combine_errors(primo_data[:errors], timdex_data[:errors]) | ||
| pagination = Analyzer.new(@enhanced_query, timdex_total, :all, primo_total, per_page).pagination | ||
|
|
||
| # Detect if results are incomplete due to timeouts | ||
| incomplete_results = detect_incomplete_results(primo_data, timdex_data) | ||
|
|
||
| show_primo_continuation = if deeper | ||
| # Use the Primo-specific API offset (calculated from the paginator) | ||
| # when deciding whether to show a Primo continuation. | ||
|
|
@@ -201,7 +209,24 @@ def assemble_all_tab_result(paginator, primo_data, timdex_data, current_page, pe | |
| primo_data[:show_continuation] | ||
| end | ||
|
|
||
| { results: merged, errors: errors, pagination: pagination, show_primo_continuation: show_primo_continuation } | ||
| { results: merged, errors: errors, pagination: pagination, show_primo_continuation: show_primo_continuation, | ||
| incomplete_results: incomplete_results } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Found 4 issues: |
||
| end | ||
|
|
||
| # Detect if search results are incomplete due to source timeouts. | ||
| # | ||
| # When a fetcher times out, it returns timed_out: true in the response. | ||
| # This method identifies which sources timed out and returns an indicator for the UI. | ||
| # | ||
| # @param primo_data [Hash] response from Primo fetcher | ||
| # @param timdex_data [Hash] response from TIMDEX fetcher | ||
| # @return [Hash, nil] { sources: Array<String> } e.g., { sources: ['Primo'] } or nil if complete | ||
| def detect_incomplete_results(primo_data, timdex_data) | ||
| timed_out_sources = [] | ||
| timed_out_sources << 'Primo' if primo_data[:timed_out] | ||
| timed_out_sources << 'TIMDEX' if timdex_data[:timed_out] | ||
|
|
||
| timed_out_sources.any? ? { sources: timed_out_sources } : nil | ||
| end | ||
|
|
||
| # Merge multiple error arrays into a single array or nil when empty. | ||
|
|
@@ -221,7 +246,7 @@ def build_paginator_from_totals(totals, current_page, per_page) | |
| current_page: current_page, per_page: per_page) | ||
| end | ||
|
|
||
| # Note: default fetcher implementations were removed to enforce explicit | ||
| # NOTE: default fetcher implementations were removed to enforce explicit | ||
| # dependency injection. Callers must provide `primo_fetcher` and | ||
| # `timdex_fetcher` when constructing `MergedSearchService`. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| <% return unless incomplete_results.present? %> | ||
| <% sources = incomplete_results[:sources] %> | ||
|
|
||
| <aside role="alert" class="incomplete-results-warning"> | ||
| <h3>Partial Results</h3> | ||
|
|
||
| <p> | ||
| Search results are incomplete because | ||
| <% if sources.length == 1 %> | ||
| <strong><%= sources.first %></strong> is temporarily unavailable. | ||
| <% else %> | ||
| <strong><%= sources.join(' and ') %></strong> are temporarily unavailable. | ||
| <% end %> | ||
| Showing available results from other sources. | ||
| </p> | ||
| </aside> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found 2 issues:
1. Assignment Branch Condition size for
resultsis too high. [<3, 20, 5> 20.83/17] [rubocop:Metrics/AbcSize]2. Method has too many lines. [22/10] [rubocop:Metrics/MethodLength]