Skip to content

Commit

Permalink
Merge pull request #4677 from tprouvot:feature/apex/avoidGetGlobalDes…
Browse files Browse the repository at this point in the history
…cribesInLoops

[apex] Add new rule: OperationWithHighCostInLoop #4677
  • Loading branch information
adangel committed Oct 28, 2023
2 parents df52c37 + 5f5c63b commit c5503c9
Show file tree
Hide file tree
Showing 8 changed files with 285 additions and 1 deletion.
10 changes: 10 additions & 0 deletions docs/pages/release_notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ The remaining section describes the complete release notes for 7.0.0.

**New Rules**

* {% rule apex/performance/OperationWithHighCostInLoop %} finds Schema class methods called in a loop, which is a
potential performance issue.
* {% rule java/codestyle/UseExplicitTypes %} reports usages of `var` keyword, which was introduced with Java 10.
* {% rule xml/bestpractices/MissingEncoding %} finds XML files without explicit encoding.

Expand All @@ -60,6 +62,8 @@ The remaining section describes the complete release notes for 7.0.0.
* miscellaneous
* [#4699](https://github.com/pmd/pmd/pull/4699): Make PMD buildable with java 21
* [#4586](https://github.com/pmd/pmd/pull/4586): Use explicit encoding in ruleset xml files
* apex-performance
* [#4675](https://github.com/pmd/pmd/issues/4675): \[apex] New Rule: OperationWithHighCostInLoop
* java-codestyle
* [#2847](https://github.com/pmd/pmd/issues/2847): \[java] New Rule: Use Explicit Types
* [#4578](https://github.com/pmd/pmd/issues/4578): \[java] CommentDefaultAccessModifier comment needs to be before annotation if present
Expand Down Expand Up @@ -89,6 +93,7 @@ The following previously deprecated classes have been removed:

* [#4640](https://github.com/pmd/pmd/pull/4640): \[cli] Launch script fails if run via "bash pmd" - [Shai Bennathan](https://github.com/shai-bennathan) (@shai-bennathan)
* [#4673](https://github.com/pmd/pmd/pull/4673): \[javascript] CPD: Added support for decorator notation - [Wener](https://github.com/wener-tiobe) (@wener-tiobe)
* [#4677](https://github.com/pmd/pmd/pull/4677): \[apex] Add new rule: OperationWithHighCostInLoop - [Thomas Prouvot](https://github.com/tprouvot) (@tprouvot)

### 🚀 Major Features and Enhancements

Expand Down Expand Up @@ -255,6 +260,8 @@ can be parsed now. PMD should now be able to parse Apex code up to version 59.0

**Apex**
* {% rule apex/design/UnusedMethod %} finds unused methods in your code.
* {% rule apex/performance/OperationWithHighCostInLoop %} finds Schema class methods called in a loop, which is a
potential performance issue.

**Java**
* {% rule java/codestyle/UnnecessaryBoxing %} reports boxing and unboxing conversions that may be made implicit.
Expand Down Expand Up @@ -513,6 +520,8 @@ Language specific fixes:
* [#2667](https://github.com/pmd/pmd/issues/2667): \[apex] Integrate nawforce/ApexLink to build robust Unused rule
* [#4509](https://github.com/pmd/pmd/issues/4509): \[apex] ExcessivePublicCount doesn't consider inner classes correctly
* [#4596](https://github.com/pmd/pmd/issues/4596): \[apex] ExcessivePublicCount ignores properties
* apex-performance
* [#4675](https://github.com/pmd/pmd/issues/4675): \[apex] New Rule: OperationWithHighCostInLoop
* apex-security
* [#4646](https://github.com/pmd/pmd/issues/4646): \[apex] ApexSOQLInjection does not recognise SObjectType or SObjectField as safe variable types
* java
Expand Down Expand Up @@ -735,6 +744,7 @@ Language specific fixes:
* [#4664](https://github.com/pmd/pmd/pull/4664): \[cli] CPD: Fix NPE when only `--file-list` is specified - [Wener](https://github.com/wener-tiobe) (@wener-tiobe)
* [#4665](https://github.com/pmd/pmd/pull/4665): \[java] Doc: Fix references AutoClosable -> AutoCloseable - [Andrey Bozhko](https://github.com/AndreyBozhko) (@AndreyBozhko)
* [#4673](https://github.com/pmd/pmd/pull/4673): \[javascript] CPD: Added support for decorator notation - [Wener](https://github.com/wener-tiobe) (@wener-tiobe)
* [#4677](https://github.com/pmd/pmd/pull/4677): \[apex] Add new rule: OperationWithHighCostInLoop - [Thomas Prouvot](https://github.com/tprouvot) (@tprouvot)

### 📈 Stats
* 5007 commits
Expand Down
2 changes: 2 additions & 0 deletions docs/pages/release_notes_pmd7.md
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,8 @@ can be parsed now. PMD should now be able to parse Apex code up to version 59.0

**Apex**
* {% rule apex/design/UnusedMethod %} finds unused methods in your code.
* {% rule apex/performance/OperationWithHighCostInLoop %} finds Schema class methods called in a loop, which is a
potential performance issue.

**Java**
* {% rule java/codestyle/UnnecessaryBoxing %} reports boxing and unboxing conversions that may be made implicit.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/**
* BSD-style license; for more info see http://pmd.sourceforge.net/license.html
*/

package net.sourceforge.pmd.lang.apex.rule.performance;

import java.util.Locale;
import java.util.Set;
import java.util.stream.Collectors;

import org.checkerframework.checker.nullness.qual.NonNull;

import net.sourceforge.pmd.lang.apex.ast.ASTMethodCallExpression;
import net.sourceforge.pmd.lang.rule.RuleTargetSelector;
import net.sourceforge.pmd.util.CollectionUtil;

/**
* Warn users when code that could impact performance is executing within a
* looping construct.
*/
public class OperationWithHighCostInLoopRule extends AbstractAvoidNodeInLoopsRule {

private static final Set<String> SCHEMA_PERFORMANCE_METHODS = CollectionUtil.setOf(
"System.Schema.getGlobalDescribe",
"Schema.getGlobalDescribe",
"System.Schema.describeSObjects",
"Schema.describeSObjects")
.stream().map(s -> s.toLowerCase(Locale.ROOT)).collect(Collectors.toSet());

@Override
protected @NonNull RuleTargetSelector buildTargetSelector() {
return RuleTargetSelector.forTypes(
// performance consuming methods
ASTMethodCallExpression.class);
}

// Begin general method invocations
@Override
public Object visit(ASTMethodCallExpression node, Object data) {
if (checkHighCostClassMethods(node)) {
return checkForViolation(node, data);
} else {
return data;
}
}

private boolean checkHighCostClassMethods(ASTMethodCallExpression node) {
return SCHEMA_PERFORMANCE_METHODS.contains(node.getFullMethodName().toLowerCase(Locale.ROOT));
}
// End general method invocations
}
69 changes: 69 additions & 0 deletions pmd-apex/src/main/resources/category/apex/performance.xml
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,74 @@ public class Foo {
</example>
</rule>

<rule name="OperationWithHighCostInLoop"
language="apex"
since="7.0.0"
message="Avoid operations in loops that may impact performances"
class="net.sourceforge.pmd.lang.apex.rule.performance.OperationWithHighCostInLoopRule"
externalInfoUrl="${pmd.website.baseurl}/pmd_rules_apex_performance.html#operationwithhighcostinloop">
<description>
This rule finds method calls inside loops that are known to be likely a performance issue. These methods should be
called only once before the loop.

Schema class methods like [Schema.getGlobalDescribe()](https://developer.salesforce.com/docs/atlas.en-us.apexref.meta/apexref/apex_methods_system_schema.htm#apex_System_Schema_getGlobalDescribe)
and [Schema.describeSObjects()](https://developer.salesforce.com/docs/atlas.en-us.apexref.meta/apexref/apex_methods_system_schema.htm#apex_System_Schema_describeSObjects)
might be slow depending on the size of your organization. Calling these methods repeatedly inside a loop creates
a potential performance issue.
</description>
<priority>3</priority>
<example><![CDATA[
public class GlobalDescribeExample {
// incorrect example
public void getGlobalDescribeInLoop() {
Set<String> fieldNameSet = new Set<String> {'Id'};
for (String fieldNameOrDefaultValue : fieldNameOrDefaultValueList) {
// Schema.getGlobalDescribe() should be called only once before the for-loop
if (Schema.getGlobalDescribe().get(objectName).getDescribe().fields.getMap().containsKey(fieldNameOrDefaultValue.trim())) {
fieldNameSet.add(fieldNameOrDefaultValue);
}
}
}
// corrected example
public void getGlobalDescribeInLoopCorrected() {
Map<String, Schema.SObjectField> fieldMap = Schema.getGlobalDescribe().get(objectName).getDescribe().fields.getMap();
Set<String> fieldNameSet = new Set<String> {'Id'};
for (String fieldNameOrDefaultValue : fieldNameOrDefaultValueList) {
if (fieldMap.containsKey(fieldNameOrDefaultValue.trim())) {
fieldNameSet.add(fieldNameOrDefaultValue);
}
}
}
}
]]></example>
<example><![CDATA[
public class DescribeSObjectsExample {
// incorrect example
public void describeSObjectsInLoop() {
Set<String> fieldNameSet = new Set<String> {'Id'};
for (String fieldNameOrDefaultValue : fieldNameOrDefaultValueList) {
Schema.DescribeSObjectResult dsr = Account.sObjectType.getDescribe();
if (Schema.describeSObjects(new List<String> { sObjectType })[0].fields.getMap().containsKey(fieldNameOrDefaultValue.trim())) {
fieldNameSet.add(fieldNameOrDefaultValue);
}
}
}
// corrected example
public void describeSObjectsInLoop() {
Map<String, Schema.SObjectField> fieldMap = Schema.describeSObjects(new List<String> { 'Account' })[0].fields.getMap();
Set<String> fieldNameSet = new Set<String> {'Id'};
for (String fieldNameOrDefaultValue : fieldNameOrDefaultValueList) {
if (fieldMap.containsKey(fieldNameOrDefaultValue.trim())) {
fieldNameSet.add(fieldNameOrDefaultValue);
}
}
}
}
]]></example>
</rule>

<rule name="OperationWithLimitsInLoop"
language="apex"
since="6.29.0"
Expand Down Expand Up @@ -275,4 +343,5 @@ public class Something {
]]>
</example>
</rule>

</ruleset>
3 changes: 3 additions & 0 deletions pmd-apex/src/main/resources/rulesets/apex/quickstart.xml
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@
<rule ref="category/apex/performance.xml/OperationWithLimitsInLoop" message="Avoid operations in loops that may hit governor limits">
<priority>3</priority>
</rule>
<rule ref="category/apex/performance.xml/OperationWithHighCostInLoop" message="Avoid operations in loops that can impact performances">
<priority>3</priority>
</rule>
<rule ref="category/apex/errorprone.xml/AvoidDirectAccessTriggerMap" message="Avoid directly accessing Trigger.old and Trigger.new">
<priority>3</priority>
</rule>
Expand Down
Original file line number Diff line number Diff line change
@@ -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.testframework.PmdRuleTst;

class OperationWithHighCostInLoopTest extends PmdRuleTst {
// no additional unit tests
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
<?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">

<!-- Begin Schema method invocations -->
<test-code>
<description>High cost performance getGlobalDescribe in loop (correct code) #4675</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
public void testGetGlobalDescribe() {
Map<String, Schema.SObjectField> fieldMap = Schema.getGlobalDescribe().get(objectName).getDescribe().fields.getMap();
Set<String> fieldNameSet = new Set<String> {'Id'};
for (String fieldNameOrDefaultValue : fieldNameOrDefaultValueList) {
if (fieldMap.containsKey(fieldNameOrDefaultValue.trim())) {
fieldNameSet.add(fieldNameOrDefaultValue);
}
}
}
}
]]></code>
</test-code>

<test-code>
<description>High cost performance getGlobalDescribe in loop #4675</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>5</expected-linenumbers>
<code><![CDATA[
public class Foo {
public void testGetGlobalDescribe() {
Set<String> fieldNameSet = new Set<String> {'Id'};
for (String fieldNameOrDefaultValue : fieldNameOrDefaultValueList) {
if (Schema.getGlobalDescribe().get(objectName).getDescribe().fields.getMap().containsKey(fieldNameOrDefaultValue.trim() )) {
fieldNameSet.add(fieldNameOrDefaultValue);
}
}
}
}
]]></code>
</test-code>

<test-code>
<description>High cost performance getGlobalDescribe in loop - fully qualified #4675</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>5</expected-linenumbers>
<code><![CDATA[
public class Foo {
public void testGetGlobalDescribe() {
Set<String> fieldNameSet = new Set<String> {'Id'};
for (String fieldNameOrDefaultValue : fieldNameOrDefaultValueList) {
if (System.Schema.getGlobalDescribe().get(objectName).getDescribe().fields.getMap().containsKey(fieldNameOrDefaultValue.trim() )) {
fieldNameSet.add(fieldNameOrDefaultValue);
}
}
}
}
]]></code>
</test-code>

<test-code>
<description>High cost performance describeSObjects in loop (correct code) #4675</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
public void testDescribeSObjects() {
Map<String, Schema.SObjectField> fieldMap = Schema.describeSObjects(new List<String> { 'Account' })[0].fields.getMap();
Set<String> fieldNameSet = new Set<String> {'Id'};
for (String fieldNameOrDefaultValue : fieldNameOrDefaultValueList) {
if (fieldMap.containsKey(fieldNameOrDefaultValue.trim())) {
fieldNameSet.add(fieldNameOrDefaultValue);
}
}
}
}
]]></code>
</test-code>

<test-code>
<description>High cost performance describeSObjects in loop #4675</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>6</expected-linenumbers>
<code><![CDATA[
public class Foo {
public void testDescribeSObjects() {
String sObjectType = 'Account';
Set<String> fieldNameSet = new Set<String> {'Id'};
for (String fieldNameOrDefaultValue : fieldNameOrDefaultValueList) {
if (Schema.describeSObjects(new List<String> { sObjectType })[0].fields.getMap().containsKey(fieldNameOrDefaultValue.trim())) {
fieldNameSet.add(fieldNameOrDefaultValue);
}
}
}
}
]]></code>
</test-code>

<test-code>
<description>High cost performance describeSObjects in loop with SObjectDescribeOptions #4675</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>6</expected-linenumbers>
<code><![CDATA[
public class Foo {
public void testDescribeSObjects() {
String sObjectType = 'Account';
Set<String> fieldNameSet = new Set<String> {'Id'};
for (String fieldNameOrDefaultValue : fieldNameOrDefaultValueList) {
if (Schema.describeSObjects(new List<String> { sObjectType }, SObjectDescribeOptions.FULL)[0].fields.getMap().containsKey(fieldNameOrDefaultValue.trim())) {
fieldNameSet.add(fieldNameOrDefaultValue);
}
}
}
}
]]></code>
</test-code>

<test-code>
<description>High cost performance describeSObjects in loop - fully qualified #4675</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>6</expected-linenumbers>
<code><![CDATA[
public class Foo {
public void testDescribeSObjects() {
String sObjectType = 'Account';
Set<String> fieldNameSet = new Set<String> {'Id'};
for (String fieldNameOrDefaultValue : fieldNameOrDefaultValueList) {
if (System.Schema.describeSObjects(new List<String> { sObjectType })[0].fields.getMap().containsKey(fieldNameOrDefaultValue.trim())) {
fieldNameSet.add(fieldNameOrDefaultValue);
}
}
}
}
]]></code>
</test-code>
<!-- End Schema method invocations -->
</test-data>
2 changes: 1 addition & 1 deletion pmd-core/src/main/resources/rulesets/releases/700.xml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ This ruleset contains links to rules that are new in PMD v7.0.0
</description>

<rule ref="category/apex/design.xml/UnusedMethod"/>
<rule ref="category/apex/performance.xml/OperationWithHighCostInLoop"/>

<rule ref="category/java/codestyle.xml/UnnecessaryBoxing"/>
<rule ref="category/java/codestyle.xml/UseExplicitTypes"/>
Expand All @@ -23,4 +24,3 @@ This ruleset contains links to rules that are new in PMD v7.0.0

<rule ref="category/xml/bestpractices.xml/MissingEncoding"/>
</ruleset>

0 comments on commit c5503c9

Please sign in to comment.