DATAJPA-491 - Support order by arbitrarily nested association paths with Querydsl.

Replaced custom left join generation logic with default Querydsl mechanisms including support for ordering by arbitrarily nested property paths. Added test cases that demonstrate ordering by nested association paths (>= 2 levels). Added additional test cases for sort by nested property path expressions based on querydsl meta model and plain string based path expressions.

Original pull request: #65.

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2013 the original author or authors.
+ * Copyright 2012-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.
@@ -16,17 +16,14 @@
 package org.springframework.data.jpa.repository.support;
 
 import java.util.ArrayList;
-import java.util.LinkedHashSet;
 import java.util.List;
-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;
 import org.springframework.data.domain.Sort.Order;
+import org.springframework.data.mapping.PropertyPath;
 import org.springframework.data.querydsl.QSort;
 import org.springframework.util.Assert;
 
@@ -38,8 +35,8 @@ 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.OrderSpecifier.NullHandling;
 import com.mysema.query.types.Path;
-import com.mysema.query.types.path.EntityPathBase;
 import com.mysema.query.types.path.PathBuilder;
 
 /**
@@ -170,7 +167,7 @@ public class Querydsl {
 
 		for (OrderSpecifier<?> order : originalOrderSpecifiers) {
 
-			Path targetPath = ((Path) order.getTarget()).getMetadata().getParent();
+			Path targetPath = ((Path<?>) order.getTarget()).getMetadata().getParent();
 
 			boolean targetPathRootIsEntityRoot = targetPath.getRoot().equals(builder.getRoot());
 			boolean targetPathEqualsRootEnityPath = targetPath.toString().equals(builder.toString());
@@ -226,78 +223,35 @@ public class Querydsl {
 	@SuppressWarnings({ "rawtypes", "unchecked" })
 	private OrderSpecifier<?> toOrderSpecifier(Order order, JPQLQuery query) {
 
-		Expression<?> property = createExpressionAndPotentionallyAddLeftJoinForReferencedAssociation(order, query);
-
 		return new OrderSpecifier(order.isAscending() ? com.mysema.query.types.Order.ASC
-				: com.mysema.query.types.Order.DESC, property);
+				: com.mysema.query.types.Order.DESC, buildOrderPropertyPathFrom(order), NullHandling.NullsLast);
 	}
 
 	/**
-	 * 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.
+	 * Creates an {@link Expression} for the given {@link Order} property.
 	 * 
 	 * @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()));
-	}
-
-	/**
-	 * Adds a left-join to the given {@link JPQLQuery} with a proper alias for the property referenced on the given
-	 * {@link Order} relative to the given parent {@link Attribute}.
-	 * 
-	 * @param parentAttribute must not be {@literal null}.
-	 * @param order must not be {@literal null}.
-	 * @param query must not be {@literal null}.
 	 * @return
 	 */
-	@SuppressWarnings({ "unchecked", "rawtypes" })
-	private Expression<?> createLeftJoinForAttributeInOrderBy(Attribute<?, ?> parentAttribute, Order order,
-			JPQLQuery query) {
+	private Expression<?> buildOrderPropertyPathFrom(Order order) {
 
-		Assert.notNull(parentAttribute, "Attribute must not be null!");
 		Assert.notNull(order, "Order must not be null!");
-		Assert.notNull(query, "Query must not be null!");
 
-		EntityPathBase<?> associationPathRoot = new EntityPathBase<Object>(parentAttribute.getJavaType(),
-				parentAttribute.getName());
-		query.leftJoin((EntityPath) builder.get(parentAttribute.getName()), associationPathRoot);
-		PathBuilder<Object> attributePathBuilder = new PathBuilder<Object>(parentAttribute.getJavaType(),
-				associationPathRoot.getMetadata());
+		PropertyPath path = PropertyPath.from(order.getProperty(), builder.getType());
+		Expression<?> sortPropertyExpression = builder;
 
-		String nestedAttributePath = order.getProperty().substring(parentAttribute.getName().length() + 1); // exclude "."
-		return order.isIgnoreCase() ? attributePathBuilder.getString(nestedAttributePath).lower() : attributePathBuilder
-				.get(nestedAttributePath);
+		while (path != null) {
+
+			if (!path.hasNext() && order.isIgnoreCase()) {
+				// if order is ignore-case we have to treat the last path segment as a String.
+				sortPropertyExpression = ((PathBuilder<?>) sortPropertyExpression).getString(path.getSegment()).lower();
+			} else {
+				sortPropertyExpression = ((PathBuilder<?>) sortPropertyExpression).get(path.getSegment());
+			}
+
+			path = path.next();
+		}
+
+		return sortPropertyExpression;
 	}
 }

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

@@ -16,8 +16,10 @@
 package org.springframework.data.jpa.domain.sample;
 
 import javax.persistence.Entity;
+import javax.persistence.FetchType;
 import javax.persistence.GeneratedValue;
 import javax.persistence.Id;
+import javax.persistence.ManyToOne;
 
 /**
  * @author Thomas Darimont
@@ -29,6 +31,8 @@ public class MailSender {
 
 	private String name;
 
+	@ManyToOne(fetch = FetchType.LAZY) private MailUser mailUser;
+
 	public MailSender() {}
 
 	public MailSender(String name) {
@@ -51,6 +55,14 @@ public class MailSender {
 		this.id = id;
 	}
 
+	public MailUser getMailUser() {
+		return mailUser;
+	}
+
+	public void setMailUser(MailUser mailUser) {
+		this.mailUser = mailUser;
+	}
+
 	/* 
 	 * (non-Javadoc)
 	 * @see java.lang.Object#hashCode()

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

@@ -0,0 +1,72 @@
+/*
+ * Copyright 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.
+ * 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.Entity;
+import javax.persistence.GeneratedValue;
+import javax.persistence.Id;
+
+/**
+ * Represents a user in a Mail-System.
+ * 
+ * @author Thomas Darimont
+ */
+@Entity
+public class MailUser {
+
+	@Id @GeneratedValue Long id;
+
+	String name;
+
+	public MailUser() {}
+
+	public MailUser(String name) {
+		this.name = name;
+	}
+
+	public String getName() {
+		return name;
+	}
+
+	public void setName(String name) {
+		this.name = name;
+	}
+
+	@Override
+	public int hashCode() {
+		final int prime = 31;
+		int result = 1;
+		result = prime * result + ((name == null) ? 0 : name.hashCode());
+		return result;
+	}
+
+	@Override
+	public boolean equals(Object obj) {
+		if (this == obj)
+			return true;
+		if (obj == null)
+			return false;
+		if (getClass() != obj.getClass())
+			return false;
+		MailUser other = (MailUser) obj;
+		if (name == null) {
+			if (other.name != null)
+				return false;
+		} else if (!name.equals(other.name))
+			return false;
+		return true;
+	}
+}

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2008-2011 the original author or authors.
+ * Copyright 2008-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,16 +15,22 @@
  */
 package org.springframework.data.jpa.domain.sample;
 
+import javax.persistence.Entity;
+import javax.persistence.GeneratedValue;
+import javax.persistence.Id;
+
 /**
  * Sample domain class representing roles. Mapped with XML.
  * 
  * @author Oliver Gierke
+ * @author Thomas Darimont
  */
+@Entity
 public class Role {
 
 	private static final String PREFIX = "ROLE_";
 
-	private Integer id;
+	@Id @GeneratedValue private Integer id;
 	private String name;
 
 	/**

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2008-2013 the original author or authors.
+ * Copyright 2008-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.
@@ -195,9 +195,9 @@ public class User {
 	/**
 	 * Returns the user's roles.
 	 * 
-	 * @return the role
+	 * @return the roles
 	 */
-	public Set<Role> getRole() {
+	public Set<Role> getRoles() {
 
 		return roles;
 	}

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2008-2013 the original author or authors.
+ * Copyright 2008-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.
@@ -85,6 +85,7 @@ public class UserRepositoryTests {
 	// Test fixture
 	User firstUser, secondUser, thirdUser, fourthUser;
 	Integer id;
+	Role adminRole;
 
 	@Before
 	public void setUp() throws Exception {
@@ -98,6 +99,7 @@ public class UserRepositoryTests {
 		thirdUser.setAge(43);
 		fourthUser = new User("kevin", "raymond", "no@gmail.com");
 		fourthUser.setAge(31);
+		adminRole = new Role("admin");
 	}
 
 	@Test
@@ -918,6 +920,8 @@ public class UserRepositoryTests {
 
 	protected void flushTestUsers() {
 
+		em.persist(adminRole);
+
 		firstUser = repository.save(firstUser);
 		secondUser = repository.save(secondUser);
 		thirdUser = repository.save(thirdUser);
@@ -1258,6 +1262,24 @@ public class UserRepositoryTests {
 		assertThat(user.getEmailAddress(), is(savedUser.getEmailAddress()));
 	}
 
+	/**
+	 * @see DATAJPA-491
+	 */
+	@Test
+	public void sortByNestedAssociationPropertyWithSortInPageable() {
+
+		firstUser.setManager(thirdUser);
+		thirdUser.setManager(fourthUser);
+
+		flushTestUsers();
+
+		Page<User> page = repository.findAll(new PageRequest(0, 10, //
+				new Sort(Sort.Direction.ASC, "manager.manager.firstname")));
+
+		assertThat(page.getContent(), hasSize(4));
+		assertThat(page.getContent().get(3), is(firstUser));
+	}
+
 	private Page<User> executeSpecWithSort(Sort sort) {
 
 		flushTestUsers();

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

@@ -21,17 +21,22 @@ import static org.springframework.data.jpa.domain.JpaSort.*;
 
 import java.util.List;
 
+import javax.persistence.EntityManager;
+import javax.persistence.PersistenceContext;
+
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.data.domain.Page;
 import org.springframework.data.domain.PageRequest;
+import org.springframework.data.domain.Sort;
 import org.springframework.data.domain.Sort.Direction;
 import org.springframework.data.jpa.domain.JpaSort;
 import org.springframework.data.jpa.domain.sample.MailMessage;
 import org.springframework.data.jpa.domain.sample.MailMessage_;
 import org.springframework.data.jpa.domain.sample.MailSender;
 import org.springframework.data.jpa.domain.sample.MailSender_;
+import org.springframework.data.jpa.domain.sample.MailUser;
 import org.springframework.data.jpa.domain.sample.QMailMessage;
 import org.springframework.data.jpa.domain.sample.QMailSender;
 import org.springframework.data.jpa.repository.sample.MailMessageRepository;
@@ -54,6 +59,8 @@ public class MailMessageRepositoryIntegrationTests {
 	static final QMailMessage message = QMailMessage.mailMessage;
 	static final QMailSender sender = QMailSender.mailSender;
 
+	@PersistenceContext EntityManager em;
+
 	@Autowired MailMessageRepository mailMessageRepository;
 
 	/**
@@ -99,7 +106,68 @@ public class MailMessageRepositoryIntegrationTests {
 		mailMessageRepository.save(message1);
 		mailMessageRepository.save(message2);
 
-		List<MailMessage> messages = mailMessageRepository.findAll(message.content.eq("abc"), message.mailSender.name.asc());
+		List<MailMessage> messages = mailMessageRepository
+				.findAll(message.content.eq("abc"), message.mailSender.name.asc());
+
+		assertThat(messages, hasSize(2));
+		assertThat(messages.get(0).getMailSender(), is(nullValue()));
+		assertThat(messages.get(1).getMailSender(), is(sender1));
+	}
+
+	/**
+	 * @see DATAJPA-491
+	 */
+	@Test
+	public void shouldSortMailWithNestedQueryDslSortCriteriaNullsFirst() {
+
+		MailUser fooMailUser = new MailUser("foo");
+		em.persist(fooMailUser);
+
+		MailMessage message1 = new MailMessage();
+		message1.setContent("abc");
+		MailSender sender1 = new MailSender("foo");
+		sender1.setMailUser(fooMailUser);
+		message1.setMailSender(sender1);
+
+		MailMessage message2 = new MailMessage();
+		message2.setContent("abc");
+
+		mailMessageRepository.save(message1);
+		mailMessageRepository.save(message2);
+
+		List<MailMessage> messages = mailMessageRepository.findAll(message.content.eq("abc"),
+				message.mailSender.mailUser.name.asc());
+
+		assertThat(messages, hasSize(2));
+		assertThat(messages.get(0).getMailSender(), is(nullValue()));
+		assertThat(messages.get(1).getMailSender(), is(sender1));
+	}
+
+	/**
+	 * @see DATAJPA-491
+	 */
+	@Test
+	public void shouldSortMailWithNestedStringBasedSortCriteriaNullsFirst() {
+
+		MailUser fooMailUser = new MailUser("foo");
+		em.persist(fooMailUser);
+
+		MailMessage message1 = new MailMessage();
+		message1.setContent("abc");
+		MailSender sender1 = new MailSender("foo");
+		sender1.setMailUser(fooMailUser);
+		message1.setMailSender(sender1);
+
+		MailMessage message2 = new MailMessage();
+		message2.setContent("abc");
+
+		mailMessageRepository.save(message1);
+		mailMessageRepository.save(message2);
+
+		Page<MailMessage> page = mailMessageRepository.findAll(new PageRequest(0, 10, new Sort(Sort.Direction.ASC,
+				"mailSender.mailUser.name")));
+
+		List<MailMessage> messages = page.getContent();
 
 		assertThat(messages, hasSize(2));
 		assertThat(messages.get(0).getMailSender(), is(nullValue()));

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2008-2011 the original author or authors.
+ * Copyright 2008-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.
@@ -33,6 +33,7 @@ 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.Role;
 import org.springframework.data.jpa.domain.sample.User;
 import org.springframework.data.querydsl.QPageRequest;
 import org.springframework.data.querydsl.QSort;
@@ -49,6 +50,7 @@ import com.mysema.query.types.path.PathBuilderFactory;
  * Integration test for {@link QueryDslJpaRepository}.
  * 
  * @author Oliver Gierke
+ * @author Thomas Darimont
  */
 @RunWith(SpringJUnit4ClassRunner.class)
 @ContextConfiguration({ "classpath:infrastructure.xml" })
@@ -60,6 +62,7 @@ public class QueryDslJpaRepositoryTests {
 	QueryDslJpaRepository<User, Integer> repository;
 	QUser user = new QUser("user");
 	User dave, carter, oliver;
+	Role adminRole;
 
 	@Before
 	public void setUp() {
@@ -71,6 +74,7 @@ public class QueryDslJpaRepositoryTests {
 		dave = repository.save(new User("Dave", "Matthews", "dave@matthews.com"));
 		carter = repository.save(new User("Carter", "Beauford", "carter@beauford.com"));
 		oliver = repository.save(new User("Oliver", "matthews", "oliver@matthews.com"));
+		adminRole = em.merge(new Role("admin"));
 	}
 
 	@Test
@@ -273,4 +277,20 @@ public class QueryDslJpaRepositoryTests {
 		assertThat(page.getContent().get(1), is(dave));
 		assertThat(page.getContent().get(2), is(oliver));
 	}
+
+	/**
+	 * @see DATAJPA-491
+	 */
+	@Test
+	public void sortByNestedAssociationPropertyWithSpecificationAndSortInPageable() {
+
+		oliver.setManager(dave);
+		dave.getRoles().add(adminRole);
+
+		Page<User> page = repository.findAll(QUser.user.id.gt(0), new PageRequest(0, 10, //
+				new Sort(Sort.Direction.ASC, "manager.roles.name")));
+
+		assertThat(page.getContent(), hasSize(3));
+		assertThat(page.getContent().get(0), is(dave));
+	}
 }

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

@@ -30,12 +30,14 @@
 		<class>org.springframework.data.jpa.domain.sample.Address</class>
 		<class>org.springframework.data.jpa.domain.sample.MailMessage</class>
 		<class>org.springframework.data.jpa.domain.sample.MailSender</class>
+		<class>org.springframework.data.jpa.domain.sample.MailUser</class>
 		<exclude-unlisted-classes>true</exclude-unlisted-classes>
 	</persistence-unit>
 	<persistence-unit name="querydsl">
 		<class>org.springframework.data.jpa.domain.sample.User</class>
 		<class>org.springframework.data.jpa.domain.sample.MailMessage</class>
 		<class>org.springframework.data.jpa.domain.sample.MailSender</class>
+		<class>org.springframework.data.jpa.domain.sample.MailUser</class>
 		<exclude-unlisted-classes>true</exclude-unlisted-classes>
 	</persistence-unit>
 	<persistence-unit name="cdi">
@@ -44,6 +46,7 @@
 		<class>org.springframework.data.jpa.repository.cdi.Person</class>
 		<class>org.springframework.data.jpa.domain.sample.MailMessage</class>
 		<class>org.springframework.data.jpa.domain.sample.MailSender</class>
+		<class>org.springframework.data.jpa.domain.sample.MailUser</class>
 		<exclude-unlisted-classes>true</exclude-unlisted-classes>
 		<properties>
 			<property name="hibernate.connection.username" value="sa" />
@@ -74,6 +77,7 @@
 		<class>org.springframework.data.jpa.domain.sample.User</class>
 		<class>org.springframework.data.jpa.domain.sample.MailMessage</class>
 		<class>org.springframework.data.jpa.domain.sample.MailSender</class>
+		<class>org.springframework.data.jpa.domain.sample.MailUser</class>
 		<class>org.springframework.data.jpa.repository.support.JpaMetamodelEntityInformationIntegrationTests$Sample</class>
 		<exclude-unlisted-classes>true</exclude-unlisted-classes>
 		<properties>
@@ -85,6 +89,7 @@
 		<class>org.springframework.data.jpa.domain.sample.User</class>
 		<class>org.springframework.data.jpa.domain.sample.MailMessage</class>
 		<class>org.springframework.data.jpa.domain.sample.MailSender</class>
+		<class>org.springframework.data.jpa.domain.sample.MailUser</class>
 		<class>org.springframework.data.jpa.repository.support.JpaMetamodelEntityInformationIntegrationTests$Sample</class>
 		<exclude-unlisted-classes>true</exclude-unlisted-classes>
 	</persistence-unit>
@@ -93,6 +98,7 @@
 		<class>org.springframework.data.jpa.domain.sample.User</class>
 		<class>org.springframework.data.jpa.domain.sample.MailMessage</class>
 		<class>org.springframework.data.jpa.domain.sample.MailSender</class>
+		<class>org.springframework.data.jpa.domain.sample.MailUser</class>
 		<class>org.springframework.data.jpa.repository.support.JpaMetamodelEntityInformationIntegrationTests$Sample</class>
 		<exclude-unlisted-classes>true</exclude-unlisted-classes>
 		<properties>

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

@@ -8,6 +8,7 @@
 		<class>org.springframework.data.jpa.domain.sample.Role</class>
 		<class>org.springframework.data.jpa.domain.sample.MailMessage</class>
 		<class>org.springframework.data.jpa.domain.sample.MailSender</class>
+		<class>org.springframework.data.jpa.domain.sample.MailUser</class>
 		<exclude-unlisted-classes>true</exclude-unlisted-classes>
 	</persistence-unit>
 	<persistence-unit name="second">
@@ -18,6 +19,7 @@
 		<class>org.springframework.data.jpa.domain.sample.AuditableRole</class>
 		<class>org.springframework.data.jpa.domain.sample.MailMessage</class>
 		<class>org.springframework.data.jpa.domain.sample.MailSender</class>
+		<class>org.springframework.data.jpa.domain.sample.MailUser</class>
 		<exclude-unlisted-classes>true</exclude-unlisted-classes>
 	</persistence-unit>
 </persistence>