-
Notifications
You must be signed in to change notification settings - Fork 37.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More expected exception tweaks for SpEL expression method invocations [SPR-6941] #11606
Comments
Andy Clement commented I've fixed (1) (for 3.0.2) For (2) it seems like it will get quite ugly. Although I can detect upon method invocation that the thrown exception is declared to be thrown by the method, what would I put on the top level SpEL API? getValue() - I would have to declare that it throws a checked exception (and then all callers always have to write catch blocks) - if I don't declare it then I can't throw it unwrapped from getValue(). |
Juergen Hoeller commented Hmm, maybe we need to do an InvocationTargetException/UndeclaredThrowableException kind of convention where the caller of getValue is required to unwrap the actual exception thrown. Or we'll have to declare "throws Exception" on getValue. I don't see any other options. Let's try to sort this out for 3.0.2 - one way or the other. Juergen |
Andy Clement commented I am leaning to doing something like Juergen suggests. I would still wrap the underlying exception (to remove the need for throws Exception on getValue) but it would be wrapped in something different (subtype of?) a normal SpelEvaluationException so that it is easily identifiable as not being a totally unexpected spel problem. |
Dave Syer commented Can't we do both: wrap checked exceptions internally in the SpEL stack an then unwrap and rethrow in the client API. Isn't that what happens in AOP proxies? |
Andy Clement commented By client api do you mean the getValue() call? I got all kinds of abuse about it not being 'the spring way' when I originally had getValue() declare that it threw checked exceptions... |
Dave Syer commented OK, I see what the issue was there. You don't want clients to have to check all exceptions. My use case is really for a framework developer though: Spring Integration wants to use SPeL as a short cut for writing POJOs, and so it is actually the Integration handler that calls SpelExpression.getValue(). So it can unpack the root cause if we make it easy enough (right now it is rather opaque). Optimum would be if getValue() was to just throw a well know RuntimeException subtype with a cause that was directly the checked exception from the POJO method. |
Juergen Hoeller commented That sounds like a good compromise to me: a well-known RuntimeException subtype with the original exception as direct cause is a good idea. Would be great to get this into 3.0.2 still... (ideally in the course of today since we're aiming for an integration test candidate tonight). Juergen |
Andy Clement commented When this situation arises, SpEL will now throw an 'ExpressionInvocationTargetException' - an unchecked exception whose top level cause is the checked exception resulting from the method invocation. If you can think of a better name for the exception let me know! |
Dave Syer opened SPR-6941 and commented
#11276 added a new exception handling pathway to MethodReference. There are two outstanding issues:
1 The new handling pathway is only taken if the cachedExecutor is not null (no presumably only on the first execution)
2 It only handles RuntimeException in a special way and not checked Exceptions (which might still be legal and expected)
1 looks like a bug? 2 is more tricky, but I believe still against the spirit of #11276 and the comments added in the code to explain the new handling pathway. I think that if the method declares that it throws the exception that is being handled it should be rethrown without wrapping.
Affects: 3.0.1
Referenced from: commits 2b0655b, 2dd1134
The text was updated successfully, but these errors were encountered: