DATAJPA-277 - Specification execution with sorting now uses left join…

…s 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.

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.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<PersistentAttributeType> 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<PersistentAttributeType> persistentAttributeTypes = new HashSet<PersistentAttributeType>();
+		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 @@ private static javax.persistence.criteria.Order toJpaOrder(Order order, Root<?>
 	@SuppressWarnings("unchecked")
 	static <T> Expression<T> toExpressionRecursively(From<?, ?> from, PropertyPath property) {
 
-		if (property.isCollection()) {
-			Join<Object, Object> join = from.join(property.getSegment());
+		Path<Object> path = from.get(property.getSegment());
+
+		if (property.isCollection() || isEntityPath(path)) {
+			Join<Object, Object> join = from.join(property.getSegment(), JoinType.LEFT);
 			return (Expression<T>) (property.hasNext() ? toExpressionRecursively((From<?, ?>) join, property.next()) : join);
 		} else {
-			Path<Object> path = from.get(property.getSegment());
 			return (Expression<T>) (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<Object> toExpressionRecursively(Path<Object> path, PropertyPath property) {
 
 		Path<Object> result = path.get(property.getSegment());

src/test/java/org/springframework/data/jpa/repository/EclipseLinkNamespaceUserRepositoryTests.java

@@ -51,4 +51,8 @@ public void handlesIterableOfIdsCorrectly() {
 	public void allowsExecutingPageableMethodWithNullPageable() {
 
 	}
+
+	@Override
+	public void doesNotDropNullValuesOnPagedSpecificationExecution() {
+	}
 }

src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java

@@ -31,6 +31,10 @@
 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 void bindsSortingToOuterJoinCorrectly() {
 		assertThat(result.getContent(), hasSize((int) repository.count()));
 	}
 
+	/**
+	 * @see DATAJPA-277
+	 */
+	@Test
+	public void doesNotDropNullValuesOnPagedSpecificationExecution() {
+
+		flushTestUsers();
+
+		Page<User> page = repository.findAll(new Specification<User>() {
+			public Predicate toPredicate(Root<User> 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<User> executeSpecWithSort(Sort sort) {
 
 		flushTestUsers();