From 2d3633e2fa9f9756c19279b230d18eb89c734a88 Mon Sep 17 00:00:00 2001 From: "Greg L. Turnquist" Date: Wed, 18 May 2022 09:01:07 -0500 Subject: [PATCH] JpaQueryLookupStrategy shouldn't use exceptions for flow control. By using exceptions for flow control, other critical exceptions are getting masked. The lack of a resolvable query should instead leverage some sort of null value object. See #2018. --- .../query/JpaQueryLookupStrategy.java | 50 +++++++++++++------ .../data/jpa/domain/sample/User.java | 4 +- .../JpaQueryLookupStrategyUnitTests.java | 28 ++++++++++- 3 files changed, 65 insertions(+), 17 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 7c25b7953..d2261ba86 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 @@ -21,13 +21,13 @@ import javax.persistence.EntityManager; import org.slf4j.Logger; import org.slf4j.LoggerFactory; - import org.springframework.data.jpa.repository.Query; import org.springframework.data.projection.ProjectionFactory; import org.springframework.data.repository.core.NamedQueries; import org.springframework.data.repository.core.RepositoryMetadata; import org.springframework.data.repository.query.QueryLookupStrategy; import org.springframework.data.repository.query.QueryLookupStrategy.Key; +import org.springframework.data.repository.query.QueryMethod; import org.springframework.data.repository.query.QueryMethodEvaluationContextProvider; import org.springframework.data.repository.query.RepositoryQuery; import org.springframework.lang.Nullable; @@ -46,6 +46,12 @@ public final class JpaQueryLookupStrategy { private static final Logger LOG = LoggerFactory.getLogger(JpaQueryLookupStrategy.class); + /** + * A null-value instance used to signal if no declared query could be found. It checks many different formats before + * falling through to this value object. + */ + private static final RepositoryQuery NO_QUERY = new NoQuery(); + /** * Private constructor to prevent instantiation. */ @@ -161,24 +167,20 @@ public final class JpaQueryLookupStrategy { } return JpaQueryFactory.INSTANCE.fromMethodWithQueryString(method, em, method.getRequiredAnnotatedQuery(), - getCountQuery(method, namedQueries, em), - evaluationContextProvider); + getCountQuery(method, namedQueries, em), evaluationContextProvider); } String name = method.getNamedQueryName(); if (namedQueries.hasQuery(name)) { - return JpaQueryFactory.INSTANCE.fromMethodWithQueryString(method, em, namedQueries.getQuery(name), getCountQuery(method, namedQueries, em), - evaluationContextProvider); + return JpaQueryFactory.INSTANCE.fromMethodWithQueryString(method, em, namedQueries.getQuery(name), + getCountQuery(method, namedQueries, em), evaluationContextProvider); } RepositoryQuery query = NamedQuery.lookupFrom(method, em); - if (null != query) { - return query; - } - - throw new IllegalStateException( - String.format("Did neither find a NamedQuery nor an annotated query for method %s!", method)); + return query != null // + ? query // + : NO_QUERY; } @Nullable @@ -248,11 +250,13 @@ public final class JpaQueryLookupStrategy { @Override protected RepositoryQuery resolveQuery(JpaQueryMethod method, EntityManager em, NamedQueries namedQueries) { - try { - return lookupStrategy.resolveQuery(method, em, namedQueries); - } catch (IllegalStateException e) { - return createStrategy.resolveQuery(method, em, namedQueries); + RepositoryQuery lookupQuery = lookupStrategy.resolveQuery(method, em, namedQueries); + + if (lookupQuery != NO_QUERY) { + return lookupQuery; } + + return createStrategy.resolveQuery(method, em, namedQueries); } } @@ -286,4 +290,20 @@ public final class JpaQueryLookupStrategy { throw new IllegalArgumentException(String.format("Unsupported query lookup strategy %s!", key)); } } + + /** + * A null value type that represents the lack of a defined query. + */ + static class NoQuery implements RepositoryQuery { + + @Override + public Object execute(Object[] parameters) { + throw new IllegalStateException("NoQuery should not be executed!"); + } + + @Override + public QueryMethod getQueryMethod() { + throw new IllegalStateException("NoQuery does not have a QueryMethod!"); + } + } } diff --git a/src/test/java/org/springframework/data/jpa/domain/sample/User.java b/src/test/java/org/springframework/data/jpa/domain/sample/User.java index 0a635af6c..8cc55d2de 100644 --- a/src/test/java/org/springframework/data/jpa/domain/sample/User.java +++ b/src/test/java/org/springframework/data/jpa/domain/sample/User.java @@ -32,6 +32,7 @@ import javax.persistence.*; * @author Jens Schauder * @author Jeff Sheets * @author JyotirmoyVS + * @author Greg Turnquist */ @Entity @NamedEntityGraphs({ @NamedEntityGraph(name = "User.overview", attributeNodes = { @NamedAttributeNode("roles") }), @@ -91,7 +92,8 @@ import javax.persistence.*; @Table(name = "SD_User") public class User { - @Id @GeneratedValue(strategy = GenerationType.AUTO) private Integer id; + @Id + @GeneratedValue(strategy = GenerationType.AUTO) private Integer id; private String firstname; private String lastname; private int age; diff --git a/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryLookupStrategyUnitTests.java b/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryLookupStrategyUnitTests.java index 6f026b355..aeae43261 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryLookupStrategyUnitTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryLookupStrategyUnitTests.java @@ -33,7 +33,6 @@ import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.mockito.junit.jupiter.MockitoSettings; import org.mockito.quality.Strictness; - import org.springframework.data.domain.Page; import org.springframework.data.domain.Pageable; import org.springframework.data.domain.Sort; @@ -167,6 +166,30 @@ public class JpaQueryLookupStrategyUnitTests { assertThat(repositoryQuery).isInstanceOf(AbstractStringBasedJpaQuery.class); } + @Test // GH-2018 + void namedQueryWithSortShouldThrowIllegalStateException() throws NoSuchMethodException { + + QueryLookupStrategy strategy = JpaQueryLookupStrategy.create(em, queryMethodFactory, Key.CREATE_IF_NOT_FOUND, + EVALUATION_CONTEXT_PROVIDER, EscapeCharacter.DEFAULT); + + Method method = UserRepository.class.getMethod("customNamedQuery", String.class, Sort.class); + RepositoryMetadata metadata = new DefaultRepositoryMetadata(UserRepository.class); + + assertThatIllegalStateException() + .isThrownBy(() -> strategy.resolveQuery(method, metadata, projectionFactory, namedQueries)) + .withMessageContaining( + "is backed by a NamedQuery and must not contain a sort parameter as we cannot modify the query! Use @Query instead!"); + } + + @Test // GH-2018 + void noQueryShouldNotBeInvoked() { + + RepositoryQuery query = new JpaQueryLookupStrategy.NoQuery(); + + assertThatIllegalStateException().isThrownBy(() -> query.execute(new Object[] {})); + assertThatIllegalStateException().isThrownBy(() -> query.getQueryMethod()); + } + interface UserRepository extends Repository { @Query("something absurd") @@ -183,5 +206,8 @@ public class JpaQueryLookupStrategyUnitTests { @Query(value = "something absurd", name = "my-query-name") User annotatedQueryWithQueryAndQueryName(); + + // This is a named query with Sort parameter, which isn't supported + List customNamedQuery(String firstname, Sort sort); } }