From 6fb88a2ca94d80260efac8fde0d3f62a8825b6c8 Mon Sep 17 00:00:00 2001 From: Julia <5765049+sxhinzvc@users.noreply.github.com> Date: Mon, 25 Sep 2023 09:02:47 -0400 Subject: [PATCH] Restrict `TypedParameterValue` usage to native queries only. Use Hibernate parameter accessor for native queries only to avoid affecting JPQL queries. Co-locate Hibernate-specific parameters accessor as the same package as JPA parameters accessor. Remove Parameter Accessor reference from Persistence Provider since it's created in AbstractJpaQuery. Closes #3137 Original Pull Request: #3173 --- .../jpa/provider/PersistenceProvider.java | 11 ----- .../repository/query/AbstractJpaQuery.java | 7 ++- ...bernateJpaParametersParameterAccessor.java | 4 +- .../query/ParameterMetadataProvider.java | 20 ++++----- .../query/AbstractJpaQueryTests.java | 45 +++++++++++++++++++ ...aParametersParameterAccessorUnitTests.java | 3 +- .../JpaParametersParameterAccessorTests.java | 6 +-- .../ParameterMetadataProviderUnitTests.java | 33 +++++++++++++- .../QueryWithNullLikeIntegrationTests.java | 16 ++++++- 9 files changed, 111 insertions(+), 34 deletions(-) rename spring-data-jpa/src/main/java/org/springframework/data/jpa/{provider => repository/query}/HibernateJpaParametersParameterAccessor.java (95%) rename spring-data-jpa/src/test/java/org/springframework/data/jpa/{provider => repository/query}/HibernateJpaParametersParameterAccessorUnitTests.java (92%) diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/provider/PersistenceProvider.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/provider/PersistenceProvider.java index fd9eec9e3..076f447c3 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/provider/PersistenceProvider.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/provider/PersistenceProvider.java @@ -106,12 +106,6 @@ public enum PersistenceProvider implements QueryExtractor, ProxyIdAccessor, Quer return new HibernateScrollableResultsIterator(jpaQuery); } - @Override - public JpaParametersParameterAccessor getParameterAccessor(JpaParameters parameters, Object[] values, - EntityManager em) { - return new HibernateJpaParametersParameterAccessor(parameters, values, em); - } - @Override public String getCommentHintKey() { return "org.hibernate.comment"; @@ -292,11 +286,6 @@ public enum PersistenceProvider implements QueryExtractor, ProxyIdAccessor, Quer return cacheAndReturn(metamodelType, GENERIC_JPA); } - public JpaParametersParameterAccessor getParameterAccessor(JpaParameters parameters, Object[] values, - EntityManager em) { - return new JpaParametersParameterAccessor(parameters, values); - } - /** * Returns the placeholder to be used for simple count queries. Default implementation returns {@code x}. * diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/AbstractJpaQuery.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/AbstractJpaQuery.java index 32c5f438b..42135e104 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/AbstractJpaQuery.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/AbstractJpaQuery.java @@ -61,6 +61,7 @@ import org.springframework.util.Assert; * @author Jens Schauder * @author Сергей Цыпанов * @author Wonchul Heo + * @author Julia Lee */ public abstract class AbstractJpaQuery implements RepositoryQuery { @@ -153,7 +154,11 @@ public abstract class AbstractJpaQuery implements RepositoryQuery { private JpaParametersParameterAccessor obtainParameterAccessor(Object[] values) { - return provider.getParameterAccessor(method.getParameters(), values, em); + if (method.isNativeQuery() && PersistenceProvider.HIBERNATE.equals(provider)) { + return new HibernateJpaParametersParameterAccessor(method.getParameters(), values, em); + } + + return new JpaParametersParameterAccessor(method.getParameters(), values); } protected JpaQueryExecution getExecution() { diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/provider/HibernateJpaParametersParameterAccessor.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/HibernateJpaParametersParameterAccessor.java similarity index 95% rename from spring-data-jpa/src/main/java/org/springframework/data/jpa/provider/HibernateJpaParametersParameterAccessor.java rename to spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/HibernateJpaParametersParameterAccessor.java index d5f153b8e..4889483ba 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/provider/HibernateJpaParametersParameterAccessor.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/HibernateJpaParametersParameterAccessor.java @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.springframework.data.jpa.provider; +package org.springframework.data.jpa.repository.query; import jakarta.persistence.EntityManager; @@ -21,7 +21,6 @@ import org.hibernate.engine.spi.SessionFactoryImplementor; import org.hibernate.query.TypedParameterValue; import org.hibernate.type.BasicType; import org.hibernate.type.BasicTypeRegistry; -import org.springframework.data.jpa.repository.query.JpaParametersParameterAccessor; import org.springframework.data.repository.query.Parameter; import org.springframework.data.repository.query.Parameters; import org.springframework.data.repository.query.ParametersParameterAccessor; @@ -38,6 +37,7 @@ import org.springframework.lang.Nullable; * @author Robert Wilson * @author Oliver Drotbohm * @author Greg Turnquist + * @author Julia Lee * @since 2.7 */ class HibernateJpaParametersParameterAccessor extends JpaParametersParameterAccessor { diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/ParameterMetadataProvider.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/ParameterMetadataProvider.java index 0b1d9a070..cd8f50ad9 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/ParameterMetadataProvider.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/ParameterMetadataProvider.java @@ -233,31 +233,27 @@ class ParameterMetadataProvider { /** * Prepares the object before it's actually bound to the {@link jakarta.persistence.Query;}. * - * @param value must not be {@literal null}. + * @param value can be {@literal null}. */ @Nullable - public Object prepare(Object value) { + public Object prepare(@Nullable Object value) { - Assert.notNull(value, "Value must not be null"); - - Object unwrapped = PersistenceProvider.unwrapTypedParameterValue(value); - - if (unwrapped == null || expression.getJavaType() == null) { - return unwrapped; + if (value == null || expression.getJavaType() == null) { + return value; } if (String.class.equals(expression.getJavaType()) && !noWildcards) { switch (type) { case STARTING_WITH: - return String.format("%s%%", escape.escape(unwrapped.toString())); + return String.format("%s%%", escape.escape(value.toString())); case ENDING_WITH: - return String.format("%%%s", escape.escape(unwrapped.toString())); + return String.format("%%%s", escape.escape(value.toString())); case CONTAINING: case NOT_CONTAINING: - return String.format("%%%s%%", escape.escape(unwrapped.toString())); + return String.format("%%%s%%", escape.escape(value.toString())); default: - return unwrapped; + return value; } } diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/AbstractJpaQueryTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/AbstractJpaQueryTests.java index f2ee0c720..e51adcf7c 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/AbstractJpaQueryTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/AbstractJpaQueryTests.java @@ -16,7 +16,9 @@ package org.springframework.data.jpa.repository.query; import static org.assertj.core.api.Assumptions.assumeThat; +import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -36,6 +38,7 @@ import java.util.List; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; import org.springframework.data.jpa.domain.sample.User; import org.springframework.data.jpa.provider.PersistenceProvider; import org.springframework.data.jpa.repository.EntityGraph; @@ -56,6 +59,7 @@ import org.springframework.transaction.annotation.Transactional; * @author Thomas Darimont * @author Mark Paluch * @author Krzysztof Krason + * @author Julia Lee */ @ExtendWith(SpringExtension.class) @ContextConfiguration("classpath:infrastructure.xml") @@ -65,12 +69,14 @@ class AbstractJpaQueryTests { private Query query; private TypedQuery countQuery; + private JpaQueryExecution execution; @BeforeEach @SuppressWarnings("unchecked") void setUp() { query = mock(Query.class); countQuery = mock(TypedQuery.class); + execution = mock(JpaQueryExecution.class); } @Test // DATADOC-97 @@ -150,6 +156,37 @@ class AbstractJpaQueryTests { verify(result).setHint("jakarta.persistence.loadgraph", entityGraph); } + @Test // GH-3137 + void shouldCreateHibernateJpaParameterParametersAccessorForNativeQuery() throws Exception { + + JpaQueryMethod queryMethod = getMethod("findByLastnameNativeQuery", String.class); + + AbstractJpaQuery jpaQuery = new DummyJpaQuery(queryMethod, em); + + jpaQuery.execute(new Object[] {"some last name"}); + + ArgumentCaptor captor = ArgumentCaptor.forClass(JpaParametersParameterAccessor.class); + verify(execution).execute(eq(jpaQuery), captor.capture()); + JpaParametersParameterAccessor parameterAccessor = captor.getValue(); + + assertThat(parameterAccessor).isInstanceOf(HibernateJpaParametersParameterAccessor.class); + } + + @Test // GH-3137 + void shouldCreateGenericJpaParameterParametersAccessorForNonNativeQuery() throws Exception { + + JpaQueryMethod queryMethod = getMethod("findByFirstname", String.class); + AbstractJpaQuery jpaQuery = new DummyJpaQuery(queryMethod, em); + + jpaQuery.execute(new Object[] {"some first name"}); + + ArgumentCaptor captor = ArgumentCaptor.forClass(JpaParametersParameterAccessor.class); + verify(execution).execute(eq(jpaQuery), captor.capture()); + JpaParametersParameterAccessor parameterAccessor = captor.getValue(); + + assertThat(parameterAccessor).isNotInstanceOf(HibernateJpaParametersParameterAccessor.class); + } + private JpaQueryMethod getMethod(String name, Class... parameterTypes) throws Exception { Method method = SampleRepository.class.getMethod(name, parameterTypes); @@ -164,6 +201,9 @@ class AbstractJpaQueryTests { @QueryHints({ @QueryHint(name = "foo", value = "bar") }) List findByLastname(String lastname); + @org.springframework.data.jpa.repository.Query(value = "select u from User u where u.lastname = ?1", nativeQuery = true) + List findByLastnameNativeQuery(String lastname); + @QueryHints(value = { @QueryHint(name = "bar", value = "foo") }, forCounting = false) List findByFirstname(String firstname); @@ -186,6 +226,11 @@ class AbstractJpaQueryTests { super(method, em); } + @Override + protected JpaQueryExecution getExecution() { + return execution; + } + @Override protected Query doCreateQuery(JpaParametersParameterAccessor accessor) { return query; diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/provider/HibernateJpaParametersParameterAccessorUnitTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/HibernateJpaParametersParameterAccessorUnitTests.java similarity index 92% rename from spring-data-jpa/src/test/java/org/springframework/data/jpa/provider/HibernateJpaParametersParameterAccessorUnitTests.java rename to spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/HibernateJpaParametersParameterAccessorUnitTests.java index fa5810927..0e7135c5c 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/provider/HibernateJpaParametersParameterAccessorUnitTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/HibernateJpaParametersParameterAccessorUnitTests.java @@ -1,4 +1,4 @@ -package org.springframework.data.jpa.provider; +package org.springframework.data.jpa.repository.query; import jakarta.persistence.EntityManager; @@ -8,6 +8,7 @@ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.data.jpa.repository.query.HibernateJpaParametersParameterAccessor; import org.springframework.data.jpa.repository.query.JpaParameters; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.context.junit.jupiter.SpringExtension; diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/JpaParametersParameterAccessorTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/JpaParametersParameterAccessorTests.java index 4577e8145..0a702eaed 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/JpaParametersParameterAccessorTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/JpaParametersParameterAccessorTests.java @@ -43,8 +43,7 @@ class JpaParametersParameterAccessorTests { Method withNativeQuery = SampleRepository.class.getMethod("withNativeQuery", Integer.class); Object[] values = { null }; JpaParameters parameters = new JpaParameters(withNativeQuery); - JpaParametersParameterAccessor accessor = PersistenceProvider.GENERIC_JPA.getParameterAccessor(parameters, values, - em); + JpaParametersParameterAccessor accessor = new JpaParametersParameterAccessor(parameters, values); bind(parameters, accessor); @@ -57,8 +56,7 @@ class JpaParametersParameterAccessorTests { Method withNativeQuery = SampleRepository.class.getMethod("withNativeQuery", Integer.class); Object[] values = { null }; JpaParameters parameters = new JpaParameters(withNativeQuery); - JpaParametersParameterAccessor accessor = PersistenceProvider.HIBERNATE.getParameterAccessor(parameters, values, - em); + JpaParametersParameterAccessor accessor = new HibernateJpaParametersParameterAccessor(parameters, values, em); bind(parameters, accessor); diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/ParameterMetadataProviderUnitTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/ParameterMetadataProviderUnitTests.java index e30d742b7..026738f95 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/ParameterMetadataProviderUnitTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/ParameterMetadataProviderUnitTests.java @@ -22,17 +22,32 @@ import java.util.Collections; import jakarta.persistence.criteria.CriteriaBuilder; -import org.junit.jupiter.api.Test; +import org.eclipse.persistence.internal.jpa.querydef.ParameterExpressionImpl; import org.springframework.data.repository.query.Parameters; import org.springframework.data.repository.query.parser.Part; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Answers; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; + /** * Unit tests for {@link ParameterMetadataProvider}. * * @author Jens Schauder + * @author Julia Lee */ +@ExtendWith(MockitoExtension.class) +@MockitoSettings(strictness = Strictness.STRICT_STUBS) class ParameterMetadataProviderUnitTests { + @Mock(answer = Answers.RETURNS_DEEP_STUBS) Part part; + + private ParameterExpressionImpl parameterExpression = new ParameterExpressionImpl(null, String.class); + @Test // DATAJPA-863 void errorMessageMentionesParametersWhenParametersAreExhausted() { @@ -49,4 +64,20 @@ class ParameterMetadataProviderUnitTests { .withMessageContaining("parameter"); } + @Test // GH-3137 + void returnAugmentedValueForStringExpressions() { + when(part.getProperty().getLeafProperty().isCollection()).thenReturn(false); + + assertThat(createParameterMetadata(Part.Type.STARTING_WITH).prepare("starting with")).isEqualTo("starting with%"); + assertThat(createParameterMetadata(Part.Type.ENDING_WITH).prepare("ending with")).isEqualTo("%ending with"); + assertThat(createParameterMetadata(Part.Type.CONTAINING).prepare("containing")).isEqualTo("%containing%"); + assertThat(createParameterMetadata(Part.Type.NOT_CONTAINING).prepare("not containing")).isEqualTo("%not containing%"); + assertThat(createParameterMetadata(Part.Type.LIKE).prepare("%like%")).isEqualTo("%like%"); + assertThat(createParameterMetadata(Part.Type.IS_NULL).prepare(null)).isEqualTo(null); + } + + private ParameterMetadataProvider.ParameterMetadata createParameterMetadata(Part.Type partType) { + when(part.getType()).thenReturn(partType); + return new ParameterMetadataProvider.ParameterMetadata<>(parameterExpression, part, null, EscapeCharacter.DEFAULT); + } } diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/QueryWithNullLikeIntegrationTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/QueryWithNullLikeIntegrationTests.java index 898975ad8..d420a70ce 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/QueryWithNullLikeIntegrationTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/QueryWithNullLikeIntegrationTests.java @@ -54,6 +54,7 @@ import org.springframework.transaction.annotation.Transactional; * * @author Greg Turnquist * @author Yuriy Tsarkov + * @author Julia Lee */ @ExtendWith(SpringExtension.class) @ContextConfiguration(classes = QueryWithNullLikeIntegrationTests.Config.class) @@ -66,7 +67,8 @@ class QueryWithNullLikeIntegrationTests { void setUp() { repository.saveAllAndFlush(List.of( // new EmployeeWithName("Frodo Baggins"), // - new EmployeeWithName("Bilbo Baggins"))); + new EmployeeWithName("Bilbo Baggins"), + new EmployeeWithName(null))); } @Test @@ -273,7 +275,14 @@ class QueryWithNullLikeIntegrationTests { @Test // GH-1184 void alignedReturnTypeShouldWork() { assertThat(repository.customQueryWithAlignedReturnType()).containsExactly(new Object[][] { - { "Frodo Baggins", "Frodo Baggins with suffix" }, { "Bilbo Baggins", "Bilbo Baggins with suffix" } }); + { "Frodo Baggins", "Frodo Baggins with suffix" }, { "Bilbo Baggins", "Bilbo Baggins with suffix" }, { null, null} }); + } + + @Test + void nullOptionalParameterShouldReturnAllEntries() { + List result = repository.customQueryWithOptionalParameter(null); + + assertThat(result).hasSize(3); } @Transactional @@ -291,6 +300,9 @@ class QueryWithNullLikeIntegrationTests { @Query(value = "select * from EmployeeWithName as e where e.name like %:partialName%", nativeQuery = true) List customQueryWithNullableParamInNative(@Nullable @Param("partialName") String partialName); + @Query("select e from EmployeeWithName e where (:partialName is null or e.name like %:partialName%)") + List customQueryWithOptionalParameter(@Nullable @Param("partialName") String partialName); + List findByNameStartsWith(@Nullable String partialName); List findByNameEndsWith(@Nullable String partialName);