Solr
  1. Solr
  2. SOLR-7254

NullPointerException thrown in the QueryComponent

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 4.10.3
    • Fix Version/s: 5.1
    • Component/s: None
    • Labels:
      None

      Description

      In case of a distributed search, if we pass invalid query parameters (e.g. negative start value), then Solr returns internal server error (HTTP 500 response) due to following NullPointerException,

      {
      "responseHeader":{
      "status":500,
      "QTime":6,
      "params":{
      "indent":"true",
      "start":"-1",
      "q":":",
      "wt":"json"}},
      "error":{
      "trace":"java.lang.NullPointerException\n\tat org.apache.solr.handler.component.QueryComponent.mergeIds(QueryComponent.java:1031)\n\tat org.apache.solr.handler.component.QueryComponent.handleRegularResponses(QueryComponent.java:715)\n\tat org.apache.solr.handler.component.QueryComponent.handleResponses(QueryComponent.java:694)\n\tat org.apache.solr.handler.component.SearchHandler.handleRequestBody(SearchHandler.java:324)\n\tat org.apache.solr.handler.RequestHandlerBase.handleRequest(RequestHandlerBase.java:135)\n\tat org.apache.solr.core.SolrCore.execute(SolrCore.java:1984)\n\tat org.apache.solr.servlet.SolrDispatchFilter.execute(SolrDispatchFilter.java:818)\n\tat org.apache.solr.servlet.SolrDispatchFilter.doFilter(SolrDispatchFilter.java:422)\n\tat org.apache.solr.servlet.SolrDispatchFilter.doFilter(SolrDispatchFilter.java:211)\n\tat org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:235)\n\tat org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)\n\tat org.apache.solr.servlet.SolrHadoopAuthenticationFilter$2.doFilter(SolrHadoopAuthenticationFilter.java:272)\n\tat org.apache.hadoop.security.authentication.server.AuthenticationFilter.doFilter(AuthenticationFilter.java:592)\n\tat org.apache.hadoop.security.token.delegation.web.DelegationTokenAuthenticationFilter.doFilter(DelegationTokenAuthenticationFilter.java:277)\n\tat org.apache.hadoop.security.authentication.server.AuthenticationFilter.doFilter(AuthenticationFilter.java:555)\n\tat org.apache.solr.servlet.SolrHadoopAuthenticationFilter.doFilter(SolrHadoopAuthenticationFilter.java:277)\n\tat org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:235)\n\tat org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)\n\tat org.apache.solr.servlet.HostnameFilter.doFilter(HostnameFilter.java:86)\n\tat org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:235)\n\tat org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206)\n\tat org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:233)\n\tat org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:191)\n\tat org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:127)\n\tat org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:103)\n\tat org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:109)\n\tat org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:293)\n\tat org.apache.coyote.http11.Http11Processor.process(Http11Processor.java:861)\n\tat org.apache.coyote.http11.Http11Protocol$Http11ConnectionHandler.process(Http11Protocol.java:606)\n\tat org.apache.tomcat.util.net.JIoEndpoint$Worker.run(JIoEndpoint.java:489)\n\tat java.lang.Thread.run(Thread.java:745)\n",
      "code":500}}

      The root cause of this error is that in case of a distributed query, input validation is missing.

      (Non distributed version)
      https://github.com/apache/lucene-solr/blob/817303840fce547a1557e330e93e5a8ac0618f34/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java#L284

      (Distributed version)
      https://github.com/apache/lucene-solr/blob/817303840fce547a1557e330e93e5a8ac0618f34/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java#L691

      1. SOLR-7254.patch
        4 kB
        Hrishikesh Gadre
      2. SOLR-7254.patch
        3 kB
        Hrishikesh Gadre
      3. SOLR-7254.patch
        6 kB
        Hrishikesh Gadre
      4. SOLR-7254.patch
        5 kB
        Hrishikesh Gadre
      5. SOLR-7254.patch
        2 kB
        Ramkumar Aiyengar

        Activity

        Hide
        Ramkumar Aiyengar added a comment -

        Here's a patch to start things off. I haven't tested this as yet or added tests for this as yet. Hrishikesh Gadre, if you can knock up some tests for this, that would be great, else I will probably get to it some time. You can start at TestDistributedSearch.

        Show
        Ramkumar Aiyengar added a comment - Here's a patch to start things off. I haven't tested this as yet or added tests for this as yet. Hrishikesh Gadre , if you can knock up some tests for this, that would be great, else I will probably get to it some time. You can start at TestDistributedSearch .
        Hide
        Hrishikesh Gadre added a comment -

        Ramkumar Aiyengar Yes I am working on the test. I will update ASAP

        Show
        Hrishikesh Gadre added a comment - Ramkumar Aiyengar Yes I am working on the test. I will update ASAP
        Hide
        Hrishikesh Gadre added a comment -

        Added a unit test for this scenario. (note I have commented out the test for rows parameter since it is not returning expected error).

        Also since this error is caused by bad parameters (rather than query syntax error), I think we should be throwing SolrException (with errorCode = BAD_REQUEST). Also is QParser the correct place for these validations or should it be in QueryComponent#prepare(...) method ?

        Show
        Hrishikesh Gadre added a comment - Added a unit test for this scenario. (note I have commented out the test for rows parameter since it is not returning expected error). Also since this error is caused by bad parameters (rather than query syntax error), I think we should be throwing SolrException (with errorCode = BAD_REQUEST). Also is QParser the correct place for these validations or should it be in QueryComponent#prepare(...) method ?
        Hide
        Yonik Seeley added a comment -

        Also is QParser the correct place for these validations or should it be in QueryComponent#prepare(...) method ?

        +1, Query component feels like the more natural place.

        Show
        Yonik Seeley added a comment - Also is QParser the correct place for these validations or should it be in QueryComponent#prepare(...) method ? +1, Query component feels like the more natural place.
        Hide
        Hrishikesh Gadre added a comment -

        Ramkumar Aiyengar Yonik Seeley There few additional validations in QueryComponent#process method. e.g.

        https://github.com/apache/lucene-solr/blob/trunk/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java#L314

        This validation does not get invoked in case of distributed search. Is this by design? If not, may be this should also go in the common validation code...

        Show
        Hrishikesh Gadre added a comment - Ramkumar Aiyengar Yonik Seeley There few additional validations in QueryComponent#process method. e.g. https://github.com/apache/lucene-solr/blob/trunk/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java#L314 This validation does not get invoked in case of distributed search. Is this by design? If not, may be this should also go in the common validation code...
        Hide
        Ramkumar Aiyengar added a comment -

        Actually I am not sure myself if QParser should be constructing the SortSpec object from request params – may be that should as such move elsewhere. But it makes sense to validate this at the time the object is created from URL params (which is where my changes currently are) than after the object is created?

        Show
        Ramkumar Aiyengar added a comment - Actually I am not sure myself if QParser should be constructing the SortSpec object from request params – may be that should as such move elsewhere. But it makes sense to validate this at the time the object is created from URL params (which is where my changes currently are) than after the object is created?
        Hide
        Hrishikesh Gadre added a comment -

        Following changes made to the patch

        >> Removed redundant validation check in QueryComponent#process
        >> Fixed a unit test failure.

        Ramkumar Aiyengar Yonik Seeley Should we commit this change and tackle the (possible) refactoring (from QueryParser to QueryComponent) as part of a different JIRA?

        Show
        Hrishikesh Gadre added a comment - Following changes made to the patch >> Removed redundant validation check in QueryComponent#process >> Fixed a unit test failure. Ramkumar Aiyengar Yonik Seeley Should we commit this change and tackle the (possible) refactoring (from QueryParser to QueryComponent) as part of a different JIRA?
        Hide
        Yonik Seeley added a comment -

        The current patch causes TestGroupingSearch to fail.
        I haven't had a chance to look into whether this is just a bad test, or if grouping search actually supports "-1" as a valid value.

        Show
        Yonik Seeley added a comment - The current patch causes TestGroupingSearch to fail. I haven't had a chance to look into whether this is just a bad test, or if grouping search actually supports "-1" as a valid value.
        Hide
        Yonik Seeley added a comment -

        OK, I confirmed through experimentation that grouped search does support "-1" to mean "all groups". This means that the parsing of SortSpec does not have enough context to throw an exception, and it needs to be in QueryComponent (or some place that does have that context).

        FWIW, I've planned to support "rows=-1" to mean "all rows" for normal queries as well... just never got around to it.

        Show
        Yonik Seeley added a comment - OK, I confirmed through experimentation that grouped search does support "-1" to mean "all groups". This means that the parsing of SortSpec does not have enough context to throw an exception, and it needs to be in QueryComponent (or some place that does have that context). FWIW, I've planned to support "rows=-1" to mean "all rows" for normal queries as well... just never got around to it.
        Hide
        Hrishikesh Gadre added a comment -

        Yonik Seeley Yes you are right. As per the SortSpec documentation, rows = -1 is acceptable (Check SortSpec#getCount() method docs). Following changes are made to the patch,
        >> Removed check for rows = -1 along with the unit test
        >> Refactored the validation code to QueryComponent#prepare method (instead of QParser).

        Please take a look.

        Show
        Hrishikesh Gadre added a comment - Yonik Seeley Yes you are right. As per the SortSpec documentation, rows = -1 is acceptable (Check SortSpec#getCount() method docs). Following changes are made to the patch, >> Removed check for rows = -1 along with the unit test >> Refactored the validation code to QueryComponent#prepare method (instead of QParser). Please take a look.
        Hide
        Yonik Seeley added a comment -

        Seems like the last patch is missing the tests that were in the previous patch?

        Show
        Yonik Seeley added a comment - Seems like the last patch is missing the tests that were in the previous patch?
        Hide
        Hrishikesh Gadre added a comment -

        Yonik Seeley OK. I updated the patch to check for rows = -1 in case of non-groupings search

        Show
        Hrishikesh Gadre added a comment - Yonik Seeley OK. I updated the patch to check for rows = -1 in case of non-groupings search
        Hide
        Yonik Seeley added a comment -

        As per the SortSpec documentation, rows = -1 is acceptable

        Looks like rows=-1 for a non grouped search just acts like rows=0 right now.
        I guess we can leave that alone though, and just tackle the start=-1 issue.

        Show
        Yonik Seeley added a comment - As per the SortSpec documentation, rows = -1 is acceptable Looks like rows=-1 for a non grouped search just acts like rows=0 right now. I guess we can leave that alone though, and just tackle the start=-1 issue.
        Hide
        Yonik Seeley added a comment -

        I updated the patch to check for rows = -1 in case of non-groupings search

        Crossed comments (I didn't refresh JIRA before I posted). This should be fine too.

        Show
        Yonik Seeley added a comment - I updated the patch to check for rows = -1 in case of non-groupings search Crossed comments (I didn't refresh JIRA before I posted). This should be fine too.
        Hide
        ASF subversion and git services added a comment -

        Commit 1668976 from Yonik Seeley in branch 'dev/trunk'
        [ https://svn.apache.org/r1668976 ]

        SOLR-7254: invalid start/rows should throw 400 rather than 500 error code

        Show
        ASF subversion and git services added a comment - Commit 1668976 from Yonik Seeley in branch 'dev/trunk' [ https://svn.apache.org/r1668976 ] SOLR-7254 : invalid start/rows should throw 400 rather than 500 error code
        Hide
        ASF subversion and git services added a comment -

        Commit 1668978 from Yonik Seeley in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1668978 ]

        SOLR-7254: invalid start/rows should throw 400 rather than 500 error code

        Show
        ASF subversion and git services added a comment - Commit 1668978 from Yonik Seeley in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1668978 ] SOLR-7254 : invalid start/rows should throw 400 rather than 500 error code
        Hide
        Ramkumar Aiyengar added a comment -

        Thanks for getting to this Yonik..

        Show
        Ramkumar Aiyengar added a comment - Thanks for getting to this Yonik..
        Hide
        Timothy Potter added a comment -

        Bulk close after 5.1 release

        Show
        Timothy Potter added a comment - Bulk close after 5.1 release

          People

          • Assignee:
            Ramkumar Aiyengar
            Reporter:
            Hrishikesh Gadre
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development