From 4c35697c79a944c2083a067fd9ec5082a54dafcf Mon Sep 17 00:00:00 2001 From: Andy Clement Date: Mon, 1 Feb 2010 19:53:34 +0000 Subject: [PATCH] SPR-6760: method called twice if exits via exception in a 'normal' case --- .../spel/ast/ConstructorReference.java | 24 +++++- .../expression/spel/ast/MethodReference.java | 23 +++++- .../spel/ConstructorInvocationTests.java | 81 +++++++++++++++++++ .../spel/MethodInvocationTests.java | 54 +++++++++++++ .../spel/testresources/Inventor.java | 16 ++++ 5 files changed, 194 insertions(+), 4 deletions(-) diff --git a/org.springframework.expression/src/main/java/org/springframework/expression/spel/ast/ConstructorReference.java b/org.springframework.expression/src/main/java/org/springframework/expression/spel/ast/ConstructorReference.java index 90a9763ca7..f75d9d6f1a 100644 --- a/org.springframework.expression/src/main/java/org/springframework/expression/spel/ast/ConstructorReference.java +++ b/org.springframework.expression/src/main/java/org/springframework/expression/spel/ast/ConstructorReference.java @@ -90,8 +90,28 @@ public class ConstructorReference extends SpelNodeImpl { return executorToUse.execute(state.getEvaluationContext(), arguments); } catch (AccessException ae) { - // this is OK - it may have gone stale due to a class change, - // let's try to get a new one and call it before giving up + // Two reasons this can occur: + // 1. the method invoked actually threw a real exception + // 2. the method invoked was not passed the arguments it expected and has become 'stale' + + // In the first case we should not retry, in the second case we should see if there is a + // better suited method. + + // To determine which situation it is, the AccessException will contain a cause - this + // will be the exception thrown by the reflective invocation. Inside this exception there + // may or may not be a root cause. If there is a root cause it is a user created exception. + // If there is no root cause it was a reflective invocation problem. + + Throwable causeOfAccessException = ae.getCause(); + Throwable rootCause = (causeOfAccessException==null?null:causeOfAccessException.getCause()); + if (rootCause!=null) { + // User exception was the root cause - exit now + String typename = (String) children[0].getValueInternal(state).getValue(); + throw new SpelEvaluationException(getStartPosition(), rootCause, SpelMessage.CONSTRUCTOR_INVOCATION_PROBLEM, + typename,FormatHelper.formatMethodForMessage("", argumentTypes)); + } + + // at this point we know it wasn't a user problem so worth a retry if a better candidate can be found this.cachedExecutor = null; } } diff --git a/org.springframework.expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java b/org.springframework.expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java index 42715f8152..a7c899511b 100644 --- a/org.springframework.expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java +++ b/org.springframework.expression/src/main/java/org/springframework/expression/spel/ast/MethodReference.java @@ -77,8 +77,27 @@ public class MethodReference extends SpelNodeImpl { state.getEvaluationContext(), state.getActiveContextObject().getValue(), arguments); } catch (AccessException ae) { - // this is OK - it may have gone stale due to a class change, - // let's try to get a new one and call it before giving up + // Two reasons this can occur: + // 1. the method invoked actually threw a real exception + // 2. the method invoked was not passed the arguments it expected and has become 'stale' + + // In the first case we should not retry, in the second case we should see if there is a + // better suited method. + + // To determine which situation it is, the AccessException will contain a cause - this + // will be the exception thrown by the reflective invocation. Inside this exception there + // may or may not be a root cause. If there is a root cause it is a user created exception. + // If there is no root cause it was a reflective invocation problem. + + Throwable causeOfAccessException = ae.getCause(); + Throwable rootCause = (causeOfAccessException==null?null:causeOfAccessException.getCause()); + if (rootCause!=null) { + // User exception was the root cause - exit now + throw new SpelEvaluationException( getStartPosition(), rootCause, SpelMessage.EXCEPTION_DURING_METHOD_INVOCATION, + this.name, state.getActiveContextObject().getValue().getClass().getName(), rootCause.getMessage()); + } + + // at this point we know it wasn't a user problem so worth a retry if a better candidate can be found this.cachedExecutor = null; } } diff --git a/org.springframework.expression/src/test/java/org/springframework/expression/spel/ConstructorInvocationTests.java b/org.springframework.expression/src/test/java/org/springframework/expression/spel/ConstructorInvocationTests.java index 217557e2c8..d1a62f14d4 100644 --- a/org.springframework.expression/src/test/java/org/springframework/expression/spel/ConstructorInvocationTests.java +++ b/org.springframework.expression/src/test/java/org/springframework/expression/spel/ConstructorInvocationTests.java @@ -16,8 +16,13 @@ package org.springframework.expression.spel; +import org.junit.Assert; import org.junit.Ignore; import org.junit.Test; +import org.springframework.expression.Expression; +import org.springframework.expression.spel.standard.SpelExpressionParser; +import org.springframework.expression.spel.support.StandardEvaluationContext; +import org.springframework.expression.spel.testresources.PlaceOfBirth; /** * Tests invocation of constructors. @@ -36,6 +41,82 @@ public class ConstructorInvocationTests extends ExpressionTestCase { evaluateAndCheckError("new FooBar()",SpelMessage.CONSTRUCTOR_INVOCATION_PROBLEM); } + static class Tester { + public static int counter; + public int i; + + public Tester() {} + + public Tester(int i) { + counter++; + if (i==1) { + throw new IllegalArgumentException("IllegalArgumentException for 1"); + } + if (i==2) { + throw new RuntimeException("RuntimeException for 2"); + } + this.i = i; + } + + public Tester(PlaceOfBirth pob) { + + } + + } + @Test + public void testConstructorThrowingException_SPR6760() { + // Test ctor on inventor: + // On 1 it will throw an IllegalArgumentException + // On 2 it will throw a RuntimeException + // On 3 it will exit normally + // In each case it increments the Tester field 'counter' when invoked + + SpelExpressionParser parser = new SpelExpressionParser(); + Expression expr = parser.parseExpression("new org.springframework.expression.spel.ConstructorInvocationTests$Tester(#bar).i"); + + // Normal exit + StandardEvaluationContext eContext = TestScenarioCreator.getTestEvaluationContext(); + eContext.setRootObject(new Tester()); + eContext.setVariable("bar",3); + Object o = expr.getValue(eContext); + Assert.assertEquals(o,3); + Assert.assertEquals(1,parser.parseExpression("counter").getValue(eContext)); + + // Now the expression has cached that throwException(int) is the right thing to call + // Let's change 'bar' to be a PlaceOfBirth which indicates the cached reference is + // out of date. + eContext.setVariable("bar",new PlaceOfBirth("London")); + o = expr.getValue(eContext); + Assert.assertEquals(0, o); + // That confirms the logic to mark the cached reference stale and retry is working + + + // Now let's cause the method to exit via exception and ensure it doesn't cause + // a retry. + + // First, switch back to throwException(int) + eContext.setVariable("bar",3); + o = expr.getValue(eContext); + Assert.assertEquals(3, o); + Assert.assertEquals(2,parser.parseExpression("counter").getValue(eContext)); + + + // Now cause it to throw an exception: + eContext.setVariable("bar",1); + try { + o = expr.getValue(eContext); + } catch (Exception e) { + // A problem occurred whilst attempting to construct an object of type 'org.springframework.expression.spel.ConstructorInvocationTests$Tester' using arguments '(java.lang.Integer)' + int idx = e.getMessage().indexOf("Tester"); + if (idx==-1) { + Assert.fail("Expected reference to Tester in :"+e.getMessage()); + } + // normal + } + // If counter is 4 then the method got called twice! + Assert.assertEquals(3,parser.parseExpression("counter").getValue(eContext)); + } + @Test public void testVarargsInvocation01() { // Calling 'Fruit(String... strings)' diff --git a/org.springframework.expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java b/org.springframework.expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java index df9e3e9aed..07bbb20be9 100644 --- a/org.springframework.expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java +++ b/org.springframework.expression/src/test/java/org/springframework/expression/spel/MethodInvocationTests.java @@ -16,7 +16,12 @@ package org.springframework.expression.spel; +import org.junit.Assert; import org.junit.Test; +import org.springframework.expression.Expression; +import org.springframework.expression.spel.standard.SpelExpressionParser; +import org.springframework.expression.spel.support.StandardEvaluationContext; +import org.springframework.expression.spel.testresources.PlaceOfBirth; /** * Tests invocation of methods. @@ -69,6 +74,55 @@ public class MethodInvocationTests extends ExpressionTestCase { evaluate("new String('hello 2.0 to you').startsWith(7.0d)", false, Boolean.class); evaluate("new String('7.0 foobar').startsWith(7.0d)", true, Boolean.class); } + + @Test + public void testMethodThrowingException_SPR6760() { + // Test method on inventor: throwException() + // On 1 it will throw an IllegalArgumentException + // On 2 it will throw a RuntimeException + // On 3 it will exit normally + // In each case it increments the Inventor field 'counter' when invoked + + SpelExpressionParser parser = new SpelExpressionParser(); + Expression expr = parser.parseExpression("throwException(#bar)"); + + // Normal exit + StandardEvaluationContext eContext = TestScenarioCreator.getTestEvaluationContext(); + eContext.setVariable("bar",3); + Object o = expr.getValue(eContext); + Assert.assertEquals(o,3); + Assert.assertEquals(1,parser.parseExpression("counter").getValue(eContext)); + + // Now the expression has cached that throwException(int) is the right thing to call + // Let's change 'bar' to be a PlaceOfBirth which indicates the cached reference is + // out of date. + eContext.setVariable("bar",new PlaceOfBirth("London")); + o = expr.getValue(eContext); + Assert.assertEquals("London", o); + // That confirms the logic to mark the cached reference stale and retry is working + + + // Now let's cause the method to exit via exception and ensure it doesn't cause + // a retry. + + // First, switch back to throwException(int) + eContext.setVariable("bar",3); + o = expr.getValue(eContext); + Assert.assertEquals(3, o); + Assert.assertEquals(2,parser.parseExpression("counter").getValue(eContext)); + + + // Now cause it to throw an exception: + eContext.setVariable("bar",1); + try { + o = expr.getValue(eContext); + } catch (Exception e) { + e.printStackTrace(); + // normal + } + // If counter is 4 then the method got called twice! + Assert.assertEquals(3,parser.parseExpression("counter").getValue(eContext)); + } @Test public void testVarargsInvocation01() { diff --git a/org.springframework.expression/src/test/java/org/springframework/expression/spel/testresources/Inventor.java b/org.springframework.expression/src/test/java/org/springframework/expression/spel/testresources/Inventor.java index e266a32c49..747b09f744 100644 --- a/org.springframework.expression/src/test/java/org/springframework/expression/spel/testresources/Inventor.java +++ b/org.springframework.expression/src/test/java/org/springframework/expression/spel/testresources/Inventor.java @@ -32,6 +32,7 @@ public class Inventor { public List listOneFive = new ArrayList(); public String[] stringArrayOfThreeItems = new String[]{"1","2","3"}; private String foo; + public int counter; public Inventor(String name, Date birthdate, String nationality) { this.name = name; @@ -89,6 +90,21 @@ public class Inventor { public PlaceOfBirth getPlaceOfBirth() { return placeOfBirth; } + + public int throwException(int valueIn) { + counter++; + if (valueIn==1) { + throw new IllegalArgumentException("IllegalArgumentException for 1"); + } + if (valueIn==2) { + throw new RuntimeException("RuntimeException for 2"); + } + return valueIn; + } + + public String throwException(PlaceOfBirth pob) { + return pob.getCity(); + } public String getName() { return name;