New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply custom ResultHandler to CURSOR type OUT parameter #493

Closed
vladchuk opened this Issue Oct 16, 2015 · 17 comments

Comments

Projects
None yet
3 participants
@vladchuk
Copy link

vladchuk commented Oct 16, 2015

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

This comment has been minimized.

Copy link
Member

harawata commented Jan 11, 2016

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

This comment has been minimized.

Copy link
Member

kazuki43zoo commented Nov 23, 2016

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

@vladchuk

This comment has been minimized.

Copy link
Author

vladchuk commented Nov 23, 2016

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 vladchuk closed this Nov 23, 2016

@vladchuk

This comment has been minimized.

Copy link
Author

vladchuk commented Nov 23, 2016

Oops, closed by mistake - just meant to comment.

@vladchuk vladchuk reopened this Nov 23, 2016

@harawata

This comment has been minimized.

Copy link
Member

harawata commented Nov 23, 2016

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 comment has been minimized.

Copy link
Author

vladchuk commented Oct 23, 2017

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

This comment has been minimized.

Copy link
Member

kazuki43zoo commented Oct 24, 2017

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

@vladchuk

This comment has been minimized.

Copy link
Author

vladchuk commented Oct 24, 2017

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

CustomResultHandlerTest.zip

@vladchuk

This comment has been minimized.

Copy link
Author

vladchuk commented Oct 27, 2017

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

@harawata

This comment has been minimized.

Copy link
Member

harawata commented Oct 27, 2017

@vladchuk ,

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

@harawata

This comment has been minimized.

Copy link
Member

harawata commented Oct 28, 2017

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

This comment has been minimized.

Copy link
Author

vladchuk commented Oct 28, 2017

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

This comment has been minimized.

Copy link
Member

harawata commented Oct 29, 2017

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 added a commit that referenced this issue Nov 11, 2017

@harawata harawata closed this in 6576938 Nov 11, 2017

@harawata

This comment has been minimized.

Copy link
Member

harawata commented Nov 11, 2017

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 kazuki43zoo added this to the 3.4.6 milestone Nov 11, 2017

@kazuki43zoo

This comment has been minimized.

Copy link
Member

kazuki43zoo commented Nov 11, 2017

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

@harawata

This comment has been minimized.

Copy link
Member

harawata commented Nov 11, 2017

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

Thanks for the heads up, @kazuki43zoo !

harawata added a commit that referenced this issue Nov 14, 2017

@harawata harawata changed the title Custom ResultHandler ignored Apply custom ResultHandler to CURSOR type OUT parameter Mar 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment