-
Notifications
You must be signed in to change notification settings - Fork 54
NEW (GraphEngine): @W-11989381@: New MultipleMassSchemaLookupRule to detect performance degrading schema lookups. #1054
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7392501
432d5a7
33d27c7
905f2e4
d21844a
4a48ffc
0946b07
81eb5d3
3b1b738
8524450
3c556cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| package com.salesforce.rules; | ||
|
|
||
| import com.salesforce.config.UserFacingMessages; | ||
| import com.salesforce.graph.ApexPath; | ||
| import com.salesforce.graph.vertex.BaseSFVertex; | ||
| import com.salesforce.rules.multiplemassschemalookup.MultipleMassSchemaLookupRuleHandler; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource; | ||
|
|
||
| /** Rule to detect possible performance degradations while invoking Schema.getGlobalDescribe(). */ | ||
| public class MultipleMassSchemaLookupRule extends AbstractPathTraversalRule { | ||
|
|
||
| private final MultipleMassSchemaLookupRuleHandler ruleHandler; | ||
|
|
||
| private MultipleMassSchemaLookupRule() { | ||
| ruleHandler = MultipleMassSchemaLookupRuleHandler.getInstance(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean test(BaseSFVertex vertex) { | ||
| return ruleHandler.test(vertex); | ||
| } | ||
|
|
||
| @Override | ||
| protected List<RuleThrowable> _run(GraphTraversalSource g, ApexPath path, BaseSFVertex vertex) { | ||
| List<RuleThrowable> violations = new ArrayList<>(); | ||
|
|
||
| violations.addAll(ruleHandler.detectViolations(g, path, vertex)); | ||
|
|
||
| return violations; | ||
| } | ||
|
|
||
| @Override | ||
| protected int getSeverity() { | ||
| return SEVERITY.HIGH.code; | ||
| } | ||
|
|
||
| @Override | ||
| protected String getDescription() { | ||
| return UserFacingMessages.RuleDescriptions.MULTIPLE_MASS_SCHEMA_LOOKUP_RULE; | ||
| } | ||
|
|
||
| @Override | ||
| protected String getCategory() { | ||
| return CATEGORY.PERFORMANCE.name; | ||
| } | ||
|
|
||
| @Override | ||
| protected boolean isEnabled() { | ||
| return false; | ||
| } | ||
|
|
||
| public static MultipleMassSchemaLookupRule getInstance() { | ||
| return MultipleMassSchemaLookupRule.LazyHolder.INSTANCE; | ||
| } | ||
|
|
||
| private static final class LazyHolder { | ||
| // Postpone initialization until first use | ||
| private static final MultipleMassSchemaLookupRule INSTANCE = | ||
| new MultipleMassSchemaLookupRule(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |
| import com.salesforce.graph.vertex.SFVertex; | ||
| import com.salesforce.rules.fls.apex.operations.FlsViolationInfo; | ||
| import com.salesforce.rules.fls.apex.operations.FlsViolationMessageUtil; | ||
| import com.salesforce.rules.multiplemassschemalookup.MultipleMassSchemaLookupInfo; | ||
| import com.salesforce.rules.ops.ProgressListener; | ||
| import com.salesforce.rules.ops.ProgressListenerProvider; | ||
| import java.util.*; | ||
|
|
@@ -119,7 +120,8 @@ private void executeRulesOnPaths(List<ApexPath> paths) { | |
| // This set holds the violations whose information couldn't be fully processed at creation | ||
| // time, and | ||
| // require post-processing. | ||
| final HashSet<FlsViolationInfo> incompleteThrowables = new HashSet<>(); | ||
| final HashSet<FlsViolationInfo> flsViolationInfos = new HashSet<>(); | ||
| final HashSet<MultipleMassSchemaLookupInfo> mmsLookupInfos = new HashSet<>(); | ||
| // For each path... | ||
| for (ApexPath path : paths) { | ||
| // If the path's metadata is present... | ||
|
|
@@ -142,7 +144,10 @@ private void executeRulesOnPaths(List<ApexPath> paths) { | |
| // to the list of such | ||
| // objects. | ||
| if (ruleThrowable instanceof FlsViolationInfo) { | ||
| incompleteThrowables.add((FlsViolationInfo) ruleThrowable); | ||
| flsViolationInfos.add((FlsViolationInfo) ruleThrowable); | ||
| } else if (ruleThrowable instanceof MultipleMassSchemaLookupInfo) { | ||
| // FIXME: PR incoming with refactors to this portion | ||
| mmsLookupInfos.add((MultipleMassSchemaLookupInfo) ruleThrowable); | ||
|
Comment on lines
146
to
+150
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's some duplication here that can be streamlined.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm assuming the subsequent PR will involve refactoring this so it doesn't require a Set of MMS violations specifically?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I'm trying to move the ownership of conversion to the ViolationInfo object so that this portion can work on the higher-level interface. |
||
| } else if (ruleThrowable instanceof Violation) { | ||
| // If the violation is done, it can just go directly into the results | ||
| // set. | ||
|
|
@@ -155,7 +160,8 @@ private void executeRulesOnPaths(List<ApexPath> paths) { | |
| } | ||
| } | ||
|
|
||
| convertToViolations(incompleteThrowables); | ||
| convertFlsInfoToViolations(flsViolationInfos); | ||
| convertMmsInfoToViolations(mmsLookupInfos); | ||
|
|
||
| if (!foundVertex) { | ||
| // If no vertices were found, we should log something so that information isn't lost, | ||
|
|
@@ -167,7 +173,16 @@ private void executeRulesOnPaths(List<ApexPath> paths) { | |
| } | ||
| } | ||
|
|
||
| private void convertToViolations(HashSet<FlsViolationInfo> flsViolationInfos) { | ||
| private void convertMmsInfoToViolations(HashSet<MultipleMassSchemaLookupInfo> mmsLookupInfos) { | ||
| for (MultipleMassSchemaLookupInfo mmsLookupInfo : mmsLookupInfos) { | ||
| Violation.RuleViolation violation = mmsLookupInfo.convert(); | ||
| violation.setPropertiesFromRule(MultipleMassSchemaLookupRule.getInstance()); | ||
| violations.add(violation); | ||
| } | ||
| } | ||
|
|
||
| // TODO: Restructure to make this logic work on other types of violation info too | ||
| private void convertFlsInfoToViolations(HashSet<FlsViolationInfo> flsViolationInfos) { | ||
| // Consolidate/regroup FLS violations across paths so that there are no | ||
| // duplicates with different field sets for the same source/sink/dmlOperation | ||
| final HashSet<FlsViolationInfo> consolidatedFlsViolationInfos = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| package com.salesforce.rules.multiplemassschemalookup; | ||
|
|
||
| import com.salesforce.config.UserFacingMessages; | ||
| import com.salesforce.graph.vertex.MethodCallExpressionVertex; | ||
| import com.salesforce.graph.vertex.SFVertex; | ||
|
|
||
| /** Utility to help with violation message creation on AvoidMassSchemaLookupRule */ | ||
| public final class MassSchemaLookupInfoUtil { | ||
| private MassSchemaLookupInfoUtil() {} | ||
|
|
||
| public static String getMessage(MultipleMassSchemaLookupInfo info) { | ||
| return getMessage( | ||
| info.getSinkVertex().getFullMethodName(), | ||
| info.getRepetitionType(), | ||
| getOccurrenceInfoValue(info.getRepetitionType(), info.getRepetitionVertex()), | ||
| info.getRepetitionVertex().getDefiningType(), | ||
| info.getRepetitionVertex().getBeginLine()); | ||
| } | ||
|
|
||
| public static String getMessage( | ||
| String sinkMethodName, | ||
| RuleConstants.RepetitionType repetitionType, | ||
| String occurrenceInfoValue, | ||
| String occurrenceClassName, | ||
| int occurrenceLine) { | ||
| final String occurrenceMessage = getOccurrenceMessage(repetitionType, occurrenceInfoValue); | ||
|
|
||
| return String.format( | ||
| UserFacingMessages.MultipleMassSchemaLookupRuleTemplates.MESSAGE_TEMPLATE, | ||
| sinkMethodName, | ||
| occurrenceMessage, | ||
| occurrenceClassName, | ||
| occurrenceLine); | ||
| } | ||
|
|
||
| private static String getOccurrenceInfoValue( | ||
| RuleConstants.RepetitionType repetitionType, SFVertex repetitionVertex) { | ||
| if (RuleConstants.RepetitionType.MULTIPLE.equals(repetitionType)) { | ||
| // Use method name on template message | ||
| return ((MethodCallExpressionVertex) repetitionVertex).getFullMethodName(); | ||
| } else { | ||
| // Use Loop type on template message | ||
| return repetitionVertex.getLabel(); | ||
| } | ||
| } | ||
|
|
||
| private static String getOccurrenceMessage( | ||
| RuleConstants.RepetitionType repetitionType, String value) { | ||
| return repetitionType.getMessage(value); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved UI text.