@W-13569674@: remove state and PathExpansionObserver from null check …

…rule
forcedotcom · Aug 23, 2023 · ca004db · ca004db
1 parent 4d8c5a6
commit ca004db
Show file tree

Hide file tree

Showing 7 changed files with 91 additions and 150 deletions.
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
@@ -545,18 +545,21 @@ private boolean shouldVisitChildren(BaseSFVertex vertex, PathVertex pathVertex)
             // allowing the caller to know which predicate is interested in
             // which vertex without the need to call
             // #test a second time
-            if (predicate.test(predicateVertex)) {
-
-                // try to convert the predicate to a PathExpansionObserver, if the config has one
-                Optional<PathExpansionObserver> observerOpt =
-                        config.getObserverFromPredicate(predicate);
-
-                // if we found a PathExpansionObserver in this predicate, call its onAcceptVertex
-                // method
-                observerOpt.ifPresent(
-                        pathExpansionObserver ->
-                                pathExpansionObserver.onAcceptVertex(
-                                        vertex, symbolProviderVisitor.getSymbolProvider()));
+            if (predicate.test(predicateVertex, symbolProviderVisitor.getSymbolProvider())) {
+
+                //                // try to convert the predicate to a PathExpansionObserver, if the
+                // config has one
+                //                Optional<PathExpansionObserver> observerOpt =
+                //                        config.getObserverFromPredicate(predicate);
+                //
+                //                // if we found a PathExpansionObserver in this predicate, call its
+                // onAcceptVertex
+                //                // method
+                ////                observerOpt.ifPresent(
+                ////                        pathExpansionObserver ->
+                ////                                pathExpansionObserver.onAcceptVertex(
+                ////                                        pathVertex,
+                // symbolProviderVisitor.getSymbolProvider()));
 
                 predicate.accept(
                         new VertexPredicateVisitor() {
@@ -569,7 +572,10 @@ public void defaultVisit(VertexPredicate predicate) {
                             @Override
                             public void visit(AbstractPathTraversalRule rule) {
                                 if (EngineDirectiveUtil.isRuleEnabled(vertex, rule)) {
-                                    apexPathVertexMetaInfo.addVertex(predicate, pathVertex);
+                                    apexPathVertexMetaInfo.addVertex(
+                                            predicate,
+                                            pathVertex); // this is the pathVertex that was passed
+                                    // into onAcceptVertex (8)
                                 } else {
                                     if (LOGGER.isInfoEnabled()) {
                                         LOGGER.info(

diff --git a/sfge/src/main/java/com/salesforce/graph/ops/expander/ApexPathExpanderConfig.java b/sfge/src/main/java/com/salesforce/graph/ops/expander/ApexPathExpanderConfig.java
@@ -21,7 +21,6 @@ public class ApexPathExpanderConfig implements Immutable<ApexPathExpanderConfig>
     private final List<ApexPathStandardConditionExcluder> conditionExcluders;
     private final List<ApexValueConstrainer> valueConstrainers;
     private final List<VertexPredicate> vertexPredicates;
-    private final Map<VertexPredicate, PathExpansionObserver> predicateToObserver;
     private final List<PathExpansionObserver> expansionObservers;
     private final DefaultSymbolProviderVertexVisitor defaultSymbolProviderVertexVisitor;
     private final String deserializedClassName;
@@ -36,7 +35,6 @@ private ApexPathExpanderConfig(Builder builder) {
         this.conditionExcluders = Collections.unmodifiableList(builder.conditionExcluders);
         this.valueConstrainers = Collections.unmodifiableList(builder.valueConstrainers);
         this.vertexPredicates = Collections.unmodifiableList(builder.vertexPredicates);
-        this.predicateToObserver = Collections.unmodifiableMap(builder.predicateToObserver);
         this.expansionObservers = Collections.unmodifiableList(builder.expansionObservers);
         this.deserializedClassName = builder.deserializedClassName;
         this.instanceClassName = builder.instanceClassName;
@@ -72,15 +70,6 @@ public List<VertexPredicate> getVertexPredicates() {
         return vertexPredicates;
     }
 
-    /**
-     * {@link VertexPredicate} and {@link PathExpansionObserver} can be the same object, though this
-     * mapping converts the former type to the latter, if set up correctly in the config, using
-     * {@link Builder#withObservedPredicate(VertexPredicate, PathExpansionObserver)}.
-     */
-    public Optional<PathExpansionObserver> getObserverFromPredicate(VertexPredicate predicate) {
-        return Optional.ofNullable(this.predicateToObserver.get(predicate));
-    }
-
     public List<PathExpansionObserver> getExpansionObservers() {
         return expansionObservers;
     }
@@ -156,16 +145,6 @@ public Builder withVertexPredicate(VertexPredicate vertexPredicate) {
             return this;
         }
 
-        /**
-         * Link a PathExpansionObserver to a VertexPredicate. These can and should be the same
-         * object. See {@link #getObserverFromPredicate(VertexPredicate)}
-         */
-        public Builder withObservedPredicate(
-                VertexPredicate vertexPredicate, PathExpansionObserver observer) {
-            predicateToObserver.put(vertexPredicate, observer);
-            return this;
-        }
-
         /** Additively add a {@link PathExpansionObserver} */
         public Builder withPathExpansionObserver(PathExpansionObserver observer) {
             expansionObservers.add(observer);

diff --git a/sfge/src/main/java/com/salesforce/graph/ops/expander/PathExpansionObserver.java b/sfge/src/main/java/com/salesforce/graph/ops/expander/PathExpansionObserver.java
@@ -1,9 +1,6 @@
 package com.salesforce.graph.ops.expander;
 
 import com.salesforce.graph.ApexPath;
-import com.salesforce.graph.symbols.SymbolProvider;
-import com.salesforce.graph.vertex.BaseSFVertex;
-import com.salesforce.graph.visitor.PathVertex;
 
 /**
  * Classes that implement this interface can be provided to {@link
@@ -16,16 +13,4 @@ public interface PathExpansionObserver {
 
     /** Hook invoked every time an {@link ApexPath} instance is visited. */
     void onPathVisit(ApexPath path);
-
-    /**
-     * Hook is called for the observer provided by the rule that accepts a vertex.
-     *
-     * <p>When a {@link com.salesforce.graph.vertex.VertexPredicate} accepts a vertex, the {@link
-     * ApexPathExpander} checks its {@link ApexPathExpanderConfig} to see if there is a {@link
-     * PathExpansionObserver} registered for this predicate/rule. If so, the observer's
-     * onAcceptVertex is called.
-     *
-     * <p>see {@link ApexPathExpander#shouldVisitChildren(BaseSFVertex, PathVertex)}
-     */
-    void onAcceptVertex(BaseSFVertex vertex, SymbolProvider symbols);
 }
diff --git a/sfge/src/main/java/com/salesforce/graph/vertex/VertexPredicate.java b/sfge/src/main/java/com/salesforce/graph/vertex/VertexPredicate.java
@@ -1,11 +1,22 @@
 package com.salesforce.graph.vertex;
 
+import com.salesforce.graph.symbols.SymbolProvider;
 import com.salesforce.graph.visitor.VertexPredicateVisitor;
 
 public interface VertexPredicate {
     /** Return true if the implementer is interested in this vertex. */
     boolean test(BaseSFVertex vertex);
 
+    /**
+     * Return true if the implementer is interested in this vertex.<br>
+     * <br>
+     * WARNING: the {@link SymbolProvider} parameter is experimental and may be removed in future
+     * releases. It should be used sparingly, for performance reasons.
+     */
+    default boolean test(BaseSFVertex vertex, SymbolProvider provider) {
+        return test(vertex);
+    }
+
     /** Dispatch to a {@link VertexPredicateVisitor} */
     void accept(VertexPredicateVisitor visitor);
 }
diff --git a/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java b/sfge/src/main/java/com/salesforce/rules/PathBasedRuleRunner.java
@@ -28,6 +28,7 @@ public class PathBasedRuleRunner {
     private final GraphTraversalSource g;
     private final List<AbstractPathBasedRule> allRules;
     private final List<AbstractPathAnomalyRule> anomalyRules;
+    private final List<AbstractPathTraversalRule> traversalRules;
     private final MethodVertex methodVertex;
     /** Set that holds the violations found by this rule execution. */
     private final Set<Violation> violations;
@@ -39,8 +40,11 @@ public PathBasedRuleRunner(
         this.g = g;
         this.allRules = rules;
         this.anomalyRules = new ArrayList<>();
+        this.traversalRules = new ArrayList<>();
         for (AbstractPathBasedRule rule : rules) {
-            if (rule instanceof AbstractPathAnomalyRule) {
+            if (rule instanceof AbstractPathTraversalRule) {
+                this.traversalRules.add((AbstractPathTraversalRule) rule);
+            } else if (rule instanceof AbstractPathAnomalyRule) {
                 this.anomalyRules.add((AbstractPathAnomalyRule) rule);
             }
         }
@@ -223,26 +227,13 @@ private ApexPathExpanderConfig getApexPathExpanderConfig() {
                 ApexPathUtil.getFullConfiguredPathExpanderConfigBuilder();
         for (AbstractPathBasedRule rule : allRules) {
             Optional<PathExpansionObserver> observerOptional = rule.getPathExpansionObserver();
-
-            // register the PathExpansionObserver if this rule has one
             if (observerOptional.isPresent()) {
                 expanderConfigBuilder =
                         expanderConfigBuilder.withPathExpansionObserver(observerOptional.get());
             }
-
-            if (rule instanceof AbstractPathTraversalRule) {
-                expanderConfigBuilder =
-                        expanderConfigBuilder.withVertexPredicate((AbstractPathTraversalRule) rule);
-
-                // allow the path expander to associate the VertexPredicate with a
-                // PathExpansionObserver
-                // Note: these could be the same object
-                if (rule.getPathExpansionObserver().isPresent()) {
-                    expanderConfigBuilder.withObservedPredicate(
-                            (AbstractPathTraversalRule) rule,
-                            rule.getPathExpansionObserver().get());
-                }
-            }
+        }
+        for (AbstractPathTraversalRule rule : traversalRules) {
+            expanderConfigBuilder = expanderConfigBuilder.withVertexPredicate(rule);
         }
         ApexPathExpanderConfig expanderConfig = expanderConfigBuilder.build();
         return expanderConfig;

diff --git a/sfge/src/main/java/com/salesforce/rules/PerformNullCheckOnSoqlVariables.java b/sfge/src/main/java/com/salesforce/rules/PerformNullCheckOnSoqlVariables.java
@@ -5,7 +5,6 @@
 import com.salesforce.exception.ProgrammingException;
 import com.salesforce.exception.UnexpectedException;
 import com.salesforce.graph.ApexPath;
-import com.salesforce.graph.ops.expander.PathExpansionObserver;
 import com.salesforce.graph.source.ApexPathSource;
 import com.salesforce.graph.symbols.ScopeUtil;
 import com.salesforce.graph.symbols.SymbolProvider;
@@ -18,32 +17,16 @@
 import java.util.*;
 import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
 
-public final class PerformNullCheckOnSoqlVariables extends AbstractPathTraversalRule
-        implements PathExpansionObserver {
+public final class PerformNullCheckOnSoqlVariables extends AbstractPathTraversalRule {
 
     private static final ImmutableSet<ApexPathSource.Type> SOURCE_TYPES =
             ImmutableSet.copyOf(ApexPathSource.Type.values());
 
     private static final String URL =
             "https://forcedotcom.github.io/sfdx-scanner/en/v3.x/salesforce-graph-engine/rules/#PerformNullCheckOnSoqlVariables";
 
-    /**
-     * maps to true if a {@link com.salesforce.graph.vertex.VariableExpressionVertex} should cause a
-     * violation, false otherwise. If said vertex is not in the map, it should not get a violation
-     * either.
-     */
-    private final Map<VariableExpressionVertex, Boolean> violationInfos;
-
-    // use LazyHolder to get instance, but map needs to be instantiated
-    private PerformNullCheckOnSoqlVariables() {
-        this.violationInfos = new HashMap<>();
-    }
-
-    /** This class implements {@link PathExpansionObserver}, return itself. */
-    @Override
-    public Optional<PathExpansionObserver> getPathExpansionObserver() {
-        return Optional.of(this);
-    }
+    // don't instantiate
+    private PerformNullCheckOnSoqlVariables() {}
 
     @Override
     public ImmutableSet<ApexPathSource.Type> getSourceTypes() {
@@ -74,93 +57,87 @@ public static PerformNullCheckOnSoqlVariables getInstance() {
         return LazyHolder.INSTANCE;
     }
 
-    @Override
-    public void onPathVisit(ApexPath path) {}
-
     @Override
     protected boolean isEnabled() {
         return true;
     }
 
     @Override
     public boolean test(BaseSFVertex vertex) {
-        // We only check for SOQL expressions.
-        // No need to check for MethodCallExpressionVertex because the only relevant method is
-        // Database.queryWithBinds, though that method already does null checks for the keys and
-        // values in its bind variables.
-        BaseSFVertex parent = vertex.getParent();
-        return vertex instanceof VariableExpressionVertex
-                && parent.getParent() instanceof SoqlExpressionVertex;
-    }
-
-    @Override
-    protected List<RuleThrowable> _run(
-            GraphTraversalSource g, ApexPath path, BaseSFVertex sinkVertex) {
-        if (!(sinkVertex instanceof VariableExpressionVertex)) {
-            throw new ProgrammingException(
-                    "PerformNullCheckOnSoqlVariables rule can only be applied to SoqlExpressionVertex sink vertex. Provided sink vertex="
-                            + sinkVertex);
-        }
-
-        VariableExpressionVertex vertex = (VariableExpressionVertex) sinkVertex;
-        final SFVertex sourceVertex = path.getMethodVertex().orElse(null);
-        List<RuleThrowable> violations = new ArrayList<>();
-
-        Boolean isViolation = this.violationInfos.get(vertex);
-        if (isViolation == null) {
-            throw new ProgrammingException(
-                    "PerformNullCheckOnSoqlVariables has no state information for accepted vertex "
-                            + vertex);
-        }
-
-        if (isViolation) {
-            // indeterminate or null = violation
-            violations.add(
-                    new Violation.PathBasedRuleViolation(
-                            String.format(
-                                    UserFacingMessages.PerformNullCheckOnSoqlVariablesTemplates
-                                            .MESSAGE_TEMPLATE,
-                                    vertex.getFullName()),
-                            sourceVertex,
-                            sinkVertex));
-        }
-
-        return violations;
+        return false;
     }
 
     /**
-     * Check to see if the vertex deserves a violation. If so, record true it in the violationInfos
-     * map. Else, record false.
+     * Note: this rule runs differently. Since the {@link SymbolProvider} is needed to determine
+     * whether a value might be null (and the symbol provider is unavailable during path walking and
+     * therefore the _run method), this rule only accepts vertices that are confirmed to be a
+     * violation. This is because the rule uses {@link Constraint}s to see if something has been
+     * null-checked, which are exclusively available during path expansion. Therefore, when walking
+     * the path, the rule automatically knows any accepted vertex is a violation.
      */
     @Override
-    public void onAcceptVertex(BaseSFVertex vertex, SymbolProvider symbols) {
-        if (!(vertex instanceof VariableExpressionVertex)) {
-            return;
+    public boolean test(BaseSFVertex vertex, SymbolProvider symbols) {
+
+        // We only check for SOQL expressions with variables as grandchildren.
+        // No need to check for MethodCallExpressionVertex because the only relevant method is
+        // Database.queryWithBinds, though that method already does null checks for the keys and
+        // values in its bind variables.
+        if (!(vertex instanceof VariableExpressionVertex)
+                || !(vertex.getParent().getParent() instanceof SoqlExpressionVertex)) {
+            return false;
         }
 
+        VariableExpressionVertex methodVertex = (VariableExpressionVertex) vertex;
+
         // try and find the value of this vertex
         Optional<ApexValue<?>> variableValueOpt =
-                ScopeUtil.resolveToApexValue(symbols, (VariableExpressionVertex) vertex);
+                ScopeUtil.resolveToApexValue(symbols, methodVertex);
         if (variableValueOpt.isPresent()) {
             ApexValue<?> variableValue = variableValueOpt.get();
-            VariableExpressionVertex variableVertex = (VariableExpressionVertex) vertex;
 
             if (variableValue.hasNegativeConstraint(Constraint.Null)) {
                 // negative constraint means the value is guaranteed NOT to be null,
                 // so this deserves no violation
-                violationInfos.put(variableVertex, false);
-            } else {
+                return false;
+            } else if (variableValue.isNull() || variableValue.isIndeterminant()) {
                 // if the variable is null (is actually null or has positive null constraint) OR the
                 // value is indeterminant, this deserves a violation
-                violationInfos.put(
-                        variableVertex, variableValue.isNull() || variableValue.isIndeterminant());
+                return true;
             }
 
         } else {
             throw new UnexpectedException(
                     "PerformNullCheckOnSoqlVariables couldn't find an apex value associated with variable vertex "
                             + vertex);
         }
+        return false;
+    }
+
+    /**
+     * Note: this rule runs differently. The {@link #test(BaseSFVertex, SymbolProvider)} method
+     * above only accepts vertices that are confirmed to be violations. So, this run method just
+     * needs to actually create and return the violation.
+     */
+    @Override
+    protected List<RuleThrowable> _run(
+            GraphTraversalSource g, ApexPath path, BaseSFVertex sinkVertex) {
+        if (!(sinkVertex instanceof VariableExpressionVertex)) {
+            throw new ProgrammingException(
+                    "PerformNullCheckOnSoqlVariables rule can only be applied to SoqlExpressionVertex sink vertex. Provided sink vertex="
+                            + sinkVertex);
+        }
+
+        VariableExpressionVertex vertex = (VariableExpressionVertex) sinkVertex;
+        final SFVertex sourceVertex = path.getMethodVertex().orElse(null);
+
+        return Collections.singletonList(
+                new Violation.PathBasedRuleViolation(
+                        String.format(
+                                UserFacingMessages.PerformNullCheckOnSoqlVariablesTemplates
+                                        .MESSAGE_TEMPLATE,
+                                vertex.getFullName()),
+                        sourceVertex,
+                        sinkVertex));
     }
 
     // lazy holder