To reproduce add the following test code to TestMethodExpressionImpl: @Test(expected=IllegalArgumentException.class) public void test() { MethodExpression me = factory.createMethodExpression(context, "${beanAA.echo2}", null , new Class[]{String.class}); me.invoke(context, new Object[0]); } The result is an ArrayIndexOutOfBoundsException. I would expect an IllegalArgumentException or a MethodNotFoundException. The same I would expect if I invoke with null: @Test(expected=IllegalArgumentException.class) public void testNull() { MethodExpression me = factory.createMethodExpression(context, "${beanAA.echo2}", null , new Class[]{String.class}); me.invoke(context, null); } But this throws a NullPointerException instead. I run into this because PrimeFaces handles AjaxListeners relatively bad and expect that there is a MethodNotFoundException or an IllegalArgumentException if they call a listener method with no pram. try { listener.invoke(elContext, new Object[]{}); } catch (MethodNotFoundException mnfe) { processArgListener(context, elContext, event); } catch (IllegalArgumentException iae) { processArgListener(context, elContext, event); }
I've looked through the EL specification and I don't see the expected behaviour defined for any of these cases. I don't think MethodNotFoundException is the right exception in this case since the method has been found. IllegalArgumentException strikes me as the better fit. I'll take a look.
Thanks for the report. This has been fixed in trunk (9.0.x), 8.0.x/trunk (for 8.0.22 onwards) and 7.0.x/trunk (for 7.0.62 onwards).
Reviewing r1676234 Apparently "null" src is equivalent to "no argument" or a zero-length array. [1] 1) When null is passed it should support both varargs and non-varargs cases. E.g. calling Foo.main(). From the code of AstValue.hava I think that "paramCount > 0 && src == null" condition fails when the only argument is varargs and src is null. 2) It looks that the message in AstValue.java prints "The method [{0}] was called with [null] parameters" when null is passed as src. Wouldn't it better to print "0" instead of "null" ? [1] http://docs.oracle.com/javaee/7/api/javax/el/MethodExpression.html#invoke%28javax.el.ELContext,%20java.lang.Object[]%29
(In reply to Konstantin Kolinko from comment #3) > Reviewing r1676234 > > Apparently "null" src is equivalent to "no argument" or a zero-length array. > [1] > > 1) When null is passed it should support both varargs and non-varargs cases. > > E.g. calling Foo.main(). ACK. I'll add a test case for that. > From the code of AstValue.hava I think that "paramCount > 0 && src == null" > condition fails when the only argument is varargs and src is null. > > 2) It looks that the message in AstValue.java prints "The method [{0}] was > called with [null] parameters" when null is passed as src. Wouldn't it > better to print "0" instead of "null" ? I'll like to be able to differentiate between a zero length array and null. > [1] > http://docs.oracle.com/javaee/7/api/javax/el/MethodExpression. > html#invoke%28javax.el.ELContext,%20java.lang.Object[]%29
I think MethodExpression.invoke() shall wrap IllegalArgumentException into an ELException. It is documented that java.lang.reflect.Method.invoke() throws IllegalArgumentException when "if the number of actual and formal parameters differ" [2]. I think that our code should work as if that exception were thrown by Method.invoke(). From MethodExpression.invoke() [1] I think that an exception that is received from a Method.invoke() has to be wrapped with an ELException. Essentially this means to move "values = convertArgs(values, m);" call into try/catch block that is around "result = m.invoke(t.base, values);" call. (lines 274 278 of AstValue.java of Tomcat 7 r1676234). [1] http://docs.oracle.com/javaee/7/api/javax/el/MethodExpression.html#invoke%28javax.el.ELContext,%20java.lang.Object[]%29 [2] http://docs.oracle.com/javase/8/docs/api/java/lang/reflect/Method.html#invoke-java.lang.Object-java.lang.Object...-
(In reply to Mark Thomas from comment #4) > (In reply to Konstantin Kolinko from comment #3) > > Reviewing r1676234 > > > > 2) It looks that the message in AstValue.java prints "The method [{0}] was > > called with [null] parameters" when null is passed as src. Wouldn't it > > better to print "0" instead of "null" ? > > I'll like to be able to differentiate between a zero length array and null. > E.g. end user may call the method without any arguments, but some higher API that translates that into a call to MethodExpression is allowed to optimize that to pass "null" instead of allocating a zero-length array of arguments. Printing a message that says "null" may be confusing for the end user.
(In reply to Konstantin Kolinko from comment #5) > I think MethodExpression.invoke() shall wrap IllegalArgumentException into > an ELException. > > > It is documented that java.lang.reflect.Method.invoke() throws > IllegalArgumentException when "if the number of actual and formal parameters > differ" [2]. > > I think that our code should work as if that exception were thrown by > Method.invoke(). From MethodExpression.invoke() [1] I think that an > exception that is received from a Method.invoke() has to be wrapped with an > ELException. > > Essentially this means to move "values = convertArgs(values, m);" call into > try/catch block that is around "result = m.invoke(t.base, values);" call. > (lines 274 278 of AstValue.java of Tomcat 7 r1676234). The problem is, the "bad" PrimeFaces AjaxBehaviorListenerImpl implementation is then broken again. Also I do not like wrapping a specific RuntimeException into an unspecific RuntimeExcpetion without additional information. But on the other hand you are right there is already a wrapping if an IllegalAccessException or an IllegalArgumentException is thrown by Method.invoke(). I also do not really like this wrapping. The Sun/Glassfish implementation does not wrap the runtime exceptions thrown by Method.invoke() as well.
The spec is unclear exactly what the required behaviour here is so (ignoring Konstantin's issue one above) I'm happy that this is an improvement. IAE with a decent error message is much better than AIOOB or NPE. What the 'right' behaviour is is TBD. I have created https://java.net/jira/browse/EL_SPEC-24 to track this. Do add any comments you have for the EL expert group to that Jira ticket. If you haven't already, I'd strongly recommend you raise a bug against PrimeFaces. Until there is some clarification from the EL EG, I'm happy (once issue 1 is fixed) with throwing an IAE in this case.
Issue 1 fixed in the same set of versions as comment #2