GH-89: Deprecate templated expressions

Fixes https://github.com/spring-projects/spring-retry/issues/89

* Use literal expressions also for other expression properties
* Revert changes as suggested in the comments
* Updated copyright
* Polishing code style
* Fix JavaDoc in the `RetryTemplateBuilder`

README.md

@@ -292,17 +292,17 @@ Version 1.2 introduces the ability to use expressions for certain properties:
 
 ```java
 
-@Retryable(exceptionExpression="#{message.contains('this can be retried')}")
+@Retryable(exceptionExpression="message.contains('this can be retried')")
 public void service1() {
   ...
 }
 
-@Retryable(exceptionExpression="#{message.contains('this can be retried')}")
+@Retryable(exceptionExpression="message.contains('this can be retried')")
 public void service2() {
   ...
 }
 
-@Retryable(exceptionExpression="#{@exceptionChecker.shouldRetry(#root)}",
+@Retryable(exceptionExpression="@exceptionChecker.shouldRetry(#root)",
     maxAttemptsExpression = "#{@integerFiveBean}",
   backoff = @Backoff(delayExpression = "#{1}", maxDelayExpression = "#{5}", multiplierExpression = "#{1.1}"))
 public void service3() {
@@ -310,7 +310,7 @@ public void service3() {
 }
 ```
 
-These use the familier Spring SpEL expression syntax (`#{...}`).
+For `exceptionExpression`, templated expressions (`#{...}`) are deprecated in favor of simple expression string (`message.contains('this can be retried')`), since Spring Retry 1.2.5.
 
 Expressions can contain property placeholders such as `#{${max.delay}}` or `#{@exceptionChecker.${retry.method}(#root)}`
 

src/main/java/org/springframework/retry/policy/ExpressionRetryPolicy.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2015 the original author or authors.
+ * Copyright 2015-2019 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -17,6 +17,8 @@ package org.springframework.retry.policy;
 
 import java.util.Map;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 import org.springframework.beans.BeansException;
 import org.springframework.beans.factory.BeanFactory;
 import org.springframework.beans.factory.BeanFactoryAware;
@@ -33,12 +35,15 @@ import org.springframework.util.Assert;
  * further evaluates an expression against the last thrown exception.
  *
  * @author Gary Russell
+ * @author Aldo Sinanaj
  * @since 1.2
  *
  */
 @SuppressWarnings("serial")
 public class ExpressionRetryPolicy extends SimpleRetryPolicy implements BeanFactoryAware {
 
+	private static final Log logger = LogFactory.getLog(ExpressionRetryPolicy.class);
+
 	private static final TemplateParserContext PARSER_CONTEXT = new TemplateParserContext();
 
 	private final Expression expression;
@@ -60,8 +65,7 @@ public class ExpressionRetryPolicy extends SimpleRetryPolicy implements BeanFact
 	 */
 	public ExpressionRetryPolicy(String expressionString) {
 		Assert.notNull(expressionString, "'expressionString' cannot be null");
-		this.expression = new SpelExpressionParser().parseExpression(expressionString,
-				PARSER_CONTEXT);
+		this.expression = getExpression(expressionString);
 	}
 
 	/**
@@ -92,8 +96,7 @@ public class ExpressionRetryPolicy extends SimpleRetryPolicy implements BeanFact
 			boolean traverseCauses, String expressionString, boolean defaultValue) {
 		super(maxAttempts, retryableExceptions, traverseCauses, defaultValue);
 		Assert.notNull(expressionString, "'expressionString' cannot be null");
-		this.expression = new SpelExpressionParser().parseExpression(expressionString,
-				PARSER_CONTEXT);
+		this.expression = getExpression(expressionString);
 	}
 
 	@Override
@@ -118,4 +121,29 @@ public class ExpressionRetryPolicy extends SimpleRetryPolicy implements BeanFact
 		}
 	}
 
+	/**
+	 * Get expression based on the expression string. At the moment supports both literal
+	 * and template expressions. Template expressions are deprecated.
+	 * @param expression the expression string
+	 * @return literal expression or template expression
+	 */
+	private static Expression getExpression(String expression) {
+		if (isTemplate(expression)) {
+			logger.warn("#{...} syntax is not required for this run-time expression "
+					+ "and is deprecated in favor of a simple expression string");
+			return new SpelExpressionParser().parseExpression(expression, PARSER_CONTEXT);
+		}
+		return new SpelExpressionParser().parseExpression(expression);
+	}
+
+	/**
+	 * Check if the expression is a template
+	 * @param expression the expression string
+	 * @return true if the expression string is a template
+	 */
+	private static boolean isTemplate(String expression) {
+		return expression.contains(PARSER_CONTEXT.getExpressionPrefix())
+				&& expression.contains(PARSER_CONTEXT.getExpressionSuffix());
+	}
+
 }

src/main/java/org/springframework/retry/support/RetryTemplateBuilder.java

@@ -1,3 +1,18 @@
+/*
+ * Copyright 2015-2019 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
 package org.springframework.retry.support;
 
 import java.util.ArrayList;
@@ -25,7 +40,8 @@ import org.springframework.util.Assert;
  * builder method - see it's doc.
  *
  * <p>
- * Examples: <pre>{@code
+ * Examples:
+ * <pre>{@code
  * RetryTemplate.builder()
  *      .maxAttempts(10)
  *      .exponentialBackoff(100, 2, 10000)
@@ -66,6 +82,8 @@ import org.springframework.util.Assert;
  * via volatile write, or other safe publication technique)
  *
  * @author Aleksandr Shamukov
+ * @author Artem Bilan
+ *
  * @since 1.3
  */
 public class RetryTemplateBuilder {
@@ -352,10 +370,9 @@ public class RetryTemplateBuilder {
 	/**
 	 * Finish configuration and build resulting {@link RetryTemplate}. For default
 	 * behaviour and concurrency note see class-level doc of {@link RetryTemplateBuilder}.
-	 *
-	 * @implNote The {@code retryPolicy} of the returned {@link RetryTemplate} is always
-	 * an instance of {@link CompositeRetryPolicy}, that consists of one base policy, and
-	 * of {@link BinaryExceptionClassifierRetryPolicy}. The motivation is: whatever base
+	 * The {@code retryPolicy} of the returned {@link RetryTemplate} is always an instance
+	 * of {@link CompositeRetryPolicy}, that consists of one base policy, and of
+	 * {@link BinaryExceptionClassifierRetryPolicy}. The motivation is: whatever base
 	 * policy we use, exception classification is extremely recommended.
 	 * @return new instance of {@link RetryTemplate}
 	 */

src/test/java/org/springframework/retry/annotation/EnableRetryTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2014-2016 the original author or authors.
+ * Copyright 2014-2019 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -46,6 +46,7 @@ import static org.junit.Assert.fail;
  * @author Dave Syer
  * @author Artem Bilan
  * @author Gary Russell
+ * @author Aldo Sinanaj
  * @since 1.1
  */
 public class EnableRetryTests {
@@ -235,6 +236,10 @@ public class EnableRetryTests {
 		SimpleRetryPolicy retryPolicy = (SimpleRetryPolicy) templateAccessor
 				.getPropertyValue("retryPolicy");
 		assertEquals(5, retryPolicy.getMaxAttempts());
+		service.service4();
+		assertEquals(11, service.getCount());
+		service.service5();
+		assertEquals(12, service.getCount());
 		context.close();
 	}
 
@@ -547,6 +552,20 @@ public class EnableRetryTests {
 			}
 		}
 
+		@Retryable(exceptionExpression = "message.contains('this can be retried')")
+		public void service4() {
+			if (count++ < 10) {
+				throw new RuntimeException("this can be retried");
+			}
+		}
+
+		@Retryable(exceptionExpression = "message.contains('this can be retried')", include = RuntimeException.class)
+		public void service5() {
+			if (count++ < 11) {
+				throw new RuntimeException("this can be retried");
+			}
+		}
+
 		public int getCount() {
 			return count;
 		}