From 7392501df386ff87218119f59638458cf3f4b785 Mon Sep 17 00:00:00 2001 From: Roopa Mohan Date: Mon, 17 Apr 2023 09:53:40 -0700 Subject: [PATCH 01/10] Outline of GGD in loop rule --- .../salesforce/config/UserFacingMessages.java | 3 +- .../visitor/DefaultNoOpPathVertexVisitor.java | 62 +++++++------------ .../graph/visitor/PathVertexVisitor.java | 50 +++------------ .../rules/GetGlobalDescribeViolationRule.java | 55 ++++++++++++++++ ...GetGlobalDescribeViolationRuleHandler.java | 46 ++++++++++++++ .../GgdLoopDetectionVisitor.java | 34 ++++++++++ .../LoopDetectionVisitor.java | 30 +++++++++ .../GetGlobalDescribeViolationRuleTest.java | 60 ++++++++++++++++++ 8 files changed, 257 insertions(+), 83 deletions(-) create mode 100644 sfge/src/main/java/com/salesforce/rules/GetGlobalDescribeViolationRule.java create mode 100644 sfge/src/main/java/com/salesforce/rules/getglobaldescribe/GetGlobalDescribeViolationRuleHandler.java create mode 100644 sfge/src/main/java/com/salesforce/rules/getglobaldescribe/GgdLoopDetectionVisitor.java create mode 100644 sfge/src/main/java/com/salesforce/rules/getglobaldescribe/LoopDetectionVisitor.java create mode 100644 sfge/src/test/java/com/salesforce/rules/getglobaldescribe/GetGlobalDescribeViolationRuleTest.java diff --git a/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java b/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java index 0c45737c0..4cbaf1030 100644 --- a/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java +++ b/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java @@ -9,11 +9,12 @@ public final class UserFacingMessages { public static final class RuleDescriptions { public static final String APEX_NULL_POINTER_EXCEPTION_RULE = - "Identfies Apex operations that dereference null objects and throw NullPointerExceptions."; + "Identifies Apex operations that dereference null objects and throw NullPointerExceptions."; public static final String UNIMPLEMENTED_TYPE_RULE = "Identifies abstract classes and interfaces that are non-global and don't have implementations or extensions."; public static final String UNUSED_METHOD_RULE = "Identifies methods that aren't invoked from recognized entry points."; + public static final String GET_GLOBAL_DESCRIBE_VIOLATION_RULE = "Detects Schema.getGlobalDescribe() calls made in scenarios that could result in performance degradation."; } public static final class RuleViolationTemplates { diff --git a/sfge/src/main/java/com/salesforce/graph/visitor/DefaultNoOpPathVertexVisitor.java b/sfge/src/main/java/com/salesforce/graph/visitor/DefaultNoOpPathVertexVisitor.java index 66ac55361..743f9bf3c 100644 --- a/sfge/src/main/java/com/salesforce/graph/visitor/DefaultNoOpPathVertexVisitor.java +++ b/sfge/src/main/java/com/salesforce/graph/visitor/DefaultNoOpPathVertexVisitor.java @@ -3,47 +3,7 @@ import com.salesforce.graph.ApexPath; import com.salesforce.graph.symbols.DefaultNoOpScopeVisitor; import com.salesforce.graph.symbols.SymbolProvider; -import com.salesforce.graph.vertex.AssignmentExpressionVertex; -import com.salesforce.graph.vertex.BaseSFVertex; -import com.salesforce.graph.vertex.BlockStatementVertex; -import com.salesforce.graph.vertex.CatchBlockStatementVertex; -import com.salesforce.graph.vertex.DmlDeleteStatementVertex; -import com.salesforce.graph.vertex.DmlInsertStatementVertex; -import com.salesforce.graph.vertex.DmlMergeStatementVertex; -import com.salesforce.graph.vertex.DmlUndeleteStatementVertex; -import com.salesforce.graph.vertex.DmlUpdateStatementVertex; -import com.salesforce.graph.vertex.DmlUpsertStatementVertex; -import com.salesforce.graph.vertex.ElseWhenBlockVertex; -import com.salesforce.graph.vertex.EmptyReferenceExpressionVertex; -import com.salesforce.graph.vertex.ExpressionStatementVertex; -import com.salesforce.graph.vertex.FieldDeclarationStatementsVertex; -import com.salesforce.graph.vertex.FieldDeclarationVertex; -import com.salesforce.graph.vertex.FieldVertex; -import com.salesforce.graph.vertex.ForEachStatementVertex; -import com.salesforce.graph.vertex.ForLoopStatementVertex; -import com.salesforce.graph.vertex.IfBlockStatementVertex; -import com.salesforce.graph.vertex.IfElseBlockStatementVertex; -import com.salesforce.graph.vertex.LiteralExpressionVertex; -import com.salesforce.graph.vertex.MethodCallExpressionVertex; -import com.salesforce.graph.vertex.MethodVertex; -import com.salesforce.graph.vertex.ModifierNodeVertex; -import com.salesforce.graph.vertex.NewKeyValueObjectExpressionVertex; -import com.salesforce.graph.vertex.NewListLiteralExpressionVertex; -import com.salesforce.graph.vertex.NewObjectExpressionVertex; -import com.salesforce.graph.vertex.ParameterVertex; -import com.salesforce.graph.vertex.PrefixExpressionVertex; -import com.salesforce.graph.vertex.ReferenceExpressionVertex; -import com.salesforce.graph.vertex.ReturnStatementVertex; -import com.salesforce.graph.vertex.SoqlExpressionVertex; -import com.salesforce.graph.vertex.StandardConditionVertex; -import com.salesforce.graph.vertex.SuperMethodCallExpressionVertex; -import com.salesforce.graph.vertex.SwitchStatementVertex; -import com.salesforce.graph.vertex.ThrowStatementVertex; -import com.salesforce.graph.vertex.TryCatchFinallyBlockStatementVertex; -import com.salesforce.graph.vertex.ValueWhenBlockVertex; -import com.salesforce.graph.vertex.VariableDeclarationStatementsVertex; -import com.salesforce.graph.vertex.VariableDeclarationVertex; -import com.salesforce.graph.vertex.VariableExpressionVertex; +import com.salesforce.graph.vertex.*; /** * A visitor that does nothing, useful for deriving visitors. Delegates to {@link @@ -284,6 +244,11 @@ public boolean visit(VariableExpressionVertex.Single vertex, SymbolProvider symb @Override public void afterVisit(BaseSFVertex vertex, SymbolProvider symbols) {} + @Override + public void afterVisit(DoLoopStatementVertex vertex, SymbolProvider symbols) { + + } + @Override public void afterVisit(DmlDeleteStatementVertex vertex, SymbolProvider symbols) {} @@ -302,6 +267,16 @@ public void afterVisit(DmlUpdateStatementVertex vertex, SymbolProvider symbols) @Override public void afterVisit(DmlUpsertStatementVertex vertex, SymbolProvider symbols) {} + @Override + public void afterVisit(ForEachStatementVertex vertex, SymbolProvider symbols) { + + } + + @Override + public void afterVisit(ForLoopStatementVertex vertex, SymbolProvider symbols) { + + } + @Override public void afterVisit(FieldDeclarationVertex vertex, SymbolProvider symbols) {} @@ -322,4 +297,9 @@ public void afterVisit(StandardConditionVertex.Positive vertex, SymbolProvider s @Override public void afterVisit(ThrowStatementVertex vertex, SymbolProvider symbols) {} + + @Override + public void afterVisit(WhileLoopStatementVertex vertex, SymbolProvider symbols) { + + } } diff --git a/sfge/src/main/java/com/salesforce/graph/visitor/PathVertexVisitor.java b/sfge/src/main/java/com/salesforce/graph/visitor/PathVertexVisitor.java index c79390204..1be9cea7b 100644 --- a/sfge/src/main/java/com/salesforce/graph/visitor/PathVertexVisitor.java +++ b/sfge/src/main/java/com/salesforce/graph/visitor/PathVertexVisitor.java @@ -2,47 +2,7 @@ import com.salesforce.graph.ApexPath; import com.salesforce.graph.symbols.SymbolProvider; -import com.salesforce.graph.vertex.AssignmentExpressionVertex; -import com.salesforce.graph.vertex.BaseSFVertex; -import com.salesforce.graph.vertex.BlockStatementVertex; -import com.salesforce.graph.vertex.CatchBlockStatementVertex; -import com.salesforce.graph.vertex.DmlDeleteStatementVertex; -import com.salesforce.graph.vertex.DmlInsertStatementVertex; -import com.salesforce.graph.vertex.DmlMergeStatementVertex; -import com.salesforce.graph.vertex.DmlUndeleteStatementVertex; -import com.salesforce.graph.vertex.DmlUpdateStatementVertex; -import com.salesforce.graph.vertex.DmlUpsertStatementVertex; -import com.salesforce.graph.vertex.ElseWhenBlockVertex; -import com.salesforce.graph.vertex.EmptyReferenceExpressionVertex; -import com.salesforce.graph.vertex.ExpressionStatementVertex; -import com.salesforce.graph.vertex.FieldDeclarationStatementsVertex; -import com.salesforce.graph.vertex.FieldDeclarationVertex; -import com.salesforce.graph.vertex.FieldVertex; -import com.salesforce.graph.vertex.ForEachStatementVertex; -import com.salesforce.graph.vertex.ForLoopStatementVertex; -import com.salesforce.graph.vertex.IfBlockStatementVertex; -import com.salesforce.graph.vertex.IfElseBlockStatementVertex; -import com.salesforce.graph.vertex.LiteralExpressionVertex; -import com.salesforce.graph.vertex.MethodCallExpressionVertex; -import com.salesforce.graph.vertex.MethodVertex; -import com.salesforce.graph.vertex.ModifierNodeVertex; -import com.salesforce.graph.vertex.NewKeyValueObjectExpressionVertex; -import com.salesforce.graph.vertex.NewListLiteralExpressionVertex; -import com.salesforce.graph.vertex.NewObjectExpressionVertex; -import com.salesforce.graph.vertex.ParameterVertex; -import com.salesforce.graph.vertex.PrefixExpressionVertex; -import com.salesforce.graph.vertex.ReferenceExpressionVertex; -import com.salesforce.graph.vertex.ReturnStatementVertex; -import com.salesforce.graph.vertex.SoqlExpressionVertex; -import com.salesforce.graph.vertex.StandardConditionVertex; -import com.salesforce.graph.vertex.SuperMethodCallExpressionVertex; -import com.salesforce.graph.vertex.SwitchStatementVertex; -import com.salesforce.graph.vertex.ThrowStatementVertex; -import com.salesforce.graph.vertex.TryCatchFinallyBlockStatementVertex; -import com.salesforce.graph.vertex.ValueWhenBlockVertex; -import com.salesforce.graph.vertex.VariableDeclarationStatementsVertex; -import com.salesforce.graph.vertex.VariableDeclarationVertex; -import com.salesforce.graph.vertex.VariableExpressionVertex; +import com.salesforce.graph.vertex.*; /** * Visits vertices in a particular path. Return true if the visitor does not want to visit the @@ -147,6 +107,8 @@ void recursionDetected( void afterVisit(BaseSFVertex vertex, SymbolProvider symbols); + void afterVisit(DoLoopStatementVertex vertex, SymbolProvider symbols); + void afterVisit(DmlDeleteStatementVertex vertex, SymbolProvider symbols); void afterVisit(DmlInsertStatementVertex vertex, SymbolProvider symbols); @@ -159,6 +121,10 @@ void recursionDetected( void afterVisit(DmlUpsertStatementVertex vertex, SymbolProvider symbols); + void afterVisit(ForEachStatementVertex vertex, SymbolProvider symbols); + + void afterVisit(ForLoopStatementVertex vertex, SymbolProvider symbols); + void afterVisit(FieldDeclarationVertex vertex, SymbolProvider symbols); void afterVisit(MethodCallExpressionVertex vertex, SymbolProvider symbols); @@ -172,4 +138,6 @@ void recursionDetected( void afterVisit(StandardConditionVertex.Positive vertex, SymbolProvider symbols); void afterVisit(ThrowStatementVertex vertex, SymbolProvider symbols); + + void afterVisit(WhileLoopStatementVertex vertex, SymbolProvider symbols); } diff --git a/sfge/src/main/java/com/salesforce/rules/GetGlobalDescribeViolationRule.java b/sfge/src/main/java/com/salesforce/rules/GetGlobalDescribeViolationRule.java new file mode 100644 index 000000000..b066ec309 --- /dev/null +++ b/sfge/src/main/java/com/salesforce/rules/GetGlobalDescribeViolationRule.java @@ -0,0 +1,55 @@ +package com.salesforce.rules; + +import com.salesforce.config.UserFacingMessages; +import com.salesforce.graph.ApexPath; +import com.salesforce.graph.vertex.BaseSFVertex; +import com.salesforce.rules.getglobaldescribe.GetGlobalDescribeViolationRuleHandler; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; + +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; + +public class GetGlobalDescribeViolationRule extends AbstractPathTraversalRule { + + private final GetGlobalDescribeViolationRuleHandler ruleHandler; + + public GetGlobalDescribeViolationRule() { + ruleHandler = GetGlobalDescribeViolationRuleHandler.getInstance(); + } + + @Override + public boolean test(BaseSFVertex vertex) { + return ruleHandler.test(vertex); + } + + @Override + protected List _run(GraphTraversalSource g, ApexPath path, BaseSFVertex vertex) { + final Set violations = ruleHandler.detectViolations(g, path, vertex); + return violations.stream().collect(Collectors.toList()); + } + + @Override + protected int getSeverity() { + return SEVERITY.HIGH.code; + } + + @Override + protected String getDescription() { + return UserFacingMessages.RuleDescriptions.GET_GLOBAL_DESCRIBE_VIOLATION_RULE; + } + + @Override + protected String getCategory() { + return CATEGORY.PERFORMANCE.name; + } + + public static GetGlobalDescribeViolationRule getInstance() { + return GetGlobalDescribeViolationRule.LazyHolder.INSTANCE; + } + + private static final class LazyHolder { + // Postpone initialization until first use + private static final GetGlobalDescribeViolationRule INSTANCE = new GetGlobalDescribeViolationRule(); + } +} diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/GetGlobalDescribeViolationRuleHandler.java b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/GetGlobalDescribeViolationRuleHandler.java new file mode 100644 index 000000000..428879f09 --- /dev/null +++ b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/GetGlobalDescribeViolationRuleHandler.java @@ -0,0 +1,46 @@ +package com.salesforce.rules.getglobaldescribe; + +import com.salesforce.exception.ProgrammingException; +import com.salesforce.graph.ApexPath; +import com.salesforce.graph.symbols.DefaultSymbolProviderVertexVisitor; +import com.salesforce.graph.vertex.BaseSFVertex; +import com.salesforce.graph.vertex.MethodCallExpressionVertex; +import com.salesforce.graph.visitor.ApexPathWalker; +import com.salesforce.rules.Violation; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; + +import java.util.Set; + +public class GetGlobalDescribeViolationRuleHandler { + + private static final String METHOD_SCHEMA_GET_GLOBAL_DESCRIBE = "Schema.getGlobalDescribe"; + + public boolean test(BaseSFVertex vertex) { + return vertex instanceof MethodCallExpressionVertex && isGetGlobalDescribeMethod((MethodCallExpressionVertex) vertex); + } + + public Set detectViolations(GraphTraversalSource g, ApexPath path, BaseSFVertex vertex) { + if (!(vertex instanceof MethodCallExpressionVertex)) { + throw new ProgrammingException("GetGlobalDescribeViolationRule unexpected invoked on an instance that's not MethodCallExpressionVertex. vertex=" + vertex); + } + final GgdLoopDetectionVisitor ruleVisitor = new GgdLoopDetectionVisitor((MethodCallExpressionVertex) vertex); + // TODO: I'm expecting to add other visitors depending on the other factors we will analyze to decide if a GGD call is not performant. + DefaultSymbolProviderVertexVisitor symbols = new DefaultSymbolProviderVertexVisitor(g); + ApexPathWalker.walkPath(g, path, ruleVisitor, symbols); + + return ruleVisitor.getViolations(); + } + + private static boolean isGetGlobalDescribeMethod(MethodCallExpressionVertex vertex) { + return METHOD_SCHEMA_GET_GLOBAL_DESCRIBE.equalsIgnoreCase(vertex.getFullMethodName()); + } + + public static GetGlobalDescribeViolationRuleHandler getInstance() { + return GetGlobalDescribeViolationRuleHandler.LazyHolder.INSTANCE; + } + + private static final class LazyHolder { + // Postpone initialization until first use + private static final GetGlobalDescribeViolationRuleHandler INSTANCE = new GetGlobalDescribeViolationRuleHandler(); + } +} diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/GgdLoopDetectionVisitor.java b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/GgdLoopDetectionVisitor.java new file mode 100644 index 000000000..4fea02603 --- /dev/null +++ b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/GgdLoopDetectionVisitor.java @@ -0,0 +1,34 @@ +package com.salesforce.rules.getglobaldescribe; + +import com.salesforce.graph.symbols.SymbolProvider; +import com.salesforce.graph.vertex.BaseSFVertex; +import com.salesforce.graph.vertex.MethodCallExpressionVertex; +import com.salesforce.rules.Violation; + +import java.util.HashSet; +import java.util.Set; + +class GgdLoopDetectionVisitor extends LoopDetectionVisitor { + private static final String VIOLATION_MESSAGE = "%s should not be invoked within a loop. Detected presence in %s loop"; + private final Set violations; + private final MethodCallExpressionVertex targetVertex; + + GgdLoopDetectionVisitor(MethodCallExpressionVertex targetVertex) { + violations = new HashSet<>(); + this.targetVertex = targetVertex; + } + + Set getViolations() { + return new HashSet<>(violations); + } + + @Override + void execAfterLoopVertexVisit(BaseSFVertex vertex, SymbolProvider symbols) { + // TODO: source vertex is incorrect + violations.add(new Violation.PathBasedRuleViolation(getViolationMessage(vertex), vertex, targetVertex)); + } + + private String getViolationMessage(BaseSFVertex vertex) { + return String.format(VIOLATION_MESSAGE, targetVertex.getFullMethodName(), vertex.getLabel()); + } +} diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/LoopDetectionVisitor.java b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/LoopDetectionVisitor.java new file mode 100644 index 000000000..d5290fca2 --- /dev/null +++ b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/LoopDetectionVisitor.java @@ -0,0 +1,30 @@ +package com.salesforce.rules.getglobaldescribe; + +import com.salesforce.graph.symbols.SymbolProvider; +import com.salesforce.graph.vertex.*; +import com.salesforce.graph.visitor.DefaultNoOpPathVertexVisitor; + +abstract class LoopDetectionVisitor extends DefaultNoOpPathVertexVisitor { + + abstract void execAfterLoopVertexVisit(BaseSFVertex vertex, SymbolProvider symbols); + + @Override + public void afterVisit(DoLoopStatementVertex vertex, SymbolProvider symbols) { + execAfterLoopVertexVisit(vertex, symbols); + } + + @Override + public void afterVisit(ForEachStatementVertex vertex, SymbolProvider symbols) { + execAfterLoopVertexVisit(vertex, symbols); + } + + @Override + public void afterVisit(ForLoopStatementVertex vertex, SymbolProvider symbols) { + execAfterLoopVertexVisit(vertex, symbols); + } + + @Override + public void afterVisit(WhileLoopStatementVertex vertex, SymbolProvider symbols) { + execAfterLoopVertexVisit(vertex, symbols); + } +} diff --git a/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/GetGlobalDescribeViolationRuleTest.java b/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/GetGlobalDescribeViolationRuleTest.java new file mode 100644 index 000000000..5a4cecb16 --- /dev/null +++ b/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/GetGlobalDescribeViolationRuleTest.java @@ -0,0 +1,60 @@ +package com.salesforce.rules.getglobaldescribe; + +import com.salesforce.rules.GetGlobalDescribeViolationRule; +import com.salesforce.testutils.BasePathBasedRuleTest; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +//TODO: break down test, add updated assertions +public class GetGlobalDescribeViolationRuleTest extends BasePathBasedRuleTest { + + private static final GetGlobalDescribeViolationRule RULE = GetGlobalDescribeViolationRule.getInstance(); + + @CsvSource({ + "ForEach, for (String s : myList)", + "For, for (Integer i; i < s.size; s++)", + "While, while(true)" + }) + @ParameterizedTest(name = "{displayName}: {0}") + public void testSimpleGgdInLoop(String testName, String loopStructure) { + // spotless:off + String sourceCode = + "public class MyClass {\n" + + " void foo() {\n" + + " List myList = new String[] {'Account','Contact'};\n" + + " " + loopStructure + " {\n" + + " Schema.getGlobalDescribe();\n" + + " }\n" + + " }\n" + + "}\n"; + // spotless:on + + assertNoViolation(RULE, sourceCode); + } + + @CsvSource({ + "ForEach, for (String s : myList)", + "For, for (Integer i; i < s.size; s++)", + "While, while(true)" + }) + @ParameterizedTest(name = "{displayName}: {0}") + public void testGgdInLoopInMethodCall(String testName, String loopStructure) { + // spotless:off + String sourceCode = + "public class MyClass {\n" + + " void foo() {\n" + + " List myList = new String[] {'Account','Contact'};\n" + + " " + loopStructure + " {\n" + + " getMoreInfo();\n" + + " }\n" + + " }\n" + + " void getMoreInfo() {\n" + + " Schema.getGlobalDescribe();\n" + + " }\n" + + "}\n"; + // spotless:on + + assertNoViolation(RULE, sourceCode); + } + +} From 432d5a71e551ea7b68bedd279309168b9a19538c Mon Sep 17 00:00:00 2001 From: Roopa Mohan Date: Wed, 19 Apr 2023 09:42:45 -0700 Subject: [PATCH 02/10] Basics + tests before changing violation generation --- .../salesforce/config/UserFacingMessages.java | 8 + .../rules/AvoidMultipleMassSchemaLookup.java | 67 +++++++ .../rules/GetGlobalDescribeViolationRule.java | 55 ----- .../salesforce/rules/PathBasedRuleRunner.java | 14 ++ .../fls/apex/operations/FlsViolationInfo.java | 23 +++ ...AvoidMultipleMassSchemaLookupHandler.java} | 30 ++- .../GgdLoopDetectionVisitor.java | 34 ---- .../LoopDetectionVisitor.java | 3 + .../MassSchemaLookupViolationInfo.java | 86 ++++++++ .../MultipleMassSchemaLookupVisitor.java | 30 +++ .../AvoidMultipleMassSchemaLookupTest.java | 188 ++++++++++++++++++ .../GetGlobalDescribeViolationRuleTest.java | 60 ------ 12 files changed, 439 insertions(+), 159 deletions(-) create mode 100644 sfge/src/main/java/com/salesforce/rules/AvoidMultipleMassSchemaLookup.java delete mode 100644 sfge/src/main/java/com/salesforce/rules/GetGlobalDescribeViolationRule.java rename sfge/src/main/java/com/salesforce/rules/getglobaldescribe/{GetGlobalDescribeViolationRuleHandler.java => AvoidMultipleMassSchemaLookupHandler.java} (58%) delete mode 100644 sfge/src/main/java/com/salesforce/rules/getglobaldescribe/GgdLoopDetectionVisitor.java create mode 100644 sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupViolationInfo.java create mode 100644 sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MultipleMassSchemaLookupVisitor.java create mode 100644 sfge/src/test/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupTest.java delete mode 100644 sfge/src/test/java/com/salesforce/rules/getglobaldescribe/GetGlobalDescribeViolationRuleTest.java diff --git a/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java b/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java index 4cbaf1030..c2a01ded1 100644 --- a/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java +++ b/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java @@ -76,4 +76,12 @@ public static final class CompilationErrors { "Graph engine encountered compilation errors. Fix the errors in %s and retry."; public static final String EXCEPTION_FORMAT_TEMPLATE = "%s, Caused by:\n%s"; } + + public static final class GetGlobalDescribeTemplates { + public static final String INVOKED_IN_A_LOOP_TEXT = "invoked in a loop"; + public static final String INVOKED_IN_A_LOOP_ADDITIONAL_INFO = ". Loop started at %s:%d."; // file name, line number + public static final String INVOKED_MULTIPLE_TIMES_TEXT = "invoked multiple times"; + public static final String INVOKED_MULTIPLE_TIMES_ADDITIONAL_INFO = ". Previous invocation occurred at %s:%d."; // file name, line number + public static final String MESSAGE_TEMPLATE = "Detected %s %s. %s"; // method name, type-specific-text, type-specific-additional-info + } } diff --git a/sfge/src/main/java/com/salesforce/rules/AvoidMultipleMassSchemaLookup.java b/sfge/src/main/java/com/salesforce/rules/AvoidMultipleMassSchemaLookup.java new file mode 100644 index 000000000..a3a05c787 --- /dev/null +++ b/sfge/src/main/java/com/salesforce/rules/AvoidMultipleMassSchemaLookup.java @@ -0,0 +1,67 @@ +package com.salesforce.rules; + +import com.salesforce.config.UserFacingMessages; +import com.salesforce.graph.ApexPath; +import com.salesforce.graph.vertex.BaseSFVertex; +import com.salesforce.rules.getglobaldescribe.MassSchemaLookupViolationInfo; +import com.salesforce.rules.getglobaldescribe.AvoidMultipleMassSchemaLookupHandler; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; + +import java.util.ArrayList; +import java.util.List; +import java.util.Set; + +/** + * Rule to detect possible performance degradations while invoking Schema.getGlobalDescribe(). + */ +public class AvoidMultipleMassSchemaLookup extends AbstractPathTraversalRule { + + private final AvoidMultipleMassSchemaLookupHandler ruleHandler; + + private AvoidMultipleMassSchemaLookup() { + ruleHandler = AvoidMultipleMassSchemaLookupHandler.getInstance(); + } + + @Override + public boolean test(BaseSFVertex vertex) { + return ruleHandler.test(vertex); + } + + @Override + protected List _run(GraphTraversalSource g, ApexPath path, BaseSFVertex vertex) { + MassSchemaLookupViolationInfo massSchemaLookupViolationInfos = ruleHandler.detectViolations(g, path, vertex); + List violations = new ArrayList<>(); + // TODO: do further processing here + violations.add(massSchemaLookupViolationInfos); + return violations; + } + + @Override + protected int getSeverity() { + return SEVERITY.HIGH.code; + } + + @Override + protected String getDescription() { + return UserFacingMessages.RuleDescriptions.GET_GLOBAL_DESCRIBE_VIOLATION_RULE; + } + + @Override + protected String getCategory() { + return CATEGORY.PERFORMANCE.name; + } + + @Override + protected boolean isEnabled() { + return false; + } + + public static AvoidMultipleMassSchemaLookup getInstance() { + return AvoidMultipleMassSchemaLookup.LazyHolder.INSTANCE; + } + + private static final class LazyHolder { + // Postpone initialization until first use + private static final AvoidMultipleMassSchemaLookup INSTANCE = new AvoidMultipleMassSchemaLookup(); + } +} diff --git a/sfge/src/main/java/com/salesforce/rules/GetGlobalDescribeViolationRule.java b/sfge/src/main/java/com/salesforce/rules/GetGlobalDescribeViolationRule.java deleted file mode 100644 index b066ec309..000000000 --- a/sfge/src/main/java/com/salesforce/rules/GetGlobalDescribeViolationRule.java +++ /dev/null @@ -1,55 +0,0 @@ -package com.salesforce.rules; - -import com.salesforce.config.UserFacingMessages; -import com.salesforce.graph.ApexPath; -import com.salesforce.graph.vertex.BaseSFVertex; -import com.salesforce.rules.getglobaldescribe.GetGlobalDescribeViolationRuleHandler; -import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; - -import java.util.List; -import java.util.Set; -import java.util.stream.Collectors; - -public class GetGlobalDescribeViolationRule extends AbstractPathTraversalRule { - - private final GetGlobalDescribeViolationRuleHandler ruleHandler; - - public GetGlobalDescribeViolationRule() { - ruleHandler = GetGlobalDescribeViolationRuleHandler.getInstance(); - } - - @Override - public boolean test(BaseSFVertex vertex) { - return ruleHandler.test(vertex); - } - - @Override - protected List _run(GraphTraversalSource g, ApexPath path, BaseSFVertex vertex) { - final Set violations = ruleHandler.detectViolations(g, path, vertex); - return violations.stream().collect(Collectors.toList()); - } - - @Override - protected int getSeverity() { - return SEVERITY.HIGH.code; - } - - @Override - protected String getDescription() { - return UserFacingMessages.RuleDescriptions.GET_GLOBAL_DESCRIBE_VIOLATION_RULE; - } - - @Override - protected String getCategory() { - return CATEGORY.PERFORMANCE.name; - } - - public static GetGlobalDescribeViolationRule getInstance() { - return GetGlobalDescribeViolationRule.LazyHolder.INSTANCE; - } - - private static final class LazyHolder { - // Postpone initialization until first use - private static final GetGlobalDescribeViolationRule INSTANCE = new GetGlobalDescribeViolationRule(); - } -} diff --git a/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java b/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java index f3f7c8fa8..a85009c9a 100644 --- a/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java +++ b/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java @@ -13,6 +13,7 @@ import com.salesforce.graph.vertex.SFVertex; import com.salesforce.rules.fls.apex.operations.FlsViolationInfo; import com.salesforce.rules.fls.apex.operations.FlsViolationMessageUtil; +import com.salesforce.rules.getglobaldescribe.MassSchemaLookupViolationInfo; import com.salesforce.rules.ops.ProgressListener; import com.salesforce.rules.ops.ProgressListenerProvider; import java.util.*; @@ -120,6 +121,7 @@ private void executeRulesOnPaths(List paths) { // time, and // require post-processing. final HashSet incompleteThrowables = new HashSet<>(); + final HashSet tempDeleteMe = new HashSet<>(); // For each path... for (ApexPath path : paths) { // If the path's metadata is present... @@ -143,6 +145,8 @@ private void executeRulesOnPaths(List paths) { // objects. if (ruleThrowable instanceof FlsViolationInfo) { incompleteThrowables.add((FlsViolationInfo) ruleThrowable); + } else if (ruleThrowable instanceof MassSchemaLookupViolationInfo) { + tempDeleteMe.add((MassSchemaLookupViolationInfo) ruleThrowable); } else if (ruleThrowable instanceof Violation) { // If the violation is done, it can just go directly into the results // set. @@ -156,6 +160,7 @@ private void executeRulesOnPaths(List paths) { } convertToViolations(incompleteThrowables); + convertGgdToViolations(tempDeleteMe); if (!foundVertex) { // If no vertices were found, we should log something so that information isn't lost, @@ -167,6 +172,15 @@ private void executeRulesOnPaths(List paths) { } } + private void convertGgdToViolations(HashSet ggdViolationInfos) { + // TODO: consolidate by sink/source + + for (MassSchemaLookupViolationInfo ggdViolationInfo: ggdViolationInfos) { + violations.add(ggdViolationInfo.convert()); + } + } + + // TODO: Restructure to make this logic work on other types of violation info too private void convertToViolations(HashSet flsViolationInfos) { // Consolidate/regroup FLS violations across paths so that there are no // duplicates with different field sets for the same source/sink/dmlOperation diff --git a/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationInfo.java b/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationInfo.java index 07d8aadae..c58e319dc 100644 --- a/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationInfo.java +++ b/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationInfo.java @@ -3,9 +3,13 @@ import com.google.common.base.Objects; import com.salesforce.collections.CollectionUtil; import com.salesforce.config.UserFacingMessages; +import com.salesforce.exception.ProgrammingException; +import com.salesforce.exception.UnexpectedException; import com.salesforce.graph.vertex.SFVertex; import com.salesforce.rules.AbstractRule; import com.salesforce.rules.RuleThrowable; +import com.salesforce.rules.Violation; + import java.util.Optional; import java.util.TreeSet; @@ -167,4 +171,23 @@ public String toString() { public FlsViolationInfo deepClone() { return new FlsViolationInfo(this); } + +// @Override + public Violation convert() { + final String violationMessage = + FlsViolationMessageUtil.constructMessage(this); + final Optional sourceVertex = this.getSourceVertex(); + final Optional sinkVertex = this.getSinkVertex(); + if (sinkVertex.isPresent()) { + final Violation.RuleViolation ruleViolation = + new Violation.PathBasedRuleViolation( + violationMessage, sourceVertex.get(), sinkVertex.get()); + ruleViolation.setPropertiesFromRule(rule); + return ruleViolation; + } else { + throw new ProgrammingException( + "Sink vertex not set in flsViolationInfo: " + + this); + } + } } diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/GetGlobalDescribeViolationRuleHandler.java b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupHandler.java similarity index 58% rename from sfge/src/main/java/com/salesforce/rules/getglobaldescribe/GetGlobalDescribeViolationRuleHandler.java rename to sfge/src/main/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupHandler.java index 428879f09..05a025d85 100644 --- a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/GetGlobalDescribeViolationRuleHandler.java +++ b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupHandler.java @@ -5,42 +5,52 @@ import com.salesforce.graph.symbols.DefaultSymbolProviderVertexVisitor; import com.salesforce.graph.vertex.BaseSFVertex; import com.salesforce.graph.vertex.MethodCallExpressionVertex; +import com.salesforce.graph.vertex.SFVertex; import com.salesforce.graph.visitor.ApexPathWalker; -import com.salesforce.rules.Violation; +import com.salesforce.rules.AvoidMultipleMassSchemaLookup; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; -import java.util.Set; - -public class GetGlobalDescribeViolationRuleHandler { +/** + * Executes internals of {@link AvoidMultipleMassSchemaLookup} + */ +public class AvoidMultipleMassSchemaLookupHandler { private static final String METHOD_SCHEMA_GET_GLOBAL_DESCRIBE = "Schema.getGlobalDescribe"; + /** + * @param vertex to consider for analysis + * @return true if the vertex parameter requires to be treated as a target vertex for {@link AvoidMultipleMassSchemaLookup}. + */ public boolean test(BaseSFVertex vertex) { return vertex instanceof MethodCallExpressionVertex && isGetGlobalDescribeMethod((MethodCallExpressionVertex) vertex); } - public Set detectViolations(GraphTraversalSource g, ApexPath path, BaseSFVertex vertex) { + public MassSchemaLookupViolationInfo detectViolations(GraphTraversalSource g, ApexPath path, BaseSFVertex vertex) { if (!(vertex instanceof MethodCallExpressionVertex)) { throw new ProgrammingException("GetGlobalDescribeViolationRule unexpected invoked on an instance that's not MethodCallExpressionVertex. vertex=" + vertex); } - final GgdLoopDetectionVisitor ruleVisitor = new GgdLoopDetectionVisitor((MethodCallExpressionVertex) vertex); + + final SFVertex sourceVertex = path.getMethodVertex().orElse(null); + + final MultipleMassSchemaLookupVisitor ruleVisitor = new MultipleMassSchemaLookupVisitor(sourceVertex, (MethodCallExpressionVertex) vertex); // TODO: I'm expecting to add other visitors depending on the other factors we will analyze to decide if a GGD call is not performant. DefaultSymbolProviderVertexVisitor symbols = new DefaultSymbolProviderVertexVisitor(g); ApexPathWalker.walkPath(g, path, ruleVisitor, symbols); - return ruleVisitor.getViolations(); + // Once it finishes walking, collect any violations thrown + return ruleVisitor.getViolationInfo(); } private static boolean isGetGlobalDescribeMethod(MethodCallExpressionVertex vertex) { return METHOD_SCHEMA_GET_GLOBAL_DESCRIBE.equalsIgnoreCase(vertex.getFullMethodName()); } - public static GetGlobalDescribeViolationRuleHandler getInstance() { - return GetGlobalDescribeViolationRuleHandler.LazyHolder.INSTANCE; + public static AvoidMultipleMassSchemaLookupHandler getInstance() { + return AvoidMultipleMassSchemaLookupHandler.LazyHolder.INSTANCE; } private static final class LazyHolder { // Postpone initialization until first use - private static final GetGlobalDescribeViolationRuleHandler INSTANCE = new GetGlobalDescribeViolationRuleHandler(); + private static final AvoidMultipleMassSchemaLookupHandler INSTANCE = new AvoidMultipleMassSchemaLookupHandler(); } } diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/GgdLoopDetectionVisitor.java b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/GgdLoopDetectionVisitor.java deleted file mode 100644 index 4fea02603..000000000 --- a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/GgdLoopDetectionVisitor.java +++ /dev/null @@ -1,34 +0,0 @@ -package com.salesforce.rules.getglobaldescribe; - -import com.salesforce.graph.symbols.SymbolProvider; -import com.salesforce.graph.vertex.BaseSFVertex; -import com.salesforce.graph.vertex.MethodCallExpressionVertex; -import com.salesforce.rules.Violation; - -import java.util.HashSet; -import java.util.Set; - -class GgdLoopDetectionVisitor extends LoopDetectionVisitor { - private static final String VIOLATION_MESSAGE = "%s should not be invoked within a loop. Detected presence in %s loop"; - private final Set violations; - private final MethodCallExpressionVertex targetVertex; - - GgdLoopDetectionVisitor(MethodCallExpressionVertex targetVertex) { - violations = new HashSet<>(); - this.targetVertex = targetVertex; - } - - Set getViolations() { - return new HashSet<>(violations); - } - - @Override - void execAfterLoopVertexVisit(BaseSFVertex vertex, SymbolProvider symbols) { - // TODO: source vertex is incorrect - violations.add(new Violation.PathBasedRuleViolation(getViolationMessage(vertex), vertex, targetVertex)); - } - - private String getViolationMessage(BaseSFVertex vertex) { - return String.format(VIOLATION_MESSAGE, targetVertex.getFullMethodName(), vertex.getLabel()); - } -} diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/LoopDetectionVisitor.java b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/LoopDetectionVisitor.java index d5290fca2..bb087a19d 100644 --- a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/LoopDetectionVisitor.java +++ b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/LoopDetectionVisitor.java @@ -4,6 +4,9 @@ import com.salesforce.graph.vertex.*; import com.salesforce.graph.visitor.DefaultNoOpPathVertexVisitor; +/** + * Visitor that gets notified when a loop vertex is invoked in the path. + */ abstract class LoopDetectionVisitor extends DefaultNoOpPathVertexVisitor { abstract void execAfterLoopVertexVisit(BaseSFVertex vertex, SymbolProvider symbols); diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupViolationInfo.java b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupViolationInfo.java new file mode 100644 index 000000000..51780a9ff --- /dev/null +++ b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupViolationInfo.java @@ -0,0 +1,86 @@ +package com.salesforce.rules.getglobaldescribe; + +import com.salesforce.config.UserFacingMessages; +import com.salesforce.graph.vertex.MethodCallExpressionVertex; +import com.salesforce.graph.vertex.SFVertex; +import com.salesforce.rules.RuleThrowable; +import com.salesforce.rules.Violation; +import org.apache.commons.lang3.tuple.Pair; + +import java.util.HashSet; +import java.util.Set; + +public class MassSchemaLookupViolationInfo implements RuleThrowable { + /** + * Type enum to indicate the category of GetGlobalDescribe performance degrade. + */ + enum Type{ + INVOKED_IN_A_LOOP(UserFacingMessages.GetGlobalDescribeTemplates.INVOKED_IN_A_LOOP_TEXT, UserFacingMessages.GetGlobalDescribeTemplates.INVOKED_IN_A_LOOP_ADDITIONAL_INFO), + INVOKED_MULTIPLE_TIMES(UserFacingMessages.GetGlobalDescribeTemplates.INVOKED_MULTIPLE_TIMES_TEXT, UserFacingMessages.GetGlobalDescribeTemplates.INVOKED_MULTIPLE_TIMES_ADDITIONAL_INFO); + + private String text; + private String additionalInfoTemplate; + + Type(String text, String additionalInfoTemplate) { + this.text = text; + this.additionalInfoTemplate = additionalInfoTemplate; + } + + public String getText() { + return text; + } + + public String getAdditionalInfoTemplate() { + return additionalInfoTemplate; + } + } + + /** + * Schema.getGlobalDescribe() operation + * TODO: Is this always only a MethodCallExpression? + **/ + private final MethodCallExpressionVertex sinkVertex; + + /** origin of the path leading to the GGD operation **/ + private SFVertex sourceVertex; + + /* Combination of type of issue and the vertex where this occurs */ + private final Set> occurrences; + + public MassSchemaLookupViolationInfo(SFVertex sourceVertex, MethodCallExpressionVertex sinkVertex) { + this.sourceVertex = sourceVertex; + this.sinkVertex = sinkVertex; + this.occurrences = new HashSet<>(); + } + + public void addOccurrence(Type type, SFVertex vertex) { + occurrences.add(Pair.of(type, vertex)); + } + + +// @Override + public Violation convert() { + return new Violation.PathBasedRuleViolation(this.getMessage(), sourceVertex, sinkVertex); + } + + private String getMessage() { + final String message = "Expensive operation %s invoked multiple times [%s]"; + return String.format(message, + sinkVertex.getFullMethodName(), + getOccurrences()); + } + + private String getOccurrences() { + // TODO: get an approved format + + final StringBuilder occurrencesBuilder = new StringBuilder(); + for (Pair occurrence : occurrences) { + occurrencesBuilder.append(occurrence.getLeft() + ": " + occurrence.getRight() + ","); + } + if (occurrencesBuilder.length() > 0) { + return occurrencesBuilder.substring(0, occurrencesBuilder.length() - 1); + } + return ""; + } + +} diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MultipleMassSchemaLookupVisitor.java b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MultipleMassSchemaLookupVisitor.java new file mode 100644 index 000000000..57d3eaa1f --- /dev/null +++ b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MultipleMassSchemaLookupVisitor.java @@ -0,0 +1,30 @@ +package com.salesforce.rules.getglobaldescribe; + +import com.salesforce.graph.symbols.SymbolProvider; +import com.salesforce.graph.vertex.BaseSFVertex; +import com.salesforce.graph.vertex.MethodCallExpressionVertex; +import com.salesforce.graph.vertex.SFVertex; + +import java.util.HashSet; +import java.util.Set; + +/** + * Visitor detects when more than one invocation of Schema.getGlobalDescribe() is made in a path. + */ +class MultipleMassSchemaLookupVisitor extends LoopDetectionVisitor { + private final MassSchemaLookupViolationInfo violationInfo; + + MultipleMassSchemaLookupVisitor(SFVertex sourceVertex, MethodCallExpressionVertex sinkVertex) { + this.violationInfo = new MassSchemaLookupViolationInfo(sourceVertex, sinkVertex); + } + + @Override + void execAfterLoopVertexVisit(BaseSFVertex vertex, SymbolProvider symbols) { + violationInfo.addOccurrence(MassSchemaLookupViolationInfo.Type.INVOKED_IN_A_LOOP, vertex); + } + + MassSchemaLookupViolationInfo getViolationInfo() { + return violationInfo; + } + +} diff --git a/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupTest.java b/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupTest.java new file mode 100644 index 000000000..e77fb6e4f --- /dev/null +++ b/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupTest.java @@ -0,0 +1,188 @@ +package com.salesforce.rules.getglobaldescribe; + +import com.salesforce.rules.AvoidMultipleMassSchemaLookup; +import com.salesforce.testutils.BasePathBasedRuleTest; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +//TODO: break down test, add updated assertions +public class AvoidMultipleMassSchemaLookupTest extends BasePathBasedRuleTest { + + private static final AvoidMultipleMassSchemaLookup RULE = AvoidMultipleMassSchemaLookup.getInstance(); + + @CsvSource({ + "ForEach, for (String s : myList)", + "For, for (Integer i; i < s.size; s++)", + "While, while(true)" + }) + @ParameterizedTest(name = "{displayName}: {0}") + public void testSimpleGgdInLoop(String testName, String loopStructure) { + // spotless:off + String sourceCode = + "public class MyClass {\n" + + " void foo() {\n" + + " List myList = new String[] {'Account','Contact'};\n" + + " " + loopStructure + " {\n" + + " Schema.getGlobalDescribe();\n" + + " }\n" + + " }\n" + + "}\n"; + // spotless:on + + assertNoViolation(RULE, sourceCode); + } + + @CsvSource({ + "ForEach, for (String s : myList)", + "For, for (Integer i; i < s.size; s++)", + "While, while(true)" + }) + @ParameterizedTest(name = "{displayName}: {0}") + public void testGgdInLoopInMethodCall(String testName, String loopStructure) { + // spotless:off + String sourceCode = + "public class MyClass {\n" + + " void foo() {\n" + + " List myList = new String[] {'Account','Contact'};\n" + + " " + loopStructure + " {\n" + + " getMoreInfo();\n" + + " }\n" + + " }\n" + + " void getMoreInfo() {\n" + + " Schema.getGlobalDescribe();\n" + + " }\n" + + "}\n"; + // spotless:on + + assertNoViolation(RULE, sourceCode); + } + + @Test + public void testLoopWithinLoop() { + // spotless:off + String sourceCode = + "public class MyClass {\n" + + " void foo(String[] objectNames) {\n" + + " for (Integer i = 0; i < objectNames.size; i++) {\n" + + " for (Integer j = 0; j < objectNames.size; j++) {\n" + + " Schema.getGlobalDescribe();\n" + + " }\n" + + " }\n" + + "}\n" + + "}\n"; + // spotless:on + + assertNoViolation(RULE, sourceCode); + } + + @Test + public void testMultipleInvocations() { + String sourceCode = + "public class MyClass {\n" + + " void foo() {\n" + + " Schema.getGlobalDescribe();\n" + + " Schema.getGlobalDescribe();\n" + + " }\n" + + "}\n"; + + assertNoViolation(RULE, sourceCode); + } + + @Test + public void testForEachLoopInClassInstance() { + String sourceCode[] = { + "public class MyClass {\n" + + " void foo() {\n" + + " Another[] anotherList = new Another[] {new Another()};\n" + + " for (Another an : anotherList) {\n" + + " an.exec();\n" + + " }\n" + + " }\n" + + "}\n", + "public class Another {\n" + + "void exec() {\n" + + " Schema.getGlobalDescribe();\n" + + "}\n" + + "}\n" + }; + + assertNoViolation(RULE, sourceCode); + + } + + @Test + public void testForLoopInClassInstance() { + String sourceCode[] = { + "public class MyClass {\n" + + " void foo() {\n" + + " Another[] anotherList = new Another[] {new Another()};\n" + + " for (Integer i = 0 ; i < anotherList.size; i++) {\n" + + " anotherList[i].exec();\n" + + " }\n" + + " }\n" + + "}\n", + "public class Another {\n" + + "void exec() {\n" + + " Schema.getGlobalDescribe();\n" + + "}\n" + + "}\n" + }; + + assertNoViolation(RULE, sourceCode); + + } + + @Test + public void testForLoopConditionalOnClassInstance() { + String sourceCode[] = { + "public class MyClass {\n" + + " void foo() {\n" + + " Another[] anotherList = new Another[] {new Another(false), new Another(false)};\n" + + " for (Another an : anotherList) {\n" + + " an.exec();\n" + + " }\n" + + " }\n" + + "}\n", + "public class Another {\n" + + "boolean shouldCheck;\n" + + "Another(boolean b) {\n" + + " shouldCheck = b;\n" + + "}\n" + + "void exec() {\n" + + " if (shouldCheck) {\n" + + " Schema.getGlobalDescribe();\n" + + " }\n" + + "}\n" + + "}\n" + }; + + assertNoViolation(RULE, sourceCode); + } + + @Test // TODO: Check if this is a false positive. Static block should get invoked only once + public void testLoopFromStaticBlock() { + String[] sourceCode = + { + "public class MyClass {\n" + + " void foo(String[] objectNames) {\n" + + " for (Integer i = 0; i < objectNames.size; i++) {\n" + + " AnotherClass.doNothing();\n" + + " }\n" + + " }\n" + + "}\n", + "public class AnotherClass {\n" + + " static {\n" + + " Schema.getGlobalDescribe();\n" + + " }\n" + + " static void doNothing() {\n" + + " // do nothing \n" + + " }\n" + + "}\n" + }; + + assertNoViolation(RULE, sourceCode); + } + + +} diff --git a/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/GetGlobalDescribeViolationRuleTest.java b/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/GetGlobalDescribeViolationRuleTest.java deleted file mode 100644 index 5a4cecb16..000000000 --- a/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/GetGlobalDescribeViolationRuleTest.java +++ /dev/null @@ -1,60 +0,0 @@ -package com.salesforce.rules.getglobaldescribe; - -import com.salesforce.rules.GetGlobalDescribeViolationRule; -import com.salesforce.testutils.BasePathBasedRuleTest; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.CsvSource; - -//TODO: break down test, add updated assertions -public class GetGlobalDescribeViolationRuleTest extends BasePathBasedRuleTest { - - private static final GetGlobalDescribeViolationRule RULE = GetGlobalDescribeViolationRule.getInstance(); - - @CsvSource({ - "ForEach, for (String s : myList)", - "For, for (Integer i; i < s.size; s++)", - "While, while(true)" - }) - @ParameterizedTest(name = "{displayName}: {0}") - public void testSimpleGgdInLoop(String testName, String loopStructure) { - // spotless:off - String sourceCode = - "public class MyClass {\n" + - " void foo() {\n" + - " List myList = new String[] {'Account','Contact'};\n" + - " " + loopStructure + " {\n" + - " Schema.getGlobalDescribe();\n" + - " }\n" + - " }\n" + - "}\n"; - // spotless:on - - assertNoViolation(RULE, sourceCode); - } - - @CsvSource({ - "ForEach, for (String s : myList)", - "For, for (Integer i; i < s.size; s++)", - "While, while(true)" - }) - @ParameterizedTest(name = "{displayName}: {0}") - public void testGgdInLoopInMethodCall(String testName, String loopStructure) { - // spotless:off - String sourceCode = - "public class MyClass {\n" + - " void foo() {\n" + - " List myList = new String[] {'Account','Contact'};\n" + - " " + loopStructure + " {\n" + - " getMoreInfo();\n" + - " }\n" + - " }\n" + - " void getMoreInfo() {\n" + - " Schema.getGlobalDescribe();\n" + - " }\n" + - "}\n"; - // spotless:on - - assertNoViolation(RULE, sourceCode); - } - -} From 33d27c7806b01346da3276a0c5a2911a1f4ee5b9 Mon Sep 17 00:00:00 2001 From: Roopa Mohan Date: Thu, 20 Apr 2023 16:36:09 -0700 Subject: [PATCH 03/10] Fixing asserts on tests, adding more rule scenarios --- .../salesforce/config/UserFacingMessages.java | 11 +- .../visitor/DefaultNoOpPathVertexVisitor.java | 16 +- .../rules/AvoidMultipleMassSchemaLookup.java | 18 +- .../salesforce/rules/PathBasedRuleRunner.java | 12 +- .../fls/apex/operations/FlsViolationInfo.java | 15 +- .../AvoidMultipleMassSchemaLookupHandler.java | 36 ++- .../LoopDetectionVisitor.java | 4 +- .../MassSchemaLookupInfo.java | 99 ++++++ .../MassSchemaLookupInfoUtil.java | 51 +++ .../MassSchemaLookupViolationInfo.java | 86 ----- .../MultipleMassSchemaLookupVisitor.java | 55 +++- .../getglobaldescribe/RuleConstants.java | 41 +++ .../AvoidMultipleMassSchemaLookupTest.java | 305 ++++++++++++------ ...BaseAvoidMultipleMassSchemaLookupTest.java | 19 ++ .../testutils/ViolationWrapper.java | 45 +++ 15 files changed, 558 insertions(+), 255 deletions(-) create mode 100644 sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupInfo.java create mode 100644 sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupInfoUtil.java delete mode 100644 sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupViolationInfo.java create mode 100644 sfge/src/main/java/com/salesforce/rules/getglobaldescribe/RuleConstants.java create mode 100644 sfge/src/test/java/com/salesforce/rules/getglobaldescribe/BaseAvoidMultipleMassSchemaLookupTest.java diff --git a/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java b/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java index c2a01ded1..70afae8e4 100644 --- a/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java +++ b/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java @@ -77,11 +77,10 @@ public static final class CompilationErrors { public static final String EXCEPTION_FORMAT_TEMPLATE = "%s, Caused by:\n%s"; } - public static final class GetGlobalDescribeTemplates { - public static final String INVOKED_IN_A_LOOP_TEXT = "invoked in a loop"; - public static final String INVOKED_IN_A_LOOP_ADDITIONAL_INFO = ". Loop started at %s:%d."; // file name, line number - public static final String INVOKED_MULTIPLE_TIMES_TEXT = "invoked multiple times"; - public static final String INVOKED_MULTIPLE_TIMES_ADDITIONAL_INFO = ". Previous invocation occurred at %s:%d."; // file name, line number - public static final String MESSAGE_TEMPLATE = "Detected %s %s. %s"; // method name, type-specific-text, type-specific-additional-info + public static final class AvoidExcessiveSchemaLookupsTemplates { + public static final String MESSAGE_TEMPLATE = + "Avoid excessive Schema lookups in a single path. Detected %s %s at %s:%d."; + public static final String OCCURRENCE_LOOP_TEMPLATE = "inside a %s"; + public static final String OCCURRENCE_MULTIPLE_TEMPLATE = "preceded by a call to %s"; } } diff --git a/sfge/src/main/java/com/salesforce/graph/visitor/DefaultNoOpPathVertexVisitor.java b/sfge/src/main/java/com/salesforce/graph/visitor/DefaultNoOpPathVertexVisitor.java index 743f9bf3c..1fcf25aa2 100644 --- a/sfge/src/main/java/com/salesforce/graph/visitor/DefaultNoOpPathVertexVisitor.java +++ b/sfge/src/main/java/com/salesforce/graph/visitor/DefaultNoOpPathVertexVisitor.java @@ -245,9 +245,7 @@ public boolean visit(VariableExpressionVertex.Single vertex, SymbolProvider symb public void afterVisit(BaseSFVertex vertex, SymbolProvider symbols) {} @Override - public void afterVisit(DoLoopStatementVertex vertex, SymbolProvider symbols) { - - } + public void afterVisit(DoLoopStatementVertex vertex, SymbolProvider symbols) {} @Override public void afterVisit(DmlDeleteStatementVertex vertex, SymbolProvider symbols) {} @@ -268,14 +266,10 @@ public void afterVisit(DmlUpdateStatementVertex vertex, SymbolProvider symbols) public void afterVisit(DmlUpsertStatementVertex vertex, SymbolProvider symbols) {} @Override - public void afterVisit(ForEachStatementVertex vertex, SymbolProvider symbols) { - - } + public void afterVisit(ForEachStatementVertex vertex, SymbolProvider symbols) {} @Override - public void afterVisit(ForLoopStatementVertex vertex, SymbolProvider symbols) { - - } + public void afterVisit(ForLoopStatementVertex vertex, SymbolProvider symbols) {} @Override public void afterVisit(FieldDeclarationVertex vertex, SymbolProvider symbols) {} @@ -299,7 +293,5 @@ public void afterVisit(StandardConditionVertex.Positive vertex, SymbolProvider s public void afterVisit(ThrowStatementVertex vertex, SymbolProvider symbols) {} @Override - public void afterVisit(WhileLoopStatementVertex vertex, SymbolProvider symbols) { - - } + public void afterVisit(WhileLoopStatementVertex vertex, SymbolProvider symbols) {} } diff --git a/sfge/src/main/java/com/salesforce/rules/AvoidMultipleMassSchemaLookup.java b/sfge/src/main/java/com/salesforce/rules/AvoidMultipleMassSchemaLookup.java index a3a05c787..d2aaf50d7 100644 --- a/sfge/src/main/java/com/salesforce/rules/AvoidMultipleMassSchemaLookup.java +++ b/sfge/src/main/java/com/salesforce/rules/AvoidMultipleMassSchemaLookup.java @@ -3,17 +3,12 @@ import com.salesforce.config.UserFacingMessages; import com.salesforce.graph.ApexPath; import com.salesforce.graph.vertex.BaseSFVertex; -import com.salesforce.rules.getglobaldescribe.MassSchemaLookupViolationInfo; import com.salesforce.rules.getglobaldescribe.AvoidMultipleMassSchemaLookupHandler; -import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; - import java.util.ArrayList; import java.util.List; -import java.util.Set; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; -/** - * Rule to detect possible performance degradations while invoking Schema.getGlobalDescribe(). - */ +/** Rule to detect possible performance degradations while invoking Schema.getGlobalDescribe(). */ public class AvoidMultipleMassSchemaLookup extends AbstractPathTraversalRule { private final AvoidMultipleMassSchemaLookupHandler ruleHandler; @@ -29,10 +24,10 @@ public boolean test(BaseSFVertex vertex) { @Override protected List _run(GraphTraversalSource g, ApexPath path, BaseSFVertex vertex) { - MassSchemaLookupViolationInfo massSchemaLookupViolationInfos = ruleHandler.detectViolations(g, path, vertex); List violations = new ArrayList<>(); - // TODO: do further processing here - violations.add(massSchemaLookupViolationInfos); + + violations.addAll(ruleHandler.detectViolations(g, path, vertex)); + return violations; } @@ -62,6 +57,7 @@ public static AvoidMultipleMassSchemaLookup getInstance() { private static final class LazyHolder { // Postpone initialization until first use - private static final AvoidMultipleMassSchemaLookup INSTANCE = new AvoidMultipleMassSchemaLookup(); + private static final AvoidMultipleMassSchemaLookup INSTANCE = + new AvoidMultipleMassSchemaLookup(); } } diff --git a/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java b/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java index a85009c9a..d3c408868 100644 --- a/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java +++ b/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java @@ -13,7 +13,7 @@ import com.salesforce.graph.vertex.SFVertex; import com.salesforce.rules.fls.apex.operations.FlsViolationInfo; import com.salesforce.rules.fls.apex.operations.FlsViolationMessageUtil; -import com.salesforce.rules.getglobaldescribe.MassSchemaLookupViolationInfo; +import com.salesforce.rules.getglobaldescribe.MassSchemaLookupInfo; import com.salesforce.rules.ops.ProgressListener; import com.salesforce.rules.ops.ProgressListenerProvider; import java.util.*; @@ -121,7 +121,7 @@ private void executeRulesOnPaths(List paths) { // time, and // require post-processing. final HashSet incompleteThrowables = new HashSet<>(); - final HashSet tempDeleteMe = new HashSet<>(); + final HashSet tempDeleteMe = new HashSet<>(); // For each path... for (ApexPath path : paths) { // If the path's metadata is present... @@ -145,8 +145,8 @@ private void executeRulesOnPaths(List paths) { // objects. if (ruleThrowable instanceof FlsViolationInfo) { incompleteThrowables.add((FlsViolationInfo) ruleThrowable); - } else if (ruleThrowable instanceof MassSchemaLookupViolationInfo) { - tempDeleteMe.add((MassSchemaLookupViolationInfo) ruleThrowable); + } else if (ruleThrowable instanceof MassSchemaLookupInfo) { + tempDeleteMe.add((MassSchemaLookupInfo) ruleThrowable); } else if (ruleThrowable instanceof Violation) { // If the violation is done, it can just go directly into the results // set. @@ -172,10 +172,10 @@ private void executeRulesOnPaths(List paths) { } } - private void convertGgdToViolations(HashSet ggdViolationInfos) { + private void convertGgdToViolations(HashSet ggdViolationInfos) { // TODO: consolidate by sink/source - for (MassSchemaLookupViolationInfo ggdViolationInfo: ggdViolationInfos) { + for (MassSchemaLookupInfo ggdViolationInfo : ggdViolationInfos) { violations.add(ggdViolationInfo.convert()); } } diff --git a/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationInfo.java b/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationInfo.java index c58e319dc..9174ff309 100644 --- a/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationInfo.java +++ b/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationInfo.java @@ -4,12 +4,10 @@ import com.salesforce.collections.CollectionUtil; import com.salesforce.config.UserFacingMessages; import com.salesforce.exception.ProgrammingException; -import com.salesforce.exception.UnexpectedException; import com.salesforce.graph.vertex.SFVertex; import com.salesforce.rules.AbstractRule; import com.salesforce.rules.RuleThrowable; import com.salesforce.rules.Violation; - import java.util.Optional; import java.util.TreeSet; @@ -172,22 +170,19 @@ public FlsViolationInfo deepClone() { return new FlsViolationInfo(this); } -// @Override + // @Override public Violation convert() { - final String violationMessage = - FlsViolationMessageUtil.constructMessage(this); + final String violationMessage = FlsViolationMessageUtil.constructMessage(this); final Optional sourceVertex = this.getSourceVertex(); final Optional sinkVertex = this.getSinkVertex(); if (sinkVertex.isPresent()) { final Violation.RuleViolation ruleViolation = - new Violation.PathBasedRuleViolation( - violationMessage, sourceVertex.get(), sinkVertex.get()); + new Violation.PathBasedRuleViolation( + violationMessage, sourceVertex.get(), sinkVertex.get()); ruleViolation.setPropertiesFromRule(rule); return ruleViolation; } else { - throw new ProgrammingException( - "Sink vertex not set in flsViolationInfo: " - + this); + throw new ProgrammingException("Sink vertex not set in flsViolationInfo: " + this); } } } diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupHandler.java b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupHandler.java index 05a025d85..2df086099 100644 --- a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupHandler.java +++ b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupHandler.java @@ -8,41 +8,42 @@ import com.salesforce.graph.vertex.SFVertex; import com.salesforce.graph.visitor.ApexPathWalker; import com.salesforce.rules.AvoidMultipleMassSchemaLookup; +import java.util.Set; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; -/** - * Executes internals of {@link AvoidMultipleMassSchemaLookup} - */ +/** Executes internals of {@link AvoidMultipleMassSchemaLookup} */ public class AvoidMultipleMassSchemaLookupHandler { - private static final String METHOD_SCHEMA_GET_GLOBAL_DESCRIBE = "Schema.getGlobalDescribe"; - /** * @param vertex to consider for analysis - * @return true if the vertex parameter requires to be treated as a target vertex for {@link AvoidMultipleMassSchemaLookup}. + * @return true if the vertex parameter requires to be treated as a target vertex for {@link + * AvoidMultipleMassSchemaLookup}. */ public boolean test(BaseSFVertex vertex) { - return vertex instanceof MethodCallExpressionVertex && isGetGlobalDescribeMethod((MethodCallExpressionVertex) vertex); + return vertex instanceof MethodCallExpressionVertex + && RuleConstants.isSchemaExpensiveMethod((MethodCallExpressionVertex) vertex); } - public MassSchemaLookupViolationInfo detectViolations(GraphTraversalSource g, ApexPath path, BaseSFVertex vertex) { + public Set detectViolations( + GraphTraversalSource g, ApexPath path, BaseSFVertex vertex) { if (!(vertex instanceof MethodCallExpressionVertex)) { - throw new ProgrammingException("GetGlobalDescribeViolationRule unexpected invoked on an instance that's not MethodCallExpressionVertex. vertex=" + vertex); + throw new ProgrammingException( + "GetGlobalDescribeViolationRule unexpected invoked on an instance that's not MethodCallExpressionVertex. vertex=" + + vertex); } final SFVertex sourceVertex = path.getMethodVertex().orElse(null); - final MultipleMassSchemaLookupVisitor ruleVisitor = new MultipleMassSchemaLookupVisitor(sourceVertex, (MethodCallExpressionVertex) vertex); - // TODO: I'm expecting to add other visitors depending on the other factors we will analyze to decide if a GGD call is not performant. + final MultipleMassSchemaLookupVisitor ruleVisitor = + new MultipleMassSchemaLookupVisitor( + sourceVertex, (MethodCallExpressionVertex) vertex); + // TODO: I'm expecting to add other visitors depending on the other factors we will analyze + // to decide if a GGD call is not performant. DefaultSymbolProviderVertexVisitor symbols = new DefaultSymbolProviderVertexVisitor(g); ApexPathWalker.walkPath(g, path, ruleVisitor, symbols); // Once it finishes walking, collect any violations thrown - return ruleVisitor.getViolationInfo(); - } - - private static boolean isGetGlobalDescribeMethod(MethodCallExpressionVertex vertex) { - return METHOD_SCHEMA_GET_GLOBAL_DESCRIBE.equalsIgnoreCase(vertex.getFullMethodName()); + return ruleVisitor.getViolation(); } public static AvoidMultipleMassSchemaLookupHandler getInstance() { @@ -51,6 +52,7 @@ public static AvoidMultipleMassSchemaLookupHandler getInstance() { private static final class LazyHolder { // Postpone initialization until first use - private static final AvoidMultipleMassSchemaLookupHandler INSTANCE = new AvoidMultipleMassSchemaLookupHandler(); + private static final AvoidMultipleMassSchemaLookupHandler INSTANCE = + new AvoidMultipleMassSchemaLookupHandler(); } } diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/LoopDetectionVisitor.java b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/LoopDetectionVisitor.java index bb087a19d..e078b0a18 100644 --- a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/LoopDetectionVisitor.java +++ b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/LoopDetectionVisitor.java @@ -4,9 +4,7 @@ import com.salesforce.graph.vertex.*; import com.salesforce.graph.visitor.DefaultNoOpPathVertexVisitor; -/** - * Visitor that gets notified when a loop vertex is invoked in the path. - */ +/** Visitor that gets notified when a loop vertex is invoked in the path. */ abstract class LoopDetectionVisitor extends DefaultNoOpPathVertexVisitor { abstract void execAfterLoopVertexVisit(BaseSFVertex vertex, SymbolProvider symbols); diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupInfo.java b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupInfo.java new file mode 100644 index 000000000..cb0a406a1 --- /dev/null +++ b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupInfo.java @@ -0,0 +1,99 @@ +package com.salesforce.rules.getglobaldescribe; + +import com.salesforce.exception.ProgrammingException; +import com.salesforce.graph.vertex.MethodCallExpressionVertex; +import com.salesforce.graph.vertex.SFVertex; +import com.salesforce.rules.RuleThrowable; +import com.salesforce.rules.Violation; + +public class MassSchemaLookupInfo implements RuleThrowable { + + /** + * vertex where the expensive Schema lookup happens TODO: Is this always only a + * MethodCallExpression? + */ + private final MethodCallExpressionVertex sinkVertex; + + /** origin of the path leading to the GGD operation * */ + private final SFVertex sourceVertex; + + /** Type of repetition that happens between source and sink * */ + private final RuleConstants.RepetitionType repetitionType; + + /** Vertex where the repetition occurs */ + private final SFVertex repetitionVertex; + + public MassSchemaLookupInfo( + SFVertex sourceVertex, + MethodCallExpressionVertex sinkVertex, + RuleConstants.RepetitionType repetitionType, + SFVertex repetitionVertex) { + validateInput(repetitionType, repetitionVertex); + this.sourceVertex = sourceVertex; + this.sinkVertex = sinkVertex; + this.repetitionType = repetitionType; + this.repetitionVertex = repetitionVertex; + } + + private void validateInput( + RuleConstants.RepetitionType repetitionType, SFVertex repetitionVertex) { + if (repetitionType == null) { + throw new ProgrammingException("repetitionType cannot be null."); + } + + if (repetitionVertex == null) { + throw new ProgrammingException("repetitionVertex cannot be null."); + } + + if (RuleConstants.RepetitionType.MULTIPLE.equals(repetitionType)) { + if (!(repetitionVertex instanceof MethodCallExpressionVertex)) { + throw new ProgrammingException( + "Repetition of type MULTIPLE can only happen on MethodCallExpressions. repetitionVertex=" + + repetitionVertex); + } + } + } + + // @Override + public Violation convert() { + return new Violation.PathBasedRuleViolation( + MassSchemaLookupInfoUtil.getMessage(this), sourceVertex, sinkVertex); + } + + public MethodCallExpressionVertex getSinkVertex() { + return sinkVertex; + } + + public SFVertex getSourceVertex() { + return sourceVertex; + } + + public RuleConstants.RepetitionType getRepetitionType() { + return repetitionType; + } + + public SFVertex getRepetitionVertex() { + return repetitionVertex; + } + + // private String getMessage() { + // return + // String.format(UserFacingMessages.AvoidExcessiveSchemaLookupsTemplates.MESSAGE_TEMPLATE, + // sinkVertex.getFullMethodName(), + // getOccurrenceMessage(), + // repetitionVertex.getDefiningType(), + // repetitionVertex.getBeginLine()); + // } + // + // private String getOccurrenceMessage() { + // if (RuleConstants.RepetitionType.MULTIPLE.equals(repetitionType)) { + // // Use method name on template message + // return repetitionType.getMessage(((MethodCallExpressionVertex) + // repetitionVertex).getFullMethodName()); + // } else { + // // Use Loop type on template message + // return repetitionType.getMessage(repetitionVertex.getDefiningType()); + // } + // } + +} diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupInfoUtil.java b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupInfoUtil.java new file mode 100644 index 000000000..180929002 --- /dev/null +++ b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupInfoUtil.java @@ -0,0 +1,51 @@ +package com.salesforce.rules.getglobaldescribe; + +import com.salesforce.config.UserFacingMessages; +import com.salesforce.graph.vertex.MethodCallExpressionVertex; +import com.salesforce.graph.vertex.SFVertex; + +/** Utility to help with violation message creation on AvoidMassSchemaLookupRule */ +public final class MassSchemaLookupInfoUtil { + private MassSchemaLookupInfoUtil() {} + + public static String getMessage(MassSchemaLookupInfo info) { + return getMessage( + info.getSinkVertex().getFullMethodName(), + info.getRepetitionType(), + getOccurrenceInfoValue(info.getRepetitionType(), info.getRepetitionVertex()), + info.getRepetitionVertex().getDefiningType(), + info.getRepetitionVertex().getBeginLine()); + } + + public static String getMessage( + String sinkMethodName, + RuleConstants.RepetitionType repetitionType, + String occurrenceInfoValue, + String occurrenceClassName, + int occurrenceLine) { + final String occurrenceMessage = getOccurrenceMessage(repetitionType, occurrenceInfoValue); + + return String.format( + UserFacingMessages.AvoidExcessiveSchemaLookupsTemplates.MESSAGE_TEMPLATE, + sinkMethodName, + occurrenceMessage, + occurrenceClassName, + occurrenceLine); + } + + private static String getOccurrenceInfoValue( + RuleConstants.RepetitionType repetitionType, SFVertex repetitionVertex) { + if (RuleConstants.RepetitionType.MULTIPLE.equals(repetitionType)) { + // Use method name on template message + return ((MethodCallExpressionVertex) repetitionVertex).getFullMethodName(); + } else { + // Use Loop type on template message + return repetitionVertex.getLabel(); + } + } + + private static String getOccurrenceMessage( + RuleConstants.RepetitionType repetitionType, String value) { + return repetitionType.getMessage(value); + } +} diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupViolationInfo.java b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupViolationInfo.java deleted file mode 100644 index 51780a9ff..000000000 --- a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupViolationInfo.java +++ /dev/null @@ -1,86 +0,0 @@ -package com.salesforce.rules.getglobaldescribe; - -import com.salesforce.config.UserFacingMessages; -import com.salesforce.graph.vertex.MethodCallExpressionVertex; -import com.salesforce.graph.vertex.SFVertex; -import com.salesforce.rules.RuleThrowable; -import com.salesforce.rules.Violation; -import org.apache.commons.lang3.tuple.Pair; - -import java.util.HashSet; -import java.util.Set; - -public class MassSchemaLookupViolationInfo implements RuleThrowable { - /** - * Type enum to indicate the category of GetGlobalDescribe performance degrade. - */ - enum Type{ - INVOKED_IN_A_LOOP(UserFacingMessages.GetGlobalDescribeTemplates.INVOKED_IN_A_LOOP_TEXT, UserFacingMessages.GetGlobalDescribeTemplates.INVOKED_IN_A_LOOP_ADDITIONAL_INFO), - INVOKED_MULTIPLE_TIMES(UserFacingMessages.GetGlobalDescribeTemplates.INVOKED_MULTIPLE_TIMES_TEXT, UserFacingMessages.GetGlobalDescribeTemplates.INVOKED_MULTIPLE_TIMES_ADDITIONAL_INFO); - - private String text; - private String additionalInfoTemplate; - - Type(String text, String additionalInfoTemplate) { - this.text = text; - this.additionalInfoTemplate = additionalInfoTemplate; - } - - public String getText() { - return text; - } - - public String getAdditionalInfoTemplate() { - return additionalInfoTemplate; - } - } - - /** - * Schema.getGlobalDescribe() operation - * TODO: Is this always only a MethodCallExpression? - **/ - private final MethodCallExpressionVertex sinkVertex; - - /** origin of the path leading to the GGD operation **/ - private SFVertex sourceVertex; - - /* Combination of type of issue and the vertex where this occurs */ - private final Set> occurrences; - - public MassSchemaLookupViolationInfo(SFVertex sourceVertex, MethodCallExpressionVertex sinkVertex) { - this.sourceVertex = sourceVertex; - this.sinkVertex = sinkVertex; - this.occurrences = new HashSet<>(); - } - - public void addOccurrence(Type type, SFVertex vertex) { - occurrences.add(Pair.of(type, vertex)); - } - - -// @Override - public Violation convert() { - return new Violation.PathBasedRuleViolation(this.getMessage(), sourceVertex, sinkVertex); - } - - private String getMessage() { - final String message = "Expensive operation %s invoked multiple times [%s]"; - return String.format(message, - sinkVertex.getFullMethodName(), - getOccurrences()); - } - - private String getOccurrences() { - // TODO: get an approved format - - final StringBuilder occurrencesBuilder = new StringBuilder(); - for (Pair occurrence : occurrences) { - occurrencesBuilder.append(occurrence.getLeft() + ": " + occurrence.getRight() + ","); - } - if (occurrencesBuilder.length() > 0) { - return occurrencesBuilder.substring(0, occurrencesBuilder.length() - 1); - } - return ""; - } - -} diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MultipleMassSchemaLookupVisitor.java b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MultipleMassSchemaLookupVisitor.java index 57d3eaa1f..22559c0ea 100644 --- a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MultipleMassSchemaLookupVisitor.java +++ b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MultipleMassSchemaLookupVisitor.java @@ -4,7 +4,6 @@ import com.salesforce.graph.vertex.BaseSFVertex; import com.salesforce.graph.vertex.MethodCallExpressionVertex; import com.salesforce.graph.vertex.SFVertex; - import java.util.HashSet; import java.util.Set; @@ -12,19 +11,63 @@ * Visitor detects when more than one invocation of Schema.getGlobalDescribe() is made in a path. */ class MultipleMassSchemaLookupVisitor extends LoopDetectionVisitor { - private final MassSchemaLookupViolationInfo violationInfo; + /** Represents the path entry point that this visitor is walking */ + private final SFVertex sourceVertex; + + /** Represents the mass schema lookup vertex that we're looking earlier calls for. */ + private final MethodCallExpressionVertex sinkVertex; + + /** Collects violation information */ + private final HashSet violations; + + /** Indicates if the sink vertex has been visited or not. */ + private boolean isSinkVisited = false; MultipleMassSchemaLookupVisitor(SFVertex sourceVertex, MethodCallExpressionVertex sinkVertex) { - this.violationInfo = new MassSchemaLookupViolationInfo(sourceVertex, sinkVertex); + this.sourceVertex = sourceVertex; + this.sinkVertex = sinkVertex; + this.violations = new HashSet<>(); } @Override void execAfterLoopVertexVisit(BaseSFVertex vertex, SymbolProvider symbols) { - violationInfo.addOccurrence(MassSchemaLookupViolationInfo.Type.INVOKED_IN_A_LOOP, vertex); + if (shouldContinue()) { + violations.add( + new MassSchemaLookupInfo( + sourceVertex, sinkVertex, RuleConstants.RepetitionType.LOOP, vertex)); + } } - MassSchemaLookupViolationInfo getViolationInfo() { - return violationInfo; + @Override + public void afterVisit(MethodCallExpressionVertex vertex, SymbolProvider symbols) { + if (vertex.equals(sinkVertex)) { + // Mark sink as visited. From this point on, we don't need to + // look for anymore loops or additional calls. + // TODO: A more performant approach would be to stop walking path from this point + isSinkVisited = true; + } else if (RuleConstants.isSchemaExpensiveMethod(vertex) && shouldContinue()) { + violations.add( + new MassSchemaLookupInfo( + sourceVertex, + sinkVertex, + RuleConstants.RepetitionType.MULTIPLE, + vertex)); + } + } + + /** + * Decides whether the rule should continue collecting violations + * + * @return true if the rule visitor should continue. + */ + private boolean shouldContinue() { + return !isSinkVisited; } + /** + * @return Violations collected by the rule. + */ + Set getViolation() { + return violations; + } } diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/RuleConstants.java b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/RuleConstants.java new file mode 100644 index 000000000..6cd735eb6 --- /dev/null +++ b/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/RuleConstants.java @@ -0,0 +1,41 @@ +package com.salesforce.rules.getglobaldescribe; + +import com.google.common.collect.ImmutableSet; +import com.salesforce.config.UserFacingMessages; +import com.salesforce.graph.vertex.MethodCallExpressionVertex; + +public class RuleConstants { + + static final String METHOD_SCHEMA_GET_GLOBAL_DESCRIBE = "schema.getglobaldescribe"; + static final String METHOD_SCHEMA_DESCRIBE_SOBJECTS = "schema.describesobjects"; + + private static final ImmutableSet EXPENSIVE_METHODS = + ImmutableSet.of(METHOD_SCHEMA_GET_GLOBAL_DESCRIBE, METHOD_SCHEMA_DESCRIBE_SOBJECTS); + + /** + * @param vertex to check + * @return true if the method call is a known expensive schema call. + */ + static boolean isSchemaExpensiveMethod(MethodCallExpressionVertex vertex) { + final String fullMethodName = vertex.getFullMethodName(); + return EXPENSIVE_METHODS.contains(fullMethodName.toLowerCase()); + } + + /** Enum to indicate the type of repetition the method call was subjected. */ + public enum RepetitionType { + LOOP(UserFacingMessages.AvoidExcessiveSchemaLookupsTemplates.OCCURRENCE_LOOP_TEMPLATE), + MULTIPLE( + UserFacingMessages.AvoidExcessiveSchemaLookupsTemplates + .OCCURRENCE_MULTIPLE_TEMPLATE); + + String messageTemplate; + + RepetitionType(String messageTemplate) { + this.messageTemplate = messageTemplate; + } + + public String getMessage(String... params) { + return String.format(messageTemplate, params); + } + } +} diff --git a/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupTest.java b/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupTest.java index e77fb6e4f..acbe869bd 100644 --- a/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupTest.java +++ b/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupTest.java @@ -1,23 +1,25 @@ package com.salesforce.rules.getglobaldescribe; +import com.salesforce.apex.jorje.ASTConstants; import com.salesforce.rules.AvoidMultipleMassSchemaLookup; -import com.salesforce.testutils.BasePathBasedRuleTest; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; -//TODO: break down test, add updated assertions -public class AvoidMultipleMassSchemaLookupTest extends BasePathBasedRuleTest { +// TODO: break down test, add updated assertions +public class AvoidMultipleMassSchemaLookupTest extends BaseAvoidMultipleMassSchemaLookupTest { - private static final AvoidMultipleMassSchemaLookup RULE = AvoidMultipleMassSchemaLookup.getInstance(); + private static final AvoidMultipleMassSchemaLookup RULE = + AvoidMultipleMassSchemaLookup.getInstance(); @CsvSource({ - "ForEach, for (String s : myList)", - "For, for (Integer i; i < s.size; s++)", - "While, while(true)" + "ForEachStatement, for (String s : myList)", + "ForLoopStatement, for (Integer i; i < s.size; s++)", + "WhileLoopStatement, while(true)" }) @ParameterizedTest(name = "{displayName}: {0}") - public void testSimpleGgdInLoop(String testName, String loopStructure) { + public void testSimpleGgdInLoop(String loopAstLabel, String loopStructure) { // spotless:off String sourceCode = "public class MyClass {\n" + @@ -30,16 +32,25 @@ public void testSimpleGgdInLoop(String testName, String loopStructure) { "}\n"; // spotless:on - assertNoViolation(RULE, sourceCode); + assertViolations( + RULE, + sourceCode, + expect( + 5, + RuleConstants.METHOD_SCHEMA_GET_GLOBAL_DESCRIBE, + 4, + "MyClass", + RuleConstants.RepetitionType.LOOP, + loopAstLabel)); } @CsvSource({ - "ForEach, for (String s : myList)", - "For, for (Integer i; i < s.size; s++)", - "While, while(true)" + "ForEachStatement, for (String s : myList)", + "ForLoopStatement, for (Integer i; i < s.size; s++)", + "WhileLoopStatement, while(true)" }) @ParameterizedTest(name = "{displayName}: {0}") - public void testGgdInLoopInMethodCall(String testName, String loopStructure) { + public void testGgdInLoopInMethodCall(String loopAstLabel, String loopStructure) { // spotless:off String sourceCode = "public class MyClass {\n" + @@ -55,17 +66,32 @@ public void testGgdInLoopInMethodCall(String testName, String loopStructure) { "}\n"; // spotless:on - assertNoViolation(RULE, sourceCode); + assertViolations( + RULE, + sourceCode, + expect( + 9, + RuleConstants.METHOD_SCHEMA_GET_GLOBAL_DESCRIBE, + 4, + "MyClass", + RuleConstants.RepetitionType.LOOP, + loopAstLabel)); } - @Test - public void testLoopWithinLoop() { + @CsvSource({ + "ForEachStatement, for (String s1 : myList), for (String s2 : myList)", + "ForLoopStatement, for (Integer i; i < myList.size; s++), for (Integer j; j < myList.size; s++)", + "WhileLoopStatement, while(true), while(true)" + }) + @ParameterizedTest(name = "{displayName}: {0}") + public void testLoopWithinLoop( + String loopAstLabel, String loopStructure1, String loopStructure2) { // spotless:off String sourceCode = "public class MyClass {\n" + - " void foo(String[] objectNames) {\n" + - " for (Integer i = 0; i < objectNames.size; i++) {\n" + - " for (Integer j = 0; j < objectNames.size; j++) {\n" + + " void foo(String[] myList) {\n" + + " " + loopStructure1 + " {\n" + + " " + loopStructure2 + " {\n" + " Schema.getGlobalDescribe();\n" + " }\n" + " }\n" + @@ -73,116 +99,199 @@ public void testLoopWithinLoop() { "}\n"; // spotless:on - assertNoViolation(RULE, sourceCode); + assertViolations( + RULE, + sourceCode, + expect( + 5, + RuleConstants.METHOD_SCHEMA_GET_GLOBAL_DESCRIBE, + 3, + "MyClass", + RuleConstants.RepetitionType.LOOP, + loopAstLabel), + expect( + 5, + RuleConstants.METHOD_SCHEMA_GET_GLOBAL_DESCRIBE, + 4, + "MyClass", + RuleConstants.RepetitionType.LOOP, + loopAstLabel)); } - @Test - public void testMultipleInvocations() { + @CsvSource({ + "GGD followed by GGD, Schema.getGlobalDescribe, Schema.getGlobalDescribe", + "GGD followed by DSO, Schema.getGlobalDescribe, Schema.describeSObjects", + "DSO followed by DSO, Schema.describeSObjects, Schema.describeSObjects", + "DSO followed by GGD, Schema.describeSObjects, Schema.getGlobalDescribe" + }) + @ParameterizedTest(name = "{displayName}: {0}") + public void testMultipleInvocations(String testName, String firstCall, String secondCall) { String sourceCode = - "public class MyClass {\n" + - " void foo() {\n" + - " Schema.getGlobalDescribe();\n" + - " Schema.getGlobalDescribe();\n" + - " }\n" + - "}\n"; + "public class MyClass {\n" + + " void foo() {\n" + + " " + + firstCall + + "();\n" + + " " + + secondCall + + "();\n" + + " }\n" + + "}\n"; - assertNoViolation(RULE, sourceCode); + assertViolations( + RULE, + sourceCode, + expect( + 4, + secondCall, + 3, + "MyClass", + RuleConstants.RepetitionType.MULTIPLE, + firstCall)); } @Test public void testForEachLoopInClassInstance() { String sourceCode[] = { - "public class MyClass {\n" + - " void foo() {\n" + - " Another[] anotherList = new Another[] {new Another()};\n" + - " for (Another an : anotherList) {\n" + - " an.exec();\n" + - " }\n" + - " }\n" + - "}\n", - "public class Another {\n" + - "void exec() {\n" + - " Schema.getGlobalDescribe();\n" + - "}\n" + - "}\n" + "public class MyClass {\n" + + " void foo() {\n" + + " Another[] anotherList = new Another[] {new Another()};\n" + + " for (Another an : anotherList) {\n" + + " an.exec();\n" + + " }\n" + + " }\n" + + "}\n", + "public class Another {\n" + + "void exec() {\n" + + " Schema.getGlobalDescribe();\n" + + "}\n" + + "}\n" }; - assertNoViolation(RULE, sourceCode); - + assertViolations( + RULE, + sourceCode, + expect( + 3, + RuleConstants.METHOD_SCHEMA_GET_GLOBAL_DESCRIBE, + 4, + "MyClass", + RuleConstants.RepetitionType.LOOP, + ASTConstants.NodeType.FOR_EACH_STATEMENT)); } + /** TODO: Handle path expansion from array[index].methodCall() */ @Test + @Disabled public void testForLoopInClassInstance() { String sourceCode[] = { - "public class MyClass {\n" + - " void foo() {\n" + - " Another[] anotherList = new Another[] {new Another()};\n" + - " for (Integer i = 0 ; i < anotherList.size; i++) {\n" + - " anotherList[i].exec();\n" + - " }\n" + - " }\n" + - "}\n", - "public class Another {\n" + - "void exec() {\n" + - " Schema.getGlobalDescribe();\n" + - "}\n" + - "}\n" + "public class MyClass {\n" + + " void foo() {\n" + + " Another[] anotherList = new Another[] {new Another()};\n" + + " for (Integer i = 0 ; i < anotherList.size; i++) {\n" + + " anotherList[i].exec();\n" + + " }\n" + + " }\n" + + "}\n", + "public class Another {\n" + + "void exec() {\n" + + " Schema.getGlobalDescribe();\n" + + "}\n" + + "}\n" }; - assertNoViolation(RULE, sourceCode); - + assertViolations( + RULE, + sourceCode, + expect( + 3, + RuleConstants.METHOD_SCHEMA_GET_GLOBAL_DESCRIBE, + 4, + "MyClass", + RuleConstants.RepetitionType.LOOP, + ASTConstants.NodeType.FOR_LOOP_STATEMENT)); } @Test public void testForLoopConditionalOnClassInstance() { String sourceCode[] = { - "public class MyClass {\n" + - " void foo() {\n" + - " Another[] anotherList = new Another[] {new Another(false), new Another(false)};\n" + - " for (Another an : anotherList) {\n" + - " an.exec();\n" + - " }\n" + - " }\n" + - "}\n", - "public class Another {\n" + - "boolean shouldCheck;\n" + - "Another(boolean b) {\n" + - " shouldCheck = b;\n" + - "}\n" + - "void exec() {\n" + - " if (shouldCheck) {\n" + - " Schema.getGlobalDescribe();\n" + - " }\n" + - "}\n" + - "}\n" + "public class MyClass {\n" + + " void foo() {\n" + + " Another[] anotherList = new Another[] {new Another(false), new Another(false)};\n" + + " for (Another an : anotherList) {\n" + + " an.exec();\n" + + " }\n" + + " }\n" + + "}\n", + "public class Another {\n" + + "boolean shouldCheck;\n" + + "Another(boolean b) {\n" + + " shouldCheck = b;\n" + + "}\n" + + "void exec() {\n" + + " if (shouldCheck) {\n" + + " Schema.getGlobalDescribe();\n" + + " }\n" + + "}\n" + + "}\n" }; - assertNoViolation(RULE, sourceCode); + assertViolations( + RULE, + sourceCode, + expect( + 8, + RuleConstants.METHOD_SCHEMA_GET_GLOBAL_DESCRIBE, + 4, + "MyClass", + RuleConstants.RepetitionType.LOOP, + ASTConstants.NodeType.FOR_EACH_STATEMENT)); } @Test // TODO: Check if this is a false positive. Static block should get invoked only once public void testLoopFromStaticBlock() { - String[] sourceCode = - { - "public class MyClass {\n" + - " void foo(String[] objectNames) {\n" + - " for (Integer i = 0; i < objectNames.size; i++) {\n" + - " AnotherClass.doNothing();\n" + - " }\n" + - " }\n" + - "}\n", - "public class AnotherClass {\n" + - " static {\n" + - " Schema.getGlobalDescribe();\n" + - " }\n" + - " static void doNothing() {\n" + - " // do nothing \n" + - " }\n" + - "}\n" - }; + String[] sourceCode = { + "public class MyClass {\n" + + " void foo(String[] objectNames) {\n" + + " for (Integer i = 0; i < objectNames.size; i++) {\n" + + " AnotherClass.doNothing();\n" + + " }\n" + + " }\n" + + "}\n", + "public class AnotherClass {\n" + + " static {\n" + + " Schema.getGlobalDescribe();\n" + + " }\n" + + " static void doNothing() {\n" + + " // do nothing \n" + + " }\n" + + "}\n" + }; - assertNoViolation(RULE, sourceCode); + assertViolations( + RULE, + sourceCode, + expect( + 3, + RuleConstants.METHOD_SCHEMA_GET_GLOBAL_DESCRIBE, + 3, + "MyClass", + RuleConstants.RepetitionType.LOOP, + ASTConstants.NodeType.FOR_LOOP_STATEMENT)); } + @Test + public void testLoopAfterGgdShouldNotGetViolation() { + String sourceCode = + "public class MyClass {\n" + + " void foo(String[] objectNames) {\n" + + " Schema.getGlobalDescribe();\n" + + " for (String name: objectNames) {\n" + + " System.debug(name);\n" + + " }\n" + + " }\n" + + "}\n"; + assertNoViolation(RULE, sourceCode); + } } diff --git a/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/BaseAvoidMultipleMassSchemaLookupTest.java b/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/BaseAvoidMultipleMassSchemaLookupTest.java new file mode 100644 index 000000000..99bc7cf88 --- /dev/null +++ b/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/BaseAvoidMultipleMassSchemaLookupTest.java @@ -0,0 +1,19 @@ +package com.salesforce.rules.getglobaldescribe; + +import com.salesforce.testutils.BasePathBasedRuleTest; +import com.salesforce.testutils.ViolationWrapper; + +/** Base class to tests for {@link com.salesforce.rules.AvoidMultipleMassSchemaLookup} */ +public abstract class BaseAvoidMultipleMassSchemaLookupTest extends BasePathBasedRuleTest { + + protected ViolationWrapper.MassSchemaLookupInfoBuilder expect( + int sinkLine, + String sinkMethodName, + int occurrenceLine, + String occurrenceClassName, + RuleConstants.RepetitionType type, + String typeInfo) { + return ViolationWrapper.MassSchemaLookupInfoBuilder.get( + sinkLine, sinkMethodName, occurrenceLine, occurrenceClassName, type, typeInfo); + } +} diff --git a/sfge/src/test/java/com/salesforce/testutils/ViolationWrapper.java b/sfge/src/test/java/com/salesforce/testutils/ViolationWrapper.java index addd5c93a..29b076ac4 100644 --- a/sfge/src/test/java/com/salesforce/testutils/ViolationWrapper.java +++ b/sfge/src/test/java/com/salesforce/testutils/ViolationWrapper.java @@ -9,6 +9,8 @@ import com.salesforce.rules.fls.apex.operations.FlsViolationInfo; import com.salesforce.rules.fls.apex.operations.FlsViolationMessageUtil; import com.salesforce.rules.fls.apex.operations.UnresolvedCrudFlsViolationInfo; +import com.salesforce.rules.getglobaldescribe.MassSchemaLookupInfoUtil; +import com.salesforce.rules.getglobaldescribe.RuleConstants; import java.util.Arrays; import java.util.TreeSet; import java.util.function.Function; @@ -197,6 +199,49 @@ public String getMessage() { } } + /** + * Message builder to help with testing {@link com.salesforce.rules.AvoidMultipleMassSchemaLookup}. + */ + public static class MassSchemaLookupInfoBuilder extends ViolationBuilder { + private final String sinkMethodName; + private final RuleConstants.RepetitionType repetitionType; + private final String typeInfo; + private final String occurrenceClassName; + private final int occurrenceLine; + + private MassSchemaLookupInfoBuilder( + int sinkLine, + String sinkMethodName, + int occurrenceLine, + String occurrenceClassName, + RuleConstants.RepetitionType type, + String typeInfo) { + super(sinkLine); + this.sinkMethodName = sinkMethodName; + this.occurrenceLine = occurrenceLine; + this.occurrenceClassName = occurrenceClassName; + this.repetitionType = type; + this.typeInfo = typeInfo; + } + + public static MassSchemaLookupInfoBuilder get( + int sinkLine, + String sinkMethodName, + int occurrenceLine, + String occurrenceClassName, + RuleConstants.RepetitionType type, + String typeInfo) { + return new MassSchemaLookupInfoBuilder( + sinkLine, sinkMethodName, occurrenceLine, occurrenceClassName, type, typeInfo); + } + + @Override + public String getMessage() { + return MassSchemaLookupInfoUtil.getMessage( + sinkMethodName, repetitionType, typeInfo, occurrenceClassName, occurrenceLine); + } + } + public static class MessageBuilder { private final int line; private final String violationMsg; From 905f2e4da420ba86722d839c10e27a6595fbae5b Mon Sep 17 00:00:00 2001 From: Roopa Mohan Date: Thu, 20 Apr 2023 17:32:38 -0700 Subject: [PATCH 04/10] Renaming to MultipleMassSchemaLookupRule --- ....java => MultipleMassSchemaLookupRule.java} | 18 +++++++++--------- .../salesforce/rules/PathBasedRuleRunner.java | 2 +- .../MassSchemaLookupInfo.java | 2 +- .../MassSchemaLookupInfoUtil.java | 2 +- .../MultipleMassSchemaLookupRuleHandler.java} | 18 +++++++++--------- .../MultipleMassSchemaLookupVisitor.java | 5 +++-- .../RuleConstants.java | 2 +- .../LoopDetectionVisitor.java | 6 +++--- .../BaseAvoidMultipleMassSchemaLookupTest.java | 5 +++-- .../MultipleMassSchemaLookupRuleTest.java} | 12 ++++++------ .../salesforce/testutils/ViolationWrapper.java | 9 ++++----- 11 files changed, 41 insertions(+), 40 deletions(-) rename sfge/src/main/java/com/salesforce/rules/{AvoidMultipleMassSchemaLookup.java => MultipleMassSchemaLookupRule.java} (68%) rename sfge/src/main/java/com/salesforce/rules/{getglobaldescribe => multiplemassschemalookup}/MassSchemaLookupInfo.java (98%) rename sfge/src/main/java/com/salesforce/rules/{getglobaldescribe => multiplemassschemalookup}/MassSchemaLookupInfoUtil.java (97%) rename sfge/src/main/java/com/salesforce/rules/{getglobaldescribe/AvoidMultipleMassSchemaLookupHandler.java => multiplemassschemalookup/MultipleMassSchemaLookupRuleHandler.java} (79%) rename sfge/src/main/java/com/salesforce/rules/{getglobaldescribe => multiplemassschemalookup}/MultipleMassSchemaLookupVisitor.java (92%) rename sfge/src/main/java/com/salesforce/rules/{getglobaldescribe => multiplemassschemalookup}/RuleConstants.java (96%) rename sfge/src/main/java/com/salesforce/rules/{getglobaldescribe => ops}/LoopDetectionVisitor.java (80%) rename sfge/src/test/java/com/salesforce/rules/{getglobaldescribe => multiplemassschemalookup}/BaseAvoidMultipleMassSchemaLookupTest.java (78%) rename sfge/src/test/java/com/salesforce/rules/{getglobaldescribe/AvoidMultipleMassSchemaLookupTest.java => multiplemassschemalookup/MultipleMassSchemaLookupRuleTest.java} (96%) diff --git a/sfge/src/main/java/com/salesforce/rules/AvoidMultipleMassSchemaLookup.java b/sfge/src/main/java/com/salesforce/rules/MultipleMassSchemaLookupRule.java similarity index 68% rename from sfge/src/main/java/com/salesforce/rules/AvoidMultipleMassSchemaLookup.java rename to sfge/src/main/java/com/salesforce/rules/MultipleMassSchemaLookupRule.java index d2aaf50d7..084fdad9c 100644 --- a/sfge/src/main/java/com/salesforce/rules/AvoidMultipleMassSchemaLookup.java +++ b/sfge/src/main/java/com/salesforce/rules/MultipleMassSchemaLookupRule.java @@ -3,18 +3,18 @@ import com.salesforce.config.UserFacingMessages; import com.salesforce.graph.ApexPath; import com.salesforce.graph.vertex.BaseSFVertex; -import com.salesforce.rules.getglobaldescribe.AvoidMultipleMassSchemaLookupHandler; +import com.salesforce.rules.multiplemassschemalookup.MultipleMassSchemaLookupRuleHandler; import java.util.ArrayList; import java.util.List; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; /** Rule to detect possible performance degradations while invoking Schema.getGlobalDescribe(). */ -public class AvoidMultipleMassSchemaLookup extends AbstractPathTraversalRule { +public class MultipleMassSchemaLookupRule extends AbstractPathTraversalRule { - private final AvoidMultipleMassSchemaLookupHandler ruleHandler; + private final MultipleMassSchemaLookupRuleHandler ruleHandler; - private AvoidMultipleMassSchemaLookup() { - ruleHandler = AvoidMultipleMassSchemaLookupHandler.getInstance(); + private MultipleMassSchemaLookupRule() { + ruleHandler = MultipleMassSchemaLookupRuleHandler.getInstance(); } @Override @@ -51,13 +51,13 @@ protected boolean isEnabled() { return false; } - public static AvoidMultipleMassSchemaLookup getInstance() { - return AvoidMultipleMassSchemaLookup.LazyHolder.INSTANCE; + public static MultipleMassSchemaLookupRule getInstance() { + return MultipleMassSchemaLookupRule.LazyHolder.INSTANCE; } private static final class LazyHolder { // Postpone initialization until first use - private static final AvoidMultipleMassSchemaLookup INSTANCE = - new AvoidMultipleMassSchemaLookup(); + private static final MultipleMassSchemaLookupRule INSTANCE = + new MultipleMassSchemaLookupRule(); } } diff --git a/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java b/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java index d3c408868..fac0b7f92 100644 --- a/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java +++ b/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java @@ -13,7 +13,7 @@ import com.salesforce.graph.vertex.SFVertex; import com.salesforce.rules.fls.apex.operations.FlsViolationInfo; import com.salesforce.rules.fls.apex.operations.FlsViolationMessageUtil; -import com.salesforce.rules.getglobaldescribe.MassSchemaLookupInfo; +import com.salesforce.rules.multiplemassschemalookup.MassSchemaLookupInfo; import com.salesforce.rules.ops.ProgressListener; import com.salesforce.rules.ops.ProgressListenerProvider; import java.util.*; diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupInfo.java b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfo.java similarity index 98% rename from sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupInfo.java rename to sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfo.java index cb0a406a1..b0c78ab34 100644 --- a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupInfo.java +++ b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfo.java @@ -1,4 +1,4 @@ -package com.salesforce.rules.getglobaldescribe; +package com.salesforce.rules.multiplemassschemalookup; import com.salesforce.exception.ProgrammingException; import com.salesforce.graph.vertex.MethodCallExpressionVertex; diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupInfoUtil.java b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfoUtil.java similarity index 97% rename from sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupInfoUtil.java rename to sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfoUtil.java index 180929002..6fab278b9 100644 --- a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MassSchemaLookupInfoUtil.java +++ b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfoUtil.java @@ -1,4 +1,4 @@ -package com.salesforce.rules.getglobaldescribe; +package com.salesforce.rules.multiplemassschemalookup; import com.salesforce.config.UserFacingMessages; import com.salesforce.graph.vertex.MethodCallExpressionVertex; diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupHandler.java b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleHandler.java similarity index 79% rename from sfge/src/main/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupHandler.java rename to sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleHandler.java index 2df086099..e6034f55e 100644 --- a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupHandler.java +++ b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleHandler.java @@ -1,4 +1,4 @@ -package com.salesforce.rules.getglobaldescribe; +package com.salesforce.rules.multiplemassschemalookup; import com.salesforce.exception.ProgrammingException; import com.salesforce.graph.ApexPath; @@ -7,17 +7,17 @@ import com.salesforce.graph.vertex.MethodCallExpressionVertex; import com.salesforce.graph.vertex.SFVertex; import com.salesforce.graph.visitor.ApexPathWalker; -import com.salesforce.rules.AvoidMultipleMassSchemaLookup; +import com.salesforce.rules.MultipleMassSchemaLookupRule; import java.util.Set; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; -/** Executes internals of {@link AvoidMultipleMassSchemaLookup} */ -public class AvoidMultipleMassSchemaLookupHandler { +/** Executes internals of {@link MultipleMassSchemaLookupRule} */ +public class MultipleMassSchemaLookupRuleHandler { /** * @param vertex to consider for analysis * @return true if the vertex parameter requires to be treated as a target vertex for {@link - * AvoidMultipleMassSchemaLookup}. + * MultipleMassSchemaLookupRule}. */ public boolean test(BaseSFVertex vertex) { return vertex instanceof MethodCallExpressionVertex @@ -46,13 +46,13 @@ public Set detectViolations( return ruleVisitor.getViolation(); } - public static AvoidMultipleMassSchemaLookupHandler getInstance() { - return AvoidMultipleMassSchemaLookupHandler.LazyHolder.INSTANCE; + public static MultipleMassSchemaLookupRuleHandler getInstance() { + return MultipleMassSchemaLookupRuleHandler.LazyHolder.INSTANCE; } private static final class LazyHolder { // Postpone initialization until first use - private static final AvoidMultipleMassSchemaLookupHandler INSTANCE = - new AvoidMultipleMassSchemaLookupHandler(); + private static final MultipleMassSchemaLookupRuleHandler INSTANCE = + new MultipleMassSchemaLookupRuleHandler(); } } diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MultipleMassSchemaLookupVisitor.java b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupVisitor.java similarity index 92% rename from sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MultipleMassSchemaLookupVisitor.java rename to sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupVisitor.java index 22559c0ea..3ce3a549f 100644 --- a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/MultipleMassSchemaLookupVisitor.java +++ b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupVisitor.java @@ -1,9 +1,10 @@ -package com.salesforce.rules.getglobaldescribe; +package com.salesforce.rules.multiplemassschemalookup; import com.salesforce.graph.symbols.SymbolProvider; import com.salesforce.graph.vertex.BaseSFVertex; import com.salesforce.graph.vertex.MethodCallExpressionVertex; import com.salesforce.graph.vertex.SFVertex; +import com.salesforce.rules.ops.LoopDetectionVisitor; import java.util.HashSet; import java.util.Set; @@ -30,7 +31,7 @@ class MultipleMassSchemaLookupVisitor extends LoopDetectionVisitor { } @Override - void execAfterLoopVertexVisit(BaseSFVertex vertex, SymbolProvider symbols) { + protected void execAfterLoopVertexVisit(BaseSFVertex vertex, SymbolProvider symbols) { if (shouldContinue()) { violations.add( new MassSchemaLookupInfo( diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/RuleConstants.java b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/RuleConstants.java similarity index 96% rename from sfge/src/main/java/com/salesforce/rules/getglobaldescribe/RuleConstants.java rename to sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/RuleConstants.java index 6cd735eb6..2714672f7 100644 --- a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/RuleConstants.java +++ b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/RuleConstants.java @@ -1,4 +1,4 @@ -package com.salesforce.rules.getglobaldescribe; +package com.salesforce.rules.multiplemassschemalookup; import com.google.common.collect.ImmutableSet; import com.salesforce.config.UserFacingMessages; diff --git a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/LoopDetectionVisitor.java b/sfge/src/main/java/com/salesforce/rules/ops/LoopDetectionVisitor.java similarity index 80% rename from sfge/src/main/java/com/salesforce/rules/getglobaldescribe/LoopDetectionVisitor.java rename to sfge/src/main/java/com/salesforce/rules/ops/LoopDetectionVisitor.java index e078b0a18..02c1f830e 100644 --- a/sfge/src/main/java/com/salesforce/rules/getglobaldescribe/LoopDetectionVisitor.java +++ b/sfge/src/main/java/com/salesforce/rules/ops/LoopDetectionVisitor.java @@ -1,13 +1,13 @@ -package com.salesforce.rules.getglobaldescribe; +package com.salesforce.rules.ops; import com.salesforce.graph.symbols.SymbolProvider; import com.salesforce.graph.vertex.*; import com.salesforce.graph.visitor.DefaultNoOpPathVertexVisitor; /** Visitor that gets notified when a loop vertex is invoked in the path. */ -abstract class LoopDetectionVisitor extends DefaultNoOpPathVertexVisitor { +public abstract class LoopDetectionVisitor extends DefaultNoOpPathVertexVisitor { - abstract void execAfterLoopVertexVisit(BaseSFVertex vertex, SymbolProvider symbols); + protected abstract void execAfterLoopVertexVisit(BaseSFVertex vertex, SymbolProvider symbols); @Override public void afterVisit(DoLoopStatementVertex vertex, SymbolProvider symbols) { diff --git a/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/BaseAvoidMultipleMassSchemaLookupTest.java b/sfge/src/test/java/com/salesforce/rules/multiplemassschemalookup/BaseAvoidMultipleMassSchemaLookupTest.java similarity index 78% rename from sfge/src/test/java/com/salesforce/rules/getglobaldescribe/BaseAvoidMultipleMassSchemaLookupTest.java rename to sfge/src/test/java/com/salesforce/rules/multiplemassschemalookup/BaseAvoidMultipleMassSchemaLookupTest.java index 99bc7cf88..da79065cc 100644 --- a/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/BaseAvoidMultipleMassSchemaLookupTest.java +++ b/sfge/src/test/java/com/salesforce/rules/multiplemassschemalookup/BaseAvoidMultipleMassSchemaLookupTest.java @@ -1,9 +1,10 @@ -package com.salesforce.rules.getglobaldescribe; +package com.salesforce.rules.multiplemassschemalookup; +import com.salesforce.rules.MultipleMassSchemaLookupRule; import com.salesforce.testutils.BasePathBasedRuleTest; import com.salesforce.testutils.ViolationWrapper; -/** Base class to tests for {@link com.salesforce.rules.AvoidMultipleMassSchemaLookup} */ +/** Base class to tests for {@link MultipleMassSchemaLookupRule} */ public abstract class BaseAvoidMultipleMassSchemaLookupTest extends BasePathBasedRuleTest { protected ViolationWrapper.MassSchemaLookupInfoBuilder expect( diff --git a/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupTest.java b/sfge/src/test/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleTest.java similarity index 96% rename from sfge/src/test/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupTest.java rename to sfge/src/test/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleTest.java index acbe869bd..fb235d42d 100644 --- a/sfge/src/test/java/com/salesforce/rules/getglobaldescribe/AvoidMultipleMassSchemaLookupTest.java +++ b/sfge/src/test/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleTest.java @@ -1,17 +1,17 @@ -package com.salesforce.rules.getglobaldescribe; +package com.salesforce.rules.multiplemassschemalookup; import com.salesforce.apex.jorje.ASTConstants; -import com.salesforce.rules.AvoidMultipleMassSchemaLookup; +import com.salesforce.rules.MultipleMassSchemaLookupRule; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; -// TODO: break down test, add updated assertions -public class AvoidMultipleMassSchemaLookupTest extends BaseAvoidMultipleMassSchemaLookupTest { +// TODO: Breakdown to more test suites +public class MultipleMassSchemaLookupRuleTest extends BaseAvoidMultipleMassSchemaLookupTest { - private static final AvoidMultipleMassSchemaLookup RULE = - AvoidMultipleMassSchemaLookup.getInstance(); + private static final MultipleMassSchemaLookupRule RULE = + MultipleMassSchemaLookupRule.getInstance(); @CsvSource({ "ForEachStatement, for (String s : myList)", diff --git a/sfge/src/test/java/com/salesforce/testutils/ViolationWrapper.java b/sfge/src/test/java/com/salesforce/testutils/ViolationWrapper.java index 29b076ac4..969316f58 100644 --- a/sfge/src/test/java/com/salesforce/testutils/ViolationWrapper.java +++ b/sfge/src/test/java/com/salesforce/testutils/ViolationWrapper.java @@ -4,13 +4,14 @@ import com.salesforce.collections.CollectionUtil; import com.salesforce.config.UserFacingMessages; import com.salesforce.graph.ops.SoqlParserUtil; +import com.salesforce.rules.MultipleMassSchemaLookupRule; import com.salesforce.rules.fls.apex.operations.FlsConstants; import com.salesforce.rules.fls.apex.operations.FlsStripInaccessibleWarningInfo; import com.salesforce.rules.fls.apex.operations.FlsViolationInfo; import com.salesforce.rules.fls.apex.operations.FlsViolationMessageUtil; import com.salesforce.rules.fls.apex.operations.UnresolvedCrudFlsViolationInfo; -import com.salesforce.rules.getglobaldescribe.MassSchemaLookupInfoUtil; -import com.salesforce.rules.getglobaldescribe.RuleConstants; +import com.salesforce.rules.multiplemassschemalookup.MassSchemaLookupInfoUtil; +import com.salesforce.rules.multiplemassschemalookup.RuleConstants; import java.util.Arrays; import java.util.TreeSet; import java.util.function.Function; @@ -199,9 +200,7 @@ public String getMessage() { } } - /** - * Message builder to help with testing {@link com.salesforce.rules.AvoidMultipleMassSchemaLookup}. - */ + /** Message builder to help with testing {@link MultipleMassSchemaLookupRule}. */ public static class MassSchemaLookupInfoBuilder extends ViolationBuilder { private final String sinkMethodName; private final RuleConstants.RepetitionType repetitionType; From d21844a7a9c2f30cb82115f7328634c665cdc0b8 Mon Sep 17 00:00:00 2001 From: Roopa Mohan Date: Tue, 25 Apr 2023 14:44:15 -0700 Subject: [PATCH 05/10] Renames to match updated rule name --- .../salesforce/config/UserFacingMessages.java | 5 +++-- .../rules/MultipleMassSchemaLookupRule.java | 2 +- .../salesforce/rules/PathBasedRuleRunner.java | 6 +++--- .../fls/apex/operations/FlsViolationInfo.java | 16 --------------- .../MassSchemaLookupInfo.java | 20 ------------------- .../MassSchemaLookupInfoUtil.java | 2 +- .../RuleConstants.java | 4 ++-- 7 files changed, 10 insertions(+), 45 deletions(-) diff --git a/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java b/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java index 70afae8e4..590a5622c 100644 --- a/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java +++ b/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java @@ -14,7 +14,8 @@ public static final class RuleDescriptions { "Identifies abstract classes and interfaces that are non-global and don't have implementations or extensions."; public static final String UNUSED_METHOD_RULE = "Identifies methods that aren't invoked from recognized entry points."; - public static final String GET_GLOBAL_DESCRIBE_VIOLATION_RULE = "Detects Schema.getGlobalDescribe() calls made in scenarios that could result in performance degradation."; + public static final String MULTIPLE_MASS_SCHEMA_LOOKUP_RULE = + "Detects Schema.getGlobalDescribe() calls made in scenarios that could result in performance degradation."; } public static final class RuleViolationTemplates { @@ -77,7 +78,7 @@ public static final class CompilationErrors { public static final String EXCEPTION_FORMAT_TEMPLATE = "%s, Caused by:\n%s"; } - public static final class AvoidExcessiveSchemaLookupsTemplates { + public static final class MultipleMassSchemaLookupRuleTemplates { public static final String MESSAGE_TEMPLATE = "Avoid excessive Schema lookups in a single path. Detected %s %s at %s:%d."; public static final String OCCURRENCE_LOOP_TEMPLATE = "inside a %s"; diff --git a/sfge/src/main/java/com/salesforce/rules/MultipleMassSchemaLookupRule.java b/sfge/src/main/java/com/salesforce/rules/MultipleMassSchemaLookupRule.java index 084fdad9c..9f0a6ddcd 100644 --- a/sfge/src/main/java/com/salesforce/rules/MultipleMassSchemaLookupRule.java +++ b/sfge/src/main/java/com/salesforce/rules/MultipleMassSchemaLookupRule.java @@ -38,7 +38,7 @@ protected int getSeverity() { @Override protected String getDescription() { - return UserFacingMessages.RuleDescriptions.GET_GLOBAL_DESCRIBE_VIOLATION_RULE; + return UserFacingMessages.RuleDescriptions.MULTIPLE_MASS_SCHEMA_LOOKUP_RULE; } @Override diff --git a/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java b/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java index fac0b7f92..bbb6d235b 100644 --- a/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java +++ b/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java @@ -172,11 +172,11 @@ private void executeRulesOnPaths(List paths) { } } - private void convertGgdToViolations(HashSet ggdViolationInfos) { + private void convertGgdToViolations(HashSet massSchemaLookupInfos) { // TODO: consolidate by sink/source - for (MassSchemaLookupInfo ggdViolationInfo : ggdViolationInfos) { - violations.add(ggdViolationInfo.convert()); + for (MassSchemaLookupInfo massSchemaLookupInfo : massSchemaLookupInfos) { + violations.add(massSchemaLookupInfo.convert()); } } diff --git a/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationInfo.java b/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationInfo.java index 9174ff309..00aa719f5 100644 --- a/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationInfo.java +++ b/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationInfo.java @@ -169,20 +169,4 @@ public String toString() { public FlsViolationInfo deepClone() { return new FlsViolationInfo(this); } - - // @Override - public Violation convert() { - final String violationMessage = FlsViolationMessageUtil.constructMessage(this); - final Optional sourceVertex = this.getSourceVertex(); - final Optional sinkVertex = this.getSinkVertex(); - if (sinkVertex.isPresent()) { - final Violation.RuleViolation ruleViolation = - new Violation.PathBasedRuleViolation( - violationMessage, sourceVertex.get(), sinkVertex.get()); - ruleViolation.setPropertiesFromRule(rule); - return ruleViolation; - } else { - throw new ProgrammingException("Sink vertex not set in flsViolationInfo: " + this); - } - } } diff --git a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfo.java b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfo.java index b0c78ab34..0b88dd0a2 100644 --- a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfo.java +++ b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfo.java @@ -76,24 +76,4 @@ public SFVertex getRepetitionVertex() { return repetitionVertex; } - // private String getMessage() { - // return - // String.format(UserFacingMessages.AvoidExcessiveSchemaLookupsTemplates.MESSAGE_TEMPLATE, - // sinkVertex.getFullMethodName(), - // getOccurrenceMessage(), - // repetitionVertex.getDefiningType(), - // repetitionVertex.getBeginLine()); - // } - // - // private String getOccurrenceMessage() { - // if (RuleConstants.RepetitionType.MULTIPLE.equals(repetitionType)) { - // // Use method name on template message - // return repetitionType.getMessage(((MethodCallExpressionVertex) - // repetitionVertex).getFullMethodName()); - // } else { - // // Use Loop type on template message - // return repetitionType.getMessage(repetitionVertex.getDefiningType()); - // } - // } - } diff --git a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfoUtil.java b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfoUtil.java index 6fab278b9..42c3f4021 100644 --- a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfoUtil.java +++ b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfoUtil.java @@ -26,7 +26,7 @@ public static String getMessage( final String occurrenceMessage = getOccurrenceMessage(repetitionType, occurrenceInfoValue); return String.format( - UserFacingMessages.AvoidExcessiveSchemaLookupsTemplates.MESSAGE_TEMPLATE, + UserFacingMessages.MultipleMassSchemaLookupRuleTemplates.MESSAGE_TEMPLATE, sinkMethodName, occurrenceMessage, occurrenceClassName, diff --git a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/RuleConstants.java b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/RuleConstants.java index 2714672f7..1571ef530 100644 --- a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/RuleConstants.java +++ b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/RuleConstants.java @@ -23,9 +23,9 @@ static boolean isSchemaExpensiveMethod(MethodCallExpressionVertex vertex) { /** Enum to indicate the type of repetition the method call was subjected. */ public enum RepetitionType { - LOOP(UserFacingMessages.AvoidExcessiveSchemaLookupsTemplates.OCCURRENCE_LOOP_TEMPLATE), + LOOP(UserFacingMessages.MultipleMassSchemaLookupRuleTemplates.OCCURRENCE_LOOP_TEMPLATE), MULTIPLE( - UserFacingMessages.AvoidExcessiveSchemaLookupsTemplates + UserFacingMessages.MultipleMassSchemaLookupRuleTemplates .OCCURRENCE_MULTIPLE_TEMPLATE); String messageTemplate; From 4a48ffcbd4b551594b6112acff702f9b9ca384a2 Mon Sep 17 00:00:00 2001 From: Roopa Mohan Date: Mon, 1 May 2023 17:19:20 -0700 Subject: [PATCH 06/10] Fixing NPE on comparator issue --- .../salesforce/rules/PathBasedRuleRunner.java | 5 +- .../MassSchemaLookupInfo.java | 3 +- .../test/java/com/salesforce/TestUtil.java | 20 +++++--- .../MultipleRuleViolationsScenarioTest.java | 47 +++++++++++++++++++ 4 files changed, 66 insertions(+), 9 deletions(-) create mode 100644 sfge/src/test/java/com/salesforce/rules/MultipleRuleViolationsScenarioTest.java diff --git a/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java b/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java index bbb6d235b..1c7daab99 100644 --- a/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java +++ b/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java @@ -176,7 +176,10 @@ private void convertGgdToViolations(HashSet massSchemaLook // TODO: consolidate by sink/source for (MassSchemaLookupInfo massSchemaLookupInfo : massSchemaLookupInfos) { - violations.add(massSchemaLookupInfo.convert()); + Violation.RuleViolation violation = massSchemaLookupInfo.convert(); + // FIXME + violation.setPropertiesFromRule(MultipleMassSchemaLookupRule.getInstance()); + violations.add(violation); } } diff --git a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfo.java b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfo.java index 0b88dd0a2..0c43f3323 100644 --- a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfo.java +++ b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfo.java @@ -55,7 +55,7 @@ private void validateInput( } // @Override - public Violation convert() { + public Violation.PathBasedRuleViolation convert() { return new Violation.PathBasedRuleViolation( MassSchemaLookupInfoUtil.getMessage(this), sourceVertex, sinkVertex); } @@ -75,5 +75,4 @@ public RuleConstants.RepetitionType getRepetitionType() { public SFVertex getRepetitionVertex() { return repetitionVertex; } - } diff --git a/sfge/src/test/java/com/salesforce/TestUtil.java b/sfge/src/test/java/com/salesforce/TestUtil.java index c07e868de..7118e9525 100644 --- a/sfge/src/test/java/com/salesforce/TestUtil.java +++ b/sfge/src/test/java/com/salesforce/TestUtil.java @@ -5,7 +5,6 @@ import static org.hamcrest.Matchers.hasSize; import static org.junit.jupiter.api.Assertions.fail; -import com.google.common.collect.Lists; import com.google.gson.Gson; import com.salesforce.apex.jorje.ASTConstants; import com.salesforce.apex.jorje.AstNodeWrapper; @@ -45,10 +44,7 @@ import java.net.URISyntaxException; import java.nio.file.Path; import java.nio.file.Paths; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; -import java.util.Optional; +import java.util.*; import java.util.stream.Collectors; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -300,6 +296,17 @@ public static List getViolations( String definingType, String methodName, Boolean renderXml) { + + return getViolations(g, sourceCode, definingType, methodName, renderXml, rule); + } + + public static List getViolations( + GraphTraversalSource g, + String[] sourceCode, + String definingType, + String methodName, + Boolean renderXml, + AbstractPathBasedRule... rules) { if (USE_EXISTING_GRAPH != sourceCode) { Config.Builder builder = Config.Builder.get(g, sourceCode); if (renderXml != null) { @@ -310,8 +317,9 @@ public static List getViolations( } final MethodVertex methodVertex = getMethodVertex(g, definingType, methodName); + final PathBasedRuleRunner ruleRunner = - new PathBasedRuleRunner(g, Lists.newArrayList(rule), methodVertex); + new PathBasedRuleRunner(g, Arrays.asList(rules), methodVertex); final List violations = new ArrayList<>(ruleRunner.runRules()); return violations; diff --git a/sfge/src/test/java/com/salesforce/rules/MultipleRuleViolationsScenarioTest.java b/sfge/src/test/java/com/salesforce/rules/MultipleRuleViolationsScenarioTest.java new file mode 100644 index 000000000..4e84e54fc --- /dev/null +++ b/sfge/src/test/java/com/salesforce/rules/MultipleRuleViolationsScenarioTest.java @@ -0,0 +1,47 @@ +package com.salesforce.rules; + +import com.salesforce.TestUtil; +import com.salesforce.collections.CollectionUtil; +import com.salesforce.testutils.BasePathBasedRuleTest; +import java.util.List; +import java.util.TreeSet; + +import org.hamcrest.MatcherAssert; +import org.hamcrest.Matchers; +import org.junit.jupiter.api.Test; + +public class MultipleRuleViolationsScenarioTest extends BasePathBasedRuleTest { + @Test + public void testFlsAndMmsRules() { + String sourceCode = + "public class MyClass {\n" + + " void foo(String[] objectNames) {\n" + + " for (String objectName: objectNames) {\n" + + " Map schemaDescribe = Schema.getGlobalDescribe();\n" + + " SObjectType sot = schemaDescribe.get(objectName);\n" + + " if (sot.getDescribe().isAccessible()){\n" + + " SObject so = sot.newSObject();\n" + + " insert so;\n" + + " }\n" + + " }\n" + + " }\n" + + "}\n"; + + List violations = + TestUtil.getViolations( + g, + new String[] {sourceCode}, + "MyClass", + "foo", + false, + ApexFlsViolationRule.getInstance(), + MultipleMassSchemaLookupRule.getInstance()); + + // Including an additional step that happens in the actual process. + // Any missed field in the violation would show up here as a NPE on comparator. + final TreeSet sortedViolations = new TreeSet<>(); + sortedViolations.addAll(violations); + + MatcherAssert.assertThat(sortedViolations, Matchers.hasSize(2)); + } +} From 0946b074f9f4c2755208cdf7cb4addcaea8eb583 Mon Sep 17 00:00:00 2001 From: Roopa Mohan Date: Wed, 3 May 2023 15:42:46 -0700 Subject: [PATCH 07/10] Applying UI text changes + minor renames --- .../salesforce/config/UserFacingMessages.java | 6 ++-- .../salesforce/rules/PathBasedRuleRunner.java | 28 +++++++++---------- .../MassSchemaLookupInfoUtil.java | 2 +- ...java => MultipleMassSchemaLookupInfo.java} | 4 +-- .../MultipleMassSchemaLookupRuleHandler.java | 2 +- .../MultipleMassSchemaLookupVisitor.java | 8 +++--- 6 files changed, 24 insertions(+), 26 deletions(-) rename sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/{MassSchemaLookupInfo.java => MultipleMassSchemaLookupInfo.java} (95%) diff --git a/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java b/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java index 590a5622c..2406d2ccc 100644 --- a/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java +++ b/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java @@ -15,7 +15,7 @@ public static final class RuleDescriptions { public static final String UNUSED_METHOD_RULE = "Identifies methods that aren't invoked from recognized entry points."; public static final String MULTIPLE_MASS_SCHEMA_LOOKUP_RULE = - "Detects Schema.getGlobalDescribe() calls made in scenarios that could result in performance degradation."; + "Detects mass schema lookups that can cause performance degradation if made more than once in a path. These methods are: Schema.getGlobalDescribe() and Schema.describeSObjects(...). Flagged lookups include those within a loop or multiple invocations in a path."; } public static final class RuleViolationTemplates { @@ -80,8 +80,8 @@ public static final class CompilationErrors { public static final class MultipleMassSchemaLookupRuleTemplates { public static final String MESSAGE_TEMPLATE = - "Avoid excessive Schema lookups in a single path. Detected %s %s at %s:%d."; - public static final String OCCURRENCE_LOOP_TEMPLATE = "inside a %s"; + "%s was %s at %s:%d."; + public static final String OCCURRENCE_LOOP_TEMPLATE = "called inside a %s"; public static final String OCCURRENCE_MULTIPLE_TEMPLATE = "preceded by a call to %s"; } } diff --git a/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java b/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java index 1c7daab99..fd3c0eb70 100644 --- a/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java +++ b/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java @@ -13,7 +13,7 @@ import com.salesforce.graph.vertex.SFVertex; import com.salesforce.rules.fls.apex.operations.FlsViolationInfo; import com.salesforce.rules.fls.apex.operations.FlsViolationMessageUtil; -import com.salesforce.rules.multiplemassschemalookup.MassSchemaLookupInfo; +import com.salesforce.rules.multiplemassschemalookup.MultipleMassSchemaLookupInfo; import com.salesforce.rules.ops.ProgressListener; import com.salesforce.rules.ops.ProgressListenerProvider; import java.util.*; @@ -120,8 +120,8 @@ private void executeRulesOnPaths(List paths) { // This set holds the violations whose information couldn't be fully processed at creation // time, and // require post-processing. - final HashSet incompleteThrowables = new HashSet<>(); - final HashSet tempDeleteMe = new HashSet<>(); + final HashSet flsViolationInfos = new HashSet<>(); + final HashSet mmsLookupInfos = new HashSet<>(); // For each path... for (ApexPath path : paths) { // If the path's metadata is present... @@ -144,9 +144,10 @@ private void executeRulesOnPaths(List paths) { // to the list of such // objects. if (ruleThrowable instanceof FlsViolationInfo) { - incompleteThrowables.add((FlsViolationInfo) ruleThrowable); - } else if (ruleThrowable instanceof MassSchemaLookupInfo) { - tempDeleteMe.add((MassSchemaLookupInfo) ruleThrowable); + flsViolationInfos.add((FlsViolationInfo) ruleThrowable); + } else if (ruleThrowable instanceof MultipleMassSchemaLookupInfo) { + // FIXME: PR incoming with refactors to this portion + mmsLookupInfos.add((MultipleMassSchemaLookupInfo) ruleThrowable); } else if (ruleThrowable instanceof Violation) { // If the violation is done, it can just go directly into the results // set. @@ -159,8 +160,8 @@ private void executeRulesOnPaths(List paths) { } } - convertToViolations(incompleteThrowables); - convertGgdToViolations(tempDeleteMe); + convertFlsInfoToViolations(flsViolationInfos); + convertMmsInfoToViolations(mmsLookupInfos); if (!foundVertex) { // If no vertices were found, we should log something so that information isn't lost, @@ -172,19 +173,16 @@ private void executeRulesOnPaths(List paths) { } } - private void convertGgdToViolations(HashSet massSchemaLookupInfos) { - // TODO: consolidate by sink/source - - for (MassSchemaLookupInfo massSchemaLookupInfo : massSchemaLookupInfos) { - Violation.RuleViolation violation = massSchemaLookupInfo.convert(); - // FIXME + private void convertMmsInfoToViolations(HashSet mmsLookupInfos) { + for (MultipleMassSchemaLookupInfo mmsLookupInfo : mmsLookupInfos) { + Violation.RuleViolation violation = mmsLookupInfo.convert(); violation.setPropertiesFromRule(MultipleMassSchemaLookupRule.getInstance()); violations.add(violation); } } // TODO: Restructure to make this logic work on other types of violation info too - private void convertToViolations(HashSet flsViolationInfos) { + private void convertFlsInfoToViolations(HashSet flsViolationInfos) { // Consolidate/regroup FLS violations across paths so that there are no // duplicates with different field sets for the same source/sink/dmlOperation final HashSet consolidatedFlsViolationInfos = diff --git a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfoUtil.java b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfoUtil.java index 42c3f4021..9bd2261fb 100644 --- a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfoUtil.java +++ b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfoUtil.java @@ -8,7 +8,7 @@ public final class MassSchemaLookupInfoUtil { private MassSchemaLookupInfoUtil() {} - public static String getMessage(MassSchemaLookupInfo info) { + public static String getMessage(MultipleMassSchemaLookupInfo info) { return getMessage( info.getSinkVertex().getFullMethodName(), info.getRepetitionType(), diff --git a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfo.java b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupInfo.java similarity index 95% rename from sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfo.java rename to sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupInfo.java index 0c43f3323..fc5238f08 100644 --- a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MassSchemaLookupInfo.java +++ b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupInfo.java @@ -6,7 +6,7 @@ import com.salesforce.rules.RuleThrowable; import com.salesforce.rules.Violation; -public class MassSchemaLookupInfo implements RuleThrowable { +public class MultipleMassSchemaLookupInfo implements RuleThrowable { /** * vertex where the expensive Schema lookup happens TODO: Is this always only a @@ -23,7 +23,7 @@ public class MassSchemaLookupInfo implements RuleThrowable { /** Vertex where the repetition occurs */ private final SFVertex repetitionVertex; - public MassSchemaLookupInfo( + public MultipleMassSchemaLookupInfo( SFVertex sourceVertex, MethodCallExpressionVertex sinkVertex, RuleConstants.RepetitionType repetitionType, diff --git a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleHandler.java b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleHandler.java index e6034f55e..4d8606038 100644 --- a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleHandler.java +++ b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleHandler.java @@ -24,7 +24,7 @@ public boolean test(BaseSFVertex vertex) { && RuleConstants.isSchemaExpensiveMethod((MethodCallExpressionVertex) vertex); } - public Set detectViolations( + public Set detectViolations( GraphTraversalSource g, ApexPath path, BaseSFVertex vertex) { if (!(vertex instanceof MethodCallExpressionVertex)) { throw new ProgrammingException( diff --git a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupVisitor.java b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupVisitor.java index 3ce3a549f..e164d7207 100644 --- a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupVisitor.java +++ b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupVisitor.java @@ -19,7 +19,7 @@ class MultipleMassSchemaLookupVisitor extends LoopDetectionVisitor { private final MethodCallExpressionVertex sinkVertex; /** Collects violation information */ - private final HashSet violations; + private final HashSet violations; /** Indicates if the sink vertex has been visited or not. */ private boolean isSinkVisited = false; @@ -34,7 +34,7 @@ class MultipleMassSchemaLookupVisitor extends LoopDetectionVisitor { protected void execAfterLoopVertexVisit(BaseSFVertex vertex, SymbolProvider symbols) { if (shouldContinue()) { violations.add( - new MassSchemaLookupInfo( + new MultipleMassSchemaLookupInfo( sourceVertex, sinkVertex, RuleConstants.RepetitionType.LOOP, vertex)); } } @@ -48,7 +48,7 @@ public void afterVisit(MethodCallExpressionVertex vertex, SymbolProvider symbols isSinkVisited = true; } else if (RuleConstants.isSchemaExpensiveMethod(vertex) && shouldContinue()) { violations.add( - new MassSchemaLookupInfo( + new MultipleMassSchemaLookupInfo( sourceVertex, sinkVertex, RuleConstants.RepetitionType.MULTIPLE, @@ -68,7 +68,7 @@ private boolean shouldContinue() { /** * @return Violations collected by the rule. */ - Set getViolation() { + Set getViolation() { return violations; } } From 81eb5d372a9456d3f41afdeddeb770390643f33b Mon Sep 17 00:00:00 2001 From: Roopa Mohan Date: Wed, 3 May 2023 16:07:29 -0700 Subject: [PATCH 08/10] Enabling MMSLookupRule + format fixes --- .../main/java/com/salesforce/config/UserFacingMessages.java | 3 +-- .../com/salesforce/rules/MultipleMassSchemaLookupRule.java | 2 +- .../salesforce/rules/fls/apex/operations/FlsViolationInfo.java | 2 -- .../salesforce/rules/MultipleRuleViolationsScenarioTest.java | 2 -- 4 files changed, 2 insertions(+), 7 deletions(-) diff --git a/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java b/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java index 2406d2ccc..37c1d3eed 100644 --- a/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java +++ b/sfge/src/main/java/com/salesforce/config/UserFacingMessages.java @@ -79,8 +79,7 @@ public static final class CompilationErrors { } public static final class MultipleMassSchemaLookupRuleTemplates { - public static final String MESSAGE_TEMPLATE = - "%s was %s at %s:%d."; + public static final String MESSAGE_TEMPLATE = "%s was %s at %s:%d."; public static final String OCCURRENCE_LOOP_TEMPLATE = "called inside a %s"; public static final String OCCURRENCE_MULTIPLE_TEMPLATE = "preceded by a call to %s"; } diff --git a/sfge/src/main/java/com/salesforce/rules/MultipleMassSchemaLookupRule.java b/sfge/src/main/java/com/salesforce/rules/MultipleMassSchemaLookupRule.java index 9f0a6ddcd..70af5bc52 100644 --- a/sfge/src/main/java/com/salesforce/rules/MultipleMassSchemaLookupRule.java +++ b/sfge/src/main/java/com/salesforce/rules/MultipleMassSchemaLookupRule.java @@ -48,7 +48,7 @@ protected String getCategory() { @Override protected boolean isEnabled() { - return false; + return true; } public static MultipleMassSchemaLookupRule getInstance() { diff --git a/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationInfo.java b/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationInfo.java index 00aa719f5..07d8aadae 100644 --- a/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationInfo.java +++ b/sfge/src/main/java/com/salesforce/rules/fls/apex/operations/FlsViolationInfo.java @@ -3,11 +3,9 @@ import com.google.common.base.Objects; import com.salesforce.collections.CollectionUtil; import com.salesforce.config.UserFacingMessages; -import com.salesforce.exception.ProgrammingException; import com.salesforce.graph.vertex.SFVertex; import com.salesforce.rules.AbstractRule; import com.salesforce.rules.RuleThrowable; -import com.salesforce.rules.Violation; import java.util.Optional; import java.util.TreeSet; diff --git a/sfge/src/test/java/com/salesforce/rules/MultipleRuleViolationsScenarioTest.java b/sfge/src/test/java/com/salesforce/rules/MultipleRuleViolationsScenarioTest.java index 4e84e54fc..6422ba50c 100644 --- a/sfge/src/test/java/com/salesforce/rules/MultipleRuleViolationsScenarioTest.java +++ b/sfge/src/test/java/com/salesforce/rules/MultipleRuleViolationsScenarioTest.java @@ -1,11 +1,9 @@ package com.salesforce.rules; import com.salesforce.TestUtil; -import com.salesforce.collections.CollectionUtil; import com.salesforce.testutils.BasePathBasedRuleTest; import java.util.List; import java.util.TreeSet; - import org.hamcrest.MatcherAssert; import org.hamcrest.Matchers; import org.junit.jupiter.api.Test; From 3b1b7384e8ea6914ae626a09fce15e0dbdd6cfdc Mon Sep 17 00:00:00 2001 From: Roopa Mohan Date: Thu, 4 May 2023 11:44:21 -0700 Subject: [PATCH 09/10] Minor cleanup --- .../MultipleMassSchemaLookupInfo.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupInfo.java b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupInfo.java index fc5238f08..05cc6b729 100644 --- a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupInfo.java +++ b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupInfo.java @@ -6,6 +6,10 @@ import com.salesforce.rules.RuleThrowable; import com.salesforce.rules.Violation; +/** + * Represents information required to create a violation from + * {@link com.salesforce.rules.MultipleMassSchemaLookupRule}. + */ public class MultipleMassSchemaLookupInfo implements RuleThrowable { /** @@ -54,7 +58,7 @@ private void validateInput( } } - // @Override + public Violation.PathBasedRuleViolation convert() { return new Violation.PathBasedRuleViolation( MassSchemaLookupInfoUtil.getMessage(this), sourceVertex, sinkVertex); From 852445086cdbcbcf816d9253db240a7b838c905e Mon Sep 17 00:00:00 2001 From: Roopa Mohan Date: Thu, 4 May 2023 14:40:51 -0700 Subject: [PATCH 10/10] Feedback items. Loop exit detection not included --- .../rules/MultipleMassSchemaLookupRule.java | 2 +- .../MultipleMassSchemaLookupInfo.java | 5 +- .../MultipleMassSchemaLookupRuleHandler.java | 2 +- .../MultipleMassSchemaLookupVisitor.java | 2 +- .../MultipleMassSchemaLookupRuleTest.java | 95 ++++++++++++++++--- 5 files changed, 89 insertions(+), 17 deletions(-) diff --git a/sfge/src/main/java/com/salesforce/rules/MultipleMassSchemaLookupRule.java b/sfge/src/main/java/com/salesforce/rules/MultipleMassSchemaLookupRule.java index 70af5bc52..9f0a6ddcd 100644 --- a/sfge/src/main/java/com/salesforce/rules/MultipleMassSchemaLookupRule.java +++ b/sfge/src/main/java/com/salesforce/rules/MultipleMassSchemaLookupRule.java @@ -48,7 +48,7 @@ protected String getCategory() { @Override protected boolean isEnabled() { - return true; + return false; } public static MultipleMassSchemaLookupRule getInstance() { diff --git a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupInfo.java b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupInfo.java index 05cc6b729..f02ed4357 100644 --- a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupInfo.java +++ b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupInfo.java @@ -7,8 +7,8 @@ import com.salesforce.rules.Violation; /** - * Represents information required to create a violation from - * {@link com.salesforce.rules.MultipleMassSchemaLookupRule}. + * Represents information required to create a violation from {@link + * com.salesforce.rules.MultipleMassSchemaLookupRule}. */ public class MultipleMassSchemaLookupInfo implements RuleThrowable { @@ -58,7 +58,6 @@ private void validateInput( } } - public Violation.PathBasedRuleViolation convert() { return new Violation.PathBasedRuleViolation( MassSchemaLookupInfoUtil.getMessage(this), sourceVertex, sinkVertex); diff --git a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleHandler.java b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleHandler.java index 4d8606038..d0eb306b9 100644 --- a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleHandler.java +++ b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleHandler.java @@ -43,7 +43,7 @@ public Set detectViolations( ApexPathWalker.walkPath(g, path, ruleVisitor, symbols); // Once it finishes walking, collect any violations thrown - return ruleVisitor.getViolation(); + return ruleVisitor.getViolations(); } public static MultipleMassSchemaLookupRuleHandler getInstance() { diff --git a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupVisitor.java b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupVisitor.java index e164d7207..fcc190999 100644 --- a/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupVisitor.java +++ b/sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupVisitor.java @@ -68,7 +68,7 @@ private boolean shouldContinue() { /** * @return Violations collected by the rule. */ - Set getViolation() { + Set getViolations() { return violations; } } diff --git a/sfge/src/test/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleTest.java b/sfge/src/test/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleTest.java index fb235d42d..718b65956 100644 --- a/sfge/src/test/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleTest.java +++ b/sfge/src/test/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupRuleTest.java @@ -44,6 +44,30 @@ public void testSimpleGgdInLoop(String loopAstLabel, String loopStructure) { loopAstLabel)); } + @CsvSource({ + "ForEachStatement, for (String s : myList)", + "ForLoopStatement, for (Integer i; i < s.size; s++)", + "WhileLoopStatement, while(true)" + }) + @ParameterizedTest(name = "{displayName}: {0}") + @Disabled // TODO: Only surrounding loop should be counted as a violation. + public void testSimpleGgdOutsideLoop(String loopAstLabel, String loopStructure) { + // spotless:off + String sourceCode = + "public class MyClass {\n" + + " void foo() {\n" + + " List myList = new String[] {'Account','Contact'};\n" + + " " + loopStructure + " {\n" + + " String s = 'abc';\n" + + " }\n" + + " Schema.getGlobalDescribe();\n" + + " }\n" + + "}\n"; + // spotless:on + + assertNoViolation(RULE, sourceCode); + } + @CsvSource({ "ForEachStatement, for (String s : myList)", "ForLoopStatement, for (Integer i; i < s.size; s++)", @@ -126,17 +150,15 @@ public void testLoopWithinLoop( }) @ParameterizedTest(name = "{displayName}: {0}") public void testMultipleInvocations(String testName, String firstCall, String secondCall) { + // spotless:off String sourceCode = "public class MyClass {\n" + " void foo() {\n" - + " " - + firstCall - + "();\n" - + " " - + secondCall - + "();\n" + + " " + firstCall + "();\n" + + " " + secondCall + "();\n" + " }\n" + "}\n"; + // spotless:on assertViolations( RULE, @@ -152,6 +174,7 @@ public void testMultipleInvocations(String testName, String firstCall, String se @Test public void testForEachLoopInClassInstance() { + // spotless:off String sourceCode[] = { "public class MyClass {\n" + " void foo() {\n" @@ -167,6 +190,7 @@ public void testForEachLoopInClassInstance() { + "}\n" + "}\n" }; + // spotless:on assertViolations( RULE, @@ -184,6 +208,7 @@ public void testForEachLoopInClassInstance() { @Test @Disabled public void testForLoopInClassInstance() { + // spotless:off String sourceCode[] = { "public class MyClass {\n" + " void foo() {\n" @@ -199,6 +224,7 @@ public void testForLoopInClassInstance() { + "}\n" + "}\n" }; + // spotless:on assertViolations( RULE, @@ -214,6 +240,7 @@ public void testForLoopInClassInstance() { @Test public void testForLoopConditionalOnClassInstance() { + // spotless:off String sourceCode[] = { "public class MyClass {\n" + " void foo() {\n" @@ -235,6 +262,7 @@ public void testForLoopConditionalOnClassInstance() { + "}\n" + "}\n" }; + // spotless:on assertViolations( RULE, @@ -250,6 +278,7 @@ public void testForLoopConditionalOnClassInstance() { @Test // TODO: Check if this is a false positive. Static block should get invoked only once public void testLoopFromStaticBlock() { + // spotless:off String[] sourceCode = { "public class MyClass {\n" + " void foo(String[] objectNames) {\n" @@ -267,6 +296,7 @@ public void testLoopFromStaticBlock() { + " }\n" + "}\n" }; + // spotless:on assertViolations( RULE, @@ -280,18 +310,61 @@ public void testLoopFromStaticBlock() { ASTConstants.NodeType.FOR_LOOP_STATEMENT)); } - @Test - public void testLoopAfterGgdShouldNotGetViolation() { + @CsvSource({ + "ForEachStatement, for (String s : myList)", + "ForLoopStatement, for (Integer i; i < s.size; s++)", + "WhileLoopStatement, while(true)" + }) + @ParameterizedTest(name = "{displayName}: {0}") + public void testLoopAfterGgdShouldNotGetViolation(String loopAstLabel, String loopStructure) { + // spotless:off String sourceCode = "public class MyClass {\n" - + " void foo(String[] objectNames) {\n" + + " void foo() {\n" + " Schema.getGlobalDescribe();\n" - + " for (String name: objectNames) {\n" - + " System.debug(name);\n" + + " " + loopStructure + " {\n" + + " List myList = new String[] {'Account','Contact'};\n" + + " System.debug('hi');\n" + " }\n" + " }\n" + "}\n"; + // spotless:on assertNoViolation(RULE, sourceCode); } + + @CsvSource({ + "ForEachStatement, for (String s : myList)", + "ForLoopStatement, for (Integer i; i < s.size; s++)", + "WhileLoopStatement, while(true)" + }) + @ParameterizedTest(name = "{displayName}: {0}") + @Disabled // TODO: Only surrounding loop should be counted as a violation. + public void testLoopBeforeAndAroundGgd(String loopAstLabel, String loopStructure) { + // spotless:off + String sourceCode = + "public class MyClass {\n" + + " void foo() {\n" + + " List myList = new String[] {'Account','Contact'};\n" + + " " + loopStructure + " {\n" + + " System.debug('hi');\n" + + " }\n" + + " " + loopStructure + " {\n" + + " Schema.getGlobalDescribe();\n" + + " }\n" + + " }\n" + + "}\n"; + // spotless:on + + assertViolations( + RULE, + sourceCode, + expect( + 8, + RuleConstants.METHOD_SCHEMA_GET_GLOBAL_DESCRIBE, + 7, + "MyClass", + RuleConstants.RepetitionType.LOOP, + loopAstLabel)); + } }