From eb284de5a3b222ed3a452688c3dfe58532341f58 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Fri, 24 Nov 2017 13:49:42 +0100 Subject: [PATCH] DATAJPA-863 - Polishing. The Iterator in ParameterMetadataProvider now exposes which Part we tried to lookup a value for to report it in case no parameter value is available for anymore. Switched to IllegalArgumentException to consistently produce the same exceptions. Removed wrapping of IllegalArgumentExceptions in CreateQueryLookupStrategy as PartTreeJpaQuery now produces IllegalArgumentExceptions in the first place. Original pull request: #232. --- .../query/JpaQueryLookupStrategy.java | 9 +----- .../query/ParameterMetadataProvider.java | 28 ++++++++++++------- .../repository/query/PartTreeJpaQuery.java | 8 ++++-- .../PartTreeJpaQueryIntegrationTests.java | 22 +++++++-------- 4 files changed, 35 insertions(+), 32 deletions(-) diff --git a/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryLookupStrategy.java b/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryLookupStrategy.java index ae652ee9b..0ef0d7a79 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryLookupStrategy.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/JpaQueryLookupStrategy.java @@ -100,15 +100,8 @@ public final class JpaQueryLookupStrategy { @Override protected RepositoryQuery resolveQuery(JpaQueryMethod method, EntityManager em, NamedQueries namedQueries) { - - try { - return new PartTreeJpaQuery(method, em, persistenceProvider); - } catch (IllegalArgumentException e) { - throw new IllegalArgumentException( - String.format("Could not create query metamodel for method %s!", method.toString()), e); - } + return new PartTreeJpaQuery(method, em, persistenceProvider); } - } /** diff --git a/src/main/java/org/springframework/data/jpa/repository/query/ParameterMetadataProvider.java b/src/main/java/org/springframework/data/jpa/repository/query/ParameterMetadataProvider.java index 45489f471..cf8a93dfd 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/ParameterMetadataProvider.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/ParameterMetadataProvider.java @@ -21,6 +21,7 @@ import java.util.Collection; import java.util.Collections; import java.util.Iterator; import java.util.List; +import java.util.function.Supplier; import javax.persistence.criteria.CriteriaBuilder; import javax.persistence.criteria.ParameterExpression; @@ -119,7 +120,8 @@ class ParameterMetadataProvider { @SuppressWarnings("unchecked") public ParameterMetadata next(Part part) { - Assert.isTrue(parameters.hasNext(), "No more parameters available."); + Assert.isTrue(parameters.hasNext(), String.format("No parameter available for part %s.", part)); + Parameter parameter = parameters.next(); return (ParameterMetadata) next(part, parameter.getType(), parameter); } @@ -159,14 +161,19 @@ class ParameterMetadataProvider { @SuppressWarnings("unchecked") Class reifiedType = Expression.class.equals(type) ? (Class) Object.class : type; - ParameterExpression expression = parameter.isExplicitlyNamed() - ? builder.parameter(reifiedType, parameter.getName().orElseThrow(() -> new IllegalArgumentException("o_O Parameter needs to be named"))) : builder.parameter(reifiedType); - ParameterMetadata value = new ParameterMetadata<>(expression, part.getType(), - bindableParameterValues == null ? ParameterMetadata.PLACEHOLDER : bindableParameterValues.next(), - this.persistenceProvider); - expressions.add(value); + Supplier name = () -> parameter.getName() + .orElseThrow(() -> new IllegalArgumentException("o_O Parameter needs to be named")); - return value; + ParameterExpression expression = parameter.isExplicitlyNamed() // + ? builder.parameter(reifiedType, name.get()) // + : builder.parameter(reifiedType); + + Object value = bindableParameterValues == null ? ParameterMetadata.PLACEHOLDER : bindableParameterValues.next(); + + ParameterMetadata metadata = new ParameterMetadata<>(expression, part.getType(), value, persistenceProvider); + expressions.add(metadata); + + return metadata; } /** @@ -236,8 +243,9 @@ class ParameterMetadataProvider { } } - return Collection.class.isAssignableFrom(expressionType) - ? persistenceProvider.potentiallyConvertEmptyCollection(toCollection(value)) : value; + return Collection.class.isAssignableFrom(expressionType) // + ? persistenceProvider.potentiallyConvertEmptyCollection(toCollection(value)) // + : value; } /** diff --git a/src/main/java/org/springframework/data/jpa/repository/query/PartTreeJpaQuery.java b/src/main/java/org/springframework/data/jpa/repository/query/PartTreeJpaQuery.java index 47c11b2a2..ddbb4978b 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/PartTreeJpaQuery.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/PartTreeJpaQuery.java @@ -72,12 +72,14 @@ public class PartTreeJpaQuery extends AbstractJpaQuery { boolean recreationRequired = parameters.hasDynamicProjection() || parameters.potentiallySortsDynamically(); try { + this.tree = new PartTree(method.getName(), domainClass); this.countQuery = new CountQueryPreparer(persistenceProvider, recreationRequired); this.query = tree.isCountProjection() ? countQuery : new QueryPreparer(persistenceProvider, recreationRequired); - } catch (Exception e) { - throw new IllegalStateException( - String.format("Failed to create query for method <%s>: %s", method, e.getMessage()), e); + + } catch (Exception o_O) { + throw new IllegalArgumentException( + String.format("Failed to create query for method %s! %s", method, o_O.getMessage()), o_O); } } diff --git a/src/test/java/org/springframework/data/jpa/repository/query/PartTreeJpaQueryIntegrationTests.java b/src/test/java/org/springframework/data/jpa/repository/query/PartTreeJpaQueryIntegrationTests.java index d19d64d83..42ce5d636 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/PartTreeJpaQueryIntegrationTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/PartTreeJpaQueryIntegrationTests.java @@ -17,8 +17,8 @@ import org.springframework.aop.framework.Advised; */ package org.springframework.data.jpa.repository.query; -import static org.assertj.core.api.Assertions.assertThat; -import static org.springframework.test.util.ReflectionTestUtils.getField; +import static org.assertj.core.api.Assertions.*; +import static org.springframework.test.util.ReflectionTestUtils.*; import java.lang.reflect.Method; import java.util.Arrays; @@ -31,7 +31,6 @@ import javax.persistence.PersistenceContext; import javax.persistence.Query; import javax.persistence.TemporalType; -import org.assertj.core.api.Assertions; import org.hibernate.Version; import org.junit.Before; import org.junit.Rule; @@ -91,7 +90,7 @@ public class PartTreeJpaQueryIntegrationTests { @Test public void cannotIgnoreCaseIfNotString() throws Exception { - thrown.expect(IllegalStateException.class); + thrown.expect(IllegalArgumentException.class); thrown.expectMessage("Unable to ignore case of java.lang.Integer types, the property 'id' must reference a String"); testIgnoreCase("findByIdIgnoringCase", 3); } @@ -161,7 +160,7 @@ public class PartTreeJpaQueryIntegrationTests { assertThat(HibernateUtils.getHibernateQuery(getValue(query, PROPERTY))).endsWith("roles is not empty"); } - @Test(expected = IllegalStateException.class) // DATAJPA-1074 + @Test(expected = IllegalArgumentException.class) // DATAJPA-1074 public void rejectsIsEmptyOnNonCollectionProperty() throws Exception { JpaQueryMethod method = getQueryMethod("findByFirstnameIsEmpty"); @@ -175,10 +174,11 @@ public class PartTreeJpaQueryIntegrationTests { JpaQueryMethod method = getQueryMethod("findByFirstname"); - Assertions.assertThatExceptionOfType(IllegalStateException.class) // + assertThatExceptionOfType(IllegalArgumentException.class) // .isThrownBy(() -> new PartTreeJpaQuery(method, entityManager, provider)) // - .withMessageContaining("findByFirstname") // - .withMessageContaining("UserRepository"); + .withMessageContaining("findByFirstname") // the method being analyzed + .withMessageContaining(" firstname ") // the property we are looking for + .withMessageContaining("UserRepository"); // the repository } @Test // DATAJPA-863 @@ -186,7 +186,7 @@ public class PartTreeJpaQueryIntegrationTests { JpaQueryMethod method = getQueryMethod("findByNoSuchProperty", String.class); - Assertions.assertThatExceptionOfType(IllegalStateException.class) // + assertThatExceptionOfType(IllegalArgumentException.class) // .isThrownBy(() -> new PartTreeJpaQuery(method, entityManager, provider)) // .withMessageContaining("findByNoSuchProperty") // the method being analyzed .withMessageContaining(" noSuchProperty ") // the property we are looking for @@ -214,7 +214,7 @@ public class PartTreeJpaQueryIntegrationTests { } @SuppressWarnings("unchecked") - private static T getValue(Object source, String path) { + private static T getValue(Object source, String path) { Iterator split = Arrays.asList(path.split("\\.")).iterator(); Object result = source; @@ -242,7 +242,7 @@ public class PartTreeJpaQueryIntegrationTests { } @SuppressWarnings("unused") - interface UserRepository extends Repository { + interface UserRepository extends Repository { Page findByFirstname(String firstname, Pageable pageable);