From 24b38df5604f473b43c621f3c143143b10189b90 Mon Sep 17 00:00:00 2001 From: Patrice Blanchardie Date: Thu, 19 Nov 2020 10:36:51 +0100 Subject: [PATCH 1/3] DATAJPA-1822 - use left outer joins for nested optionals --- .../data/jpa/repository/query/QueryUtils.java | 156 ++++++++++-------- .../data/jpa/domain/sample/Customer.java | 5 +- .../data/jpa/domain/sample/Invoice.java | 35 ++++ .../data/jpa/domain/sample/InvoiceItem.java | 33 ++++ .../query/QueryUtilsIntegrationTests.java | 101 +++++++++++- src/test/resources/META-INF/persistence.xml | 2 + 6 files changed, 262 insertions(+), 70 deletions(-) create mode 100644 src/test/java/org/springframework/data/jpa/domain/sample/Invoice.java create mode 100644 src/test/java/org/springframework/data/jpa/domain/sample/InvoiceItem.java 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 7ef7133f1d..e32ac29033 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 @@ -29,6 +29,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -41,11 +42,9 @@ import javax.persistence.Query; import javax.persistence.criteria.CriteriaBuilder; import javax.persistence.criteria.Expression; -import javax.persistence.criteria.Fetch; import javax.persistence.criteria.From; import javax.persistence.criteria.Join; import javax.persistence.criteria.JoinType; -import javax.persistence.criteria.Path; import javax.persistence.metamodel.Attribute; import javax.persistence.metamodel.Attribute.PersistentAttributeType; import javax.persistence.metamodel.Bindable; @@ -620,12 +619,74 @@ static Expression toExpressionRecursively(From from, PropertyPath p return toExpressionRecursively(from, property, false); } - @SuppressWarnings("unchecked") static Expression toExpressionRecursively(From from, PropertyPath property, boolean isForSelection) { + return toExpressionRecursively(from, property, isForSelection, false); + } + + /** + * Creates an expression with proper inner and left joins by recursively navigating the path + * + * @param from the {@link From} + * @param property the property path + * @param isForSelection is the property navigated for the selection or ordering part of the query? + * @param hasRequiredOuterJoin has a parent already required an outer join? + * @param the type of the expression + * @return the expression + */ + @SuppressWarnings("unchecked") static Expression toExpressionRecursively(From from, + PropertyPath property, boolean isForSelection, boolean hasRequiredOuterJoin) { + + String segment = property.getSegment(); + + boolean isLeafProperty = !property.hasNext(); + + boolean requiresOuterJoin = requiresOuterJoin(from, property, isForSelection, hasRequiredOuterJoin); + + // if it does not require an outer join and is a leaf, simply get the segment + if (!requiresOuterJoin && isLeafProperty) { + return from.get(segment); + } + + // get or create the join + JoinType joinType = requiresOuterJoin ? JoinType.LEFT : JoinType.INNER; + Join join = getOrCreateJoin(from, segment, joinType); + + // if it's a leaf, return the join + if (isLeafProperty) { + return (Expression) join; + } + + PropertyPath nextProperty = Objects.requireNonNull(property.next(), "An element of the property path is null!"); + + // recurse with the next property + return toExpressionRecursively(join, nextProperty, isForSelection, requiresOuterJoin); + } + + /** + * Checks if this attribute requires an outer join. + * This is the case eg. if it hadn't already been fetched with an inner join and if it's an a optional association, + * and if previous paths has already required outer joins. + * It also ensures outer joins are used even when Hibernate defaults to inner joins (HHH-12712 and HHH-12999). + * + * @param from the {@link From} to check for fetches. + * @param property the property path + * @param isForSelection is the property navigated for the selection or ordering part of the query? if true, + * we need to generate an explicit outer join in order to prevent Hibernate to use an + * inner join instead. see https://hibernate.atlassian.net/browse/HHH-12999 + * @param hasRequiredOuterJoin has a parent already required an outer join? + * @return whether an outer join is to be used for integrating this attribute in a query. + */ + private static boolean requiresOuterJoin(From from, PropertyPath property, boolean isForSelection, + boolean hasRequiredOuterJoin) { + + String segment = property.getSegment(); + + // already inner joined so outer join is useless + if (isAlreadyInnerJoined(from, segment)) + return false; Bindable propertyPathModel; Bindable model = from.getModel(); - String segment = property.getSegment(); if (model instanceof ManagedType) { @@ -638,29 +699,10 @@ static Expression toExpressionRecursively(From from, PropertyPath p propertyPathModel = from.get(segment).getModel(); } - if (requiresOuterJoin(propertyPathModel, model instanceof PluralAttribute, !property.hasNext(), isForSelection) - && !isAlreadyFetched(from, segment)) { - Join join = getOrCreateJoin(from, segment); - return (Expression) (property.hasNext() ? toExpressionRecursively(join, property.next(), isForSelection) - : join); - } else { - Path path = from.get(segment); - return (Expression) (property.hasNext() ? toExpressionRecursively(path, property.next()) : path); - } - } + // is the attribute of Collection type? + boolean isPluralAttribute = model instanceof PluralAttribute; - /** - * Returns whether the given {@code propertyPathModel} requires the creation of a join. This is the case if we find a - * optional association. - * - * @param propertyPathModel may be {@literal null}. - * @param isPluralAttribute is the attribute of Collection type? - * @param isLeafProperty is this the final property navigated by a {@link PropertyPath}? - * @param isForSelection is the property navigated for the selection part of the query? - * @return whether an outer join is to be used for integrating this attribute in a query. - */ - private static boolean requiresOuterJoin(@Nullable Bindable propertyPathModel, boolean isPluralAttribute, - boolean isLeafProperty, boolean isForSelection) { + boolean isLeafProperty = !property.hasNext(); if (propertyPathModel == null && isPluralAttribute) { return true; @@ -672,24 +714,23 @@ private static boolean requiresOuterJoin(@Nullable Bindable propertyPathModel Attribute attribute = (Attribute) propertyPathModel; + // not a persistent attribute type association (@OneToOne, @ManyToOne) if (!ASSOCIATION_TYPES.containsKey(attribute.getPersistentAttributeType())) { return false; } - // if this path is an optional one to one attribute navigated from the not owning side we also need an explicit - // outer join to avoid https://hibernate.atlassian.net/browse/HHH-12712 and - // https://github.com/eclipse-ee4j/jpa-api/issues/170 + boolean isCollection = attribute.isCollection(); + // if this path is an optional one to one attribute navigated from the not owning side we also need an + // explicit outer join to avoid https://hibernate.atlassian.net/browse/HHH-12712 + // and https://github.com/eclipse-ee4j/jpa-api/issues/170 boolean isInverseOptionalOneToOne = PersistentAttributeType.ONE_TO_ONE == attribute.getPersistentAttributeType() && StringUtils.hasText(getAnnotationProperty(attribute, "mappedBy", "")); - // if this path is part of the select list we need to generate an explicit outer join in order to prevent Hibernate - // to use an inner join instead. - // see https://hibernate.atlassian.net/browse/HHH-12999. - if (isLeafProperty && !isForSelection && !attribute.isCollection() && !isInverseOptionalOneToOne) { + if (isLeafProperty && !isForSelection && !isCollection && !isInverseOptionalOneToOne && !hasRequiredOuterJoin) { return false; } - return getAnnotationProperty(attribute, "optional", true); + return hasRequiredOuterJoin || getAnnotationProperty(attribute, "optional", true); } private static T getAnnotationProperty(Attribute attribute, String propertyName, T defaultValue) { @@ -710,52 +751,37 @@ private static T getAnnotationProperty(Attribute attribute, String pro return annotation == null ? defaultValue : (T) AnnotationUtils.getValue(annotation, propertyName); } - static Expression toExpressionRecursively(Path path, PropertyPath property) { - - Path result = path.get(property.getSegment()); - return property.hasNext() ? toExpressionRecursively(result, property.next()) : result; - } - /** * Returns an existing join for the given attribute if one already exists or creates a new one if not. * - * @param from the {@link From} to get the current joins from. + * @param from the {@link From} to get the current joins from. * @param attribute the {@link Attribute} to look for in the current joins. + * @param joinType the join type to create if none was found * @return will never be {@literal null}. */ - private static Join getOrCreateJoin(From from, String attribute) { - - for (Join join : from.getJoins()) { - - boolean sameName = join.getAttribute().getName().equals(attribute); - - if (sameName && join.getJoinType().equals(JoinType.LEFT)) { - return join; - } - } - - return from.join(attribute, JoinType.LEFT); + private static Join getOrCreateJoin(From from, String attribute, JoinType joinType) { + return from.getJoins().stream() + .filter(join -> join.getAttribute().getName().equals(attribute)) + .findFirst() + .orElseGet(() -> from.join(attribute, joinType)); } /** - * Return whether the given {@link From} contains a fetch declaration for the attribute with the given name. + * Return whether the given {@link From} contains an inner join for the attribute with the given name. * - * @param from the {@link From} to check for fetches. + * @param from the {@link From} to check for joins. * @param attribute the attribute name to check. - * @return + * @return true if the attribute has already been inner joined */ - private static boolean isAlreadyFetched(From from, String attribute) { - - for (Fetch fetch : from.getFetches()) { + private static boolean isAlreadyInnerJoined(From from, String attribute) { - boolean sameName = fetch.getAttribute().getName().equals(attribute); + boolean isInnerJoinFetched = from.getFetches().stream().anyMatch( + fetch -> fetch.getAttribute().getName().equals(attribute) && fetch.getJoinType().equals(JoinType.INNER)); - if (sameName && fetch.getJoinType().equals(JoinType.LEFT)) { - return true; - } - } + boolean isSimplyInnerJoined = from.getJoins().stream() + .anyMatch(join -> join.getAttribute().getName().equals(attribute) && join.getJoinType().equals(JoinType.INNER)); - return false; + return isInnerJoinFetched || isSimplyInnerJoined; } /** diff --git a/src/test/java/org/springframework/data/jpa/domain/sample/Customer.java b/src/test/java/org/springframework/data/jpa/domain/sample/Customer.java index 84f160bbfb..4db1982f43 100644 --- a/src/test/java/org/springframework/data/jpa/domain/sample/Customer.java +++ b/src/test/java/org/springframework/data/jpa/domain/sample/Customer.java @@ -20,9 +20,12 @@ /** * @author Oliver Gierke + * @author Patrice Blanchardie */ @Entity public class Customer { - @Id Long id; + @Id Long id; + + String name; } diff --git a/src/test/java/org/springframework/data/jpa/domain/sample/Invoice.java b/src/test/java/org/springframework/data/jpa/domain/sample/Invoice.java new file mode 100644 index 0000000000..eceed11d99 --- /dev/null +++ b/src/test/java/org/springframework/data/jpa/domain/sample/Invoice.java @@ -0,0 +1,35 @@ +/* + * Copyright 2020 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 + * + * https://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.Id; +import javax.persistence.ManyToOne; +import javax.persistence.Table; + +/** + * @author Patrice Blanchardie + */ +@Entity +@Table(name = "INVOICES") +public class Invoice { + + @Id Long id; + + @ManyToOne(optional = false) Customer customer; + + @ManyToOne Order order; +} diff --git a/src/test/java/org/springframework/data/jpa/domain/sample/InvoiceItem.java b/src/test/java/org/springframework/data/jpa/domain/sample/InvoiceItem.java new file mode 100644 index 0000000000..21b6cd10e5 --- /dev/null +++ b/src/test/java/org/springframework/data/jpa/domain/sample/InvoiceItem.java @@ -0,0 +1,33 @@ +/* + * Copyright 2020 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 + * + * https://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.Id; +import javax.persistence.ManyToOne; +import javax.persistence.Table; + +/** + * @author Patrice Blanchardie + */ +@Entity +@Table(name = "INVOICE_ITEMS") +public class InvoiceItem { + + @Id Long id; + + @ManyToOne(optional = false) Invoice invoice; +} diff --git a/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java b/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java index f951ef723e..fef51fdf41 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java @@ -35,6 +35,7 @@ import javax.persistence.PersistenceContext; import javax.persistence.criteria.CriteriaBuilder; import javax.persistence.criteria.CriteriaQuery; +import javax.persistence.criteria.From; import javax.persistence.criteria.Join; import javax.persistence.criteria.JoinType; import javax.persistence.criteria.Root; @@ -49,6 +50,8 @@ import org.springframework.data.domain.Sort; import org.springframework.data.domain.Sort.Direction; import org.springframework.data.jpa.domain.sample.Category; +import org.springframework.data.jpa.domain.sample.Invoice; +import org.springframework.data.jpa.domain.sample.InvoiceItem; import org.springframework.data.jpa.domain.sample.Order; import org.springframework.data.jpa.domain.sample.User; import org.springframework.data.jpa.infrastructure.HibernateTestUtils; @@ -61,6 +64,7 @@ * * @author Oliver Gierke * @author Sébastien Péralta + * @author Patrice Blanchardie */ @ExtendWith(SpringExtension.class) @ContextConfiguration("classpath:infrastructure.xml") @@ -111,6 +115,88 @@ void createsJoinForOptionalOneToOneInReverseDirection() { }); } + @Test // DATAJPA-1822 + void createsLeftJoinForOptionalToOneWithNestedNonOptional() { + + CriteriaBuilder builder = em.getCriteriaBuilder(); + CriteriaQuery query = builder.createQuery(Invoice.class); + Root root = query.from(Invoice.class); + + QueryUtils.toExpressionRecursively(root, PropertyPath.from("order.customer.name", Invoice.class), false); + + assertThat(getNonInnerJoins(root)).hasSize(1); // left join order + assertThat(getInnerJoins(root)).isEmpty(); // no inner join order + Join leftJoin = root.getJoins().iterator().next(); + assertThat(getNonInnerJoins(leftJoin)).hasSize(1); // left join customer + assertThat(getInnerJoins(leftJoin)).isEmpty(); // no inner join customer + } + + @Test // DATAJPA-1822 + void createsLeftJoinForNonOptionalToOneWithNestedOptional() { + + CriteriaBuilder builder = em.getCriteriaBuilder(); + CriteriaQuery query = builder.createQuery(InvoiceItem.class); + Root root = query.from(InvoiceItem.class); + + QueryUtils + .toExpressionRecursively(root, PropertyPath.from("invoice.order.customer.name", InvoiceItem.class), false); + + assertThat(getInnerJoins(root)).hasSize(1); // join invoice + Join rootInnerJoin = getInnerJoins(root).iterator().next(); + assertThat(getNonInnerJoins(rootInnerJoin)).hasSize(1); // left join order + assertThat(getInnerJoins(rootInnerJoin)).isEmpty(); // no inner join order + Join leftJoin = getNonInnerJoins(rootInnerJoin).iterator().next(); + assertThat(getNonInnerJoins(leftJoin)).hasSize(1); // left join customer + assertThat(getInnerJoins(leftJoin)).isEmpty(); // no inner join customer + } + + @Test // DATAJPA-1822 + void reusesLeftJoinForNonOptionalToOneWithNestedOptional() { + + CriteriaBuilder builder = em.getCriteriaBuilder(); + CriteriaQuery query = builder.createQuery(InvoiceItem.class); + Root root = query.from(InvoiceItem.class); + + // given existing left joins + root.join("invoice", JoinType.LEFT).join("order", JoinType.LEFT); + + // when navigating through a path with nested optionals + QueryUtils + .toExpressionRecursively(root, PropertyPath.from("invoice.order.customer.name", InvoiceItem.class), false); + + // assert that existing joins are reused and no additional joins are created + assertThat(getInnerJoins(root)).isEmpty(); // no inner join invoice + assertThat(getNonInnerJoins(root)).hasSize(1); // reused left join invoice + Join rootInnerJoin = getNonInnerJoins(root).iterator().next(); + assertThat(getInnerJoins(rootInnerJoin)).isEmpty(); // no inner join order + assertThat(getNonInnerJoins(rootInnerJoin)).hasSize(1); // reused left join order + Join leftJoin = getNonInnerJoins(rootInnerJoin).iterator().next(); + assertThat(getNonInnerJoins(leftJoin)).hasSize(1); // left join customer + } + + @Test // DATAJPA-1822 + void reusesInnerJoinForNonOptionalToOneWithNestedOptional() { + + CriteriaBuilder builder = em.getCriteriaBuilder(); + CriteriaQuery query = builder.createQuery(InvoiceItem.class); + Root root = query.from(InvoiceItem.class); + + // given an existing inner join an nested optional + root.join("invoice").join("order"); + + QueryUtils + .toExpressionRecursively(root, PropertyPath.from("invoice.order.customer.name", InvoiceItem.class), false); + + // assert that no useless left joins are created + assertThat(getInnerJoins(root)).hasSize(1); // join invoice + Join rootInnerJoin = getInnerJoins(root).iterator().next(); + assertThat(getNonInnerJoins(rootInnerJoin)).isEmpty(); // no left join order + assertThat(getInnerJoins(rootInnerJoin)).hasSize(1); // inner join order + Join innerJoin = getInnerJoins(rootInnerJoin).iterator().next(); + assertThat(getNonInnerJoins(innerJoin)).isEmpty(); // no left join customer + assertThat(getInnerJoins(innerJoin)).hasSize(1); // inner join customer + } + @Test // DATAJPA-1404 void createsNoJoinForOptionalOneToOneInNormalDirection() { @@ -248,10 +334,17 @@ int getNumberOfJoinsAfterCreatingAPath() { private Set> getNonInnerJoins(Root root) { - return root.getJoins() // - .stream() // - .filter(j -> j.getJoinType() != JoinType.INNER) // - .collect(Collectors.toSet()); + return getNonInnerJoins((From) root); + } + + private Set> getNonInnerJoins(From root) { + + return root.getJoins().stream().filter(j -> j.getJoinType() != JoinType.INNER).collect(Collectors.toSet()); + } + + private Set> getInnerJoins(From root) { + + return root.getJoins().stream().filter(j -> j.getJoinType() == JoinType.INNER).collect(Collectors.toSet()); } @Entity diff --git a/src/test/resources/META-INF/persistence.xml b/src/test/resources/META-INF/persistence.xml index bf512e2748..42fe37d433 100644 --- a/src/test/resources/META-INF/persistence.xml +++ b/src/test/resources/META-INF/persistence.xml @@ -22,6 +22,8 @@ org.springframework.data.jpa.domain.sample.EmbeddedIdExampleDepartment org.springframework.data.jpa.domain.sample.IdClassExampleEmployee org.springframework.data.jpa.domain.sample.IdClassExampleDepartment + org.springframework.data.jpa.domain.sample.Invoice + org.springframework.data.jpa.domain.sample.InvoiceItem org.springframework.data.jpa.domain.sample.Item org.springframework.data.jpa.domain.sample.ItemSite org.springframework.data.jpa.domain.sample.MailMessage From 372ca69c99f14b6ab88a0baaf8cb42593cef054d Mon Sep 17 00:00:00 2001 From: Patrice Blanchardie Date: Fri, 26 Feb 2021 18:27:28 +0100 Subject: [PATCH 2/3] DATAJPA-1822 - skip optional detection with EclipseLink --- .../EclipseLinkQueryUtilsIntegrationTests.java | 4 ++++ .../query/QueryUtilsIntegrationTests.java | 16 ++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/src/test/java/org/springframework/data/jpa/repository/query/EclipseLinkQueryUtilsIntegrationTests.java b/src/test/java/org/springframework/data/jpa/repository/query/EclipseLinkQueryUtilsIntegrationTests.java index 3b052b061a..474dd44822 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/EclipseLinkQueryUtilsIntegrationTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/EclipseLinkQueryUtilsIntegrationTests.java @@ -27,4 +27,8 @@ int getNumberOfJoinsAfterCreatingAPath() { return 1; } + boolean isSkipOptionalDetection() { + return true; + } + } diff --git a/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java b/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java index fef51fdf41..920d84815a 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java @@ -118,6 +118,9 @@ void createsJoinForOptionalOneToOneInReverseDirection() { @Test // DATAJPA-1822 void createsLeftJoinForOptionalToOneWithNestedNonOptional() { + if(isSkipOptionalDetection()) + return; + CriteriaBuilder builder = em.getCriteriaBuilder(); CriteriaQuery query = builder.createQuery(Invoice.class); Root root = query.from(Invoice.class); @@ -134,6 +137,9 @@ void createsLeftJoinForOptionalToOneWithNestedNonOptional() { @Test // DATAJPA-1822 void createsLeftJoinForNonOptionalToOneWithNestedOptional() { + if(isSkipOptionalDetection()) + return; + CriteriaBuilder builder = em.getCriteriaBuilder(); CriteriaQuery query = builder.createQuery(InvoiceItem.class); Root root = query.from(InvoiceItem.class); @@ -153,6 +159,9 @@ void createsLeftJoinForNonOptionalToOneWithNestedOptional() { @Test // DATAJPA-1822 void reusesLeftJoinForNonOptionalToOneWithNestedOptional() { + if(isSkipOptionalDetection()) + return; + CriteriaBuilder builder = em.getCriteriaBuilder(); CriteriaQuery query = builder.createQuery(InvoiceItem.class); Root root = query.from(InvoiceItem.class); @@ -177,6 +186,9 @@ void reusesLeftJoinForNonOptionalToOneWithNestedOptional() { @Test // DATAJPA-1822 void reusesInnerJoinForNonOptionalToOneWithNestedOptional() { + if(isSkipOptionalDetection()) + return; + CriteriaBuilder builder = em.getCriteriaBuilder(); CriteriaQuery query = builder.createQuery(InvoiceItem.class); Root root = query.from(InvoiceItem.class); @@ -332,6 +344,10 @@ int getNumberOfJoinsAfterCreatingAPath() { return 0; } + boolean isSkipOptionalDetection() { + return false; + } + private Set> getNonInnerJoins(Root root) { return getNonInnerJoins((From) root); From 0860d7f73972455338c2d4cc8bc1369bbeb97129 Mon Sep 17 00:00:00 2001 From: Patrice Blanchardie Date: Sun, 28 Feb 2021 01:46:53 +0100 Subject: [PATCH 3/3] DATAJPA-1822 - workaround for EclipseLink --- .../data/jpa/repository/query/QueryUtils.java | 19 +++++++++++++------ ...EclipseLinkQueryUtilsIntegrationTests.java | 4 ---- .../query/QueryUtilsIntegrationTests.java | 16 ---------------- 3 files changed, 13 insertions(+), 26 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 e32ac29033..770b976410 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 @@ -50,6 +50,7 @@ import javax.persistence.metamodel.Bindable; import javax.persistence.metamodel.ManagedType; import javax.persistence.metamodel.PluralAttribute; +import javax.persistence.metamodel.SingularAttribute; import org.springframework.core.annotation.AnnotationUtils; import org.springframework.dao.InvalidDataAccessApiUsageException; @@ -688,13 +689,19 @@ private static boolean requiresOuterJoin(From from, PropertyPath property, Bindable propertyPathModel; Bindable model = from.getModel(); + // required for EclipseLink: we try to avoid using from.get as EclipseLink produces an inner join + // regardless of which join operation is specified next + // see: https://bugs.eclipse.org/bugs/show_bug.cgi?id=413892 + // still occurs as of 2.7 + ManagedType managedType = null; if (model instanceof ManagedType) { - - /* - * Required to keep support for EclipseLink 2.4.x. TODO: Remove once we drop that (probably Dijkstra M1) - * See: https://bugs.eclipse.org/bugs/show_bug.cgi?id=413892 - */ - propertyPathModel = (Bindable) ((ManagedType) model).getAttribute(segment); + managedType = (ManagedType) model; + } else if (model instanceof SingularAttribute + && ((SingularAttribute) model).getType() instanceof ManagedType) { + managedType = (ManagedType) ((SingularAttribute) model).getType(); + } + if (managedType != null) { + propertyPathModel = (Bindable) managedType.getAttribute(segment); } else { propertyPathModel = from.get(segment).getModel(); } diff --git a/src/test/java/org/springframework/data/jpa/repository/query/EclipseLinkQueryUtilsIntegrationTests.java b/src/test/java/org/springframework/data/jpa/repository/query/EclipseLinkQueryUtilsIntegrationTests.java index 474dd44822..3b052b061a 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/EclipseLinkQueryUtilsIntegrationTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/EclipseLinkQueryUtilsIntegrationTests.java @@ -27,8 +27,4 @@ int getNumberOfJoinsAfterCreatingAPath() { return 1; } - boolean isSkipOptionalDetection() { - return true; - } - } diff --git a/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java b/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java index 920d84815a..fef51fdf41 100644 --- a/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java @@ -118,9 +118,6 @@ void createsJoinForOptionalOneToOneInReverseDirection() { @Test // DATAJPA-1822 void createsLeftJoinForOptionalToOneWithNestedNonOptional() { - if(isSkipOptionalDetection()) - return; - CriteriaBuilder builder = em.getCriteriaBuilder(); CriteriaQuery query = builder.createQuery(Invoice.class); Root root = query.from(Invoice.class); @@ -137,9 +134,6 @@ void createsLeftJoinForOptionalToOneWithNestedNonOptional() { @Test // DATAJPA-1822 void createsLeftJoinForNonOptionalToOneWithNestedOptional() { - if(isSkipOptionalDetection()) - return; - CriteriaBuilder builder = em.getCriteriaBuilder(); CriteriaQuery query = builder.createQuery(InvoiceItem.class); Root root = query.from(InvoiceItem.class); @@ -159,9 +153,6 @@ void createsLeftJoinForNonOptionalToOneWithNestedOptional() { @Test // DATAJPA-1822 void reusesLeftJoinForNonOptionalToOneWithNestedOptional() { - if(isSkipOptionalDetection()) - return; - CriteriaBuilder builder = em.getCriteriaBuilder(); CriteriaQuery query = builder.createQuery(InvoiceItem.class); Root root = query.from(InvoiceItem.class); @@ -186,9 +177,6 @@ void reusesLeftJoinForNonOptionalToOneWithNestedOptional() { @Test // DATAJPA-1822 void reusesInnerJoinForNonOptionalToOneWithNestedOptional() { - if(isSkipOptionalDetection()) - return; - CriteriaBuilder builder = em.getCriteriaBuilder(); CriteriaQuery query = builder.createQuery(InvoiceItem.class); Root root = query.from(InvoiceItem.class); @@ -344,10 +332,6 @@ int getNumberOfJoinsAfterCreatingAPath() { return 0; } - boolean isSkipOptionalDetection() { - return false; - } - private Set> getNonInnerJoins(Root root) { return getNonInnerJoins((From) root);