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.

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

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

Merged
merged 6 commits into from
Sep 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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: "/" } or {Text: "'" } or { 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,22 @@ 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 } or null)
{
var currentToken = CurrentToken;
if (currentToken is not null && (currentToken is not ({ 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))
{
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 +694,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
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public void Add(HttpRequestSeparatorNode separatorNode)
var embeddedExpressionNodes = node.ValueNode.ChildNodes.OfType<HttpEmbeddedExpressionNode>();
if (!embeddedExpressionNodes.Any())
{
foundVariableValues.Add(node.DeclarationNode.VariableName, node.ValueNode.Text);
foundVariableValues[node.DeclarationNode.VariableName] = node.ValueNode.Text;
declaredVariables[node.DeclarationNode.VariableName] = new DeclaredVariable(node.DeclarationNode.VariableName, node.ValueNode.Text, HttpBindingResult<string>.Success(Text));
}
else
Expand Down
165 changes: 163 additions & 2 deletions src/Microsoft.DotNet.Interactive.Http.Tests/HttpKernelTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Linq.Expressions;
using System.Net;
using System.Net.Http;
using System.Net.Http.Json;
using System.Reflection;
using System.Text;
using System.Threading.Tasks;
Expand Down Expand Up @@ -111,6 +112,65 @@ public async Task can_handle_multiple_request_in_a_single_submission()
}

[Fact]
public async Task binding_for_variables_that_have_been_sent_work_well()
{
HttpRequestMessage request = null;
var handler = new InterceptingHttpMessageHandler((message, _) =>
{
request = message;
var response = new HttpResponseMessage(HttpStatusCode.OK);
return Task.FromResult(response);
});
var client = new HttpClient(handler);
using var kernel = new HttpKernel(client: client);

using var _ = new AssertionScope();

var result = await kernel.SendAsync(new SendValue("my_host", "my.host.com"));
result.Events.Should().NotContainErrors();

result = await kernel.SendAsync(new SubmitCode("get https://{{my_host}}:1200/endpoint"));
result.Events.Should().NotContainErrors();

request.RequestUri.Should().Be("https://my.host.com:1200/endpoint");
}

[Fact]
public async Task send_request_and_variables_are_saved()
{
HttpRequestMessage request = null;
var handler = new InterceptingHttpMessageHandler((message, _) =>
{
request = message;
var response = new HttpResponseMessage(HttpStatusCode.OK);
return Task.FromResult(response);
});
var client = new HttpClient(handler);
using var kernel = new HttpKernel(client: client);

using var _ = new AssertionScope();

var code = """
@host=https://httpbin.org

Get {{host}}
""";

await kernel.SendAsync(new SubmitCode(code));


var result = await kernel.SendAsync(new RequestValueInfos());

result.Events.Should().NotContainErrors();
var valueInfo = result.Events.Should().ContainSingle<ValueInfosProduced>()
.Which
.ValueInfos.Should().ContainSingle()
.Which;
valueInfo.Name.Should().Be("host");
valueInfo.FormattedValue.Should().BeEquivalentTo(new FormattedValue(PlainTextSummaryFormatter.MimeType, "https://httpbin.org"));
}

[Fact]
public async Task can_set_request_headers()
{
HttpRequestMessage request = null;
Expand Down Expand Up @@ -2196,8 +2256,26 @@ public async Task responses_to_named_requests_can_be_accessed_as_symbols_in_late
// 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);
const int ContentByteLengthThreshold = 100;

var largeResponseHandler = new InterceptingHttpMessageHandler((message, _) =>
{
var response = new HttpResponseMessage(HttpStatusCode.OK);
response.RequestMessage = message;
var builder = new StringBuilder();
for (int i = 0; i < ContentByteLengthThreshold + 1; ++i)
{
builder.Append('a');
}
response.Content = JsonContent.Create(builder.ToString());
return Task.FromResult(response);
});

var client = new HttpClient(largeResponseHandler);
using var kernel = new HttpKernel("http", client, contentByteLengthThreshold: ContentByteLengthThreshold);

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

var code = """
@baseUrl = https://httpbin.org/anything
Expand Down Expand Up @@ -2233,6 +2311,89 @@ 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", "Wake up to WonderWidgets!")]
[InlineData("json.response.body.$.slideshow.slides.type", "all")]
public async Task json_named_requests_with_sub_routes_can_be_accessed_correctly(string path, string end)
{
// 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 responseHandler = new InterceptingHttpMessageHandler((message, _) =>
{
var response = new HttpResponseMessage(HttpStatusCode.OK);
response.RequestMessage = message;
var contentString = @"
{
""slideshow"": {
""author"": ""Yours Truly"",
""date"": ""date of publication"",
""slides"": [
{
""title"": ""Wake up to WonderWidgets!"",
""type"": ""all""
},
{
""items"": [
""Why <em>WonderWidgets</em> are great"",
""Who <em>buys</em> WonderWidgets""
],
""title"": ""Overview"",
""type"": ""all""
}
],
""title"": ""Sample Slide Show""
}
}
";
response.Content = new StringContent(contentString, Encoding.UTF8, "application/json");
return Task.FromResult(response);
});
var client = new HttpClient(responseHandler);
// var client = new HttpClient();
using var kernel = new HttpKernel("http", 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

var returnValue = secondResult.Events.OfType<ReturnValueProduced>().First();

var response = (HttpResponse)returnValue.Value;

response.Request.Content.Raw.Split(":").Last().TrimEnd("\r\n}".ToCharArray()).Should().Be(end);

}

[Theory]
[InlineData("login.response.$")]
[InlineData("login.response.//")]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -283,5 +283,50 @@ public void spaces_after_variable_do_not_produce_diagnostics()

result.SyntaxTree.RootNode.ChildNodes.Count().Should().Be(1);
}

[Fact]
public void single_quotes_in_variable_values_are_supported()
{
var result = Parse(
"""

@host='https://httpbin.org'
"""
);

var variables = result.SyntaxTree.RootNode.TryGetDeclaredVariables().declaredVariables;
variables.Should().Contain(n => n.Key == "host").Which.Value.Should().BeOfType<DeclaredVariable>().Which.Value.Should().Be("'https://httpbin.org'");

}

[Fact]
public void double_quotes_in_variable_values_are_supported()
{
var result = Parse(
"""

@host="https://httpbin.org"
"""
);

var variables = result.SyntaxTree.RootNode.TryGetDeclaredVariables().declaredVariables;
variables.Should().Contain(n => n.Key == "host").Which.Value.Should().BeOfType<DeclaredVariable>().Which.Value.Should().Be("\"https://httpbin.org\"");
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a little surprising to me that the quotes are part of the variable value.

Maybe related, are spaces in variable values supported like this then?

@blah = one two three


}

[Fact]
public void spaces_in_variable_values_are_supported()
{
var result = Parse(
"""

@host = one two three
"""
);

var variables = result.SyntaxTree.RootNode.TryGetDeclaredVariables().declaredVariables;
variables.Should().Contain(n => n.Key == "host").Which.Value.Should().BeOfType<DeclaredVariable>().Which.Value.Should().Be("one two three");

}
}
}
51 changes: 33 additions & 18 deletions src/Microsoft.DotNet.Interactive.Http/HttpKernel.cs
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,31 +125,45 @@ 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))
_variables[valueName] = value;
return Task.CompletedTask;
}

async Task IKernelCommandHandler<SubmitCode>.HandleAsync(SubmitCode command, KernelInvocationContext context)
{
var parseResult = HttpRequestParser.Parse(command.Code);
var requestNodes = parseResult.SyntaxTree.RootNode.ChildNodes.OfType<HttpRequestNode>();
//var lastSpan = requestNodes.Select(n => n.Span).LastOrDefault();



if (command.Parameters.TryGetValue("Document", out var doc))
{
var variable = (HttpVariableDeclarationAndAssignmentNode) value;
if(variable.ValueNode is not null)
var parsedDoc = HttpRequestParser.Parse(doc);
var lastSpan = parsedDoc.SyntaxTree.RootNode.ChildNodes
.OfType<HttpRequestNode>()
.FirstOrDefault(n => n.Text == requestNodes.Last().Text)?.Span;
if(lastSpan != null)
{
var bindingResult = variable.ValueNode.TryGetValue(BindExpressionValues);
if (bindingResult.IsSuccessful && bindingResult.Value is not null)
var docVariableNodes = parsedDoc.SyntaxTree.RootNode.ChildNodes.OfType<HttpVariableDeclarationAndAssignmentNode>();
var docVariableNames = docVariableNodes.Where(n => n.Span.Start < lastSpan?.Start).Select(n => n.DeclarationNode?.VariableName).ToHashSet();
foreach (DeclaredVariable dv in parsedDoc.SyntaxTree.RootNode.TryGetDeclaredVariables(BindExpressionValues).declaredVariables.Values)
{
_variables[valueName] = bindingResult.Value;
if (docVariableNames.Contains(dv.Name))
{
_variables[dv.Name] = dv.Value;
}
}
}

}
}
else
{
_variables[valueName] = value;
foreach (DeclaredVariable dv in parseResult.SyntaxTree.RootNode.TryGetDeclaredVariables(BindExpressionValues).declaredVariables.Values)
{
_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.

}
}
return Task.CompletedTask;
}

async Task IKernelCommandHandler<SubmitCode>.HandleAsync(SubmitCode command, KernelInvocationContext context)
{
var parseResult = HttpRequestParser.Parse(command.Code);

var requestNodes = parseResult.SyntaxTree.RootNode.ChildNodes.OfType<HttpRequestNode>();

var httpBoundResults = new List<HttpBindingResult<HttpRequestMessage>>();
var httpNamedBoundResults = new List<(HttpRequestNode requestNode, HttpBindingResult<HttpRequestMessage> bindingResult)>();
Expand All @@ -168,7 +183,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
Loading