Derby
  1. Derby
  2. DERBY-5872

Inconsistency between isWrapperFor() and unwrap() in logical statements

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.4.1.3, 10.5.1.1, 10.6.1.0, 10.7.1.1, 10.8.1.2, 10.9.1.0
    • Fix Version/s: 10.10.1.1
    • Component/s: JDBC

      Description

      I noticed this when I refactored the logical statement classes in DERBY-5868. The isWrapperFor() method forwards calls to the underlying physical statement, but the unwrap() method works purely at the logical level.

      For example, if you produce a LogicalPreparedStatement40 instance with this code

      ClientConnectionPoolDataSource ds = new ClientConnectionPoolDataSource();
      ds.setDatabaseName("testdb");
      ds.setCreateDatabase("create");
      ds.setMaxStatements(10);
      PooledConnection pc = ds.getPooledConnection();
      Connection c = pc.getConnection();
      PreparedStatement ps = c.prepareStatement("values 1");

      you'll see that

      System.out.println(ps.isWrapperFor(LogicalPreparedStatement40.class));

      prints false, telling that ps is not a wrapper for LogicalPreparedStatement40. However, trying to unwrap ps as a LogicalPreparedStatement succeeds:

      LogicalPreparedStatement40 lps = ps.unwrap(LogicalPreparedStatement40.class);

      On the other hand

      System.out.println(ps.isWrapperFor(PreparedStatement40.class));

      prints true, indicating that ps is a wrapper for PreparedStatement40, but trying to unwrap it as one, fails:

      PreparedStatement40 ps4 = ps.unwrap(PreparedStatement40.class);

      Exception in thread "main" java.sql.SQLException: Unable to unwrap for 'class org.apache.derby.client.am.PreparedStatement40'
      at org.apache.derby.client.am.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:108)
      at org.apache.derby.client.am.SqlException.getSQLException(SqlException.java:364)
      at org.apache.derby.client.am.LogicalStatementEntity.unwrap(LogicalStatementEntity.java:258)
      at org.apache.derby.client.am.LogicalPreparedStatement.unwrap(LogicalPreparedStatement.java:57)
      at Test.main(Test.java:37)
      Caused by: org.apache.derby.client.am.SqlException: Unable to unwrap for 'class org.apache.derby.client.am.PreparedStatement40'
      ... 3 more

      1. derby-5872-1a.diff
        9 kB
        Knut Anders Hatlen

        Activity

        Hide
        Knut Anders Hatlen added a comment -

        The other implementations of isWrapperFor() and unwrap() in the logical classes (LogicalConnection40 and LogicalDatabaseMetaData40) work strictly at the logical level. That is, their unwrap() methods behave the same way as in the statement classes, but their isWrapperFor() methods don't. In those classes, isWrapperFor() returns true if and only if unwrap() would succeed, and unwrap() succeeds if and only if the instance implements/extends the specified interface/class. I think we should change the statement classes to behave the same way.

        Show
        Knut Anders Hatlen added a comment - The other implementations of isWrapperFor() and unwrap() in the logical classes (LogicalConnection40 and LogicalDatabaseMetaData40) work strictly at the logical level. That is, their unwrap() methods behave the same way as in the statement classes, but their isWrapperFor() methods don't. In those classes, isWrapperFor() returns true if and only if unwrap() would succeed, and unwrap() succeeds if and only if the instance implements/extends the specified interface/class. I think we should change the statement classes to behave the same way.
        Hide
        Knut Anders Hatlen added a comment -

        The attached patch makes LogicalStatementEntity.isWrapperFor() work at the logical level, like it does in the other Logical* classes.

        The patch also adds test cases in jdbc4.PreparedStatementTest and jdbc4.CallableStatementTest that expose the bug.

        Additionally, it makes jdbcapi.ClosedObjectTest run on the Logical* classes too. I did this because I noticed ClosedObjectTest didn't fail with my first attempt at a fix, where I had forgotten to add a check for the statement being closed in isWrapperFor(). So I enabled the test for these classes too in order to get more complete testing of the method.

        All the regression tests passed with the patch.

        Show
        Knut Anders Hatlen added a comment - The attached patch makes LogicalStatementEntity.isWrapperFor() work at the logical level, like it does in the other Logical* classes. The patch also adds test cases in jdbc4.PreparedStatementTest and jdbc4.CallableStatementTest that expose the bug. Additionally, it makes jdbcapi.ClosedObjectTest run on the Logical* classes too. I did this because I noticed ClosedObjectTest didn't fail with my first attempt at a fix, where I had forgotten to add a check for the statement being closed in isWrapperFor(). So I enabled the test for these classes too in order to get more complete testing of the method. All the regression tests passed with the patch.
        Hide
        Bryan Pendleton added a comment -

        Looks quite thorough and complete. Thanks for cleaning this up; I'm always pleased
        to see patches that remove redundant code!

        I'm not that familiar with java.sql.Wrapper. It is a JDBC4/JDK6 feature, is that right?

        I tried to look back through svn history to figure out when these wrapper methods were
        introduced, and it looks like it was at least 5 years ago. It's interesting that nobody
        encountered these problems until now; I suppose this means that the Wrapper
        interface isn't heavily used?

        Regardless, thanks again for cleaning up the implementation and adding the extra tests.

        Show
        Bryan Pendleton added a comment - Looks quite thorough and complete. Thanks for cleaning this up; I'm always pleased to see patches that remove redundant code! I'm not that familiar with java.sql.Wrapper. It is a JDBC4/JDK6 feature, is that right? I tried to look back through svn history to figure out when these wrapper methods were introduced, and it looks like it was at least 5 years ago. It's interesting that nobody encountered these problems until now; I suppose this means that the Wrapper interface isn't heavily used? Regardless, thanks again for cleaning up the implementation and adding the extra tests.
        Hide
        Knut Anders Hatlen added a comment -

        Hi Bryan,

        You're right that java.sql.Wrapper was new in JDBC 4.0/JDK 6. I think you're also right that they are not heavily used, at least not in Derby. My understanding is that the purpose of these methods is to provide a standard way to expose vendor-specific extension to the JDBC classes. But we don't publish any extensions, so a Derby application would not benefit much from them.

        One might imagine, though, that someone would write code like this because Derby doesn't implement Statement.cancel():

        if (conn.isWrapperFor(EmbedConnection.class))

        { conn.unwrap(EmbedConnection.class).cancelRunningStatement(); }

        However, they're as likely to write

        if (conn instanceof EmbedConnection)

        { ((EmbedConnection) conn).cancelRunningStatement(); }

        which also works when running on JDBC 3.0 (or JSR-169).

        Show
        Knut Anders Hatlen added a comment - Hi Bryan, You're right that java.sql.Wrapper was new in JDBC 4.0/JDK 6. I think you're also right that they are not heavily used, at least not in Derby. My understanding is that the purpose of these methods is to provide a standard way to expose vendor-specific extension to the JDBC classes. But we don't publish any extensions, so a Derby application would not benefit much from them. One might imagine, though, that someone would write code like this because Derby doesn't implement Statement.cancel(): if (conn.isWrapperFor(EmbedConnection.class)) { conn.unwrap(EmbedConnection.class).cancelRunningStatement(); } However, they're as likely to write if (conn instanceof EmbedConnection) { ((EmbedConnection) conn).cancelRunningStatement(); } which also works when running on JDBC 3.0 (or JSR-169).
        Hide
        Knut Anders Hatlen added a comment -

        Committed revision 1364524.

        Show
        Knut Anders Hatlen added a comment - Committed revision 1364524.
        Hide
        Mike Matrigali added a comment -

        I reviewed just the comments, not sure if this is ok for backport. Leaning toward not, but not sure.

        Show
        Mike Matrigali added a comment - I reviewed just the comments, not sure if this is ok for backport. Leaning toward not, but not sure.
        Hide
        Mamta A. Satoor added a comment -

        Knut, I was wondering if it would be ok to backport this to 10.9 and 10.8. It is on the list of jiras to backport.

        Show
        Mamta A. Satoor added a comment - Knut, I was wondering if it would be ok to backport this to 10.9 and 10.8. It is on the list of jiras to backport.
        Hide
        Kathey Marsden added a comment -

        One thing to note is that the list is just a list of issues to consider for backport, so if upon analysis it looks like it is not a bug that would likely be hit or is difficult to backport, it is sensible to mark it backport reject and move on.

        Show
        Kathey Marsden added a comment - One thing to note is that the list is just a list of issues to consider for backport, so if upon analysis it looks like it is not a bug that would likely be hit or is difficult to backport, it is sensible to mark it backport reject and move on.
        Hide
        Mamta A. Satoor added a comment - - edited

        I will move this jira off of the backport list for now. We can reopen it if we decide to backport it

        Show
        Mamta A. Satoor added a comment - - edited I will move this jira off of the backport list for now. We can reopen it if we decide to backport it

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development