sakaiproject/nakamura — pull request #930 by dgillman (2012-07-19 23:09:25+00:00)
Title: Kern 2880 minimal

This is a minimalist stab at the refactor I've been working on since Atlanta. Given the timing I've made some compromises to get out of the way of progress to be made by others on the search api and impl. The goal is to make the search service available to other bundles instead of the current design which forces client code in other bundles to make an HTTP request to complete a search.

The original work on this issue sought to:

1) develop a new search workflow which eliminates dependency on Servlet API classes
2) revise existing searches to take advantage of the new workflow.

The work to accomplish #2 has turned out to be extensive and complicated. The ambition to have a "light touch" leaving as much exiting code intact as possible has been a barely tractable time-sink. Further, the new search approach introduces by Ray [1] created two chains of logic that both needed to be accommodated. Given the timing and the dependence of others' work on KERN-2880 I have revised the strategy to eliminate #2 and to aim at making the new QueryHandler from Ray's work function without reliance on Servlet classes, and to leave "legacy" searches alone until they can be switched to the new QueryHandler model.

The actual mechanism I've used is to focus on SolrSearchServletImpl [2] as the key interface for performing a search, so I have started the change there. I have deprecated the original getSearchResultSet (line 94) and have replaced it with a new version (line 99). The new version no longer takes a SlingHttpServletRequest. In it's place it takes a sparse Session, an Authorizable, a fully completed Query object, and a new class SolrSearchParameters [3] which encapsulates metadata about the search results (page number, number of items per page, traversal depth, additional options, ...) outside of the actual search terms. This should be the API method used to initiate all searches from client services. All subsequent search API changes fall out of this change.

I have next altered DomainObjectSearchQueryHandler [4] (and it's subclasses). The old search process followed a convoluted process of customizing a Query for a user with a SolrSearchPropertyProvider, then preforming the request using a SolrSearchResultProcessor. This distributed search logic across multiple classes (as well as configuration information in JCR). Additionally it inverted the control of the search, making the search call within the getSearchResultSet method of each and every SolrSearchResultProcessor meaning each instance duplicated the same call to SolrSearchServiceFactoryImpl. Ray's work consolidated all of this logic into a single class, the DomainObjectSearchQueryHandler, but was required to maintain the inverted control to work with the rest of the search framework.

My improvement here was to add a getQuery(...) call to DomainObjectSearchQueryHandler. This call executes all of the customization steps that were originally preformed by the SolrSearchPropertyProvider and SolrSearchResultProcessor calls with two changes: first, no SlingHttpServletRequest is used (the method takes a Map of parameters instead), and second getQuery simply returns a ready-to-go, configured Query Object that can be used in a call to SolrSearchServiceFactoryImpl.getSearchResultSet() thereby centralizing the search workflow.

Finally, the locus of that centralized workflow is now in SolrSearchServlet [5]. Where SolrSearchServlet was originally driving the Query configuration using consecutive calls to SolrSearchPropertyProviders and SolrSearchResultProcessors, the new workflow simply converts the incoming SlingHttpServletRequest into a Session, an Authorizable, a Map of parameters and an additional SolrSearchParameters object. It calls the QueryHandler's getQuery method, then executes the search. Of course at present the old logic needs to live alongside this workflow, so the doGet() method in SolrSearchServlet has conditional statements that branch based on whether the search is being handled by a DomainObjectQueryHandler or not.

This is not the completion of the refactor I think is deserved. For one thing the SolrSearchRestulProcessor presently formats search results into JSON using the writeResults(...) call. writeResults(...) still takes a SlingHttpServletRequest and so needs to be modified to operate independent of the Sling/Servlet API as well. And, of course, the conditional "legacy" logic should ultimately go away and all the "legacy" logic should be converted to use the new QueryHandler model.

That work remains to be done. However, I believe this first step is a better "nibble" that provides useful restructuring without requiring as vast a refactor as I had originally undertaken. Therefore I hope this is a stepping stone which can be consumed by the project and which can provide direction for further pursuit of an improved search API.

[1] https://jira.sakaiproject.org/browse/KERN-2892
[2] https://github.com/dgillman/nakamura/blob/KERN-2880-minimal/bundles/search/impl/src/main/java/org/sakaiproject/nakamura/search/solr/SolrSearchServiceFactoryImpl.java
[3] https://github.com/dgillman/nakamura/blob/KERN-2880-minimal/bundles/search/api/src/main/java/org/sakaiproject/nakamura/api/search/solr/SolrSearchParameters.java
[4] https://github.com/dgillman/nakamura/blob/KERN-2880-minimal/bundles/search/api/src/main/java/org/sakaiproject/nakamura/api/search/solr/DomainObjectSearchQueryHandler.java
[5] https://github.com/dgillman/nakamura/blob/KERN-2880-minimal/bundles/search/impl/src/main/java/org/sakaiproject/nakamura/search/solr/SolrSearchServlet.java