From e163a917cf1d816c67538e0c2ff0ed4de49210c3 Mon Sep 17 00:00:00 2001 From: Willie Scholtz Date: Wed, 20 Nov 2024 12:56:03 +0100 Subject: [PATCH] #101: Keep track of pending creations that were set as property mappings - We check for the existence of these (and build them if needed) before returning the final object - Fixes `testImmutableNestedObjects()` --- .../resultset/DefaultResultSetHandler.java | 53 ++++++++++++++++++- src/site/markdown/sqlmap-xml.md | 2 +- .../CollectionInConstructorTest.java | 12 ----- .../CollectionInjectionTest.java | 10 ++++ .../immutable/HousePortfolio.java | 25 +++++++++ .../immutable/ImmutableHouseMapper.java | 1 + .../immutable/ImmutableHouseMapper.xml | 14 ++++- .../collection_injection/mybatis_config.xml | 1 + 8 files changed, 102 insertions(+), 16 deletions(-) create mode 100644 src/test/java/org/apache/ibatis/submitted/collection_injection/immutable/HousePortfolio.java diff --git a/src/main/java/org/apache/ibatis/executor/resultset/DefaultResultSetHandler.java b/src/main/java/org/apache/ibatis/executor/resultset/DefaultResultSetHandler.java index f7eed8e4159..77cf9adb0d7 100644 --- a/src/main/java/org/apache/ibatis/executor/resultset/DefaultResultSetHandler.java +++ b/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 pendingPccRelations = new HashMap<>(); + // nested resultmaps private final Map nestedResultObjects = new HashMap<>(); private final Map ancestorObjects = new HashMap<>(); @@ -387,9 +390,14 @@ private void storeObject(ResultHandler resultHandler, DefaultResultContext is always ResultHandler */) @@ -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 pendingCreations = (Collection) 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); } diff --git a/src/site/markdown/sqlmap-xml.md b/src/site/markdown/sqlmap-xml.md index 3089c5b9579..e6abf781dd9 100644 --- a/src/site/markdown/sqlmap-xml.md +++ b/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 diff --git a/src/test/java/org/apache/ibatis/submitted/collection_in_constructor/CollectionInConstructorTest.java b/src/test/java/org/apache/ibatis/submitted/collection_in_constructor/CollectionInConstructorTest.java index 1f00f9a74a1..4a9edb3f819 100644 --- a/src/test/java/org/apache/ibatis/submitted/collection_in_constructor/CollectionInConstructorTest.java +++ b/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(); diff --git a/src/test/java/org/apache/ibatis/submitted/collection_injection/CollectionInjectionTest.java b/src/test/java/org/apache/ibatis/submitted/collection_injection/CollectionInjectionTest.java index c6079320884..2fd006805c3 100644 --- a/src/test/java/org/apache/ibatis/submitted/collection_injection/CollectionInjectionTest.java +++ b/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); + } + } } diff --git a/src/test/java/org/apache/ibatis/submitted/collection_injection/immutable/HousePortfolio.java b/src/test/java/org/apache/ibatis/submitted/collection_injection/immutable/HousePortfolio.java new file mode 100644 index 00000000000..287e4403b02 --- /dev/null +++ b/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 houses; + + public int getPortfolioId() { + return portfolioId; + } + + public void setPortfolioId(int portfolioId) { + this.portfolioId = portfolioId; + } + + public List getHouses() { + return houses; + } + + public void setHouses(List houses) { + this.houses = houses; + } +} diff --git a/src/test/java/org/apache/ibatis/submitted/collection_injection/immutable/ImmutableHouseMapper.java b/src/test/java/org/apache/ibatis/submitted/collection_injection/immutable/ImmutableHouseMapper.java index a9c85fc033b..5a97da27ae9 100644 --- a/src/test/java/org/apache/ibatis/submitted/collection_injection/immutable/ImmutableHouseMapper.java +++ b/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); } diff --git a/src/test/resources/org/apache/ibatis/submitted/collection_injection/immutable/ImmutableHouseMapper.xml b/src/test/resources/org/apache/ibatis/submitted/collection_injection/immutable/ImmutableHouseMapper.xml index dc8c52a7415..f504a62fb03 100644 --- a/src/test/resources/org/apache/ibatis/submitted/collection_injection/immutable/ImmutableHouseMapper.xml +++ b/src/test/resources/org/apache/ibatis/submitted/collection_injection/immutable/ImmutableHouseMapper.xml @@ -63,7 +63,11 @@ - 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} + + + + + + diff --git a/src/test/resources/org/apache/ibatis/submitted/collection_injection/mybatis_config.xml b/src/test/resources/org/apache/ibatis/submitted/collection_injection/mybatis_config.xml index e6fb9dde9d5..735a80163e2 100644 --- a/src/test/resources/org/apache/ibatis/submitted/collection_injection/mybatis_config.xml +++ b/src/test/resources/org/apache/ibatis/submitted/collection_injection/mybatis_config.xml @@ -34,6 +34,7 @@ +