From fbb88e8c76e304fb372aca6060d73cf4f944bbed Mon Sep 17 00:00:00 2001 From: Thomas Darimont Date: Fri, 26 Jul 2013 11:31:50 +0200 Subject: [PATCH] DATAJPA-346 - EclipseLink specific workaround for joins in QueryUtils. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added fix in QueryUtils to work around an EclipseLinks specialty to add strict joins on a call to root.get(…) even if a later root.join(…, JoinType.LEFT) should trump this. We needed the first call to examine the metamodel of the path obtained to decide whether to join at all in next steps. We now work around this issue by doing a lot of ugly type checking and casting on the Metamodel directly. We filed https://bugs.eclipse.org/bugs/show_bug.cgi?id=413892 to maybe let EclipseLink improve at that point. Original pull request: #30. --- .../data/jpa/repository/query/QueryUtils.java | 30 ++++++++++++------- ...lipseLinkNamespaceUserRepositoryTests.java | 12 +++++++- .../jpa/repository/UserRepositoryTests.java | 28 ++++++++++++++--- 3 files changed, 55 insertions(+), 15 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 c3d071399..37aca64bf 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 @@ -43,6 +43,7 @@ import javax.persistence.metamodel.Attribute; import javax.persistence.metamodel.Attribute.PersistentAttributeType; import javax.persistence.metamodel.Bindable; import javax.persistence.metamodel.Bindable.BindableType; +import javax.persistence.metamodel.ManagedType; import org.springframework.data.domain.Sort; import org.springframework.data.domain.Sort.Order; @@ -428,33 +429,42 @@ public abstract class QueryUtils { @SuppressWarnings("unchecked") static Expression toExpressionRecursively(From from, PropertyPath property) { - Path path = from.get(property.getSegment()); + Bindable propertyPathModel = null; + if (from.getModel() instanceof ManagedType) { + /* + * Avoid calling from.get(...) because this triggers the generation of an inner-join instead + * of and outer-join in eclipse-link. + * See: https://bugs.eclipse.org/bugs/show_bug.cgi?id=413892 + */ + propertyPathModel = (Bindable) ((ManagedType) from.getModel()).getAttribute(property.getSegment()); + } else { + propertyPathModel = (Bindable) from.get(property.getSegment()).getModel(); + } - if (property.isCollection() || isEntityPath(path)) { + if (property.isCollection() || isEntityPath(propertyPathModel)) { 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. + * Returns whether the given {@code propertyPathModel} can be considered referring an entity. * - * @param path must not be {@literal null}. + * @param propertyPathModel must not be {@literal null}. * @return */ - private static boolean isEntityPath(Path path) { + private static boolean isEntityPath(Bindable propertyPathModel) { - Bindable model = path.getModel(); - - if (BindableType.ENTITY_TYPE.equals(model.getBindableType())) { + if (BindableType.ENTITY_TYPE.equals(propertyPathModel.getBindableType())) { return true; } - if (model instanceof Attribute) { + if (propertyPathModel instanceof Attribute) { - Attribute attribute = (Attribute) model; + Attribute attribute = (Attribute) propertyPathModel; if (attribute.isAssociation()) { return true; 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 617107dac..7327b1dcf 100644 --- a/src/test/java/org/springframework/data/jpa/repository/EclipseLinkNamespaceUserRepositoryTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/EclipseLinkNamespaceUserRepositoryTests.java @@ -51,6 +51,16 @@ public class EclipseLinkNamespaceUserRepositoryTests extends NamespaceUserReposi } @Override - public void doesNotDropNullValuesOnPagedSpecificationExecution() { + public void doesNotDropNullValuesOnPagedSpecificationExecution() {} + + /** + * Works with a workaround in QueryUtils.toExpressionRecursively(…). TODO: remove once EclipseLink bug is fixed. + * + * @see DATAJPA-346 + * @see https://bugs.eclipse.org/bugs/show_bug.cgi?id=413892 + */ + @Override + public void shouldGenerateLeftOuterJoinInfindAllWithPaginationAndSortOnNestedPropertyPath() { + super.shouldGenerateLeftOuterJoinInfindAllWithPaginationAndSortOnNestedPropertyPath(); } } 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 2b162bfe5..a8a41caf9 100644 --- a/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java @@ -74,12 +74,10 @@ import org.springframework.transaction.annotation.Transactional; @Transactional public class UserRepositoryTests { - @PersistenceContext - EntityManager em; + @PersistenceContext EntityManager em; // CUT - @Autowired - UserRepository repository; + @Autowired UserRepository repository; // Test fixture User firstUser, secondUser, thirdUser, fourthUser; @@ -1000,6 +998,28 @@ public class UserRepositoryTests { assertThat(page, hasItem(firstUser)); } + /** + * @see DATAJPA-346 + */ + @Test + public void shouldGenerateLeftOuterJoinInfindAllWithPaginationAndSortOnNestedPropertyPath() { + + firstUser.setManager(null); + secondUser.setManager(null); + thirdUser.setManager(firstUser); // manager Oliver + fourthUser.setManager(secondUser); // manager Joachim + + flushTestUsers(); + + Page pages = repository.findAll(new PageRequest(0, 4, new Sort(Sort.Direction.ASC, "manager.firstname"))); + assertThat(pages.getSize(), is(4)); + assertThat(pages.getContent().get(0).getManager(), is(nullValue())); + assertThat(pages.getContent().get(1).getManager(), is(nullValue())); + assertThat(pages.getContent().get(2).getManager().getFirstname(), is("Joachim")); + assertThat(pages.getContent().get(3).getManager().getFirstname(), is("Oliver")); + assertThat(pages.getTotalElements(), is(4L)); + } + /** * @see DATAJPA-292 */