DATACMNS-525 - Add KeyValuePersistenceExceptionTranslator, fixing Todos.

Added key/value specific PersistenceExceptionTranslator and use it within KeyValueTemplate. Fixed some todos in JavaDoc and added missing tests. Renamed test class for SimpleKeyValueRepository according to the changed name.

Original pull request: #95.

src/main/java/org/springframework/data/keyvalue/core/KeyValuePersistenceExceptionTranslator.java

@@ -0,0 +1,54 @@
+/*
+ * 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.keyvalue.core;
+
+import java.util.NoSuchElementException;
+
+import org.springframework.dao.DataAccessException;
+import org.springframework.dao.DataRetrievalFailureException;
+import org.springframework.dao.support.PersistenceExceptionTranslator;
+
+/**
+ * Simple {@link PersistenceExceptionTranslator} implementation for key/value stores that converts the given runtime
+ * exception to an appropriate exception from the {@code org.springframework.dao} hierarchy.
+ * 
+ * @author Christoph Strobl
+ * @since 1.10
+ */
+public class KeyValuePersistenceExceptionTranslator implements PersistenceExceptionTranslator {
+
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.dao.support.PersistenceExceptionTranslator#translateExceptionIfPossible(java.lang.RuntimeException)
+	 */
+	@Override
+	public DataAccessException translateExceptionIfPossible(RuntimeException e) {
+
+		if (e == null || e instanceof DataAccessException) {
+			return (DataAccessException) e;
+		}
+
+		if (e instanceof NoSuchElementException || e instanceof IndexOutOfBoundsException
+				|| e instanceof IllegalStateException) {
+			return new DataRetrievalFailureException(e.getMessage(), e);
+		}
+
+		if (e.getClass().getName().startsWith("java")) {
+			return new UncategorizedKeyValueException(e.getMessage(), e);
+		}
+		return null;
+	}
+}

src/main/java/org/springframework/data/keyvalue/core/KeyValueTemplate.java

@@ -23,7 +23,9 @@ import java.util.Collection;
 import java.util.List;
 import java.util.concurrent.ConcurrentHashMap;
 
+import org.springframework.dao.DataAccessException;
 import org.springframework.dao.InvalidDataAccessApiUsageException;
+import org.springframework.dao.support.PersistenceExceptionTranslator;
 import org.springframework.data.domain.Sort;
 import org.springframework.data.keyvalue.core.mapping.context.KeyValueMappingContext;
 import org.springframework.data.keyvalue.core.query.KeyValueQuery;
@@ -43,10 +45,13 @@ import org.springframework.util.StringUtils;
  */
 public class KeyValueTemplate implements KeyValueOperations {
 
+	private static final PersistenceExceptionTranslator DEFAULT_PERSISTENCE_EXCEPTION_TRANSLATOR = new KeyValuePersistenceExceptionTranslator();
+
 	private final KeyValueAdapter adapter;
 	private final ConcurrentHashMap<Class<?>, String> keySpaceCache = new ConcurrentHashMap<Class<?>, String>();
 	private final MappingContext<? extends PersistentEntity<?, ? extends PersistentProperty<?>>, ? extends PersistentProperty<?>> mappingContext;
 	private final IdentifierGenerator identifierGenerator;
+	private PersistenceExceptionTranslator exceptionTranslator = DEFAULT_PERSISTENCE_EXCEPTION_TRANSLATOR;
 
 	/**
 	 * Create new {@link KeyValueTemplate} using the given {@link KeyValueAdapter} with a default
@@ -299,9 +304,7 @@ public class KeyValueTemplate implements KeyValueOperations {
 		try {
 			return action.doInKeyValue(this.adapter);
 		} catch (RuntimeException e) {
-
-			// TODO: Use PersistenceExceptionTranslator to translate exception
-			throw e;
+			throw resolveExceptionIfPossible(e);
 		}
 	}
 
@@ -401,6 +404,17 @@ public class KeyValueTemplate implements KeyValueOperations {
 		this.adapter.clear();
 	}
 
+	/**
+	 * Set the {@link PersistenceExceptionTranslator} used for converting {@link RuntimeException}.
+	 * 
+	 * @param exceptionTranslator must not be {@literal null}.
+	 */
+	public void setExceptionTranslator(PersistenceExceptionTranslator exceptionTranslator) {
+
+		Assert.notNull(exceptionTranslator, "ExceptionTranslator must not be null.");
+		this.exceptionTranslator = exceptionTranslator;
+	}
+
 	protected String resolveKeySpace(Class<?> type) {
 
 		Class<?> userClass = ClassUtils.getUserClass(type);
@@ -429,4 +443,10 @@ public class KeyValueTemplate implements KeyValueOperations {
 	private static boolean typeCheck(Class<?> requiredType, Object candidate) {
 		return candidate == null ? true : ClassUtils.isAssignable(requiredType, candidate.getClass());
 	}
+
+	private RuntimeException resolveExceptionIfPossible(RuntimeException e) {
+
+		DataAccessException translatedException = exceptionTranslator.translateExceptionIfPossible(e);
+		return translatedException != null ? translatedException : e;
+	}
 }

src/main/java/org/springframework/data/keyvalue/core/UncategorizedKeyValueException.java

@@ -0,0 +1,31 @@
+/*
+ * 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.keyvalue.core;
+
+import org.springframework.dao.UncategorizedDataAccessException;
+
+/**
+ * @author Christoph Strobl
+ * @since 1.10
+ */
+public class UncategorizedKeyValueException extends UncategorizedDataAccessException {
+
+	private static final long serialVersionUID = -8087116071859122297L;
+
+	public UncategorizedKeyValueException(String msg, Throwable cause) {
+		super(msg, cause);
+	}
+}

src/main/java/org/springframework/data/keyvalue/repository/support/SimpleKeyValueRepository.java

@@ -72,15 +72,15 @@ public class SimpleKeyValueRepository<T, ID extends Serializable> implements Key
 	@Override
 	public Page<T> findAll(Pageable pageable) {
 
+		if (pageable == null) {
+			List<T> result = findAll();
+			return new PageImpl<T>(result, null, result.size());
+		}
+
 		List<T> content = null;
 
-		// TODO: do we need that guard? Can't findInRange(…) be able to deal with null sorts?
-		if (pageable.getSort() != null) {
-			content = operations.findInRange(pageable.getOffset(), pageable.getPageSize(), pageable.getSort(),
-					entityInformation.getJavaType());
-		} else {
-			content = operations.findInRange(pageable.getOffset(), pageable.getPageSize(), entityInformation.getJavaType());
-		}
+		content = operations.findInRange(pageable.getOffset(), pageable.getPageSize(), pageable.getSort(),
+				entityInformation.getJavaType());
 
 		return new PageImpl<T>(content, pageable, this.operations.count(entityInformation.getJavaType()));
 	}

src/test/java/org/springframework/data/keyvalue/core/KeyValuePersistenceExceptionTranslatorUnitTests.java

@@ -0,0 +1,96 @@
+/*
+ * 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.keyvalue.core;
+
+import static org.hamcrest.core.IsInstanceOf.*;
+import static org.hamcrest.core.IsNull.*;
+import static org.junit.Assert.*;
+
+import java.util.NoSuchElementException;
+
+import org.junit.Test;
+import org.springframework.beans.factory.NoSuchBeanDefinitionException;
+import org.springframework.dao.DataRetrievalFailureException;
+
+/**
+ * @author Christoph Strobl
+ */
+public class KeyValuePersistenceExceptionTranslatorUnitTests {
+
+	KeyValuePersistenceExceptionTranslator translator = new KeyValuePersistenceExceptionTranslator();
+
+	/**
+	 * @see DATACMNS-525
+	 */
+	@Test
+	public void translateExeptionShouldReturnDataAccessExceptionWhenGivenOne() {
+		assertThat(translator.translateExceptionIfPossible(new DataRetrievalFailureException("booh")),
+				instanceOf(DataRetrievalFailureException.class));
+	}
+
+	/**
+	 * @see DATACMNS-525
+	 */
+	@Test
+	public void translateExeptionShouldReturnNullWhenGivenNull() {
+		assertThat(translator.translateExceptionIfPossible(null), nullValue());
+	}
+
+	/**
+	 * @see DATACMNS-525
+	 */
+	@Test
+	public void translateExeptionShouldTranslateNoSuchElementExceptionToDataRetrievalFailureException() {
+		assertThat(translator.translateExceptionIfPossible(new NoSuchElementException("")),
+				instanceOf(DataRetrievalFailureException.class));
+	}
+
+	/**
+	 * @see DATACMNS-525
+	 */
+	@Test
+	public void translateExeptionShouldTranslateIndexOutOfBoundsExceptionToDataRetrievalFailureException() {
+		assertThat(translator.translateExceptionIfPossible(new IndexOutOfBoundsException("")),
+				instanceOf(DataRetrievalFailureException.class));
+	}
+
+	/**
+	 * @see DATACMNS-525
+	 */
+	@Test
+	public void translateExeptionShouldTranslateIllegalStateExceptionToDataRetrievalFailureException() {
+		assertThat(translator.translateExceptionIfPossible(new IllegalStateException("")),
+				instanceOf(DataRetrievalFailureException.class));
+	}
+
+	/**
+	 * @see DATACMNS-525
+	 */
+	@Test
+	public void translateExeptionShouldTranslateAnyJavaExceptionToUncategorizedKeyValueException() {
+		assertThat(translator.translateExceptionIfPossible(new UnsupportedOperationException("")),
+				instanceOf(UncategorizedKeyValueException.class));
+	}
+
+	/**
+	 * @see DATACMNS-525
+	 */
+	@Test
+	public void translateExeptionShouldReturnNullForNonJavaExceptions() {
+		assertThat(translator.translateExceptionIfPossible(new NoSuchBeanDefinitionException("")), nullValue());
+	}
+
+}

src/test/java/org/springframework/data/keyvalue/core/KeyValueTemplateUnitTests.java

@@ -404,6 +404,14 @@ public class KeyValueTemplateUnitTests {
 		assertThat(template.findById("1", SUBCLASS_OF_ALIASED.getClass()), nullValue());
 	}
 
+	/**
+	 * @see DATACMNS-525
+	 */
+	@Test(expected = IllegalArgumentException.class)
+	public void setttingNullPersistenceExceptionTranslatorShouldThrowException() {
+		template.setExceptionTranslator(null);
+	}
+
 	static class Foo {
 
 		String foo;

src/test/java/org/springframework/data/keyvalue/repository/SimpleKeyValueRepositoryUnitTests.java

@@ -27,6 +27,9 @@ import org.mockito.Mock;
 import org.mockito.runners.MockitoJUnitRunner;
 import org.springframework.data.annotation.Id;
 import org.springframework.data.annotation.Persistent;
+import org.springframework.data.domain.PageRequest;
+import org.springframework.data.domain.Pageable;
+import org.springframework.data.domain.Sort;
 import org.springframework.data.keyvalue.core.KeyValueOperations;
 import org.springframework.data.keyvalue.repository.support.SimpleKeyValueRepository;
 import org.springframework.data.repository.core.support.ReflectionEntityInformation;
@@ -35,7 +38,7 @@ import org.springframework.data.repository.core.support.ReflectionEntityInformat
  * @author Christoph Strobl
  */
 @RunWith(MockitoJUnitRunner.class)
-public class BasicKeyValueRepositoryUnitTests {
+public class SimpleKeyValueRepositoryUnitTests {
 
 	private SimpleKeyValueRepository<Foo, String> repo;
 	private @Mock KeyValueOperations opsMock;
@@ -58,7 +61,10 @@ public class BasicKeyValueRepositoryUnitTests {
 		SimpleKeyValueRepository<WithNumericId, Integer> temp = new SimpleKeyValueRepository<WithNumericId, Integer>(ei,
 				opsMock);
 
-		WithNumericId foo = temp.save(new WithNumericId());
+		WithNumericId withNumericId = new WithNumericId();
+		temp.save(withNumericId);
+
+		verify(opsMock, times(1)).insert(eq(withNumericId));
 	}
 
 	/**
@@ -136,6 +142,40 @@ public class BasicKeyValueRepositoryUnitTests {
 		verify(opsMock, times(3)).findById(anyString(), eq(Foo.class));
 	}
 
+	/**
+	 * @see DATACMNS-525
+	 */
+	@Test
+	public void findAllWithPageableShouldDelegateToOperationsCorrectlyWhenPageableDoesNotContainSort() {
+
+		repo.findAll(new PageRequest(10, 15));
+
+		verify(opsMock, times(1)).findInRange(eq(150), eq(15), isNull(Sort.class), eq(Foo.class));
+	}
+
+	/**
+	 * @see DATACMNS-525
+	 */
+	@Test
+	public void findAllWithPageableShouldDelegateToOperationsCorrectlyWhenPageableContainsSort() {
+
+		Sort sort = new Sort("for", "bar");
+		repo.findAll(new PageRequest(10, 15, sort));
+
+		verify(opsMock, times(1)).findInRange(eq(150), eq(15), eq(sort), eq(Foo.class));
+	}
+
+	/**
+	 * @see DATACMNS-525
+	 */
+	@Test
+	public void findAllShouldFallbackToFindAllOfWhenGivenNullPageable() {
+
+		repo.findAll((Pageable) null);
+
+		verify(opsMock, times(1)).findAll(eq(Foo.class));
+	}
+
 	static class Foo {
 
 		private @Id String id;