Derby
  1. Derby
  2. DERBY-2515

Network client does not retain the INOUT parameter value change for subsequent execution

    Details

    • Urgency:
      Urgent
    • Issue & fix info:
      High Value Fix, Newcomer, Repro attached
    • Bug behavior facts:
      Embedded/Client difference, Wrong query result

      Description

      If I set a INOUT parameter to a value (say 12.3) and it gets
      modified by the procedure to another value (say 45.6) then on
      the next execution
      of the same CallableStatement, embedded maintains the
      current value (45.6), while network server reverts to the
      former value (12.3).

      This issue was found while converting the test lang/procedure.java. See references to this issue in the converted LangProcedureTest.java

      1. Test_2515.java
        3 kB
        Rick Hillegas
      2. derby-2515-01-ac-copyINOUTreturnValues.diff
        24 kB
        Rick Hillegas
      3. derby-2515-02-aa-polishCatchBlock.diff
        3 kB
        Rick Hillegas

        Issue Links

          Activity

          Hide
          Knut Anders Hatlen added a comment -

          [bulk update] Close all resolved issues that haven't been updated for more than one year.

          Show
          Knut Anders Hatlen added a comment - [bulk update] Close all resolved issues that haven't been updated for more than one year.
          Hide
          Kathey Marsden added a comment -

          Reassigning to Rick and resolving. Also restoring the affects version. This was accidentally changed with the bulk reopen. I had intended to add 10.5 as an affects version but unfortunately that replaced what was there.

          Show
          Kathey Marsden added a comment - Reassigning to Rick and resolving. Also restoring the affects version. This was accidentally changed with the bulk reopen. I had intended to add 10.5 as an affects version but unfortunately that replaced what was there.
          Hide
          Kathey Marsden added a comment -

          Assign temporarily for backport

          Show
          Kathey Marsden added a comment - Assign temporarily for backport
          Hide
          Kathey Marsden added a comment -

          Reopen for 10.5 backport consideration. If working on the backport for this issue. Temporarily assign yourself and add a comment that you are working on it. When finished, reresolve with the new fix versions or label backport_reject_10_x as appropriate.

          Show
          Kathey Marsden added a comment - Reopen for 10.5 backport consideration. If working on the backport for this issue. Temporarily assign yourself and add a comment that you are working on it. When finished, reresolve with the new fix versions or label backport_reject_10_x as appropriate.
          Hide
          Rick Hillegas added a comment -

          I think that this work should be safe to backport to older releases.

          Show
          Rick Hillegas added a comment - I think that this work should be safe to backport to older releases.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-2515-02-aa-polishCatchBlock.diff, which incorporates Knut's excellent suggestions. Committed at subversion revision 1087346.

          Show
          Rick Hillegas added a comment - Attaching derby-2515-02-aa-polishCatchBlock.diff, which incorporates Knut's excellent suggestions. Committed at subversion revision 1087346.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for the explanations, Rick.

          1) I asked because I wasn't sure if the Integer objects originated from user input. If they come from the server, and the server says the type is SMALLINT, I think we should trust the server. So I'm comfortable with the current code.

          2) Right, it looks like Cursor.getObject() raises an exception if it doesn't understand the JDBC type of the column, which would indicate a bug. And adding a new checked exception type to the throws clause is a bit awkward here, I agree, at least when it's just to handle a case that's not supposed to happen. Narrowing it down to SqlException only, and possibly also making the try block smaller so that it only encloses the expression that may raise the exception, sounds like a good improvement. For extra bonus, you might want to call initCause() on the wrapper to preserve the stack trace if this is ever going to happen.

          Show
          Knut Anders Hatlen added a comment - Thanks for the explanations, Rick. 1) I asked because I wasn't sure if the Integer objects originated from user input. If they come from the server, and the server says the type is SMALLINT, I think we should trust the server. So I'm comfortable with the current code. 2) Right, it looks like Cursor.getObject() raises an exception if it doesn't understand the JDBC type of the column, which would indicate a bug. And adding a new checked exception type to the throws clause is a bit awkward here, I agree, at least when it's just to handle a case that's not supposed to happen. Narrowing it down to SqlException only, and possibly also making the try block smaller so that it only encloses the expression that may raise the exception, sounds like a good improvement. For extra bonus, you might want to call initCause() on the wrapper to preserve the stack trace if this is ever going to happen.
          Hide
          Rick Hillegas added a comment -

          Thanks for looking at the patch, Knut. Some answers to your questions follow:

          1) No checking has been made to ensure that the Integer won't truncate silently when it is stuffed into the Short. I can run some experiments to see what happens when you try to send a large int across the network for use by a SMALLINT INOUT arg.

          2) Cursor.getObject() can raise SqlException. But if it does that probably indicates a bug--it is returning what java.sql.CallableStatement.getObject() is supposed to return. I could narrow that catch down to just filter out SqlExceptions. StatementCallbackInterface.completeExecuteCall() raises no errors and its callers perform no error handling other than to relay DisconnectExceptions up the call stack.

          Thanks,
          -Rick

          Show
          Rick Hillegas added a comment - Thanks for looking at the patch, Knut. Some answers to your questions follow: 1) No checking has been made to ensure that the Integer won't truncate silently when it is stuffed into the Short. I can run some experiments to see what happens when you try to send a large int across the network for use by a SMALLINT INOUT arg. 2) Cursor.getObject() can raise SqlException. But if it does that probably indicates a bug--it is returning what java.sql.CallableStatement.getObject() is supposed to return. I could narrow that catch down to just filter out SqlExceptions. StatementCallbackInterface.completeExecuteCall() raises no errors and its callers perform no error handling other than to relay DisconnectExceptions up the call stack. Thanks, -Rick
          Hide
          Knut Anders Hatlen added a comment -

          Hi Rick,

          Thanks for fixing this bug. I have two questions about the patch:

          1)
          + //
          + // special case to coerce Integer to Short for SMALLINT
          + //
          + if ( parameterMetaData_.types_[ i ] == Types.SMALLINT )
          + {
          + if ( (returnArg != null) && (returnArg instanceof Integer) )
          +

          { + returnArg = new Short( ((Integer) returnArg).shortValue() ); + }

          + }

          When we get here, have we already checked that the Integer argument fits in a SMALLINT, or do we risk that Integer.shortValue() silently truncates the original value?

          2)
          + } catch (Exception se)
          +

          { + throw new IllegalArgumentException( se.getMessage() ); + }

          What kind of exceptions do we expect here, and why convert to IllegalArgumentException? Is that something expected by the higher levels of the code?

          Show
          Knut Anders Hatlen added a comment - Hi Rick, Thanks for fixing this bug. I have two questions about the patch: 1) + // + // special case to coerce Integer to Short for SMALLINT + // + if ( parameterMetaData_.types_[ i ] == Types.SMALLINT ) + { + if ( (returnArg != null) && (returnArg instanceof Integer) ) + { + returnArg = new Short( ((Integer) returnArg).shortValue() ); + } + } When we get here, have we already checked that the Integer argument fits in a SMALLINT, or do we risk that Integer.shortValue() silently truncates the original value? 2) + } catch (Exception se) + { + throw new IllegalArgumentException( se.getMessage() ); + } What kind of exceptions do we expect here, and why convert to IllegalArgumentException? Is that something expected by the higher levels of the code?
          Hide
          Rick Hillegas added a comment -

          Now that the blocking bug is fixed, committed derby-2515-01-ac-copyINOUTreturnValues.diff at subversion revision 1086799.

          Show
          Rick Hillegas added a comment - Now that the blocking bug is fixed, committed derby-2515-01-ac-copyINOUTreturnValues.diff at subversion revision 1086799.
          Hide
          Rick Hillegas added a comment -

          Attaching derby-2515-01-ac-copyINOUTreturnValues.diff. This patch fixes the bug by copying output values to the corresponding input array in the network driver.

          The tests passed cleanly for me except for known Heisenbugs and a new failure in ProcedureTest introduced by this patch. The new test case in ProcedureTest discloses DERBY-5162. Once that bug is fixed, ProcedureTest should run cleanly too.

          Touches the following files:

          ----------

          M java/client/org/apache/derby/client/am/CallableStatement.java

          In the network driver, input args and output args are maintained in two separate arrays. This patch copies the output values of INOUT args to the corresponding slots in the input array.

          ----------

          M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/ProcedureTest.java

          Adds a test case to verify that INOUT values are preserved across invocations for all datatypes.

          Show
          Rick Hillegas added a comment - Attaching derby-2515-01-ac-copyINOUTreturnValues.diff. This patch fixes the bug by copying output values to the corresponding input array in the network driver. The tests passed cleanly for me except for known Heisenbugs and a new failure in ProcedureTest introduced by this patch. The new test case in ProcedureTest discloses DERBY-5162 . Once that bug is fixed, ProcedureTest should run cleanly too. Touches the following files: ---------- M java/client/org/apache/derby/client/am/CallableStatement.java In the network driver, input args and output args are maintained in two separate arrays. This patch copies the output values of INOUT args to the corresponding slots in the input array. ---------- M java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/ProcedureTest.java Adds a test case to verify that INOUT values are preserved across invocations for all datatypes.
          Hide
          Rick Hillegas added a comment -

          Attaching Test_2515.java, a test case which shows this behavior. The embedded and network drivers both reset purely OUT args the same way. INOUT args are treated differently: the embedded driver does not touch the last value returned but the client driver resets the INOUT arg to the last value set by its setXXX() method.

          Show
          Rick Hillegas added a comment - Attaching Test_2515.java, a test case which shows this behavior. The embedded and network drivers both reset purely OUT args the same way. INOUT args are treated differently: the embedded driver does not touch the last value returned but the client driver resets the INOUT arg to the last value set by its setXXX() method.
          Hide
          Kathey Marsden added a comment -

          Triage for 10.8. Marking Urgent as it seems we would get wrong results if the in/out value is not updated properly.
          Marking newcomer hoping it is a contained jdbc fix.

          Show
          Kathey Marsden added a comment - Triage for 10.8. Marking Urgent as it seems we would get wrong results if the in/out value is not updated properly. Marking newcomer hoping it is a contained jdbc fix.
          Hide
          Kathey Marsden added a comment -

          This issue was found while converting procedure.java

          Show
          Kathey Marsden added a comment - This issue was found while converting procedure.java

            People

            • Assignee:
              Rick Hillegas
              Reporter:
              Kathey Marsden
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development