DATAREDIS-347 - Fix unintended duplicate script execution.

We now inspect the message of the exception thrown to determine whether the error resulted from a script not being present or an other error. In case the script is not present we simply invoke eval.

Original pull request: #108

src/main/java/org/springframework/data/redis/core/script/DefaultRedisScript.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2013 the original author or authors.
+ * Copyright 2013-2014 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.
@@ -30,6 +30,7 @@ import org.springframework.util.Assert;
  * Singleton to avoid re-calculation of SHA1 on every script execution.
  * 
  * @author Jennifer Hickey
+ * @author Christoph Strobl
  * @param <T> The script result type. Should be one of Long, Boolean, List, or deserialized value type. Can be null if
  *          the script returns a throw-away status (i.e "OK")
  */
@@ -43,6 +44,23 @@ public class DefaultRedisScript<T> implements RedisScript<T>, InitializingBean {
 
 	private final Object shaModifiedMonitor = new Object();
 
+	/**
+	 * Creates a new {@link DefaultRedisScript}
+	 */
+	public DefaultRedisScript() {}
+
+	/**
+	 * Creates a new {@link DefaultRedisScript}
+	 * 
+	 * @param script
+	 * @param resultType
+	 */
+	public DefaultRedisScript(String script, Class<T> resultType) {
+
+		this.setScriptText(script);
+		this.resultType = resultType;
+	}
+
 	public void afterPropertiesSet() throws Exception {
 		Assert.notNull(this.scriptSource, "Either script, script location," + " or script source is required");
 	}

src/main/java/org/springframework/data/redis/core/script/DefaultScriptExecutor.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2013 the original author or authors.
+ * Copyright 2013-2014 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.
@@ -19,6 +19,8 @@ import java.util.ArrayList;
 import java.util.List;
 
 import org.springframework.dao.DataAccessException;
+import org.springframework.dao.UncategorizedDataAccessException;
+import org.springframework.data.redis.RedisSystemException;
 import org.springframework.data.redis.connection.RedisConnection;
 import org.springframework.data.redis.connection.ReturnType;
 import org.springframework.data.redis.core.RedisCallback;
@@ -31,6 +33,8 @@ import org.springframework.data.redis.serializer.RedisSerializer;
  * executed in a pipeline or transaction.
  * 
  * @author Jennifer Hickey
+ * @author Christoph Strobl
+ * @author Thomas Darimont
  * @param <K> The type of keys that may be passed during script execution
  */
 public class DefaultScriptExecutor<K> implements ScriptExecutor<K> {
@@ -71,15 +75,23 @@ public class DefaultScriptExecutor<K> implements ScriptExecutor<K> {
 
 	protected <T> T eval(RedisConnection connection, RedisScript<T> script, ReturnType returnType, int numKeys,
 			byte[][] keysAndArgs, RedisSerializer<T> resultSerializer) {
+		
 		Object result;
 		try {
 			result = connection.evalSha(script.getSha1(), returnType, numKeys, keysAndArgs);
 		} catch (Exception e) {
+
+			if (!expectionContainsNoScriptError(e)) {
+				throw e instanceof RuntimeException ? (RuntimeException) e : new RedisSystemException(e.getMessage(), e);
+			}
+
 			result = connection.eval(scriptBytes(script), returnType, numKeys, keysAndArgs);
 		}
+		
 		if (script.getResultType() == null) {
 			return null;
 		}
+		
 		return deserializeResult(resultSerializer, result);
 	}
 
@@ -133,4 +145,25 @@ public class DefaultScriptExecutor<K> implements ScriptExecutor<K> {
 	protected RedisSerializer keySerializer() {
 		return template.getKeySerializer();
 	}
+
+	private boolean expectionContainsNoScriptError(Exception e) {
+
+		if (!(e instanceof UncategorizedDataAccessException)) {
+			return false;
+		}
+
+		Throwable current = e;
+		while (current != null) {
+
+			String exMessage = current.getMessage();
+			if (exMessage != null && exMessage.contains("NOSCRIPT")) {
+				return true;
+			}
+
+			current = current.getCause();
+		}
+
+		return false;
+	}
+
 }

src/test/java/org/springframework/data/redis/core/script/DefaultScriptExecutorUnitTests.java

@@ -0,0 +1,119 @@
+/*
+ * Copyright 2014 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
+ *
+ *      http://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.data.redis.core.script;
+
+import static org.mockito.Matchers.*;
+import static org.mockito.Mockito.*;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.runners.MockitoJUnitRunner;
+import org.springframework.data.redis.RedisSystemException;
+import org.springframework.data.redis.connection.RedisConnection;
+import org.springframework.data.redis.connection.RedisConnectionFactory;
+import org.springframework.data.redis.connection.ReturnType;
+import org.springframework.data.redis.core.StringRedisTemplate;
+
+/**
+ * @author Christoph Strobl
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class DefaultScriptExecutorUnitTests {
+
+	private final DefaultRedisScript<String> SCRIPT = new DefaultRedisScript<String>("return KEYS[0]", String.class);
+
+	private StringRedisTemplate template;
+	private @Mock RedisConnection redisConnectionMock;
+	private @Mock RedisConnectionFactory connectionFactoryMock;
+
+	private DefaultScriptExecutor<String> executor;
+
+	@Before
+	public void setUp() {
+
+		when(connectionFactoryMock.getConnection()).thenReturn(redisConnectionMock);
+		template = spy(new StringRedisTemplate(connectionFactoryMock));
+		template.afterPropertiesSet();
+
+		executor = new DefaultScriptExecutor<String>(template);
+	}
+
+	/**
+	 * @see DATAREDIS-347
+	 */
+	@Test
+	public void excuteCheckForPresenceOfScriptViaEvalSha1() {
+
+		when(redisConnectionMock.evalSha(anyString(), any(ReturnType.class), anyInt())).thenReturn("FOO".getBytes());
+
+		executor.execute(SCRIPT, null);
+
+		verify(redisConnectionMock, times(1)).evalSha(anyString(), any(ReturnType.class), anyInt());
+	}
+
+	/**
+	 * @see DATAREDIS-347
+	 */
+	@Test
+	public void excuteShouldNotCallEvalWhenSha1Exists() {
+
+		when(redisConnectionMock.evalSha(anyString(), any(ReturnType.class), anyInt())).thenReturn("FOO".getBytes());
+
+		executor.execute(SCRIPT, null);
+
+		verify(redisConnectionMock, never()).eval(any(byte[].class), any(ReturnType.class), anyInt());
+	}
+
+	/**
+	 * @see DATAREDIS-347
+	 */
+	@Test
+	public void excuteShouldUseEvalInCaseNoSha1PresentForGivenScript() {
+
+		when(redisConnectionMock.evalSha(anyString(), any(ReturnType.class), anyInt())).thenThrow(
+				new RedisSystemException("NOSCRIPT No matching script. Please use EVAL.", new Exception()));
+
+		executor.execute(SCRIPT, null);
+
+		verify(redisConnectionMock, times(1)).eval(any(byte[].class), any(ReturnType.class), anyInt());
+	}
+
+	/**
+	 * @see DATAREDIS-347
+	 */
+	@Test(expected = UnsupportedOperationException.class)
+	public void excuteShouldThrowExceptionInCaseEvalShaFailsWithOtherThanRedisSystemException() {
+
+		when(redisConnectionMock.evalSha(anyString(), any(ReturnType.class), anyInt())).thenThrow(
+				new UnsupportedOperationException("NOSCRIPT No matching script. Please use EVAL.", new Exception()));
+
+		executor.execute(SCRIPT, null);
+	}
+
+	/**
+	 * @see DATAREDIS-347
+	 */
+	@Test(expected = RedisSystemException.class)
+	public void excuteShouldThrowExceptionInCaseEvalShaFailsWithAlthoughTheScriptExists() {
+
+		when(redisConnectionMock.evalSha(anyString(), any(ReturnType.class), anyInt())).thenThrow(
+				new RedisSystemException("Found Script but could not execute it.", new Exception()));
+
+		executor.execute(SCRIPT, null);
+	}
+}