Fix #1629: Page command shouldn't throw on options error.

src/ReleaseHistory.md

@@ -452,4 +452,5 @@
 * BUGFIX: Multitool merge command produced invalid SARIF if there were 0 input files. https://github.com/microsoft/sarif-sdk/issues/1592
 * BUGFIX: FortifyFpr converter produced invalid SARIF. https://github.com/microsoft/sarif-sdk/issues/1593
 * BUGFIX: FxCop converter produced empty `result.message` objects. https://github.com/microsoft/sarif-sdk/issues/1594
-* FEATURE: Add validation rule to ensure correctness of `originalUriBaseIds` entries.
+* FEATURE: Add validation rule to ensure correctness of `originalUriBaseIds` entries. https://github.com/microsoft/sarif-sdk/issues/1485
+* FEATURE: Improve presentation of option validation messages from the Multitool `page` command. https://github.com/microsoft/sarif-sdk/issues/1629

src/Sarif.Multitool/CommandBase.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Globalization;
 using System.IO;
 using Microsoft.CodeAnalysis.Sarif.Driver;
 using Newtonsoft.Json;
@@ -12,6 +13,24 @@ namespace Microsoft.CodeAnalysis.Sarif.Multitool
 {
     public abstract class CommandBase
     {
+        protected static bool ValidateNonNegativeCommandLineOption<T>(int optionValue, string optionName)
+        {
+            bool valid = true;
+
+            if (optionValue < 0)
+            {
+                string optionDescription = CommandUtilities.GetOptionDescription<T>(optionName);
+                Console.Error.WriteLine(
+                    string.Format(
+                        CultureInfo.CurrentCulture,
+                        MultitoolResources.OptionValueMustBeNonNegative,
+                        optionDescription));
+                valid = false;
+            }
+
+            return valid;
+        }
+
         public static T ReadSarifFile<T>(IFileSystem fileSystem, string filePath, IContractResolver contractResolver = null)
         {
             var serializer = new JsonSerializer() { ContractResolver = contractResolver };

src/Sarif.Multitool/MultitoolResources.resx

@@ -135,6 +135,12 @@
   <data name="ErrorOutputFilePathAndInline" xml:space="preserve">
     <value>Exactly one of the '{0}' and '{1}' options must be present.</value>
   </data>
+  <data name="InputFileNotFound" xml:space="preserve">
+    <value>Input file '{0}' was not found.</value>
+  </data>
+  <data name="OptionValueMustBeNonNegative" xml:space="preserve">
+    <value>The value of the '{0}' option must be greater than or equal to zero.</value>
+  </data>
   <data name="ResultDifferenceSummary" xml:space="preserve">
     <value>{0:n0} identical, {1:n0} changed</value>
   </data>

src/Sarif.Multitool/PageCommand.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
+using System.Globalization;
 using System.IO;
 using System.Linq;
 using Microsoft.CodeAnalysis.Sarif.Driver;
@@ -40,22 +41,39 @@ public int Run(PageOptions options)
 
         public int RunWithoutCatch(PageOptions options)
         {
-            // TODO: Report these to the console instead of throwing.
-            if (options.RunIndex < 0) { throw new ArgumentOutOfRangeException("runIndex"); }
-            if (options.Index < 0) { throw new ArgumentOutOfRangeException("index"); }
-            if (options.Count < 0) { throw new ArgumentOutOfRangeException("count"); }
-            if (!_fileSystem.FileExists(options.InputFilePath)) { throw new FileNotFoundException($"Input file \"{options.InputFilePath}\" not found."); }
-
-            bool valid = DriverUtilities.ReportWhetherOutputFileCanBeCreated(options.OutputFilePath, options.Force, _fileSystem);
-            if (!valid) { return 1; }
+            if (!ValidateOptions(options, _fileSystem)) { return 1; }
 
             // Load the JsonMap, if previously built and up-to-date, or rebuild it
             JsonMapNode root = LoadOrRebuildMap(options);
 
             // Write the desired page from the Sarif file
             ExtractPage(options, root);
 
-            return 1;
+            return 0;
+        }
+
+        internal bool ValidateOptions(PageOptions options, IFileSystem fileSystem)
+        {
+            bool valid = true;
+
+            valid &= ValidateNonNegativeCommandLineOption<PageOptions>(options.RunIndex, nameof(options.RunIndex));
+            valid &= ValidateNonNegativeCommandLineOption<PageOptions>(options.Index, nameof(options.Index));
+            valid &= ValidateNonNegativeCommandLineOption<PageOptions>(options.Count, nameof(options.Count));
+
+
+            if (!fileSystem.FileExists(options.InputFilePath))
+            {
+                Console.Error.WriteLine(
+                    string.Format(
+                        CultureInfo.CurrentCulture,
+                        MultitoolResources.InputFileNotFound,
+                        options.InputFilePath));
+                valid = false;
+            }
+
+            valid &= DriverUtilities.ReportWhetherOutputFileCanBeCreated(options.OutputFilePath, options.Force, fileSystem);
+
+            return valid;
         }
 
         internal SarifLog PageViaOm(PageOptions options)

src/Sarif.Multitool/PageOptions.cs

@@ -50,7 +50,7 @@ internal class PageOptions
             "force",
             Default = true,
             HelpText = "Force overwrite of output file if it exists.")]
-        public bool Force { get; set; } = true;
+        public bool Force { get; set; }
 
         [Option(
             'o',

src/Test.UnitTests.Sarif.Multitool/PageCommandTests.cs

@@ -2,17 +2,20 @@
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using System;
+using System.Collections.Generic;
 using System.IO;
+using FluentAssertions;
 using Microsoft.CodeAnalysis.Sarif;
 using Microsoft.CodeAnalysis.Sarif.Multitool;
+using Moq;
 using Newtonsoft.Json;
 using Xunit;
 
 namespace Microsoft.CodeAnalysis.Test.UnitTests.Sarif.Multitool
 {
     public class PageCommandTests
     {
-        private static ResourceExtractor Extractor = new ResourceExtractor(typeof(PageCommandTests));
+        private static readonly ResourceExtractor Extractor = new ResourceExtractor(typeof(PageCommandTests));
 
         [Fact]
         public void PageCommand_Basics()
@@ -53,30 +56,135 @@ public void PageCommand_MapTooSmallFallback()
             RunAndCompare(new PageOptions() { TargetMapSizeRatio = 0.050, Index = 1, Count = 2, InputFilePath = sampleFilePath, OutputFilePath = pagedSamplePath });
         }
 
+        // Some option validations (for example, "--index must be non-negative") can be performed
+        // without reading the input SARIF file. We refer to these as "static" validations. When a
+        // a static validation fails, we can simply print an error message and exit. By not
+        // throwing an exception in these cases, we save the user from having to dig the error
+        // message out of an exception dump.
+        //
+        // On the other hand, other option validations (for example, "--run-index must be less than
+        // the number of runs in the input SARIF file") can only be performed once we've started
+        // reading the input file. We refer to these as "dynamic" validations. When a dynamic
+        // validation fails, we have to throw an exception.
+        //
+        // We have separate unit tests for the static and dynamic validations, since they are set
+        // up and detected differently.
+        internal struct StaticValidationTestCase
+        {
+            public string Title;
+            public PageOptions Options;
+            public bool InputFileExists;
+            public bool OutputFileExists;
+            public bool ExpectedReturnValue;
+        }
+
+        private const string InputFilePath = "example.sarif";
+        private const string OutputFilePath = "example.paged.sarif";
+
+        private static readonly StaticValidationTestCase[] s_validationTestCases = new StaticValidationTestCase[]
+        {
+            new StaticValidationTestCase
+            {
+                Title = "Valid options",
+                Options = new PageOptions { RunIndex = 0, Index = 0, Count = 0, InputFilePath = InputFilePath, OutputFilePath = OutputFilePath },
+                InputFileExists = true,
+                OutputFileExists = false,
+                ExpectedReturnValue = true
+            },
+
+            new StaticValidationTestCase
+            {
+                Title = "RunIndex < 0",
+                Options = new PageOptions { RunIndex = -1, Index = 0, Count = 0, InputFilePath = InputFilePath, OutputFilePath = OutputFilePath },
+                InputFileExists = true,
+                OutputFileExists = false,
+                ExpectedReturnValue = false
+            },
+
+            new StaticValidationTestCase
+            {
+                Title = "Index < 0",
+                Options = new PageOptions { RunIndex = 0, Index = -1, Count = 0, InputFilePath = InputFilePath, OutputFilePath = OutputFilePath },
+                InputFileExists = true,
+                OutputFileExists = false,
+                ExpectedReturnValue = false
+            },
+
+            new StaticValidationTestCase
+            {
+                Title = "Count < 0",
+                Options = new PageOptions { RunIndex = 0, Index = 0, Count = -1, InputFilePath = InputFilePath, OutputFilePath = OutputFilePath },
+                InputFileExists = true,
+                OutputFileExists = false,
+                ExpectedReturnValue = false
+            },
+
+            new StaticValidationTestCase
+            {
+                Title = "Nonexistent input file",
+                Options = new PageOptions { RunIndex = 0, Index = 0, Count = 0, InputFilePath = InputFilePath, OutputFilePath = OutputFilePath },
+                InputFileExists = false,
+                OutputFileExists = false,
+                ExpectedReturnValue = false
+            },
+
+            new StaticValidationTestCase
+            {
+                Title = "Existing output file without force",
+                Options = new PageOptions { RunIndex = 0, Index = 0, Count = 0, InputFilePath = InputFilePath, OutputFilePath = OutputFilePath },
+                InputFileExists = true,
+                OutputFileExists = true,
+                ExpectedReturnValue = false
+            },
+
+            new StaticValidationTestCase
+            {
+                Title = "Existing output file with force",
+                Options = new PageOptions { RunIndex = 0, Index = 0, Count = 0, InputFilePath = InputFilePath, OutputFilePath = OutputFilePath, Force = true },
+                InputFileExists = true,
+                OutputFileExists = true,
+                ExpectedReturnValue = true
+            }
+        };
+
         [Fact]
-        public void PageCommand_Validation()
+        [Trait(TestTraits.Bug, "1629")]
+        public void PageCommand_StaticOptionValidation()
+        {
+            var failedTestCases = new List<string>();
+
+            foreach (StaticValidationTestCase testCase in s_validationTestCases)
+            {
+                var mockFileSystem = new Mock<IFileSystem>();
+                mockFileSystem.Setup(x => x.FileExists(InputFilePath)).Returns(testCase.InputFileExists);
+                mockFileSystem.Setup(x => x.FileExists(OutputFilePath)).Returns(testCase.OutputFileExists);
+                IFileSystem fileSystem = mockFileSystem.Object;
+
+                var command = new PageCommand();
+                if (command.ValidateOptions(testCase.Options, fileSystem) != testCase.ExpectedReturnValue)
+                {
+                    failedTestCases.Add(testCase.Title);
+                }
+            }
+
+            failedTestCases.Should().BeEmpty();
+        }
+
+        [Fact]
+        public void PageCommand_DynamicOptionValidation()
         {
             string sampleFilePath = "elfie-arriba.sarif";
             string pagedSamplePath = "elfie-arriba.paged.sarif";
             File.WriteAllText(sampleFilePath, Extractor.GetResourceText(@"PageCommand.elfie-arriba.sarif"));
 
-            // Index < 0
-            Assert.Throws<ArgumentOutOfRangeException>(() => RunAndCompare(new PageOptions() { Index = -1, Count = 1, InputFilePath = sampleFilePath, OutputFilePath = pagedSamplePath }));
-
             // Index >= Count
             Assert.Throws<ArgumentOutOfRangeException>(() => RunAndCompare(new PageOptions() { Index = 5, Count = 1, InputFilePath = sampleFilePath, OutputFilePath = pagedSamplePath }));
 
             // Index + Count > Count
             Assert.Throws<ArgumentOutOfRangeException>(() => RunAndCompare(new PageOptions() { Index = 0, Count = 6, InputFilePath = sampleFilePath, OutputFilePath = pagedSamplePath }));
 
-            // RunIndex < 0
-            Assert.Throws<ArgumentOutOfRangeException>(() => RunAndCompare(new PageOptions() { RunIndex = -1, Index = 1, Count = 1, InputFilePath = sampleFilePath, OutputFilePath = pagedSamplePath }));
-
             // RunIndex >= RunCount
             Assert.Throws<ArgumentOutOfRangeException>(() => RunAndCompare(new PageOptions() { RunIndex = 1, Index = 1, Count = 1, InputFilePath = sampleFilePath, OutputFilePath = pagedSamplePath }));
-
-            // No input file
-            Assert.Throws<FileNotFoundException>(() => RunAndCompare(new PageOptions() { InputFilePath = "NotExists.sarif", OutputFilePath = "NotExists.out.sarif" }));
         }
 
         private static void RunAndCompare(PageOptions options)