From ef88877563df83caf8c870d5934eecc9e3de761f Mon Sep 17 00:00:00 2001 From: Ryan Shaffer Date: Mon, 15 Jun 2020 06:11:37 -0700 Subject: [PATCH 1/4] Use updated parsing method everywhere --- src/Jupyter/Magic/AbstractMagic.cs | 18 ------------------ src/Kernel/Magic/PackageMagic.cs | 5 ++++- src/Kernel/Magic/WorkspaceMagic.cs | 5 ++++- 3 files changed, 8 insertions(+), 20 deletions(-) diff --git a/src/Jupyter/Magic/AbstractMagic.cs b/src/Jupyter/Magic/AbstractMagic.cs index 09a3df31b8..4b6b304949 100644 --- a/src/Jupyter/Magic/AbstractMagic.cs +++ b/src/Jupyter/Magic/AbstractMagic.cs @@ -63,24 +63,6 @@ public Func> SafeExecute(Func - /// Parses the input to a magic command, interpreting the input as - /// a name followed by a JSON-serialized dictionary. - /// - public static (string, Dictionary) ParseInput(string input) - { - if (input == null) return (string.Empty, new Dictionary { }); - var BLANK_SPACE = new char[1] { ' ' }; - - var inputParts = input.Split(BLANK_SPACE, 2, StringSplitOptions.RemoveEmptyEntries); - var name = inputParts.Length > 0 ? inputParts[0] : string.Empty; - var args = inputParts.Length > 1 - ? JsonConverters.JsonToDict(inputParts[1]) - : new Dictionary { }; - - return (name, args); - } - /// /// Parses the input to a magic command, interpreting the input as /// a name followed by a JSON-serialized dictionary. diff --git a/src/Kernel/Magic/PackageMagic.cs b/src/Kernel/Magic/PackageMagic.cs index 11a635e462..8ca8aa2d43 100644 --- a/src/Kernel/Magic/PackageMagic.cs +++ b/src/Kernel/Magic/PackageMagic.cs @@ -17,6 +17,8 @@ namespace Microsoft.Quantum.IQSharp.Kernel /// public class PackageMagic : AbstractMagic { + private const string ParameterNamePackageName = "__packageName__"; + /// /// Constructs a new magic command that adds package references to /// a given references collection. @@ -39,7 +41,8 @@ public PackageMagic(IReferences references) : base( /// public override ExecutionResult Run(string input, IChannel channel) { - var (name, _) = ParseInput(input); + var inputParameters = ParseInputParameters(input, firstParameterInferredName: ParameterNamePackageName); + var name = inputParameters.DecodeParameter(ParameterNamePackageName); var status = new Jupyter.TaskStatus($"Adding package {name}"); var statusUpdater = channel.DisplayUpdatable(status); void Update() => statusUpdater.Update(status); diff --git a/src/Kernel/Magic/WorkspaceMagic.cs b/src/Kernel/Magic/WorkspaceMagic.cs index 7ef9ee2996..22b089f7b7 100644 --- a/src/Kernel/Magic/WorkspaceMagic.cs +++ b/src/Kernel/Magic/WorkspaceMagic.cs @@ -14,6 +14,8 @@ namespace Microsoft.Quantum.IQSharp.Kernel /// public class WorkspaceMagic : AbstractMagic { + private const string ParameterNameCommand = "__command__"; + /// /// Given a workspace, constructs a new magic symbol to control /// that workspace. @@ -51,7 +53,8 @@ public void CheckIfReady() /// public override ExecutionResult Run(string input, IChannel channel) { - var (command, _) = ParseInput(input); + var inputParameters = ParseInputParameters(input, firstParameterInferredName: ParameterNameCommand); + var command = inputParameters.DecodeParameter(ParameterNameCommand); if (string.IsNullOrWhiteSpace(command)) { From c1c911fccff0e9f90c53c2309eb5f596f34966df Mon Sep 17 00:00:00 2001 From: Ryan Shaffer Date: Mon, 15 Jun 2020 07:20:06 -0700 Subject: [PATCH 2/4] Improve handling of quotes and spaces --- src/Jupyter/Magic/AbstractMagic.cs | 64 ++++++++++++++++++------------ 1 file changed, 39 insertions(+), 25 deletions(-) diff --git a/src/Jupyter/Magic/AbstractMagic.cs b/src/Jupyter/Magic/AbstractMagic.cs index 4b6b304949..b4ab365a5b 100644 --- a/src/Jupyter/Magic/AbstractMagic.cs +++ b/src/Jupyter/Magic/AbstractMagic.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.IO; using System.Linq; +using System.Text.RegularExpressions; using System.Threading.Tasks; using Microsoft.Jupyter.Core; using Microsoft.Quantum.IQSharp.Common; @@ -79,46 +80,59 @@ public static Dictionary ParseInputParameters(string input, stri { Dictionary inputParameters = new Dictionary(); - var args = input.Split(null as char[], StringSplitOptions.RemoveEmptyEntries); + // This regex looks for four types of matches: + // 1. (\{.*\}) + // Matches anything enclosed in matching curly braces. + // 2. [^\s"]+(?:\s*=\s*)(?:"[^"]*"|[^\s"]*)* + // Matches things that look like key=value, allowing whitespace around the equals sign, + // and allowing value to be a quoted string, e.g., key="value". + // 3. [^\s"]+(?:"[^"]*"[^\s"]*)* + // Matches things that are single words, not inside quotes. + // 4. (?:"[^"]*"[^\s"]*)+ + // Matches quoted strings. + var regex = new Regex(@"(\{.*\})|[^\s""]+(?:\s*=\s*)(?:""[^""]*""|[^\s""]*)*|[^\s""]+(?:""[^""]*""[^\s""]*)*|(?:""[^""]*""[^\s""]*)+"); + var args = regex.Matches(input).Select(match => match.Value); // If we are expecting a first inferred-name parameter, see if it exists. // If so, serialize it to the dictionary as JSON and remove it from the list of args. - if (args.Length > 0 && - !args[0].StartsWith("{") && - !args[0].Contains("=") && + if (args.Any() && + !args.First().StartsWith("{") && + !args.First().Contains("=") && !string.IsNullOrEmpty(firstParameterInferredName)) { - using (var writer = new StringWriter()) - { - Json.Serializer.Serialize(writer, args[0]); - inputParameters[firstParameterInferredName] = writer.ToString(); - } - args = args.Where((_, index) => index != 0).ToArray(); + using var writer = new StringWriter(); + Json.Serializer.Serialize(writer, args.First()); + inputParameters[firstParameterInferredName] = writer.ToString(); + args = args.Skip(1); } - // See if the remaining arguments look like JSON. If so, try to parse as JSON. - // Otherwise, try to parse as key=value pairs and serialize into the dictionary as JSON. - if (args.Length > 0 && args[0].StartsWith("{")) + // See if the remaining arguments look like JSON. If so, parse as JSON. + if (args.Any() && args.First().StartsWith("{")) { - var jsonArgs = JsonToDict(string.Join(" ", args)); + var jsonArgs = JsonToDict(args.First()); foreach (var (key, jsonValue) in jsonArgs) { inputParameters[key] = jsonValue; } + + return inputParameters; } - else + + // Otherwise, try to parse as key=value pairs and serialize into the dictionary as JSON. + foreach (string arg in args) { - foreach (string arg in args) + var tokens = arg.Split("=", 2); + var key = tokens[0].Trim(); + var value = tokens.Length switch { - var tokens = arg.Split("=", 2); - var key = tokens[0].Trim(); - var value = (tokens.Length == 1) ? true as object : tokens[1].Trim() as object; - using (var writer = new StringWriter()) - { - Json.Serializer.Serialize(writer, value); - inputParameters[key] = writer.ToString(); - } - } + // If there was no value provided explicitly, treat it as an implicit "true" value + 1 => true as object, + // Trim whitespace and also enclosing single-quotes or double-quotes before returning + _ => Regex.Replace(tokens[1].Trim(), @"^['""]|['""]$", string.Empty) as object + }; + using var writer = new StringWriter(); + Json.Serializer.Serialize(writer, value); + inputParameters[key] = writer.ToString(); } return inputParameters; From 069ede8c4f4a5afe92d403fdfb050209dcc3ed12 Mon Sep 17 00:00:00 2001 From: Ryan Shaffer Date: Mon, 15 Jun 2020 07:47:57 -0700 Subject: [PATCH 3/4] Small improvement to switch statement --- src/Jupyter/Magic/AbstractMagic.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Jupyter/Magic/AbstractMagic.cs b/src/Jupyter/Magic/AbstractMagic.cs index b4ab365a5b..64a1326266 100644 --- a/src/Jupyter/Magic/AbstractMagic.cs +++ b/src/Jupyter/Magic/AbstractMagic.cs @@ -127,8 +127,12 @@ public static Dictionary ParseInputParameters(string input, stri { // If there was no value provided explicitly, treat it as an implicit "true" value 1 => true as object, + // Trim whitespace and also enclosing single-quotes or double-quotes before returning - _ => Regex.Replace(tokens[1].Trim(), @"^['""]|['""]$", string.Empty) as object + 2 => Regex.Replace(tokens[1].Trim(), @"^['""]|['""]$", string.Empty) as object, + + // We called arg.Split("=", 2), so there should never be more than 2 + _ => throw new InvalidOperationException() }; using var writer = new StringWriter(); Json.Serializer.Serialize(writer, value); From d22d50624112f371e59fd73e0079ecda90c798f8 Mon Sep 17 00:00:00 2001 From: Ryan Shaffer Date: Mon, 15 Jun 2020 08:14:59 -0700 Subject: [PATCH 4/4] Add some tests for the new parsing --- src/Tests/AzureClientMagicTests.cs | 68 +++++++++++++++++++++++------- 1 file changed, 53 insertions(+), 15 deletions(-) diff --git a/src/Tests/AzureClientMagicTests.cs b/src/Tests/AzureClientMagicTests.cs index b06833d336..51763fdbac 100644 --- a/src/Tests/AzureClientMagicTests.cs +++ b/src/Tests/AzureClientMagicTests.cs @@ -50,9 +50,25 @@ public void TestConnectMagic() resourceGroupName={resourceGroupName} workspaceName={workspaceName} storageAccountConnectionString={storageAccountConnectionString}"); - Assert.AreEqual(azureClient.LastAction, AzureClientAction.Connect); + Assert.AreEqual(AzureClientAction.Connect, azureClient.LastAction); Assert.IsFalse(azureClient.RefreshCredentials); - Assert.AreEqual(azureClient.ConnectionString, storageAccountConnectionString); + Assert.AreEqual(subscriptionId, azureClient.SubscriptionId); + Assert.AreEqual(resourceGroupName, azureClient.ResourceGroupName); + Assert.AreEqual(workspaceName, azureClient.WorkspaceName); + Assert.AreEqual(storageAccountConnectionString, azureClient.ConnectionString); + + // valid input with extra whitespace and quotes + connectMagic.Test( + @$"subscriptionId = {subscriptionId} + resourceGroupName= ""{resourceGroupName}"" + workspaceName ={workspaceName} + storageAccountConnectionString = '{storageAccountConnectionString}'"); + Assert.AreEqual(AzureClientAction.Connect, azureClient.LastAction); + Assert.IsFalse(azureClient.RefreshCredentials); + Assert.AreEqual(subscriptionId, azureClient.SubscriptionId); + Assert.AreEqual(resourceGroupName, azureClient.ResourceGroupName); + Assert.AreEqual(workspaceName, azureClient.WorkspaceName); + Assert.AreEqual(storageAccountConnectionString, azureClient.ConnectionString); // valid input with forced login connectMagic.Test( @@ -60,7 +76,6 @@ public void TestConnectMagic() resourceGroupName={resourceGroupName} workspaceName={workspaceName} storageAccountConnectionString={storageAccountConnectionString}"); - Assert.IsTrue(azureClient.RefreshCredentials); } @@ -71,13 +86,19 @@ public void TestStatusMagic() var azureClient = new MockAzureClient(); var statusMagic = new StatusMagic(azureClient); statusMagic.Test(string.Empty); - Assert.AreEqual(azureClient.LastAction, AzureClientAction.GetJobStatus); + Assert.AreEqual(AzureClientAction.GetJobStatus, azureClient.LastAction); // single argument - should print job status azureClient = new MockAzureClient(); statusMagic = new StatusMagic(azureClient); statusMagic.Test($"{jobId}"); - Assert.AreEqual(azureClient.LastAction, AzureClientAction.GetJobStatus); + Assert.AreEqual(AzureClientAction.GetJobStatus, azureClient.LastAction); + + // single argument with quotes - should print job status + azureClient = new MockAzureClient(); + statusMagic = new StatusMagic(azureClient); + statusMagic.Test($"\"{jobId}\""); + Assert.AreEqual(AzureClientAction.GetJobStatus, azureClient.LastAction); } [TestMethod] @@ -87,11 +108,11 @@ public void TestSubmitMagic() var azureClient = new MockAzureClient(); var submitMagic = new SubmitMagic(azureClient); submitMagic.Test(string.Empty); - Assert.AreEqual(azureClient.LastAction, AzureClientAction.SubmitJob); + Assert.AreEqual(AzureClientAction.SubmitJob, azureClient.LastAction); // single argument submitMagic.Test($"{operationName}"); - Assert.AreEqual(azureClient.LastAction, AzureClientAction.SubmitJob); + Assert.AreEqual(AzureClientAction.SubmitJob, azureClient.LastAction); Assert.IsTrue(azureClient.SubmittedJobs.Contains(operationName)); } @@ -102,11 +123,11 @@ public void TestExecuteMagic() var azureClient = new MockAzureClient(); var executeMagic = new ExecuteMagic(azureClient); executeMagic.Test(string.Empty); - Assert.AreEqual(azureClient.LastAction, AzureClientAction.ExecuteJob); + Assert.AreEqual(AzureClientAction.ExecuteJob, azureClient.LastAction); // single argument executeMagic.Test($"{operationName}"); - Assert.AreEqual(azureClient.LastAction, AzureClientAction.ExecuteJob); + Assert.AreEqual(AzureClientAction.ExecuteJob, azureClient.LastAction); Assert.IsTrue(azureClient.ExecutedJobs.Contains(operationName)); } @@ -117,13 +138,19 @@ public void TestOutputMagic() var azureClient = new MockAzureClient(); var outputMagic = new OutputMagic(azureClient); outputMagic.Test(string.Empty); - Assert.AreEqual(azureClient.LastAction, AzureClientAction.GetJobResult); + Assert.AreEqual(AzureClientAction.GetJobResult, azureClient.LastAction); - // single argument - should print job status + // single argument - should print job result azureClient = new MockAzureClient(); outputMagic = new OutputMagic(azureClient); outputMagic.Test($"{jobId}"); - Assert.AreEqual(azureClient.LastAction, AzureClientAction.GetJobResult); + Assert.AreEqual(AzureClientAction.GetJobResult, azureClient.LastAction); + + // single argument with quotes - should print job result + azureClient = new MockAzureClient(); + outputMagic = new OutputMagic(azureClient); + outputMagic.Test($"'{jobId}'"); + Assert.AreEqual(AzureClientAction.GetJobResult, azureClient.LastAction); } [TestMethod] @@ -133,7 +160,7 @@ public void TestJobsMagic() var azureClient = new MockAzureClient(); var jobsMagic = new JobsMagic(azureClient); jobsMagic.Test(string.Empty); - Assert.AreEqual(azureClient.LastAction, AzureClientAction.GetJobList); + Assert.AreEqual(AzureClientAction.GetJobList, azureClient.LastAction); } [TestMethod] @@ -143,13 +170,18 @@ public void TestTargetMagic() var azureClient = new MockAzureClient(); var targetMagic = new TargetMagic(azureClient); targetMagic.Test(targetId); - Assert.AreEqual(azureClient.LastAction, AzureClientAction.SetActiveTarget); + Assert.AreEqual(AzureClientAction.SetActiveTarget, azureClient.LastAction); + + // single argument with quotes - should set active target + targetMagic = new TargetMagic(azureClient); + targetMagic.Test($"\"{targetId}\""); + Assert.AreEqual(AzureClientAction.SetActiveTarget, azureClient.LastAction); // no arguments - should print active target azureClient = new MockAzureClient(); targetMagic = new TargetMagic(azureClient); targetMagic.Test(string.Empty); - Assert.AreEqual(azureClient.LastAction, AzureClientAction.GetActiveTarget); + Assert.AreEqual(AzureClientAction.GetActiveTarget, azureClient.LastAction); } } @@ -170,6 +202,9 @@ internal enum AzureClientAction public class MockAzureClient : IAzureClient { internal AzureClientAction LastAction = AzureClientAction.None; + internal string SubscriptionId = string.Empty; + internal string ResourceGroupName = string.Empty; + internal string WorkspaceName = string.Empty; internal string ConnectionString = string.Empty; internal bool RefreshCredentials = false; internal string ActiveTargetId = string.Empty; @@ -205,6 +240,9 @@ public async Task ExecuteJobAsync(IChannel channel, AzureSubmis public async Task ConnectAsync(IChannel channel, string subscriptionId, string resourceGroupName, string workspaceName, string storageAccountConnectionString, bool refreshCredentials) { LastAction = AzureClientAction.Connect; + SubscriptionId = subscriptionId; + ResourceGroupName = resourceGroupName; + WorkspaceName = workspaceName; ConnectionString = storageAccountConnectionString; RefreshCredentials = refreshCredentials; return ExecuteStatus.Ok.ToExecutionResult();