Fix GitHubDspIngestionVisitor #2061
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,17 +2,29 @@ | |
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.Sarif.Visitors | ||
| { | ||
| public class GitHubDspIngestionVisitor : SarifRewritingVisitor | ||
| { | ||
| // DSP requires that every related location have a message. It's not clear why this | ||
| // requirement exists, as this data is mostly used to build embedded links from | ||
| // results (where the link anchor text actually resides). | ||
| private const string PlaceholderRelatedLocationMessage = "[No message provided.]"; | ||
|
|
||
| // GitHub DSP reportedly has an ingestion limit of 500 issues. | ||
| // Internal static rather than private const to allow a unit test with a practical limit. | ||
| internal static int s_MaxResults = 500; | ||
|
|
||
| private IList<Artifact> artifacts; | ||
| private IList<ThreadFlowLocation> threadFlowLocations; | ||
|
|
||
| public override Run VisitRun(Run node) | ||
| { | ||
| this.artifacts = node.Artifacts; | ||
| this.threadFlowLocations = node.ThreadFlowLocations; | ||
|
|
||
| // DSP does not support submitting invocation objects. Invocations | ||
| // contains potentially sensitive environment details, such as | ||
| // account names embedded in paths. Invocations also store | ||
|
|
@@ -31,7 +43,7 @@ public override Run VisitRun(Run node) | |
| errorsCount++; | ||
| } | ||
| } | ||
|
|
||
| if (errorsCount != node.Results.Count) | ||
| { | ||
| var errors = new List<Result>(); | ||
|
|
@@ -43,32 +55,103 @@ public override Run VisitRun(Run node) | |
| errors.Add(result); | ||
| } | ||
|
|
||
| if (errors.Count == s_MaxResults) { break; } | ||
| } | ||
|
|
||
| node.Results = errors; | ||
| } | ||
|
|
||
| if (node.Results.Count > s_MaxResults) | ||
| { | ||
| node.Results = node.Results.Take(s_MaxResults).ToList(); | ||
| } | ||
| } | ||
|
|
||
| node = base.VisitRun(node); | ||
|
|
||
| // DSP prefers a relative path local to the result. We clear | ||
| // the artifacts table, as all artifact information is now | ||
| // inlined with each result. | ||
| node.Artifacts = null; | ||
|
|
||
| // DSP requires threadFlowLocations to be inlined in the result, | ||
| // not referenced from run.threadFlowLocations. | ||
| node.ThreadFlowLocations = null; | ||
|
|
||
| return node; | ||
| } | ||
|
|
||
| public override ThreadFlowLocation VisitThreadFlowLocation(ThreadFlowLocation node) | ||
| { | ||
| if (this.threadFlowLocations != null && node.Index > -1) | ||
| { | ||
| ThreadFlowLocation sharedLocation = this.threadFlowLocations[node.Index]; | ||
|
|
||
| // Location.Message might vary per usage. For example, on one usage, | ||
| // we might be tracking an uninitialized variable, and the location | ||
| // might have the message "Uninitialized variable 'ptr' passed to 'f'". | ||
| // Another usage of the same location might have a different message, | ||
| // or none at all. So even though we are getting the location from | ||
| // the shared object, don't take its message unless the current object | ||
| // does not have one. | ||
| Message message = node.Location?.Message; | ||
|
|
||
| node.Location = sharedLocation.Location; | ||
|
|
||
| if (message != null) | ||
| { | ||
| // Make sure there's a place to put the message. threadFlowLocation.Location | ||
| // is not required, so the shared object might not have had one. | ||
| if (node.Location == null) | ||
| { | ||
| node.Location = new Location(); | ||
| } | ||
|
|
||
| node.Location.Message = message; | ||
| } | ||
|
|
||
| // Copy other properties that should be the same for each usage of the | ||
| // shared location. | ||
| node.Kinds = sharedLocation.Kinds; | ||
| node.Module = sharedLocation.Module; | ||
|
|
||
| // Merge properties from the shared location, preferring the properties from | ||
| // this location if there are any duplicates. | ||
| MergeProperties(node, sharedLocation); | ||
|
|
||
| // Do NOT copy properties that can be different for each use of the shared | ||
| // threadFlowLocation: | ||
| // - ExecutionOrder | ||
| // - ExecutionTimeUtc | ||
| // - Importance | ||
| // - NestingLevel | ||
| // - Stack | ||
| // - State | ||
| // - Taxa | ||
| // - WebRequest | ||
| // - WebResponse | ||
|
|
||
| node.Index = -1; | ||
| } | ||
|
|
||
| return base.VisitThreadFlowLocation(node); | ||
| } | ||
|
|
||
| public override ArtifactLocation VisitArtifactLocation(ArtifactLocation node) | ||
| { | ||
| if (this.artifacts != null && node.Index > -1) | ||
| { | ||
| ArtifactLocation sharedLocation = this.artifacts[node.Index].Location; | ||
| node.Uri = sharedLocation.Uri; | ||
| node.UriBaseId = sharedLocation.UriBaseId; | ||
|
|
||
| // Merge properties from the shared location, preferring the properties from | ||
| // this location if there are any duplicates. | ||
| MergeProperties(node, sharedLocation); | ||
|
|
||
| node.Index = -1; | ||
| } | ||
|
|
||
| return base.VisitArtifactLocation(node); | ||
| } | ||
|
|
||
|
|
@@ -78,15 +161,11 @@ public override Result VisitResult(Result node) | |
| { | ||
| foreach (Location relatedLocation in node.RelatedLocations) | ||
| { | ||
| if (string.IsNullOrEmpty(relatedLocation.Message?.Text)) | ||
| { | ||
| relatedLocation.Message = new Message | ||
| { | ||
| Text = PlaceholderRelatedLocationMessage | ||
|
There was a problem hiding this comment.
I thought that Messages could have either a Text or Id property to define the string. Here, if the message contains an Id, we're adding a text string. Is that OK? #ByDesign There was a problem hiding this comment. ... I should have added, I realize the previous code did this too, but I thought I'd ask anyway. In reply to: 483750179 [](ancestors = 483750179) There was a problem hiding this comment. It's a good question. We're compensating here for a deficiency in the DSP -- the DSP looks for the In reply to: 483750179 [](ancestors = 483750179) There was a problem hiding this comment. ... when in fact it shouldn't care about the presence of related location messages at all. In reply to: 483751157 [](ancestors = 483751157,483750179) |
||
| }; | ||
| } | ||
| } | ||
|
|
@@ -98,17 +177,32 @@ public override Result VisitResult(Result node) | |
| // partial fingerprints property in order to prefer these | ||
| // values for matching (over DSP's built-in SARIF-driven | ||
| // results matching heuristics). | ||
| foreach (string fingerprintKey in node.Fingerprints.Keys) | ||
| { | ||
| node.PartialFingerprints ??= new Dictionary<string, string>(); | ||
| node.PartialFingerprints[fingerprintKey] = node.Fingerprints[fingerprintKey]; | ||
| } | ||
| node.Fingerprints = null; | ||
| } | ||
|
|
||
| return base.VisitResult(node); | ||
| } | ||
|
|
||
| // Merge properties from a source to a target, preferring the existing properties | ||
| // on the target if there are any duplicates. | ||
| private static void MergeProperties(PropertyBagHolder target, PropertyBagHolder source) | ||
| { | ||
| // Are there any properties to merge? | ||
| if (source.Properties != null) | ||
| { | ||
| // If so, make sure there's someplace to put them. | ||
| if (target.Properties == null) | ||
| { | ||
| target.Properties = new Dictionary<string, SerializedPropertyInfo>(); | ||
| } | ||
|
|
||
| target.Properties = target.Properties.MergePreferFirst(source.Properties); | ||
| } | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| { | ||
| "$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.5.json", | ||
| "version": "2.1.0", | ||
| "runs": [ | ||
| { | ||
| "tool": { | ||
| "driver": { | ||
| "name": "Sarif.UnitTests" | ||
| } | ||
| }, | ||
| "results": [ | ||
| { | ||
| "ruleId": "TEST1001", | ||
| "level": "error", | ||
| "message": { | ||
| "text": "The message." | ||
| }, | ||
| "partialFingerprints": { | ||
| "f1": "f1-value", | ||
| "f2": "f2-value" | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| { | ||
| "$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.5.json", | ||
| "version": "2.1.0", | ||
| "runs": [ | ||
| { | ||
| "tool": { | ||
| "driver": { | ||
| "name": "Sarif.UnitTests" | ||
| } | ||
| }, | ||
| "results": [ | ||
| { | ||
| "ruleId": "TEST1002", | ||
| "level": "error", | ||
| "message": { | ||
| "text": "The message." | ||
| } | ||
| }, | ||
| { | ||
| "ruleId": "TEST1003", | ||
| "level": "error", | ||
| "message": { | ||
| "text": "The message." | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Moved to ExtensionMethods.cs because the
GitHubDspIngestionVisitornow needs it, too. #ByDesign