Derby
  1. Derby
  2. DERBY-1201

ResultSet.updateRow() in network client should not throw an exception if no fields in the row have been modified.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Invalid
    • Affects Version/s: 10.1.1.0, 10.1.2.1, 10.1.3.1, 10.2.1.6
    • Fix Version/s: None
    • Component/s: Network Client
    • Labels:
      None
    • Issue & fix info:
      Patch Available

      Description

      ResultSet.updateRow() in the network client throws an exception if no fields in the current row have been modified. In the embedded client, the method silently does nothing in the same circumstances. Although the spec is yet to go final, our understanding is that JDBC 4 spec indicates that the behavior of the embedded client is correct, so we should correct this in the network client.

      1. derby-1201.diff
        2 kB
        Suran Jayathilaka
      2. derby-1201-2.diff
        2 kB
        Suran Jayathilaka
      3. TestUpdateRow.java
        2 kB
        Mayuresh Nirhali

        Activity

        David Van Couvering created issue -
        David Van Couvering made changes -
        Field Original Value New Value
        Component/s Newcomer [ 12310640 ]
        Hide
        Mayuresh Nirhali added a comment -

        This issue is reproducible on 10.2.2, but not on 10.3 trunk. repro is attached.

        The behavior on trunk is consistent, but both the implementations throw exception now. In the description it is hinted that both modes should be silent for such case.

        If community agress that throwing exception in this case is the right behavior, we can close this bug as invalid/Not reproducible.
        Hoping that someone else could comment on this.

        Show
        Mayuresh Nirhali added a comment - This issue is reproducible on 10.2.2, but not on 10.3 trunk. repro is attached. The behavior on trunk is consistent, but both the implementations throw exception now. In the description it is hinted that both modes should be silent for such case. If community agress that throwing exception in this case is the right behavior, we can close this bug as invalid/Not reproducible. Hoping that someone else could comment on this.
        Mayuresh Nirhali made changes -
        Attachment TestUpdateRow.java [ 12359266 ]
        Hide
        Thomas Nielsen added a comment -

        The finalized JDBC 4.0 spec explicitly states that updateRow() called on a ResultSet whos concurrency level is ResultSet.CONCUR_UPDATABLE, and without changes to any row, will be treated as a no-op.

        A no-op should not result in throwing an exception - so both embedded and client drivers needs to change IMHO.

        For the embedded driver:
        A check for any updated rows before calling checksBeforeUpdateOrDelete() in updateRow() is probably in order, as checksBeforeUpdateOrDelete() is also used for deleteRow() and cancelRowUpdates().

        For client driver:
        Same as embedded, but check before calling checkForClosedResultSet() in updateRowX().

        Show
        Thomas Nielsen added a comment - The finalized JDBC 4.0 spec explicitly states that updateRow() called on a ResultSet whos concurrency level is ResultSet.CONCUR_UPDATABLE, and without changes to any row, will be treated as a no-op. A no-op should not result in throwing an exception - so both embedded and client drivers needs to change IMHO. For the embedded driver: A check for any updated rows before calling checksBeforeUpdateOrDelete() in updateRow() is probably in order, as checksBeforeUpdateOrDelete() is also used for deleteRow() and cancelRowUpdates(). For client driver: Same as embedded, but check before calling checkForClosedResultSet() in updateRowX().
        Suran Jayathilaka made changes -
        Assignee Suran Jayathilaka [ suranjay ]
        Suran Jayathilaka made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Hide
        Suran Jayathilaka added a comment -

        Makes the above suggested changes to check if the row has actually been updated.

        Please review.

        Show
        Suran Jayathilaka added a comment - Makes the above suggested changes to check if the row has actually been updated. Please review.
        Suran Jayathilaka made changes -
        Attachment derby-1201.diff [ 12380574 ]
        Kathey Marsden made changes -
        Derby Info [Patch Available]
        Hide
        Kathey Marsden added a comment -

        I verified your fix with the repro, but am not totally sure about the code change. I would feel better if someone familiar with updatable ResultSets took a quick look. A couple of general comments:
        1) The indentation use of tabs/spaces does not seem to match the surrounding code.
        2) We should add a regression test for this case.
        3) Did you run tests with your patch?

        Show
        Kathey Marsden added a comment - I verified your fix with the repro, but am not totally sure about the code change. I would feel better if someone familiar with updatable ResultSets took a quick look. A couple of general comments: 1) The indentation use of tabs/spaces does not seem to match the surrounding code. 2) We should add a regression test for this case. 3) Did you run tests with your patch?
        Hide
        Mamta A. Satoor added a comment -

        It will be great if someone else can also look at the patch attached by Suran.

        From my understanding, it looks like the change is good for embedded driver but I am not sure about the change for client driver.

        From Thomas's comment "The finalized JDBC 4.0 spec explicitly states that updateRow() called on a ResultSet whos concurrency level is ResultSet.CONCUR_UPDATABLE, and without changes to any row, will be treated as a no-op." it appears that we should first make sure ResultSet is ResultSet.CONCUR_UPDATABLE and if yes, then we should make updateRow() as no-op if no changes were made. For client driver, the attached patch first checks if no changes were made and if so, it simply returns from the method. It does not verify whether ResultSet. has ResultSet.CONCUR_UPDATABLE set on it or not. In addition, the patch is removing the call checkForClosedResultSet(); from updateRowX method. I am not sure if that change is ok or not. Maybe someone else knows for sure.

        Show
        Mamta A. Satoor added a comment - It will be great if someone else can also look at the patch attached by Suran. From my understanding, it looks like the change is good for embedded driver but I am not sure about the change for client driver. From Thomas's comment "The finalized JDBC 4.0 spec explicitly states that updateRow() called on a ResultSet whos concurrency level is ResultSet.CONCUR_UPDATABLE, and without changes to any row, will be treated as a no-op." it appears that we should first make sure ResultSet is ResultSet.CONCUR_UPDATABLE and if yes, then we should make updateRow() as no-op if no changes were made. For client driver, the attached patch first checks if no changes were made and if so, it simply returns from the method. It does not verify whether ResultSet. has ResultSet.CONCUR_UPDATABLE set on it or not. In addition, the patch is removing the call checkForClosedResultSet(); from updateRowX method. I am not sure if that change is ok or not. Maybe someone else knows for sure.
        Hide
        Dag H. Wanvik added a comment -

        -1.

        I think this issue is old and can be closed; Derby seems correct, from 10.3 at least.

        If the result set is updatable, Derby does not throw in either
        network client or embedded cases. as far as I can tell. The repro as it stands, uses a read-only result set,
        in which case updateRow is always an error. If I modify the result set to be updatable,
        updateRow becomes a no-op (as required by JDBC4).

        The patch makes the repro not throw for the read-only case, which is wrong, I think.

        Show
        Dag H. Wanvik added a comment - -1. I think this issue is old and can be closed; Derby seems correct, from 10.3 at least. If the result set is updatable, Derby does not throw in either network client or embedded cases. as far as I can tell. The repro as it stands, uses a read-only result set, in which case updateRow is always an error. If I modify the result set to be updatable, updateRow becomes a no-op (as required by JDBC4). The patch makes the repro not throw for the read-only case, which is wrong, I think.
        Hide
        Suran Jayathilaka added a comment -

        Added a check for the client to see if the ResultSet was ResultSet.CONCUR_UPDATABLE before checking if the row has been updated.
        The checkForClosedResultSet() in updateRowX() was removed because a call to that method is available in the rowUpdated() method.

        Show
        Suran Jayathilaka added a comment - Added a check for the client to see if the ResultSet was ResultSet.CONCUR_UPDATABLE before checking if the row has been updated. The checkForClosedResultSet() in updateRowX() was removed because a call to that method is available in the rowUpdated() method.
        Suran Jayathilaka made changes -
        Attachment derby-1201-2.diff [ 12380773 ]
        Hide
        Kathey Marsden added a comment -

        Suran, did you see Dag's earlier comment. Looks like maybe there is nothing to be fixed.
        https://issues.apache.org/jira/browse/DERBY-1201?focusedCommentId=12591615#action_12591615

        Show
        Kathey Marsden added a comment - Suran, did you see Dag's earlier comment. Looks like maybe there is nothing to be fixed. https://issues.apache.org/jira/browse/DERBY-1201?focusedCommentId=12591615#action_12591615
        Hide
        Suran Jayathilaka added a comment -

        I ran the repro with a CONCUR_UPDATABLE statement against the 10.4.1.3 distro and the exceptions were not thrown. Which leads me to believe that this issue is fixed.
        I think it's ok to close this issue.

        Show
        Suran Jayathilaka added a comment - I ran the repro with a CONCUR_UPDATABLE statement against the 10.4.1.3 distro and the exceptions were not thrown. Which leads me to believe that this issue is fixed. I think it's ok to close this issue.
        Hide
        Kathey Marsden added a comment -

        Closing per Suran's and Dag's comments.

        Show
        Kathey Marsden added a comment - Closing per Suran's and Dag's comments.
        Kathey Marsden made changes -
        Resolution Invalid [ 6 ]
        Status In Progress [ 3 ] Closed [ 6 ]
        Dag H. Wanvik made changes -
        Component/s Newcomer [ 12310640 ]
        Gavin made changes -
        Workflow jira [ 12355006 ] Default workflow, editable Closed status [ 12801394 ]

          People

          • Assignee:
            Suran Jayathilaka
            Reporter:
            David Van Couvering
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development