Skip to content

DATAJPA-1657 - @Procedure annotation doesn't work with cursors (NULL when using REF_CURSOR) and ResultSets that don't come from cursors #406

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
wants to merge 6 commits into from

Conversation

GabrielBB
Copy link
Contributor

@GabrielBB GabrielBB commented Jan 7, 2020

  • [ x ] You have read the Spring Data contribution guidelines.
  • [ x ] There is a ticket in the bug tracker for the project in our JIRA.
  • [ x ] You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • [ x] You submit test cases (unit or integration tests) that back your changes.
  • [ x ] You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

Objectives:

  • Make it possible to return ResultSets from @procedure (not ref_cursor)

  • Make it possible to return cursors from @procedure

  • 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 and REF_CURSORs with 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:

@Procedure("my_procedure",  refCursor = true)
List<Entity> myProcedure(Integer input);

However if you're using MySQL or SQL Server, you don't need that parameter. This will work:

@Procedure("my_procedure")
List<Entity> myProcedure(Integer input);

You can also return a single Entity from your procedure. For Example:

@Procedure("my_procedure")
Entity myProcedure();

One more... You can also return a list of generic objects. For example:

@Procedure("my_procedure")
List<Object[]> myProcedure();

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:

@Procedure("my_procedure")
Integer myProcedure();

Okay... So if I use MySQL, how do you know that my procedure is returning a ResultSet if I don't use the refCursor parameter? Here is the answer. I coded thinking first about databases that don't support REF_CURSOR but do support ResultSets, when I made sure that @procedure is finally able to return ResultSets, then I introduced the refCursor parameter.

How is the ResultSet parsed to the method return type, for example, an Entity? I check if your method's return type is an Entity (don't worry, i re-used some logic used by @query), if it is, then I pass the class type to StoredProcedureQuery when instantiating it, if not, then I omit the class type.

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:

        @Procedure("0_input_1_row_resultset")
        Dummy singleEntityFromResultSetAndNoInput();

        @Procedure("1_input_1_row_resultset")
        Dummy singleEntityFrom1RowResultSetWithInput(Integer arg);

        @Procedure("0_input_1_resultset")
        List<Dummy> entityListFrom1RowResultSetWithNoInput();

        @Procedure("1_input_1_resultset")
        List<Dummy> entityListFrom1RowResultSetWithInput(Integer arg);

        @Procedure(value = "1_input_1_resultset", outputParameterName = "dummies")
        List<Dummy> entityListFrom1RowResultSetWithInputAndNamedOutput(Integer arg);

        @Procedure(value = "1_input_1_resultset", outputParameterName = "dummies", refCursor = true)
        List<Dummy> entityListFrom1RowResultSetWithInputAndNamedOutputAndCursor(Integer arg);

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:

Stored procedures with ResultSets are currently not supported for any
JPA Provider

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). If you try to use REF_CURSOR with MySQL it will tell you the dialect doesn't support it, but as I said before, you don't need the cursor, just make a normal select inside your procedure and @procedure will pick the result. That's why i changed that text inside the code to:

Stored procedures with REF_CURSOR are currently not supported by the HSQL dialect

I know I touched many classes, but I needed to refactor some code to make this change leaving the code easily maintainable. For example I made a new class (ProcedureParameter) to store the parameter properties because passing around the properties was not clean now that I also needed to pass the parameter type (if it’s out or refCursor). I couldn’t name it StoredProcedureParameter since there’s an annotation names like that already.

Of course, this will work only if the ResultSet returns 1 row
Of course, this will work only if the ResultSet returns 1 row
Now that @procedure works with ResultSets, I'm adding a new annotation boolean parameter called "refCursor". If you're working with databases like Oracle and Postgres, that support REF_CURSOR, then setting this parameter to TRUE will make @procedure return the ResultSet from the cursor with no problem. But if you're working with databases like MySQL, that don't support REF_CURSOR, don't worry, MySQL still let you return a ResultSet without using REF_CURSOR. In this cases just set the parameter to FALSE (that's the default value), and the ResultSet will be retrieved with no problem. Both ways will try to convert the result to the return type, this can be an Entity, a List of Entities or a List of generic Objects (just like @query works).
@schauder schauder changed the title DATAJPA-652 - Provide support for ParameterMode.REF_CURSOR DATAJPA-1657 - @Procedure annotation doesn't work with cursors (NULL when using REF_CURSOR) and ResultSets that don't come from cursors Jan 9, 2020
@GabrielBB
Copy link
Contributor Author

I noticed that when i formatted the code using IntelliJ a lot of changes were added to my commits. It makes it look like I change a lot of code for this PR and now it is even creating conflicts. I used the Eclipse Code Formatter for IntelliJ that was recommended in the guidelines so I don't know what went wrong. I will close this PR and open another one making sure this doesn't happen

@GabrielBB GabrielBB closed this Jan 14, 2020
@schauder
Copy link
Contributor

Did you apply the necessary configuration to the formatter? https://github.com/spring-projects/spring-data-build/tree/master/etc/ide

There is no need to create a new pull request. Just push the changes to your branch of the pull request and it will become part of the pull request.

@GabrielBB
Copy link
Contributor Author

GabrielBB commented Jan 14, 2020

@schauder That was my problem, I didn't apply the formatter configuration. In the new PR this:
image
Changed to this:
image

Crazy. It will be easier for you guys to review now.

I couldn't re-open this PR because I reverted my commits and re-committed with the right format, also added the right ticket number to the commits. Github didn't let me re-open since the commits in this PR no longer existed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants