[MGNLEESOLR-53] FacetedSolrSearchProvider#search should throw exception if search fails Created: 29/Mar/15  Updated: 27/Apr/15  Resolved: 14/Apr/15

Status: Closed
Project: Solr Search Provider
Component/s: None
Affects Version/s: 2.1.1
Fix Version/s: 2.2

Type: Improvement Priority: Neutral
Reporter: Edgar Vonk Assignee: Milan Divilek
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Date of First Response:

 Description   

I was wondering if it possible to improve the error handling of the Magnolia Solr Module. Specifically: in our code we want to be able to catch SolrServerExceptions when they are thrown so we can take appropriate action (like showing an error message to the user and setting our application's health check to fail). Instead these exceptions are now caught by FacetedSolrSearchProvider#performFacettedSearch giving us no chance to do anything with them..

try {
            log.debug("solrQuery: " + solrQuery);
            response = getMagnoliaSolrBridge().getSolrServer().query(solrQuery);
        } catch (SolrServerException e) {
            log.error(e.getMessage(), e);
        }

I would suggest that this method and all methods that use them simply throw SolrServerExceptions so clients of the Magnolia Solr Module can take appropriate action. And by default I would suggest that the FacetedSearchResultModel#execute method catches them and returns an 'ERROR' status code in case they happen instead of an empty string that it does currently (no matter what happens).



 Comments   
Comment by Edgar Vonk [ 29/Mar/15 ]

As a workaround we have extended the FacetedSearchResultModel class and in our model class we get whether the results are null or not. But this is not a nice solution at all. I would much rather have an exception being thrown.

	@Override
	public String execute() {
		String status;
		if (getQueryStr() == null || getQueryStr().isEmpty()) {
			status = STATUS_NO_QUERY_STRING;
		} else {
			try {	
				// only thing we can do is check if the results are null; if so this indicates an error.
				// for a valid search without search results the results list is an empty list instead of null
				super.execute();

				if (null == getResult()) {
					status = STATUS_ERROR;
				} else {
					status = STATUS_OK;
				}
			}
		}
		return status;
	}

In our superclass we also check if there actually was a query string provided because we do not want to perform searches on an empty query string as the FacetedSearchResultModel class does.

Comment by Edgar Vonk [ 13/Apr/15 ]

Hi @milan, can you maybe explain the fix so that we can remove our workaround as soon as we have upgraded?

Comment by Jan Haderka [ 13/Apr/15 ]

In many cases you are too eager when catching exceptions and catching all instead of just IOException and Solr...Exception. This way you might unintentionally swallow also NPE or other kinds of errors that should be shown.

Comment by Milan Divilek [ 14/Apr/15 ]

Hi Edgar,

If SolrServerException and IOException occur during communication with SolrServer then providers simply throws those exceptions instead of swallowing them. So in model class you can catch those exceptions and take appropriate action. Model classes provided by solr-search-provider catches those exceptions and returns "error" status code.

Generated at Mon Feb 12 10:59:38 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.