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

Revert Execute Stored Procedure directly feature #2488

Merged
merged 6 commits into from
Aug 21, 2024
Merged

Conversation

tkyc
Copy link
Member

@tkyc tkyc commented Aug 14, 2024

Revert of the following:

Fixed regression with specifying arg names in call syntax (#2480)
0e97689

Fixed parentheses parsing for stored procedures and functions names (#2467)
8f53c02

Fixed CallableStatement default value regression (#2452)
9307051

Fixed timestamp string conversion error for cstmt (#2449)
9754078

Execute stored procedures directly for RPC calls (#2410)
df5bfa6

Fix calling procedures with output parameters by their four-part syntax (#2349)
aa46637

Re-added support for stored procedure 'exec' escape syntax in CallableStatements (#2325)
ba88da8

Execute cstmt directly - Additional testing and changes (#2284)
92cfe0d

Execute Stored Procedures Directly (#2154)
11680a6

Copy link

codecov bot commented Aug 14, 2024

Codecov Report

Attention: Patch coverage is 46.29630% with 29 lines in your changes missing coverage. Please review.

Project coverage is 50.58%. Comparing base (0e97689) to head (c5de6df).
Report is 3 commits behind head on main.

Files Patch % Lines
.../microsoft/sqlserver/jdbc/SQLServerXAResource.java 0.00% 14 Missing ⚠️
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java 18.18% 9 Missing ⚠️
.../microsoft/sqlserver/jdbc/SQLServerConnection.java 33.33% 2 Missing ⚠️
.../microsoft/sqlserver/jdbc/SQLServerDataSource.java 0.00% 2 Missing ⚠️
...n/java/com/microsoft/sqlserver/jdbc/Parameter.java 83.33% 1 Missing ⚠️
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2488      +/-   ##
============================================
- Coverage     50.78%   50.58%   -0.21%     
+ Complexity     3888     3817      -71     
============================================
  Files           145      145              
  Lines         33481    33137     -344     
  Branches       5690     5560     -130     
============================================
- Hits          17004    16762     -242     
+ Misses        14030    13974      -56     
+ Partials       2447     2401      -46     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tkyc tkyc marked this pull request as ready for review August 14, 2024 23:00
pom.xml Outdated Show resolved Hide resolved
src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java Outdated Show resolved Hide resolved
@lilgreenbird lilgreenbird changed the title Exec cstmt dir revert Revert Execute Stored Procedure directly feature Aug 21, 2024
lilgreenbird
lilgreenbird previously approved these changes Aug 21, 2024
Jeffery-Wasty
Jeffery-Wasty previously approved these changes Aug 21, 2024
@tkyc tkyc dismissed stale reviews from Jeffery-Wasty and lilgreenbird via c5de6df August 21, 2024 20:08
@tkyc tkyc merged commit 4c6227a into main Aug 21, 2024
17 of 19 checks passed
@tkyc tkyc deleted the exec-cstmt-dir-revert branch August 21, 2024 22:52
tkyc added a commit that referenced this pull request Aug 21, 2024
* Revert "Execute Stored Procedures Directly (#2154)"

This reverts commit 11680a6.

* Revert "Execute cstmt directly - Additional testing and changes (#2284)"

This reverts commit 92cfe0d.

* Revert "Re-added support for stored procedure 'exec' escape syntax in CallableStatements (#2325)"

This reverts commit ba88da8.

* Additional revert of missed lines

* Added no-op for getters/setters

* RequestBoundaryMethods no-op test fix
@Jeffery-Wasty Jeffery-Wasty modified the milestones: 12.8.1, 12.9.0 Aug 21, 2024
tkyc added a commit that referenced this pull request Aug 22, 2024
* Revert "Execute Stored Procedures Directly (#2154)"

This reverts commit 11680a6.

* Revert "Execute cstmt directly - Additional testing and changes (#2284)"

This reverts commit 92cfe0d.

* Revert "Re-added support for stored procedure 'exec' escape syntax in CallableStatements (#2325)"

This reverts commit ba88da8.

* Additional revert of missed lines

* Added no-op for getters/setters

* RequestBoundaryMethods no-op test fix
tkyc added a commit that referenced this pull request Aug 23, 2024
* Revert "Execute Stored Procedures Directly (#2154)"

This reverts commit 11680a6.

* Revert "Execute cstmt directly - Additional testing and changes (#2284)"

This reverts commit 92cfe0d.

* Revert "Re-added support for stored procedure 'exec' escape syntax in CallableStatements (#2325)"

This reverts commit ba88da8.

* Additional revert of missed lines

* Added no-op for getters/setters

* RequestBoundaryMethods no-op test fix
tkyc added a commit that referenced this pull request Aug 28, 2024
* Revert "Execute Stored Procedures Directly (#2154)"

This reverts commit 11680a6.

* Revert "Execute cstmt directly - Additional testing and changes (#2284)"

This reverts commit 92cfe0d.

* Revert "Re-added support for stored procedure 'exec' escape syntax in CallableStatements (#2325)"

This reverts commit ba88da8.

* Additional revert of missed lines

* Added no-op for getters/setters

* RequestBoundaryMethods no-op test fix
@b0fh042
Copy link

b0fh042 commented Sep 16, 2024

will this ever be brought back at some point? the way the old 'wrapper' works makes it impossible to return (n)varchar(max) from a stored procedure, which is very limiting... - thanks.

@tkyc
Copy link
Member Author

tkyc commented Sep 16, 2024

@b0fh042

Yeah, the PR will be reintroduced later. you can track the PR here: #2499

We decided it was best to revert as this was a significant change and we've noticed more edge case issues popping up from customers. As for a timeline, it's likely anticipated to be worked on in the new year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed/Merged PRs
4 participants