Skip to content

Commit

Permalink
Restrict TypedParameterValue usage to native queries only.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
sxhinzvc authored and mp911de committed Sep 27, 2023
1 parent 5eb9c30 commit 0a5ac69
Show file tree
Hide file tree
Showing 9 changed files with 111 additions and 34 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -106,12 +106,6 @@ public CloseableIterator<Object> executeQueryWithResultStream(Query jpaQuery) {
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";
Expand Down Expand Up @@ -292,11 +286,6 @@ public static PersistenceProvider fromMetamodel(Metamodel metamodel) {
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}.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
* @author Jens Schauder
* @author Сергей Цыпанов
* @author Wonchul Heo
* @author Julia Lee
*/
public abstract class AbstractJpaQuery implements RepositoryQuery {

Expand Down Expand Up @@ -153,7 +154,11 @@ private Object doExecute(JpaQueryExecution execution, Object[] values) {

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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,14 @@
* 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;

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;
Expand All @@ -38,6 +37,7 @@
* @author Robert Wilson
* @author Oliver Drotbohm
* @author Greg Turnquist
* @author Julia Lee
* @since 2.7
*/
class HibernateJpaParametersParameterAccessor extends JpaParametersParameterAccessor {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,31 +226,27 @@ public boolean isIsNullParameter() {
/**
* 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;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
*/
package org.springframework.data.jpa.repository.query;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.*;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.*;
import static org.springframework.data.jpa.support.EntityManagerTestUtils.*;

Expand All @@ -33,6 +35,7 @@
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;
Expand All @@ -53,6 +56,7 @@
* @author Oliver Gierke
* @author Thomas Darimont
* @author Mark Paluch
* @author Julia Lee
*/
@ExtendWith(SpringExtension.class)
@ContextConfiguration("classpath:infrastructure.xml")
Expand All @@ -62,12 +66,14 @@ public class AbstractJpaQueryTests {

private Query query;
private TypedQuery<Long> countQuery;
private JpaQueryExecution execution;

@BeforeEach
@SuppressWarnings("unchecked")
void setUp() {
query = mock(Query.class);
countQuery = mock(TypedQuery.class);
execution = mock(JpaQueryExecution.class);
}

@Test // DATADOC-97
Expand Down Expand Up @@ -147,6 +153,37 @@ void shouldAddEntityGraphHintForLoad() throws Exception {
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<JpaParametersParameterAccessor> 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<JpaParametersParameterAccessor> 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);
Expand All @@ -161,6 +198,9 @@ interface SampleRepository extends Repository<User, Integer> {
@QueryHints({ @QueryHint(name = "foo", value = "bar") })
List<User> findByLastname(String lastname);

@org.springframework.data.jpa.repository.Query(value = "select u from User u where u.lastname = ?1", nativeQuery = true)
List<User> findByLastnameNativeQuery(String lastname);

@QueryHints(value = { @QueryHint(name = "bar", value = "foo") }, forCounting = false)
List<User> findByFirstname(String firstname);

Expand All @@ -183,6 +223,11 @@ class DummyJpaQuery extends AbstractJpaQuery {
super(method, em);
}

@Override
protected JpaQueryExecution getExecution() {
return execution;
}

@Override
protected Query doCreateQuery(JpaParametersParameterAccessor accessor) {
return query;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package org.springframework.data.jpa.provider;
package org.springframework.data.jpa.repository.query;

import jakarta.persistence.EntityManager;

Expand All @@ -8,6 +8,7 @@
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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,7 @@ void createsJpaParametersParameterAccessor() throws Exception {
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);

Expand All @@ -57,8 +56,7 @@ void createsHibernateParametersParameterAccessor() throws Exception {
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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,32 @@

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() {

Expand All @@ -49,4 +64,20 @@ void errorMessageMentionesParametersWhenParametersAreExhausted() {
.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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
*
* @author Greg Turnquist
* @author Yuriy Tsarkov
* @author Julia Lee
*/
@ExtendWith(SpringExtension.class)
@ContextConfiguration(classes = QueryWithNullLikeHibernateIntegrationTests.Config.class)
Expand All @@ -66,7 +67,8 @@ public class QueryWithNullLikeHibernateIntegrationTests {
void setUp() {
repository.saveAllAndFlush(List.of( //
new EmployeeWithName("Frodo Baggins"), //
new EmployeeWithName("Bilbo Baggins")));
new EmployeeWithName("Bilbo Baggins"),
new EmployeeWithName(null)));
}

@Test
Expand Down Expand Up @@ -273,7 +275,14 @@ void mismatchedReturnTypeShouldCauseException() {
@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<EmployeeWithName> result = repository.customQueryWithOptionalParameter(null);

assertThat(result).hasSize(3);
}

@Transactional
Expand All @@ -291,6 +300,9 @@ public interface EmployeeWithNullLikeRepository extends JpaRepository<EmployeeWi
@Query(value = "select * from EmployeeWithName as e where e.name like %:partialName%", nativeQuery = true)
List<EmployeeWithName> customQueryWithNullableParamInNative(@Nullable @Param("partialName") String partialName);

@Query("select e from EmployeeWithName e where (:partialName is null or e.name like %:partialName%)")
List<EmployeeWithName> customQueryWithOptionalParameter(@Nullable @Param("partialName") String partialName);

List<EmployeeWithName> findByNameStartsWith(@Nullable String partialName);

List<EmployeeWithName> findByNameEndsWith(@Nullable String partialName);
Expand Down

0 comments on commit 0a5ac69

Please sign in to comment.