From bc6e2a7bc9093a40e9bc56c44498752fefa25254 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Thu, 17 Jan 2013 13:56:43 +0100 Subject: [PATCH] DATAJPA-277 - Specification execution with sorting now uses left joins where necessary. When applying sorting options to a Specification based query we have to make sure to apply left joins where necessary as we otherwise exclude results with null associations. Applying the sort options does an enhanced inspection of the target path and declares a left join instead of a get if the path logically points to an entity. The Metamodel API is not without caveats here, see http://bit.ly/10geC02. --- .../data/jpa/repository/query/QueryUtils.java | 52 +++++++++++++++++-- ...lipseLinkNamespaceUserRepositoryTests.java | 4 ++ .../jpa/repository/UserRepositoryTests.java | 22 ++++++++ 3 files changed, 75 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java b/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java index 6f0dc6037..f6121cac9 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java @@ -16,8 +16,10 @@ package org.springframework.data.jpa.repository.query; import static java.util.regex.Pattern.*; +import static javax.persistence.metamodel.Attribute.PersistentAttributeType.*; import java.util.ArrayList; +import java.util.Collections; import java.util.HashSet; import java.util.Iterator; import java.util.List; @@ -34,8 +36,13 @@ import javax.persistence.criteria.CriteriaBuilder; import javax.persistence.criteria.Expression; import javax.persistence.criteria.From; import javax.persistence.criteria.Join; +import javax.persistence.criteria.JoinType; import javax.persistence.criteria.Path; import javax.persistence.criteria.Root; +import javax.persistence.metamodel.Attribute; +import javax.persistence.metamodel.Attribute.PersistentAttributeType; +import javax.persistence.metamodel.Bindable; +import javax.persistence.metamodel.Bindable.BindableType; import org.springframework.data.domain.Sort; import org.springframework.data.domain.Sort.Order; @@ -67,6 +74,8 @@ public abstract class QueryUtils { private static final String LEFT_JOIN = "left (outer )?join " + IDENTIFIER + " (as )?" + IDENTIFIER_GROUP; private static final Pattern LEFT_JOIN_PATTERN = Pattern.compile(LEFT_JOIN, Pattern.CASE_INSENSITIVE); + private static final Set ASSOCIATION_TYPES; + static { StringBuilder builder = new StringBuilder(); @@ -87,6 +96,14 @@ public abstract class QueryUtils { builder.append("(.*)"); COUNT_MATCH = compile(builder.toString(), CASE_INSENSITIVE); + + Set persistentAttributeTypes = new HashSet(); + persistentAttributeTypes.add(ONE_TO_ONE); + persistentAttributeTypes.add(ONE_TO_MANY); + persistentAttributeTypes.add(MANY_TO_ONE); + persistentAttributeTypes.add(MANY_TO_MANY); + + ASSOCIATION_TYPES = Collections.unmodifiableSet(persistentAttributeTypes); } /** @@ -365,15 +382,44 @@ public abstract class QueryUtils { @SuppressWarnings("unchecked") static Expression toExpressionRecursively(From from, PropertyPath property) { - if (property.isCollection()) { - Join join = from.join(property.getSegment()); + Path path = from.get(property.getSegment()); + + if (property.isCollection() || isEntityPath(path)) { + Join join = from.join(property.getSegment(), JoinType.LEFT); return (Expression) (property.hasNext() ? toExpressionRecursively((From) join, property.next()) : join); } else { - Path path = from.get(property.getSegment()); return (Expression) (property.hasNext() ? toExpressionRecursively(path, property.next()) : path); } } + /** + * Returns whether the given path can be considered referring an entity. + * + * @param path must not be {@literal null}. + * @return + */ + private static boolean isEntityPath(Path path) { + + Bindable model = path.getModel(); + + if (BindableType.ENTITY_TYPE.equals(model.getBindableType())) { + return true; + } + + if (model instanceof Attribute) { + + Attribute attribute = (Attribute) model; + + if (attribute.isAssociation()) { + return true; + } + + return ASSOCIATION_TYPES.contains(attribute.getPersistentAttributeType()); + } + + return false; + } + static Expression toExpressionRecursively(Path path, PropertyPath property) { Path result = path.get(property.getSegment()); diff --git a/src/test/java/org/springframework/data/jpa/repository/EclipseLinkNamespaceUserRepositoryTests.java b/src/test/java/org/springframework/data/jpa/repository/EclipseLinkNamespaceUserRepositoryTests.java index 3117eb5b8..f635d12d5 100644 --- a/src/test/java/org/springframework/data/jpa/repository/EclipseLinkNamespaceUserRepositoryTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/EclipseLinkNamespaceUserRepositoryTests.java @@ -51,4 +51,8 @@ public class EclipseLinkNamespaceUserRepositoryTests extends NamespaceUserReposi public void allowsExecutingPageableMethodWithNullPageable() { } + + @Override + public void doesNotDropNullValuesOnPagedSpecificationExecution() { + } } 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 7221c8e6c..1a2c66dd1 100644 --- a/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java @@ -31,6 +31,10 @@ import java.util.Set; 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.Predicate; +import javax.persistence.criteria.Root; import org.hamcrest.Matchers; import org.junit.Before; @@ -948,6 +952,24 @@ public class UserRepositoryTests { assertThat(result.getContent(), hasSize((int) repository.count())); } + /** + * @see DATAJPA-277 + */ + @Test + public void doesNotDropNullValuesOnPagedSpecificationExecution() { + + flushTestUsers(); + + Page page = repository.findAll(new Specification() { + public Predicate toPredicate(Root root, CriteriaQuery query, CriteriaBuilder cb) { + return cb.equal(root.get("lastname"), "Gierke"); + } + }, new PageRequest(0, 20, new Sort("manager.lastname"))); + + assertThat(page.getNumberOfElements(), is(1)); + assertThat(page, hasItem(firstUser)); + } + private Page executeSpecWithSort(Sort sort) { flushTestUsers();