From 63b72c70051ff5d234c27b6cfa29ec159f301450 Mon Sep 17 00:00:00 2001 From: Jens Schauder Date: Thu, 29 Mar 2018 14:42:22 +0200 Subject: [PATCH] DATAJPA-1307 - JDBC style query parameters work for native queries. JDBC style query parameters denoted by a simple ? work for native queries. They can't be used with JPA queries nor with other parameter formats like ?3, :name or SpEL parameters. --- .../query/AbstractStringBasedJpaQuery.java | 3 + .../jpa/repository/query/DeclaredQuery.java | 7 ++ .../repository/query/EmptyDeclaredQuery.java | 9 +++ .../jpa/repository/query/StringQuery.java | 81 ++++++++++++++++--- .../jpa/repository/UserRepositoryTests.java | 8 ++ .../ParameterBindingParserUnitTests.java | 3 +- .../query/SimpleJpaQueryUnitTests.java | 27 ++++++- .../query/StringQueryUnitTests.java | 73 +++++++++++++++++ .../jpa/repository/sample/UserRepository.java | 4 + 9 files changed, 199 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/springframework/data/jpa/repository/query/AbstractStringBasedJpaQuery.java b/src/main/java/org/springframework/data/jpa/repository/query/AbstractStringBasedJpaQuery.java index 0993f0723..5ac9baa85 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/AbstractStringBasedJpaQuery.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/AbstractStringBasedJpaQuery.java @@ -64,6 +64,9 @@ abstract class AbstractStringBasedJpaQuery extends AbstractJpaQuery { this.countQuery = query.deriveCountQuery(method.getCountQuery(), method.getCountQueryProjection()); this.parser = parser; + + Assert.isTrue(method.isNativeQuery() || !query.usesJdbcStyleParameters(), + "JDBC style parameters (?) are not supported for JPA queries."); } /* diff --git a/src/main/java/org/springframework/data/jpa/repository/query/DeclaredQuery.java b/src/main/java/org/springframework/data/jpa/repository/query/DeclaredQuery.java index 047d8ec8d..e9aaa114a 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/DeclaredQuery.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/DeclaredQuery.java @@ -91,4 +91,11 @@ interface DeclaredQuery { default boolean usesPaging() { return false; } + + /** + * Returns wether the query uses JDBC style parameters, i.e. parameters denoted by a simple ? without any index or name. + * + * @return Wether the query uses JDBC style parameters. + */ + boolean usesJdbcStyleParameters(); } diff --git a/src/main/java/org/springframework/data/jpa/repository/query/EmptyDeclaredQuery.java b/src/main/java/org/springframework/data/jpa/repository/query/EmptyDeclaredQuery.java index 34f273e25..e9c564fda 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/EmptyDeclaredQuery.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/EmptyDeclaredQuery.java @@ -99,4 +99,13 @@ class EmptyDeclaredQuery implements DeclaredQuery { return DeclaredQuery.of(countQuery); } + + /* + * (non-Javadoc) + * @see org.springframework.data.jpa.repository.query.DeclaredQuery#usesJdbcStyleParameters() + */ + @Override + public boolean usesJdbcStyleParameters() { + return false; + } } diff --git a/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java b/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java index a96331c27..71f4c283f 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java @@ -54,6 +54,7 @@ class StringQuery implements DeclaredQuery { private final @Nullable String alias; private final boolean hasConstructorExpression; private final boolean containsPageableInSpel; + private final boolean usesJdbcStyleParameters; /** * Creates a new {@link StringQuery} from the given JPQL query. @@ -67,9 +68,11 @@ class StringQuery implements DeclaredQuery { this.bindings = new ArrayList<>(); this.containsPageableInSpel = query.contains("#pageable"); + Metadata queryMeta = new Metadata(); this.query = ParameterBindingParser.INSTANCE.parseParameterBindingsOfQueryIntoBindingsAndReturnCleanedQuery(query, - this.bindings); + this.bindings, queryMeta); + this.usesJdbcStyleParameters = queryMeta.usesJdbcStyleParameters; this.alias = QueryUtils.detectAlias(query); this.hasConstructorExpression = QueryUtils.hasConstructorExpression(query); } @@ -106,6 +109,15 @@ class StringQuery implements DeclaredQuery { .of(countQuery != null ? countQuery : QueryUtils.createCountQueryFor(query, countQueryProjection)); } + /* + * (non-Javadoc) + * @see org.springframework.data.jpa.repository.query.DeclaredQuery#usesJdbcStyleParameters() + */ + @Override + public boolean usesJdbcStyleParameters() { + return usesJdbcStyleParameters; + } + /* * (non-Javadoc) * @see org.springframework.data.jpa.repository.query.DeclaredQuery#getQueryString() @@ -171,7 +183,11 @@ class StringQuery implements DeclaredQuery { INSTANCE; static final String EXPRESSION_PARAMETER_PREFIX = "__$synthetic$__"; - private static final Pattern PARAMETER_BINDING_BY_INDEX = Pattern.compile("\\?(\\d+)"); + public static final String POSITIONAL_OR_INDEXED_PARAMETER = "\\?(\\d*+(?![#\\w]))"; + // .....................................................................^ not followed by a hash or a letter. + // .................................................................^ zero or more digits. + // .............................................................^ 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 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."; @@ -197,7 +213,7 @@ class StringQuery implements DeclaredQuery { builder.append("(?: )?"); // some whitespace builder.append("\\(?"); // optional braces around parameters builder.append("("); - builder.append("%?(\\?(\\d+))%?"); // position parameter and parameter index + builder.append("%?(" + POSITIONAL_OR_INDEXED_PARAMETER + ")%?"); // position parameter and parameter index builder.append("|"); // or // named parameter and the parameter name @@ -214,8 +230,8 @@ class StringQuery implements DeclaredQuery { * Parses {@link ParameterBinding} instances from the given query and adds them to the registered bindings. Returns * the cleaned up query. */ - String parseParameterBindingsOfQueryIntoBindingsAndReturnCleanedQuery(String query, - List bindings) { + String parseParameterBindingsOfQueryIntoBindingsAndReturnCleanedQuery(String query, List bindings, + Metadata queryMeta) { String result = query; Matcher matcher = PARAMETER_BINDING_PATTERN.matcher(query); @@ -240,6 +256,7 @@ class StringQuery implements DeclaredQuery { QuotationMap quotationMap = new QuotationMap(query); + boolean usesJpaStyleParameters = false; while (matcher.find()) { if (quotationMap.isQuoted(matcher.start())) { @@ -248,30 +265,50 @@ class StringQuery implements DeclaredQuery { String parameterIndexString = matcher.group(INDEXED_PARAMETER_GROUP); String parameterName = parameterIndexString != null ? null : matcher.group(NAMED_PARAMETER_GROUP); - Integer parameterIndex = parameterIndexString == null ? null : Integer.valueOf(parameterIndexString); + Integer parameterIndex = getParameterIndex(parameterIndexString); + String typeSource = matcher.group(COMPARISION_TYPE_GROUP); String expression = null; String replacement = null; if (parameterName == null && parameterIndex == null) { + expressionParameterIndex++; - if (parametersShouldBeAccessedByIndex) { + if ("".equals(parameterIndexString)) { + parameterIndex = expressionParameterIndex; - replacement = "?" + parameterIndex; + queryMeta.usesJdbcStyleParameters = true; } else { - parameterName = EXPRESSION_PARAMETER_PREFIX + expressionParameterIndex; - replacement = ":" + parameterName; + + usesJpaStyleParameters = true; + + if (parametersShouldBeAccessedByIndex) { + + parameterIndex = expressionParameterIndex; + replacement = "?" + parameterIndex; + } else { + + parameterName = EXPRESSION_PARAMETER_PREFIX + expressionParameterIndex; + replacement = ":" + parameterName; + } } expression = matcher.group(EXPRESSION_GROUP); + } else { + usesJpaStyleParameters = true; } + if (usesJpaStyleParameters && queryMeta.usesJdbcStyleParameters) { + throw new IllegalArgumentException("Mixing of ? parameters and other forms like ?1 is not supported"); + } + + String replacementTarget = matcher.group(2); switch (ParameterBindingType.of(typeSource)) { case LIKE: - Type likeType = LikeParameterBinding.getLikeTypeFrom(matcher.group(2)); + Type likeType = LikeParameterBinding.getLikeTypeFrom(replacementTarget); replacement = replacement != null ? replacement : matcher.group(3); if (parameterIndex != null) { @@ -299,10 +336,11 @@ class StringQuery implements DeclaredQuery { bindings.add(parameterIndex != null ? new ParameterBinding(null, parameterIndex, expression) : new ParameterBinding(parameterName, null, expression)); + } if (replacement != null) { - result = replaceFirst(result, matcher.group(2), replacement); + result = replaceFirst(result, replacementTarget, replacement); } } @@ -310,6 +348,15 @@ class StringQuery implements DeclaredQuery { return result; } + @Nullable + private Integer getParameterIndex(@Nullable String parameterIndexString) { + + if (parameterIndexString == null || parameterIndexString.isEmpty()) { + return null; + } + return Integer.valueOf(parameterIndexString); + } + private static String replaceFirst(String text, String substring, String replacement) { int index = text.indexOf(substring); @@ -326,8 +373,12 @@ class StringQuery implements DeclaredQuery { int greatestParameterIndex = -1; while (parameterIndexMatcher.find()) { + String parameterIndexString = parameterIndexMatcher.group(1); - greatestParameterIndex = Math.max(greatestParameterIndex, Integer.parseInt(parameterIndexString)); + Integer parameterIndex = getParameterIndex(parameterIndexString); + if (parameterIndex != null) { + greatestParameterIndex = Math.max(greatestParameterIndex, parameterIndex); + } } return greatestParameterIndex; @@ -839,4 +890,8 @@ class StringQuery implements DeclaredQuery { return quotedRanges.stream().anyMatch(r -> r.contains(index)); } } + + static class Metadata { + private boolean usesJdbcStyleParameters = false; + } } 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 e94d4d463..22fcc0f17 100644 --- a/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java @@ -2202,6 +2202,14 @@ public class UserRepositoryTests { softly.assertAll(); } + @Test // DATAJPA-1307 + public void testFindByEmailAddressJdbcStyleParameter() throws Exception { + + flushTestUsers(); + + assertThat(repository.findByEmailNativeAddressJdbcStyleParameter("gierke@synyx.de")).isEqualTo(firstUser); + } + private Page executeSpecWithSort(Sort sort) { flushTestUsers(); diff --git a/src/test/java/org/springframework/data/jpa/repository/query/ParameterBindingParserUnitTests.java b/src/test/java/org/springframework/data/jpa/repository/query/ParameterBindingParserUnitTests.java index 1ca7aa2e3..33015e5ed 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/ParameterBindingParserUnitTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/ParameterBindingParserUnitTests.java @@ -70,7 +70,8 @@ public class ParameterBindingParserUnitTests { public void checkHasParameter(SoftAssertions softly, String query, boolean containsParameter, String label) { List bindings = new ArrayList<>(); - ParameterBindingParser.INSTANCE.parseParameterBindingsOfQueryIntoBindingsAndReturnCleanedQuery(query, bindings); + ParameterBindingParser.INSTANCE.parseParameterBindingsOfQueryIntoBindingsAndReturnCleanedQuery(query, bindings, + new StringQuery.Metadata()); softly.assertThat(bindings.size()) // .describedAs(String.format("<%s> (%s)", query, label)) // .isEqualTo(containsParameter ? 1 : 0); diff --git a/src/test/java/org/springframework/data/jpa/repository/query/SimpleJpaQueryUnitTests.java b/src/test/java/org/springframework/data/jpa/repository/query/SimpleJpaQueryUnitTests.java index 06b3c1a1c..ca2b8a8ea 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/SimpleJpaQueryUnitTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/SimpleJpaQueryUnitTests.java @@ -15,9 +15,12 @@ */ package org.springframework.data.jpa.repository.query; +import static org.assertj.core.api.Assertions.*; import static org.hamcrest.Matchers.*; -import static org.junit.Assert.*; -import static org.mockito.ArgumentMatchers.*; +import static org.junit.Assert.assertThat; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.*; import java.lang.reflect.Method; @@ -219,6 +222,20 @@ public class SimpleJpaQueryUnitTests { verify(em, times(2)).createQuery(anyString()); } + @Test // DATAJPA-1307 + public void jdbcStyleParametersOnlyAllowedInNativeQueries() throws Exception { + + // just verifying that it doesn't throw an exception + createJpaQuery(SampleRepository.class.getMethod("legalUseOfJdbcStyleParameters", String.class)); + + assertThatExceptionOfType(IllegalArgumentException.class) // + .isThrownBy( // + () -> createJpaQuery( // + SampleRepository.class.getMethod("illegalUseOfJdbcStyleParameters", String.class) // + ) // + ); + } + private AbstractJpaQuery createJpaQuery(Method method) { JpaQueryMethod queryMethod = new JpaQueryMethod(method, metadata, factory, extractor); @@ -236,6 +253,12 @@ public class SimpleJpaQueryUnitTests { @Query(value = "SELECT u FROM User u WHERE u.lastname = ?1", nativeQuery = true) List findNativeByLastname(String lastname, Pageable pageable); + @Query(value = "SELECT u FROM User u WHERE u.lastname = ?", nativeQuery = true) + List legalUseOfJdbcStyleParameters(String lastname); + + @Query(value = "SELECT u FROM User u WHERE u.lastname = ?") + List illegalUseOfJdbcStyleParameters(String lastname); + @Query(USER_QUERY) List findByAnnotatedQuery(); diff --git a/src/test/java/org/springframework/data/jpa/repository/query/StringQueryUnitTests.java b/src/test/java/org/springframework/data/jpa/repository/query/StringQueryUnitTests.java index 2f5233f50..43608a68b 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/StringQueryUnitTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/StringQueryUnitTests.java @@ -18,8 +18,10 @@ package org.springframework.data.jpa.repository.query; import static org.hamcrest.Matchers.*; import static org.junit.Assert.*; +import java.util.Arrays; import java.util.List; +import org.assertj.core.api.Assertions; import org.assertj.core.api.SoftAssertions; import org.junit.Rule; import org.junit.Test; @@ -396,6 +398,77 @@ public class StringQueryUnitTests { softly.assertAll(); } + @Test // DATAJPA-1307 + public void detectsMultiplePositionalParameterBindingsWithoutIndex() { + + SoftAssertions softly = new SoftAssertions(); + + String queryString = "select u from User u where u.id in ? and u.names in ? and foo = ?"; + StringQuery query = new StringQuery(queryString); + + softly.assertThat(query.getQueryString()).isEqualTo(queryString); + softly.assertThat(query.hasParameterBindings()).isTrue(); + softly.assertThat(query.getParameterBindings()).hasSize(3); + + softly.assertAll(); + } + + @Test // DATAJPA-1307 + public void failOnMixedBindingsWithoutIndex() { + + List testQueries = Arrays.asList( // + "something = ? and something = ?1", // + "something = ?1 and something = ?", // + "something = :name and something = ?", // + "something = ?#{xx} and something = ?" // + ); + + for (String testQuery : testQueries) { + + Assertions.assertThatExceptionOfType(IllegalArgumentException.class) // + .describedAs(testQuery).isThrownBy(() -> new StringQuery(testQuery)); + } + } + + @Test // DATAJPA + public void makesUsageOfJdbcStyleParameterAvailable() { + + SoftAssertions softly = new SoftAssertions(); + + softly.assertThat(new StringQuery("something = ?").usesJdbcStyleParameters()).isTrue(); + + List testQueries = Arrays.asList( // + "something = ?1", // + "something = :name", // + "something = ?#{xx}" // + ); + + for (String testQuery : testQueries) { + + softly.assertThat(new StringQuery(testQuery) // + .usesJdbcStyleParameters()) // + .describedAs(testQuery) // + .isFalse(); + } + + softly.assertAll(); + } + + @Test // DATAJPA-1307 + public void questionMarkInStringLiteral() { + + SoftAssertions softly = new SoftAssertions(); + + String queryString = "select '? ' from dual"; + StringQuery query = new StringQuery(queryString); + + softly.assertThat(query.getQueryString()).isEqualTo(queryString); + softly.assertThat(query.hasParameterBindings()).isFalse(); + softly.assertThat(query.getParameterBindings()).hasSize(0); + + softly.assertAll(); + } + public void checkNumberOfNamedParameters(String query, int expectedSize, String label) { DeclaredQuery declaredQuery = DeclaredQuery.of(query); 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 18008d26a..adaea2a36 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 @@ -549,6 +549,10 @@ public interface UserRepository @Query("select firstname as firstname, lastname as lastname from User u where u.firstname = 'Oliver'") Map findMapWithNullValues(); + // DATAJPA-1307 + @Query(value = "select * from SD_User u where u.emailAddress = ?", nativeQuery = true) + User findByEmailNativeAddressJdbcStyleParameter(String emailAddress); + interface RolesAndFirstname { String getFirstname();