Derby
  1. Derby
  2. DERBY-1629

Exceptions thrown by code in procedures or triggers are not handled/thrown correctly by Derby

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Duplicate
    • Affects Version/s: None
    • Fix Version/s: 10.3.1.4
    • Component/s: JDBC, SQL
    • Labels:
      None
    • Urgency:
      Normal
    • Issue & fix info:
      Release Note Needed
    • Bug behavior facts:
      Regression

      Description

      In Java SE 6 runtime environments, an application may not get the SQL Exception with SQL State 38000 when execution of a trigger or procedure fails with an exception caused by Derby internals. Instead, they will get the underlying exception with it's SQL State. For an example of this, see lang/procedureInTrigger.sql, which has different output for Java SE 6 (encoded in the master/jdk16/procedureInTrigger.out file) than for other Java SE runtimes.

      1. DERBY-1629_j9canon.diff
        0.9 kB
        Myrna van Lunteren

        Issue Links

          Activity

          Hide
          Øystein Grøvlen added a comment -

          It seems like this issue is a duplicate of DERBY-1440. At least, Knut Anders patch removes the check in lang/ProcedureInTriggerTest here mention by Myrna, and which seems to fit the original description of the problem. I am not quite sure I understand all the comments made on this JIRA, and whether there are other issues here, but since the original problem seems to be solved, I suggest this JIRA should be closed.

          Show
          Øystein Grøvlen added a comment - It seems like this issue is a duplicate of DERBY-1440 . At least, Knut Anders patch removes the check in lang/ProcedureInTriggerTest here mention by Myrna, and which seems to fit the original description of the problem. I am not quite sure I understand all the comments made on this JIRA, and whether there are other issues here, but since the original problem seems to be solved, I suggest this JIRA should be closed.
          Hide
          Myrna van Lunteren added a comment -

          When this gets fixed, the if (!JDBC.vmsupportsJDBC4) check in test lang/ProcedureInTriggerTest should be removed.

          Show
          Myrna van Lunteren added a comment - When this gets fixed, the if (!JDBC.vmsupportsJDBC4) check in test lang/ProcedureInTriggerTest should be removed.
          Hide
          Andrew McIntyre added a comment -

          Unsetting Fix Version for unassigned issues.

          Show
          Andrew McIntyre added a comment - Unsetting Fix Version for unassigned issues.
          Hide
          Rick Hillegas added a comment -

          Move to 10.2.3.0.

          Show
          Rick Hillegas added a comment - Move to 10.2.3.0.
          Hide
          Daniel John Debrunner added a comment -

          I think there may be a correction to the SQL spec for the correct way to handle exceptions. I don't have the details, just saw it while I was searching for something else. The correction is in an official SQL standard correction document which modifies the text in SQL Part 13 which itself modifies the SQL/Foundation spec. Thus it's sometimes confusing to realise what exactly is expected

          Show
          Daniel John Debrunner added a comment - I think there may be a correction to the SQL spec for the correct way to handle exceptions. I don't have the details, just saw it while I was searching for something else. The correction is in an official SQL standard correction document which modifies the text in SQL Part 13 which itself modifies the SQL/Foundation spec. Thus it's sometimes confusing to realise what exactly is expected
          Hide
          Rick Hillegas added a comment -

          Moving to 10.2.2.0.

          Show
          Rick Hillegas added a comment - Moving to 10.2.2.0.
          Hide
          David Van Couvering added a comment -

          Committed Myrna's patch, revision 428887

          Show
          David Van Couvering added a comment - Committed Myrna's patch, revision 428887
          Hide
          Myrna van Lunteren added a comment -

          attaching a patch - DERBY-1629_j9canon.diff that updates the j9_22 canon. tested the test with wctme5.7.

          Show
          Myrna van Lunteren added a comment - attaching a patch - DERBY-1629 _j9canon.diff that updates the j9_22 canon. tested the test with wctme5.7.
          Hide
          David Van Couvering added a comment -

          Added a comment to the procedureInTrigger.sql script and fixed the main master file. Also added a master file specific to jdk16.
          I noticed procedureInTrigger.out also exists in a j9-specific directory, but as I don' t have the j9 VM to verify I didn't try to fix this file.

          Committed revision 42818

          Show
          David Van Couvering added a comment - Added a comment to the procedureInTrigger.sql script and fixed the main master file. Also added a master file specific to jdk16. I noticed procedureInTrigger.out also exists in a j9-specific directory, but as I don' t have the j9 VM to verify I didn't try to fix this file. Committed revision 42818
          Hide
          David Van Couvering added a comment -

          I think what you described is in the SQL spec is not what is happening in this code right now, and should be fixed. And I think you're right that this does not require us to know whether the exception is a Derby exception or not, who knows, the procedure could be calling some other backend database. That simplifies things I think.

          Show
          David Van Couvering added a comment - I think what you described is in the SQL spec is not what is happening in this code right now, and should be fixed. And I think you're right that this does not require us to know whether the exception is a Derby exception or not, who knows, the procedure could be calling some other backend database. That simplifies things I think.
          Hide
          David Van Couvering added a comment -

          Fixed this to describe user-perceived behavior. Here is the original comment, which provides useful implementation details:

          In non-JDK 1.6 builds, the exceptions Derby throws are all of class EmbedSQLException. As of JDK 1.6, that is no longer true. Instead we throw a "native" SQL Exception.

          That makes the following code incorrect:

          public static StandardException unexpectedUserException(Throwable t)
          {
          /*

            • If we have a SQLException that isn't a Util
            • (i.e. it didn't come from cloudscape), then we check
            • to see if it is a valid user defined exception range
            • (38001-38XXX). If so, then we convert it into a
            • StandardException without further ado.
              */
              if ((t instanceof SQLException) &&
              !(t instanceof EmbedSQLException))
              {
              SQLException sqlex = (SQLException)t;
              String state = sqlex.getSQLState();
              if ((state != null) &&
              (state.length() == 5) &&
              state.startsWith("38") &&
              !state.equals("38000"))
              {
              StandardException se = new StandardException(state, sqlex.getMessage());
              if (sqlex.getNextException() != null) { se.setNestedException(sqlex.getNextException()); }

              return se;
              }
              }

          I am not sure how we can detect internally-thrown SQL Exceptions and distinguish them from user exceptions, but this does need to be looked at.

          Right now procedureInTrigger.sql is failing for JDK 1.6 due to this error. I may check in a jdk16-specific version of this file so at least derbyall can pass.

          Show
          David Van Couvering added a comment - Fixed this to describe user-perceived behavior. Here is the original comment, which provides useful implementation details: In non-JDK 1.6 builds, the exceptions Derby throws are all of class EmbedSQLException. As of JDK 1.6, that is no longer true. Instead we throw a "native" SQL Exception. That makes the following code incorrect: public static StandardException unexpectedUserException(Throwable t) { /* If we have a SQLException that isn't a Util (i.e. it didn't come from cloudscape), then we check to see if it is a valid user defined exception range (38001-38XXX). If so, then we convert it into a StandardException without further ado. */ if ((t instanceof SQLException) && !(t instanceof EmbedSQLException)) { SQLException sqlex = (SQLException)t; String state = sqlex.getSQLState(); if ((state != null) && (state.length() == 5) && state.startsWith("38") && !state.equals("38000")) { StandardException se = new StandardException(state, sqlex.getMessage()); if (sqlex.getNextException() != null) { se.setNestedException(sqlex.getNextException()); } return se; } } I am not sure how we can detect internally-thrown SQL Exceptions and distinguish them from user exceptions, but this does need to be looked at. Right now procedureInTrigger.sql is failing for JDK 1.6 due to this error. I may check in a jdk16-specific version of this file so at least derbyall can pass.
          Hide
          Daniel John Debrunner added a comment -

          I think it's also worth re-validating the requirements, some of the assumptions about that method are from the old Cloudscape days when any java method could be executed from SQL. Looking again it seems as though the current behaviour for routines is wrong and would not require knowing if an exception was thrown by Derby or not. Other uses of the method might though. The behaviour for routines is defined in SQL part 13, section 15.1, which seems to boil down to:

          If a method throws an exception E then

          if (method instance of java.sql.Exception)

          { if SQL state starts with "38", is at least 5 characters and is not 38000 throw an exception with E's SQL state else throw an exception with SQL state 39001 }

          else

          { throw an exception with SQL state 38000 }

          Assuming we can take the meaning of "class of E is java.sql.SQLException" to mean instanceof and not class has to be java.sql.SQLException,
          since this was written before SQLException had sub-classes.

          Show
          Daniel John Debrunner added a comment - I think it's also worth re-validating the requirements, some of the assumptions about that method are from the old Cloudscape days when any java method could be executed from SQL. Looking again it seems as though the current behaviour for routines is wrong and would not require knowing if an exception was thrown by Derby or not. Other uses of the method might though. The behaviour for routines is defined in SQL part 13, section 15.1, which seems to boil down to: If a method throws an exception E then if (method instance of java.sql.Exception) { if SQL state starts with "38", is at least 5 characters and is not 38000 throw an exception with E's SQL state else throw an exception with SQL state 39001 } else { throw an exception with SQL state 38000 } Assuming we can take the meaning of "class of E is java.sql.SQLException" to mean instanceof and not class has to be java.sql.SQLException, since this was written before SQLException had sub-classes.
          Hide
          Daniel John Debrunner added a comment -

          Can the summary of this bug be changed to reflect the impact on/change to/incorect behavaiour an application would see. This will be a regression when running on jdk 16 for an application, but what exactly is the regression?

          Show
          Daniel John Debrunner added a comment - Can the summary of this bug be changed to reflect the impact on/change to/incorect behavaiour an application would see. This will be a regression when running on jdk 16 for an application, but what exactly is the regression?
          Hide
          David Van Couvering added a comment -

          Thanks for your thoughts, Rick. It seems to me that for (3) if a user set a Derby exception as a cause, getCause() would return a SQLException, not an EmbeddedSQLException, so I would argue your wormhole can be closed...

          But I don't like how it relies on behavior that could be changed at any time, and it's overloading the intention of setCause()/getCause(). But unless we go for (1) I can't see any way out of piggybacking extra semantics on one of the SQLException methods..., and getCause() seems like the best choice.

          I would suggest that whoever fixes this adds a comment to the SQLExceptionFactory40 code that indicates that changing the cause will impact other code and describe how and why.

          David

          Show
          David Van Couvering added a comment - Thanks for your thoughts, Rick. It seems to me that for (3) if a user set a Derby exception as a cause, getCause() would return a SQLException, not an EmbeddedSQLException, so I would argue your wormhole can be closed... But I don't like how it relies on behavior that could be changed at any time, and it's overloading the intention of setCause()/getCause(). But unless we go for (1) I can't see any way out of piggybacking extra semantics on one of the SQLException methods..., and getCause() seems like the best choice. I would suggest that whoever fixes this adds a comment to the SQLExceptionFactory40 code that indicates that changing the cause will impact other code and describe how and why. David
          Hide
          Rick Hillegas added a comment -

          Here are various ways to tackle this problem:

          1) We could create a parallel universe of SQLException subclasses, which extend the JDBC4 SQLException subclasses and implement some vacuous, Derby-specific interface. These would be the exceptions raised by SQLExceptionFactory40. EmbedSQLException could implement this vacuous interface too. In the code above (StandardException.unexpectedUserException()), we could then check whether the exception class was an instance of the vacuous interface.

          + I think this covers the cases

          • Bloats up the server with around 20 new classes
          • As more SQLException subclasses are added to JDBC, we will have to add additional parallel Derby classes

          2) We could check whether the errorCode was one of the Derby-specific severities. This would be our flag that the exception was generated by Derby.

          + Not a lot of code bloat.

          • Has wormholes: What if the user-created exception uses one of our severities as its error code?

          3) We could check whether the passed-in SQLException's getCause() method returns an EmbedSQLException. This would be true for all exceptions created by SQLExceptionFactory40

          + Also not a lot of code bloat

          • Still has the wormhole that the user-created exception could set a Derby exception as its cause
          Show
          Rick Hillegas added a comment - Here are various ways to tackle this problem: 1) We could create a parallel universe of SQLException subclasses, which extend the JDBC4 SQLException subclasses and implement some vacuous, Derby-specific interface. These would be the exceptions raised by SQLExceptionFactory40. EmbedSQLException could implement this vacuous interface too. In the code above (StandardException.unexpectedUserException()), we could then check whether the exception class was an instance of the vacuous interface. + I think this covers the cases Bloats up the server with around 20 new classes As more SQLException subclasses are added to JDBC, we will have to add additional parallel Derby classes 2) We could check whether the errorCode was one of the Derby-specific severities. This would be our flag that the exception was generated by Derby. + Not a lot of code bloat. Has wormholes: What if the user-created exception uses one of our severities as its error code? 3) We could check whether the passed-in SQLException's getCause() method returns an EmbedSQLException. This would be true for all exceptions created by SQLExceptionFactory40 + Also not a lot of code bloat Still has the wormhole that the user-created exception could set a Derby exception as its cause
          Hide
          Andrew McIntyre added a comment -

          fix typo

          Show
          Andrew McIntyre added a comment - fix typo

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development