[
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 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:
{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):
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
> 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.
