DATAJPA-1404 - Create explicit outer join for inverse, optional one-to-one relationships.

When deciding between simple navigation and explicit outer join we consider this additional case to work around peculiar behavior of JPA implementations.

See also:
https://hibernate.atlassian.net/browse/HHH-12712
https://github.com/eclipse-ee4j/jpa-api/issues/170

Original pull request: #304.

src/main/java/org/springframework/data/jpa/repository/query/QueryUtils.java

@@ -595,7 +595,7 @@ public abstract class QueryUtils {
 			propertyPathModel = from.get(segment).getModel();
 		}
 
-		if (requiresJoin(propertyPathModel, model instanceof PluralAttribute, !property.hasNext(), isForSelection)
+		if (requiresOuterJoin(propertyPathModel, model instanceof PluralAttribute, !property.hasNext(), isForSelection)
 				&& !isAlreadyFetched(from, segment)) {
 			Join<?, ?> join = getOrCreateJoin(from, segment);
 			return (Expression<T>) (property.hasNext() ? toExpressionRecursively(join, property.next(), isForSelection)
@@ -616,7 +616,7 @@ public abstract class QueryUtils {
 	 * @param isForSelection is the property navigated for the selection part of the query?
 	 * @return wether an outer join is to be used for integrating this attribute in a query.
 	 */
-	private static boolean requiresJoin(@Nullable Bindable<?> propertyPathModel, boolean isPluralAttribute,
+	private static boolean requiresOuterJoin(@Nullable Bindable<?> propertyPathModel, boolean isPluralAttribute,
 			boolean isLeafProperty, boolean isForSelection) {
 
 		if (propertyPathModel == null && isPluralAttribute) {
@@ -633,27 +633,37 @@ public abstract class QueryUtils {
 			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 isInverseOptionalOneToOne = PersistentAttributeType.ONE_TO_ONE == attribute.getPersistentAttributeType()
+				&& !getAnnotationProperty(attribute, "mappedBy", "").isEmpty();
+
 		// 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()) {
+		if (isLeafProperty && !isForSelection && !attribute.isCollection() && !isInverseOptionalOneToOne) {
 			return false;
 		}
 
+		return getAnnotationProperty(attribute, "optional", true);
+	}
+
+	private static <T> T getAnnotationProperty(Attribute<?, ?> attribute, String propertyName, T defaultValue) {
 		Class<? extends Annotation> associationAnnotation = ASSOCIATION_TYPES.get(attribute.getPersistentAttributeType());
 
 		if (associationAnnotation == null) {
-			return true;
+			return defaultValue;
 		}
 
 		Member member = attribute.getJavaMember();
 
 		if (!(member instanceof AnnotatedElement)) {
-			return true;
+			return defaultValue;
 		}
 
 		Annotation annotation = AnnotationUtils.getAnnotation((AnnotatedElement) member, associationAnnotation);
-		return annotation == null ? true : (boolean) AnnotationUtils.getValue(annotation, "optional");
+		return annotation == null ? defaultValue : (T) AnnotationUtils.getValue(annotation, propertyName);
 	}
 
 	static Expression<Object> toExpressionRecursively(Path<Object> path, PropertyPath property) {

src/test/java/org/springframework/data/jpa/repository/query/QueryUtilsIntegrationTests.java

@@ -22,6 +22,7 @@ import static org.mockito.Mockito.*;
 import java.util.Collections;
 import java.util.List;
 import java.util.Set;
+import java.util.function.Consumer;
 import java.util.stream.Collectors;
 
 import javax.persistence.Entity;
@@ -29,6 +30,7 @@ import javax.persistence.EntityManager;
 import javax.persistence.EntityManagerFactory;
 import javax.persistence.Id;
 import javax.persistence.OneToMany;
+import javax.persistence.OneToOne;
 import javax.persistence.Persistence;
 import javax.persistence.PersistenceContext;
 import javax.persistence.criteria.CriteriaBuilder;
@@ -93,6 +95,36 @@ public class QueryUtilsIntegrationTests {
 		assertThat(getNonInnerJoins(root)).hasSize(1);
 	}
 
+	@Test // DATAJPA-1238
+	public void createsJoinForOptionalOneToOneInReverseDirection() {
+
+		doInMerchantContext(emf -> {
+
+			CriteriaBuilder builder = emf.getCriteriaBuilder();
+			CriteriaQuery<Address> query = builder.createQuery(Address.class);
+			Root<Address> root = query.from(Address.class);
+
+			QueryUtils.toExpressionRecursively(root, PropertyPath.from("merchant", Address.class));
+
+			assertThat(getNonInnerJoins(root)).hasSize(1);
+		});
+	}
+
+	@Test // DATAJPA-1238
+	public void createsNoJoinForOptionalOneToOneInNormalDirection() {
+
+		doInMerchantContext(emf -> {
+
+			CriteriaBuilder builder = emf.getCriteriaBuilder();
+			CriteriaQuery<Merchant> query = builder.createQuery(Merchant.class);
+			Root<Merchant> root = query.from(Merchant.class);
+
+			QueryUtils.toExpressionRecursively(root, PropertyPath.from("address", Merchant.class));
+
+			assertThat(getNonInnerJoins(root)).isEmpty();
+		});
+	}
+
 	@Test // DATAJPA-401, DATAJPA-1238
 	public void doesNotCreateJoinForOptionalAssociationWithoutFurtherNavigation() {
 
@@ -130,21 +162,31 @@ public class QueryUtilsIntegrationTests {
 	@Test // DATAJPA-476
 	public void traversesPluralAttributeCorrectly() {
 
-		PersistenceProviderResolver originalPersistenceProviderResolver = PersistenceProviderResolverHolder
-				.getPersistenceProviderResolver();
+		doInMerchantContext(((emf) -> {
 
-		try {
-
-			PersistenceProviderResolverHolder.setPersistenceProviderResolver(new HibernateOnlyPersistenceProviderResolver());
-			EntityManagerFactory entityManagerFactory = Persistence.createEntityManagerFactory("merchant");
-			CriteriaBuilder builder = entityManagerFactory.createEntityManager().getCriteriaBuilder();
+			CriteriaBuilder builder = emf.createEntityManager().getCriteriaBuilder();
 			CriteriaQuery<Merchant> query = builder.createQuery(Merchant.class);
 			Root<Merchant> root = query.from(Merchant.class);
 
 			QueryUtils.toExpressionRecursively(root, PropertyPath.from("employeesCredentialsUid", Merchant.class));
+		}));
+	}
 
+	public void doInMerchantContext(Consumer<EntityManagerFactory> emfConsumer) {
+		PersistenceProviderResolver originalPersistenceProviderResolver = PersistenceProviderResolverHolder
+				.getPersistenceProviderResolver();
+
+		EntityManagerFactory entityManagerFactory = null;
+		try {
+
+			PersistenceProviderResolverHolder.setPersistenceProviderResolver(new HibernateOnlyPersistenceProviderResolver());
+			entityManagerFactory = Persistence.createEntityManagerFactory("merchant");
+			emfConsumer.accept(entityManagerFactory);
 		} finally {
 			PersistenceProviderResolverHolder.setPersistenceProviderResolver(originalPersistenceProviderResolver);
+			if (entityManagerFactory != null) {
+				entityManagerFactory.close();
+			}
 		}
 	}
 
@@ -203,20 +245,29 @@ public class QueryUtilsIntegrationTests {
 		return 0;
 	}
 
+	private Set<Join<?, ?>> getNonInnerJoins(Root<?> root) {
+
+		return root.getJoins() //
+				.stream() //
+				.filter(j -> j.getJoinType() != JoinType.INNER) //
+				.collect(Collectors.toSet());
+	}
+
 	@Entity
 	@SuppressWarnings("unused")
 	static class Merchant {
 
 		@Id String id;
 		@OneToMany Set<Employee> employees;
+
+		@OneToOne Address address;
 	}
 
-	private Set<Join<User, ?>> getNonInnerJoins(Root<User> root) {
-
-		return root.getJoins() //
-				.stream() //
-				.filter(j -> j.getJoinType() != JoinType.INNER) //
-				.collect(Collectors.toSet());
+	@Entity
+	@SuppressWarnings("unused")
+	static class Address {
+		@Id String id;
+		@OneToOne(mappedBy = "address") Merchant merchant;
 	}
 
 	@Entity
@@ -251,4 +302,5 @@ public class QueryUtilsIntegrationTests {
 		@Override
 		public void clearCachedProviders() {}
 	}
+
 }

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

@@ -78,6 +78,7 @@
 	<persistence-unit name="merchant">
 		<class>org.springframework.data.jpa.domain.sample.User</class>
 		<class>org.springframework.data.jpa.repository.query.QueryUtilsIntegrationTests$Merchant</class>
+		<class>org.springframework.data.jpa.repository.query.QueryUtilsIntegrationTests$Address</class>
 		<class>org.springframework.data.jpa.repository.query.QueryUtilsIntegrationTests$Employee</class>
 		<class>org.springframework.data.jpa.repository.query.QueryUtilsIntegrationTests$Credential</class>
 		<exclude-unlisted-classes>true</exclude-unlisted-classes>