From f1b6ca53c9e7e1a7d28cded7ae4119f82d2a6083 Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Wed, 9 Jun 2021 15:00:31 -0400 Subject: [PATCH 01/15] Add Notice Command --- src/Runner.Common/ExtensionManager.cs | 1 + src/Runner.Worker/ActionCommandManager.cs | 10 ++++++++++ src/Runner.Worker/ExecutionContext.cs | 11 +++++++++++ src/Runner.Worker/Handlers/OutputManager.cs | 4 ++++ src/Sdk/DTWebApi/WebApi/IssueType.cs | 5 ++++- 5 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/Runner.Common/ExtensionManager.cs b/src/Runner.Common/ExtensionManager.cs index 09a094c1cfe..a432d6d3988 100644 --- a/src/Runner.Common/ExtensionManager.cs +++ b/src/Runner.Common/ExtensionManager.cs @@ -51,6 +51,7 @@ private List LoadExtensions() where T : class, IExtension Add(extensions, "GitHub.Runner.Worker.RemoveMatcherCommandExtension, Runner.Worker"); Add(extensions, "GitHub.Runner.Worker.WarningCommandExtension, Runner.Worker"); Add(extensions, "GitHub.Runner.Worker.ErrorCommandExtension, Runner.Worker"); + Add(extensions, "GitHub.Runner.Worker.NoticeCommandExtension, Runner.Worker"); Add(extensions, "GitHub.Runner.Worker.DebugCommandExtension, Runner.Worker"); Add(extensions, "GitHub.Runner.Worker.GroupCommandExtension, Runner.Worker"); Add(extensions, "GitHub.Runner.Worker.EndGroupCommandExtension, Runner.Worker"); diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index f10e159f303..dfa6bd78186 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -498,6 +498,13 @@ public sealed class ErrorCommandExtension : IssueCommandExtension public override string Command => "error"; } + public sealed class NoticeCommandExtension : IssueCommandExtension + { + public override IssueType Type => IssueType.Notice; + + public override string Command => "notice"; + } + public abstract class IssueCommandExtension : RunnerService, IActionCommandExtension { public abstract IssueType Type { get; } @@ -567,7 +574,10 @@ private static class IssueCommandProperties { public const String File = "file"; public const String Line = "line"; + public const String EndLine = "end_line"; public const String Column = "col"; + public const String EndColumn = "end_column"; + public const String Title = "title"; } } diff --git a/src/Runner.Worker/ExecutionContext.cs b/src/Runner.Worker/ExecutionContext.cs index f0890b8ee63..79aca8f117a 100644 --- a/src/Runner.Worker/ExecutionContext.cs +++ b/src/Runner.Worker/ExecutionContext.cs @@ -516,6 +516,17 @@ public void AddIssue(Issue issue, string logMessage = null) } _record.WarningCount++; + } else if (issue.Type == IssueType.Notice) { + + // tracking line number for each issue in log file + // log UI use this to navigate from issue to log + if (!string.IsNullOrEmpty(logMessage)) + { + long logLineNumber = Write(ConsoleColor.White, logMessage); + issue.Data["logFileLineNumber"] = logLineNumber.ToString(); + } + + _record.Issues.Add(issue); } _jobServerQueue.QueueTimelineRecordUpdate(_mainTimelineId, _record); diff --git a/src/Runner.Worker/Handlers/OutputManager.cs b/src/Runner.Worker/Handlers/OutputManager.cs index 855e6bd0252..03bbe789fbb 100644 --- a/src/Runner.Worker/Handlers/OutputManager.cs +++ b/src/Runner.Worker/Handlers/OutputManager.cs @@ -210,6 +210,10 @@ private DTWebApi.Issue ConvertToIssue(IssueMatch match) { issueType = DTWebApi.IssueType.Warning; } + else if (string.Equals(match.Severity, "notice", StringComparison.OrdinalIgnoreCase)) + { + issueType = DTWebApi.IssueType.Notice; + } else { _executionContext.Debug($"Skipped logging an issue for the matched line because the severity '{match.Severity}' is not supported."); diff --git a/src/Sdk/DTWebApi/WebApi/IssueType.cs b/src/Sdk/DTWebApi/WebApi/IssueType.cs index 8b8e52d1e88..603d0b59ca3 100644 --- a/src/Sdk/DTWebApi/WebApi/IssueType.cs +++ b/src/Sdk/DTWebApi/WebApi/IssueType.cs @@ -9,6 +9,9 @@ public enum IssueType Error = 1, [EnumMember] - Warning = 2 + Warning = 2, + + [EnumMember] + Notice = 3 } } From 350ac77b453a21cd6648265b6cefcf4d0edb6378 Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Fri, 18 Jun 2021 13:34:35 +0000 Subject: [PATCH 02/15] Add Feature Flag For Enhanced Annotations --- src/Runner.Common/JobServerQueue.cs | 5 +++++ src/Runner.Worker/ActionCommandManager.cs | 16 ++++++++++++++++ src/Runner.Worker/ExecutionContext.cs | 14 +++++++++++--- src/Sdk/DTWebApi/WebApi/TimelineRecord.cs | 8 ++++++++ 4 files changed, 40 insertions(+), 3 deletions(-) diff --git a/src/Runner.Common/JobServerQueue.cs b/src/Runner.Common/JobServerQueue.cs index 5cabca2a98e..785637e5753 100644 --- a/src/Runner.Common/JobServerQueue.cs +++ b/src/Runner.Common/JobServerQueue.cs @@ -544,6 +544,11 @@ private List MergeTimelineRecords(List timelineR timelineRecord.WarningCount = rec.WarningCount; } + if (rec.NoticeCount != null && rec.NoticeCount > 0) + { + timelineRecord.NoticeCount = rec.NoticeCount; + } + if (rec.Issues.Count > 0) { timelineRecord.Issues.Clear(); diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index dfa6bd78186..c957a942f05 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -75,6 +75,11 @@ public bool TryProcessCommand(IExecutionContext context, string input, Container return false; } + if (!ActionCommandManager.EnhancedAnnotationsEnabled(context) && actionCommand.Command == "notice") + { + return false; + } + // Serialize order lock (_commandSerializeLock) { @@ -141,6 +146,10 @@ public bool TryProcessCommand(IExecutionContext context, string input, Container return true; } + + internal static bool EnhancedAnnotationsEnabled(IExecutionContext context) { + return context.Global.Variables.GetBoolean("DistributedTask.EnhancedAnnotations") ?? false; + } } public interface IActionCommandExtension : IExtension @@ -567,6 +576,13 @@ public void ProcessCommand(IExecutionContext context, string inputLine, ActionCo } } + if (!ActionCommandManager.EnhancedAnnotationsEnabled(context)) + { + issue.Data.Remove(IssueCommandProperties.EndLine); + issue.Data.Remove(IssueCommandProperties.EndColumn); + issue.Data.Remove(IssueCommandProperties.Title); + } + context.AddIssue(issue); } diff --git a/src/Runner.Worker/ExecutionContext.cs b/src/Runner.Worker/ExecutionContext.cs index 79aca8f117a..0a8f107f433 100644 --- a/src/Runner.Worker/ExecutionContext.cs +++ b/src/Runner.Worker/ExecutionContext.cs @@ -106,6 +106,7 @@ public sealed class ExecutionContext : RunnerService, IExecutionContext { private const int _maxIssueCount = 10; private const int _throttlingDelayReportThreshold = 10 * 1000; // Don't report throttling with less than 10 seconds delay + private const string _noticeLogPrefix = "\u001b[31m"; //white text private readonly TimelineRecord _record = new TimelineRecord(); private readonly Dictionary _detailRecords = new Dictionary(); @@ -516,17 +517,23 @@ public void AddIssue(Issue issue, string logMessage = null) } _record.WarningCount++; - } else if (issue.Type == IssueType.Notice) { + } else if (issue.Type == IssueType.Notice) + { // tracking line number for each issue in log file // log UI use this to navigate from issue to log if (!string.IsNullOrEmpty(logMessage)) { - long logLineNumber = Write(ConsoleColor.White, logMessage); + long logLineNumber = Write(_noticeLogPrefix, logMessage); issue.Data["logFileLineNumber"] = logLineNumber.ToString(); } - _record.Issues.Add(issue); + if (_record.NoticeCount < _maxIssueCount) + { + _record.Issues.Add(issue); + } + + _record.NoticeCount++; } _jobServerQueue.QueueTimelineRecordUpdate(_mainTimelineId, _record); @@ -852,6 +859,7 @@ private void InitializeTimelineRecord(Guid timelineId, Guid timelineRecordId, Gu _record.State = TimelineRecordState.Pending; _record.ErrorCount = 0; _record.WarningCount = 0; + _record.NoticeCount = 0; if (parentTimelineRecordId != null && parentTimelineRecordId.Value != Guid.Empty) { diff --git a/src/Sdk/DTWebApi/WebApi/TimelineRecord.cs b/src/Sdk/DTWebApi/WebApi/TimelineRecord.cs index a14bded63b1..45043c8a192 100644 --- a/src/Sdk/DTWebApi/WebApi/TimelineRecord.cs +++ b/src/Sdk/DTWebApi/WebApi/TimelineRecord.cs @@ -38,6 +38,7 @@ private TimelineRecord(TimelineRecord recordToBeCloned) this.RefName = recordToBeCloned.RefName; this.ErrorCount = recordToBeCloned.ErrorCount; this.WarningCount = recordToBeCloned.WarningCount; + this.NoticeCount = recordToBeCloned.NoticeCount; this.AgentPlatform = recordToBeCloned.AgentPlatform; if (recordToBeCloned.Log != null) @@ -222,6 +223,13 @@ public Int32? WarningCount set; } + [DataMember(Order = 60)] + public Int32? NoticeCount + { + get; + set; + } + public List Issues { get From a4cb119d082534c02e9d6b55bd791f4e04f3c8cc Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Mon, 28 Jun 2021 15:22:52 +0000 Subject: [PATCH 03/15] Fix tests --- src/Test/L0/Worker/ActionCommandManagerL0.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Test/L0/Worker/ActionCommandManagerL0.cs b/src/Test/L0/Worker/ActionCommandManagerL0.cs index 3bea6eeb177..703a622fc2e 100644 --- a/src/Test/L0/Worker/ActionCommandManagerL0.cs +++ b/src/Test/L0/Worker/ActionCommandManagerL0.cs @@ -36,7 +36,7 @@ public void EnablePluginInternalCommand() { hc.GetTrace().Info($"{issue.Type} {issue.Message} {message ?? string.Empty}"); }); - + _commandManager.EnablePluginInternalCommand(); Assert.True(_commandManager.TryProcessCommand(_ec.Object, "##[internal-set-repo-path repoFullName=actions/runner;workspaceRepo=true]somepath", null)); @@ -175,7 +175,6 @@ public void EchoProcessCommandDebugOn() var ec = new Runner.Worker.ExecutionContext(); ec.Initialize(hc); ec.InitializeJob(jobRequest, System.Threading.CancellationToken.None); - ec.Complete(); Assert.True(ec.EchoOnActionCommand); @@ -285,6 +284,10 @@ private TestHostContext CreateTestContext([CallerMemberName] string testName = " _ec = new Mock(); _ec.SetupAllProperties(); _ec.Setup(x => x.Global).Returns(new GlobalContext()); + _ec.Object.Global.Variables = new Variables( + hostContext, + new Dictionary() + ); // Command manager _commandManager = new ActionCommandManager(); From b7b8dc0f67b3a23b0c5957b328d72ef58407cf84 Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Mon, 28 Jun 2021 21:01:21 +0000 Subject: [PATCH 04/15] Add validation for columns and lines --- src/Runner.Worker/ActionCommandManager.cs | 35 +++++++++++++++++--- src/Test/L0/Worker/ActionCommandManagerL0.cs | 25 ++++++++++++++ 2 files changed, 55 insertions(+), 5 deletions(-) diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index c957a942f05..54560e405b8 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -77,6 +77,7 @@ public bool TryProcessCommand(IExecutionContext context, string input, Container if (!ActionCommandManager.EnhancedAnnotationsEnabled(context) && actionCommand.Command == "notice") { + context.Debug($"Enhanced Annotations not enabled on the server: 'notice' command will not be processed."); return false; } @@ -528,6 +529,13 @@ public void ProcessCommand(IExecutionContext context, string inputLine, ActionCo command.Properties.TryGetValue(IssueCommandProperties.Line, out string line); command.Properties.TryGetValue(IssueCommandProperties.Column, out string column); + if (!ActionCommandManager.EnhancedAnnotationsEnabled(context)) + { + context.Debug("Enhanced Annotations not enabled on the server. The 'title', 'end_line', and 'end_column' fields are unsupported."); + } + + IssueCommandExtension.ValidateLinesAndColumns(command); + Issue issue = new Issue() { Category = "General", @@ -576,14 +584,31 @@ public void ProcessCommand(IExecutionContext context, string inputLine, ActionCo } } - if (!ActionCommandManager.EnhancedAnnotationsEnabled(context)) + context.AddIssue(issue); + } + + static void ValidateLinesAndColumns(ActionCommand command) + { + command.Properties.TryGetValue(IssueCommandProperties.Line, out string line); + command.Properties.TryGetValue(IssueCommandProperties.EndLine, out string endLine); + command.Properties.TryGetValue(IssueCommandProperties.Column, out string column); + command.Properties.TryGetValue(IssueCommandProperties.EndColumn, out string endColumn); + + var hasColumnValue = column != null || endColumn != null; + var hasLine = line != null; + var hasEndLine = endLine != null; + + Console.WriteLine($"hasColumnValue: {hasColumnValue}, hasLine: {hasLine}"); + + if (!hasLine && hasColumnValue) { - issue.Data.Remove(IssueCommandProperties.EndLine); - issue.Data.Remove(IssueCommandProperties.EndColumn); - issue.Data.Remove(IssueCommandProperties.Title); + throw new Exception($"Invalid {command.Command} command value. 'column' and 'end_column' can only be set if 'line' value is provided."); } - context.AddIssue(issue); + if (hasEndLine && line != endLine && hasColumnValue) + { + throw new Exception($"Invalid {command.Command} command value. 'column' and 'end_column' cannot be set if 'line' and 'end line' are different values."); + } } private static class IssueCommandProperties diff --git a/src/Test/L0/Worker/ActionCommandManagerL0.cs b/src/Test/L0/Worker/ActionCommandManagerL0.cs index 703a622fc2e..d8ab102ea95 100644 --- a/src/Test/L0/Worker/ActionCommandManagerL0.cs +++ b/src/Test/L0/Worker/ActionCommandManagerL0.cs @@ -187,6 +187,30 @@ public void EchoProcessCommandDebugOn() } } + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void IssueCommandInvalidColumns() + { + using (TestHostContext hc = CreateTestContext()) + { + _ec.Setup(x => x.Write(It.IsAny(), It.IsAny())) + .Returns((string tag, string line) => + { + hc.GetTrace().Info($"{tag} {line}"); + return 1; + }); + + // Different lines with columns + Assert.True(_commandManager.TryProcessCommand(_ec.Object, "::warning line=1,end_line=2,col=1,end_column=2::this is a warning", null)); + Assert.Equal(TaskResult.Failed, _ec.Object.CommandResult); + + // No lines with columns + Assert.True(_commandManager.TryProcessCommand(_ec.Object, "::warning col=1,end_column=2::this is a warning", null)); + Assert.Equal(TaskResult.Failed, _ec.Object.CommandResult); + } + } + [Fact] [Trait("Level", "L0")] [Trait("Category", "Worker")] @@ -267,6 +291,7 @@ private TestHostContext CreateTestContext([CallerMemberName] string testName = " new EchoCommandExtension(), new InternalPluginSetRepoPathCommandExtension(), new SetEnvCommandExtension(), + new WarningCommandExtension(), }; foreach (var command in commands) { From 99bacb79784002958dc1509f8d62948a36ac82b1 Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Tue, 29 Jun 2021 13:17:22 -0400 Subject: [PATCH 05/15] Fix order to match service --- src/Sdk/DTWebApi/WebApi/TimelineRecord.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Sdk/DTWebApi/WebApi/TimelineRecord.cs b/src/Sdk/DTWebApi/WebApi/TimelineRecord.cs index 45043c8a192..51317d1713b 100644 --- a/src/Sdk/DTWebApi/WebApi/TimelineRecord.cs +++ b/src/Sdk/DTWebApi/WebApi/TimelineRecord.cs @@ -223,7 +223,7 @@ public Int32? WarningCount set; } - [DataMember(Order = 60)] + [DataMember(Order = 55)] public Int32? NoticeCount { get; From 52cc77fa22677046f7c297a07604775ce9ebc58b Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Tue, 29 Jun 2021 13:18:50 -0400 Subject: [PATCH 06/15] Remove console.write --- src/Runner.Worker/ActionCommandManager.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index 54560e405b8..6d8c1d19353 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -598,8 +598,6 @@ static void ValidateLinesAndColumns(ActionCommand command) var hasLine = line != null; var hasEndLine = endLine != null; - Console.WriteLine($"hasColumnValue: {hasColumnValue}, hasLine: {hasLine}"); - if (!hasLine && hasColumnValue) { throw new Exception($"Invalid {command.Command} command value. 'column' and 'end_column' can only be set if 'line' value is provided."); From e33b238d2abf3f5f79387f6a8860857f77e26a87 Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Wed, 30 Jun 2021 10:01:09 -0400 Subject: [PATCH 07/15] Make Validation Better --- src/Runner.Worker/ActionCommandManager.cs | 31 +++++++++++++++----- src/Test/L0/Worker/ActionCommandManagerL0.cs | 26 ++++++++++++---- 2 files changed, 43 insertions(+), 14 deletions(-) diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index 6d8c1d19353..a26544d702b 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -529,13 +529,16 @@ public void ProcessCommand(IExecutionContext context, string inputLine, ActionCo command.Properties.TryGetValue(IssueCommandProperties.Line, out string line); command.Properties.TryGetValue(IssueCommandProperties.Column, out string column); - if (!ActionCommandManager.EnhancedAnnotationsEnabled(context)) + if (ActionCommandManager.EnhancedAnnotationsEnabled(context)) + { + IssueCommandExtension.ValidateLinesAndColumns(command); + } + else { context.Debug("Enhanced Annotations not enabled on the server. The 'title', 'end_line', and 'end_column' fields are unsupported."); } - IssueCommandExtension.ValidateLinesAndColumns(command); - + Issue issue = new Issue() { Category = "General", @@ -587,23 +590,35 @@ public void ProcessCommand(IExecutionContext context, string inputLine, ActionCo context.AddIssue(issue); } - static void ValidateLinesAndColumns(ActionCommand command) + public static void ValidateLinesAndColumns(ActionCommand command) { command.Properties.TryGetValue(IssueCommandProperties.Line, out string line); command.Properties.TryGetValue(IssueCommandProperties.EndLine, out string endLine); command.Properties.TryGetValue(IssueCommandProperties.Column, out string column); command.Properties.TryGetValue(IssueCommandProperties.EndColumn, out string endColumn); - var hasColumnValue = column != null || endColumn != null; - var hasLine = line != null; + var hasStartLine = line != null; var hasEndLine = endLine != null; + var hasStartColumn = column != null; + var hasEndColumn = endColumn != null; + var hasColumn = hasStartColumn || hasEndColumn; + + if (hasEndLine && !hasStartLine) + { + throw new Exception($"Invalid {command.Command} command value. 'end_line' can only be set of 'line' is provided"); + } + + if (hasEndColumn && !hasStartColumn) + { + throw new Exception($"Invalid {command.Command} command value. 'end_column' can only be set of 'col' is provided"); + } - if (!hasLine && hasColumnValue) + if (!hasStartLine && hasColumn) { throw new Exception($"Invalid {command.Command} command value. 'column' and 'end_column' can only be set if 'line' value is provided."); } - if (hasEndLine && line != endLine && hasColumnValue) + if (hasEndLine && line != endLine && hasColumn) { throw new Exception($"Invalid {command.Command} command value. 'column' and 'end_column' cannot be set if 'line' and 'end line' are different values."); } diff --git a/src/Test/L0/Worker/ActionCommandManagerL0.cs b/src/Test/L0/Worker/ActionCommandManagerL0.cs index d8ab102ea95..d2844c83bd5 100644 --- a/src/Test/L0/Worker/ActionCommandManagerL0.cs +++ b/src/Test/L0/Worker/ActionCommandManagerL0.cs @@ -36,7 +36,7 @@ public void EnablePluginInternalCommand() { hc.GetTrace().Info($"{issue.Type} {issue.Message} {message ?? string.Empty}"); }); - + _commandManager.EnablePluginInternalCommand(); Assert.True(_commandManager.TryProcessCommand(_ec.Object, "##[internal-set-repo-path repoFullName=actions/runner;workspaceRepo=true]somepath", null)); @@ -201,13 +201,27 @@ public void IssueCommandInvalidColumns() return 1; }); - // Different lines with columns - Assert.True(_commandManager.TryProcessCommand(_ec.Object, "::warning line=1,end_line=2,col=1,end_column=2::this is a warning", null)); - Assert.Equal(TaskResult.Failed, _ec.Object.CommandResult); + var registeredCommands = new HashSet(new string[1]{ "warning" }); + ActionCommand command; + + ActionCommand.TryParseV2("::warning line=1,end_line=2,col=1,end_column=2::this is a warning", registeredCommands, out command); + Assert.Throws(() => IssueCommandExtension.ValidateLinesAndColumns(command)); // No lines with columns - Assert.True(_commandManager.TryProcessCommand(_ec.Object, "::warning col=1,end_column=2::this is a warning", null)); - Assert.Equal(TaskResult.Failed, _ec.Object.CommandResult); + ActionCommand.TryParseV2("::warning col=1,end_column=2::this is a warning", registeredCommands, out command); + Assert.Throws(() => IssueCommandExtension.ValidateLinesAndColumns(command)); + + // No line with endLine + ActionCommand.TryParseV2("::warning end_line=1::this is a warning", registeredCommands, out command); + Assert.Throws(() => IssueCommandExtension.ValidateLinesAndColumns(command)); + + + // No column with end_column + ActionCommand.TryParseV2("::warning end_column=2::this is a warning", registeredCommands, out command); + Assert.Throws(() => IssueCommandExtension.ValidateLinesAndColumns(command)); + + // Valid + ActionCommand.TryParseV2("::warning line=1,end_line=1,col=1,end_column=2::this is a warning", registeredCommands, out command); } } From 4b57dd1fc0d91597d6c686feaa3ec3f25b2fed97 Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Wed, 30 Jun 2021 10:09:20 -0400 Subject: [PATCH 08/15] Cleanup --- src/Runner.Worker/ActionCommandManager.cs | 1 - src/Runner.Worker/ExecutionContext.cs | 3 ++- src/Test/L0/Worker/ActionCommandManagerL0.cs | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index a26544d702b..1af3a618554 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -537,7 +537,6 @@ public void ProcessCommand(IExecutionContext context, string inputLine, ActionCo { context.Debug("Enhanced Annotations not enabled on the server. The 'title', 'end_line', and 'end_column' fields are unsupported."); } - Issue issue = new Issue() { diff --git a/src/Runner.Worker/ExecutionContext.cs b/src/Runner.Worker/ExecutionContext.cs index 0a8f107f433..25a6420615d 100644 --- a/src/Runner.Worker/ExecutionContext.cs +++ b/src/Runner.Worker/ExecutionContext.cs @@ -517,7 +517,8 @@ public void AddIssue(Issue issue, string logMessage = null) } _record.WarningCount++; - } else if (issue.Type == IssueType.Notice) + } + else if (issue.Type == IssueType.Notice) { // tracking line number for each issue in log file diff --git a/src/Test/L0/Worker/ActionCommandManagerL0.cs b/src/Test/L0/Worker/ActionCommandManagerL0.cs index d2844c83bd5..8926cf34c44 100644 --- a/src/Test/L0/Worker/ActionCommandManagerL0.cs +++ b/src/Test/L0/Worker/ActionCommandManagerL0.cs @@ -175,6 +175,7 @@ public void EchoProcessCommandDebugOn() var ec = new Runner.Worker.ExecutionContext(); ec.Initialize(hc); ec.InitializeJob(jobRequest, System.Threading.CancellationToken.None); + ec.Complete(); Assert.True(ec.EchoOnActionCommand); From db13a228374039d44748a4e606e27ac09687391e Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Wed, 30 Jun 2021 11:44:49 -0400 Subject: [PATCH 09/15] Handle empty/whitespace strings --- src/Runner.Worker/ActionCommandManager.cs | 8 ++++---- src/Test/L0/Worker/ActionCommandManagerL0.cs | 5 ++++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index 1af3a618554..74fc1de1ab9 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -596,10 +596,10 @@ public static void ValidateLinesAndColumns(ActionCommand command) command.Properties.TryGetValue(IssueCommandProperties.Column, out string column); command.Properties.TryGetValue(IssueCommandProperties.EndColumn, out string endColumn); - var hasStartLine = line != null; - var hasEndLine = endLine != null; - var hasStartColumn = column != null; - var hasEndColumn = endColumn != null; + var hasStartLine = !String.IsNullOrWhiteSpace(line); + var hasEndLine = !String.IsNullOrWhiteSpace(endLine); + var hasStartColumn = !String.IsNullOrWhiteSpace(column); + var hasEndColumn = !String.IsNullOrWhiteSpace(endColumn); var hasColumn = hasStartColumn || hasEndColumn; if (hasEndLine && !hasStartLine) diff --git a/src/Test/L0/Worker/ActionCommandManagerL0.cs b/src/Test/L0/Worker/ActionCommandManagerL0.cs index 8926cf34c44..3ba4c645d44 100644 --- a/src/Test/L0/Worker/ActionCommandManagerL0.cs +++ b/src/Test/L0/Worker/ActionCommandManagerL0.cs @@ -216,11 +216,14 @@ public void IssueCommandInvalidColumns() ActionCommand.TryParseV2("::warning end_line=1::this is a warning", registeredCommands, out command); Assert.Throws(() => IssueCommandExtension.ValidateLinesAndColumns(command)); - // No column with end_column ActionCommand.TryParseV2("::warning end_column=2::this is a warning", registeredCommands, out command); Assert.Throws(() => IssueCommandExtension.ValidateLinesAndColumns(command)); + // Empty Strings + ActionCommand.TryParseV2("::warning line=,end_line=3::this is a warning", registeredCommands, out command); + Assert.Throws(() => IssueCommandExtension.ValidateLinesAndColumns(command)); + // Valid ActionCommand.TryParseV2("::warning line=1,end_line=1,col=1,end_column=2::this is a warning", registeredCommands, out command); } From 38b06507e75afcbaf8f9a581708c8d709bcb982a Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Wed, 30 Jun 2021 16:04:53 -0400 Subject: [PATCH 10/15] Add more validation for line/column ranges --- src/Runner.Worker/ActionCommandManager.cs | 22 ++++++++++++++------ src/Test/L0/Worker/ActionCommandManagerL0.cs | 8 +++++++ 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index 74fc1de1ab9..d22340c9117 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -596,20 +596,20 @@ public static void ValidateLinesAndColumns(ActionCommand command) command.Properties.TryGetValue(IssueCommandProperties.Column, out string column); command.Properties.TryGetValue(IssueCommandProperties.EndColumn, out string endColumn); - var hasStartLine = !String.IsNullOrWhiteSpace(line); - var hasEndLine = !String.IsNullOrWhiteSpace(endLine); - var hasStartColumn = !String.IsNullOrWhiteSpace(column); - var hasEndColumn = !String.IsNullOrWhiteSpace(endColumn); + var hasStartLine = int.TryParse(line, out int lineNumber); + var hasEndLine = int.TryParse(endLine, out int endLineNumber); + var hasStartColumn = int.TryParse(column, out int columnNumber); + var hasEndColumn = int.TryParse(endColumn, out int endColumnNumber); var hasColumn = hasStartColumn || hasEndColumn; if (hasEndLine && !hasStartLine) { - throw new Exception($"Invalid {command.Command} command value. 'end_line' can only be set of 'line' is provided"); + throw new Exception($"Invalid {command.Command} command value. 'end_line' can only be set if 'line' is provided"); } if (hasEndColumn && !hasStartColumn) { - throw new Exception($"Invalid {command.Command} command value. 'end_column' can only be set of 'col' is provided"); + throw new Exception($"Invalid {command.Command} command value. 'end_column' can only be set if 'col' is provided"); } if (!hasStartLine && hasColumn) @@ -621,6 +621,16 @@ public static void ValidateLinesAndColumns(ActionCommand command) { throw new Exception($"Invalid {command.Command} command value. 'column' and 'end_column' cannot be set if 'line' and 'end line' are different values."); } + + if (hasStartLine && hasEndLine && endLineNumber < lineNumber) + { + throw new Exception($"Invalid {command.Command} command value. 'end_line' cannot be less than 'line'."); + } + + if (hasStartColumn && hasEndColumn && endColumnNumber < columnNumber) + { + throw new Exception($"Invalid {command.Command} command value. 'end_column' cannot be less than 'col'."); + } } private static class IssueCommandProperties diff --git a/src/Test/L0/Worker/ActionCommandManagerL0.cs b/src/Test/L0/Worker/ActionCommandManagerL0.cs index 3ba4c645d44..313c3958cda 100644 --- a/src/Test/L0/Worker/ActionCommandManagerL0.cs +++ b/src/Test/L0/Worker/ActionCommandManagerL0.cs @@ -224,6 +224,14 @@ public void IssueCommandInvalidColumns() ActionCommand.TryParseV2("::warning line=,end_line=3::this is a warning", registeredCommands, out command); Assert.Throws(() => IssueCommandExtension.ValidateLinesAndColumns(command)); + // Nonsensical line values + ActionCommand.TryParseV2("::warning line=4,end_line=3::this is a warning", registeredCommands, out command); + Assert.Throws(() => IssueCommandExtension.ValidateLinesAndColumns(command)); + + /// Nonsensical column values + ActionCommand.TryParseV2("::warning line=1,end_line=1,col=3,end_column=2::this is a warning", registeredCommands, out command); + Assert.Throws(() => IssueCommandExtension.ValidateLinesAndColumns(command)); + // Valid ActionCommand.TryParseV2("::warning line=1,end_line=1,col=1,end_column=2::this is a warning", registeredCommands, out command); } From aa1f00f86968ccc121d51c6a1009c5a7ba75da5d Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Fri, 2 Jul 2021 12:31:02 -0400 Subject: [PATCH 11/15] Make Validation Debug, Not Throw --- src/Runner.Worker/ActionCommandManager.cs | 31 ++++++++++++++------ src/Test/L0/Worker/ActionCommandManagerL0.cs | 16 +++++----- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index d22340c9117..b766bfd59f2 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -531,7 +531,12 @@ public void ProcessCommand(IExecutionContext context, string inputLine, ActionCo if (ActionCommandManager.EnhancedAnnotationsEnabled(context)) { - IssueCommandExtension.ValidateLinesAndColumns(command); + + if (!IssueCommandExtension.ValidateLinesAndColumns(command, context)) + { + context.Debug($"Validation failed for the {command.Command}. It will not be processed."); + return; + } } else { @@ -589,7 +594,7 @@ public void ProcessCommand(IExecutionContext context, string inputLine, ActionCo context.AddIssue(issue); } - public static void ValidateLinesAndColumns(ActionCommand command) + public static bool ValidateLinesAndColumns(ActionCommand command, IExecutionContext context) { command.Properties.TryGetValue(IssueCommandProperties.Line, out string line); command.Properties.TryGetValue(IssueCommandProperties.EndLine, out string endLine); @@ -604,33 +609,41 @@ public static void ValidateLinesAndColumns(ActionCommand command) if (hasEndLine && !hasStartLine) { - throw new Exception($"Invalid {command.Command} command value. 'end_line' can only be set if 'line' is provided"); + context.Debug($"Invalid {command.Command} command value. 'end_line' can only be set if 'line' is provided"); + return false; } if (hasEndColumn && !hasStartColumn) { - throw new Exception($"Invalid {command.Command} command value. 'end_column' can only be set if 'col' is provided"); + context.Debug($"Invalid {command.Command} command value. 'end_column' can only be set if 'col' is provided"); + return false; } if (!hasStartLine && hasColumn) { - throw new Exception($"Invalid {command.Command} command value. 'column' and 'end_column' can only be set if 'line' value is provided."); + context.Debug($"Invalid {command.Command} command value. 'column' and 'end_column' can only be set if 'line' value is provided."); + return false; } if (hasEndLine && line != endLine && hasColumn) { - throw new Exception($"Invalid {command.Command} command value. 'column' and 'end_column' cannot be set if 'line' and 'end line' are different values."); + context.Debug($"Invalid {command.Command} command value. 'column' and 'end_column' cannot be set if 'line' and 'end line' are different values."); + return false; } if (hasStartLine && hasEndLine && endLineNumber < lineNumber) { - throw new Exception($"Invalid {command.Command} command value. 'end_line' cannot be less than 'line'."); + context.Debug($"Invalid {command.Command} command value. 'end_line' cannot be less than 'line'."); + return false; } if (hasStartColumn && hasEndColumn && endColumnNumber < columnNumber) { - throw new Exception($"Invalid {command.Command} command value. 'end_column' cannot be less than 'col'."); - } + context.Debug($"Invalid {command.Command} command value. 'end_column' cannot be less than 'col'."); + return false; + } + + return true; } private static class IssueCommandProperties diff --git a/src/Test/L0/Worker/ActionCommandManagerL0.cs b/src/Test/L0/Worker/ActionCommandManagerL0.cs index 313c3958cda..33456ef625e 100644 --- a/src/Test/L0/Worker/ActionCommandManagerL0.cs +++ b/src/Test/L0/Worker/ActionCommandManagerL0.cs @@ -206,34 +206,36 @@ public void IssueCommandInvalidColumns() ActionCommand command; ActionCommand.TryParseV2("::warning line=1,end_line=2,col=1,end_column=2::this is a warning", registeredCommands, out command); - Assert.Throws(() => IssueCommandExtension.ValidateLinesAndColumns(command)); + Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); // No lines with columns ActionCommand.TryParseV2("::warning col=1,end_column=2::this is a warning", registeredCommands, out command); - Assert.Throws(() => IssueCommandExtension.ValidateLinesAndColumns(command)); + Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); // No line with endLine ActionCommand.TryParseV2("::warning end_line=1::this is a warning", registeredCommands, out command); - Assert.Throws(() => IssueCommandExtension.ValidateLinesAndColumns(command)); + Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); // No column with end_column ActionCommand.TryParseV2("::warning end_column=2::this is a warning", registeredCommands, out command); - Assert.Throws(() => IssueCommandExtension.ValidateLinesAndColumns(command)); + Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); // Empty Strings ActionCommand.TryParseV2("::warning line=,end_line=3::this is a warning", registeredCommands, out command); - Assert.Throws(() => IssueCommandExtension.ValidateLinesAndColumns(command)); + Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); // Nonsensical line values ActionCommand.TryParseV2("::warning line=4,end_line=3::this is a warning", registeredCommands, out command); - Assert.Throws(() => IssueCommandExtension.ValidateLinesAndColumns(command)); + Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); /// Nonsensical column values ActionCommand.TryParseV2("::warning line=1,end_line=1,col=3,end_column=2::this is a warning", registeredCommands, out command); - Assert.Throws(() => IssueCommandExtension.ValidateLinesAndColumns(command)); + Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); // Valid ActionCommand.TryParseV2("::warning line=1,end_line=1,col=1,end_column=2::this is a warning", registeredCommands, out command); + Assert.True(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); + } } From 76bc6b79056162ee0719ac25d894c0706528dfe8 Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Fri, 2 Jul 2021 12:41:54 -0400 Subject: [PATCH 12/15] Change casing to :camel: from :snake: --- src/Runner.Worker/ActionCommandManager.cs | 16 +++++++------- src/Test/L0/Worker/ActionCommandManagerL0.cs | 22 ++++++++++++-------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index b766bfd59f2..001a14a0987 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -609,37 +609,37 @@ public static bool ValidateLinesAndColumns(ActionCommand command, IExecutionCont if (hasEndLine && !hasStartLine) { - context.Debug($"Invalid {command.Command} command value. 'end_line' can only be set if 'line' is provided"); + context.Debug($"Invalid {command.Command} command value. '{IssueCommandProperties.EndLine}' can only be set if '{IssueCommandProperties.Line}' is provided"); return false; } if (hasEndColumn && !hasStartColumn) { - context.Debug($"Invalid {command.Command} command value. 'end_column' can only be set if 'col' is provided"); + context.Debug($"Invalid {command.Command} command value. '{IssueCommandProperties.EndColumn}' can only be set if '{IssueCommandProperties.Column}' is provided"); return false; } if (!hasStartLine && hasColumn) { - context.Debug($"Invalid {command.Command} command value. 'column' and 'end_column' can only be set if 'line' value is provided."); + context.Debug($"Invalid {command.Command} command value. '{IssueCommandProperties.Column}' and '{IssueCommandProperties.EndColumn}' can only be set if '{IssueCommandProperties.Line}' value is provided."); return false; } if (hasEndLine && line != endLine && hasColumn) { - context.Debug($"Invalid {command.Command} command value. 'column' and 'end_column' cannot be set if 'line' and 'end line' are different values."); + context.Debug($"Invalid {command.Command} command value. '{IssueCommandProperties.Column}' and '{IssueCommandProperties.EndColumn}' cannot be set if '{IssueCommandProperties.Line}' and '{IssueCommandProperties.EndLine}' are different values."); return false; } if (hasStartLine && hasEndLine && endLineNumber < lineNumber) { - context.Debug($"Invalid {command.Command} command value. 'end_line' cannot be less than 'line'."); + context.Debug($"Invalid {command.Command} command value. '{IssueCommandProperties.EndLine}' cannot be less than '{IssueCommandProperties.Line}'."); return false; } if (hasStartColumn && hasEndColumn && endColumnNumber < columnNumber) { - context.Debug($"Invalid {command.Command} command value. 'end_column' cannot be less than 'col'."); + context.Debug($"Invalid {command.Command} command value. '{IssueCommandProperties.EndColumn}' cannot be less than '{IssueCommandProperties.Column}'."); return false; } @@ -650,9 +650,9 @@ private static class IssueCommandProperties { public const String File = "file"; public const String Line = "line"; - public const String EndLine = "end_line"; + public const String EndLine = "endLine"; public const String Column = "col"; - public const String EndColumn = "end_column"; + public const String EndColumn = "endColumn"; public const String Title = "title"; } diff --git a/src/Test/L0/Worker/ActionCommandManagerL0.cs b/src/Test/L0/Worker/ActionCommandManagerL0.cs index 33456ef625e..f987ef3d745 100644 --- a/src/Test/L0/Worker/ActionCommandManagerL0.cs +++ b/src/Test/L0/Worker/ActionCommandManagerL0.cs @@ -205,37 +205,41 @@ public void IssueCommandInvalidColumns() var registeredCommands = new HashSet(new string[1]{ "warning" }); ActionCommand command; - ActionCommand.TryParseV2("::warning line=1,end_line=2,col=1,end_column=2::this is a warning", registeredCommands, out command); + // Columns when lines are different + ActionCommand.TryParseV2("::warning line=1,endLine=2,col=1,endColumn=2::this is a warning", registeredCommands, out command); Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); // No lines with columns - ActionCommand.TryParseV2("::warning col=1,end_column=2::this is a warning", registeredCommands, out command); + ActionCommand.TryParseV2("::warning col=1,endColumn=2::this is a warning", registeredCommands, out command); Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); // No line with endLine - ActionCommand.TryParseV2("::warning end_line=1::this is a warning", registeredCommands, out command); + ActionCommand.TryParseV2("::warning endLine=1::this is a warning", registeredCommands, out command); Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); - // No column with end_column - ActionCommand.TryParseV2("::warning end_column=2::this is a warning", registeredCommands, out command); + // No column with endColumn + ActionCommand.TryParseV2("::warning endColumn=2::this is a warning", registeredCommands, out command); Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); // Empty Strings - ActionCommand.TryParseV2("::warning line=,end_line=3::this is a warning", registeredCommands, out command); + ActionCommand.TryParseV2("::warning line=,endLine=3::this is a warning", registeredCommands, out command); Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); // Nonsensical line values - ActionCommand.TryParseV2("::warning line=4,end_line=3::this is a warning", registeredCommands, out command); + ActionCommand.TryParseV2("::warning line=4,endLine=3::this is a warning", registeredCommands, out command); Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); /// Nonsensical column values - ActionCommand.TryParseV2("::warning line=1,end_line=1,col=3,end_column=2::this is a warning", registeredCommands, out command); + ActionCommand.TryParseV2("::warning line=1,endLine=1,col=3,endColumn=2::this is a warning", registeredCommands, out command); Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); // Valid - ActionCommand.TryParseV2("::warning line=1,end_line=1,col=1,end_column=2::this is a warning", registeredCommands, out command); + ActionCommand.TryParseV2("::warning line=1,endLine=1,col=1,endColumn=2::this is a warning", registeredCommands, out command); Assert.True(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); + // Backwards compatibility + ActionCommand.TryParseV2("::warning line=1,col=1,file=test.txt::this is a warning", registeredCommands, out command); + Assert.True(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); } } From 81bb5adf87219685abc8a8b20f1733492815e02a Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Fri, 2 Jul 2021 12:47:32 -0400 Subject: [PATCH 13/15] Give notice a well known tag --- src/Runner.Worker/ExecutionContext.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Runner.Worker/ExecutionContext.cs b/src/Runner.Worker/ExecutionContext.cs index 25a6420615d..748e6b3387a 100644 --- a/src/Runner.Worker/ExecutionContext.cs +++ b/src/Runner.Worker/ExecutionContext.cs @@ -106,7 +106,6 @@ public sealed class ExecutionContext : RunnerService, IExecutionContext { private const int _maxIssueCount = 10; private const int _throttlingDelayReportThreshold = 10 * 1000; // Don't report throttling with less than 10 seconds delay - private const string _noticeLogPrefix = "\u001b[31m"; //white text private readonly TimelineRecord _record = new TimelineRecord(); private readonly Dictionary _detailRecords = new Dictionary(); @@ -525,7 +524,7 @@ public void AddIssue(Issue issue, string logMessage = null) // log UI use this to navigate from issue to log if (!string.IsNullOrEmpty(logMessage)) { - long logLineNumber = Write(_noticeLogPrefix, logMessage); + long logLineNumber = Write(WellKnownTags.Notice, logMessage); issue.Data["logFileLineNumber"] = logLineNumber.ToString(); } @@ -1032,6 +1031,7 @@ public static class WellKnownTags public static readonly string Command = "##[command]"; public static readonly string Error = "##[error]"; public static readonly string Warning = "##[warning]"; + public static readonly string Notice = "##[notice]"; public static readonly string Debug = "##[debug]"; } } From 8ba5e0e068ea21521f5c5f25cbc3bd522d4271ae Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Fri, 2 Jul 2021 13:05:59 -0400 Subject: [PATCH 14/15] Cleanup --- src/Runner.Worker/ActionCommandManager.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index 001a14a0987..9e0ba8014f3 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -655,7 +655,6 @@ private static class IssueCommandProperties public const String EndColumn = "endColumn"; public const String Title = "title"; } - } public sealed class GroupCommandExtension : GroupingCommandExtension From e2ec4863e168988c631983eb31605e32eb0775f6 Mon Sep 17 00:00:00 2001 From: Luke Tomlinson Date: Mon, 12 Jul 2021 10:00:35 -0400 Subject: [PATCH 15/15] Sanitize invalid commands rather than fail --- src/Runner.Worker/ActionCommandManager.cs | 35 ++++++++-------- src/Test/L0/Worker/ActionCommandManagerL0.cs | 43 +++++++++++++++----- 2 files changed, 49 insertions(+), 29 deletions(-) diff --git a/src/Runner.Worker/ActionCommandManager.cs b/src/Runner.Worker/ActionCommandManager.cs index 9e0ba8014f3..75588aca363 100644 --- a/src/Runner.Worker/ActionCommandManager.cs +++ b/src/Runner.Worker/ActionCommandManager.cs @@ -529,16 +529,7 @@ public void ProcessCommand(IExecutionContext context, string inputLine, ActionCo command.Properties.TryGetValue(IssueCommandProperties.Line, out string line); command.Properties.TryGetValue(IssueCommandProperties.Column, out string column); - if (ActionCommandManager.EnhancedAnnotationsEnabled(context)) - { - - if (!IssueCommandExtension.ValidateLinesAndColumns(command, context)) - { - context.Debug($"Validation failed for the {command.Command}. It will not be processed."); - return; - } - } - else + if (!ActionCommandManager.EnhancedAnnotationsEnabled(context)) { context.Debug("Enhanced Annotations not enabled on the server. The 'title', 'end_line', and 'end_column' fields are unsupported."); } @@ -594,7 +585,7 @@ public void ProcessCommand(IExecutionContext context, string inputLine, ActionCo context.AddIssue(issue); } - public static bool ValidateLinesAndColumns(ActionCommand command, IExecutionContext context) + public static void ValidateLinesAndColumns(ActionCommand command, IExecutionContext context) { command.Properties.TryGetValue(IssueCommandProperties.Line, out string line); command.Properties.TryGetValue(IssueCommandProperties.EndLine, out string endLine); @@ -610,40 +601,46 @@ public static bool ValidateLinesAndColumns(ActionCommand command, IExecutionCont if (hasEndLine && !hasStartLine) { context.Debug($"Invalid {command.Command} command value. '{IssueCommandProperties.EndLine}' can only be set if '{IssueCommandProperties.Line}' is provided"); - return false; + command.Properties[IssueCommandProperties.Line] = endLine; + hasStartLine = true; + line = endLine; } if (hasEndColumn && !hasStartColumn) { context.Debug($"Invalid {command.Command} command value. '{IssueCommandProperties.EndColumn}' can only be set if '{IssueCommandProperties.Column}' is provided"); - return false; + command.Properties[IssueCommandProperties.Column] = endColumn; + hasStartColumn = true; + column = endColumn; } if (!hasStartLine && hasColumn) { context.Debug($"Invalid {command.Command} command value. '{IssueCommandProperties.Column}' and '{IssueCommandProperties.EndColumn}' can only be set if '{IssueCommandProperties.Line}' value is provided."); - return false; + command.Properties.Remove(IssueCommandProperties.Column); + command.Properties.Remove(IssueCommandProperties.EndColumn); } if (hasEndLine && line != endLine && hasColumn) { context.Debug($"Invalid {command.Command} command value. '{IssueCommandProperties.Column}' and '{IssueCommandProperties.EndColumn}' cannot be set if '{IssueCommandProperties.Line}' and '{IssueCommandProperties.EndLine}' are different values."); - return false; + command.Properties.Remove(IssueCommandProperties.Column); + command.Properties.Remove(IssueCommandProperties.EndColumn); } if (hasStartLine && hasEndLine && endLineNumber < lineNumber) { context.Debug($"Invalid {command.Command} command value. '{IssueCommandProperties.EndLine}' cannot be less than '{IssueCommandProperties.Line}'."); - return false; + command.Properties.Remove(IssueCommandProperties.Line); + command.Properties.Remove(IssueCommandProperties.EndLine); } if (hasStartColumn && hasEndColumn && endColumnNumber < columnNumber) { context.Debug($"Invalid {command.Command} command value. '{IssueCommandProperties.EndColumn}' cannot be less than '{IssueCommandProperties.Column}'."); - return false; + command.Properties.Remove(IssueCommandProperties.Column); + command.Properties.Remove(IssueCommandProperties.EndColumn); } - - return true; } private static class IssueCommandProperties diff --git a/src/Test/L0/Worker/ActionCommandManagerL0.cs b/src/Test/L0/Worker/ActionCommandManagerL0.cs index f987ef3d745..f9080dbfcc2 100644 --- a/src/Test/L0/Worker/ActionCommandManagerL0.cs +++ b/src/Test/L0/Worker/ActionCommandManagerL0.cs @@ -207,39 +207,62 @@ public void IssueCommandInvalidColumns() // Columns when lines are different ActionCommand.TryParseV2("::warning line=1,endLine=2,col=1,endColumn=2::this is a warning", registeredCommands, out command); - Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); + Assert.Equal("1", command.Properties["col"]); + IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object); + Assert.False(command.Properties.ContainsKey("col")); // No lines with columns ActionCommand.TryParseV2("::warning col=1,endColumn=2::this is a warning", registeredCommands, out command); - Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); + Assert.Equal("1", command.Properties["col"]); + Assert.Equal("2", command.Properties["endColumn"]); + IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object); + Assert.False(command.Properties.ContainsKey("col")); + Assert.False(command.Properties.ContainsKey("endColumn")); // No line with endLine ActionCommand.TryParseV2("::warning endLine=1::this is a warning", registeredCommands, out command); - Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); + Assert.Equal("1", command.Properties["endLine"]); + IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object); + Assert.Equal(command.Properties["endLine"], command.Properties["line"]); // No column with endColumn - ActionCommand.TryParseV2("::warning endColumn=2::this is a warning", registeredCommands, out command); - Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); + ActionCommand.TryParseV2("::warning line=1,endColumn=2::this is a warning", registeredCommands, out command); + Assert.Equal("2", command.Properties["endColumn"]); + IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object); + Assert.Equal(command.Properties["endColumn"], command.Properties["col"]); // Empty Strings ActionCommand.TryParseV2("::warning line=,endLine=3::this is a warning", registeredCommands, out command); - Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); + IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object); + Assert.Equal(command.Properties["line"], command.Properties["endLine"]); // Nonsensical line values ActionCommand.TryParseV2("::warning line=4,endLine=3::this is a warning", registeredCommands, out command); - Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); + IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object); + Assert.False(command.Properties.ContainsKey("line")); + Assert.False(command.Properties.ContainsKey("endLine")); /// Nonsensical column values ActionCommand.TryParseV2("::warning line=1,endLine=1,col=3,endColumn=2::this is a warning", registeredCommands, out command); - Assert.False(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); + IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object); + Assert.False(command.Properties.ContainsKey("col")); + Assert.False(command.Properties.ContainsKey("endColumn")); // Valid ActionCommand.TryParseV2("::warning line=1,endLine=1,col=1,endColumn=2::this is a warning", registeredCommands, out command); - Assert.True(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); + IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object); + Assert.Equal("1", command.Properties["line"]); + Assert.Equal("1", command.Properties["endLine"]); + Assert.Equal("1", command.Properties["col"]); + Assert.Equal("2", command.Properties["endColumn"]); // Backwards compatibility ActionCommand.TryParseV2("::warning line=1,col=1,file=test.txt::this is a warning", registeredCommands, out command); - Assert.True(IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object)); + IssueCommandExtension.ValidateLinesAndColumns(command, _ec.Object); + Assert.Equal("1", command.Properties["line"]); + Assert.False(command.Properties.ContainsKey("endLine")); + Assert.Equal("1", command.Properties["col"]); + Assert.False(command.Properties.ContainsKey("endColumn")); } }