Derby
  1. Derby
  2. DERBY-100

Add support for insert functionality using JDBC 2.0 updatable resultset apis

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.1.1.0
    • Fix Version/s: 10.2.1.6
    • Component/s: JDBC
    • Labels:
      None
    • Urgency:
      Normal

      Description

      The JDBC 2.0 API introduced the ability to update/delete/insert rows from a resultset using methods in the Java programming language rather than having to send an SQL command. This Jira entry is to track the insert rows functionality using JDBC 2.0 apis.

      1. DERBY-100.diff
        49 kB
        Fernanda Pizzorno
      2. DERBY-100.stat
        0.8 kB
        Fernanda Pizzorno
      3. DERBY-100-2.diff
        461 kB
        Fernanda Pizzorno
      4. DERBY-100-2.stat
        0.8 kB
        Fernanda Pizzorno
      5. DERBY-100-3.diff
        451 kB
        Fernanda Pizzorno
      6. DERBY-100-3.stat
        0.8 kB
        Fernanda Pizzorno
      7. DERBY-100-4.diff
        0.7 kB
        Fernanda Pizzorno
      8. DERBY-100-4.stat
        0.1 kB
        Fernanda Pizzorno

        Issue Links

          Activity

          Hide
          Fernanda Pizzorno added a comment -

          The patch attached is an implementation of insertRow() for both embedded and client/server drivers. Can someone please look into what I have done so far and let me know if there is something I have forgotten, something that should be done in a different way and/or more tests I could write?

          I am going to be on holidays for the next 3 weeks so it might be that I am slow to reply to comments during this period.

          Show
          Fernanda Pizzorno added a comment - The patch attached is an implementation of insertRow() for both embedded and client/server drivers. Can someone please look into what I have done so far and let me know if there is something I have forgotten, something that should be done in a different way and/or more tests I could write? I am going to be on holidays for the next 3 weeks so it might be that I am slow to reply to comments during this period.
          Hide
          Daniel John Debrunner added a comment -

          Looks good generally, some minor comments:

          In EmbedResultSet.moveToInsertRow you always generate a new blank row, such a holder could be re-used across multiple insertRow operations from the same ResultSet

          The SQL state you added is CANNOT_INSERT_AT_CURRENT_POSITION, which in my thinking is a little misleading.
          The text of the message describes the error better, so I wonder if the constant should be:
          NOT_POSITIONED_ON_INSERT_ROW

          There seems to be potential for code sharing in EmbedResultSet with the updateRow and deleteRow case, maybe once this patch is in that could be a further improvement.

          Typically for tests when an expected exception is caught, its message and sql state are displayed to ensure the correct message is being returned.
          And in all cases when you catch and display a SQL exception you should display the SQLState and the message, and walk down the SQLException chain using getNextException reporting all of them.

          Show
          Daniel John Debrunner added a comment - Looks good generally, some minor comments: In EmbedResultSet.moveToInsertRow you always generate a new blank row, such a holder could be re-used across multiple insertRow operations from the same ResultSet The SQL state you added is CANNOT_INSERT_AT_CURRENT_POSITION, which in my thinking is a little misleading. The text of the message describes the error better, so I wonder if the constant should be: NOT_POSITIONED_ON_INSERT_ROW There seems to be potential for code sharing in EmbedResultSet with the updateRow and deleteRow case, maybe once this patch is in that could be a further improvement. Typically for tests when an expected exception is caught, its message and sql state are displayed to ensure the correct message is being returned. And in all cases when you catch and display a SQL exception you should display the SQLState and the message, and walk down the SQLException chain using getNextException reporting all of them.
          Hide
          Daniel John Debrunner added a comment -

          In Positive test 47 you check to see what happens if you call insertRow() without setting any of the column values.
          You code will just do nothing in this case, no exceptions, insertRow just returns having done nothing due to:

          + if (currentRowHasBeenUpdated == false) // not columns got updated
          + return; //nothing to do since no updateXXX method has been called
          +

          But that got me thinking, if a table's columns in the result set then why can't I do an insert without setting any values?
          E.g. on a result set of SELECT A,B FROM T, execute INSERT INTO T VALUES DEFAULT

          Then I looked at the javadoc for insertRow (JDK142) and it says:

          Throws:
          SQLException - if a database access error occurs, if this method is called when the cursor is not on the insert row, or if not all of non-nullable columns in the insert row have been given a value

          You code does not throw that exception, but it's a strange wording. It implies to me that with the above SELECT if I set only A then the correct SQL is

          INSERT INTO T(A,B) VALUES (?, NULL) [I don't this is correct, but it's what that wording implies]

          rather than what you are executing,

          INSERT INTO T(A) VALUES
          which is the same as
          INSERT INTO T(A,B) VALUES (?, DEFAULT)

          The JDBC 3.0 spec is somewhat clearer, from section: 14.2.4.3 Inserting a Row

          Each column in the insert row that does not allow null as a value and does not
          have a default value must be given a value using the approriate update method. If
          this is not the case, the method insertRow will throw an SQLException.

          I'm still unclear as to why the reference to not allowing null, that does not seem relevant.

          So, since the engine will throw the SQLException if a value does not have a default, I believe that an insertRow
          with no modified columns should be allowed and should insert a row.

          Show
          Daniel John Debrunner added a comment - In Positive test 47 you check to see what happens if you call insertRow() without setting any of the column values. You code will just do nothing in this case, no exceptions, insertRow just returns having done nothing due to: + if (currentRowHasBeenUpdated == false) // not columns got updated + return; //nothing to do since no updateXXX method has been called + But that got me thinking, if a table's columns in the result set then why can't I do an insert without setting any values? E.g. on a result set of SELECT A,B FROM T, execute INSERT INTO T VALUES DEFAULT Then I looked at the javadoc for insertRow (JDK142) and it says: Throws: SQLException - if a database access error occurs, if this method is called when the cursor is not on the insert row, or if not all of non-nullable columns in the insert row have been given a value You code does not throw that exception, but it's a strange wording. It implies to me that with the above SELECT if I set only A then the correct SQL is INSERT INTO T(A,B) VALUES (?, NULL) [I don't this is correct, but it's what that wording implies] rather than what you are executing, INSERT INTO T(A) VALUES which is the same as INSERT INTO T(A,B) VALUES (?, DEFAULT) The JDBC 3.0 spec is somewhat clearer, from section: 14.2.4.3 Inserting a Row Each column in the insert row that does not allow null as a value and does not have a default value must be given a value using the approriate update method. If this is not the case, the method insertRow will throw an SQLException. I'm still unclear as to why the reference to not allowing null, that does not seem relevant. So, since the engine will throw the SQLException if a value does not have a default, I believe that an insertRow with no modified columns should be allowed and should insert a row.
          Hide
          Daniel John Debrunner added a comment -

          OK - the coffee has kicked in and I understand the allow null comment in the spec. I'd forgotten that nullable meant a default of NULL, so compiling a truth table made it clear.

          Each column in the insert row that does not allow null as a value and does not
          have a default value must be given a value using the appropriate update method.

          nullable has default

          no no needs user supplied value (enforced by engine, exception)
          no yes no user supplied value needed (handled by engine, set to default)
          yes no no user supplied value needed (handled by engine, set to null)
          yes yes no user supplied value needed (handled by engine, set to default)

          So I believe that this code does not implement the spec correctly, in the case of no user supllied values.

          Show
          Daniel John Debrunner added a comment - OK - the coffee has kicked in and I understand the allow null comment in the spec. I'd forgotten that nullable meant a default of NULL, so compiling a truth table made it clear. Each column in the insert row that does not allow null as a value and does not have a default value must be given a value using the appropriate update method. nullable has default no no needs user supplied value (enforced by engine, exception) no yes no user supplied value needed (handled by engine, set to default) yes no no user supplied value needed (handled by engine, set to null) yes yes no user supplied value needed (handled by engine, set to default) So I believe that this code does not implement the spec correctly, in the case of no user supllied values.
          Hide
          Fernanda Pizzorno added a comment -

          Thank you for looking into my previous patch.

          In this new patch I have:

          • changed moveToInsertRow in java/engine/org/apache/derby/impl/jdbc/EmbedResultSet.java so that it will not create a new blank row each time
          • changed the sqlState I have added from CANNOT_INSERT_AT_CURRENT_POSITION to NOT_POSITIONED_ON_INSERT_ROW
          • changed the tests so that they will print sqlState and message for all exceptions and walk down the SQLException chain
          • added some more tests
          • changed the behavior of insertRow when no updateXXX has been called before insertRow is called. It now inserts a row with the row's default values.

          Can someone please review this patch?

          I have not updated the canon for updatableResultSet.java under j9_13 and DerbyNet.

          I have successfully run derbyall.

          Show
          Fernanda Pizzorno added a comment - Thank you for looking into my previous patch. In this new patch I have: changed moveToInsertRow in java/engine/org/apache/derby/impl/jdbc/EmbedResultSet.java so that it will not create a new blank row each time changed the sqlState I have added from CANNOT_INSERT_AT_CURRENT_POSITION to NOT_POSITIONED_ON_INSERT_ROW changed the tests so that they will print sqlState and message for all exceptions and walk down the SQLException chain added some more tests changed the behavior of insertRow when no updateXXX has been called before insertRow is called. It now inserts a row with the row's default values. Can someone please review this patch? I have not updated the canon for updatableResultSet.java under j9_13 and DerbyNet. I have successfully run derbyall.
          Hide
          Bryan Pendleton added a comment -

          In java/client/org/apache/derby/client/am/ResultSet.java, I did not understand why you deleted the exit tracing at the end of the getBoolean() method (near line 541), and I also did not understand why you deleted the entry tracing in the rowInserted() method (near line 2643). Are these trace messages no longer needed/wanted?

          Show
          Bryan Pendleton added a comment - In java/client/org/apache/derby/client/am/ResultSet.java, I did not understand why you deleted the exit tracing at the end of the getBoolean() method (near line 541), and I also did not understand why you deleted the entry tracing in the rowInserted() method (near line 2643). Are these trace messages no longer needed/wanted?
          Hide
          Fernanda Pizzorno added a comment -

          Thank you for looking into the patch Bryan Pendleton.

          The change to the getBoolean() method that you mentioned (deleting the exit trancing), that was an accident that happened while I was manually merging changes. I am sorry I did not notice it before. I am attaching a patch to fix it.

          For rowInserted(), my intention was to make the implementation of this method as similar as possible to that of the rowDeteled() and rowUpdated() methods. Since these two methods do not have entry trancing, I removed it from the rowInserted() method.

          I have only run derbylang with this new patch since the changes where so small. Please let me know if you think it is necessary that I run derbyall and I will do it.

          Show
          Fernanda Pizzorno added a comment - Thank you for looking into the patch Bryan Pendleton. The change to the getBoolean() method that you mentioned (deleting the exit trancing), that was an accident that happened while I was manually merging changes. I am sorry I did not notice it before. I am attaching a patch to fix it. For rowInserted(), my intention was to make the implementation of this method as similar as possible to that of the rowDeteled() and rowUpdated() methods. Since these two methods do not have entry trancing, I removed it from the rowInserted() method. I have only run derbylang with this new patch since the changes where so small. Please let me know if you think it is necessary that I run derbyall and I will do it.
          Hide
          Daniel John Debrunner added a comment -

          Just to confirm - to make this change I need to apply two patches?

          DERBY-100-3.diff
          DERBY-100-4.diff

          I'll commit this once this is confirmed.

          Show
          Daniel John Debrunner added a comment - Just to confirm - to make this change I need to apply two patches? DERBY-100 -3.diff DERBY-100 -4.diff I'll commit this once this is confirmed.
          Hide
          Daniel John Debrunner added a comment -

          never mind Bernt already did it.

          Can the bug be closed now or is there more work ot be done?

          Show
          Daniel John Debrunner added a comment - never mind Bernt already did it. Can the bug be closed now or is there more work ot be done?
          Hide
          Fernanda Pizzorno added a comment -

          Do you mean if the jira issue should be closed? I seems to me that this issue, DERBY-99 and DERBY-98 are general for all types of result sets. Today we only have deleteRow, insertRow and updateRow for forward-only updatable result sets. DERBY-99 and DERBY-98 have been left open so that they can be implemented for other result set types. In my opinion, we should either close all three issues (98, 99 and 100), and create new ones, if necessary, when we implement other result set types; or we should leave this issue opened as it has been done with the other two.

          Show
          Fernanda Pizzorno added a comment - Do you mean if the jira issue should be closed? I seems to me that this issue, DERBY-99 and DERBY-98 are general for all types of result sets. Today we only have deleteRow, insertRow and updateRow for forward-only updatable result sets. DERBY-99 and DERBY-98 have been left open so that they can be implemented for other result set types. In my opinion, we should either close all three issues (98, 99 and 100), and create new ones, if necessary, when we implement other result set types; or we should leave this issue opened as it has been done with the other two.
          Hide
          Daniel John Debrunner added a comment -

          Patch has been applied

          Show
          Daniel John Debrunner added a comment - Patch has been applied
          Hide
          Andreas Korneliussen added a comment -

          Release note suggestion:
          Updatable resultsets now supports inserting rows using ResultSet.insertRow()

          Show
          Andreas Korneliussen added a comment - Release note suggestion: Updatable resultsets now supports inserting rows using ResultSet.insertRow()
          Hide
          Andreas Korneliussen added a comment -

          I think this issue can be closed, along with DERBY-99 and DERBY-98, since work has been done to support these features on all types of resultsets which Derby supports. Derby does not support resultsets of type TYPE_SCROLL_SENSITIVE at all, however that could be filed in another JIRA issue. I will leave it up to the reporter to close the issues.

          Show
          Andreas Korneliussen added a comment - I think this issue can be closed, along with DERBY-99 and DERBY-98 , since work has been done to support these features on all types of resultsets which Derby supports. Derby does not support resultsets of type TYPE_SCROLL_SENSITIVE at all, however that could be filed in another JIRA issue. I will leave it up to the reporter to close the issues.
          Hide
          Kathey Marsden added a comment -

          Removing patch available. As best I can tell all patches for this issue have been committed. Please correct if I am wrong.

          Show
          Kathey Marsden added a comment - Removing patch available. As best I can tell all patches for this issue have been committed. Please correct if I am wrong.
          Hide
          Mamta A. Satoor added a comment -

          Closing this issue since insert functionality using JDBC 2.0 has been implemented for all the types of resultsets supported by Derby at this point.

          Show
          Mamta A. Satoor added a comment - Closing this issue since insert functionality using JDBC 2.0 has been implemented for all the types of resultsets supported by Derby at this point.

            People

            • Assignee:
              Fernanda Pizzorno
              Reporter:
              Mamta A. Satoor
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development