Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-6965

Expression's JavascriptCompiler to throw ParseExceptions with bad function names or arity

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.5, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Currently JavascriptCompiler will throw IllegalArgumentException for bad function names (or functions that don't exist) and for bad arity. I can see why this was done this way, but I believe ParseException would also be correct and it would be better since that's the exception clients will be prepared to receive.

      1. LUCENE-6965.patch
        7 kB
        Tomás Fernández Löbbe
      2. LUCENE-6965.patch
        8 kB
        Tomás Fernández Löbbe
      3. LUCENE-6965.patch
        7 kB
        Tomás Fernández Löbbe
      4. LUCENE-6965-2.patch
        4 kB
        Uwe Schindler

        Activity

        Hide
        tomasflobbe Tomás Fernández Löbbe added a comment - - edited

        Possible implementation. Ryan Ernst, Jack Conradson, any thoughts?

        Show
        tomasflobbe Tomás Fernández Löbbe added a comment - - edited Possible implementation. Ryan Ernst , Jack Conradson , any thoughts?
        Hide
        rjernst Ryan Ernst added a comment -

        ParseException seems fine, but why the intermediate exception? Also, it looks like you are losing stack frames when creating a new exception from the intermediate.

        Show
        rjernst Ryan Ernst added a comment - ParseException seems fine, but why the intermediate exception? Also, it looks like you are losing stack frames when creating a new exception from the intermediate.
        Hide
        tomasflobbe Tomás Fernández Löbbe added a comment -

        why the intermediate exception?

        I couldn't find a better way to propagate the exception. Throwing ParseException from the visitor can't be done, since the visitor is called from RuleContext.accept, which doesn't throw any exceptions.
        I wouldn't want to catch a general IllegalArgumentException to convert it to ParseException, something unexpected inside visitor.visit could throw an IllegalArgumentException and I don't want to change the type of that.

        Also, it looks like you are losing stack frames when creating a new exception from the intermediate.

        Yes, unfortunately the ParseException doesn't support providing a cause in the constructor. I can add the cause by using: exception.initCause(e);. Attached new patch with this change and made the intermediate exception static

        Show
        tomasflobbe Tomás Fernández Löbbe added a comment - why the intermediate exception? I couldn't find a better way to propagate the exception. Throwing ParseException from the visitor can't be done, since the visitor is called from RuleContext.accept, which doesn't throw any exceptions. I wouldn't want to catch a general IllegalArgumentException to convert it to ParseException, something unexpected inside visitor.visit could throw an IllegalArgumentException and I don't want to change the type of that. Also, it looks like you are losing stack frames when creating a new exception from the intermediate. Yes, unfortunately the ParseException doesn't support providing a cause in the constructor. I can add the cause by using: exception.initCause(e); . Attached new patch with this change and made the intermediate exception static
        Hide
        rjernst Ryan Ernst added a comment -

        Thanks...looks ok I guess then.

        Show
        rjernst Ryan Ernst added a comment - Thanks...looks ok I guess then.
        Hide
        tomasflobbe Tomás Fernández Löbbe added a comment -

        Committed to trunk with revission 1723631. Sorry, I forgot to include the Jira code to the commit message.

        Show
        tomasflobbe Tomás Fernández Löbbe added a comment - Committed to trunk with revission 1723631. Sorry, I forgot to include the Jira code to the commit message.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1723632 from Tomás Fernández Löbbe in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1723632 ]

        LUCENE-6965: Expression's JavascriptCompiler now throw ParseException with bad function names or bad arity instead of IllegalArgumentException

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1723632 from Tomás Fernández Löbbe in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1723632 ] LUCENE-6965 : Expression's JavascriptCompiler now throw ParseException with bad function names or bad arity instead of IllegalArgumentException
        Hide
        thetaphi Uwe Schindler added a comment -

        Can we revert the useless variable for the JavaScriptVisitor? It was made like that to have it encapsulated.

        Show
        thetaphi Uwe Schindler added a comment - Can we revert the useless variable for the JavaScriptVisitor? It was made like that to have it encapsulated.
        Hide
        thetaphi Uwe Schindler added a comment -

        OK, it is because of the try/catch. Sorry. I hate this extra RuntimeException, sorry!

        Show
        thetaphi Uwe Schindler added a comment - OK, it is because of the try/catch. Sorry. I hate this extra RuntimeException, sorry!
        Hide
        thetaphi Uwe Schindler added a comment -

        Instead of the runtime exception as cause, it is much better to simply replace the stack trace of the ParseException by the private runtime one:

        exception.initCause(e);
        // gets:
        exception.setStackTrace(e.getStackTrace());
        

        By that it looks like the Exception was thrown from the visitor and the internal private exception is completely hidden.
        I did the same in MMapDirectory to convert wrong OOM because mmap failed to IOEx.

        Show
        thetaphi Uwe Schindler added a comment - Instead of the runtime exception as cause, it is much better to simply replace the stack trace of the ParseException by the private runtime one: exception.initCause(e); // gets: exception.setStackTrace(e.getStackTrace()); By that it looks like the Exception was thrown from the visitor and the internal private exception is completely hidden. I did the same in MMapDirectory to convert wrong OOM because mmap failed to IOEx.
        Hide
        rjernst Ryan Ernst added a comment -

        +1 to Uwe's suggestion. I did not know that was possible!

        Show
        rjernst Ryan Ernst added a comment - +1 to Uwe's suggestion. I did not know that was possible!
        Hide
        thetaphi Uwe Schindler added a comment -

        Another alternative is to use the "rethrower" and directly throw ParseException without declaring it. Currently only in test-framework and AttributeFactory classes (to handle invokeExact rethrowing, the one in AttributeFactory is more compact...).

        I can make a patch for that.

        Show
        thetaphi Uwe Schindler added a comment - Another alternative is to use the "rethrower" and directly throw ParseException without declaring it. Currently only in test-framework and AttributeFactory classes (to handle invokeExact rethrowing, the one in AttributeFactory is more compact...). I can make a patch for that.
        Hide
        tomasflobbe Tomás Fernández Löbbe added a comment -

        Another alternative is to use the "rethrower" and directly throw ParseException without declaring it. Currently only in test-framework and AttributeFactory classes (to handle invokeExact rethrowing, the one in AttributeFactory is more compact...).

        OK, that would remove the need of the ParseRuntimeException, right?

        Show
        tomasflobbe Tomás Fernández Löbbe added a comment - Another alternative is to use the "rethrower" and directly throw ParseException without declaring it. Currently only in test-framework and AttributeFactory classes (to handle invokeExact rethrowing, the one in AttributeFactory is more compact...). OK, that would remove the need of the ParseRuntimeException, right?
        Hide
        thetaphi Uwe Schindler added a comment -

        Yes!

        Show
        thetaphi Uwe Schindler added a comment - Yes!
        Hide
        tomasflobbe Tomás Fernández Löbbe added a comment -

        (complete) patch with suggestions. Something like this Uwe Schindler?

        Show
        tomasflobbe Tomás Fernández Löbbe added a comment - (complete) patch with suggestions. Something like this Uwe Schindler ?
        Hide
        thetaphi Uwe Schindler added a comment -

        Here is the patch. In Java 8 it would also work with a single method, but this one is more safe [as it does not rely on type inference].

        Show
        thetaphi Uwe Schindler added a comment - Here is the patch. In Java 8 it would also work with a single method, but this one is more safe [as it does not rely on type inference] .
        Hide
        thetaphi Uwe Schindler added a comment -

        Mine has the method more private and non-static. I also updated the messages. Should I commit?

        Show
        thetaphi Uwe Schindler added a comment - Mine has the method more private and non-static. I also updated the messages. Should I commit?
        Hide
        tomasflobbe Tomás Fernández Löbbe added a comment -

        Looks good.
        Thanks!

        Show
        tomasflobbe Tomás Fernández Löbbe added a comment - Looks good. Thanks!
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1723642 from Uwe Schindler in branch 'dev/trunk'
        [ https://svn.apache.org/r1723642 ]

        LUCENE-6965: Changed exception handling in JavascriptCompiler using "sneaky rethrow"

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1723642 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1723642 ] LUCENE-6965 : Changed exception handling in JavascriptCompiler using "sneaky rethrow"
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1723643 from Uwe Schindler in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1723643 ]

        Merged revision(s) 1723642 from lucene/dev/trunk:
        LUCENE-6965: Changed exception handling in JavascriptCompiler using "sneaky rethrow"

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1723643 from Uwe Schindler in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1723643 ] Merged revision(s) 1723642 from lucene/dev/trunk: LUCENE-6965 : Changed exception handling in JavascriptCompiler using "sneaky rethrow"
        Hide
        thetaphi Uwe Schindler added a comment -

        I'm done. You can resolve.

        Show
        thetaphi Uwe Schindler added a comment - I'm done. You can resolve.
        Hide
        tomasflobbe Tomás Fernández Löbbe added a comment -

        Thanks for the help!

        Show
        tomasflobbe Tomás Fernández Löbbe added a comment - Thanks for the help!

          People

          • Assignee:
            Unassigned
            Reporter:
            tomasflobbe Tomás Fernández Löbbe
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development