Derby
  1. Derby
  2. DERBY-3991

Clob.truncate(0) throws exception

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.3.3.1, 10.4.2.1, 10.6.1.0
    • Fix Version/s: 10.4.2.1, 10.5.2.0, 10.6.1.0
    • Component/s: JDBC
    • Labels:
      None
    • Issue & fix info:
      Newcomer, Release Note Needed
    • Bug behavior facts:
      Deviation from standard

      Description

      Truncating a Clob to zero length is allowed according to the JDBC specification, which says the following about the len argument (in the @throws tag):
      "SQLException - if there is an error accessing the CLOB value or if len is less than 0 "

      Derby throws an exception if zero is passed to truncate.

      A quick inspection of the code suggests that truncating a Clob to the empty string is easy to support.

      1. releaseNote.html
        5 kB
        Kristian Waagan
      2. releaseNote.html
        4 kB
        Yun Lee
      3. releaseNote.html
        4 kB
        Myrna van Lunteren
      4. derby-3991-3.stat
        0.1 kB
        Yun Lee
      5. derby-3991-3.releaseNote.html
        4 kB
        Yun Lee
      6. derby-3991-3.diff
        3 kB
        Yun Lee
      7. derby-3991-2a.diff
        3 kB
        Yun Lee
      8. derby-3991-1a-ClobTruncateZeroTest.diff
        2 kB
        Kristian Waagan

        Activity

        Hide
        Kristian Waagan added a comment -

        'derby-3991-1a-ClobTruncateZeroTest.diff' adds two tests demonstrating the issue. When run, you should get 4 failures.

        Show
        Kristian Waagan added a comment - 'derby-3991-1a-ClobTruncateZeroTest.diff' adds two tests demonstrating the issue. When run, you should get 4 failures.
        Hide
        Yun Lee added a comment -

        Hi, all. I'm a prospective student for GSOC, Yun Lee. I have posted a patch for this issue, wish for your comments.

        At this moment, I'm in the user group, without the permission to assign this issue to myself. How to apply for the developer group, please?

        Thanks!

        Show
        Yun Lee added a comment - Hi, all. I'm a prospective student for GSOC, Yun Lee. I have posted a patch for this issue, wish for your comments. At this moment, I'm in the user group, without the permission to assign this issue to myself. How to apply for the developer group, please? Thanks!
        Hide
        Yun Lee added a comment -

        The patch derby-3991-2a.diff adopts the test cases in derby-3991-1a-ClobTruncateZeroTest.diff provided by Kristian, and contains a change on java/engine/org/apache/derby/impl/jdbc/EmbedClob.java. With the patch, Test cases can run without any problem. Please check it. Thanks!

        Show
        Yun Lee added a comment - The patch derby-3991-2a.diff adopts the test cases in derby-3991-1a-ClobTruncateZeroTest.diff provided by Kristian, and contains a change on java/engine/org/apache/derby/impl/jdbc/EmbedClob.java. With the patch, Test cases can run without any problem. Please check it. Thanks!
        Hide
        Kristian Waagan added a comment -

        The patch looks good to me.

        You could consider trying to access the empty string using a stream too (currently the test I wrote originally only uses getSubString).
        Users will also see a different error message with the embedded driver now. Does this warrant a release note?
        The new error message is both more specific and it is the same as the one thrown by the client driver, which is justification enough for the change.

        I'll kick off a test run.

        Show
        Kristian Waagan added a comment - The patch looks good to me. You could consider trying to access the empty string using a stream too (currently the test I wrote originally only uses getSubString). Users will also see a different error message with the embedded driver now. Does this warrant a release note? The new error message is both more specific and it is the same as the one thrown by the client driver, which is justification enough for the change. I'll kick off a test run.
        Hide
        Kristian Waagan added a comment -

        derbyall and suites.All ran without failures in my sandbox (Solaris 10, Sun JDK 1.6).

        Show
        Kristian Waagan added a comment - derbyall and suites.All ran without failures in my sandbox (Solaris 10, Sun JDK 1.6).
        Hide
        Yun Lee added a comment -

        Kistian, thanks for your comments.

        >You could consider trying to access the empty string using a stream too (currently the test I wrote originally only uses getSubString).
        I will do it later.

        >Users will also see a different error message with the embedded driver now. Does this warrant a release note?
        >The new error message is both more specific and it is the same as the one thrown by the client driver, which is justification enough for the change.
        I'm not sure. Do you mean I replaced 'SQLState.BLOB_BAD_POSITION' with 'SQLState.BLOB_NONPOSITIVE_LENGTH' in EmbedClob.java? If you do, I just want to give a clearer exception message.As a new comer, I don't know whether it's proper to do so, and don't know what it does with ' a release note'. Wish for your explanation.

        Thanks!

        Show
        Yun Lee added a comment - Kistian, thanks for your comments. >You could consider trying to access the empty string using a stream too (currently the test I wrote originally only uses getSubString). I will do it later. >Users will also see a different error message with the embedded driver now. Does this warrant a release note? >The new error message is both more specific and it is the same as the one thrown by the client driver, which is justification enough for the change. I'm not sure. Do you mean I replaced 'SQLState.BLOB_BAD_POSITION' with 'SQLState.BLOB_NONPOSITIVE_LENGTH' in EmbedClob.java? If you do, I just want to give a clearer exception message.As a new comer, I don't know whether it's proper to do so, and don't know what it does with ' a release note'. Wish for your explanation. Thanks!
        Hide
        Kristian Waagan added a comment -

        Regarding the error message, I'm just wondering if the users should be notified about the change through a release note [1]. We have a release note template under "trunk/tools/release/templates/releaseNote.html", if you want to take a look.
        I agree the change is a good one, but I don't know if the change requires that the community notifies the users about the changed error message.
        Maybe someone else in the community has a stronger opinion about this?

        [1] FYI, here are the release notes for 10.4: http://db.apache.org/derby/releases/release-10.4.2.0.cgi#Release%20Notes%20for%20Derby%2010.4.2.0
        Note especially under "Issues" and the "Note for DERBY-X" paragraphs.

        Show
        Kristian Waagan added a comment - Regarding the error message, I'm just wondering if the users should be notified about the change through a release note [1] . We have a release note template under "trunk/tools/release/templates/releaseNote.html", if you want to take a look. I agree the change is a good one, but I don't know if the change requires that the community notifies the users about the changed error message. Maybe someone else in the community has a stronger opinion about this? [1] FYI, here are the release notes for 10.4: http://db.apache.org/derby/releases/release-10.4.2.0.cgi#Release%20Notes%20for%20Derby%2010.4.2.0 Note especially under "Issues" and the "Note for DERBY-X" paragraphs.
        Hide
        Dag H. Wanvik added a comment -

        I think if the error message is documented for the usage case (often they are not..), then it is a API change and we should have
        a release note. If the actual error message is not documented in the user docs, it is less clear to me that a release note is needed..
        It may still be prudent do add one, though. I would not object to it.

        Show
        Dag H. Wanvik added a comment - I think if the error message is documented for the usage case (often they are not..), then it is a API change and we should have a release note. If the actual error message is not documented in the user docs, it is less clear to me that a release note is needed.. It may still be prudent do add one, though. I would not object to it.
        Hide
        Yun Lee added a comment -

        Thanks for your detailed advice, Kristian and Dag.

        I have added Stream test for empty String.

        Besides, I provide a release note for the change on truncate(). It addresses two changes:
        1.Accept zero length.
        2.Give a more specific error message for nonpositive length.

        I think accepting zero length is more important than giving a more specific error message. As the type of this issue is 'Bug'.

        Wish for your comments!

        Yun

        Show
        Yun Lee added a comment - Thanks for your detailed advice, Kristian and Dag. I have added Stream test for empty String. Besides, I provide a release note for the change on truncate(). It addresses two changes: 1.Accept zero length. 2.Give a more specific error message for nonpositive length. I think accepting zero length is more important than giving a more specific error message. As the type of this issue is 'Bug'. Wish for your comments! Yun
        Hide
        Kristian Waagan added a comment -

        Thank you, Yun.

        I have committed patch 3 to trunk with revision 764800.
        I think the release note needs some more work. For instance, do we want to describe two issues in one release note (zero length allowed, and the change of error message), or would it be better with a release note for each issue?

        Show
        Kristian Waagan added a comment - Thank you, Yun. I have committed patch 3 to trunk with revision 764800. I think the release note needs some more work. For instance, do we want to describe two issues in one release note (zero length allowed, and the change of error message), or would it be better with a release note for each issue?
        Hide
        Kristian Waagan added a comment -

        As a start, I tried to rewrite the release note, but I'm not sure if it is acceptable. Please review.

        The file is now attached as 'releaseNote.html' to allow the release note generator to pick it up.

        Show
        Kristian Waagan added a comment - As a start, I tried to rewrite the release note, but I'm not sure if it is acceptable. Please review. The file is now attached as 'releaseNote.html' to allow the release note generator to pick it up.
        Hide
        Yun Lee added a comment -

        I'm sorry to make some mistake in operating on the 'edit' option of the issue, now just restore it to the old state.

        Show
        Yun Lee added a comment - I'm sorry to make some mistake in operating on the 'edit' option of the issue, now just restore it to the old state.
        Hide
        Yun Lee added a comment -

        >I think the release note needs some more work. For instance, do we want to describe two issues in one release note (zero length allowed, and the change of error message), or would it be better with a release note for each issue?

        Kristian, thanks for your committing, it's a good message. And I would love to provide two release notes for the two change points.

        However, I have some questions about the name of release notes. You have said 'The file is now attached as 'releaseNote.html' to allow the release note generator to pick it up', do you mean a release note must have the identical name of 'releaseNote.html'? If so, should I post the two release notes in two times with the same name of 'releaseNote.html'?

        Thanks.

        Show
        Yun Lee added a comment - >I think the release note needs some more work. For instance, do we want to describe two issues in one release note (zero length allowed, and the change of error message), or would it be better with a release note for each issue? Kristian, thanks for your committing, it's a good message. And I would love to provide two release notes for the two change points. However, I have some questions about the name of release notes. You have said 'The file is now attached as 'releaseNote.html' to allow the release note generator to pick it up', do you mean a release note must have the identical name of 'releaseNote.html'? If so, should I post the two release notes in two times with the same name of 'releaseNote.html'? Thanks.
        Hide
        Knut Anders Hatlen added a comment -

        Do we actually need to mention that truncate(0) no longer throws an exception? We don't normally write release notes for that kind of changes ("something that didn't work now works"). Mentioning the changed SQLState is sufficient, in my opinion.

        Show
        Knut Anders Hatlen added a comment - Do we actually need to mention that truncate(0) no longer throws an exception? We don't normally write release notes for that kind of changes ("something that didn't work now works"). Mentioning the changed SQLState is sufficient, in my opinion.
        Hide
        Kristian Waagan added a comment -

        Yun,
        > However, I have some questions about the name of release notes. You have said 'The file is now attached as 'releaseNote.html' to allow the release note generator to pick it up', do you mean a release note must have the identical name of 'releaseNote.html'? If so, should I post the two release notes in two times with the same name of 'releaseNote.html'?

        I don't think we can attach two release notes to the same issue and have the release notes generator pick them both up. If we end up with such a scenario, I think we have to split the issue into two (or create a sub-issue).

        The only feedback from the community so far has been to write a release note for the changed SQLState only. If you agree, you can start rewriting the release note again.
        You can attach the next version of the release note as 'releaseNote.html' as well. The release notes generator will pick up the newest version.

        Thanks,

        Show
        Kristian Waagan added a comment - Yun, > However, I have some questions about the name of release notes. You have said 'The file is now attached as 'releaseNote.html' to allow the release note generator to pick it up', do you mean a release note must have the identical name of 'releaseNote.html'? If so, should I post the two release notes in two times with the same name of 'releaseNote.html'? I don't think we can attach two release notes to the same issue and have the release notes generator pick them both up. If we end up with such a scenario, I think we have to split the issue into two (or create a sub-issue). The only feedback from the community so far has been to write a release note for the changed SQLState only. If you agree, you can start rewriting the release note again. You can attach the next version of the release note as 'releaseNote.html' as well. The release notes generator will pick up the newest version. Thanks,
        Hide
        Yun Lee added a comment -

        OK, Kristian. I've got it, and will post a new releasenote on SQLState later.

        Good luck
        Yun

        Show
        Yun Lee added a comment - OK, Kristian. I've got it, and will post a new releasenote on SQLState later. Good luck Yun
        Hide
        Yun Lee added a comment -

        Kristian, after receiving your and Knut's advice, I have provided a new releaseNote.html to focus on the change on SQLException message, it's clearer indeed. Please check it! Thanks!

        Yun

        Show
        Yun Lee added a comment - Kristian, after receiving your and Knut's advice, I have provided a new releaseNote.html to focus on the change on SQLException message, it's clearer indeed. Please check it! Thanks! Yun
        Hide
        Kristian Waagan added a comment -

        The release note looks good to me.
        I'll start the work on backporting the patch to older branches.

        Show
        Kristian Waagan added a comment - The release note looks good to me. I'll start the work on backporting the patch to older branches.
        Hide
        Kristian Waagan added a comment -

        Backported patch 3 to 10.5 with revision 768939 and to 10.4 with revision 768940.

        Show
        Kristian Waagan added a comment - Backported patch 3 to 10.5 with revision 768939 and to 10.4 with revision 768940.
        Hide
        Kathey Marsden added a comment -

        Looks like this has been resolved.

        Show
        Kathey Marsden added a comment - Looks like this has been resolved.
        Hide
        Kristian Waagan added a comment -

        Closing this issue.
        Thanks for doing the work, Yun!

        Show
        Kristian Waagan added a comment - Closing this issue. Thanks for doing the work, Yun!
        Hide
        Myrna van Lunteren added a comment -

        The releaseNote.html failed the checks performed by the ReleaseNoteReader (java org.apache.derbyBuild.ReleaseNoteReader) because of a missing </p> tag, so I fixed that up.

        Show
        Myrna van Lunteren added a comment - The releaseNote.html failed the checks performed by the ReleaseNoteReader (java org.apache.derbyBuild.ReleaseNoteReader) because of a missing </p> tag, so I fixed that up.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development