Solr
  1. Solr
  2. SOLR-141

Errors/Exceptions should be formated by ResponseWriter

    Details

    • Type: Wish Wish
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0-ALPHA
    • Component/s: None
    • Labels:
      None

      Description

      Whenever possible, the Solr Dispatcher should to let the ResponseWriter format Exceptions using the format the user expects – this should in no way change the fact that Exceptions currently generate non 200 HTTP status codes, nor should it prevent the Dispatcher from using the exception message as the HTTP status message – but clients that want the full details of the error should be able to parse them in the format they expected based on the request.

      this would also give RequestHandlers the oportunity to return "partial" results - by adding both whatever results they have to the Response as well as the Exception they encountered.

      situations where this can't happen are obviously:

      • Exception thrown by ResponseWriter
      • Exception thrown so early in the request thta the DIspather doesn't know which ResposneWriter the client wanted.
        ...in those cases, plain text is a wise choice.

      thing that would probably need to be done to make this work:
      1) if the Dispatcher catches an exception, it should call SolrQueryResponse.setException, set the HTTP status code/message as it currently does, but then hand off to the ResponseWriter.
      2) Dispatcher needs to check SolrQueryResponse.getException and set the HTTP status code/message just like if it caught the exception itself.
      3) all of the ResponseWriters should start looking at SolrQueryResponse.getException if they aren't already, and formatting it in a usefull way.
      4) if the ResponseWriter throws an exception, Dispatcher needs to return a nice plain text error page

      extension to this idea... add a new method to ResponseWriter to generate a generic error message in the appropriate format that Dispatcher can use if the ResponseWriter throws an exception (as a backup before resorting to plain text)

      1. error-response.patch
        5 kB
        Mike Sokolov
      2. SOLR-141-3.x-Backport.patch
        19 kB
        Greg Bowyer
      3. SOLR-141-sendError.patch
        15 kB
        Ryan McKinley
      4. SOLR-141-sendError.patch
        0.7 kB
        Rich Cariens
      5. solr-exception-writer-solr-1.2.diff
        18 kB
        Daniel Naber
      6. solr-exception-writer-v2.diff
        16 kB
        Daniel Naber
      7. solr-exception-writer-v3.diff
        17 kB
        Daniel Naber

        Issue Links

          Activity

          Hide
          Daniel Naber added a comment -

          almost the same patch but for Solr 1.2.0, just for reference

          Show
          Daniel Naber added a comment - almost the same patch but for Solr 1.2.0, just for reference
          Hide
          Daniel Naber added a comment -

          I've attached a patch that adds an ErrorResponseWriter, similar to QueryResponseWriter. The default behaviour is unmodified, i.e. the error is returned with an http error code and text. It's now possible, however, to configure the new XMLErrorResponseWriter so that the error is returned as an XML page (http code 200, the original error code is in the XML).

          I didn't modify SolrServlet because it's deprecated anyway. There's a FIXME because the XML doesn't contain the stacktrace yet.

          The patch against 1.2 is attached just in case someone might find it useful (SolrServlet is modified in this version).

          Show
          Daniel Naber added a comment - I've attached a patch that adds an ErrorResponseWriter, similar to QueryResponseWriter. The default behaviour is unmodified, i.e. the error is returned with an http error code and text. It's now possible, however, to configure the new XMLErrorResponseWriter so that the error is returned as an XML page (http code 200, the original error code is in the XML). I didn't modify SolrServlet because it's deprecated anyway. There's a FIXME because the XML doesn't contain the stacktrace yet. The patch against 1.2 is attached just in case someone might find it useful (SolrServlet is modified in this version).
          Hide
          Daniel Naber added a comment -

          same as before, but now also shows stacktrace

          Show
          Daniel Naber added a comment - same as before, but now also shows stacktrace
          Hide
          Daniel Naber added a comment -

          New version with two differences: includes an example in solrconfig.xml (commented out); sets the http status code according to the exception or 500 if it's not a SolrException.

          Show
          Daniel Naber added a comment - New version with two differences: includes an example in solrconfig.xml (commented out); sets the http status code according to the exception or 500 if it's not a SolrException.
          Hide
          Hoss Man added a comment -

          Jogged by an email today, i took a quick glance at this patch and wanted to post some brief notes for anyone who might be interested.

          on the whole this looks good, the few things that seem to be missing to me are...

          1) more smarts in SolrCore.getErrorResponseWriter to be able to deal with the possibility that the ResponseWriter is also an ErrorResponseWriter – that way people wouldn't have to register a dual purpose impl twice
          2) giving the ErrorResponseWRiter's access to the SolrQueryResponse in case they want to try and output "partial" results along with the error message
          3) the RepestDispatcher should still be using the status code from the Exception – or at the very least asking the ErrorResponseWriter which status code to use.
          4) i don't see the Content-Type getting specified anywhere
          5) test cases.

          Show
          Hoss Man added a comment - Jogged by an email today, i took a quick glance at this patch and wanted to post some brief notes for anyone who might be interested. on the whole this looks good, the few things that seem to be missing to me are... 1) more smarts in SolrCore.getErrorResponseWriter to be able to deal with the possibility that the ResponseWriter is also an ErrorResponseWriter – that way people wouldn't have to register a dual purpose impl twice 2) giving the ErrorResponseWRiter's access to the SolrQueryResponse in case they want to try and output "partial" results along with the error message 3) the RepestDispatcher should still be using the status code from the Exception – or at the very least asking the ErrorResponseWriter which status code to use. 4) i don't see the Content-Type getting specified anywhere 5) test cases.
          Hide
          Frank Wesemann added a comment -

          1) more smarts in SolrCore.getErrorResponseWriter to be able to deal with the possibility that the ResponseWriter is also an ErrorResponseWriter - that way people wouldn't have to register a dual purpose impl twice

          wouldn't it be sufficient to extend the interface QueryResponseWriter with a writeError(Writer, HttpServletResponse, SolrResponse, SolrRequest) Method?

          The DispatchFilter may set the last caught exception on SolrResponse and call it in the sendError() method.
          The ResponseWriter impl may than do something like:

          public void  writeError(Writer out, HttpServletResponse res, SolrResponse solrRes, SolrRequest solrReq) {
            Throwable th = solrRes.getException();
            res.setStatus(code);
            res.setContentType( getContentType() );
            myTextResponseWriterImpl.writeInt( code );
            myTextResponseWriterImpl.writeStr( th.getMessage );
            if ( ! th instanceof QueryResponseWritingExecption ) {
              try {
                 myTextResponseWriterImpl.writeResponse();
              } catch ( Exception e) {
                 myTextResonseWriterIml.writeStr("an additionalError in writing the result occured: " + e.getMessage() );
              }
            }
           //... do what else you need to.
          }
          
          
          Show
          Frank Wesemann added a comment - 1) more smarts in SolrCore.getErrorResponseWriter to be able to deal with the possibility that the ResponseWriter is also an ErrorResponseWriter - that way people wouldn't have to register a dual purpose impl twice wouldn't it be sufficient to extend the interface QueryResponseWriter with a writeError(Writer, HttpServletResponse, SolrResponse, SolrRequest) Method? The DispatchFilter may set the last caught exception on SolrResponse and call it in the sendError() method. The ResponseWriter impl may than do something like: public void writeError(Writer out, HttpServletResponse res, SolrResponse solrRes, SolrRequest solrReq) { Throwable th = solrRes.getException(); res.setStatus(code); res.setContentType( getContentType() ); myTextResponseWriterImpl.writeInt( code ); myTextResponseWriterImpl.writeStr( th.getMessage ); if ( ! th instanceof QueryResponseWritingExecption ) { try { myTextResponseWriterImpl.writeResponse(); } catch ( Exception e) { myTextResonseWriterIml.writeStr( "an additionalError in writing the result occured: " + e.getMessage() ); } } //... do what else you need to. }
          Hide
          Hoss Man added a comment -

          Bulk updating 240 Solr issues to set the Fix Version to "next" per the process outlined in this email...

          http://mail-archives.apache.org/mod_mbox/lucene-dev/201005.mbox/%3Calpine.DEB.1.10.1005251052040.24672@radix.cryptio.net%3E

          Selection criteria was "Unresolved" with a Fix Version of 1.5, 1.6, 3.1, or 4.0. email notifications were suppressed.

          A unique token for finding these 240 issues in the future: hossversioncleanup20100527

          Show
          Hoss Man added a comment - Bulk updating 240 Solr issues to set the Fix Version to "next" per the process outlined in this email... http://mail-archives.apache.org/mod_mbox/lucene-dev/201005.mbox/%3Calpine.DEB.1.10.1005251052040.24672@radix.cryptio.net%3E Selection criteria was "Unresolved" with a Fix Version of 1.5, 1.6, 3.1, or 4.0. email notifications were suppressed. A unique token for finding these 240 issues in the future: hossversioncleanup20100527
          Hide
          Rich Cariens added a comment -

          If Solr is a JEE webapp, why not just have SolrServlet use HttpServletResponse.sendError(int, String) when an exception is raised? That way the user can register custom error documents for 500's, 400's, etc in web.xml.

          Or is there some other rationale for not using this container feature?

          Show
          Rich Cariens added a comment - If Solr is a JEE webapp, why not just have SolrServlet use HttpServletResponse.sendError(int, String) when an exception is raised? That way the user can register custom error documents for 500's, 400's, etc in web.xml. Or is there some other rationale for not using this container feature?
          Hide
          Rich Cariens added a comment -

          Patches SolrServlet.sendErr() to use HttpServletResponse.sendError(rc, msg). This allows the user to hook into custom error documents via web.xml.

          Show
          Rich Cariens added a comment - Patches SolrServlet.sendErr() to use HttpServletResponse.sendError(rc, msg). This allows the user to hook into custom error documents via web.xml.
          Hide
          Hoss Man added a comment -

          Frank:

          wouldn't it be sufficient to extend the interface QueryResponseWriter with a writeError(Writer, HttpServletResponse, SolrResponse, SolrRequest) Method?

          1) that would break back compat for anyone who already had QueryResponseWriter impl (unfortunately it's an interface not an abstract class).
          2) we don't want to introduce direct dependencies on javax.servlet.* into the core solr code (ie: QueryResponseWriter) ... only the org.apache.solr.servlet code in Solr should have those dependencies.

          Rich:

          If Solr is a JEE webapp, why not just have SolrServlet use HttpServletResponse.sendError(int, String)

          SolrServlet has been deprecated for quite some time, it's error handling is attrocious, but was been left that way for back compat (at this point we should just remove it)

          The issue here is SolrDispatchFilter which already does use HttpServletResponse.sendError ... that's the entire problem, and the goal of this issue is to change that. By using sendError we give hte servlet container the authority of formatting hte response, but that undermines the users ability to select the format using the "wt" param. SolrDispatchFilter needs to take responsibility for formatting the response document directly in order to ensure we can always give the user the response in the format they want.

          Show
          Hoss Man added a comment - Frank: wouldn't it be sufficient to extend the interface QueryResponseWriter with a writeError(Writer, HttpServletResponse, SolrResponse, SolrRequest) Method? 1) that would break back compat for anyone who already had QueryResponseWriter impl (unfortunately it's an interface not an abstract class). 2) we don't want to introduce direct dependencies on javax.servlet.* into the core solr code (ie: QueryResponseWriter) ... only the org.apache.solr.servlet code in Solr should have those dependencies. Rich: If Solr is a JEE webapp, why not just have SolrServlet use HttpServletResponse.sendError(int, String) SolrServlet has been deprecated for quite some time, it's error handling is attrocious, but was been left that way for back compat (at this point we should just remove it) The issue here is SolrDispatchFilter which already does use HttpServletResponse.sendError ... that's the entire problem, and the goal of this issue is to change that. By using sendError we give hte servlet container the authority of formatting hte response, but that undermines the users ability to select the format using the "wt" param. SolrDispatchFilter needs to take responsibility for formatting the response document directly in order to ensure we can always give the user the response in the format they want.
          Hide
          Mike Sokolov added a comment - - edited

          In this patch, SolrDispatchFilter.sendError() uses the ResponseWriter to format an error response.

          When the ResponseWriter is a BinaryResponseWriter, the response is written directly to the servlet's output stream.

          The error text is included in the response with key="error". Error responses come back with HTTP status=200, although the status code in the SolrResponse is as expected. I added a test to SolrExampleTests that shows this.

          It would be a trivial change to set the proper HTTP status codes, but a corresponding change in CommonsHttpSolrServer.request() would be needed so as to return a usable response in that case, as the issue describes. For my part, I think raising an exception might be better than returning a response that seems to indicate success, but happens to include an error message. However this goes against the idea of including partial data in a response, which was mentioned as a goal, so I just made a minimal change.

          Show
          Mike Sokolov added a comment - - edited In this patch, SolrDispatchFilter.sendError() uses the ResponseWriter to format an error response. When the ResponseWriter is a BinaryResponseWriter, the response is written directly to the servlet's output stream. The error text is included in the response with key="error". Error responses come back with HTTP status=200, although the status code in the SolrResponse is as expected. I added a test to SolrExampleTests that shows this. It would be a trivial change to set the proper HTTP status codes, but a corresponding change in CommonsHttpSolrServer.request() would be needed so as to return a usable response in that case, as the issue describes. For my part, I think raising an exception might be better than returning a response that seems to indicate success, but happens to include an error message. However this goes against the idea of including partial data in a response, which was mentioned as a goal, so I just made a minimal change.
          Hide
          Robert Muir added a comment -

          Bulk move 3.2 -> 3.3

          Show
          Robert Muir added a comment - Bulk move 3.2 -> 3.3
          Hide
          Robert Muir added a comment -

          3.4 -> 3.5

          Show
          Robert Muir added a comment - 3.4 -> 3.5
          Hide
          Matthew Simoneau added a comment -

          This would be extremely helpful to me. I'm doing a cross-domain JSONP request and there aren't good ways to detect that the call has failed with a 400 response code.

          Show
          Matthew Simoneau added a comment - This would be extremely helpful to me. I'm doing a cross-domain JSONP request and there aren't good ways to detect that the call has failed with a 400 response code.
          Hide
          Ryan McKinley added a comment -

          This patch uses the responeWriter to output the error. It also updates Solrj parsing to read this error

          There are enough API changes, that I am reluctant to port to 3.x

          Show
          Ryan McKinley added a comment - This patch uses the responeWriter to output the error. It also updates Solrj parsing to read this error There are enough API changes, that I am reluctant to port to 3.x
          Hide
          Ryan McKinley added a comment -

          Added in revision: 1297749

          Show
          Ryan McKinley added a comment - Added in revision: 1297749
          Hide
          Mike Sokolov added a comment -

          Thank you! This is a little step that will make a huge difference.

          -Mike

          Show
          Mike Sokolov added a comment - Thank you! This is a little step that will make a huge difference. -Mike
          Hide
          Greg Bowyer added a comment -

          Backport of SOLR-141 to 3.x

          Show
          Greg Bowyer added a comment - Backport of SOLR-141 to 3.x

            People

            • Assignee:
              Ryan McKinley
              Reporter:
              Hoss Man
            • Votes:
              4 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development