DATAJPA-427 - Generate left joins for referenced associations in sort expressions.

Previously sorting by property of an associated object generated an inner join instead of a left join with QueryDsl and Hibernate. That excluded records that had null values on their join columns. We now generate appropriate left joins if we detect associations in the sort property expression.

Ignored test cases for EclipseLink since eclipse link generates an inner-join instead of an outer-join to fetch associations in order by. Filed: https://bugs.eclipse.org/bugs/show_bug.cgi?id=422450

Original pull request: #53.

src/main/java/org/springframework/data/jpa/repository/support/Querydsl.java

@@ -15,7 +15,12 @@
  */
 package org.springframework.data.jpa.repository.support;
 
+import java.util.LinkedHashSet;
+import java.util.Set;
+
 import javax.persistence.EntityManager;
+import javax.persistence.metamodel.Attribute;
+import javax.persistence.metamodel.EntityType;
 
 import org.springframework.data.domain.Pageable;
 import org.springframework.data.domain.Sort;
@@ -30,6 +35,7 @@ import com.mysema.query.jpa.impl.JPAQuery;
 import com.mysema.query.types.EntityPath;
 import com.mysema.query.types.Expression;
 import com.mysema.query.types.OrderSpecifier;
+import com.mysema.query.types.path.EntityPathBase;
 import com.mysema.query.types.path.PathBuilder;
 
 /**
@@ -121,7 +127,7 @@ public class Querydsl {
 		}
 
 		for (Order order : sort) {
-			query.orderBy(toOrder(order));
+			query.orderBy(toOrder(order, query));
 		}
 
 		return query;
@@ -134,16 +140,71 @@ public class Querydsl {
 	 * @return
 	 */
 	@SuppressWarnings({ "rawtypes", "unchecked" })
-	private OrderSpecifier<?> toOrder(Order order) {
+	private OrderSpecifier<?> toOrder(Order order, JPQLQuery query) {
 
-		Expression<?> property = builder.get(order.getProperty());
-
-		// Apply ignore case in case we have a String and ignore case ordering is requested
-		if (order.isIgnoreCase()) {
-			property = builder.getString(order.getProperty()).lower();
-		}
+		Expression<?> property = createExpressionAndPotentionallyAddLeftJoinForReferencedAssociation(order, query);
 
 		return new OrderSpecifier(order.isAscending() ? com.mysema.query.types.Order.ASC
 				: com.mysema.query.types.Order.DESC, property);
 	}
+
+	/**
+	 * Potentially adds a left join to the given {@link JPQLQuery} query if the order contains a property path that uses
+	 * an association and returns the property expression build from the path of the association.
+	 * 
+	 * @param order must not be {@literal null}.
+	 * @param query must not be {@literal null}.
+	 * @return property expression.
+	 */
+	private Expression<?> createExpressionAndPotentionallyAddLeftJoinForReferencedAssociation(Order order, JPQLQuery query) {
+
+		Assert.notNull(order, "Order must not be null!");
+		Assert.notNull(query, "JPQLQuery must not be null!");
+
+		if (!order.getProperty().contains(".")) {
+			// Apply ignore case in case we have a String and ignore case ordering is requested
+			return order.isIgnoreCase() ? builder.getString(order.getProperty()).lower() : builder.get(order.getProperty());
+		}
+
+		EntityType<?> entitytype = em.getMetamodel().entity(builder.getType());
+
+		Set<Attribute<?, ?>> combinedAttributes = new LinkedHashSet<Attribute<?, ?>>();
+		combinedAttributes.addAll(entitytype.getSingularAttributes());
+		combinedAttributes.addAll(entitytype.getPluralAttributes());
+
+		for (Attribute<?, ?> attribute : combinedAttributes) {
+
+			if (order.getProperty().startsWith(attribute.getName() + ".")) {
+
+				switch (attribute.getPersistentAttributeType()) {
+					case EMBEDDED:
+						return builder.get(order.getProperty());
+					default:
+						return createLeftJoinForAttributeInOrderBy(attribute, order, query);
+				}
+			}
+		}
+
+		throw new IllegalArgumentException(
+				String.format("Could not create property expression for %s", order.getProperty()));
+	}
+
+	/**
+	 * @param attribute
+	 * @param order
+	 * @param query
+	 * @return
+	 */
+	@SuppressWarnings({ "unchecked", "rawtypes" })
+	private Expression<?> createLeftJoinForAttributeInOrderBy(Attribute<?, ?> attribute, Order order, JPQLQuery query) {
+
+		EntityPathBase<?> associationPathRoot = new EntityPathBase<Object>(attribute.getJavaType(), attribute.getName());
+		query.leftJoin((EntityPath) builder.get(attribute.getName()), associationPathRoot);
+		PathBuilder<Object> attributePathBuilder = new PathBuilder<Object>(attribute.getJavaType(),
+				associationPathRoot.getMetadata());
+
+		String nestedAttributePath = order.getProperty().substring(attribute.getName().length() + 1); // exclude "."
+		return order.isIgnoreCase() ? attributePathBuilder.getString(nestedAttributePath).lower() : attributePathBuilder
+				.get(nestedAttributePath);
+	}
 }

src/test/java/org/springframework/data/jpa/domain/sample/Address.java

@@ -0,0 +1,55 @@
+/*
+ * Copyright 2013 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.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.springframework.data.jpa.domain.sample;
+
+import javax.persistence.Embeddable;
+
+/**
+ * @author Thomas Darimont
+ */
+@Embeddable
+public class Address {
+
+	private String country;
+	private String city;
+	private String streetName;
+	private String streetNo;
+
+	public Address() {}
+
+	public Address(String country, String city, String streetName, String streetNo) {
+		this.country = country;
+		this.city = city;
+		this.streetName = streetName;
+		this.streetNo = streetNo;
+	}
+
+	public String getCountry() {
+		return country;
+	}
+
+	public String getCity() {
+		return city;
+	}
+
+	public String getStreetName() {
+		return streetName;
+	}
+
+	public String getStreetNo() {
+		return streetNo;
+	}
+}

src/test/java/org/springframework/data/jpa/domain/sample/User.java

@@ -21,6 +21,7 @@ import java.util.Set;
 
 import javax.persistence.CascadeType;
 import javax.persistence.Column;
+import javax.persistence.Embedded;
 import javax.persistence.Entity;
 import javax.persistence.GeneratedValue;
 import javax.persistence.GenerationType;
@@ -63,6 +64,8 @@ public class User {
 	@ManyToOne
 	private User manager;
 
+	@Embedded private Address address;
+
 	/**
 	 * Creates a new empty instance of {@code User}.
 	 */
@@ -281,6 +284,20 @@ public class User {
 		return createdAt;
 	}
 
+	/**
+	 * @return the address
+	 */
+	public Address getAddress() {
+		return address;
+	}
+
+	/**
+	 * @param address the address to set
+	 */
+	public void setAddress(Address address) {
+		this.address = address;
+	}
+
 	/*
 	 * (non-Javadoc)
 	 * 

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

@@ -63,4 +63,16 @@ public class EclipseLinkNamespaceUserRepositoryTests extends NamespaceUserReposi
 	public void shouldGenerateLeftOuterJoinInfindAllWithPaginationAndSortOnNestedPropertyPath() {
 		super.shouldGenerateLeftOuterJoinInfindAllWithPaginationAndSortOnNestedPropertyPath();
 	}
+
+	/**
+	 * Ignored until https://bugs.eclipse.org/bugs/show_bug.cgi?id=422450 is resolved.
+	 */
+	@Override
+	public void sortByAssociationPropertyShouldUseLeftOuterJoin() {}
+
+	/**
+	 * Ignored until https://bugs.eclipse.org/bugs/show_bug.cgi?id=422450 is resolved.
+	 */
+	@Override
+	public void sortByAssociationPropertyInPageableShouldUseLeftOuterJoin() {}
 }

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

@@ -53,6 +53,7 @@ import org.springframework.data.domain.Sort;
 import org.springframework.data.domain.Sort.Direction;
 import org.springframework.data.domain.Sort.Order;
 import org.springframework.data.jpa.domain.Specification;
+import org.springframework.data.jpa.domain.sample.Address;
 import org.springframework.data.jpa.domain.sample.Role;
 import org.springframework.data.jpa.domain.sample.User;
 import org.springframework.data.jpa.repository.sample.UserRepository;
@@ -68,6 +69,7 @@ import org.springframework.transaction.annotation.Transactional;
  * 
  * @author Oliver Gierke
  * @author Kevin Raymond
+ * @author Thomas Darimont
  */
 @RunWith(SpringJUnit4ClassRunner.class)
 @ContextConfiguration("classpath:application-context.xml")
@@ -1107,6 +1109,51 @@ public class UserRepositoryTests {
 		assertThat(result, contains(secondUser, firstUser, thirdUser, fourthUser));
 	}
 
+	/**
+	 * @see DATAJPA-427
+	 */
+	@Test
+	public void sortByAssociationPropertyShouldUseLeftOuterJoin() {
+
+		secondUser.getColleagues().add(firstUser);
+		fourthUser.getColleagues().add(thirdUser);
+		flushTestUsers();
+
+		List<User> result = repository.findAll(new Sort(Sort.Direction.ASC, "colleagues.id"));
+
+		assertThat(result, hasSize(4));
+	}
+
+	/**
+	 * @see DATAJPA-427
+	 */
+	@Test
+	public void sortByAssociationPropertyInPageableShouldUseLeftOuterJoin() {
+
+		secondUser.getColleagues().add(firstUser);
+		fourthUser.getColleagues().add(thirdUser);
+		flushTestUsers();
+
+		Page<User> page = repository.findAll(new PageRequest(0, 10, new Sort(Sort.Direction.ASC, "colleagues.id")));
+
+		assertThat(page.getContent(), hasSize(4));
+	}
+
+	/**
+	 * @see DATAJPA-427
+	 */
+	@Test
+	public void sortByEmbeddedProperty() {
+
+		thirdUser.setAddress(new Address("Germany", "Saarbrücken", "HaveItYourWay", "123"));
+		flushTestUsers();
+
+		Page<User> page = repository.findAll(new PageRequest(0, 10, new Sort(Sort.Direction.ASC, "address.streetName")));
+
+		assertThat(page.getContent(), hasSize(4));
+		assertThat(page.getContent().get(3), is(thirdUser));
+	}
+
 	private Page<User> executeSpecWithSort(Sort sort) {
 
 		flushTestUsers();

src/test/java/org/springframework/data/jpa/repository/support/QueryDslJpaRepositoryTests.java

@@ -31,6 +31,7 @@ import org.springframework.data.domain.PageRequest;
 import org.springframework.data.domain.Sort;
 import org.springframework.data.domain.Sort.Direction;
 import org.springframework.data.domain.Sort.Order;
+import org.springframework.data.jpa.domain.sample.Address;
 import org.springframework.data.jpa.domain.sample.QUser;
 import org.springframework.data.jpa.domain.sample.User;
 import org.springframework.test.context.ContextConfiguration;
@@ -52,8 +53,7 @@ import com.mysema.query.types.path.PathBuilderFactory;
 @Transactional
 public class QueryDslJpaRepositoryTests {
 
-	@PersistenceContext
-	EntityManager em;
+	@PersistenceContext EntityManager em;
 
 	QueryDslJpaRepository<User, Integer> repository;
 	QUser user = new QUser("user");
@@ -131,4 +131,88 @@ public class QueryDslJpaRepositoryTests {
 		assertThat(result.getContent().get(0), is(dave));
 		assertThat(result.getContent().get(1), is(oliver));
 	}
+
+	/**
+	 * @see DATAJPA-427
+	 */
+	@Test
+	public void findBySpecificationWithSortByPluralAssociationPropertyInPageableShouldUseSortNullValuesLast() {
+
+		oliver.getColleagues().add(dave);
+		dave.getColleagues().add(oliver);
+
+		QUser user = QUser.user;
+
+		Page<User> page = repository.findAll(user.firstname.isNotNull(), new PageRequest(0, 10, new Sort(
+				Sort.Direction.ASC, "colleagues.firstname")));
+
+		assertThat(page.getContent(), hasSize(3));
+		assertThat(page.getContent(), hasItems(oliver, dave, carter));
+	}
+
+	/**
+	 * @see DATAJPA-427
+	 */
+	@Test
+	public void findBySpecificationWithSortBySingularAssociationPropertyInPageableShouldUseSortNullValuesLast() {
+
+		oliver.setManager(dave);
+		dave.setManager(carter);
+
+		QUser user = QUser.user;
+
+		Page<User> page = repository.findAll(user.firstname.isNotNull(), new PageRequest(0, 10, new Sort(
+				Sort.Direction.ASC, "manager.firstname")));
+
+		assertThat(page.getContent(), hasSize(3));
+		assertThat(page.getContent(), hasItems(dave, oliver, carter));
+	}
+
+	/**
+	 * @see DATAJPA-427
+	 */
+	@Test
+	public void findBySpecificationWithSortBySingularPropertyInPageableShouldUseSortNullValuesFirst() {
+
+		QUser user = QUser.user;
+
+		Page<User> page = repository.findAll(user.firstname.isNotNull(), new PageRequest(0, 10, new Sort(
+				Sort.Direction.ASC, "firstname")));
+
+		assertThat(page.getContent(), hasSize(3));
+		assertThat(page.getContent(), hasItems(carter, dave, oliver));
+	}
+
+	/**
+	 * @see DATAJPA-427
+	 */
+	@Test
+	public void findBySpecificationWithSortByOrderIgnoreCaseBySingularPropertyInPageableShouldUseSortNullValuesFirst() {
+
+		QUser user = QUser.user;
+
+		Page<User> page = repository.findAll(user.firstname.isNotNull(), new PageRequest(0, 10, new Sort(new Order(
+				Sort.Direction.ASC, "firstname").ignoreCase())));
+
+		assertThat(page.getContent(), hasSize(3));
+		assertThat(page.getContent(), hasItems(carter, dave, oliver));
+	}
+
+	/**
+	 * @see DATAJPA-427
+	 */
+	@Test
+	public void findBySpecificationWithSortByNestedEmbeddedPropertyInPageableShouldUseSortNullValuesFirst() {
+
+		oliver.setAddress(new Address("Germany", "Saarbrücken", "HaveItYourWay", "123"));
+
+		QUser user = QUser.user;
+
+		Page<User> page = repository.findAll(user.firstname.isNotNull(), new PageRequest(0, 10, new Sort(
+				Sort.Direction.ASC, "address.streetName")));
+
+		assertThat(page.getContent(), hasSize(3));
+		assertThat(page.getContent(), hasItems(dave, carter, oliver));
+		assertThat(page.getContent().get(2), is(oliver));
+	}
 }

src/test/resources/META-INF/persistence.xml

@@ -26,6 +26,7 @@
 		<class>org.springframework.data.jpa.domain.sample.EmbeddedIdExampleDepartment</class>
 		<class>org.springframework.data.jpa.domain.sample.Customer</class>
 		<class>org.springframework.data.jpa.domain.sample.Order</class>
+		<class>org.springframework.data.jpa.domain.sample.Address</class>
 		<exclude-unlisted-classes>true</exclude-unlisted-classes>
 	</persistence-unit>
 	<persistence-unit name="querydsl">