diff --git a/.circleci/config.yml b/.circleci/config.yml index 1481cbf0f..63e4b3ec9 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -80,6 +80,9 @@ commands: - gradle/collect_test_results: reports_path: pmd-cataloger/build/reports/ test_results_path: pmd-cataloger/build/test-results/ + - gradle/collect_test_results: + reports_path: sfge/build/reports/ + test_results_path: sfge/build/test-results/ - store_test_results: path: test-results - store_artifacts: # upload nyc test coverage as artifact. @@ -218,12 +221,21 @@ jobs: # Purpose: Runs the unit tests in a Windows environment. windows-unit-tests: + # `parallelism` indicates how many simultaneous executors should be run, allowing us to split + # long-running tasks across multiple executors. + parallelism: 4 # larger values didn't seem to affect performance greatly executor: name: win/default # executor type - size: "medium" + size: "large" shell: bash.exe parameters: node-version: *node_version_param + # Specify a subset of unit tests to be run, instead of the whole suite. + # This allows us to work around the suboptimal performance of the Windows executor by running + # multiple executors in parallel where different unit tests are ran in each. + test-type: + type: string + default: all working_directory: C:\repo steps: - attach_workspace: @@ -259,12 +271,90 @@ jobs: - run: mkdir test-results # Unit tests - - run: - name: test - # Necessary to explicitly use bash, otherwise gradlew's status code won't be received and the job will hang. - shell: bash.exe - command: yarn test --reporter mocha-junit-reporter --reporter-option mochaFile=test-results/mocha/test-results.xml - when: always + - when: + condition: + equal: [ all, << parameters.test-type >> ] + steps: + - run: + name: test + # Necessary to explicitly use bash, otherwise gradlew's status code won't be received and the job will hang. + shell: bash.exe + command: yarn test --reporter mocha-junit-reporter --reporter-option mochaFile=test-results/mocha/test-results.xml + when: always + + - when: + condition: + equal: [ sfge, << parameters.test-type >> ] + steps: + - run: + name: test-sfge + # Necessary to explicitly use bash, otherwise gradlew's status code won't be received and the job will hang. + shell: bash.exe + # Identify all the test files and allocate them between parallelized executors by timing data. + # Then turn the array of tests into something that gradle can accept, and run the tests. + command: | + TESTGLOB=$(circleci tests glob "sfge/src/test/**/*Test.java" | circleci tests split --split-by=timings) + echo $TESTGLOB + TESTARRAY=($TESTGLOB) + TESTARG="" + for element in "${TESTARRAY[@]}" + do + TESTARG="$TESTARG --tests `basename $element .java`" + done + echo $TESTARG + yarn test-sfge $TESTARG + when: always + + - when: + condition: + equal: [ cli-messaging, << parameters.test-type >> ] + steps: + - run: + name: test-cli-messaging + # Necessary to explicitly use bash, otherwise gradlew's status code won't be received and the job will hang. + shell: bash.exe + # This unit test suite is fast, so we have the first parallel executor run the tests, and all others exit early. + command: | + if [[ $CIRCLE_NODE_INDEX -gt 0 ]] + then + exit 0 + fi + yarn test-cli-messaging + when: always + + - when: + condition: + equal: [ pmd-cataloger, << parameters.test-type >> ] + steps: + - run: + name: test-pmd-cataloger + # Necessary to explicitly use bash, otherwise gradlew's status code won't be received and the job will hang. + shell: bash.exe + # This unit test suite is fast, so we have the first parallel executor run the tests, and all others exit early. + command: | + if [[ $CIRCLE_NODE_INDEX -gt 0 ]] + then + exit 0 + fi + yarn test-pmd-cataloger + when: always + + - when: + condition: + equal: [ ts, << parameters.test-type >> ] + steps: + - run: + name: test-ts + # Explicitly using bash, for simplicity of required shell script. + shell: bash.exe + # This unit test suite is relatively fast, so we have the first parallel executor run the tests, and all others exit early. + command: | + if [[ $CIRCLE_NODE_INDEX -gt 0 ]] + then + exit 0 + fi + yarn test-ts --reporter mocha-junit-reporter --reporter-option mochaFile=test-results/mocha/test-results.xml + when: always # Linting - run: @@ -510,6 +600,11 @@ workflows: <<: *testing_filters requires: - setup + matrix: + parameters: + # The values of the parameters will be appended to the jobs they create. + # So we'll get "windows-unit-tests-pmd-cataloger", "windows-unit-tests-ts", etc. + test-type: [pmd-cataloger, cli-messaging, ts, sfge] - linux-tarball-test: filters: <<: *testing_filters diff --git a/.editorconfig b/.editorconfig index 00543da0b..3afaa32df 100644 --- a/.editorconfig +++ b/.editorconfig @@ -7,6 +7,10 @@ charset = utf-8 trim_trailing_whitespace = true insert_final_newline = true +[*.java] +indent_style = space +indent_size = 4 + [*.md] trim_trailing_whitespace = false diff --git a/messages/common.js b/messages/common.js new file mode 100644 index 000000000..be555a676 --- /dev/null +++ b/messages/common.js @@ -0,0 +1,3 @@ +module.exports = { + FEEDBACK_SURVEY_BANNER: `We're constantly improving Salesforce Code Analyzer. Tell us what you think! Give feedback at https://research.net/r/SalesforceCA.` +}; diff --git a/package.json b/package.json index 756d0a15f..e35f376fd 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "@salesforce/sfdx-scanner", "description": "Static code scanner that applies quality and security rules to Apex code, and provides feedback.", - "version": "3.2.0", + "version": "3.3.0", "author": "ISV SWAT", "bugs": "https://github.com/forcedotcom/sfdx-scanner/issues", "dependencies": { @@ -133,6 +133,10 @@ "postpack": "rm -f oclif.manifest.json", "lint": "eslint ./src --ext .ts", "test": "./gradlew test jacocoTestCoverageVerification && nyc mocha --timeout 10000 --retries 5 \"./test/**/*.test.ts\"", + "test-cli-messaging": "./gradlew cli-messaging:test", + "test-pmd-cataloger": "./gradlew pmd-cataloger:test", + "test-sfge": "./gradlew sfge:test", + "test-ts": "nyc mocha --timeout 10000 --retries 5 \"./test/**/*.test.ts\"", "coverage": "nyc report --reporter text", "version": "oclif-dev readme && git add README.md" } diff --git a/pmd-cataloger/build.gradle.kts b/pmd-cataloger/build.gradle.kts index 1c6a061cc..966b43f10 100644 --- a/pmd-cataloger/build.gradle.kts +++ b/pmd-cataloger/build.gradle.kts @@ -9,7 +9,7 @@ group = "sfdx" version = "1.0" val distDir = "$buildDir/../../dist" -val pmdVersion = "6.45.0" +val pmdVersion = "6.47.0" val pmdFile = "pmd-bin-$pmdVersion.zip" val pmdUrl = "https://github.com/pmd/pmd/releases/download/pmd_releases%2F${pmdVersion}/${pmdFile}" val skippableJarRegexes = setOf("""^common_[\d\.-]*\.jar""".toRegex(), diff --git a/sfge/.editorconfig b/sfge/.editorconfig index 60c0d3274..76d2d297a 100644 --- a/sfge/.editorconfig +++ b/sfge/.editorconfig @@ -9,5 +9,5 @@ trim_trailing_whitespace = true insert_final_newline = true [*.java] -indent_style = tab +indent_style = space indent_size = 4 diff --git a/sfge/src/main/java/com/salesforce/Main.java b/sfge/src/main/java/com/salesforce/Main.java index d7f4e0c31..aabe40ea9 100644 --- a/sfge/src/main/java/com/salesforce/Main.java +++ b/sfge/src/main/java/com/salesforce/Main.java @@ -4,6 +4,7 @@ import com.salesforce.cli.OutputFormatter; import com.salesforce.exception.SfgeException; import com.salesforce.exception.SfgeRuntimeException; +import com.salesforce.exception.UnexpectedException; import com.salesforce.graph.ops.GraphUtil; import com.salesforce.messaging.CliMessager; import com.salesforce.metainfo.MetaInfoCollector; @@ -88,7 +89,7 @@ private int catalog() { LOGGER.info("Invoked CATALOG flow"); List rules; try { - rules = RuleUtil.getAllRules(); + rules = RuleUtil.getEnabledRules(); } catch (SfgeException | SfgeRuntimeException ex) { System.err.println(ex.getMessage()); return INTERNAL_ERROR; @@ -129,6 +130,11 @@ private int execute(String... args) { LOGGER.error("Error while loading graph", ex); System.err.println(formatError(ex)); return INTERNAL_ERROR; + } catch (UnexpectedException ex) { + LOGGER.error("Unexpected exception while loading graph", ex); + System.err.println( + "Unexpected exception while loading graph. See logs for more information."); + return INTERNAL_ERROR; } // Run all of the rules. diff --git a/sfge/src/main/java/com/salesforce/apex/jorje/ModifierNodeWrapper.java b/sfge/src/main/java/com/salesforce/apex/jorje/ModifierNodeWrapper.java index 52e0e5b10..d98304b95 100644 --- a/sfge/src/main/java/com/salesforce/apex/jorje/ModifierNodeWrapper.java +++ b/sfge/src/main/java/com/salesforce/apex/jorje/ModifierNodeWrapper.java @@ -23,5 +23,7 @@ protected void fillProperties(Map properties) { properties.put(Schema.ABSTRACT, Modifier.isAbstract(javaModifiers)); properties.put(Schema.STATIC, Modifier.isStatic(javaModifiers)); properties.put(Schema.GLOBAL, getNode().getModifiers().has(ModifierTypeInfos.GLOBAL)); + properties.put(Schema.VIRTUAL, getNode().getModifiers().has(ModifierTypeInfos.VIRTUAL)); + properties.put(Schema.OVERRIDE, getNode().getModifiers().has(ModifierTypeInfos.OVERRIDE)); } } diff --git a/sfge/src/main/java/com/salesforce/cli/CliArgParser.java b/sfge/src/main/java/com/salesforce/cli/CliArgParser.java index e8e7eabd3..973bea8b3 100644 --- a/sfge/src/main/java/com/salesforce/cli/CliArgParser.java +++ b/sfge/src/main/java/com/salesforce/cli/CliArgParser.java @@ -126,9 +126,13 @@ private List readFile(String fileName) throws IOException { private void identifyRules(List rulesToRun) { try { - for (String ruleName : rulesToRun) { - AbstractRule rule = RuleUtil.getRule(ruleName); - selectedRules.add(rule); + if (rulesToRun.isEmpty()) { + selectedRules.addAll(RuleUtil.getEnabledRules()); + } else { + for (String ruleName : rulesToRun) { + AbstractRule rule = RuleUtil.getRule(ruleName); + selectedRules.add(rule); + } } } catch (RuleUtil.RuleNotFoundException ex) { throw new InvocationException(ex.getMessage(), ex); diff --git a/sfge/src/main/java/com/salesforce/config/UserFacingErrorMessages.java b/sfge/src/main/java/com/salesforce/config/UserFacingErrorMessages.java new file mode 100644 index 000000000..7d94730d4 --- /dev/null +++ b/sfge/src/main/java/com/salesforce/config/UserFacingErrorMessages.java @@ -0,0 +1,14 @@ +package com.salesforce.config; + +/** + * Contains error message constants that will be displayed to users. TODO: move all other + * user-facing messages here. + */ +public final class UserFacingErrorMessages { + + /** UserActionException * */ + + // format: filename,defined type, line number + public static final String UNREACHABLE_CODE = + "Please remove unreachable code to proceed with analysis: %s,%s:%d"; +} diff --git a/sfge/src/main/java/com/salesforce/graph/ApexPath.java b/sfge/src/main/java/com/salesforce/graph/ApexPath.java index 340c19c53..08dc08eb7 100644 --- a/sfge/src/main/java/com/salesforce/graph/ApexPath.java +++ b/sfge/src/main/java/com/salesforce/graph/ApexPath.java @@ -269,7 +269,10 @@ public void addVertices(List vertices) { !(vertices.get(0) instanceof BlockStatementVertex) && // Class Instantiation Path - !(vertices.get(0) instanceof FieldVertex)) { + !(vertices.get(0) instanceof FieldVertex) + && + // Static blocks + !(vertices.get(0) instanceof MethodCallExpressionVertex)) { throw new UnexpectedException(vertices); } this.vertices.addAll(vertices); diff --git a/sfge/src/main/java/com/salesforce/graph/Schema.java b/sfge/src/main/java/com/salesforce/graph/Schema.java index 068be7f13..f5b011cd1 100644 --- a/sfge/src/main/java/com/salesforce/graph/Schema.java +++ b/sfge/src/main/java/com/salesforce/graph/Schema.java @@ -28,7 +28,9 @@ public class Schema { public static final String IDENTIFIER = "Identifier"; public static final String IMPLEMENTATION_OF = "ImplementationOf"; public static final String IMPLEMENTED_BY = "ImplementedBy"; + public static final String INTERFACE_DEFINING_TYPES = "InterfaceDefiningTypes"; public static final String INTERFACE_NAMES = "InterfaceNames"; + public static final String INVOCABLE_METHOD = "InvocableMethod"; /** True if this vertex is part of the Apex Standard Library */ public static final String IS_STANDARD = "IsStandard"; /** @@ -44,8 +46,11 @@ public class Schema { public static final String MODIFIERS = "Modifiers"; public static final String NAME = "Name"; public static final String NAMES = "Names"; + public static final String NAMESPACE_ACCESSIBLE = "NamespaceAccessible"; public static final String OPERATOR = "Operator"; + public static final String OVERRIDE = "Override"; public static final String REFERENCE_TYPE = "ReferenceType"; + public static final String REMOTE_ACTION = "RemoteAction"; public static final String RETURN_TYPE = "ReturnType"; public static final String RULE_NAMES = "RulesNames"; public static final String STATIC = "Static"; @@ -56,6 +61,7 @@ public class Schema { public static final String TYPE_REF = "TypeRef"; public static final String VALUE = "Value"; + public static final String VIRTUAL = "Virtual"; public static final String QUERY = "Query"; public static final class JorjeNodeType { @@ -67,4 +73,16 @@ public static final class JorjeNodeType { public static final String CHILD = "Child"; public static final String PARENT = "Parent"; public static final String NEXT_SIBLING = "NextSibling"; + + /** Mark a vertex as synthetic */ + public static final String IS_SYNTHETIC = "IsSynthetic"; + /** Indicates if a method is a synthetic static block method */ + public static final String IS_STATIC_BLOCK_METHOD = "IsStaticBlockMethod"; + /** Indicates if a method is a synthetic static block invoker method */ + public static final String IS_STATIC_BLOCK_INVOKER_METHOD = "IsStaticBlockInvokerMethod"; + /** + * Indicates if a MethodCallExpression is a synthetic invocation of static block from invoker + * method + */ + public static final String IS_STATIC_BLOCK_INVOCATION = "IsStaticBlockInvocation"; } diff --git a/sfge/src/main/java/com/salesforce/graph/build/AbstractApexVertexBuilder.java b/sfge/src/main/java/com/salesforce/graph/build/AbstractApexVertexBuilder.java index e9c02ffe1..30ee78a57 100644 --- a/sfge/src/main/java/com/salesforce/graph/build/AbstractApexVertexBuilder.java +++ b/sfge/src/main/java/com/salesforce/graph/build/AbstractApexVertexBuilder.java @@ -1,13 +1,12 @@ package com.salesforce.graph.build; -import com.google.common.collect.ImmutableSet; import com.salesforce.apex.jorje.ASTConstants; import com.salesforce.apex.jorje.JorjeNode; import com.salesforce.collections.CollectionUtil; import com.salesforce.exception.UnexpectedException; import com.salesforce.graph.Schema; -import java.util.Arrays; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -26,10 +25,6 @@ @SuppressWarnings("PMD.AbstractClassWithoutAbstractMethod") abstract class AbstractApexVertexBuilder { private static final Logger LOGGER = LogManager.getLogger(AbstractApexVertexBuilder.class); - protected static final Set ROOT_VERTICES = - ImmutableSet.builder() - .addAll(Arrays.asList(ASTConstants.NodeType.ROOT_VERTICES)) - .build(); protected final GraphTraversalSource g; protected AbstractApexVertexBuilder(GraphTraversalSource g) { @@ -65,30 +60,48 @@ private void buildVertices(JorjeNode node, Vertex vNodeParam, String fileName) { // The engine assumes that only certain types of nodes can be roots. We need to enforce // that assumption and // fail noisily if it's violated. - if (!ROOT_VERTICES.contains(vNode.label())) { + if (!GremlinUtil.ROOT_VERTICES.contains(vNode.label())) { throw new UnexpectedException("Unexpected root vertex of type " + vNode.label()); } vNode.property(Schema.FILE_NAME, fileName); } Vertex vPreviousSibling = null; - for (JorjeNode child : node.getChildren()) { - Vertex vChild = g.addV(child.getLabel()).next(); + final List children = node.getChildren(); + final Set verticesAddressed = new HashSet<>(); + verticesAddressed.add(vNode); + + for (int i = 0; i < children.size(); i++) { + final JorjeNode child = children.get(i); + final Vertex vChild = g.addV(child.getLabel()).next(); addProperties(g, child, vChild); - // We are currently adding PARENT and CHILD, in theory the same edge could be navigated - // both ways, however - // the code looks messy when that is done. - // TODO: Determine if this causes performance or resource issues. Consider a single edge - g.addE(Schema.PARENT).from(vChild).to(vNode).iterate(); - g.addE(Schema.CHILD).from(vNode).to(vChild).iterate(); + + /** + * Handle static block if we are looking at a method that has a block + * statement. See {@linkplain StaticBlockUtil} on why this is needed and how we handle + * it. + */ + if (StaticBlockUtil.isStaticBlockStatement(node, child)) { + final Vertex parentVertexForChild = + StaticBlockUtil.createSyntheticStaticBlockMethod(g, vNode, i); + GremlinVertexUtil.addParentChildRelationship(g, parentVertexForChild, vChild); + verticesAddressed.add(parentVertexForChild); + } else { + GremlinVertexUtil.addParentChildRelationship(g, vNode, vChild); + } + if (vPreviousSibling != null) { g.addE(Schema.NEXT_SIBLING).from(vPreviousSibling).to(vChild).iterate(); } vPreviousSibling = vChild; + // To save memory in the graph, don't pass the source name into recursive calls. buildVertices(child, vChild, null); } - afterInsert(g, node, vNode); + // Execute afterInsert() on each vertex we addressed + for (Vertex vertex : verticesAddressed) { + afterInsert(g, node, vertex); + } if (rootVNode != null) { // Only call this for the root node afterFileInsert(g, rootVNode); @@ -101,6 +114,8 @@ private void buildVertices(JorjeNode node, Vertex vNodeParam, String fileName) { */ private final void afterInsert(GraphTraversalSource g, JorjeNode node, Vertex vNode) { if (node.getLabel().equals(ASTConstants.NodeType.METHOD)) { + // If we just added a method, create forward and + // backward code flow for the contents of the method MethodPathBuilderVisitor.apply(g, vNode); } } @@ -111,7 +126,9 @@ private final void afterInsert(GraphTraversalSource g, JorjeNode node, Vertex vN * @param vNode root node that corresponds to the file */ protected void afterFileInsert(GraphTraversalSource g, Vertex vNode) { - // Intentionally left blank + // If the root (class/trigger/etc) contained any static blocks, + // create an invoker method to invoke the static blocks + StaticBlockUtil.createSyntheticStaticBlockInvocation(g, vNode); } protected void addProperties(GraphTraversalSource g, JorjeNode node, Vertex vNode) { @@ -122,11 +139,12 @@ protected void addProperties(GraphTraversalSource g, JorjeNode node, Vertex vNod String key = entry.getKey(); Object value = entry.getValue(); value = adjustPropertyValue(node, key, value); - addProperty(previouslyInsertedKeys, traversal, key, value); + GremlinVertexUtil.addProperty(previouslyInsertedKeys, traversal, key, value); } for (Map.Entry entry : getAdditionalProperties(node).entrySet()) { - addProperty(previouslyInsertedKeys, traversal, entry.getKey(), entry.getValue()); + GremlinVertexUtil.addProperty( + previouslyInsertedKeys, traversal, entry.getKey(), entry.getValue()); } // Commit the changes. @@ -142,49 +160,4 @@ protected Object adjustPropertyValue(JorjeNode node, String key, Object value) { protected Map getAdditionalProperties(JorjeNode node) { return new HashMap<>(); } - - /** Add a property to the traversal, throwing an exception if any keys are duplicated. */ - protected void addProperty( - TreeSet previouslyInsertedKeys, - GraphTraversal traversal, - String keyParam, - Object value) { - final String key = keyParam.intern(); - - if (!previouslyInsertedKeys.add(key)) { - throw new UnexpectedException(key); - } - - if (value instanceof List) { - List list = (List) value; - // Convert ArrayList to an Array. There seems to be a Tinkerpop bug where a singleton - // ArrayList - // isn't properly stored. Remote graphs also have issues with empty lists, so don't add - // an empty list. - if (!list.isEmpty()) { - traversal.property(key, list.toArray()); - } else { - // return so that we don't store a case insensitive version - return; - } - } else if (value instanceof Boolean - || value instanceof Double - || value instanceof Integer - || value instanceof Long - || value instanceof String) { - traversal.property(key, value); - } else { - if (value != null) { - if (!(value instanceof Enum)) { - if (LOGGER.isWarnEnabled()) { - LOGGER.warn( - "Using string for value. type=" + value.getClass().getSimpleName()); - } - } - final String strValue = String.valueOf(value).intern(); - traversal.property(key, strValue); - } - } - CaseSafePropertyUtil.addCaseSafeProperty(traversal, key, value); - } } diff --git a/sfge/src/main/java/com/salesforce/graph/build/ApexStandardLibraryVertexBuilder.java b/sfge/src/main/java/com/salesforce/graph/build/ApexStandardLibraryVertexBuilder.java index 8486cd02f..141ad1e71 100644 --- a/sfge/src/main/java/com/salesforce/graph/build/ApexStandardLibraryVertexBuilder.java +++ b/sfge/src/main/java/com/salesforce/graph/build/ApexStandardLibraryVertexBuilder.java @@ -51,8 +51,7 @@ protected Object adjustPropertyValue(JorjeNode node, String key, Object value) { Object result = value; final boolean isRootVertexNameName = - AbstractApexVertexBuilder.ROOT_VERTICES.contains(node.getLabel()) - && key.equals(Schema.NAME); + GremlinUtil.ROOT_VERTICES.contains(node.getLabel()) && key.equals(Schema.NAME); if (isRootVertexNameName || key.equals(Schema.DEFINING_TYPE)) { result = getFullName(currentPackage, (String) value); if (LOGGER.isTraceEnabled()) { diff --git a/sfge/src/main/java/com/salesforce/graph/build/CaseSafePropertyUtil.java b/sfge/src/main/java/com/salesforce/graph/build/CaseSafePropertyUtil.java index e0439ee71..3d068a49c 100644 --- a/sfge/src/main/java/com/salesforce/graph/build/CaseSafePropertyUtil.java +++ b/sfge/src/main/java/com/salesforce/graph/build/CaseSafePropertyUtil.java @@ -39,12 +39,13 @@ public class CaseSafePropertyUtil { // The following properties are (with rare exception) assumed to be case-sensitive. private static final ImmutableSet CASE_SENSITIVE_PROPERTIES = ImmutableSet.of( - Schema.NAME, Schema.DEFINING_TYPE, Schema.FULL_METHOD_NAME, + Schema.INTERFACE_DEFINING_TYPES, Schema.INTERFACE_NAMES, Schema.METHOD_NAME, Schema.NAME, + Schema.RETURN_TYPE, Schema.SUPER_CLASS_NAME, Schema.SUPER_INTERFACE_NAME); @@ -80,16 +81,16 @@ private static String toCaseSafeValue(String value) { return value.toLowerCase(Locale.ROOT); } - private static ArrayList toCaseSafeValue(ArrayList value) { + private static Object[] toCaseSafeValue(ArrayList value) { if (value.isEmpty() || !(value.get(0) instanceof String)) { // An empty array or non-string array can't be made case-safe. - return value; + return value.toArray(); } else { ArrayList stringList = new ArrayList<>(); for (Object o : value) { stringList.add(((String) o).toLowerCase(Locale.ROOT)); } - return stringList; + return stringList.toArray(); } } @@ -191,5 +192,19 @@ public static GraphTraversal hasEndingWith( return __.has(nodeType, property, TextP.endingWith(value)); } } + + public static GraphTraversal hasArrayContaining( + String nodeType, String property, String value) { + if (property.equalsIgnoreCase(Schema.DEFINING_TYPE)) { + throw new UnexpectedException(property); + } + String caseSafeVariant = toCaseSafeProperty(property).orElse(null); + if (caseSafeVariant != null) { + return __.hasLabel(nodeType) + .has(caseSafeVariant, __.unfold().is(toCaseSafeValue(value))); + } else { + return __.hasLabel(nodeType).has(property, __.unfold().is(value)); + } + } } } diff --git a/sfge/src/main/java/com/salesforce/graph/build/CustomerApexVertexBuilder.java b/sfge/src/main/java/com/salesforce/graph/build/CustomerApexVertexBuilder.java index b7419fbcf..073695448 100644 --- a/sfge/src/main/java/com/salesforce/graph/build/CustomerApexVertexBuilder.java +++ b/sfge/src/main/java/com/salesforce/graph/build/CustomerApexVertexBuilder.java @@ -24,6 +24,7 @@ public void build() { @Override protected void afterFileInsert(GraphTraversalSource g, Vertex vNode) { + super.afterFileInsert(g, vNode); ApexPropertyAnnotator.apply(g, vNode); } } diff --git a/sfge/src/main/java/com/salesforce/graph/build/GremlinUtil.java b/sfge/src/main/java/com/salesforce/graph/build/GremlinUtil.java index 72c3ac09b..ffbfdec16 100644 --- a/sfge/src/main/java/com/salesforce/graph/build/GremlinUtil.java +++ b/sfge/src/main/java/com/salesforce/graph/build/GremlinUtil.java @@ -1,14 +1,19 @@ package com.salesforce.graph.build; +import com.google.common.collect.ImmutableSet; +import com.salesforce.apex.jorje.ASTConstants; import com.salesforce.exception.UnexpectedException; import com.salesforce.graph.Schema; +import java.util.Arrays; import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import org.apache.tinkerpop.gremlin.process.traversal.Order; import org.apache.tinkerpop.gremlin.process.traversal.Scope; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; import org.apache.tinkerpop.gremlin.structure.T; import org.apache.tinkerpop.gremlin.structure.Vertex; @@ -17,6 +22,11 @@ * and labels. */ public final class GremlinUtil { + protected static final Set ROOT_VERTICES = + ImmutableSet.builder() + .addAll(Arrays.asList(ASTConstants.NodeType.ROOT_VERTICES)) + .build(); + public static Long getId(Map vertexProperties) { return (Long) vertexProperties.get(T.id); } @@ -32,7 +42,11 @@ public static Optional getOnlyChild( if (children.isEmpty()) { return Optional.empty(); } else if (children.size() > 1) { - throw new UnexpectedException(children); + throw new UnexpectedException( + "Did not expect more than one child node of type " + + childLabel + + ". Actual count: " + + children.size()); } else { return Optional.of(children.get(0)); } @@ -56,6 +70,11 @@ public static List getChildren(GraphTraversalSource g, Vertex vertex) { .toList(); } + public static List getChildren( + GraphTraversalSource g, Vertex vertex, String childLabel) { + return g.V(vertex).out(Schema.CHILD).hasLabel(childLabel).toList(); + } + public static Optional getPreviousSibling(GraphTraversalSource g, Vertex vertex) { Iterator it = g.V(vertex).in(Schema.NEXT_SIBLING); if (it.hasNext()) { @@ -87,5 +106,28 @@ public static Optional getFirstChild(GraphTraversalSource g, Vertex vert } } + public static Optional getParent(GraphTraversalSource g, Vertex vertex) { + Iterator it = g.V(vertex).out(Schema.PARENT); + if (it.hasNext()) { + return Optional.of(it.next()); + } else { + return Optional.empty(); + } + } + + public static String getFileName(GraphTraversalSource g, Vertex vertex) { + if (ROOT_VERTICES.contains(vertex.label())) { + return vertex.value(Schema.FILE_NAME); + } + List verticesWithFileName = + g.V(vertex).repeat(__.in(Schema.CHILD)).until(__.has(Schema.FILE_NAME)).toList(); + + if (verticesWithFileName.isEmpty()) { + return "UNKNOWN_FILENAME"; + } + + return verticesWithFileName.get(0).value(Schema.FILE_NAME); + } + private GremlinUtil() {} } diff --git a/sfge/src/main/java/com/salesforce/graph/build/GremlinVertexUtil.java b/sfge/src/main/java/com/salesforce/graph/build/GremlinVertexUtil.java new file mode 100644 index 000000000..3efb7f3c1 --- /dev/null +++ b/sfge/src/main/java/com/salesforce/graph/build/GremlinVertexUtil.java @@ -0,0 +1,92 @@ +package com.salesforce.graph.build; + +import com.salesforce.exception.UnexpectedException; +import com.salesforce.graph.Schema; +import java.util.List; +import java.util.Optional; +import java.util.TreeSet; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.apache.tinkerpop.gremlin.structure.Vertex; + +/** Handles common operations performed while creating vertices on Graph */ +public final class GremlinVertexUtil { + private static final Logger LOGGER = LogManager.getLogger(GremlinVertexUtil.class); + + private GremlinVertexUtil() {} + + /** Create parent-child relationship between vertices on graph */ + static void addParentChildRelationship( + GraphTraversalSource g, Vertex parentVertex, Vertex childVertex) { + // We are currently adding PARENT and CHILD, in theory the same edge could be navigated + // both ways, however + // the code looks messy when that is done. + // TODO: Determine if this causes performance or resource issues. Consider a single edge + g.addE(Schema.PARENT).from(childVertex).to(parentVertex).iterate(); + g.addE(Schema.CHILD).from(parentVertex).to(childVertex).iterate(); + } + + /** Make a synthetic vertex a sibling of an existing vertex on graph */ + static void makeSiblings(GraphTraversalSource g, Vertex vertex, Vertex syntheticVertex) { + final Vertex rootVertex = getParentVertex(g, vertex); + + addParentChildRelationship(g, rootVertex, syntheticVertex); + } + + static Vertex getParentVertex(GraphTraversalSource g, Vertex vertex) { + // Get parent node of vertex + final Optional rootVertex = GremlinUtil.getParent(g, vertex); + if (!rootVertex.isPresent()) { + throw new UnexpectedException( + "Did not expect vertex to not have a parent vertex. vertex=" + vertex); + } + return rootVertex.get(); + } + + /** Add a property to the traversal, throwing an exception if any keys are duplicated. */ + protected static void addProperty( + TreeSet previouslyInsertedKeys, + GraphTraversal traversal, + String keyParam, + Object value) { + final String key = keyParam.intern(); + + if (!previouslyInsertedKeys.add(key)) { + throw new UnexpectedException(key); + } + + if (value instanceof List) { + List list = (List) value; + // Convert ArrayList to an Array. There seems to be a Tinkerpop bug where a singleton + // ArrayList + // isn't properly stored. Remote graphs also have issues with empty lists, so don't add + // an empty list. + if (!list.isEmpty()) { + traversal.property(key, list.toArray()); + } else { + // return so that we don't store a case insensitive version + return; + } + } else if (value instanceof Boolean + || value instanceof Double + || value instanceof Integer + || value instanceof Long + || value instanceof String) { + traversal.property(key, value); + } else { + if (value != null) { + if (!(value instanceof Enum)) { + if (LOGGER.isWarnEnabled()) { + LOGGER.warn( + "Using string for value. type=" + value.getClass().getSimpleName()); + } + } + final String strValue = String.valueOf(value).intern(); + traversal.property(key, strValue); + } + } + CaseSafePropertyUtil.addCaseSafeProperty(traversal, key, value); + } +} diff --git a/sfge/src/main/java/com/salesforce/graph/build/InheritanceEdgeBuilder.java b/sfge/src/main/java/com/salesforce/graph/build/InheritanceInformationBuilder.java similarity index 57% rename from sfge/src/main/java/com/salesforce/graph/build/InheritanceEdgeBuilder.java rename to sfge/src/main/java/com/salesforce/graph/build/InheritanceInformationBuilder.java index 7847dac40..2442a370a 100644 --- a/sfge/src/main/java/com/salesforce/graph/build/InheritanceEdgeBuilder.java +++ b/sfge/src/main/java/com/salesforce/graph/build/InheritanceInformationBuilder.java @@ -10,7 +10,6 @@ import com.salesforce.graph.vertex.InheritableSFVertex; import com.salesforce.graph.vertex.SFVertexFactory; import com.salesforce.graph.vertex.UserClassVertex; -import com.salesforce.graph.vertex.UserInterfaceVertex; import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; @@ -18,20 +17,21 @@ import java.util.Map; import java.util.Optional; import java.util.TreeMap; -import java.util.stream.Collectors; +import java.util.TreeSet; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.tinkerpop.gremlin.process.traversal.P; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; import org.apache.tinkerpop.gremlin.structure.Vertex; -public class InheritanceEdgeBuilder implements GraphBuilder { - private static final Logger LOGGER = LogManager.getLogger(InheritanceEdgeBuilder.class); +public class InheritanceInformationBuilder implements GraphBuilder { + private static final Logger LOGGER = LogManager.getLogger(InheritanceInformationBuilder.class); private final GraphTraversalSource g; private final Map verticesById; private final TreeMap verticesByDefiningType; - public InheritanceEdgeBuilder(GraphTraversalSource g) { + public InheritanceInformationBuilder(GraphTraversalSource g) { this.g = g; this.verticesById = new HashMap<>(); this.verticesByDefiningType = CollectionUtil.newTreeMap(); @@ -73,7 +73,8 @@ private void buildInheritanceMaps() { private void processVertexInheritance(InheritableSFVertex vertex) { InheritableSFVertex extendedVertex = findExtendedVertex(vertex).orElse(null); - List implementedVertices = findImplementedVertices(vertex); + Map implementedVerticesByName = + findImplementedVerticesByName(vertex); // It's possible for the extendedVertex to be null, if the extended class is declared in a // file that wasn't scanned. @@ -85,17 +86,30 @@ private void processVertexInheritance(InheritableSFVertex vertex) { Schema.EXTENSION_OF); } - if (implementedVertices != null) { - for (InheritableSFVertex implementedVertex : implementedVertices) { - // It's possible for the implementedVertex to be null if the interface being - // implemented is declared in a file - // that wasn't scanned. + if (!implementedVerticesByName.isEmpty()) { + List implementedVertexDefiningTypes = new ArrayList<>(); + Long myId = vertex.getId(); + for (String implementedVertexName : implementedVerticesByName.keySet()) { + InheritableSFVertex implementedVertex = + implementedVerticesByName.get(implementedVertexName); + // If we actually have a vertex for this interface, we need to create edges + // connecting it to the implementor, + // and we can use its defining type for our list. if (implementedVertex != null) { Long implId = implementedVertex.getId(); - Long myId = vertex.getId(); addEdges(implId, myId, Schema.IMPLEMENTED_BY, Schema.IMPLEMENTATION_OF); + implementedVertexDefiningTypes.add(implementedVertex.getDefiningType()); + } else { + // If there's no vertex for this interface, we can assume it's defined in some + // other codebase, and therefore + // whatever name was used by the implementor to reference it can be assumed to + // be the defining type. + implementedVertexDefiningTypes.add(implementedVertexName); } } + // Give the implementor new properties indicating the full names of the implemented + // interfaces. + addProperties(myId, implementedVertexDefiningTypes); } } @@ -108,18 +122,20 @@ private Optional findExtendedVertex(InheritableSFVertex ver } } - private List findImplementedVertices(InheritableSFVertex vertex) { - if (vertex instanceof UserInterfaceVertex) { - return new ArrayList<>(); - } else { + private Map findImplementedVerticesByName( + InheritableSFVertex vertex) { + Map results = new HashMap<>(); + // Only classes can implement interfaces. + if (vertex instanceof UserClassVertex) { String inheritorType = vertex.getDefiningType(); - return ((UserClassVertex) vertex) - .getInterfaceNames().stream() - .map(i -> findInheritedVertex(i, inheritorType)) - .filter(v -> v.isPresent()) - .map(v -> v.get()) - .collect(Collectors.toList()); + List interfaceNames = ((UserClassVertex) vertex).getInterfaceNames(); + for (String interfaceName : interfaceNames) { + InheritableSFVertex inheritedVertex = + findInheritedVertex(interfaceName, inheritorType).orElse(null); + results.put(interfaceName, inheritedVertex); + } } + return results; } private Optional findInheritedVertex( @@ -127,17 +143,19 @@ private Optional findInheritedVertex( if (inheritableType == null) { throw new UnexpectedException("inheritableType can't be null"); } - // If the inheritor type contains a period, it's an inner type. If the inheritable type does - // not contain a period, - // it could be either an outer type, or an inner type declared in the same outer type. The - // latter overrides the - // former, so we'll derive the full name of such an inner type so we can check if it exists. - if (inheritorType.contains(".") && !inheritableType.contains(".")) { - String[] parts = inheritorType.split("\\."); - - String possibleInnerType = parts[0] + "." + inheritableType; - if (verticesByDefiningType.containsKey(possibleInnerType)) { - return Optional.ofNullable(verticesByDefiningType.get(possibleInnerType)); + // If the inheritable type does NOT contain a period, then it's either an outer type, or an + // inner type declared + // in the same outer type as the inheritor type. + // The latter takes priority over the former, so we should check if it's the case. + if (!inheritableType.contains(".")) { + // If the inheritor type contains a period, then it's an inner type, and we should use + // its outer type. + // Otherwise, we should use the inheritor type as a potential outer type. + String potentialOuterType = + inheritorType.contains(".") ? inheritorType.split("\\.")[0] : inheritorType; + String potentialInnerType = potentialOuterType + "." + inheritableType; + if (verticesByDefiningType.containsKey(potentialInnerType)) { + return Optional.ofNullable(verticesByDefiningType.get(potentialInnerType)); } } @@ -163,4 +181,19 @@ private void addEdges(Long inheritableId, Long inheritorId, String fromEdge, Str g.addE(fromEdge).from(inheritable).to(inheritor).iterate(); g.addE(toEdge).from(inheritor).to(inheritable).iterate(); } + + private void addProperties(Long inheritorId, List implementedVertexDefiningTypes) { + GraphTraversal traversal = g.V(inheritorId); + // It's probably not best practice to be using an empty treeset here, but we no longer have + // access to the treeset + // that was used when adding the base properties, and we're setting a property that + // definitely isn't set elsewhere, + // so it should be fine. + GremlinVertexUtil.addProperty( + new TreeSet<>(), + traversal, + Schema.INTERFACE_DEFINING_TYPES, + implementedVertexDefiningTypes); + traversal.iterate(); + } } diff --git a/sfge/src/main/java/com/salesforce/graph/build/MethodPathBuilderVisitor.java b/sfge/src/main/java/com/salesforce/graph/build/MethodPathBuilderVisitor.java index 5d7ba6c86..247b811e5 100644 --- a/sfge/src/main/java/com/salesforce/graph/build/MethodPathBuilderVisitor.java +++ b/sfge/src/main/java/com/salesforce/graph/build/MethodPathBuilderVisitor.java @@ -2,8 +2,10 @@ import com.google.common.collect.ImmutableSet; import com.salesforce.apex.jorje.ASTConstants.NodeType; +import com.salesforce.config.UserFacingErrorMessages; import com.salesforce.exception.TodoException; import com.salesforce.exception.UnexpectedException; +import com.salesforce.exception.UserActionException; import com.salesforce.graph.Schema; import com.salesforce.graph.vertex.SFVertexFactory; import java.util.ArrayList; @@ -165,7 +167,7 @@ private void _visit(Vertex vertex, Vertex parent, boolean lastChild) { currentInnerScope.pop(); nextVertexInOuterScope.pop(); } - } catch (TodoException | UnexpectedException ex) { + } catch (UserActionException | TodoException | UnexpectedException ex) { throw ex; } catch (Exception ex) { throw new UnexpectedException(g, vertex, ex); @@ -592,7 +594,13 @@ private void addEdgeFromPreviousSibling(Vertex vertex) { private void addEdge(String name, Vertex from, Vertex to) { if (NodeType.TERMINAL_VERTEX_LABELS.contains(from.label())) { - throw new UnexpectedException("Vertex is terminal. vertex=" + vertexToString(from)); + // Ask user to fix unreachable code + throw new UserActionException( + String.format( + UserFacingErrorMessages.UNREACHABLE_CODE, + GremlinUtil.getFileName(g, to), + to.value(Schema.DEFINING_TYPE), + to.value(Schema.BEGIN_LINE))); } if (LOGGER.isTraceEnabled()) { diff --git a/sfge/src/main/java/com/salesforce/graph/build/StaticBlockUtil.java b/sfge/src/main/java/com/salesforce/graph/build/StaticBlockUtil.java new file mode 100644 index 000000000..339cce69f --- /dev/null +++ b/sfge/src/main/java/com/salesforce/graph/build/StaticBlockUtil.java @@ -0,0 +1,374 @@ +package com.salesforce.graph.build; + +import com.salesforce.apex.jorje.ASTConstants; +import com.salesforce.apex.jorje.JorjeNode; +import com.salesforce.collections.CollectionUtil; +import com.salesforce.exception.ProgrammingException; +import com.salesforce.graph.Schema; +import com.salesforce.graph.ops.MethodUtil; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.TreeSet; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.apache.tinkerpop.gremlin.structure.Vertex; + +/** + * Handles creation of synthetic methods and vertices to gracefully invoke static code blocks. + * + *

Consider this example: + * class StaticBlockClass { + * static { + * System.debug("inside static block 1"); + * } + * static { + * System.debug("inside static block 2"); + * } + * } + * In Jorje's compilation structure, static blocks are represented like this: + * class StaticBlockClass { + * private static void () { + * { + * System.debug("inside static block 1"); + * } + * { + * System.debug("inside static block 2"); + * } + * } + * } + * + * + *

Having multiple block statements inside a method breaks SFGE's normal code flow logic. This + * makes handling code blocks in () impossible. + * + *

As an alternative, we are creating synthetic vertices in the Graph to represent the static + * blocks as individual methods. + * + *

We also create one top-level synthetic method ("StaticBlockInvoker") that invokes individual + * static block methods. While creating static scope for this class, we should invoke the method + * call expressions inside the top-level synthetic method. + * + *

New structure looks like this: + * class StaticBlockClass { + * private static void SyntheticStaticBlock_1() { + * System.debug("inside static block 1"); + * } + * private static void SyntheticStaticBlock_2() { + * System.debug("inside static block 2"); + * } + * private static void StaticBlockInvoker() { + * SyntheticStaticBlock_1(); + * SyntheticStaticBlock_2(); + * } + * } + * + */ +public final class StaticBlockUtil { + private static final Logger LOGGER = LogManager.getLogger(StaticBlockUtil.class); + + static final String SYNTHETIC_STATIC_BLOCK_METHOD_NAME = "SyntheticStaticBlock_%d"; + static final String STATIC_BLOCK_INVOKER_METHOD = "StaticBlockInvoker"; + + private StaticBlockUtil() {} + + /** + * Creates a synthetic method vertex to represent a static code block + * + * @param g traversal graph + * @param clinitVertex () method's vertex + * @param staticBlockIndex index to use for name uniqueness - TODO: this index is currently just + * the child count index and does not necessarily follow sequence + * @return new synthetic method vertex with name SyntheticStaticBlock_%d, which will be the + * parent for blockStatementVertex, and a sibling of () + */ + public static Vertex createSyntheticStaticBlockMethod( + GraphTraversalSource g, Vertex clinitVertex, int staticBlockIndex) { + final Vertex syntheticMethodVertex = g.addV(ASTConstants.NodeType.METHOD).next(); + final String definingType = clinitVertex.value(Schema.DEFINING_TYPE); + final List siblings = + GremlinUtil.getChildren(g, GremlinVertexUtil.getParentVertex(g, clinitVertex)); + final int nextSiblingIndex = siblings.size(); + + addSyntheticStaticBlockMethodProperties( + g, definingType, syntheticMethodVertex, staticBlockIndex, nextSiblingIndex); + + final Vertex modifierNodeVertex = g.addV(ASTConstants.NodeType.MODIFIER_NODE).next(); + addStaticModifierProperties(g, definingType, modifierNodeVertex); + GremlinVertexUtil.addParentChildRelationship(g, syntheticMethodVertex, modifierNodeVertex); + + GremlinVertexUtil.makeSiblings(g, clinitVertex, syntheticMethodVertex); + + return syntheticMethodVertex; + } + + /** + * If rootNode contains synthetic static block methods (created through {@link + * #createSyntheticStaticBlockMethod}), adds a top-level synthetic method that invokes each + * static block method. + */ + public static void createSyntheticStaticBlockInvocation( + GraphTraversalSource g, Vertex rootNode) { + // Check if root node contains any static block methods + final List staticBlockMethods = getStaticBlockMethods(g, rootNode); + + // Create static block invocation method + if (!staticBlockMethods.isEmpty()) { + final String definingType = rootNode.value(Schema.DEFINING_TYPE); + if (LOGGER.isDebugEnabled()) { + LOGGER.debug( + "Creating synthetic invocation method for {} to invoke {} static block(s)", + definingType, + staticBlockMethods.size()); + } + + final Vertex invokerMethodVertex = + createSyntheticInvocationsMethod(g, rootNode, staticBlockMethods, definingType); + MethodPathBuilderVisitor.apply(g, invokerMethodVertex); + } + } + + static boolean isStaticBlockStatement(JorjeNode node, JorjeNode child) { + return isClinitMethod(node) && containsStaticBlock(node) && isBlockStatement(child); + } + + private static Vertex createSyntheticInvocationsMethod( + GraphTraversalSource g, + Vertex rootNode, + List staticBlockMethods, + String definingType) { + // Create new synthetic method StaticBlockInvoker to invoke each synthetic static block + // method + final List siblings = GremlinUtil.getChildren(g, rootNode); + final Vertex invokerMethodVertex = g.addV(ASTConstants.NodeType.METHOD).next(); + addStaticBlockInvokerProperties(g, definingType, invokerMethodVertex, siblings.size()); + GremlinVertexUtil.addParentChildRelationship(g, rootNode, invokerMethodVertex); + + final Vertex modifierNodeVertex = g.addV(ASTConstants.NodeType.MODIFIER_NODE).next(); + addStaticModifierProperties(g, definingType, modifierNodeVertex); + GremlinVertexUtil.addParentChildRelationship(g, invokerMethodVertex, modifierNodeVertex); + + // Create synthetic BlockStatement inside StaticBlockInvoker to hold the method invocations + final Vertex blockStatementInInvoker = g.addV(ASTConstants.NodeType.BLOCK_STATEMENT).next(); + addBlockStatementProperties(g, definingType, blockStatementInInvoker); + GremlinVertexUtil.addParentChildRelationship( + g, invokerMethodVertex, blockStatementInInvoker); + + for (int i = 0; i < staticBlockMethods.size(); i++) { + final Vertex staticBlockMethod = staticBlockMethods.get(i); + boolean isLastMethod = i == staticBlockMethods.size() - 1; + // Create a method call invocation to the synthetic static block method + final Vertex exprStaticBlockMethodCall = + createMethodCallExpression(g, definingType, staticBlockMethod, isLastMethod); + + // Add the expression statement containing the method call inside StaticBlockInvoker's + // block statement + GremlinVertexUtil.addParentChildRelationship( + g, blockStatementInInvoker, exprStaticBlockMethodCall); + } + + return invokerMethodVertex; + } + + private static Vertex createMethodCallExpression( + GraphTraversalSource g, + String definingType, + Vertex staticBlockMethod, + boolean isLastMethod) { + final Vertex expressionStmt = g.addV(ASTConstants.NodeType.EXPRESSION_STATEMENT).next(); + addExpressionStmtProperties(g, definingType, expressionStmt, isLastMethod); + + // Create new MethodCallExpression for the synthetic static block method + final Vertex staticBlockMethodCall = + g.addV(ASTConstants.NodeType.METHOD_CALL_EXPRESSION).next(); + addMethodCallExpressionProperties( + g, definingType, staticBlockMethod, staticBlockMethodCall); + GremlinVertexUtil.addParentChildRelationship(g, expressionStmt, staticBlockMethodCall); + + // Create EmptyReferenceExpression inside MethodCallExpression + final Vertex emptyMethodReference = + g.addV(ASTConstants.NodeType.EMPTY_REFERENCE_EXPRESSION).next(); + addEmptyMethodReferenceProperties(g, definingType, emptyMethodReference); + GremlinVertexUtil.addParentChildRelationship( + g, staticBlockMethodCall, emptyMethodReference); + return expressionStmt; + } + + private static void addExpressionStmtProperties( + GraphTraversalSource g, + String definingType, + Vertex expressionStmt, + boolean isLastMethod) { + verifyType(expressionStmt, ASTConstants.NodeType.EXPRESSION_STATEMENT); + + final Map properties = new HashMap<>(); + properties.put(Schema.IS_SYNTHETIC, true); + properties.put(Schema.DEFINING_TYPE, definingType); + properties.put(Schema.FIRST_CHILD, true); + properties.put(Schema.LAST_CHILD, true); + properties.put(Schema.CHILD_INDEX, 0); + + addProperties(g, expressionStmt, properties); + } + + private static void addEmptyMethodReferenceProperties( + GraphTraversalSource g, String definingType, Vertex emptyMethodReference) { + verifyType(emptyMethodReference, ASTConstants.NodeType.EMPTY_REFERENCE_EXPRESSION); + + final Map properties = new HashMap<>(); + properties.put(Schema.IS_SYNTHETIC, true); + properties.put(Schema.DEFINING_TYPE, definingType); + properties.put(Schema.FIRST_CHILD, true); + properties.put(Schema.LAST_CHILD, true); + properties.put(Schema.CHILD_INDEX, 0); + + addProperties(g, emptyMethodReference, properties); + } + + private static void addMethodCallExpressionProperties( + GraphTraversalSource g, + String definingType, + Vertex staticBlockMethod, + Vertex staticBlockMethodCall) { + verifyType(staticBlockMethodCall, ASTConstants.NodeType.METHOD_CALL_EXPRESSION); + + final Map properties = new HashMap<>(); + properties.put(Schema.IS_SYNTHETIC, true); + properties.put(Schema.DEFINING_TYPE, definingType); + properties.put(Schema.METHOD_NAME, staticBlockMethod.value(Schema.NAME)); + properties.put(Schema.FULL_METHOD_NAME, staticBlockMethod.value(Schema.NAME)); + properties.put(Schema.IS_STATIC_BLOCK_INVOCATION, true); + properties.put(Schema.FIRST_CHILD, true); + properties.put(Schema.LAST_CHILD, true); + properties.put(Schema.CHILD_INDEX, 0); + + addProperties(g, staticBlockMethodCall, properties); + } + + private static void addBlockStatementProperties( + GraphTraversalSource g, String definingType, Vertex blockStatementInInvoker) { + verifyType(blockStatementInInvoker, ASTConstants.NodeType.BLOCK_STATEMENT); + + final Map properties = new HashMap<>(); + properties.put(Schema.IS_SYNTHETIC, true); + properties.put(Schema.DEFINING_TYPE, definingType); + properties.put(Schema.CHILD_INDEX, 1); + properties.put(Schema.FIRST_CHILD, false); + properties.put(Schema.LAST_CHILD, true); + + addProperties(g, blockStatementInInvoker, properties); + } + + private static void addStaticModifierProperties( + GraphTraversalSource g, String definingType, Vertex modifier) { + verifyType(modifier, ASTConstants.NodeType.MODIFIER_NODE); + + final Map properties = new HashMap<>(); + properties.put(Schema.IS_SYNTHETIC, true); + properties.put(Schema.DEFINING_TYPE, definingType); + properties.put(Schema.STATIC, true); + properties.put(Schema.ABSTRACT, false); + properties.put(Schema.GLOBAL, false); + properties.put(Schema.MODIFIERS, 8); // Apparently, static methods have modifiers 8 + properties.put(Schema.FIRST_CHILD, true); + properties.put(Schema.LAST_CHILD, false); + properties.put(Schema.CHILD_INDEX, 0); + + addProperties(g, modifier, properties); + } + + private static void addStaticBlockInvokerProperties( + GraphTraversalSource g, + String definingType, + Vertex staticBlockInvokerVertex, + int childIndex) { + verifyType(staticBlockInvokerVertex, ASTConstants.NodeType.METHOD); + final Map properties = new HashMap<>(); + properties.put(Schema.NAME, STATIC_BLOCK_INVOKER_METHOD); + properties.put(Schema.IS_STATIC_BLOCK_INVOKER_METHOD, true); + properties.put(Schema.CHILD_INDEX, childIndex); + addCommonSynthMethodProperties(g, definingType, staticBlockInvokerVertex, properties); + } + + private static void addSyntheticStaticBlockMethodProperties( + GraphTraversalSource g, + String definingType, + Vertex syntheticMethodVertex, + int staticBlockIndex, + int childIndex) { + verifyType(syntheticMethodVertex, ASTConstants.NodeType.METHOD); + final Map properties = new HashMap<>(); + properties.put( + Schema.NAME, String.format(SYNTHETIC_STATIC_BLOCK_METHOD_NAME, staticBlockIndex)); + properties.put(Schema.IS_STATIC_BLOCK_METHOD, true); + properties.put(Schema.CHILD_INDEX, childIndex); + + addCommonSynthMethodProperties(g, definingType, syntheticMethodVertex, properties); + } + + private static void addCommonSynthMethodProperties( + GraphTraversalSource g, + String definingType, + Vertex staticBlockInvokerVertex, + Map properties) { + properties.put(Schema.ARITY, 0); + properties.put(Schema.CONSTRUCTOR, false); + properties.put(Schema.DEFINING_TYPE, definingType); + properties.put(Schema.IS_SYNTHETIC, true); + properties.put(Schema.RETURN_TYPE, ASTConstants.TYPE_VOID); + + addProperties(g, staticBlockInvokerVertex, properties); + } + + private static void addProperties( + GraphTraversalSource g, Vertex vertex, Map properties) { + final TreeSet previouslyInsertedKeys = CollectionUtil.newTreeSet(); + final GraphTraversal traversal = g.V(vertex.id()); + + for (Map.Entry entry : properties.entrySet()) { + GremlinVertexUtil.addProperty( + previouslyInsertedKeys, traversal, entry.getKey(), entry.getValue()); + previouslyInsertedKeys.add(entry.getKey()); + } + + // Commit the changes. + traversal.next(); + } + + /** @return true if given node represents () */ + private static boolean isClinitMethod(JorjeNode node) { + return ASTConstants.NodeType.METHOD.equals(node.getLabel()) + && MethodUtil.STATIC_CONSTRUCTOR_CANONICAL_NAME.equals( + node.getProperties().get(Schema.NAME)); + } + + /** @return true if () node contains any static block definitions */ + private static boolean containsStaticBlock(JorjeNode node) { + for (JorjeNode childNode : node.getChildren()) { + if (ASTConstants.NodeType.BLOCK_STATEMENT.equals(childNode.getLabel())) { + return true; + } + } + return false; + } + + private static List getStaticBlockMethods(GraphTraversalSource g, Vertex root) { + return g.V(root) + .out(Schema.CHILD) + .hasLabel(ASTConstants.NodeType.METHOD) + .has(Schema.IS_STATIC_BLOCK_METHOD, true) + .toList(); + } + + private static boolean isBlockStatement(JorjeNode child) { + return ASTConstants.NodeType.BLOCK_STATEMENT.equals(child.getLabel()); + } + + private static void verifyType(Vertex vertex, String expectedType) { + if (!expectedType.equals(vertex.label())) { + throw new ProgrammingException("Incorrect vertex type: " + vertex.label()); + } + } +} diff --git a/sfge/src/main/java/com/salesforce/graph/build/Util.java b/sfge/src/main/java/com/salesforce/graph/build/Util.java index cb0854ecf..b0c6b5d37 100644 --- a/sfge/src/main/java/com/salesforce/graph/build/Util.java +++ b/sfge/src/main/java/com/salesforce/graph/build/Util.java @@ -23,7 +23,7 @@ public static void buildGraph(Config config) { new GraphBuilder[] { new ApexStandardLibraryVertexBuilder(config.g), new CustomerApexVertexBuilder(config.g, config.customerCompilations), - new InheritanceEdgeBuilder(config.g) + new InheritanceInformationBuilder(config.g) }) { if (config.ignoreBuilders.contains(graphBuilder.getClass())) { if (LOGGER.isWarnEnabled()) { 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 5e6720e96..aaad1de86 100644 --- a/sfge/src/main/java/com/salesforce/graph/ops/MethodUtil.java +++ b/sfge/src/main/java/com/salesforce/graph/ops/MethodUtil.java @@ -81,6 +81,9 @@ public final class MethodUtil { private static final Logger LOGGER = LogManager.getLogger(MethodUtil.class); private static final String PAGE_REFERENCE = "PageReference"; + private static final String INBOUND_EMAIL_HANDLER = "Messaging.InboundEmailHandler"; + private static final String HANDLE_INBOUND_EMAIL = "handleInboundEmail"; + private static final String INBOUND_EMAIL_RESULT = "Messaging.InboundEmailResult"; public static final String INSTANCE_CONSTRUCTOR_CANONICAL_NAME = ""; public static final String STATIC_CONSTRUCTOR_CANONICAL_NAME = ""; @@ -165,24 +168,46 @@ private static void addMessagesForTarget(RuleRunnerTarget target, List getAuraEnabledMethods( GraphTraversalSource g, List targetFiles) { - // Only look at UserClass vertices. Not interested in Enums, Interfaces, or Triggers - final String[] labels = new String[] {NodeType.USER_CLASS}; + return getMethodsWithAnnotation(g, targetFiles, Schema.AURA_ENABLED); + } + + /** + * Returns non-test methods in the target files with a @NamespaceAccessible annotation. An empty + * list implicitly includes all files. + */ + public static List getNamespaceAccessibleMethods( + GraphTraversalSource g, List targetFiles) { + return getMethodsWithAnnotation(g, targetFiles, Schema.NAMESPACE_ACCESSIBLE); + } + + /** + * 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) { + return getMethodsWithAnnotation(g, targetFiles, Schema.REMOTE_ACTION); + } + + /** + * Returns non-test methods in the target files with an @InvocableMethod annotation. An empty + * list implicitly includes all files. + */ + public static List getInvocableMethodMethods( + GraphTraversalSource g, List targetFiles) { + return getMethodsWithAnnotation(g, targetFiles, Schema.INVOCABLE_METHOD); + } + + static List getMethodsWithAnnotation( + GraphTraversalSource g, List targetFiles, String annotation) { return SFVertexFactory.loadVertices( g, - TraversalUtil.fileRootTraversal(g, labels, targetFiles) - .not(has(Schema.IS_TEST, true)) - .repeat(__.out(Schema.CHILD)) - .until(__.hasLabel(NodeType.METHOD)) - .not(has(Schema.IS_TEST, true)) + rootMethodTraversal(g, targetFiles) .where( out(Schema.CHILD) .hasLabel(NodeType.MODIFIER_NODE) .out(Schema.CHILD) - .where( - H.has( - NodeType.ANNOTATION, - Schema.NAME, - Schema.AURA_ENABLED))) + .where(H.has(NodeType.ANNOTATION, Schema.NAME, annotation))) .order(Scope.global) .by(Schema.DEFINING_TYPE, Order.asc) .by(Schema.NAME, Order.asc)); @@ -194,21 +219,69 @@ public static List getAuraEnabledMethods( */ public static List getPageReferenceMethods( GraphTraversalSource g, List targetFiles) { - // Only look at UserClass vertices. Not interested in Enums, Interfaces, or Triggers - final String[] labels = new String[] {NodeType.USER_CLASS}; return SFVertexFactory.loadVertices( g, - TraversalUtil.fileRootTraversal(g, labels, targetFiles) - .not(has(Schema.IS_TEST, true)) - .repeat(__.out(Schema.CHILD)) - .until(__.hasLabel(NodeType.METHOD)) - .not(has(Schema.IS_TEST, true)) + rootMethodTraversal(g, targetFiles) .where(H.has(NodeType.METHOD, Schema.RETURN_TYPE, PAGE_REFERENCE)) .order(Scope.global) .by(Schema.DEFINING_TYPE, Order.asc) .by(Schema.NAME, Order.asc)); } + private static GraphTraversal rootMethodTraversal( + GraphTraversalSource g, List targetFiles) { + // Only look at UserClass vertices. Not interested in Enums, Interfaces, or Triggers + final String[] labels = new String[] {NodeType.USER_CLASS}; + return TraversalUtil.fileRootTraversal(g, labels, targetFiles) + .not(has(Schema.IS_TEST, true)) + .repeat(__.out(Schema.CHILD)) + .until(__.hasLabel(NodeType.METHOD)) + .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. + 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))))); + } + + public static List getInboundEmailHandlerMethods( + GraphTraversalSource g, List targetFiles) { + return SFVertexFactory.loadVertices( + g, + // Get any target class that implements the email handler interface. + TraversalUtil.traverseImplementationsOf(g, targetFiles, INBOUND_EMAIL_HANDLER) + // Get every implementation of the handle email method. + .out(Schema.CHILD) + .where(H.has(NodeType.METHOD, Schema.NAME, HANDLE_INBOUND_EMAIL)) + // Filter the results by return type and arity to limit the possibility of + // getting unnecessary results. + .where(H.has(NodeType.METHOD, Schema.RETURN_TYPE, INBOUND_EMAIL_RESULT)) + .has(Schema.ARITY, 2)); + } + /** * 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/graph/ops/TraversalUtil.java b/sfge/src/main/java/com/salesforce/graph/ops/TraversalUtil.java index bdc0e831b..8c98aa4fa 100644 --- a/sfge/src/main/java/com/salesforce/graph/ops/TraversalUtil.java +++ b/sfge/src/main/java/com/salesforce/graph/ops/TraversalUtil.java @@ -1,7 +1,9 @@ package com.salesforce.graph.ops; import com.salesforce.apex.jorje.ASTConstants; +import com.salesforce.apex.jorje.ASTConstants.NodeType; import com.salesforce.graph.Schema; +import com.salesforce.graph.build.CaseSafePropertyUtil.H; import com.salesforce.rules.AbstractRuleRunner.RuleRunnerTarget; import java.util.List; import java.util.stream.Collectors; @@ -68,5 +70,70 @@ public static GraphTraversal ruleTargetTraversal( .toArray(GraphTraversal[]::new)); } + /** + * Returns a traversal containing every class in the target files that implements the specified + * interface, either directly or indirectly (i.e., by extending a class that implements it, + * implementing an interface that extends it, or extending a class that does either of those + * things). An empty target array implicitly targets the whole graph. + */ + public static GraphTraversal traverseImplementationsOf( + GraphTraversalSource g, List targetFiles, String interfaceName) { + // For our traversal, we want to start with every class that implements either the target + // interface or one of its + // subtypes. Do that with a union of these two subtraversals. + // Also, start with the hasLabel() call, because doing an initial filter along an indexed + // field saves us a ton of time. + GraphTraversal traversal = + g.V().hasLabel(NodeType.USER_CLASS, NodeType.USER_INTERFACE) + .union( + // Subtraversal 1: Get every class that implements the target + // interface, via a helper method. + __.where( + H.hasArrayContaining( + NodeType.USER_CLASS, + Schema.INTERFACE_DEFINING_TYPES, + interfaceName)), + // Subtraversal 2: Get every class that implements a subtype of the + // target interface, by starting with all + // the direct subtypes of the interface... + __.where( + H.has( + NodeType.USER_INTERFACE, + Schema.SUPER_INTERFACE_NAME, + interfaceName)) + .union( + __.identity(), + // ...recursively adding subtypes of those + // interfaces... + __.repeat(__.out(Schema.EXTENDED_BY)).emit()) + // ... and getting every class that implements one of those + // interfaces. + .out(Schema.IMPLEMENTED_BY) + // Now, we add every subclass of any of these classes, recursively. + ) + .union(__.identity(), __.repeat(__.out(Schema.EXTENDED_BY)).emit()); + + // If there are no target files, we're clear to return this traversal since it includes all + // relevant classes in + // the graph. + if (targetFiles.isEmpty()) { + return traversal; + } else { + // Otherwise, we need to filter so we're only returning the classes in a target file. To + // do that, do a traversal + // to get all the IDs of classes defined in the target files. + Object[] targetIds = + fileRootTraversal(g, targetFiles) + .union(__.identity(), __.repeat(__.out(Schema.CHILD)).emit()) + .hasLabel(NodeType.USER_CLASS) + .id() + .toList() + .toArray(); + // Note, there's no .hasId(Object...) overload, so we're using the .hasId(Object, + // ...Object) overload instead. + return traversal.hasId(targetIds); + } + } + private TraversalUtil() {} } diff --git a/sfge/src/main/java/com/salesforce/graph/ops/expander/ApexPathExpander.java b/sfge/src/main/java/com/salesforce/graph/ops/expander/ApexPathExpander.java index af23a3009..f1dbb9665 100644 --- a/sfge/src/main/java/com/salesforce/graph/ops/expander/ApexPathExpander.java +++ b/sfge/src/main/java/com/salesforce/graph/ops/expander/ApexPathExpander.java @@ -150,6 +150,12 @@ final class ApexPathExpander implements ClassStaticScopeProvider, EngineDirectiv */ private final TreeSet currentlyInitializingStaticClasses; + /** + * Track classes whose static scopes have been initialized. This would help double visiting some + * nodes. + */ + private final TreeSet alreadyInitializedStaticClasses; + /** The symbol provider that corresponds to the {@link #topMostPath} */ private /*finalTODO Add clear method to SymbolProviderVertexVisitor*/ SymbolProviderVertexVisitor symbolProviderVisitor; @@ -195,6 +201,7 @@ final class ApexPathExpander implements ClassStaticScopeProvider, EngineDirectiv this.classStaticScopes = CollectionUtil.newTreeMap(); this.engineDirectiveContext = new EngineDirectiveContext(); this.currentlyInitializingStaticClasses = CollectionUtil.newTreeSet(); + this.alreadyInitializedStaticClasses = CollectionUtil.newTreeSet(); } /** @@ -272,6 +279,8 @@ final class ApexPathExpander implements ClassStaticScopeProvider, EngineDirectiv this.startScope = (SymbolProvider) CloneUtil.clone((DeepCloneable) other.startScope); this.currentlyInitializingStaticClasses = CloneUtil.cloneTreeSet(other.currentlyInitializingStaticClasses); + this.alreadyInitializedStaticClasses = + CloneUtil.cloneTreeSet(other.alreadyInitializedStaticClasses); } /** @@ -370,15 +379,18 @@ void initializeClassStaticScope(String className) apexPath = getTopMostPath().getStaticInitializationPath(className).get(); } - if (!classStaticScope.getState().equals(AbstractClassScope.State.INITIALIZED)) { + if (!classStaticScope.getState().equals(AbstractClassScope.State.INITIALIZED) + && !alreadyInitializedStaticClasses.contains(fullClassName)) { if (apexPath != null && apexPath.getCollectible() != null) { visit(apexPath.getCollectible()); } symbolProviderVisitor.popScope(classStaticScope); classStaticScope.setState(AbstractClassScope.State.INITIALIZED); + alreadyInitializedStaticClasses.add(fullClassName); } } finally { - if (!currentlyInitializingStaticClasses.remove(fullClassName)) { + if (!currentlyInitializingStaticClasses.remove(fullClassName) + && alreadyInitializedStaticClasses.contains(fullClassName)) { throw new ProgrammingException( "Set did not contain class. values=" + currentlyInitializingStaticClasses); diff --git a/sfge/src/main/java/com/salesforce/graph/symbols/ClassStaticScope.java b/sfge/src/main/java/com/salesforce/graph/symbols/ClassStaticScope.java index 940f23704..df046f8bf 100644 --- a/sfge/src/main/java/com/salesforce/graph/symbols/ClassStaticScope.java +++ b/sfge/src/main/java/com/salesforce/graph/symbols/ClassStaticScope.java @@ -2,6 +2,10 @@ import static com.salesforce.apex.jorje.ASTConstants.NodeType; +import com.salesforce.Collectible; +import com.salesforce.apex.jorje.ASTConstants; +import com.salesforce.collections.CollectionUtil; +import com.salesforce.exception.ProgrammingException; import com.salesforce.exception.UnexpectedException; import com.salesforce.graph.ApexPath; import com.salesforce.graph.DeepCloneable; @@ -10,15 +14,20 @@ import com.salesforce.graph.ops.MethodUtil; import com.salesforce.graph.symbols.apex.ApexValue; import com.salesforce.graph.vertex.BaseSFVertex; +import com.salesforce.graph.vertex.BlockStatementVertex; import com.salesforce.graph.vertex.FieldDeclarationVertex; +import com.salesforce.graph.vertex.MethodCallExpressionVertex; +import com.salesforce.graph.vertex.MethodVertex; import com.salesforce.graph.vertex.SFVertexFactory; import com.salesforce.graph.vertex.UserClassVertex; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Optional; import org.apache.tinkerpop.gremlin.process.traversal.Order; import org.apache.tinkerpop.gremlin.process.traversal.Scope; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.apache.tinkerpop.gremlin.structure.T; /** Used when invoking a static method on a class. */ public final class ClassStaticScope extends AbstractClassScope @@ -105,7 +114,37 @@ private static List getFieldDeclarations( return results; } - /** + private static List getStaticBlocks( + GraphTraversalSource g, UserClassVertex userClass) { + List results = new ArrayList<>(); + + String superClassName = userClass.getSuperClassName().orElse(null); + if (superClassName != null) { + UserClassVertex superClass = ClassUtil.getUserClass(g, superClassName).orElse(null); + if (superClass != null) { + results.addAll(getStaticBlocks(g, superClass)); + } + } + + results.addAll(SFVertexFactory.loadVertices( + g, + g.V(userClass.getId()) + .out(Schema.CHILD) + .hasLabel(NodeType.METHOD) + .has(Schema.IS_STATIC_BLOCK_INVOKER_METHOD, true) + .out(Schema.CHILD) + .hasLabel(ASTConstants.NodeType.BLOCK_STATEMENT) + .order(Scope.global) + .by(Schema.CHILD_INDEX, Order.asc) + .out(Schema.CHILD) + .hasLabel(NodeType.EXPRESSION_STATEMENT) + .out(Schema.CHILD) + .hasLabel(NodeType.METHOD_CALL_EXPRESSION))); + + return results; + } + + /** * Returns a path that represents the static properties defined by the class. The following * example would contain a path for 'MyClass' that contains the Field and FieldDeclarations for * 's'. {@code @@ -121,9 +160,13 @@ private static List getFieldDeclarations( public static Optional getInitializationPath( GraphTraversalSource g, String classname) { ClassStaticScope classStaticScope = ClassStaticScope.get(g, classname); + if (classStaticScope.getState().equals(State.INITIALIZED)) { + throw new ProgrammingException("Initialization path does not need to be invoked on a class that's already initialized: " + classStaticScope.getClassName()); + } List vertices = new ArrayList<>(); vertices.addAll(classStaticScope.getFields()); vertices.addAll(getFieldDeclarations(g, classStaticScope.userClass)); + vertices.addAll(getStaticBlocks(g, classStaticScope.userClass)); if (vertices.isEmpty()) { return Optional.empty(); } else { @@ -132,4 +175,5 @@ public static Optional getInitializationPath( return Optional.of(apexPath); } } + } diff --git a/sfge/src/main/java/com/salesforce/rules/AbstractRule.java b/sfge/src/main/java/com/salesforce/rules/AbstractRule.java index 90dd7406d..d7a545914 100644 --- a/sfge/src/main/java/com/salesforce/rules/AbstractRule.java +++ b/sfge/src/main/java/com/salesforce/rules/AbstractRule.java @@ -48,6 +48,11 @@ public Descriptor getDescriptor() { // probably un-abstract this method. protected abstract String getCategory(); + protected boolean isEnabled() { + // By default, every rule is disabled, unless specifically enabled + return false; + } + /** * Unless the rule has a predetermined URL, we'll return a link to information about the engine. */ diff --git a/sfge/src/main/java/com/salesforce/rules/ApexFlsViolationRule.java b/sfge/src/main/java/com/salesforce/rules/ApexFlsViolationRule.java index 87f28b8fb..21ecc1553 100644 --- a/sfge/src/main/java/com/salesforce/rules/ApexFlsViolationRule.java +++ b/sfge/src/main/java/com/salesforce/rules/ApexFlsViolationRule.java @@ -48,6 +48,11 @@ protected String getUrl() { return URL; } + @Override + protected boolean isEnabled() { + return true; + } + @Override protected List _run(GraphTraversalSource g, ApexPath path, BaseSFVertex vertex) { final HashSet flsViolationInfos = new HashSet<>(); diff --git a/sfge/src/main/java/com/salesforce/rules/RuleUtil.java b/sfge/src/main/java/com/salesforce/rules/RuleUtil.java index 82a78bdb9..358b70f4e 100644 --- a/sfge/src/main/java/com/salesforce/rules/RuleUtil.java +++ b/sfge/src/main/java/com/salesforce/rules/RuleUtil.java @@ -59,8 +59,18 @@ public static List getPathEntryPoints( if (!fileLevelTargets.isEmpty() || targets.isEmpty()) { // Use the file-level targets to get aura-enabled methods... methods.addAll(MethodUtil.getAuraEnabledMethods(g, fileLevelTargets)); + // ...and NamespaceAccessible methods... + methods.addAll(MethodUtil.getNamespaceAccessibleMethods(g, fileLevelTargets)); + // ...and RemoteAction methods... + methods.addAll(MethodUtil.getRemoteActionMethods(g, fileLevelTargets)); + // ...and InvocableMethod methods... + methods.addAll(MethodUtil.getInvocableMethodMethods(g, fileLevelTargets)); // ...and PageReference methods... methods.addAll(MethodUtil.getPageReferenceMethods(g, fileLevelTargets)); + // ...and global-exposed methods... + methods.addAll(MethodUtil.getGlobalMethods(g, fileLevelTargets)); + // ...and implementations of Messaging.InboundEmailHandler#handleInboundEmail... + methods.addAll(MethodUtil.getInboundEmailHandlerMethods(g, fileLevelTargets)); // ...and exposed methods on VF controllers. methods.addAll(MethodUtil.getExposedControllerMethods(g, fileLevelTargets)); } @@ -73,7 +83,12 @@ public static List getPathEntryPoints( return new ArrayList<>(methods); } - public static List getAllRules() throws RuleNotFoundException { + public static List getEnabledRules() throws RuleNotFoundException { + final List allRules = getAllRules(); + return allRules.stream().filter(rule -> rule.isEnabled()).collect(Collectors.toList()); + } + + static List getAllRules() throws RuleNotFoundException { // Get a set of every class in the Rules package that extends AbstractRule. Reflections reflections = new Reflections(PackageConstants.RULES_PACKAGE); Set> ruleTypes = @@ -84,7 +99,8 @@ public static List getAllRules() throws RuleNotFoundException { for (Class ruleType : ruleTypes) { // Skip abstract classes. if (!Modifier.isAbstract(ruleType.getModifiers())) { - rules.add(getRuleInner(ruleType.getName())); + final AbstractRule rule = getRuleInner(ruleType.getName()); + rules.add(rule); } } return rules; diff --git a/sfge/src/test/java/com/salesforce/graph/build/CustomerApexVertexBuilderTest.java b/sfge/src/test/java/com/salesforce/graph/build/CustomerApexVertexBuilderTest.java index 5c63f768a..b568e8370 100644 --- a/sfge/src/test/java/com/salesforce/graph/build/CustomerApexVertexBuilderTest.java +++ b/sfge/src/test/java/com/salesforce/graph/build/CustomerApexVertexBuilderTest.java @@ -13,7 +13,6 @@ import com.salesforce.graph.ops.MethodUtil; import com.salesforce.graph.vertex.MethodVertex; import com.salesforce.graph.vertex.SFVertexFactory; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -117,11 +116,11 @@ public void testCaseSafeProperties() { Schema.SUPER_CLASS_NAME + CaseSafePropertyUtil.CASE_SAFE_SUFFIX)); assertEquals( "someinterface", - ((ArrayList) + ((Object[]) userClassVertex.get( Schema.INTERFACE_NAMES + CaseSafePropertyUtil.CASE_SAFE_SUFFIX)) - .get(0)); + [0]); Map userInterfaceVertex = g.V().hasLabel(NodeType.USER_INTERFACE).elementMap().next(); diff --git a/sfge/src/test/java/com/salesforce/graph/build/GraphBuildTestUtil.java b/sfge/src/test/java/com/salesforce/graph/build/GraphBuildTestUtil.java new file mode 100644 index 000000000..07cc97edf --- /dev/null +++ b/sfge/src/test/java/com/salesforce/graph/build/GraphBuildTestUtil.java @@ -0,0 +1,61 @@ +package com.salesforce.graph.build; + +import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.has; + +import com.salesforce.apex.jorje.ASTConstants; +import com.salesforce.apex.jorje.JorjeUtil; +import com.salesforce.graph.ApexPath; +import com.salesforce.graph.Schema; +import com.salesforce.graph.cache.VertexCacheProvider; +import com.salesforce.graph.ops.ApexPathUtil; +import com.salesforce.graph.symbols.DefaultSymbolProviderVertexVisitor; +import com.salesforce.graph.vertex.MethodVertex; +import com.salesforce.graph.vertex.SFVertexFactory; +import com.salesforce.graph.visitor.ApexPathWalker; +import com.salesforce.graph.visitor.DefaultNoOpPathVertexVisitor; +import com.salesforce.graph.visitor.PathVertexVisitor; +import java.util.ArrayList; +import java.util.List; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; + +public class GraphBuildTestUtil { + static void buildGraph(GraphTraversalSource g, String sourceCode) { + buildGraph(g, new String[] {sourceCode}); + } + + static void buildGraph(GraphTraversalSource g, String[] sourceCodes) { + List compilations = new ArrayList(); + for (int i = 0; i < sourceCodes.length; i++) { + compilations.add( + new Util.CompilationDescriptor( + "TestCode" + i, JorjeUtil.compileApexFromString(sourceCodes[i]))); + } + + VertexCacheProvider.get().initialize(g); + CustomerApexVertexBuilder customerApexVertexBuilder = + new CustomerApexVertexBuilder(g, compilations); + + for (GraphBuilder graphBuilder : new GraphBuilder[] {customerApexVertexBuilder}) { + graphBuilder.build(); + } + } + + /** Sanity method to walk all paths. Helps to ensure all of the push/pops are correct */ + static List walkAllPaths(GraphTraversalSource g, String methodName) { + MethodVertex methodVertex = + SFVertexFactory.load( + g, + g.V().hasLabel(ASTConstants.NodeType.METHOD) + .has(Schema.NAME, methodName) + .not(has(Schema.IS_STANDARD, true))); + List paths = ApexPathUtil.getForwardPaths(g, methodVertex); + + for (ApexPath path : paths) { + DefaultSymbolProviderVertexVisitor symbols = new DefaultSymbolProviderVertexVisitor(g); + PathVertexVisitor visitor = new DefaultNoOpPathVertexVisitor(); + ApexPathWalker.walkPath(g, path, visitor, symbols); + } + + return paths; + } +} diff --git a/sfge/src/test/java/com/salesforce/graph/build/InheritanceEdgeBuilderTest.java b/sfge/src/test/java/com/salesforce/graph/build/InheritanceEdgeBuilderTest.java deleted file mode 100644 index 2f9bcc137..000000000 --- a/sfge/src/test/java/com/salesforce/graph/build/InheritanceEdgeBuilderTest.java +++ /dev/null @@ -1,35 +0,0 @@ -package com.salesforce.graph.build; - -import com.salesforce.TestUtil; -import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; - -public class InheritanceEdgeBuilderTest { - private GraphTraversalSource g; - - @BeforeEach - public void setup() { - this.g = TestUtil.getGraph(); - } - - @Test - public void testSimpleExtension() { - // TODO - } - - @Test - public void testInnerTypeExtension() { - // TODO - } - - @Test - public void testSimpleImplementation() { - // TODO - } - - @Test - public void testInnerTypeImplementation() { - // TODO - } -} diff --git a/sfge/src/test/java/com/salesforce/graph/build/InheritanceInformationBuilderTest.java b/sfge/src/test/java/com/salesforce/graph/build/InheritanceInformationBuilderTest.java new file mode 100644 index 000000000..4213fbc92 --- /dev/null +++ b/sfge/src/test/java/com/salesforce/graph/build/InheritanceInformationBuilderTest.java @@ -0,0 +1,343 @@ +package com.salesforce.graph.build; + +import static org.hamcrest.Matchers.arrayContaining; +import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.core.IsEqual.equalTo; + +import com.salesforce.TestUtil; +import com.salesforce.apex.jorje.ASTConstants.NodeType; +import com.salesforce.graph.Schema; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import org.apache.tinkerpop.gremlin.process.traversal.Order; +import org.apache.tinkerpop.gremlin.process.traversal.Scope; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.apache.tinkerpop.gremlin.structure.Vertex; +import org.hamcrest.MatcherAssert; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class InheritanceInformationBuilderTest { + private GraphTraversalSource g; + + @BeforeEach + public void setup() { + this.g = TestUtil.getGraph(); + } + + @Test + public void testNoInheritance() { + String[] sourceCodes = {"public class c1 {}", "public interface i1 {}"}; + + // Build the graph. + // TODO: Ideally, this would call the edge builder directly against an incomplete graph. + TestUtil.buildGraph(g, sourceCodes); + + // Verify that nothing has any inheritance edges. + verifyNoEdges(g, Schema.IMPLEMENTED_BY); + verifyNoEdges(g, Schema.IMPLEMENTATION_OF); + verifyNoEdges(g, Schema.EXTENDED_BY); + verifyNoEdges(g, Schema.EXTENSION_OF); + } + + @Test + public void testSimpleClassExtension() { + String[] sourceCodes = { + "public class c1 {}", "public class c2 extends c1{}", + }; + + // Build the graph. + // TODO: Ideally, this would call the edge builder directly against an incomplete graph. + TestUtil.buildGraph(g, sourceCodes); + + // Verify that the extension looks right. + verifyExtension(g, "c1", "c2"); + + // Verify that there are no implementation edges. + verifyNoEdges(g, Schema.IMPLEMENTED_BY); + verifyNoEdges(g, Schema.IMPLEMENTATION_OF); + } + + @Test + public void testSimpleInterfaceExtension() { + String[] sourceCodes = { + "public interface i1 {}", "public interface i2 extends i1{}", + }; + + // Build the graph. + // TODO: Ideally, this would call the edge builder directly against an incomplete graph. + TestUtil.buildGraph(g, sourceCodes); + + // Verify that the extension looks right. + verifyExtension(g, "i1", "i2"); + + // Verify that there are no implementation edges. + verifyNoEdges(g, Schema.IMPLEMENTED_BY); + verifyNoEdges(g, Schema.IMPLEMENTATION_OF); + } + + @Test + public void testSimpleImplementation() { + String[] sourceCodes = { + "public interface i1 {}", "public class c1 implements i1{}", + }; + + // Build the graph. + // TODO: Ideally, this would call the edge builder directly against an incomplete graph. + TestUtil.buildGraph(g, sourceCodes); + + // Verify that the implementations look right. + verifyImplementation(g, "i1", "c1"); + + // Verify that there are no extension edges. + verifyNoEdges(g, Schema.EXTENDED_BY); + verifyNoEdges(g, Schema.EXTENSION_OF); + } + + @Test + public void testImplementationOfExternalInterface() { + String[] sourceCodes = {"public class c1 implements ExternallyDeclaredInterface {}"}; + + // Build the graph. + // TODO: Ideally, this would call the edge builder directly against an incomplete graph. + TestUtil.buildGraph(g, sourceCodes); + + // Verify that the right properties were set. + verifyInterfaceDefiningTypeProperty( + g, "ExternallyDeclaredInterface", Collections.singletonList("c1")); + // Verify that no edges were created. + verifyNoEdges(g, Schema.IMPLEMENTED_BY); + verifyNoEdges(g, Schema.IMPLEMENTATION_OF); + verifyNoEdges(g, Schema.EXTENDED_BY); + verifyNoEdges(g, Schema.EXTENSION_OF); + } + + @Test + public void testExtensionOfInnerClass() { + String[] sourceCodes = { + // Extensions in this class don't use the outer class name. + "public class OuterClass1 extends InnerClass1 {\n" + + " public class InnerClass1 {}\n" + + " public class InnerClass2 extends InnerClass1 {}\n" + + "}", + // Extensions in this class use the outer class name. + "public class OuterClass2 extends OuterClass2.InnerClass1 {\n" + + " public class InnerClass1 {}\n" + + " public class InnerClass2 extends InnerClass1 {}\n" + + "}", + // Third class extends one of the inner classes in another class. + "public class OuterClass3 extends OuterClass2.InnerClass1 {}" + }; + + // Build the graph. + // TODO: Ideally, this would call the edge builder directly against an incomplete graph. + TestUtil.buildGraph(g, sourceCodes); + + // Make sure OuterClass1.InnerClass1 is extended by the right classes. + verifyExtension( + g, + "OuterClass1.InnerClass1", + Arrays.asList("OuterClass1", "OuterClass1.InnerClass2")); + + // Make sure OuterClass2.InnerClass1 is extended by the right classes. + verifyExtension( + g, + "OuterClass2.InnerClass1", + Arrays.asList("OuterClass2", "OuterClass2.InnerClass2", "OuterClass3")); + + // Verify that there are no implementation edges. + verifyNoEdges(g, Schema.IMPLEMENTED_BY); + verifyNoEdges(g, Schema.IMPLEMENTATION_OF); + } + + @Test + public void testImplementationOfInnerInterface() { + String[] sourceCodes = { + // Implementations in this class don't use the outer class name. + "public class OuterClass1 implements InnerInterface1 {\n" + + " public class InnerInterface1 {}\n" + + " public class InnerClass1 implements InnerInterface1 {}\n" + + "}", + // Implementations in this class use the outer class name. + "public class OuterClass2 implements OuterClass2.InnerInterface1 {\n" + + " public class InnerInterface1 {}\n" + + " public class InnerClass1 implements InnerInterface1 {}\n" + + "}", + // Third class extends one of the inner classes in another class. + "public class OuterClass3 implements OuterClass2.InnerInterface1 {}" + }; + + // Build the graph. + // TODO: Ideally, this would call the edge builder directly against an incomplete graph. + TestUtil.buildGraph(g, sourceCodes); + + // Make sure OuterClass1.InnerInterface1 is implemented by the right classes. + verifyImplementation( + g, + "OuterClass1.InnerInterface1", + Arrays.asList("OuterClass1", "OuterClass1.InnerClass1")); + + // Make sure OuterClass2.InnerInterface1 is implemented by the right classes. + verifyImplementation( + g, + "OuterClass2.InnerInterface1", + Arrays.asList("OuterClass2", "OuterClass2.InnerClass1", "OuterClass3")); + + // Verify that there are no extension edges. + verifyNoEdges(g, Schema.EXTENDED_BY); + verifyNoEdges(g, Schema.EXTENSION_OF); + } + + @Test + public void testExtensionOfOwnOuterClass() { + String[] sourceCodes = { + "public class OuterClass1 {\n" + + " public class InnerClass1 extends OuterClass1 {}\n" + + "}" + }; + + // Build the graph. + // TODO: Ideally, this would call the edge builder directly against an incomplete graph. + TestUtil.buildGraph(g, sourceCodes); + + // Verify that the right extension edges exist. + verifyExtension(g, "OuterClass1", "OuterClass1.InnerClass1"); + + // Verify that there are no implementation edges. + verifyNoEdges(g, Schema.IMPLEMENTED_BY); + verifyNoEdges(g, Schema.IMPLEMENTATION_OF); + } + + @Test + public void testClassNameOverlap() { + String[] sourceCodes = { + // This class extends its own inner class without using the outer class name. + "public class OuterClass1 extends NameOverlappedClass {\n" + + " public class NameOverlappedClass {}\n" + + "}", + // This class has the same name as the inner class the other class extends. + "public class NameOverlappedClass {}", + // This class extends the outer class with the conflict name. + "public class OuterClass2 extends NameOverlappedClass {}" + }; + + // Build the graph. + // TODO: Ideally, this would call the edge builder directly against an incomplete graph. + TestUtil.buildGraph(g, sourceCodes); + + // Verify that the right extension edges exist. + verifyExtension(g, "NameOverlappedClass", "OuterClass2"); + verifyExtension(g, "OuterClass1.NameOverlappedClass", "OuterClass1"); + + // Verify that there are no implementation edges. + verifyNoEdges(g, Schema.IMPLEMENTED_BY); + verifyNoEdges(g, Schema.IMPLEMENTATION_OF); + } + + @Test + public void testInterfaceNameOverlap() { + String[] sourceCodes = { + // This class implements its own inner interface without using the outer class name. + "public class OuterClass1 implements NameOverlappedInterface {\n" + + " public class NameOverlappedInterface {}\n" + + "}", + // This interface has the same name as the inner interface the other class extends. + "public interface NameOverlappedInterface {}", + // This class implements the outer class with the conflict name. + "public class OuterClass2 implements NameOverlappedInterface {}" + }; + + // Build the graph. + // TODO: Ideally, this would call the edge builder directly against an incomplete graph. + TestUtil.buildGraph(g, sourceCodes); + + // Verify that the right implementation edges exist. + verifyImplementation(g, "NameOverlappedInterface", "OuterClass2"); + verifyImplementation(g, "OuterClass1.NameOverlappedInterface", "OuterClass1"); + + // Verify that there are no extension edges. + verifyNoEdges(g, Schema.EXTENDED_BY); + verifyNoEdges(g, Schema.EXTENSION_OF); + } + + private void verifyNoEdges(GraphTraversalSource g, String edgeName) { + List vertices = g.V().out(edgeName).toList(); + MatcherAssert.assertThat(vertices, hasSize(equalTo(0))); + } + + private void verifyExtension( + GraphTraversalSource g, String parentDefiningType, String childDefiningType) { + verifyExtension(g, parentDefiningType, Collections.singletonList(childDefiningType)); + } + + private void verifyExtension( + GraphTraversalSource g, String parentDefiningType, List childDefiningTypes) { + verifyEdges( + g, Schema.EXTENDED_BY, Schema.EXTENSION_OF, parentDefiningType, childDefiningTypes); + } + + private void verifyImplementation( + GraphTraversalSource g, String parentDefiningType, String childDefiningType) { + verifyImplementation(g, parentDefiningType, Collections.singletonList(childDefiningType)); + } + + private void verifyImplementation( + GraphTraversalSource g, String parentDefiningType, List childDefiningTypes) { + verifyEdges( + g, + Schema.IMPLEMENTED_BY, + Schema.IMPLEMENTATION_OF, + parentDefiningType, + childDefiningTypes); + verifyInterfaceDefiningTypeProperty(g, parentDefiningType, childDefiningTypes); + } + + private void verifyEdges( + GraphTraversalSource g, + String outgoingEdge, + String incomingEdge, + String parentDefiningType, + List childDefiningTypes) { + List outgoingVertices = + g.V().has(Schema.DEFINING_TYPE, parentDefiningType) + .out(outgoingEdge) + .values(Schema.DEFINING_TYPE) + .order(Scope.global) + .by(Order.asc) + .toList(); + List incomingVertices = + g.V().has(Schema.DEFINING_TYPE, parentDefiningType) + .in(incomingEdge) + .values(Schema.DEFINING_TYPE) + .order(Scope.global) + .by(Order.asc) + .toList(); + + // Both lists should have the same vertices in the same order. + MatcherAssert.assertThat(outgoingVertices, hasSize(equalTo(childDefiningTypes.size()))); + MatcherAssert.assertThat(incomingVertices, hasSize(equalTo(childDefiningTypes.size()))); + for (int i = 0; i < childDefiningTypes.size(); i++) { + MatcherAssert.assertThat( + outgoingVertices.get(i).toString(), equalTo(childDefiningTypes.get(i))); + MatcherAssert.assertThat( + incomingVertices.get(i).toString(), equalTo(childDefiningTypes.get(i))); + } + } + + private void verifyInterfaceDefiningTypeProperty( + GraphTraversalSource g, String parentDefiningType, List childDefiningTypes) { + // For each child type, get the property on that vertex. + List propValues = + g.V().where( + CaseSafePropertyUtil.H.hasWithin( + NodeType.USER_CLASS, + Schema.DEFINING_TYPE, + childDefiningTypes)) + .values(Schema.INTERFACE_DEFINING_TYPES) + .toList(); + for (Object propValue : propValues) { + MatcherAssert.assertThat((Object[]) propValue, arrayContaining(parentDefiningType)); + } + } +} diff --git a/sfge/src/test/java/com/salesforce/graph/build/MethodPathBuilderTest.java b/sfge/src/test/java/com/salesforce/graph/build/MethodPathBuilderTest.java index 49698f5da..cbddbcc22 100644 --- a/sfge/src/test/java/com/salesforce/graph/build/MethodPathBuilderTest.java +++ b/sfge/src/test/java/com/salesforce/graph/build/MethodPathBuilderTest.java @@ -3,22 +3,22 @@ import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.has; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.core.IsEqual.equalTo; import static org.hamcrest.core.IsInstanceOf.instanceOf; import static org.hamcrest.core.IsNot.not; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.fail; import com.salesforce.TestUtil; import com.salesforce.apex.jorje.ASTConstants.NodeType; -import com.salesforce.apex.jorje.JorjeUtil; +import com.salesforce.exception.UserActionException; import com.salesforce.graph.ApexPath; import com.salesforce.graph.Schema; -import com.salesforce.graph.cache.VertexCacheProvider; import com.salesforce.graph.ops.ApexPathUtil; -import com.salesforce.graph.symbols.DefaultSymbolProviderVertexVisitor; import com.salesforce.graph.vertex.BaseSFVertex; import com.salesforce.graph.vertex.BlockStatementVertex; import com.salesforce.graph.vertex.CatchBlockStatementVertex; @@ -39,9 +39,6 @@ import com.salesforce.graph.vertex.ValueWhenBlockVertex; import com.salesforce.graph.vertex.VariableDeclarationStatementsVertex; import com.salesforce.graph.vertex.VariableExpressionVertex; -import com.salesforce.graph.visitor.ApexPathWalker; -import com.salesforce.graph.visitor.DefaultNoOpPathVertexVisitor; -import com.salesforce.graph.visitor.PathVertexVisitor; import java.util.ArrayList; import java.util.HashSet; import java.util.List; @@ -57,7 +54,7 @@ import org.junit.jupiter.api.Test; public class MethodPathBuilderTest { - private GraphTraversalSource g; + protected GraphTraversalSource g; private static final String[] BLOCK = new String[] {NodeType.BLOCK_STATEMENT}; @@ -129,7 +126,7 @@ public void testMethodWithSingleExpression() { + " }\n" + "}"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); // // @@ -159,7 +156,7 @@ public void testMethodWithSingleExpression() { MatcherAssert.assertThat(getVerticesWithEndScope(), hasSize(equalTo(1))); assertEndScopes(BLOCK, ExpressionStatementVertex.class, 3); - walkAllPaths("doSomething"); + GraphBuildTestUtil.walkAllPaths(g, "doSomething"); } @Test @@ -175,7 +172,7 @@ public void testMethodWithNestedIfs() { + " }\n" + "}"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); // // // // @@ -437,7 +434,7 @@ public void testMethodWithIfStatement() { // Implicit else assertEndScopes(BLOCK_IF_BLOCK, BlockStatementVertex.class, 3, 3); - walkAllPaths("doSomething"); + GraphBuildTestUtil.walkAllPaths(g, "doSomething"); } @Test @@ -453,7 +450,7 @@ public void testMethodWithSingleIfElseStatement() { + " }\n" + "}"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); // // @@ -542,7 +539,7 @@ public void testMethodWithSingleIfElseStatement() { // System.debug('GoodBye'); assertEndScopes(BLOCK_IF_BLOCK, ExpressionStatementVertex.class, 6); - walkAllPaths("doSomething"); + GraphBuildTestUtil.walkAllPaths(g, "doSomething"); } @Test @@ -560,7 +557,7 @@ public void testMethodWithSingleIf_ElseIf_ElseStatement() { + " }\n" + "}"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); // // @@ -692,7 +689,7 @@ public void testMethodWithSingleIf_ElseIf_ElseStatement() { // System.debug('GoodBye'); assertEndScopes(BLOCK_IF_BLOCK, ExpressionStatementVertex.class, 8); - walkAllPaths("doSomething"); + GraphBuildTestUtil.walkAllPaths(g, "doSomething"); } @Test @@ -712,7 +709,7 @@ public void testMethodWithNestedIfElses() { + " }\n" + "}"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); // // @@ -819,7 +816,7 @@ public void testMethodWithNestedIfElses() { // System.debug('Not Logged'); assertEndScopes(BLOCK_IF_BLOCK, ExpressionStatementVertex.class, 10); - walkAllPaths("doSomething"); + GraphBuildTestUtil.walkAllPaths(g, "doSomething"); } @Test @@ -835,7 +832,7 @@ public void testMethodWithExpressionBeforeAndAfterIf() { + " }\n" + "}"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); // // @@ -946,7 +943,7 @@ public void testMethodWithExpressionBeforeAndAfterIf() { // System.debug('After'); assertEndScopes(BLOCK, ExpressionStatementVertex.class, 7); - walkAllPaths("doSomething"); + GraphBuildTestUtil.walkAllPaths(g, "doSomething"); } @Test @@ -965,7 +962,7 @@ public void testMethodWithInnerIfExpressionBeforeAndAfterIf() { + " }\n" + "}"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); // // @@ -1114,7 +1111,7 @@ public void testMethodWithInnerIfExpressionBeforeAndAfterIf() { // System.debug('After'); assertEndScopes(BLOCK, ExpressionStatementVertex.class, 10); - walkAllPaths("doSomething"); + GraphBuildTestUtil.walkAllPaths(g, "doSomething"); } @Test @@ -1133,7 +1130,7 @@ public void testMethodWithForEach() { + " }\n" + "}"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); // (9 edges) BlockStatement->VariableDeclarationStatements->ForEachStatement // @@ -1185,7 +1182,7 @@ public void testMethodWithForEach() { // System.debug('Not Logged'); assertEndScopes(BLOCK_IF_BLOCK_FOREACH_BLOCK, ExpressionStatementVertex.class, 8); - walkAllPaths("doSomething"); + GraphBuildTestUtil.walkAllPaths(g, "doSomething"); } @Test @@ -1205,7 +1202,7 @@ public void testMethodWithForLoop() { + " }\n" + "}"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); // (10 edges) BlockStatement->VariableDeclarationStatements->ForLoopStatement // @@ -1258,7 +1255,7 @@ public void testMethodWithForLoop() { // System.debug('Not Logged'); assertEndScopes(BLOCK_IF_BLOCK_FORLOOP_BLOCK, ExpressionStatementVertex.class, 9); - walkAllPaths("doSomething"); + GraphBuildTestUtil.walkAllPaths(g, "doSomething"); } /** @@ -1283,7 +1280,7 @@ public void testMethodWithForLoopNoInitializerOrStandardConditionOrIncrementer() + " }\n" + "}"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); // (8 edges) BlockStatement->VariableDeclarationStatements->ForLoopStatement // ->BlockStatement @@ -1335,7 +1332,7 @@ public void testMethodWithForLoopNoInitializerOrStandardConditionOrIncrementer() // System.debug('Not Logged'); assertEndScopes(BLOCK_IF_BLOCK_FORLOOP_BLOCK, ExpressionStatementVertex.class, 10); - walkAllPaths("doSomething"); + GraphBuildTestUtil.walkAllPaths(g, "doSomething"); } /** Tests the case where the for loop doesn't contain an initializer. */ @@ -1357,7 +1354,7 @@ public void testMethodWithForLoopNoInitializer() { + " }\n" + "}"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); // (10 edges) // BlockStatement->VariableDeclarationStatements->ForLoopStatement->StandardCondition->PostfixExpression @@ -1410,7 +1407,7 @@ public void testMethodWithForLoopNoInitializer() { // System.debug('Not Logged'); assertEndScopes(BLOCK_IF_BLOCK_FORLOOP_BLOCK, ExpressionStatementVertex.class, 10); - walkAllPaths("doSomething"); + GraphBuildTestUtil.walkAllPaths(g, "doSomething"); } /** @@ -1435,7 +1432,7 @@ public void testMethodWithForLoopNoInitializerOrStandardCondition() { + " }\n" + "}"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); // (9 edges) // BlockStatement->VariableDeclarationStatements->ForLoopStatement->PostfixExpression @@ -1488,7 +1485,7 @@ public void testMethodWithForLoopNoInitializerOrStandardCondition() { // System.debug('Not Logged'); assertEndScopes(BLOCK_IF_BLOCK_FORLOOP_BLOCK, ExpressionStatementVertex.class, 10); - walkAllPaths("doSomething"); + GraphBuildTestUtil.walkAllPaths(g, "doSomething"); } @Test @@ -1509,7 +1506,7 @@ public void testMethodWithForLoopExpressionBeforeAndAfter() { + " }\n" + "}"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); // (10 edges) BlockStatement->VariableDeclarationStatements->ForLoopStatement // @@ -1591,7 +1588,7 @@ public void testMethodWithForLoopExpressionBeforeAndAfter() { // System.debug('After'); assertEndScopes(BLOCK, ExpressionStatementVertex.class, 12); - walkAllPaths("doSomething"); + GraphBuildTestUtil.walkAllPaths(g, "doSomething"); } @Test @@ -1613,7 +1610,7 @@ public void testMethodWithForLoopExpressionBeforeAndAfterEndsForScope() { + " }\n" + "}"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); MatcherAssert.assertThat(getVerticesWithEndScope(), hasSize(equalTo(4))); @@ -1629,7 +1626,7 @@ public void testMethodWithForLoopExpressionBeforeAndAfterEndsForScope() { // System.debug('After'); assertEndScopes(BLOCK, ExpressionStatementVertex.class, 13); - walkAllPaths("doSomething"); + GraphBuildTestUtil.walkAllPaths(g, "doSomething"); } @Test @@ -1644,7 +1641,7 @@ public void testMethodWithEarlyReturn() { + " }\n" + "}"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); // // // @@ -2046,7 +2043,7 @@ public void testMultipleEarlyReturns() { // insert assertEndScopes(BLOCK, DmlInsertStatementVertex.class, 10); - walkAllPaths("doSomething"); + GraphBuildTestUtil.walkAllPaths(g, "doSomething"); } @Test @@ -2066,7 +2063,7 @@ public void testInsertGuardedByExceptionInOtherMethodWithReturn() { + "}\n" }; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); List paths; MethodVertex methodVertex = TestUtil.getVertexOnLine(g, MethodVertex.class, 2); @@ -2092,7 +2089,7 @@ public void testInsertGuardedByExceptionInOtherMethodWithReturn() { // insert assertEndScopes(BLOCK, ThrowStatementVertex.class, 10); - walkAllPaths("doSomething"); + GraphBuildTestUtil.walkAllPaths(g, "doSomething"); } @Test @@ -2108,7 +2105,7 @@ public void testMethodWithSingleTryCatch() { + " }\n" + "}"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); // // @@ -2322,7 +2319,7 @@ public void testMethodWithSingleTryCatchAndExpressionBeforeAndAfter() { // System.debug('After'); assertEndScopes(BLOCK, ExpressionStatementVertex.class, 9); - walkAllPaths("doSomething"); + GraphBuildTestUtil.walkAllPaths(g, "doSomething"); } @Test @@ -2339,7 +2336,7 @@ public void testIfWithMethodInStandardCondition() { + " }\n" + "}\n"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); // doSomething // @@ -2397,7 +2394,7 @@ public void testIfWithMethodInStandardCondition() { List edges = g.V().outE(Schema.CFG_PATH).toList(); MatcherAssert.assertThat(edges, hasSize(7)); - walkAllPaths("doSomething"); + GraphBuildTestUtil.walkAllPaths(g, "doSomething"); } @Test @@ -2413,7 +2410,7 @@ public void testWhileStatement() { + " }\n" + "}\n"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); // (6 edges) // BlockStatement->VariableDeclarationStatements->WhileLoopStatement->StandardCondition->BlockStatement->ExpressionStatement->ExpressionStatement @@ -2424,7 +2421,7 @@ public void testWhileStatement() { ExpressionStatementVertex.class, 6); - List paths = walkAllPaths("doSomething"); + List paths = GraphBuildTestUtil.walkAllPaths(g, "doSomething"); MatcherAssert.assertThat(paths, hasSize(equalTo(1))); } @@ -2448,9 +2445,9 @@ public void testWhileStatementInOtherMethod() { + " }\n" + "}\n"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); - List paths = walkAllPaths("doSomething"); + List paths = GraphBuildTestUtil.walkAllPaths(g, "doSomething"); MatcherAssert.assertThat(paths, hasSize(equalTo(1))); } @@ -2535,7 +2532,7 @@ public void testEnumSwitchStatement() { + " }\n" + "}\n"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); BlockStatementVertex blockStatementLine2 = TestUtil.getVertexOnLine(g, BlockStatementVertex.class, 2); @@ -2611,7 +2608,7 @@ public void testEnumSwitchStatement() { Pair.of(elseWhenBlock, elseWhenBlockBlockStatement), Pair.of(elseWhenBlockBlockStatement, elseWhenBlockExpressionStatement)); - List paths = walkAllPaths("doSomething"); + List paths = GraphBuildTestUtil.walkAllPaths(g, "doSomething"); MatcherAssert.assertThat(paths, hasSize(equalTo(3))); } @@ -2635,7 +2632,7 @@ public void testIntegerSwitchStatement() { + " }\n" + "}\n"; - buildGraph(g, sourceCode); + GraphBuildTestUtil.buildGraph(g, sourceCode); BlockStatementVertex blockStatementLine2 = TestUtil.getVertexOnLine(g, BlockStatementVertex.class, 2); @@ -2709,7 +2706,7 @@ public void testIntegerSwitchStatement() { Pair.of(elseWhenBlock, elseWhenBlockBlockStatement), Pair.of(elseWhenBlockBlockStatement, elseWhenBlockExpressionStatement)); - List paths = walkAllPaths("doSomething"); + List paths = GraphBuildTestUtil.walkAllPaths(g, "doSomething"); MatcherAssert.assertThat(paths, hasSize(equalTo(3))); } @@ -2736,12 +2733,31 @@ public void testMethodSwitchStatement() { + " }\n" + "}\n"; - buildGraph(g, sourceCode); - List paths = walkAllPaths("doSomething"); + GraphBuildTestUtil.buildGraph(g, sourceCode); + List paths = GraphBuildTestUtil.walkAllPaths(g, "doSomething"); // There are 3 paths since #walkPaths does not use any excluders MatcherAssert.assertThat(paths, hasSize(equalTo(3))); } + @Test + public void testUnreachableCodeDetection() { + final String sourceCode = + "public class MyClass {\n" + + " String doSomething() {\n" + + " return 'hello';\n" + + " System.debug('This line is unreachable');\n" + + " }\n" + + "}\n"; + + UserActionException thrown = + assertThrows( + UserActionException.class, + () -> GraphBuildTestUtil.buildGraph(g, sourceCode), + "UserActionException should've been thrown before this point"); + + MatcherAssert.assertThat(thrown.getMessage(), containsString("MyClass:4")); + } + /** * Asserts that the number of {@link Schema#CFG_PATH} edges matches the number of vertex pairs * provided and that the edges match. @@ -2809,44 +2825,4 @@ private void assertEndScopes( hasSize(equalTo(endScopes.length))); MatcherAssert.assertThat(vertex.getEndScopes(), contains(endScopes)); } - - /** Sanity method to walk all paths. Helps to ensure all of the push/pops are correct */ - private List walkAllPaths(String methodName) { - MethodVertex methodVertex = - SFVertexFactory.load( - g, - g.V().hasLabel(NodeType.METHOD) - .has(Schema.NAME, methodName) - .not(has(Schema.IS_STANDARD, true))); - List paths = ApexPathUtil.getForwardPaths(g, methodVertex); - - for (ApexPath path : paths) { - DefaultSymbolProviderVertexVisitor symbols = new DefaultSymbolProviderVertexVisitor(g); - PathVertexVisitor visitor = new DefaultNoOpPathVertexVisitor(); - ApexPathWalker.walkPath(g, path, visitor, symbols); - } - - return paths; - } - - private static void buildGraph(GraphTraversalSource g, String sourceCode) { - buildGraph(g, new String[] {sourceCode}); - } - - private static void buildGraph(GraphTraversalSource g, String[] sourceCodes) { - List compilations = new ArrayList<>(); - for (int i = 0; i < sourceCodes.length; i++) { - compilations.add( - new Util.CompilationDescriptor( - "TestCode" + i, JorjeUtil.compileApexFromString(sourceCodes[i]))); - } - - VertexCacheProvider.get().initialize(g); - CustomerApexVertexBuilder customerApexVertexBuilder = - new CustomerApexVertexBuilder(g, compilations); - - for (GraphBuilder graphBuilder : new GraphBuilder[] {customerApexVertexBuilder}) { - graphBuilder.build(); - } - } } diff --git a/sfge/src/test/java/com/salesforce/graph/build/MethodUtilTest.java b/sfge/src/test/java/com/salesforce/graph/build/MethodUtilTest.java index a5d23d619..c2622c96d 100644 --- a/sfge/src/test/java/com/salesforce/graph/build/MethodUtilTest.java +++ b/sfge/src/test/java/com/salesforce/graph/build/MethodUtilTest.java @@ -1189,55 +1189,6 @@ public void testIsTestClass() { MatcherAssert.assertThat(myTestClass.isTest(), equalTo(true)); } - /** - * This only tests the positive UserClass case. The compiler does not accept annotations on the - * other types of vertices that are excluded from the query. Exclude test classes and methods - */ - @Test - public void testGetAuraEnabledMethods() { - String[] sourceCode = { - "public class MyClass {\n" - + " @AuraEnabled\n" - + " public static void foo() {\n" - + " }\n" - + " @AuraEnabled\n" - + " public static testMethod void shouldBeExcludedByModifier() {\n" - + " }\n" - + " @AuraEnabled\n" - + " @isTest\n" - + " public static void shouldBeExcludedByAnnotation() {\n" - + " }\n" - + " public static void bar() {\n" - + " }\n" - + "}\n", - "@isTest\n" - + "public class MyTestClass {\n" - + " @AuraEnabled\n" - + " public static void foo() {\n" - + " }\n" - + "}\n", - }; - - TestUtil.buildGraph(g, sourceCode); - - List methods = MethodUtil.getAuraEnabledMethods(g, new ArrayList<>()); - MatcherAssert.assertThat(methods, hasSize(equalTo(1))); - - MethodVertex method = methods.get(0); - MatcherAssert.assertThat(method.getName(), equalTo("foo")); - MatcherAssert.assertThat(method.isTest(), equalTo(false)); - - 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)); - } - } - /** * Test that UserClass methods that return are PageReference included. UserInterface methods are * excluded. Test methods are exclude. Methods from test classes are excluded. diff --git a/sfge/src/test/java/com/salesforce/graph/build/StaticBlockUtilTest.java b/sfge/src/test/java/com/salesforce/graph/build/StaticBlockUtilTest.java new file mode 100644 index 000000000..075bb92b5 --- /dev/null +++ b/sfge/src/test/java/com/salesforce/graph/build/StaticBlockUtilTest.java @@ -0,0 +1,117 @@ +package com.salesforce.graph.build; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; + +import com.salesforce.TestUtil; +import com.salesforce.graph.ApexPath; +import com.salesforce.graph.vertex.BaseSFVertex; +import com.salesforce.graph.vertex.BlockStatementVertex; +import com.salesforce.graph.vertex.ExpressionStatementVertex; +import com.salesforce.graph.vertex.LiteralExpressionVertex; +import com.salesforce.graph.vertex.MethodCallExpressionVertex; +import java.util.List; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class StaticBlockUtilTest { + protected GraphTraversalSource g; + private static final String SYNTHETIC_STATIC_BLOCK_METHOD_1 = + String.format(StaticBlockUtil.SYNTHETIC_STATIC_BLOCK_METHOD_NAME, 1); + + @BeforeEach + public void setup() { + this.g = TestUtil.getGraph(); + } + + // does each static block have a corresponding synthetic method block? + @Test + public void testMethodForStaticBlock() { + String[] sourceCode = { + "public class MyClass {\n" + + " static {\n" + + " System.debug('inside static block 1');\n" + + "}\n" + + " static {\n" + + " System.debug('inside static block 2');\n" + + " }\n" + + "void doSomething() {\n" + + " System.debug('inside doSomething');\n" + + "}\n" + + "}" + }; + + GraphBuildTestUtil.buildGraph(g, sourceCode); + + verifyStaticBlockMethod( + String.format(StaticBlockUtil.SYNTHETIC_STATIC_BLOCK_METHOD_NAME, 1), + "inside static block 1"); + verifyStaticBlockMethod( + String.format(StaticBlockUtil.SYNTHETIC_STATIC_BLOCK_METHOD_NAME, 2), + "inside static block 2"); + } + + private void verifyStaticBlockMethod(String methodName, String printedString) { + final List staticBlockPaths = GraphBuildTestUtil.walkAllPaths(g, methodName); + assertThat(staticBlockPaths, hasSize(1)); + final ApexPath path = staticBlockPaths.get(0); + + // Make sure synthetic method is linked to the static block and its contents + final BlockStatementVertex blockStatementVertex = (BlockStatementVertex) path.firstVertex(); + final List blockStmtChildren = blockStatementVertex.getChildren(); + assertThat(blockStmtChildren, hasSize(1)); + + final ExpressionStatementVertex expressionStatementVertex = + (ExpressionStatementVertex) blockStmtChildren.get(0); + final List exprStmtChildren = expressionStatementVertex.getChildren(); + assertThat(exprStmtChildren, hasSize(1)); + + final MethodCallExpressionVertex methodCallExpressionVertex = + (MethodCallExpressionVertex) exprStmtChildren.get(0); + assertThat(methodCallExpressionVertex.getFullMethodName(), equalTo("System.debug")); + final LiteralExpressionVertex literalExpressionVertex = + (LiteralExpressionVertex) methodCallExpressionVertex.getParameters().get(0); + + assertThat(literalExpressionVertex.getLiteralAsString(), equalTo(printedString)); + } + + // does synthetic invoker method exist and contain the necessary parts and relationship? + @Test + public void testMethodInvokerForStaticBlock() { + String[] sourceCode = { + "public class MyClass {\n" + + " static {\n" + + " System.debug('inside static block');\n" + + "}\n" + + "void doSomething() {\n" + + " System.debug('inside doSomething');\n" + + "}\n" + + "}" + }; + + GraphBuildTestUtil.buildGraph(g, sourceCode); + + final List staticBlockPaths = + GraphBuildTestUtil.walkAllPaths(g, StaticBlockUtil.STATIC_BLOCK_INVOKER_METHOD); + assertThat(staticBlockPaths, hasSize(1)); + final ApexPath path = staticBlockPaths.get(0); + + // Make sure synthetic method is linked to the static block and its contents + final BlockStatementVertex blockStatementVertex = (BlockStatementVertex) path.firstVertex(); + final List blockStmtChildren = blockStatementVertex.getChildren(); + assertThat(blockStmtChildren, hasSize(1)); + + final ExpressionStatementVertex expressionStatementVertex = + (ExpressionStatementVertex) blockStmtChildren.get(0); + final List exprStmtChildren = expressionStatementVertex.getChildren(); + assertThat(exprStmtChildren, hasSize(1)); + + final MethodCallExpressionVertex methodCallExpressionVertex = + (MethodCallExpressionVertex) exprStmtChildren.get(0); + assertThat( + methodCallExpressionVertex.getFullMethodName(), + equalTo(SYNTHETIC_STATIC_BLOCK_METHOD_1)); + } +} 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 a139421a8..b69e82bfc 100644 --- a/sfge/src/test/java/com/salesforce/graph/ops/MethodUtilTest.java +++ b/sfge/src/test/java/com/salesforce/graph/ops/MethodUtilTest.java @@ -8,7 +8,10 @@ import static org.junit.jupiter.api.Assertions.fail; import com.salesforce.TestUtil; +import com.salesforce.apex.jorje.ASTConstants; +import com.salesforce.graph.Schema; import com.salesforce.graph.vertex.MethodVertex; +import com.salesforce.graph.vertex.SFVertexFactory; import com.salesforce.messaging.CliMessager; import com.salesforce.messaging.EventKey; import com.salesforce.rules.AbstractRuleRunner.RuleRunnerTarget; @@ -20,6 +23,8 @@ import org.hamcrest.MatcherAssert; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; public class MethodUtilTest { private GraphTraversalSource g; @@ -175,12 +180,21 @@ public void getTargetMethods_targetMultipleMethods() { List methodVertices = MethodUtil.getTargetedMethods(g, targets); MatcherAssert.assertThat(methodVertices, hasSize(equalTo(2))); - MethodVertex firstVertex = methodVertices.get(0); - assertEquals(METHOD_WITHOUT_OVERLOADS_1, firstVertex.getName()); - - MethodVertex secondVertex = methodVertices.get(1); - assertEquals(METHOD_WITHOUT_OVERLOADS_2, secondVertex.getName()); + boolean method1Found = false; + boolean method2Found = false; + for (MethodVertex methodVertex : methodVertices) { + String name = methodVertex.getName(); + if (METHOD_WITHOUT_OVERLOADS_1.equals(name)) { + method1Found = true; + } else if (METHOD_WITHOUT_OVERLOADS_2.equals(name)) { + method2Found = true; + } else { + fail("Unexpected method name " + name); + } + } + assertTrue(method1Found); + assertTrue(method2Found); String messages = CliMessager.getInstance().getAllMessages(); assertEquals("[]", messages); } @@ -227,13 +241,21 @@ public void getTargetMethods_targetNameDupedMethods() { List methodVertices = MethodUtil.getTargetedMethods(g, targets); MatcherAssert.assertThat(methodVertices, hasSize(equalTo(2))); - MethodVertex firstVertex = methodVertices.get(0); - assertEquals(METHOD_WITH_EXTERNAL_NAME_DUPLICATION, firstVertex.getName()); - assertEquals(18, firstVertex.getBeginLine()); - MethodVertex secondVertex = methodVertices.get(1); - assertEquals(METHOD_WITH_EXTERNAL_NAME_DUPLICATION, secondVertex.getName()); - assertEquals(22, secondVertex.getBeginLine()); + boolean line18Found = false; + boolean line22Found = false; + for (MethodVertex methodVertex : methodVertices) { + assertEquals(METHOD_WITH_EXTERNAL_NAME_DUPLICATION, methodVertex.getName()); + if (methodVertex.getBeginLine() == 18) { + line18Found = true; + } else if (methodVertex.getBeginLine() == 22) { + line22Found = true; + } else { + fail("Unexpected line number " + methodVertex.getBeginLine()); + } + } + assertTrue(line18Found); + assertTrue(line22Found); String messages = CliMessager.getInstance().getAllMessages(); MatcherAssert.assertThat( @@ -312,4 +334,134 @@ public void getTargetMethods_targetMethodInInnerAndOuterClass() { messages, containsString(EventKey.WARNING_MULTIPLE_METHOD_TARGET_MATCHES.getMessageKey())); } + + @ValueSource( + strings = { + Schema.AURA_ENABLED, + Schema.INVOCABLE_METHOD, + Schema.REMOTE_ACTION, + Schema.NAMESPACE_ACCESSIBLE + }) + @ParameterizedTest(name = "{displayName}: {0}") + public void testGetMethodsWithAnnotation(String annotation) { + String[] sourceCode = { + "public class MyClass {\n" + + " @" + + annotation + + "\n" + + " public static void foo() {\n" + + " }\n" + + " @" + + annotation + + "\n" + + " public static testMethod void shouldBeExcludedByModifier() {\n" + + " }\n" + + " @" + + annotation + + "\n" + + " @isTest\n" + + " public static void shouldBeExcludedByAnnotation() {\n" + + " }\n" + + " public static void bar() {\n" + + " }\n" + + "}\n", + "@isTest\n" + + "public class MyTestClass {\n" + + " @" + + annotation + + "\n" + + " public static void foo() {\n" + + " }\n" + + "}\n", + }; + + TestUtil.buildGraph(g, sourceCode, true); + + List methods = + MethodUtil.getMethodsWithAnnotation(g, new ArrayList<>(), annotation); + MatcherAssert.assertThat(methods, hasSize(equalTo(1))); + + MethodVertex method = methods.get(0); + MatcherAssert.assertThat(method.getName(), equalTo("foo")); + MatcherAssert.assertThat(method.isTest(), equalTo(false)); + + 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)); + } + } + + @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. + MatcherAssert.assertThat(methods, hasSize(equalTo(1))); + MatcherAssert.assertThat(methods.get(0).getName(), equalTo("foo")); + + 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)); + } + } + + @Test + public void testGetInboundEmailHandlerMethods() { + String[] sourceCode = { + "public class MyClass implements Messaging.InboundEmailHandler {\n" + + " public Messaging.InboundEmailResult handleInboundEmail(Messaging.InboundEmail email, Messaging.InboundEnvelope envelope) {\n" + + " return null;\n" + + " }\n" + + " public Messaging.InboundEmailHandler someSecondaryMethod() {\n" + + " return null;\n" + + " }\n" + + "}\n", + "public class MyClass2 {\n" + + " public Messaging.InboundEmailResult handleInboundEmail(Messaging.InboundEmail email, Messaging.InboundEnvelope envelope) {\n" + + " return null;\n" + + " }\n" + + " public Messaging.InboundEmailHandler someSecondaryMethod() {\n" + + " return null;\n" + + " }\n" + + "}\n" + }; + TestUtil.buildGraph(g, sourceCode); + + List methods = MethodUtil.getInboundEmailHandlerMethods(g, new ArrayList<>()); + // The `MyClass#handleInboundEmail` method should be included because it's an implementation + // of the desired interface. + MatcherAssert.assertThat(methods, hasSize(equalTo(1))); + MatcherAssert.assertThat(methods.get(0).getName(), equalTo("handleInboundEmail")); + MatcherAssert.assertThat(methods.get(0).getDefiningType(), equalTo("MyClass")); + } } diff --git a/sfge/src/test/java/com/salesforce/graph/ops/TraversalUtilTest.java b/sfge/src/test/java/com/salesforce/graph/ops/TraversalUtilTest.java index 434bb67ce..35e946355 100644 --- a/sfge/src/test/java/com/salesforce/graph/ops/TraversalUtilTest.java +++ b/sfge/src/test/java/com/salesforce/graph/ops/TraversalUtilTest.java @@ -2,6 +2,7 @@ import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.hasSize; +import static org.junit.jupiter.api.Assertions.assertTrue; import com.salesforce.TestUtil; import com.salesforce.apex.jorje.ASTConstants; @@ -11,6 +12,8 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; import org.apache.tinkerpop.gremlin.process.traversal.P; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__; @@ -18,6 +21,8 @@ import org.hamcrest.MatcherAssert; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; public class TraversalUtilTest { private GraphTraversalSource g; @@ -63,6 +68,47 @@ public class TraversalUtilTest { + " }\n" + "}\n"; + private String[] interfaceTestSourceCodes = { + // An interface + "public interface MyInterface1 {}", + // An interface that extends another interface. + "public interface MyInterface2 extends MyInterface1 {}", + // A class that implements the first interface. + "public class MyClass1 implements MyInterface1 {}", + // A class that extends a class that implements an interface. + "public class MyClass2 extends MyClass1 {}", + // A class that implements an interface that extends another interface. + "public class MyClass3 implements MyInterface2 {}", + // A class that implements its own inner interface without using the outer class syntax. + "public class MyClass4 implements InnerInterface1 {\n" + + " public interface InnerInterface1 {}\n" + // A class that implements an inner interface in the same outer class, without using + // the outer class syntax. + + " public class InnerClass1 implements InnerInterface1 {}\n" + // A class that implements an inner interface in the same outer class, using the + // outer class syntax. + + " public class InnerClass2 implements MyClass4.InnerInterface1 {}\n" + // A class that does nothing in particular. + + " public class InnerClass3 {}\n" + + "}", + // A class that implements its own inner interface using the outer class syntax. + "public class MyClass5 implements MyClass5.InnerInterface1 {\n" + + " public interface InnerInterface1 {}\n" + + "}", + // A class that implements another class's inner interface. + "public class MyClass6 implements MyClass4.InnerInterface1 {}", + // An interface whose name will be shared by an inner interface in another class. + "public interface SharedInterfaceName1 {}", + // A class implementing an inner interface whose name is shared by another outer interface. + "public class MyClass7 implements SharedInterfaceName1 {\n" + + " public interface SharedInterfaceName1 {}\n" + + "}", + // A class implementing the outer interface with the duplicate name. + "public class MyClass8 implements SharedInterfaceName1 {}", + // A class that does nothing in particular. + "public class MyClass9 {}" + }; + @BeforeEach public void setup() { this.g = TestUtil.getGraph(); @@ -213,4 +259,53 @@ public void ruleTargetTraversal_MethodAndFile() { MatcherAssert.assertThat( targetResults, hasSize(expectedWholeFile.size() + expectedSingleMethod.size())); } + + @ParameterizedTest(name = "{displayName}: {0}") + @CsvSource({ + // MyInterface1 is directly implemented by MyClass1, and indirectly implemented by MyClass2 + // and MyClass3. + "MyInterface1, 'MyClass1,MyClass2,MyClass3'", + // MyInterface2 is directly implemented by MyClass3 + "MyInterface2, 'MyClass3'", + // MyClass4.InnerInterface1 is directly implemented by MyClass4, two of its inner classes, + // and MyClass6. + "MyClass4.InnerInterface1, 'MyClass4,MyClass4.InnerClass1,MyClass4.InnerClass2,MyClass6'", + // MyClass5.InnerInterface1 is implemented only by its outer class, MyClass5. + "MyClass5.InnerInterface1, 'MyClass5'", + // SharedInterfaceName1 is implemented by MyClass8. + "SharedInterfaceName1, 'MyClass8'", + // MyClass7.SharedInterfaceName1 is implemented by MyClass7. + "MyClass7.SharedInterfaceName1, 'MyClass7'" + }) + public void traverseImplementationsOf_testWithoutTargetFiles( + String interfaceName, String expectedClassNameString) { + String[] expectedClassNames = expectedClassNameString.split(","); + TestUtil.buildGraph(g, interfaceTestSourceCodes); + + // RUN THE TESTED METHOD. + List traversedVertices = + TraversalUtil.traverseImplementationsOf(g, new ArrayList<>(), interfaceName) + // For testing purposes, get the name of each vertex instead of returning + // the whole vertex. + .values(Schema.DEFINING_TYPE) + .toList(); + // Turn the object list into a string set for comparison purposes. + Set traversedVertexNames = + traversedVertices.stream().map(Object::toString).collect(Collectors.toSet()); + + // Create an array of booleans indicating whether we returned each expected class. + boolean[] expectedClassPresent = new boolean[expectedClassNames.length]; + Arrays.fill(expectedClassPresent, false); + + // Iterate over the expected classes and set each class's boolean based on whether it's in + // the name set. + for (int i = 0; i < expectedClassPresent.length; i++) { + expectedClassPresent[i] = traversedVertexNames.contains(expectedClassNames[i]); + } + + // If any of the booleans are still false, the test fails. + for (int i = 0; i < expectedClassPresent.length; i++) { + assertTrue(expectedClassPresent[i], "Class " + expectedClassNames[i] + " is absent"); + } + } } diff --git a/sfge/src/test/java/com/salesforce/graph/symbols/StaticCodeBlockInvocationTest.java b/sfge/src/test/java/com/salesforce/graph/symbols/StaticCodeBlockInvocationTest.java new file mode 100644 index 000000000..914be6b5b --- /dev/null +++ b/sfge/src/test/java/com/salesforce/graph/symbols/StaticCodeBlockInvocationTest.java @@ -0,0 +1,300 @@ +package com.salesforce.graph.symbols; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.salesforce.TestRunner; +import com.salesforce.TestUtil; +import com.salesforce.graph.symbols.apex.ApexStringValue; +import com.salesforce.graph.symbols.apex.ApexValue; +import com.salesforce.graph.visitor.SystemDebugAccumulator; +import java.util.List; +import java.util.Optional; +import java.util.stream.Collectors; +import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; + +public class StaticCodeBlockInvocationTest { + protected GraphTraversalSource g; + + @BeforeEach + public void setup() { + this.g = TestUtil.getGraph(); + } + + @Test + public void testSingleStaticBlockFromStaticMethod() { + String[] sourceCode = { + "public class StaticBlockClass {\n" + + " static {\n" + + " System.debug('static block');\n" + + " }\n" + + " public static String foo() {\n" + + " return 'hello';\n" + + "}\n" + + "}", + "public class MyClass {\n" + + " public void doSomething() {\n" + + " System.debug(StaticBlockClass.foo());\n" + + " }\n" + + "}" + }; + + TestRunner.Result result = TestRunner.walkPath(g, sourceCode); + SystemDebugAccumulator visitor = result.getVisitor(); + final List>> allResults = visitor.getAllResults(); + + assertThat(allResults, hasSize(2)); + + // verify that static block is invoked first + ApexStringValue stringValue = (ApexStringValue) allResults.get(0).get(); + assertThat(stringValue.getValue().get(), equalTo("static block")); + } + + @Test + public void testStaticBlockFromConstructor() { + String[] sourceCode = { + "public class StaticBlockClass {\n" + + " static {\n" + + " System.debug('static block');\n" + + " }\n" + + "}", + "public class MyClass {\n" + + " public void doSomething() {\n" + + " StaticBlockClass sb = new StaticBlockClass();\n" + + " }\n" + + "}" + }; + + TestRunner.Result result = TestRunner.walkPath(g, sourceCode); + SystemDebugAccumulator visitor = result.getVisitor(); + final List>> allResults = visitor.getAllResults(); + + assertThat(allResults, hasSize(1)); + + // verify that static block is invoked first + ApexStringValue stringValue = (ApexStringValue) allResults.get(0).get(); + assertThat(stringValue.getValue().get(), equalTo("static block")); + } + + @Test + public void testNoStaticBlock() { + String[] sourceCode = { + "public class StaticBlockClass {\n" + " public static String foo = 'hello';\n" + "}", + "public class MyClass {\n" + + " public void doSomething() {\n" + + " System.debug(StaticBlockClass.foo);\n" + + " }\n" + + "}" + }; + + TestRunner.Result result = TestRunner.walkPath(g, sourceCode); + SystemDebugAccumulator visitor = result.getVisitor(); + final List>> allResults = visitor.getAllResults(); + + assertThat(allResults, hasSize(1)); + + // verify non-static-block case is handled correctly + ApexStringValue stringValue = (ApexStringValue) allResults.get(0).get(); + assertThat(stringValue.getValue().get(), equalTo("hello")); + } + + @Test + @Disabled // TODO: static field defined along with static block should not be double-invoked + public void testSingleStaticBlockAndField() { + String[] sourceCode = { + "public class StaticBlockClass {\n" + + " static {\n" + + " System.debug('static block');\n" + + " }\n" + + " static String myStr = 'hello';\n" + + " public static String foo() {\n" + + " return 'hello';\n" + + "}\n" + + "}", + "public class MyClass {\n" + + " public void doSomething() {\n" + + " System.debug(StaticBlockClass.foo());\n" + + " }\n" + + "}" + }; + + TestRunner.Result result = TestRunner.walkPath(g, sourceCode); + SystemDebugAccumulator visitor = result.getVisitor(); + final List>> allResults = visitor.getAllResults(); + + assertThat(allResults, hasSize(2)); + + // verify that static block is invoked first + ApexStringValue stringValue = (ApexStringValue) allResults.get(0).get(); + assertThat(stringValue.getValue().get(), equalTo("static block")); + } + + @Test + @Disabled // TODO: static field defined along with static block should not be double-invoked + public void testSingleStaticBlockAndFieldInvokingAMethod() { + String[] sourceCode = { + "public class StaticBlockClass {\n" + + " static {\n" + + " System.debug('static block');\n" + + " }\n" + + " static String myStr = foo();\n" + + " public static String bar() {\n" + + " return 'bar';\n" + + "}\n" + + "}", + "public class MyClass {\n" + + " public void doSomething() {\n" + + " System.debug(StaticBlockClass.bar());\n" + + " }\n" + + "}" + }; + + TestRunner.Result result = TestRunner.walkPath(g, sourceCode); + SystemDebugAccumulator visitor = result.getVisitor(); + final List>> allResults = visitor.getAllResults(); + + assertThat(allResults, hasSize(2)); + + // verify that static block is invoked first + ApexStringValue stringValue = (ApexStringValue) allResults.get(0).get(); + assertThat(stringValue.getValue().get(), equalTo("static block")); + } + + @Test + public void testMultipleStaticBlocks() { + String[] sourceCode = { + "public class StaticBlockClass {\n" + + " static {\n" + + " System.debug('static block 1');\n" + + " }\n" + + " static {\n" + + " System.debug('static block 2');\n" + + " }\n" + + " public static String foo() {\n" + + " return 'hello';\n" + + "}\n" + + "}", + "public class MyClass {\n" + + " public void doSomething() {\n" + + " System.debug(StaticBlockClass.foo());\n" + + " }\n" + + "}" + }; + + TestRunner.Result result = TestRunner.walkPath(g, sourceCode); + SystemDebugAccumulator visitor = result.getVisitor(); + final List>> allResults = visitor.getAllResults(); + + final List resultStrings = getResultStrings(allResults); + assertTrue(resultStrings.contains("static block 1")); + assertTrue(resultStrings.contains("static block 2")); + assertTrue(resultStrings.contains("hello")); + } + + @Test + @Disabled // TODO: fix issue where static block 2 is invoked twice instead of once + public void testEachStaticBlockIsInvokedOnlyOnce() { + String[] sourceCode = { + "public class StaticBlockClass {\n" + + " static {\n" + + " System.debug('static block 1');\n" + + " }\n" + + " static {\n" + + " System.debug('static block 2');\n" + + " }\n" + + " public static String foo() {\n" + + " return 'hello';\n" + + "}\n" + + "}", + "public class MyClass {\n" + + " public void doSomething() {\n" + + " System.debug(StaticBlockClass.foo());\n" + + " }\n" + + "}" + }; + + TestRunner.Result result = TestRunner.walkPath(g, sourceCode); + SystemDebugAccumulator visitor = result.getVisitor(); + final List>> allResults = visitor.getAllResults(); + + assertThat( + allResults, + hasSize(3)); // TODO: this is currently returning 4 where static block 2 gets + // invoked twice + } + + @Test + public void testSuperClassStaticBlocks() { + String[] sourceCode = { + "public class SuperStaticBlockClass {\n" + + " static {\n" + + " System.debug('super static block');\n" + + " }\n" + + "}", + "public class StaticBlockClass extends SuperStaticBlockClass {\n" + + " static {\n" + + " System.debug('static block');\n" + + " }\n" + + " public static String foo() {\n" + + " return 'hello';\n" + + "}\n" + + "}", + "public class MyClass {\n" + + " public void doSomething() {\n" + + " System.debug(StaticBlockClass.foo());\n" + + " }\n" + + "}" + }; + + TestRunner.Result result = TestRunner.walkPath(g, sourceCode); + SystemDebugAccumulator visitor = result.getVisitor(); + final List>> allResults = visitor.getAllResults(); + + final List resultStrings = getResultStrings(allResults); + assertTrue(resultStrings.contains("super static block")); + assertTrue(resultStrings.contains("static block")); + assertTrue(resultStrings.contains("hello")); + } + + @Test + @Disabled // TODO: super class's static block should be invoked only once + public void testSuperClassStaticBlocksInvokedOnceEach() { + String[] sourceCode = { + "public class SuperStaticBlockClass {\n" + + " static {\n" + + " System.debug('super static block');\n" + + " }\n" + + "}", + "public class StaticBlockClass extends SuperStaticBlockClass {\n" + + " static {\n" + + " System.debug('static block');\n" + + " }\n" + + " public static String foo() {\n" + + " return 'hello';\n" + + "}\n" + + "}", + "public class MyClass {\n" + + " public void doSomething() {\n" + + " System.debug(StaticBlockClass.foo());\n" + + " }\n" + + "}" + }; + + TestRunner.Result result = TestRunner.walkPath(g, sourceCode); + SystemDebugAccumulator visitor = result.getVisitor(); + final List>> allResults = visitor.getAllResults(); + + assertThat(allResults, hasSize(3)); + } + + private List getResultStrings(List>> allResults) { + return allResults.stream() + .map(r -> ((ApexStringValue) r.get()).getValue().get()) + .collect(Collectors.toList()); + } +} diff --git a/sfge/src/test/java/com/salesforce/rules/RuleUtilTest.java b/sfge/src/test/java/com/salesforce/rules/RuleUtilTest.java index 0e8618138..59033c7ce 100644 --- a/sfge/src/test/java/com/salesforce/rules/RuleUtilTest.java +++ b/sfge/src/test/java/com/salesforce/rules/RuleUtilTest.java @@ -5,10 +5,10 @@ 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; import com.salesforce.graph.vertex.MethodVertex; import com.salesforce.metainfo.MetaInfoCollectorTestProvider; import com.salesforce.metainfo.VisualForceHandlerImpl; @@ -25,6 +25,8 @@ import org.hamcrest.MatcherAssert; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; public class RuleUtilTest { private static final Logger LOGGER = LogManager.getLogger(RuleUtilTest.class); @@ -35,16 +37,25 @@ public void setup() { this.g = TestUtil.getGraph(); } - @Test - public void getPathEntryPoints_includesAuraEnabledMethods() { + @ValueSource( + strings = { + Schema.AURA_ENABLED, + Schema.INVOCABLE_METHOD, + Schema.REMOTE_ACTION, + Schema.NAMESPACE_ACCESSIBLE + }) + @ParameterizedTest(name = "{displayName}: {0}") + public void getPathEntryPoints_includesAnnotatedMethods(String annotation) { String sourceCode = "public class Foo {\n" - + " @AuraEnabled\n" - + " public boolean auraMethod() {\n" + + " @" + + annotation + + "\n" + + " public boolean annotatedMethod() {\n" + " return true;\n" + " }\n" + "\n" - + " public boolean nonAuraMethod() {\n" + + " public boolean nonAnnotatedMethod() {\n" + " return true;\n" + " }\n" + "}\n"; @@ -54,7 +65,41 @@ public void getPathEntryPoints_includesAuraEnabledMethods() { MatcherAssert.assertThat(entryPoints, hasSize(equalTo(1))); MethodVertex firstVertex = entryPoints.get(0); - assertEquals("auraMethod", firstVertex.getName()); + 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 @@ -78,6 +123,26 @@ public void getPathEntryPoints_includesPageReferenceMethods() { assertEquals("pageRefMethod", firstVertex.getName()); } + @Test + public void getPathEntryPoints_includesInboundEmailHandlerMethods() { + String sourceCode = + "public class MyClass implements Messaging.InboundEmailHandler {\n" + + " public Messaging.InboundEmailResult handleInboundEmail(Messaging.InboundEmail email, Messaging.InboundEnvelope envelope) {\n" + + " return null;\n" + + " }\n" + + " public Messaging.InboundEmailHandler someSecondaryMethod() {\n" + + " return null;\n" + + " }\n" + + "}\n"; + TestUtil.buildGraph(g, sourceCode, true); + + List entryPoints = RuleUtil.getPathEntryPoints(g); + + MatcherAssert.assertThat(entryPoints, hasSize(equalTo(1))); + MethodVertex firstVertex = entryPoints.get(0); + assertEquals("handleInboundEmail", firstVertex.getName()); + } + @Test public void getPathEntryPoints_includesExposedControllerMethods() { try { @@ -256,7 +321,9 @@ public void getPathEntryPoints_includeMethodAndFileLevelTargets() { @Test public void getAllRules_noExceptionThrown() { try { - List allRules = RuleUtil.getAllRules(); + List allRules = RuleUtil.getEnabledRules(); + MatcherAssert.assertThat(allRules, hasSize(1)); + assertTrue(allRules.contains(ApexFlsViolationRule.getInstance())); } catch (Exception ex) { fail("Unexpected " + ex.getClass().getSimpleName() + ": " + ex.getMessage()); } diff --git a/src/Constants.ts b/src/Constants.ts index e9f153217..2481a2b4a 100644 --- a/src/Constants.ts +++ b/src/Constants.ts @@ -1,7 +1,7 @@ import os = require('os'); import path = require('path'); -export const PMD_VERSION = '6.45.0'; +export const PMD_VERSION = '6.47.0'; export const SFGE_VERSION = '1.0.1-pilot'; export const CATALOG_FILE = 'Catalog.json'; export const CUSTOM_PATHS_FILE = 'CustomPaths.json'; diff --git a/src/commands/scanner/rule/add.ts b/src/commands/scanner/rule/add.ts index 54e76c231..b4e34515b 100644 --- a/src/commands/scanner/rule/add.ts +++ b/src/commands/scanner/rule/add.ts @@ -13,7 +13,7 @@ Messages.importMessagesDirectory(__dirname); // Load the specific messages for this file. Messages from @salesforce/command, @salesforce/core, // or any library that is using the messages framework can also be loaded this way. const messages = Messages.loadMessages('@salesforce/sfdx-scanner', 'add'); - +const commonMessages = Messages.loadMessages('@salesforce/sfdx-scanner', 'common'); export default class Add extends SfdxCommand { @@ -41,6 +41,7 @@ export default class Add extends SfdxCommand { public async run(): Promise { this.validateFlags(); + this.ux.styledHeader(commonMessages.getMessage('FEEDBACK_SURVEY_BANNER')); const language = this.flags.language as string; const paths = this.resolvePaths(); diff --git a/src/commands/scanner/rule/describe.ts b/src/commands/scanner/rule/describe.ts index 1e2557305..c5dd74f6a 100644 --- a/src/commands/scanner/rule/describe.ts +++ b/src/commands/scanner/rule/describe.ts @@ -12,6 +12,7 @@ Messages.importMessagesDirectory(__dirname); // Load the specific messages for this file. Messages from @salesforce/command, @salesforce/core, // or any library that is using the messages framework can also be loaded this way. const messages = Messages.loadMessages('@salesforce/sfdx-scanner', 'describe'); +const commonMessages = Messages.loadMessages('@salesforce/sfdx-scanner', 'common'); type DescribeStyledRule = Rule & { enabled: boolean; @@ -41,6 +42,7 @@ export default class Describe extends ScannerCommand { }; public async run(): Promise { + this.ux.styledHeader(commonMessages.getMessage('FEEDBACK_SURVEY_BANNER')); const ruleFilters = this.buildRuleFilters(); // It's possible for this line to throw an error, but that's fine because the error will be an SfdxError that we can // allow to boil over. diff --git a/src/commands/scanner/rule/list.ts b/src/commands/scanner/rule/list.ts index ffe4ddc84..ba5406632 100644 --- a/src/commands/scanner/rule/list.ts +++ b/src/commands/scanner/rule/list.ts @@ -13,6 +13,7 @@ Messages.importMessagesDirectory(__dirname); // Load the specific messages for this file. Messages from @salesforce/command, @salesforce/core, // or any library that is using the messages framework can also be loaded this way. const messages = Messages.loadMessages('@salesforce/sfdx-scanner', 'list'); +const commonMessages = Messages.loadMessages('@salesforce/sfdx-scanner', 'common'); const columns = [messages.getMessage('columnNames.name'), messages.getMessage('columnNames.languages'), messages.getMessage('columnNames.categories'), @@ -64,6 +65,7 @@ export default class List extends ScannerCommand { }; public async run(): Promise { + this.ux.styledHeader(commonMessages.getMessage('FEEDBACK_SURVEY_BANNER')); const ruleFilters = this.buildRuleFilters(); // It's possible for this line to throw an error, but that's fine because the error will be an SfdxError that we can // allow to boil over. diff --git a/src/commands/scanner/rule/remove.ts b/src/commands/scanner/rule/remove.ts index b87f712ec..f121782de 100644 --- a/src/commands/scanner/rule/remove.ts +++ b/src/commands/scanner/rule/remove.ts @@ -15,6 +15,7 @@ Messages.importMessagesDirectory(__dirname); // Load the specific messages for this file. Messages from @salesforce/command, @salesforce/core, // or any library that is using the messages framework can also be loaded this way. const messages = Messages.loadMessages('@salesforce/sfdx-scanner', 'remove'); +const commonMessages = Messages.loadMessages('@salesforce/sfdx-scanner', 'common'); export default class Remove extends ScannerCommand { // These determine what's displayed when the --help/-h flag is supplied. @@ -44,6 +45,7 @@ export default class Remove extends ScannerCommand { }; public async run(): Promise { + this.ux.styledHeader(commonMessages.getMessage('FEEDBACK_SURVEY_BANNER')); // Step 1: Validate our input. this.validateFlags(); diff --git a/src/lib/ScannerRunCommand.ts b/src/lib/ScannerRunCommand.ts index 4e4fb69e3..6cc092fee 100644 --- a/src/lib/ScannerRunCommand.ts +++ b/src/lib/ScannerRunCommand.ts @@ -14,6 +14,7 @@ Messages.importMessagesDirectory(__dirname); // Load the specific messages for this file. Messages from @salesforce/command, @salesforce/core, // or any library that is using the messages framework can also be loaded this way. const messages = Messages.loadMessages('@salesforce/sfdx-scanner', 'run'); +const commonMessages = Messages.loadMessages('@salesforce/sfdx-scanner', 'common'); // This code is used for internal errors. export const INTERNAL_ERROR_CODE = 1; @@ -22,6 +23,7 @@ export abstract class ScannerRunCommand extends ScannerCommand { public async run(): Promise { // First, do any validations that can't be handled with out-of-the-box stuff. await this.validateFlags(); + this.ux.styledHeader(commonMessages.getMessage('FEEDBACK_SURVEY_BANNER')); // If severity-threshold is used, that implicitly normalizes the severity. const normalizeSeverity: boolean = (this.flags['normalize-severity'] || this.flags['severity-threshold']) as boolean; diff --git a/src/lib/sfge/SfgeEngine.ts b/src/lib/sfge/SfgeEngine.ts index 09de49885..ba7e1b7a7 100644 --- a/src/lib/sfge/SfgeEngine.ts +++ b/src/lib/sfge/SfgeEngine.ts @@ -83,10 +83,6 @@ export class SfgeEngine extends AbstractRuleEngine { const categoryNames: Set = new Set(); partialRules.forEach(({name, description, category}) => { - // TODO: This should be accomplished by actually disabling the rules within SFGE, instead of this hacky fix. - if (name !== 'ApexFlsViolationRule') { - return; - } completeRules.push({ engine: ENGINE.SFGE, sourcepackage: "sfge", diff --git a/test/TestUtils.ts b/test/TestUtils.ts index b50b3770d..06da598c4 100644 --- a/test/TestUtils.ts +++ b/test/TestUtils.ts @@ -1,6 +1,7 @@ import fs = require('fs'); import path = require('path'); -import { test } from '@salesforce/command/lib/test'; +import { test, expect } from '@salesforce/command/lib/test'; +import * as CommonMessages from '../messages/common.js'; import * as TestOverrides from './test-related-lib/TestOverrides'; import Sinon = require('sinon'); import LocalCatalog from '../src/lib/services/LocalCatalog'; @@ -15,6 +16,12 @@ export function prettyPrint(obj: any): string { return JSON.stringify(obj, null, 2); } +export function stripExtraneousOutput(stdout: string): string { + const splitOutput = stdout.split('\n'); + expect(splitOutput[0]).to.equal('=== ' + CommonMessages.FEEDBACK_SURVEY_BANNER); + return splitOutput.slice(1).join('\n'); +} + export function stubCatalogFixture(): void { // Make sure all catalogs exist where they're supposed to. if (!fs.existsSync(CATALOG_FIXTURE_PATH)) { diff --git a/test/commands/scanner/rule/list.test.ts b/test/commands/scanner/rule/list.test.ts index 3bfc3ef50..973782dfd 100644 --- a/test/commands/scanner/rule/list.test.ts +++ b/test/commands/scanner/rule/list.test.ts @@ -1,5 +1,5 @@ import {expect} from '@salesforce/command/lib/test'; -import {setupCommandTest} from '../../../TestUtils'; +import {setupCommandTest, stripExtraneousOutput} from '../../../TestUtils'; import {Rule} from '../../../../src/types'; import {CATALOG_FILE, ENGINE} from '../../../../src/Constants'; import fs = require('fs'); @@ -56,7 +56,8 @@ describe('scanner:rule:list', () => { // Split the output table by newline and throw out the first two rows, since they just contain header information. That // should leave us with the actual data. - const rows = ctx.stdout.trim().split('\n'); + const output = stripExtraneousOutput(ctx.stdout); + const rows = output.trim().split('\n'); rows.shift(); rows.shift(); expect(rows).to.have.lengthOf(totalRuleCount, 'All rules should have been returned'); @@ -306,7 +307,8 @@ describe('scanner:rule:list', () => { .command(['scanner:rule:list', '--category', 'Beebleborp']) .it('Without --json flag, an empty table is printed', ctx => { // Split the result by newline, and make sure there are two rows. - const rows = ctx.stdout.trim().split('\n'); + const output = stripExtraneousOutput(ctx.stdout); + const rows = output.trim().split('\n'); expect(rows).to.have.lengthOf(2, 'Only the header rows should have been printed'); }); diff --git a/test/commands/scanner/run.severity.test.ts b/test/commands/scanner/run.severity.test.ts index 6c9384dc4..4e411acff 100644 --- a/test/commands/scanner/run.severity.test.ts +++ b/test/commands/scanner/run.severity.test.ts @@ -1,5 +1,5 @@ import {expect} from '@salesforce/command/lib/test'; -import {setupCommandTest} from '../../TestUtils'; +import {setupCommandTest, stripExtraneousOutput} from '../../TestUtils'; import {Messages} from '@salesforce/core'; import path = require('path'); @@ -31,12 +31,12 @@ describe('scanner:run', function () { '--severity-threshold', '1' ]) .it('When no violations are found equal to or greater than flag value, no error is thrown', ctx => { - - const output = JSON.parse(ctx.stdout); + const output = stripExtraneousOutput(ctx.stdout); + const outputJson = JSON.parse(output); // check that test file still has severities of 3 - for (let i=0; i { - - const output = JSON.parse(ctx.stdout); + const output = stripExtraneousOutput(ctx.stdout); + const outputJson = JSON.parse(output); // check that test file still has severities of 3 - for (let i=0; i { - const output = JSON.parse(ctx.stdout); - - for (let i=0; i { - const output = JSON.parse(ctx.stdout); + const output = stripExtraneousOutput(ctx.stdout); + const outputJson = JSON.parse(output); - for (let i=0; i { - // Split the output by newline characters and throw away the first entry, so we're left with just the rows. - validateCsvOutput(ctx.stdout, false); + const output = stripExtraneousOutput(ctx.stdout); + validateCsvOutput(output, false); }); setupCommandTest