From e072a56a27869081dfcc43e07e7045c649359852 Mon Sep 17 00:00:00 2001 From: Jens Schauder Date: Wed, 6 Sep 2017 07:49:34 +0200 Subject: [PATCH] DATAJPA-1172 - We do not swallow exceptions from setting parameters. 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. --- .../query/QueryParameterSetter.java | 64 +++++++++---------- .../query/QueryParameterSetterFactory.java | 4 +- ...lipseLinkNamespaceUserRepositoryTests.java | 19 ++++++ .../jpa/repository/UserRepositoryTests.java | 20 +++++- .../query/ParameterBinderUnitTests.java | 21 +++++- .../jpa/repository/sample/UserRepository.java | 4 ++ 6 files changed, 95 insertions(+), 37 deletions(-) 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 e1773ee32..d330bb828 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 @@ -49,23 +49,20 @@ interface QueryParameterSetter { private final Function 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 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) 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) 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) parameter, value); + } else if (parameter.getName() != null && QueryUtils.hasNamedParameter(query)) { + query.setParameter(parameter.getName(), value); } else { - - if (parameter instanceof ParameterExpression) { - query.setParameter((Parameter) 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"); + } } } diff --git a/src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetterFactory.java b/src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetterFactory.java index 6b02a50ea..75b9e8f60 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetterFactory.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetterFactory.java @@ -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 diff --git a/src/test/java/org/springframework/data/jpa/repository/EclipseLinkNamespaceUserRepositoryTests.java b/src/test/java/org/springframework/data/jpa/repository/EclipseLinkNamespaceUserRepositoryTests.java index 30792c8f8..670c757be 100644 --- a/src/test/java/org/springframework/data/jpa/repository/EclipseLinkNamespaceUserRepositoryTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/EclipseLinkNamespaceUserRepositoryTests.java @@ -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); + } } 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 8a83fa826..f08a6d636 100644 --- a/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java @@ -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 executeSpecWithSort(Sort sort) { flushTestUsers(); diff --git a/src/test/java/org/springframework/data/jpa/repository/query/ParameterBinderUnitTests.java b/src/test/java/org/springframework/data/jpa/repository/query/ParameterBinderUnitTests.java index 26fefe61a..94f016e06 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/ParameterBinderUnitTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/ParameterBinderUnitTests.java @@ -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 validWithVarArgs(Integer... ids); User optionalParameter(Optional 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; 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 f00a249a5..e841189d7 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 @@ -491,6 +491,10 @@ public interface UserRepository List findRolesAndFirstnameBy(); + // DATAJPA-1172 + @Query("select u from User u where u.age = :age") + List findByStringAge(@Param("age") String age); + static interface RolesAndFirstname { String getFirstname();