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.

Author: gregturn

src/main/java/org/springframework/data/jpa/repository/query/JpaQueryLookupStrategy.java

@@ -21,13 +21,13 @@
 
 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 @@ protected RepositoryQuery resolveQuery(JpaQueryMethod method, EntityManager em,
 				}
 
 				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 CreateIfNotFoundQueryLookupStrategy(EntityManager em, JpaQueryMethodFacto
 		@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 static QueryLookupStrategy create(EntityManager em, JpaQueryMethodFactory
 				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!");
+		}
+	}
 }

src/test/java/org/springframework/data/jpa/domain/sample/User.java

@@ -32,6 +32,7 @@
  * @author Jens Schauder
  * @author Jeff Sheets
  * @author JyotirmoyVS
+ * @author Greg Turnquist
  */
 @Entity
 @NamedEntityGraphs({ @NamedEntityGraph(name = "User.overview", attributeNodes = { @NamedAttributeNode("roles") }),
@@ -91,7 +92,8 @@
 @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;

src/test/java/org/springframework/data/jpa/repository/query/JpaQueryLookupStrategyUnitTests.java

@@ -33,7 +33,6 @@
 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 @@ void prefersDeclaredQuery() throws Exception {
 		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<User, Integer> {
 
 		@Query("something absurd")
@@ -183,5 +206,8 @@ interface UserRepository extends Repository<User, Integer> {
 
 		@Query(value = "something absurd", name = "my-query-name")
 		User annotatedQueryWithQueryAndQueryName();
+
+		// This is a named query with Sort parameter, which isn't supported
+		List<User> customNamedQuery(String firstname, Sort sort);
 	}
 }