Spring/spring-data-jpa
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

DATAJPA-1233 - Parameter setting for count query is no more lenient.

Browse Source 
For count queries parameters get set even when it might fail.
Exceptions during parameter setting for count queries get ignored.
This commit is contained in:
Jens Schauder
2017-12-04 14:51:54 +01:00
committed by Oliver Gierke
parent d131c957c9
commit d016d2e06a
7 changed files with 321 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
src/main/java/org/springframework/data/jpa/repository/query/AbstractStringBasedJpaQuery.java
View File

@@ -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);
}
/**

3
src/main/java/org/springframework/data/jpa/repository/query/NamedQuery.java
View File

@@ -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);
}
}

9
src/main/java/org/springframework/data/jpa/repository/query/ParameterBinder.java
View File

@@ -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 extends Query> T bind(T jpaQuery, Object[] values) {
public <T extends Query> 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 extends Query> 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).
*

62
src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetter.java
View File

@@ -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<Date>) parameter, (Date) value, temporalType);
errorHandling.execute(() -> query.setParameter((Parameter<Date>) 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<Object>) parameter, value);
errorHandling.execute(() -> query.setParameter((Parameter<Object>) 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);
}
}

46
src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java
View File

@@ -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<User> executeSpecWithSort(Sort sort) {
flushTestUsers();

216
src/test/java/org/springframework/data/jpa/repository/query/NamedOrIndexedQueryParameterSetterUnitTests.java Normal file
View File

@@ -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<Object[], Object> firstValueExtractor = args -> args[0];
Object[] methodArguments = { new Date() };
List<TemporalType> temporalTypes = asList(null, TIME);
List<Parameter> 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<Object> {
private final String name;
private final Integer position;
@Override
public String getName() {
return name;
}
@Override
public Integer getPosition() {
return position;
}
@Override
public Class<Object> getParameterType() {
return Object.class;
}
}
}

12
src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java
View File

@@ -517,6 +517,18 @@ public interface UserRepository
@Query("SELECT u FROM User u where u.firstname >= ?1 and u.lastname = '000:1'")
List<User> 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<User> 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<User> 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<User> findAllOrderedBySpecialNameMultipleParamsIndexed(String name, String other, Pageable page);
interface RolesAndFirstname {
String getFirstname();