pmd: fix #942 CheckResultSet False Positive

* replacing the xpath rule with a java based rule implementation

pmd/etc/changelog.txt

@@ -1,5 +1,6 @@
 ????? ??, 2013 - 5.0.3:
 
+Fixed bug  942: CheckResultSet False Positive and Negative
 Fixed bug  943: PreserveStackTrace false positive if a StringBuffer exists
 Fixed bug  945: PMD generates RuleSets it cannot read.
 Fixed bug  958: Intermittent NullPointerException while loading XPath node attributes

pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/basic/CheckResultSetRule.java

@@ -0,0 +1,81 @@
+/**
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+package net.sourceforge.pmd.lang.java.rule.basic;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+import net.sourceforge.pmd.lang.ast.Node;
+import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType;
+import net.sourceforge.pmd.lang.java.ast.ASTIfStatement;
+import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration;
+import net.sourceforge.pmd.lang.java.ast.ASTName;
+import net.sourceforge.pmd.lang.java.ast.ASTType;
+import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclarator;
+import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
+import net.sourceforge.pmd.lang.java.ast.ASTWhileStatement;
+import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule;
+
+/**
+ * Rule that verifies, that the return values of next(), first(), last(), etc.
+ * calls to a java.sql.ResultSet are actually verified.
+ *
+ */
+public class CheckResultSetRule extends AbstractJavaRule {
+
+    private Map<String, Node> resultSetVariables = new HashMap<String, Node>();
+    
+    private static Set<String> methods = new HashSet<String>();
+    static {
+        methods.add(".next");
+        methods.add(".previous");
+        methods.add(".last");
+        methods.add(".first");
+    }
+    
+    @Override
+    public Object visit(ASTLocalVariableDeclaration node, Object data) {
+        ASTClassOrInterfaceType type = node.getFirstChildOfType(ASTType.class).getFirstDescendantOfType(ASTClassOrInterfaceType.class);
+        if (type != null
+                && ((type.getType() != null && "java.sql.ResultSet".equals(type.getType().getName())) || "ResultSet"
+                        .equals(type.getImage()))) {
+            ASTVariableDeclarator declarator = node.getFirstChildOfType(ASTVariableDeclarator.class);
+            if (declarator != null) {
+                ASTName name = declarator.getFirstDescendantOfType(ASTName.class);
+                if (name != null && name.getImage().endsWith("executeQuery")) {
+
+                    ASTVariableDeclaratorId id = declarator.getFirstChildOfType(ASTVariableDeclaratorId.class);
+                    resultSetVariables.put(id.getImage(), node);
+                }
+            }
+        }
+        return super.visit(node, data);
+    }
+
+    @Override
+    public Object visit(ASTName node, Object data) {
+        String image = node.getImage();
+        String var = getResultSetVariableName(image);
+        if (var != null && resultSetVariables.containsKey(var)
+            && node.getFirstParentOfType(ASTIfStatement.class)== null
+            && node.getFirstParentOfType(ASTWhileStatement.class) == null) {
+
+            addViolation(data, resultSetVariables.get(var));
+        }
+        return super.visit(node, data);
+    }
+
+    private String getResultSetVariableName(String image) {
+        if (image.contains(".")) {
+            for (String method : methods) {
+                if (image.endsWith(method)) {
+                    return image.substring(0, image.lastIndexOf(method));
+                }
+            }
+        }
+        return null;
+    }
+}

pmd/src/main/resources/rulesets/java/basic.xml

@@ -553,7 +553,7 @@ public class Foo {
   <rule name="CheckResultSet"
     	language="java"
         since="4.1"
-        class="net.sourceforge.pmd.lang.rule.XPathRule"
+        class="net.sourceforge.pmd.lang.java.rule.basic.CheckResultSetRule"
         message="Always check the return of one of the navigation method (next,previous,first,last) of a ResultSet."
         externalInfoUrl="${pmd.website.baseurl}/rules/java/basic.html#CheckResultSet">
         <description>
@@ -563,47 +563,6 @@ If the value return is 'false', it should be handled properly.
             ]]>
         </description>
         <priority>3</priority>
-        <properties>
-            <property name="xpath">
-                <value>
-		        	<![CDATA[
-//Type/ReferenceType/ClassOrInterfaceType[
-        (@Image = 'ResultSet')
-        and
-        (../../../descendant::Name[ends-with(@Image,'executeQuery')])
-        and
-        (
-	(not (contains(
-                        (./ancestor::Block/descendant::WhileStatement/descendant::Name/attribute::Image),
-                        concat(../../../VariableDeclarator/VariableDeclaratorId/attribute::Image,'.next')
-		)  ) )
-	and ( not ( contains(
-                        (./ancestor::Block/descendant::IfStatement/descendant::Name/attribute::Image),
-                        concat(../../../VariableDeclarator/VariableDeclaratorId/attribute::Image,'.next')
-		) ) )
-	and (not (contains(
-                        (./ancestor::Block/descendant::WhileStatement/descendant::Name/attribute::Image),
-                        concat(../../../VariableDeclarator/VariableDeclaratorId/attribute::Image,'.previous')
-		)  ) )
-	and ( not ( contains(
-                        (./ancestor::Block/descendant::IfStatement/descendant::Name/attribute::Image),
-                        concat(../../../VariableDeclarator/VariableDeclaratorId/attribute::Image,'.previous')
-		) ) )
-	and ( not ( contains(
-                        (./ancestor::Block/descendant::IfStatement/descendant::Name/attribute::Image),
-                        concat(../../../VariableDeclarator/VariableDeclaratorId/attribute::Image,'.last')
-		) ) )
-	and ( not ( contains(
-                        (./ancestor::Block/descendant::IfStatement/descendant::Name/attribute::Image),
-                        concat(../../../VariableDeclarator/VariableDeclaratorId/attribute::Image,'.first')
-		) ) )
-
-         )
-]
-		        	]]>
-            	</value>
-            </property>
-        </properties>
         <example>
             <![CDATA[
 Statement stat = conn.createStatement();

pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/basic/xml/CheckResultSet.xml

@@ -89,6 +89,10 @@
 					public void executeSql(Statement statement, String query) throws SQLException
 				    {
 				        ResultSet rst = stat.executeQuery("SELECT name FROM person");
+        // stupid while loop to have an unrelated while
+        while (_count > 0) {
+            _count--;
+        }
 				        while (rst.next())
 				        {
 				            firsString = rst.getString(1);
@@ -98,4 +102,63 @@
      		]]>
  		</code>
     </test-code>
+
+    <test-code>
+        <description>#942 CheckResultSet False Positive</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+public class Test {
+    public int countReadOnlyForwardOnlyJDBC() throws SQLException, ClassNotFoundException {
+        int _count = 0;
+
+        if (conn == null) {
+            conn = getConnection();
+            if (conn == null) {
+                return null;
+            }
+        }
+
+        if (stmt == null) {
+            stmt = conn.createStatement(
+                ResultSet.TYPE_FORWARD_ONLY, 
+                ResultSet.CONCUR_READ_ONLY);
+        }
+
+        ResultSet _rs = stmt.executeQuery(QueryString);
+
+        while (_rs.next() != false) {
+            _count++;
+        }   
+
+        return _count;
+    }
+    
+
+    public int countReadOnlyForwardOnlyJDBC2() throws SQLException, ClassNotFoundException {
+        int _count = 0;
+
+        if (conn == null) {
+            conn = getConnection();
+            if (conn == null) {
+                return null;
+            }
+        }
+
+        if (stmt == null) {
+            stmt = conn.createStatement(
+                ResultSet.TYPE_FORWARD_ONLY, 
+                ResultSet.CONCUR_READ_ONLY);
+        }
+
+        ResultSet _rs = stmt.executeQuery(QueryString);
+        _rs.next();  // This line should throw a PMD violation.  < - - - violation here
+        while (_rs.next() != false) {
+            _count++;
+        }   
+
+        return _count;
+    }
+}
+        ]]></code>
+    </test-code>
 </test-data>