From a721243a0b45f0f28325f1cfc07e16cccded90f9 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Fri, 21 Jul 2023 14:57:34 +0200 Subject: [PATCH] Ensure correct bindings for parameter reuse. We now verify all bindings to ensure that a like-parameter doesn't mix up with plain bindings (e.g. firstname = %:myparam or lastname = :myparam). We also create unique synthetic parameters for positional bindings. Also, fix Regex to properly detect named, anonymous and positional bind markers. See #3041 --- .../repository/query/ParameterBinding.java | 115 ++++++----- .../query/QueryParameterSetterFactory.java | 44 ++++- .../jpa/repository/query/StringQuery.java | 187 ++++++++++++------ .../jpa/repository/UserRepositoryTests.java | 3 + .../ExpressionBasedStringQueryUnitTests.java | 56 ++++++ .../query/StringQueryUnitTests.java | 129 +++++++++--- .../jpa/repository/sample/UserRepository.java | 4 + 7 files changed, 392 insertions(+), 146 deletions(-) diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinding.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinding.java index bc0086fab1..3db2ab15c9 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinding.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinding.java @@ -24,7 +24,6 @@ import java.util.List; import org.springframework.data.jpa.provider.PersistenceProvider; -import org.springframework.data.jpa.repository.query.JpaParameters.JpaParameter; import org.springframework.data.repository.query.parser.Part.Type; import org.springframework.lang.Nullable; import org.springframework.util.Assert; @@ -151,8 +150,8 @@ public Object prepare(@Nullable Object valueToBind) { /** * Check whether the {@code other} binding uses the same bind target. * - * @param other - * @return + * @param other must not be {@literal null}. + * @return {@code true} if the other binding uses the same parameter to bind to as this one. */ public boolean bindsTo(ParameterBinding other) { @@ -171,6 +170,17 @@ public boolean bindsTo(ParameterBinding other) { return false; } + /** + * Check whether this binding can be bound as the {@code other} binding by checking its type and origin. Subclasses + * may override this method to include other properties for the compatibility check. + * + * @param other + * @return {@code true} if the other binding is compatible with this one. + */ + public boolean isCompatibleWith(ParameterBinding other) { + return other.getClass() == getClass() && other.getOrigin().equals(getOrigin()); + } + /** * Represents a {@link ParameterBinding} in a JPQL query augmented with instructions of how to apply a parameter as an * {@code IN} parameter. @@ -294,6 +304,16 @@ public String toString() { getType()); } + @Override + public boolean isCompatibleWith(ParameterBinding binding) { + + if (super.isCompatibleWith(binding) && binding instanceof LikeParameterBinding other) { + return getType() == other.getType(); + } + + return false; + } + /** * Extracts the like {@link Type} from the given JPA like expression. * @@ -319,52 +339,12 @@ static Type getLikeTypeFrom(String expression) { } } - static class ParameterImpl implements jakarta.persistence.Parameter { - - private final BindingIdentifier identifier; - private final Class parameterType; - - /** - * Creates a new {@link ParameterImpl} for the given {@link JpaParameter} and {@link ParameterBinding}. - * - * @param parameter can be {@literal null}. - * @param binding must not be {@literal null}. - * @return a {@link jakarta.persistence.Parameter} object based on the information from the arguments. - */ - static jakarta.persistence.Parameter of(@Nullable JpaParameter parameter, ParameterBinding binding) { - - Class type = parameter == null ? Object.class : parameter.getType(); - - return new ParameterImpl<>(binding.getIdentifier(), type); - } - - public ParameterImpl(BindingIdentifier identifier, Class parameterType) { - this.identifier = identifier; - this.parameterType = parameterType; - } - - @Nullable - @Override - public String getName() { - return identifier.hasName() ? identifier.getName() : null; - } - - @Nullable - @Override - public Integer getPosition() { - return identifier.hasPosition() ? identifier.getPosition() : null; - } - - @Override - public Class getParameterType() { - return parameterType; - } - - } - /** * Identifies a binding parameter by name, position or both. Used to bind parameters to a query or to describe a * {@link MethodInvocationArgument} origin. + * + * @author Mark Paluch + * @since 3.1.2 */ sealed interface BindingIdentifier permits Named,Indexed,NamedAndIndexed { @@ -453,6 +433,11 @@ public boolean hasName() { public String getName() { return name(); } + + @Override + public String toString() { + return name(); + } } private record Indexed(int position) implements BindingIdentifier { @@ -466,6 +451,11 @@ public boolean hasPosition() { public int getPosition() { return position(); } + + @Override + public String toString() { + return "[" + position() + "]"; + } } private record NamedAndIndexed(String name, int position) implements BindingIdentifier { @@ -489,10 +479,18 @@ public boolean hasPosition() { public int getPosition() { return position(); } + + @Override + public String toString() { + return "[" + name() + ", " + position() + "]"; + } } /** * Value type hierarchy to describe where a binding parameter comes from, either method call or an expression. + * + * @author Mark Paluch + * @since 3.1.2 */ sealed interface ParameterOrigin permits Expression,MethodInvocationArgument { @@ -508,11 +506,11 @@ static Expression ofExpression(String expression) { /** * Creates a {@link MethodInvocationArgument} object for {@code name} and {@code position}. Either the name or the - * position must be given, + * position must be given. * * @param name the parameter name from the method invocation, can be {@literal null}. * @param position the parameter position (1-based) from the method invocation, can be {@literal null}. - * @return {@link MethodInvocationArgument} object for {@code name} and {@code position} + * @return {@link MethodInvocationArgument} object for {@code name} and {@code position}. */ static MethodInvocationArgument ofParameter(@Nullable String name, @Nullable Integer position) { @@ -528,6 +526,16 @@ static MethodInvocationArgument ofParameter(@Nullable String name, @Nullable Int return ofParameter(identifier); } + /** + * Creates a {@link MethodInvocationArgument} object for {@code position}. + * + * @param position the parameter position (1-based) from the method invocation. + * @return {@link MethodInvocationArgument} object for {@code position}. + */ + static MethodInvocationArgument ofParameter(int position) { + return ofParameter(BindingIdentifier.of(position)); + } + /** * Creates a {@link MethodInvocationArgument} using {@link BindingIdentifier}. * @@ -535,12 +543,17 @@ static MethodInvocationArgument ofParameter(@Nullable String name, @Nullable Int * @return {@link MethodInvocationArgument} for {@link BindingIdentifier}. */ static MethodInvocationArgument ofParameter(BindingIdentifier identifier) { - return new MethodInvocationArgument(identifier); } + /** + * @return {@code true} if the origin is a method argument reference. + */ boolean isMethodArgument(); + /** + * @return {@code true} if the origin is an expression. + */ boolean isExpression(); } @@ -548,6 +561,8 @@ static MethodInvocationArgument ofParameter(BindingIdentifier identifier) { * Value object capturing the expression of which a binding parameter originates. * * @param expression + * @author Mark Paluch + * @since 3.1.2 */ public record Expression(String expression) implements ParameterOrigin { @@ -566,6 +581,8 @@ public boolean isExpression() { * Value object capturing the method invocation parameter reference. * * @param identifier + * @author Mark Paluch + * @since 3.1.2 */ public record MethodInvocationArgument(BindingIdentifier identifier) implements ParameterOrigin { diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetterFactory.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetterFactory.java index 0ea048f32b..b6c57b7ebf 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetterFactory.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetterFactory.java @@ -24,7 +24,6 @@ import org.springframework.data.jpa.repository.query.JpaParameters.JpaParameter; import org.springframework.data.jpa.repository.query.ParameterBinding.BindingIdentifier; import org.springframework.data.jpa.repository.query.ParameterBinding.MethodInvocationArgument; -import org.springframework.data.jpa.repository.query.ParameterBinding.ParameterImpl; import org.springframework.data.jpa.repository.query.ParameterMetadataProvider.ParameterMetadata; import org.springframework.data.jpa.repository.query.QueryParameterSetter.NamedOrIndexedQueryParameterSetter; import org.springframework.data.repository.query.Parameter; @@ -323,4 +322,47 @@ private Object getAndPrepare(JpaParameter parameter, ParameterMetadata metada } } + static class ParameterImpl implements jakarta.persistence.Parameter { + + private final BindingIdentifier identifier; + private final Class parameterType; + + /** + * Creates a new {@link ParameterImpl} for the given {@link JpaParameter} and {@link ParameterBinding}. + * + * @param parameter can be {@literal null}. + * @param binding must not be {@literal null}. + * @return a {@link jakarta.persistence.Parameter} object based on the information from the arguments. + */ + static jakarta.persistence.Parameter of(@Nullable JpaParameter parameter, ParameterBinding binding) { + + Class type = parameter == null ? Object.class : parameter.getType(); + + return new ParameterImpl<>(binding.getIdentifier(), type); + } + + public ParameterImpl(BindingIdentifier identifier, Class parameterType) { + this.identifier = identifier; + this.parameterType = parameterType; + } + + @Nullable + @Override + public String getName() { + return identifier.hasName() ? identifier.getName() : null; + } + + @Nullable + @Override + public Integer getPosition() { + return identifier.hasPosition() ? identifier.getPosition() : null; + } + + @Override + public Class getParameterType() { + return parameterType; + } + + } + } diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java index 8d82fac556..ada8793f29 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java @@ -16,14 +16,18 @@ package org.springframework.data.jpa.repository.query; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.function.BiFunction; +import java.util.function.Consumer; +import java.util.function.Function; import java.util.regex.Matcher; import java.util.regex.Pattern; import org.springframework.data.jpa.repository.query.ParameterBinding.BindingIdentifier; import org.springframework.data.jpa.repository.query.ParameterBinding.InParameterBinding; import org.springframework.data.jpa.repository.query.ParameterBinding.LikeParameterBinding; +import org.springframework.data.jpa.repository.query.ParameterBinding.MethodInvocationArgument; import org.springframework.data.jpa.repository.query.ParameterBinding.ParameterOrigin; import org.springframework.data.repository.query.SpelQueryContext; import org.springframework.data.repository.query.SpelQueryContext.SpelExtractor; @@ -41,7 +45,7 @@ * Encapsulation of a JPA query String. Offers access to parameters as bindings. The internal query String is cleaned * from decorated parameters like {@literal %:lastname%} and the matching bindings take care of applying the decorations * in the {@link ParameterBinding#prepare(Object)} method. Note that this class also handles replacing SpEL expressions - * with synthetic bind parameters + * with synthetic bind parameters. * * @author Oliver Gierke * @author Thomas Darimont @@ -169,9 +173,9 @@ enum ParameterBindingParser { // .............................................................^ start with a question mark. private static final Pattern PARAMETER_BINDING_BY_INDEX = Pattern.compile(POSITIONAL_OR_INDEXED_PARAMETER); private static final Pattern PARAMETER_BINDING_PATTERN; - private static final Pattern JDBC_STYLE_PARAM = Pattern.compile(" \\?(?!\\d)"); // ?[no digit] - private static final Pattern NUMBERED_STYLE_PARAM = Pattern.compile(" \\?(?=\\d)"); // ?[digit] - private static final Pattern NAMED_STYLE_PARAM = Pattern.compile(" :\\w+"); // :[text] + private static final Pattern JDBC_STYLE_PARAM = Pattern.compile("(?!\\\\)\\?(?!\\d)"); // no \ and [no digit] + private static final Pattern NUMBERED_STYLE_PARAM = Pattern.compile("(?!\\\\)\\?\\d"); // no \ and [digit] + private static final Pattern NAMED_STYLE_PARAM = Pattern.compile("(?!\\\\):\\w+"); // no \ and :[text] private static final String MESSAGE = "Already found parameter binding with same index / parameter name but differing binding type; " + "Already have: %s, found %s; If you bind a parameter multiple times make sure they use the same binding"; @@ -233,10 +237,21 @@ private String parseParameterBindingsOfQueryIntoBindingsAndReturnCleanedQuery(St Matcher matcher = PARAMETER_BINDING_PATTERN.matcher(resultingQuery); int expressionParameterIndex = parametersShouldBeAccessedByIndex ? greatestParameterIndex : 0; + int syntheticParameterIndex = expressionParameterIndex + spelExtractor.size(); - LikeParameterBindings likeParameterBindings = new LikeParameterBindings(); + ParameterBindings parameterBindings = new ParameterBindings(bindings, it -> checkAndRegister(it, bindings), + syntheticParameterIndex); + int currentIndex = 0; boolean usesJpaStyleParameters = false; + if (JDBC_STYLE_PARAM.matcher(resultingQuery).find()) { + queryMeta.usesJdbcStyleParameters = true; + } + + if (NUMBERED_STYLE_PARAM.matcher(resultingQuery).find() || NAMED_STYLE_PARAM.matcher(resultingQuery).find()) { + usesJpaStyleParameters = true; + } + while (matcher.find()) { if (spelExtractor.isQuoted(matcher.start())) { @@ -253,10 +268,6 @@ private String parseParameterBindingsOfQueryIntoBindingsAndReturnCleanedQuery(St String expression = spelExtractor.getParameter(parameterName == null ? parameterIndexString : parameterName); String replacement = null; - queryMeta.usesJdbcStyleParameters = JDBC_STYLE_PARAM.matcher(resultingQuery).find(); - usesJpaStyleParameters = NUMBERED_STYLE_PARAM.matcher(resultingQuery).find() - || NAMED_STYLE_PARAM.matcher(resultingQuery).find(); - expressionParameterIndex++; if ("".equals(parameterIndexString)) { parameterIndex = expressionParameterIndex; @@ -266,51 +277,65 @@ private String parseParameterBindingsOfQueryIntoBindingsAndReturnCleanedQuery(St throw new IllegalArgumentException("Mixing of ? parameters and other forms like ?1 is not supported"); } - BindingIdentifier identifier; + BindingIdentifier queryParameter; if (parameterIndex != null) { - identifier = BindingIdentifier.of(parameterIndex); + queryParameter = BindingIdentifier.of(parameterIndex); } else { - identifier = BindingIdentifier.of(parameterName); + queryParameter = BindingIdentifier.of(parameterName); } ParameterOrigin origin = ObjectUtils.isEmpty(expression) ? ParameterOrigin.ofParameter(parameterName, parameterIndex) : ParameterOrigin.ofExpression(expression); + BindingIdentifier targetBinding = queryParameter; switch (ParameterBindingType.of(typeSource)) { case LIKE: - Type likeType = ParameterBinding.LikeParameterBinding.getLikeTypeFrom(matcher.group(2)); - replacement = matcher.group(3); + Type likeType = LikeParameterBinding.getLikeTypeFrom(matcher.group(2)); - if (parameterIndex != null) { - checkAndRegister(new LikeParameterBinding(identifier, origin, likeType), bindings); + if (origin.isExpression()) { + parameterBindings.register(new LikeParameterBinding(queryParameter, origin, likeType)); } else { - - LikeParameterBinding binding = likeParameterBindings.getOrCreate(parameterName, likeType, origin); - checkAndRegister(binding, bindings); - - replacement = ":" + binding.getRequiredName(); + targetBinding = parameterBindings.register(queryParameter, origin, + (identifier) -> new LikeParameterBinding(identifier, origin, likeType)); } break; case IN: - checkAndRegister(new InParameterBinding(identifier, origin), bindings); + parameterBindings.register(new InParameterBinding(queryParameter, origin)); break; - case AS_IS: // fall-through we don't need a special parameter binding for the given parameter. + case AS_IS: // fall-through we don't need a special parameter queryParameter for the given parameter. default: - bindings.add(new ParameterBinding(identifier, origin)); + if (origin.isExpression()) { + parameterBindings.register(new ParameterBinding(queryParameter, origin)); + } else { + targetBinding = parameterBindings.register(queryParameter, origin, + (identifier) -> new ParameterBinding(identifier, origin)); + } } - if (replacement != null) { - resultingQuery = replaceFirst(resultingQuery, matcher.group(2), replacement); + replacement = targetBinding.hasName() ? ":" + targetBinding.getName() + : ((!usesJpaStyleParameters && queryMeta.usesJdbcStyleParameters) ? "?" + : "?" + targetBinding.getPosition()); + String result; + String substring = matcher.group(2); + + int index = resultingQuery.indexOf(substring, currentIndex); + if (index < 0) { + result = resultingQuery; + } else { + currentIndex = index + replacement.length(); + result = resultingQuery.substring(0, index) + replacement + + resultingQuery.substring(index + substring.length()); } + resultingQuery = result; } return resultingQuery; @@ -336,16 +361,6 @@ private static SpelExtractor createSpelExtractor(String queryWithSpel, boolean p return SpelQueryContext.of(indexToParameterName, parameterNameToReplacement).parse(queryWithSpel); } - private static String replaceFirst(String text, String substring, String replacement) { - - int index = text.indexOf(substring); - if (index < 0) { - return text; - } - - return text.substring(0, index) + replacement + text.substring(index + substring.length()); - } - @Nullable private static Integer getParameterIndex(@Nullable String parameterIndexString) { @@ -438,54 +453,96 @@ private static class Metadata { } /** - * Utility to create unique parameter bindings for LIKE that can be evaluated by - * {@code LikeRewritingQueryParameterSetterFactory}. + * Utility to create unique parameter bindings for LIKE that refer to the same underlying method parameter but are + * bound to potentially unique query parameters for {@link LikeParameterBinding#prepare(Object) LIKE rewrite}. * * @author Mark Paluch * @since 3.1.2 */ - static class LikeParameterBindings { + static class ParameterBindings { + + private final MultiValueMap methodArgumentToLikeBindings = new LinkedMultiValueMap<>(); + + private final Consumer registration; + private int syntheticParameterIndex; + + public ParameterBindings(List bindings, Consumer registration, + int syntheticParameterIndex) { - private final MultiValueMap likeBindings = new LinkedMultiValueMap<>(); + for (ParameterBinding binding : bindings) { + this.methodArgumentToLikeBindings.put(binding.getIdentifier(), new ArrayList<>(List.of(binding))); + } + + this.registration = registration; + this.syntheticParameterIndex = syntheticParameterIndex; + } /** - * Get an existing or create a new {@link LikeParameterBinding} if a previously bound {@code LIKE} expression cannot - * be reused. + * Return whether the identifier is already bound. * - * @param parameterName the parameter name as declared in the actual JPQL query. - * @param likeType type of the LIKE expression. - * @param origin origin of the parameter. - * @return the Like binding. Can return an already existing binding. + * @param identifier + * @return */ - LikeParameterBinding getOrCreate(String parameterName, Type likeType, ParameterOrigin origin) { + public boolean isBound(BindingIdentifier identifier) { + return !getBindings(identifier).isEmpty(); + } - List likeParameterBindings = likeBindings.computeIfAbsent(parameterName, - s -> new ArrayList<>()); - LikeParameterBinding reuse = null; + BindingIdentifier register(BindingIdentifier identifier, ParameterOrigin origin, + Function bindingFactory) { - // unique parameters only required for literals as expressions create unique parameter names - if (origin.isMethodArgument()) { - for (LikeParameterBinding likeParameterBinding : likeParameterBindings) { + Assert.isInstanceOf(MethodInvocationArgument.class, origin); - if (likeParameterBinding.getType() == likeType) { - reuse = likeParameterBinding; - break; - } - } + BindingIdentifier methodArgument = ((MethodInvocationArgument) origin).identifier(); + List bindingsForOrigin = getBindings(methodArgument); + + if (!isBound(identifier)) { + + ParameterBinding binding = bindingFactory.apply(identifier); + registration.accept(binding); + bindingsForOrigin.add(binding); + return binding.getIdentifier(); } - if (reuse != null) { - return reuse; + ParameterBinding binding = bindingFactory.apply(identifier); + + for (ParameterBinding existing : bindingsForOrigin) { + + if (existing.isCompatibleWith(binding)) { + return existing.getIdentifier(); + } } - if (!likeParameterBindings.isEmpty()) { - parameterName = parameterName + "_" + likeParameterBindings.size(); + BindingIdentifier syntheticIdentifier; + if (identifier.hasName() && methodArgument.hasName()) { + + int index = 0; + String newName = methodArgument.getName(); + while (existsBoundParameter(newName)) { + index++; + newName = methodArgument.getName() + "_" + index; + } + syntheticIdentifier = BindingIdentifier.of(newName); + } else { + syntheticIdentifier = BindingIdentifier.of(++syntheticParameterIndex); } - LikeParameterBinding binding = new LikeParameterBinding(BindingIdentifier.of(parameterName), origin, likeType); - likeParameterBindings.add(binding); + ParameterBinding newBinding = bindingFactory.apply(syntheticIdentifier); + registration.accept(newBinding); + bindingsForOrigin.add(newBinding); + return newBinding.getIdentifier(); + } + + private boolean existsBoundParameter(String key) { + return methodArgumentToLikeBindings.values().stream().flatMap(Collection::stream) + .anyMatch(it -> key.equals(it.getName())); + } + + private List getBindings(BindingIdentifier identifier) { + return methodArgumentToLikeBindings.computeIfAbsent(identifier, s -> new ArrayList<>()); + } - return binding; + public void register(ParameterBinding parameterBinding) { + registration.accept(parameterBinding); } } } diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java index 17e8e2638d..f3b7c9a9b7 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java @@ -984,6 +984,9 @@ void executesManualQueryWithNamedLikeExpressionCorrectly() { assertThat(repository.findByFirstnameLikeNamed("Da")).containsOnly(thirdUser); assertThat(repository.findByFirstnameLikeNamed("in")).containsOnly(fourthUser); + + assertThat(repository.findByFirstnameLikePositional("Da")).containsOnly(thirdUser); + assertThat(repository.findByFirstnameLikePositional("in")).containsOnly(fourthUser); } @Test // DATAJPA-231 diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/ExpressionBasedStringQueryUnitTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/ExpressionBasedStringQueryUnitTests.java index 874656158a..5da5b41526 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/ExpressionBasedStringQueryUnitTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/ExpressionBasedStringQueryUnitTests.java @@ -18,12 +18,16 @@ import static org.assertj.core.api.Assertions.*; import static org.mockito.Mockito.*; +import java.util.List; + import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.mockito.junit.jupiter.MockitoSettings; import org.mockito.quality.Strictness; +import org.springframework.data.jpa.repository.query.ParameterBinding.LikeParameterBinding; +import org.springframework.data.repository.query.parser.Part.Type; import org.springframework.expression.spel.standard.SpelExpressionParser; /** @@ -99,6 +103,7 @@ void shouldDetectComplexNativeQueriesWithSpelAsNonNative() { + "+ \"AND (n.updatedAt >= ?#{#networkRequest.updatedTime.startDateTime}) AND (n.updatedAt <=?#{#networkRequest.updatedTime.endDateTime})", metadata, SPEL_PARSER, true); + System.out.println(query.getQueryString()); assertThat(query.isNativeQuery()).isFalse(); } @@ -117,4 +122,55 @@ void shouldDetectSimpleNativeQueriesWithoutSpelAsNative() { assertThat(query.isNativeQuery()).isTrue(); } + + @Test // GH-3041 + void namedExpressionsShouldCreateLikeBindings() { + + StringQuery query = new ExpressionBasedStringQuery( + "select u from User u where u.firstname like %:#{foo} or u.firstname like :#{foo}%", metadata, SPEL_PARSER, + false); + + assertThat(query.hasParameterBindings()).isTrue(); + assertThat(query.getQueryString()).isEqualTo( + "select u from User u where u.firstname like :__$synthetic$__1 or u.firstname like :__$synthetic$__2"); + + List bindings = query.getParameterBindings(); + assertThat(bindings).hasSize(2); + + LikeParameterBinding binding = (LikeParameterBinding) bindings.get(0); + assertThat(binding).isNotNull(); + assertThat(binding.getName()).isEqualTo("__$synthetic$__1"); + assertThat(binding.getType()).isEqualTo(Type.ENDING_WITH); + + binding = (LikeParameterBinding) bindings.get(1); + assertThat(binding).isNotNull(); + assertThat(binding.getName()).isEqualTo("__$synthetic$__2"); + assertThat(binding.getType()).isEqualTo(Type.STARTING_WITH); + } + + @Test // GH-3041 + void indexedExpressionsShouldCreateLikeBindings() { + + StringQuery query = new ExpressionBasedStringQuery( + "select u from User u where u.firstname like %?#{foo} or u.firstname like ?#{foo}%", metadata, SPEL_PARSER, + false); + + assertThat(query.hasParameterBindings()).isTrue(); + assertThat(query.getQueryString()) + .isEqualTo("select u from User u where u.firstname like ?1 or u.firstname like ?2"); + + List bindings = query.getParameterBindings(); + assertThat(bindings).hasSize(2); + + LikeParameterBinding binding = (LikeParameterBinding) bindings.get(0); + assertThat(binding).isNotNull(); + assertThat(binding.getPosition()).isEqualTo(1); + assertThat(binding.getType()).isEqualTo(Type.ENDING_WITH); + + binding = (LikeParameterBinding) bindings.get(1); + assertThat(binding).isNotNull(); + assertThat(binding.getPosition()).isEqualTo(2); + assertThat(binding.getType()).isEqualTo(Type.STARTING_WITH); + } + } diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/StringQueryUnitTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/StringQueryUnitTests.java index dd523fc749..532af840ea 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/StringQueryUnitTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/StringQueryUnitTests.java @@ -23,9 +23,12 @@ import org.assertj.core.api.Assertions; import org.assertj.core.api.SoftAssertions; import org.junit.jupiter.api.Test; +import org.springframework.data.jpa.repository.query.ParameterBinding.BindingIdentifier; import org.springframework.data.jpa.repository.query.ParameterBinding.Expression; import org.springframework.data.jpa.repository.query.ParameterBinding.InParameterBinding; import org.springframework.data.jpa.repository.query.ParameterBinding.LikeParameterBinding; +import org.springframework.data.jpa.repository.query.ParameterBinding.MethodInvocationArgument; +import org.springframework.data.jpa.repository.query.ParameterBinding.ParameterOrigin; import org.springframework.data.repository.query.parser.Part.Type; /** @@ -85,7 +88,33 @@ void detectsPositionalLikeBindings() { assertThat(binding.getType()).isEqualTo(Type.ENDING_WITH); } - @Test // DATAJPA-292 + @Test // DATAJPA-292, GH-3041 + void detectsAnonymousLikeBindings() { + + StringQuery query = new StringQuery( + "select u from User u where u.firstname like %?% or u.lastname like %? or u.lastname=?", true); + + assertThat(query.hasParameterBindings()).isTrue(); + assertThat(query.getQueryString()) + .isEqualTo("select u from User u where u.firstname like ? or u.lastname like ? or u.lastname=?"); + + List bindings = query.getParameterBindings(); + assertThat(bindings).hasSize(3); + + LikeParameterBinding binding = (LikeParameterBinding) bindings.get(0); + assertThat(binding).isNotNull(); + assertThat(binding.getOrigin()).isEqualTo(ParameterOrigin.ofParameter(1)); + assertThat(binding.getRequiredPosition()).isEqualTo(1); + assertThat(binding.getType()).isEqualTo(Type.CONTAINING); + + binding = (LikeParameterBinding) bindings.get(1); + assertThat(binding).isNotNull(); + assertThat(binding.getOrigin()).isEqualTo(ParameterOrigin.ofParameter(2)); + assertThat(binding.getRequiredPosition()).isEqualTo(2); + assertThat(binding.getType()).isEqualTo(Type.ENDING_WITH); + } + + @Test // DATAJPA-292, GH-3041 void detectsNamedLikeBindings() { StringQuery query = new StringQuery("select u from User u where u.firstname like %:firstname", true); @@ -102,18 +131,19 @@ void detectsNamedLikeBindings() { assertThat(binding.getType()).isEqualTo(Type.ENDING_WITH); } - @Test // DATAJPA-292 + @Test // GH-3041 void rewritesNamedLikeToUniqueParametersIfNecessary() { StringQuery query = new StringQuery( - "select u from User u where u.firstname like %:firstname or u.firstname like :firstname%", true); + "select u from User u where u.firstname like %:firstname or u.firstname like :firstname% or u.firstname = :firstname", + true); assertThat(query.hasParameterBindings()).isTrue(); - assertThat(query.getQueryString()) - .isEqualTo("select u from User u where u.firstname like :firstname or u.firstname like :firstname_1"); + assertThat(query.getQueryString()).isEqualTo( + "select u from User u where u.firstname like :firstname or u.firstname like :firstname_1 or u.firstname = :firstname_2"); List bindings = query.getParameterBindings(); - assertThat(bindings).hasSize(2); + assertThat(bindings).hasSize(3); LikeParameterBinding binding = (LikeParameterBinding) bindings.get(0); assertThat(binding).isNotNull(); @@ -126,8 +156,22 @@ void rewritesNamedLikeToUniqueParametersIfNecessary() { assertThat(binding.getType()).isEqualTo(Type.STARTING_WITH); } - @Test // DATAJPA-292 - void reusesLikeBindingsWherePossible() { + @Test // GH-3041 + void rewritesPositionalLikeToUniqueParametersIfNecessary() { + + StringQuery query = new StringQuery( + "select u from User u where u.firstname like %?1 or u.firstname like ?1% or u.firstname = ?1", true); + + assertThat(query.hasParameterBindings()).isTrue(); + assertThat(query.getQueryString()) + .isEqualTo("select u from User u where u.firstname like ?1 or u.firstname like ?2 or u.firstname = ?3"); + + List bindings = query.getParameterBindings(); + assertThat(bindings).hasSize(3); + } + + @Test // GH-3041 + void reusesNamedLikeBindingsWherePossible() { StringQuery query = new StringQuery( "select u from User u where u.firstname like %:firstname or u.firstname like %:firstname% or u.firstname like %:firstname% or u.firstname like %:firstname", @@ -137,18 +181,50 @@ void reusesLikeBindingsWherePossible() { assertThat(query.getQueryString()).isEqualTo( "select u from User u where u.firstname like :firstname or u.firstname like :firstname_1 or u.firstname like :firstname_1 or u.firstname like :firstname"); - List bindings = query.getParameterBindings(); - assertThat(bindings).hasSize(2); + query = new StringQuery("select u from User u where u.firstname like %:firstname or u.firstname =:firstname", true); - LikeParameterBinding binding = (LikeParameterBinding) bindings.get(0); - assertThat(binding).isNotNull(); - assertThat(binding.getName()).isEqualTo("firstname"); - assertThat(binding.getType()).isEqualTo(Type.ENDING_WITH); + assertThat(query.hasParameterBindings()).isTrue(); + assertThat(query.getQueryString()) + .isEqualTo("select u from User u where u.firstname like :firstname or u.firstname =:firstname_1"); + } - binding = (LikeParameterBinding) bindings.get(1); - assertThat(binding).isNotNull(); - assertThat(binding.getName()).isEqualTo("firstname_1"); - assertThat(binding.getType()).isEqualTo(Type.CONTAINING); + @Test // GH-3041 + void reusesPositionalLikeBindingsWherePossible() { + + StringQuery query = new StringQuery( + "select u from User u where u.firstname like %?1 or u.firstname like %?1% or u.firstname like %?1% or u.firstname like %?1", + false); + + assertThat(query.hasParameterBindings()).isTrue(); + assertThat(query.getQueryString()).isEqualTo( + "select u from User u where u.firstname like ?1 or u.firstname like ?2 or u.firstname like ?2 or u.firstname like ?1"); + + query = new StringQuery("select u from User u where u.firstname like %?1 or u.firstname =?1", false); + + assertThat(query.hasParameterBindings()).isTrue(); + assertThat(query.getQueryString()).isEqualTo("select u from User u where u.firstname like ?1 or u.firstname =?2"); + } + + @Test // GH-3041 + void shouldRewritePositionalBindingsWithParameterReuse() { + + StringQuery query = new StringQuery( + "select u from User u where u.firstname like ?2 or u.firstname like %?2% or u.firstname like %?1% or u.firstname like %?1 OR u.firstname like ?1", + false); + + assertThat(query.hasParameterBindings()).isTrue(); + assertThat(query.getQueryString()).isEqualTo( + "select u from User u where u.firstname like ?2 or u.firstname like ?3 or u.firstname like ?1 or u.firstname like ?4 OR u.firstname like ?5"); + + List bindings = query.getParameterBindings(); + assertThat(bindings).hasSize(5); + + assertThat(bindings).extracting(ParameterBinding::getRequiredPosition).containsOnly(1, 2, 3, 4, 5); + assertThat(bindings).extracting(ParameterBinding::getOrigin) // + .map(MethodInvocationArgument.class::cast) // + .map(MethodInvocationArgument::identifier) // + .map(BindingIdentifier::getPosition) // + .containsOnly(1, 2); } @Test // DATAJPA-461 @@ -228,12 +304,6 @@ void handlesMultipleNamedLikeBindingsCorrectly() { new StringQuery("select u from User u where u.firstname like %:firstname or foo like :bar", true); } - @Test // DATAJPA-292, DATAJPA-362 - void rejectsDifferentBindingsForRepeatedParameter() { - assertThatIllegalArgumentException().isThrownBy( - () -> new StringQuery("select u from User u where u.firstname like %?1 and u.lastname like ?1%", true)); - } - @Test // DATAJPA-461 void treatsGreaterThanBindingAsSimpleBinding() { @@ -327,12 +397,6 @@ void detectsInBindingWithSpecialCharactersAndWordCharactersMixedInParentheses() softly.assertAll(); } - @Test // DATAJPA-362 - void rejectsDifferentBindingsForRepeatedParameter2() { - assertThatIllegalArgumentException().isThrownBy( - () -> new StringQuery("select u from User u where u.firstname like ?1 and u.lastname like %?1", true)); - } - @Test // DATAJPA-712 void shouldReplaceAllNamedExpressionParametersWithInClause() { @@ -529,7 +593,10 @@ void failOnMixedBindingsWithoutIndex() { @Test // DATAJPA-1307 void makesUsageOfJdbcStyleParameterAvailable() { - softly.assertThat(new StringQuery("something = ?", false).usesJdbcStyleParameters()).isTrue(); + assertThat(new StringQuery("from Something something where something = ?", false).usesJdbcStyleParameters()) + .isTrue(); + assertThat(new StringQuery("from Something something where something =?", false).usesJdbcStyleParameters()) + .isTrue(); List testQueries = Arrays.asList( // "something = ?1", // diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java index 1cfff59c28..344f064f9d 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java @@ -160,6 +160,10 @@ public interface UserRepository extends JpaRepository, JpaSpecifi @Query("select u from User u where u.firstname like :firstname% or u.firstname like %:firstname") List findByFirstnameLikeNamed(@Param("firstname") String firstname); + // DATAJPA-292, GH-3041 + @Query("select u from User u where u.firstname like ?1% or u.firstname like %?1") + List findByFirstnameLikePositional(String firstname); + /** * Manipulating query to set all {@link User}'s names to the given one. *