Derby
  1. Derby
  2. DERBY-5236

Client driver silently truncates strings that exceed 32KB

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.8.1.2
    • Fix Version/s: 10.8.2.2, 10.9.1.0
    • Component/s: Network Client
    • Labels:
      None
    • Issue & fix info:
      Repro attached
    • Bug behavior facts:
      Embedded/Client difference, Wrong query result

      Description

      Can be seen with this JUnit test case that retrieves a VARCHAR value with 20000 characters. With the client driver, the string is truncated to 10900 characters (32700 bytes when encoded in UTF-8).

      public void testLongColumn() throws SQLException

      { PreparedStatement ps = prepareStatement( "values cast(? as varchar(20000))"); char[] chars = new char[20000]; Arrays.fill(chars, '\u4e10'); String str = new String(chars); ps.setString(1, str); JDBC.assertSingleValueResultSet(ps.executeQuery(), str); }
      1. d5236-5a-fix-version-check.diff
        1 kB
        Knut Anders Hatlen
      2. d5236-4b.diff
        4 kB
        Knut Anders Hatlen
      3. d5236-4a-client-compatibility.diff
        4 kB
        Knut Anders Hatlen
      4. d5236-3a-warning.diff
        26 kB
        Knut Anders Hatlen
      5. d5236-2a-longer-strings.diff
        10 kB
        Knut Anders Hatlen
      6. d5236-1a-client-fetch-complete.diff
        14 kB
        Knut Anders Hatlen
      7. write-full-string.diff
        3 kB
        Knut Anders Hatlen
      8. repro.diff
        3 kB
        Knut Anders Hatlen

        Issue Links

          Activity

          Hide
          Knut Anders Hatlen added a comment -

          The attached patch adds a regression test case for the bug. The test case isn't enabled in any suites yet. Committed revision 1104365.

          The test case passes with the embedded driver, and fails like this with the client driver:

          There was 1 failure:
          1) testLongColumn(org.apache.derbyTesting.functionTests.tests.jdbcapi.Derby5236Test)junit.framework.AssertionFailedError: Column value mismatch @ column '1', row 1:
          Expected: >丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐(...)<
          Found: >丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐(...)<
          at org.apache.derbyTesting.junit.JDBC.assertRowInResultSet(JDBC.java:1213)
          at org.apache.derbyTesting.junit.JDBC.assertRowInResultSet(JDBC.java:1125)
          at org.apache.derbyTesting.junit.JDBC.assertFullResultSetMinion(JDBC.java:1012)
          at org.apache.derbyTesting.junit.JDBC.assertFullResultSet(JDBC.java:935)
          at org.apache.derbyTesting.junit.JDBC.assertFullResultSet(JDBC.java:892)
          at org.apache.derbyTesting.junit.JDBC.assertFullResultSet(JDBC.java:850)
          at org.apache.derbyTesting.junit.JDBC.assertSingleValueResultSet(JDBC.java:835)
          at org.apache.derbyTesting.functionTests.tests.jdbcapi.Derby5236Test.testLongColumn(Derby5236Test.java:57)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:112)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)

          Show
          Knut Anders Hatlen added a comment - The attached patch adds a regression test case for the bug. The test case isn't enabled in any suites yet. Committed revision 1104365. The test case passes with the embedded driver, and fails like this with the client driver: There was 1 failure: 1) testLongColumn(org.apache.derbyTesting.functionTests.tests.jdbcapi.Derby5236Test)junit.framework.AssertionFailedError: Column value mismatch @ column '1', row 1: Expected: >丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐(...)< Found: >丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐丐(...)< at org.apache.derbyTesting.junit.JDBC.assertRowInResultSet(JDBC.java:1213) at org.apache.derbyTesting.junit.JDBC.assertRowInResultSet(JDBC.java:1125) at org.apache.derbyTesting.junit.JDBC.assertFullResultSetMinion(JDBC.java:1012) at org.apache.derbyTesting.junit.JDBC.assertFullResultSet(JDBC.java:935) at org.apache.derbyTesting.junit.JDBC.assertFullResultSet(JDBC.java:892) at org.apache.derbyTesting.junit.JDBC.assertFullResultSet(JDBC.java:850) at org.apache.derbyTesting.junit.JDBC.assertSingleValueResultSet(JDBC.java:835) at org.apache.derbyTesting.functionTests.tests.jdbcapi.Derby5236Test.testLongColumn(Derby5236Test.java:57) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:112) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
          Hide
          Rick Hillegas added a comment -

          Linking to DERBY-5235 since Knut discovered this wrong-results bug as part of the discussion on that issue.

          Show
          Rick Hillegas added a comment - Linking to DERBY-5235 since Knut discovered this wrong-results bug as part of the discussion on that issue.
          Hide
          Knut Anders Hatlen added a comment -

          DDMWriter.writeLDString() truncates the string down to LONGVARCHAR_MAX_LEN bytes (not characters). I tried to change it to just send the full string, regardless of its length (see the attached patch), and that made the repro pass. (I haven't run any other tests.)

          At the location where DRDAConnThread calls writeLDString(), there is a comment about the truncation:

          //WriteLDString and generate warning if truncated
          // which will be picked up by checkWarning()
          writer.writeLDString(val.toString(), index);

          But there is no code that actually generates the warning if the string is truncated.

          Show
          Knut Anders Hatlen added a comment - DDMWriter.writeLDString() truncates the string down to LONGVARCHAR_MAX_LEN bytes (not characters). I tried to change it to just send the full string, regardless of its length (see the attached patch), and that made the repro pass. (I haven't run any other tests.) At the location where DRDAConnThread calls writeLDString(), there is a comment about the truncation: //WriteLDString and generate warning if truncated // which will be picked up by checkWarning() writer.writeLDString(val.toString(), index); But there is no code that actually generates the warning if the string is truncated.
          Hide
          Knut Anders Hatlen added a comment -

          All the regression tests pass with the patch, but I don't think it's the correct solution. At least not a complete solution.

          The patch does make the repro pass, but that repro uses a string that's only 20000 characters and needs 60000 bytes in UTF-8. But since a VARCHAR may contain up to ~32K characters, it may need as much as 96KB when represented in UTF-8. But writeLDString() uses a two-byte length field, so it can only be used for strings that take up to 64KB. If I change the repro to use a 25000 character string that takes 75000 bytes, it still fails.

          Show
          Knut Anders Hatlen added a comment - All the regression tests pass with the patch, but I don't think it's the correct solution. At least not a complete solution. The patch does make the repro pass, but that repro uses a string that's only 20000 characters and needs 60000 bytes in UTF-8. But since a VARCHAR may contain up to ~32K characters, it may need as much as 96KB when represented in UTF-8. But writeLDString() uses a two-byte length field, so it can only be used for strings that take up to 64KB. If I change the repro to use a 25000 character string that takes 75000 bytes, it still fails.
          Hide
          Kathey Marsden added a comment -

          Does it make sense to check in the partial solution to trunk and 10.8? It seems like a significant improvement.
          We could resolve this issue and then open a new one for Strings larger than the one handled by this patch.

          Show
          Kathey Marsden added a comment - Does it make sense to check in the partial solution to trunk and 10.8? It seems like a significant improvement. We could resolve this issue and then open a new one for Strings larger than the one handled by this patch.
          Hide
          Knut Anders Hatlen added a comment -

          I think we could do that. I'll refresh the patch, as I think in its current form it might have made some existing bad behaviour slightly worse. That is, if we get an overflow in the length field, the string will be truncated even more than it was before. Perhaps we should raise an exception in case of an overflow? Sounds better than silently returning the wrong result, IMO.

          By the way, I did a quick check of the DRDA spec back when I looked into this issue, and I found some indications that this length field was supposed to contain the number of characters, not the number of bytes. If that turns out to be the case, the two-byte field should be sufficient for all VARCHARs with the current ~32K limit. Whether we can change the meaning of the length field without breaking client/server compatibility is another question...

          Show
          Knut Anders Hatlen added a comment - I think we could do that. I'll refresh the patch, as I think in its current form it might have made some existing bad behaviour slightly worse. That is, if we get an overflow in the length field, the string will be truncated even more than it was before. Perhaps we should raise an exception in case of an overflow? Sounds better than silently returning the wrong result, IMO. By the way, I did a quick check of the DRDA spec back when I looked into this issue, and I found some indications that this length field was supposed to contain the number of characters, not the number of bytes. If that turns out to be the case, the two-byte field should be sufficient for all VARCHARs with the current ~32K limit. Whether we can change the meaning of the length field without breaking client/server compatibility is another question...
          Hide
          Kathey Marsden added a comment -

          Thank you for working on this. I agree it is better to raise an exception for overflow.
          It is surprising to me that it would be the number of characters. I thought it was all about how many bytes to read. I'll have to take a look.

          Show
          Kathey Marsden added a comment - Thank you for working on this. I agree it is better to raise an exception for overflow. It is surprising to me that it would be the number of characters. I thought it was all about how many bytes to read. I'll have to take a look.
          Hide
          Knut Anders Hatlen added a comment -

          Right, I only did a quick search for how length was represented various places, and I haven't yet done any research into what's the relevant definition for the length field in this particular method. One example I found was in V4, vol 2, p 48:

          Field Length
          format: signed binary integer
          units: characters for DBCS; else bytes
          value:
          0 - 32767 for SBCS and mixed SBCS/DBCS
          0 - 16383 for DBCS

          I believe SBCS is single-byte character set and DBCS is double-byte character set. Now, UTF-8 is a variable-width encoding and therefore neither SBCS nor DBCS, strictly speaking.

          Show
          Knut Anders Hatlen added a comment - Right, I only did a quick search for how length was represented various places, and I haven't yet done any research into what's the relevant definition for the length field in this particular method. One example I found was in V4, vol 2, p 48: Field Length format: signed binary integer units: characters for DBCS; else bytes value: 0 - 32767 for SBCS and mixed SBCS/DBCS 0 - 16383 for DBCS I believe SBCS is single-byte character set and DBCS is double-byte character set. Now, UTF-8 is a variable-width encoding and therefore neither SBCS nor DBCS, strictly speaking.
          Hide
          Knut Anders Hatlen added a comment -

          An alternative to failing when the column is too long, is to add a
          data truncation warning to the result set. The javadoc for
          java.sql.DataTruncation says:

          http://download.oracle.com/javase/6/docs/api/java/sql/DataTruncation.html

          > An exception thrown as a DataTruncation exception (on writes) or
          > reported as a DataTruncation warning (on reads) when a data values is
          > unexpectedly truncated for reasons other than its having execeeded
          > MaxFieldSize.

          That description seems to match the scenario in this bug report. And
          it also looks like that was what the code intended to do (it had
          comments about adding a warning, just not the code to do it).

          Show
          Knut Anders Hatlen added a comment - An alternative to failing when the column is too long, is to add a data truncation warning to the result set. The javadoc for java.sql.DataTruncation says: http://download.oracle.com/javase/6/docs/api/java/sql/DataTruncation.html > An exception thrown as a DataTruncation exception (on writes) or > reported as a DataTruncation warning (on reads) when a data values is > unexpectedly truncated for reasons other than its having execeeded > MaxFieldSize. That description seems to match the scenario in this bug report. And it also looks like that was what the code intended to do (it had comments about adding a warning, just not the code to do it).
          Hide
          Dag H. Wanvik added a comment -

          +1 to error or warning when this happens. Since client behavior is different than the embedded driver here, I'd personally prefer an error since warnings are typically not checked, but I agree DataTruncation warning is reasonable. I wish there were a way to get the entire string over the protocol, though...

          Show
          Dag H. Wanvik added a comment - +1 to error or warning when this happens. Since client behavior is different than the embedded driver here, I'd personally prefer an error since warnings are typically not checked, but I agree DataTruncation warning is reasonable. I wish there were a way to get the entire string over the protocol, though...
          Hide
          Knut Anders Hatlen added a comment -

          I previously mentioned that strings are still truncated when they are longer than 64 KB (because of overflow in the length field). But it also turns out that strings that are just below 64 KB cause a StringIndexOutOfBoundsException on the client. So the current state is:

          0-65514 bytes: OK
          65515 - 65535 bytes: StringIndexOutOfBoundsException
          >=65536: Truncated result

          I'm looking into why the client has a limit at 65514 bytes.

          Show
          Knut Anders Hatlen added a comment - I previously mentioned that strings are still truncated when they are longer than 64 KB (because of overflow in the length field). But it also turns out that strings that are just below 64 KB cause a StringIndexOutOfBoundsException on the client. So the current state is: 0-65514 bytes: OK 65515 - 65535 bytes: StringIndexOutOfBoundsException >=65536: Truncated result I'm looking into why the client has a limit at 65514 bytes.
          Hide
          Kathey Marsden added a comment -

          Knut said So the current state is:

          0-65514 bytes: OK
          65515 - 65535 bytes: StringIndexOutOfBoundsException
          >=65536: Truncated result

          Is that the current state of trunk or the current state of the patch? It seems it would be good to get these improvements into 10.8.2 for the release as the extension to 65514 would be a good improvement even if the StringIndexOutOfBounds is not understood.

          Show
          Kathey Marsden added a comment - Knut said So the current state is: 0-65514 bytes: OK 65515 - 65535 bytes: StringIndexOutOfBoundsException >=65536: Truncated result Is that the current state of trunk or the current state of the patch? It seems it would be good to get these improvements into 10.8.2 for the release as the extension to 65514 would be a good improvement even if the StringIndexOutOfBounds is not understood.
          Hide
          Knut Anders Hatlen added a comment -

          The StringIndexOutOfBoundsException only happens with the patch (and I said the wrong number, it fails on 65510 bytes). The current state of trunk is to send only 32700 bytes, so it'll never attempt to send strings of that size.

          I had a look in a debugger, and it looks like 65509 bytes completely fills two DSSs, and the client never sees the last byte in the 65510-byte string. I haven't figured out yet if the server doesn't send the byte or if it's the client that just doesn't fetch it.

          Show
          Knut Anders Hatlen added a comment - The StringIndexOutOfBoundsException only happens with the patch (and I said the wrong number, it fails on 65510 bytes). The current state of trunk is to send only 32700 bytes, so it'll never attempt to send strings of that size. I had a look in a debugger, and it looks like 65509 bytes completely fills two DSSs, and the client never sees the last byte in the 65510-byte string. I haven't figured out yet if the server doesn't send the byte or if it's the client that just doesn't fetch it.
          Hide
          Knut Anders Hatlen added a comment -

          The problem with strings that take 65510 bytes is indeed related to the string being split in three parts on transfer from the server. The client already knows that values can be split, and handles that by sending a CNTQRY command to the server when it detects that it needs more data, but it implicitly assumes that each value is only split once. That assumption is valid currently, since we never send strings long enough to span three blocks. This changed with the server-side fix to stop truncating the strings, and the client stops sending CNTQRY commands too early, and there's not enough data in the receive buffer to construct the string when ResultSet.getString() is called.

          Just changing an "if" statement to a "while" statement in NetCursor.skipFdocaBytes() appears to be enough to get the client to send CNTQRY commands until the entire value has been retreived. This pattern is repeated in many methods in NetCursor. I think only skipFdocaBytes() will ever need to send more than one CNTQRY, even when the truncation is fixed on the server, but for completeness and consistency I factored out the common code into a helper method so that all of them now call CNTQRY in a loop. See the attached patch d5236-1a-client-fetch-complete.diff.

          With the d5236-1a patch in combination with the write-full-string.diff patch, strings up to 65535 (2^16-1) bytes can be fetched. Larger strings will still cause problems because they overflow the two-byte length field.

          Show
          Knut Anders Hatlen added a comment - The problem with strings that take 65510 bytes is indeed related to the string being split in three parts on transfer from the server. The client already knows that values can be split, and handles that by sending a CNTQRY command to the server when it detects that it needs more data, but it implicitly assumes that each value is only split once. That assumption is valid currently, since we never send strings long enough to span three blocks. This changed with the server-side fix to stop truncating the strings, and the client stops sending CNTQRY commands too early, and there's not enough data in the receive buffer to construct the string when ResultSet.getString() is called. Just changing an "if" statement to a "while" statement in NetCursor.skipFdocaBytes() appears to be enough to get the client to send CNTQRY commands until the entire value has been retreived. This pattern is repeated in many methods in NetCursor. I think only skipFdocaBytes() will ever need to send more than one CNTQRY, even when the truncation is fixed on the server, but for completeness and consistency I factored out the common code into a helper method so that all of them now call CNTQRY in a loop. See the attached patch d5236-1a-client-fetch-complete.diff. With the d5236-1a patch in combination with the write-full-string.diff patch, strings up to 65535 (2^16-1) bytes can be fetched. Larger strings will still cause problems because they overflow the two-byte length field.
          Hide
          Knut Anders Hatlen added a comment -

          All the regression tests ran cleanly with the d5236-1a patch. I think that patch can go in separately before we go on with the server-side fix, so I'm setting the patch available flag.

          Show
          Knut Anders Hatlen added a comment - All the regression tests ran cleanly with the d5236-1a patch. I think that patch can go in separately before we go on with the server-side fix, so I'm setting the patch available flag.
          Hide
          Bryan Pendleton added a comment -

          I remember wondering about this years ago when I looked at the DRDA code. Your approach
          seems simple and reasonable, and I am pleased with the reduction in code by extracting all
          that common logic to a single place. Nice work!

          +1

          Show
          Bryan Pendleton added a comment - I remember wondering about this years ago when I looked at the DRDA code. Your approach seems simple and reasonable, and I am pleased with the reduction in code by extracting all that common logic to a single place. Nice work! +1
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for the quick review, Bryan!

          Committed revision 1163131.

          Show
          Knut Anders Hatlen added a comment - Thanks for the quick review, Bryan! Committed revision 1163131.
          Hide
          Knut Anders Hatlen added a comment -

          Attached is a patch that improves the "write-full-string.diff" patch in the following ways:

          • Truncate long strings to 65535 bytes to avoid overflow in the length field (which caused protocol errors)
          • Align with character boundaries when truncating so that the server doesn't send invalid UTF-8 sequences to the client
          • Add more test cases and move them into the existing derbynet.PrepareStatementTest

          The patch still doesn't make the queries fail or add any warnings if the strings are truncated, but I think it can go in separately since it allows more strings to be sent untruncated, and those that are still truncated are sent as longer values than currently.

          All the regression tests ran cleanly with the patch.

          Show
          Knut Anders Hatlen added a comment - Attached is a patch that improves the "write-full-string.diff" patch in the following ways: Truncate long strings to 65535 bytes to avoid overflow in the length field (which caused protocol errors) Align with character boundaries when truncating so that the server doesn't send invalid UTF-8 sequences to the client Add more test cases and move them into the existing derbynet.PrepareStatementTest The patch still doesn't make the queries fail or add any warnings if the strings are truncated, but I think it can go in separately since it allows more strings to be sent untruncated, and those that are still truncated are sent as longer values than currently. All the regression tests ran cleanly with the patch.
          Hide
          Knut Anders Hatlen added a comment -

          Marking patch available.

          In a later patch, we may want to change the truncation logic so that it only allows these longer strings if the client has the fix that makes it able to handle them. Otherwise, the StringIndexOutOfBoundsException might show up in mixed version environments. Falling back to the old behaviour (truncating to 32700 bytes) would probably be good enough if the server detects that the client is too old.

          Show
          Knut Anders Hatlen added a comment - Marking patch available. In a later patch, we may want to change the truncation logic so that it only allows these longer strings if the client has the fix that makes it able to handle them. Otherwise, the StringIndexOutOfBoundsException might show up in mixed version environments. Falling back to the old behaviour (truncating to 32700 bytes) would probably be good enough if the server detects that the client is too old.
          Hide
          Knut Anders Hatlen added a comment -

          Committed revision 1164495.

          Show
          Knut Anders Hatlen added a comment - Committed revision 1164495.
          Hide
          Knut Anders Hatlen added a comment -

          I don't see a quick solution to get the entire string sent to the client, so I'm adding a patch that adds a java.sql.DataTruncation warning to the result instead for now.

          The server adds warnings before the row data. However, we don't know until after we've tried writing the data whether we had to truncate any values. Because of this, I've changed the server code to save the position of the SQLCAGRP section in which the warnings are written, and if it detects that data was truncated, it goes back and overwrites it with the data truncation warnings later.

          On the client side, some changes were needed for it to correctly deserialize a java.sql.DataTruncation object. It wouldn't break without these changes, but it would create an ordinary SQLWarning instead of DataTruncation. The DataTruncation object has fields so that the caller can find out exactly which column was truncated, and by how much.

          Unfortunately, the server isn't capable of sending chained warnings yet, so in the case of multiple columns being truncated, we only get a warning for the first one. But that's another bug...

          The patch also makes the test verify that the proper DataTruncation warnings are received, and a test case that verifies that it also works as expected with output parameters in stored procedures has been added.

          All the regression tests ran cleanly with the patch.

          Show
          Knut Anders Hatlen added a comment - I don't see a quick solution to get the entire string sent to the client, so I'm adding a patch that adds a java.sql.DataTruncation warning to the result instead for now. The server adds warnings before the row data. However, we don't know until after we've tried writing the data whether we had to truncate any values. Because of this, I've changed the server code to save the position of the SQLCAGRP section in which the warnings are written, and if it detects that data was truncated, it goes back and overwrites it with the data truncation warnings later. On the client side, some changes were needed for it to correctly deserialize a java.sql.DataTruncation object. It wouldn't break without these changes, but it would create an ordinary SQLWarning instead of DataTruncation. The DataTruncation object has fields so that the caller can find out exactly which column was truncated, and by how much. Unfortunately, the server isn't capable of sending chained warnings yet, so in the case of multiple columns being truncated, we only get a warning for the first one. But that's another bug... The patch also makes the test verify that the proper DataTruncation warnings are received, and a test case that verifies that it also works as expected with output parameters in stored procedures has been added. All the regression tests ran cleanly with the patch.
          Hide
          Knut Anders Hatlen added a comment -

          Committed revision 1167017.

          Show
          Knut Anders Hatlen added a comment - Committed revision 1167017.
          Hide
          Knut Anders Hatlen added a comment -

          Attaching a new patch that disables the server-side fix when talking to old clients, since those clients may get a StringIndexOutOfBoundsException if they receive longer strings, and also because they don't know exactly how to handle java.sql.DataTruncation warnings.

          I've manually verified that old clients (tested 10.1.2.1 and 10.8.1.2) receive shorter strings (just like they did before this issue) instead of failing with a StringIndexOutOfBoundsException for these longer strings. Running the full regression test suite now.

          The fix is only disabled on releases prior to 10.8.2, on the assumption that the fix gets into the upcoming 10.8.2 release. If it doesn't we'll have to change that logic on trunk.

          Show
          Knut Anders Hatlen added a comment - Attaching a new patch that disables the server-side fix when talking to old clients, since those clients may get a StringIndexOutOfBoundsException if they receive longer strings, and also because they don't know exactly how to handle java.sql.DataTruncation warnings. I've manually verified that old clients (tested 10.1.2.1 and 10.8.1.2) receive shorter strings (just like they did before this issue) instead of failing with a StringIndexOutOfBoundsException for these longer strings. Running the full regression test suite now. The fix is only disabled on releases prior to 10.8.2, on the assumption that the fix gets into the upcoming 10.8.2 release. If it doesn't we'll have to change that logic on trunk.
          Hide
          Knut Anders Hatlen added a comment -

          The tests didn't pass because the AppRequester used to check the client version wasn't available when the requests came from NetworkServerControl, which caused NPEs. Will try to come up with a new patch.

          Show
          Knut Anders Hatlen added a comment - The tests didn't pass because the AppRequester used to check the client version wasn't available when the requests came from NetworkServerControl, which caused NPEs. Will try to come up with a new patch.
          Hide
          Kathey Marsden added a comment -

          I guess this means it will be hard to backport this change to older branches without making a new release where we can bump the third digit. I have some vague recollection of a DRDA maintenance version that could be bumped for such a fix.

          Show
          Kathey Marsden added a comment - I guess this means it will be hard to backport this change to older branches without making a new release where we can bump the third digit. I have some vague recollection of a DRDA maintenance version that could be bumped for such a fix.
          Hide
          Knut Anders Hatlen added a comment -

          Yes, I think that's right. But since the fix only reduces the original problem (strings are still truncated, just not as much as before), I'm not sure it's worthwhile putting too much effort into backporting it far back.

          Show
          Knut Anders Hatlen added a comment - Yes, I think that's right. But since the fix only reduces the original problem (strings are still truncated, just not as much as before), I'm not sure it's worthwhile putting too much effort into backporting it far back.
          Hide
          Knut Anders Hatlen added a comment -

          Attached is an updated patch (4b) which handles the case where
          writeLDString() sends data to a NetworkServerControl client and the
          AppRequester is null. In that case, we don't need to disable the fix,
          since the StringIndexOutOfBoundsException we're trying to avoid, only
          happens in the client JDBC driver.

          All the regression tests ran cleanly with this version of the patch.

          Committed to trunk with revision 1167470.

          Show
          Knut Anders Hatlen added a comment - Attached is an updated patch (4b) which handles the case where writeLDString() sends data to a NetworkServerControl client and the AppRequester is null. In that case, we don't need to disable the fix, since the StringIndexOutOfBoundsException we're trying to avoid, only happens in the client JDBC driver. All the regression tests ran cleanly with this version of the patch. Committed to trunk with revision 1167470.
          Hide
          Knut Anders Hatlen added a comment -

          It turns out I misunderstood the third parameter to AppRequester.greaterThanOrEqualTo(). It's not the third digit of the version number, it's actually the DRDA maintenance version Kathey mentioned. The product id sent by the 10.8.1.2 client is DNC10080, and that won't change when we bump the version number. So it looks like it's the DRDA maintenance version and not the Derby version number we should bump when backporting.

          Show
          Knut Anders Hatlen added a comment - It turns out I misunderstood the third parameter to AppRequester.greaterThanOrEqualTo(). It's not the third digit of the version number, it's actually the DRDA maintenance version Kathey mentioned. The product id sent by the 10.8.1.2 client is DNC10080, and that won't change when we bump the version number. So it looks like it's the DRDA maintenance version and not the Derby version number we should bump when backporting.
          Hide
          Knut Anders Hatlen added a comment -

          Attaching d5236-5a-fix-version-check.diff which fixes the version check in AppRequester.supportsLongerLDStrings() to check for the DRDA maintenance version (1) instead of the third digit in the version number (2).

          Committed revision 1169692.

          Show
          Knut Anders Hatlen added a comment - Attaching d5236-5a-fix-version-check.diff which fixes the version check in AppRequester.supportsLongerLDStrings() to check for the DRDA maintenance version (1) instead of the third digit in the version number (2). Committed revision 1169692.
          Hide
          Knut Anders Hatlen added a comment -

          I've merged all the fixes to the 10.8 branch and bumped the DRDA maintenance version on the branch. Committed revision 1169698.

          Marking the issue as resolved.

          Show
          Knut Anders Hatlen added a comment - I've merged all the fixes to the 10.8 branch and bumped the DRDA maintenance version on the branch. Committed revision 1169698. Marking the issue as resolved.

            People

            • Assignee:
              Knut Anders Hatlen
              Reporter:
              Knut Anders Hatlen
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development