Fixed a bug where calling length() after obtaining a stream would close the stream for Clobs/NClobs #799

rene-ye · 2018-09-05T16:46:52Z

Addresses #788. Also addressed an issue where varchar(max)/Clob objects were always being encoded to UTF-16LE instead of using the Collation specified in the Clob object.

List of Changes (the following applies to NClob as well):

calling Clob.length() no longer attempts to load the stream into a string. This fixes a null pointer issue as well as length() closing user streams.
added streaming capabilities for Clob.getAsciiStream(). NClobs will always have a non-streaming implementation for getAsciiStream().
Clobs no longer default to UTF-16LE. Clobs now respect the collation from the SQL Server. NClobs remain unchanged, will always be UTF-16LE encoding.

pull

…tead of always UTF-16LE

codecov-io · 2018-09-05T17:12:44Z

Codecov Report

Merging #799 into dev will increase coverage by 0.07%.
The diff coverage is 76.66%.

@@             Coverage Diff              @@
##                dev     #799      +/-   ##
============================================
+ Coverage     48.53%   48.61%   +0.07%     
- Complexity     2806     2809       +3     
============================================
  Files           116      116              
  Lines         27862    27882      +20     
  Branches       4641     4645       +4     
============================================
+ Hits          13523    13554      +31     
+ Misses        12197    12195       -2     
+ Partials       2142     2133       -9

Flag	Coverage Δ	Complexity Δ
#JDBC42	`48.08% <76.66%> (+0.43%)`	`2766 <3> (+30)`	⬆️
#JDBC43	`48.24% <76.66%> (-0.19%)`	`2781 <3> (-20)`

Impacted Files	Coverage Δ	Complexity Δ
...om/microsoft/sqlserver/jdbc/SimpleInputStream.java	`52.94% <ø> (ø)`	`11 <0> (ø)`	⬇️
...a/com/microsoft/sqlserver/jdbc/PLPInputStream.java	`58.33% <ø> (+5.35%)`	`33 <0> (+4)`	⬆️
...a/com/microsoft/sqlserver/jdbc/SQLServerNClob.java	`74.07% <100%> (+4.5%)`	`13 <3> (+1)`	⬆️
...va/com/microsoft/sqlserver/jdbc/SQLServerClob.java	`35.58% <72%> (+3.3%)`	`12 <0> (ø)`	⬇️
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java	`43.95% <0%> (-4.4%)`	`15% <0%> (-1%)`
...m/microsoft/sqlserver/jdbc/SQLServerException.java	`77.27% <0%> (-1.52%)`	`32% <0%> (-1%)`
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java	`44.63% <0%> (-1.1%)`	`106% <0%> (-1%)`
...n/java/com/microsoft/sqlserver/jdbc/Parameter.java	`63.52% <0%> (-0.21%)`	`64% <0%> (ø)`
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java	`62.25% <0%> (-0.12%)`	`0% <0%> (ø)`
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java	`32.69% <0%> (-0.08%)`	`255% <0%> (-1%)`
... and 5 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 550d441...8746b95. Read the comment docs.

src/test/java/com/microsoft/sqlserver/jdbc/unit/lobs/lobsTest.java

src/test/java/com/microsoft/sqlserver/jdbc/unit/lobs/lobsStreamingTest.java

…trieving stream but before consuming stream

cheenamalhotra · 2018-09-18T18:17:38Z

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerClob.java

+            try {
+                inputStream.reset();
+            } catch (IOException e) {
+                throw new SQLServerException(e.getMessage(), null, 0, e);


Please use "SQLServerException.makeFromDriverError" instead. Also applies to other locations in the changes made.

cheenamalhotra · 2018-09-18T18:26:22Z

src/test/java/com/microsoft/sqlserver/jdbc/unit/lobs/LobsStreamingTest.java

+                    NClob c = rs.getNClob(2);
+                    assertEquals(c.length(), lobs.get(rs.getInt(1)).length());
+                    lobsFromServer.add(c);
+                    String recieved = getStringFromInputStream(c.getAsciiStream());// streaming string


Please move Resultsets and Streams to try-with-resources. Avoid hidden streams calls.

Resultsets are automatically closed when statement is closed, and statement is in try-with-resources. Inputstreams are closed when resultset is closed, and I manually call resultset.close() in order to load stream into memory.

cheenamalhotra · 2018-09-18T18:34:57Z

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerClob.java

+            InputStream inputStream = (InputStream) activeStreams.get(0);
+            try {
+                inputStream.reset();
+                getterStream = inputStream;


Can we close 'inputStream' here?

It's the same input stream that the user gets, if we close it, the stream returned to the user gets closed.

cheenamalhotra · 2018-09-18T18:37:37Z

src/test/java/com/microsoft/sqlserver/jdbc/unit/lobs/LobsStreamingTest.java

+                }
+                rs.close();
+            }
+            stmt.execute("DROP TABLE [" + tableName + "]");


Drop table in finally block to avoid table leftovers in case exception occurs.

stmt was created in try-catch, cannot access in finally block.

There is already dropTableIfExists() in Utils.java, consider using it.

Move the drop table into a finally block. You can either create a new Statement for it or just use regular try-catch blocks.

Or a cleaner solution would be to modify dropTableIfExists() to create and close its own Statement.

lilgreenbird · 2018-09-18T22:04:59Z

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerClob.java

-
-        if (value == null && activeStreams.get(0) instanceof PLPInputStream) {
-            return (long) ((PLPInputStream) activeStreams.get(0)).payloadLength / 2;
+        if (value == null && activeStreams.get(0) instanceof BaseInputStream) {


null == value

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerClob.java

src/test/java/com/microsoft/sqlserver/jdbc/unit/lobs/LobsStreamingTest.java

ulvii · 2018-09-19T21:19:32Z

src/test/java/com/microsoft/sqlserver/jdbc/unit/lobs/LobsStreamingTest.java

+                        assertEquals(lobs.get(index), recieved);// compare streamed string to initial string
+                    }
+                    rs.close();
+                    for (int i = 0; i < lobs.size(); i++) {


This part is unnecessary now.

ulvii · 2018-09-19T21:20:31Z

src/test/java/com/microsoft/sqlserver/jdbc/unit/lobs/LobsStreamingTest.java

+                        assertEquals(c.length(), lobs.get(index).length());
+                        lobsFromServer.add(c);
+                        String recieved = getStringFromInputStream(c.getAsciiStream(),
+                                java.nio.charset.StandardCharsets.US_ASCII);// streaming string


Please fix the comment (with explanation), we are not streaming anymore.

src/test/java/com/microsoft/sqlserver/jdbc/unit/lobs/LobsStreamingTest.java

…sue788

lilgreenbird · 2018-09-21T20:05:53Z

src/test/java/com/microsoft/sqlserver/jdbc/unit/lobs/LobsStreamingTest.java

+            + "Ǥ⚌c♮ƺåYèĢù⚏Ȓ★ǌäõpƸŃōoƝĤßuÙőƆE♹gǇÜŬȺǱ!Û☵ŦãǁĸNQŰǚǻTÖC]ǶýåÉbɉ☩=\\ȍáźŗǃĻýű☓☄¸T☑ö^k☏I:x☑⚀läiȉ☱☚⚅ǸǎãÂ";
+    private static String tableName = null;
+
+    private static enum LoB {


seem to be odd camel casing B here??

lilgreenbird · 2018-09-21T20:24:49Z

src/test/java/com/microsoft/sqlserver/jdbc/unit/lobs/LobsStreamingTest.java

+        while (true) {
+            int amountRead = r.read(buffer, 0, (int) l);
+            if (amountRead == -1) {
+                break;


hmm better if we do while ((amountRead = r.read(buffer, 0, (int) l)) != null)? then we don't have to break out of a while which makes me cringe a little when I see it... ha not big deal ;)

lilgreenbird · 2018-09-21T20:36:19Z

src/test/java/com/microsoft/sqlserver/jdbc/unit/lobs/LobsStreamingTest.java

+                    Clob c = rs.getClob(2);
+                    Reader r = c.getCharacterStream();
+                    long clobLength = c.length();
+                    String recieved = getStringFromReader(r, clobLength);// streaming string


received - "i before e except after c" ;)

lilgreenbird · 2018-09-21T21:20:39Z

src/test/java/com/microsoft/sqlserver/jdbc/unit/lobs/LobsStreamingTest.java

+                    NClob c = rs.getNClob(2);
+                    assertEquals(c.length(), lob_data.get(index).length());
+                    lobsFromServer.add(c);
+                    String recieved = getStringFromInputStream(c.getAsciiStream());// NClob AsciiStream is never


rene-ye and others added 4 commits August 28, 2018 17:08

filling clob if stream is not of type plpinputstream

bb254f2

fix for simpleinputstream

225c985

Merge pull request #46 from Microsoft/dev

86a5887

pull

Added more streaming tests. Changed Clob to use different charset ins…

8f6d197

…tead of always UTF-16LE

jonyschak mentioned this pull request Sep 7, 2018

Clob.getCharacterStream() behavior difference when called before vs after ResultSet is closed for varchar(max) #800

Closed

rene-ye added 8 commits September 7, 2018 09:43

cleaning up implementation

a6ad39a

removing unused imports

9619543

added length check to tests and updated length() for clobs/nclobs

1ebc48c

enabled streaming for getAsciiStream

72b002c

cleaning up ascii stream implementation.

6b7956d

length() for NClob

0785932

reverting exposing private methods as protected methods

d272502

update test

0cc0b90

peterbae reviewed Sep 14, 2018

View reviewed changes

src/test/java/com/microsoft/sqlserver/jdbc/unit/lobs/lobsTest.java Outdated Show resolved Hide resolved

peterbae reviewed Sep 14, 2018

View reviewed changes

src/test/java/com/microsoft/sqlserver/jdbc/unit/lobs/lobsStreamingTest.java Outdated Show resolved Hide resolved

peterbae reviewed Sep 14, 2018

View reviewed changes

src/test/java/com/microsoft/sqlserver/jdbc/unit/lobs/lobsStreamingTest.java Outdated Show resolved Hide resolved

upped clob/nclob max length, added test for calling length() after re…

26df3df

…trieving stream but before consuming stream

cheenamalhotra added this to the 7.1.1 milestone Sep 17, 2018

cheenamalhotra reviewed Sep 18, 2018

View reviewed changes

lilgreenbird reviewed Sep 18, 2018

View reviewed changes

rene-ye added 3 commits September 19, 2018 10:24

changes addressing reviews

4ffee2e

returning bufferedinputstream instead of inputstream

557b4a1

formatting

9a0fd0c

lilgreenbird reviewed Sep 19, 2018

View reviewed changes

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerClob.java Show resolved Hide resolved

normalize nclob getAsciiStream behavior

1844970

rene-ye added 2 commits September 19, 2018 12:49

test fix

e613240

use getCharacterStream for nclobs

c692c68

ulvii reviewed Sep 19, 2018

View reviewed changes

src/test/java/com/microsoft/sqlserver/jdbc/unit/lobs/LobsStreamingTest.java Outdated Show resolved Hide resolved

ulvii reviewed Sep 19, 2018

View reviewed changes

removing redundant code

c3fcc9d

ulvii reviewed Sep 20, 2018

View reviewed changes

src/test/java/com/microsoft/sqlserver/jdbc/unit/lobs/LobsStreamingTest.java Show resolved Hide resolved

rene-ye added 2 commits September 21, 2018 09:22

removing unused imports

7ebaa44

Merge branch 'dev' of https://github.com/Microsoft/mssql-jdbc into is…

1702730

…sue788

lilgreenbird reviewed Sep 21, 2018

View reviewed changes

changing enum name

df7e79b

lilgreenbird reviewed Sep 21, 2018

View reviewed changes

rene-ye added 2 commits September 21, 2018 13:36

refractor while loop

9ac2149

corrected spelling of 'recieved' to 'received'

8260ce9

lilgreenbird reviewed Sep 21, 2018

View reviewed changes

place rs in try-catch

d3179aa

lilgreenbird previously approved these changes Sep 24, 2018

View reviewed changes

free lobs when not needed

8746b95

rene-ye dismissed lilgreenbird’s stale review via 8746b95 September 24, 2018 20:07

ulvii approved these changes Sep 24, 2018

View reviewed changes

cheenamalhotra approved these changes Sep 24, 2018

View reviewed changes

rene-ye merged commit 037a11d into microsoft:dev Sep 25, 2018

rene-ye mentioned this pull request Sep 25, 2018

NullPointerException after upgrading JDBC-driver from 6.4 to 7.0 #788

Closed

rene-ye mentioned this pull request Oct 17, 2018

materialized_clob (hibernate) returns iso encoded content as utf16 (chinese!!) - fixed with 7.1.1 #832

Closed

rene-ye deleted the issue788 branch October 10, 2019 23:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed a bug where calling length() after obtaining a stream would close the stream for Clobs/NClobs #799

Fixed a bug where calling length() after obtaining a stream would close the stream for Clobs/NClobs #799

rene-ye commented Sep 5, 2018 •

edited

Loading

codecov-io commented Sep 5, 2018 •

edited

Loading

cheenamalhotra Sep 18, 2018 •

edited

Loading

cheenamalhotra Sep 18, 2018

rene-ye Sep 18, 2018

cheenamalhotra Sep 18, 2018

rene-ye Sep 18, 2018

cheenamalhotra Sep 18, 2018

rene-ye Sep 18, 2018

ulvii Sep 18, 2018

ulvii Sep 19, 2018

lilgreenbird Sep 18, 2018

ulvii Sep 19, 2018

ulvii Sep 19, 2018

lilgreenbird Sep 21, 2018

lilgreenbird Sep 21, 2018

lilgreenbird Sep 21, 2018

lilgreenbird Sep 21, 2018

Fixed a bug where calling length() after obtaining a stream would close the stream for Clobs/NClobs #799

Fixed a bug where calling length() after obtaining a stream would close the stream for Clobs/NClobs #799

Conversation

rene-ye commented Sep 5, 2018 • edited Loading

codecov-io commented Sep 5, 2018 • edited Loading

Codecov Report

cheenamalhotra Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rene-ye commented Sep 5, 2018 •

edited

Loading

codecov-io commented Sep 5, 2018 •

edited

Loading

cheenamalhotra Sep 18, 2018 •

edited

Loading