Derby
  1. Derby
  2. DERBY-2769

Implement error handling/parameter checking in Clob.setString

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.3.1.4
    • Fix Version/s: 10.6.1.0
    • Component/s: JDBC
    • Labels:
      None
    • Urgency:
      Normal
    • Issue & fix info:
      Newcomer, Release Note Needed

      Description

      The error handling, or parameter checking, in Clob.subString is not adequate.
      There are four parameters that can be invalid;

      • pos
      • str
      • offset
      • len

      The first one is already handled properly, the remaining three are not. They typically result in some low-level exception like a NPE.
      I have not found anything in the JDBC specification nor JavaDoc that dictates the behavior, except for that SQLException should use states defined in the SQL 2003 specification. A brief search there resulted in the following possibilities:
      22003 - numeric value out of range
      22004 - null value not allowed
      2200F - zero-length character string
      22011 - substring error
      22023 - invalid parameter value

      Some of these are already defined by Derby, but with unsuitable or very specific error messages.

      1. DERBY-2769-1.patch
        5 kB
        Yun Lee
      2. DERBY-2769-1.stat
        0.2 kB
        Yun Lee
      3. DERBY-2769-2.patch
        5 kB
        Yun Lee
      4. DERBY-2769-2.stat
        0.2 kB
        Yun Lee
      5. DERBY-2769-3-a.diff
        7 kB
        Yun Lee
      6. DERBY-2769-3-a.stat
        0.2 kB
        Yun Lee
      7. DERBY-2769-3-b.diff
        5 kB
        Yun Lee
      8. DERBY-2769-3-b.stat
        0.3 kB
        Yun Lee
      9. DERBY-2769-4.diff
        10 kB
        Yun Lee
      10. DERBY-2769-4.stat
        0.4 kB
        Yun Lee
      11. releaseNote.html
        5 kB
        Kristian Waagan
      12. releaseNote.html
        5 kB
        Kristian Waagan
      13. releaseNote.html
        5 kB
        Yun Lee

        Issue Links

          Activity

          Hide
          Yun Lee added a comment -

          I have added param check into Clob.setString(). While, I think empty String should be accepted by Clob.setString(), as it's accepted by

          testLengthAfterInsertOnEmpty()
          testTruncateExactInMemory()
          testTruncateTooLongInMemory()

          in ClobTest in revision 759427.

          Please check it. Thanks!

          Show
          Yun Lee added a comment - I have added param check into Clob.setString(). While, I think empty String should be accepted by Clob.setString(), as it's accepted by testLengthAfterInsertOnEmpty() testTruncateExactInMemory() testTruncateTooLongInMemory() in ClobTest in revision 759427. Please check it. Thanks!
          Hide
          Bryan Pendleton added a comment -

          I think it should be legal to specify a 0-length string to Clob.setString(). For example,
          I think code like this should be legal:

          myClob.setString(0, "bryan", 0, 0);

          Show
          Bryan Pendleton added a comment - I think it should be legal to specify a 0-length string to Clob.setString(). For example, I think code like this should be legal: myClob.setString(0, "bryan", 0, 0);
          Hide
          Kristian Waagan added a comment -

          > myClob.setString(0, "bryan", 0, 0);

          I think you may get into trouble with that one due to the invalid insert position, but I guess this should work (and return zero):
          myClob.setString(1, "bryan", 0, 0);

          I also think we have discussed the following note from the JavaDoc, but I don't remember where we discussed it:
          "If the value specified for pos is greater then the length+1 of the CLOB value then the behavior is undefined. Some JDBC drivers may throw a SQLException while other drivers may support this operation. "
          This may be handled properly already for setString, but it wouldn't hurt to confirm it.

          Show
          Kristian Waagan added a comment - > myClob.setString(0, "bryan", 0, 0); I think you may get into trouble with that one due to the invalid insert position, but I guess this should work (and return zero): myClob.setString(1, "bryan", 0, 0); I also think we have discussed the following note from the JavaDoc, but I don't remember where we discussed it: "If the value specified for pos is greater then the length+1 of the CLOB value then the behavior is undefined. Some JDBC drivers may throw a SQLException while other drivers may support this operation. " This may be handled properly already for setString, but it wouldn't hurt to confirm it.
          Hide
          Yun Lee added a comment -

          > myClob.setString(0, "bryan", 0, 0);
          It's not supported by the current function.

          >I think you may get into trouble with that one due to the invalid insert position, but I guess this should work (and return zero):
          >myClob.setString(1, "bryan", 0, 0);
          It's supported.

          As to the 'empty String' I mentioned, I should do some clarificaiton. I just meant 'empty String ' as '', a Stirng with zero length. And It should be accepted, as some test cases have adopted this way.

          Show
          Yun Lee added a comment - > myClob.setString(0, "bryan", 0, 0); It's not supported by the current function. >I think you may get into trouble with that one due to the invalid insert position, but I guess this should work (and return zero): >myClob.setString(1, "bryan", 0, 0); It's supported. As to the 'empty String' I mentioned, I should do some clarificaiton. I just meant 'empty String ' as '', a Stirng with zero length. And It should be accepted, as some test cases have adopted this way.
          Hide
          Kristian Waagan added a comment -

          BTW, the current patch doesn't compile in my sandbox (due to an import). The import also looks incorrect, as it introduces a dependency from the product jar (derby.jar) to the testing jar.

          To catch issues like this, the 'clobber' target should be run prior to the compile targets. This target makes sure your patch can be compiled from scratch, with no existing class files lying around etc.

          Thanks,

          Show
          Kristian Waagan added a comment - BTW, the current patch doesn't compile in my sandbox (due to an import). The import also looks incorrect, as it introduces a dependency from the product jar (derby.jar) to the testing jar. To catch issues like this, the 'clobber' target should be run prior to the compile targets. This target makes sure your patch can be compiled from scratch, with no existing class files lying around etc. Thanks,
          Hide
          Yun Lee added a comment -

          Kistian, thanks for your comments. I will turn back on weekend to provide new patch,

          Show
          Yun Lee added a comment - Kistian, thanks for your comments. I will turn back on weekend to provide new patch,
          Hide
          Yun Lee added a comment -

          Kristian, I have removed the wrong import in my new patch,.Please check it! I won't make the same mistake anymore.

          Yun

          Show
          Yun Lee added a comment - Kristian, I have removed the wrong import in my new patch,.Please check it! I won't make the same mistake anymore. Yun
          Hide
          Kathey Marsden added a comment -

          I'll take a look at the new patch.

          Show
          Kathey Marsden added a comment - I'll take a look at the new patch.
          Hide
          Kathey Marsden added a comment -

          Thank you Yun for the patch. Below are my comments.
          ClobTest.java
          Tests should use assertSQLState("<expected state>", sqle) to verify we got the correct exception instead of assertTrue;

          Clob.java
          if (offset + len > str.length())

          { throw new SqlException(agent_.logWriter_, new ClientMessageId(SQLState.BLOB_LENGTH_TOO_LONG), new Integer(len)); }

          For this condition we throw the exception
          "The length specified '

          {0}' exceeds the size of the BLOB/BLOB."
          but the actual problem is not the blob length but something like.
          "The range specified for the substring with offset {0}

          and len

          {1}

          is not valid"
          We would need a new message for this, perhaps with SQLState 22011. You can have multiple messages with the same SQLState.

          BTW, I noticed the XJ079 message should say BLOB/CLOB not BLOB/BLOB. Could we fix that as part of this issue?

          I noticed another existing condition which doesn't look quite right.
          if ((offset < 0) || offset > str.length() ) {
          Should this be if ((offset < 0) || offset >= str.length() ) {

          EmbedClob.java
          same comments as client for the (offset + len > str.length()) and ((offset < 0) || offset > str.length() ) cases.

          Show
          Kathey Marsden added a comment - Thank you Yun for the patch. Below are my comments. ClobTest.java Tests should use assertSQLState("<expected state>", sqle) to verify we got the correct exception instead of assertTrue; Clob.java if (offset + len > str.length()) { throw new SqlException(agent_.logWriter_, new ClientMessageId(SQLState.BLOB_LENGTH_TOO_LONG), new Integer(len)); } For this condition we throw the exception "The length specified ' {0}' exceeds the size of the BLOB/BLOB." but the actual problem is not the blob length but something like. "The range specified for the substring with offset {0} and len {1} is not valid" We would need a new message for this, perhaps with SQLState 22011. You can have multiple messages with the same SQLState. BTW, I noticed the XJ079 message should say BLOB/CLOB not BLOB/BLOB. Could we fix that as part of this issue? I noticed another existing condition which doesn't look quite right. if ((offset < 0) || offset > str.length() ) { Should this be if ((offset < 0) || offset >= str.length() ) { EmbedClob.java same comments as client for the (offset + len > str.length()) and ((offset < 0) || offset > str.length() ) cases.
          Hide
          Yun Lee added a comment -

          Kathey, I agree with what you have pointed out. I have adopted them in the new patch "DERBY-2769-3-a.diff", but one thing is different from the last patch: a empty string will be accepted, as 4 test cases in ClobTest.java has done in this way, such as testTruncateExactInMemory(), testTruncateExactOnDisk(), which use insertDataWithToken().

          Please check it, thanks!

          Show
          Yun Lee added a comment - Kathey, I agree with what you have pointed out. I have adopted them in the new patch " DERBY-2769 -3-a.diff", but one thing is different from the last patch: a empty string will be accepted, as 4 test cases in ClobTest.java has done in this way, such as testTruncateExactInMemory(), testTruncateExactOnDisk(), which use insertDataWithToken(). Please check it, thanks!
          Hide
          Yun Lee added a comment -

          "DERBY-2769-3-b.diff" changes the XJ079 message from BLOB/BLOB to BLOB/CLOB.

          Show
          Yun Lee added a comment - " DERBY-2769 -3-b.diff" changes the XJ079 message from BLOB/BLOB to BLOB/CLOB.
          Hide
          Kathey Marsden added a comment -

          Thanks Yun, I'll take a look.

          Show
          Kathey Marsden added a comment - Thanks Yun, I'll take a look.
          Hide
          Kathey Marsden added a comment -

          Thank you Yun for the new patch.
          message change patch DERBY-2769-3-b.diff looks fine. I will run tests and check that in.

          Clob.java

          remove import org.apache.derby.impl.jdbc.Util;
          This is unused and may bring engine code into the client which is not good.

          For the condition below there are a few issues:
          if (offset + len > str.length())

          { throw new SqlException(agent_.logWriter_, new ClientMessageId( SQLState.LANG_SUBSTR_START_OR_LEN_OUT_OF_RANGE), new Integer(offset), new Integer(len)); }

          Even though we are sharing a SQLState with SQLState.LANG_SUBSTR_START_OR_LEN_OUT_OF_RANGE we need a new message, because the existing one, "The second or third argument of the SUBSTR function is out of range." doesn't make sense in this context and doesn't take parameters. I think the way to do this is add a new constant to SQLState.java with value "22011.S.1", then add that message to messages.xml. The message should be something like: "The range specified for the substring with offset

          {0}

          and len

          {1}

          is not valid". There is another step too when adding SQLState messages to the client. They have to be added to java/build/org/apache/derbyBuild/splitmessages.java to get picked up by the client build. Otherwise it will just print UNKOWN MESSAGE. After making the changes, you may want to verify them manually or temporarily print out the message in your JUnit test to make sure it all worked. Unfortunately our JUnit tests just check SQLState so can miss issues with message text.

          ClobTest.java
          There is a comment
          //if offset + len == str.length(), it's accepted.
          I think this is incorrect and the test case below it useds str.length() -1

          EmbedClob.java
          Again we should use a new message instead of LANG_SUBSTR_START_OR_LEN_OUT_OF_RANGE

          I think the new checking causes some cases that used to execute to now fail with client. For example I found I could execute setString(1,"goodbye",2,6) where the offset + len were greater than the length of the string before the change, but not after. I think the changes are ok but we should probably have a release note.

          Show
          Kathey Marsden added a comment - Thank you Yun for the new patch. message change patch DERBY-2769 -3-b.diff looks fine. I will run tests and check that in. Clob.java remove import org.apache.derby.impl.jdbc.Util; This is unused and may bring engine code into the client which is not good. For the condition below there are a few issues: if (offset + len > str.length()) { throw new SqlException(agent_.logWriter_, new ClientMessageId( SQLState.LANG_SUBSTR_START_OR_LEN_OUT_OF_RANGE), new Integer(offset), new Integer(len)); } Even though we are sharing a SQLState with SQLState.LANG_SUBSTR_START_OR_LEN_OUT_OF_RANGE we need a new message, because the existing one, "The second or third argument of the SUBSTR function is out of range." doesn't make sense in this context and doesn't take parameters. I think the way to do this is add a new constant to SQLState.java with value "22011.S.1", then add that message to messages.xml. The message should be something like: "The range specified for the substring with offset {0} and len {1} is not valid". There is another step too when adding SQLState messages to the client. They have to be added to java/build/org/apache/derbyBuild/splitmessages.java to get picked up by the client build. Otherwise it will just print UNKOWN MESSAGE. After making the changes, you may want to verify them manually or temporarily print out the message in your JUnit test to make sure it all worked. Unfortunately our JUnit tests just check SQLState so can miss issues with message text. ClobTest.java There is a comment //if offset + len == str.length(), it's accepted. I think this is incorrect and the test case below it useds str.length() -1 EmbedClob.java Again we should use a new message instead of LANG_SUBSTR_START_OR_LEN_OUT_OF_RANGE I think the new checking causes some cases that used to execute to now fail with client. For example I found I could execute setString(1,"goodbye",2,6) where the offset + len were greater than the length of the string before the change, but not after. I think the changes are ok but we should probably have a release note.
          Hide
          Yun Lee added a comment -

          Kathey, thanks for your advice. I have added a '22011.S.1' message into SQLState, and applied it to two Clobs.

          >>ClobTest.java
          >>There is a comment
          >>//if offset + len == str.length(), it's accepted.
          >>I think this is incorrect and the test case below it useds str.length() -1

          I think the comment is correct, it just shows a boundary condition, and the code below the start of ' str.length() -1' and the len of '1' is accepted, as ( str.length() -1) + 1 = str.length(). However I have replaced it with "//if (offset + len) == str.length(), it's accepted. " to make it clear to understand.

          Please also check the release not for this issue. Thanks!

          Show
          Yun Lee added a comment - Kathey, thanks for your advice. I have added a '22011.S.1' message into SQLState, and applied it to two Clobs. >>ClobTest.java >>There is a comment >>//if offset + len == str.length(), it's accepted. >>I think this is incorrect and the test case below it useds str.length() -1 I think the comment is correct, it just shows a boundary condition, and the code below the start of ' str.length() -1' and the len of '1' is accepted, as ( str.length() -1) + 1 = str.length(). However I have replaced it with "//if (offset + len) == str.length(), it's accepted. " to make it clear to understand. Please also check the release not for this issue. Thanks!
          Hide
          Kathey Marsden added a comment -

          Thanks Yun. derby-2769-4.diff looks good. I verified the message prints out ok now and changed the message file a bit to correct an indentation issue and changed the mssage just a bit. The new message is: "The range specified for the substring with offset

          {0}

          and len

          {1}

          is out of range for the String:

          {2}

          ."

          I will run tests and check this in. Thanks for the contribution!

          On the release note, under Rationale for Change, you should refer to the JDBC specification, not the SQL specification. Under Application Changes Required you should not list the changes made to Derby, but rather the changes that an application calling Clob.setString() might need to make.

          Show
          Kathey Marsden added a comment - Thanks Yun. derby-2769-4.diff looks good. I verified the message prints out ok now and changed the message file a bit to correct an indentation issue and changed the mssage just a bit. The new message is: "The range specified for the substring with offset {0} and len {1} is out of range for the String: {2} ." I will run tests and check this in. Thanks for the contribution! On the release note, under Rationale for Change, you should refer to the JDBC specification, not the SQL specification. Under Application Changes Required you should not list the changes made to Derby, but rather the changes that an application calling Clob.setString() might need to make.
          Hide
          Kathey Marsden added a comment -

          Tests were clean but I had a conflict in messages.xml on commit. It seemed to merge cleanly when I synched up, but when I rebuilt I got:
          runmessagecheck:
          [runMessageBundleTest] WARNING: Message id XRE04.U.1 in messages_en.properties is not referenced in either SQLState.java
          or MessageId.java
          [runMessageBundleTest] WARNING: Message id XRE04.U.2 in messages_en.properties is not referenced in either SQLState.java
          or MessageId.java
          [runMessageBundleTest] WARNING: Message id XRE05 in messages_en.properties is not referenced in either SQLState.java or
          MessageId.java
          [runMessageBundleTest] WARNING: Message id XRE09 in messages_en.properties is not referenced in either SQLState.java or
          MessageId.java
          [runMessageBundleTest] WARNING: Message id XRE11 in messages_en.properties is not referenced in either SQLState.java or
          MessageId.java
          [runMessageBundleTest] WARNING: Message id XRE21 in messages_en.properties is not referenced in either SQLState.java or
          MessageId.java
          [runMessageBundleTest] WARNING: Message id XRE22 in messages_en.properties is not referenced in either SQLState.java or
          MessageId.java
          [runMessageBundleTest] WARNING: Message id XRE41 in messages_en.properties is not referenced in either SQLState.java or
          MessageId.java
          [runMessageBundleTest] WARNING: Message id XRE42 in messages_en.properties is not referenced in either SQLState.java or
          MessageId.java

          BUILD FAILED
          C:\svn4\trunk\build.xml:514: Message check failed.
          See error in build output or call ant runmessagecheck.

          I will investigate.

          Show
          Kathey Marsden added a comment - Tests were clean but I had a conflict in messages.xml on commit. It seemed to merge cleanly when I synched up, but when I rebuilt I got: runmessagecheck: [runMessageBundleTest] WARNING: Message id XRE04.U.1 in messages_en.properties is not referenced in either SQLState.java or MessageId.java [runMessageBundleTest] WARNING: Message id XRE04.U.2 in messages_en.properties is not referenced in either SQLState.java or MessageId.java [runMessageBundleTest] WARNING: Message id XRE05 in messages_en.properties is not referenced in either SQLState.java or MessageId.java [runMessageBundleTest] WARNING: Message id XRE09 in messages_en.properties is not referenced in either SQLState.java or MessageId.java [runMessageBundleTest] WARNING: Message id XRE11 in messages_en.properties is not referenced in either SQLState.java or MessageId.java [runMessageBundleTest] WARNING: Message id XRE21 in messages_en.properties is not referenced in either SQLState.java or MessageId.java [runMessageBundleTest] WARNING: Message id XRE22 in messages_en.properties is not referenced in either SQLState.java or MessageId.java [runMessageBundleTest] WARNING: Message id XRE41 in messages_en.properties is not referenced in either SQLState.java or MessageId.java [runMessageBundleTest] WARNING: Message id XRE42 in messages_en.properties is not referenced in either SQLState.java or MessageId.java BUILD FAILED C:\svn4\trunk\build.xml:514: Message check failed. See error in build output or call ant runmessagecheck. I will investigate.
          Hide
          Kathey Marsden added a comment -

          A lunch break and ant clobber seems to have cleared up this issue. I can see in my history that I clobbered before, but now it seems to work ok. I will check in after rerunning tests.

          Show
          Kathey Marsden added a comment - A lunch break and ant clobber seems to have cleared up this issue. I can see in my history that I clobbered before, but now it seems to work ok. I will check in after rerunning tests.
          Hide
          Kathey Marsden added a comment -

          Committed revision 769948. I will leave this open until we get the release note finalized. I don't think we should backport it because of the behavior changes.

          Show
          Kathey Marsden added a comment - Committed revision 769948. I will leave this open until we get the release note finalized. I don't think we should backport it because of the behavior changes.
          Hide
          Kristian Waagan added a comment -

          Set fix version to 10.6 (trunk), and removed the patch available flag.
          I understand some changes are required for the release note before we can close this issue?

          Show
          Kristian Waagan added a comment - Set fix version to 10.6 (trunk), and removed the patch available flag. I understand some changes are required for the release note before we can close this issue?
          Hide
          Rick Hillegas added a comment -

          Triaged for 10.5.2: assigned normal urgency.

          Show
          Rick Hillegas added a comment - Triaged for 10.5.2: assigned normal urgency.
          Hide
          Kristian Waagan added a comment -

          Attaching a second version of the release notes.

          Please review and suggest changes such that the issue can be closed.

          Show
          Kristian Waagan added a comment - Attaching a second version of the release notes. Please review and suggest changes such that the issue can be closed.
          Hide
          Dag H. Wanvik added a comment -

          Thanks for the new release note. One typo: "suceeded" -> "succeeded".

          Show
          Dag H. Wanvik added a comment - Thanks for the new release note. One typo: "suceeded" -> "succeeded".
          Hide
          Kristian Waagan added a comment -

          Thanks for reading the release note, Dag!

          I have corrected the spelling error and uploaded a new version.

          Show
          Kristian Waagan added a comment - Thanks for reading the release note, Dag! I have corrected the spelling error and uploaded a new version.
          Hide
          Kristian Waagan added a comment -

          I'm resolving the issue. If I don't get any more feedback on the release note I'll close it as well.

          Show
          Kristian Waagan added a comment - I'm resolving the issue. If I don't get any more feedback on the release note I'll close it as well.
          Hide
          Kristian Waagan added a comment -

          No feedback received on the release note. Closing.

          Show
          Kristian Waagan added a comment - No feedback received on the release note. Closing.

            People

            • Assignee:
              Yun Lee
              Reporter:
              Kristian Waagan
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development