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
1 change: 1 addition & 0 deletions sfge/src/main/java/com/salesforce/graph/Schema.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ 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";
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 property added because the existing INTERFACE_NAMES property isn't guaranteed to contain the full defining types of interfaces, which was making queries very complicated.

public static final String INTERFACE_NAMES = "InterfaceNames";
/** True if this vertex is part of the Apex Standard Library */
public static final String IS_STANDARD = "IsStandard";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,11 @@ private void buildVertices(JorjeNode node, Vertex vNodeParam, String fileName) {
final Vertex vChild = g.addV(child.getLabel()).next();
addProperties(g, child, vChild);

/** Handle static block if we are looking at a <clinit> method that has a block statement.
* See {@linkplain StaticBlockUtil} on why this is needed
* and how we handle it. */
/**
* Handle static block if we are looking at a <clinit> 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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,13 @@ public class CaseSafePropertyUtil {
// The following properties are (with rare exception) assumed to be case-sensitive.
private static final ImmutableSet<String> CASE_SENSITIVE_PROPERTIES =
ImmutableSet.of(
Schema.NAME,
Schema.DEFINING_TYPE,
Schema.FULL_METHOD_NAME,
Schema.INTERFACE_DEFINING_TYPES,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Case-safe equivalent for new property.

Schema.INTERFACE_NAMES,
Schema.METHOD_NAME,
Schema.NAME,
Schema.RETURN_TYPE,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The lack of a case-safe return type was a problem.

Schema.SUPER_CLASS_NAME,
Schema.SUPER_INTERFACE_NAME);

Expand Down Expand Up @@ -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<String> stringList = new ArrayList<>();
for (Object o : value) {
stringList.add(((String) o).toLowerCase(Locale.ROOT));
}
return stringList;
return stringList.toArray();
}
}

Expand Down Expand Up @@ -191,5 +192,19 @@ public static GraphTraversal<Object, Object> hasEndingWith(
return __.has(nodeType, property, TextP.endingWith(value));
}
}

public static GraphTraversal<Object, Object> hasArrayContaining(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the opposite of H.hasWithin(). Instead of checking whether a property's value is contained by the provided array, it checks whether a property represents an array containing the provided value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be helpful to have this information as javadoc.

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));
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,18 @@
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;
import java.util.List;
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;

Expand Down Expand Up @@ -73,7 +73,8 @@ private void buildInheritanceMaps() {

private void processVertexInheritance(InheritableSFVertex vertex) {
InheritableSFVertex extendedVertex = findExtendedVertex(vertex).orElse(null);
List<InheritableSFVertex> implementedVertices = findImplementedVertices(vertex);
Map<String, InheritableSFVertex> 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.
Expand All @@ -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()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to isEmpty check, since the map is always non-null.

List<String> implementedVertexDefiningTypes = new ArrayList<>();
Long myId = vertex.getId();
for (String implementedVertexName : implementedVerticesByName.keySet()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As indicated at line 124, the map contains an entry for every interface implemented by the class. If no vertex was found, the name was mapped to null.
This lets us iterate over every interface the class implements, instead of only those with corresponding vertices.

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);
}
}

Expand All @@ -108,36 +122,40 @@ private Optional<InheritableSFVertex> findExtendedVertex(InheritableSFVertex ver
}
}

private List<InheritableSFVertex> findImplementedVertices(InheritableSFVertex vertex) {
if (vertex instanceof UserInterfaceVertex) {
return new ArrayList<>();
} else {
private Map<String, InheritableSFVertex> findImplementedVerticesByName(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This returns a Map instead of a List, and interface names that don't have an associated vertex will be mapped to null.

InheritableSFVertex vertex) {
Map<String, InheritableSFVertex> 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<String> interfaceNames = ((UserClassVertex) vertex).getInterfaceNames();
for (String interfaceName : interfaceNames) {
InheritableSFVertex inheritedVertex =
findInheritedVertex(interfaceName, inheritorType).orElse(null);
results.put(interfaceName, inheritedVertex);
}
}
return results;
}

private Optional<InheritableSFVertex> findInheritedVertex(
String inheritableType, String inheritorType) {
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(".")) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original code had a bug where inner classes that extended other inner classes weren't getting handled correctly. That bug has been fixed.

// 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));
}
}

Expand All @@ -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<String> implementedVertexDefiningTypes) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting the new property and its case-safe equivalent.

GraphTraversal<Vertex, Vertex> 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();
}
}
51 changes: 24 additions & 27 deletions sfge/src/main/java/com/salesforce/graph/build/StaticBlockUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@
/**
* Handles creation of synthetic methods and vertices to gracefully invoke static code blocks.
*
* <p>Consider this example:
*
* <code>
* <p>Consider this example: <code>
* class StaticBlockClass {
* static {
* System.debug("inside static block 1");
Expand All @@ -30,10 +28,7 @@
* System.debug("inside static block 2");
* }
* }
* </code>
*
* In Jorje's compilation structure, static blocks are represented like this:
* <code>
* </code> In Jorje's compilation structure, static blocks are represented like this: <code>
* class StaticBlockClass {
* private static void <clinit>() {
* {
Expand All @@ -46,17 +41,17 @@
* }
* </code>
*
* <p>Having multiple block statements inside a method breaks SFGE's normal code flow logic.
* This makes handling code blocks in <code><clinit>()</code> impossible.
* <p>As an alternative, we are creating synthetic vertices in the Graph to
* represent the static blocks as individual methods.
* <p>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.
* <p>Having multiple block statements inside a method breaks SFGE's normal code flow logic. This
* makes handling code blocks in <code><clinit>()</code> impossible.
*
* <p>As an alternative, we are creating synthetic vertices in the Graph to represent the static
* blocks as individual methods.
*
* <p>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.
*
* <p>New structure looks like this:
* <code>
* <p>New structure looks like this: <code>
* class StaticBlockClass {
* private static void SyntheticStaticBlock_1() {
* System.debug("inside static block 1");
Expand Down Expand Up @@ -90,12 +85,11 @@ private StaticBlockUtil() {}
* parent for blockStatementVertex, and a sibling of <clinit>()
*/
public static Vertex createSyntheticStaticBlockMethod(
GraphTraversalSource g,
Vertex clinitVertex,
int staticBlockIndex) {
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<Vertex> siblings = GremlinUtil.getChildren(g, GremlinVertexUtil.getParentVertex(g, clinitVertex));
final List<Vertex> siblings =
GremlinUtil.getChildren(g, GremlinVertexUtil.getParentVertex(g, clinitVertex));
final int nextSiblingIndex = siblings.size();

addSyntheticStaticBlockMethodProperties(
Expand Down Expand Up @@ -286,7 +280,10 @@ private static void addStaticModifierProperties(
}

private static void addStaticBlockInvokerProperties(
GraphTraversalSource g, String definingType, Vertex staticBlockInvokerVertex, int childIndex) {
GraphTraversalSource g,
String definingType,
Vertex staticBlockInvokerVertex,
int childIndex) {
verifyType(staticBlockInvokerVertex, ASTConstants.NodeType.METHOD);
final Map<String, Object> properties = new HashMap<>();
properties.put(Schema.NAME, STATIC_BLOCK_INVOKER_METHOD);
Expand All @@ -296,11 +293,11 @@ private static void addStaticBlockInvokerProperties(
}

private static void addSyntheticStaticBlockMethodProperties(
GraphTraversalSource g,
String definingType,
Vertex syntheticMethodVertex,
int staticBlockIndex,
int childIndex) {
GraphTraversalSource g,
String definingType,
Vertex syntheticMethodVertex,
int staticBlockIndex,
int childIndex) {
verifyType(syntheticMethodVertex, ASTConstants.NodeType.METHOD);
final Map<String, Object> properties = new HashMap<>();
properties.put(
Expand Down
18 changes: 18 additions & 0 deletions sfge/src/main/java/com/salesforce/graph/ops/MethodUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -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 = "<init>";
public static final String STATIC_CONSTRUCTOR_CANONICAL_NAME = "<clinit>";

Expand Down Expand Up @@ -257,6 +260,21 @@ public static List<MethodVertex> getGlobalMethods(
__.not(__.has(Schema.IS_STANDARD, true)))));
}

public static List<MethodVertex> getInboundEmailHandlerMethods(
GraphTraversalSource g, List<String> targetFiles) {
return SFVertexFactory.loadVertices(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The modular traversal added in TraversalUtil makes the bespoke parts of this traversal very straightforward.

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));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this time, we're only checking the arity, instead of checking the types of arguments. We could add that in the future, but that feels like an excessive level of complication unless we know we'll need it. (He said, having already overcomplicated the PR...)

}

/**
* 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.
Expand Down
Loading