Derby
  1. Derby
  2. DERBY-2601

Server SQLException error codes are not returned to client

    Details

    • Urgency:
      Normal
    • Issue & fix info:
      Release Note Needed, Repro attached
    • Bug behavior facts:
      Embedded/Client difference

      Description

      ErrorCodes from returned SQLExceptions are not retained in client. e.g. in the example below, client reports an errorcode of -1 instead of 30000. If DRDA allows it would be good for the errorCode to be retained

      [C:/test] java -Dij.showErrorCode=true org.apache.derby.tools.ij
      ij version 10.3
      ij> connect 'jdbc:derby:wombat';
      ij> create table t(i nt, s smallint);
      ERROR 42X01: Syntax error: Encountered "" at line 1, column 18. (errorCode = 30000)
      ij> exit;
      [C:/test] ns start -noSecurityManager &
      [2] 5712
      [C:/test] Apache Derby Network Server - 10.3.0.0 alpha - (1) started and ready to accept connections on port 1527 at 200
      7-04-20 17:36:27.188 GMT

      [C:/test] java -Dij.showErrorCode=true org.apache.derby.tools.ij
      ij version 10.3
      ij> connect 'jdbc:derby://localhost:1527/wombat';
      ij> create table t(i nt, s smallint);
      ERROR 42X01: Syntax error: Encountered "" at line 1, column 18. (errorCode = -1)
      ij>

      Once this has been fixed ErrorCodeTest can be enabled for client.

      1. d2601-1a.diff
        23 kB
        Knut Anders Hatlen
      2. releaseNote.html
        5 kB
        Knut Anders Hatlen

        Issue Links

          Activity

          Hide
          Knut Anders Hatlen added a comment -

          Attaching d2601-1a.diff which makes the server send error codes to the
          client. Derbyall, suites.All and the compatibility test passed with
          the patch.

          In the current code, when an exception happens on the server, the
          server always sends an SQLCARD with SQLCODE=-1, and this value is used
          as error code when the client generates the SQLException.

          Simply changing the server to send the actual error code in the
          SQLCODE field wouldn't work, though, since the client uses the sign of
          the SQLCODE to tell whether the condition is a warning or an error.
          Positive values are interpreted as warnings, and negative as errors.

          The client doesn't depend on the negative value being -1, so we could
          send any negative value to tell that an error occurred. The patch
          exploits this by encoding the always non-negative error code seen on
          the server as a negative value in the SQL code:

          sqlCode = -errorCode - 1

          When the client generates an SQLException, it can convert the SQL code
          back to the original error code:

          errorCode = -sqlCode - 1

          More detailed description of the changes:

          • drda/org/apache/derby/impl/drda/DRDAConnThread.java
          • Push down some of the logic that determines the SQLCODE from
            writeSQLCAGRP() to getSqlCode().
          • Make getSqlCode() return the error code encoded in a negative
            integer if the exception represents an error.
          • Remove unused severity parameter in writeSQLCARD().
          • client/org/apache/derby/client/am/Sqlca.java
          • Added getErrorCode() method to convert SQL code to error code.
          • Promote STATEMENT_SEVERITY errors to TRANSACTION_SEVERITY errors
            if the connection is in auto-commit mode. This is needed in order
            to match the error codes returned by the embedded driver in
            auto-commit (see TransactionResourceImpl.handleException() for the
            corresponding code in the engine). (This does not happen
            automatically otherwise because the server-side connection is
            never auto-commit, even if the client-side connection is.)
          • getUnformattedMessage() now shows the error code instead of the
            SQL code.
          • client/org/apache/derby/client/am/SqlException.java
          • If the exception is generated from an SQLCA, use getErrorCode()
            instead of getSqlCode() to initialize the error code.
          • client/org/apache/derby/client/am/SQLExceptionFactory40.java
          • Check for error code only if the sub-class of SQLException cannot
            be determined from the SQL state. (I don't know why the original
            code checked the error code in the first place. The embedded
            driver only looks at the SQL state.) Now that the error code is
            something else than -1, the early checks for error code for
            example made syntax errors that happened in auto-commit mode
            (whose error codes match TRANSACTION_SEVERITY) would be returned
            as SQLTransactionRollbackExceptions instead of
            SQLSyntaxErrorExceptions.
          • testing/org/apache/derbyTesting/functionTests/master/testclientij.out
          • Update master to expect the correct error codes.
          • testing/org/apache/derbyTesting/junit/BaseJDBCTestCase.java
          • Added helper method that checks the error code.
          • testing/org/apache/derbyTesting/functionTests/tests/derbynet/BadConnectionTest.java
          • Expect the correct error code.
          • testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/J2EEDataSourceTest.java
          • Use new helper method to check error codes.
          • testing/org/apache/derbyTesting/functionTests/tests/lang/ErrorCodeTest.java
          • Enable for client/server.
          • testing/org/apache/derbyTesting/functionTests/tests/replicationTests/ReplicationRun.java
          • testing/org/apache/derbyTesting/functionTests/tests/replicationTests/ShutdownMaster.java
          • testing/org/apache/derbyTesting/functionTests/tests/replicationTests/ShutdownSlave.java
          • Expect correct error codes.
          Show
          Knut Anders Hatlen added a comment - Attaching d2601-1a.diff which makes the server send error codes to the client. Derbyall, suites.All and the compatibility test passed with the patch. In the current code, when an exception happens on the server, the server always sends an SQLCARD with SQLCODE=-1, and this value is used as error code when the client generates the SQLException. Simply changing the server to send the actual error code in the SQLCODE field wouldn't work, though, since the client uses the sign of the SQLCODE to tell whether the condition is a warning or an error. Positive values are interpreted as warnings, and negative as errors. The client doesn't depend on the negative value being -1, so we could send any negative value to tell that an error occurred. The patch exploits this by encoding the always non-negative error code seen on the server as a negative value in the SQL code: sqlCode = -errorCode - 1 When the client generates an SQLException, it can convert the SQL code back to the original error code: errorCode = -sqlCode - 1 More detailed description of the changes: drda/org/apache/derby/impl/drda/DRDAConnThread.java Push down some of the logic that determines the SQLCODE from writeSQLCAGRP() to getSqlCode(). Make getSqlCode() return the error code encoded in a negative integer if the exception represents an error. Remove unused severity parameter in writeSQLCARD(). client/org/apache/derby/client/am/Sqlca.java Added getErrorCode() method to convert SQL code to error code. Promote STATEMENT_SEVERITY errors to TRANSACTION_SEVERITY errors if the connection is in auto-commit mode. This is needed in order to match the error codes returned by the embedded driver in auto-commit (see TransactionResourceImpl.handleException() for the corresponding code in the engine). (This does not happen automatically otherwise because the server-side connection is never auto-commit, even if the client-side connection is.) getUnformattedMessage() now shows the error code instead of the SQL code. client/org/apache/derby/client/am/SqlException.java If the exception is generated from an SQLCA, use getErrorCode() instead of getSqlCode() to initialize the error code. client/org/apache/derby/client/am/SQLExceptionFactory40.java Check for error code only if the sub-class of SQLException cannot be determined from the SQL state. (I don't know why the original code checked the error code in the first place. The embedded driver only looks at the SQL state.) Now that the error code is something else than -1, the early checks for error code for example made syntax errors that happened in auto-commit mode (whose error codes match TRANSACTION_SEVERITY) would be returned as SQLTransactionRollbackExceptions instead of SQLSyntaxErrorExceptions. testing/org/apache/derbyTesting/functionTests/master/testclientij.out Update master to expect the correct error codes. testing/org/apache/derbyTesting/junit/BaseJDBCTestCase.java Added helper method that checks the error code. testing/org/apache/derbyTesting/functionTests/tests/derbynet/BadConnectionTest.java Expect the correct error code. testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/J2EEDataSourceTest.java Use new helper method to check error codes. testing/org/apache/derbyTesting/functionTests/tests/lang/ErrorCodeTest.java Enable for client/server. testing/org/apache/derbyTesting/functionTests/tests/replicationTests/ReplicationRun.java testing/org/apache/derbyTesting/functionTests/tests/replicationTests/ShutdownMaster.java testing/org/apache/derbyTesting/functionTests/tests/replicationTests/ShutdownSlave.java Expect correct error codes.
          Hide
          Knut Anders Hatlen added a comment -

          One note about client/server compatibility.

          If, for example, a transaction-severity error happens on the server,
          the server sees an exception whose getErrorCode() method returns
          30000. Here's what getErrorCode() on the client-side exception will
          return for the various combinations of new and old servers and
          clients:

          (old server, old client) -> -1
          (old server, new client) -> 0
          (new server, old client) -> -30001
          (new server, new client) -> 30000

          ("old" means any version without the fix, "new" means any version with
          the fix)

          With an old server and a new client, 0 is returned because the old
          servers always send SQLCODE -1, and the client converts that to an
          error code by adding one and changing the sign. Since 0 matches
          ExceptionSeverity.NO_APPLICABLE_SEVERITY (intended for "when the
          system was unable to determine the severity"), I think it's slightly
          less wrong than -1. But if someone thinks returning -1, as we did
          before, is better, and that it's worthwhile adding logic to handle
          that, it should be possible to change it.

          With a new server and an old client, the server will send an SQLCODE
          that encodes the original error code, but the client doesn't know how
          to transform that back to an error code, and will return the SQLCODE
          unchanged (-30001). Again, it should be possible to make the server
          send SQLCODE -1 when it's talking to an old client. However, since we
          don't document the error codes, and since -30001 is no more or less
          correct than -1 (they're both wrong), I'm not sure if it's worth
          adding special handling of this case. Maybe a release note?

          Show
          Knut Anders Hatlen added a comment - One note about client/server compatibility. If, for example, a transaction-severity error happens on the server, the server sees an exception whose getErrorCode() method returns 30000. Here's what getErrorCode() on the client-side exception will return for the various combinations of new and old servers and clients: (old server, old client) -> -1 (old server, new client) -> 0 (new server, old client) -> -30001 (new server, new client) -> 30000 ("old" means any version without the fix, "new" means any version with the fix) With an old server and a new client, 0 is returned because the old servers always send SQLCODE -1, and the client converts that to an error code by adding one and changing the sign. Since 0 matches ExceptionSeverity.NO_APPLICABLE_SEVERITY (intended for "when the system was unable to determine the severity"), I think it's slightly less wrong than -1. But if someone thinks returning -1, as we did before, is better, and that it's worthwhile adding logic to handle that, it should be possible to change it. With a new server and an old client, the server will send an SQLCODE that encodes the original error code, but the client doesn't know how to transform that back to an error code, and will return the SQLCODE unchanged (-30001). Again, it should be possible to make the server send SQLCODE -1 when it's talking to an old client. However, since we don't document the error codes, and since -30001 is no more or less correct than -1 (they're both wrong), I'm not sure if it's worth adding special handling of this case. Maybe a release note?
          Hide
          Knut Anders Hatlen added a comment -

          Committed revision 1346833. I'll leave the issue open for now until we've decided whether more logic is needed to handle the case with mixed versions, and whether a release note would be required.

          Show
          Knut Anders Hatlen added a comment - Committed revision 1346833. I'll leave the issue open for now until we've decided whether more logic is needed to handle the case with mixed versions, and whether a release note would be required.
          Hide
          Knut Anders Hatlen added a comment -

          Attaching a release note describing the changes.

          Since no one has voiced their support for adding special handling of the mixed version scenario, I've mentioned the issue in the release notes under "symptoms". Also, in the "application changes required" section, I've added a recommendation that both the client and the server are upgraded to 10.10 or later if the error code is significant to the application.

          Show
          Knut Anders Hatlen added a comment - Attaching a release note describing the changes. Since no one has voiced their support for adding special handling of the mixed version scenario, I've mentioned the issue in the release notes under "symptoms". Also, in the "application changes required" section, I've added a recommendation that both the client and the server are upgraded to 10.10 or later if the error code is significant to the application.
          Hide
          Knut Anders Hatlen added a comment -

          Marking as resolved, since I don't expect any more work on this issue. Also marking it as not for backport because of the behaviour change.

          Show
          Knut Anders Hatlen added a comment - Marking as resolved, since I don't expect any more work on this issue. Also marking it as not for backport because of the behaviour change.
          Hide
          Knut Anders Hatlen added a comment -

          [bulk update: close all resolved issues that haven't had any activity the last year]

          Show
          Knut Anders Hatlen added a comment - [bulk update: close all resolved issues that haven't had any activity the last year]

            People

            • Assignee:
              Knut Anders Hatlen
              Reporter:
              Kathey Marsden
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development