From 1d4b57f6847b2d43ce5a371b055ba541d557af59 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Tue, 11 Feb 2014 15:07:02 +0100 Subject: [PATCH] DATAJPA-461 - Polishing of binding implementation for StringQueries. We now always create a ParameterBinding for all parameters to simplify the client code so that it can safely always lookup bindings and apply them. Changed the setup of the regular expression to work with the keywords provided by the binding types to ease future extensions. Added integration test to quickly verify the EclipseLink bug we're running into now for further reference. Original pull request: #56. --- .../jpa/repository/query/ParameterBinder.java | 21 ++-- .../jpa/repository/query/StringQuery.java | 100 ++++++++++++------ .../query/StringQueryParameterBinder.java | 22 ++-- .../EclipseLinkMetamodelIntegrationTests.java | 13 ++- .../MetamodelIntegrationTests.java | 14 ++- .../query/LikeBindingUnitTests.java | 5 +- .../query/StringQueryUnitTests.java | 6 +- 7 files changed, 115 insertions(+), 66 deletions(-) diff --git a/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinder.java b/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinder.java index 1341f4e65..c759e2ee4 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinder.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinder.java @@ -40,8 +40,8 @@ public class ParameterBinder { /** * Creates a new {@link ParameterBinder}. * - * @param parameters - * @param values + * @param parameters must not be {@literal null}. + * @param values must not be {@literal null}. */ public ParameterBinder(JpaParameters parameters, Object[] values) { @@ -55,7 +55,6 @@ public class ParameterBinder { } ParameterBinder(JpaParameters parameters) { - this(parameters, new Object[0]); } @@ -128,12 +127,10 @@ public class ParameterBinder { return; } - Object valueToUse = value; - if (hasNamedParameter(query) && parameter.isNamedParameter()) { - query.setParameter(parameter.getName(), valueToUse); + query.setParameter(parameter.getName(), value); } else { - query.setParameter(position, valueToUse); + query.setParameter(position, value); } } @@ -144,10 +141,13 @@ public class ParameterBinder { * @return */ public Query bindAndPrepare(Query query) { - return bindAndPrepare(query, parameters); } + boolean hasNamedParameter(Query query) { + return QueryUtils.hasNamedParameter(query); + } + private Query bindAndPrepare(Query query, Parameters parameters) { Query result = bind(query); @@ -161,9 +161,4 @@ public class ParameterBinder { return result; } - - boolean hasNamedParameter(Query query) { - - return QueryUtils.hasNamedParameter(query); - } } 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 497206881..7ea37fd9d 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 @@ -97,18 +97,20 @@ class StringQuery { /** * Returns the {@link ParameterBinding} for the given name. * - * @param name + * @param name must not be {@literal null} or empty. * @return */ public ParameterBinding getBindingFor(String name) { - for (StringQuery.ParameterBinding binding : bindings) { + Assert.hasText(name, "Name must not be null or empty!"); + + for (ParameterBinding binding : bindings) { if (binding.hasName(name)) { return binding; } } - return null; + throw new IllegalArgumentException(String.format("No parameter binding found for name %s!", name)); } /** @@ -118,13 +120,14 @@ class StringQuery { * @return */ public ParameterBinding getBindingFor(int position) { + for (ParameterBinding binding : bindings) { if (binding.hasPosition(position)) { return binding; } } - return null; + throw new IllegalArgumentException(String.format("No parameter binding found for position %s!", position)); } /** @@ -132,7 +135,7 @@ class StringQuery { * * @author Thomas Darimont */ - static enum ParameterBindingParser { + private static enum ParameterBindingParser { INSTANCE; @@ -142,9 +145,19 @@ class StringQuery { static { + List keywords = new ArrayList(); + + for (ParameterBindingType type : ParameterBindingType.values()) { + if (type.getKeyword() != null) { + keywords.add(type.getKeyword()); + } + } + StringBuilder builder = new StringBuilder(); - builder.append("(?<=(like|in))"); // parameterBindingType -> starts with "like" or "in" - builder.append("(?: )+"); // some whitespace + builder.append("(?<=("); + builder.append(StringUtils.collectionToDelimitedString(keywords, "|")); // keywords + builder.append("))"); + builder.append("(?: )?"); // some whitespace builder.append("("); builder.append("%?(\\?(\\d+))%?"); // position parameter builder.append("|"); // or @@ -173,7 +186,8 @@ class StringQuery { String parameterName = parameterIndexString != null ? null : matcher.group(6); Integer parameterIndex = parameterIndexString == null ? null : Integer.valueOf(parameterIndexString); - switch (ParameterBindingKind.of(matcher.group(1))) { + switch (ParameterBindingType.of(matcher.group(1))) { + case LIKE: Type likeType = LikeParameterBinding.getLikeTypeFrom(matcher.group(2)); @@ -187,7 +201,6 @@ class StringQuery { } result = StringUtils.replace(result, matcher.group(2), replacement); - break; case IN: @@ -198,11 +211,15 @@ class StringQuery { checkAndRegister(new InParameterBinding(parameterName), bindings); } + result = query; break; case AS_IS: // fall-through we don't need a special parameter binding for the given parameter. default: - ; + + bindings.add(parameterIndex != null ? new ParameterBinding(parameterIndex) : new ParameterBinding( + parameterName)); + result = query; } } @@ -221,25 +238,44 @@ class StringQuery { } /** - * An enum for the supported parameter binding kinds. + * An enum for the different types of bindings. * * @author Thomas Darimont + * @author Oliver Gierke */ - enum ParameterBindingKind { + private static enum ParameterBindingType { - LIKE, IN, AS_IS; + // Trailing whitespace is intentional to reflect that the keywords must be used with at least one whitespace + // character, while = does not. + LIKE("like "), IN("in "), AS_IS("="); + + private final String keyword; + + private ParameterBindingType(String keyword) { + this.keyword = keyword; + } /** - * Return the appropriate {@link ParameterBindingKind} for the given {@link String}. Returns {@value #AS_IS} in - * case no other {@link ParameterBindingKind} could be found. + * Returns the keyword that will tirgger the binding type or {@literal null} if the type is not triggered by a + * keyword. + * + * @return the keyword + */ + public String getKeyword() { + return keyword; + } + + /** + * Return the appropriate {@link ParameterBindingType} for the given {@link String}. Returns {@keyword + * #AS_IS} in case no other {@link ParameterBindingType} could be found. * * @param parameterBindingKindName * @return */ - static ParameterBindingKind of(String parameterBindingKindName) { + static ParameterBindingType of(String parameterBindingKindName) { - for (ParameterBindingKind type : values()) { - if (type.name().equalsIgnoreCase(parameterBindingKindName)) { + for (ParameterBindingType type : values()) { + if (type.name().equalsIgnoreCase(parameterBindingKindName.trim())) { return type; } } @@ -321,6 +357,10 @@ class StringQuery { return position; } + /* + * (non-Javadoc) + * @see java.lang.Object#hashCode() + */ @Override public int hashCode() { @@ -332,6 +372,10 @@ class StringQuery { return result; } + /* + * (non-Javadoc) + * @see java.lang.Object#equals(java.lang.Object) + */ @Override public boolean equals(Object obj) { @@ -393,16 +437,7 @@ class StringQuery { * @see org.springframework.data.jpa.repository.query.StringQuery.ParameterBinding#prepare(java.lang.Object) */ @Override - public Object prepare(Object valueToBind) { - return convertArrayToCollectionIfNecessary(valueToBind); - } - - /** - * Returns the given value as collection if it is an array or as is if not. - * - * @return - */ - private Object convertArrayToCollectionIfNecessary(Object value) { + public Object prepare(Object value) { if (!ObjectUtils.isArray(value)) { return value; @@ -472,19 +507,20 @@ class StringQuery { } /** - * Returns the type of the {@link LikeParameterBinding}. + * Returns the {@link Type} of the binding. * - * @return + * @return the type */ public Type getType() { return type; } /** - * Prepares the given raw value according to the like type. + * Prepares the given raw keyword according to the like type. * - * @param value + * @param keyword */ + @Override public Object prepare(Object value) { if (value == null) { diff --git a/src/main/java/org/springframework/data/jpa/repository/query/StringQueryParameterBinder.java b/src/main/java/org/springframework/data/jpa/repository/query/StringQueryParameterBinder.java index b5c61d614..b792d311a 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/StringQueryParameterBinder.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/StringQueryParameterBinder.java @@ -57,18 +57,8 @@ public class StringQueryParameterBinder extends ParameterBinder { @Override protected void bind(Query jpaQuery, JpaParameter methodParameter, Object value, int position) { - Object valueToBind = value; - - if (query.hasParameterBindings()) { - - ParameterBinding binding = getBindingFor(jpaQuery, position, methodParameter); - - if (binding != null) { - valueToBind = binding.prepare(valueToBind); - } - } - - super.bind(jpaQuery, methodParameter, valueToBind, position); + ParameterBinding binding = getBindingFor(jpaQuery, position, methodParameter); + super.bind(jpaQuery, methodParameter, binding.prepare(value), position); } /** @@ -77,7 +67,7 @@ public class StringQueryParameterBinder extends ParameterBinder { * @param jpaQuery must not be {@literal null}. * @param position * @param methodParameter must not be {@literal null}. - * @return the {@link LikeParameterBinding} for the given parameters or {@literal null} if none available. + * @return the {@link ParameterBinding} for the given parameters or {@literal null} if none available. */ private ParameterBinding getBindingFor(Query jpaQuery, int position, Parameter methodParameter) { @@ -93,6 +83,10 @@ public class StringQueryParameterBinder extends ParameterBinder { } } - return null; + // We should actually reject parameters unavailable, but as EclipseLink doesn't implement ….getParameter(int) for + // native queries correctly we need to fall back to an indexed parameter + // @see https://bugs.eclipse.org/bugs/show_bug.cgi?id=427892 + + return new ParameterBinding(position); } } diff --git a/src/test/java/org/springframework/data/jpa/infrastructure/EclipseLinkMetamodelIntegrationTests.java b/src/test/java/org/springframework/data/jpa/infrastructure/EclipseLinkMetamodelIntegrationTests.java index a1aabf9da..b8a4bfa19 100644 --- a/src/test/java/org/springframework/data/jpa/infrastructure/EclipseLinkMetamodelIntegrationTests.java +++ b/src/test/java/org/springframework/data/jpa/infrastructure/EclipseLinkMetamodelIntegrationTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2013 the original author or authors. + * Copyright 2013-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -15,6 +15,8 @@ */ package org.springframework.data.jpa.infrastructure; +import org.junit.Ignore; +import org.junit.Test; import org.springframework.test.context.ContextConfiguration; /** @@ -25,4 +27,13 @@ import org.springframework.test.context.ContextConfiguration; @ContextConfiguration("classpath:eclipselink.xml") public class EclipseLinkMetamodelIntegrationTests extends MetamodelIntegrationTests { + /** + * TODO: Remove, once https://bugs.eclipse.org/bugs/show_bug.cgi?id=427892 is fixed. + */ + @Test + @Ignore + @Override + public void canAccessParametersByIndexForNativeQueries() { + + } } diff --git a/src/test/java/org/springframework/data/jpa/infrastructure/MetamodelIntegrationTests.java b/src/test/java/org/springframework/data/jpa/infrastructure/MetamodelIntegrationTests.java index cb84f39da..9e5fecdf7 100644 --- a/src/test/java/org/springframework/data/jpa/infrastructure/MetamodelIntegrationTests.java +++ b/src/test/java/org/springframework/data/jpa/infrastructure/MetamodelIntegrationTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2013 the original author or authors. + * Copyright 2013-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,6 +20,7 @@ import static org.junit.Assert.*; import javax.persistence.EntityManager; import javax.persistence.PersistenceContext; +import javax.persistence.Query; import javax.persistence.criteria.CriteriaBuilder; import javax.persistence.criteria.CriteriaQuery; import javax.persistence.criteria.Path; @@ -42,8 +43,7 @@ import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; @ContextConfiguration({ "classpath:infrastructure.xml" }) public class MetamodelIntegrationTests { - @PersistenceContext - EntityManager em; + @PersistenceContext EntityManager em; @Test public void considersOneToOneAttributeAnAssociation() { @@ -66,4 +66,12 @@ public class MetamodelIntegrationTests { assertThat(path.getModel().getBindableType(), is(BindableType.ENTITY_TYPE)); } + + @Test + public void canAccessParametersByIndexForNativeQueries() { + + Query query = em.createNativeQuery("SELECT u from User u where u.lastname = ?1"); + + assertThat(query.getParameter(1), is(notNullValue())); + } } diff --git a/src/test/java/org/springframework/data/jpa/repository/query/LikeBindingUnitTests.java b/src/test/java/org/springframework/data/jpa/repository/query/LikeBindingUnitTests.java index 248eddf83..5c6e59942 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/LikeBindingUnitTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/LikeBindingUnitTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2013 the original author or authors. + * Copyright 2013-2014 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -23,7 +23,10 @@ import org.springframework.data.jpa.repository.query.StringQuery.LikeParameterBi import org.springframework.data.repository.query.parser.Part.Type; /** + * Unit tests for {@link LikeParameterBinding}. + * * @author Oliver Gierke + * @author Thomas Darimont */ public class LikeBindingUnitTests { 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 9373d625b..d9df93cc3 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 @@ -124,10 +124,11 @@ public class StringQueryUnitTests { assertThat(query.getQueryString(), is(queryString)); List bindings = query.getParameterBindings(); - assertThat(bindings, hasSize(2)); + assertThat(bindings, hasSize(3)); assertNamedBinding(InParameterBinding.class, "ids", bindings.get(0)); assertNamedBinding(InParameterBinding.class, "names", bindings.get(1)); + assertNamedBinding(ParameterBinding.class, "bar", bindings.get(2)); } /** @@ -161,10 +162,11 @@ public class StringQueryUnitTests { assertThat(query.getQueryString(), is(queryString)); List bindings = query.getParameterBindings(); - assertThat(bindings, hasSize(2)); + assertThat(bindings, hasSize(3)); assertPositionalBinding(InParameterBinding.class, 1, bindings.get(0)); assertPositionalBinding(InParameterBinding.class, 2, bindings.get(1)); + assertPositionalBinding(ParameterBinding.class, 3, bindings.get(2)); } /**