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

Rename UriHelper.Encode #648

Merged
merged 1 commit into from
Jun 6, 2016
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
29 changes: 10 additions & 19 deletions samples/SampleApp/Program.cs
Original file line number Diff line number Diff line change
@@ -1,30 +1,21 @@
using System;
using System.Diagnostics;
using Microsoft.Extensions.Primitives;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Extensions;

namespace SampleApp
{
public class Program
{
public void Main(string[] args)
public static void Main(string[] args)
Copy link
Member

Choose a reason for hiding this comment

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

This sample app is bogus....

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but it's convenient for debugging things.

Copy link
Member

Choose a reason for hiding this comment

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

So are unit tests

{
for (int i = 0; i < 10; i++)
var query = new QueryBuilder()
{
Stopwatch timer = new Stopwatch();
timer.Start();
string myString;
string[] myArray;
StringValues myValues;
for (int j = 0; j < 100000000; j++)
{
myString = new string('a', 40);
myArray = new[] { myString };
// myValues = new StringValues(myString);
myValues = new StringValues(myArray);
}
timer.Stop();
Console.WriteLine(timer.Elapsed + ", " + Environment.WorkingSet);
}
{ "hello", "world" }
}.ToQueryString();

var uri = UriHelper.BuildAbsolute("http", new HostString("contoso.com"), query: query);

Console.WriteLine(uri);
}
}
}
22 changes: 17 additions & 5 deletions samples/SampleApp/project.json
Original file line number Diff line number Diff line change
@@ -1,12 +1,24 @@
{
"version": "1.0.0-*",
"dependencies": {
"Microsoft.AspNetCore.Http": "1.0.0-*"
},
"commands": {
"SampleApp": "SampleApp"
"Microsoft.AspNetCore.Http": "1.0.0-*",
"Microsoft.AspNetCore.Http.Extensions": "1.0.0-*"
},
"frameworks": {
"net451": {}
"net451": { },
"netcoreapp1.0": {
"imports": [
"dnxcore50"
],
"dependencies": {
"Microsoft.NETCore.App": {
"version": "1.0.0-*",
"type": "platform"
}
}
}
},
"buildOptions": {
"emitEntryPoint": true
}
}
34 changes: 17 additions & 17 deletions src/Microsoft.AspNetCore.Http.Extensions/UriHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@ public static class UriHelper
/// <summary>
/// Combines the given URI components into a string that is properly encoded for use in HTTP headers.
/// </summary>
/// <param name="pathBase"></param>
/// <param name="path"></param>
/// <param name="query"></param>
/// <param name="fragment"></param>
/// <param name="pathBase">The first portion of the request path associated with application root.</param>
/// <param name="path">The portion of the request path that identifies the requested resource.</param>
/// <param name="query">The query, if any.</param>
/// <param name="fragment">The fragment, if any.</param>
/// <returns></returns>
public static string Encode(
public static string BuildRelative(
PathString pathBase = new PathString(),
PathString path = new PathString(),
QueryString query = new QueryString(),
Expand All @@ -35,14 +35,14 @@ public static string Encode(
/// Combines the given URI components into a string that is properly encoded for use in HTTP headers.
/// Note that unicode in the HostString will be encoded as punycode.
/// </summary>
/// <param name="scheme"></param>
/// <param name="host"></param>
/// <param name="pathBase"></param>
/// <param name="path"></param>
/// <param name="query"></param>
/// <param name="fragment"></param>
/// <param name="scheme">http, https, etc.</param>
/// <param name="host">The host portion of the uri normally included in the Host header. This may include the port.</param>
/// <param name="pathBase">The first portion of the request path associated with application root.</param>
/// <param name="path">The portion of the request path that identifies the requested resource.</param>
/// <param name="query">The query, if any.</param>
/// <param name="fragment">The fragment, if any.</param>
/// <returns></returns>
public static string Encode(
public static string BuildAbsolute(
string scheme,
HostString host,

Choose a reason for hiding this comment

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

Might make sense to add an overload that takes a string variation of this. Almost every call into this ends up doing new HostString(...)

Copy link
Member Author

@Tratcher Tratcher Jun 3, 2016

Choose a reason for hiding this comment

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

Almost every call into this...

Insufficient data. I see 5 references and the ones that call new HostString are samples and tests. The actual code references pull the value from request.Host or HostString.FromUriComponent(uri).

Choose a reason for hiding this comment

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

🆗, less concerned about this guy. Can always add the additional overloads later if it turns out to be a burden on users.

PathString pathBase = new PathString(),
Expand Down Expand Up @@ -74,13 +74,13 @@ public static string Encode(
/// Generates a string from the given absolute or relative Uri that is appropriately encoded for use in
/// HTTP headers. Note that a unicode host name will be encoded as punycode.
/// </summary>
/// <param name="uri"></param>
/// <param name="uri">The Uri to encode.</param>
/// <returns></returns>
public static string Encode(Uri uri)
{
if (uri.IsAbsoluteUri)
{
return Encode(
return BuildAbsolute(

Choose a reason for hiding this comment

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

Curious: Why is there not a corresponding BuildRelative call in the else?

Copy link
Member Author

Choose a reason for hiding this comment

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

System.Uri has minimal support for relative uris. It won't parse it into path?query#fragment for you. The most it will do is some basic escaping.

scheme: uri.Scheme,
host: HostString.FromUriComponent(uri),
pathBase: PathString.FromUriComponent(uri),
Expand All @@ -97,18 +97,18 @@ public static string Encode(Uri uri)
/// Returns the combined components of the request URL in a fully escaped form suitable for use in HTTP headers
/// and other HTTP operations.
/// </summary>
/// <param name="request"></param>
/// <param name="request">The request to assemble the uri pieces from.</param>
/// <returns></returns>
public static string GetEncodedUrl(this HttpRequest request)

Choose a reason for hiding this comment

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

Super odd to have an extension method in this class. Also, any reason the above Encode method shouldn't be an extension method/renamed GetEncodedUrl?

Copy link
Member Author

Choose a reason for hiding this comment

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

Encode could be an extension, but I think you'd loose context of what you were encoding it for. It's only used here:
https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNetCore.Http.Extensions/ResponseHeaders.cs#L145

Choose a reason for hiding this comment

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

Not sure I follow. Wouldn't it just be:

Headers.Set(HeaderNames.Location, value == null ? null : value.GetEncodedUrl());

Value's a Uri which is the contextual part. It's odd having this method and the below extension method. They both should be consistent whatever is decided.

Copy link
Member Author

Choose a reason for hiding this comment

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

Encoded in what way and for what purpose? This method does a particular type of encoding for use in Http headers that is not applicable elsewhere.

Choose a reason for hiding this comment

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

Lets talk in person.

Copy link
Member Author

Choose a reason for hiding this comment

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

HttpEncode?

Choose a reason for hiding this comment

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

Talked offline, this is 🆗

{
return Encode(request.Scheme, request.Host, request.PathBase, request.Path, request.QueryString);
return BuildAbsolute(request.Scheme, request.Host, request.PathBase, request.Path, request.QueryString);
}

/// <summary>
/// Returns the combined components of the request URL in a fully un-escaped form (except for the QueryString)
/// suitable only for display. This format should not be used in HTTP headers or other HTTP operations.
/// </summary>
/// <param name="request"></param>
/// <param name="request">The request to assemble the uri pieces from.</param>
/// <returns></returns>
public static string GetDisplayUrl(this HttpRequest request)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,15 @@ public class UriHelperTests
[Fact]
public void EncodeEmptyPartialUrl()
{
var result = UriHelper.Encode();
var result = UriHelper.BuildRelative();

Assert.Equal("/", result);
}

[Fact]
public void EncodePartialUrl()
{
var result = UriHelper.Encode(new PathString("/un?escaped/base"), new PathString("/un?escaped"),
var result = UriHelper.BuildRelative(new PathString("/un?escaped/base"), new PathString("/un?escaped"),
new QueryString("?name=val%23ue"), new FragmentString("#my%20value"));

Assert.Equal("/un%3Fescaped/base/un%3Fescaped?name=val%23ue#my%20value", result);
Expand All @@ -27,15 +27,15 @@ public void EncodePartialUrl()
[Fact]
public void EncodeEmptyFullUrl()
{
var result = UriHelper.Encode("http", new HostString(string.Empty));
var result = UriHelper.BuildAbsolute("http", new HostString(string.Empty));

Assert.Equal("http:///", result);
}

[Fact]
public void EncodeFullUrl()
{
var result = UriHelper.Encode("http", new HostString("my.HoΨst:80"), new PathString("/un?escaped/base"), new PathString("/un?escaped"),
var result = UriHelper.BuildAbsolute("http", new HostString("my.HoΨst:80"), new PathString("/un?escaped/base"), new PathString("/un?escaped"),
new QueryString("?name=val%23ue"), new FragmentString("#my%20value"));

Assert.Equal("http://my.xn--host-cpd:80/un%3Fescaped/base/un%3Fescaped?name=val%23ue#my%20value", result);
Expand Down