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

DATAJPA-1172 - We do not swallow exceptions from setting parameters.

Browse Source 
Instead of ignoring exceptions, we now only register extra parameters when we are using EclipseLink at the query claims there are no parameters expected. With this any exception thrown are actual exceptions.

This allowed some simplification of the code. Added test to ensure the new behaviour and the presence of the eclipse bug. Added references to the eclipse bug.

See also: https://bugs.eclipse.org/bugs/show_bug.cgi?id=521915
Original pull request: #216.
This commit is contained in:
Jens Schauder
2017-09-06 07:49:34 +02:00
committed by Oliver Gierke
parent 2c54644847
commit e072a56a27
6 changed files with 95 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -49,23 +49,20 @@ interface QueryParameterSetter {
private final Function<Object[], Object> valueExtractor;
private final Parameter<?> parameter;
private final @Nullable TemporalType temporalType;
private final boolean lenient;
/**
* @param valueExtractor must not be {@literal null}.
* @param parameter must not be {@literal null}.
* @param temporalType may be {@literal null}.
* @param lenient must not be {@literal null}.
*/
NamedOrIndexedQueryParameterSetter(Function<Object[], Object> valueExtractor, Parameter<?> parameter,
@Nullable TemporalType temporalType, boolean lenient) {
@Nullable TemporalType temporalType) {
Assert.notNull(valueExtractor, "ValueExtractor must not be null!");
this.valueExtractor = valueExtractor;
this.parameter = parameter;
this.temporalType = temporalType;
this.lenient = lenient;
}
/*
@@ -77,44 +74,47 @@ interface QueryParameterSetter {
Object value = valueExtractor.apply(values);
try {
if (temporalType != null) {
if (temporalType != null) {
// One would think we can simply use parameter to identify the parameter we want to set.
// But that does not work with list valued parameters. At least Hibernate tries to bind them by name.
// TODO: move to using setParameter(Parameter, value) when https://hibernate.atlassian.net/browse/HHH-11870 is
// fixed.
// One would think we can simply use parameter to identify the parameter we want to set.
// But that does not work with list valued parameters. At least Hibernate tries to bind them by name.
// TODO: move to using setParameter(Parameter, value) when https://hibernate.atlassian.net/browse/HHH-11870 is
// fixed.
if (parameter instanceof ParameterExpression) {
query.setParameter((Parameter<Date>) parameter, (Date) value, temporalType);
} else if (parameter.getName() != null && QueryUtils.hasNamedParameter(query)) {
query.setParameter(parameter.getName(), (Date) value, temporalType);
} else {
if (parameter instanceof ParameterExpression) {
query.setParameter((Parameter<Date>) parameter, (Date) value, temporalType);
} else if (parameter.getName() != null && QueryUtils.hasNamedParameter(query)) {
query.setParameter(parameter.getName(), (Date) value, temporalType);
} else {
if (query.getParameters().size() >= parameter.getPosition() || registerExcessParameters(query)) {
query.setParameter(parameter.getPosition(), (Date) value, temporalType);
}
}
} else {
if (parameter instanceof ParameterExpression) {
query.setParameter((Parameter<Object>) parameter, value);
} else if (parameter.getName() != null && QueryUtils.hasNamedParameter(query)) {
query.setParameter(parameter.getName(), value);
} else {
if (parameter instanceof ParameterExpression) {
query.setParameter((Parameter<Object>) parameter, value);
} else if (parameter.getName() != null && QueryUtils.hasNamedParameter(query)) {
query.setParameter(parameter.getName(), value);
} else {
if (query.getParameters().size() >= parameter.getPosition() || registerExcessParameters(query)) {
query.setParameter(parameter.getPosition(), value);
}
}
} catch (IllegalArgumentException o_O) {
if (!lenient) {
throw o_O;
}
// Since EclipseLink doesn't reliably report whether a query has parameters
// we simply try to set the parameters and ignore possible failures.
// this is relevant for queries with SpEL expressions, where the method parameters don't have to match the
// parameters in the query.
}
}
private boolean registerExcessParameters(Query query) {
// DATAJPA-1172
// Since EclipseLink doesn't reliably report whether a query has parameters
// we simply try to set the parameters and ignore possible failures.
// this is relevant for native queries with SpEL expressions, where the method parameters don't have to match the
// parameters in the query.
// https://bugs.eclipse.org/bugs/show_bug.cgi?id=521915
return query.getParameters().size() == 0 && query.getClass().getName().startsWith("org.eclipse");
}
}
}

4
src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetterFactory.java
View File

@@ -114,7 +114,7 @@ abstract class QueryParameterSetterFactory {
: null;
return new NamedOrIndexedQueryParameterSetter(valueExtractor.andThen(binding::prepare),
ParameterImpl.of(parameter, binding), temporalType, lenient);
ParameterImpl.of(parameter, binding), temporalType);
}
/**
@@ -279,7 +279,7 @@ abstract class QueryParameterSetterFactory {
TemporalType temporalType = parameter.isTemporalParameter() ? parameter.getRequiredTemporalType() : null;
return new NamedOrIndexedQueryParameterSetter(values -> getAndPrepare(parameter, metadata, values),
metadata.getExpression(), temporalType, false);
metadata.getExpression(), temporalType);
}
@Nullable

19
src/test/java/org/springframework/data/jpa/repository/EclipseLinkNamespaceUserRepositoryTests.java
View File

@@ -15,6 +15,11 @@
*/
package org.springframework.data.jpa.repository;
import static org.assertj.core.api.Assertions.*;
import javax.persistence.Query;
import org.junit.Test;
import org.springframework.data.jpa.repository.sample.UserRepository;
import org.springframework.test.context.ContextConfiguration;
@@ -23,6 +28,7 @@ import org.springframework.test.context.ContextConfiguration;
*
* @author Oliver Gierke
* @author Thomas Darimont
* @author Jens Schauder
*/
@ContextConfiguration(value = "classpath:eclipselink.xml")
public class EclipseLinkNamespaceUserRepositoryTests extends NamespaceUserRepositoryTests {
@@ -66,4 +72,17 @@ public class EclipseLinkNamespaceUserRepositoryTests extends NamespaceUserReposi
*/
@Override
public void findByElementCollectionAttribute() {}
/**
* This test will fail once https://bugs.eclipse.org/bugs/show_bug.cgi?id=521915 is fixed.
*/
@Override
@Test // DATAJPA-1172
public void queryProvidesCorrectNumberOfParametersForNativeQuery() {
Query query = em.createNativeQuery("select 1 from User where firstname=? and lastname=?");
assertThat(query.getParameters()).describedAs(
"Due to a bug eclipse has size 0. If this is no longer the case the special code path triggered in NamedOrIndexedQueryParameterSetter.registerExcessParameters can be removed")
.hasSize(0);
}
}

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

@@ -20,6 +20,7 @@ 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.springframework.data.jpa.domain.Specification.not;
import static org.springframework.data.jpa.domain.sample.UserSpecifications.*;
import java.util.ArrayList;
@@ -50,8 +51,6 @@ 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.ExampleMatcher.GenericPropertyMatcher;
import org.springframework.data.domain.ExampleMatcher.StringMatcher;
import org.springframework.data.domain.Page;
import org.springframework.data.domain.PageImpl;
import org.springframework.data.domain.PageRequest;
@@ -60,6 +59,7 @@ import org.springframework.data.domain.Slice;
import org.springframework.data.domain.Sort;
import org.springframework.data.domain.Sort.Direction;
import org.springframework.data.domain.Sort.Order;
import org.springframework.data.domain.ExampleMatcher.*;
import org.springframework.data.jpa.domain.Specification;
import org.springframework.data.jpa.domain.sample.Address;
import org.springframework.data.jpa.domain.sample.Role;
@@ -85,6 +85,7 @@ import com.google.common.base.Optional;
* @author Christoph Strobl
* @author Mark Paluch
* @author Kevin Peters
* @author Jens Schauder
*/
@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration("classpath:application-context.xml")
@@ -2048,6 +2049,21 @@ public class UserRepositoryTests {
assertThat(result.getContent().get(0)).isEqualTo(thirdUser);
}
@Test // DATAJPA-1172
public void exceptionsDuringParameterSettingGetThrown() {
assertThatExceptionOfType(InvalidDataAccessApiUsageException.class) //
.isThrownBy(() -> repository.findByStringAge("twelve")) //
.matches(e -> !e.getMessage().contains("Named parameter [age] not set"));
}
@Test // DATAJPA-1172
public void queryProvidesCorrectNumberOfParametersForNativeQuery() {
Query query = em.createNativeQuery("select 1 from User where firstname=? and lastname=?");
assertThat(query.getParameters()).hasSize(2);
}
private Page<User> executeSpecWithSort(Sort sort) {
flushTestUsers();

21
src/test/java/org/springframework/data/jpa/repository/query/ParameterBinderUnitTests.java
View File

@@ -34,6 +34,7 @@ import javax.persistence.TemporalType;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Answers;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;
import org.springframework.data.domain.Pageable;
@@ -51,9 +52,10 @@ import org.springframework.data.repository.query.Param;
@RunWith(MockitoJUnitRunner.class)
public class ParameterBinderUnitTests {
public static final int MAX_PARAMETERS = 1;
private Method valid;
@Mock private Query query;
@Mock(answer = Answers.RETURNS_DEEP_STUBS) private Query query;
private Method useIndexedParameters;
private Method indexedParametersWithSort;
@@ -64,6 +66,8 @@ public class ParameterBinderUnitTests {
useIndexedParameters = SampleRepository.class.getMethod("useIndexedParameters", String.class);
indexedParametersWithSort = SampleRepository.class.getMethod("indexedParameterWithSort", String.class, Sort.class);
when(query.getParameters().size()).thenReturn(MAX_PARAMETERS);
}
static class User {
@@ -91,6 +95,9 @@ public class ParameterBinderUnitTests {
List<User> validWithVarArgs(Integer... ids);
User optionalParameter(Optional<String> name);
@org.springframework.data.jpa.repository.Query("select x from User where name = :name")
User withQuery(String name, String other);
}
@Test
@@ -209,6 +216,18 @@ public class ParameterBinderUnitTests {
verify(query).setParameter(eq(1), eq("Foo"));
}
@Test // DATAJPA-1172
public void doesNotBindExcessParameters() throws Exception {
Method method = SampleRepository.class.getMethod("withQuery", String.class, String.class);
Object[] values = { "foo", "superfluous" };
ParameterBinderFactory.createBinder(new JpaParameters(method)).bind(query, values);
verify(query).setParameter(eq(1), any());
verify(query, never()).setParameter(eq(2), any());
}
public SampleEntity findByEmbeddable(SampleEmbeddable embeddable) {
return null;

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

@@ -491,6 +491,10 @@ public interface UserRepository
List<RolesAndFirstname> findRolesAndFirstnameBy();
// DATAJPA-1172
@Query("select u from User u where u.age = :age")
List<User> findByStringAge(@Param("age") String age);
static interface RolesAndFirstname {
String getFirstname();