From 52108ed006abe32fa558bcf258fad828c5b39e02 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Tue, 25 Nov 2014 12:44:00 +0100 Subject: [PATCH] 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. --- ...eyValuePersistenceExceptionTranslator.java | 54 +++++++++++ .../data/keyvalue/core/KeyValueTemplate.java | 26 ++++- .../core/UncategorizedKeyValueException.java | 31 ++++++ .../support/SimpleKeyValueRepository.java | 14 +-- ...rsistenceExceptionTranslatorUnitTests.java | 96 +++++++++++++++++++ .../core/KeyValueTemplateUnitTests.java | 8 ++ ...=> SimpleKeyValueRepositoryUnitTests.java} | 44 ++++++++- 7 files changed, 261 insertions(+), 12 deletions(-) create mode 100644 src/main/java/org/springframework/data/keyvalue/core/KeyValuePersistenceExceptionTranslator.java create mode 100644 src/main/java/org/springframework/data/keyvalue/core/UncategorizedKeyValueException.java create mode 100644 src/test/java/org/springframework/data/keyvalue/core/KeyValuePersistenceExceptionTranslatorUnitTests.java rename src/test/java/org/springframework/data/keyvalue/repository/{BasicKeyValueRepositoryUnitTests.java => SimpleKeyValueRepositoryUnitTests.java} (78%) diff --git a/src/main/java/org/springframework/data/keyvalue/core/KeyValuePersistenceExceptionTranslator.java b/src/main/java/org/springframework/data/keyvalue/core/KeyValuePersistenceExceptionTranslator.java new file mode 100644 index 000000000..5a5ed663b --- /dev/null +++ b/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; + } +} diff --git a/src/main/java/org/springframework/data/keyvalue/core/KeyValueTemplate.java b/src/main/java/org/springframework/data/keyvalue/core/KeyValueTemplate.java index b0a53bef9..331c27019 100644 --- a/src/main/java/org/springframework/data/keyvalue/core/KeyValueTemplate.java +++ b/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, String> keySpaceCache = new ConcurrentHashMap, String>(); private final MappingContext>, ? 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; + } } diff --git a/src/main/java/org/springframework/data/keyvalue/core/UncategorizedKeyValueException.java b/src/main/java/org/springframework/data/keyvalue/core/UncategorizedKeyValueException.java new file mode 100644 index 000000000..47df71324 --- /dev/null +++ b/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); + } +} diff --git a/src/main/java/org/springframework/data/keyvalue/repository/support/SimpleKeyValueRepository.java b/src/main/java/org/springframework/data/keyvalue/repository/support/SimpleKeyValueRepository.java index cf609e2d5..ac79a9fe4 100644 --- a/src/main/java/org/springframework/data/keyvalue/repository/support/SimpleKeyValueRepository.java +++ b/src/main/java/org/springframework/data/keyvalue/repository/support/SimpleKeyValueRepository.java @@ -72,15 +72,15 @@ public class SimpleKeyValueRepository implements Key @Override public Page findAll(Pageable pageable) { + if (pageable == null) { + List result = findAll(); + return new PageImpl(result, null, result.size()); + } + List 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(content, pageable, this.operations.count(entityInformation.getJavaType())); } diff --git a/src/test/java/org/springframework/data/keyvalue/core/KeyValuePersistenceExceptionTranslatorUnitTests.java b/src/test/java/org/springframework/data/keyvalue/core/KeyValuePersistenceExceptionTranslatorUnitTests.java new file mode 100644 index 000000000..5d5d1a4c8 --- /dev/null +++ b/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()); + } + +} diff --git a/src/test/java/org/springframework/data/keyvalue/core/KeyValueTemplateUnitTests.java b/src/test/java/org/springframework/data/keyvalue/core/KeyValueTemplateUnitTests.java index 8ba65805f..eb2deab8a 100644 --- a/src/test/java/org/springframework/data/keyvalue/core/KeyValueTemplateUnitTests.java +++ b/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; diff --git a/src/test/java/org/springframework/data/keyvalue/repository/BasicKeyValueRepositoryUnitTests.java b/src/test/java/org/springframework/data/keyvalue/repository/SimpleKeyValueRepositoryUnitTests.java similarity index 78% rename from src/test/java/org/springframework/data/keyvalue/repository/BasicKeyValueRepositoryUnitTests.java rename to src/test/java/org/springframework/data/keyvalue/repository/SimpleKeyValueRepositoryUnitTests.java index bec01472c..0c6935bb2 100644 --- a/src/test/java/org/springframework/data/keyvalue/repository/BasicKeyValueRepositoryUnitTests.java +++ b/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 repo; private @Mock KeyValueOperations opsMock; @@ -58,7 +61,10 @@ public class BasicKeyValueRepositoryUnitTests { SimpleKeyValueRepository temp = new SimpleKeyValueRepository(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;