Wrong handling of positional INOUT parameters when extracting output parameters

[thjanssen]

When extracting positional output parameters of a stored procedure, the StoredProcedureJpaQuery.extractOutputParameterValue method tries calculating the position of an output parameter based on its index and the number of input parameters (https://github.com/spring-projects/spring-data-jpa/blob/main/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/StoredProcedureJpaQuery.java#L146).

Based on some tests in a client project, the methodParameters.getNumberOfParameters() method seems to count IN and INOUT parameters as input parameters. As a result the position of INOUT parameters get skipped instead of extracted.

Example: When extracting the output parameters of a stored procedure with 2 INOUT and 7 OUT parameters, the method tries to extract positions 3-11 instead of positions 1-9.

[christophstrobl]

Thank you @thjanssen! You don't happen to have a test at hand that reproduces the problem?

[thjanssen]

I'm working on it. I hope to provide one by Monday

[thjanssen]

@christophstrobl I added a test case here: https://github.com/thjanssen/spring-data-jpa/blob/issue/3460/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/procedures/PostgresStoredProcedureIntegrationTests.java

It fails when I run it within my IDE, but the Maven build still passes. Any idea why?

BTW: The other test cases in that class were deactivated, but they seem to work fine. I've reactivated them on my branch.

[thjanssen]

Suggested fix:
Add position information to ProcedureParameter class, set it in StoredProcedureAttributeSource.extractOutputParametersFrom(...), and use that position in StoredProcedureJpaQuery.extractOutputParameterValue to get the result value.
This should work based on the assumption that NamedStoredProcedureQuery.parameters() returns the parameters in the correct order. As far as I understand the rest of the code, it already makes this assumption when using positional parameters.

I did a quick prototype implementation of this fix. You can find it here: 5e19dba
The final version would need some cleanup, and the output format feels a little strange. But it seems to fix the issue without breaking anything else.

[christophstrobl]

thank you @thjanssen!

[thjanssen]

Hey @christophstrobl
If you think my suggested fix is the right way to go, I would love to clean it up and provide a pull request.

Any suggestions on how to improve the result presentation?
Using the position as the map key looks a little strange in the test case, but I didn't come up with a better idea.

BTW: This change also fixes an issue in the current implementation that all positional output parameters use the same map key and override each other.

[christophstrobl]

not right off the top of my head. let me have a look at it (may take a bit of time).

[christophstrobl]

I think the Map is fine in the test. care to open a PR, we can take it from there :)

[thjanssen]

@christophstrobl Awesome. I've send a PR