Merge pull request #80 from guitarrapc/feature/output

fix: quoted argument output with invalid result.
guitarrapc · Jul 12, 2023 · 0949847 · 0949847
2 parents 3754b34 + 1e9356f
commit 0949847
Show file tree

Hide file tree

Showing 5 changed files with 110 additions and 17 deletions.
diff --git a/src/UnityBuildRunner.Core/DefaultBuilder.cs b/src/UnityBuildRunner.Core/DefaultBuilder.cs
@@ -58,6 +58,7 @@ public async Task BuildAsync(CancellationToken ct = default)
         logger.LogInformation($"  - Command:     {settings.UnityPath} {settings.ArgumentString}");
         logger.LogInformation($"  - WorkingDir:  {settings.WorkingDirectory}");
         logger.LogInformation($"  - LogFilePath: {settings.LogFilePath}");
+        logger.LogInformation($"  - Timeout:     {settings.TimeOut}");
         var sw = Stopwatch.StartNew();
         using var process = Process.Start(new ProcessStartInfo()
         {

diff --git a/src/UnityBuildRunner.Core/DefaultSettings.cs b/src/UnityBuildRunner.Core/DefaultSettings.cs
@@ -92,7 +92,7 @@ public CancellationTokenSource CreateCancellationTokenSource(CancellationToken?
     /// <summary>
     /// Parse args and create <see cref="DefaultSettings"/>.
     /// </summary>
-    public static bool TryParse(string[] args, string unityPath, TimeSpan timeout, [NotNullWhen(true)] out DefaultSettings? settings)
+    public static bool TryParse(IReadOnlyList<string> args, string unityPath, TimeSpan timeout, [NotNullWhen(true)] out DefaultSettings? settings)
     {
         try
         {
@@ -128,7 +128,7 @@ public static DefaultSettings Parse(IReadOnlyList<string> args, string unityPath
         }
 
         var arguments = args.Where(x => !string.IsNullOrWhiteSpace(x)).ToArray();
-        var argumentString = string.Join(" ", arguments.Select(s => s.First() == '-' ? s : "\"" + s + "\""));
+        var argumentString = string.Join(" ", arguments.Select(s => s.AsSpan()[0] == '-' ? s : QuoteString(s)));
 
         // WorkingDirectory should be cli launch path.
         var workingDirectory = Directory.GetCurrentDirectory();
@@ -142,6 +142,56 @@ public static DefaultSettings Parse(IReadOnlyList<string> args, string unityPath
         return settings;
     }
 
+    /// <summary>
+    /// QuoteString when possible.
+    /// </summary>
+    /// <param name="text"></param>
+    /// <returns></returns>
+    /// <exception cref="ArgumentException"></exception>
+    internal static string QuoteString(string text)
+    {
+        var span = text.AsSpan();
+
+        // `` is invalid
+        if (span.Length == 0)
+        {
+            throw new ArgumentException($"Argument is empty and is not valid string to quote. input: {text}");
+        }
+
+        // `"` is invalid
+        if (span.Length == 1 && span[0] == '"')
+        {
+            throw new ArgumentException($"Argument is \" and is not valid string to quote. input: {text}");
+        }
+
+        // `"foo` is invalid
+        if (span.Length >= 2 && span[0] == '"' && span[^1] != '"')
+        {
+            throw new ArgumentException($"Argument begin with \" but not closed, please complete quote. input: {text}");
+        }
+
+        // `foo"` is invalid
+        if (span.Length >= 2 && span[0] != '"' && span[^1] == '"')
+        {
+            throw new ArgumentException($"Argument end with \" but not begin with \", please complete quote. input: {text}");
+        }
+
+        // `foo"foo` or `foo"foo"foo` is invalid
+        if (span[1..^1].Contains('"'))
+        {
+            throw new ArgumentException($"Argument contains \", but is invalid. input: {text}");
+        }
+
+        // `""` and `"foo"` is valid
+        if (span.Length >= 2 && span[0] == '"' && span[^1] == '"')
+        {
+            return text;
+        }
+
+        // `foo` is valid
+        return $"\"{text}\"";
+    }
+
     /// <summary>
     /// Parse `-logFile <logfile>` from arguments.
     /// </summary>
@@ -161,6 +211,11 @@ internal static string ParseLogFile(IReadOnlyList<string> args)
         return logFile;
     }
 
+    /// <summary>
+    /// Detect Logfile name is valid or not
+    /// </summary>
+    /// <param name="fileName"></param>
+    /// <returns></returns>
     internal static bool IsValidLogFileName(string? fileName)
     {
         // missing filename is not valid

diff --git a/src/UnityBuildRunner/Program.cs b/src/UnityBuildRunner/Program.cs
@@ -31,22 +31,20 @@ public UnityBuildRunnerCommand(ILogger<UnityBuildRunnerCommand> logger)
         {
             arguments = arguments.Except(new[] { "--unity-path", unityPath });
         }
+        var args = arguments?.ToArray();
 
-        if (arguments is not null && arguments.Any())
+        if (args is null || !args.Any())
         {
-            var timeoutSpan = TimeSpan.TryParse(timeout, out var r) ? r : timeoutDefault;
-            var settings = DefaultSettings.Parse(arguments.ToArray()!, unityPath, timeoutSpan);
-            using var cts = settings.CreateCancellationTokenSource(Context.CancellationToken);
-
-            // build
-            var builder = new DefaultBuilder(settings, logger);
-            await builder.BuildAsync(cts.Token);
-            return builder.ExitCode;
-        }
-        else
-        {
-            logger.LogError($"No valid argument found, exiting. You have specified arguments: {string.Join(" ", arguments?.ToArray() ?? Array.Empty<string>())}");
-            return 1;
+            throw new ArgumentOutOfRangeException($"No valid argument found, exiting. You have specified arguments: {string.Join(" ", args ?? Array.Empty<string>())}");
         }
+
+        // build
+        var timeoutSpan = TimeSpan.TryParse(timeout, out var r) ? r : timeoutDefault;
+        var settings = DefaultSettings.Parse(args!, unityPath, timeoutSpan);
+        using var cts = settings.CreateCancellationTokenSource(Context.CancellationToken);
+
+        var builder = new DefaultBuilder(settings, logger);
+        await builder.BuildAsync(cts.Token);
+        return builder.ExitCode;
     }
 }
diff --git a/src/UnityBuildRunner/Properties/launchSettings.json b/src/UnityBuildRunner/Properties/launchSettings.json
@@ -18,6 +18,18 @@
     "UnityBuildRunner (-logFile -)": {
       "commandName": "Project",
       "commandLineArgs": "--unity-path \"C:/Program Files/Unity/Hub/Editor/2021.3.17f1/Editor/Unity.exe\" -quit -batchmode -nographics -silent-crashes -logfile - -buildTarget StandaloneWindows64 -projectPath ../../../../../sandbox/Sandbox.Unity -executeMethod BuildUnity.BuildGame"
+    },
+    "UnityBuildRunner (quote-slash)": {
+      "commandName": "Project",
+      "commandLineArgs": "--unity-path \"C:/Program Files/Unity/Hub/Editor/2021.3.17f1/Editor/Unity.exe\" -quit -batchmode -nographics -silent-crashes -logfile - -buildTarget StandaloneWindows64 -projectPath \"../../../../../sandbox/Sandbox.Unity/\" -executeMethod BuildUnity.BuildGame"
+    },
+    "UnityBuildRunner (quote)": {
+      "commandName": "Project",
+      "commandLineArgs": "--unity-path \"C:/Program Files/Unity/Hub/Editor/2021.3.17f1/Editor/Unity.exe\" -quit -batchmode -nographics -silent-crashes -logfile - -buildTarget StandaloneWindows64 -projectPath \"..\\..\\..\\..\\..\\sandbox/Sandbox.Unity\\\\\" -executeMethod BuildUnity.BuildGame"
+    },
+    "UnityBuildRunner (quote-bad)": {
+      "commandName": "Project",
+      "commandLineArgs": "--unity-path \"C:/Program Files/Unity/Hub/Editor/2021.3.17f1/Editor/Unity.exe\" -quit -batchmode -nographics -silent-crashes -logfile - -buildTarget StandaloneWindows64 -projectPath \"..\\..\\..\\..\\..\\sandbox/Sandbox.Unity\\\" -executeMethod BuildUnity.BuildGame"
     }
   }
 }
diff --git a/tests/UnityBuildRunner.Core.Tests/SettingsUnitTests.cs b/tests/UnityBuildRunner.Core.Tests/SettingsUnitTests.cs
@@ -93,9 +93,13 @@ public void ArgsShouldNotContainNullOrWhiteSpace(string[] actual, string[] expec
     [InlineData(new[] { "-bathmode", "", "-nographics", "-projectpath", "HogemogeProject", "-executeMethod", "MethodName", "-quite", "-logfile", "build.log" }, "-bathmode -nographics -projectpath \"HogemogeProject\" -executeMethod \"MethodName\" -quite -logfile \"build.log\"")]
     [InlineData(new[] { "-logfile", "hoge.log", "-bathmode", "-nographics", "-projectpath", "HogemogeProject", "-executeMethod", "MethodName", "-quite", " " }, "-logfile \"hoge.log\" -bathmode -nographics -projectpath \"HogemogeProject\" -executeMethod \"MethodName\" -quite")]
     [InlineData(new[] { "-logfile", "hoge.log", "-bathmode", "-nographics", "-projectpath", " ", "HogemogeProject", "-executeMethod", "MethodName", "-quite" }, "-logfile \"hoge.log\" -bathmode -nographics -projectpath \"HogemogeProject\" -executeMethod \"MethodName\" -quite")]
+    [InlineData(new[] { "-logfile", "hoge.log", "-bathmode", "-nographics", "-projectpath", "foo/bar/baz", "-executeMethod", "MethodName", "-quite" }, "-logfile \"hoge.log\" -bathmode -nographics -projectpath \"foo/bar/baz\" -executeMethod \"MethodName\" -quite")] // -projectpath ends without /
     [InlineData(new[] { "-logfile", "hoge.log", "-bathmode", "-nographics", "-projectpath", "foo/bar/baz/", "-executeMethod", "MethodName", "-quite" }, "-logfile \"hoge.log\" -bathmode -nographics -projectpath \"foo/bar/baz/\" -executeMethod \"MethodName\" -quite")]
-    [InlineData(new[] { "-logfile", "hoge.log", "-bathmode", "-nographics", "-projectpath", @"foo\bar\baz", "-executeMethod", "MethodName", "-quite" }, "-logfile \"hoge.log\" -bathmode -nographics -projectpath \"foo\\bar\\baz\" -executeMethod \"MethodName\" -quite")]
+    [InlineData(new[] { "-logfile", "hoge.log", "-bathmode", "-nographics", "-projectpath", @"foo\bar\baz", "-executeMethod", "MethodName", "-quite" }, "-logfile \"hoge.log\" -bathmode -nographics -projectpath \"foo\\bar\\baz\" -executeMethod \"MethodName\" -quite")] // -projectpath ends without \
     [InlineData(new[] { "-logfile", "hoge.log", "-bathmode", "-nographics", "-projectpath", @"foo\bar\baz\", "-executeMethod", "MethodName", "-quite" }, "-logfile \"hoge.log\" -bathmode -nographics -projectpath \"foo\\bar\\baz\\\" -executeMethod \"MethodName\" -quite")]
+    [InlineData(new[] { "-logfile", "\"hoge.log\"", "-bathmode", "-nographics", "-projectpath", @"""foo\bar\baz\""", "-executeMethod", "\"MethodName\"", "-quite" }, "-logfile \"hoge.log\" -bathmode -nographics -projectpath \"foo\\bar\\baz\\\" -executeMethod \"MethodName\" -quite")] // input is already quoted
+    [InlineData(new[] { "-logfile", "\"hoge.log\"", "-bathmode", "-nographics", "-projectpath", @"""foo\bar\baz\""", "-executeMethod", "MethodName", "-quite" }, "-logfile \"hoge.log\" -bathmode -nographics -projectpath \"foo\\bar\\baz\\\" -executeMethod \"MethodName\" -quite")] // input is already quoted
+    [InlineData(new[] { "-logfile", "\"hoge.log\"", "-bathmode", "-nographics", "-projectpath", @"""foo\bar\baz""", "-executeMethod", "\"MethodName\"", "-quite" }, "-logfile \"hoge.log\" -bathmode -nographics -projectpath \"foo\\bar\\baz\" -executeMethod \"MethodName\" -quite")] // input is already quoted
     public void ArgsumentStringShouldFormated(string[] actual, string expected)
     {
         ISettings settings = DefaultSettings.Parse(actual, _unityPath, _timeout);
@@ -112,4 +116,27 @@ public void IsValidLogFilePath(string? logFilePath, bool expected)
     {
         DefaultSettings.IsValidLogFileName(logFilePath).Should().Be(expected);
     }
+
+    [Theory]
+    [InlineData("")]
+    [InlineData("\"")]
+    [InlineData("\"foo")]
+    [InlineData("foo\"")]
+    [InlineData("fo\"o")]
+    [InlineData("f\"o\"o")]
+    [InlineData("\"fo\"o\"")]
+    [InlineData("\"f\"o\"o\"")]
+    public void QuoteStringInvalidInput(string input)
+    {
+        Assert.Throws<ArgumentException>(() => DefaultSettings.QuoteString(input));
+    }
+
+    [Theory]
+    [InlineData("\"\"", "\"\"")]
+    [InlineData("\"foo\"", "\"foo\"")]
+    [InlineData("foo", "\"foo\"")]
+    public void QuoteStringValidValue(string input, string expected)
+    {
+        DefaultSettings.QuoteString(input).Should().Be(expected);
+    }
 }