OpenJPA
  1. OpenJPA
  2. OPENJPA-699

SQLWarnings not handled properly with WarningAction set to "handle"

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.0
    • Fix Version/s: 2.0.0-M1
    • Component/s: diagnostics
    • Labels:
      None
    • Environment:
      Kodo 4.1.4
      OpenJPA 1.0.0
      MS SQL 2005
      MS JDBC DRIVER VERSION 1.1
      JDK 1.5

      Description

      If set "kodo.ConnectionFactoryProperties" "warningAction" to "throw" or "handle", when an INSERT statement fails due
      to an attempt to insert null into a non-null column, log output is "The statement has been
      terminated".
      If didn't set "warningAction", the default value is "ignore". It logs correct SQLException which is as follows:

      Cannot
      insert the value NULL into column 'TestTimeNotNullNoDefaultUtc', table
      'STADatabase.dbo.DefaultValuesJ'; column does not allow nulls. INSERT fails.
      {prepstmnt 112461492
      ... ...

      In org.apache.openjpa.lib.jdbc.LoggingConnectionDecorator.LoggingPreparedStatement.executeUpdate(), the
      code is
      public int executeUpdate(String sql) throws SQLException {
      _sql = sql;
      logSQL(this);
      long start = System.currentTimeMillis();
      try

      { return super.executeUpdate(sql); }

      catch (SQLException se)

      { throw wrap(se, LoggingStatement.this); }

      finally

      { logTime(start); handleSQLWarning(LoggingStatement.this); }

      }
      In this test case, it got a SQLWarning with msg 'The statement has been
      terminated' and a SQLException which tells Column null is not allowed.
      When WarningAction is set to 'throw' or 'handle' and if handle doesn't
      consume the warning but throw it, the SQLWarning is thrown from finally
      block.
      The SQLWarning which it is a subclass of SQLException will be processed by
      DBdictionary.newStoreException() so we see the incorrect message.

      1. openJPA-699-v2.patch
        31 kB
        Xiaoqin Feng
      2. openJPA-699.patch
        9 kB
        Xiaoqin Feng

        Activity

        Hide
        Xiaoqin Feng added a comment -

        This is the proposed patch.
        Proposed solution is when a SQLException is catched and WarningAction is set to 'throw' or 'handle', we skip handleSQLwarning.

        Solution is tested on customer case.

        Show
        Xiaoqin Feng added a comment - This is the proposed patch. Proposed solution is when a SQLException is catched and WarningAction is set to 'throw' or 'handle', we skip handleSQLwarning. Solution is tested on customer case.
        Hide
        Abe White added a comment -

        Why don't we just always skip SQL warnings when we're throwing an exception? I.e. just move all handleSQLWarning(...) calls to within the try

        { ... }

        after calling super(...) rather than in the finally block? It seems that when there is a thrown exception, warnings will always be either extraneous or completely meaningless as in this bug report.

        Show
        Abe White added a comment - Why don't we just always skip SQL warnings when we're throwing an exception? I.e. just move all handleSQLWarning(...) calls to within the try { ... } after calling super(...) rather than in the finally block? It seems that when there is a thrown exception, warnings will always be either extraneous or completely meaningless as in this bug report.
        Hide
        Xiaoqin Feng added a comment -

        I was thinking to add fix with least change of existing behavior. So customer can still get log message of warning with other warning action property when exception is thrown.
        I agree that usually when exception is thrown, warnings are meaningless.
        I am fine with move handleSQLWarning() call to try block.

        Show
        Xiaoqin Feng added a comment - I was thinking to add fix with least change of existing behavior. So customer can still get log message of warning with other warning action property when exception is thrown. I agree that usually when exception is thrown, warnings are meaningless. I am fine with move handleSQLWarning() call to try block.
        Hide
        Joe Weinstein added a comment -

        Hi. Actually sometimes the DBMS and driver implement it so the one SQLException
        is fairly generic, with a lot of detail relegated to the messages in the SQLWarnings.
        I have even seen SQLExceptions which state "Please see the information in the
        chained SQLWarnings"!
        Joe Weinstein

        Show
        Joe Weinstein added a comment - Hi. Actually sometimes the DBMS and driver implement it so the one SQLException is fairly generic, with a lot of detail relegated to the messages in the SQLWarnings. I have even seen SQLExceptions which state "Please see the information in the chained SQLWarnings"! Joe Weinstein
        Hide
        Abe White added a comment -

        If Joe is right then neither my proposed solution nor the attached patch are acceptable. We need to capture the output of both the exception and any warnings. So I propose the following changes to LoggingConnectionDecorator:

        1. Change the following pattern:
        try

        {...} catch(SQLException se) { throw wrap(se...); } finally { ... handleSQLWarnings(...); }
        to:
        SQLException err = null;
        try {...}

        catch(SQLException se)

        { err = wrap(se...); }

        finally

        { ... handleSQLErrors(..., err); }

        2. Change all the handleSQLWarnings methods except handleSQLWarnings(SQLWarning) to be named handleSQLErrors and to accept an extra SQLException argument. The try/finally blocks in these methods will become try/catch/finally blocks where the catch captures the thrown exception. At the end of the method we combine the passed-in exception with the thrown exception and throw the result as a single exception (unless either/both are null of course, in which case we throw the non-null one or nothing at all).

        3. Fix all the prepareStatement/createStatement methods that currently don't have handleSQLWarnings in a finally block at all to use a try/catch/finally block as outlined in change #1 above.

        Does that sound acceptable? We might still get extraneous "The statement has been closed" output in the combined exceptions we throw under certain drivers, but at least no information will get lost.

        Show
        Abe White added a comment - If Joe is right then neither my proposed solution nor the attached patch are acceptable. We need to capture the output of both the exception and any warnings. So I propose the following changes to LoggingConnectionDecorator: 1. Change the following pattern: try {...} catch(SQLException se) { throw wrap(se...); } finally { ... handleSQLWarnings(...); } to: SQLException err = null; try {...} catch(SQLException se) { err = wrap(se...); } finally { ... handleSQLErrors(..., err); } 2. Change all the handleSQLWarnings methods except handleSQLWarnings(SQLWarning) to be named handleSQLErrors and to accept an extra SQLException argument. The try/finally blocks in these methods will become try/catch/finally blocks where the catch captures the thrown exception. At the end of the method we combine the passed-in exception with the thrown exception and throw the result as a single exception (unless either/both are null of course, in which case we throw the non-null one or nothing at all). 3. Fix all the prepareStatement/createStatement methods that currently don't have handleSQLWarnings in a finally block at all to use a try/catch/finally block as outlined in change #1 above. Does that sound acceptable? We might still get extraneous "The statement has been closed" output in the combined exceptions we throw under certain drivers, but at least no information will get lost.
        Hide
        Xiaoqin Feng added a comment -

        That is a good plan.

        Show
        Xiaoqin Feng added a comment - That is a good plan.
        Hide
        Xiaoqin Feng added a comment -

        Here is the revised patch according to our discussion.

        Show
        Xiaoqin Feng added a comment - Here is the revised patch according to our discussion.
        Hide
        Abe White added a comment -

        Applied v2 patch per comments in SVN revision 689518.

        Show
        Abe White added a comment - Applied v2 patch per comments in SVN revision 689518.

          People

          • Assignee:
            Unassigned
            Reporter:
            Xiaoqin Feng
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development