DATAJPA-1041 - Fixed application of complex ad-hoc fetch graphs.

We now properly calculate complex ad-hoch fetch graphs. Previously, deeply nested graphs could have been mixed up in certain circumstances.

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

@@ -21,6 +21,7 @@ import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 
+import javax.persistence.AttributeNode;
 import javax.persistence.EntityGraph;
 import javax.persistence.EntityManager;
 import javax.persistence.Query;
@@ -28,6 +29,7 @@ import javax.persistence.Subgraph;
 
 import org.springframework.util.Assert;
 import org.springframework.util.ClassUtils;
+import org.springframework.util.ObjectUtils;
 import org.springframework.util.ReflectionUtils;
 import org.springframework.util.StringUtils;
 
@@ -36,6 +38,7 @@ import org.springframework.util.StringUtils;
  * 
  * @author Thomas Darimont
  * @author Oliver Gierke
+ * @author Christoph Strobl
  * @since 1.6
  */
 public class Jpa21Utils {
@@ -152,7 +155,10 @@ public class Jpa21Utils {
 
 			// Fast path - just single attribute
 			if (!path.contains(".")) {
-				entityGraph.addAttributeNodes(path);
+
+				if(findAttributeNode(path, entityGraph) == null) {
+					entityGraph.addAttributeNodes(path);
+				}
 				continue;
 			}
 
@@ -161,10 +167,37 @@ public class Jpa21Utils {
 			Subgraph<?> parent = null;
 
 			for (int c = 0; c < pathComponents.length - 1; c++) {
-				parent = c == 0 ? entityGraph.addSubgraph(pathComponents[c]) : parent.addSubgraph(pathComponents[c]);
+				parent = c == 0 ? findOrCreateSubgraph(pathComponents[c], entityGraph) : parent.addSubgraph(pathComponents[c]);
 			}
 
 			parent.addAttributeNodes(pathComponents[pathComponents.length - 1]);
 		}
 	}
+
+	private static Subgraph<?> findOrCreateSubgraph(String attributeNode, EntityGraph<?> entityGraph) {
+
+		Subgraph<?> subgraph = findSubgraph(attributeNode, entityGraph);
+		return subgraph != null ? subgraph : entityGraph.addSubgraph(attributeNode);
+	}
+
+	private static Subgraph<?> findSubgraph(String attributeNode, EntityGraph<?> entityGraph) {
+
+		AttributeNode<?> node = findAttributeNode(attributeNode, entityGraph);
+		if(node != null && !ObjectUtils.isEmpty(node.getSubgraphs())) {
+			return node.getSubgraphs().values().iterator().next();
+		}
+
+		return null;
+	}
+
+	private static AttributeNode<?> findAttributeNode(String attributeNode, EntityGraph<?> entityGraph) {
+
+		for(AttributeNode<?> node : entityGraph.getAttributeNodes()) {
+			if(ObjectUtils.nullSafeEquals(node.getAttributeName(), attributeNode)) {
+				return node;
+			}
+		}
+
+		return null;
+	}
 }

src/test/java/org/springframework/data/jpa/domain/sample/User.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2008-2015 the original author or authors.
+ * Copyright 2008-2017 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.
@@ -37,6 +37,7 @@ import javax.persistence.NamedEntityGraphs;
 import javax.persistence.NamedQuery;
 import javax.persistence.NamedStoredProcedureQueries;
 import javax.persistence.NamedStoredProcedureQuery;
+import javax.persistence.NamedSubgraph;
 import javax.persistence.ParameterMode;
 import javax.persistence.StoredProcedureParameter;
 import javax.persistence.Table;
@@ -49,6 +50,7 @@ import javax.persistence.TemporalType;
  * 
  * @author Oliver Gierke
  * @author Thomas Darimont
+ * @author Christoph Strobl
  */
 @Entity
 @NamedEntityGraphs({
@@ -56,7 +58,16 @@ import javax.persistence.TemporalType;
 		@NamedEntityGraph(name = "User.detail", attributeNodes = { @NamedAttributeNode("roles"),
 				@NamedAttributeNode("manager"), @NamedAttributeNode("colleagues") }),
 		@NamedEntityGraph(name = "User.getOneWithDefinedEntityGraphById", attributeNodes = { @NamedAttributeNode("roles"),
-				@NamedAttributeNode("manager"), @NamedAttributeNode("colleagues") }) })
+				@NamedAttributeNode("manager"), @NamedAttributeNode("colleagues") }),
+        @NamedEntityGraph(name = "User.withSubGraph",
+				attributeNodes = {
+        			@NamedAttributeNode("roles"),
+					@NamedAttributeNode(value="colleagues", subgraph = "User.colleagues")
+				},
+				subgraphs = {
+        			@NamedSubgraph(name = "User.colleagues", attributeNodes = {@NamedAttributeNode("colleagues"), @NamedAttributeNode("roles")})
+				}
+		)})
 @NamedQuery(name = "User.findByEmailAddress", query = "SELECT u FROM User u WHERE u.emailAddress = ?1")
 @NamedStoredProcedureQueries({ //
 @NamedStoredProcedureQuery(name = "User.plus1", procedureName = "plus1inout", parameters = {

src/test/java/org/springframework/data/jpa/repository/EntityGraphRepositoryMethodsIntegrationTests.java

@@ -19,10 +19,11 @@ import static org.hamcrest.Matchers.*;
 import static org.junit.Assert.*;
 import static org.springframework.data.jpa.support.EntityManagerTestUtils.*;
 
+import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 
-import javax.persistence.EntityManager;
-import javax.persistence.Persistence;
+import javax.persistence.*;
 
 import org.junit.Assume;
 import org.junit.Before;
@@ -38,6 +39,7 @@ import org.springframework.data.jpa.repository.sample.RepositoryMethodsWithEntit
 import org.springframework.test.context.ContextConfiguration;
 import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
 import org.springframework.transaction.annotation.Transactional;
+import org.springframework.util.StringUtils;
 
 /**
  * Integration tests for RepositoryMethodsWithEntityGraphConfigJpaRepository.
@@ -45,6 +47,7 @@ import org.springframework.transaction.annotation.Transactional;
  * @author Thomas Darimont
  * @author Oliver Gierke
  * @author Jocelyn Ntakpe
+ * @author Christoph Strobl
  */
 @RunWith(SpringJUnit4ClassRunner.class)
 @ContextConfiguration("classpath:config/namespace-autoconfig-context.xml")
@@ -56,6 +59,7 @@ public class EntityGraphRepositoryMethodsIntegrationTests {
 
 	User tom;
 	User ollie;
+	User christoph;
 	Role role;
 
 	@Before
@@ -63,6 +67,7 @@ public class EntityGraphRepositoryMethodsIntegrationTests {
 
 		tom = new User("Thomas", "Darimont", "tdarimont@example.org");
 		ollie = new User("Oliver", "Gierke", "ogierke@example.org");
+		christoph = new User("Christoph", "Strobl", "cstrobl@example.org");
 
 		role = new Role("Developer");
 		em.persist(role);
@@ -71,6 +76,11 @@ public class EntityGraphRepositoryMethodsIntegrationTests {
 
 		ollie = repository.save(ollie);
 		tom.getColleagues().add(ollie);
+
+		christoph = repository.save(christoph);
+
+		ollie.getColleagues().add(christoph);
+		repository.save(ollie);
 	}
 
 	/**
@@ -83,8 +93,8 @@ public class EntityGraphRepositoryMethodsIntegrationTests {
 
 		List<User> result = repository.findAll();
 
-		assertThat(result.size(), is(2));
-		assertThat(Persistence.getPersistenceUtil().isLoaded(result.get(0).getRoles()), is(true));
+		assertThat(result.size(), is(3));
+		assertThat(Persistence.getPersistenceUtil().isLoaded(result.get(0), "roles"), is(true));
 		assertThat(result.get(0), is(tom));
 	}
 
@@ -100,7 +110,7 @@ public class EntityGraphRepositoryMethodsIntegrationTests {
 
 		assertThat(user, is(notNullValue()));
 		assertThat("colleages should be fetched with 'user.detail' fetchgraph",
-				Persistence.getPersistenceUtil().isLoaded(user.getColleagues()), is(true));
+				Persistence.getPersistenceUtil().isLoaded(user, "colleagues"), is(true));
 	}
 
 	/**
@@ -115,7 +125,7 @@ public class EntityGraphRepositoryMethodsIntegrationTests {
 
 		assertThat(user, is(notNullValue()));
 		assertThat("colleages should be fetched with 'user.detail' fetchgraph",
-				Persistence.getPersistenceUtil().isLoaded(user.getColleagues()), is(true));
+				Persistence.getPersistenceUtil().isLoaded(user, "colleagues"), is(true));
 	}
 
 	/**
@@ -126,11 +136,19 @@ public class EntityGraphRepositoryMethodsIntegrationTests {
 
 		Assume.assumeTrue(currentEntityManagerIsAJpa21EntityManager(em));
 
+		em.flush();
+		em.clear();
+
 		User user = repository.getOneWithAttributeNamesById(tom.getId());
 
 		assertThat(user, is(notNullValue()));
 		assertThat("colleages should be fetched with 'user.detail' fetchgraph",
-				Persistence.getPersistenceUtil().isLoaded(user.getColleagues()), is(true));
+				Persistence.getPersistenceUtil().isLoaded(user, "colleagues"), is(true));
+		assertThat(Persistence.getPersistenceUtil().isLoaded(user, "colleagues"), is(true));
+
+		for (User colleague : user.getColleagues()) {
+			assertThat(Persistence.getPersistenceUtil().isLoaded(colleague, "roles"), is(true));
+		}
 	}
 
 	/**
@@ -144,8 +162,94 @@ public class EntityGraphRepositoryMethodsIntegrationTests {
 		Page<User> page = repository.findAll(QUser.user.firstname.isNotNull(), new PageRequest(0, 100));
 		List<User> result = page.getContent();
 
-		assertThat(result.size(), is(2));
+		assertThat(result.size(), is(3));
 		assertThat(Persistence.getPersistenceUtil().isLoaded(result.get(0).getRoles()), is(true));
 		assertThat(result.get(0), is(tom));
 	}
+
+	@Test // DATAJPA-1041
+	public void shouldRespectNamedEntitySubGraph() {
+
+		Assume.assumeTrue(currentEntityManagerIsAJpa21EntityManager(em));
+
+		em.flush();
+		em.clear();
+
+		User user = repository.findOneWithMultipleSubGraphsUsingNamedEntityGraphById(tom.getId());
+
+		assertThat(user, is(notNullValue()));
+
+		assertThat("colleagues on root should have been fetched by named 'User.colleagues' subgraph declaration",
+				Persistence.getPersistenceUtil().isLoaded(user, "colleagues"), is(true));
+
+		for (User colleague : user.getColleagues()) {
+			assertThat(Persistence.getPersistenceUtil().isLoaded(colleague, "colleagues"), is(true));
+			assertThat(Persistence.getPersistenceUtil().isLoaded(colleague, "roles"), is(true));
+		}
+	}
+
+	@Test // DATAJPA-1041
+	public void shouldRespectMultipleSubGraphForSameAttributeWithDynamicFetchGraph() {
+
+		Assume.assumeTrue(currentEntityManagerIsAJpa21EntityManager(em));
+
+		em.flush();
+		em.clear();
+
+		User user = repository.findOneWithMultipleSubGraphsById(tom.getId());
+
+		assertThat(user, is(notNullValue()));
+
+		assertThat("colleagues on root should have been fetched by dynamic subgraph declaration",
+				Persistence.getPersistenceUtil().isLoaded(user, "colleagues"), is(true));
+
+		for (User colleague : user.getColleagues()) {
+			assertThat(Persistence.getPersistenceUtil().isLoaded(colleague, "colleagues"), is(true));
+			assertThat(Persistence.getPersistenceUtil().isLoaded(colleague, "roles"), is(true));
+		}
+	}
+
+	@Test // DATAJPA-1041 - TODO: remove when done fighting with eclipselink.
+	public void thisOneFailsWithEclipselink() {
+
+		Assume.assumeTrue(currentEntityManagerIsAJpa21EntityManager(em));
+
+		em.flush();
+		em.clear();
+
+		javax.persistence.EntityGraph<?> graph = em.getEntityGraph("User.withSubGraph");
+
+		printGraph(graph);
+
+		User result = (User) em.createQuery("Select u from User u where u.id = " + tom.getId())
+				.setHint("javax.persistence.loadgraph", graph).getResultList().get(0);
+
+		assertThat(Persistence.getPersistenceUtil().isLoaded(result, "roles"), is(true));
+		assertThat(Persistence.getPersistenceUtil().isLoaded(result, "colleagues"), is(true));
+	}
+
+	private void printGraph(javax.persistence.EntityGraph<?> graph) {
+
+		try {
+			for (AttributeNode<?> node : graph.getAttributeNodes()) {
+				System.out.println("|- node.getAttributeName(): " + node.getAttributeName());
+				for (Map.Entry<Class, Subgraph> subGraph : node.getSubgraphs().entrySet()) {
+					System.out.print("|  +- subGraph: " + subGraph.getKey().getSimpleName() + " -> [");
+
+					Iterator it = subGraph.getValue().getAttributeNodes().iterator();
+					while (it.hasNext()) {
+
+						AttributeNode<?> an = (AttributeNode<?>) it.next();
+						System.out.print(an.getAttributeName());
+						if (it.hasNext()) {
+							System.out.print(", ");
+						}
+					}
+					System.out.println("]");
+				}
+			}
+		} catch (Exception e) {
+			// o_O what happened here - ignore it - it's just debug output.
+		}
+	}
 }

src/test/java/org/springframework/data/jpa/repository/sample/RepositoryMethodsWithEntityGraphConfigRepository.java

@@ -33,6 +33,7 @@ import com.querydsl.core.types.Predicate;
  * 
  * @author Thomas Darimont
  * @author Jocelyn Ntakpe
+ * @author Christoph Strobl
  */
 public interface RepositoryMethodsWithEntityGraphConfigRepository
 		extends CrudRepository<User, Integer>, QueryDslPredicateExecutor<User> {
@@ -66,4 +67,13 @@ public interface RepositoryMethodsWithEntityGraphConfigRepository
 	 */
 	@EntityGraph("User.detail")
 	Page<User> findAll(Predicate predicate, Pageable pageable);
+
+	// DATAJPA-1041
+	@EntityGraph(type = EntityGraphType.FETCH, value = "User.withSubGraph")
+	User findOneWithMultipleSubGraphsUsingNamedEntityGraphById(Integer id);
+
+	// DATAJPA-1041
+	@EntityGraph(attributePaths = { "colleagues", "colleagues.roles", "colleagues.colleagues" })
+	User findOneWithMultipleSubGraphsById(Integer id);
+
 }