3.2.x release: NestedResultSetHandler.handleRowValues discarding row.

[ghost]

I ran into a bug in which I expected a result set of about 3600 rows and get 1. The row I get back is the last row of the result set I see in Oracle SQL Developer. I've traced the issue to handleRowValues() in NestedResultSetHandler. Based on tracing thru that code in the debugger the row value gets set by the line:

rowValue = getRowValue(rs, discriminatedResultMap, rowKey, rowKey, null, resultColumnCache, partialObject);

but then nothing actually happens with the value. It would appear that the if statement right after should be within the bracket directly above it (existing source below):

while (shouldProcessMoreRows(rs, resultContext, rowBounds)) {
  final ResultMap discriminatedResultMap = resolveDiscriminatedResultMap(rs, resultMap, null);
  final CacheKey rowKey = createRowKey(discriminatedResultMap, rs, null, resultColumnCache);
  Object partialObject = objectCache.get(rowKey);
  if (partialObject == null && rowValue != null) { // issue #542 delay calling ResultHandler until object ends
    if (mappedStatement.isResultOrdered()) objectCache.clear(); // issue #577 clear memory if ordered
    callResultHandler(resultHandler, resultContext, rowValue);
  } 
  rowValue = getRowValue(rs, discriminatedResultMap, rowKey, rowKey, null, resultColumnCache, partialObject);
}
if (rowValue != null) callResultHandler(resultHandler, resultContext, rowValue);

In the 3.1.1 release the rowValue is bieng put into another collection:

while (shouldProcessMoreRows(rs, resultContext, rowBounds)) {
  final ResultMap discriminatedResultMap = resolveDiscriminatedResultMap(rs, resultMap, null);
  final CacheKey rowKey = createRowKey(discriminatedResultMap, rs, null, resultColumnCache);
  final boolean knownValue = objectCache.containsKey(rowKey);
  Object rowValue = getRowValue(rs, discriminatedResultMap, rowKey, resultColumnCache);
  if (!knownValue) {
    resultContext.nextResultObject(rowValue);
    resultHandler.handleResult(resultContext);
  }
}

EDIT: maybe not quite that simple as I didn't look at the source enough before posting. Based on more debugging the callResultHandler inside the if statement (thats inside the while loop) is never getting called. That would explain why i am only seeing the last row of the resultset in the returned collection. its being put in there by the second if.

[emacarron]

That is fixed in 3.2.1. Can you try with 3.2.2 and tell the results?

[ghost]

Actually the code I posted above is from 3.2.1. I tried 3.2.2 and 3.2.3 snapshot with the same results before entering this bug. I had to roll back to 3.1.1 for the time being.

[emacarron]

Sorry, the fix was in 3.2.2 so the code you posted is indeed wrong. But I expected it to be fixed in 3.2.2...

We should have a look at it. And I need a way to reproduce it. Can you write a test for this?

https://code.google.com/p/mybatis/wiki/Test

[emacarron]

If not with a test, is there any way I can reproduce the bug?

[ghost]

Eduardo sorry for not getting back to you sooner. I have a software release coming up at work thats kept me pretty busy. Let me look into it a bit. I'll also retry with 3.2.2 to make sure that I was really using that version of the library. Pretty sure I was but certainly worth a quick retest to verify.

[ghost]

Is there a 3.2.2 sources jar I can use during debugging? the issue is still happening with 3.2.2 but i'd like to run it thru the debugger and see what is going on with that version.

[ghost]

Forget it. answered my own question. i'll grab it and debug more.

[ghost]

Eduardo....just did more debugging with 3.2.2. When the call to get the rowKey is made in NestedResultSetHandler.handleRowValues() the key is always coming back to be the same value regardless of the data in the row. In getRowValue when this line: if (combinedKey != CacheKey.NULL_CACHE_KEY) objectCache.put(combinedKey, resultObject); is run the combinedKey is always the same value. So ultimately what is happening is it keeps overwriting the same value in the object cache.

Not sure how I would even start by creating a test case for this as its looping thru the rows properly and returning the values....it just keeps overwriting whats in the cache. If I set partialObject to null manually inside the debugger I can get it to add stuff to the cache and get more than one result back. Not sure how to guide you further. Also....the mapping in question here is a map with three nested maps. Not sure if that helps. It didn't seem to make a difference in the code as I was following in th debugger.

[emacarron]

Hi! Thanks for looking into it!

That would happen if you are using an id element that does not hold a unique value. But... we can be guessing for a long time with no conclusion. For me the best way would be to create a test.

1.- Fork the repo
2.- Create a new package by copy pasting the BaseTest. A short guide is available here: https://code.google.com/p/mybatis/wiki/Test

Try to reproduce the behavior by inserting some test data into the hsqldb database that looks like the real data you have and create a similar resultmap.

Hopefully it will fail with the test.

[emacarron]

Hi again.

Sorry to be annoying. We have some fixes in master that would like to release but I am curious about what happens with this issue.

Note that given that 3.2.2 has been out for more than a month and no other user reported this problem it is very likely that the bug is in your code but cannot be 100% sure without that test.

[ghost]

Eduardo...

Sorry for the delay in responding but I've been down with the flu for about
the last week.

That being said I have not had a chance to look at this at all. Not sure
I'd agree that it's a bug in our code because there is no code....its just
a nested mapping. I even fooled around a bit with the nested maps with the
same result that the hash values being returned inside MyBatis are the same
so we always get back the last row that was put into the collection. On
the other hand if nobody else has reported the issue I can see how that
points to something we're not doing right (at least not right for
3.2....these mappings havent changed in well over a year and work fine with
3.1).

Things are easing up around here a bit so I probably will have a bit more
time to take a look.

thanks,

Jason

On Sat, May 11, 2013 at 5:04 AM, Eduardo Macarron
notifications@github.comwrote:

Hi again.

Sorry to be annoying. We have some fixes in master that would like to
release but I am curious about what happens with this issue.

Note that given that 3.2.2 is being out for more than a month and no other
user reported this problem it is very likely that the bug is in your code
but cannot be 100% sure without that test.

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

[emacarron]

No problem. Take your time. We would like to release 3.2.3 with some minor fixes and enhancements but I would like to wait to see what happens with this issue.

As I said, mappings should work in 3.2 in the same way they did in 3.1

[ghost]

I wouldn't wait on a release. And yes the key point is should work the
same ;). In my case they don't and the source has significantly changed.

Jason

On Thursday, May 16, 2013, Eduardo Macarron wrote:

No problem. Take your time. We would like to release 3.2.3 with some minor
fixes and enhancements but I would like to wait to see what happens with
this issue.

As I said, mappings should work in 3.2 in the same way they did in 3.1

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

[HermanBovens]

I'm having the same problem: only one row is returned for a result map with 1 association using 3.2.2. It works using 3.1.1.
I don't see how to attach files here, so here goes the unit test code:

(org/apache/ibatis/submitted/resultmapwithassociationstest/mybatis-config.xml)

<?xml version="1.0" encoding="UTF-8" ?>
<!DOCTYPE configuration
    PUBLIC "-//mybatis.org//DTD Config 3.0//EN"
    "http://mybatis.org/dtd/mybatis-3-config.dtd">

<configuration>

    <environments default="development">
        <environment id="development">
            <transactionManager type="JDBC">
                <property name="" value="" />
            </transactionManager>
            <dataSource type="UNPOOLED">
                <property name="driver" value="org.hsqldb.jdbcDriver" />
                <property name="url" value="jdbc:hsqldb:mem:resultmapwithassociationstest" />
                <property name="username" value="sa" />
            </dataSource>
        </environment>
    </environments>

    <mappers>
        <mapper resource="org/apache/ibatis/submitted/resultmapwithassociationstest/Mapper.xml" />
    </mappers>

</configuration>

(org/apache/ibatis/submitted/resultmapwithassociationstest/Mapper.xml)

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE mapper
    PUBLIC "-//mybatis.org//DTD Mapper 3.0//EN"
    "http://mybatis.org/dtd/mybatis-3-mapper.dtd">

<mapper namespace="org.apache.ibatis.submitted.resultmapwithassociationstest.Mapper">
  <resultMap id="personRM" type="org.apache.ibatis.submitted.resultmapwithassociationstest.Person" >
    <id property="id"/>
    <association property="address" column="id_address" javaType="org.apache.ibatis.submitted.resultmapwithassociationstest.Address">
      <id property="id" column="address_id" />
    </association>
  </resultMap>

  <select id="findAll" parameterType="map" resultMap="personRM">
    SELECT p.id, id_address, 
      a.id as address_id
    FROM person p
      JOIN address a on a.id = p.id_address
  </select>
</mapper>

(org/apache/ibatis/submitted/resultmapwithassociationstest/CreateDB.sql)

drop table person if exists;
drop table address if exists;

CREATE TABLE address
(
    id int NOT NULL,
    CONSTRAINT pk_address PRIMARY KEY (id),
)
;


CREATE TABLE person
(
    id int NOT NULL,
    id_address integer NOT NULL,
    CONSTRAINT pk_person PRIMARY KEY (id),
    CONSTRAINT fk_person_id_address FOREIGN KEY (id_address) REFERENCES address (id) ON UPDATE RESTRICT ON DELETE RESTRICT
)
;

INSERT INTO address (id) VALUES  (1);
INSERT INTO address (id) VALUES  (2);
INSERT INTO address (id) VALUES  (3);
INSERT INTO person (id, id_address) VALUES (1, 1);
INSERT INTO person (id, id_address) VALUES (2, 2);
INSERT INTO person (id, id_address) VALUES (3, 3);

(org/apache/ibatis/submitted/resultmapwithassociationstest/Mapper.java)

package org.apache.ibatis.submitted.resultmapwithassociationstest;

import java.util.List;

public interface Mapper {
    List<Person> findAll();
}

(org/apache/ibatis/submitted/resultmapwithassociationstest/Address.java)

package org.apache.ibatis.submitted.resultmapwithassociationstest;

public class Address {
    private int id;

    public int getId() {
        return id;
    }

    public void setId(final int id) {
        this.id = id;
    }
}

(org/apache/ibatis/submitted/resultmapwithassociationstest/Person.java)

package org.apache.ibatis.submitted.resultmapwithassociationstest;

public class Person {
    private int id;
    private Address address;

    public int getId() {
        return id;
    }

    public void setId(final int id) {
        this.id = id;
    }

    public Address getAddress() {
        return address;
    }

    public void setAddress(final Address address) {
        this.address = address;
    }
}

(org/apache/ibatis/submitted/resultmapwithassociationstest/ResultMapsWithAssociationsTest.java)

package org.apache.ibatis.submitted.resultmapwithassociationstest;

import java.io.Reader;
import java.sql.Connection;
import java.util.List;

import org.apache.ibatis.io.Resources;
import org.apache.ibatis.jdbc.ScriptRunner;
import org.apache.ibatis.session.SqlSession;
import org.apache.ibatis.session.SqlSessionFactory;
import org.apache.ibatis.session.SqlSessionFactoryBuilder;
import org.junit.Assert;
import org.junit.BeforeClass;
import org.junit.Test;

public class ResultMapWithAssociationsTest {

    private static SqlSessionFactory sqlSessionFactory;

    @BeforeClass
    public static void setUp() throws Exception {
        // create a SqlSessionFactory
        Reader reader = Resources.getResourceAsReader("org/apache/ibatis/submitted/resultmapwithassociationstest/mybatis-config.xml");
        sqlSessionFactory = new SqlSessionFactoryBuilder().build(reader);
        reader.close();

        // populate in-memory database
        SqlSession session = sqlSessionFactory.openSession();
        Connection conn = session.getConnection();
        reader = Resources.getResourceAsReader("org/apache/ibatis/submitted/resultmapwithassociationstest/CreateDB.sql");
        ScriptRunner runner = new ScriptRunner(conn);
        runner.setLogWriter(null);
        runner.runScript(reader);
        reader.close();
        session.close();
    }

    @Test
    public void shouldFindAllPersonRecordsWithAssociatedAddressRecord() {
        SqlSession sqlSession = sqlSessionFactory.openSession();
        try {
            Mapper mapper = sqlSession.getMapper(Mapper.class);
            List<Person> resultList = mapper.findAll();
            Assert.assertEquals(3, resultList.size());
        }
        finally {
            sqlSession.close();
        }
    }

}

Test output:

java.lang.AssertionError: expected:<3> but was:<1>

[ghost]

Herman bless you :) (and i'm not religious). I have been trying to find a spare minute for this so thank you!

[emacarron]

Amen!! :)

Herman. I was able to reproduce the behaviour. Looks like the problem is that the resultmap is wrong. To generate the unique key MyBatis has to know which is the column to get data from. The problem is that the mapping has no column.

<id property="id"/>

It works with this one:

<id column="id"/>

When reproducing it in 3.1 I found out that it works by chance. 3.1 cannot get any value to create the key so it creates a new object. When coding a cyclic resultmap (a person has N addresses, each one has a person) that caused an infite loop on certain circumstances.

So the problem in 3.2 is that it should fail and not return a wrong result. I will try to fix it in this way.

jkratz, can you review you resultmap? Probably in your case the problem is not a missing column attirbute but a wrong column name.

[HermanBovens]

Thanks for the quick resolution!
I thought it would be something like this. Indeed a nice warning/error
message regarding the missing column attribute would be nice :)

Herman

2013/5/29 Eduardo Macarron notifications@github.com

Amen!! :)

Herman. I was able to reproduce the behaviour. Looks like the problem is
that the resultmap is wrong. To generate the unique key MyBatis has to know
which is the column to get data from. The problem is that the mapping has
no column.

It works with this one:

When reproducing it in 3.1 I found out that it works by chance. 3.1 cannot
get any value to create the key so it creates a new object. When coding a
cyclic resultmap (a person has N addresses, each one has a person) that
caused an infite loop on certain circumstances.

So the problem in 3.2 is that it should fail and not return a wrong
result. I will try to fix it in this way.

jkratz, can you review you resultmap? Probably in your case the problem is
not a missing column attirbute but a wrong column name.

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

[emacarron]

Finally I have added a check to ensure the colums is there. If it is not MB will not start up.

In some minutes there should be an snapshot here:
https://oss.sonatype.org/content/repositories/snapshots/org/mybatis/mybatis/3.2.3-SNAPSHOT/

[christianpoitras]

Thanks Eduardo!
That is surprisingly similar to the old issue here https://code.google.com/p/mybatis/issues/detail?id=34.

[ghost]

Eduardo...

In our mapping the ID mapping in use has the right column referenced:

<id property="operationalCustomerId" column="OPER_CUST_ID" />

[emacarron]

That is bad news. But can be, because we made the diagnostic for your case based only on intuition.

You will probably have realised that without a test we will never fix it. We will just chat for a quite a long while instead :)

Please, work on that test and open a new issue once you have it.

[HermanBovens]

Yes, maybe you can use my test as a basis and gradually extend it towards
your case, until it breaks.

2013/6/5 Eduardo Macarron notifications@github.com

That is bad news. But can be, because we made the diagnostic for your case
based only on intuition.

You will probably have realised that without a test we will never fix it.
We will just chat for a quite a long while instead :)

Please, work on that test and open a new issue once you have it.

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

[ghost]

Well...I still need to test with the new version. I'm not convinced the
mapping is correct at this point.

Thanks,

Jason

On Tuesday, June 4, 2013, Eduardo Macarron wrote:

That is bad news. But can be, because we made the diagnostic for your case
based only on intuition.

You will probably have realised that without a test we will never fix it.
We will just chat for a quite a long while instead :)

Please, work on that test and open a new issue once you have it.

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

[paulkrause88]

Hi. Sorry to be joining this conversation so late, but I just tried upgrading to mybatis 3.2.3 and immediately ran afoul of this change. I'm not sure if it is the intended behavior or not, but as a result of this change, ResultMapper.validate() throws an IllegalStateException if any column attribute in a collection is not explicitly defined.

This is a breaking change, and should at the very least be highlighted in the release notes..

[emacarron]

Hi Paul. It was not my intention to change the behaviour. Probably you are going through this block:

// Issue #4 and GH #39: column is optional only in nested resultmaps but not in the rest
if (resultMapping.nestedResultMapId == null && resultMapping.column == null && resultMapping.composites.size() == 0) {
  throw new IllegalStateException("Missing column attribute for nested select in mapping " + resultMapping.property);
}

BTW, now I see that the text is wrong, it should say "Missing column attribute for mapping", this check applies to all mappings, not just nested selects.

Possibilities for collections are:

• For a nested Resultmap the column attribute is not used. The docs shown some samples of nested Resultmaps with a column attribute so we know for sure that there must be a lot of people with that wrong setting. This does not fail, is just ignored.
• For a nested select the column attribute is mandatory, and to my knowledge it has always been this way. If this check is removed it should fail elsewhere.

I suppose that your mapping has a nested select and no column attribute. That means that your nested select will be executed with no input parameters. Is that the case?

[emacarron]

Hi @eyalfa Frank pointed me to this issue. I will have a look at your proposal. Can you please send it as a pull request?

[eyalfa]

I'll try to come up with a repro,is a Scala test acceptable? After all this is how I hit the issue in the first place.

[emacarron]

Errr... the idea is to add that test to the project and keep it there so Java will make my life easier :)

[eyalfa]

I can start by adding it to the mb-scala project,I might be able to pull it off by tomorrow noon (il time),we can later make the effort of porting it to java.

[mnesarco]

Nice Eyal, I will try to get some time to help you to port the scala test
into a java test.

On Wed, Nov 25, 2015 at 4:22 PM, eyalfa notifications@github.com wrote:

I can start by adding it to the mb-scala project,I might be able to pull
it off by tomorrow noon (il time),we can later make the effort of porting
it to java.

—
Reply to this email directly or view it on GitHub
#39 (comment).

Frank D. Martínez M.

[eyalfa]

https://youtu.be/QIKBq9TeFlw 😁

[eyalfa]

ok here goes,
I've created a fork and a branch here: https://github.com/eyalfa/scala/tree/mb3-109-reopen
and the test code can be found here: https://github.com/eyalfa/scala/blob/mb3-109-reopen/mybatis-scala-core/src/test/scala/org/mybatis/scala/mapping/MBIssue39ReproSpec.scala

the test case shows the failing mapping (two associations, no results or ids) and the fix that actually worked for me in which I used inheritance instead of association for one of the fields. This of course required a slight change of my object model (which I could live with as the query results goes through additional transformations after being fetched).

[eyalfa]

Frank, Eduardo,
did you get to see the test case?

please let me know if you need assstance.
Eyal.

On Wed, Nov 25, 2015 at 11:28 PM, Frank David Martínez M <
notifications@github.com> wrote:

Nice Eyal, I will try to get some time to help you to port the scala test
into a java test.

On Wed, Nov 25, 2015 at 4:22 PM, eyalfa notifications@github.com wrote:

I can start by adding it to the mb-scala project,I might be able to pull
it off by tomorrow noon (il time),we can later make the effort of porting
it to java.

—
Reply to this email directly or view it on GitHub
#39 (comment).

Frank D. Martínez M.

—
Reply to this email directly or view it on GitHub
#39 (comment).

[emacarron]

@eyalfa sorry again but it will be easier to add a test to the core. It should't take too long and will be easier for us.

Instructions are here https://github.com/mybatis/mybatis-3/wiki/Unit-Test

[eyalfa]

Hi Eduardo,
I'll try to take a stub at this in the coming week.
I think you can use the Scala test case quite easily debug the issue, but
I'll do my best.
On Dec 5, 2015 09:19, "Eduardo Macarron" notifications@github.com wrote:

@eyalfa https://github.com/eyalfa sorry again but it will be easier to
add a test to the core. It should't take too long and will be easier for
us.

Instructions are here https://github.com/mybatis/mybatis-3/wiki/Unit-Test

—
Reply to this email directly or view it on GitHub
#39 (comment).

[emacarron]

@eyalfa I am sure I can but lately I do not have too much time for the project. So please forgive me :) and thanks by my side for your time and your help!

[eyalfa]

I've piggybacked an existing test here: https://github.com/eyalfa/mybatis-3/blob/reopen_issue39/src/test/java/org/apache/ibatis/submitted/nestedresulthandler/NestedResultHandlerTest.java

the test is built ontop of 3.2.8, branch is here: https://github.com/eyalfa/mybatis-3/tree/reopen_issue39

please let me know if you need anything else.

eyal.

[emacarron]

Hi again @eyalfa. Sorry for the delay and thanks again for your contribution!

I would say that you found a regression that has been out so long that the fix can also be considered a regression itself!

To a certain point in time an object with no mapped properties (or no values for its mapped properties) was considered "new" for each row processed. This was broken (do not know in which version) with a fix for recursive mappings. With recursive mappings, an instance with no own values can still have children with values and can be referenced from them. We needed to find the instance even if its values are null so we assigned that instance a key (resultmapId) and it was considered "the same". With this fix we solved an StackOverflow but broke the case you pointed in the test case.

The fix works so that:

• An object with no self mapped properties & nested results will not be given a key so a new one will be created for each row.
• An object with self mapped properties that has no value (null) will get a key (adding the null values to that key) and will be considered the same one for any other row.

With this change, an object, with no self properties & nested results that is pointed recursively from a child will cause an StackOverflow again. I think this situation can be solved but may need a huge change. I will have a deeper look in short.

[emacarron]

Hi again @eyalfa

Looks that is was easier than I thought. We used the same key for two caches:

• nestedResultObjects that holds all previously read objects
• ancestorObjects that holds ancestors for current row for recursive nested result mappings

In fact, there is no need to use the row key at all in ancestorObjects. The result map id is enough (and Iwao was already using it for storing the column prefix). So current version should work ok in all scenarios.

[yunnuoyang]

if we have to use 3.1.1 version ,how to avoid the problem of NestedResultSetHandler.handleRowValues cache?maybe we could modify our mybatis-config.xml to made it in somewhere?thanks