[apex] New Rule: Queueable Should Attach Finalizer (#5303)

Merge pull request #5303 from mitchspano:Require_Finalizer

.all-contributorsrc

@@ -7517,6 +7517,7 @@
       "avatar_url": "https://avatars.githubusercontent.com/u/18402464?v=4",
       "profile": "https://github.com/mitchspano",
       "contributions": [
+        "code",
         "bug"
       ]
     },

docs/pages/release_notes.md

@@ -14,10 +14,18 @@ This is a {{ site.pmd.release_type }} release.
 
 ### 🚀 New and noteworthy
 
+### 🌟 New and changed rules
+
+#### New Rules
+* The new Apex rule {% rule apex/bestpractices/QueueableWithoutFinalizer %} detects when the Queueable interface
+  is used but a Finalizer is not attached. Without attaching a Finalizer, there is no way of designing error
+  recovery actions should the Queueable action fail.
+
 ### 🐛 Fixed Issues
 * ant
   * [#1860](https://github.com/pmd/pmd/issues/1860): \[ant] Reflective access warnings on java > 9 and java < 17
 * apex
+  * [#5302](https://github.com/pmd/pmd/issues/5302): \[apex] New Rule: Queueable Should Attach Finalizer
   * [#5333](https://github.com/pmd/pmd/issues/5333): \[apex] Token recognition errors for string containing unicode escape sequence
 * html
   * [#5322](https://github.com/pmd/pmd/issues/5322): \[html] CPD throws exception on when HTML file is missing closing tag
@@ -42,6 +50,7 @@ This is a {{ site.pmd.release_type }} release.
 
 ### ✨ External Contributions
 * [#5284](https://github.com/pmd/pmd/pull/5284): \[apex] Use case-insensitive input stream to avoid choking on Unicode escape sequences - [Willem A. Hajenius](https://github.com/wahajenius) (@wahajenius)
+* [#5303](https://github.com/pmd/pmd/pull/5303): \[apex] New Rule: Queueable Should Attach Finalizer - [Mitch Spano](https://github.com/mitchspano) (@mitchspano)
 
 {% endtocmaker %}
 

pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/QueueableWithoutFinalizerRule.java

@@ -0,0 +1,92 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.apex.rule.bestpractices;
+
+import java.util.List;
+
+import org.checkerframework.checker.nullness.qual.NonNull;
+
+import net.sourceforge.pmd.lang.apex.ast.ASTMethod;
+import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression;
+import net.sourceforge.pmd.lang.apex.ast.ASTParameter;
+import net.sourceforge.pmd.lang.apex.ast.ASTUserClass;
+import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule;
+import net.sourceforge.pmd.lang.rule.RuleTargetSelector;
+
+/**
+ * Scans classes which implement the `Queueable` interface. If the `public void
+ * execute(QueueableContext context)` method does not call the
+ * `System.attachFinalizer(Finalizer f)` method, then a violation will be added
+ * to the `execute` method.
+ *
+ * @author mitchspano
+ */
+public class QueueableWithoutFinalizerRule extends AbstractApexRule {
+
+    private static final String EXECUTE = "execute";
+    private static final String QUEUEABLE = "queueable";
+    private static final String QUEUEABLE_CONTEXT = "queueablecontext";
+    private static final String SYSTEM_ATTACH_FINALIZER = "system.attachfinalizer";
+
+    @Override
+    protected @NonNull RuleTargetSelector buildTargetSelector() {
+        return RuleTargetSelector.forTypes(ASTUserClass.class);
+    }
+
+    /**
+     * If the class implements the `Queueable` interface and the
+     * `execute(QueueableContext context)` does not call the
+     * `System.attachFinalizer(Finalizer f)` method, then add a violation.
+     */
+    @Override
+    public Object visit(ASTUserClass theClass, Object data) {
+        if (!implementsTheQueueableInterface(theClass)) {
+            return data;
+        }
+        for (ASTMethod theMethod : theClass.descendants(ASTMethod.class).toList()) {
+            if (isTheExecuteMethodOfTheQueueableInterface(theMethod)
+                    && !callsTheSystemAttachFinalizerMethod(theMethod)) {
+                asCtx(data).addViolation(theMethod);
+            }
+        }
+        return data;
+    }
+
+    /** Determines if the class implements the Queueable interface. */
+    private boolean implementsTheQueueableInterface(ASTUserClass theClass) {
+        for (String interfaceName : theClass.getInterfaceNames()) {
+            if (QUEUEABLE.equalsIgnoreCase(interfaceName)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    /**
+     * Determines if the method is the `execute(QueueableContext context)`
+     * method. Parameter count is checked to account for method overloading.
+     */
+    private boolean isTheExecuteMethodOfTheQueueableInterface(ASTMethod theMethod) {
+        if (!EXECUTE.equalsIgnoreCase(theMethod.getCanonicalName())) {
+            return false;
+        }
+        List<ASTParameter> parameters = theMethod.descendants(ASTParameter.class).toList();
+        return parameters.size() == 1 && QUEUEABLE_CONTEXT.equalsIgnoreCase(parameters.get(0).getType());
+    }
+
+    /**
+     * Determines if the method calls the `System.attachFinalizer(Finalizer f)`
+     * method.
+     */
+    private boolean callsTheSystemAttachFinalizerMethod(ASTMethod theMethod) {
+        for (ASTMethodCallExpression methodCallExpression : theMethod.descendants(ASTMethodCallExpression.class)
+                .toList()) {
+            if (SYSTEM_ATTACH_FINALIZER.equalsIgnoreCase(methodCallExpression.getFullMethodName())) {
+                return true;
+            }
+        }
+        return false;
+    }
+}

pmd-apex/src/main/resources/category/apex/bestpractices.xml

@@ -285,4 +285,56 @@ Detects when a local variable is declared and/or assigned but not used.
         </example>
     </rule>
 
+    <rule name="QueueableWithoutFinalizer"
+          since="7.8.0"
+          language="apex"
+          message="This Queueable doesn't attach a Finalizer"
+          class="net.sourceforge.pmd.lang.apex.rule.bestpractices.QueueableWithoutFinalizerRule"
+          externalInfoUrl="${pmd.website.baseurl}/pmd_rules_apex_bestpractices.html#queueablewithoutfinalizer">
+        <description>
+Detects when the Queueable interface is used but a Finalizer is not attached.
+It is best practice to call the `System.attachFinalizer(Finalizer f)` method within the `execute` method of a class which implements the `Queueable` interface.
+Without attaching a Finalizer, there is no way of designing error recovery actions should the Queueable action fail.
+        </description>
+        <priority>5</priority>
+        <example>
+<![CDATA[
+// Incorrect code, does not attach a finalizer.
+public class UserUpdater implements Queueable {
+    public List<User> usersToUpdate;
+
+    public UserUpdater(List<User> usersToUpdate) {
+        this.usersToUpdate = usersToUpdate;
+    }
+
+    public void execute(QueueableContext context) { // no Finalizer is attached
+        update usersToUpdate;
+    }
+}
+
+// Proper code, attaches a finalizer.
+public class UserUpdater implements Queueable, Finalizer {
+    public List<User> usersToUpdate;
+
+    public UserUpdater(List<User> usersToUpdate) {
+        this.usersToUpdate = usersToUpdate;
+    }
+
+    public void execute(QueueableContext context) {
+        System.attachFinalizer(this);
+        update usersToUpdate;
+    }
+
+    public void execute(FinalizerContext ctx) {
+        if (ctx.getResult() == ParentJobResult.SUCCESS) {
+            // Handle success
+        } else {
+            // Handle failure
+        }
+    }
+}
+]]>
+        </example>
+    </rule>
+
 </ruleset>

pmd-apex/src/main/resources/rulesets/apex/quickstart.xml

@@ -209,6 +209,7 @@
       <priority>3</priority>
    </rule>
    <!-- <rule ref="category/apex/bestpractices.xml/UnusedLocalVariable"/> -->
+   <!-- <rule ref="category/apex/bestpractices.xml/QueueableWithoutFinalizer"/> -->
    <!-- <rule ref="category/apex/errorprone.xml/OverrideBothEqualsAndHashcode" /> -->
    <!-- <rule ref="category/apex/errorprone.xml/InaccessibleAuraEnabledGetter" /> -->
 

pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/bestpractices/QueueableWithoutFinalizerTest.java

@@ -0,0 +1,11 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.apex.rule.bestpractices;
+
+import net.sourceforge.pmd.test.PmdRuleTst;
+
+class QueueableWithoutFinalizerTest extends PmdRuleTst {
+    // no additional unit tests
+}

pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/QueueableWithoutFinalizer.xml

@@ -0,0 +1,51 @@
+<?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>[apex] Queueable Without Finalizer - positive test case #5302</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>8</expected-linenumbers>
+        <code><![CDATA[
+public class UserUpdater implements Queueable {
+    public List<User> usersToUpdate;
+
+    public UserUpdater(List<User> usersToUpdate) {
+        this.usersToUpdate = usersToUpdate;
+    }
+
+    public void execute(QueueableContext context) {
+        update usersToUpdate;
+    }
+}
+        ]]></code>
+    </test-code>
+    <test-code>
+        <description>[apex] Queueable Without Finalizer - negative test case #5302</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class UserUpdater implements Queueable, Finalizer {
+    public List<User> usersToUpdate;
+
+    public UserUpdater(List<User> usersToUpdate) {
+        this.usersToUpdate = usersToUpdate;
+    }
+
+    public void execute(QueueableContext context) {
+        System.attachFinalizer(this);
+        update usersToUpdate;
+    }
+
+    public void execute(FinalizerContext ctx) {
+        if (ctx.getResult() == ParentJobResult.SUCCESS) {
+            // Handle success
+        } else {
+            // Handle failure
+        }
+    }
+}
+        ]]></code>
+    </test-code>
+</test-data>