update HttpRequestKernel to use new parser; support variable expressions in request body #3122
Conversation
src/Microsoft.DotNet.Interactive.HttpRequestParser/HttpSyntaxNode.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Interactive.HttpRequestParser/HttpSyntaxNode.cs
Outdated
Show resolved
Hide resolved
|
|
||
| public static AndWhichConstraint<ObjectAssertions, T> ContainSingle<T>( | ||
| this GenericCollectionAssertions<HttpSyntaxNodeOrToken> should) | ||
| where T : HttpSyntaxNode |
There was a problem hiding this comment.
Can we remove the overload on line 13 since HttpSyntaxNodeOrToken is a base type of HttpSyntaxNode (and since IReadOnlyList supports covariance)?
src/Microsoft.DotNet.Interactive.HttpRequestParser/HttpSyntaxNodeOrToken.cs
Outdated
Show resolved
Hide resolved
| /// <summary> | ||
| /// Gets the text of the current node, including trivia. | ||
| /// </summary> | ||
| public string FullText => SourceText.ToString(FullSpan); |
There was a problem hiding this comment.
It is a bit strange that FullSpan is available on the base HttpSyntaxNodeOrToken but FullText is not.
If we think that FullSpan and FullText are only meaningful for HttpSyntaxNode, perhaps we should also move FullSpan down to HttpSyntaxNode.
Alternatively, we can move FullText up to the base type so that a caller does not have to think about whether they are dealing a node or a token when they want to get its full text.
There was a problem hiding this comment.
This makes sense, but the meaning of the two is also different. On a token, Span really means the full span. FullSpan is used for things like FindNode and GrowSpan. Removing HttpSyntaxNodeOrToken.FullSpan results in needing to switch on the two types in a number of places.
There was a problem hiding this comment.
I see... Should we also move FullText to the base type then so that the API is consistent?
src/Microsoft.DotNet.Interactive.HttpRequest.Tests/ParserTests.Comments.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Interactive.HttpRequest.Tests/ParserTests.Comments.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Interactive.HttpRequest.Tests/ParserTests.Method.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Interactive.HttpRequest.Tests/ParserTests.Request.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Interactive.HttpRequest.Tests/ParserTests.Trivia.cs
Show resolved
Hide resolved
| @@ -70,10 +74,9 @@ internal class HttpRequestNode : HttpSyntaxNode | |||
| public HttpBindingResult<HttpRequestMessage> TryGetHttpRequestMessage(HttpBindingDelegate bind) | |||
| { | |||
| var request = new HttpRequestMessage(); | |||
| var diagnostics = new List<Diagnostic>(); | |||
| var success = true; | |||
| var diagnostics = new List<Diagnostic>(base.GetDiagnostics()); | |||
There was a problem hiding this comment.
The binding methods for the child nodes (such as TryGetUri and TryGetBody) should probably also call base.GetDiagnostics() to include syntax errors (and return false if there are any diagnostics with severity Error).
There was a problem hiding this comment.
I guess that could lead to some duplication at higher levels (unless we change the list to a hashset)...
There was a problem hiding this comment.
Yeah, I deliberately kept these concerns separate.
There was a problem hiding this comment.
Thinking more perhaps it may be better for the binding API calls to only return binding errors. The syntax errors are already available on the node which the caller is trying to bind.
This may be more flexible from an API standpoint and would also keep the behavior consistent regardless of which construct is being bound... Otherwise, it forces callers to reason about which construct they are binding and whether or not they should fetch the syntax diagnostics separately.
| diagnostics.AddRange(uriBindingResult.Diagnostics); | ||
| } | ||
|
|
||
| if (success) | ||
| var headers = | ||
| HeadersNode?.HeaderNodes.Select(h => new KeyValuePair<string, string>(h.NameNode.Text, h.ValueNode.Text)).ToArray() |
There was a problem hiding this comment.
The headers could also contain expressions. But I guess we will flesh that out in future PRs cc @bleaphar
| case "content-type": | ||
| if (request.Content is null) | ||
| { | ||
| request.Content = new StringContent(""); |
There was a problem hiding this comment.
Can this be done globally in the else clause for the if block on line 136 below instead (i.e. always set to empty string if a parsed body is empty)?
The null check for request.Content is also confusing since we have not touched request.Content above this line. Did you mean to check something else perhaps?
There was a problem hiding this comment.
No, I meant to check this. Coverage indicates that request.Content is null 100% of the time here. But I realized there's a bug in that this header can be get overridden when we set the Content again for the body below, so some refactoring is needed.
There was a problem hiding this comment.
Ah does the System.Net.Http API try to deduce and set the content-type header implicitly when Content is set below? That is certainly not very intuitive 😄
I agree this needs some refactoring... I think we can delete this if block for now as it is dead code.
There was a problem hiding this comment.
The code isn't dead code. 100% of tests enter the if block.
Ah does the System.Net.Http API try to deduce and set the content-type header implicitly when Content is set below?
It doesn't and there's no way it reliably could.
| .FirstOrDefault(n => n.IsSignificant); | ||
|
|
||
| var lastSignificantNodeOrToken = ChildNodesAndTokens | ||
| .LastOrDefault(n => n.IsSignificant); |
There was a problem hiding this comment.
I think we can improve this part by remembering the first and last significant children in fields as children are added inside Add below. That way we won't need to traverse all the child nodes here each time a child is added.
| var parsedRequests = new List<ParsedHttpRequest>(); | ||
|
|
||
| foreach (var (request, diagnostics) in InterpolateAndGetDiagnostics(requests)) | ||
| foreach (var expressionNode in parseResult.SyntaxTree.RootNode.DescendantNodesAndTokensAndSelf().OfType<HttpExpressionNode>()) |
There was a problem hiding this comment.
Ah why not just call parseResult.TryGetRequest() and get the diagnostics that way (and throw away the request in the success case)? It seems a bit strange to have to repeat the binding logic here + keeping the set of diagnostics the same for both code paths may prove cumbersome.
For example, we may add more specific diagnostics for bad URIs / bad header declations inside the .TryGetUri / TryGetHeaders code path. Some of those diagnostics may be reported in URIs / headers with no embedded expressions (but would still be diagnostics that are reported at bind-time). It would be difficult to replicate the same diagnostics over here since this code only tries to bind embedded expressions (but does not bind individual parts of the syntax).
There was a problem hiding this comment.
Some of those diagnostics may be reported in URIs / headers with no embedded expressions (but would still be diagnostics that are reported at bind-time).
The block before this (parseResult.GetDiagnostics()) is getting the syntactic diagnostics and this code is getting the symbolic diagnostics. My thinking was that keeping them as separate steps is useful in this code path (i.e. in the context of RequestDiagnostics) because the latter will sometimes include side effects such as UI prompts which we shouldn't trigger here.
There was a problem hiding this comment.
What I meant is that there could be more to the symbolic diagnostics beyond variable resolution and this loop only seems to be handling variable resolution.
The TryGetRequest API (and other APIs it calls such as TryGetUri) would already be considering all such diagnostics - so my thinking was that it may be better to rely exclusively on that API and centralize all validation / binding logic over there.
There was a problem hiding this comment.
What I meant is that there could be more to the symbolic diagnostics beyond variable resolution and this loop only seems to be handling variable resolution.
Do we have an example of this?
My concern about binding those types to a specific API call is that it's also not necessarily correct. TryGetHttpRequest isn't the only possible use of the diagnostics request, so using it here feels odd from a layering perspective.
There was a problem hiding this comment.
One example may be if we add validation for supported version numbers for HttpVersionNode. Another may be if we need to add validation for specific values that are supported / unsupported for HeaderValueNode depending on the HeaderNameNode of the corresponding header...
Wonder if we should try and create a more general TryBind() API which performs the same binding steps that TryGetHttpRequest() does without creating / returning the System.Net.Http.* objects. However, I feel some of these validations may require calling into System.Net.Http regardless so we may not be saving much...
There was a problem hiding this comment.
I would expect these to be directly handled by the parser, since they don't change often and they don't vary based on the host environment. These are closer to syntactic than to symbolic validations.
There was a problem hiding this comment.
@jonsequitur The changes mostly look great 👍🏾
I've added a few comments that may be worth a look. Some of those (such as a potential perf improvement for GrowSpan()) may be worth fixing up in follow up PRs.
No description provided.