-
Notifications
You must be signed in to change notification settings - Fork 54
@W-10778080@ Handle unresolved apex value in DML statements gracefully #785
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
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 |
|---|---|---|
|
|
@@ -4,7 +4,6 @@ | |
| import com.google.common.collect.Multimap; | ||
| import com.google.common.collect.Sets; | ||
| import com.salesforce.config.SfgeConfigProvider; | ||
| import com.salesforce.exception.TodoException; | ||
| import com.salesforce.exception.UnexpectedException; | ||
| import com.salesforce.graph.symbols.ScopeUtil; | ||
| import com.salesforce.graph.symbols.SymbolProvider; | ||
|
|
@@ -156,18 +155,31 @@ public void createExpectedValidations(DmlStatementVertex vertex, SymbolProvider | |
| final BaseSFVertex childVertex = dmlStatementVertexChildren.get(0); | ||
|
|
||
| final ValidationConverter validationConverter = new ValidationConverter(validationType); | ||
| final Set<FlsValidationRepresentation> validationReps; | ||
| final Set<FlsValidationRepresentation> validationReps = new HashSet<>(); | ||
| if (childVertex instanceof ChainedVertex) { | ||
| final Optional<ApexValue<?>> apexValue = | ||
| ScopeUtil.resolveToApexValue(symbols, (ChainedVertex) childVertex); | ||
| if (apexValue.isPresent()) { | ||
| validationReps = validationConverter.convertToExpectedValidations(apexValue.get()); | ||
| validationReps.addAll( | ||
| validationConverter.convertToExpectedValidations(apexValue.get())); | ||
| } else { | ||
| throw new TodoException( | ||
| "Apex value not detected for dml's child vertex: " + childVertex); | ||
| if (LOGGER.isWarnEnabled()) { | ||
| LOGGER.warn( | ||
| "TODO: Apex value not detected for dml's child vertex: " + childVertex); | ||
| // TODO: add telemetry | ||
| } | ||
| violations.add( | ||
| FlsViolationCreatorUtil.createUnresolvedCrudFlsViolation( | ||
| validationType, vertex)); | ||
| } | ||
| } else { | ||
| throw new TodoException("Child vertex of DML is not a chained vertex: " + childVertex); | ||
| if (LOGGER.isWarnEnabled()) { | ||
| LOGGER.warn("TODO: Child vertex of DML is not a chained vertex: " + childVertex); | ||
| // TODO: add telemetry | ||
| } | ||
| violations.add( | ||
| FlsViolationCreatorUtil.createUnresolvedCrudFlsViolation( | ||
| validationType, vertex)); | ||
|
Comment on lines
+180
to
+182
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. This is possibly an unexpected situation - we haven't seen it anywhere or had a repro, but I'm modifying it so that users don't see an internal error for this. |
||
| } | ||
|
|
||
| expectedValidations.addAll(validationReps); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -138,4 +138,85 @@ public void testReadWithUnresolvedBinaryExpression() { | |
| sourceCode, | ||
| expectUnresolvedCrudFls(3, FlsConstants.FlsValidationType.READ)); | ||
| } | ||
|
|
||
|
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. Adding more test cases to get a repro of how far we can resolve apex values. |
||
| @Test | ||
| public void testDmlOnSecondLevelObject() { | ||
| String sourceCode = | ||
| "public class MyClass {\n" | ||
| + " public void foo(My_Obj__c myObj) {\n" | ||
| + " myObj.Custom_Field__c.Name = 'Acme Inc.';\n" | ||
| + " update myObj.Custom_Field__c;\n" | ||
| + " }\n" | ||
| + "}\n"; | ||
|
|
||
| assertViolations( | ||
| ApexFlsViolationRule.getInstance(), | ||
| sourceCode, | ||
| expect(4, FlsConstants.FlsValidationType.UPDATE, "Custom_Field__c")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testDmlOnReferenceObject() { | ||
| String sourceCode = | ||
| "public class MyClass {\n" | ||
| + " public void foo(My_Obj__c myObj) {\n" | ||
| + " myObj.Custom_Field__c.Reference__r.Name = 'Acme Inc.';\n" | ||
| + " update myObj.Custom_Field__c.Reference__r;\n" | ||
| + " }\n" | ||
| + "}\n"; | ||
|
|
||
| assertViolations( | ||
| ApexFlsViolationRule.getInstance(), | ||
| sourceCode, | ||
| expectUnresolvedCrudFls(4, FlsConstants.FlsValidationType.UPDATE)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testDmlOnMethodReturnValue() { | ||
| String sourceCode = | ||
| "public class MyClass {\n" | ||
| + " public void foo() {\n" | ||
| + " /* sfge-disable-next-line ApexFlsViolationRule */\n" | ||
| + " List<SObject> accounts = [SELECT Id, Name FROM Account WHERE Type='something'];\n" | ||
| + " Map<String, List<SObject>> listByType = new Map<String, List<SObject>>();\n" | ||
| + " listByType.put('Account', accounts);\n" | ||
| + " delete listByType.get('Account');\n" | ||
| + " }\n" | ||
| + "}\n"; | ||
|
|
||
| assertViolations( | ||
| ApexFlsViolationRule.getInstance(), | ||
| sourceCode, | ||
| expect(7, FlsConstants.FlsValidationType.DELETE, "Account")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testDmlOnMethodReturnValueIndeterminant() { | ||
| String sourceCode = | ||
| "public class MyClass {\n" | ||
| + " public void foo(Map<String, List<SObject>> listByType) {\n" | ||
| + " delete listByType.get('Account');\n" | ||
| + " }\n" | ||
| + "}\n"; | ||
|
|
||
| assertViolations( | ||
| ApexFlsViolationRule.getInstance(), | ||
| sourceCode, | ||
| expect(3, FlsConstants.FlsValidationType.DELETE, "SObject")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testDmlOnMethodInvokedbyMethod() { | ||
| String sourceCode = | ||
| "public class MyClass {\n" | ||
| + " public void foo(Map<String, List<SObject>> listByType, Schema.SObjectType sObjectType) {\n" | ||
| + " delete listByType.get(sObjectType.getDescribe().getName());\n" | ||
| + " }\n" | ||
| + "}\n"; | ||
|
|
||
| assertViolations( | ||
| ApexFlsViolationRule.getInstance(), | ||
| sourceCode, | ||
| expect(3, FlsConstants.FlsValidationType.DELETE, "SObject")); | ||
| } | ||
| } | ||
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.
Related action happens here