Solr
  1. Solr
  2. SOLR-2813

TrieTokenizerFactory should catch NumberFormatException, return 400 (not 500)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 3.4, 4.0-ALPHA
    • Fix Version/s: 4.0-ALPHA
    • Component/s: Schema and Analysis
    • Labels:
      None
    • Environment:

      4.0 trunk, snapshot taken 09/08/2011.

      Description

      TrieTokenizerFactory is allowing bad user input to result in a 500 error rather than a 400. For a long-valued field, for example, this code in TrieTokenizerFactory.reset() will throw a NumberFormatException:

      > case LONG:
      > ts.setLongValue(Long.parseLong(v));
      > break;

      The NFE gets all the way out to RequestHandlerBase.handleRequest():

      > catch (Exception e) {
      > SolrException.log(SolrCore.log,e);
      > if (e instanceof ParseException)

      { > e = new SolrException(SolrException.ErrorCode.BAD_REQUEST, e); > }

      but is not caught here, and ends up coming out of SolrDispatchFilter.sendError as a 500.

      Simply catching NFE and turning it into a SolrException does the trick:

      ==== solr/core/src/java/org/apache/solr/analysis/TrieTokenizerFactory.java#1 - /4.0-trunk-09082011/solr/core/src/java/org/apache/solr/analysis/TrieTokenizerFactory.java ====
      110a111,112
      > } catch (NumberFormatException e) {
      > throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Unable to parse input", e);

        Activity

        Hide
        Hoss Man added a comment -

        Jeff: thanks for reporting this.

        I've committed a match similar to what you proposed (except with slightly different scoping so we can include the value in the error message) to trunk...

        Committed revision 1202499.

        ...and i'm currently testing the backport to 3x

        Show
        Hoss Man added a comment - Jeff: thanks for reporting this. I've committed a match similar to what you proposed (except with slightly different scoping so we can include the value in the error message) to trunk... Committed revision 1202499. ...and i'm currently testing the backport to 3x
        Hide
        Hoss Man added a comment -

        Ok .. backporting has lead me into some confusion.

        The new tests were failing, which lead me to an investigation where i discovered that TrieTokenizerFactory isn't used as much as I thought – some of the things I was testing for were actually dependent on SOLR-2402, so I backported that to 3x.

        Once that was done, backporting this patch caused queries and updates with malformed numbers to both return 400 errors instead of 500 errors in ad-hoc testing, but it still didn't fix the failures in BasicFunctionalityTest. Digging in a little more the problem seems to be that on trunk, the TestHarness.update method uses DirectSolrConnection which propgates any errors into the test framework, while on the 3x branch it's still using XmlUpdateRequestHandler.doLegacyUpdate so the exception is hidden from the test.

        I need to look at this again tomorrow when i have some more time – the functionality of this patch is good to go on 3x, it's just the damn test case that is anoying.

        (Looks like the TestHarness changes are from r1054124 as part of SOLR-1930 ... maybe just backport the test changes from that commit w/o removing the deprecated method?)

        Show
        Hoss Man added a comment - Ok .. backporting has lead me into some confusion. The new tests were failing, which lead me to an investigation where i discovered that TrieTokenizerFactory isn't used as much as I thought – some of the things I was testing for were actually dependent on SOLR-2402 , so I backported that to 3x. Once that was done, backporting this patch caused queries and updates with malformed numbers to both return 400 errors instead of 500 errors in ad-hoc testing, but it still didn't fix the failures in BasicFunctionalityTest. Digging in a little more the problem seems to be that on trunk, the TestHarness.update method uses DirectSolrConnection which propgates any errors into the test framework, while on the 3x branch it's still using XmlUpdateRequestHandler.doLegacyUpdate so the exception is hidden from the test. I need to look at this again tomorrow when i have some more time – the functionality of this patch is good to go on 3x, it's just the damn test case that is anoying. (Looks like the TestHarness changes are from r1054124 as part of SOLR-1930 ... maybe just backport the test changes from that commit w/o removing the deprecated method?)
        Hide
        Hoss Man added a comment -

        Sorry ... I've given up on trying to backport this to 3x.

        After beating my head against a wall trying to merge r1054124, I just manually hacked the cahnges in, got all existing tests to pass, and then backported r1202499 only to discover that on the 3x branch TrieTokenizerFactory evidently isn't used in all the same places, so the NFE was propagating all the way up.

        I'm moving on to other things, so resolving this for now. Can be re-opened as needed if someone decides to tackle the backport.

        Show
        Hoss Man added a comment - Sorry ... I've given up on trying to backport this to 3x. After beating my head against a wall trying to merge r1054124, I just manually hacked the cahnges in, got all existing tests to pass, and then backported r1202499 only to discover that on the 3x branch TrieTokenizerFactory evidently isn't used in all the same places, so the NFE was propagating all the way up. I'm moving on to other things, so resolving this for now. Can be re-opened as needed if someone decides to tackle the backport.

          People

          • Assignee:
            Hoss Man
            Reporter:
            Jeff Crump
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 1h
              1h
              Remaining:
              Remaining Estimate - 1h
              1h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development