Skip to content
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

Separate out handling of Solr timeout errors with it's own exception class #2597

Merged
merged 5 commits into from
Feb 9, 2022

Commits on Dec 15, 2021

  1. Separate out handling of Solr timeout errors with it's own exception …

    …class
    
    == context ==
    
    Almost any RSolr exception is wrapped by Blacklight in a `Blacklight::Exceptions::InvalidRequest`
    
    A Blacklight Catalog controller [rescues](https://github.com/projectblacklight/blacklight/blob/main/app/controllers/concerns/blacklight/catalog.rb#L21) this exception with a [method](https://github.com/projectblacklight/blacklight/blob/main/app/controllers/concerns/blacklight/catalog.rb#L282-L299) that just re-displays the search page, with the message "Sorry, I don't understand your search."
    
    "Connection refused" errors are handled differently -- which is good, it would make no sense to prompt the user to search again with "Sorry, I don't understand your search" to network errors like connection refused.
    
    What about a timeout error, where Solr did not return in a response within the timeout value RSolr had configured?  Prior to this PR, that was handled the same as any `Blacklight::Exceptions::InvalidRequest` -- redisplay search page to the user, with message "Sorry, I don't understand your search."
    
    That is not a desirable UX, and also causes Solr timeout errors to generally not make it to many error monitoring and logging systems, since Blacklight rescues them and displays a 200.
    
    == change ==
    
    With the release of RSolr 2.4.0, RSolr uses a specific exception class to raise timeout errors, so Blacklight can rescue them and handle them differently.
    
    After this PR, they are handled analogously to connection refused errors, with a custom exception that won't be rescued as a `Blacklight::Exceptions::InvalidRequest`.
    
    a `defined?` check is done so Blacklight can still work with earlier versions of RSolr, and just won't do this new rescue.
    
    == weird code in test ==
    
    RSolr::Error api has some really rough edges, which requires some weirdness in setting up the test.
    
    == to be done in future ==
    
    I formatted the message of this exception just like the existing `Blacklight::Exceptions::ECONNREFUSED`... even though that formatting isn't necessarily so useful, there's a lot of stuff in there.
    
    But I opted to do it consistently.
    jrochkind committed Dec 15, 2021
    Configuration menu
    Copy the full SHA
    f0d6498 View commit details
    Browse the repository at this point in the history

Commits on Dec 20, 2021

  1. Configuration menu
    Copy the full SHA
    6e36b67 View commit details
    Browse the repository at this point in the history

Commits on Jan 13, 2022

  1. Separate out conditional on RSolr exception being defined to separate…

    … method
    
    Thanks @barmintor, I think you're right this is more clear.
    jrochkind committed Jan 13, 2022
    Configuration menu
    Copy the full SHA
    ed2df39 View commit details
    Browse the repository at this point in the history
  2. remove unnecessary comment

    jrochkind committed Jan 13, 2022
    Configuration menu
    Copy the full SHA
    467796e View commit details
    Browse the repository at this point in the history
  3. pull construction of rsolr request params into separate method of Bla…

    …cklight::Solr::Repository
    barmintor committed Jan 13, 2022
    Configuration menu
    Copy the full SHA
    a5395f6 View commit details
    Browse the repository at this point in the history