Merge pull request #679 from forcedotcom/d/W-11111352

@W-11111352@: SARIF and Table format now properly format output in the event of SFGE errors.

sfge/src/main/java/com/salesforce/rules/Violation.java

@@ -6,6 +6,9 @@
 import java.util.Objects;
 
 public abstract class Violation implements Comparable<Violation>, RuleThrowable {
+    private static final String INTERNAL_ERROR_RULENAME = "InternalExecutionError";
+    private static final String INTERNAL_ERROR_CATEGORY = "InternalExecutionError";
+
     /** The simple violation message that will be conveyed to users of the plugin. */
     protected final String message;
 
@@ -323,8 +326,8 @@ public InternalErrorViolation(final String details, final SFVertex vertex) {
             // Internal error violations don't have a rule associated with them, so we should set
             // the rule-related properties
             // to non-null values now, to avoid NPEs during sorting.
-            this.ruleName = "";
-            this.category = "";
+            this.ruleName = INTERNAL_ERROR_RULENAME;
+            this.category = INTERNAL_ERROR_CATEGORY;
             this.description = "";
             this.severity = AbstractRule.SEVERITY.LOW.code;
         }
@@ -404,8 +407,8 @@ public TimeoutViolation(final String message, final SFVertex vertex) {
             // Timeout violations don't have an actual rule associated with them, so we should set
             // the rule-related properties
             // to non-null values now, to avoid NPEs during sorting.
-            this.ruleName = "";
-            this.category = "";
+            this.ruleName = INTERNAL_ERROR_RULENAME;
+            this.category = INTERNAL_ERROR_CATEGORY;
             this.description = "";
             this.severity = AbstractRule.SEVERITY.LOW.code;
         }

src/lib/formatter/RuleResultRecombinator.ts

@@ -30,9 +30,9 @@ type DfaTableRow = BaseTableRow & {
 	"Source Location": string;
 	"Source Line": number;
 	"Source Column": number;
-	"Sink Location": string;
-	"Sink Line": number;
-	"Sink Column": number;
+	"Sink Location": string|null;
+	"Sink Line": number|null;
+	"Sink Column": number|null;
 };
 
 export class RuleResultRecombinator {
@@ -307,13 +307,26 @@ URL: ${url}`;
 					Severity: violation.severity,
 					Engine: result.engine
 				};
+				// A pathless violation can be trivially converted into a row.
 				if (isPathlessViolation(violation)) {
 					rows.push({
 						...baseRow,
 						Location: `${relativeFile}:${violation.line}`,
 						Line: violation.line,
 						Column: violation.column
 					});
+				} else if (!(violation.sinkFileName)) {
+					// If the violation is path-based but has no sink file, then the violation indicates an error of some
+					// kind. So use the source information we were given, but use null for everything else.
+					rows.push({
+						...baseRow,
+						"Source Location": `${relativeFile}:${violation.sourceLine}`,
+						"Source Line": violation.sourceLine,
+						"Source Column": violation.sourceColumn,
+						"Sink Location": "",
+						"Sink Line": null,
+						"Sink Column": null
+					});
 				} else {
 					const relativeSinkFile = path.relative(process.cwd(), violation.sinkFileName);
 					rows.push({

src/lib/formatter/SarifFormatter.ts

@@ -160,7 +160,7 @@ abstract class SarifFormatter {
 					result.locations = [location];
 
 					// If the violation is DFA, we'll want to create a secondary location for the sink vertex.
-					if (!isPathless) {
+					if (!isPathless && v.sinkFileName) {
 						const secondaryLocation = SarifFormatter.getLocation(v.sinkFileName);
 						secondaryLocation.message = {
 							text: SINK_VERTEX_MESSAGE

src/lib/sfge/SfgeEngine.ts

@@ -243,9 +243,9 @@ export class SfgeEngine extends AbstractRuleEngine {
 				severity: sfgeViolation.severity,
 				message: sfgeViolation.message,
 				category: sfgeViolation.category,
-				sinkLine: sfgeViolation.sinkLineNumber,
-				sinkColumn: sfgeViolation.sinkColumnNumber,
-				sinkFileName: sfgeViolation.sinkFileName,
+				sinkLine: sfgeViolation.sinkLineNumber || null,
+				sinkColumn: sfgeViolation.sinkColumnNumber || null,
+				sinkFileName: sfgeViolation.sinkFileName || "",
 				sourceLine: sfgeViolation.sourceLineNumber,
 				sourceColumn: sfgeViolation.sourceColumnNumber,
 				sourceType: sfgeViolation.sourceType,

src/types.d.ts

@@ -102,9 +102,9 @@ export type DfaRuleViolation = BaseViolation & {
 	sourceColumn: number;
 	sourceType: string;
 	sourceMethodName: string;
-	sinkLine: number;
-	sinkColumn: number;
-	sinkFileName: string;
+	sinkLine: number|null;
+	sinkColumn: number|null;
+	sinkFileName: string|null;
 };
 
 export type RuleViolation = PathlessRuleViolation | DfaRuleViolation;

test/lib/formatter/RuleResultRecombinator.test.ts

@@ -244,6 +244,28 @@ const allFakeDfaRuleResults: RuleResult[] = [
 	}
 ];
 
+const fakeDfaWithError: RuleResult[] = [
+	{
+		engine: 'sfge',
+		fileName: sampleFile5,
+		violations: [{
+			"sourceLine": 15,
+			"sourceColumn": 8,
+			"sourceType": "BearController",
+			"sourceMethodName": "getAllBears",
+			"sinkFileName": "",
+			"sinkLine": null,
+			"sinkColumn": null,
+			"ruleName": "InternalExecutionError",
+			"severity": 3,
+			"message": "Path evaluation timed out after 40 ms",
+			"url": "https://thisurldoesnotmatter.com",
+			"category": "InternalExecutionError"
+		}]
+	}
+]
+
+
 const allFakeDfaRuleResultsNormalized: RuleResult[] = [
 	{
 		engine: 'sfge',
@@ -525,7 +547,7 @@ describe('RuleResultRecombinator', () => {
 				expect(invocation.toolExecutionNotifications).to.have.lengthOf(0, 'Tool Execution');
 			}
 
-			function validateSfgeSarif(run: unknown, normalizeSeverity: boolean): void {
+			function validateSfgeSarif(run: unknown, normalizeSeverity: boolean, expectingErrors: boolean): void {
 				const driver = run['tool']['driver'];
 				expect(driver.name).to.equal(ENGINE.SFGE);
 				expect(driver.version).to.equal(SFGE_VERSION);
@@ -534,7 +556,11 @@ describe('RuleResultRecombinator', () => {
 
 				// tool.driver.rules
 				expect(driver['rules']).to.have.lengthOf(1, 'Incorrect rule count');
-				expect(driver['rules'][0].id).to.equal('ApexFlsViolationRule');
+				if (expectingErrors) {
+					expect(driver['rules'][0].id).to.equal('InternalExecutionError');
+				} else {
+					expect(driver['rules'][0].id).to.equal('ApexFlsViolationRule');
+				}
 				if (normalizeSeverity) {
 					expect(driver['rules'][0].properties.normalizedSeverity).to.equal(1);
 				} else {
@@ -546,7 +572,11 @@ describe('RuleResultRecombinator', () => {
 				expect(run['results']).to.have.lengthOf(1, 'Incorrect result count');
 				const results = run['results'];
 				expect(results[0].ruleIndex).to.equal(0);
-				expect(results[0].relatedLocations).to.exist;
+				if (expectingErrors) {
+					expect(results[0].relatedLocations).to.not.exist;
+				} else {
+					expect(results[0].relatedLocations).to.exist;
+				}
 
 				// Invocations
 				expect(run['invocations']).to.have.lengthOf(1, 'Invocations');
@@ -613,7 +643,7 @@ describe('RuleResultRecombinator', () => {
 					const run = runs[i];
 					const engine = run['tool']['driver']['name'];
 					if (engine === ENGINE.SFGE) {
-						validateSfgeSarif(run, false);
+						validateSfgeSarif(run, false, false);
 					} else {
 						fail(`Unexpected engine: ${engine}`);
 					}
@@ -634,7 +664,7 @@ describe('RuleResultRecombinator', () => {
 					const run = runs[i];
 					const engine = run['tool']['driver']['name'];
 					if (engine === ENGINE.SFGE) {
-						validateSfgeSarif(run, true);
+						validateSfgeSarif(run, true, false);
 					} else {
 						fail(`Unexpected engine: ${engine}`);
 					}
@@ -645,6 +675,27 @@ describe('RuleResultRecombinator', () => {
 				expect(summaryMap.get('sfge')).to.deep.equal({fileCount: 1, violationCount: 1}, 'Sfge summary should be correct');
 			});
 
+			it ('DFA with timeout error', async () => {
+				const {minSev, results, summaryMap} = await RuleResultRecombinator.recombineAndReformatResults(fakeDfaWithError, OUTPUT_FORMAT.SARIF, new Set(['sfge']));
+				const sarifResults: unknown[] = JSON.parse(results as string);
+				expect(sarifResults['runs']).to.have.lengthOf(1, 'Runs');
+				const runs = sarifResults['runs'];
+
+				for (let i = 0; i < runs.length; i++) {
+					const run = runs[i];
+					const engine = run['tool']['driver']['name'];
+					if (engine === ENGINE.SFGE) {
+						validateSfgeSarif(run, false, true);
+					} else {
+						fail(`Unexpected engine: ${engine}`);
+					}
+				}
+
+				expect(minSev).to.equal(3, 'Most severe problem');
+				expect(summaryMap.size).to.equal(1, 'Each supposedly executed engine needs a summary');
+				expect(summaryMap.get('sfge')).to.deep.equal({fileCount: 1, violationCount: 1}, 'Sfge summary should be correct');
+			});
+
 			it('Handles all pathless engines', async () => {
 				const allEngines = PathlessEngineFilters.map(engine => engine.valueOf());
 				for (const engine of allEngines) {