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

fix for comment following headers #3173

Merged
merged 1 commit into from
Sep 13, 2023
Merged

fix for comment following headers #3173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 22 additions & 0 deletions src/Microsoft.DotNet.Interactive.HttpRequest.Tests/ParserTests.Comments.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// 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 System;
using System.Collections.Generic;
using FluentAssertions;
using Microsoft.DotNet.Interactive.HttpRequest.Tests.Utility;
using Xunit;
Expand Down Expand Up @@ -41,5 +43,25 @@ public void comment_without_text_at_end_of_request_is_parsed_correctly()
.ContainSingle<HttpCommentNode>()
.Which.Text.Should().Be("#");
}

[Fact]
public void Comment_node_can_immediately_follow_headers()
{
var code = """
GET https://example.com
Accept: text/plain
# This is a comment

this is the body
""";

var result = Parse(code);

result.GetDiagnostics().Should().BeEmpty();

result.SyntaxTree.RootNode.DescendantNodesAndTokens()
.Should().ContainSingle<HttpCommentNode>()
.Which.Text.Should().Be("# This is a comment");
}
}
}
4 changes: 2 additions & 2 deletions src/Microsoft.DotNet.Interactive.HttpRequest.Tests/ParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,6 @@ private static HttpRequestParseResult Parse(string code)
return result;
}

private static HttpDiagnosticInfo CreateDiagnosticInfo(string message)
=> new HttpDiagnosticInfo(id: "HTTP0000", message, DiagnosticSeverity.Error);
private static HttpDiagnosticInfo CreateDiagnosticInfo(string message) =>
new(id: "HTTP0000", message, DiagnosticSeverity.Error);
}
5 changes: 5 additions & 0 deletions src/Microsoft.DotNet.Interactive.HttpRequestParser/Parser/HttpRequestNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@ public void Add(HttpBodyNode node)
AddInternal(node);
}

public void Add(HttpCommentNode node)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? So far, the explicit Add methods were only required for nodes that appear as properties...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're needed for all node types that can be added.

Copy link
Contributor

@shyamnamboodiripad shyamnamboodiripad Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We had discussed this and decided to include the Add method for comments on the base HttpSyntaxNode class (because otherwise implementing ParseLeadingCommentsAndWhitespace becomes cumbersome),

HttpSyntaxNode already has an Add method that handles comments. Looks like it has an additional addBefore parameter - perhaps we can make that parameter optional with a default value and remove the explicit overload in HttpRequestNode above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had discussed this and decided to include the Add method for comments on the base HttpSyntaxNode class (because otherwise implementing ParseLeadingCommentsAndWhitespace becomes cumbersome),

Since comments aren't valid in all syntax comments, adding this Add method on HttpSyntaxNode would be misleading.

{
AddInternal(node);
}

public HttpBindingResult<HttpRequestMessage> TryGetHttpRequestMessage(HttpBindingDelegate bind)
{
var request = new HttpRequestMessage();
Expand Down
28 changes: 20 additions & 8 deletions src/Microsoft.DotNet.Interactive.HttpRequestParser/Parser/HttpRequestParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,14 +83,17 @@ public HttpSyntaxTree Parse()
return _syntaxTree;
}

private static void PrependCommentsIfAny(List<HttpCommentNode> commentsToPrepend, HttpSyntaxNode toNode)
private static void AddCommentsIfAny(
List<HttpCommentNode> comments,
HttpSyntaxNode toNode,
bool addBefore = true)
{
foreach (var comment in commentsToPrepend)
foreach (var comment in comments)
{
toNode.Add(comment, addBefore: true);
toNode.Add(comment, addBefore: addBefore);
}

commentsToPrepend.Clear();
comments.Clear();
}

private IEnumerable<HttpVariableDeclarationAndAssignmentNode>? ParseVariableDeclarations()
Expand Down Expand Up @@ -336,18 +339,27 @@ private T ParseTrailingWhitespace<T>(T node, bool stopAfterNewLine = false, bool
ParseTrailingWhitespace(requestNode, stopAfterNewLine: true);
}

var commentsToPrepend = new List<HttpCommentNode>();
var comments = new List<HttpCommentNode>();
if (ParseComment() is { } comment)
{
commentsToPrepend.Add(comment);
comments.Add(comment);
}

var headersNode = ParseHeaders();
if (headersNode is not null)
{
PrependCommentsIfAny(commentsToPrepend, headersNode);
AddCommentsIfAny(comments, headersNode);
requestNode.Add(headersNode);
}
else
{
AddCommentsIfAny(comments, requestNode, addBefore: false);
}

if (ParseComment() is { } commentAfterHeaders)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work if there are multiple comments OR multiple comments separated by new lines?

Also looks like this won't handle any leading whitespace before # or //.

I still feel this is better handled via ParseLeadingCommentsAndWhitespace in the Parse method for whatever construct follows. However, I am yet to review your responses in the earlier PR - so please ignore if you already addressed this there :)

Copy link
Contributor

Choose a reason for hiding this comment

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

One other thought is that perhaps ParseTrailingWhitespace can be updated to also parse trailing comments (and perhaps that will handle the case where ParseLeadingCommentsAndWhitespace can't handle the above comment (i.e., in the case where there is no construct that follows after the header section).

(Also wondering how we currently parse a document that only contains comments but no requests i.e., under what construct do such comments end up being added today?

It may be a good idea to add (skipped) tests for the above cases assuming we don't have them already.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have a consistent pattern here but the same problems applies as we encountered with comments generally. Parsing leading whitespace in that way e.g. within ParseComment requires gathering it up and only adding it to the tree if we actually encounter a comment node. Otherwise, it needs to be stored and applied to the next node type that we do encounter.

Leading whitespace before comments is not supported in the old parser for either kind of comment prefix (# or //).

We don't support //-prefixed comments in the new parser at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't support //-prefixed comments in the new parser at all.

Based on the following code in the parser, // should be supported.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

The current design where we can only parse a single comment in these spots seems problematic. It is also odd that we don't support leading whitespace before comments just at these spots (we do support leading whitespace for any comments parsed via ParseLeadingCommentsAndWhitespace in other spots).

I can take a closer look and take a stab at improving this later. Will sync up offline once I have a PR / proposal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the following code in the parser, // should be supported.

This might work correctly now. I believe there were lookaheads in places that didn't take // into account.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current design where we can only parse a single comment in these spots seems problematic.

This is just a bug. I haven't added combinatorial tests for all known syntax combinations in order to avoid trying to fix too many issues in a single massive PR. That and //-prefixed comments are up next.

{
requestNode.Add(commentAfterHeaders);
}

ParseTrailingWhitespace(requestNode);

Expand All @@ -358,7 +370,7 @@ private T ParseTrailingWhitespace<T>(T node, bool stopAfterNewLine = false, bool
}
else
{
PrependCommentsIfAny(commentsToPrepend, requestNode);
AddCommentsIfAny(comments, requestNode, addBefore: false);
}

ParseTrailingWhitespace(requestNode);
Expand Down