DATAJDBC-384 - Fixed MyBatisDataAccessStrategy.findAllByPath.

findAllByPath now falls back to the older findAllByProperty for better backward compatibility.
Also the path is included in the query name used for MyBatis.

Original Pull Request: #157

Author: schauder

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/mybatis/MyBatisContext.java

@@ -15,8 +15,10 @@
  */
 package org.springframework.data.jdbc.mybatis;
 
+import java.util.Collections;
 import java.util.Map;
 
+import org.springframework.data.relational.domain.Identifier;
 import org.springframework.lang.Nullable;
 
 /**
@@ -30,18 +32,29 @@ public class MyBatisContext {
 
 	private final Object id;
 	private final Object instance;
+	private final Identifier identifier;
 	private final Class domainType;
 	private final Map<String, Object> additonalValues;
 
 	public MyBatisContext(@Nullable Object id, @Nullable Object instance, Class domainType,
 			Map<String, Object> additonalValues) {
 
 		this.id = id;
+		this.identifier = null;
 		this.instance = instance;
 		this.domainType = domainType;
 		this.additonalValues = additonalValues;
 	}
 
+	public MyBatisContext(Identifier identifier, Object instance, Class<?> domainType) {
+
+		this.id = null;
+		this.identifier = identifier;
+		this.instance = instance;
+		this.domainType = domainType;
+		this.additonalValues = Collections.emptyMap();
+	}
+
 	/**
 	 * The ID of the entity to query/act upon.
 	 *
@@ -52,6 +65,15 @@ public Object getId() {
 		return id;
 	}
 
+	/**
+	 * The {@link Identifier} for a path to query.
+	 *
+	 * @return Might return {@literal null}.
+	 */
+	public Identifier getIdentifier() {
+		return identifier;
+	}
+
 	/**
 	 * The entity to act upon. This is {@code null} for queries, since the object doesn't exist before the query.
 	 * 
@@ -82,4 +104,5 @@ public Class getDomainType() {
 	public Object get(String key) {
 		return additonalValues.get(key);
 	}
+
 }

spring-data-jdbc/src/main/java/org/springframework/data/jdbc/mybatis/MyBatisDataAccessStrategy.java

@@ -20,8 +20,11 @@
 import java.util.Collections;
 import java.util.Map;
 
+import org.apache.ibatis.exceptions.PersistenceException;
 import org.apache.ibatis.session.SqlSession;
 import org.mybatis.spring.SqlSessionTemplate;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import org.springframework.data.jdbc.core.convert.CascadingDataAccessStrategy;
 import org.springframework.data.jdbc.core.convert.DataAccessStrategy;
 import org.springframework.data.jdbc.core.convert.DefaultDataAccessStrategy;
@@ -52,6 +55,8 @@
  */
 public class MyBatisDataAccessStrategy implements DataAccessStrategy {
 
+	private static final Logger LOG = LoggerFactory.getLogger(MyBatisDataAccessStrategy.class);
+
 	private final SqlSession sqlSession;
 	private NamespaceStrategy namespaceStrategy = NamespaceStrategy.DEFAULT_INSTANCE;
 
@@ -245,8 +250,19 @@ public <T> Iterable<T> findAllById(Iterable<?> ids, Class<T> domainType) {
 	@Override
 	public Iterable<Object> findAllByPath(Identifier identifier,
 			PersistentPropertyPath<RelationalPersistentProperty> path) {
-		return sqlSession().selectList(namespace(path.getBaseProperty().getOwner().getType()) + ".findAllByPath",
-				new MyBatisContext(identifier, null, path.getLeafProperty().getType(), Collections.emptyMap()));
+
+		String statementName = namespace(path.getBaseProperty().getOwner().getType()) + ".findAllByPath-"
+				+ path.toDotPath();
+
+		try {
+			return sqlSession().selectList(statementName,
+					new MyBatisContext(identifier, null, path.getRequiredLeafProperty().getType()));
+		} catch (PersistenceException pex) {
+
+			LOG.debug("Didn't find %s in the MyBatis session. Falling back to findAllByPath", pex);
+
+			return DataAccessStrategy.super.findAllByPath(identifier, path);
+		}
 	}
 
 	/*
@@ -255,6 +271,7 @@ public Iterable<Object> findAllByPath(Identifier identifier,
 	 */
 	@Override
 	public <T> Iterable<T> findAllByProperty(Object rootId, RelationalPersistentProperty property) {
+
 		return sqlSession().selectList(
 				namespace(property.getOwner().getType()) + ".findAllByProperty-" + property.getName(),
 				new MyBatisContext(rootId, null, property.getType(), Collections.emptyMap()));
@@ -266,6 +283,7 @@ public <T> Iterable<T> findAllByProperty(Object rootId, RelationalPersistentProp
 	 */
 	@Override
 	public <T> boolean existsById(Object id, Class<T> domainType) {
+
 		return sqlSession().selectOne(namespace(domainType) + ".existsById",
 				new MyBatisContext(id, null, domainType, Collections.emptyMap()));
 	}

spring-data-jdbc/src/test/java/org/springframework/data/jdbc/mybatis/MyBatisDataAccessStrategyUnitTests.java

@@ -16,22 +16,24 @@
 package org.springframework.data.jdbc.mybatis;
 
 import static java.util.Arrays.*;
+import static java.util.Collections.*;
 import static org.assertj.core.api.Assertions.*;
 import static org.mockito.Mockito.*;
 
 import java.util.Collections;
 
+import org.apache.ibatis.exceptions.PersistenceException;
 import org.apache.ibatis.session.SqlSession;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.ArgumentCaptor;
 import org.mockito.Mockito;
-
 import org.springframework.data.jdbc.core.PropertyPathTestingUtils;
 import org.springframework.data.jdbc.core.mapping.JdbcMappingContext;
 import org.springframework.data.mapping.PersistentPropertyPath;
 import org.springframework.data.relational.core.mapping.RelationalMappingContext;
 import org.springframework.data.relational.core.mapping.RelationalPersistentProperty;
+import org.springframework.data.relational.domain.Identifier;
 
 /**
  * Unit tests for the {@link MyBatisDataAccessStrategy}, mainly ensuring that the correct statements get's looked up.
@@ -48,7 +50,8 @@ public class MyBatisDataAccessStrategyUnitTests {
 
 	MyBatisDataAccessStrategy accessStrategy = new MyBatisDataAccessStrategy(session);
 
-	PersistentPropertyPath<RelationalPersistentProperty> path = PropertyPathTestingUtils.toPath("one.two", DummyEntity.class, context);
+	PersistentPropertyPath<RelationalPersistentProperty> path = PropertyPathTestingUtils.toPath("one.two",
+			DummyEntity.class, context);
 
 	@Before
 	public void before() {
@@ -127,8 +130,8 @@ public void deleteAllByPath() {
 
 		accessStrategy.deleteAll(path);
 
-		verify(session).delete(
-				eq("org.springframework.data.jdbc.mybatis.MyBatisDataAccessStrategyUnitTests$DummyEntityMapper.deleteAll-one-two"),
+		verify(session).delete(eq(
+				"org.springframework.data.jdbc.mybatis.MyBatisDataAccessStrategyUnitTests$DummyEntityMapper.deleteAll-one-two"),
 				captor.capture());
 
 		assertThat(captor.getValue()) //
@@ -285,6 +288,65 @@ public void findAllByProperty() {
 				);
 	}
 
+	@SuppressWarnings("unchecked")
+	@Test // DATAJDBC-384
+	public void findAllByPath() {
+
+		RelationalPersistentProperty property = mock(RelationalPersistentProperty.class, RETURNS_DEEP_STUBS);
+		PersistentPropertyPath path = mock(PersistentPropertyPath.class, RETURNS_DEEP_STUBS);
+
+		when(path.getBaseProperty()).thenReturn(property);
+		when(property.getOwner().getType()).thenReturn((Class) String.class);
+
+		when(path.getRequiredLeafProperty()).thenReturn(property);
+		when(property.getType()).thenReturn((Class) Number.class);
+
+		when(path.toDotPath()).thenReturn("dot.path");
+
+		accessStrategy.findAllByPath(Identifier.empty(), path);
+
+		verify(session).selectList(eq("java.lang.StringMapper.findAllByPath-dot.path"), captor.capture());
+
+		assertThat(captor.getValue()) //
+				.isNotNull() //
+				.extracting( //
+						MyBatisContext::getInstance, //
+						MyBatisContext::getId, //
+						MyBatisContext::getIdentifier, //
+						MyBatisContext::getDomainType, //
+						c -> c.get("key") //
+				).containsExactly( //
+						null, //
+						null, //
+						Identifier.empty(), //
+						Number.class, //
+						null //
+				);
+	}
+
+	@SuppressWarnings("unchecked")
+	@Test // DATAJDBC-384
+	public void findAllByPathFallsBackToFindAllByProperty() {
+
+		RelationalPersistentProperty property = mock(RelationalPersistentProperty.class, RETURNS_DEEP_STUBS);
+		PersistentPropertyPath path = mock(PersistentPropertyPath.class, RETURNS_DEEP_STUBS);
+
+		when(path.getBaseProperty()).thenReturn(property);
+		when(property.getOwner().getType()).thenReturn((Class) String.class);
+
+		when(path.getRequiredLeafProperty()).thenReturn(property);
+		when(property.getType()).thenReturn((Class) Number.class);
+
+		when(path.toDotPath()).thenReturn("dot.path");
+
+		when(session.selectList(any(), any())).thenThrow(PersistenceException.class).thenReturn(emptyList());
+
+		accessStrategy.findAllByPath(Identifier.empty(), path);
+
+		verify(session, times(2)).selectList(any(), any());
+
+	}
+
 	@Test // DATAJDBC-123
 	public void existsById() {
 

src/main/asciidoc/jdbc.adoc

@@ -517,12 +517,19 @@ Note that the type used for prefixing the statement name is the name of the aggr
 
 `getDomainType`: The type of the entity to load.
 
-| `findAllByProperty-<propertyName>` | Select a set of entities that is referenced by another entity. The type of the referencing entity is used for the prefix. The referenced entities type is used as the suffix. | All `find*` methods.|
+| `findAllByProperty-<propertyName>` | Select a set of entities that is referenced by another entity. The type of the referencing entity is used for the prefix. The referenced entities type is used as the suffix. _This method is deprecated. Use `findAllByPath` instead_ | All `find*` methods. If no query is defined for `findAllByPath`|
 
 `getId`: The ID of the entity referencing the entities to be loaded.
 
 `getDomainType`: The type of the entity to load.
 
+
+| `findAllByPath-<propertyPath>` | Select a set of entities that is referenced by another entity via a property path. | All `find*` methods.|
+
+`getIdentifier`: The `Identifier` holding the id of the aggregate root plus the keys and list indexes of all path elements.
+
+`getDomainType`: The type of the entity to load.
+
 | `count` | Count the number of aggregate root of the type used as prefix | `count` |
 
 `getDomainType`: The type of aggregate roots to count.