From e32333a9b614a24f37314cf15fdf6121e4ef5239 Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Wed, 24 Jun 2020 15:38:43 -0300 Subject: [PATCH 1/3] Adding rule SARIF2001 --- .../Rules/RuleResources.Designer.cs | 20 +++- src/Sarif.Multitool/Rules/RuleResources.resx | 8 +- .../SARIF2001.AuthorHighQualityMessages.cs | 105 +++++++++++------- 3 files changed, 89 insertions(+), 44 deletions(-) diff --git a/src/Sarif.Multitool/Rules/RuleResources.Designer.cs b/src/Sarif.Multitool/Rules/RuleResources.Designer.cs index 27074b9f3..3ca725c32 100644 --- a/src/Sarif.Multitool/Rules/RuleResources.Designer.cs +++ b/src/Sarif.Multitool/Rules/RuleResources.Designer.cs @@ -301,7 +301,25 @@ internal static string SARIF2001_AuthorHighQualityMessages_FullDescription_Text } /// - /// Looks up a localized string similar to {0}: The message "{1}" does not end with a period.. + /// Looks up a localized string similar to {0}: Placeholder '{1}'. + /// + internal static string SARIF2001_AuthorHighQualityMessages_Warning_EnquoteDynamicContent_Text { + get { + return ResourceManager.GetString("SARIF2001_AuthorHighQualityMessages_Warning_EnquoteDynamicContent_Text", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to {0}: Placeholder '{1}'. + /// + internal static string SARIF2001_AuthorHighQualityMessages_Warning_IncludeDynamicContent_Text { + get { + return ResourceManager.GetString("SARIF2001_AuthorHighQualityMessages_Warning_IncludeDynamicContent_Text", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to {0}: Placeholder '{1}'. /// internal static string SARIF2001_AuthorHighQualityMessages_Warning_TerminateWithPeriod_Text { get { diff --git a/src/Sarif.Multitool/Rules/RuleResources.resx b/src/Sarif.Multitool/Rules/RuleResources.resx index 79ca87dd7..b0fc89a4d 100644 --- a/src/Sarif.Multitool/Rules/RuleResources.resx +++ b/src/Sarif.Multitool/Rules/RuleResources.resx @@ -136,7 +136,7 @@ The end time of a run must not precede the start time. To allow for the possibility that the duration of the run is less than the resolution of the string representation of the time, the start time and the end time may be equal. - {0}: The message "{1}" does not end with a period. + {0}: Placeholder '{1}' Messages should consist of one or more complete sentences, ending with a period. @@ -216,4 +216,10 @@ {0}: Placeholder '{1}' + + {0}: Placeholder '{1}' + + + {0}: Placeholder '{1}' + \ No newline at end of file diff --git a/src/Sarif.Multitool/Rules/SARIF2001.AuthorHighQualityMessages.cs b/src/Sarif.Multitool/Rules/SARIF2001.AuthorHighQualityMessages.cs index f03516be4..df9f424c5 100644 --- a/src/Sarif.Multitool/Rules/SARIF2001.AuthorHighQualityMessages.cs +++ b/src/Sarif.Multitool/Rules/SARIF2001.AuthorHighQualityMessages.cs @@ -3,85 +3,106 @@ using System; using System.Collections.Generic; +using System.Text.RegularExpressions; + using Microsoft.Json.Pointer; namespace Microsoft.CodeAnalysis.Sarif.Multitool.Rules { public class AuthorHighQualityMessages : SarifValidationSkimmerBase { - public override MultiformatMessageString FullDescription => new MultiformatMessageString - { - Text = RuleResources.SARIF2001_AuthorHighQualityMessages_FullDescription_Text + /// + /// SARIF2001 + /// + public override string Id => RuleId.AuthorHighQualityMessages; + + /// + /// Placeholder (full description). + /// + public override MultiformatMessageString FullDescription => new MultiformatMessageString { Text = RuleResources.SARIF2001_AuthorHighQualityMessages_FullDescription_Text }; + + protected override IEnumerable MessageResourceNames => new string[] { + nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_EnquoteDynamicContent_Text), + nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_IncludeDynamicContent_Text), + nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_TerminateWithPeriod_Text) }; public override FailureLevel DefaultLevel => FailureLevel.Warning; - public override string Id => RuleId.AuthorHighQualityMessages; + private static readonly Regex s_dynamicContentRegex = new Regex(@"{[0-9]+}", RegexOptions.Compiled | RegexOptions.CultureInvariant); + private static readonly Regex s_enquoteDynamicContentRegex = new Regex(@"'{[0-9]+}'", RegexOptions.Compiled | RegexOptions.CultureInvariant); - protected override IEnumerable MessageResourceNames + protected override void Analyze(Tool tool, string toolPointer) { - get + if (tool.Driver != null) { - return new string[] - { - nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_TerminateWithPeriod_Text) - }; + AnalyzeToolDriver(tool.Driver, toolPointer.AtProperty(SarifPropertyName.Driver)); } } - protected override void Analyze(ReportingDescriptor reportingDescriptor, string reportingDescriptorPointer) + private void AnalyzeToolDriver(ToolComponent toolComponent, string toolDriverPointer) { - AnalyzeMessageStrings(reportingDescriptor.MessageStrings, reportingDescriptorPointer); + if (toolComponent.Rules != null && toolComponent.Rules.Count > 0) + { + foreach (ReportingDescriptor rule in toolComponent.Rules) + { + AnalyzeReportingDescriptor(rule, toolDriverPointer.AtProperty(SarifPropertyName.Rules)); + } + } } - private void AnalyzeMessageStrings( - IDictionary messageStrings, - string reportingDescriptorPointer) + private void AnalyzeReportingDescriptor(ReportingDescriptor rule, string reportingDescriptorPointer) { - if (messageStrings != null) + if (rule.MessageStrings != null && rule.MessageStrings.Count > 0) { string messageStringsPointer = reportingDescriptorPointer.AtProperty(SarifPropertyName.MessageStrings); - - foreach (string key in messageStrings.Keys) + foreach (KeyValuePair message in rule.MessageStrings) { - MultiformatMessageString messageString = messageStrings[key]; - - string messageStringPointer = messageStringsPointer.AtProperty(key); - - AnalyzeMessageString(messageString.Text, messageStringPointer, SarifPropertyName.Text); + AnalyzeMessageString(message.Value.Text, messageStringsPointer.AtProperty(message.Key)); } } } - protected override void Analyze(MultiformatMessageString multiformatMessageString, string multiformatMessageStringPointer) + private void AnalyzeMessageString(string messageString, string messagePointer) { - AnalyzeMessageString(multiformatMessageString.Text, multiformatMessageStringPointer, SarifPropertyName.Text); - } + if (string.IsNullOrEmpty(messageString)) + { + return; + } - protected override void Analyze(Message message, string messagePointer) - { - AnalyzeMessageString(message.Text, messagePointer, SarifPropertyName.Text); - } + string textPointer = messagePointer.AtProperty(SarifPropertyName.Text); - private void AnalyzeMessageString( - string messageString, - string messagePointer, - string propertyName) - { - if (!string.IsNullOrEmpty(messageString) && DoesNotEndWithPeriod(messageString)) + // IncludeDynamicContent: check if messageString has dynamic content + if (!s_dynamicContentRegex.IsMatch(messageString)) + { + // {0}: Placeholder '{1}' + LogResult( + textPointer, + nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_IncludeDynamicContent_Text), + messageString + ); + } + + // EnquoteDynamicContent: check if messageString has enquoted dynamic content + if (!s_enquoteDynamicContentRegex.IsMatch(messageString)) { - string textPointer = messagePointer.AtProperty(propertyName); + // {0}: Placeholder '{1}' + LogResult( + textPointer, + nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_EnquoteDynamicContent_Text), + messageString + ); + } + // TerminateWithPeriod: check if messageString ends with period + if (!messageString.EndsWith(".", StringComparison.Ordinal)) + { + // {0}: Placeholder '{1}' LogResult( textPointer, nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_TerminateWithPeriod_Text), messageString); } } - - private static bool DoesNotEndWithPeriod(string message) - { - return message != null && !message.EndsWith(".", StringComparison.Ordinal); - } } } From 4e6c09adb0908cd6863abc8e353c9ab27f6f829a Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Wed, 24 Jun 2020 16:10:10 -0300 Subject: [PATCH 2/3] code review - 1 --- .../Rules/RuleResources.Designer.cs | 6 +- src/Sarif.Multitool/Rules/RuleResources.resx | 6 +- .../SARIF2001.AuthorHighQualityMessages.cs | 36 +- ...01.AuthorHighQualityMessages_Invalid.sarif | 308 +++--------------- ...01.AuthorHighQualityMessages_Invalid.sarif | 174 +--------- ...2001.AuthorHighQualityMessages_Valid.sarif | 170 +--------- 6 files changed, 90 insertions(+), 610 deletions(-) diff --git a/src/Sarif.Multitool/Rules/RuleResources.Designer.cs b/src/Sarif.Multitool/Rules/RuleResources.Designer.cs index 3ca725c32..6d1002f62 100644 --- a/src/Sarif.Multitool/Rules/RuleResources.Designer.cs +++ b/src/Sarif.Multitool/Rules/RuleResources.Designer.cs @@ -301,7 +301,7 @@ internal static string SARIF2001_AuthorHighQualityMessages_FullDescription_Text } /// - /// Looks up a localized string similar to {0}: Placeholder '{1}'. + /// Looks up a localized string similar to {0}: Placeholder '{1}' '{2}'. /// internal static string SARIF2001_AuthorHighQualityMessages_Warning_EnquoteDynamicContent_Text { get { @@ -310,7 +310,7 @@ internal static string SARIF2001_AuthorHighQualityMessages_Warning_EnquoteDynami } /// - /// Looks up a localized string similar to {0}: Placeholder '{1}'. + /// Looks up a localized string similar to {0}: Placeholder '{1}' '{2}'. /// internal static string SARIF2001_AuthorHighQualityMessages_Warning_IncludeDynamicContent_Text { get { @@ -319,7 +319,7 @@ internal static string SARIF2001_AuthorHighQualityMessages_Warning_IncludeDynami } /// - /// Looks up a localized string similar to {0}: Placeholder '{1}'. + /// Looks up a localized string similar to {0}: Placeholder '{1}' '{2}'. /// internal static string SARIF2001_AuthorHighQualityMessages_Warning_TerminateWithPeriod_Text { get { diff --git a/src/Sarif.Multitool/Rules/RuleResources.resx b/src/Sarif.Multitool/Rules/RuleResources.resx index b0fc89a4d..ba21d817e 100644 --- a/src/Sarif.Multitool/Rules/RuleResources.resx +++ b/src/Sarif.Multitool/Rules/RuleResources.resx @@ -136,7 +136,7 @@ The end time of a run must not precede the start time. To allow for the possibility that the duration of the run is less than the resolution of the string representation of the time, the start time and the end time may be equal. - {0}: Placeholder '{1}' + {0}: Placeholder '{1}' '{2}' Messages should consist of one or more complete sentences, ending with a period. @@ -217,9 +217,9 @@ {0}: Placeholder '{1}' - {0}: Placeholder '{1}' + {0}: Placeholder '{1}' '{2}' - {0}: Placeholder '{1}' + {0}: Placeholder '{1}' '{2}' \ No newline at end of file diff --git a/src/Sarif.Multitool/Rules/SARIF2001.AuthorHighQualityMessages.cs b/src/Sarif.Multitool/Rules/SARIF2001.AuthorHighQualityMessages.cs index df9f424c5..373ce0de0 100644 --- a/src/Sarif.Multitool/Rules/SARIF2001.AuthorHighQualityMessages.cs +++ b/src/Sarif.Multitool/Rules/SARIF2001.AuthorHighQualityMessages.cs @@ -42,28 +42,29 @@ protected override void Analyze(Tool tool, string toolPointer) private void AnalyzeToolDriver(ToolComponent toolComponent, string toolDriverPointer) { - if (toolComponent.Rules != null && toolComponent.Rules.Count > 0) + if (toolComponent.Rules != null) { - foreach (ReportingDescriptor rule in toolComponent.Rules) + string rulesPointer = toolDriverPointer.AtProperty(SarifPropertyName.Rules); + for (int i = 0; i < toolComponent.Rules.Count; i++) { - AnalyzeReportingDescriptor(rule, toolDriverPointer.AtProperty(SarifPropertyName.Rules)); + AnalyzeReportingDescriptor(toolComponent.Rules[i], rulesPointer.AtIndex(i)); } } } private void AnalyzeReportingDescriptor(ReportingDescriptor rule, string reportingDescriptorPointer) { - if (rule.MessageStrings != null && rule.MessageStrings.Count > 0) + if (rule.MessageStrings != null) { string messageStringsPointer = reportingDescriptorPointer.AtProperty(SarifPropertyName.MessageStrings); foreach (KeyValuePair message in rule.MessageStrings) { - AnalyzeMessageString(message.Value.Text, messageStringsPointer.AtProperty(message.Key)); + AnalyzeMessageString(message.Value.Text, message.Key, messageStringsPointer.AtProperty(message.Key)); } } } - private void AnalyzeMessageString(string messageString, string messagePointer) + private void AnalyzeMessageString(string messageString, string messageKey, string messagePointer) { if (string.IsNullOrEmpty(messageString)) { @@ -72,36 +73,37 @@ private void AnalyzeMessageString(string messageString, string messagePointer) string textPointer = messagePointer.AtProperty(SarifPropertyName.Text); - // IncludeDynamicContent: check if messageString has dynamic content + // IncludeDynamicContent: check if messageString has dynamic content. if (!s_dynamicContentRegex.IsMatch(messageString)) { - // {0}: Placeholder '{1}' + // {0}: Placeholder '{1}' '{2}' LogResult( textPointer, nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_IncludeDynamicContent_Text), - messageString - ); + messageString, + messageKey); } - // EnquoteDynamicContent: check if messageString has enquoted dynamic content + // EnquoteDynamicContent: check if messageString has enquoted dynamic content. if (!s_enquoteDynamicContentRegex.IsMatch(messageString)) { - // {0}: Placeholder '{1}' + // {0}: Placeholder '{1}' '{2}' LogResult( textPointer, nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_EnquoteDynamicContent_Text), - messageString - ); + messageString, + messageKey); } - // TerminateWithPeriod: check if messageString ends with period + // TerminateWithPeriod: check if messageString ends with period. if (!messageString.EndsWith(".", StringComparison.Ordinal)) { - // {0}: Placeholder '{1}' + // {0}: Placeholder '{1}' '{2}' LogResult( textPointer, nameof(RuleResources.SARIF2001_AuthorHighQualityMessages_Warning_TerminateWithPeriod_Text), - messageString); + messageString, + messageKey); } } } diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF2001.AuthorHighQualityMessages_Invalid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF2001.AuthorHighQualityMessages_Invalid.sarif index ad2ce6e91..68a6b78b0 100644 --- a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF2001.AuthorHighQualityMessages_Invalid.sarif +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF2001.AuthorHighQualityMessages_Invalid.sarif @@ -17,8 +17,14 @@ "text": "Messages should consist of one or more complete sentences, ending with a period." }, "messageStrings": { + "Warning_EnquoteDynamicContent": { + "text": "{0}: Placeholder '{1}' '{2}'" + }, + "Warning_IncludeDynamicContent": { + "text": "{0}: Placeholder '{1}' '{2}'" + }, "Warning_TerminateWithPeriod": { - "text": "{0}: The message \"{1}\" does not end with a period." + "text": "{0}: Placeholder '{1}' '{2}'" } }, "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" @@ -46,56 +52,9 @@ "message": { "id": "Warning_TerminateWithPeriod", "arguments": [ - "runs[0].results[0].attachments[0].description.text", - "attachment description without period" - ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "index": 0 - }, - "region": { - "startLine": 137, - "startColumn": 63 - } - } - } - ] - }, - { - "ruleId": "SARIF2001", - "ruleIndex": 0, - "message": { - "id": "Warning_TerminateWithPeriod", - "arguments": [ - "runs[0].results[0].attachments[0].rectangles[0].message.text", - "rectangle message without period" - ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "index": 0 - }, - "region": { - "startLine": 148, - "startColumn": 62 - } - } - } - ] - }, - { - "ruleId": "SARIF2001", - "ruleIndex": 0, - "message": { - "id": "Warning_TerminateWithPeriod", - "arguments": [ - "runs[0].results[0].locations[0].message.text", - "location message without period" + "runs[0].tool.driver.rules[0].messageStrings.Warning_DontTerminateWithPeriod.text", + "{0}: Placeholder '{1}'", + "Warning_DontTerminateWithPeriod" ] }, "locations": [ @@ -105,8 +64,8 @@ "index": 0 }, "region": { - "startLine": 76, - "startColumn": 57 + "startLine": 22, + "startColumn": 50 } } } @@ -116,10 +75,11 @@ "ruleId": "SARIF2001", "ruleIndex": 0, "message": { - "id": "Warning_TerminateWithPeriod", + "id": "Warning_IncludeDynamicContent", "arguments": [ - "runs[0].results[0].locations[0].physicalLocation.region.message.text", - "region message without period" + "runs[0].tool.driver.rules[0].messageStrings.Warning_DontHavePlaceholders.text", + "Placeholder text", + "Warning_DontHavePlaceholders" ] }, "locations": [ @@ -129,8 +89,8 @@ "index": 0 }, "region": { - "startLine": 70, - "startColumn": 59 + "startLine": 25, + "startColumn": 44 } } } @@ -140,10 +100,11 @@ "ruleId": "SARIF2001", "ruleIndex": 0, "message": { - "id": "Warning_TerminateWithPeriod", + "id": "Warning_EnquoteDynamicContent", "arguments": [ - "runs[0].results[0].codeFlows[0].message.text", - "codeFlow message without period" + "runs[0].tool.driver.rules[0].messageStrings.Warning_DontHavePlaceholders.text", + "Placeholder text", + "Warning_DontHavePlaceholders" ] }, "locations": [ @@ -153,8 +114,8 @@ "index": 0 }, "region": { - "startLine": 102, - "startColumn": 57 + "startLine": 25, + "startColumn": 44 } } } @@ -166,8 +127,9 @@ "message": { "id": "Warning_TerminateWithPeriod", "arguments": [ - "runs[0].results[0].codeFlows[0].threadFlows[0].message.text", - "threadFlow message without period" + "runs[0].tool.driver.rules[0].messageStrings.Warning_DontHavePlaceholders.text", + "Placeholder text", + "Warning_DontHavePlaceholders" ] }, "locations": [ @@ -177,8 +139,8 @@ "index": 0 }, "region": { - "startLine": 108, - "startColumn": 63 + "startLine": 25, + "startColumn": 44 } } } @@ -188,202 +150,11 @@ "ruleId": "SARIF2001", "ruleIndex": 0, "message": { - "id": "Warning_TerminateWithPeriod", - "arguments": [ - "runs[0].results[0].codeFlows[0].threadFlows[0].locations[0].location.message.text", - "ThreadFlow location message without period" - ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "index": 0 - }, - "region": { - "startLine": 124, - "startColumn": 78 - } - } - } - ] - }, - { - "ruleId": "SARIF2001", - "ruleIndex": 0, - "message": { - "id": "Warning_TerminateWithPeriod", - "arguments": [ - "runs[0].results[0].message.text", - "result message without period" - ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "index": 0 - }, - "region": { - "startLine": 58, - "startColumn": 51 - } - } - } - ] - }, - { - "ruleId": "SARIF2001", - "ruleIndex": 0, - "message": { - "id": "Warning_TerminateWithPeriod", - "arguments": [ - "runs[0].results[0].stacks[0].frames[0].location.message.text", - "frame message without period" - ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "index": 0 - }, - "region": { - "startLine": 91, - "startColumn": 60 - } - } - } - ] - }, - { - "ruleId": "SARIF2001", - "ruleIndex": 0, - "message": { - "id": "Warning_TerminateWithPeriod", - "arguments": [ - "runs[0].results[0].stacks[0].message.text", - "stack message without period" - ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "index": 0 - }, - "region": { - "startLine": 84, - "startColumn": 54 - } - } - } - ] - }, - { - "ruleId": "SARIF2001", - "ruleIndex": 0, - "message": { - "id": "Warning_TerminateWithPeriod", - "arguments": [ - "runs[0].results[0].fixes[0].description.text", - "fix description without period" - ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "index": 0 - }, - "region": { - "startLine": 158, - "startColumn": 56 - } - } - } - ] - }, - { - "ruleId": "SARIF2001", - "ruleIndex": 0, - "message": { - "id": "Warning_TerminateWithPeriod", - "arguments": [ - "runs[0].invocations[0].toolExecutionNotifications[0].message.text", - "toolNotification message without period" - ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "index": 0 - }, - "region": { - "startLine": 36, - "startColumn": 65 - } - } - } - ] - }, - { - "ruleId": "SARIF2001", - "ruleIndex": 0, - "message": { - "id": "Warning_TerminateWithPeriod", - "arguments": [ - "runs[0].invocations[0].toolConfigurationNotifications[0].message.text", - "configurationNotification message without period" - ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "index": 0 - }, - "region": { - "startLine": 44, - "startColumn": 74 - } - } - } - ] - }, - { - "ruleId": "SARIF2001", - "ruleIndex": 0, - "message": { - "id": "Warning_TerminateWithPeriod", - "arguments": [ - "runs[0].tool.driver.rules[0].messageStrings.Default.text", - "rule message without period" - ] - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "index": 0 - }, - "region": { - "startLine": 23, - "startColumn": 55 - } - } - } - ] - }, - { - "ruleId": "SARIF2001", - "ruleIndex": 0, - "message": { - "id": "Warning_TerminateWithPeriod", + "id": "Warning_EnquoteDynamicContent", "arguments": [ - "runs[0].tool.driver.rules[0].shortDescription.text", - "Short rule description without period" + "runs[0].tool.driver.rules[0].messageStrings.Warning_DontHaveEnquotedPlaceholders.text", + "{0}: Placeholder {1}", + "Warning_DontHaveEnquotedPlaceholders" ] }, "locations": [ @@ -393,8 +164,8 @@ "index": 0 }, "region": { - "startLine": 14, - "startColumn": 63 + "startLine": 28, + "startColumn": 48 } } } @@ -406,8 +177,9 @@ "message": { "id": "Warning_TerminateWithPeriod", "arguments": [ - "runs[0].tool.driver.rules[0].fullDescription.text", - "Short full rule description without period" + "runs[0].tool.driver.rules[0].messageStrings.Warning_DontHaveEnquotedPlaceholders.text", + "{0}: Placeholder {1}", + "Warning_DontHaveEnquotedPlaceholders" ] }, "locations": [ @@ -417,8 +189,8 @@ "index": 0 }, "region": { - "startLine": 18, - "startColumn": 68 + "startLine": 28, + "startColumn": 48 } } } diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF2001.AuthorHighQualityMessages_Invalid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF2001.AuthorHighQualityMessages_Invalid.sarif index 6b5f64926..1f1dab4b6 100644 --- a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF2001.AuthorHighQualityMessages_Invalid.sarif +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF2001.AuthorHighQualityMessages_Invalid.sarif @@ -5,178 +5,34 @@ { "tool": { "driver": { - "name": "CodeScanner", - "version": "1.0", + "name": "SARIF Functional Testing", + "version": "1.2", "rules": [ { - "id": "TST0001", + "id": "SARIF2001", + "name": "AuthorHighQualityMessages", "shortDescription": { - "text": "Short rule description without period", - "markdown": "_Rich_ short rule description without period" + "text": "Messages should consist of one or more complete sentences, ending with a period." }, "fullDescription": { - "text": "Short full rule description without period", - "markdown": "_Rich_ full rule description without period" + "text": "Messages should consist of one or more complete sentences, ending with a period." }, "messageStrings": { - "Default": { - "text": "rule message without period", - "markdown": "_Rich_ rule message without period" - } - } - } - ] - } - }, - "invocations": [ - { - "toolExecutionNotifications": [ - { - "message": { - "text": "toolNotification message without period", - "markdown": "_Rich_ toolNotification message without period" - } - } - ], - "toolConfigurationNotifications": [ - { - "message": { - "text": "configurationNotification message without period", - "markdown": "_Rich_ configuratioNotification message without period" - } - } - ], - "executionSuccessful": true - } - ], - "results": [ - { - "ruleId": "TST0001", - "ruleIndex": 0, - "level": "error", - "message": { - "text": "result message without period", - "markdown": "_Rich_ result message without period" - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "uri": "file:///c:/src/file.c" + "Warning_DontTerminateWithPeriod": { + "text": "{0}: Placeholder '{1}'" }, - "region": { - "startLine": 1, - "message": { - "text": "region message without period", - "markdown": "_Rich_ region message without period" - } - } - }, - "message": { - "text": "location message without period", - "markdown": "_Rich_ location message without period" - } - } - ], - "stacks": [ - { - "message": { - "text": "stack message without period", - "markdown": "_Rich_ stack message without period" - }, - "frames": [ - { - "location": { - "message": { - "text": "frame message without period", - "markdown": "_Rich_ frame message without period" - } - } - } - ] - } - ], - "codeFlows": [ - { - "message": { - "text": "codeFlow message without period", - "markdown": "_Rich_ codeFlow message without period" - }, - "threadFlows": [ - { - "message": { - "text": "threadFlow message without period", - "markdown": "_Rich_ threadFlow message without period" - }, - "locations": [ - { - "location": { - "physicalLocation": { - "artifactLocation": { - "uri": "file:///c:/src/file.c" - }, - "region": { - "startLine": 42, - "startColumn": 12 - } - }, - "message": { - "text": "ThreadFlow location message without period", - "markdown": "_Rich_ ThreadFlow location message without period" - } - } - } - ] - } - ] - } - ], - "attachments": [ - { - "description": { - "text": "attachment description without period", - "markdown": "_Rich_ attachment description without period" - }, - "artifactLocation": { - "uri": "file:///c:/output/screenshot.png" - }, - "rectangles": [ - { - "left": 100.0, - "right": 100.0, - "message": { - "text": "rectangle message without period", - "markdown": "_Rich_ rectangle message without period" - } + "Warning_DontHavePlaceholders": { + "text": "Placeholder text" + }, + "Warning_DontHaveEnquotedPlaceholders": { + "text": "{0}: Placeholder {1}" } - ] - } - ], - "fixes": [ - { - "description": { - "text": "fix description without period", - "markdown": "_Rich_ fix description without period" }, - "artifactChanges": [ - { - "artifactLocation": { - "uri": "file:///c:/src/file.c" - }, - "replacements": [ - { - "deletedRegion": { - "byteOffset": 0, - "byteLength": 1 - } - } - ] - } - ] + "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" } ] } - ], + }, "columnKind": "utf16CodeUnits" } ] diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF2001.AuthorHighQualityMessages_Valid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF2001.AuthorHighQualityMessages_Valid.sarif index db75a7e8a..d2cbc02d8 100644 --- a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF2001.AuthorHighQualityMessages_Valid.sarif +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/Inputs/SARIF2001.AuthorHighQualityMessages_Valid.sarif @@ -5,178 +5,28 @@ { "tool": { "driver": { - "name": "CodeScanner", - "version": "1.0", + "name": "SARIF Functional Testing", + "version": "1.2", "rules": [ { - "id": "TST0001", + "id": "SARIF2001", + "name": "AuthorHighQualityMessages", "shortDescription": { - "text": "Short rule description with period.", - "markdown": "_Rich_ short rule description with period." + "text": "Messages should consist of one or more complete sentences, ending with a period." }, "fullDescription": { - "text": "Short full rule description with period.", - "markdown": "_Rich_ full rule description with period." + "text": "Messages should consist of one or more complete sentences, ending with a period." }, "messageStrings": { - "Default": { - "text": "rule message with period.", - "markdown": "_Rich_ rule message with period." - } - } - } - ] - } - }, - "invocations": [ - { - "toolExecutionNotifications": [ - { - "message": { - "text": "toolNotification message with period.", - "markdown": "_Rich_ toolNotification message with period." - } - } - ], - "toolConfigurationNotifications": [ - { - "message": { - "text": "configurationNotification message with period.", - "markdown": "_Rich_ configuratioNotification message with period." - } - } - ], - "executionSuccessful": true - } - ], - "results": [ - { - "ruleId": "TST0001", - "ruleIndex": 0, - "level": "error", - "message": { - "text": "result message with period.", - "markdown": "_Rich_ result message with period." - }, - "locations": [ - { - "physicalLocation": { - "artifactLocation": { - "uri": "file:///c:/src/file.c" - }, - "region": { - "startLine": 1, - "message": { - "text": "region message with period.", - "markdown": "_Rich_ region message with period." - } + "Warning_TerminateWithPeriod": { + "text": "{0}: Placeholder '{1}'." } }, - "message": { - "text": "location message with period.", - "markdown": "_Rich_ location message with period." - } - } - ], - "stacks": [ - { - "message": { - "text": "stack message with period.", - "markdown": "_Rich_ stack message with period." - }, - "frames": [ - { - "location": { - "message": { - "text": "frame message with period.", - "markdown": "_Rich_ frame message with period." - } - } - } - ] - } - ], - "codeFlows": [ - { - "message": { - "text": "codeFlow message with period.", - "markdown": "_Rich_ codeFlow message with period." - }, - "threadFlows": [ - { - "message": { - "text": "threadFlow message with period.", - "markdown": "_Rich_ threadFlow message with period." - }, - "locations": [ - { - "location": { - "physicalLocation": { - "artifactLocation": { - "uri": "file:///c:/src/file.c" - }, - "region": { - "startLine": 42, - "startColumn": 12 - } - }, - "message": { - "text": "ThreadFlow location message with period.", - "markdown": "_Rich_ ThreadFlow location message with period." - } - } - } - ] - } - ] - } - ], - "attachments": [ - { - "description": { - "text": "attachment description with period.", - "markdown": "_Rich_ attachment description with period." - }, - "artifactLocation": { - "uri": "file:///c:/output/screenshot.png" - }, - "rectangles": [ - { - "left": 100.0, - "right": 100.0, - "message": { - "text": "rectangle message with period.", - "markdown": "_Rich_ rectangle message with period." - } - } - ] - } - ], - "fixes": [ - { - "description": { - "text": "fix description with period.", - "markdown": "_Rich_ fix description with period." - }, - "artifactChanges": [ - { - "artifactLocation": { - "uri": "file:///c:/src/file.c" - }, - "replacements": [ - { - "deletedRegion": { - "byteOffset": 0, - "byteLength": 1 - } - } - ] - } - ] + "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" } ] } - ], + }, "columnKind": "utf16CodeUnits" } ] From 881f1b7fc6ef5662048463ce60339e0b4b48054c Mon Sep 17 00:00:00 2001 From: Eddy Nakamura Date: Wed, 24 Jun 2020 16:58:41 -0300 Subject: [PATCH 3/3] code review - 2 --- .../SARIF2001.AuthorHighQualityMessages.cs | 6 +-- ...01.AuthorHighQualityMessages_Invalid.sarif | 16 +++--- ...2001.AuthorHighQualityMessages_Valid.sarif | 54 ++++++++++++++++++- 3 files changed, 63 insertions(+), 13 deletions(-) diff --git a/src/Sarif.Multitool/Rules/SARIF2001.AuthorHighQualityMessages.cs b/src/Sarif.Multitool/Rules/SARIF2001.AuthorHighQualityMessages.cs index 373ce0de0..092f5dfd5 100644 --- a/src/Sarif.Multitool/Rules/SARIF2001.AuthorHighQualityMessages.cs +++ b/src/Sarif.Multitool/Rules/SARIF2001.AuthorHighQualityMessages.cs @@ -29,8 +29,8 @@ public class AuthorHighQualityMessages : SarifValidationSkimmerBase public override FailureLevel DefaultLevel => FailureLevel.Warning; - private static readonly Regex s_dynamicContentRegex = new Regex(@"{[0-9]+}", RegexOptions.Compiled | RegexOptions.CultureInvariant); - private static readonly Regex s_enquoteDynamicContentRegex = new Regex(@"'{[0-9]+}'", RegexOptions.Compiled | RegexOptions.CultureInvariant); + private static readonly Regex s_dynamicContentRegex = new Regex(@"\{[0-9]+\}", RegexOptions.Compiled | RegexOptions.CultureInvariant); + private static readonly Regex s_nonEnquotedDynamicContextRegex = new Regex(@"(^|[^'])\{[0-9]+\}", RegexOptions.Compiled | RegexOptions.CultureInvariant); protected override void Analyze(Tool tool, string toolPointer) { @@ -85,7 +85,7 @@ private void AnalyzeMessageString(string messageString, string messageKey, strin } // EnquoteDynamicContent: check if messageString has enquoted dynamic content. - if (!s_enquoteDynamicContentRegex.IsMatch(messageString)) + if (s_nonEnquotedDynamicContextRegex.IsMatch(messageString)) { // {0}: Placeholder '{1}' '{2}' LogResult( diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF2001.AuthorHighQualityMessages_Invalid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF2001.AuthorHighQualityMessages_Invalid.sarif index 68a6b78b0..0a6157e59 100644 --- a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF2001.AuthorHighQualityMessages_Invalid.sarif +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF2001.AuthorHighQualityMessages_Invalid.sarif @@ -50,7 +50,7 @@ "ruleId": "SARIF2001", "ruleIndex": 0, "message": { - "id": "Warning_TerminateWithPeriod", + "id": "Warning_EnquoteDynamicContent", "arguments": [ "runs[0].tool.driver.rules[0].messageStrings.Warning_DontTerminateWithPeriod.text", "{0}: Placeholder '{1}'", @@ -75,11 +75,11 @@ "ruleId": "SARIF2001", "ruleIndex": 0, "message": { - "id": "Warning_IncludeDynamicContent", + "id": "Warning_TerminateWithPeriod", "arguments": [ - "runs[0].tool.driver.rules[0].messageStrings.Warning_DontHavePlaceholders.text", - "Placeholder text", - "Warning_DontHavePlaceholders" + "runs[0].tool.driver.rules[0].messageStrings.Warning_DontTerminateWithPeriod.text", + "{0}: Placeholder '{1}'", + "Warning_DontTerminateWithPeriod" ] }, "locations": [ @@ -89,8 +89,8 @@ "index": 0 }, "region": { - "startLine": 25, - "startColumn": 44 + "startLine": 22, + "startColumn": 50 } } } @@ -100,7 +100,7 @@ "ruleId": "SARIF2001", "ruleIndex": 0, "message": { - "id": "Warning_EnquoteDynamicContent", + "id": "Warning_IncludeDynamicContent", "arguments": [ "runs[0].tool.driver.rules[0].messageStrings.Warning_DontHavePlaceholders.text", "Placeholder text", diff --git a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF2001.AuthorHighQualityMessages_Valid.sarif b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF2001.AuthorHighQualityMessages_Valid.sarif index da2eff404..e333f7814 100644 --- a/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF2001.AuthorHighQualityMessages_Valid.sarif +++ b/src/Test.FunctionalTests.Sarif/TestData/Multitool/ValidateCommand/ExpectedOutputs/SARIF2001.AuthorHighQualityMessages_Valid.sarif @@ -5,7 +5,31 @@ { "tool": { "driver": { - "name": "SARIF Functional Testing" + "name": "SARIF Functional Testing", + "rules": [ + { + "id": "SARIF2001", + "name": "AuthorHighQualityMessages", + "shortDescription": { + "text": "Messages should consist of one or more complete sentences, ending with a period." + }, + "fullDescription": { + "text": "Messages should consist of one or more complete sentences, ending with a period." + }, + "messageStrings": { + "Warning_EnquoteDynamicContent": { + "text": "{0}: Placeholder '{1}' '{2}'" + }, + "Warning_IncludeDynamicContent": { + "text": "{0}: Placeholder '{1}' '{2}'" + }, + "Warning_TerminateWithPeriod": { + "text": "{0}: Placeholder '{1}' '{2}'" + } + }, + "helpUri": "http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html" + } + ] } }, "invocations": [ @@ -21,7 +45,33 @@ } } ], - "results": [], + "results": [ + { + "ruleId": "SARIF2001", + "ruleIndex": 0, + "message": { + "id": "Warning_EnquoteDynamicContent", + "arguments": [ + "runs[0].tool.driver.rules[0].messageStrings.Warning_TerminateWithPeriod.text", + "{0}: Placeholder '{1}'.", + "Warning_TerminateWithPeriod" + ] + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "index": 0 + }, + "region": { + "startLine": 22, + "startColumn": 51 + } + } + } + ] + } + ], "columnKind": "utf16CodeUnits" } ]