Apply custom ResultHandler to CURSOR type OUT parameter

[vladchuk]

In the DefaultResultSetHandler, a new result handler is instantiated and used unconditionally. That prevents one from using his own; although the API allows for a custom handler it is silently ignored:

private void handleRefCursorOutputParameter(ResultSet rs, ParameterMapping parameterMapping, MetaObject metaParam) throws SQLException {
    try {
      final String resultMapId = parameterMapping.getResultMapId();
      final ResultMap resultMap = configuration.getResultMap(resultMapId);
      final DefaultResultHandler resultHandler = new DefaultResultHandler(objectFactory);
      final ResultSetWrapper rsw = new ResultSetWrapper(rs, configuration);
      handleRowValues(rsw, resultMap, resultHandler, new RowBounds(), null);
      metaParam.setValue(parameterMapping.getProperty(), resultHandler.getResultList());
    } finally {
      // issue #228 (close resultsets)
      closeResultSet(rs);
    }
  }

Might it be possible to do something like:

            ResultHandler rh;
            Object list = new ArrayList<Object>();
            if (resultHandler == null) {
                rh = new DefaultResultHandler(objectFactory);
                list = ((DefaultResultHandler)rh).getResultList();
            }
            else {
                rh = resultHandler;
            }
            metaParam.setValue(parameterMapping.getProperty(), list);

to give a custom handler a chance?

It may not be a %100 solution, but it seems to work for us.

[harawata]

Thank you for the report!
Could you please create a failing test case or an example project?
I want to see the use case that is affected by these lines you pointed out.

Regards,
Iwao

[kazuki43zoo]

Hi @vladchuk , Could you provide a repro project via GitHub ?

[vladchuk]

Unfortunately, it looks prohibitively expensive for me to come up with a repro project. However, just by looking at the code you should be able to see that the custom ResultHandler never has a chance as currently implemented. For reference, take a a look how it's handled in handleResultSet() - notice the resultHandler == null check? Same idea.

Also, I agree with the csportics fix (mine was unnecessarily complicated), but in our case we need an extra check for null ResultSet, something like

      private void handleRefCursorOutputParameter(ResultSet rs, ParameterMapping parameterMapping,     MetaObject metaParam)
            throws SQLException {
        try {
            final String resultMapId = parameterMapping.getResultMapId();
            final ResultMap resultMap = configuration.getResultMap(resultMapId);
            // check for null RS and custom result handler
            if (rs != null) {
                ResultSetWrapper rsw = new ResultSetWrapper(rs, configuration);
                if (resultHandler == null) {
                    DefaultResultHandler drh = new DefaultResultHandler(objectFactory);
                    handleRowValues(rsw, resultMap, drh, new RowBounds(), null);
                    metaParam.setValue(parameterMapping.getProperty(), drh.getResultList());
                }
                else {
                    handleRowValues(rsw, resultMap, resultHandler, new RowBounds(), null);
                }
            }
        }
        finally {
            // issue #228 (close resultsets)
            closeResultSet(rs);
        }
    }

The above is exactly what we currently use and it seems to work in all cases. I hope this helps.

[vladchuk]

Oops, closed by mistake - just meant to comment.

[harawata]

Hi @vladchuk ,

The fix actually used in a real solution is helpful.
Thank you!

We should have a test case that verifies the fix and, for me, it's the difficult part of this issue ;)
It would look something like this, I suppose.

If it's difficult to provide a complete test case, could you post a simple procedure definition and a corresponding mapper statement (<select /> with call) ?

[vladchuk]

This is still not fixed in v. 3.4.5. I cooked up a unit test that demonstrates the problem but I see no way to attach a file here.

[kazuki43zoo]

Probably, you can attach file by dragging & dropping at comment area.

[kazuki43zoo]

or I suggest to add a reproduce project on your account's repository.
See ...

• https://help.github.com/articles/creating-a-new-repository/.
• https://help.github.com/articles/cloning-a-repository/
• https://help.github.com/articles/adding-a-file-to-a-repository-using-the-command-line/

[vladchuk]

Here it is. I hope this will help to finally resolve this issue!

CustomResultHandlerTest.zip

[vladchuk]

Is it possible to get this fix in the next build? I'm tired of building my own jars...:(

[harawata]

@vladchuk ,

Thank you for the test case!
I'll try to look into it as soon as I have time.

[harawata]

I took a look at the proposed patch, but it seems that it does not work when there are multiple OUT parameters and requires unused/unnecessary resultMap or resultType attribute to be specified on <select /> element.

We should keep looking for a better solution, I think.
The multiple OUT parameters case might require bigger change, though.

[vladchuk]

I'm afraid I don't understand the comments about the proposed patch. In submitting the test case, my primary goal was to demonstrate the problem, namely - "custom ResultHandler is not used, despite being supplied", not so much to propose a solution. It it a major problem, and I solved it, perhaps naively and sub-optimally, to be able to use the latest version of Mybatis, and it seems to work.

It might be the case that the correct solution is not trivial, but can you at least acknowledge the problem as stated above? Or was it always understood that the issue was always present, but was just not serious enough? I would really like to use the original Mybatis distro instead of making my own with each new release...

[harawata]

I'm afraid I don't understand the comments about the proposed patch.

By 'the proposed patch', I meant that the change you posted in the issue description.

Or was it always understood that the issue was always present

Yes, it was obvious from the code that a custom ResultHandler was not used.
As I asked, I needed an example of the procedure and the mapper statement to reproduce the issue.
Hope you understand that I don't have enough spare time to create a repro for every reported issue.
Anyway, thanks to your test case, I managed to create one with PostgreSQL and found some difficulties I mentioned in my previous comment.

but was just not serious enough?

I don't say that. It's just I am not sure how to fix it properly at the moment.

[harawata]

Hi @vladchuk ,

I have committed the fix that works with a stored procedure with a single refcursor out parameter.
Could you verify the fix with the latest 3.4.6-SNAPSHOT ?
https://github.com/mybatis/mybatis-3/wiki/Maven

Regarding a stored procedure that has multiple refcursor out parameters, I have a fix, but don’t like it very much, so I’ll reconsider it when/if someone actually needs it (my fix won’t break existing solutions).
And even with the current implementation, checking the instance type could be used as a workaround.

Object obj = resultContext.getObject();
if (obj instanceof Person) {
  // ...
} else if (obj instanceof Pet) {
  // ...

[kazuki43zoo]

@harawata Is this bug? Could you add an appropriate label?

[harawata]

In case someone is interested. Here is the patch for multiple refcursor out parameters.
harawata@f7a2ccf

Thanks for the heads up, @kazuki43zoo !