From 337262597f90c57ad11ae516e6f2f826620009fa Mon Sep 17 00:00:00 2001 From: Jon Sequeira Date: Mon, 4 Dec 2023 14:34:29 -0800 Subject: [PATCH] Fix input prompt quoting issue (#3360) * fix issue with multiple #!set commands using @input in one submission * cleanup * fix quoted prompt bug * fix warning --- ...ractive.PowerShell.Tests.v3.ncrunchproject | 3 - .../PowerShellKernel.cs | 36 +++-- .../InputsWithinMagicCommandsTests.cs | 151 +++++++++++++++++- ...DotNet.Interactive.Tests.v3.ncrunchproject | 3 + .../VariableSharingTests.SetMagicCommand.cs | 24 ++- .../KernelExtensions.cs | 83 +++++----- .../KernelScheduler.cs | 2 +- .../Parsing/SubmissionParser.cs | 135 +++++++++++++++- .../PasswordString.cs | 1 - .../KernelExtensionLoader.cs | 3 - 10 files changed, 361 insertions(+), 80 deletions(-) diff --git a/src/Microsoft.DotNet.Interactive.PowerShell.Tests/Microsoft.DotNet.Interactive.PowerShell.Tests.v3.ncrunchproject b/src/Microsoft.DotNet.Interactive.PowerShell.Tests/Microsoft.DotNet.Interactive.PowerShell.Tests.v3.ncrunchproject index 88707d5502..b2209558d9 100644 --- a/src/Microsoft.DotNet.Interactive.PowerShell.Tests/Microsoft.DotNet.Interactive.PowerShell.Tests.v3.ncrunchproject +++ b/src/Microsoft.DotNet.Interactive.PowerShell.Tests/Microsoft.DotNet.Interactive.PowerShell.Tests.v3.ncrunchproject @@ -29,9 +29,6 @@ Microsoft.DotNet.Interactive.PowerShell.Tests.PowerShellKernelTests.GetCorrectProfilePaths - - Microsoft.DotNet.Interactive.PowerShell.Tests.PowerShellKernelTests.When_code_produces_errors_then_the_command_fails - \ No newline at end of file diff --git a/src/Microsoft.DotNet.Interactive.PowerShell/PowerShellKernel.cs b/src/Microsoft.DotNet.Interactive.PowerShell/PowerShellKernel.cs index 6b9d7b63f6..70105d1671 100644 --- a/src/Microsoft.DotNet.Interactive.PowerShell/PowerShellKernel.cs +++ b/src/Microsoft.DotNet.Interactive.PowerShell/PowerShellKernel.cs @@ -280,7 +280,7 @@ async Task IKernelCommandHandler.HandleAsync( { var success = RunSubmitCodeLocally(code); - if (!success || pwsh.HadErrors) + if (!success) { context.Fail(context.Command); } @@ -383,23 +383,31 @@ private bool RunSubmitCodeLocally(string code) pwsh.InvokeAndClear(); - pwsh.AddScript("$error"); - var output = new List(); - try + if (pwsh.HadErrors) { - pwsh.Invoke(input: null, output: output); - } - finally - { - pwsh.Clear(); + succeeded = false; } - - if (output.Count > _errorCount) + else { - succeeded = false; + // certain kinds of errors aren't signaled by the HadErrors so we can check using $error + pwsh.AddScript("$error"); + var output = new List(); + try + { + pwsh.Invoke(input: null, output: output); + } + finally + { + pwsh.Clear(); + } + + if (output.Count > _errorCount) + { + succeeded = false; + } + + _errorCount = output.Count; } - - _errorCount = output.Count; } catch (Exception e) { diff --git a/src/Microsoft.DotNet.Interactive.Tests/InputsWithinMagicCommandsTests.cs b/src/Microsoft.DotNet.Interactive.Tests/InputsWithinMagicCommandsTests.cs index bf375c0818..bd43466b33 100644 --- a/src/Microsoft.DotNet.Interactive.Tests/InputsWithinMagicCommandsTests.cs +++ b/src/Microsoft.DotNet.Interactive.Tests/InputsWithinMagicCommandsTests.cs @@ -2,6 +2,7 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System; +using System.Collections.Generic; using System.CommandLine; using System.IO; using System.Threading.Tasks; @@ -10,6 +11,7 @@ using Microsoft.DotNet.Interactive.Commands; using Microsoft.DotNet.Interactive.CSharp; using Microsoft.DotNet.Interactive.Events; +using Microsoft.DotNet.Interactive.Tests.Utility; using Xunit; namespace Microsoft.DotNet.Interactive.Tests; @@ -17,10 +19,15 @@ namespace Microsoft.DotNet.Interactive.Tests; public class InputsWithinMagicCommandsTests : IDisposable { private readonly CompositeKernel kernel; + private RequestInput _receivedRequestInput = null; - private string _receivedUserInput = null; + + private readonly List _receivedUserInput = new(); + private readonly Command _shimCommand; + private readonly Queue _responses = new(); + public InputsWithinMagicCommandsTests() { kernel = CreateKernel(); @@ -28,7 +35,7 @@ public InputsWithinMagicCommandsTests() kernel.RegisterCommandHandler((requestInput, context) => { _receivedRequestInput = requestInput; - context.Publish(new InputProduced("hello!", requestInput)); + context.Publish(new InputProduced(_responses.Dequeue(), requestInput)); return Task.CompletedTask; }); @@ -41,7 +48,7 @@ public InputsWithinMagicCommandsTests() }; _shimCommand.SetHandler(context => { - _receivedUserInput = context.ParseResult.GetValueForOption(stringOption); + _receivedUserInput.Add(context.ParseResult.GetValueForOption(stringOption)); }); kernel.FindKernelByName("csharp").AddDirective(_shimCommand); @@ -85,11 +92,27 @@ public async Task Input_token_in_magic_command_includes_requested_value_name() } [Fact] - public async Task Input_token_in_magic_command_prompts_user_passes_user_input_to_directive__to_handler() + public async Task Input_token_in_magic_command_prompts_user_passes_user_input_to_directive_to_handler() { + _responses.Enqueue("one"); + await kernel.SendAsync(new SubmitCode("#!shim --string @input:input-please", "csharp")); - _receivedUserInput.Should().Be("hello!"); + _receivedUserInput.Should().ContainSingle().Which.Should().Be("one"); + } + + [Fact] + public async Task Input_token_in_magic_command_prompts_user_passes_user_input_to_directive_to_handler_when_there_are_multiple_inputs() + { + _responses.Enqueue("one"); + _responses.Enqueue("two"); + + await kernel.SendAsync(new SubmitCode(""" + #!shim --string @input:input-please + #!shim --string @input:input-please + """, "csharp")); + + _receivedUserInput.Should().BeEquivalentTo("one", "two"); } [Fact] @@ -109,11 +132,27 @@ public async Task Input_token_in_magic_command_prompts_user_for_password() } [Fact] - public async Task An_input_type_hint_is_set_for_file_inputs() + public async Task An_input_type_hint_is_set_for_file_inputs_when_prompt_is_unquoted() { _shimCommand.Add(new Option("--file")); - await kernel.SendAsync(new SubmitCode("#!shim --file @input:file-please\n// some more stuff", "csharp")); + await kernel.SendAsync(new SubmitCode(""" + #!shim --file @input:file-please + // some more stuff + """, "csharp")); + + _receivedRequestInput.InputTypeHint.Should().Be("file"); + } + + [Fact] + public async Task An_input_type_hint_is_set_for_file_inputs_when_prompt_is_quoted() + { + _shimCommand.Add(new Option("--file")); + + await kernel.SendAsync(new SubmitCode(""" + #!shim --file @input:"file please" + // some more stuff + """, "csharp")); _receivedRequestInput.InputTypeHint.Should().Be("file"); } @@ -128,6 +167,104 @@ public async Task Unknown_types_return_type_hint_of_text() _receivedRequestInput.InputTypeHint.Should().Be("text"); } + [Fact] + public async Task multiple_set_commands_with_inputs_can_be_used_in_single_submission() + { + using var kernel = new CompositeKernel + { + new CSharpKernel().UseValueSharing() + }; + + var responses = new Queue(); + responses.Enqueue("one"); + responses.Enqueue("two"); + responses.Enqueue("three"); + + kernel.RegisterCommandHandler((requestInput, context) => + { + context.Publish(new InputProduced(responses.Dequeue(), requestInput)); + return Task.CompletedTask; + }); + + var result = await kernel.SendAsync(new SubmitCode(""" + #!set --name value1 --value @input:"input-please " + #!set --name value2 --value @input:"input-please " + #!set --name value3 --value @input:"input-please" + """, targetKernelName: "csharp")); + + result.Events.Should().NotContainErrors(); + + var csharpKernel = (CSharpKernel)kernel.FindKernelByName("csharp"); + + csharpKernel.TryGetValue("value1", out object value1) + .Should().BeTrue(); + value1.Should().Be("one"); + + csharpKernel.TryGetValue("value2", out object value2) + .Should().BeTrue(); + value2.Should().Be("two"); + + csharpKernel.TryGetValue("value3", out object value3) + .Should().BeTrue(); + value3.Should().Be("three"); + } + + [Fact] + public async Task additional_properties_of_input_request_are_set_by_input_properties_when_prompt_is_quoted() + { + using var kernel = new CSharpKernel(); + + var command = new Command("#!test") + { + new Argument() + }; + kernel.AddDirective(command); + + RequestInput requestInput = null; + kernel.RegisterCommandHandler((input, context) => + { + requestInput = input; + + return Task.CompletedTask; + }); + + var magicCommand = """#!test @input:"pick a number",save,type=file """; + + await kernel.SendAsync(new SubmitCode(magicCommand)); + + requestInput.Prompt.Should().Be("pick a number"); + // FIX: requestInput.Persistent.Should().BeTrue(); + requestInput.InputTypeHint.Should().Be("file"); + } + + [Fact(Skip = "Evaluating different syntax approaches")] + public async Task additional_properties_of_input_request_are_set_by_input_properties_when_prompt_or_field_name_is_not_quoted() + { + using var kernel = new CSharpKernel(); + + var command = new Command("#!test") + { + new Argument() + }; + kernel.AddDirective(command); + + RequestInput requestInput = null; + kernel.RegisterCommandHandler((input, context) => + { + requestInput = input; + + return Task.CompletedTask; + }); + + var magicCommand = """#!test @input:promptOrFieldName,save,type=file """; + + await kernel.SendAsync(new SubmitCode(magicCommand)); + + requestInput.Prompt.Should().Be("promptOrFieldName"); + // FIX: requestInput.Persistent.Should().BeTrue(); + requestInput.InputTypeHint.Should().Be("file"); + } + private static CompositeKernel CreateKernel() => new() { diff --git a/src/Microsoft.DotNet.Interactive.Tests/Microsoft.DotNet.Interactive.Tests.v3.ncrunchproject b/src/Microsoft.DotNet.Interactive.Tests/Microsoft.DotNet.Interactive.Tests.v3.ncrunchproject index 1b4c26dc5f..bc657690b3 100644 --- a/src/Microsoft.DotNet.Interactive.Tests/Microsoft.DotNet.Interactive.Tests.v3.ncrunchproject +++ b/src/Microsoft.DotNet.Interactive.Tests/Microsoft.DotNet.Interactive.Tests.v3.ncrunchproject @@ -12,6 +12,9 @@ Microsoft.DotNet.Interactive.Tests.LanguageKernelPackageTests + + Microsoft.DotNet.Interactive.Tests.StdIoKernelConnectorTests + \ No newline at end of file diff --git a/src/Microsoft.DotNet.Interactive.Tests/VariableSharingTests.SetMagicCommand.cs b/src/Microsoft.DotNet.Interactive.Tests/VariableSharingTests.SetMagicCommand.cs index 34ab3d644f..6b63d50657 100644 --- a/src/Microsoft.DotNet.Interactive.Tests/VariableSharingTests.SetMagicCommand.cs +++ b/src/Microsoft.DotNet.Interactive.Tests/VariableSharingTests.SetMagicCommand.cs @@ -77,19 +77,19 @@ public async Task can_set_value_prompting_user_for_password() } [Fact] - public async Task can_handle_multiple_set_commands_in_single_submission() + public async Task multiple_set_commands_can_be_used_in_single_submission() { using var kernel = CreateCompositeKernel(); kernel.RegisterCommandHandler((requestInput, context) => { - context.Publish(new InputProduced("hello!", requestInput)); + context.Publish(new InputProduced("three", requestInput)); return Task.CompletedTask; }); - await kernel.SendAsync( new SubmitCode(""" - let var1 = "a" - let var2 = "b" + await kernel.SendAsync(new SubmitCode(""" + let var1 = "one" + let var2 = "two" """, targetKernelName:"fsharp")); var result = await kernel.SendAsync(new SubmitCode(""" @@ -107,6 +107,20 @@ public async Task can_handle_multiple_set_commands_in_single_submission() .Which; valueInfosProduced.ValueInfos.Select(v => v.Name).Should().BeEquivalentTo("newVar1", "newVar2", "newVar3"); + + var csharpKernel = (CSharpKernel)kernel.FindKernelByName("csharp"); + + csharpKernel.TryGetValue("newVar1", out object newVar1) + .Should().BeTrue(); + newVar1.Should().Be("one"); + + csharpKernel.TryGetValue("newVar2", out object newVar2) + .Should().BeTrue(); + newVar2.Should().Be("two"); + + csharpKernel.TryGetValue("newVar3", out object newVar3) + .Should().BeTrue(); + newVar3.Should().Be("three"); } [Fact] diff --git a/src/Microsoft.DotNet.Interactive/KernelExtensions.cs b/src/Microsoft.DotNet.Interactive/KernelExtensions.cs index 08b472025e..677ef07a42 100644 --- a/src/Microsoft.DotNet.Interactive/KernelExtensions.cs +++ b/src/Microsoft.DotNet.Interactive/KernelExtensions.cs @@ -168,32 +168,32 @@ private static async Task HandleSetMagicCommand( InvocationContext cmdLineContext, Option nameOption, Option valueOption, - Option mimeTypeOption, Option byrefOption) where T : Kernel { - var valueName = cmdLineContext.ParseResult.GetValueForOption(nameOption); - var mimeType = cmdLineContext.ParseResult.GetValueForOption(mimeTypeOption); var context = cmdLineContext.GetService(); - var isByref = cmdLineContext.ParseResult.GetValueForOption(byrefOption); if (kernel.SupportsCommandType(typeof(SendValue))) { - var events = new List(); - InputProduced inputProduced = null; - using var subscription = context.KernelEvents.Where(e => e is ValueProduced or InputProduced).Subscribe( - e => - { - switch (e) - { - case ValueProduced vp: - events.Add(vp); - break; - case InputProduced ip: - inputProduced = ip; - break; - } - }); + var valueProducedEvents = new List(); + + var inputProducedEvents = new List(); + + using var subscription = context.KernelEvents + .Where(e => e is ValueProduced or InputProduced) + .Subscribe( + e => + { + switch (e) + { + case ValueProduced vp: + valueProducedEvents.Add(vp); + break; + case InputProduced ip: + inputProducedEvents.Add(ip); + break; + } + }); var valueOptionResult = cmdLineContext.ParseResult.GetValueForOption(valueOption); @@ -201,46 +201,51 @@ private static async Task HandleSetMagicCommand( ValueProduced valueProduced = null; - if (valueOptionResult is { Name: var sourceValueName, Kernel: var sourceKernelName } - && sourceKernelName != "input") + if (valueOptionResult is { Name: var sourceValueName, Kernel: var sourceKernelName } && + sourceKernelName != "input") { if (sourceKernel?.KernelInfo.IsProxy == true) { var destinationUri = sourceKernel?.KernelInfo.RemoteUri; - valueProduced = events.SingleOrDefault(e => - e.Name == sourceValueName && e.Command.DestinationUri == destinationUri); + valueProduced = valueProducedEvents.SingleOrDefault(e => + e.Name == sourceValueName && e.Command.DestinationUri == destinationUri); } else { - valueProduced = events.SingleOrDefault(e => - e.Name == sourceValueName && e.Command.TargetKernelName == sourceKernelName); + valueProduced = valueProducedEvents.SingleOrDefault(e => + e.Name == sourceValueName && e.Command.TargetKernelName == sourceKernelName); } } if (valueProduced is { }) { + var isByref = cmdLineContext.ParseResult.GetValueForOption(byrefOption); + var valueNameFromCommandLine = cmdLineContext.ParseResult.GetValueForOption(nameOption); + var referenceValue = isByref ? valueProduced.Value : null; var formattedValue = valueProduced.FormattedValue; - await SendValue(context, kernel, referenceValue, formattedValue, valueName); + await SendValue(context, kernel, referenceValue, formattedValue, valueNameFromCommandLine); } - else if (inputProduced is { }) + + if (inputProducedEvents.Count > 0) { - if (inputProduced.Command is RequestInput { IsPassword: true }) - { - await SendValue(context, kernel, new PasswordString(inputProduced.Value), null, valueName); - } - else + foreach (var inputProduced in inputProducedEvents) { - await SendValue(context, kernel, inputProduced.Value, null, valueName); + if (inputProduced.Command is RequestInput requestInput) + { + if (requestInput.IsPassword) + { + await SendValue(context, kernel, new PasswordString(inputProduced.Value), null, requestInput.ValueName); + } + else + { + await SendValue(context, kernel, inputProduced.Value, null, requestInput.ValueName); + } + } } } - else - { - await SendValue(context, kernel, valueOptionResult?.Value, null, valueName); - } - } else { @@ -346,7 +351,7 @@ destinationKernel.RootKernel is CompositeKernel root && }; set.SetHandler(async cmdLineContext => - await HandleSetMagicCommand(destinationKernel, cmdLineContext, nameOption, valueOption, mimeTypeOption, byrefOption)); + await HandleSetMagicCommand(destinationKernel, cmdLineContext, nameOption, valueOption, byrefOption)); destinationKernel.AddDirective(set); diff --git a/src/Microsoft.DotNet.Interactive/KernelScheduler.cs b/src/Microsoft.DotNet.Interactive/KernelScheduler.cs index fa8dffd6e5..76f20fe910 100644 --- a/src/Microsoft.DotNet.Interactive/KernelScheduler.cs +++ b/src/Microsoft.DotNet.Interactive/KernelScheduler.cs @@ -96,7 +96,7 @@ public Task RunAsync( internal async Task IdleAsync() { - if (_currentlyRunningTopLevelOperation is { } currentlyRunning && !currentlyRunning.IsCompleted) + if (_currentlyRunningTopLevelOperation is { IsCompleted: false } currentlyRunning) { await currentlyRunning.TaskCompletionSource.Task; } diff --git a/src/Microsoft.DotNet.Interactive/Parsing/SubmissionParser.cs b/src/Microsoft.DotNet.Interactive/Parsing/SubmissionParser.cs index 900af6ecc4..9210dfcfdf 100644 --- a/src/Microsoft.DotNet.Interactive/Parsing/SubmissionParser.cs +++ b/src/Microsoft.DotNet.Interactive/Parsing/SubmissionParser.cs @@ -503,18 +503,17 @@ void ReplaceTokensWithUserInput(out IReadOnlyList replacementTokens) var fixedUpText = currentDirectiveNode .Text + .Replace("\"", "") // paired quotes are removed from tokenToReplace by the command line string splitter so we also need to remove them from the raw text to find possible matches .Replace($"@{tokenToReplace}", replaceMe) - .Replace(" @", ""); + .Replace(" @", " "); var parseResult = currentDirectiveNode.DirectiveParser.Parse(fixedUpText); - var symbolResult = parseResult.CommandResult.Children.FirstOrDefault(c => c.Tokens.Any(t => t.Value == replaceMe)); - if (targetKernelName == "password") { typeHint = "password"; } - else if (symbolResult is { Symbol: { } symbol }) + else if (parseResult.CommandResult.Children.FirstOrDefault(c => c.Tokens.Any(t => t.Value == replaceMe)) is { Symbol: { } symbol }) { typeHint = GetTypeHint(symbol); } @@ -530,10 +529,58 @@ void ReplaceTokensWithUserInput(out IReadOnlyList replacementTokens) break; } + var startOfReplaceMe = fixedUpText.IndexOf(replaceMe, StringComparison.OrdinalIgnoreCase); + var rawTokenTextPlusRawTrailingText = currentDirectiveNode.Text[startOfReplaceMe..]; + var endOfReplaceMe = startOfReplaceMe + replaceMe.Length; + var rawTrailingText = currentDirectiveNode.Text[Math.Min(endOfReplaceMe, currentDirectiveNode.Text.Length)..]; + string rawTokenText; + + if (rawTrailingText.Length > 0) + { + if (rawTokenTextPlusRawTrailingText.EndsWith(rawTrailingText)) + { + rawTokenText = rawTokenTextPlusRawTrailingText.Remove(rawTokenTextPlusRawTrailingText.LastIndexOf(rawTrailingText)); + } + else + { + rawTokenText = rawTokenTextPlusRawTrailingText; + } + } + else + { + rawTokenText = rawTokenTextPlusRawTrailingText; + } + + // FIX: (InterpolateValueFromKernel) bool persistent = false; + + foreach (var annotation in ParseInputProperties(rawTokenText)) + { + switch (annotation.key) + { + case "prompt": + prompt = annotation.value; + break; + case "valueName": + valueName ??= annotation.value; + break; + case "save": + // FIX: (InterpolateValueFromKernel) persistent = true; + break; + case "type": + typeHint = annotation.value; + break; + default: + break; + } + } + var inputRequest = new RequestInput( valueName: valueName, prompt: prompt, - inputTypeHint: typeHint); + inputTypeHint: typeHint) + { + // Persistent = persistent + }; var requestInputResult = _kernel.RootKernel.SendAsync(inputRequest).GetAwaiter().GetResult(); @@ -553,16 +600,90 @@ static bool ContainsInvalidCharactersForValueReference(ReadOnlySpan tokenT { var c = tokenToReplace[i]; - if (c is '\\' or '/') + if (c is '\\' or '/') { return true; } } - + return false; } } + private static IEnumerable<(string key, string value)> ParseInputProperties(string input) + { + int? quoteStartIndex = null; + int currentAnnotationStartIndex = 0; + + if (input.StartsWith("@input:")) + { + input = input["@input:".Length ..]; + } + else if (input.StartsWith("@password:")) + { + input = input["@password:".Length ..]; + } + + for (var i = 0; i < input.Length; i++) + { + var c = input[i]; + + if (c == '"') + { + if (quoteStartIndex is { } start) + { + var value = input[(start + 1) .. i]; + yield return GetPromptOrFieldName(value); + } + else + { + quoteStartIndex = i; + } + + currentAnnotationStartIndex = i + 1; + } + else if (c == ',' || i == input.Length - 1) + { + var annotation = input[currentAnnotationStartIndex..i]; + + if (quoteStartIndex is null) + { + yield return GetPromptOrFieldName(annotation); + } + else + { + var keyAndValue = annotation.Split('='); + + if (annotation.Length > 0) + { + if (keyAndValue.Length == 1) + { + yield return (annotation, null); + } + else + { + yield return (keyAndValue[0], keyAndValue[1]); + } + } + } + + currentAnnotationStartIndex = i + 1; + } + } + + static (string key, string value) GetPromptOrFieldName(string value) + { + if (value.Contains(" ")) + { + return ("prompt", value); + } + else + { + return ("valueName", value); + } + } + } + private string GetTypeHint(Symbol symbol) { string hint; diff --git a/src/Microsoft.DotNet.Interactive/PasswordString.cs b/src/Microsoft.DotNet.Interactive/PasswordString.cs index b54fd6a345..d4935c8af4 100644 --- a/src/Microsoft.DotNet.Interactive/PasswordString.cs +++ b/src/Microsoft.DotNet.Interactive/PasswordString.cs @@ -1,7 +1,6 @@ // Copyright (c) .NET Foundation and contributors. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. -using Microsoft.DotNet.Interactive.Formatting; using System.Text.Json.Serialization; namespace Microsoft.DotNet.Interactive; diff --git a/src/dotnet-interactive/KernelExtensionLoader.cs b/src/dotnet-interactive/KernelExtensionLoader.cs index 3fe3334b39..fd50bcbb4c 100644 --- a/src/dotnet-interactive/KernelExtensionLoader.cs +++ b/src/dotnet-interactive/KernelExtensionLoader.cs @@ -3,15 +3,12 @@ using System; using System.Collections.Concurrent; -using System.Collections.Generic; using System.IO; using System.Reactive.Linq; -using System.Reflection; using System.Runtime.Loader; using System.Threading.Tasks; using Microsoft.DotNet.Interactive.Events; using Microsoft.DotNet.Interactive.Telemetry; -using Pocket; namespace Microsoft.DotNet.Interactive.App;