diff --git a/src/Sarif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/ExpectedOutputs/OneRunWithRules.sarif b/src/Sarif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/ExpectedOutputs/OneRunWithRules.sarif index 2b518bc8c..c43fcdec4 100644 --- a/src/Sarif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/ExpectedOutputs/OneRunWithRules.sarif +++ b/src/Sarif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/ExpectedOutputs/OneRunWithRules.sarif @@ -10,7 +10,7 @@ }, "results": [ { - "ruleId": "C2009", + "ruleId": "C2001", "message": "Some testing occurred." }, { @@ -45,10 +45,16 @@ "defaultLevel": "error", "helpUri": "http://www.domain.com/rules/c2002.html" }, + "C2003": { + "id": "C2003", + "name": "Rule C2003 for C#", + "shortDescription": "C# rules were meant to be broken.", + "fullDescription": "Rent internal rebellion competence biography photograph." + }, "C2003-1": { "id": "C2003", - "name": "Rule C2003", - "shortDescription": "Rules were meant to be broken.", + "name": "Rule C2003 for VB", + "shortDescription": "VB rules were meant to be broken.", "fullDescription": "Rent internal rebellion competence biography photograph." } } diff --git a/src/Sarif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/ExpectedOutputs/ResultLocations.sarif b/src/Sarif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/ExpectedOutputs/ResultLocations.sarif index b4e47a388..e8ebd560a 100644 --- a/src/Sarif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/ExpectedOutputs/ResultLocations.sarif +++ b/src/Sarif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/ExpectedOutputs/ResultLocations.sarif @@ -8,18 +8,18 @@ "language": "en-US" }, "logicalLocations": { - "collections::list::add": { - "name": "add", - "parentKey": "collections::list", - "kind": "function" + "collections": { + "kind": "namespace" }, "collections::list": { "name": "list", "parentKey": "collections", "kind": "type" }, - "collections": { - "kind": "namespace" + "collections::list::add": { + "name": "add", + "parentKey": "collections::list", + "kind": "function" } }, "results": [ @@ -101,14 +101,14 @@ } ], "rules": { - "WEB1079.AttributeValueIsNotQuoted": { + "WEB1079": { "id": "WEB1079", "shortDescription": "The attribute value is not quoted.", "messageFormats": { "default": "The value of the '{0}' attribute is not quoted. Wrap the attribute value in single or double quotes." } }, - "WEB1066.TagNameIsNotLowercase": { + "WEB1066": { "id": "WEB1066", "shortDescription": "The tag name is not lowercase.", "messageFormats": { diff --git a/src/Sarif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/ExpectedOutputs/TwoResultsWithFixes.sarif b/src/Sarif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/ExpectedOutputs/TwoResultsWithFixes.sarif index 93c09cf6e..284d80ba1 100644 --- a/src/Sarif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/ExpectedOutputs/TwoResultsWithFixes.sarif +++ b/src/Sarif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/ExpectedOutputs/TwoResultsWithFixes.sarif @@ -156,14 +156,14 @@ } ], "rules": { - "WEB1079.AttributeValueIsNotQuoted": { + "WEB1079": { "id": "WEB1079", "shortDescription": "The attribute value is not quoted.", "messageFormats": { "default": "The value of the '{0}' attribute is not quoted. Wrap the attribute value in single or double quotes." } }, - "WEB1066.TagNameIsNotLowercase": { + "WEB1066": { "id": "WEB1066", "shortDescription": "The tag name is not lowercase.", "messageFormats": { diff --git a/src/Sarif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/Inputs/OneRunWithRules.sarif b/src/Sarif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/Inputs/OneRunWithRules.sarif index 4aa7c0b12..0ad49b061 100644 --- a/src/Sarif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/Inputs/OneRunWithRules.sarif +++ b/src/Sarif.UnitTests/TestData/SarifCurrentToVersionOneVisitor/Inputs/OneRunWithRules.sarif @@ -9,25 +9,29 @@ }, "results": [ { - "ruleId": "C2009", + "ruleId": "C2001", + "ruleIndex": 0, "message": { "text": "Some testing occurred." } }, { "ruleId": "C2002", + "ruleIndex": 1, "message": { "text": "Some testing occurred." } }, { "ruleId": "C2003", + "ruleIndex": 2, "message": { "text": "Some testing occurred." } }, { - "ruleId": "C2003-1", + "ruleId": "C2003", + "ruleIndex": 3, "message": { "text": "Some testing occurred." } } ], "resources": { - "rules": { - "C2001": { + "rules": [ + { "id": "C2001", "shortDescription": { "text": "A variable was used without being initialized." @@ -39,7 +43,7 @@ "some_key": "FoxForceFive" } }, - "C2002": { + { "id": "C2002", "fullDescription": { "text": "Catfish season continuous hen lamb include dose copy grant." @@ -50,19 +54,31 @@ }, "helpUri": "http://www.domain.com/rules/c2002.html" }, - "C2003-1": { + { "id": "C2003", "name": { - "text": "Rule C2003" + "text": "Rule C2003 for C#" }, "shortDescription": { - "text": "Rules were meant to be broken." + "text": "C# rules were meant to be broken." + }, + "fullDescription": { + "text": "Rent internal rebellion competence biography photograph." + } + }, + { + "id": "C2003", + "name": { + "text": "Rule C2003 for VB" + }, + "shortDescription": { + "text": "VB rules were meant to be broken." }, "fullDescription": { "text": "Rent internal rebellion competence biography photograph." } } - } + ] } } ] diff --git a/src/Sarif.UnitTests/Visitors/SarifCurrentToVersionOneVisitorTests.cs b/src/Sarif.UnitTests/Visitors/SarifCurrentToVersionOneVisitorTests.cs index 6efa9700a..0da9b7118 100644 --- a/src/Sarif.UnitTests/Visitors/SarifCurrentToVersionOneVisitorTests.cs +++ b/src/Sarif.UnitTests/Visitors/SarifCurrentToVersionOneVisitorTests.cs @@ -44,23 +44,21 @@ protected override string ConstructTestOutputFromInputResource(string inputResou [Fact] public void SarifTransformerTests_ToVersionOne_OneRunWithFiles() => RunTest("OneRunWithFiles.sarif"); -#if TRANSFORM_CODE_AUTHORED [Fact] public void SarifTransformerTests_ToVersionOne_OneRunWithRules() => RunTest("OneRunWithRules.sarif"); -#endif + [Fact] public void SarifTransformerTests_ToVersionOne_OneRunWithBasicInvocation() => RunTest("OneRunWithBasicInvocation.sarif"); [Fact] public void SarifTransformerTests_ToVersionOne_NotificationExceptionWithStack() => RunTest("NotificationExceptionWithStack.sarif"); -#if TRANSFORM_CODE_AUTHORED [Fact] public void SarifTransformerTests_ToVersionOne_ResultLocations() => RunTest("ResultLocations.sarif"); [Fact] public void SarifTransformerTests_ToVersionOne_TwoResultsWithFixes() => RunTest("TwoResultsWithFixes.sarif"); -#endif + [Fact] public void SarifTransformerTests_ToVersionOne_Regions() => RunTest("Regions.sarif"); } diff --git a/src/Sarif/Schemata/sarif-schema.json b/src/Sarif/Schemata/sarif-schema.json index 4d53761c4..f14d2f879 100644 --- a/src/Sarif/Schemata/sarif-schema.json +++ b/src/Sarif/Schemata/sarif-schema.json @@ -1,6 +1,6 @@ { "$schema": "http://json-schema.org/draft-04/schema#", - "title": "Static Analysis Results Format (SARIF) Version 2.0.0-csd.2.beta.2018-10-10 JSON Schema", + "title": "Static Analysis Results Format (SARIF) Version 2.0.0-csd.2.beta.2018-11-28 JSON Schema", "description": "Static Analysis Results Format (SARIF) Version 2.0.0-csd.2.beta-2018-11-28 JSON Schema: a standard format for the output of static analysis tools.", "additionalProperties": false, "type": "object", @@ -347,7 +347,7 @@ "description": "An array of external property files containing run.results arrays to be merged with the root log file.", "type": "array", "minItems": 1, - "uniqueItems": true, + "uniqueItems": false, "items": { "$ref": "#/definitions/externalPropertyFile" } @@ -1002,10 +1002,17 @@ }, "ruleId": { - "description": "The stable, unique identifier of the rule (if any) to which this notification is relevant. This member can be used to retrieve rule metadata from the rules dictionary, if it exists.", + "description": "The stable, unique identifier of the rule, if any, to which this notification is relevant.", "type": "string" }, + "ruleIndex": { + "description": "The index within the run resources array of the rule object, if any, associated with this notification.", + "type": "integer", + "default": -1, + "minimum": -1 + }, + "physicalLocation": { "description": "The file and region relevant to this notification.", "$ref": "#/definitions/physicalLocation" @@ -1274,7 +1281,7 @@ "properties": { "ruleId": { - "description": "The stable, unique identifier of the rule (if any) to which this notification is relevant. This member can be used to retrieve rule metadata from the rules dictionary, if it exists.", + "description": "The stable, unique identifier of the rule, if any, to which this notification is relevant. This member can be used to retrieve rule metadata from the rules dictionary, if it exists.", "type": "string" }, diff --git a/src/Sarif/Visitors/SarifCurrentToVersionOneVisitor.cs b/src/Sarif/Visitors/SarifCurrentToVersionOneVisitor.cs index 394734311..9cb58be22 100644 --- a/src/Sarif/Visitors/SarifCurrentToVersionOneVisitor.cs +++ b/src/Sarif/Visitors/SarifCurrentToVersionOneVisitor.cs @@ -23,8 +23,11 @@ public class SarifCurrentToVersionOneVisitor : SarifRewritingVisitor private RunVersionOne _currentRun = null; // To understand the purpose of these fields, see the comment on CreateFileKeyIndexMappings. - private IDictionary _v1FileKeyToV2IndexDictionary; - private IDictionary _v2FileIndexToV1KeyDictionary; + private IDictionary _v1FileKeyToV2IndexMap; + private IDictionary _v2FileIndexToV1KeyMap; + + // To understand the purpose of this field, see the comment on CreateRuleIndexToKeyMapping. + private IDictionary _v2RuleIndexToV1KeyMap; public bool EmbedVersionTwoContentInPropertyBag { get; set; } @@ -155,8 +158,8 @@ private string GetFileEncodingName(Uri uri) if (uri != null && files != null - && _v1FileKeyToV2IndexDictionary != null - && _v1FileKeyToV2IndexDictionary.TryGetValue(uri.OriginalString, out int index)) + && _v1FileKeyToV2IndexMap != null + && _v1FileKeyToV2IndexMap.TryGetValue(uri.OriginalString, out int index)) { encodingName = files[index].Encoding; } @@ -186,7 +189,7 @@ private FileDataVersionOne CreateFileDataVersionOne(FileData v2FileData) int parentIndex = v2FileData.ParentIndex; string parentKey = parentIndex == -1 ? null - : _v2FileIndexToV1KeyDictionary?[parentIndex]; + : _v2FileIndexToV1KeyMap?[parentIndex]; fileData = new FileDataVersionOne { @@ -606,9 +609,9 @@ private Stream GetContentStream(Uri uri, out Encoding encoding) string failureReason = null; IList files = _currentV2Run.Files; - if (uri != null && files != null && _v1FileKeyToV2IndexDictionary != null) + if (uri != null && files != null && _v1FileKeyToV2IndexMap != null) { - if (_v1FileKeyToV2IndexDictionary.TryGetValue(uri.OriginalString, out int index)) + if (_v1FileKeyToV2IndexMap.TryGetValue(uri.OriginalString, out int index)) { FileData fileData = files[index]; @@ -735,7 +738,7 @@ internal Dictionary CreateResponseFilesDictionary(IList rules = _currentV2Run.Resources.Rules; - - if (v2Result.RuleId != null && - rules.TryGetValue(v2Result.RuleId, out Rule v2Rule) && - v2Rule.Id != v2Result.RuleId) - { - result.RuleId = v2Rule.Id; - result.RuleKey = v2Result.RuleId; - } - else - { - result.RuleId = v2Result.RuleId; - } -#endif - } - else + result.RuleId = v2Result.RuleId; + string ruleKey = GetV1RuleKeyFromV2Index(v2Result.RuleIndex, _v2RuleIndexToV1KeyMap); + + // If the rules dictionary key is the same as the rule id, don't set result.RuleKey; + // leave it null. This way, we don't unnecessarily persist ruleKey in the v1 SARIF file. + // That is, we persist + // + // "ruleId": "TST0001" + // + // instead of + // + // "ruleId": "TST0001", + // "ruleKey": "TST0001" + // + if (ruleKey != result.RuleId) { - result.RuleId = v2Result.RuleId; + result.RuleKey = ruleKey; } if (!string.IsNullOrWhiteSpace(v2Result.Message?.MessageId)) @@ -822,7 +821,7 @@ internal ResultVersionOne CreateResultVersionOne(Result v2Result) return result; } - internal RuleVersionOne CreateRuleVersionOne(IRule v2IRule) + internal static RuleVersionOne CreateRuleVersionOne(IRule v2IRule) { RuleVersionOne rule = null; @@ -872,7 +871,8 @@ internal RunVersionOne CreateRunVersionOne(Run v2Run) run = new RunVersionOne(); _currentRun = run; - CreateFileKeyIndexMappings(v2Run.Files, out _v1FileKeyToV2IndexDictionary, out _v2FileIndexToV1KeyDictionary); + CreateFileKeyIndexMappings(v2Run.Files, out _v1FileKeyToV2IndexMap, out _v2FileIndexToV1KeyMap); + _v2RuleIndexToV1KeyMap = CreateV2RuleIndexToV1KeyMapping(v2Run.Resources?.Rules); run.BaselineId = v2Run.BaselineInstanceGuid; run.Files = CreateFileDataVersionOneDictionary(); @@ -886,9 +886,7 @@ internal RunVersionOne CreateRunVersionOne(Run v2Run) run.Properties = v2Run.Properties; run.Results = new List(); -#if TRANSFORM_CODE_AUTHORED - run.Rules = v2Run.Resources?.Rules?.ToDictionary(v => v.Key, v => CreateRuleVersionOne(v.Value)); -#endif + run.Rules = ConvertRulesArrayToDictionary(_currentV2Run.Resources?.Rules, _v2RuleIndexToV1KeyMap); run.Tool = CreateToolVersionOne(v2Run.Tool); foreach (Result v2Result in v2Run.Results) @@ -992,10 +990,10 @@ private IDictionary CreateFileDataVersionOneDictiona { Dictionary filesVersionOne = null; - if (_v1FileKeyToV2IndexDictionary != null) + if (_v1FileKeyToV2IndexMap != null) { filesVersionOne = new Dictionary(); - foreach (KeyValuePair entry in _v1FileKeyToV2IndexDictionary) + foreach (KeyValuePair entry in _v1FileKeyToV2IndexMap) { string key = entry.Key; int index = entry.Value; @@ -1032,6 +1030,73 @@ private IDictionary CreateLogicalLocationVers return logicalLocationsVersionOne; } + // In SARIF v1, run.resources.rules was a dictionary, whereas in v2 it is an array. + // Normally, the lookup key into the v1 rules dictionary is the rule id. But some + // tools allow multiple rules to have the same id. In that case we must synthesize + // a unique key for each rule with that id. We choose "-", where is + // 1 for the second occurrence, 2 for the third, and so on. + private static IDictionary CreateV2RuleIndexToV1KeyMapping(IList rules) + { + var v2RuleIndexToV1KeyMap = new Dictionary(); + + if (rules != null) + { + // Keep track of how many distinct rules have each id. + var ruleIdToCountMap = new Dictionary(); + + for (int i = 0; i < rules.Count; ++i) + { + string ruleId = rules[i].Id; + if (ruleId != null) + { + ruleIdToCountMap[ruleId] = ruleIdToCountMap.ContainsKey(ruleId) + ? ruleIdToCountMap[ruleId] + 1 + : 1; + + v2RuleIndexToV1KeyMap[i] = ruleIdToCountMap[ruleId] == 1 + ? ruleId + : ruleId + '-' + (ruleIdToCountMap[ruleId] - 1).ToString(); + } + } + } + + return v2RuleIndexToV1KeyMap; + } + + private static string GetV1RuleKeyFromV2Index( + int ruleIndex, + IDictionary v2RuleIndexToV1KeyMap) + { + v2RuleIndexToV1KeyMap.TryGetValue(ruleIndex, out string ruleKey); + + // If TryGetValue returned false, ruleKey was set to default(string), + // otherwise known as null, which is what we want. + return ruleKey; + } + + private static IDictionary ConvertRulesArrayToDictionary( + IList v2Rules, + IDictionary v2RuleIndexToV1KeyMap) + { + IDictionary v1Rules = null; + + if (v2Rules != null) + { + v1Rules = new Dictionary(); + for (int i = 0; i < v2Rules.Count; ++i) + { + Rule v2Rule = v2Rules[i]; + + RuleVersionOne v1Rule = CreateRuleVersionOne(v2Rule); + string key = GetV1RuleKeyFromV2Index(i, v2RuleIndexToV1KeyMap); + + v1Rules[key] = v1Rule; + } + } + + return v1Rules; + } + internal StackVersionOne CreateStackVersionOne(Stack v2Stack) { StackVersionOne stack = null; diff --git a/src/Sarif/Writers/PrereleaseCompatibilityTransformer.cs b/src/Sarif/Writers/PrereleaseCompatibilityTransformer.cs index 7f515c22b..bb405976d 100644 --- a/src/Sarif/Writers/PrereleaseCompatibilityTransformer.cs +++ b/src/Sarif/Writers/PrereleaseCompatibilityTransformer.cs @@ -184,10 +184,6 @@ private static bool ApplyChangesFromTC25ThroughTC28( if (run["resources"] is JObject resources) { - // Remove 'open' from rule configuration default level enumeration - // https://github.com/oasis-tcs/sarif-spec/issues/288 - modifiedLog |= RemapRuleDefaultLevelFromOpenToNote(resources); - ruleKeyToIndexMap = new Dictionary(); if (resources["rules"] is JObject rules) @@ -199,6 +195,10 @@ private static bool ApplyChangesFromTC25ThroughTC28( } modifiedLog |= ruleKeyToIndexMap.Count > 0; + + // Remove 'open' from rule configuration default level enumeration + // https://github.com/oasis-tcs/sarif-spec/issues/288 + modifiedLog |= RemapRuleDefaultLevelFromOpenToNote(resources); } if (run["results"] is JArray results) @@ -525,12 +525,12 @@ private static bool RemapRuleDefaultLevelFromOpenToNote(JObject resources) if (resources == null) { return modifiedResources; } - JObject rules = (JObject)resources["rules"]; + var rules = (JArray)resources["rules"]; if (rules == null ) { return modifiedResources; } - foreach (JProperty rule in rules.Values()) + foreach (JObject rule in rules) { - JObject configuration = (JObject)rule.Value["configuration"]; + var configuration = (JObject)rule["configuration"]; if (configuration == null) { continue; } if ("open".Equals((string)configuration["defaultLevel"]))