SPR-6760: method called twice if exits via exception in a 'normal' case

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;
 			}
 		}

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;
 			}
 		}

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)'

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() {

org.springframework.expression/src/test/java/org/springframework/expression/spel/testresources/Inventor.java

@@ -32,6 +32,7 @@ public class Inventor {
 	public List<Integer> listOneFive = new ArrayList<Integer>();
 	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;