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

recognize CallableStatement parameter names with leading '@' #495

Merged
merged 2 commits into from
Oct 7, 2017
Merged

recognize CallableStatement parameter names with leading '@' #495

merged 2 commits into from
Oct 7, 2017

Conversation

gordthompson
Copy link
Contributor

When passing parameters to a CallableStatement by name, recognize names that start with '@' as reported by DatabaseMetaData#getProcedureColumns.

(ref: Stack Overflow question here)

@codecov-io
Copy link

codecov-io commented Sep 20, 2017

Codecov Report

Merging #495 into dev will increase coverage by 0.08%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #495      +/-   ##
============================================
+ Coverage     46.47%   46.56%   +0.08%     
- Complexity     2214     2220       +6     
============================================
  Files           108      108              
  Lines         25312    25316       +4     
  Branches       4183     4182       -1     
============================================
+ Hits          11765    11788      +23     
+ Misses        11513    11498      -15     
+ Partials       2034     2030       -4
Flag Coverage Δ Complexity Δ
#JDBC41 46.32% <83.33%> (+0.02%) 2204 <0> (-3) ⬇️
#JDBC42 46.48% <83.33%> (+0.13%) 2218 <0> (+8) ⬆️
Impacted Files Coverage Δ Complexity Δ
...oft/sqlserver/jdbc/SQLServerCallableStatement.java 30.49% <83.33%> (+1.29%) 96 <0> (+6) ⬆️
...va/com/microsoft/sqlserver/jdbc/ThreePartName.java 71.42% <0%> (-14.29%) 7% <0%> (-1%)
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java 52.45% <0%> (-0.13%) 239% <0%> (ø)
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java 59.36% <0%> (-0.11%) 130% <0%> (-1%)
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java 32.94% <0%> (-0.05%) 242% <0%> (-1%)
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 53.71% <0%> (-0.02%) 0% <0%> (ø)
...erverColumnEncryptionCertificateStoreProvider.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...osoft/sqlserver/jdbc/SQLServerSecurityUtility.java 50.74% <0%> (ø) 10% <0%> (ø) ⬇️
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java 63.24% <0%> (+0.12%) 0% <0%> (ø) ⬇️
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java 42.42% <0%> (+0.21%) 46% <0%> (ø) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6f66e9...e2ec02e. Read the comment docs.

@gordthompson
Copy link
Contributor Author

Addresses issue #496

@microsoft microsoft deleted a comment from msftclas Sep 26, 2017
@cheenamalhotra cheenamalhotra added this to the 6.3.4 milestone Sep 26, 2017
Copy link
Contributor

@peterbae peterbae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also ran some more testing, and the changes look good. Thanks for the contribution!

@@ -1451,6 +1451,12 @@ public NClob getNClob(String parameterName) throws SQLException {
if (paramNames != null)
l = paramNames.size();

// handle `@name` as well as `name`, since `@name` is what's returned
// by DatabaseMetaData#getProcedureColumns
if (columnName.startsWith("@")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can add the check at this line

if (!columnName.startsWith("@"))
                sParam = sParam.substring(1, sParam.length());

The reason I am proposing this is that to have the original column name when we throw the exception at this line Otherwise looks great! thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@v-afrafi - Good point. Your change would actually have to be made in two places, so I tweaked it a bit.

@AfsanehR-zz AfsanehR-zz merged commit b8daa42 into microsoft:dev Oct 7, 2017
@AfsanehR-zz
Copy link
Contributor

Thanks @gordthompson for fixing this issue 🎉

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.

7 participants