Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Implement IHttpRequestFeature.RawTarget (aspnet/HttpAbstractions#596) #880

Merged
merged 1 commit into from
May 31, 2016
Merged

Implement IHttpRequestFeature.RawTarget (aspnet/HttpAbstractions#596) #880

merged 1 commit into from
May 31, 2016

Conversation

cesarblum
Copy link
Contributor

@cesarblum cesarblum commented May 25, 2016 •
edited
Loading

Tratcher
Tratcher reviewed May 26, 2016
View reviewed changes
test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs
@@ -204,6 +205,58 @@ public void RequestPathIsNormalized()
}
}

[Theory]
[InlineData("/")]
Copy link
Member

Choose a reason for hiding this comment

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

*?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in another test.

@Tratcher
Copy link
Member

👍

@cesarblum
Copy link
Contributor Author

Added test for paths not starting with '/'.

@cesarblum cesarblum force-pushed the cesarbs/raw-target-request-feature branch from bc35a05 to 15aea76 Compare May 26, 2016 22:16
halter73
halter73 reviewed May 26, 2016
View reviewed changes
test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs
[InlineData("*")]
[InlineData("DoesNotStartWith/")]
[InlineData("*?arg=value")]
[InlineData("*/?arg=value")]
Copy link
Member

@halter73 halter73 May 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

[InlineData("%2F")]?

@cesarblum cesarblum force-pushed the cesarbs/raw-target-request-feature branch from 85762c2 to d2095c0 Compare May 26, 2016 23:27
@cesarblum
Copy link
Contributor Author

Checked in some unintended changes, reverting.

@cesarblum cesarblum force-pushed the cesarbs/raw-target-request-feature branch from d2095c0 to 52980f6 Compare May 26, 2016 23:29
@cesarblum
Copy link
Contributor Author

Updated with a small perf detail.

@cesarblum
Copy link
Contributor Author

Squirrel?

@cesarblum
Copy link
Contributor Author

🔔 I'd like to merge this tonight.

@halter73
Copy link
Member

Would it be so bad to merge tomorrow? The build's broken.

@cesarblum cesarblum force-pushed the cesarbs/raw-target-request-feature branch 3 times, most recently from b4fa841 to efa1890 Compare May 27, 2016 22:28
@cesarblum cesarblum changed the title Implement IHttpRequestFeature.RawTarget. Implement IHttpRequestFeature.RawTarget (aspnet/HttpAbstractions#596) May 27, 2016
@cesarblum
Copy link
Contributor Author

Squashed.

@halter73
Copy link
Member

This make only the second usage of FunctionalTests.TestConnection. I want to remove that class, so can we please not use it?

halter73
halter73 reviewed May 27, 2016
View reviewed changes
test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs
response.Append(Encoding.ASCII.GetString(buffer, 0, length));
}

Assert.StartsWith("HTTP/1.1 200 OK", response.ToString());
Copy link
Member

Choose a reason for hiding this comment

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

Testing only StartsWith seems wrong. In most of our tests, we verify the entire response is what we expect.

@cesarblum
Copy link
Contributor Author

@halter73 Removed TestConnection (the FunctionalTests one).

@cesarblum
Copy link
Contributor Author

🔔

halter73
halter73 reviewed May 31, 2016
View reviewed changes
test/Microsoft.AspNetCore.Server.KestrelTests/RequestTargetProcessingTests.cs
[InlineData(".././")]
[InlineData("../..")]
[InlineData("../../")]
public async Task NonPathRequestTargetSetInRawTarget(string requestTarget)
Copy link
Member

@halter73 halter73 May 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Would these test cases pass with WebListenter after you add RawTarget there? I mainly wondering whether Path, PathBase and QueryString would all be an empty string.

Should this be added to ServerTests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WebListener actually rejects those requests. Maybe we should do the same in Kestrel? The only request I was able to make to a target not starting with / was OPTIONS *.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, WebListener turned * into /* in Path.

@cesarblum cesarblum force-pushed the cesarbs/raw-target-request-feature branch from 8bb2b85 to 1bf2f6b Compare May 31, 2016 18:46
@halter73
Copy link
Member

:shipit:

halter73
halter73 reviewed May 31, 2016
View reviewed changes
src/Microsoft.AspNetCore.Server.Kestrel/Http/Frame.cs
else
{
rawTarget = pathBegin.GetAsciiString(queryEnd);
}
}

requestUrlPath = PathNormalizer.RemoveDotSegments(requestUrlPath);
Copy link
Member

Choose a reason for hiding this comment

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

New variable please.

@cesarblum cesarblum force-pushed the cesarbs/raw-target-request-feature branch from 7f2aaac to 50208a3 Compare May 31, 2016 22:30
@cesarblum cesarblum merged commit 50208a3 into dev May 31, 2016
@cesarblum cesarblum deleted the cesarbs/raw-target-request-feature branch May 31, 2016 22:47
@cesarblum cesarblum mentioned this pull request Aug 5, 2016
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cesarblum @Tratcher @halter73 @mikeharder @dnfclas