Fix | Fix possible Statement Leak in SQLServerConnection.isValid() API

[cheenamalhotra]

Fixes #954

• Create SQLServerStatement in try-with-resources block.

[codecov-io]

Codecov Report

Merging #955 into dev will decrease coverage by 0.01%.
The diff coverage is 62.5%.

@@             Coverage Diff              @@
##                dev     #955      +/-   ##
============================================
- Coverage     50.58%   50.56%   -0.02%     
- Complexity     2915     2918       +3     
============================================
  Files           120      120              
  Lines         27987    27989       +2     
  Branches       4678     4679       +1     
============================================
- Hits          14156    14154       -2     
- Misses        11535    11542       +7     
+ Partials       2296     2293       -3

Flag	Coverage Δ	Complexity Δ
#JDBC42	50.07% <62.5%> (+0.05%)	2876 <0> (+9)	⬆️
#JDBC43	50.42% <62.5%> (-0.11%)	2908 <0> (-6)

Impacted Files	Coverage Δ	Complexity Δ
.../microsoft/sqlserver/jdbc/SQLServerConnection.java	46.43% <62.5%> (-0.07%)	342 <0> (ø)
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java	43.95% <0%> (-3.3%)	15% <0%> (ø)
...om/microsoft/sqlserver/jdbc/SimpleInputStream.java	52.23% <0%> (-1.5%)	11% <0%> (-1%)
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java	39% <0%> (-0.44%)	43% <0%> (-1%)
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java	68.35% <0%> (-0.18%)	0% <0%> (ø)
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java	47.3% <0%> (ø)	111% <0%> (-1%)	⬇️
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java	55.78% <0%> (+0.03%)	0% <0%> (ø)	⬇️
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java	51.63% <0%> (+0.12%)	254% <0%> (+1%)	⬆️
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java	43.86% <0%> (+0.19%)	319% <0%> (+4%)	⬆️
...n/java/com/microsoft/sqlserver/jdbc/DataTypes.java	79.92% <0%> (+0.19%)	6% <0%> (+1%)	⬆️

---

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 bb72f76...917f831. Read the comment docs.

[ecki]

I am afraid that PR changes the code semantics. The valid=true is set before the close, the following comment explicitly says that an exception on close is also invalid. Maybe it is easier to add a return false to the catch and remove the flag completely. This also allows for nicer logging

[cheenamalhotra]

I added invalid=false on exception, to keep the exit gate consistent to end of method - takes care of exit logger too.

[lilgreenbird]

I added invalid=false on exception, to keep the exit gate consistent to end of method - takes care of exit logger too.

it's unnecessary as valid was init to false, so unless set to true it would already still be false

[ecki]

it's unnecessary as valid was init to false, so unless set to true it would already still be false

As long as isValid=true is inside the try block it will be executed before the auto close. If the autoclose fails the catch is executed while isValid is already true. So setting it to false is one possible fix to keep the old semantic.

(Personally i don’t like single-exit because of exactly that kind of problem but imguess its a good idea to stay consistent with existing conventions)

[ulvii]

Would you re-run the CI? Seems like it fails on ConnectionDriverTest?