commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robert Poskrobek (JIRA)" <j...@apache.org>
Subject [jira] Issue Comment Edited: (DBCP-347) DelegatingStatement class has incomplete isWrapperFor method
Date Wed, 27 Oct 2010 18:45:19 GMT

    [ https://issues.apache.org/jira/browse/DBCP-347?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12925376#action_12925376
] 

Robert Poskrobek edited comment on DBCP-347 at 10/27/10 2:44 PM:
-----------------------------------------------------------------

Ok! Let's 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:

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

I suggest adding additional alternative condition here so:

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

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

{code:title=org.apache.commons.dbcp.TestDelegatingStatement|borderStyle=solid}
    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));
    }
{code}

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

{code:title=org.apache.commons.dbcp.TesterStatement|borderStyle=solid}
    public boolean isWrapperFor(Class<?> iface) throws SQLException {
        return false;
    }
{code}

Please let me know your thoughts. Regards, r

      was (Author: rposcro):
    Sure, I'll.
 
"Gary Gregory (JIRA)" <jira@apache.org> napisaƂ(a): 
 > 
 >     [ https://issues.apache.org/jira/browse/DBCP-347?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12925341#action_12925341
] 
 > 
 > Gary Gregory commented on DBCP-347:
 > -----------------------------------
 > 
 > Would you be willing to provide a patch that includes a unit test to prove what is broken?
 > 
 > > DelegatingStatement class has incomplete isWrapperFor method
 > > ------------------------------------------------------------
 > >
 > >                 Key: DBCP-347
 > >                 URL: https://issues.apache.org/jira/browse/DBCP-347
 > >             Project: Commons Dbcp
 > >          Issue Type: Bug
 > >         Environment: Windows 7. java version "1.6.0_21". Dell Latitude E6410.
 > >            Reporter: Robert Poskrobek
 > >   Original Estimate: 1h
 > >  Remaining Estimate: 1h
 > >
 > > 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);
 > >     }
 > 
 > -- 
 > This message is automatically generated by JIRA.
 > -
 > You can reply to this email to add a comment to the issue online.
 > 

  
> DelegatingStatement class has incomplete isWrapperFor method
> ------------------------------------------------------------
>
>                 Key: DBCP-347
>                 URL: https://issues.apache.org/jira/browse/DBCP-347
>             Project: Commons Dbcp
>          Issue Type: Bug
>         Environment: Windows 7. java version "1.6.0_21". Dell Latitude E6410.
>            Reporter: Robert Poskrobek
>   Original Estimate: 1h
>  Remaining Estimate: 1h
>
> 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);
>     }

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message