Uploaded image for project: 'Derby'
  1. Derby
  2. DERBY-5872

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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
        knutanders 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
        knutanders 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
        knutanders 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
        knutanders 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
        bryanpendleton 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
        bryanpendleton 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
        knutanders 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
        knutanders 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
        knutanders Knut Anders Hatlen added a comment -

        Committed revision 1364524.

        Show
        knutanders Knut Anders Hatlen added a comment - Committed revision 1364524.
        Hide
        mikem 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
        mikem 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
        mamtas 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
        mamtas 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
        kmarsden 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
        kmarsden 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
        mamtas 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
        mamtas 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:
            knutanders Knut Anders Hatlen
            Reporter:
            knutanders Knut Anders Hatlen
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development