From c6fa3e8abe12fa8268dcff50eb93f7d47efd3981 Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Mon, 29 Jun 2020 21:06:04 -0300 Subject: [PATCH 1/7] Adding rule 1012 --- .../Rules/RuleResources.Designer.cs | 6 +- src/Sarif.Multitool/Rules/RuleResources.resx | 12 +- ...essageArgumentsMustBeConsistentWithRule.cs | 92 +++++++++++ .../Multitool/ValidateCommandTests.cs | 8 + ...entsMustBeConsistentWithRule_Invalid.sarif | 152 ++++++++++++++++++ ...umentsMustBeConsistentWithRule_Valid.sarif | 103 ++++++++++++ ...entsMustBeConsistentWithRule_Invalid.sarif | 97 +++++++++++ ...umentsMustBeConsistentWithRule_Valid.sarif | 101 ++++++++++++ 8 files changed, 562 insertions(+), 9 deletions(-) create mode 100644 src/Sarif.Multitool/Rules/SARIF1012.MessageArgumentsMustBeConsistentWithRule.cs create mode 100644 src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif create mode 100644 src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif create mode 100644 src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif create mode 100644 src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif diff --git a/src/Sarif.Multitool/Rules/RuleResources.Designer.cs b/src/Sarif.Multitool/Rules/RuleResources.Designer.cs index bc3c4dff8..467ab860f 100644 --- a/src/Sarif.Multitool/Rules/RuleResources.Designer.cs +++ b/src/Sarif.Multitool/Rules/RuleResources.Designer.cs @@ -351,7 +351,7 @@ internal static string SARIF1011_ReferenceFinalSchema_FullDescription_Text { } /// - /// Looks up a localized string similar to Placeholder_SARIF1012_MessageArgumentsMustBeConsistentWithRule_Error_MessageIdMustExist_Text. + /// Looks up a localized string similar to {0}: Placeholder '{1}' '{2}'. /// internal static string SARIF1012_MessageArgumentsMustBeConsistentWithRule_Error_MessageIdMustExist_Text { get { @@ -360,7 +360,7 @@ internal static string SARIF1012_MessageArgumentsMustBeConsistentWithRule_Error_ } /// - /// Looks up a localized string similar to Placeholder_SARIF1012_MessageArgumentsMustBeConsistentWithRule_Error_SupplyEnoughMessageArguments_Text. + /// Looks up a localized string similar to {0}: Placeholder '{1}' '{2}' '{3}' '{4}' '{5}'. /// internal static string SARIF1012_MessageArgumentsMustBeConsistentWithRule_Error_SupplyEnoughMessageArguments_Text { get { @@ -370,7 +370,7 @@ internal static string SARIF1012_MessageArgumentsMustBeConsistentWithRule_Error_ } /// - /// Looks up a localized string similar to Placeholder_SARIF1012_MessageArgumentsMustBeConsistentWithRule_FullDescription_Text. + /// Looks up a localized string similar to Placeholder. /// internal static string SARIF1012_MessageArgumentsMustBeConsistentWithRule_FullDescription_Text { get { diff --git a/src/Sarif.Multitool/Rules/RuleResources.resx b/src/Sarif.Multitool/Rules/RuleResources.resx index a96c1c8ef..d5fe95f7a 100644 --- a/src/Sarif.Multitool/Rules/RuleResources.resx +++ b/src/Sarif.Multitool/Rules/RuleResources.resx @@ -331,14 +331,14 @@ Many tool use similar names for 'uriBaseId' symbols. We suggest 'REPOROOT' for t Placeholder_SARIF2011_ProvideContextRegion_Note_Default_Text - - Placeholder_SARIF2012_ProvideHelpUris_FullDescription_Text + + {0}: Placeholder '{1}' '{2}' - - Placeholder_SARIF2012_ProvideHelpUris_Note_Default_Text + + {0}: Placeholder '{1}' '{2}' '{3}' '{4}' '{5}' - - Placeholder_SARIF2013_ProvideEmbeddedFileContent_FullDescription_Text + + Placeholder Placeholder_SARIF2013_ProvideEmbeddedFileContent_Note_Default_Text diff --git a/src/Sarif.Multitool/Rules/SARIF1012.MessageArgumentsMustBeConsistentWithRule.cs b/src/Sarif.Multitool/Rules/SARIF1012.MessageArgumentsMustBeConsistentWithRule.cs new file mode 100644 index 000000000..f4e43075e --- /dev/null +++ b/src/Sarif.Multitool/Rules/SARIF1012.MessageArgumentsMustBeConsistentWithRule.cs @@ -0,0 +1,92 @@ +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System.Collections.Generic; +using System.Linq; +using System.Text.RegularExpressions; + +namespace Microsoft.CodeAnalysis.Sarif.Multitool.Rules +{ + public class MessageArgumentsMustBeConsistentWithRule : SarifValidationSkimmerBase + { + /// + /// SARIF1012 + /// + public override string Id => RuleId.MessageArgumentsMustBeConsistentWithRule; + + /// + /// Placeholder + /// + public override MultiformatMessageString FullDescription => new MultiformatMessageString { Text = RuleResources.SARIF1012_MessageArgumentsMustBeConsistentWithRule_FullDescription_Text }; + + protected override IEnumerable MessageResourceNames => new string[] { + nameof(RuleResources.SARIF1012_MessageArgumentsMustBeConsistentWithRule_Error_MessageIdMustExist_Text), + nameof(RuleResources.SARIF1012_MessageArgumentsMustBeConsistentWithRule_Error_SupplyEnoughMessageArguments_Text) + }; + + public override FailureLevel DefaultLevel => FailureLevel.Error; + + private IList currentRules; + + protected override void Analyze(Run run, string runPointer) + { + this.currentRules = run?.Tool?.Driver?.Rules; + } + + protected override void Analyze(Result result, string resultPointer) + { + // if message.id is not null, we have to check if that exists in rules + if (!string.IsNullOrEmpty(result.Message.Id)) + { + ReportingDescriptor rule = result.RuleIndex >= 0 + ? (this.currentRules?[result.RuleIndex]) + : (this.currentRules?.FirstOrDefault(r => r.Id == (result.RuleId ?? result.Rule?.Id))); + + if (this.currentRules == null + || rule == null + || !rule.MessageStrings.ContainsKey(result.Message.Id)) + { + // {0}: Placeholder {1} {2} + LogResult( + resultPointer, + nameof(RuleResources.SARIF1012_MessageArgumentsMustBeConsistentWithRule_Error_MessageIdMustExist_Text), + result.Message.Id, + (result.RuleIndex >= 0 + ? result.RuleIndex.ToString() + : (result.RuleId ?? result.Rule?.Id ?? "null"))); + return; + } + + // we have rules, we find it but, the count of parameters are invalid + string messageText = rule.MessageStrings[result.Message.Id].Text; + int countOfPlaceholders = CountOfPlaceholders(messageText); + if (countOfPlaceholders > (result.Message.Arguments?.Count ?? 0)) + { + // {0}: Placeholder {1} {2} {3} {4} {5} + LogResult( + resultPointer, + nameof(RuleResources.SARIF1012_MessageArgumentsMustBeConsistentWithRule_Error_SupplyEnoughMessageArguments_Text), + result.Message.Arguments.Count.ToString(), + result.Message.Id, + (result.RuleIndex >= 0 + ? result.RuleIndex.ToString() + : (result.RuleId ?? result.Rule?.Id ?? "null")), + countOfPlaceholders.ToString(), + messageText); + } + } + } + + private int CountOfPlaceholders(string text) + { + MatchCollection matchCollection = Regex.Matches(text, "{\\d+}"); + HashSet matches = new HashSet(); + foreach (Match match in matchCollection) + { + matches.Add(match.Value); + } + + return matches.Count; + } + } +} diff --git a/src/Test.FunctionalTests.Sarif/Multitool/ValidateCommandTests.cs b/src/Test.FunctionalTests.Sarif/Multitool/ValidateCommandTests.cs index 7be9f2a50..bb998ac5a 100644 --- a/src/Test.FunctionalTests.Sarif/Multitool/ValidateCommandTests.cs +++ b/src/Test.FunctionalTests.Sarif/Multitool/ValidateCommandTests.cs @@ -123,6 +123,14 @@ public void SARIF1011_ReferenceFinalSchema_Valid() public void SARIF1011_ReferenceFinalSchema_Invalid() => RunTest(MakeInvalidTestFileName(RuleId.ReferenceFinalSchema, nameof(RuleId.ReferenceFinalSchema))); + [Fact] + public void SARIF1012_MessageArgumentsMustBeConsistentWithRule_Valid() + => RunTest(MakeValidTestFileName(RuleId.MessageArgumentsMustBeConsistentWithRule, nameof(RuleId.MessageArgumentsMustBeConsistentWithRule))); + + [Fact] + public void SARIF1012_MessageArgumentsMustBeConsistentWithRule_Invalid() + => RunTest(MakeInvalidTestFileName(RuleId.MessageArgumentsMustBeConsistentWithRule, nameof(RuleId.MessageArgumentsMustBeConsistentWithRule))); + [Fact] public void SARIF2001_AuthorHighQualityMessages_Valid() => RunTest(MakeValidTestFileName(RuleId.AuthorHighQualityMessages, nameof(RuleId.AuthorHighQualityMessages))); diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif new file mode 100644 index 000000000..88b9ff15d --- /dev/null +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif @@ -0,0 +1,152 @@ +{ + "$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.5.json", + "version": "2.1.0", + "runs": [ + { + "tool": { + "driver": { + "name": "SARIF Functional Testing", + "rules": [ + { + "id": "SARIF1012", + "name": "MessageArgumentsMustBeConsistentWithRule", + "shortDescription": { + "text": "Placeholder." + }, + "fullDescription": { + "text": "Placeholder" + }, + "messageStrings": { + "Error_MessageIdMustExist": { + "text": "{0}: Placeholder '{1}' '{2}'" + }, + "Error_SupplyEnoughMessageArguments": { + "text": "{0}: Placeholder '{1}' '{2}' '{3}' '{4}' '{5}'" + } + }, + "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" + }, + { + "id": "SARIF2001", + "name": "AuthorHighQualityMessages", + "shortDescription": { + "text": "Follow authoring practices that make your rule messages readable, understandable, and actionable." + }, + "fullDescription": { + "text": "Follow authoring practices that make your rule messages readable, understandable, and actionable.\r\n\r\nIncluding \"dynamic content\" (information that varies among results from the same rule) makes your messages more specific. It avoids the \"wall of bugs\" phenomenon, where hundreds of occurrences of the same message appear unapproachable.\r\n\r\nPlacing dynamic content in quotes sets it off from the static text, making it easier to spot. It's especially helpful when the dynamic content is a string that might contain spaces, and most especially when the string might be empty (and so would be invisible if it weren't for the quotes). We recommend single quotes for a less cluttered appearance, even though English usage would require double quotes.\r\n\r\nFinally, write in complete sentences and end each sentence with a period. This guidance does not apply to Markdown messages, which might include formatting that makes the punctuation unnecessary." + }, + "messageStrings": { + "Warning_EnquoteDynamicContent": { + "text": "{0}: In rule '{1}', the message with id '{2}' includes dynamic content that is not enclosed in single quotes. Enquoting dynamic content makes it easier to spot, and single quotes give a less cluttered appearance." + }, + "Warning_IncludeDynamicContent": { + "text": "{0}: In rule '{1}', the message with id '{2}' does not include any dynamic content. Dynamic content makes your messages more specific and avoids the \"wall of bugs\" phenomenon." + }, + "Warning_TerminateWithPeriod": { + "text": "{0}: In rule '{1}', the message with id '{2}' does not end in a period. Write rule messages as complete sentences." + } + }, + "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" + } + ] + } + }, + "invocations": [ + { + "executionSuccessful": true + } + ], + "artifacts": [ + { + "location": { + "uri": "FunctionalTestOutput.ValidateCommand/Inputs.SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif", + "uriBaseId": "TEST_DIR" + } + } + ], + "results": [ + { + "ruleId": "SARIF1012", + "ruleIndex": 0, + "level": "error", + "message": { + "id": "Error_MessageIdMustExist", + "arguments": [ + "runs[0].results[0]", + "DoesNotExist", + "0" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 44, + "startColumn": 9 + } + } + } + ] + }, + { + "ruleId": "SARIF1012", + "ruleIndex": 0, + "level": "error", + "message": { + "id": "Error_SupplyEnoughMessageArguments", + "arguments": [ + "runs[0].results[1]", + "1", + "DoesExist", + "0", + "3", + "{0}: Placeholder {1} {2}." + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 69, + "startColumn": 9 + } + } + } + ] + }, + { + "ruleId": "SARIF2001", + "ruleIndex": 1, + "message": { + "id": "Warning_EnquoteDynamicContent", + "arguments": [ + "runs[0].tool.driver.rules[0].messageStrings.DoesExist.text", + "TEST1001", + "DoesExist" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 22, + "startColumn": 53 + } + } + } + ] + } + ], + "columnKind": "utf16CodeUnits" + } + ] +} \ No newline at end of file diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif new file mode 100644 index 000000000..0d305f0a3 --- /dev/null +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif @@ -0,0 +1,103 @@ +{ + "$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.5.json", + "version": "2.1.0", + "runs": [ + { + "tool": { + "driver": { + "name": "SARIF Functional Testing", + "rules": [ + { + "id": "SARIF2001", + "name": "AuthorHighQualityMessages", + "shortDescription": { + "text": "Follow authoring practices that make your rule messages readable, understandable, and actionable." + }, + "fullDescription": { + "text": "Follow authoring practices that make your rule messages readable, understandable, and actionable.\r\n\r\nIncluding \"dynamic content\" (information that varies among results from the same rule) makes your messages more specific. It avoids the \"wall of bugs\" phenomenon, where hundreds of occurrences of the same message appear unapproachable.\r\n\r\nPlacing dynamic content in quotes sets it off from the static text, making it easier to spot. It's especially helpful when the dynamic content is a string that might contain spaces, and most especially when the string might be empty (and so would be invisible if it weren't for the quotes). We recommend single quotes for a less cluttered appearance, even though English usage would require double quotes.\r\n\r\nFinally, write in complete sentences and end each sentence with a period. This guidance does not apply to Markdown messages, which might include formatting that makes the punctuation unnecessary." + }, + "messageStrings": { + "Warning_EnquoteDynamicContent": { + "text": "{0}: In rule '{1}', the message with id '{2}' includes dynamic content that is not enclosed in single quotes. Enquoting dynamic content makes it easier to spot, and single quotes give a less cluttered appearance." + }, + "Warning_IncludeDynamicContent": { + "text": "{0}: In rule '{1}', the message with id '{2}' does not include any dynamic content. Dynamic content makes your messages more specific and avoids the \"wall of bugs\" phenomenon." + }, + "Warning_TerminateWithPeriod": { + "text": "{0}: In rule '{1}', the message with id '{2}' does not end in a period. Write rule messages as complete sentences." + } + }, + "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" + } + ] + } + }, + "invocations": [ + { + "executionSuccessful": true + } + ], + "artifacts": [ + { + "location": { + "uri": "FunctionalTestOutput.ValidateCommand/Inputs.SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif", + "uriBaseId": "TEST_DIR" + } + } + ], + "results": [ + { + "ruleId": "SARIF2001", + "ruleIndex": 0, + "message": { + "id": "Warning_EnquoteDynamicContent", + "arguments": [ + "runs[0].tool.driver.rules[0].messageStrings.Note_UseConventionalRuleIds.text", + "SARIF2009", + "Note_UseConventionalRuleIds" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 22, + "startColumn": 301 + } + } + } + ] + }, + { + "ruleId": "SARIF2001", + "ruleIndex": 0, + "message": { + "id": "Warning_EnquoteDynamicContent", + "arguments": [ + "runs[0].tool.driver.rules[0].messageStrings.Note_UseConventionalUriBaseIdNames.text", + "SARIF2009", + "Note_UseConventionalUriBaseIdNames" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 25, + "startColumn": 476 + } + } + } + ] + } + ], + "columnKind": "utf16CodeUnits" + } + ] +} \ No newline at end of file diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif new file mode 100644 index 000000000..3087c87f7 --- /dev/null +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif @@ -0,0 +1,97 @@ +{ + "$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.5.json", + "version": "2.1.0", + "runs": [ + { + "tool": { + "driver": { + "name": "SARIF Functional Testing", + "version": "1.2.3", + "rules": [ + { + "id": "TEST1001", + "name": "Test 1001", + "shortDescription": { + "text": "Test 1001 short description." + }, + "fullDescription": { + "text": "Test 1001 full description." + }, + "messageStrings": { + "DoesExist": { + "text": "{0}: Placeholder {1} {2}." + } + }, + "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" + } + ] + } + }, + "invocations": [ + { + "executionSuccessful": true + } + ], + "artifacts": [ + { + "location": { + "uri": "FunctionalTestOutput.ValidateCommand/Inputs.SARIF2009.ConsiderConventionalIdentifierValues_Invalid.sarif", + "uriBaseId": "TEST_DIR" + } + } + ], + "results": [ + { + "ruleId": "TEST1001", + "ruleIndex": 0, + "level": "note", + "message": { + "id": "DoesNotExist", + "arguments": [ + "runs[0].originalUriBaseIds.SRCINVALID", + "SRCINVALID" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 21, + "startColumn": 23 + } + } + } + ] + }, + { + "ruleId": "TEST1001", + "ruleIndex": 0, + "level": "note", + "message": { + "id": "DoesExist", + "arguments": [ + "runs[0].tool.driver.rules[0].id" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 13, + "startColumn": 32 + } + } + } + ] + } + ], + "columnKind": "utf16CodeUnits" + } + ] +} \ No newline at end of file diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif new file mode 100644 index 000000000..44064db74 --- /dev/null +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif @@ -0,0 +1,101 @@ +{ + "$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.5.json", + "version": "2.1.0", + "runs": [ + { + "tool": { + "driver": { + "name": "SARIF Functional Testing", + "version": "1.2.3", + "rules": [ + { + "id": "SARIF2009", + "name": "ConsiderConventionalIdentifierValues", + "shortDescription": { + "text": "Adopt uniform naming conventions for the symbolic names that SARIF uses various contexts." + }, + "fullDescription": { + "text": "Adopt uniform naming conventions for the symbolic names that SARIF uses various contexts.\r\n\r\nMany tools follow a conventional format for the 'reportingDescriptor.id' property: a short string identifying the tool concatenated with a numeric rule number,\r\nfor example, 'CS2001' for a diagnostic from the Roslyn C# compiler. For uniformity of experience across tools, we recommend this format.\r\n\r\nMany tool use similar names for 'uriBaseId' symbols. We suggest 'REPOROOT' for the root of a repository, 'SRCROOT' for the root of the directory containing all source code, 'TESTROOT' for the root of the directory containing all test code (if your repository is organized in that way), and 'BINROOT' for the root of the directory containing build output (if your project places all build output in a common directory)." + }, + "messageStrings": { + "Note_UseConventionalRuleIds": { + "text": "{0}: The 'id' property of the rule '{1}' does not follow the recommended format: a short string identifying the tool concatenated with a numeric rule number, for example, `CS2001`. Using a conventional format for the rule id provides a more uniform experience across tools." + }, + "Note_UseConventionalUriBaseIdNames": { + "text": "{0}: The 'originalUriBaseIds' symbol '{1}' is not one of the conventional symbols. We suggest 'REPOROOT' for the root of a repository, 'SRCROOT' for the root of the directory containing all source code, 'TESTROOT' for the root of the directory containing all test code (if your repository is organized in that way), and 'BINROOT' for the root of the directory containing build output (if your project places all build output in a common directory)." + } + }, + "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" + } + ] + } + }, + "invocations": [ + { + "executionSuccessful": true + } + ], + "artifacts": [ + { + "location": { + "uri": "FunctionalTestOutput.ValidateCommand/Inputs.SARIF2009.ConsiderConventionalIdentifierValues_Invalid.sarif", + "uriBaseId": "TEST_DIR" + } + } + ], + "results": [ + { + "ruleId": "SARIF2009", + "ruleIndex": 0, + "level": "note", + "message": { + "id": "Note_UseConventionalUriBaseIdNames", + "arguments": [ + "runs[0].originalUriBaseIds.SRCINVALID", + "SRCINVALID" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 21, + "startColumn": 23 + } + } + } + ] + }, + { + "ruleId": "SARIF2009", + "ruleIndex": 0, + "level": "note", + "message": { + "id": "Note_UseConventionalRuleIds", + "arguments": [ + "runs[0].tool.driver.rules[0].id", + "WRONG00001" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 13, + "startColumn": 32 + } + } + } + ] + } + ], + "columnKind": "utf16CodeUnits" + } + ] +} \ No newline at end of file From 2b8738b86c9d86abea89b7f14728de007531c89f Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Mon, 29 Jun 2020 21:35:55 -0300 Subject: [PATCH 2/7] code review - 1 --- ...MessageArgumentsMustBeConsistentWithRule.cs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/Sarif.Multitool/Rules/SARIF1012.MessageArgumentsMustBeConsistentWithRule.cs b/src/Sarif.Multitool/Rules/SARIF1012.MessageArgumentsMustBeConsistentWithRule.cs index f4e43075e..bdd9c7077 100644 --- a/src/Sarif.Multitool/Rules/SARIF1012.MessageArgumentsMustBeConsistentWithRule.cs +++ b/src/Sarif.Multitool/Rules/SARIF1012.MessageArgumentsMustBeConsistentWithRule.cs @@ -27,10 +27,12 @@ public class MessageArgumentsMustBeConsistentWithRule : SarifValidationSkimmerBa public override FailureLevel DefaultLevel => FailureLevel.Error; private IList currentRules; + private Run run; protected override void Analyze(Run run, string runPointer) { - this.currentRules = run?.Tool?.Driver?.Rules; + this.run = run; + this.currentRules = run.Tool.Driver?.Rules; } protected override void Analyze(Result result, string resultPointer) @@ -38,12 +40,10 @@ protected override void Analyze(Result result, string resultPointer) // if message.id is not null, we have to check if that exists in rules if (!string.IsNullOrEmpty(result.Message.Id)) { - ReportingDescriptor rule = result.RuleIndex >= 0 - ? (this.currentRules?[result.RuleIndex]) - : (this.currentRules?.FirstOrDefault(r => r.Id == (result.RuleId ?? result.Rule?.Id))); + ReportingDescriptor rule = result.GetRule(this.run); if (this.currentRules == null - || rule == null + || rule.MessageStrings == null || !rule.MessageStrings.ContainsKey(result.Message.Id)) { // {0}: Placeholder {1} {2} @@ -51,9 +51,7 @@ protected override void Analyze(Result result, string resultPointer) resultPointer, nameof(RuleResources.SARIF1012_MessageArgumentsMustBeConsistentWithRule_Error_MessageIdMustExist_Text), result.Message.Id, - (result.RuleIndex >= 0 - ? result.RuleIndex.ToString() - : (result.RuleId ?? result.Rule?.Id ?? "null"))); + result.ResolvedRuleId(run) ?? "null"); return; } @@ -68,9 +66,7 @@ protected override void Analyze(Result result, string resultPointer) nameof(RuleResources.SARIF1012_MessageArgumentsMustBeConsistentWithRule_Error_SupplyEnoughMessageArguments_Text), result.Message.Arguments.Count.ToString(), result.Message.Id, - (result.RuleIndex >= 0 - ? result.RuleIndex.ToString() - : (result.RuleId ?? result.Rule?.Id ?? "null")), + result.ResolvedRuleId(run) ?? "null", countOfPlaceholders.ToString(), messageText); } From dbafe94aaacae4106a0af5d3aca65459a8d60303 Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Mon, 29 Jun 2020 21:51:45 -0300 Subject: [PATCH 3/7] code review - 2 --- ...essageArgumentsMustBeConsistentWithRule.cs | 23 +++++++++++-------- ...entsMustBeConsistentWithRule_Invalid.sarif | 6 ++--- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/src/Sarif.Multitool/Rules/SARIF1012.MessageArgumentsMustBeConsistentWithRule.cs b/src/Sarif.Multitool/Rules/SARIF1012.MessageArgumentsMustBeConsistentWithRule.cs index bdd9c7077..2bb4577cd 100644 --- a/src/Sarif.Multitool/Rules/SARIF1012.MessageArgumentsMustBeConsistentWithRule.cs +++ b/src/Sarif.Multitool/Rules/SARIF1012.MessageArgumentsMustBeConsistentWithRule.cs @@ -43,8 +43,7 @@ protected override void Analyze(Result result, string resultPointer) ReportingDescriptor rule = result.GetRule(this.run); if (this.currentRules == null - || rule.MessageStrings == null - || !rule.MessageStrings.ContainsKey(result.Message.Id)) + || rule.MessageStrings?.ContainsKey(result.Message.Id) == false) { // {0}: Placeholder {1} {2} LogResult( @@ -57,8 +56,8 @@ protected override void Analyze(Result result, string resultPointer) // we have rules, we find it but, the count of parameters are invalid string messageText = rule.MessageStrings[result.Message.Id].Text; - int countOfPlaceholders = CountOfPlaceholders(messageText); - if (countOfPlaceholders > (result.Message.Arguments?.Count ?? 0)) + int placeholderMaxPosition = PlaceholderMaxPosition(messageText); + if (placeholderMaxPosition > (result.Message.Arguments?.Count ?? 0)) { // {0}: Placeholder {1} {2} {3} {4} {5} LogResult( @@ -67,22 +66,28 @@ protected override void Analyze(Result result, string resultPointer) result.Message.Arguments.Count.ToString(), result.Message.Id, result.ResolvedRuleId(run) ?? "null", - countOfPlaceholders.ToString(), + placeholderMaxPosition.ToString(), messageText); } } } - private int CountOfPlaceholders(string text) + private int PlaceholderMaxPosition(string text) { MatchCollection matchCollection = Regex.Matches(text, "{\\d+}"); - HashSet matches = new HashSet(); + int max = -1; foreach (Match match in matchCollection) { - matches.Add(match.Value); + if (int.TryParse(match.Value.Replace("{", string.Empty).Replace("}", string.Empty), out int temp)) + { + if (max < temp) + { + max = temp; + } + } } - return matches.Count; + return max++; } } } diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif index 88b9ff15d..01bc8c6b6 100644 --- a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif @@ -74,7 +74,7 @@ "arguments": [ "runs[0].results[0]", "DoesNotExist", - "0" + "TEST1001" ] }, "locations": [ @@ -101,8 +101,8 @@ "runs[0].results[1]", "1", "DoesExist", - "0", - "3", + "TEST1001", + "2", "{0}: Placeholder {1} {2}." ] }, From 3c5be66d8a2033b37f645e4ab1912ace385452d3 Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Mon, 29 Jun 2020 21:58:57 -0300 Subject: [PATCH 4/7] code review - 3 --- ...entsMustBeConsistentWithRule_Invalid.sarif | 49 +----------- ...umentsMustBeConsistentWithRule_Valid.sarif | 79 +------------------ ...entsMustBeConsistentWithRule_Invalid.sarif | 2 +- ...umentsMustBeConsistentWithRule_Valid.sarif | 44 ++--------- 4 files changed, 12 insertions(+), 162 deletions(-) diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif index 01bc8c6b6..b3f841916 100644 --- a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif @@ -25,28 +25,6 @@ } }, "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" - }, - { - "id": "SARIF2001", - "name": "AuthorHighQualityMessages", - "shortDescription": { - "text": "Follow authoring practices that make your rule messages readable, understandable, and actionable." - }, - "fullDescription": { - "text": "Follow authoring practices that make your rule messages readable, understandable, and actionable.\r\n\r\nIncluding \"dynamic content\" (information that varies among results from the same rule) makes your messages more specific. It avoids the \"wall of bugs\" phenomenon, where hundreds of occurrences of the same message appear unapproachable.\r\n\r\nPlacing dynamic content in quotes sets it off from the static text, making it easier to spot. It's especially helpful when the dynamic content is a string that might contain spaces, and most especially when the string might be empty (and so would be invisible if it weren't for the quotes). We recommend single quotes for a less cluttered appearance, even though English usage would require double quotes.\r\n\r\nFinally, write in complete sentences and end each sentence with a period. This guidance does not apply to Markdown messages, which might include formatting that makes the punctuation unnecessary." - }, - "messageStrings": { - "Warning_EnquoteDynamicContent": { - "text": "{0}: In rule '{1}', the message with id '{2}' includes dynamic content that is not enclosed in single quotes. Enquoting dynamic content makes it easier to spot, and single quotes give a less cluttered appearance." - }, - "Warning_IncludeDynamicContent": { - "text": "{0}: In rule '{1}', the message with id '{2}' does not include any dynamic content. Dynamic content makes your messages more specific and avoids the \"wall of bugs\" phenomenon." - }, - "Warning_TerminateWithPeriod": { - "text": "{0}: In rule '{1}', the message with id '{2}' does not end in a period. Write rule messages as complete sentences." - } - }, - "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" } ] } @@ -103,7 +81,7 @@ "DoesExist", "TEST1001", "2", - "{0}: Placeholder {1} {2}." + "'{0}': Placeholder '{1}' '{2}'." ] }, "locations": [ @@ -119,31 +97,6 @@ } } ] - }, - { - "ruleId": "SARIF2001", - "ruleIndex": 1, - "message": { - "id": "Warning_EnquoteDynamicContent", - "arguments": [ - "runs[0].tool.driver.rules[0].messageStrings.DoesExist.text", - "TEST1001", - "DoesExist" - ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "index": 0 - }, - "region": { - "startLine": 22, - "startColumn": 53 - } - } - } - ] } ], "columnKind": "utf16CodeUnits" diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif index 0d305f0a3..1f9513761 100644 --- a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif @@ -5,31 +5,7 @@ { "tool": { "driver": { - "name": "SARIF Functional Testing", - "rules": [ - { - "id": "SARIF2001", - "name": "AuthorHighQualityMessages", - "shortDescription": { - "text": "Follow authoring practices that make your rule messages readable, understandable, and actionable." - }, - "fullDescription": { - "text": "Follow authoring practices that make your rule messages readable, understandable, and actionable.\r\n\r\nIncluding \"dynamic content\" (information that varies among results from the same rule) makes your messages more specific. It avoids the \"wall of bugs\" phenomenon, where hundreds of occurrences of the same message appear unapproachable.\r\n\r\nPlacing dynamic content in quotes sets it off from the static text, making it easier to spot. It's especially helpful when the dynamic content is a string that might contain spaces, and most especially when the string might be empty (and so would be invisible if it weren't for the quotes). We recommend single quotes for a less cluttered appearance, even though English usage would require double quotes.\r\n\r\nFinally, write in complete sentences and end each sentence with a period. This guidance does not apply to Markdown messages, which might include formatting that makes the punctuation unnecessary." - }, - "messageStrings": { - "Warning_EnquoteDynamicContent": { - "text": "{0}: In rule '{1}', the message with id '{2}' includes dynamic content that is not enclosed in single quotes. Enquoting dynamic content makes it easier to spot, and single quotes give a less cluttered appearance." - }, - "Warning_IncludeDynamicContent": { - "text": "{0}: In rule '{1}', the message with id '{2}' does not include any dynamic content. Dynamic content makes your messages more specific and avoids the \"wall of bugs\" phenomenon." - }, - "Warning_TerminateWithPeriod": { - "text": "{0}: In rule '{1}', the message with id '{2}' does not end in a period. Write rule messages as complete sentences." - } - }, - "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" - } - ] + "name": "SARIF Functional Testing" } }, "invocations": [ @@ -45,58 +21,7 @@ } } ], - "results": [ - { - "ruleId": "SARIF2001", - "ruleIndex": 0, - "message": { - "id": "Warning_EnquoteDynamicContent", - "arguments": [ - "runs[0].tool.driver.rules[0].messageStrings.Note_UseConventionalRuleIds.text", - "SARIF2009", - "Note_UseConventionalRuleIds" - ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "index": 0 - }, - "region": { - "startLine": 22, - "startColumn": 301 - } - } - } - ] - }, - { - "ruleId": "SARIF2001", - "ruleIndex": 0, - "message": { - "id": "Warning_EnquoteDynamicContent", - "arguments": [ - "runs[0].tool.driver.rules[0].messageStrings.Note_UseConventionalUriBaseIdNames.text", - "SARIF2009", - "Note_UseConventionalUriBaseIdNames" - ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "index": 0 - }, - "region": { - "startLine": 25, - "startColumn": 476 - } - } - } - ] - } - ], + "results": [], "columnKind": "utf16CodeUnits" } ] diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif index 3087c87f7..4a9f7ed92 100644 --- a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif @@ -19,7 +19,7 @@ }, "messageStrings": { "DoesExist": { - "text": "{0}: Placeholder {1} {2}." + "text": "'{0}': Placeholder '{1}' '{2}'." } }, "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif index 44064db74..9387a998b 100644 --- a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif @@ -9,20 +9,17 @@ "version": "1.2.3", "rules": [ { - "id": "SARIF2009", - "name": "ConsiderConventionalIdentifierValues", + "id": "TEST1001", + "name": "Test 1001", "shortDescription": { - "text": "Adopt uniform naming conventions for the symbolic names that SARIF uses various contexts." + "text": "Test 1001 short description." }, "fullDescription": { - "text": "Adopt uniform naming conventions for the symbolic names that SARIF uses various contexts.\r\n\r\nMany tools follow a conventional format for the 'reportingDescriptor.id' property: a short string identifying the tool concatenated with a numeric rule number,\r\nfor example, 'CS2001' for a diagnostic from the Roslyn C# compiler. For uniformity of experience across tools, we recommend this format.\r\n\r\nMany tool use similar names for 'uriBaseId' symbols. We suggest 'REPOROOT' for the root of a repository, 'SRCROOT' for the root of the directory containing all source code, 'TESTROOT' for the root of the directory containing all test code (if your repository is organized in that way), and 'BINROOT' for the root of the directory containing build output (if your project places all build output in a common directory)." + "text": "Test 1001 full description." }, "messageStrings": { - "Note_UseConventionalRuleIds": { - "text": "{0}: The 'id' property of the rule '{1}' does not follow the recommended format: a short string identifying the tool concatenated with a numeric rule number, for example, `CS2001`. Using a conventional format for the rule id provides a more uniform experience across tools." - }, - "Note_UseConventionalUriBaseIdNames": { - "text": "{0}: The 'originalUriBaseIds' symbol '{1}' is not one of the conventional symbols. We suggest 'REPOROOT' for the root of a repository, 'SRCROOT' for the root of the directory containing all source code, 'TESTROOT' for the root of the directory containing all test code (if your repository is organized in that way), and 'BINROOT' for the root of the directory containing build output (if your project places all build output in a common directory)." + "DoesExist": { + "text": "'{0}': Placeholder '{1}'." } }, "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" @@ -45,11 +42,11 @@ ], "results": [ { - "ruleId": "SARIF2009", + "ruleId": "TEST1001", "ruleIndex": 0, "level": "note", "message": { - "id": "Note_UseConventionalUriBaseIdNames", + "id": "DoesExist", "arguments": [ "runs[0].originalUriBaseIds.SRCINVALID", "SRCINVALID" @@ -68,31 +65,6 @@ } } ] - }, - { - "ruleId": "SARIF2009", - "ruleIndex": 0, - "level": "note", - "message": { - "id": "Note_UseConventionalRuleIds", - "arguments": [ - "runs[0].tool.driver.rules[0].id", - "WRONG00001" - ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "index": 0 - }, - "region": { - "startLine": 13, - "startColumn": 32 - } - } - } - ] } ], "columnKind": "utf16CodeUnits" From 53564a03adca0199f58b357a8fcb8d56cd876ee1 Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Tue, 30 Jun 2020 06:46:22 -0300 Subject: [PATCH 5/7] removing duplicated tags --- .../Rules/RuleResources.Designer.cs | 27 ------------------- src/Sarif.Multitool/Rules/RuleResources.resx | 9 ------- 2 files changed, 36 deletions(-) diff --git a/src/Sarif.Multitool/Rules/RuleResources.Designer.cs b/src/Sarif.Multitool/Rules/RuleResources.Designer.cs index 467ab860f..5862afec7 100644 --- a/src/Sarif.Multitool/Rules/RuleResources.Designer.cs +++ b/src/Sarif.Multitool/Rules/RuleResources.Designer.cs @@ -644,33 +644,6 @@ internal static string SARIF2011_ProvideContextRegion_Note_Default_Text { } } - /// - /// Looks up a localized string similar to Placeholder_SARIF2012_ProvideHelpUris_FullDescription_Text. - /// - internal static string SARIF2012_ProvideHelpUris_FullDescription_Text { - get { - return ResourceManager.GetString("SARIF2012_ProvideHelpUris_FullDescription_Text", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to Placeholder_SARIF2012_ProvideHelpUris_Note_Default_Text. - /// - internal static string SARIF2012_ProvideHelpUris_Note_Default_Text { - get { - return ResourceManager.GetString("SARIF2012_ProvideHelpUris_Note_Default_Text", resourceCulture); - } - } - - /// - /// Looks up a localized string similar to Placeholder_SARIF2013_ProvideEmbeddedFileContent_FullDescription_Text. - /// - internal static string SARIF2013_ProvideEmbeddedFileContent_FullDescription_Text { - get { - return ResourceManager.GetString("SARIF2013_ProvideEmbeddedFileContent_FullDescription_Text", resourceCulture); - } - } - /// /// Looks up a localized string similar to Placeholder_SARIF2013_ProvideEmbeddedFileContent_Note_Default_Text. /// diff --git a/src/Sarif.Multitool/Rules/RuleResources.resx b/src/Sarif.Multitool/Rules/RuleResources.resx index d5fe95f7a..7c60be2f8 100644 --- a/src/Sarif.Multitool/Rules/RuleResources.resx +++ b/src/Sarif.Multitool/Rules/RuleResources.resx @@ -283,15 +283,6 @@ Many tool use similar names for 'uriBaseId' symbols. We suggest 'REPOROOT' for t {0}: Placeholder_SARIF2004_OptimizeFileSize_Warning_EliminateLocationOnlyArtifacts_Text - - Placeholder_SARIF1012_MessageArgumentsMustBeConsistentWithRule_Error_MessageIdMustExist_Text - - - Placeholder_SARIF1012_MessageArgumentsMustBeConsistentWithRule_Error_SupplyEnoughMessageArguments_Text - - - Placeholder_SARIF1012_MessageArgumentsMustBeConsistentWithRule_FullDescription_Text - Placeholder_SARIF2002_ProvideMessageArguments_FullDescription_Text From 6a1c0ad00063c942489976264be4541c2fcf60da Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Tue, 30 Jun 2020 11:38:32 -0300 Subject: [PATCH 6/7] code review - 4 --- ...essageArgumentsMustBeConsistentWithRule.cs | 18 +++--- ...entsMustBeConsistentWithRule_Invalid.sarif | 33 +--------- ...entsMustBeConsistentWithRule_Invalid.sarif | 62 +++---------------- ...umentsMustBeConsistentWithRule_Valid.sarif | 35 +---------- 4 files changed, 19 insertions(+), 129 deletions(-) diff --git a/src/Sarif.Multitool/Rules/SARIF1012.MessageArgumentsMustBeConsistentWithRule.cs b/src/Sarif.Multitool/Rules/SARIF1012.MessageArgumentsMustBeConsistentWithRule.cs index 2bb4577cd..8a5fca540 100644 --- a/src/Sarif.Multitool/Rules/SARIF1012.MessageArgumentsMustBeConsistentWithRule.cs +++ b/src/Sarif.Multitool/Rules/SARIF1012.MessageArgumentsMustBeConsistentWithRule.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using System; using System.Collections.Generic; using System.Linq; using System.Text.RegularExpressions; @@ -26,6 +27,7 @@ public class MessageArgumentsMustBeConsistentWithRule : SarifValidationSkimmerBa public override FailureLevel DefaultLevel => FailureLevel.Error; + private static readonly Regex s_replacementSequenceRegex = new Regex(@"\{(\d+)\}", RegexOptions.Compiled | RegexOptions.CultureInvariant); private IList currentRules; private Run run; @@ -37,7 +39,7 @@ protected override void Analyze(Run run, string runPointer) protected override void Analyze(Result result, string resultPointer) { - // if message.id is not null, we have to check if that exists in rules + // If message.id is present, check that a message with that id exists in the rule. if (!string.IsNullOrEmpty(result.Message.Id)) { ReportingDescriptor rule = result.GetRule(this.run); @@ -54,7 +56,7 @@ protected override void Analyze(Result result, string resultPointer) return; } - // we have rules, we find it but, the count of parameters are invalid + // A message with the specified key is present in the rule. Check if the result supplied enough arguments. string messageText = rule.MessageStrings[result.Message.Id].Text; int placeholderMaxPosition = PlaceholderMaxPosition(messageText); if (placeholderMaxPosition > (result.Message.Arguments?.Count ?? 0)) @@ -74,17 +76,11 @@ protected override void Analyze(Result result, string resultPointer) private int PlaceholderMaxPosition(string text) { - MatchCollection matchCollection = Regex.Matches(text, "{\\d+}"); int max = -1; - foreach (Match match in matchCollection) + foreach (Match match in s_replacementSequenceRegex.Matches(text)) { - if (int.TryParse(match.Value.Replace("{", string.Empty).Replace("}", string.Empty), out int temp)) - { - if (max < temp) - { - max = temp; - } - } + int index = int.Parse(match.Groups["index"].Value); + max = Math.Max(max, index); } return max++; diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif index b3f841916..634982330 100644 --- a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif @@ -50,7 +50,7 @@ "message": { "id": "Error_MessageIdMustExist", "arguments": [ - "runs[0].results[0]", + "runs[0].results[1]", "DoesNotExist", "TEST1001" ] @@ -62,36 +62,7 @@ "index": 0 }, "region": { - "startLine": 44, - "startColumn": 9 - } - } - } - ] - }, - { - "ruleId": "SARIF1012", - "ruleIndex": 0, - "level": "error", - "message": { - "id": "Error_SupplyEnoughMessageArguments", - "arguments": [ - "runs[0].results[1]", - "1", - "DoesExist", - "TEST1001", - "2", - "'{0}': Placeholder '{1}' '{2}'." - ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "index": 0 - }, - "region": { - "startLine": 69, + "startLine": 37, "startColumn": 9 } } diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif index 4a9f7ed92..a97b971f5 100644 --- a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif @@ -11,84 +11,38 @@ { "id": "TEST1001", "name": "Test 1001", - "shortDescription": { - "text": "Test 1001 short description." - }, "fullDescription": { "text": "Test 1001 full description." }, "messageStrings": { "DoesExist": { - "text": "'{0}': Placeholder '{1}' '{2}'." + "text": "'{0}': Placeholder '{1}'." } - }, - "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" + } } ] } }, - "invocations": [ - { - "executionSuccessful": true - } - ], - "artifacts": [ - { - "location": { - "uri": "FunctionalTestOutput.ValidateCommand/Inputs.SARIF2009.ConsiderConventionalIdentifierValues_Invalid.sarif", - "uriBaseId": "TEST_DIR" - } - } - ], "results": [ { "ruleId": "TEST1001", "ruleIndex": 0, - "level": "note", "message": { - "id": "DoesNotExist", + "id": "DoesExist", "arguments": [ - "runs[0].originalUriBaseIds.SRCINVALID", - "SRCINVALID" + "runs[0].originalUriBaseIds.SRCINVALID" ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "index": 0 - }, - "region": { - "startLine": 21, - "startColumn": 23 - } - } - } - ] + } }, { "ruleId": "TEST1001", "ruleIndex": 0, - "level": "note", "message": { - "id": "DoesExist", + "id": "DoesNotExist", "arguments": [ - "runs[0].tool.driver.rules[0].id" + "runs[0].originalUriBaseIds.SRCINVALID" ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "index": 0 - }, - "region": { - "startLine": 13, - "startColumn": 32 - } - } - } - ] + } } ], "columnKind": "utf16CodeUnits" diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif index 9387a998b..8d731a9b6 100644 --- a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif @@ -11,9 +11,6 @@ { "id": "TEST1001", "name": "Test 1001", - "shortDescription": { - "text": "Test 1001 short description." - }, "fullDescription": { "text": "Test 1001 full description." }, @@ -21,50 +18,22 @@ "DoesExist": { "text": "'{0}': Placeholder '{1}'." } - }, - "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" + } } ] } }, - "invocations": [ - { - "executionSuccessful": true - } - ], - "artifacts": [ - { - "location": { - "uri": "FunctionalTestOutput.ValidateCommand/Inputs.SARIF2009.ConsiderConventionalIdentifierValues_Invalid.sarif", - "uriBaseId": "TEST_DIR" - } - } - ], "results": [ { "ruleId": "TEST1001", "ruleIndex": 0, - "level": "note", "message": { "id": "DoesExist", "arguments": [ "runs[0].originalUriBaseIds.SRCINVALID", "SRCINVALID" ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "index": 0 - }, - "region": { - "startLine": 21, - "startColumn": 23 - } - } - } - ] + } } ], "columnKind": "utf16CodeUnits" From a38ede243c19e0926d7b6534a8cbd4c22ab344ce Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Tue, 30 Jun 2020 11:51:34 -0300 Subject: [PATCH 7/7] updating tests --- ...F1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif | 2 +- ...F1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif | 1 - ...RIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif | 1 - 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif index 634982330..8e3dee912 100644 --- a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif @@ -62,7 +62,7 @@ "index": 0 }, "region": { - "startLine": 37, + "startLine": 36, "startColumn": 9 } } diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif index a97b971f5..82c78fc4b 100644 --- a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Invalid.sarif @@ -10,7 +10,6 @@ "rules": [ { "id": "TEST1001", - "name": "Test 1001", "fullDescription": { "text": "Test 1001 full description." }, diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif index 8d731a9b6..1e5cefa32 100644 --- a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF1012.MessageArgumentsMustBeConsistentWithRule_Valid.sarif @@ -10,7 +10,6 @@ "rules": [ { "id": "TEST1001", - "name": "Test 1001", "fullDescription": { "text": "Test 1001 full description." },