#776 3 bugs in CloseResourceRule (with fixes attached)

[Conrad]

I found and fixed 3 problems with CloseResourceRule.

1. The closeTargets property does not work if the target method is called by its class name prefix. I.e., MyHelper.closeDbResources. You can reproduce this by changing the existing MyHelper.close regression test to MyHelper.myClose in both the test case java code and the closeTargets property. The reason the MyHelper.close test case works is because the utility method is named "close", which is what the code looks for while ignoring the "MyHelper." part.

2. Just because a closeTarget method is called in the finally block doesn't mean that the resource is closed -- the resource needs to be passed to that method. E.g., this test case passes the current rule but should be flagged:
   public class Foo {
   void bar() {
   Statement c = pool.getStmt();
   try {
   } finally {
   MyHelper.close(null);
   }
   }
   }

The fixed rule will flag the above test case but realize that this one passes:
public class Foo {
void bar() {
Statement c = pool.getStmt();
try {
} finally {
MyHelper.close(c);
}
}
}

1. This may be a little subjective, but if a utility method is used to allocate a db resource (typically a Connection) and return it, that method should not be flagged -- there is no way the method can close the resource because that's the whole point of the method -- to allocate and return a Connection. The fixed rule doesn't flag db resource variables that are returned by the method.
   This is the test case that is flagged by the current rule but passes the fixed rule:
   public class Foo {
   public Connection getConnection() {
   Connection c = pool.getConnection();
   return c;
   }
   }

The attached CloseResourceRule.java contains my fixes for all 3 issues above, applied to the latest file from the trunk. I tried in vain for a few hours but was unable to get SVN to work for me, so I'm afraid all I can do is attach the file here and hope that someone else run regression tests and check in this change if it is OK.

[Conrad]

CloseResourceRule.java with 3 fixes

[Romain PELISSE]

Logged In: YES
user_id=1679130
Originator: NO

Hi,

First of all thanks for both this bug report and the fixes. It's a shame you were not able to connect to SVN, having a proper patch, would have help me a little bit. Also note, that you should have created this entry in patch tracker. It's easier for us, if patches are with patches, and bugs with bugs :)

I'm working on your modification, however the caused the case "closeTargets properties for statements" to fail. I failed to reproduce properly issue (1).

Could you check in your code to see what cause this regression test to fail ? Could provide me a test-code xml to reproduce the issue (1) ?

Thanks

[Conrad]

Logged In: YES
user_id=2086301
Originator: YES

Upon closer examination, I see that the #1 problem above is incorrectly stated. The problem is not with any "MyHelper.myClose" closeTarget, but only when the close target contains the "this" prefix. The test-code below reproduces it. This means that, at a minimum, the two "Found a call to a" comments in my code should be changed.

<test-code reinitializeRule="true">
    <description><![CDATA[

Ok, closeTargets properties for statements
]]>
<rule-property name="closeTargets">this.closeConnection</rule-property>
<rule-property name="types">Connection,Statement,ResultSet</rule-property>
<expected-problems>0</expected-problems>

[Romain PELISSE]

Logged In: YES
user_id=1679130
Originator: NO

Thanks a lot for providing the test case.

Ok, i add your test case (and swith the expected problem from <expected-problems>0</expected-problems> to <expected-problems>1</expected-problems>, which seems to me "more like it"). Ok now we've got a proper test for your improvement.

Now only remains the issue of the failing regression test. I'm not really happy on commiting your changes without fixing this issues. If you have the time, i'll appreciate if you could look at it. Have you still issues accessing our svn repository ? If not, i think you could easily reproduce the issue.

This is the failing regression test:

</test-code>
<code-fragment id="executeQuery"><![CDATA[

import java.sql.*;
public class Foo {
void bar() {
Statement c = pool.getStmt();
Statement st = c.executeQuery("SELECT * FROM FOO");
try {
} finally {
MyHelper.close(c);
cleanup(st);
}
}
}
]]>
<test-code reinitializerule="true">
<description></description>
<rule-property name="closeTargets">MyHelper.close, cleanup</rule-property>
<rule-property name="types">Connection,Statement,ResultSet</rule-property>
<expected-problems>0</expected-problems>
<code-ref id="user-content-executeQuery">
</code-ref></test-code>

[Conrad]

Logged In: YES
user_id=2086301
Originator: YES

Your test case fails regression test because there is whitespace before "cleanup" in the closeTargets property value. Either that can be removed from the test case, or, if whitespace is supposed to be allowed between comma-separated values in a list property value, then adding ".trim()" after "st.nextToken()" (two places) will fix that.

But I'm not sure why you changed expected-problems from 0 to 1 for my "this.closeConnection" test case. That test case produces one problem using the "old" CloseResource.java, but the expected zero problems using my new rule code. Or am I not understanding the use of expected-problems?

BTW, I didn't update the description of my "this.closeConnection" test case -- it should be something like:
Ok, closeTargets properties specifies a method with "this" prefix.

[Romain PELISSE]

Logged In: YES
user_id=1679130
Originator: NO

"Your test case fails regression test because there is whitespace before
"cleanup" in the closeTargets property value. Either that can be removed
from the test case, or, if whitespace is supposed to be allowed between
comma-separated values in a list property value, then adding ".trim()"
after "st.nextToken()" (two places) will fix that."

Ok. I feel silly. Well, at least, this is fixed !

"But I'm not sure why you changed expected-problems from 0 to 1 for my
"this.closeConnection" test case. That test case produces one problem using
the "old" CloseResource.java, but the expected zero problems using my new
rule code. Or am I not understanding the use of expected-problems?"

I think i got confused over there... Forget it.

"BTW, I didn't update the description of my "this.closeConnection" test
case -- it should be something like:
Ok, closeTargets properties specifies a method with "this" prefix."

Ok, done.

I've committed all your changes however, the very last test case is still failing:

http://pmd.svn.sourceforge.net/viewvc/pmd/trunk/pmd/regress/test/net/sourceforge/pmd/rules/design/xml/CloseResource.xml?view=log

Would be very nice of you, if you could take a look :)

Here is the current CloseResource class (merged between the recently modified trunk and your submission):

http://pmd.svn.sourceforge.net/viewvc/pmd/trunk/pmd/src/net/sourceforge/pmd/lang/java/rule/design/CloseResourceRule.java?view=markup

[Conrad]

Logged In: YES
user_id=2086301
Originator: YES

I'm not sure which test case you mean by "the very last test case". Looking at revision 6250 of CloseResource.xml that you directed me to, the content of the code element in last test case (lines 252-267) passes my rule with no problems (but beware that I'm still based off of PMD 4.1). If that test case doesn't pass the PMD5 trunk's CloseResourceRule.java, then I don't know what could be wrong -- something different between PMD4's findChildrenOfType() and PMD5's findDescendantsOfType()?

However, I did notice that the test case on lines 230-250 incorrectly states expected-problems of 1 -- it should be 0. Is that the test case you were referring to? And the description of that test case (line 232) should probably be changed to:
[1964798] 3 bugs in CloseResourceRule : object is passed to close conection method with "this" prefix

I apologize for not being able to use SVN and thank you for upgrading my proposed changes to fit with the 5.0 API and architecture.

[Romain PELISSE]

Logged In: YES
user_id=1679130
Originator: NO

Hi conrad,

Thanks for answer.

"However, I did notice that the test case on lines 230-250 incorrectly states expected-problems of 1 -- it should be 0. " => yes you're right i remove this extra test, it wasn't necessary at all (again, a slight confusion from my part).

Yes, i had to modify a little bit your code, to make it consistent with recent changes in pmd-trunk.

I made a zip of the current trunk available to you here : http://belaran.eu/wordpress/pmd-trunk.zip, so maybe you can directly see what is wrong here.

[Shahzad Masud]

A different scenario:

types = Connection, ResultSet, PreparedStatement
closeTargets=returnConnection, returnStatement, returnResultset

public void Conn ( ) throws Exception {
Connection c = null ;
ResultSet rs = null ;
PreparedStatement stmt = null ;
try {
c = DatabaseHandler.getConnection(getClass().getName()) ;
stmt = c.prepareStatement("") ;
rs = stmt.executeQuery() ;
if ( rs.next() ) {

    }
} catch (Exception e) {
}
finally {
    DatabaseHandler.returnResultset(rs, getClass().getName()) ;
    DatabaseHandler.returnStatement(stmt, getClass().getName()) ;
    DatabaseHandler.returnConnection(c, getClass().getName()) ;
}

}
Working okay, as expected. Let me change the scenario a little bit.

types = Connection, ResultSet, PreparedStatement
closeTargets=returnConnection

public void Conn ( ) throws Exception {
Connection c = null ;
ResultSet rs = null ;
PreparedStatement stmt = null ;
try {
c = DatabaseHandler.getConnection(getClass().getName()) ;
stmt = c.prepareStatement("") ;
rs = stmt.executeQuery() ;
if ( rs.next() ) {

    }
} catch (Exception e) {
}
finally {
    // DatabaseHandler.returnResultset(rs, getClass().getName()) ;
    // DatabaseHandler.returnStatement(stmt, getClass().getName()) ;
    DatabaseHandler.returnConnection(c, getClass().getName()) ;
}

}

It should return an error that resultset & statement objects are not closing. But it not returning any error.

[Conrad]

Responding to shahzadmasud's comment:
In the version of CloseResource that I fixed, not only are rs and stmt correctly flagged as not being closed in your second example, but all 3 variables in BOTH of your examples are correctly flagged as not being closed, because having closeTargets=returnConnection should really be closeTargets=DatabaseHandler.returnConnection, etc.

[Shahzad Masud]

Please share the your version of PMD plugin (compiled with your fix of CloseResource). I am having problems while compiling the plugin.

[Romain PELISSE]

Well sounds to me that shahzadmasud and Conrad stop discussing on this issue, so let's close it now!

[Shantanoo]

Hi all,

I'm facing the same problem. I'm using method

:::java
public static void close(Statement stmt)
:::java

from class
:::java
public final class DbUtils
:::java

in the given commons-dbutils-1.2 jar.

When I'm running a PMD scan, the statement is shown not getting closed in the report.

However, it is already closed.

I came through the above discussion, but could not figure what was the final outcome. Please help.

[Andreas Dangel]

Can you verify, that you have configured the rule property "closeTargets"? I think, you'll need to set it to "DbUtils.close" so that the rule recognizes this as a call to something, that closes your resource.

See also http://pmd.sourceforge.net/pmd-5.1.2/rules/java/design.html#CloseResource and http://pmd.sourceforge.net/pmd-5.1.2/howtomakearuleset.html