From 9a95da2b708d5c7a0d323cfa4faf19ab10b89e81 Mon Sep 17 00:00:00 2001
From: Dave Syer <dsyer@pivotal.io>
Date: Fri, 23 Sep 2016 16:08:56 +0100
Subject: [PATCH] If retry state key is null, context is not cached

If the retry state has a null key it could be an error, but there
are real work scenarios where that just means "no information"
and the safest thing to do in that case is to simply not cache the
retry context (so the retry callback is executed "naturally" and
the exception bubbles up).

See comment in gh-49 about Spring AMQP use case
---
 .../retry/support/RetryTemplate.java          | 15 +++---
 .../policy/StatefulRetryIntegrationTests.java | 46 ++++++++++++++++---
 2 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/src/main/java/org/springframework/retry/support/RetryTemplate.java b/src/main/java/org/springframework/retry/support/RetryTemplate.java
index 390ca7b..434b67c 100644
--- a/src/main/java/org/springframework/retry/support/RetryTemplate.java
+++ b/src/main/java/org/springframework/retry/support/RetryTemplate.java
@@ -411,13 +411,16 @@ public class RetryTemplate implements RetryOperations {
 	private void registerContext(RetryContext context, RetryState state) {
 		if (state != null) {
 			Object key = state.getKey();
-			if (context.getRetryCount() > 0 && !this.retryContextCache.containsKey(key)) {
-				throw new RetryException(
-						"Inconsistent state for failed item key: cache key has changed. "
-								+ "Consider whether equals() or hashCode() for the key might be inconsistent, "
-								+ "or if you need to supply a better key");
+			if (key != null) {
+				if (context.getRetryCount() > 0
+						&& !this.retryContextCache.containsKey(key)) {
+					throw new RetryException(
+							"Inconsistent state for failed item key: cache key has changed. "
+									+ "Consider whether equals() or hashCode() for the key might be inconsistent, "
+									+ "or if you need to supply a better key");
+				}
+				this.retryContextCache.put(key, context);
 			}
-			this.retryContextCache.put(key, context);
 		}
 	}
 
diff --git a/src/test/java/org/springframework/retry/policy/StatefulRetryIntegrationTests.java b/src/test/java/org/springframework/retry/policy/StatefulRetryIntegrationTests.java
index deda94b..197c9ac 100644
--- a/src/test/java/org/springframework/retry/policy/StatefulRetryIntegrationTests.java
+++ b/src/test/java/org/springframework/retry/policy/StatefulRetryIntegrationTests.java
@@ -52,8 +52,9 @@ public class StatefulRetryIntegrationTests {
 		RetryTemplate retryTemplate = new RetryTemplate();
 		MapRetryContextCache cache = new MapRetryContextCache();
 		retryTemplate.setRetryContextCache(cache);
-		retryTemplate.setRetryPolicy(new SimpleRetryPolicy(1, Collections
-				.<Class<? extends Throwable>, Boolean> singletonMap(Exception.class, true)));
+		retryTemplate.setRetryPolicy(new SimpleRetryPolicy(1,
+				Collections.<Class<? extends Throwable>, Boolean>singletonMap(
+						Exception.class, true)));
 
 		assertFalse(cache.containsKey("foo"));
 
@@ -94,8 +95,9 @@ public class StatefulRetryIntegrationTests {
 		RetryTemplate retryTemplate = new RetryTemplate();
 		MapRetryContextCache cache = new MapRetryContextCache();
 		retryTemplate.setRetryContextCache(cache);
-		retryTemplate.setRetryPolicy(new SimpleRetryPolicy(2, Collections
-				.<Class<? extends Throwable>, Boolean> singletonMap(Exception.class, true)));
+		retryTemplate.setRetryPolicy(new SimpleRetryPolicy(2,
+				Collections.<Class<? extends Throwable>, Boolean>singletonMap(
+						Exception.class, true)));
 
 		assertFalse(cache.containsKey("foo"));
 
@@ -136,8 +138,7 @@ public class StatefulRetryIntegrationTests {
 						throw new Exception("Fail");
 					}
 				}, new RecoveryCallback<String>() {
-					public String recover(RetryContext context)
-							throws Exception {
+					public String recover(RetryContext context) throws Exception {
 						return null;
 					}
 				}, retryState);
@@ -151,11 +152,42 @@ public class StatefulRetryIntegrationTests {
 		assertTrue(times.get(2) - times.get(1) >= 150);
 	}
 
+	@Test
+	public void testExternalRetryWithFailAndNoRetryWhenKeyIsNull() throws Throwable {
+		MockRetryCallback callback = new MockRetryCallback();
+
+		RetryState retryState = new DefaultRetryState(null);
+
+		RetryTemplate retryTemplate = new RetryTemplate();
+		MapRetryContextCache cache = new MapRetryContextCache();
+		retryTemplate.setRetryContextCache(cache);
+		retryTemplate.setRetryPolicy(new SimpleRetryPolicy(1,
+				Collections.<Class<? extends Throwable>, Boolean>singletonMap(
+						Exception.class, true)));
+
+		try {
+			retryTemplate.execute(callback, retryState);
+			// The first failed attempt...
+			fail("Expected RuntimeException");
+		}
+		catch (RuntimeException e) {
+			assertEquals(null, e.getMessage());
+		}
+
+		retryTemplate.execute(callback, retryState);
+		// The second attempt is successful by design...
+
+		// Callback is called twice because its state is null: the recovery path should
+		// not be called...
+		assertEquals(2, callback.attempts);
+	}
+
 	/**
 	 * @author Dave Syer
 	 * 
 	 */
-	private static final class MockRetryCallback implements RetryCallback<String, Exception> {
+	private static final class MockRetryCallback
+			implements RetryCallback<String, Exception> {
 		int attempts = 0;
 
 		public String doWithRetry(RetryContext context) throws Exception {