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@github.comwrote:

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...