SpEL expression evaluation should rethrow (not wrap) RuntimeExceptions [SPR-6610]

[spring-projects-issues]

Dave Syer opened SPR-6610 and commented

SpEL expression evaluation should rethrow (not wrap) RuntimeExceptions. SpEL gets used to invoke methods a lot now (maybe that wasn't the primary use case that we had in mind to start with) and it can be really difficult for the user to find out what actually went wrong because it gets wrapped in a large stack of SpEL exceptions. INT-941 is an example of where this surfaced recently.

I guess (like an AOP interceptor) you could argue that sometimes even checked Exceptions should be rethrown. Hard to see how to determine that at runtime.

---

Affects: 3.0 GA

Referenced from: commits 644f306

[spring-projects-issues]

Andy Clement commented

Some recent changes have reduced the stack trace from this (taken from INT-941):

org.springframework.integration.message.MessageHandlingException: Failed to process Message.
...
Caused by: org.springframework.expression.spel.SpelEvaluationException: EL1029E:(pos 8): A problem occurred when trying to execute method 'adapt' on object of type 'org.springframework.batch.admin.integration.JobNameToJobRestartRequestAdapter': 'Problem invoking method: public org.springframework.batch.integration.launch.JobLaunchRequest org.springframework.batch.admin.integration.JobNameToJobRestartRequestAdapter.adapt(java.lang.String) throws org.springframework.batch.core.launch.NoSuchJobException,org.springframework.batch.core.launch.JobParametersNotFoundException'
...
Caused by: org.springframework.expression.AccessException: Problem invoking method: public org.springframework.batch.integration.launch.JobLaunchRequest org.springframework.batch.admin.integration.JobNameToJobRestartRequestAdapter.adapt(java.lang.String) throws org.springframework.batch.core.launch.NoSuchJobException,org.springframework.batch.core.launch.JobParametersNotFoundException
...
Caused by: java.lang.reflect.InvocationTargetException
...
Caused by: org.springframework.batch.core.launch.JobParametersNotFoundException: No job instance found for job=staging

to this (2 layers removed):

org.springframework.integration.message.MessageHandlingException: Failed to process Message.
...
Caused by: org.springframework.expression.spel.SpelEvaluationException: EL1029E:(pos 8): A problem occurred when trying to execute method 'adapt' on object of type 'org.springframework.batch.admin.integration.JobNameToJobRestartRequestAdapter': 'Problem invoking method: public org.springframework.batch.integration.launch.JobLaunchRequest org.springframework.batch.admin.integration.JobNameToJobRestartRequestAdapter.adapt(java.lang.String) throws org.springframework.batch.core.launch.NoSuchJobException,org.springframework.batch.core.launch.JobParametersNotFoundException'
...
Caused by: org.springframework.batch.core.launch.JobParametersNotFoundException: No job instance found for job=staging

I am slightly nervous about also removing the SpEL layer as it adds helpful positional context for a compound expression - but I can see it being unhelpful if the underlying method being invoked exits via exception during normal processing.

But I'll change it. RuntimeExceptions will not be wrapped if thrown from invoked methods.

[spring-projects-issues]

Andy Clement commented

test and fix committed.

Still a little nervous about this - mainly because it means the user wrapping SpEL cannot write 'catch (SpelEvaluationException see) {' and know they will intercept anything that goes wrong.

[spring-projects-issues]

Juergen Hoeller commented

Alternatively, we could throw a specific SpelEvaluationException subclass that clearly indicates an application exception being wrapped - analogous to InvocationTargetException in java.lang.reflect, or MethodInvocationException in org.springframework.beans...

Juergen

[spring-projects-issues]

Dave Syer commented

Can we at least make the "bare" exceptions an option (possibly the default - and just log the diagnostics)? It's quite a big deal for components like in Spring Integration (and potentially Spring Batch when we get to use 3.0 properly) where we want to use SpEL to create POJO delegation patterns. The POJO that is wrapped won't beave as expected if its exceptions are wrapped in anything.

[spring-projects-issues]

Andy Clement commented

Can we at least make the "bare" exceptions an option (possibly the default - and just log
the diagnostics)? It's quite a big deal for components like in Spring Integration
(and potentially Spring Batch when we get to use 3.0 properly) where we want to use SpEL
to create POJO delegation patterns. The POJO that is wrapped won't beave as expected if
its exceptions are wrapped in anything.

'bare' exceptions is now what will happen with my recent commits. I am becoming a little more comfortable
with it if I say that a SpelEvaluationException is thrown if an unexpected problem occurs during evaluation.
If the user is calling code that is designed to throw an exception in some circumstances then that isn't
really an unexpected problem - so it isn't wrapped.