MyBatis Constructor Injection should support <collection> as well?

[djvdorp]

I currently have the problem that it seems like MyBatis constructor injection does not support the <collection> in resultMaps and I don't seem to understand why. I have already read this previous issues related:
mybatis/old-google-code-issues#15
mybatis/old-google-code-issues#480

There also is no example in the docs using <collection> in constructor injection:
for example I should expect it here:
http://mybatis.github.io/mybatis-3/sqlmap-xml.html#Result_Maps --> Advanced Result Maps

Reasoning behind this issue:

We want Immutable objects without no-args constructors and setters, only final fields which are set in a constructor with all info needed to set those fields. Some of the fields are a Set so I expect the constructor injection to support as this currently works without constructor injection (but we need a private constructor for this, and the fields cannot be made final now).

Let say we have the following table structure:

OBJA_ID | OBJA_NAME | OBJB_RELATED_NAME
---------------------------------------------------------------------------
1       | NAME1     | A
1       | NAME1     | B
1       | NAME1     | C
2       | NAME2     | D
2       | NAME2     | E

We have the following object structure and hierarchy:

public class ObjA {

private String name;

private Set<ObjB> objectBs;

private ObjA() {
// MyBatis only constructor
}

public ObjA(String name, Set<ObjB> objectBs) {
this.name = name;
this.objectBs = objectBs;
}

}

public class ObjB {

private String relatedName;

private ObjB() {
// MyBatis only constuctor
}

public ObjB(String relatedName) {
this.relatedName = relatedName;
}

}

I expect ObjA with ID 1 to have a Set { 1,2,3 } and ObjA with ID 2 to have a Set { 4,5 }.

We currently map it this way, but we need that private constructor and can't make the fields final:

<resultMap id="ObjAResult" type="ObjA" >
        <result property="name" column="OBJA_NAME"/>
         <collection property="objectBs" resultMap="ObjBResult"/>
    </resultMap>

    <resultMap id="ObjBResult" type="ObjB">
         <result property="relatedName" column="OBJB_RELATED_NAME"/>
    </resultMap>

We would prefer the following objects, by using constructor injection:

public class ObjA {

private final String name;

private final Set<ObjB> objectBs;

public ObjA(String name, Set<ObjB> objectBs) {
this.name = name;
this.objectBs = objectBs;
}

}

public class ObjB {

private final String relatedName;

public ObjB(String relatedName) {
this.relatedName = relatedName;
}

}

For this we would use something like the following resultmap:

<resultMap id="ObjAResult" type="ObjA" >
        <constructor>
              <arg column="OBJA_NAME" javaType="String">
              <arg resultMap="ObjBResult" javaType="java.util.Set">
        </constructor>
    </resultMap>

    <resultMap id="ObjBResult" type="ObjB">
        <constructor>
              <arg column="OBJB_RELATED_NAME" javaType="String">
        </constructor>
    </resultMap>

However, it seems impossible to use the right javaType here (java.util.Set<ObjB>) in the constructor, and the biggest issue is that there is no <collection> behaviour here, meaning that ObjA with ID 1 will never have a Set { 1,2,3 } and ObjA with ID 2 will never have a Set { 4,5 }, because it's the <collection>behaviour that does this, and this <collection>can not be used when using constructor injection.

Referring to the 2 other related issues in the past (Google Code links), it seems that I am not the only one with this problem, but it bothers me a lot, as I think the cleaner solution here (constructor injection everywhere) should just be possible, instead of private constructors for MyBatis only and Reflection.

I hope all is clear and look forward to any response,
Daniel

[emacarron]

This issue has been there for two years because the change is quite tricky. I will have a look to see what can be done and tell you back.

[emacarron]

Hi. I have a working version in a forked repo. It needs a lot of cleaning yet. There is a test for this in BindingTest#shouldExecuteBoundSelectBlogUsingConstructorWithResultMapCollection()

I have found a pair of problems that I would like to share.

When processing resultmaps, we first create the parent, then the children. If a parent has a collection as an argument, when we create it there is no way to know if it will have elements or not. So if a bean has a constructor with a collection it will have always an not null empty collection.

The second problem is that, given the way we fill objects, the collection will grow after the parent object is created.

Is all this acceptable?

[djvdorp]

Thanks for your prompt reply.

I am currently overthinking the impact of the pair of problems that you shared with us above. I will try to post back to you as soon I get my head around the impact, thanks for taking the issue so serious and your work on a fork with this feature working.

[djvdorp]

Hi. I have taken some time to think about the pair of problems that you've shared with us earlier:

The problems that you've shared will most likely have a negative impact on the initial idea and issue report by me, specifically the part about our main goal to make truly immutable objects.

While the solution you've proposed and demonstrated in your fork of mybatis-3 does manage to make constructor injection work for collections, the <collections> given as arguments in the constructor injection parts are not filled at the time they're passed as a constructor argument (as you explained in pt 1 of your pair of problems) and because it will grow after the parent object is created (pt 2 of your pair of problems), the constructor does not have full control over the collection.

For example, if you'd want the constructor to handle the collection (passed in through constructor injection) differently depending on the .size() of the collection, the pair of problems you've mentioned would make this a very tough exercise.

While our current solution for this (private constructors for MyBatis only and Reflection) also doesn't facilitate in my previously sketched example, it at least feels more transparent and clear for the users of the library, as they don't have to have knowledge of the internal workings of the framework (MyBatis 3).
(For example, that parents are created before children and that the collection will grow afterwards)

Please do understand however, that this is no criticism on your work and I appreciate the work on the fork very much.
If I had known the pair of problems you've shared before posting this issue (maybe an explaination in the docs why <collection> is not used with constructor injection in MyBatis and it's examples), I might have not even opened the issue as I see the change would be quite tricky and complex.

I am looking forward to your point of view on this input.

[emacarron]

Hi Daniel. I do agree with you.

Those two points I told are key. If we provide parameters to a constructor, that paremeters should be inmutable, otherwise this will be a "leaky" solution and may end up in creating more problems than it solves.

Right now, when processing joins, collections are filled while the ResultSet is read. So what I did is creating the collection in advance and storing the reference for further use.

I am sure that we can create a mapping logic that works fine for constructors but the change will be massive. Not for the 3.2.x branch for sure.

Regarding the code I wrote. No problem at all in throwing it away. Sometimes we get to good ideas, somethimes not.

Given that this is not impossible but just difficult I will keep the issue open. Maybe someone has an stroke of genius in the feature :)

For the time being I think we should just throw an exception when the constructor arg is a collection and not a single object.

[mnesarco]

To support this, we need to defer the bean creation (constructor call)
until the relations (association, collections) are fully populated, and
there could be many levels of nested collections. So this issue requires an
intermediate temporary structure to hold the data until they are ready to
be injected into their owners.

I think it is a big performance problem.

Another way is to support this only for "Nested Select" but not for "Nested
Results" but then, you will have the N+1 queries problem.

On Mon, Nov 25, 2013 at 7:15 AM, Eduardo Macarron
notifications@git.luolix.topwrote:

Hi Daniel. I do agree with you.

Those two points I told are key. If we provide parameters to a
constructor, that paremeters should be inmutable, otherwise this will be a
"leaky" solution and may end up in creating more problems than it solves.

Right now, when processing joins, collections are filled while the
ResultSet is read. So what I did is creating the collection in advance and
storing the reference for further use.

I am sure that we can create a mapping logic that works fine for
constructors but the change will be massive. Not for the 3.2.x branch for
sure.

Regarding the code I wrote. No problem at all in throwing it away.
Sometimes we get to good ideas, somethimes not.

Given that this is not impossible but just difficult I will keep the issue
open. Maybe someone has an stroke of genius in the feature :)

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/101#issuecomment-29196998
.

Frank D. Martínez M.

[emacarron]

Yes, probably using a meta-structure alone could be acceptable from a performance view, but when processing a join we can only be sure that is no more data yet to come when the ResultSet is fully read.

So the whole object tree should remain in memory using that meta-structure as a node and then travese the whole tree to build the result. Probably this means double memory and double time.

This is why I doubt it is worth paying this price just for the constructor feature.

[mnesarco]

Just a crazy idea:

The problem with this is the fact that reading a collection from a jdbc
resultset is an incremental task. so to build immutable beans we need to
wait until the incremental list construction finishes. But what if we
inject a kind of Future in the constructor as we can do in Scala? so
the constructor of the bean can register a listener to react to when the
collection is filled.

More: All associations/collections can be implemented as Future :)

The bad part is that the beans must be aware of the Futureness of the
relations and mybatis philosophy is to maintain beans as POJOs.

It was just a crazy idea.

Frank.

On Mon, Nov 25, 2013 at 12:05 PM, Eduardo Macarron <notifications@github.com

wrote:

Yes, probably using a meta-structure alone could be acceptable from a
performance view, but when processing a join we can only be sure that is no
more data yet to come when the ResultSet is fully read.

So the whole object tree should remain in memory using that meta-structure
as a node and then travese the whole tree to build the result. Probably
this means double memory and double time.

This is why I doubt it is worth paying this price just for the constructor
feature.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/101#issuecomment-29219858
.

Frank D. Martínez M.

[emacarron]

That will not differ too much to the patch I did in the fork. I just created an empty collection that was going filled while the RS was being read.

Jus for the record. this is in fact this how mybatis injects collections for properties. A bean with collection property may expect that when MyBatis sets the collection it is fully filled but it is not. We can live with that in the case of the property but gets too weird when it happens in a constructor.

[mnesarco]

The fundamental difference is that there will not be a growing collection
visible from the bean. There will be an initial empty future object, and at
some point in the future a full filled collection will be injected. I will
do some experiments in a fork.
El 29/11/2013 00:48, "Eduardo Macarron" notifications@github.com escribió:

That will not differ too much to the patch I did in the fork. I just
created an empty collection that was going filled while the RS was being
read.

Jus for the record. this is in fact this how mybatis injects collections
for properties. A bean with collection property may expect that when
MyBatis sets the collection it is fully filled but it is not. We can live
with that in the case of the property but gets too weird when it happens in
a constructor.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/101#issuecomment-29497875
.

[usethe4ce]

BTW, if the parent object is immutable, the collection is immutable, and the child object in the collection is also immutable, and then you also want the child to hold a reference to the parent (I forget, is this supported?), you end up with a chicken-and-egg problem in construction, tricky though not intractable.

Some thoughts....

Passing in a mutable collection that will be filled after construction seems like a reasonable concession, a very simple way to cover 90% of real-world cases. If the collection type is one that can be "locked down", made immutable after being filled, then the remaining important complaints would be (1) that a constructor cannot use the contents of the collection to initialize other fields, and (2) that, as with setters, even if you can defer certain initialization until the collection is filled, there is no way to be notified when that happens.

The cases where a partially filled collection gets accessed are harder to envision. If it's really an issue, you can either make the collection class itself play empty (or "not yet filled", possibly as an exception) until it's complete, possibly via a proxy collection, or do the same via some other wrapper object such as a future. In the latter case, users would have the option to define constructors taking that type, rather than a simple collection, and MyBatis could support both.

Being able to react (via a callback?) to a collection being filled seems more useful, though still an uncommon need. The thing to beware here is that you should not consider a collection filled until all its descendants are also filled.

[emacarron]

You can indeed hold a reference from a child to a parent. But I see no problem in throwing an exception when a loop is found, that is not a constraint created by MyBatis because you cannot do it in java either so I see no problem there.

You are right when you point that a mutable collection may work in most of the cases, but the main reason to use a constructor is building real inmuatble objects and that will still be impossible. I think it is not worth doing that change.

IMHO I think that using a proxy, a custom object or an interface for a callback is going in the opposite direction.I would like that people can use MyBatis without reading the manual!! :) (Most of then won't read it anyway so better use plain javabeans that probably everybody knows). This is a MyBatis issue and I think we should fix it completely or fail explicitly.

The problem can be solved at a cost of traversing the tree from down to top to build the final result when the ResultSet is fully consumed. Unfortunately this will probably happen also for "normal" objects with no constructors. That is a CPU intesive operation. Is the change worth?

[harawata]

IMHO, a custom ResultHandler or ResultSetHandler is a better fit for such a complex immutable object.
It would be more memory efficient as the developer knows when the collection is filled...

[usethe4ce]

I would definitely want at least basic support for immutable objects with collections, even with the caveat that the collection itself is not initially immutable.

Basically, there are two usage scenarios. The most common one is where MyBatis loads everything into strongly typed objects, then user code takes over and performs any analysis on the complete result. The types MyBatis usually works with here are strictly data objects with no logic. This is very straightforward and problem-free.

Further, if the collections become immutable by the time MyBatis is done, then immutability is not compromised in this scenario. In practice, though, I doubt that most users who would use immutable types are so fanatical about bulletproofing them.

The other scenario is where users try to get clever and do something while the incoming data is still being processed. Well, ideally they would not need to know when that happens, or be able to get into trouble, and with a major overhaul MyBatis could construct the object graph in a suitable order such that constructors get already-filled collections. (BTW, it would also be helpful for setters to receive already-filled collections.) That kind of change is worth deferring as a separate issue, and users actually impacted by the current way collections are filled can be considered the advanced users who will read the manual and use things like ResultSetHandler.

So, including your change benefits users in the first scenario, which I believe is the majority, and leaves those in the second scenario no worse off.

Thanks for working on this!

[BenRomberg]

To the OP:

A (non MyBatis) workaround would be to create builder objects, that are only used in the persistence layer together with MyBatis. Then you have a build() method within those classes, constructing your pretty model objects just the way you want them.

Of course it would be much easier if MyBatis would support this out of the box. I've stumbled across the same issue when getting familiar with MyBatis.

[technok]

Is the fix for injecting collections via constructor available now ?

[h3adache]

I'm jumping on this issue 5 years late but I don't see the difference in how we construct objects with collections now and how we could for constructor injection in terms of memory. We currently hold an object until it's fully populated already in MetaObjects. And we do not 'release' this until the ResultSet has been fully processed. So I'm unclear on why you think this would require double memory and time @emacarron

So the whole object tree should remain in memory using that meta-structure as a node and then travese the whole tree to build the result. Probably this means double memory and double time.

[nlenoire]

Hello,

Is there any plan to support this feature in a short term?

[rsprinkle]

If I understand correctly the issue with implementing this is as follows:

1. A primary use case for constructor injection is the creation of immutable objects (this is in fact my use case).
2. Collections are not fully populated when injected into beans, so are not immutable.
3. Collections cannot be easily, fully constructed before injection, because MyBatis places no requirements on the order of data in a ResultSet and MyBatis also allows Collections to be populated by queries that return multiple ResultSets.

I love MyBatis, but I think that the documentation around mapping files needs to be expanded to provide more depth.

[stahloss]

Hello,

Is there any plan to support this feature in a short term?

It's been 8 years soon. Wouldn't count on it.

[paper-tiger]

My UseCase is combining http://immutables.github.io/ with MyBatis.
I generate my entities using @modifiable and @immutable. That way the generated modifiable classes have a toImmutable method.
My workaround to getting immutables out of my Mappers while also using collections and associations is to use to following simple interceptor which converts to Modifiables to Immutables after they have been fully constructed.

@Intercepts({ @Signature(type = ResultSetHandler.class, method = "handleResultSets", args = { Statement.class }) })
public class ModifiableToImmutablePlugin implements Interceptor
{
    private Properties properties = new Properties();

    @Override
    public Object intercept(Invocation invocation) throws Throwable
    {
        Object returnObject = invocation.proceed();

        if (returnObject instanceof List)
        {
            List list = (List) returnObject;
            list.replaceAll(this::convertToImmutableIfModifiable);
        }
        return convertToImmutableIfModifiable(returnObject);
    }

    private Object convertToImmutableIfModifiable(Object returnObject)
    {
        Class<? extends Object> returnObjectClass = returnObject.getClass();
        if (isModifiableClass(returnObjectClass))
        {
            Optional<Method> toImmutableMethod = getToImmutableMethod(returnObjectClass);

            if (toImmutableMethod.isPresent())
            {
                try
                {
                    return toImmutableMethod.get()
                                            .invoke(returnObject);
                }
                catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e)
                {
                    throw new RuntimeException(e);
                }
            }
            else
            {
                return returnObject;
            }
        }

        return returnObject;
    }

    private Optional<Method> getToImmutableMethod(Class<? extends Object> returnObjectClass)
    {
        return Stream.of(returnObjectClass.getMethods())
                     .filter(m -> m.getName()
                                   .equals("toImmutable"))
                     .findAny();
    }

    private boolean isModifiableClass(Class<? extends Object> returnObjectClass)
    {
        return returnObjectClass.getSimpleName()
                                .startsWith("Modifiable");
    }

    @Override
    public void setProperties(Properties properties)
    {
        this.properties = properties;
    }
}

I use only Modifiable classes in my XML-Definitions and only Immutable classes in my Interfaces and for now it seems to work.
Maybe this helps someone.

[dpawlak-pgs]

Have some other solution to pass collection as constructor parameter -> select attribute. I have already checked it and its working.

To deal with composite keys, you can specify multiple column names to pass to the nested select statement by using the syntax column="{prop1=col1,prop2=col2}".

Reference doc: https://mybatis.org/mybatis-3/sqlmap-xml.html

[epochcoder]

I have been experimenting with a solution for this (based on all above comments) in MyBatis itself, will post an MR soon, however it is reaaaly experimental and probably not really production ready, so I hid everything behind a configuration flag.

Basically i'm adding support to build up constructor arguments in memory, when the final object is ready, we call the constructor as a final step, rather than a on-the-fly step. This will suffer from all issues mentioned above, such as chicken/egg with parent child and such.

I had to modify the original test case also, as doing this with nested queries using <collection> is not viable. So i ended up using a nested result map, but this test passes now:

shouldExecuteBoundSelectBlogUsingConstructorWithResultMapCollection

The modified mapper xml looks as follows:

    <resultMap id="blogUsingConstructorWithResultMapCollection" type="Blog">
        <constructor>
            <idArg column="id" javaType="_int"/>
            <arg column="title" javaType="java.lang.String"/>
            <arg javaType="org.apache.ibatis.domain.blog.Author" resultMap="org.apache.ibatis.binding.BoundAuthorMapper.authorResultMap"/>
            <arg javaType="java.util.List" resultMap="postForConstructorInit" /> <!-- note resultmap containing post rather than blog -->
        </constructor>
    </resultMap>

    <resultMap id="postForConstructorInit" type="org.apache.ibatis.domain.blog.Post">
        <id property="id" column="post_id"/>
        <!--    <result property="blog" column="blog_id"/> impossible due to chicken egg -->
        <result property="createdOn" column="created_on"/>
        <result property="section" column="section" javaType="org.apache.ibatis.domain.blog.Section"/>
        <result property="subject" column="subject"/>
        <result property="body" column="body"/>
        <discriminator javaType="int" column="draft">
            <case value="1">
                <association property="author" column="author_id" select="selectAuthorWithInlineParams"/>
            </case>
        </discriminator>
    </resultMap>

TLDR: This only makes using JOIN results possible for constructor injection, it does NOT support the collection property as far as I could test. definitely fixes #3021

But if it is viable, will probably need some help from maintainers to polish it.

While this does work for the mentioned test, a more involved test containing multiple nested resultmaps is not working, so it might be a while before I figure that out. (figured it out)

Below is the full (blog, post comment) example containing only immutable (all fields final) objects which are fully constructed before injection:

<resultMap id="immutableBlogResultMap" type="org.apache.ibatis.domain.blog.immutable.ImmutableBlog">
    <constructor>
        <idArg column="id" javaType="_int"/>
        <arg column="title" javaType="java.lang.String"/>
        <arg javaType="org.apache.ibatis.domain.blog.immutable.ImmutableAuthor" resultMap="immutableAuthorResultMap" columnPrefix="author_"/>
        <arg javaType="java.util.List" resultMap="immutablePostResultMap" columnPrefix="post_" />
    </constructor>
</resultMap>

<resultMap id="immutableAuthorResultMap" type="org.apache.ibatis.domain.blog.immutable.ImmutableAuthor">
    <constructor>
        <idArg column="id" javaType="_int"/>
        <arg column="username" javaType="String"/>
        <arg column="password" javaType="String"/>
        <arg column="email" javaType="String"/>
        <arg column="bio" javaType="String"/>
        <arg column="favorite_section" javaType="org.apache.ibatis.domain.blog.Section"/>
    </constructor>
</resultMap>

<resultMap id="immutablePostResultMap" type="org.apache.ibatis.domain.blog.immutable.ImmutablePost">
    <constructor>
        <idArg column="id" javaType="_int"/>
        <arg javaType="org.apache.ibatis.domain.blog.immutable.ImmutableAuthor" resultMap="immutableAuthorResultMap" columnPrefix="author_"/>
        <arg column="created_on" javaType="Date"/>
        <arg column="section" javaType="org.apache.ibatis.domain.blog.Section"/>
        <arg column="subject" javaType="String"/>
        <arg column="body" javaType="String"/>
        <arg javaType="java.util.List" resultMap="immutablePostCommentResultMap" columnPrefix="comment_" />
        <arg javaType="java.util.List" resultMap="immutablePostTagResultMap" columnPrefix="tag_" />
    </constructor>
</resultMap>

<resultMap id="immutablePostCommentResultMap" type="org.apache.ibatis.domain.blog.immutable.ImmutableComment">
    <constructor>
        <idArg column="id" javaType="_int"/>
        <arg column="name" javaType="String"/>
        <arg column="comment" javaType="String"/>
    </constructor>
</resultMap>

<resultMap id="immutablePostTagResultMap" type="org.apache.ibatis.domain.blog.immutable.ImmutableTag">
    <constructor>
        <idArg column="id" javaType="_int"/>
        <arg column="name" javaType="String"/>
    </constructor>
</resultMap>