Skip to content

Fix | Fix for ResultSets being consumed when reading warnings#991

Merged
cheenamalhotra merged 8 commits intomicrosoft:devfrom
cheenamalhotra:issue969
Apr 11, 2019
Merged

Fix | Fix for ResultSets being consumed when reading warnings#991
cheenamalhotra merged 8 commits intomicrosoft:devfrom
cheenamalhotra:issue969

Conversation

@cheenamalhotra
Copy link
Copy Markdown
Member

@cheenamalhotra cheenamalhotra commented Mar 15, 2019

Fixes issue #969

@cheenamalhotra cheenamalhotra mentioned this pull request Mar 15, 2019
Closed
@cheenamalhotra
Copy link
Copy Markdown
Member Author

cheenamalhotra commented Mar 15, 2019

Jars for testing: mssql-jdbc-PR991.zip

@codecov-io
Copy link
Copy Markdown

codecov-io commented Mar 15, 2019 •
edited
Loading

Codecov Report

Merging #991 into dev will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##                dev    #991     +/-   ##
==========================================
+ Coverage     50.09%   50.2%   +0.1%     
- Complexity     2878    2888     +10     
==========================================
  Files           120     120             
  Lines         27989   27996      +7     
  Branches       4677    4680      +3     
==========================================
+ Hits          14022   14055     +33     
+ Misses        11713   11686     -27     
- Partials       2254    2255      +1
Flag Coverage Δ Complexity Δ
#JDBC42 49.77% <100%> (+0.11%) 2848 <0> (+11) ⬆️
#JDBC43 50.06% <100%> (+0.04%) 2878 <0> (+1) ⬆️
Impacted Files Coverage Δ Complexity Δ
...n/java/com/microsoft/sqlserver/jdbc/tdsparser.java 69.42% <100%> (+1.31%) 0 <0> (ø) ⬇️
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java 61.75% <100%> (+0.84%) 142 <0> (+5) ⬆️
...om/microsoft/sqlserver/jdbc/SimpleInputStream.java 52.23% <0%> (-1.5%) 11% <0%> (-1%)
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java 43.95% <0%> (-1.1%) 15% <0%> (ø)
...n/java/com/microsoft/sqlserver/jdbc/Parameter.java 66.93% <0%> (-0.21%) 63% <0%> (ø)
...n/java/com/microsoft/sqlserver/jdbc/DataTypes.java 79.72% <0%> (-0.2%) 5% <0%> (-1%)
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 55.55% <0%> (+0.06%) 0% <0%> (ø) ⬇️
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java 38.57% <0%> (+0.21%) 43% <0%> (+1%) ⬆️
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java 44.17% <0%> (+0.3%) 322% <0%> (+4%) ⬆️
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java 68.65% <0%> (+0.4%) 0% <0%> (ø) ⬇️
... and 3 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 a1a004e...cb05baa. Read the comment docs.

biasb
biasb suggested changes Mar 15, 2019
View reviewed changes
Copy link
Copy Markdown

@biasb biasb left a comment

Choose a reason for hiding this comment

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

I think testMultipleResultSets() should also test that the following is working (copied from getMoreResults() Javadoc:):

There are no more results when the following is true:
   // stmt is a Statement object
   ((stmt.getMoreResults() == false) && (stmt.getUpdateCount() == -1))

Update the test with an update of any value (or no value) that returns an update count and do another select afterwards so that a ResultSet is expected after the update count.

@biasb
Copy link
Copy Markdown

biasb commented Mar 15, 2019

I have verified that the jre8 version jar works in our test suite.

@cheenamalhotra
Copy link
Copy Markdown
Member Author

cheenamalhotra commented Mar 15, 2019

Hi @biasb

I've added the scenario as you mentioned, please let me know if I misunderstood anything.

sehrope
sehrope reviewed Mar 16, 2019
View reviewed changes
Comment thread src/main/java/com/microsoft/sqlserver/jdbc/SQLServerCallableStatement.java Outdated
sehrope
sehrope reviewed Mar 16, 2019
View reviewed changes
Comment thread src/main/java/com/microsoft/sqlserver/jdbc/tdsparser.java
@cheenamalhotra cheenamalhotra added this to the 7.3.1 milestone Mar 28, 2019
ulvii
ulvii reviewed Apr 8, 2019
View reviewed changes
Comment thread src/test/java/com/microsoft/sqlserver/jdbc/resultset/ResultSetTest.java Outdated
ulvii
ulvii reviewed Apr 8, 2019
View reviewed changes
Comment thread src/main/java/com/microsoft/sqlserver/jdbc/SQLServerStatement.java Outdated
ulvii
ulvii reviewed Apr 8, 2019
View reviewed changes
Comment thread src/test/java/com/microsoft/sqlserver/jdbc/resultset/ResultSetTest.java Outdated
ulvii
ulvii reviewed Apr 8, 2019
View reviewed changes
Comment thread src/test/java/com/microsoft/sqlserver/jdbc/resultset/ResultSetTest.java
@cheenamalhotra cheenamalhotra merged commit 8acab59 into microsoft:dev Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

6 more reviewers

@sehrope sehrope sehrope left review comments

@biasb biasb biasb requested changes

@rene-ye rene-ye rene-ye approved these changes

@lilgreenbird lilgreenbird lilgreenbird approved these changes

@ulvii ulvii ulvii 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

7.3.1

Development

Successfully merging this pull request may close these issues.

8 participants

@cheenamalhotra @codecov-io @biasb @sehrope @rene-ye @lilgreenbird @ulvii @peterbae