Commons Dbcp
  1. Commons Dbcp
  2. DBCP-347

DelegatingStatement class has incomplete isWrapperFor method

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3.1, 1.4.1, 1.5.1, 2.0
    • Labels:
      None
    • Environment:

      Windows 7. java version "1.6.0_21". Dell Latitude E6410.

      Description

      Currently org.apache.commons.dbcp.DelegatingStatement#isWrapperFor checks only if:

      1. Requested class is assignable from the DelegatingStatement instance: iface.isAssignableFrom(getClass())
      2. Wrapped object is a wrapper for the requested class: _stmt.isWrapperFor(iface)

      I think there should be another option checked i.e. requested class is assignable from the wrapped object's class. For example: iface.isAssignableFrom(_stmt.getClass())
      This is especially that in fact unwrap method properly assumes this possiblity i.e.: return iface.cast(_stmt);

      The whole method should be:

      public boolean isWrapperFor(Class<?> iface) throws SQLException

      { return iface.isAssignableFrom(getClass()) || iface.isAssignableFrom(_stmt.getClass()) || _stmt.isWrapperFor(iface); }

        Activity

        Robert Poskrobek created issue -
        Hide
        ggregory@seagullsw.com added a comment -

        Would you be willing to provide a patch that includes a unit test to prove what is broken?

        Show
        ggregory@seagullsw.com added a comment - Would you be willing to provide a patch that includes a unit test to prove what is broken?
        Hide
        Robert Poskrobek added a comment - - edited

        Ok! Let's start with java.sql.Wrapper#isWrapperFor specification. It says that class implementing Wrapper should check if directly implements requested interface. Otherwise it should immediately delegate the call to its wrapped object. According to this org.apache.commons.dbcp.DelegatingStatement#isWrapperFor is written correctly. On the other hand Sun's specificifaction for java.sql.Wrapper#unwrap method assumes wrapper to try to cast wrapped object rather than immediately delegating to its unwrap method. Having this inconsistency I'd vote for adding another level of checking for wrapper. This is because some wrapped classes doesn't implement java.sql.Wrapper correctly (or at all) but still are valid classes for casting.

        Probably better to show my suggestion as code.
        Currently we've got:

        org.apache.commons.dbcp.DelegatingStatement
            public boolean isWrapperFor(Class<?> iface) throws SQLException {
                  return iface.isAssignableFrom(getClass()) || _stmt.isWrapperFor(iface);
            }
        

        I suggest adding additional alternative condition here so:

        org.apache.commons.dbcp.DelegatingStatement
            public boolean isWrapperFor(Class<?> iface) throws SQLException {
                  return iface.isAssignableFrom(getClass()) || iface.isAssignableFrom(_stmt.getClass()) || _stmt.isWrapperFor(iface);
            }
        

        There is no unit test for this method so I suggest adding it:

        org.apache.commons.dbcp.TestDelegatingStatement
            public void testIsWrapperFor() throws Exception {
                TesterConnection tstConn = new TesterConnection("rposcro", "rposcro");
                TesterStatement tstStmt = new TesterStatement(tstConn);
                DelegatingConnection conn = new DelegatingConnection(tstConn);
                DelegatingStatement stmt = new DelegatingStatement(conn, tstStmt);
        
                Class<?> stmtProxyClass = Proxy.getProxyClass(
                        this.getClass().getClassLoader(), 
                        Statement.class);
                
                assertTrue(stmt.isWrapperFor(DelegatingStatement.class));
                assertTrue(stmt.isWrapperFor(TesterStatement.class));
                assertFalse(stmt.isWrapperFor(stmtProxyClass));
            }
        

        In order to pass the test there is fix needed for tester helper class which is not following specification currently:

        org.apache.commons.dbcp.TesterStatement
            public boolean isWrapperFor(Class<?> iface) throws SQLException {
                return false;
            }
        

        Please let me know your thoughts. Regards, r

        Show
        Robert Poskrobek added a comment - - edited Ok! Let's start with java.sql.Wrapper#isWrapperFor specification. It says that class implementing Wrapper should check if directly implements requested interface. Otherwise it should immediately delegate the call to its wrapped object. According to this org.apache.commons.dbcp.DelegatingStatement#isWrapperFor is written correctly. On the other hand Sun's specificifaction for java.sql.Wrapper#unwrap method assumes wrapper to try to cast wrapped object rather than immediately delegating to its unwrap method. Having this inconsistency I'd vote for adding another level of checking for wrapper. This is because some wrapped classes doesn't implement java.sql.Wrapper correctly (or at all) but still are valid classes for casting. Probably better to show my suggestion as code. Currently we've got: org.apache.commons.dbcp.DelegatingStatement public boolean isWrapperFor( Class <?> iface) throws SQLException { return iface.isAssignableFrom(getClass()) || _stmt.isWrapperFor(iface); } I suggest adding additional alternative condition here so: org.apache.commons.dbcp.DelegatingStatement public boolean isWrapperFor( Class <?> iface) throws SQLException { return iface.isAssignableFrom(getClass()) || iface.isAssignableFrom(_stmt.getClass()) || _stmt.isWrapperFor(iface); } There is no unit test for this method so I suggest adding it: org.apache.commons.dbcp.TestDelegatingStatement public void testIsWrapperFor() throws Exception { TesterConnection tstConn = new TesterConnection( "rposcro" , "rposcro" ); TesterStatement tstStmt = new TesterStatement(tstConn); DelegatingConnection conn = new DelegatingConnection(tstConn); DelegatingStatement stmt = new DelegatingStatement(conn, tstStmt); Class <?> stmtProxyClass = Proxy.getProxyClass( this .getClass().getClassLoader(), Statement.class); assertTrue(stmt.isWrapperFor(DelegatingStatement.class)); assertTrue(stmt.isWrapperFor(TesterStatement.class)); assertFalse(stmt.isWrapperFor(stmtProxyClass)); } In order to pass the test there is fix needed for tester helper class which is not following specification currently: org.apache.commons.dbcp.TesterStatement public boolean isWrapperFor( Class <?> iface) throws SQLException { return false ; } Please let me know your thoughts. Regards, r
        Phil Steitz made changes -
        Field Original Value New Value
        Fix Version/s 1.3.1 [ 12314492 ]
        Fix Version/s 1.4.1 [ 12314493 ]
        Hide
        Mark Thomas added a comment -

        Thanks for the report, patch and test case.

        Similar issues in DelegatingDatabaseMetaData.isWrapperFor() and DelegatingResultSet.isWrapperFor() were also fixed.

        Show
        Mark Thomas added a comment - Thanks for the report, patch and test case. Similar issues in DelegatingDatabaseMetaData.isWrapperFor() and DelegatingResultSet.isWrapperFor() were also fixed.
        Mark Thomas made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 1.5.1 [ 12325670 ]
        Fix Version/s 2.0 [ 12313721 ]
        Resolution Fixed [ 1 ]
        Phil Steitz made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Unassigned
            Reporter:
            Robert Poskrobek
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 1h
              1h
              Remaining:
              Remaining Estimate - 1h
              1h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development