-
Notifications
You must be signed in to change notification settings - Fork 1.5k
DATAJPA-1657 - @Procedure annotation doesn't work with cursors (NULL when using REF_CURSOR) and ResultSets that don't come from cursors #409
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
Closed
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
"InvalidDataAccessApiUsageException" will be thrown if the @procedure returns a ResultSet and no transaction is active. It is recommended to use @transactional in this case
It determines whether the procedure has a REF_CURSOR output parameter or not. Default is false
Passing the stored procedure parameter properties around was not easily maintainable. I had to pass 3 Lists from place to place, each containing the 3 properties of the parameters (names, types and modes). I created a class that has the name, type and mode inside so it is cleaner to pass this values around. I couldn't name the class "StoredProcedureParameter" because javax.persistence package has an annotation named like that already
javax.persistence.StoredProcedureQuery.execute method tells us if a ResultSet is being returned or not. This is the key to finally decide what to return: Output parameter values or javax.persistence.StoredProcedureQuery.getResultList(). I added a logic to return single entities from it if the method annotated with @procedure does not have a Collection return type. When registering the procedure output parameters I needed to find a way to predict if the user is intending to return a ResultSet or not. I ended up doing it with this condition: If the method annotated with @procedure has a collection return type or it doesn't have a collection but it has an entity return type then I know for sure that a ResultSet is intended to be returned because regular output parameters are NEVER returned in collections but in arrays (Object[]) and also regular output parameters don't return entities, but primitive types.
I wrote unit tests but unfortunately we can't make integration tests for this change because the HSQL database dialect doesn't work with procedure returning ResultSets. But MySQL, Oracle, Postgres and SQL Server dialects do work with it. Check this project that tests with all those databases: https://github.com/GabrielBB/spring-data-jpa-procedure-tests
Looks very appealing. We'll take it from here. |
gregturn
pushed a commit
that referenced
this pull request
Jul 9, 2021
When registered procedure output parameters, if the method annotated with @procedure has a collection return type or an entity return type, then use ResultSet. Otherwise, handle either the array (Object[]) or the primitive types. * Throw a proper exception when using an @Procedure-annotated method and no transaction is active. * Introduce refCursor as a boolean flag to @procedure denoting when to use REF_CURSORs for OUT parameters.. * Extract a ProcedureParameter command object to ease usage of a parameter's name, type and mode. (NOTE: javax.persistence already has "StoredProcedureParameter".) NOTE: Integration testing of stored procedures is very limited by lack of HSQL's support for REF CURSORS. See ##2256 to track future work for potential testing against MySQL, Oracle, Postgres, or SQL Server. See: #1959, #409. Related: #2014.
Resolved via 554bd3d. Thanks @GabrielBB! I have also opened #2256 as a path to pull in stronger integration testing analogous to the efforts you have made against MySQL, etc. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
in: query-parser
Everything related to parsing JPQL or SQL
in: repository
Repositories abstraction
type: enhancement
A general enhancement
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objectives:
Make it possible to return ResultSets from @procedure without using REF_CURSOR
Make it possible to return ResultSets from @procedure using REF_CURSOR
Remove the need to use @NamedStoredProcedureQuery in your entities to return cursors. We should be able to return it with a simple @procedure annotation
Details:
I made a change so you can call procedures that return ResultSets using the @procedure annotation. I tested this in this secondary project with MySQL, Postgres, Oracle and SQL Server databases
What did I do to make it work for any of those 4 databases? I added a new parameter called "refCursor" to express that the procedure in your database is using a REF_CURSOR. Here is an Example that would work with Oracle:
However if you're using MySQL or SQL Server, you don't need that parameter. This will work:
You can also return a single Entity from your procedure. For Example:
One more... You can also return a list of generic objects. For example:
Nothing else is needed to call your procedure with ResultSets. I made sure you don't have to use Hibernate's @Namedstoredprocedurequery in your entities, making them dirty. I also made sure you can still return regular output parameters. This still works:
I predict if you are intending to return a ResultSet using this logic: If the method annotated with @procedure has a Collection return type or if it's not a collection but it is an entity. This works 100% of the time because OUT parameters are never returned in collections (List) but in arrays (Object[]) and also OUT parameters will never return an Entity. However when the refCursor attribute is set to true, this logic is obviously not needed because the user is explicitly saying that he is intending to return a ResultSet from the database using a cursor. That logic is only for procedures that return resultsets without cursors, for example, when using MySQL.
I could pass a boolean all around the code saying if the procedure will return a resulset so at the end, in the JPAQueryExecution class, I can know if I should call getResultList() or read output parameter values, but my condition is based instead on the javax/persistence/StoredProcedureQuery.execute return value:
To return ResultSets the code calling the @procedure method needs to have an active transaction. I re-used some logic I found to check if no transaction is active, if not, then I throw InvalidDataAccessApiUsageException
I added 6 unit tests in spring-data-jpa covering the following cases:
However, spring-data-jpa uses HSQL for the integration tests and the HSQL Dialect for Hibernate doesn't support returning REF_CURSORs so there's no way to make integration tests for this. That's why I made another project to test this with MySQL, Oracle, Postgres and SQL Server dockerized databases. All the tests passed! ResultSets with or without REF_CURSORs are working perfectly.
I found this text somewhere in the spring-data-jpa code that justifies not supporting REF_CURSORs:
That is not true. Hibernate works with it (maybe the support was added after that comment) and all the major database dialects support it (I mentioned 4 databases that I tested with, maybe there are more). That's why i changed that comment to: