provide some tests to verify behaviour

Author: christophstrobl

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/AbstractStringBasedJpaQuery.java

@@ -93,13 +93,11 @@ public AbstractStringBasedJpaQuery(JpaQueryMethod method, EntityManager em, Stri
 	public Query doCreateQuery(JpaParametersParameterAccessor accessor) {
 
 		Sort sort = accessor.getSort();
-		String sortedQueryString = sort.isSorted() ? QueryEnhancerFactory.forQuery(query) //
-				.applySorting(sort, query.getAlias()) : query.getQueryString();
+		String sortedQueryString = applySortingIfNecessary(query, sort);
 
 		ResultProcessor processor = getQueryMethod().getResultProcessor().withDynamicProjection(accessor);
 
-		Query query = createJpaQuery(sortedQueryString, accessor.getSort(), accessor.getPageable(),
-				processor.getReturnedType());
+		Query query = createJpaQuery(sortedQueryString, sort, accessor.getPageable(), processor.getReturnedType());
 
 		QueryParameterSetter.QueryMetadata metadata = metadataCache.getMetadata(sortedQueryString, query);
 
@@ -108,6 +106,18 @@ public Query doCreateQuery(JpaParametersParameterAccessor accessor) {
 		return parameterBinder.get().bindAndPrepare(query, metadata, accessor);
 	}
 
+	private String applySortingIfNecessary(DeclaredQuery query, Sort sort) {
+
+		if (sort.isUnsorted()) {
+			return query.getQueryString();
+		}
+		return applySorting(query, sort);
+	}
+
+	protected String applySorting(DeclaredQuery query, Sort sort) {
+		return QueryEnhancerFactory.forQuery(query).applySorting(sort, query.getAlias());
+	}
+
 	@Override
 	protected ParameterBinder createBinder() {
 

spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/AbstractStringBasedJpaQueryUnitTests.java

@@ -0,0 +1,215 @@
+/*
+ * Copyright 2024 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
+ *
+ *      https://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.jpa.repository.query;
+
+import static org.mockito.Mockito.*;
+
+import jakarta.persistence.EntityManager;
+import jakarta.persistence.metamodel.Metamodel;
+
+import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.function.Supplier;
+
+import org.assertj.core.api.Assertions;
+import org.assertj.core.util.Arrays;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+import org.springframework.core.annotation.AnnotatedElementUtils;
+import org.springframework.data.domain.Pageable;
+import org.springframework.data.domain.Sort;
+import org.springframework.data.jpa.provider.QueryExtractor;
+import org.springframework.data.jpa.repository.Query;
+import org.springframework.data.jpa.repository.QueryRewriter;
+import org.springframework.data.projection.SpelAwareProxyProjectionFactory;
+import org.springframework.data.repository.Repository;
+import org.springframework.data.repository.core.RepositoryMetadata;
+import org.springframework.data.repository.core.support.DefaultRepositoryMetadata;
+import org.springframework.data.repository.query.ParametersSource;
+import org.springframework.data.repository.query.QueryMethodEvaluationContextProvider;
+import org.springframework.data.repository.query.ReturnedType;
+import org.springframework.expression.spel.standard.SpelExpressionParser;
+import org.springframework.lang.Nullable;
+import org.springframework.util.LinkedMultiValueMap;
+import org.springframework.util.MultiValueMap;
+import org.springframework.util.ReflectionUtils;
+
+/**
+ * @author Christoph Strobl
+ */
+public class AbstractStringBasedJpaQueryUnitTests {
+
+	@Test // GH-3310
+	void shouldNotAttemptToAppendSortIfNoSortArgumentPresent() {
+
+		InvocationCapturingStringQueryStub stringQuery = forMethod(TestRepo.class, "find");
+		stringQuery.createQueryWithArguments();
+
+		stringQuery.neverCalled("applySorting");
+	}
+
+	@Test // GH-3310
+	void shouldNotAttemptToAppendSortIfSortIndicatesUnsorted() {
+
+		InvocationCapturingStringQueryStub stringQuery = forMethod(TestRepo.class, "find", Sort.class);
+		stringQuery.createQueryWithArguments(Sort.unsorted());
+
+		stringQuery.neverCalled("applySorting");
+	}
+
+	@Test // GH-3310
+	void shouldAppendSortIfSortPresent() {
+
+		InvocationCapturingStringQueryStub stringQuery = forMethod(TestRepo.class, "find", Sort.class);
+		stringQuery.createQueryWithArguments(Sort.by("name"));
+
+		stringQuery.called("applySorting").times(1);
+	}
+
+	interface TestRepo extends Repository<Object, Object> {
+
+		@Query("SELECT e FROM Employee e")
+		Object find();
+
+		@Query("SELECT e FROM Employee e")
+		Object find(Sort sort);
+	}
+
+	static InvocationCapturingStringQueryStub forMethod(Class<?> repository, String method, Class<?>... args) {
+
+		Method respositoryMethod = ReflectionUtils.findMethod(repository, method, args);
+		RepositoryMetadata repositoryMetadata = new DefaultRepositoryMetadata(repository);
+		SpelAwareProxyProjectionFactory projectionFactory = Mockito.mock(SpelAwareProxyProjectionFactory.class);
+		QueryExtractor queryExtractor = Mockito.mock(QueryExtractor.class);
+		JpaQueryMethod queryMethod = new JpaQueryMethod(respositoryMethod, repositoryMetadata, projectionFactory,
+				queryExtractor);
+
+		Query query = AnnotatedElementUtils.getMergedAnnotation(respositoryMethod, Query.class);
+
+		return new InvocationCapturingStringQueryStub(respositoryMethod, queryMethod, query.value(), query.countQuery(),
+				new SpelExpressionParser());
+
+	}
+
+	static class InvocationCapturingStringQueryStub extends AbstractStringBasedJpaQuery {
+
+		private final Method targetMethod;
+		private final MultiValueMap<String, Arguments> capturedArguments = new LinkedMultiValueMap<>(3);
+
+		InvocationCapturingStringQueryStub(Method targetMethod, JpaQueryMethod queryMethod, String queryString,
+				@Nullable String countQueryString, SpelExpressionParser parser) {
+			super(queryMethod, new Supplier<EntityManager>() {
+
+				@Override
+				public EntityManager get() {
+
+					EntityManager em = Mockito.mock(EntityManager.class);
+
+					Metamodel meta = mock(Metamodel.class);
+					when(em.getMetamodel()).thenReturn(meta);
+					when(em.getDelegate()).thenReturn(new Object()); // some generic jpa
+
+					return em;
+				}
+			}.get(), queryString, countQueryString, Mockito.mock(QueryRewriter.class),
+					Mockito.mock(QueryMethodEvaluationContextProvider.class), parser);
+
+			this.targetMethod = targetMethod;
+		}
+
+		@Override
+		protected String applySorting(DeclaredQuery query, Sort sort) {
+
+			captureInvocation("applySorting", query, sort);
+
+			return super.applySorting(query, sort);
+		}
+
+		@Override
+		protected jakarta.persistence.Query createJpaQuery(String queryString, Sort sort, @Nullable Pageable pageable,
+				ReturnedType returnedType) {
+
+			captureInvocation("createJpaQuery", queryString, sort, pageable, returnedType);
+
+			jakarta.persistence.Query jpaQuery = super.createJpaQuery(queryString, sort, pageable, returnedType);
+			return jpaQuery == null ? Mockito.mock(jakarta.persistence.Query.class) : jpaQuery;
+		}
+
+		// --> convenience for tests
+
+		JpaParameters getParameters() {
+			return new JpaParameters(ParametersSource.of(targetMethod));
+		}
+
+		JpaParametersParameterAccessor getParameterAccessor(Object... args) {
+			return new JpaParametersParameterAccessor(getParameters(), args);
+		}
+
+		jakarta.persistence.Query createQueryWithArguments(Object... args) {
+			return doCreateQuery(getParameterAccessor(args));
+		}
+
+		// --> capturing methods
+
+		private void captureInvocation(String key, Object... args) {
+			capturedArguments.add(key, new Arguments(args));
+		}
+
+		// --> verification methdos
+
+		int getInvocationCount(String method) {
+
+			List<Arguments> invocations = capturedArguments.get(method);
+			return invocations != null ? invocations.size() : 0;
+		}
+
+		public void neverCalled(String method) {
+			called(method).never();
+		}
+
+		public Times called(String method) {
+
+			return (invocationCount -> {
+
+				int actualCount = getInvocationCount(method);
+				Assertions.assertThat(actualCount)
+						.withFailMessage(
+								() -> "Expected %d invocations for %s, but recorded %d".formatted(invocationCount, method, actualCount))
+						.isEqualTo(invocationCount);
+			});
+		}
+
+		static class Arguments {
+
+			List<Object> values = new ArrayList<>(3);
+
+			public Arguments(Object... values) {
+				this.values = Arrays.asList(values);
+			}
+		}
+
+		interface Times {
+
+			void times(int invocationCount);
+
+			default void never() {
+				times(0);
+			}
+		}
+
+	}
+}