Derby
  1. Derby
  2. DERBY-2443

Implement ResultSet updateClob/updateBlob methods on the NetworkClient

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.3.1.4
    • Component/s: Network Client
    • Labels:
      None
    • Environment:
      All
    • Issue & fix info:
      Release Note Needed

      Description

      Implement the following ResultSet methods on the Network Client

      updateBlob(int columnIndex, Blob x)
      updateBlob(String columnName, Blob x)

      updateClob(int columnIndex, Clob x)
      updateClob(String columnName, Clob x)

      updateBlob(int columnIndex, InputStream x, long length)
      updateBlob(String columnName, InputStream x, long length)

      updateClob(int columnIndex, Reader x, long length)
      updateClob(String columnName, Reader x, long length)

      1. ResultSetNotImplMethods_v3.stat
        0.6 kB
        V.Narayanan
      2. ResultSetNotImplMethods_v3.diff
        50 kB
        V.Narayanan
      3. ResultSetNotImplMethods_v2.stat
        0.6 kB
        V.Narayanan
      4. ResultSetNotImplMethods_v2.diff
        57 kB
        V.Narayanan
      5. ResultSetNotImplMethods_v1.stat
        0.7 kB
        V.Narayanan
      6. ResultSetNotImplMethods_v1.diff
        54 kB
        V.Narayanan
      7. releaseNote.html
        4 kB
        V.Narayanan
      8. releaseNote.html
        4 kB
        V.Narayanan
      9. releaseNote.html
        4 kB
        Rick Hillegas
      10. releaseNote.html
        4 kB
        V.Narayanan
      11. releaseNote.html
        4 kB
        Rick Hillegas
      12. releaseNote_v2.html
        4 kB
        V.Narayanan

        Issue Links

          Activity

          Hide
          Rick Hillegas added a comment -

          Reattach releaseNote. Convert uppercase paragraph tags to lowercase. Because of the casing issue, the wrong summary line was plucked out by the release note generator.

          Show
          Rick Hillegas added a comment - Reattach releaseNote. Convert uppercase paragraph tags to lowercase. Because of the casing issue, the wrong summary line was plucked out by the release note generator.
          Hide
          V.Narayanan added a comment -

          updated the release note

          Show
          V.Narayanan added a comment - updated the release note
          Hide
          V.Narayanan added a comment -

          I ran the Lint tool on the lates version and it said

          "passes the currently known checks performed by the release note generator."

          Show
          V.Narayanan added a comment - I ran the Lint tool on the lates version and it said "passes the currently known checks performed by the release note generator."
          Hide
          V.Narayanan added a comment -

          Changing J2SE 6 to Java SE 6. Sorry for the late correction.

          Show
          V.Narayanan added a comment - Changing J2SE 6 to Java SE 6. Sorry for the late correction.
          Hide
          V.Narayanan added a comment -

          Re-opening to fix mistake found in the release note.

          Show
          V.Narayanan added a comment - Re-opening to fix mistake found in the release note.
          Hide
          Rick Hillegas added a comment -

          Attaching a new release note which will hopefully fare better in the release note generator. Removed a lot of cruft which was added by an html editor. Replaced naked <br> tags with <br/>.

          Show
          Rick Hillegas added a comment - Attaching a new release note which will hopefully fare better in the release note generator. Removed a lot of cruft which was added by an html editor. Replaced naked <br> tags with <br/>.
          Hide
          Rick Hillegas added a comment -

          Re-open in order to scrub the release note.

          Show
          Rick Hillegas added a comment - Re-open in order to scrub the release note.
          Hide
          V.Narayanan added a comment -

          All patches submitted and committed and release note attached!

          Show
          V.Narayanan added a comment - All patches submitted and committed and release note attached!
          Hide
          V.Narayanan added a comment -

          attaching with the proper name of the release note!

          Show
          V.Narayanan added a comment - attaching with the proper name of the release note!
          Hide
          V.Narayanan added a comment -

          The comments that were received for the release note in DERBY-1341
          seemed very relevant to the work I have done to.

          I have modified the release note to keep with the suggested changes

          Show
          V.Narayanan added a comment - The comments that were received for the release note in DERBY-1341 seemed very relevant to the work I have done to. I have modified the release note to keep with the suggested changes
          Hide
          V.Narayanan added a comment -

          re-opening to attach release note

          Show
          V.Narayanan added a comment - re-opening to attach release note
          Hide
          V.Narayanan added a comment -

          I have raised DERBY-2826 to address the
          test re-writing follow-up required. The patch
          submitted to this issue has been submitted
          and committed.

          Show
          V.Narayanan added a comment - I have raised DERBY-2826 to address the test re-writing follow-up required. The patch submitted to this issue has been submitted and committed.
          Hide
          V.Narayanan added a comment -

          I will do this. Thank You for the comments and commit Knut. Attempting
          to send this comment as a email to jira@apache.org .

          Show
          V.Narayanan added a comment - I will do this. Thank You for the comments and commit Knut. Attempting to send this comment as a email to jira@apache.org .
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for the new patch, Narayanan! I have committed it with revision 522873.

          One more minor comment: jdbcapi/UpdatableResultSetTest.java has two methods, fetch() and fetchUpd(), which create statements without closing them. I don't know if it's a problem here, but other JUnit tests have occasionally failed because statements or result sets were not closed and some resources were held until they were garbage collected. Perhaps it would be good to rewrite them so that the statements could be closed?

          Show
          Knut Anders Hatlen added a comment - Thanks for the new patch, Narayanan! I have committed it with revision 522873. One more minor comment: jdbcapi/UpdatableResultSetTest.java has two methods, fetch() and fetchUpd(), which create statements without closing them. I don't know if it's a problem here, but other JUnit tests have occasionally failed because statements or result sets were not closed and some resources were held until they were garbage collected. Perhaps it would be good to rewrite them so that the statements could be closed?
          Hide
          V.Narayanan added a comment -

          for v3 I ran only the tests modified as part of this patch. They passed.

          I did not run derbyall or junit since going from v2 to v3 I have made only javadoc changes to the main code line and I had already run derbyall and junit tests for v2 and they passed.

          Show
          V.Narayanan added a comment - for v3 I ran only the tests modified as part of this patch. They passed. I did not run derbyall or junit since going from v2 to v3 I have made only javadoc changes to the main code line and I had already run derbyall and junit tests for v2 and they passed.
          Hide
          V.Narayanan added a comment -

          Thank You for the comments. The attached patch resolve the issues pointed.

          Show
          V.Narayanan added a comment - Thank You for the comments. The attached patch resolve the issues pointed.
          Hide
          Knut Anders Hatlen added a comment -

          The patch looks good to me. A couple of small comments:

          I get these warnings when generating javadoc:

          client/am/ResultSet.java:6100: warning - @param argument "columnLabel" is not a parameter name.
          client/am/ResultSet.java:6125: warning - @param argument "columnLabel" is not a parameter name.

          The tests have much code similar to this:
          + Reader r1 = new java.io.StringReader(new String(BYTES1));
          I'm not sure how safe it is to use the String(byte[]) constructor without specifying the encoding. Perhaps it would be better to use a hard-coded string?

          I assume the changes to index.html were not supposed to be part of the patch?

          Show
          Knut Anders Hatlen added a comment - The patch looks good to me. A couple of small comments: I get these warnings when generating javadoc: client/am/ResultSet.java:6100: warning - @param argument "columnLabel" is not a parameter name. client/am/ResultSet.java:6125: warning - @param argument "columnLabel" is not a parameter name. The tests have much code similar to this: + Reader r1 = new java.io.StringReader(new String(BYTES1)); I'm not sure how safe it is to use the String(byte[]) constructor without specifying the encoding. Perhaps it would be better to use a hard-coded string? I assume the changes to index.html were not supposed to be part of the patch?
          Hide
          V.Narayanan added a comment -

          Attaching the latest patch. I have removed the changes pertaining to 2430 that was in the previously attached patch. I have also removed formatting mistakes I had made.

          I ran derbyall and Junit suites and fixed all the failures I encountered. All the tests are passing with this patch now.

          Show
          V.Narayanan added a comment - Attaching the latest patch. I have removed the changes pertaining to 2430 that was in the previously attached patch. I have also removed formatting mistakes I had made. I ran derbyall and Junit suites and fixed all the failures I encountered. All the tests are passing with this patch now.
          Hide
          V.Narayanan added a comment -

          I had placed this patch primarily to show how the update methods would be using the changes to setObject calls(2430). I generally look for formatting problems before submitting my patches. I however did not do a very detailed review of formatting related problems before doing this since I wanted it to show how the update methods would use the changes made as part of 2430.

          Pls pardon any formatting mistakes I would have committed here.

          Also this patch is not for a commit. I will be submitting a follow up patch.

          Show
          V.Narayanan added a comment - I had placed this patch primarily to show how the update methods would be using the changes to setObject calls(2430). I generally look for formatting problems before submitting my patches. I however did not do a very detailed review of formatting related problems before doing this since I wanted it to show how the update methods would use the changes made as part of 2430. Pls pardon any formatting mistakes I would have committed here. Also this patch is not for a commit. I will be submitting a follow up patch.
          Hide
          V.Narayanan added a comment -

          Thank you for looking at the patch!!

          >Can't you just remove the if-else block and do the assert, which seems to be common for all cases?

          The existing code does it that way and I did not disturb it. I just changed the SQLState. However I agree to this and shall make this change in my follow-up patch.

          >There are also some long lines in client/am/ResultSet.

          I shall fix these formatting issues in a follow-up patch.

          Show
          V.Narayanan added a comment - Thank you for looking at the patch!! >Can't you just remove the if-else block and do the assert, which seems to be common for all cases? The existing code does it that way and I did not disturb it. I just changed the SQLState. However I agree to this and shall make this change in my follow-up patch. >There are also some long lines in client/am/ResultSet. I shall fix these formatting issues in a follow-up patch.
          Hide
          Kristian Waagan added a comment -

          In ResultSetJDBC30Test:
          @@ -118,7 +118,7 @@
          if (usingEmbedded())

          { assertSQLState(UPDATABLE_RESULTSET_API_DISALLOWED, se); }

          else

          { - assertSQLState(NOT_IMPLEMENTED, se); + assertSQLState(UPDATABLE_RESULTSET_API_DISALLOWED, se); }

          Can't you just remove the if-else block and do the assert, which seems to be common for all cases?

          There are also some long lines in client/am/ResultSet.

          I haven't reviewed this patch thoroughly, so another pair of eyes would be good.

          Show
          Kristian Waagan added a comment - In ResultSetJDBC30Test: @@ -118,7 +118,7 @@ if (usingEmbedded()) { assertSQLState(UPDATABLE_RESULTSET_API_DISALLOWED, se); } else { - assertSQLState(NOT_IMPLEMENTED, se); + assertSQLState(UPDATABLE_RESULTSET_API_DISALLOWED, se); } Can't you just remove the if-else block and do the assert, which seems to be common for all cases? There are also some long lines in client/am/ResultSet. I haven't reviewed this patch thoroughly, so another pair of eyes would be good.
          Hide
          V.Narayanan added a comment -

          THIS PATCH IS NOT FOR COMMIT

          EXPLN OF CHANGES
          ----------------
          This patch is blocked by DERBY-2430 and contains the diff's pertainining to that issue as the following files

          M java/testing/org/apache/derbyTesting/functionTests/tests/lang/UpdatableResultSetTest.java
          M java/client/org/apache/derby/client/am/CrossConverters.java

          Tests for the updateClob/UpdateBlob that take a Clob/Blob as a parameter for the embedded case had been present in jdbc4/ResultSetTest.java. They are however present from earlier versions than JDBC 4.0. I have hence migrated tests for these to jdbcapi/UpdatableResultSetTest.java.

          I have however not deleted the tests from jdbc4/ResultSetTest. I simply enabled them for running with the Network Client also.

          The tests for the methods accepting InputStream/Reader can be found in jdbc4/ResultSetTest.

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ResultSetTest.java
          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/UpdatableResultSetTest.java

          Post 2430 updateBlob and updateClob would fail with all types except BLOB and CLOB respectively. These methods have basically been forwarded to setObject. Moving these methods into ResultSet from NetResultSet40.

          M java/client/org/apache/derby/client/net/NetResultSet40.java
          M java/client/org/apache/derby/client/am/ResultSet.java

          Show
          V.Narayanan added a comment - THIS PATCH IS NOT FOR COMMIT EXPLN OF CHANGES ---------------- This patch is blocked by DERBY-2430 and contains the diff's pertainining to that issue as the following files M java/testing/org/apache/derbyTesting/functionTests/tests/lang/UpdatableResultSetTest.java M java/client/org/apache/derby/client/am/CrossConverters.java Tests for the updateClob/UpdateBlob that take a Clob/Blob as a parameter for the embedded case had been present in jdbc4/ResultSetTest.java. They are however present from earlier versions than JDBC 4.0. I have hence migrated tests for these to jdbcapi/UpdatableResultSetTest.java. I have however not deleted the tests from jdbc4/ResultSetTest. I simply enabled them for running with the Network Client also. The tests for the methods accepting InputStream/Reader can be found in jdbc4/ResultSetTest. M java/testing/org/apache/derbyTesting/functionTests/tests/jdbc4/ResultSetTest.java M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/UpdatableResultSetTest.java Post 2430 updateBlob and updateClob would fail with all types except BLOB and CLOB respectively. These methods have basically been forwarded to setObject. Moving these methods into ResultSet from NetResultSet40. M java/client/org/apache/derby/client/net/NetResultSet40.java M java/client/org/apache/derby/client/am/ResultSet.java

            People

            • Assignee:
              V.Narayanan
              Reporter:
              V.Narayanan
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development