Lucene - Core
  1. Lucene - Core
  2. LUCENE-6965

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

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major 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
        Tomás Fernández Löbbe added a comment - - edited

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

        Show
        Tomás Fernández Löbbe added a comment - - edited Possible implementation. Ryan Ernst , Jack Conradson , any thoughts?
        Hide
        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
        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
        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
        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
        Ryan Ernst added a comment -

        Thanks...looks ok I guess then.

        Show
        Ryan Ernst added a comment - Thanks...looks ok I guess then.
        Hide
        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
        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
        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
        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
        Uwe Schindler added a comment -

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

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

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

        Show
        Uwe Schindler added a comment - OK, it is because of the try/catch. Sorry. I hate this extra RuntimeException, sorry!
        Hide
        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
        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
        Ryan Ernst added a comment -

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

        Show
        Ryan Ernst added a comment - +1 to Uwe's suggestion. I did not know that was possible!
        Hide
        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
        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
        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
        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
        Uwe Schindler added a comment -

        Yes!

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

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

        Show
        Tomás Fernández Löbbe added a comment - (complete) patch with suggestions. Something like this Uwe Schindler ?
        Hide
        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
        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
        Uwe Schindler added a comment -

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

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

        Looks good.
        Thanks!

        Show
        Tomás Fernández Löbbe added a comment - Looks good. Thanks!
        Hide
        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
        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
        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
        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
        Uwe Schindler added a comment -

        I'm done. You can resolve.

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

        Thanks for the help!

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

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development