Merge pull request #295 from jborgers/kotlin/AvoidRecompilingXPathExp…

…ression AvoidRecompilingXPathExpression for Kotlin
jborgers · Mar 14, 2024 · 22eec3b · 22eec3b
2 parents ac8957a + aca86ac
commit 22eec3b
Show file tree

Hide file tree

Showing 5 changed files with 229 additions and 6 deletions.
diff --git a/rulesets/kotlin/jpinpoint-kotlin-rules.xml b/rulesets/kotlin/jpinpoint-kotlin-rules.xml
@@ -11,6 +11,58 @@
 <!-- IMPORTANT NOTICE: The content of this file is generated. Do not edit this file directly since changes may be lost when this file is regenerated! -->
 
 <!-- BEGIN Included file 'common.xml' -->
+    <rule name="AvoidDecimalAndChoiceFormatAsField"
+          since="7.0"
+          language="kotlin"
+          message="Avoid using NumberFormat, DecimalFormat or ChoiceFormat as field since it is thread-unsafe."
+          class="net.sourceforge.pmd.lang.rule.XPathRule"
+          typeResolution="true"
+          externalInfoUrl="https://github.com/jborgers/PMD-jPinpoint-rules/tree/master/docs/JavaCodePerformance.md#isio01">
+        <description>
+            Problem: java.text.DecimalFormat and java.text.ChoiceFormat are thread-unsafe.
+            Solution: usual solution is to create a new local one when needed in a method.
+            (jpinpoint-rules)
+        (jpinpoint-kotlin-rules)</description>
+        <priority>1</priority>
+        <properties>
+            <property name="tag" value="jpinpoint-rule" type="String" description="for-sonar"/>
+            <property name="version" value="3.1"/>
+            <property name="xpath">
+                <value><![CDATA[
+(: check if java.text imports exists as filter for same named classes in other packages :)
+//ImportHeader[.//T-Identifier[@Text='java'] and .//T-Identifier[@Text='text']]/ancestor::KotlinFile
+(: PrimaryExpression finds the constructors instead of type declarations :)
+(: filter matches in functions :)
+//ClassDeclaration//PrimaryExpression//SimpleIdentifier//T-Identifier[
+ (@Text="DecimalFormat" or @Text="ChoiceFormat")
+ and not(ancestor::FunctionDeclaration)
+]
+]]>
+                </value>
+            </property>
+        </properties>
+        <example><![CDATA[
+class Foo {
+
+    companion object {
+        val NUMBER_FORMAT: DecimalFormat = DecimalFormat("###.###") // bad
+    }
+
+    private val numFormat: NumberFormat = DecimalFormat("###.###") //bad
+
+    private val limits = doubleArrayOf(1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0)
+    private val dayOfWeekNames = arrayOf("Sun", "Mon", "Tue", "Wed", "Thur", "Fri", "Sat")
+    private val form = ChoiceFormat(limits, dayOfWeekNames) // bad
+
+    fun shouldNotMatchInsideMethod() {
+        val format: NumberFormat = DecimalFormat("##.##")
+        val choiceFormat = ChoiceFormat(limits, dayOfWeekNames)
+    }
+}
+]]>
+        </example>
+    </rule>
+
     <rule name="AvoidInMemoryStreamingDefaultConstructor"
           since="7.0"
           language="kotlin"
@@ -29,7 +81,7 @@
             <property name="version" value="3.1"/>
             <property name="xpath">
                 <value><![CDATA[
-//ImportHeader[.//T-Identifier[@Text='java'] and .//T-Identifier[@Text='io']]/ancestor::KotlinFile
+//ImportHeader[.//T-Identifier[@Text='java'] and .//T-Identifier[@Text='io']][1]/ancestor::KotlinFile
 //Expression//T-Identifier[(@Text='ByteArrayOutputStream' and ../../../
 PostfixUnarySuffix//ValueArguments[not(ValueArgument) or ValueArgument//T-IntegerLiteral[number(@Text)<=32]])
 or (@Text='StringWriter' and ../../../
@@ -57,6 +109,54 @@ class AvoidInMemoryStreamingDefaultConstructor {
         </example>
     </rule>
 
+    <rule name="AvoidRecompilingXPathExpression" class="net.sourceforge.pmd.lang.rule.XPathRule"
+          language="kotlin"
+          message="XPathExpression is created and compiled every time. Beware it is thread-unsafe."
+          typeResolution="true"
+          externalInfoUrl="https://github.com/jborgers/PMD-jPinpoint-rules/tree/master/docs/JavaCodePerformance.md#ux02">
+        <description>XPathExpression is created and compiled on every method call, compiled possibly implicitly by XPath::evaluate.
+            Problem: Creation of XPath and compilation of XPathExpression takes time. It may slow down your application. &#13;
+            Solution: 1. Avoid XPath usage. 2. Compile the xpath expression as String into a XPathExpression. However, since XPath and XPathExpression classes are thread-unsafe, they are not easily cached. Caching it in a ThreadLocal may be a solution (sure? - TODO) .
+            (jpinpoint-rules)</description>
+        <priority>2</priority>
+        <properties>
+            <property name="tag" value="jpinpoint-rule" type="String" description="for-sonar"/>
+            <property name="version" value="2.0"/>
+            <property name="xpath">
+                <value><![CDATA[
+//ImportHeader[.//T-Identifier[@Text='javax'] and .//T-Identifier[@Text='xml'] and .//T-Identifier[@Text='xpath']]/ancestor::KotlinFile
+//FunctionBody//T-Identifier[@Text='newXPath']
+/ancestor::FunctionBody//T-Identifier[@Text='compile'
+or @Text='evaluate' and ancestor::PostfixUnarySuffix/..//ValueArguments[count(./ValueArgument) = 3]]
+(: note on limitation, the 2-argument version is not found, difficult to do the typeIs checking like in Java :)
+]]></value>
+            </property>
+        </properties>
+        <example>
+            <![CDATA[
+import org.w3c.dom.Document
+import org.w3c.dom.NodeList
+import javax.xml.xpath.*
+
+class AvoidRecompilingXPathExpressionKotlin {
+    fun bad1(doc: Document?): NodeList {
+        val xpath: XPath = XPathFactory.newInstance().newXPath()
+        val expr: XPathExpression = xpath.compile("//book[author='Isaac Asimov']/title/text()") // bad
+        return expr.evaluate(doc, XPathConstants.NODESET) as NodeList
+    }
+
+    @Throws(XPathExpressionException::class)
+    fun bad2(doc: Document?): NodeList {
+        val xpath: XPath = XPathFactory.newInstance().newXPath()
+        val xPathQuery = "//book[author='Isaac Asimov']/title/text()"
+        return xpath.evaluate(xPathQuery, doc, XPathConstants.NODESET) as NodeList // bad
+    }
+}
+// TODO Good example
+            ]]>
+        </example>
+    </rule>
+
     <rule name="AvoidStringBuffer" class="net.sourceforge.pmd.lang.rule.XPathRule"
           since="7.0"
           language="kotlin"

diff --git a/src/main/resources/category/kotlin/common.xml b/src/main/resources/category/kotlin/common.xml
@@ -17,7 +17,6 @@
         <priority>2</priority>
         <properties>
             <property name="tag" value="jpinpoint-rule" type="String" description="for-sonar"/>
-            <property name="version" value="3.1"/>
             <property name="xpath">
                 <value><![CDATA[
 //ImportHeader[.//T-Identifier[@Text='java'] and .//T-Identifier[@Text='io']][1]/ancestor::KotlinFile
@@ -59,7 +58,6 @@ class AvoidInMemoryStreamingDefaultConstructor {
         <priority>3</priority>
         <properties>
             <property name="tag" value="jpinpoint-rule" type="String" description="for-sonar"/>
-            <property name="version" value="3.1"/>
             <property name="xpath">
                 <value><![CDATA[
 //VariableDeclaration/Type//T-Identifier[@Text='StringBuffer'],
@@ -96,11 +94,10 @@ class AvoidStringBuffer {
         <priority>1</priority>
         <properties>
             <property name="tag" value="jpinpoint-rule" type="String" description="for-sonar"/>
-            <property name="version" value="3.1"/>
             <property name="xpath">
                 <value><![CDATA[
-(: check if java.text imports exists as filter for same named classes in other packages :)
-//ImportHeader[.//T-Identifier[@Text='java'] and .//T-Identifier[@Text='text']]/ancestor::KotlinFile
+(: check if at least one java.text imports exists as filter for same named classes in other packages :)
+//ImportHeader[.//T-Identifier[@Text='java'] and .//T-Identifier[@Text='text']][1]/ancestor::KotlinFile
 (: PrimaryExpression finds the constructors instead of type declarations :)
 (: filter matches in functions :)
 //ClassDeclaration//PrimaryExpression//SimpleIdentifier//T-Identifier[
@@ -133,6 +130,72 @@ class Foo {
         </example>
     </rule>
 
+    <rule name="AvoidRecompilingXPathExpression" class="net.sourceforge.pmd.lang.rule.XPathRule"
+          language="kotlin"
+          message="XPathExpression is created and compiled every time. Beware it is thread-unsafe."
+          typeResolution="true"
+          externalInfoUrl="${doc_root}/JavaCodePerformance.md#ux02">
+        <description>XPathExpression is created and compiled on every method call, compiled possibly implicitly by XPath::evaluate.
+            Problem: Creation of XPath and compilation of XPathExpression takes time. It may slow down your application. &#13;
+            Solution: 1. Avoid XPath usage. 2. Compile the xpath expression as String into a XPathExpression. However, since XPath and XPathExpression classes are thread-unsafe, they are not easily cached. Caching it in a ThreadLocal may be a solution.
+            (jpinpoint-rules)</description>
+        <priority>2</priority>
+        <properties>
+            <property name="tag" value="jpinpoint-rule" type="String" description="for-sonar"/>
+            <property name="xpath">
+                <value><![CDATA[
+//ImportHeader[.//T-Identifier[@Text='javax'] and .//T-Identifier[@Text='xml'] and .//T-Identifier[@Text='xpath']]/ancestor::KotlinFile
+//FunctionBody//T-Identifier[@Text='newXPath']
+/ancestor::FunctionBody//T-Identifier[@Text='compile'
+or (@Text='evaluate' and ancestor::PostfixUnarySuffix/..//ValueArguments[count(./ValueArgument) = 3])]
+(: note on limitation, the 2-argument version is not found, difficult to do the typeIs checking like in Java :)
+]]></value>
+            </property>
+        </properties>
+        <example>
+            <![CDATA[
+import org.w3c.dom.Document
+import org.w3c.dom.NodeList
+import javax.xml.xpath.*
+
+class AvoidRecompilingXPathExpressionKotlin {
+    fun bad1(doc: Document?): NodeList {
+        val xpath: XPath = XPathFactory.newInstance().newXPath()
+        val expr: XPathExpression = xpath.compile("//book[author='Isaac Asimov']/title/text()") // bad
+        return expr.evaluate(doc, XPathConstants.NODESET) as NodeList
+    }
+
+    @Throws(XPathExpressionException::class)
+    fun bad2(doc: Document?): NodeList {
+        val xpath: XPath = XPathFactory.newInstance().newXPath()
+        val xPathQuery = "//book[author='Isaac Asimov']/title/text()"
+        return xpath.evaluate(xPathQuery, doc, XPathConstants.NODESET) as NodeList // bad
+    }
+}
+
+object GoodAvoidRecompilingXPathExpressionKt {
+    private val tlFac = ThreadLocal.withInitial { XPathFactory.newInstance() }
+    private val tlExpr: ThreadLocal<XPathExpression>
+
+    init {
+        val xpath = tlFac.get().newXPath()
+        val expr: XPathExpression = try {
+            xpath.compile("//book[author='Isaac Asimov']/title/text()")
+        } catch (e: XPathExpressionException) {
+            throw RuntimeException(e)
+        }
+        tlExpr = ThreadLocal.withInitial { expr } // good
+    }
+
+    @Throws(XPathExpressionException::class)
+    fun good(doc: Document?): NodeList {
+        return tlExpr!!.get().evaluate(doc, XPathConstants.NODESET) as NodeList // good
+    }
+}
+            ]]>
+        </example>
+    </rule>
+
     <rule name="AvoidSimpleDateFormat"
           class="net.sourceforge.pmd.lang.rule.XPathRule"
           dfa="false"

diff --git a/...va/com/jpinpoint/perf/lang/kotlin/ruleset/common/AvoidRecompilingXPathExpressionTest.java b/...va/com/jpinpoint/perf/lang/kotlin/ruleset/common/AvoidRecompilingXPathExpressionTest.java
@@ -0,0 +1,6 @@
+package com.jpinpoint.perf.lang.kotlin.ruleset.common;
+
+import net.sourceforge.pmd.testframework.PmdRuleTst;
+
+public class AvoidRecompilingXPathExpressionTest extends PmdRuleTst {
+}
diff --git a/...ces/com/jpinpoint/perf/lang/kotlin/ruleset/common/xml/AvoidRecompilingXPathExpression.xml b/...ces/com/jpinpoint/perf/lang/kotlin/ruleset/common/xml/AvoidRecompilingXPathExpression.xml
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<test-data
+        xmlns="http://pmd.sourceforge.net/rule-tests"
+        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+        xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd">
+    <test-code>
+        <description>AvoidRecompilingXPathExpression</description>
+        <expected-problems>2</expected-problems>
+        <expected-linenumbers>8, 16</expected-linenumbers>
+        <code><![CDATA[
+import org.w3c.dom.Document
+import org.w3c.dom.NodeList
+import javax.xml.xpath.*
+
+class AvoidRecompilingXPathExpressionKotlin {
+    fun bad1(doc: Document?): NodeList {
+        val xpath: XPath = XPathFactory.newInstance().newXPath()
+        val expr: XPathExpression = xpath.compile("//book[author='Isaac Asimov']/title/text()") // bad
+        return expr.evaluate(doc, XPathConstants.NODESET) as NodeList
+    }
+
+    @Throws(XPathExpressionException::class)
+    fun bad2(doc: Document?): NodeList {
+        val xpath: XPath = XPathFactory.newInstance().newXPath()
+        val xPathQuery = "//book[author='Isaac Asimov']/title/text()"
+        return xpath.evaluate(xPathQuery, doc, XPathConstants.NODESET) as NodeList // bad
+    }
+}
+
+object GoodAvoidRecompilingXPathExpressionKt {
+    private val tlFac = ThreadLocal.withInitial { XPathFactory.newInstance() }
+    private val tlExpr: ThreadLocal<XPathExpression>
+
+    init {
+        val xpath = tlFac.get().newXPath()
+        val expr: XPathExpression = try {
+            xpath.compile("//book[author='Isaac Asimov']/title/text()")
+        } catch (e: XPathExpressionException) {
+            throw RuntimeException(e)
+        }
+        tlExpr = ThreadLocal.withInitial { expr } // good
+    }
+
+    @Throws(XPathExpressionException::class)
+    fun good(doc: Document?): NodeList {
+        return tlExpr!!.get().evaluate(doc, XPathConstants.NODESET) as NodeList // good
+    }
+}
+]]></code>
+    </test-code>
+
+</test-data>
diff --git a/testkt b/testkt
@@ -0,0 +1,2 @@
+#!/bin/sh
+mvn test -Pkotlin-pmd7