mybatis#101: Keep track of pending creations that were set as propert…

…y mappings

- We check for the existence of these (and build them if needed) before returning the final object
- Fixes `testImmutableNestedObjects()`

src/main/java/org/apache/ibatis/executor/resultset/DefaultResultSetHandler.java

@@ -91,6 +91,9 @@ public class DefaultResultSetHandler implements ResultSetHandler {
   private final ObjectFactory objectFactory;
   private final ReflectorFactory reflectorFactory;
 
+  // pending creations property tracker
+  private final Map<Object, PendingRelation> pendingPccRelations = new HashMap<>();
+
   // nested resultmaps
   private final Map<CacheKey, Object> nestedResultObjects = new HashMap<>();
   private final Map<String, Object> ancestorObjects = new HashMap<>();
@@ -387,9 +390,14 @@ private void storeObject(ResultHandler<?> resultHandler, DefaultResultContext<Ob
       ResultMapping parentMapping, ResultSet rs) throws SQLException {
     if (parentMapping != null) {
       linkToParents(rs, parentMapping, rowValue);
-    } else {
-      callResultHandler(resultHandler, resultContext, rowValue);
+      return;
+    }
+
+    if (pendingPccRelations.containsKey(rowValue)) {
+      createPendingConstructorCreations(rowValue);
     }
+
+    callResultHandler(resultHandler, resultContext, rowValue);
   }
 
   @SuppressWarnings("unchecked" /* because ResultHandler<?> is always ResultHandler<Object> */)
@@ -1265,6 +1273,36 @@ private boolean applyNestedPendingConstructorCreations(ResultSetWrapper rsw, Res
     return foundValues;
   }
 
+  private void createPendingConstructorCreations(Object rowValue) {
+    // handle possible pending creations within this object
+    // by now, the property mapping has been completely built, we can reconstruct it
+    final PendingRelation pendingRelation = pendingPccRelations.remove(rowValue);
+    final MetaObject metaObject = pendingRelation.metaObject;
+    final ResultMapping resultMapping = pendingRelation.propertyMapping;
+
+    // get the list to be built
+    Object collectionProperty = instantiateCollectionPropertyIfAppropriate(resultMapping, metaObject);
+    if (collectionProperty != null) {
+      // we expect pending creations now
+      final Collection<Object> pendingCreations = (Collection<Object>) collectionProperty;
+
+      // remove the link to the old collection
+      metaObject.setValue(resultMapping.getProperty(), null);
+
+      // create new collection property
+      collectionProperty = instantiateCollectionPropertyIfAppropriate(resultMapping, metaObject);
+      final MetaObject targetMetaObject = configuration.newMetaObject(collectionProperty);
+
+      // create the pending objects
+      for (Object pendingCreation : pendingCreations) {
+        if (pendingCreation instanceof PendingConstructorCreation) {
+          final PendingConstructorCreation pendingConstructorCreation = (PendingConstructorCreation) pendingCreation;
+          targetMetaObject.add(pendingConstructorCreation.create(objectFactory, false));
+        }
+      }
+    }
+  }
+
   private void verifyPendingCreationPreconditions(ResultMapping parentMapping) {
     if (parentMapping != null) {
       throw new ExecutorException(
@@ -1481,6 +1519,17 @@ private void linkObjects(MetaObject metaObject, ResultMapping resultMapping, Obj
     if (collectionProperty != null) {
       final MetaObject targetMetaObject = configuration.newMetaObject(collectionProperty);
       targetMetaObject.add(rowValue);
+
+      // it is possible for pending creations to get set via property mappings,
+      // keep track of these, so we can rebuild them.
+      final Object originalObject = metaObject.getOriginalObject();
+      if (rowValue instanceof PendingConstructorCreation && !pendingPccRelations.containsKey(originalObject)) {
+        PendingRelation pendingRelation = new PendingRelation();
+        pendingRelation.propertyMapping = resultMapping;
+        pendingRelation.metaObject = metaObject;
+
+        pendingPccRelations.put(originalObject, pendingRelation);
+      }
     } else {
       metaObject.setValue(resultMapping.getProperty(), rowValue);
     }

src/site/markdown/sqlmap-xml.md

@@ -759,7 +759,7 @@ User{username=Peter, roles=[Users, Maintainers, Approvers]}
 This functionality is still experimental, please report any issues you may find on the issue tracker.
 
 It is important to note that mixed mappings have limited support, i.e. property mappings combined with nested constructor mappings are likely to fail.
-When using this functionality, it is preferable for the entire mapping hieracrhy to use immutable constructor mappings.
+When using this functionality, it is preferable for the entire mapping hierarchy to use immutable constructor mappings.
 
 #### association
 

src/test/java/org/apache/ibatis/submitted/collection_in_constructor/CollectionInConstructorTest.java

@@ -165,19 +165,7 @@ void testCollectionArgWithTypeHandler() {
   }
 
   @Test
-  @Disabled
   void testImmutableNestedObjects() {
-    /*
-     * This resultmap contains mixed property and constructor mappings, the logic assumes the entire chain will be
-     * immutable when we have mixed mappings, we don't know when to create the final object, as property mappings could
-     * still be modified at any point in time This brings us to a design question, is this really what we want from this
-     * functionality, as the point was to create immutable objects in my opinion, supporting this defeats the purpose;
-     * for example propery mapping -> immutable collection -> immutable object -> mapped by property mapping. we cannot
-     * build the final object if it can still be modified; i.e, the signal to build the immutable object is lost. the
-     * code in this pr assumes and relies on the base object also being immutable, i.e: constructor mapping -> immutable
-     * collection -> immutable object -> mapped by constructor mapping. Imo, there is only one option here, it should be
-     * added in the documentation; as doing (and supporting this, will be extremely complex)
-     */
     try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
       Mapper mapper = sqlSession.getMapper(Mapper.class);
       Container container = mapper.getAContainer();

src/test/java/org/apache/ibatis/submitted/collection_injection/CollectionInjectionTest.java

@@ -24,6 +24,7 @@
 import org.apache.ibatis.session.SqlSession;
 import org.apache.ibatis.session.SqlSessionFactory;
 import org.apache.ibatis.session.SqlSessionFactoryBuilder;
+import org.apache.ibatis.submitted.collection_injection.immutable.HousePortfolio;
 import org.apache.ibatis.submitted.collection_injection.immutable.ImmutableDefect;
 import org.apache.ibatis.submitted.collection_injection.immutable.ImmutableFurniture;
 import org.apache.ibatis.submitted.collection_injection.immutable.ImmutableHouse;
@@ -117,4 +118,13 @@ private static void assertResult(StringBuilder builder) {
 
     assertThat(builder.toString()).isNotEmpty().isEqualTo(expected);
   }
+
+  @Test
+  void getHousePortfolio() {
+    try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
+      final ImmutableHouseMapper mapper = sqlSession.getMapper(ImmutableHouseMapper.class);
+      final HousePortfolio portfolio = mapper.getHousePortfolio(1);
+      Assertions.assertNotNull(portfolio);
+    }
+  }
 }

src/test/java/org/apache/ibatis/submitted/collection_injection/immutable/HousePortfolio.java

@@ -0,0 +1,25 @@
+package org.apache.ibatis.submitted.collection_injection.immutable;
+
+import java.util.List;
+
+public class HousePortfolio {
+
+  private int portfolioId;
+  private List<ImmutableHouse> houses;
+
+  public int getPortfolioId() {
+    return portfolioId;
+  }
+
+  public void setPortfolioId(int portfolioId) {
+    this.portfolioId = portfolioId;
+  }
+
+  public List<ImmutableHouse> getHouses() {
+    return houses;
+  }
+
+  public void setHouses(List<ImmutableHouse> houses) {
+    this.houses = houses;
+  }
+}

src/test/java/org/apache/ibatis/submitted/collection_injection/immutable/ImmutableHouseMapper.java

@@ -23,4 +23,5 @@ public interface ImmutableHouseMapper {
 
   ImmutableHouse getHouse(int it);
 
+  HousePortfolio getHousePortfolio(int id);
 }

src/test/resources/org/apache/ibatis/submitted/collection_injection/immutable/ImmutableHouseMapper.xml

@@ -63,7 +63,11 @@
     </resultMap>
 
     <sql id="housesSelect">
-        select h.*
+        select
+
+          1 as portfolioId
+
+          , h.*
 
           , r.id as room_id
           , r.name as room_name
@@ -92,4 +96,12 @@
         where h.id = #{id}
     </select>
 
+    <resultMap id="housePortfolioMap" type="HousePortfolio">
+      <id property="portfolioId" javaType="_int" column="portfolioId"/>
+      <collection property="houses" resultMap="houseMap"/>
+    </resultMap>
+
+    <select id="getHousePortfolio" resultMap="housePortfolioMap" resultOrdered="true">
+      <include refid="housesSelect"/>
+    </select>
 </mapper>

src/test/resources/org/apache/ibatis/submitted/collection_injection/mybatis_config.xml

@@ -34,6 +34,7 @@
         <typeAlias type="org.apache.ibatis.submitted.collection_injection.immutable.ImmutableRoomDetail" alias="ImmutableRoomDetail"/>
         <typeAlias type="org.apache.ibatis.submitted.collection_injection.immutable.ImmutableFurniture" alias="ImmutableFurniture"/>
         <typeAlias type="org.apache.ibatis.submitted.collection_injection.immutable.ImmutableDefect" alias="ImmutableDefect"/>
+        <typeAlias type="org.apache.ibatis.submitted.collection_injection.immutable.HousePortfolio" alias="HousePortfolio"/>
     </typeAliases>
 
     <environments default="development">