Skip to content

Commit d5f3573

Browse files
rmohan20rmohan20
authored
Merge pull request #1084 from forcedotcom/rm/MMSRuleMethodIndirection
CHANGE (GraphEngine): @W-13363157@: Handles multiple levels of method call from loop definition
2 parents d2eaf85 + b0c5ee8 commit d5f3573

File tree

6 files changed

+232
-15
lines changed

6 files changed

+232
-15
lines changed

‎sfge/src/main/java/com/salesforce/rules/multiplemassschemalookup/MultipleMassSchemaLookupVisitor.java‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import com.salesforce.graph.symbols.SymbolProvider;
44
import com.salesforce.graph.vertex.*;
5-
import com.salesforce.rules.ops.boundary.LoopBoundary;
65
import com.salesforce.rules.ops.visitor.LoopDetectionVisitor;
76
import java.util.HashSet;
87
import java.util.Optional;
@@ -46,16 +45,16 @@ public void afterVisit(MethodCallExpressionVertex vertex, SymbolProvider symbols
4645
} else if (RuleConstants.isSchemaExpensiveMethod(vertex) && shouldContinue()) {
4746
createViolation(RuleConstants.RepetitionType.MULTIPLE, vertex);
4847
}
48+
49+
// Perform super method's logic as well to remove exclusion boundary if needed.
50+
super.afterVisit(vertex, symbols);
4951
}
5052

5153
private void checkIfInsideLoop(MethodCallExpressionVertex vertex, SymbolProvider symbols) {
52-
final Optional<LoopBoundary> loopBoundaryOptional = loopBoundaryDetector.peek();
53-
if (loopBoundaryOptional.isPresent()) {
54-
if (!isLoopOutlier(vertex, symbols)) {
55-
// Method has been invoked inside a loop. Create a violation.
56-
final SFVertex loopVertex = loopBoundaryOptional.get().getBoundaryItem();
57-
createViolation(RuleConstants.RepetitionType.LOOP, loopVertex);
58-
}
54+
final Optional<? extends SFVertex> loopedVertexOpt = isInsideLoop();
55+
if (loopedVertexOpt.isPresent()) {
56+
// Method has been invoked inside a loop. Create a violation.
57+
createViolation(RuleConstants.RepetitionType.LOOP, loopedVertexOpt.get());
5958
}
6059
}
6160

‎sfge/src/main/java/com/salesforce/rules/ops/boundary/BoundaryDetector.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,9 @@ public Optional<T> peek() {
4444
* @param boundary item that governs this extent.
4545
*/
4646
public void pushBoundary(T boundary) {
47-
LOGGER.debug("Entering loop boundary with boundaryItem=" + boundary.getBoundaryItem());
47+
if (LOGGER.isDebugEnabled()) {
48+
LOGGER.debug("Entering loop boundary with boundaryItem=" + boundary.getBoundaryItem());
49+
}
4850
push(boundary);
4951
}
5052

@@ -54,7 +56,9 @@ public void pushBoundary(T boundary) {
5456
* @param boundaryItem that is expected to govern the current boundary that's to be ended.
5557
*/
5658
public void popBoundary(R boundaryItem) {
57-
LOGGER.debug("Exiting boundary with boundaryItem=" + boundaryItem);
59+
if (LOGGER.isDebugEnabled()) {
60+
LOGGER.debug("Exiting boundary with boundaryItem=" + boundaryItem);
61+
}
5862

5963
// Check if a boundary is actually in place
6064
Optional<T> boundaryOptional = peek();

‎sfge/src/main/java/com/salesforce/rules/ops/boundary/LoopBoundaryDetector.java‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,24 @@
11
package com.salesforce.rules.ops.boundary;
22

33
import com.salesforce.graph.vertex.SFVertex;
4+
import java.util.Optional;
45

56
/** Implementation of boundary detection for loops. */
67
public class LoopBoundaryDetector extends BoundaryDetector<LoopBoundary, SFVertex> {
7-
// Intentionally blank
8+
public Optional<? extends SFVertex> isInsideLoop() {
9+
final Optional<LoopBoundary> loopBoundaryOpt = peek();
10+
if (loopBoundaryOpt.isPresent()) {
11+
// Check if we are inside a loop exclusion.
12+
if (loopBoundaryOpt.get() instanceof LoopExclusionBoundary) {
13+
// This is exclusion zone. We consider this not a loop.
14+
return Optional.empty();
15+
}
16+
17+
// Inside a loop.
18+
return Optional.of(loopBoundaryOpt.get().getBoundaryItem());
19+
}
20+
21+
// Not inside a loop
22+
return Optional.empty();
23+
}
824
}

‎sfge/src/main/java/com/salesforce/rules/ops/boundary/LoopExclusionBoundary.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
package com.salesforce.rules.ops.boundary;
2+
3+
import com.salesforce.graph.vertex.SFVertex;
4+
5+
/**
6+
* Indicates parts of the loop that get run only once. For example:
7+
*
8+
* <ul>
9+
* <li>getValues() method in: <code>for (String s: getValues())</code>
10+
* <li>Static initialization of a class
11+
* </ul>
12+
*/
13+
public class LoopExclusionBoundary extends LoopBoundary {
14+
public LoopExclusionBoundary(SFVertex loopVertex) {
15+
super(loopVertex);
16+
}
17+
}

‎sfge/src/main/java/com/salesforce/rules/ops/visitor/LoopDetectionVisitor.java‎

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,12 @@
55
import com.salesforce.graph.visitor.DefaultNoOpPathVertexVisitor;
66
import com.salesforce.rules.ops.boundary.LoopBoundary;
77
import com.salesforce.rules.ops.boundary.LoopBoundaryDetector;
8+
import com.salesforce.rules.ops.boundary.LoopExclusionBoundary;
9+
import java.util.Optional;
810

911
/** Visitor that gets notified when a loop vertex is invoked in the path. */
1012
public abstract class LoopDetectionVisitor extends DefaultNoOpPathVertexVisitor {
11-
protected final LoopBoundaryDetector loopBoundaryDetector;
13+
private final LoopBoundaryDetector loopBoundaryDetector;
1214

1315
public LoopDetectionVisitor() {
1416
loopBoundaryDetector = new LoopBoundaryDetector();
@@ -57,4 +59,58 @@ public boolean visit(WhileLoopStatementVertex vertex, SymbolProvider symbols) {
5759
public void afterVisit(WhileLoopStatementVertex vertex, SymbolProvider symbols) {
5860
loopBoundaryDetector.popBoundary(vertex);
5961
}
62+
63+
/**
64+
* This case is specific to method calls on ForEach loop definition. These methods are called
65+
* only once even though they are technically under a loop definition. We create this boundary
66+
* to show that calls here are not actually called multiple times.
67+
*
68+
* <p>For example, <code>getValues()</code> in this forEach gets called only once: <code>
69+
* for (String s: getValues())</code>
70+
*
71+
* @param vertex Method call in question
72+
* @param symbols SymbolProvider at this state
73+
* @return true to visit the children
74+
*/
75+
@Override
76+
public boolean visit(MethodCallExpressionVertex vertex, SymbolProvider symbols) {
77+
// If already within a loop's boundary, get the loop item
78+
final Optional<LoopBoundary> currentLoopBoundaryOpt = loopBoundaryDetector.peek();
79+
if (currentLoopBoundaryOpt.isPresent()) {
80+
final SFVertex loopBoundaryItem = currentLoopBoundaryOpt.get().getBoundaryItem();
81+
if (loopBoundaryItem instanceof ForEachStatementVertex) {
82+
// We are within a ForEach statement.
83+
// Check if the method calls parent is the same as this ForEach statement.
84+
// If they are the same, this method would get invoked only once.
85+
BaseSFVertex parentVertex = vertex.getParent();
86+
if (parentVertex instanceof ForEachStatementVertex
87+
&& parentVertex.equals(loopBoundaryItem)) {
88+
// This method would get invoked only once.
89+
loopBoundaryDetector.pushBoundary(new LoopExclusionBoundary(vertex));
90+
}
91+
}
92+
}
93+
return true;
94+
}
95+
96+
@Override
97+
public void afterVisit(MethodCallExpressionVertex vertex, SymbolProvider symbols) {
98+
// If within a method call loop exclusion, pop boundary here.
99+
final Optional<LoopBoundary> currentLoopBoundaryOpt = loopBoundaryDetector.peek();
100+
if (currentLoopBoundaryOpt.isPresent()) {
101+
final LoopBoundary loopBoundary = currentLoopBoundaryOpt.get();
102+
if (loopBoundary instanceof LoopExclusionBoundary) {
103+
// We are in exclusion zone. Check if the method call on the loop exclusion boundary
104+
// is the same as the current method call.
105+
if (vertex.equals(loopBoundary.getBoundaryItem())) {
106+
// Pop the method call
107+
loopBoundaryDetector.popBoundary(vertex);
108+
}
109+
}
110+
}
111+
}
112+
113+
protected Optional<? extends SFVertex> isInsideLoop() {
114+
return loopBoundaryDetector.isInsideLoop();
115+
}
60116
}

‎sfge/src/test/java/com/salesforce/rules/multiplemassschemalookup/SchemaLookupInAnotherMethodTest.java‎

Lines changed: 128 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,47 @@
22

33
import org.junit.jupiter.api.Disabled;
44
import org.junit.jupiter.api.Test;
5+
import org.junit.jupiter.params.ParameterizedTest;
6+
import org.junit.jupiter.params.provider.CsvSource;
57

68
public class SchemaLookupInAnotherMethodTest extends BaseAvoidMultipleMassSchemaLookupTest {
9+
10+
@CsvSource({
11+
"ForEachStatement, for (String s : myList)",
12+
"ForLoopStatement, for (Integer i; i < s.size; s++)",
13+
"WhileLoopStatement, while(true)"
14+
})
15+
@ParameterizedTest(name = "{displayName}: {0}")
16+
public void testExternalInvocationFromLoop(String loopAstLabel, String loopStructure) {
17+
// spotless:off
18+
String sourceCode =
19+
"public class MyClass {\n" +
20+
" void foo() {\n" +
21+
" List<String> myList = new String[] {'Account','Contact'};\n" +
22+
" " + loopStructure + " {\n" +
23+
" getObjectDescribes(myList);\n" +
24+
" }\n" +
25+
" }\n"
26+
+ " List<Schema.DescribeSObjectResult> getObjectDescribes(String[] objectList) {\n"
27+
+ " return Schema.describeSObjects(objectList);\n"
28+
+ " }\n"
29+
+ "}\n";
30+
// spotless:on
31+
32+
assertViolations(
33+
RULE,
34+
sourceCode,
35+
expect(
36+
9,
37+
RuleConstants.METHOD_SCHEMA_DESCRIBE_SOBJECTS,
38+
4,
39+
"MyClass",
40+
RuleConstants.RepetitionType.LOOP,
41+
loopAstLabel));
42+
}
43+
744
@Test
8-
@Disabled // TODO: Address Schema calls that happen through indirection
9-
public void testMethodCallWithinForEachLoopIsSafe() {
45+
public void testMethodCallWithinForEachLoopIsSafe_Level1() {
1046
// spotless:off
1147
String sourceCode[] = {
1248
"public class MyClass {\n"
@@ -27,7 +63,31 @@ public void testMethodCallWithinForEachLoopIsSafe() {
2763
}
2864

2965
@Test
30-
@Disabled // TODO: Address Schema calls that happen through indirection
66+
public void testMethodCallWithinForEachLoopIsSafe_Level2() {
67+
// spotless:off
68+
String sourceCode[] = {
69+
"public class MyClass {\n"
70+
+ " void foo() {\n"
71+
+ " String[] objectList = new String[] {'Account','Contact'};\n"
72+
+ " for (Schema.DescribeSObjectResult objDesc: getObjectDescribes(objectList)) {\n"
73+
+ " System.debug(objDesc.getLabel());\n"
74+
+ " }\n"
75+
+ " }\n"
76+
+ " List<Schema.DescribeSObjectResult> getObjectDescribes(String[] objectList) {\n"
77+
+ " return getObjectDescribesLevel2(objectList);\n"
78+
+ " }\n"
79+
+ " List<Schema.DescribeSObjectResult> getObjectDescribesLevel2(String[] objectList) {\n"
80+
+ " return Schema.describeSObjects(objectList);\n"
81+
+ " }\n"
82+
+ "}\n"
83+
};
84+
// spotless:on
85+
86+
assertNoViolation(RULE, sourceCode);
87+
}
88+
89+
@Test
90+
@Disabled // TODO: Handle tracking multiple calls to the same method
3191
public void testMultipleCallsToExternalSchemaMethod() {
3292
// spotless:off
3393
String sourceCode[] = {
@@ -55,4 +115,69 @@ public void testMultipleCallsToExternalSchemaMethod() {
55115
RuleConstants.RepetitionType.MULTIPLE,
56116
"Schema.describeSObjects"));
57117
}
118+
119+
@Test
120+
@Disabled // TODO: Handle tracking multiple calls to the same method
121+
public void testMultipleCallsToExternalSchemaMethod_differentPaths() {
122+
// spotless:off
123+
String sourceCode[] = {
124+
"public class MyClass {\n"
125+
+ " void foo() {\n"
126+
+ " String[] objectList = new String[] {'Account','Contact'};\n" +
127+
" getObjectDescribes1(objectList);\n" +
128+
" getObjectDescribes2(objectList);\n"
129+
+ " }\n"
130+
+ " List<Schema.DescribeSObjectResult> getObjectDescribes1(String[] objectList) {\n"
131+
+ " return getObjectDescribes2(objectList);\n"
132+
+ " }\n"
133+
+ " List<Schema.DescribeSObjectResult> getObjectDescribes2(String[] objectList) {\n"
134+
+ " return Schema.describeSObjects(objectList);\n"
135+
+ " }\n"
136+
+ "}\n"
137+
};
138+
// spotless:on
139+
140+
assertViolations(
141+
RULE,
142+
sourceCode,
143+
expect(
144+
5,
145+
"getObjectDescribes",
146+
1,
147+
"MyClass",
148+
RuleConstants.RepetitionType.MULTIPLE,
149+
"Schema.describeSObjects"));
150+
}
151+
152+
@Test
153+
public void testMultipleCallsToExternalSchemaMethod_differentMethods() {
154+
// spotless:off
155+
String sourceCode[] = {
156+
"public class MyClass {\n"
157+
+ " void foo() {\n"
158+
+ " String[] objectList = new String[] {'Account','Contact'};\n" +
159+
" getObjectDescribes1(objectList);\n" +
160+
" getObjectDescribes2(objectList);\n"
161+
+ " }\n"
162+
+ " List<Schema.DescribeSObjectResult> getObjectDescribes1(String[] objectList) {\n"
163+
+ " return Schema.describeSObjects(objectList);\n"
164+
+ " }\n"
165+
+ " List<Schema.DescribeSObjectResult> getObjectDescribes2(String[] objectList) {\n"
166+
+ " return Schema.describeSObjects(objectList);\n"
167+
+ " }\n"
168+
+ "}\n"
169+
};
170+
// spotless:on
171+
172+
assertViolations(
173+
RULE,
174+
sourceCode,
175+
expect(
176+
11,
177+
RuleConstants.METHOD_SCHEMA_DESCRIBE_SOBJECTS,
178+
8,
179+
"MyClass",
180+
RuleConstants.RepetitionType.MULTIPLE,
181+
"Schema.describeSObjects"));
182+
}
58183
}

0 commit comments

Comments
 (0)