fix for rule MinimizeAttributesInSession #32

jborgers · Apr 9, 2020 · e9e0a27 · e9e0a27
1 parent e60e410
commit e9e0a27
Show file tree

Hide file tree

Showing 4 changed files with 166 additions and 82 deletions.
diff --git a/pom.xml b/pom.xml
@@ -72,6 +72,20 @@
             <artifactId>javax.xml.soap-api</artifactId>
             <version>1.4.0</version>
         </dependency>
+        <!-- https://mvnrepository.com/artifact/javax.servlet/servlet-api -->
+        <dependency>
+            <groupId>javax.servlet</groupId>
+            <artifactId>servlet-api</artifactId>
+            <version>2.5</version>
+            <scope>provided</scope>
+        </dependency>
+        <!-- https://mvnrepository.com/artifact/javax.portlet/portlet-api -->
+        <dependency>
+            <groupId>javax.portlet</groupId>
+            <artifactId>portlet-api</artifactId>
+            <version>3.0.1</version>
+            <scope>provided</scope>
+        </dependency>
     </dependencies>
 
 </project>
diff --git a/rulesets/java/jpinpoint-rules.xml b/rulesets/java/jpinpoint-rules.xml
@@ -679,34 +679,49 @@ and not(ancestor::MethodDeclaration//TryStatement/FinallyStatement//StatementExp
             (jpinpoint-rules)</description>
         <priority>2</priority>
         <properties>
-            <property name="version" value="1.0"/>
+            <property name="version" value="2.0"/>
             <property name="xpath">
                 <value><![CDATA[
-//ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration//Block/BlockStatement/Statement[
-./StatementExpression/PrimaryExpression/PrimarySuffix[ends-with(@Image, 'setAttribute')]
-/ancestor::ClassOrInterfaceBodyDeclaration//ClassOrInterfaceBody[not(
-ClassOrInterfaceBodyDeclaration/MethodDeclaration//Block/BlockStatement/Statement/StatementExpression/PrimaryExpression/PrimarySuffix[ends-with(@Image, 'removeAttribute')]
-or
-ClassOrInterfaceBodyDeclaration/MethodDeclaration//Block/BlockStatement/Statement/StatementExpression/PrimaryExpression/PrimaryPrefix/Name[ends-with(@Image, 'removeAttribute')]
-)]]
-|
-//ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration//Block/BlockStatement/Statement[
-./StatementExpression/PrimaryExpression/PrimaryPrefix/Name[ends-with(@Image, 'setAttribute')]
-/ancestor::ClassOrInterfaceBodyDeclaration//ClassOrInterfaceBody[not(
-ClassOrInterfaceBodyDeclaration/MethodDeclaration//Block/BlockStatement/Statement/StatementExpression/PrimaryExpression/PrimarySuffix[ends-with(@Image, 'removeAttribute')]
-or
-ClassOrInterfaceBodyDeclaration/MethodDeclaration//Block/BlockStatement/Statement/StatementExpression/PrimaryExpression/PrimaryPrefix/Name[ends-with(@Image, 'removeAttribute')]
+//ClassOrInterfaceBodyDeclaration/MethodDeclaration[../..//(VariableDeclaratorId |ReturnStatement/Expression)[pmd-java:typeIs('javax.portlet.PortletSession') or pmd-java:typeIs('javax.servlet.http.HttpSession')]]
+/Block/BlockStatement/Statement[
+./StatementExpression/PrimaryExpression/(PrimaryPrefix/Name|PrimarySuffix)[ends-with(@Image, 'setAttribute')]
+/ancestor::ClassOrInterfaceBody[not(
+ClassOrInterfaceBodyDeclaration//MethodDeclaration/Block//PrimaryExpression/(PrimaryPrefix/Name|PrimarySuffix)[ends-with(@Image, 'removeAttribute')]
 )]]
-|
-//ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration//Block/BlockStatement/Statement[
+,
+//ClassOrInterfaceBodyDeclaration/MethodDeclaration/Block/BlockStatement/Statement[
 .//StatementExpression/PrimaryExpression/PrimaryPrefix/Name[@Image = 'PortletUtils.setSessionAttribute']
-/ancestor::ClassOrInterfaceBodyDeclaration//ClassOrInterfaceBody[not(
-ClassOrInterfaceBodyDeclaration/MethodDeclaration//Block/BlockStatement//Statement/StatementExpression/PrimaryExpression/PrimaryPrefix/Name[@Image = 'PortletUtils.setSessionAttribute']
+/ancestor::ClassOrInterfaceBody[not(
+ClassOrInterfaceBodyDeclaration//MethodDeclaration/Block//PrimaryExpression/PrimaryPrefix/Name[@Image = 'PortletUtils.setSessionAttribute']
 /../../PrimarySuffix/Arguments/ArgumentList/Expression/PrimaryExpression/PrimaryPrefix/Literal/NullLiteral
 )]]
-	]]></value>
+]]></value>
             </property>
         </properties>
+        <example>
+            <![CDATA[
+class Bad {
+    public void setAttribute(String name, Object obj) {
+	    getPortletSession().setAttribute(name, obj);                 // bad
+	}
+	public Object getAttribute(String sessionAttributeName) {
+		return getPortletSession().getAttribute(sessionAttributeName);
+	}
+}
+
+class Good {
+	public void setAttribute(String name, final Object obj) {
+		getPortletSession().setAttribute(name, obj);
+	}
+	public Object getAttribute(String sessionAttributeName) {
+		return getPortletSession().getAttribute(sessionAttributeName);
+	}
+	public void removeAttribute(String name) {
+		getPortletSession().removeAttribute(name);
+	}
+}
+            ]]>
+        </example>
     </rule>
 
     <rule name="ObjectMapperCreatedForEachMethodCall" message="An ObjectMapper is created for each method call, which is expensive." class="net.sourceforge.pmd.lang.rule.XPathRule" deprecated="false" dfa="false" language="java"   typeResolution="true" externalInfoUrl="http://www.jpinpoint.com/doc/Java+Code+Performance#JavaCodePerformance-IUOJAR01">

diff --git a/src/main/resources/category/java/common.xml b/src/main/resources/category/java/common.xml
@@ -651,34 +651,51 @@ and not(ancestor::MethodDeclaration//TryStatement/FinallyStatement//StatementExp
             (jpinpoint-rules)</description>
         <priority>2</priority>
         <properties>
-            <property name="version" value="1.0"/>
+            <property name="version" value="2.0"/>
             <property name="xpath">
                 <value><![CDATA[
-//ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration//Block/BlockStatement/Statement[
-./StatementExpression/PrimaryExpression/PrimarySuffix[ends-with(@Image, 'setAttribute')]
-/ancestor::ClassOrInterfaceBodyDeclaration//ClassOrInterfaceBody[not(
-ClassOrInterfaceBodyDeclaration/MethodDeclaration//Block/BlockStatement/Statement/StatementExpression/PrimaryExpression/PrimarySuffix[ends-with(@Image, 'removeAttribute')]
-or
-ClassOrInterfaceBodyDeclaration/MethodDeclaration//Block/BlockStatement/Statement/StatementExpression/PrimaryExpression/PrimaryPrefix/Name[ends-with(@Image, 'removeAttribute')]
-)]]
-|
-//ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration//Block/BlockStatement/Statement[
-./StatementExpression/PrimaryExpression/PrimaryPrefix/Name[ends-with(@Image, 'setAttribute')]
-/ancestor::ClassOrInterfaceBodyDeclaration//ClassOrInterfaceBody[not(
-ClassOrInterfaceBodyDeclaration/MethodDeclaration//Block/BlockStatement/Statement/StatementExpression/PrimaryExpression/PrimarySuffix[ends-with(@Image, 'removeAttribute')]
-or
-ClassOrInterfaceBodyDeclaration/MethodDeclaration//Block/BlockStatement/Statement/StatementExpression/PrimaryExpression/PrimaryPrefix/Name[ends-with(@Image, 'removeAttribute')]
+//ClassOrInterfaceBodyDeclaration/MethodDeclaration[../..//(VariableDeclaratorId |ReturnStatement/Expression)[pmd-java:typeIs('javax.portlet.PortletSession') or pmd-java:typeIs('javax.servlet.http.HttpSession')]]
+/Block/BlockStatement/Statement[
+./StatementExpression/PrimaryExpression/(PrimaryPrefix/Name|PrimarySuffix)[ends-with(@Image, 'setAttribute')]
+/ancestor::ClassOrInterfaceBody[not(
+ClassOrInterfaceBodyDeclaration//MethodDeclaration/Block//PrimaryExpression/(PrimaryPrefix/Name|PrimarySuffix)[ends-with(@Image, 'removeAttribute')]
 )]]
-|
-//ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration//Block/BlockStatement/Statement[
+,
+//ClassOrInterfaceBodyDeclaration/MethodDeclaration/Block/BlockStatement/Statement[
 .//StatementExpression/PrimaryExpression/PrimaryPrefix/Name[@Image = 'PortletUtils.setSessionAttribute']
-/ancestor::ClassOrInterfaceBodyDeclaration//ClassOrInterfaceBody[not(
-ClassOrInterfaceBodyDeclaration/MethodDeclaration//Block/BlockStatement//Statement/StatementExpression/PrimaryExpression/PrimaryPrefix/Name[@Image = 'PortletUtils.setSessionAttribute']
+/ancestor::ClassOrInterfaceBody[not(
+ClassOrInterfaceBodyDeclaration//MethodDeclaration/Block//PrimaryExpression/PrimaryPrefix/Name[@Image = 'PortletUtils.setSessionAttribute']
 /../../PrimarySuffix/Arguments/ArgumentList/Expression/PrimaryExpression/PrimaryPrefix/Literal/NullLiteral
 )]]
-	]]></value>
+]]></value>
             </property>
         </properties>
+        <example>
+            <![CDATA[
+class Bad {
+    PortletSession session; // same for HttpSession
+    public void setAttribute(String name, Object obj) {
+	    session.setAttribute(name, obj);                 // bad
+	}
+	public Object getAttribute(String name) {
+		return session.getAttribute(name);
+	}
+}
+
+class Good {
+    PortletSession session; // same for HttpSession
+	public void setAttribute(String name, final Object obj) {
+		session.setAttribute(name, obj);
+	}
+	public Object getAttribute(String name) {
+		return session.getAttribute(name);
+	}
+	public void removeAttribute(String name) {
+		session.removeAttribute(name);
+	}
+}
+            ]]>
+        </example>
     </rule>
 
     <rule name="ObjectMapperCreatedForEachMethodCall" message="An ObjectMapper is created for each method call, which is expensive." class="net.sourceforge.pmd.lang.rule.XPathRule" deprecated="false" dfa="false" language="java"   typeResolution="true" externalInfoUrl="http://www.jpinpoint.com/doc/Java+Code+Performance#JavaCodePerformance-IUOJAR01">

diff --git a/...resources/com/jpinpoint/perf/lang/java/ruleset/common/xml/MinimizeAttributesInSession.xml b/...resources/com/jpinpoint/perf/lang/java/ruleset/common/xml/MinimizeAttributesInSession.xml
@@ -5,54 +5,50 @@
         xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd">
     <test-code>
         <description>Atributes in session objects take up heap space</description>
-        <expected-problems>5</expected-problems>
+        <expected-problems>6</expected-problems>
+        <expected-linenumbers>11, 41, 70, 99, 103, 161</expected-linenumbers>
         <code><![CDATA[
-import org.springframework.web.context.request.RequestAttributes;
-import org.springframework.web.context.request.RequestContextHolder;
-import org.springframework.web.portlet.context.PortletRequestAttributes;
-import org.springframework.web.portlet.util.PortletUtils;
-
 import javax.portlet.PortletSession;
 import javax.portlet.ResourceRequest;
+import javax.servlet.http.HttpSession;
+import org.springframework.web.portlet.util.PortletUtils;
 
-/**
- * If a .setAttribute occurs in a method in a certain class, a .removeAttribute must occur in a method of the same class
- */
 public class MinimizeAttributesInSessionTest {
+	private PortletSession ps;
 
-
-	static class Test1_Flag {
-		public void setAttribute(final String sessionAttributeName,
-				final Object object) {
-			getPortletSession().setAttribute(sessionAttributeName, object);
+	class Bad {
+		public void setAttribute(final String name, final Object obj) {
+			getPortletSession().setAttribute(name, obj);                 // bad
 		}
-
 		public Object getAttribute(final String sessionAttributeName) {
 			return getPortletSession().getAttribute(sessionAttributeName);
 		}
-
-		public void removeAttribute(final String sessionAttributeName) {
-			// getPortletSession().removeAttribute(sessionAttributeName);
-		}
+	        PortletSession getPortletSession() {
+		    return ps;
+	        }
 	}
 
-	static class Test1_Ok {
-		public void setAttribute(final String sessionAttributeName,
-				final Object object) {
-			getPortletSession().setAttribute(sessionAttributeName, object);
+	class Good {
+		public void setAttribute(final String name, final Object obj) {
+			getPortletSession().setAttribute(name, obj);
 		}
-
-		public void removeAttribute(final String sessionAttributeName) {
-			getPortletSession().removeAttribute(sessionAttributeName);
+		public Object getAttribute(final String sessionAttributeName) {
+			return getPortletSession().getAttribute(sessionAttributeName);
+		}
+		public void removeAttribute(final String name) {
+			getPortletSession().removeAttribute(name);
 		}
+                PortletSession getPortletSession() {
+		    return ps;
+	        }
 	}
 
 	static class Test2_Flag {
 		PortletSession ses;
 
 		public void setAttribute(final String sessionAttributeName,
 				final Object object) {
-			ses.setAttribute(sessionAttributeName, object);
+			ses.setAttribute(sessionAttributeName, object);         // bad
 		}
 
 		public Object getAttribute(final String sessionAttributeName) {
@@ -77,11 +73,11 @@ public class MinimizeAttributesInSessionTest {
 		}
 	}
 
-	static class Test3_Flag {
+	 class Test3_Flag {
 
 		public void setAttribute(final String sessionAttributeName,
 				final Object object) {
-			getPortletSession().setAttribute(sessionAttributeName, object);
+			getPortletSession().setAttribute(sessionAttributeName, object); // bad
 		}
 
 		public void removeAttributes(final String... sessionAttributeNames) {
@@ -92,7 +88,7 @@ public class MinimizeAttributesInSessionTest {
 		}
 	}
 
-	static class Test3_Ok {
+	 class Test3_Ok {
 
 		public void setAttribute(final String sessionAttributeName,
 				final Object object) {
@@ -107,14 +103,14 @@ public class MinimizeAttributesInSessionTest {
 		}
 	}
 
-	static class Test4_Flag {
+	 class Test4_Flag {
 
 		private void setUploadStatus(final ResourceRequest request) {
-			PortletUtils.setSessionAttribute(request, "status", "value");
+			PortletUtils.setSessionAttribute(request, "status", "value"); // bad
 		}
 
 		public void removeUploadStatus(final ResourceRequest request) {
-			PortletUtils.setSessionAttribute(request, "status", "");
+			PortletUtils.setSessionAttribute(request, "status", ""); // bad
 		}
 	}
 
@@ -129,15 +125,16 @@ public class MinimizeAttributesInSessionTest {
 		}
 	}
 
-	// TODO: case: /*(ActionRequest)*/ request.getPortletSession().setAttribute(SIGN_SPARKLE_SESSION_KEY, signSparkle);
 
-	/**
-	 * Get the {@link PortletSession} from the current request.
-	 *
-	 * @return {@link PortletSession}
-	 * @see org.springframework.web.portlet.context.PortletApplicationContextUtils
-	 */
-	public static PortletSession getPortletSession() {
+    // TODO: case: /*(ActionRequest)*/ request.getPortletSession().setAttribute(SIGN_SPARKLE_SESSION_KEY, signSparkle);
+
+    /**
+     * Get the {@link PortletSession} from the current request.
+     *
+     * @return {@link PortletSession}
+     * @see org.springframework.web.portlet.context.PortletApplicationContextUtils
+     */
+/*	public static PortletSession getPortletSession() {
 		final RequestAttributes requestAttr = RequestContextHolder
 				.currentRequestAttributes();
 		if (!(requestAttr instanceof PortletRequestAttributes)) {
@@ -146,9 +143,50 @@ public class MinimizeAttributesInSessionTest {
 		}
 		return ((PortletRequestAttributes) requestAttr).getRequest()
 				.getPortletSession();
+	}*/
+
+}
+ class Test5_Ok {
+	public void setAttribute(final String sessionAttributeName,
+							 final Object object) {
+		getOther().setAttribute(sessionAttributeName, object); // good, no [Http/Portlet]Session]
+	}
+
+	public Object getAttribute(final String sessionAttributeName) {
+		return getOther().getAttribute(sessionAttributeName);
 	}
 
+	Other getOther() {
+		return new Other();
+	}
+}
+ class Other {
+	void setAttribute(String n, Object o) {}
+	Object getAttribute(String n) {}
+}
+
+class Bad2 {
+	private HttpSession hs;
+	public void setAttribute(final String name, final Object obj) {
+		hs.setAttribute(name, obj);                 // bad
+	}
+	public Object getAttribute(final String sessionAttributeName) {
+		return hs.getAttribute(sessionAttributeName);
+	}
+}
+
+class Good2 {
+	private HttpSession hs;
+	public void setAttribute(final String name, final Object obj) {
+		hs.setAttribute(name, obj); // good
+	}
+	public Object getAttribute(final String sessionAttributeName) {
+		return hs.getAttribute(sessionAttributeName);
+	}
+	public void removeAttribute(final String name) {
+		hs.removeAttribute(name);
+	}
 }
-     ]]></code>
+]]></code>
     </test-code>
 </test-data>