Runtime: Ctor of System.Uri transform Uri to lowercase

[alexsokolov]

I got into this bug when was moving library to .Net Standard 2:
When we construct Uri by combining two Uri's, one with absolute path, and another with Relative:
var sut = new Uri(new Uri("https://your_azure_account.restv2.westeurope.media.azure.net/api/", UriKind.Absolute), new Uri("ContentKeys('nb:kid:UUID:7deef38d-3432-4747-a5b2-0cd2f06a6edd')/GetKeyDeliveryUrl", UriKind.Relative));

It lowers "ContentKeys":
https://your_azure_account.restv2.westeurope.media.azure.net/api/contentkeys('nb:kid:UUID:7deef38d-3432-4747-a5b2-0cd2f06a6edd')/GetKeyDeliveryUrl
instead of
https://your_azure_account.restv2.westeurope.media.azure.net/api/ContentKeys('nb:kid:UUID:7deef38d-3432-4747-a5b2-0cd2f06a6edd')/GetKeyDeliveryUrl
which you'll get e.g. in .Net 4.7.2.

Interesting, but GetKeyDeliveryUrl isn't getting lowered.

This bug breaks retrieving of key delivery Url if you're using Microsoft.Data.Services.Client library.

[davidsh]

@alexsokolov Has this worked before in .NET Core (i.e. with .NET Core 1.0 or .NET Core 1.1)? We want to understand if this broke recently in .NET Core 2.0. Or was it always broken in .NET Core vs. .NET Framework?

[alexsokolov]

Hi @davidsh, I did a brief test, and it showed that it a newly introduced in .Net Core 2.0.
With 1.0 and 1.1 all works fine - Uri doesn't become lowered.

[svick]

@davidsh It seems to be a regression in .Net Core 2.0.

This minimized repro:

```c#
// see https://github.com/dotnet/BenchmarkDotNet/blob/15d7238/src/BenchmarkDotNet.Core/Portability/RuntimeInformation.cs#L147-L155
Console.WriteLine(GetNetCoreVersion());

Console.WriteLine(new Uri(
new Uri("http://example.com/", UriKind.Absolute),
new Uri("C(B:G", UriKind.Relative)));

Produces this output:

dotnet run -f netcoreapp1.0
1.0.8
http://example.com/C(B:G
dotnet run -f netcoreapp1.1
1.1.5
http://example.com/C(B:G
dotnet run -f netcoreapp2.0
2.0.3
http://example.com/c(B:G
dotnet run -f netcoreapp2.1
2.1.0-preview1-25919-02
http://example.com/c(B:G

---

Also, when I remove the parenthesis (i.e. `new Uri(new Uri("http://example.com/", UriKind.Absolute), new Uri("CB:G", UriKind.Relative))`), I get an exception:

System.UriFormatException: A relative URI cannot be created because the 'uriString' parameter represents an absolute URI.
at System.Uri.CreateThis(String uri, Boolean dontEscape, UriKind uriKind)
at System.Uri..ctor(String uriString, UriKind uriKind)
at Program.Main()
```

Though this part is also the same on .Net Core 1.0, so it's not a regression. Is this the expected behavior, or should I open a separate issue about it?

[davidsh]

Is this the expected behavior, or should I open a separate issue about it?

What happens on .NET Framework?

[rmkerr]

That code fails with the same error on .NET Framework. I'd like to do some more digging before I say anything for certain, but it sure looks like a bug to me.

[JonHanna]

That last part throws on netfx too. It's just the new Uri("CB:G", UriKind.Relative) component and is documented, "uriString contains an absolute URI and uriKind is Relative". Whether one would argue that "CG:B" shouldn't be seen as an absolute URI is another question, though one can create an equivalent relative URI with new Uri("./CB:G", UriKind.Relative) (or am I missing a context where that would combine differently?)

[rmkerr]

Here we go. Section 4.2 of RFC 3986:

A path segment that contains a colon character (e.g., "this:that") cannot be used as the first segment of a relative-path reference, as it would be mistaken for a scheme name. Such a segment must be preceded by a dot-segment (e.g., "./this:that") to make a relative-path reference.

Based on that I'd say that the exception isn't a bug, and we can just focus on the original issue.

[stephentoub]

This was broken by https://github.com/dotnet/corefx/pull/12061. I'll revert.
cc: @jamesqo

[davidsh]

This was broken by dotnet/corefx#12061. I'll revert.

Thanks. But, how did you determine that? Looking at the diff of that change, it isn't obvious it would break something like this:

[stephentoub]

how did you determine that?

Stepped through to see where things went awry, then looked at the history to see how we ended up with code mutating the original string like that. You can see my description of why the change broke it here: https://github.com/dotnet/corefx/pull/25560#issue-277530565.

[svick]

@rmkerr In that case, shouldn't new Uri("C(B:G", UriKind.Relative) also throw that exception?

C(B is not a valid scheme, so the justification of being "mistaken for a scheme name" doesn't really apply. But, if I understand it correctly, the RFC syntax for relative URIs forbids colons in the first segment altogether. The derivation would go relative-ref → relative-part → path-noscheme → segment-nz-nc, but segment-nz-nc does not allow colons. This means that, according to the RFC, C(B:G is not a valid relative URI.

[karelz]

It's a breaking change in .NET Core 2.0, we should back port it to 2.0.x servicing branch.
@stephentoub do you want help from @rmkerr or do you plan to do it yourself?

[stephentoub]

do you want help

I'd be happy for someone else to do it :)

[karelz]

@rmkerr let me know offline if you need details about the servicing process.

[rmkerr]

@svick Yes, I think you're right about that. My best guess is that our implementation just tests if the first segment is a valid scheme and throws an exception if that's the case. Since the ( means the first segment would not be a valid scheme, it accepts it as a valid relative URI. I don't know if it's an issue we should be concerned about though. Since C(B:G is unambiguously not a scheme, I think that it is reasonable to treat it as a segment. If we want to go for total compliance with the spec it might be worth looking into, but otherwise I think the behavior is consistent enough that we can just leave it.

@karelz Sounds good. I'll shoot you an email.

[karelz]

Reopening to track 2.0.x servicing port

[rmkerr]

Closing this since the 2.0.x servicing port has been merged.