Skip to content
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

Provide support for ParameterMode.REF_CURSOR - Hibernate [DATAJPA-1145] #1486

Closed
spring-projects-issues opened this issue Jul 7, 2017 · 23 comments
Assignees
Labels
in: core status: declined type: bug

Comments

@spring-projects-issues
Copy link

spring-projects-issues commented Jul 7, 2017

Vishnudev K opened DATAJPA-1145 and commented

I was trying to execute a oracle procedure with spring-data-jpa and hibernate

PROCEDURE MY_PROC (
    P_ID IN NUMBER,
    P_PERIOD IN VARCHAR2,
    P_LIMIT IN NUMBER,
    P_CURSOR OUT T_CURSOR);

MyEntity.java

@NamedStoredProcedureQuery(
        name = "myProc",
        procedureName = "MY_PROC",
        resultClasses = ResultEntity.class,
        parameters = {
            @StoredProcedureParameter(mode = ParameterMode.IN, type = Long.class),
            @StoredProcedureParameter(mode = ParameterMode.IN, type = String.class),
            @StoredProcedureParameter(mode = ParameterMode.IN, type = Long.class),
            @StoredProcedureParameter(mode = ParameterMode.REF_CURSOR, type = void.class)

MyRepository.java

@Procedure(name = "myProc", procedureName = "MY_PROC")
    List<ResultEntity> execMyProc(Long userId,String period,Long idClientLimit);
StoredProcedureQuery query = entityManager.createNamedStoredProcedureQuery("extractWebUser");
query.setParameter(1, userId);
query.setParameter(2, period);
query.setParameter(3, idClientLimit);
query.execute();
List resultList = query.getResultList();

But on spring-data we need to use getOutputParameterValue Method instead of getResultList

Object outputParameterValue = query.getOutputParameterValue(4);

Then I discovered that the hibernate does not support REF_CURSOR

org.hibernate.procedure.internal.AbstractParameterRegistrationImpl.java

...
...
public T extract(CallableStatement statement) {
...
else if ( mode == ParameterMode.REF_CURSOR ) {
            throw new ParameterMisuseException( "REF_CURSOR parameters should be accessed via results" );
        }
...

As per the ticket
https://jira.spring.io/browse/DATAJPA-652
its working fine on eclipse link, but broken in hibernate for quite sometime.
(I have taken the data/details from the original ticket)


Reference URL: #207

Attachments:

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Jul 10, 2017

Jens Schauder commented

Thanks for the PR.

I'm not too fond to blindly try getResultList() on catching a PersistenceException since the exception is quite general.

If it is thrown for some other reason than the one we are trying to handle, we'll probably get another exception and thereby hide the original one. I would be nicer to detect that we are dealing with hibernate in the first place and act accordingly. We probably should add this to org.springframework.data.jpa.provider.PersistenceProvider as a new method, get the proper instance of that enum and invoke it.

Also, we should check what the spec says about this and if the behavior of Hibernate (or the others) is actually a bug

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Jul 10, 2017

Jens Schauder commented

I looked into the spec and I'd say it is a Hibernate bug. From section 3.10.17.3 Stored Procedure Query Execution of the JPA2.1 spec:

When using REF_CURSOR parameters for result sets, the update counts should be exhausted before
calling getResultList to retrieve the result set. Alternatively, the REF_CURSOR result set can be
retrieved through getOutputParameterValue. Result set mappings will be applied to results corresponding
to REF_CURSOR parameters in the order the REF_CURSOR parameters were registered
with the query.

In my interpretation, this means both getResultList and getOutputParameterValue should be supported.

Of course, this also means that we could switch to getResultList, but that does not allow for index/name as a parameter.

@kvishnudev can you please file a bug against Hibernate?

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Jul 10, 2017

Vishnudev K commented

I have raised a ticket in hibernate bucket also.https://hibernate.atlassian.net/browse/HHH-11863

But I feel this is relatively a straight forward scenario that we can fix instead of depending on Hibernate

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Jul 13, 2017

Vishnudev K commented

Hibrenate community is dead unlike spring data. :(

It does't looks like we will have a fix from hibernate anytime soon, Can we have a fix here?

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Jul 13, 2017

Jens Schauder commented

To defend the Hibernate community: We have some pretty stale issues as well ...

Anyway: You are right, doesn't look like we get a fix from Hibernate anytime soon, so I think a workaround from our side would be appropriate.

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Jul 14, 2017

Jens Schauder commented

A test case reproducing the issue would be most helpful

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Jul 17, 2017

Vishnudev K commented

I have attached a zip file to replicate this issue.
Could you see that one?

Thank you for sparing time on this

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Jul 17, 2017

Jens Schauder commented

Oh, overlooked that one. I'll take a look

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Jul 28, 2017

Jens Schauder commented

I wasn't able to reproduce the issue.

The project in the attached file tries to create an EclipeLink entity manager (and fails due to missing/wrongly configured agent).

The maven build doesn't work, because it references a snapshot version that is long gone.

I created a little project to play around with stored procedures: https://github.com/schauder/calling-oracle-stored-procedures-with-cursors
Can you fork it and create a version reproducing the issue? The readme contains instructions what you have to do to make it executable. (providing a database and adding the oracle driver)

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Aug 1, 2017

Vishnudev K commented

I could replicate that issue, I gave you a pull request for the same.
Please have a look at that one. let me know anything required

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Aug 1, 2017

Vishnudev K commented

Made changes?

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Aug 1, 2017

Jens Schauder commented

I tried implementing a workaround.

The idea is to add a
{{ if (storedProcedureQuery.hasMoreResults())
return storedProcedureQuery.getResultList();}}

before we try to access getOutputParameterValue

Unfortunately Hibernate returns false for hasMoreResults().

This makes implementing a workaround rather invasive because we would now have to lug around the information if we are expecting a ResultSet as out parameter in order to call the correct method. I don't think it makes sense under these circumstances to implement a workaround for this Hibernate bug in Spring Data

I updated the Hibernate issue with a link to my repo for demonstrating the behavior

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Aug 3, 2017

Vishnudev K commented

I could see hibernate is returning

callString.hasMoreResults()

as false.
But in eclipselink callString.hasMoreResults() is returning true

Can we use a hasMoreResults check to determine whether to use getOutputParameterValue(1) or getResultList()

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Aug 3, 2017

Jens Schauder commented

At this point, I don't want to put any more effort into this.

If you can provide a pull request that works with the repository I used for testing, I'll take a look.

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Aug 4, 2017

Vishnudev K commented

Please find the pull request for with the hibernate condition check
schauder/calling-oracle-stored-procedures-with-cursors#2

Please find the branch for eclipse link demo for the same
https://github.com/vishnudevk/calling-oracle-stored-procedures-with-cursors/tree/eclipselink

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Aug 7, 2017

Jens Schauder commented

This

if(callString.hasMoreResults()){
	//calling orignal method incase of eclispse link (Any other jpa)
	System.out.println(callString.getOutputParameterValue(1));
}else{
	//in case of hibernate use this call
	System.out.println(callString.getResultList());
}

really just works by accident. What it says is: if the is a resultset use the parameter; If there is no resultset use the resultset.

If one uses a JPA provider without the bug that Hibernate has with a SP that has an out parameter not returning a resultset but a normal value hasMoreResults will most certainly return false. This would lead to getResultList() getting executed which would fail

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Sep 21, 2017

MIhalcea Vlad commented

The Hibernate issue is fixed and tested on Oracle and PostgreSQL. It will be available for 5.2.12 onwards

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Jan 3, 2018

Jens Schauder commented

The issue does not appear any longer with the new Hibernate version

@spring-projects-issues
Copy link
Author

spring-projects-issues commented May 22, 2019

Jens Schauder commented

Batch closing resolved issue without a fix version and a resolution indicating that there is nothing to release (Won't fix, Invalid ...)

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Jan 7, 2020

GabrielBB commented

I made a made a PR to make this work with or without NamedProcedures: #406

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Jan 7, 2020

Jens Schauder commented

GabrielBB This issue was closed so I don't really understand what your PR is for. If you think there is something that needs fixing, please create a new issue.

The description of the PR makes me sceptical though.
Spring Data JPA is build on JPA.
We try to stay away from database specific stuff

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Jan 7, 2020

GabrielBB commented

Jens Schauder The procedure annotation has a logic to work without Named Procedure Queries, what I did is to extend that logic to return resultsets. Right now if you want to return a cursor the only way is to put all those annotations on top of your entity. But if you called a procedure with normal output variables you don’t need the annotations, it works with a simple @procedure tag in your repository. Just like @query annotation works, you put the query right there in the annotation and can even make a native query right there, without going to your entity and declaring named queries. The thing is that feature of @procedure is missing result sets and cursors. That “AND” between result sets and cursors is important. Declaring Ref_cursors are not the only way to return results sets from procedures, take for example MySQL, right now using spring-data-JPA, as far as I know, the only way to return result sets from a procedure is to make this: @query(“ CALL procedure() “ ), because MySQL doesn’t use ref_cursor for result sets. My PR makes it work using @procedure because I did exactly what you proposed some comments above, about calling getResultList instead of getParameterValue. But instead of asking for hasMoreResults, I used the Boolean that the method “execute” returns, that Boolean tells you if the is a result set coming from the database. In conclusion, what I did is make the api a little more consistent , since when we use @procedure without cursors OR resultsets we don’t used named procedure queries, we just put the procedure name in the annotation, also when using query we don’t use named queries, so my PR finishes the work so no named queries is needed in any case, and also make @procedure work with MySQL and SQL Server, which doesn’t work right now (Postgres can also return resultsets from procedures without the need of ref cursors, those procedures don’t work with spring-data-JPA neither, but with my change they do, because to work with any database the only thing you need to do is call getResultList)

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Jan 7, 2020

GabrielBB commented

Jens Schauder I will create another Ticket, indeed. Since my PR is doing several things

@spring-projects-issues spring-projects-issues added type: bug status: declined in: core labels Dec 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core status: declined type: bug
Projects
None yet
Development

No branches or pull requests

2 participants