Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support for saving named requests in variables and improved json parsing #3642

Merged
merged 6 commits into from
Sep 11, 2024
Merged

Support for saving named requests in variables and improved json parsing #3642

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d905db5
Support for saving named requests in variables and improved json parsing
bleaphar Aug 16, 2024
52d15f1
Changes to tests, saving current document, and restructuring json par…
bleaphar Aug 16, 2024
c00c14a
Support for variable changes
bleaphar Sep 6, 2024
df41a52
Removal of comments and changes to named requests
bleaphar Sep 10, 2024
6da1db5
Addition of kernel tests
bleaphar Sep 10, 2024
5ca3d59
Addition of test
bleaphar Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 15 additions & 19 deletions src/Microsoft.DotNet.Interactive.Http.Parsing/Parsing/HttpRequestParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ private static void AddCommentsIfAny(
{
if (node is null)
{
if (CurrentToken is { Kind: TokenKind.Word } or ({ Kind: TokenKind.Punctuation} and { Text: "/"}))
if (CurrentToken is { Kind: TokenKind.Word } or ({ Kind: TokenKind.Punctuation } and { Text: "/" }))
{
node = new HttpVariableValueNode(_sourceText, _syntaxTree);

Expand Down Expand Up @@ -268,7 +268,7 @@ private T ParseLeadingWhitespaceAndComments<T>(T node) where T : HttpSyntaxNode
foreach (var commentNode in ParseComments())
{
node.Add(commentNode, addBefore: true);

}
}
else
Expand Down Expand Up @@ -668,25 +668,21 @@ private HttpNamedRequestNameNode ParseNamedRequestNameNode()
node.AddDiagnostic(diagnostic);
}
bool wordParsedOnce = false;
while (MoreTokens() && CurrentToken is not {Kind: TokenKind.NewLine })
{
while (MoreTokens() && CurrentToken is not { Kind: TokenKind.NewLine })
{

if (CurrentToken is not null && (!(CurrentToken is { Kind: TokenKind.Word or TokenKind.Whitespace} or { Text: "_" or "@" or "."}) || CurrentToken is {Kind: TokenKind.Word } && wordParsedOnce))
{
var diagnostic = CurrentToken.CreateDiagnostic(HttpDiagnostics.InvalidNamedRequestName());
node.AddDiagnostic(diagnostic);
wordParsedOnce = false;
}
if (CurrentToken is not null && (!(CurrentToken is { Kind: TokenKind.Word or TokenKind.Whitespace } or { Text: "_" or "@" or "." }) || CurrentToken is { Kind: TokenKind.Word } && wordParsedOnce))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (CurrentToken is not null && (!(CurrentToken is { Kind: TokenKind.Word or TokenKind.Whitespace } or { Text: "_" or "@" or "." }) || CurrentToken is { Kind: TokenKind.Word } && wordParsedOnce))
if (CurrentToken is not null && (CurrentToken is not ({ Kind: TokenKind.Word or TokenKind.Whitespace } or { Text: "_" or "@" or "." }) || CurrentToken is { Kind: TokenKind.Word } && wordParsedOnce))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, a CurrentToken is { ... } pattern won't match null, so the initial null check shouldn't be necessary.

{
var diagnostic = CurrentToken.CreateDiagnostic(HttpDiagnostics.InvalidNamedRequestName());
node.AddDiagnostic(diagnostic);
wordParsedOnce = false;
}

if (CurrentToken is { Kind: TokenKind.Word })
{
wordParsedOnce = true;
}
ConsumeCurrentTokenInto(node);
/*if (CurrentToken is { Kind: TokenKind.Whitespace })
if (CurrentToken is { Kind: TokenKind.Word })
{
ParseTrailingWhitespace(node, stopBeforeNewLine: true);
}*/
bleaphar marked this conversation as resolved.
Show resolved Hide resolved
wordParsedOnce = true;
}
ConsumeCurrentTokenInto(node);

}

Expand All @@ -697,7 +693,7 @@ private bool isCommentNamedRequest()
{
var nextTokenIndicatesName = CurrentTokenPlus(1) != null ? CurrentTokenPlus(1)!.Text.StartsWith("name") : false;
return (CurrentToken is { Text: "@" } &&
nextTokenIndicatesName &&
nextTokenIndicatesName &&
CurrentTokenPlus(2) is { Kind: TokenKind.Whitespace }
);
}
Expand Down
45 changes: 45 additions & 0 deletions src/Microsoft.DotNet.Interactive.Http.Tests/HttpKernelTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2233,6 +2233,51 @@ public async Task responses_to_named_requests_can_be_accessed_as_symbols_in_late
secondResult.Events.Should().NotContainErrors();
}

[Theory]
[InlineData("json.response.body.$.slideshow.slides.title")]
[InlineData("json.response.body.$.slideshow.slides.type")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we relying on httpbin.org to return a json that looks like this? I think it would be better to use a mock client with our own handler where we can provide our own json response (like we do in some other tests).

Otherwise, we could end up having to deal with failed builds in CI if httpbin.org happens to be down or if the format for the response ends up changing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would also be nice to see what the response shape looks like in the test so that it is easier to understand what the test is validating...

public async Task json_named_requests_with_sub_routes_can_be_accessed_correctly(string path)
{
// Request Variables
// Request variables are similar to file variables in some aspects like scope and definition location.However, they have some obvious differences.The definition syntax of request variables is just like a single-line comment, and follows // @name requestName or # @name requestName just before the desired request url.

var client = new HttpClient();
using var kernel = new HttpKernel(client: client);

var code = """
@baseUrl = https://httpbin.org/json

# @name json
GET {{baseUrl}}
Content-Type: application/json

###
""";

var result = await kernel.SendAsync(new SubmitCode(code));
result.Events.Should().NotContainErrors();

var secondCode = $$$"""

@pathContents = {{{{{path}}}}}


# @name createComment
POST https://example.com/api/comments HTTP/1.1
Content-Type: application/json

{
"path" : {{pathContents}}
}

###
""";

var secondResult = await kernel.SendAsync(new SubmitCode(secondCode));

secondResult.Events.Should().NotContainErrors();
bleaphar marked this conversation as resolved.
Show resolved Hide resolved
}

[Theory]
[InlineData("login.response.$")]
[InlineData("login.response.//")]
Expand Down
31 changes: 13 additions & 18 deletions src/Microsoft.DotNet.Interactive.Http/HttpKernel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
using Microsoft.DotNet.Interactive.Formatting;
using Microsoft.DotNet.Interactive.Http.Parsing;
using Microsoft.DotNet.Interactive.ValueSharing;
using Microsoft.DotNet.Interactive.Http.Parsing.Parsing;

namespace Microsoft.DotNet.Interactive.Http;

Expand Down Expand Up @@ -124,28 +125,22 @@ async Task IKernelCommandHandler<SendValue>.HandleAsync(SendValue command, Kerne

private Task SetValueAsync(string valueName, object value, Type? declaredType = null)
shyamnamboodiripad marked this conversation as resolved.
Show resolved Hide resolved
{
if(value is (HttpVariableDeclarationAndAssignmentNode))
{
var variable = (HttpVariableDeclarationAndAssignmentNode) value;
if(variable.ValueNode is not null)
{
var bindingResult = variable.ValueNode.TryGetValue(BindExpressionValues);
if (bindingResult.IsSuccessful && bindingResult.Value is not null)
{
_variables[valueName] = bindingResult.Value;
}
}

}
else
{
_variables[valueName] = value;
}
_variables[valueName] = value;
return Task.CompletedTask;
}

async Task IKernelCommandHandler<SubmitCode>.HandleAsync(SubmitCode command, KernelInvocationContext context)
{
string? doc = "";
bleaphar marked this conversation as resolved.
Show resolved Hide resolved
if (command.Parameters.TryGetValue("Document", out doc))
bleaphar marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are we planning to make this would work in the case of notebooks in VS Code or the dotnet-repl tool? Would we need changes for those clients to repeat the notebook cell contents in both 'Document' parameter as well as in SubmitCode.Code to ensure that any variables included in the code are parsed and bound?

Do we need code in the http kernel above to also parse and bind variables present in the SubmitCode.Code (in addition to the Document parameter) to support the notebooks and dotnet-repl scenarios?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a property to the kernel that tracks the current document and if there are any changes from it then we'll go ahead with the re-parse

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has the potential to introduce bugs where the kernel state gets out of sync with the editor state. What's the performance impact of resending and reparsing the whole document?

{
var parsedDoc = HttpRequestParser.Parse(doc);
foreach (DeclaredVariable dv in parsedDoc.SyntaxTree.RootNode.TryGetDeclaredVariables(BindExpressionValues).declaredVariables.Values)
Copy link
Contributor

@shyamnamboodiripad shyamnamboodiripad Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above approach means that the user could see unexpected delays when executing requests in a large document (since we would need to bind all variables including some that are completely unrelated to the user's request).

An alternate approach may be to support a workflow where the code in the SubmitCode command can either be the entire document or a single request. We could then include a parameter to indicate which case it was - for example the parameter could be called Execute. If the entire document is being passed, we could set Execute to false and the kernel could parse and bind all the file level variables without executing any request. If Execute is set to true (or omitted as will be the case in notebooks and dotnet-repl), we could execute the specified request.

One potential advantage of this approach would be that as the user makes changes to the document, we could asynchronously pass the entire document down to the kernel with Execute set to false whenever there is a long enough pause in the user's typing. This would allow the kernel to parse and bind the variables in the background after the user has made the file changes but before the user wants to execute a request. Then when the user clicks on Send request, we could pass down a SubmitCode command with just the request that was selected for execution with Execute set to true (or with the parameter omitted...). This could reduce any delays caused due to binding in larger documents.

There may also other ways in which something like above could be implemented...

Copy link
Contributor

@shyamnamboodiripad shyamnamboodiripad Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whichever approach we decide to go with should also work in the case of notebooks and dotnet-repl. So I think we would need to choose an approach where the 'default' implementation (i.e. where no additional parameters are specified) naturally supports http cells in notebooks and dotnet-repl...

Copy link
Contributor

@shyamnamboodiripad shyamnamboodiripad Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bleaphar This may be worth a discussion. Lets chat offline once you are back.

I don't think this can be resolved merely by checking whether the file has changed since even minor whitespace changes to one character in the file could end up resulting in a long delay for the subsequent request. And storing and comparing contents of large files may also be problematic from a memory perspective. It may be better to hold on to a checksum / hash of some sort...

In general, and especially in large files, we need ways to
a) amortize some of the cost of parsing and binding so that we don't pay all this cost at the point when user wants to execute a request and
b) avoid redundant binding (and if possible, even avoid redundant parsing) when large portion of the file stays unchanged

For a, we would need to do something like what I mentioned above where we kick off parsing and binding in the background soon after the user stops typing so that we get a head start on parsing and binding before the user executes a request.

For b, it will be hard to avoid redundant parsing (unless we can do something like Roslyn does for C# by incrementally parsing just the lines that changed and fixing up the syntax tree to reflect the changes). But there may be ways to avoid redundant binding (e.g. check if the changed lines contain any variable declarations and only bind new variables that appear in changed code).

Otherwise, users could end up seeing large delays when executing requests and we could end up seeing feedback like the below one even in files that are not as large as the one the user mentions in the below case -

https://developercommunity.visualstudio.com/t/Big-HTTP-file-is-not-interpreted-by-Visu/10717885

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments.

The above approach means that the user could see unexpected delays when executing requests in a large document (since we would need to bind all variables including some that are completely unrelated to the user's request).

We should measure this before making a design decision. I strongly suspect that the difference is not perceptible and that, to the extent that it is, serialization is a larger overhead than redundant parsing. But we should measure it.

We could then include a parameter to indicate which case it was - for example the parameter could be called Execute.

The intent is different so I would suggest this is no longer a SubmitCode. Does the RequestDiagnostics command not already do what's needed here?

{
_variables[dv.Name] = dv.Value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something to think about... There is a potential for _variables to retain bunch of stale variables / named requests over time. Not super high pri - but especially for named requests which can have large payloads, it may be nice to clean up stale entries...

Now that we have a specific code path that re-binds all file-level variables, we may be able to drop all old file-level variables whenever we get a new request to parse and bind the document.

To do this, we would need to separate out the dictionaries for file-level variables, named requests and the remaining (.env) variables. The file-level dictionary can be cleared out whenever we get a new request to bind the document. Any named requests that no longer exist in the document (based on the new syntax tree) could also be cleared out from the dictionary for named requests at this point. But for named requests that still exist in the document, we would need to preserve the old responses.

The dictionary for .env vars would be harder to clean up since we don't have a way to know when .env files change at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would measure this before complicating the design. Separate dictionaries will increase cyclomatic complexity and the potential for bugs.

An alternative might be to store each variable with some metadata including a scope or origin concept, e.g. to differentiate variables whose values came from responses (maybe these are transient) versus those that came from a SendValue command (which the kernel shouldn't have a say in clearing.)

Consider this. In the typical REPL or notebook workflow, a single cell can be edited and resubmitted. Let's say I run this in the C# kernel:

var x = 123;

Then let's say I delete that code and run this:

Console.WriteLine(x);

It would be incorrect for the variable x to be cleared, and probably very surprising if the HTTP kernel had a different behavior.

It's very typical in REPL and notebook workflows for memory to grow over time, which is why restarting the kernel is a familiar gesture. But if we find there's a performance issue here that's significantly different from other use cases, I'd prefer to address it with an explicit user gesture.

}
}

var parseResult = HttpRequestParser.Parse(command.Code);

var requestNodes = parseResult.SyntaxTree.RootNode.ChildNodes.OfType<HttpRequestNode>();
Expand All @@ -168,7 +163,7 @@ async Task IKernelCommandHandler<SubmitCode>.HandleAsync(SubmitCode command, Ker
}
}


var diagnostics = httpBoundResults.SelectMany(n => n.Diagnostics).Concat(httpNamedBoundResults.SelectMany(n => n.bindingResult.Diagnostics)).ToArray();

PublishDiagnostics(context, command, diagnostics);
Expand Down
28 changes: 25 additions & 3 deletions src/Microsoft.DotNet.Interactive.Http/Model/HttpNamedRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.Reactive.Concurrency;
using System.Xml.XPath;
using System.Xml;
using Microsoft.CodeAnalysis.CSharp.Syntax;

namespace Microsoft.DotNet.Interactive.Http;

Expand Down Expand Up @@ -233,11 +234,32 @@ internal HttpNamedRequest(HttpRequestNode httpRequestNode, HttpResponse response
{
if (currentIndex + 1 == path.Length)
{
var result = responseJSON[path[currentIndex]];
return result?.ToString();
if(responseJSON.GetType() == typeof(JsonArray))
bleaphar marked this conversation as resolved.
Show resolved Hide resolved
{
var jsonArray = (JsonArray)responseJSON;
bleaphar marked this conversation as resolved.
Show resolved Hide resolved
return jsonArray.FirstOrDefault(n => n?[path[currentIndex]] != null)?.ToJsonString();
}
else if(responseJSON.GetType() == typeof(JsonObject))
bleaphar marked this conversation as resolved.
Show resolved Hide resolved
{
var jsonObject = (JsonObject)responseJSON;
return jsonObject[path[currentIndex]]?.ToString();
}
else
{
return null;
}
}

var newResponseJSON = responseJSON[path[currentIndex + 1]];
JsonNode? newResponseJSON = null;
try
{
newResponseJSON = responseJSON[path[currentIndex]];
}
catch (Exception)
bleaphar marked this conversation as resolved.
Show resolved Hide resolved
{
return null;
}

if (newResponseJSON is null)
{
return null;
Expand Down