[apex] New Rule: AvoidNonRestrictiveQueries (#5089)

Merge pull request #5089 from adangel:apex-issue-635-avoid-non-restrictive-queries

docs/pages/release_notes.md

@@ -16,12 +16,18 @@ This is a {{ site.pmd.release_type }} release.
 
 ### 🌟 New and changed rules
 
+#### New Rules
+
+* The new Apex rule {% apex/performance/AvoidNonRestrictiveQueries %} finds SOQL and SOSL queries without a where
+  or limit statement. This can quickly cause governor limit exceptions.
+
 #### Changed rules
 * {%rule apex/codestyle/ClassNamingConvention %}: Two new properties to configure different patterns
   for inner classes and interfaces: `innerClassPattern` and `innerInterfacePattern`.
 
 ### 🐛 Fixed Issues
-* apex-codestyle
+* apex
+  * [#635](https://github.com/pmd/pmd/issues/635): \[apex] New Rule: Avoid soql/sosl queries without a where clause or limit statement
   * [#4800](https://github.com/pmd/pmd/issues/4800): \[apex] ClassNamingConvention: Support naming convention for *inner* classes
 * plsql
   * [#5086](https://github.com/pmd/pmd/pull/5086): \[plsql] Fixed issue with missing optional table alias in MERGE usage

pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/performance/AvoidNonRestrictiveQueriesRule.java

@@ -0,0 +1,110 @@
+/*
+ * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
+ */
+
+package net.sourceforge.pmd.lang.apex.rule.performance;
+
+import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import org.checkerframework.checker.nullness.qual.NonNull;
+
+import net.sourceforge.pmd.lang.apex.ast.ASTAnnotation;
+import net.sourceforge.pmd.lang.apex.ast.ASTAnnotationParameter;
+import net.sourceforge.pmd.lang.apex.ast.ASTMethod;
+import net.sourceforge.pmd.lang.apex.ast.ASTModifierNode;
+import net.sourceforge.pmd.lang.apex.ast.ASTSoqlExpression;
+import net.sourceforge.pmd.lang.apex.ast.ASTSoslExpression;
+import net.sourceforge.pmd.lang.apex.ast.ASTUserClass;
+import net.sourceforge.pmd.lang.apex.ast.ApexNode;
+import net.sourceforge.pmd.lang.apex.rule.AbstractApexRule;
+import net.sourceforge.pmd.lang.ast.NodeStream;
+import net.sourceforge.pmd.lang.rule.RuleTargetSelector;
+import net.sourceforge.pmd.reporting.RuleContext;
+
+public class AvoidNonRestrictiveQueriesRule extends AbstractApexRule {
+    private static final Pattern RESTRICTIVE_PATTERN = Pattern.compile("(where\\s+)|(limit\\s+)", Pattern.CASE_INSENSITIVE);
+    private static final Pattern SELECT_OR_FIND_PATTERN = Pattern.compile("(select\\s+|find\\s+)", Pattern.CASE_INSENSITIVE);
+    private static final Pattern SUB_QUERY_PATTERN = Pattern.compile("(?i)\\(\\s*select\\s+[^)]+\\)");
+
+    @Override
+    protected @NonNull RuleTargetSelector buildTargetSelector() {
+        return RuleTargetSelector.forTypes(ASTSoqlExpression.class, ASTSoslExpression.class);
+    }
+
+    @Override
+    public Object visit(ASTSoqlExpression node, Object data) {
+        visitSoqlOrSosl(node, "SOQL", node.getQuery(), asCtx(data));
+        return data;
+    }
+
+    @Override
+    public Object visit(ASTSoslExpression node, Object data) {
+        visitSoqlOrSosl(node, "SOSL", node.getQuery(), asCtx(data));
+        return data;
+    }
+
+    private void visitSoqlOrSosl(ApexNode<?> node, String type, String query, RuleContext ruleContext) {
+        ASTMethod method = node.ancestors(ASTMethod.class).first();
+        if (method != null && method.getModifiers().isTest()) {
+            Optional<ASTAnnotation> methodAnnotation = method
+                    .children(ASTModifierNode.class)
+                    .children(ASTAnnotation.class)
+                    .filter(a -> "isTest".equalsIgnoreCase(a.getName()))
+                    .firstOpt();
+
+            Optional<ASTAnnotation> classAnnotation = method
+                    .ancestors(ASTUserClass.class)
+                    .firstOpt()
+                    .map(u -> u.children(ASTModifierNode.class))
+                    .map(s -> s.children(ASTAnnotation.class))
+                    .map(NodeStream::first);
+
+            Optional<Boolean> methodSeeAllData = methodAnnotation.flatMap(m -> m.children(ASTAnnotationParameter.class)
+                    .filter(p -> ASTAnnotationParameter.SEE_ALL_DATA.equalsIgnoreCase(p.getName()))
+                    .firstOpt()
+                    .map(ASTAnnotationParameter::getBooleanValue));
+            boolean classSeeAllData = classAnnotation.flatMap(m -> m.children(ASTAnnotationParameter.class)
+                            .filter(p -> ASTAnnotationParameter.SEE_ALL_DATA.equalsIgnoreCase(p.getName()))
+                            .firstOpt()
+                            .map(ASTAnnotationParameter::getBooleanValue))
+                    .orElse(false);
+
+            if (methodSeeAllData.isPresent()) {
+                if (!methodSeeAllData.get()) {
+                    return;
+                }
+            } else if (!classSeeAllData) {
+                return;
+            }
+        }
+
+        Matcher subQueryMatcher = SUB_QUERY_PATTERN.matcher(query);
+        StringBuffer queryWithoutSubQueries = new StringBuffer(query.length());
+        while (subQueryMatcher.find()) {
+            subQueryMatcher.appendReplacement(queryWithoutSubQueries, "(replaced_subquery)");
+        }
+        subQueryMatcher.appendTail(queryWithoutSubQueries);
+
+        verifyQuery(ruleContext, node, type, queryWithoutSubQueries.toString());
+    }
+
+    private void verifyQuery(RuleContext ctx, ApexNode<?> node, String type, String query) {
+        int occurrencesSelectOrFind = countOccurrences(SELECT_OR_FIND_PATTERN, query);
+        int occurrencesWhereOrLimit = countOccurrences(RESTRICTIVE_PATTERN, query);
+
+        if (occurrencesSelectOrFind > 0 && occurrencesWhereOrLimit == 0) {
+            ctx.addViolation(node, type);
+        }
+    }
+
+    private int countOccurrences(Pattern pattern, String s) {
+        int occurrences = 0;
+        Matcher matcher = pattern.matcher(s);
+        while (matcher.find()) {
+            occurrences++;
+        }
+        return occurrences;
+    }
+}

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

@@ -53,6 +53,32 @@ public class Foo {
         </example>
     </rule>
 
+    <rule name="AvoidNonRestrictiveQueries"
+          language="apex"
+          since="7.4.0"
+          message="Avoid {0} queries without a where or limit statement"
+          class="net.sourceforge.pmd.lang.apex.rule.performance.AvoidNonRestrictiveQueriesRule"
+          externalInfoUrl="${pmd.website.baseurl}/pmd_rules_apex_performance.html#avoidnonrestrictivequeries">
+        <description>
+            When working with very large amounts of data, unfiltered SOQL or SOSL queries can quickly cause
+            [governor limit](https://developer.salesforce.com/docs/atlas.en-us.apexcode.meta/apexcode/apex_gov_limits.htm)
+            exceptions.
+        </description>
+        <priority>3</priority>
+        <example>
+            <![CDATA[
+public class Something {
+    public static void main( String[] as ) {
+        Account[] accs1 = [ select id from account ];  // Bad
+        Account[] accs2 = [ select id from account limit 10 ];  // better
+
+        List<List<SObject>> searchList = [FIND 'map*' IN ALL FIELDS RETURNING Account (Id, Name), Contact, Opportunity, Lead]; // bad
+    }
+}
+]]>
+        </example>
+    </rule>
+
     <rule name="EagerlyLoadedDescribeSObjectResult"
           language="apex"
           since="6.40.0"

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

@@ -87,6 +87,7 @@
       <priority>3</priority>
    </rule>
 <!--    <rule ref="category/apex/performance.xml/AvoidDebugStatements" /> -->
+<!--    <rule ref="category/apex/performance.xml/AvoidNonRestrictiveQueries"/> -->
 <!--    <rule ref="category/apex/performance.xml/EagerlyLoadedDescribeSObjectResult" /> -->
 
    <!-- NAMING -->

pmd-apex/src/test/java/net/sourceforge/pmd/lang/apex/rule/performance/AvoidNonRestrictiveQueriesTest.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.performance;
+
+import net.sourceforge.pmd.test.PmdRuleTst;
+
+class AvoidNonRestrictiveQueriesTest extends PmdRuleTst {
+    // no additional unit tests
+}

pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule/performance/xml/AvoidNonRestrictiveQueries.xml

@@ -0,0 +1,265 @@
+<?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>Query without a where statement</description>
+        <expected-problems>1</expected-problems>
+        <expected-messages>
+            <message>Avoid SOQL queries without a where or limit statement</message>
+        </expected-messages>
+        <code><![CDATA[
+public class Something {
+    public static void main( String[] as ) {
+        Account[] accs = [ select id from account ];  //Bad
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Query with where and limit statement</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Something {
+    public static void main( String[] as ) {
+        Account[] accs = [ select id from account where id = 1 limit 1];
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Subquery without limit</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Something {
+    public static void main( String[] as ) {
+        Account[] accs = [ select id, (SELECT Id FROM Contact) from account where id = 1]; // That's ok, as long as the main query is restricted
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Multiple non restrictive queries</description>
+        <expected-problems>3</expected-problems>
+        <expected-linenumbers>3,4,5</expected-linenumbers>
+        <expected-messages>
+            <message>Avoid SOQL queries without a where or limit statement</message>
+            <message>Avoid SOQL queries without a where or limit statement</message>
+            <message>Avoid SOQL queries without a where or limit statement</message>
+        </expected-messages>
+        <code><![CDATA[
+public class Something {
+    public static Account[] main( String[] as ) {
+        Account[] accs = [ select id from account ];
+        Account[] accs2 = [ select id from account ];
+        return [SELECT Id FROM Contact];
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Restrictive queries</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Something {
+    public static Account[] main( String[] as ) {
+        Account[] accs = [ select id from account where name='foo' and id < 100 LIMIT 5 ];
+        Account[] accs2 = [ select id from account where id > 1 ];
+        return [SELECT Id FROM Contact limit 1];
+    }
+}
+        ]]></code>
+    </test-code>
+
+    <test-code>
+        <description>subquery with limit, main query without</description>
+        <expected-problems>1</expected-problems>
+        <expected-messages>
+            <message>Avoid SOQL queries without a where or limit statement</message>
+        </expected-messages>
+        <code><![CDATA[
+class Query {
+    void method() {
+        Account[] accounts = [ select id, (SELECT Id FROM Contact where id = 1 limit 1) from account ];
+    }
+}
+]]></code>
+    </test-code>
+
+    <test-code>
+        <description>main query with limit, subquery without is OK</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+class Query {
+    void method() {
+        Account[] accounts = [ select id, (SELECT Id FROM Contact) from account limit 1 ];
+    }
+}
+]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Test case with SOQL query - non-restrictive - SeeAllData=true</description>
+        <expected-problems>1</expected-problems>
+        <code><![CDATA[
+@IsTest(SeeAllData=true)
+public class TestDataAccessClass {
+    @IsTest
+    static void myTestMethod() {
+        Account[] accounts = [SELECT Id, Name FROM Account];
+    }
+}
+]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Test case with SOQL query - non-restrictive - SeeAllData=False is OK</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+@IsTest(SeeAllData=false)
+public class TestDataAccessClass {
+    @IsTest
+    static void myTestMethod() {
+        Account[] accounts = [SELECT Id, Name FROM Account];
+    }
+}
+]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Test case with SOQL query - restrictive is OK</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+@IsTest(SeeAllData=true)
+public class TestDataAccessClass {
+    @IsTest
+    static void myTestMethod() {
+        Account[] accounts = [SELECT Id, Name FROM Account LIMIT 1];
+    }
+}
+]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Test case with SOQL query - restrictive - SeeAllData=false is OK</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+@IsTest(SeeAllData=false)
+public class TestDataAccessClass {
+    @IsTest
+    static void myTestMethod() {
+        Account[] accounts = [SELECT Id, Name FROM Account LIMIT 1];
+    }
+}
+]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Test case with SOQL query - non-restrictive - SeeAllData=false on class but true on method</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>5</expected-linenumbers>
+        <code><![CDATA[
+@IsTest(SeeAllData=false)
+public class TestDataAccessClass {
+    @IsTest(SeeAllData=true)
+    static void myTestMethod1() {
+        Account[] accounts = [SELECT Id, Name FROM Account];
+    }
+
+    @IsTest
+    static void myTestMethod2() {
+        Account[] accounts = [SELECT Id, Name FROM Account]; // inherits from class annotation SeeAllData=false
+    }
+}
+]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Test case with SOQL query - non-restrictive - SeeAllData=true on class but false on method</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>10</expected-linenumbers>
+        <code><![CDATA[
+@IsTest(SeeAllData=true)
+public class TestDataAccessClass {
+    @IsTest(SeeAllData=false)
+    static void myTestMethod1() {
+        Account[] accounts = [SELECT Id, Name FROM Account]; // that's ok, since SeeAllData=false
+    }
+
+    @IsTest
+    static void myTestMethod2() {
+        Account[] accounts = [SELECT Id, Name FROM Account]; // not good, inherits SeeAllData=true from class
+    }
+}
+]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Test case with SOSL query - missing where</description>
+        <expected-problems>1</expected-problems>
+        <expected-messages>
+            <message>Avoid SOSL queries without a where or limit statement</message>
+        </expected-messages>
+        <code><![CDATA[
+public class Something {
+    public static void main( String[] as ) {
+        List<List<SObject>> searchList = [FIND 'map*' IN ALL FIELDS RETURNING Account (Id, Name), Contact, Opportunity, Lead]; // bad
+    }
+}
+]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Test case with SOSL query - with limit is OK</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Something {
+    public static void main( String[] as ) {
+        List<List<SObject>> searchList = [FIND 'map*' IN ALL FIELDS RETURNING Account (Id, Name), Contact, Opportunity, Lead LIMIT 1];
+    }
+}
+]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Test case with SOSL query - with where is OK</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Something {
+    public static void main( String[] as ) {
+        List<List<SObject>> searchList = [FIND 'map*' IN ALL FIELDS RETURNING Account (Id, Name WHERE Name like 'test'), Contact, Opportunity, Lead];
+    }
+}
+]]></code>
+    </test-code>
+    
+    <test-code>
+        <description>False positives for SOQL queries with WHERE on multiple lines</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+public class Something {
+    private static final String NAMESPACE_PREFIX = 'some_prefix';
+    private static final String PERMISSION_SET = 'some_permission';
+
+    public static Boolean hasPermission(Id userId) {
+        return ![
+            SELECT Id
+            FROM PermissionSetAssignment
+            WHERE
+              AssigneeId = :userId
+              AND PermissionSet.Name = :PERMISSION_SET
+              AND PermissionSet.NamespacePrefix = :NAMESPACE_PREFIX
+          ]
+          .isEmpty();
+  }
+}
+]]></code>
+    </test-code>
+</test-data>