[WIP] Add the HTTP REPL #468

mlorbetske · 2018-07-29T21:38:09Z

Mainly looking for verification that the build process changes to include the HTTP REPL are correct and that style guidelines are being met

Before merge:

More tests
Cleanup help

natemcmaster

A few comments about infrastructure. I didn't read the code. 7k lines is a bit much to bite off in one chunk. Is someone else reviewing the actual implementation here?

natemcmaster · 2018-07-30T22:43:42Z

build/dependencies.props

    <NETStandardLibrary20PackageVersion>2.0.3</NETStandardLibrary20PackageVersion>
    <SystemDataSqlClientPackageVersion>4.5.1</SystemDataSqlClientPackageVersion>
    <SystemSecurityCryptographyCngPackageVersion>4.5.0</SystemSecurityCryptographyCngPackageVersion>
+    <CommandLine_NewtonsoftJsonPackageVersion>10.0.1</CommandLine_NewtonsoftJsonPackageVersion>


Any particular reasonly to use 10? The rest of the runtime uses 11.

I don't believe there was a particular reason, i'll try bumping to the latest patch of 11

natemcmaster · 2018-07-30T22:43:54Z

src/Microsoft.HttpRepl/AggregateDirectoryStructure.cs

@@ -0,0 +1,38 @@
+using System;


Add the copyright header to all .cs files.

natemcmaster · 2018-07-30T22:47:51Z

src/Microsoft.Repl/Microsoft.Repl.csproj

+
+  <PropertyGroup>
+    <TargetFramework>netcoreapp2.2</TargetFramework>
+    <Description>A framework for creating REPLs in .NET Core.</Description>


Is this meant to be a package that is distributed to NuGet.org for others to use? If so, there are a few more infrastructure hoops to jump through.

Yep, intended to be distributed via NuGet

We talked in person yesterday. Let's not ship Microsoft.Repl.dll as a package yet. The demand is unclear, and we don't want to be on the hook to support the package.

You can disable producing a nupkg by adding <IsPackable>false</IsPackable> to this .csproj file.

natemcmaster · 2018-07-30T22:51:26Z

src/Microsoft.HttpRepl/Microsoft.HttpRepl.csproj

+
+  <ItemGroup>
+    <SignedPackageFile Include="tools/$(TargetFramework)/any/Microsoft.Repl.dll" Certificate="$(AssemblySigningCertName)" />
+    <SignedPackageFile Include="tools/$(TargetFramework)/any/Newtonsoft.Json.dll" Certificate="$(AssemblySigningCertName)" />


Can you change third party binaries to use Certificate="Microsoft3rdPartyAppComponentDual". If it uses $(AssemblySigningCertName), it would cause us to sign JSON.NET with the Microsoft cert, which is wrong. I see dotnet-user-secrets.csproj also has this wrong. We're not actually using this metadata yet, but will be in the next few weeks.

natemcmaster · 2018-07-30T22:52:27Z

src/Microsoft.HttpRepl/Microsoft.HttpRepl.csproj

+    <OutputType>Exe</OutputType>
+    <TargetFramework>netcoreapp2.2</TargetFramework>
+    <PackAsTool>true</PackAsTool>
+    <AssemblyName>dotnet-httprepl</AssemblyName>


Is this shipping as a package to nuget.org, a bundled command in the CLI, or both?

As far as I know, only distribution is planned for NuGet.org, not as a bundled tool with the CLI, but that's really a question for @glennc

The plan right now is only to ship it on NuGet.org, I expect us to re-examine that decision at some point though.

Bump version of JSON.NET to 11.0.2

…pComponentDual

natemcmaster

I got through like half of the code. Here are some of the things I noticed so far. Some questions, a few pieces of feedback.

It might be nice to set down with Glenn and go over the design a little more. I'm a bit fuzzy on the requirements for the project w.r.t swagger, graphql, interaction with oauth, etc.

natemcmaster · 2018-08-02T19:41:14Z

build/dependencies.props

    <NETStandardLibrary20PackageVersion>2.0.3</NETStandardLibrary20PackageVersion>
    <SystemDataSqlClientPackageVersion>4.5.1</SystemDataSqlClientPackageVersion>
    <SystemSecurityCryptographyCngPackageVersion>4.5.0</SystemSecurityCryptographyCngPackageVersion>
+    <CommandLine_NewtonsoftJsonPackageVersion>11.0.2</CommandLine_NewtonsoftJsonPackageVersion>


Can you change the variable name to NewtonsoftJsonPackageVersion? This will align it with the rest of our automation for managing dependencies.

natemcmaster · 2018-08-02T19:49:20Z

src/Microsoft.Repl/Input/KeyHandlers.cs

+            inputManager.RegisterKeyHandler(ConsoleKey.Tab, Tab);
+
+            //Input manipulation
+            inputManager.RegisterKeyHandler(ConsoleKey.Escape, Escape);


Have you considered also adding other common bash/zsh key mappings? e.g. here are some common ones

CTRL+U - clear input (equivalent to Escape in this mapping)

CTRL+A - go to beginning of input (equivalent to Home)

CTRL+E - go to endof input (equivalent to End)

natemcmaster · 2018-08-02T19:51:40Z

src/Microsoft.HttpRepl/Program.cs

+                shell.ShellState.InputManager.SetInput(shell.ShellState, $"set base \"{args[0]}\"");
+                await shell.ShellState.CommandDispatcher.ExecuteCommandAsync(shell.ShellState, CancellationToken.None).ConfigureAwait(false);
+            }
+            Task result = shell.RunAsync(source.Token);


Would it make sense to check Console.IsOutputRedirected and exit with an error before launching the shell? I'm assuming piping the repl should never work.

natemcmaster · 2018-08-02T19:53:00Z

src/Microsoft.HttpRepl/Program.cs

+            CancellationTokenSource source = new CancellationTokenSource();
+            var shell = new Shell(dispatcher);
+            shell.ShellState.ConsoleManager.AddBreakHandler(() => source.Cancel());
+            if (args.Length > 0)


IMO we should support a --help option which prints usage and exits. This is a standard for almost every dotnet command.

natemcmaster · 2018-08-02T19:57:04Z

src/Microsoft.HttpRepl/Preferences/WellKnownPreference.cs

+            }
+        }
+
+        #region JSON


I generally dislike regions, so if you don't feel strongly about this, would be nice to remove. But I'm also not a militant anti-region-legion-er so I'll let it slide.

I'm generally pretty anti-region as well, but wanted these out of the way as I was adding more settings, will remove the region

natemcmaster · 2018-08-02T20:08:37Z

src/Microsoft.HttpRepl/OpenApi/PointerUtil.cs

+            return toResolve;
+        }
+
+        //public static async Task<JToken> ResolvePointerAsync(this JToken root, HttpClient client, string pointer)


Is there a TODO for this block?

natemcmaster · 2018-08-02T20:11:28Z

src/Microsoft.Repl/Commanding/DefaultCommandDispatcher.cs

+                    }
+                }
+
+                shellState.ConsoleManager.Error.WriteLine("No matching command found".Red().Bold());


Consider also printing to output a message like "Execute 'help' to see available commands".

natemcmaster · 2018-08-02T20:56:29Z

src/Microsoft.HttpRepl/Commands/BaseHttpCommand.cs

+                || contentType.EndsWith("-JAVASCRIPT", StringComparison.OrdinalIgnoreCase)
+                || contentType.EndsWith("+JAVASCRIPT", StringComparison.OrdinalIgnoreCase))
+            {
+                if (await FormatJsonAsync(programState, consoleManager, content, bodyFileWriter))


Can someone disable formatting? It seems like it may be useful to see the actual response, unmodified, with the option to have the repl
pretty-print.

I'm also a little concerned about how this will behave if someone uses the repl to hit an endpoint that dumps a large XML or JSON response.
Hopefully that's not super common, but when someone inevitably downloads a 1000 line JSON file, it's going to bombard console output. Have
we considered using a pager for large responses?

natemcmaster · 2018-08-02T20:59:20Z

src/Microsoft.HttpRepl/Commands/BaseHttpCommand.cs

+                                break;
+                            }
+
+                            string str = new string(buffer.Span.Slice(0, readTask.Result));


You may run into trouble here with encoding and charsets.

I had thought of this too, with this being the streaming scenario & the end of the chunk being determined by the server, no good ways around it are coming to mind.

Pagination hasn't been dealt with yet mac keybindings may be able to be simplified, pending some additional verification Fix 3rd party signing cert Bind a few common bash/zsh mappings Check for output redirection Make package version variable name consistent Add --help option for the command line Remove regions Remove old pointer resolution code Make command not found message more friendly Fix issue where base address was required for requests to absolute URIs Add options to suppress response formatting and streaming Turn off packing for the REPL project

natemcmaster

Thanks for updating in response to PR feedback. You already called out the last concern I have - low test coverage. Before merging, can you file an issue to follow-up on this?

No additional engineering concerns right now. I'll let @glennc sign off on experience review.

natemcmaster · 2018-08-03T17:21:02Z

src/Microsoft.Repl/Commanding/DefaultCommandDispatcher.cs

@@ -156,6 +156,7 @@ private async Task ExecuteCommandInternalAsync(IShellState shellState, Cancellat
                }

                shellState.ConsoleManager.Error.WriteLine("No matching command found".Red().Bold());
+                shellState.ConsoleManager.Error.WriteLine("Execute 'help' to se available commands".Red().Bold());


Typo: "to see"

natemcmaster · 2018-08-03T17:25:04Z

NuGetPackageVerifier.json

      "Microsoft.AspNetCore.DeveloperCertificates.XPlat": {
        "Exclusions": {
          "DOC_MISSING": {
            "lib/netcoreapp2.2/Microsoft.AspNetCore.DeveloperCertificates.XPlat.dll": "Docs not required to shipoob package"
          }
        }
+      },
+      "Microsoft.Repl": {


Removing this should make builds happy :)

Default to requerying swagger for each exec Auto-set the content-type if not yet set for requests based on swagger Fix swagger UI command Make error and warning colors configurable Allow aliases to be specified in structured input based commands Have delete not require a body Show allowed methods for . and .. Show verbs on the announced path after CD Issue a request on set base and warn if there's a socket error

mlorbetske · 2018-08-07T16:40:03Z

I believe we're waiting on some additional changes from @glennc around some of the experiences before merging

mlorbetske · 2018-08-28T19:16:18Z

Merging per Glenn

mlorbetske requested a review from natemcmaster July 29, 2018 21:38

natemcmaster reviewed Jul 30, 2018

View reviewed changes

mlorbetske added 2 commits August 1, 2018 12:37

Initial commit of the HTTP REPL

89ab0cf

Add license headers to all files

7f8f210

Bump version of JSON.NET to 11.0.2

mlorbetske force-pushed the mlorbe/AddHttpRepl branch from 4923057 to 7f8f210 Compare August 1, 2018 19:40

Switch signing certificate for Newtonsoft.JSON to Microsoft3rdPartyAp…

7487da1

…pComponentDual

natemcmaster reviewed Aug 2, 2018

View reviewed changes

natemcmaster reviewed Aug 3, 2018

View reviewed changes

mlorbetske added 4 commits August 3, 2018 12:27

Use netcoreapp2.1 instead of 2.2

8a9b407

Remove unnecessary exclusions from NuGetPackageVerifier.json

b242704

Attempt to fix exclusions in NuGetPackageVerifier.json again

eb591fb

Fix incorrectly resolved cross-document pointers

b53d50f

mlorbetske merged commit 624c2ec into aspnet:release/2.2 Aug 28, 2018

[WIP] Add the HTTP REPL #468

[WIP] Add the HTTP REPL #468

Uh oh!

Conversation

mlorbetske commented Jul 29, 2018

Uh oh!

natemcmaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natemcmaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natemcmaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlorbetske commented Aug 7, 2018

Uh oh!

mlorbetske commented Aug 28, 2018

Uh oh!

Uh oh!