From 2deabaced3ebdc1378dc5b076773fd6af536e7d5 Mon Sep 17 00:00:00 2001 From: Patrice Blanchardie Date: Thu, 19 Nov 2020 10:36:51 +0100 Subject: [PATCH] fix DATAJPA-1822 --- .../data/jpa/repository/query/QueryUtils.java | 38 ++++++++----- .../data/jpa/domain/sample/Customer.java | 6 +- .../data/jpa/domain/sample/Invoice.java | 35 ++++++++++++ .../data/jpa/domain/sample/InvoiceItem.java | 33 +++++++++++ .../query/QueryUtilsIntegrationTests.java | 57 ++++++++++++++++--- src/test/resources/META-INF/persistence.xml | 2 + 6 files changed, 148 insertions(+), 23 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 f59aeb58572..eb6b5d1549a 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 @@ -605,7 +605,7 @@ public static String getProjection(String query) { private static javax.persistence.criteria.Order toJpaOrder(Order order, From from, CriteriaBuilder cb) { PropertyPath property = PropertyPath.from(order.getProperty(), from.getJavaType()); - Expression expression = toExpressionRecursively(from, property); + Expression expression = toExpressionRecursively(from, property, true); if (order.isIgnoreCase() && String.class.equals(expression.getJavaType())) { Expression lower = cb.lower((Expression) expression); @@ -619,8 +619,12 @@ 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); + } + + @SuppressWarnings("unchecked") + static Expression toExpressionRecursively(From from, PropertyPath property, boolean isForSelection, boolean hasRequiredOuterJoin) { Bindable propertyPathModel; Bindable model = from.getModel(); @@ -637,10 +641,14 @@ static Expression toExpressionRecursively(From from, PropertyPath p propertyPathModel = from.get(segment).getModel(); } - if (requiresOuterJoin(propertyPathModel, model instanceof PluralAttribute, !property.hasNext(), isForSelection) + if (requiresOuterJoin(propertyPathModel, model instanceof PluralAttribute, !property.hasNext(), isForSelection, hasRequiredOuterJoin) && !isAlreadyFetched(from, segment)) { - Join join = getOrCreateJoin(from, segment); - return (Expression) (property.hasNext() ? toExpressionRecursively(join, property.next(), isForSelection) + Join join = getOrCreateJoin(from, segment, JoinType.LEFT); + return (Expression) (property.hasNext() ? toExpressionRecursively(join, property.next(), isForSelection, true) + : join); + } else if(property.hasNext()) { + Join join = getOrCreateJoin(from, segment, JoinType.INNER); + return (Expression) (property.hasNext() ? toExpressionRecursively(join, property.next(), isForSelection, false) : join); } else { Path path = from.get(segment); @@ -655,11 +663,12 @@ static Expression toExpressionRecursively(From from, PropertyPath p * @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? + * @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? * @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, boolean isForSelection, boolean hasRequiredOuterJoin) { if (propertyPathModel == null && isPluralAttribute) { return true; @@ -681,14 +690,14 @@ private static boolean requiresOuterJoin(@Nullable Bindable propertyPathModel 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 + // if this path is part of the select or order 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 && !attribute.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) { @@ -716,24 +725,25 @@ static Expression toExpressionRecursively(Path path, PropertyPat } /** - * Returns an existing join for the given attribute if one already exists or creates a new one if not. + * Returns an existing join for the given attribute and join type if one already exists or creates a new one if not. * * @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 * @return will never be {@literal null}. */ - private static Join getOrCreateJoin(From from, String attribute) { + private static Join getOrCreateJoin(From from, String attribute, JoinType joinType) { for (Join join : from.getJoins()) { boolean sameName = join.getAttribute().getName().equals(attribute); - if (sameName && join.getJoinType().equals(JoinType.LEFT)) { + if (sameName && join.getJoinType().equals(joinType)) { return join; } } - return from.join(attribute, JoinType.LEFT); + return from.join(attribute, joinType); } /** 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 e9926daf513..a61628431a6 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,13 @@ /** * @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 00000000000..eceed11d997 --- /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 00000000000..21b6cd10e5d --- /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 7e2d727f75d..7231efa3fd8 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 @@ -33,11 +33,7 @@ import javax.persistence.OneToOne; import javax.persistence.Persistence; import javax.persistence.PersistenceContext; -import javax.persistence.criteria.CriteriaBuilder; -import javax.persistence.criteria.CriteriaQuery; -import javax.persistence.criteria.Join; -import javax.persistence.criteria.JoinType; -import javax.persistence.criteria.Root; +import javax.persistence.criteria.*; import javax.persistence.spi.PersistenceProvider; import javax.persistence.spi.PersistenceProviderResolver; import javax.persistence.spi.PersistenceProviderResolverHolder; @@ -49,6 +45,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 +59,7 @@ * * @author Oliver Gierke * @author Sébastien Péralta + * @author Patrice Blanchardie */ @ExtendWith(SpringExtension.class) @ContextConfiguration("classpath:infrastructure.xml") @@ -111,6 +110,35 @@ 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(getNonInnerJoins(root.getJoins().iterator().next())).hasSize(1); // left 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 + Join leftJoin = getNonInnerJoins(rootInnerJoin).iterator().next(); + assertThat(getNonInnerJoins(leftJoin)).hasSize(1); // left join customer + } + @Test // DATAJPA-1404 void createsNoJoinForOptionalOneToOneInNormalDirection() { @@ -248,9 +276,22 @@ int getNumberOfJoinsAfterCreatingAPath() { private Set> getNonInnerJoins(Root root) { - return root.getJoins() // - .stream() // - .filter(j -> j.getJoinType() != JoinType.INNER) // + 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()); } diff --git a/src/test/resources/META-INF/persistence.xml b/src/test/resources/META-INF/persistence.xml index bf512e27489..42fe37d4333 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