Skip to content

recognize CallableStatement parameter names with leading '@'#495

Merged
AfsanehR-zz merged 2 commits intomicrosoft:devfrom
gordthompson:allow-at-sign-prefixed-param-names
Oct 7, 2017
Merged

recognize CallableStatement parameter names with leading '@'#495
AfsanehR-zz merged 2 commits intomicrosoft:devfrom
gordthompson:allow-at-sign-prefixed-param-names

Conversation

@gordthompson
Copy link
Copy Markdown
Contributor

@gordthompson gordthompson commented Sep 20, 2017

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
Copy Markdown

codecov-io commented Sep 20, 2017 •
edited
Loading

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
Copy Markdown
Contributor Author

gordthompson commented Sep 20, 2017

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
peterbae
peterbae approved these changes Sep 28, 2017
View reviewed changes
Copy link
Copy Markdown
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!

AfsanehR-zz
AfsanehR-zz reviewed Oct 6, 2017
View reviewed changes
src/main/java/com/microsoft/sqlserver/jdbc/SQLServerCallableStatement.java

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

@AfsanehR-zz AfsanehR-zz Oct 6, 2017

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
Copy Markdown
Contributor Author

@gordthompson gordthompson Oct 6, 2017

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
Copy Markdown
Contributor

AfsanehR-zz commented Oct 7, 2017

Thanks @gordthompson for fixing this issue 🎉

@gordthompson gordthompson mentioned this pull request Oct 7, 2017
Closed
@gordthompson gordthompson deleted the allow-at-sign-prefixed-param-names branch October 7, 2017 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@AfsanehR-zz AfsanehR-zz AfsanehR-zz approved these changes

@peterbae peterbae peterbae approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

6.3.4

Development

Successfully merging this pull request may close these issues.

8 participants

@gordthompson @codecov-io @AfsanehR-zz @peterbae @lilgreenbird @msftclas @msftgits @cheenamalhotra