From 500cd315d15752238df88eb16761761ba27a3a21 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Wed, 4 Dec 2013 14:04:15 +0100 Subject: [PATCH] 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. --- .../data/jpa/repository/support/Querydsl.java | 77 ++++++++++++++-- .../data/jpa/domain/sample/Address.java | 55 ++++++++++++ .../data/jpa/domain/sample/User.java | 17 ++++ ...lipseLinkNamespaceUserRepositoryTests.java | 12 +++ .../jpa/repository/UserRepositoryTests.java | 47 ++++++++++ .../support/QueryDslJpaRepositoryTests.java | 88 ++++++++++++++++++- src/test/resources/META-INF/persistence.xml | 1 + 7 files changed, 287 insertions(+), 10 deletions(-) create mode 100644 src/test/java/org/springframework/data/jpa/domain/sample/Address.java diff --git a/src/main/java/org/springframework/data/jpa/repository/support/Querydsl.java b/src/main/java/org/springframework/data/jpa/repository/support/Querydsl.java index 1186ba9fd..6efedc31d 100644 --- a/src/main/java/org/springframework/data/jpa/repository/support/Querydsl.java +++ b/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> combinedAttributes = new LinkedHashSet>(); + 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(attribute.getJavaType(), attribute.getName()); + query.leftJoin((EntityPath) builder.get(attribute.getName()), associationPathRoot); + PathBuilder attributePathBuilder = new PathBuilder(attribute.getJavaType(), + associationPathRoot.getMetadata()); + + String nestedAttributePath = order.getProperty().substring(attribute.getName().length() + 1); // exclude "." + return order.isIgnoreCase() ? attributePathBuilder.getString(nestedAttributePath).lower() : attributePathBuilder + .get(nestedAttributePath); + } } diff --git a/src/test/java/org/springframework/data/jpa/domain/sample/Address.java b/src/test/java/org/springframework/data/jpa/domain/sample/Address.java new file mode 100644 index 000000000..e7b93eca1 --- /dev/null +++ b/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; + } +} diff --git a/src/test/java/org/springframework/data/jpa/domain/sample/User.java b/src/test/java/org/springframework/data/jpa/domain/sample/User.java index a9022d1ac..0a99c7678 100644 --- a/src/test/java/org/springframework/data/jpa/domain/sample/User.java +++ b/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) * 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 7327b1dcf..5e9d99450 100644 --- a/src/test/java/org/springframework/data/jpa/repository/EclipseLinkNamespaceUserRepositoryTests.java +++ b/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() {} } 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 dde7a5828..4fa5aa49a 100644 --- a/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java +++ b/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 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 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 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 executeSpecWithSort(Sort sort) { flushTestUsers(); diff --git a/src/test/java/org/springframework/data/jpa/repository/support/QueryDslJpaRepositoryTests.java b/src/test/java/org/springframework/data/jpa/repository/support/QueryDslJpaRepositoryTests.java index ae92839e8..06c24d4ad 100644 --- a/src/test/java/org/springframework/data/jpa/repository/support/QueryDslJpaRepositoryTests.java +++ b/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 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 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 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 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 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 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)); + } } diff --git a/src/test/resources/META-INF/persistence.xml b/src/test/resources/META-INF/persistence.xml index 2c107f047..1f8dad143 100644 --- a/src/test/resources/META-INF/persistence.xml +++ b/src/test/resources/META-INF/persistence.xml @@ -26,6 +26,7 @@ org.springframework.data.jpa.domain.sample.EmbeddedIdExampleDepartment org.springframework.data.jpa.domain.sample.Customer org.springframework.data.jpa.domain.sample.Order + org.springframework.data.jpa.domain.sample.Address true