fix DATAJPA-1822

Author: pblanchardie

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<String> lower = cb.lower((Expression<String>) expression);
@@ -619,8 +619,12 @@ static <T> Expression<T> toExpressionRecursively(From<?, ?> from, PropertyPath p
 		return toExpressionRecursively(from, property, false);
 	}
 
-	@SuppressWarnings("unchecked")
 	static <T> Expression<T> toExpressionRecursively(From<?, ?> from, PropertyPath property, boolean isForSelection) {
+		return toExpressionRecursively(from, property, isForSelection, false);
+	}
+
+	@SuppressWarnings("unchecked")
+	static <T> Expression<T> toExpressionRecursively(From<?, ?> from, PropertyPath property, boolean isForSelection, boolean hasRequiredOuterJoin) {
 
 		Bindable<?> propertyPathModel;
 		Bindable<?> model = from.getModel();
@@ -637,10 +641,14 @@ static <T> Expression<T> 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<T>) (property.hasNext() ? toExpressionRecursively(join, property.next(), isForSelection)
+			Join<?, ?> join = getOrCreateJoin(from, segment, JoinType.LEFT);
+			return (Expression<T>) (property.hasNext() ? toExpressionRecursively(join, property.next(), isForSelection, true)
+					: join);
+		} else if(property.hasNext()) {
+			Join<?, ?> join = getOrCreateJoin(from, segment, JoinType.INNER);
+			return (Expression<T>) (property.hasNext() ? toExpressionRecursively(join, property.next(), isForSelection, false)
 					: join);
 		} else {
 			Path<Object> path = from.get(segment);
@@ -655,11 +663,12 @@ static <T> Expression<T> 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> T getAnnotationProperty(Attribute<?, ?> attribute, String propertyName, T defaultValue) {
@@ -716,24 +725,25 @@ static Expression<Object> toExpressionRecursively(Path<Object> 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);
 	}
 
 	/**

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;
 }

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;
+}

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;
+}

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,36 @@ void createsJoinForOptionalOneToOneInReverseDirection() {
 		});
 	}
 
+	@Test // DATAJPA-1822
+	void createsLeftJoinForOptionalToOneWithNestedNonOptional() {
+
+		CriteriaBuilder builder = em.getCriteriaBuilder();
+		CriteriaQuery<Invoice> query = builder.createQuery(Invoice.class);
+		Root<Invoice> 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<InvoiceItem> query = builder.createQuery(InvoiceItem.class);
+		Root<InvoiceItem> 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 +277,22 @@ int getNumberOfJoinsAfterCreatingAPath() {
 
 	private Set<Join<?, ?>> getNonInnerJoins(Root<?> root) {
 
-		return root.getJoins() //
-				.stream() //
-				.filter(j -> j.getJoinType() != JoinType.INNER) //
+		return getNonInnerJoins((From<?,?>) root);
+	}
+
+	private Set<Join<?, ?>> getNonInnerJoins(From<?,?> root) {
+
+		return root.getJoins()
+				.stream()
+				.filter(j -> j.getJoinType() != JoinType.INNER)
+				.collect(Collectors.toSet());
+	}
+
+	private Set<Join<?, ?>> getInnerJoins(From<?,?> root) {
+
+		return root.getJoins()
+				.stream()
+				.filter(j -> j.getJoinType() == JoinType.INNER)
 				.collect(Collectors.toSet());
 	}
 

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

@@ -22,6 +22,8 @@
 		<class>org.springframework.data.jpa.domain.sample.EmbeddedIdExampleDepartment</class>
 		<class>org.springframework.data.jpa.domain.sample.IdClassExampleEmployee</class>
 		<class>org.springframework.data.jpa.domain.sample.IdClassExampleDepartment</class>
+		<class>org.springframework.data.jpa.domain.sample.Invoice</class>
+		<class>org.springframework.data.jpa.domain.sample.InvoiceItem</class>
 		<class>org.springframework.data.jpa.domain.sample.Item</class>
 		<class>org.springframework.data.jpa.domain.sample.ItemSite</class>
 		<class>org.springframework.data.jpa.domain.sample.MailMessage</class>