diff --git a/src/main/java/org/springframework/data/jpa/repository/query/AbstractStringBasedJpaQuery.java b/src/main/java/org/springframework/data/jpa/repository/query/AbstractStringBasedJpaQuery.java index b46f2c7e0..145c68cc2 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/AbstractStringBasedJpaQuery.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/AbstractStringBasedJpaQuery.java @@ -19,11 +19,8 @@ import javax.persistence.EntityManager; import javax.persistence.Query; import javax.persistence.Tuple; -import org.springframework.data.repository.query.EvaluationContextProvider; -import org.springframework.data.repository.query.ParameterAccessor; -import org.springframework.data.repository.query.ParametersParameterAccessor; -import org.springframework.data.repository.query.ResultProcessor; -import org.springframework.data.repository.query.ReturnedType; +import org.springframework.data.jpa.repository.query.QueryParameterSetter.ErrorHandling; +import org.springframework.data.repository.query.*; import org.springframework.expression.spel.standard.SpelExpressionParser; import org.springframework.util.Assert; @@ -105,9 +102,11 @@ abstract class AbstractStringBasedJpaQuery extends AbstractJpaQuery { String queryString = countQuery.getQueryString(); EntityManager em = getEntityManager(); - return parameterBinder.get().bind( - getQueryMethod().isNativeQuery() ? em.createNativeQuery(queryString) : em.createQuery(queryString, Long.class), - values); + Query query = getQueryMethod().isNativeQuery() // + ? em.createNativeQuery(queryString) // + : em.createQuery(queryString, Long.class); + + return parameterBinder.get().bind(query, values, ErrorHandling.LENIENT); } /** diff --git a/src/main/java/org/springframework/data/jpa/repository/query/NamedQuery.java b/src/main/java/org/springframework/data/jpa/repository/query/NamedQuery.java index a54142aa3..ee7984288 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/NamedQuery.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/NamedQuery.java @@ -22,6 +22,7 @@ import javax.persistence.TypedQuery; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.data.jpa.provider.QueryExtractor; +import org.springframework.data.jpa.repository.query.QueryParameterSetter.ErrorHandling; import org.springframework.data.repository.query.Parameters; import org.springframework.data.repository.query.QueryCreationException; import org.springframework.data.repository.query.RepositoryQuery; @@ -168,6 +169,6 @@ final class NamedQuery extends AbstractJpaQuery { countQuery = em.createQuery(QueryUtils.createCountQueryFor(queryString, countProjection), Long.class); } - return parameterBinder.get().bind(countQuery, values); + return parameterBinder.get().bind(countQuery, values, ErrorHandling.LENIENT); } } diff --git a/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinder.java b/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinder.java index d59d40ed2..7d7f4dd88 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinder.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinder.java @@ -17,6 +17,7 @@ package org.springframework.data.jpa.repository.query; import javax.persistence.Query; +import org.springframework.data.jpa.repository.query.QueryParameterSetter.ErrorHandling; import org.springframework.data.repository.query.ParametersParameterAccessor; import org.springframework.util.Assert; @@ -52,13 +53,17 @@ public class ParameterBinder { this.parameterSetters = parameterSetters; } - public T bind(T jpaQuery, Object[] values) { + public T bind(T jpaQuery, Object[] values, ErrorHandling errorHandling) { - parameterSetters.forEach(it -> it.setParameter(jpaQuery, values)); + parameterSetters.forEach(it -> it.setParameter(jpaQuery, values, errorHandling)); return jpaQuery; } + public T bind(T jpaQuery, Object[] values) { + return bind(jpaQuery, values, ErrorHandling.STRICT); + } + /** * Binds the parameters to the given query and applies special parameter types (e.g. pagination). * diff --git a/src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetter.java b/src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetter.java index 188db08b0..0ec4e8d9d 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetter.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetter.java @@ -15,6 +15,8 @@ */ package org.springframework.data.jpa.repository.query; +import static org.springframework.data.jpa.repository.query.QueryParameterSetter.ErrorHandling.LENIENT; + import java.util.Date; import java.util.function.Function; @@ -23,6 +25,8 @@ import javax.persistence.Query; import javax.persistence.TemporalType; import javax.persistence.criteria.ParameterExpression; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.lang.Nullable; import org.springframework.util.Assert; @@ -36,10 +40,10 @@ import org.springframework.util.Assert; */ interface QueryParameterSetter { - void setParameter(Query query, Object[] values); + void setParameter(Query query, Object[] values, ErrorHandling errorHandling); /** Noop implementation */ - QueryParameterSetter NOOP = (query, values) -> {}; + QueryParameterSetter NOOP = (query, values, errorHandling) -> {}; /** * {@link QueryParameterSetter} for named or indexed parameters that might have a {@link TemporalType} specified. @@ -70,7 +74,7 @@ interface QueryParameterSetter { * @see org.springframework.data.jpa.repository.query.QueryParameterSetter#setParameter(javax.persistence.Query, java.lang.Object[]) */ @SuppressWarnings("unchecked") - public void setParameter(Query query, Object[] values) { + public void setParameter(Query query, Object[] values, ErrorHandling errorHandling) { Object value = valueExtractor.apply(values); @@ -82,24 +86,33 @@ interface QueryParameterSetter { // fixed. if (parameter instanceof ParameterExpression) { - query.setParameter((Parameter) parameter, (Date) value, temporalType); + errorHandling.execute(() -> query.setParameter((Parameter) parameter, (Date) value, temporalType)); } else if (parameter.getName() != null && QueryUtils.hasNamedParameter(query)) { - query.setParameter(parameter.getName(), (Date) value, temporalType); + errorHandling.execute(() -> query.setParameter(parameter.getName(), (Date) value, temporalType)); } else { - if (query.getParameters().size() >= parameter.getPosition() || registerExcessParameters(query)) { - query.setParameter(parameter.getPosition(), (Date) value, temporalType); + + Integer position = parameter.getPosition(); + if (position != null && (query.getParameters().size() >= parameter.getPosition() + || registerExcessParameters(query) || errorHandling == LENIENT)) { + + errorHandling.execute(() -> query.setParameter(parameter.getPosition(), (Date) value, temporalType)); } } } else { if (parameter instanceof ParameterExpression) { - query.setParameter((Parameter) parameter, value); + errorHandling.execute(() -> query.setParameter((Parameter) parameter, value)); } else if (parameter.getName() != null && QueryUtils.hasNamedParameter(query)) { - query.setParameter(parameter.getName(), value); + errorHandling.execute(() -> query.setParameter(parameter.getName(), value)); + } else { - if (query.getParameters().size() >= parameter.getPosition() || registerExcessParameters(query)) { - query.setParameter(parameter.getPosition(), value); + + Integer position = parameter.getPosition(); + if (position != null && (query.getParameters().size() >= position || errorHandling == LENIENT + || registerExcessParameters(query))) { + + errorHandling.execute(() -> query.setParameter(position, value)); } } } @@ -117,4 +130,31 @@ interface QueryParameterSetter { return query.getParameters().size() == 0 && query.getClass().getName().startsWith("org.eclipse"); } } + + enum ErrorHandling { + + STRICT { + + @Override + public void execute(Runnable block) { + block.run(); + } + }, + + LENIENT { + @Override + public void execute(Runnable block) { + + try { + block.run(); + } catch (RuntimeException rex) { + LOG.info("Silently ignoring", rex); + } + } + }; + + private static final Logger LOG = LoggerFactory.getLogger(ErrorHandling.class); + + abstract void execute(Runnable block); + } } diff --git a/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java b/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java index 1170d383b..6bbfc6047 100644 --- a/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java @@ -15,21 +15,17 @@ */ package org.springframework.data.jpa.repository; -import static org.assertj.core.api.Assertions.*; -import static org.springframework.data.domain.Example.*; -import static org.springframework.data.domain.ExampleMatcher.*; -import static org.springframework.data.domain.Sort.Direction.*; -import static org.springframework.data.jpa.domain.Specification.*; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.springframework.data.domain.Example.of; +import static org.springframework.data.domain.ExampleMatcher.matching; +import static org.springframework.data.domain.Sort.Direction.ASC; +import static org.springframework.data.domain.Sort.Direction.DESC; import static org.springframework.data.jpa.domain.Specification.not; +import static org.springframework.data.jpa.domain.Specification.where; import static org.springframework.data.jpa.domain.sample.UserSpecifications.*; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; -import java.util.Collections; -import java.util.HashSet; -import java.util.List; -import java.util.Set; +import java.util.*; import java.util.stream.Stream; import javax.persistence.EntityManager; @@ -48,14 +44,9 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.dao.DataAccessException; import org.springframework.dao.IncorrectResultSizeDataAccessException; import org.springframework.dao.InvalidDataAccessApiUsageException; -import org.springframework.data.domain.Example; -import org.springframework.data.domain.ExampleMatcher; -import org.springframework.data.domain.Page; -import org.springframework.data.domain.PageImpl; -import org.springframework.data.domain.PageRequest; -import org.springframework.data.domain.Pageable; -import org.springframework.data.domain.Slice; -import org.springframework.data.domain.Sort; +import org.springframework.data.domain.*; +import org.springframework.data.domain.ExampleMatcher.GenericPropertyMatcher; +import org.springframework.data.domain.ExampleMatcher.StringMatcher; import org.springframework.data.domain.Sort.Direction; import org.springframework.data.domain.Sort.Order; import org.springframework.data.domain.ExampleMatcher.*; @@ -2092,6 +2083,21 @@ public class UserRepositoryTests { assertThat(users).extracting(User::getId).containsExactly(expected.getId()); } + @Test // DATAJPA-1233 + public void handlesCountQueriesWithLessParametersSingleParam() { + repository.findAllOrderedBySpecialNameSingleParam("Oliver", PageRequest.of(2, 3)); + } + + @Test // DATAJPA-1233 + public void handlesCountQueriesWithLessParametersMoreThanOne() { + repository.findAllOrderedBySpecialNameMultipleParams("Oliver", "x", PageRequest.of(2, 3)); + } + + @Test // DATAJPA-1233 + public void handlesCountQueriesWithLessParametersMoreThanOneIndexed() { + repository.findAllOrderedBySpecialNameMultipleParamsIndexed("Oliver", "x", PageRequest.of(2, 3)); + } + private Page executeSpecWithSort(Sort sort) { flushTestUsers(); diff --git a/src/test/java/org/springframework/data/jpa/repository/query/NamedOrIndexedQueryParameterSetterUnitTests.java b/src/test/java/org/springframework/data/jpa/repository/query/NamedOrIndexedQueryParameterSetterUnitTests.java new file mode 100644 index 000000000..9c4a6cb30 --- /dev/null +++ b/src/test/java/org/springframework/data/jpa/repository/query/NamedOrIndexedQueryParameterSetterUnitTests.java @@ -0,0 +1,216 @@ +/* + * Copyright 2017 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.jpa.repository.query; + +import static java.util.Arrays.asList; +import static javax.persistence.TemporalType.TIME; +import static org.mockito.Mockito.*; +import static org.springframework.data.jpa.repository.query.QueryParameterSetter.ErrorHandling.LENIENT; +import static org.springframework.data.jpa.repository.query.QueryParameterSetter.ErrorHandling.STRICT; + +import lombok.RequiredArgsConstructor; + +import java.util.Collections; +import java.util.Date; +import java.util.List; +import java.util.function.Function; + +import javax.persistence.Parameter; +import javax.persistence.Query; +import javax.persistence.TemporalType; +import javax.persistence.criteria.ParameterExpression; + +import org.assertj.core.api.SoftAssertions; +import org.junit.Test; +import org.springframework.data.jpa.repository.query.QueryParameterSetter.NamedOrIndexedQueryParameterSetter; + +/** + * @author Jens Schauder + */ +public class NamedOrIndexedQueryParameterSetterUnitTests { + + static final String EXCEPTION_MESSAGE = "mock exception"; + Function firstValueExtractor = args -> args[0]; + Object[] methodArguments = { new Date() }; + + List temporalTypes = asList(null, TIME); + List parameters = asList( // + mock(ParameterExpression.class), // + new ParameterImpl("name", null), // + new ParameterImpl(null, 1) // + ); + + SoftAssertions softly = new SoftAssertions(); + + @Test // DATAJPA-1233 + public void strictErrorHandlingThrowsExceptionForAllVariationsOfParameters() { + + Query query = mockExceptionThrowingQueryWithNamedParameters(); + + for (Parameter parameter : parameters) { + for (TemporalType temporalType : temporalTypes) { + + NamedOrIndexedQueryParameterSetter setter = new NamedOrIndexedQueryParameterSetter( // + firstValueExtractor, // + parameter, // + temporalType // + ); + + softly.assertThatThrownBy(() -> setter.setParameter(query, methodArguments, STRICT)) // + .describedAs("p-type: %s, p-name: %s, p-position: %s, temporal: %s", // + parameter.getClass(), // + parameter.getName(), // + parameter.getPosition(), // + temporalType) // + .hasMessage(EXCEPTION_MESSAGE); + } + } + + softly.assertAll(); + } + + @Test // DATAJPA-1233 + public void lenientErrorHandlingThrowsNoExceptionForAllVariationsOfParameters() { + + Query query = mockExceptionThrowingQueryWithNamedParameters(); + + for (Parameter parameter : parameters) { + for (TemporalType temporalType : temporalTypes) { + + NamedOrIndexedQueryParameterSetter setter = new NamedOrIndexedQueryParameterSetter( // + firstValueExtractor, // + parameter, // + temporalType // + ); + + softly.assertThatCode(() -> setter.setParameter(query, methodArguments, LENIENT)) // + .describedAs("p-type: %s, p-name: %s, p-position: %s, temporal: %s", // + parameter.getClass(), // + parameter.getName(), // + parameter.getPosition(), // + temporalType) // + .doesNotThrowAnyException(); + } + } + + softly.assertAll(); + } + + /** + * setParameter should be called in the lenient case even if the number of parameters seems to suggest that it fails, + * since the index might not be continuous due to missing parts of count queries compared to the main query. + * + * This happens when a parameter gets used in the ORDER BY clause which gets stripped of for the count query. + */ + @Test // DATAJPA-1233 + public void lenientSetsParameterWhenSuccessIsUnsure() { + + Query query = mock(Query.class); + + for (TemporalType temporalType : temporalTypes) { + + NamedOrIndexedQueryParameterSetter setter = new NamedOrIndexedQueryParameterSetter( // + firstValueExtractor, // + new ParameterImpl(null, 11), // parameter position is beyond number of parametes in query (0) + temporalType // + ); + + setter.setParameter(query, methodArguments, LENIENT); + + if (temporalType == null) + verify(query).setParameter(eq(11), any(Date.class)); + else + verify(query).setParameter(eq(11), any(Date.class), eq(temporalType)); + } + + softly.assertAll(); + + } + + /** + * This scenario happens when the only (name) parameter is part of an ORDER BY clause and gets stripped of for the count query. + * + * Then the count query has no named parameter but the parameter provided has a {@literal null} position. + */ + @Test // DATAJPA-1233 + public void parameterNotSetWhenSuccessImpossible() { + + Query query = mock(Query.class); + + for (TemporalType temporalType : temporalTypes) { + + NamedOrIndexedQueryParameterSetter setter = new NamedOrIndexedQueryParameterSetter( // + firstValueExtractor, // + new ParameterImpl(null, null), // no position (and no name) makes a success of a setParameter impossible + temporalType // + ); + + setter.setParameter(query, methodArguments, LENIENT); + + if (temporalType == null) + verify(query, never()).setParameter(anyInt(), any(Date.class)); + else + verify(query, never()).setParameter(anyInt(), any(Date.class), eq(temporalType)); + } + + softly.assertAll(); + + } + + @SuppressWarnings("unchecked") + public Query mockExceptionThrowingQueryWithNamedParameters() { + Query query = mock(Query.class); + + // make it a query with named parameters + doReturn(Collections.singleton(new ParameterImpl("aName", 3))).when(query).getParameters(); + doThrow(new RuntimeException(EXCEPTION_MESSAGE)).when(query) // + .setParameter(any(Parameter.class), any(Date.class), any(TemporalType.class)); + doThrow(new RuntimeException(EXCEPTION_MESSAGE)).when(query) // + .setParameter(any(Parameter.class), any(Date.class)); + doThrow(new RuntimeException(EXCEPTION_MESSAGE)).when(query) // + .setParameter(anyString(), any(Date.class), any(TemporalType.class)); + doThrow(new RuntimeException(EXCEPTION_MESSAGE)).when(query) // + .setParameter(anyString(), any(Date.class)); + doThrow(new RuntimeException(EXCEPTION_MESSAGE)).when(query) // + .setParameter(anyInt(), any(Date.class), any(TemporalType.class)); + doThrow(new RuntimeException(EXCEPTION_MESSAGE)).when(query) // + .setParameter(anyInt(), any(Date.class)); + return query; + } + + @RequiredArgsConstructor + private static class ParameterImpl implements Parameter { + + private final String name; + private final Integer position; + + @Override + public String getName() { + return name; + } + + @Override + public Integer getPosition() { + return position; + } + + @Override + public Class getParameterType() { + return Object.class; + } + } + +} diff --git a/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java b/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java index 48c8a6e88..a714aa5ff 100644 --- a/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java +++ b/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java @@ -517,6 +517,18 @@ public interface UserRepository @Query("SELECT u FROM User u where u.firstname >= ?1 and u.lastname = '000:1'") List queryWithIndexedParameterAndColonFollowedByIntegerInString(String firstname); + // DATAJPA-1233 + @Query(value = "SELECT u FROM User u ORDER BY CASE WHEN (u.firstname >= :name) THEN 0 ELSE 1 END, u.firstname") + Page findAllOrderedBySpecialNameSingleParam(@Param("name") String name, Pageable page); + + // DATAJPA-1233 + @Query(value = "SELECT u FROM User u WHERE :other = 'x' ORDER BY CASE WHEN (u.firstname >= :name) THEN 0 ELSE 1 END, u.firstname") + Page findAllOrderedBySpecialNameMultipleParams(@Param("name") String name, @Param("other") String other, Pageable page); + + // DATAJPA-1233 + @Query(value = "SELECT u FROM User u WHERE ?2 = 'x' ORDER BY CASE WHEN (u.firstname >= ?1) THEN 0 ELSE 1 END, u.firstname") + Page findAllOrderedBySpecialNameMultipleParamsIndexed(String name, String other, Pageable page); + interface RolesAndFirstname { String getFirstname();