Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a simple constants file at the moment, but as we move more messages to here, would be easier to switch to a different mechanism.


/** UserActionException * */

// format: filename,defined type, line number
public static final String UNREACHABLE_CODE =
"Please remove unreachable code to proceed with analysis: %s,%s:%d";
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
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;
Expand All @@ -27,10 +25,6 @@
@SuppressWarnings("PMD.AbstractClassWithoutAbstractMethod")
abstract class AbstractApexVertexBuilder {
private static final Logger LOGGER = LogManager.getLogger(AbstractApexVertexBuilder.class);
protected static final Set<String> ROOT_VERTICES =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this to GremlinUtil to avoid two-way reference between both the classes.

ImmutableSet.<String>builder()
.addAll(Arrays.asList(ASTConstants.NodeType.ROOT_VERTICES))
.build();
protected final GraphTraversalSource g;

protected AbstractApexVertexBuilder(GraphTraversalSource g) {
Expand Down Expand Up @@ -66,7 +60,7 @@ 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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down
24 changes: 24 additions & 0 deletions sfge/src/main/java/com/salesforce/graph/build/GremlinUtil.java
Original file line number Diff line number Diff line change
@@ -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;

Expand All @@ -17,6 +22,11 @@
* and labels.
*/
public final class GremlinUtil {
protected static final Set<String> ROOT_VERTICES =
ImmutableSet.<String>builder()
.addAll(Arrays.asList(ASTConstants.NodeType.ROOT_VERTICES))
.build();

public static Long getId(Map<Object, Object> vertexProperties) {
return (Long) vertexProperties.get(T.id);
}
Expand Down Expand Up @@ -105,5 +115,19 @@ public static Optional<Vertex> getParent(GraphTraversalSource g, Vertex vertex)
}
}

public static String getFileName(GraphTraversalSource g, Vertex vertex) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New method to get filename from a vertex.

if (ROOT_VERTICES.contains(vertex.label())) {
return vertex.value(Schema.FILE_NAME);
}
List<Vertex> 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() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replacing the cryptic message.

// 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()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,19 @@
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.exception.UserActionException;
import com.salesforce.graph.ApexPath;
import com.salesforce.graph.Schema;
import com.salesforce.graph.ops.ApexPathUtil;
Expand Down Expand Up @@ -2736,6 +2739,25 @@ public void testMethodSwitchStatement() {
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.
Expand Down