From ca004db9b22730d2bb7ee8ae521dbb51bc10b30d Mon Sep 17 00:00:00 2001 From: Leo Horwitz Date: Wed, 23 Aug 2023 11:57:38 -0700 Subject: [PATCH] @W-13569674@: remove state and PathExpansionObserver from null check rule --- .../graph/ops/expander/ApexPathExpander.java | 32 +++-- .../ops/expander/ApexPathExpanderConfig.java | 21 --- .../ops/expander/PathExpansionObserver.java | 15 -- .../graph/vertex/VertexPredicate.java | 11 ++ .../salesforce/rules/PathBasedRuleRunner.java | 25 ++-- .../PerformNullCheckOnSoqlVariables.java | 129 +++++++----------- .../unusedmethod/operations/UsageTracker.java | 8 -- 7 files changed, 91 insertions(+), 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 index cc026ad45..2720a8eb0 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 @@ -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 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 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 index 06dd170c0..5697488bd 100644 --- 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 private final List conditionExcluders; private final List valueConstrainers; private final List vertexPredicates; - private final Map predicateToObserver; private final List 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 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 getObserverFromPredicate(VertexPredicate predicate) { - return Optional.ofNullable(this.predicateToObserver.get(predicate)); - } - public List 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 index 4d316713b..f3441cde2 100644 --- 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. - * - *

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. - * - *

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 index 48344f650..27fa8025c 100644 --- 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.
+ *
+ * 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 index ef5ed6466..397bf6b6f 100644 --- 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 allRules; private final List anomalyRules; + private final List traversalRules; private final MethodVertex methodVertex; /** Set that holds the violations found by this rule execution. */ private final Set 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 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 index df850201a..f8908ce9e 100644 --- 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,8 +17,7 @@ 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 SOURCE_TYPES = ImmutableSet.copyOf(ApexPathSource.Type.values()); @@ -27,23 +25,8 @@ public final class PerformNullCheckOnSoqlVariables extends AbstractPathTraversal 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 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 getPathExpansionObserver() { - return Optional.of(this); - } + // don't instantiate + private PerformNullCheckOnSoqlVariables() {} @Override public ImmutableSet getSourceTypes() { @@ -74,9 +57,6 @@ public static PerformNullCheckOnSoqlVariables getInstance() { return LazyHolder.INSTANCE; } - @Override - public void onPathVisit(ApexPath path) {} - @Override protected boolean isEnabled() { return true; @@ -84,76 +64,45 @@ protected boolean isEnabled() { @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 _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 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> 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 { @@ -161,6 +110,34 @@ public void onAcceptVertex(BaseSFVertex vertex, SymbolProvider symbols) { "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 _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 diff --git a/sfge/src/main/java/com/salesforce/rules/unusedmethod/operations/UsageTracker.java b/sfge/src/main/java/com/salesforce/rules/unusedmethod/operations/UsageTracker.java index 6d0160f3b..db40958a1 100644 --- a/sfge/src/main/java/com/salesforce/rules/unusedmethod/operations/UsageTracker.java +++ b/sfge/src/main/java/com/salesforce/rules/unusedmethod/operations/UsageTracker.java @@ -2,8 +2,6 @@ import com.salesforce.graph.ApexPath; import com.salesforce.graph.ops.expander.PathExpansionObserver; -import com.salesforce.graph.symbols.SymbolProvider; -import com.salesforce.graph.vertex.BaseSFVertex; import com.salesforce.graph.vertex.MethodVertex; import com.salesforce.rules.RemoveUnusedMethod; import java.util.Optional; @@ -23,12 +21,6 @@ public void onPathVisit(ApexPath path) { methodOptional.ifPresent(this::markAsUsed); } - /** onAcceptVertex is irrelevant to {@link RemoveUnusedMethod} rule */ - @Override - public void onAcceptVertex(BaseSFVertex vertex, SymbolProvider symbols) { - // intentional no-op - } - /** Mark the provided {@link MethodVertex} as one with a confirmed usage. */ private void markAsUsed(MethodVertex methodVertex) { UsageTrackerDataProvider.add(methodVertex.generateUniqueKey());