Confusing UriHelper method names

[strohhut]

Sorry, I find this helper class a bit confusing.

Why is a method that does more than encoding called Encode? It not only encodes it also combines several uri parts into an uri. And why are two methods called Encode when one creates an absolute path and the other creates an absolute uri? Why is the unescaped version called so differently (GetDisplayUrl)? Why is the convenience method GetEncodedUrl named this way although it does exactly the same as the Encodemethod? Only difference is that it is an extension method for HttpRequest. Why is there no convenience method for the non-absolute version?

Maybe I just don't get it but here is my proposal:

Why not just have two method names GetAbsolutePath and GetUrl? Then give those two methods an encode or httpEncode or encodeComponents bool parameter so there is no need for any Display methods. Then make overloads of both methods that act as convenience methods for HttpRequest.

So we would get

string GetAbsolutePath(PathString pathBase, PathString path, QueryString query, FragmentString fragment, bool encodeComponents)

gives /foo/bar

Note that this would conform to RFC 3986

A relative reference that begins with a single slash character is
termed an absolute-path reference. A relative reference that does
not begin with a slash character is termed a relative-path reference.

string GetUrl(string scheme, HostString host, PathString pathBase, PathString path, QueryString query, FragmentString fragment, bool encodeComponents)

gives http://example.com/foo/bar

[muratg]

Putting in 1.0.0 to consider this post RC2.

[NTaylorMullen]

I don't entirely agree with the proposed but I 100% agree that the existing methods are completely confusing and lead users down a pit of failure. Hence spawning my other issue here.

We should consider fixing/designing this for RTM seeing how confusing the APIs currently are.

[muratg]

@NTaylorMullen @Tratcher Do you guys have a set of proposed names?

cc @Eilon

[Tratcher]

One proposal from offline discussion:
Rename UriHelper.Encode to UriHelper.BuildEncoded or just Build. If you really wanted to give the overloads unique names then how about BuildAbsolute vs BuildRelative

[NTaylorMullen]

👍 for BuildAbsolute / BuildRelative

[Tratcher]

FYI: I only see one usage of this API in our code base, and that's in a sample:
https://github.com/aspnet/Security/blob/c9f8455dbc94162abce5687e8c7f95c6b59e6d3c/samples/OpenIdConnect.AzureAdSample/Startup.cs#L90

[Tratcher]

To answer some of @strohhut original questions:
GetDisplayUrl and GetEncodedUrl give you the full url for a given request, usually used for logging or similar diagnostics. We wanted to be very explicit that the un-encoded format is for human display only, it is not suitable for network operations. We could add a GetEncodedRelativeUrl or similar extension, but we haven't seen a need for it yet.

UriHelper helps you assemble urls from parts for links or redirects. These scenarios always require encoding. These parts are usually a mixture items from the current request and new items for the next operation. E.g. You'd usually keep the original Host and PathBase, but may change the scheme, query, etc..

[strohhut]

Thanks, it helped me to understand the purpose of the methods.