[apex] Fix parsing of triggers with declarations

Only the grandchildren of a trigger block were ending up in the tree,
but the direct children of triggerBlock were missing, e.g.
ForLoopStatement. This caused OperationWithHighCostInLoop to not
find the loop anymore in triggers.

This will probably fix other false negatives in triggers in
other rules as well.

Fixes #5139
This commit is contained in:
Andreas Dangel 2024-07-27 20:15:23 +02:00
parent d2fbe14afa
commit 3735fd145b
No known key found for this signature in database
GPG Key ID: 93450DF2DF9A3FA3
8 changed files with 109 additions and 6 deletions

View File

@ -15,6 +15,8 @@ This is a {{ site.pmd.release_type }} release.
### 🚀 New and noteworthy
### 🐛 Fixed Issues
* apex-performance
* [#5139](https://github.com/pmd/pmd/issues/5139): \[apex] OperationWithHighCostInLoop not firing in triggers
* plsql-bestpractices
* [#5132](https://github.com/pmd/pmd/issues/5132): \[plsql] TomKytesDespair - exception for more complex exception handler

View File

@ -10,6 +10,7 @@ import java.util.stream.Collectors;
import net.sourceforge.pmd.lang.document.TextDocument;
import net.sourceforge.pmd.lang.document.TextPos2d;
import net.sourceforge.pmd.lang.document.TextRegion;
import net.sourceforge.pmd.lang.rule.xpath.NoAttribute;
import com.google.summit.ast.SourceLocation;
import com.google.summit.ast.declaration.MethodDeclaration;
@ -27,6 +28,12 @@ public final class ASTMethod extends AbstractApexNode implements ApexQualifiable
*/
private static final String STATIC_INIT_ID = "<clinit>";
/**
* Internal name used by the synthetic trigger method.
* @see #isTriggerBlock()
*/
private static final String TRIGGER_INVOKE_ID = "<invoke>";
// Store the details instead of wrapping a com.google.summit.ast.Node.
// This is to allow synthetic ASTMethod nodes.
// An example is the trigger `invoke` method.
@ -150,4 +157,14 @@ public final class ASTMethod extends AbstractApexNode implements ApexQualifiable
public int getArity() {
return parameterTypes.size();
}
/**
* Checks whether this method is the synthetic trigger method.
* @return true if this method is the synthetic trigger method
* @since 7.5.0
*/
@NoAttribute
public boolean isTriggerBlock() {
return TRIGGER_INVOKE_ID.equals(internalName);
}
}

View File

@ -229,7 +229,7 @@ class ApexTreeBuilder(private val task: ParserTask, private val proc: ApexLangua
// 2. Add the expected ASTModifier child node
buildModifiers(emptyList()).also { it.setParent(invokeMethod) }
// 3. Elide the body CompoundStatement->ASTBlockStatement
node.body.forEach { buildChildren(it, parent = invokeMethod as AbstractApexNode) }
node.body.forEach { buildAndSetParent(it, parent = invokeMethod as AbstractApexNode) }
} else {
buildChildren(node, parent = this, exclude = { it in node.modifiers })
}
@ -737,18 +737,18 @@ class ApexTreeBuilder(private val task: ParserTask, private val proc: ApexLangua
findDescendants(root, nodeType = ASTProperty::class).forEach { node -> generateFields(node) }
// Sort resulting nodes
findDescendants(root, nodeType = ASTUserClass::class).forEach { node ->
findDescendants(root, nodeType = BaseApexClass::class).forEach { node ->
sortUserClassChildren(node)
}
}
/**
* Sort children of [ASTUserClass] in historical order.
* Sort children of [BaseApexClass] (ASTUserClass, ASTUserTrigger, ...) in historical order.
*
* This sorts [ASTField] nodes immediately after [ASTModifierNode] nodes at
* the start of the ordered children.
*/
private fun sortUserClassChildren(node: ASTUserClass) {
private fun sortUserClassChildren(node: BaseApexClass<*>) {
val children = ArrayList(node.children().toList())
children.sortBy{ when (it) {
@ -772,7 +772,13 @@ class ApexTreeBuilder(private val task: ParserTask, private val proc: ApexLangua
/** Generates [ASTField] nodes for the [ASTFieldDeclarationStatements]. */
private fun generateFields(node: ASTFieldDeclarationStatements) {
val parent = node.parent as BaseApexClass<*>
val parent = if (node.parent is BaseApexClass<*>) {
node.parent as BaseApexClass<*>
} else if (node.parent is ASTMethod && (node.parent as ASTMethod).isTriggerBlock) {
node.parent.parent as BaseApexClass<*>
} else {
throw IllegalStateException("Unexpected apex tree - field declaration $node cannot appear hear")
}
node.node.declarations
.map { decl ->

View File

@ -65,4 +65,9 @@ class ApexTreeDumpTest extends BaseTreeDumpTest {
void switchStatements() {
doTest("SwitchStatements");
}
@Test
void trigger() {
doTest("AccountTrigger");
}
}

View File

@ -0,0 +1,9 @@
// see https://github.com/pmd/pmd/issues/5139
trigger AccountTrigger on Account (before insert, before update) {
integer i = 0;
for (i = 0; i <15; i++) {
SObjectType token = Schema.getGlobalDescribe().get('Account');
}
integer anotherField = 2;
System.debug('test');
}

View File

@ -0,0 +1,50 @@
+- ApexFile[@DefiningType = "AccountTrigger", @RealLoc = true]
+- UserTrigger[@DefiningType = "AccountTrigger", @Image = "AccountTrigger", @Nested = false, @RealLoc = true, @SimpleName = "AccountTrigger", @TargetName = "Account", @Usages = (TriggerUsage.BEFORE_INSERT, TriggerUsage.BEFORE_UPDATE)]
+- ModifierNode[@Abstract = false, @DefiningType = "AccountTrigger", @DeprecatedTestMethod = false, @Final = false, @Global = false, @InheritedSharing = false, @Modifiers = 0, @Override = false, @Private = false, @Protected = false, @Public = false, @RealLoc = false, @Static = false, @Test = false, @TestOrTestSetup = false, @Transient = false, @Virtual = false, @WebService = false, @WithSharing = false, @WithoutSharing = false]
+- Field[@DefiningType = "AccountTrigger", @Image = "i", @Name = "i", @RealLoc = true, @Type = "Integer", @Value = "0"]
| +- ModifierNode[@Abstract = false, @DefiningType = "AccountTrigger", @DeprecatedTestMethod = false, @Final = false, @Global = false, @InheritedSharing = false, @Modifiers = 0, @Override = false, @Private = false, @Protected = false, @Public = false, @RealLoc = false, @Static = false, @Test = false, @TestOrTestSetup = false, @Transient = false, @Virtual = false, @WebService = false, @WithSharing = false, @WithoutSharing = false]
+- Field[@DefiningType = "AccountTrigger", @Image = "anotherField", @Name = "anotherField", @RealLoc = true, @Type = "Integer", @Value = "2"]
| +- ModifierNode[@Abstract = false, @DefiningType = "AccountTrigger", @DeprecatedTestMethod = false, @Final = false, @Global = false, @InheritedSharing = false, @Modifiers = 0, @Override = false, @Private = false, @Protected = false, @Public = false, @RealLoc = false, @Static = false, @Test = false, @TestOrTestSetup = false, @Transient = false, @Virtual = false, @WebService = false, @WithSharing = false, @WithoutSharing = false]
+- Method[@Arity = 0, @CanonicalName = "invoke", @Constructor = false, @DefiningType = "AccountTrigger", @Image = "invoke", @RealLoc = false, @ReturnType = "void", @StaticInitializer = false]
+- ModifierNode[@Abstract = false, @DefiningType = "AccountTrigger", @DeprecatedTestMethod = false, @Final = false, @Global = false, @InheritedSharing = false, @Modifiers = 0, @Override = false, @Private = false, @Protected = false, @Public = false, @RealLoc = false, @Static = false, @Test = false, @TestOrTestSetup = false, @Transient = false, @Virtual = false, @WebService = false, @WithSharing = false, @WithoutSharing = false]
+- FieldDeclarationStatements[@DefiningType = "AccountTrigger", @RealLoc = true, @TypeArguments = (), @TypeName = "Integer"]
| +- ModifierNode[@Abstract = false, @DefiningType = "AccountTrigger", @DeprecatedTestMethod = false, @Final = false, @Global = false, @InheritedSharing = false, @Modifiers = 0, @Override = false, @Private = false, @Protected = false, @Public = false, @RealLoc = false, @Static = false, @Test = false, @TestOrTestSetup = false, @Transient = false, @Virtual = false, @WebService = false, @WithSharing = false, @WithoutSharing = false]
| +- FieldDeclaration[@DefiningType = "AccountTrigger", @Image = "i", @Name = "i", @RealLoc = true]
| +- LiteralExpression[@Boolean = false, @Decimal = false, @DefiningType = "AccountTrigger", @Double = false, @Image = "0", @Integer = true, @LiteralType = LiteralType.INTEGER, @Long = false, @Name = null, @Null = false, @RealLoc = true, @String = false]
| +- VariableExpression[@DefiningType = "AccountTrigger", @Image = "i", @RealLoc = true]
| +- EmptyReferenceExpression[@DefiningType = null, @RealLoc = false]
+- ForLoopStatement[@DefiningType = "AccountTrigger", @RealLoc = true]
| +- StandardCondition[@DefiningType = "AccountTrigger", @RealLoc = true]
| | +- BooleanExpression[@DefiningType = "AccountTrigger", @Op = BooleanOperator.LESS_THAN, @RealLoc = true]
| | +- VariableExpression[@DefiningType = "AccountTrigger", @Image = "i", @RealLoc = true]
| | | +- EmptyReferenceExpression[@DefiningType = null, @RealLoc = false]
| | +- LiteralExpression[@Boolean = false, @Decimal = false, @DefiningType = "AccountTrigger", @Double = false, @Image = "15", @Integer = true, @LiteralType = LiteralType.INTEGER, @Long = false, @Name = null, @Null = false, @RealLoc = true, @String = false]
| +- Expression[@DefiningType = "AccountTrigger", @RealLoc = true]
| | +- AssignmentExpression[@DefiningType = "AccountTrigger", @Op = AssignmentOperator.EQUALS, @RealLoc = true]
| | +- VariableExpression[@DefiningType = "AccountTrigger", @Image = "i", @RealLoc = true]
| | | +- EmptyReferenceExpression[@DefiningType = null, @RealLoc = false]
| | +- LiteralExpression[@Boolean = false, @Decimal = false, @DefiningType = "AccountTrigger", @Double = false, @Image = "0", @Integer = true, @LiteralType = LiteralType.INTEGER, @Long = false, @Name = null, @Null = false, @RealLoc = true, @String = false]
| +- BlockStatement[@CurlyBrace = true, @DefiningType = "AccountTrigger", @RealLoc = true]
| | +- VariableDeclarationStatements[@DefiningType = "AccountTrigger", @RealLoc = true]
| | +- ModifierNode[@Abstract = false, @DefiningType = "AccountTrigger", @DeprecatedTestMethod = false, @Final = false, @Global = false, @InheritedSharing = false, @Modifiers = 0, @Override = false, @Private = false, @Protected = false, @Public = false, @RealLoc = false, @Static = false, @Test = false, @TestOrTestSetup = false, @Transient = false, @Virtual = false, @WebService = false, @WithSharing = false, @WithoutSharing = false]
| | +- VariableDeclaration[@DefiningType = "AccountTrigger", @Image = "token", @RealLoc = true, @Type = "SObjectType"]
| | +- MethodCallExpression[@DefiningType = "AccountTrigger", @FullMethodName = "get", @InputParametersSize = 1, @MethodName = "get", @RealLoc = true]
| | | +- ReferenceExpression[@DefiningType = "AccountTrigger", @Image = "", @RealLoc = false, @ReferenceType = ReferenceType.METHOD, @SObjectType = false, @SafeNav = false]
| | | | +- MethodCallExpression[@DefiningType = "AccountTrigger", @FullMethodName = "Schema.getGlobalDescribe", @InputParametersSize = 0, @MethodName = "getGlobalDescribe", @RealLoc = true]
| | | | +- ReferenceExpression[@DefiningType = "AccountTrigger", @Image = "Schema", @RealLoc = true, @ReferenceType = ReferenceType.METHOD, @SObjectType = false, @SafeNav = false]
| | | +- LiteralExpression[@Boolean = false, @Decimal = false, @DefiningType = "AccountTrigger", @Double = false, @Image = "Account", @Integer = false, @LiteralType = LiteralType.STRING, @Long = false, @Name = null, @Null = false, @RealLoc = true, @String = true]
| | +- VariableExpression[@DefiningType = "AccountTrigger", @Image = "token", @RealLoc = true]
| | +- EmptyReferenceExpression[@DefiningType = null, @RealLoc = false]
| +- PostfixExpression[@DefiningType = "AccountTrigger", @Op = PostfixOperator.INCREMENT, @RealLoc = true]
| +- VariableExpression[@DefiningType = "AccountTrigger", @Image = "i", @RealLoc = true]
| +- EmptyReferenceExpression[@DefiningType = null, @RealLoc = false]
+- FieldDeclarationStatements[@DefiningType = "AccountTrigger", @RealLoc = true, @TypeArguments = (), @TypeName = "Integer"]
| +- ModifierNode[@Abstract = false, @DefiningType = "AccountTrigger", @DeprecatedTestMethod = false, @Final = false, @Global = false, @InheritedSharing = false, @Modifiers = 0, @Override = false, @Private = false, @Protected = false, @Public = false, @RealLoc = false, @Static = false, @Test = false, @TestOrTestSetup = false, @Transient = false, @Virtual = false, @WebService = false, @WithSharing = false, @WithoutSharing = false]
| +- FieldDeclaration[@DefiningType = "AccountTrigger", @Image = "anotherField", @Name = "anotherField", @RealLoc = true]
| +- LiteralExpression[@Boolean = false, @Decimal = false, @DefiningType = "AccountTrigger", @Double = false, @Image = "2", @Integer = true, @LiteralType = LiteralType.INTEGER, @Long = false, @Name = null, @Null = false, @RealLoc = true, @String = false]
| +- VariableExpression[@DefiningType = "AccountTrigger", @Image = "anotherField", @RealLoc = true]
| +- EmptyReferenceExpression[@DefiningType = null, @RealLoc = false]
+- ExpressionStatement[@DefiningType = "AccountTrigger", @RealLoc = true]
+- MethodCallExpression[@DefiningType = "AccountTrigger", @FullMethodName = "System.debug", @InputParametersSize = 1, @MethodName = "debug", @RealLoc = true]
+- ReferenceExpression[@DefiningType = "AccountTrigger", @Image = "System", @RealLoc = true, @ReferenceType = ReferenceType.METHOD, @SObjectType = false, @SafeNav = false]
+- LiteralExpression[@Boolean = false, @Decimal = false, @DefiningType = "AccountTrigger", @Double = false, @Image = "test", @Integer = false, @LiteralType = LiteralType.STRING, @Long = false, @Name = null, @Null = false, @RealLoc = true, @String = true]

View File

@ -263,7 +263,7 @@ trigger CaseAssignLevel on CaseAssignLevel__c (after delete, after insert, after
<rule-property name="methodReportLevel">1</rule-property>
<expected-problems>1</expected-problems>
<expected-messages>
<message>The trigger 'CaseAssignLevel' has a cyclomatic complexity of 9.</message>
<message>The trigger 'CaseAssignLevel' has a cyclomatic complexity of 12.</message>
</expected-messages>
<code-ref id="trigger"/>
</test-code>

View File

@ -135,4 +135,18 @@ public class Foo {
]]></code>
</test-code>
<!-- End Schema method invocations -->
<test-code>
<description>#5139 [apex] OperationWithHighCostInLoop not firing in triggers</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>4</expected-linenumbers>
<code><![CDATA[
trigger AccountTrigger on Account (before insert, before update) {
integer i = 0;
for (i = 0; i <15; i++) {
SObjectType token = Schema.getGlobalDescribe().get('Account');
}
}
]]></code>
</test-code>
</test-data>