Extract Blacklight::Base module out of Blacklight::Catalog with all the methods needed to work with Blacklight::SolrHelper

[cbeer]

There are many times it'd be useful to work with e.g. Blacklight::SolrHelper and other Blacklight classes and modules outside of a controller context. Blacklight::Catalog provides both the Blacklight configuration, as well as the controller actions. We should extract a Blacklight::Base module out of Blacklight::Catalog that provides just the Blacklight configuration for non-controller classes to mix in.

This also moves the default search_action_url helper into Blacklight::Controller, which fixes #620.

[cbeer]

@dchandekstark I think this addresses the issue you raised in #620. Can you confirm, or give more detail about what "break in other controllers" means?

[dchandekstark]

@cbeer An example (experimental) of a controller that includes Blacklight::Catalog and a layout that has the BL search form, and which breaks on a routing error with the search_action_url helper in Blacklight::Catalog:

https://github.com/duke-libraries/dul-hydra/blob/cb8cdb8e7f506a2cf5b943ca655bf4861e2f5fe4/app/controllers/objects_controller.rb

The layout is a slightly modified version of the BL default layout:

https://github.com/duke-libraries/dul-hydra/blob/cb8cdb8e7f506a2cf5b943ca655bf4861e2f5fe4/app/views/layouts/application.html.erb

... which renders the BL partial shared/header_navbar, which renders the search form, which calls search_action_url.

If I overwrite search_action_url as shown in the controller, the routing error goes away. I suppose I could have customized the search form partial to pass a :controller => :objects option, but I prefer not to override partials, especially since I'm a bit behind the current BL version (at 4.2.1).

Hope that helps.

Thanks,
David

[dchandekstark]

P.S. Looks like this change will accomplish the desired result. Short of upgrading to this commit (when merge), I could try patching my code likewise, if that would be helpful.

[cbeer]

I think overriding #search_action_url, as you have done, is a perfectly valid thing to do. The search_action_url feature in Blacklight::Catalog expects your implementing Controller will behave like a Blacklight::Catalog, and rightly (I'd argue) assumes you're going to provide an index route.

Maybe the "right" thing to do in dul-hydra is, instead of having ObjectsController mixin Blacklight::Catalog (perhaps erroneously, as it doesn't really behave like a Blacklight::Catalog anyway), have ObjectsController mixin Blacklight::Configurable and Blacklight::SolrHelper directly.

I'd also welcome a pull request that creates Blacklight::Base or somesuch that extracts the useful bits out of the Blacklight::Catalog, maybe something like:

module Blacklight::Base

  extend ActiveSupport::Concern

  include Blacklight::Configurable
  include Blacklight::SolrHelper

  included do
    # Whenever an action raises SolrHelper::InvalidSolrID, this block gets executed.
    # Hint: the SolrHelper #get_solr_response_for_doc_id method raises this error,
    # which is used in the #show action here.
    rescue_from Blacklight::Exceptions::InvalidSolrID, :with => :invalid_solr_id_error
    # When RSolr::RequestError is raised, the rsolr_request_error method is executed.
    # The index action will more than likely throw this one.
    # Example, when the standard query parser is used, and a user submits a "bad" query.
    rescue_from RSolr::Error::Http, :with => :rsolr_request_error
  end


    # when solr (RSolr) throws an error (RSolr::RequestError), this method is executed.
    def rsolr_request_error(exception)

      if Rails.env.development?
        raise exception # Rails own code will catch and give usual Rails error page with stack trace
      else

        flash_notice = I18n.t('blacklight.search.errors.request_error')

        # If there are errors coming from the index page, we want to trap those sensibly

        if flash[:notice] == flash_notice
          logger.error "Cowardly aborting rsolr_request_error exception handling, because we redirected to a page that raises another exception"
          raise exception
        end

        logger.error exception

        flash[:notice] = flash_notice 
        redirect_to root_path
      end
    end

    # when a request for /catalog/BAD_SOLR_ID is made, this method is executed...
    def invalid_solr_id_error
      flash[:notice] = I18n.t('blacklight.search.errors.invalid_solr_id')
      params.delete(:id)
      index
      render "index", :status => 404
    end

    def blacklight_solr
      @solr ||=  RSolr.connect(blacklight_solr_config)
    end

    def blacklight_solr_config
      Blacklight.solr_config
    end
end

[cbeer]

I've added the Blacklight::Base module to 8107db3.

[dchandekstark]

Thanks, Chris. This seems like a positive direction. I agree that including Blacklight::Catalog as I have done is a hack, but it seemed (up til now) possibly the only way to get BL's Solr goodness (for search/retrieval and display) into what is essentially an ActiveFedora context. AF's SolrService obviously isn't intended to be as full-featured and UI-oriented as BL. I will try the new module ASAP.

[cbeer]

Ok. I think it makes sense to hold off merging this until @dchandekstark can evaluate it, but merge it before 4.5.0. Going from 4.2.1 to master shouldn't be much trouble.

[jrochkind]

I've been thinking -- and discussed with JCoyne at the last code4lib -- that it's a mistake to have 'BL's Solr Goodness' in a mix-in to a Rails controller at all -- sometimes you want this goodness in something that isn't even a Rails Controller, let alone a blacklight catalog::Controller.

And that the right way to do it would have been to have a completely separate SolrFetcher object or something, that the controller (or anyone else could use). With the name of the particular SolrFetcher class/subclass to be used perhaps a configuration on the controller, and with the custom localizations (solr_search_logic etc) done on the SolrFetcher object -- which could then be re-used between controllers too, which is a current design pain point -- instead of the controller.

But I'm aware this may be a larger refactor than anyone has time for now. I just wanted to bring it up again in case you wanted to consider it, as an alternative to doing a bunch of work on refactors that still leave the stuff as a mix-in to a controller, and thus don't fix the underlying inherent design flaw.

[cbeer]

I think this patch probably does start to take us down the road of non-controller solr interactions. Blacklight::Base has nothing controller-specific (except the optional #rescue_from) in it, so, in theory, downstream applications could mix them into their own SolrFetcher class and operate on it normally.

One weird thing, and something we probably will have to address in the future, is that the solr search params logic is fairly user-supplied-params-centric, and may not always make sense for programmatic access to solr.. That's clearly a separate issue, though.

[jrochkind]

Okay, cool. In my mind, the SolrFetcher should be 'seperation of concerns' focused only on Solr fetching, not a grab bag of "lots of things related to blacklight" like Blacklight::Base implies. We've coupled different functions too tightly together in the past, is part of how we got here.

I think it works fine for programmatic access too. It's defining an API for your local solr -- whether the API is triggered over URL query parameters, or progarmmatically with :some_param => "some_value" instead, it works either way. It's just a local api for fetching things from your solr according to local business logic.

[dchandekstark]

FWIW I think the SoC idea especially makes sense in a Hydra context, which is my domain, and what specifically led to this PR. I would mention, though, that it's not just "Solr fetching" which is useful a separate concern, but also the ability to render a SolrResponse and list of SolrDocument objects (you know, all the good pagination, sorting, etc.).

[dchandekstark]

@cbeer I've test the Blacklight::Base module on an ad-hoc basis and it appears to work as intended. There are a few other things that could be done in the view context to make BL easier to use outside of a Catalog-like controller context, but that should perhaps be another conversation. Thanks!

[cbeer]

thanks @dchandekstark. Feel free to lob tickets into the tracker with other ideas.

[dchandekstark]

@cbeer Thoughts on merging and releasing this code?