This content has been marked as final.
Show 5 replies
-
1. Re: Installing instance of wrong class gives weird error
alesj Jun 11, 2008 10:12 AM (in response to wolfc)"wolfc" wrote:
It should then check whether the declaring class is assignable from the target class.
I'll add this check to ReflectionUtils:protected static void checkMember(Object target, Member member, String memberType) throws Throwable { if (member == null) throw new IllegalArgumentException("Null " + memberType); if (target != null) { Class<?> declaringClass = member.getDeclaringClass(); if (declaringClass.isInstance(target) == false) throw new IllegalArgumentException("Target (" + Strings.defaultToString(target) + ") doesn't match " + memberType + " 's (" + member + ") declaring class."); } }
-
2. Re: Installing instance of wrong class gives weird error
adrian.brock Jun 11, 2008 10:42 AM (in response to wolfc)That's not correct. It's now doing the check twice.
Once when you do it and once inside the JDK reflection api.
That's horribly inefficient, since these reflection calls are used everywhere.
The whole point of ReflectionUtils is give more reasonable error messages
after reflection calls fail, because the JDK is usually hopeless.
e.g. you'll get something likejava.lang.IllegalArgumentException: wrong type java.lang.String
Without it telling you what invocation is being attempted or which
parameter is wrong.
What's required is to fix "handleErrors" to also check whether IllegalArgumentException
is due to the wrong target object being passed for non-status invocations
and not just assuming that it is due to an invalid parameter being passed.public static Throwable handleErrors(String context, Object target, + Class<?> clazz, + Object instance, Class<?>[] parameters, Object[] arguments, Throwable t) throws Throwable { if (t instanceof IllegalArgumentException) { if (target == null) throw new IllegalArgumentException("Null target for " + context); + if (clazz != null && instance != null && clazz.isAssignableFrom(instance) == false) + throw new IllegalArgumentException("Wrong target. " + instance.getClass().getName() + " is not a " + clazz.getName()); ArrayList<String> expected = new ArrayList<String>(); if (parameters != null) { for (int i = 0; i < parameters.length; ++i) expected.add(parameters.getName()); } ArrayList<String> actual = new ArrayList<String>(); if (arguments != null) { for (int i = 0; i < arguments.length; ++i) { if (arguments == null) actual.add(null); else actual.add(arguments.getClass().getName()); } } throw new IllegalArgumentException("Wrong arguments. " + context + " for target " + target + " expected=" + expected + " actual=" + actual); } else if (t instanceof InvocationTargetException) { throw ((InvocationTargetException) t).getTargetException(); } throw t; }
andpublic static Object invoke(Method method, Object target, Object[] arguments) throws Throwable { if (method == null) throw new IllegalArgumentException("Null method"); try { return method.invoke(target, arguments); } catch (Throwable t) { throw handleErrors(method.getName(), Strings.defaultToString(target), + method.getDeclaringClass(), target, method.getParameterTypes(), arguments, t); } }
An alternative fix would be to produce one generic error message for both
that says something
Invalid invocation TargetClass.methodName(ActualParamType1, ActualParamType2, ...)
for MethodDeclaringClass.methodName(MethodParam1, MethodParam2, ...) -
3. Re: Installing instance of wrong class gives weird error
alesj Jun 12, 2008 5:31 AM (in response to wolfc)With the current methods on ReflectUtils this only comes in play with method and field invocation, so I only added that code there - and not to handleError method.
-
4. Re: Installing instance of wrong class gives weird error
adrian.brock Jun 12, 2008 11:25 AM (in response to wolfc)The point is that it shouldn't do anything at all unless there is an error.
You effectively have:public void doSomething(X parameter) { check(parameter); doIt(parameter); } private void doIt(parameter) { check(parameter); ... }
The correct code - since we don't implement doIt() and it gives bad error messages should be:public void doSomething(X parameter) { try { doIt(parameter); } catch (Exception e) { throw fixErrorMessage(e); } }
-
5. Re: Installing instance of wrong class gives weird error
alesj Jun 12, 2008 11:38 AM (in response to wolfc)"adrian@jboss.org" wrote:
The point is that it shouldn't do anything at all unless there is an error.
That's what I do.
"that code" in my last post is meant your error handling code. ;-)