From 68bb2c7105a06932f4d80169829768891e49c460 Mon Sep 17 00:00:00 2001 From: Joshua Feingold Date: Mon, 27 Jun 2022 14:27:45 -0500 Subject: [PATCH 1/3] @W-10459675@: Part 3 of several. Added global methods as sources. --- .../com/salesforce/graph/ops/MethodUtil.java | 19 +++++- .../java/com/salesforce/rules/RuleUtil.java | 2 + .../salesforce/graph/ops/MethodUtilTest.java | 62 +++++++++++++++++++ 3 files changed, 81 insertions(+), 2 deletions(-) diff --git a/sfge/src/main/java/com/salesforce/graph/ops/MethodUtil.java b/sfge/src/main/java/com/salesforce/graph/ops/MethodUtil.java index 37a144e2f..145973e0f 100644 --- a/sfge/src/main/java/com/salesforce/graph/ops/MethodUtil.java +++ b/sfge/src/main/java/com/salesforce/graph/ops/MethodUtil.java @@ -178,8 +178,8 @@ public static List getNamespaceAccessibleMethods( } /** - * Returns non-test methods in the target files with a @RemoteAction annotation. An empty - * list implicitly includes all files. + * Returns non-test methods in the target files with a @RemoteAction annotation. An empty list + * implicitly includes all files. */ public static List getRemoteActionMethods( GraphTraversalSource g, List targetFiles) { @@ -229,6 +229,21 @@ private static GraphTraversal rootMethodTraversal( .not(has(Schema.IS_TEST, true)); } + /** + * Returns non-test methods in the target files whose modifier scope is `global`. An empty list implicitly includes + * all files. + */ + public static List getGlobalMethods( + GraphTraversalSource g, List targetFiles) { + // Get all methods in the target files. + List methodVertices = + SFVertexFactory.loadVertices(g, rootMethodTraversal(g, targetFiles)); + // Filter for the global methods. + return methodVertices.stream() + .filter(m -> m.getModifierNode().isGlobal() && !m.isStandardType()) + .collect(Collectors.toList()); + } + /** * Returns all non-test public- and global-scoped methods in controllers referenced by * VisualForce pages, filtered by target file list. An empty list implicitly includes all files. diff --git a/sfge/src/main/java/com/salesforce/rules/RuleUtil.java b/sfge/src/main/java/com/salesforce/rules/RuleUtil.java index b0b7c6ec0..5e088f338 100644 --- a/sfge/src/main/java/com/salesforce/rules/RuleUtil.java +++ b/sfge/src/main/java/com/salesforce/rules/RuleUtil.java @@ -65,6 +65,8 @@ public static List getPathEntryPoints( methods.addAll(MethodUtil.getRemoteActionMethods(g, fileLevelTargets)); // ...and PageReference methods... methods.addAll(MethodUtil.getPageReferenceMethods(g, fileLevelTargets)); + // ...and global-exposed methods... + methods.addAll(MethodUtil.getGlobalMethods(g, fileLevelTargets)); // ...and exposed methods on VF controllers. methods.addAll(MethodUtil.getExposedControllerMethods(g, fileLevelTargets)); } diff --git a/sfge/src/test/java/com/salesforce/graph/ops/MethodUtilTest.java b/sfge/src/test/java/com/salesforce/graph/ops/MethodUtilTest.java index 051ed13a0..dfc6a9837 100644 --- a/sfge/src/test/java/com/salesforce/graph/ops/MethodUtilTest.java +++ b/sfge/src/test/java/com/salesforce/graph/ops/MethodUtilTest.java @@ -389,4 +389,66 @@ public void testGetMethodsWithAnnotation(String annotation) { MatcherAssert.assertThat(excludedName, excludedMethod.isTest(), equalTo(true)); } } + + @Test + public void testGetGlobalMethods() { + String[] sourceCode = { + "public class MyClass {\n" + + " global static void foo() {\n" + + " }\n" + + " global static testMethod void shouldBeExcludedByModifier() {\n" + + " }\n" + + " @isTest\n" + + " global static void shouldBeExcludedByAnnotation() {\n" + + " }\n" + + " public static void bar() {\n" + + " }\n" + + "}\n", + "@isTest\n" + + "public class MyTestClass {\n" + + " public static void foo() {\n" + + " }\n" + + "}\n", + }; + + TestUtil.buildGraph(g, sourceCode); + + List methods = MethodUtil.getGlobalMethods(g, new ArrayList<>()); + // The `foo` method should be included because it's declared as global. The `` method + // (i.e. the constructor) and the `clone` method are included because they implicitly exist even though they were + // never actually declared. The inclusion of unreal methods isn't a problem, because they're just empty paths. + MatcherAssert.assertThat(methods, hasSize(equalTo(3))); + + boolean cloneFound = false; + boolean fooFound = false; + boolean initFound = false; + for (MethodVertex methodVertex : methods) { + switch (methodVertex.getName()) { + case "clone": + cloneFound = true; + break; + case "": + initFound = true; + break; + case "foo": + fooFound = true; + break; + default: + fail("Unexpected method name: " + methodVertex.getName()); + } + } + assertTrue(cloneFound); + assertTrue(initFound); + assertTrue(fooFound); + + for (String excludedName : + new String[] {"shouldBeExcludedByModifier", "shouldBeExcludedByAnnotation"}) { + MethodVertex excludedMethod = + SFVertexFactory.load( + g, + g.V().hasLabel(ASTConstants.NodeType.METHOD) + .has(Schema.NAME, excludedName)); + MatcherAssert.assertThat(excludedName, excludedMethod.isTest(), equalTo(true)); + } + } } From e4f834c3f3ebff2f87b0cc7635025d7dcb49e7e5 Mon Sep 17 00:00:00 2001 From: Joshua Feingold Date: Mon, 27 Jun 2022 17:22:00 -0500 Subject: [PATCH 2/3] @W-10459675@: Fixed issue in which implicitly declared global methods were being returned. --- .../com/salesforce/graph/ops/MethodUtil.java | 29 ++++++++++++++----- .../salesforce/graph/ops/MethodUtilTest.java | 29 ++----------------- 2 files changed, 24 insertions(+), 34 deletions(-) diff --git a/sfge/src/main/java/com/salesforce/graph/ops/MethodUtil.java b/sfge/src/main/java/com/salesforce/graph/ops/MethodUtil.java index 145973e0f..9632bb1d6 100644 --- a/sfge/src/main/java/com/salesforce/graph/ops/MethodUtil.java +++ b/sfge/src/main/java/com/salesforce/graph/ops/MethodUtil.java @@ -230,18 +230,31 @@ private static GraphTraversal rootMethodTraversal( } /** - * Returns non-test methods in the target files whose modifier scope is `global`. An empty list implicitly includes - * all files. + * Returns non-test methods in the target files whose modifier scope is `global`. An empty list + * implicitly includes all files. */ public static List getGlobalMethods( GraphTraversalSource g, List targetFiles) { // Get all methods in the target files. - List methodVertices = - SFVertexFactory.loadVertices(g, rootMethodTraversal(g, targetFiles)); - // Filter for the global methods. - return methodVertices.stream() - .filter(m -> m.getModifierNode().isGlobal() && !m.isStandardType()) - .collect(Collectors.toList()); + return SFVertexFactory.loadVertices( + g, + rootMethodTraversal(g, targetFiles) + .filter( + __.and( + // If a method has at least one block statement, then it is + // definitely actually declared, as + // opposed to being an implicit method. + out(Schema.CHILD) + .hasLabel(NodeType.BLOCK_STATEMENT) + .count() + .is(P.gte(1)), + // We only want global methods. + out(Schema.CHILD) + .hasLabel(NodeType.MODIFIER_NODE) + .has(Schema.GLOBAL, true), + // Ignore any standard methods, otherwise will get a ton of + // extra results. + __.not(__.has(Schema.IS_STANDARD, true))))); } /** diff --git a/sfge/src/test/java/com/salesforce/graph/ops/MethodUtilTest.java b/sfge/src/test/java/com/salesforce/graph/ops/MethodUtilTest.java index dfc6a9837..52336ec4f 100644 --- a/sfge/src/test/java/com/salesforce/graph/ops/MethodUtilTest.java +++ b/sfge/src/test/java/com/salesforce/graph/ops/MethodUtilTest.java @@ -414,32 +414,9 @@ public void testGetGlobalMethods() { TestUtil.buildGraph(g, sourceCode); List methods = MethodUtil.getGlobalMethods(g, new ArrayList<>()); - // The `foo` method should be included because it's declared as global. The `` method - // (i.e. the constructor) and the `clone` method are included because they implicitly exist even though they were - // never actually declared. The inclusion of unreal methods isn't a problem, because they're just empty paths. - MatcherAssert.assertThat(methods, hasSize(equalTo(3))); - - boolean cloneFound = false; - boolean fooFound = false; - boolean initFound = false; - for (MethodVertex methodVertex : methods) { - switch (methodVertex.getName()) { - case "clone": - cloneFound = true; - break; - case "": - initFound = true; - break; - case "foo": - fooFound = true; - break; - default: - fail("Unexpected method name: " + methodVertex.getName()); - } - } - assertTrue(cloneFound); - assertTrue(initFound); - assertTrue(fooFound); + // The `foo` method should be included because it's declared as global. + MatcherAssert.assertThat(methods, hasSize(equalTo(1))); + MatcherAssert.assertThat(methods.get(0).getName(), equalTo("foo")); for (String excludedName : new String[] {"shouldBeExcludedByModifier", "shouldBeExcludedByAnnotation"}) { From 10fc89e96da0bbc754a17ae17d85ef8ec60aaac5 Mon Sep 17 00:00:00 2001 From: Joshua Feingold Date: Tue, 28 Jun 2022 13:36:13 -0500 Subject: [PATCH 3/3] @W-10459675@: Added additional tests as per code review. --- .../com/salesforce/rules/RuleUtilTest.java | 37 ++++++++++++++++++- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/sfge/src/test/java/com/salesforce/rules/RuleUtilTest.java b/sfge/src/test/java/com/salesforce/rules/RuleUtilTest.java index 0479775a4..e55890da4 100644 --- a/sfge/src/test/java/com/salesforce/rules/RuleUtilTest.java +++ b/sfge/src/test/java/com/salesforce/rules/RuleUtilTest.java @@ -5,8 +5,7 @@ import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.core.IsNot.not; import static org.hamcrest.core.IsNull.nullValue; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.fail; +import static org.junit.jupiter.api.Assertions.*; import com.salesforce.TestUtil; import com.salesforce.graph.Schema; @@ -63,6 +62,40 @@ public void getPathEntryPoints_includesAnnotatedMethods(String annotation) { assertEquals("annotatedMethod", firstVertex.getName()); } + @Test + public void getPathEntryPoints_includesGlobalMethods() { + String sourceCode = + "public class Foo {\n" + + " global static void globalStaticMethod() {\n" + + " }\n" + + " global void globalInstanceMethod() {\n" + + " }\n" + + " public static void publicStaticMethod() {\n" + + " }\n" + + "}\n"; + TestUtil.buildGraph(g, sourceCode, true); + + List entryPoints = RuleUtil.getPathEntryPoints(g); + + MatcherAssert.assertThat(entryPoints, hasSize(equalTo(2))); + boolean staticMethodFound = false; + boolean instanceMethodFound = false; + for (MethodVertex entrypoint : entryPoints) { + switch (entrypoint.getName()) { + case "globalStaticMethod": + staticMethodFound = true; + break; + case "globalInstanceMethod": + instanceMethodFound = true; + break; + default: + fail("Unexpected method " + entrypoint.getName()); + } + } + assertTrue(staticMethodFound); + assertTrue(instanceMethodFound); + } + @Test public void getPathEntryPoints_includesPageReferenceMethods() { String sourceCode =