dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.47k stars 4.76k forks source link

[URI] Uri serialization is missing Scheme #90140

Open sagimar opened 1 year ago

sagimar commented 1 year ago

Description

When serializing a local path URI, the scheme ('file:///') is being lost. This was checked both with Newtonsoft.Json and System.Text.Json. On Windows, when de-serializing the URI, it knows to add back the scheme. But, on Linux, it doesn't know to do this conversion, so the following uri: file:///tmp/ is being converted to /tmp/, thus the absolute URI became a relative one, and methods as .AbsoluteUri will throw an exception.

Reproduction Steps

var uri = new Uri(Path.GetTempPath()); var serializedUri = JsonConvert.SerializeObject(uri); var deserializedUri = JsonConvert.DeserializeObject(serializedUri);

serializedUri equals to: "\"C:\\Users\\****\\AppData\\Local\\Temp\\\""

Expected behavior

serializedUri should contain file:///

Actual behavior

serializedUri does not contain file:/// - in Windows it knows to add it back on deserialization, but on Linux it doesn't.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

Issue Details
### Description When serializing a local path URI, the scheme ('file:///') is being lost. This was checked both with Newtonsoft.Json and System.Text.Json. On Windows, when de-serializing the URI, it knows to add back the scheme. **But, on Linux, it doesn't know to do this conversion, so the following uri: file:///tmp/ is being converted to /tmp/, thus the absolute URI became a relative one, and methods as .AbsoluteUri will throw an exception.** ### Reproduction Steps var uri = new Uri(Path.GetTempPath()); var serializedUri = JsonConvert.SerializeObject(uri); var deserializedUri = JsonConvert.DeserializeObject(serializedUri); serializedUri equals to: "\"C:\\\\Users\\\\********\\\\AppData\\\\Local\\\\Temp\\\\\"" ### Expected behavior serializedUri should contain file:/// ### Actual behavior serializedUri does not contain file:/// - in Windows it knows to add it back on deserialization, but on Linux it doesn't. ### Regression? _No response_ ### Known Workarounds _No response_ ### Configuration _No response_ ### Other information _No response_
Author: sagimar
Assignees: -
Labels: `area-System.Text.Json`
Milestone: -
ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/ncl See info in area-owners.md if you want to be subscribed.

Issue Details
### Description When serializing a local path URI, the scheme ('file:///') is being lost. This was checked both with Newtonsoft.Json and System.Text.Json. On Windows, when de-serializing the URI, it knows to add back the scheme. **But, on Linux, it doesn't know to do this conversion, so the following uri: file:///tmp/ is being converted to /tmp/, thus the absolute URI became a relative one, and methods as .AbsoluteUri will throw an exception.** ### Reproduction Steps var uri = new Uri(Path.GetTempPath()); var serializedUri = JsonConvert.SerializeObject(uri); var deserializedUri = JsonConvert.DeserializeObject(serializedUri); serializedUri equals to: "\"C:\\\\Users\\\\********\\\\AppData\\\\Local\\\\Temp\\\\\"" ### Expected behavior serializedUri should contain file:/// ### Actual behavior serializedUri does not contain file:/// - in Windows it knows to add it back on deserialization, but on Linux it doesn't. ### Regression? _No response_ ### Known Workarounds _No response_ ### Configuration _No response_ ### Other information _No response_
Author: sagimar
Assignees: -
Labels: `area-System.Net`, `untriaged`
Milestone: -
gregsdennis commented 1 year ago

Tangentially related to https://github.com/dotnet/runtime/issues/85229

MihaZupan commented 1 year ago

Do you mean that you took the string "/tmp/", passed it to new Uri on Linux, serialized the Uri instance to json, then deserialized said json on a Windows system?

Json will just save the original string you passed to the ctor. Uri only recognizes absolute Unix file paths when running on Linux. A string like "/tmp/" will therefore be treated as absolute on Linux and relative on Windows. Similar issues for reference: #80118, #76816, #69308

@eiriktsarpalis would it be a breaking change for Json to serialize AbsoluteUri instead of OriginalString?

value.IsAbsolute ? value.AbsoluteUri : value.OriginalString
sagimar commented 1 year ago

No no, sorry will correct myself- I don't move it between Linux to Windows- I just checked the behaviour once on Windows and once on Linux.

Let me fully explain my scenario in Linux- When I create a uri: var uri = new Uri(Path.GetTempPath()); I am receiving a Uri that contains 'File:///'- printing the uri results with: file:///tmp/ When I serialize the uri, the serialized string is: /tmp/ Thus, on deserialization, I am receiving a Uri with the value: /tmp/ (missing the 'File:///') - so instead of holding an absolute uri as was before when constructed, it is now only a relative one.

MihaZupan commented 1 year ago

Ah, I see now. What you are running into is

var uri1 = new Uri("/tmp/");
var uri2 = new Uri("/tmp/", UriKind.RelativeOrAbsolute); // How System.Text.Json parses it

Console.WriteLine(uri1.IsAbsoluteUri); // True
Console.WriteLine(uri2.IsAbsoluteUri); // False

Due to some backward-compatibility behavior of Uri, implicit Unix file paths will be treated as relative if you specify UriKind.RelativeOrAbsolute.

I would recommend that you try to avoid creating Uris with implicit file paths (without the file:// scheme prefix). You should be able to workaround this issue by using new Uri($"file://{Path.GetTempPath()}").

sagimar commented 1 year ago

The workaround will probably work- but just to emphasize, I'm not sending UriKind.RelativeOrAbsolute. Even if I send UriKind.Absolute, it acts the same. The issue is probably that the serialization takes the 'OriginalString', but the constructor of the Uri takes the 'OriginalString' and adds to it file:/// - finally this ends with a different uri when constructing the uri through the constructor and when serializing it.

If the Uri constructor added 'file://' to the uri, I believe it should be propogated to the serializer too, since currently there is a different behaviour when constructing the uri through the constructor or through serialiation+deserialization.

MihaZupan commented 1 year ago

I'm not sending UriKind.RelativeOrAbsolute

Right, the Json serializer is.

https://github.com/dotnet/runtime/blob/5730c2098b2fe3f18c0d43303eecc6b96823274e/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/UriConverter.cs#L23

https://github.com/dotnet/runtime/blob/5730c2098b2fe3f18c0d43303eecc6b96823274e/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/UriConverter.cs#L36

The problem is that inputs like /tmp/ are ambiguous - they can mean different things depending on the platform, and depending on which ctor arguments you pass in. If you use new Uri(string, UriKind.RelativeOrAbsolute) instead of new Uri(string), you should see the same behavior as when you deserialize with System.Text.Json.

sagimar commented 1 year ago

Yes, so I think that if you pass it to a constructor that added the 'file://' to the url, so when serializing it, it should also add the 'file://' upon serialization, then we'll have a consistency behaviour, no?

MihaZupan commented 1 year ago

The serializer could potentially try to preserve that behavior, yes. This is what I was referring to when asking

would it be a breaking change for Json to serialize AbsoluteUri instead of OriginalString?

sagimar commented 1 year ago

Yes, sounds good! If it is indeed not a breaking change, I think this is a good improvement. Thank you!

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

Issue Details
### Description When serializing a local path URI, the scheme ('file:///') is being lost. This was checked both with Newtonsoft.Json and System.Text.Json. On Windows, when de-serializing the URI, it knows to add back the scheme. **But, on Linux, it doesn't know to do this conversion, so the following uri: file:///tmp/ is being converted to /tmp/, thus the absolute URI became a relative one, and methods as .AbsoluteUri will throw an exception.** ### Reproduction Steps var uri = new Uri(Path.GetTempPath()); var serializedUri = JsonConvert.SerializeObject(uri); var deserializedUri = JsonConvert.DeserializeObject(serializedUri); serializedUri equals to: "\"C:\\\\Users\\\\********\\\\AppData\\\\Local\\\\Temp\\\\\"" ### Expected behavior serializedUri should contain file:/// ### Actual behavior serializedUri does not contain file:/// - in Windows it knows to add it back on deserialization, but on Linux it doesn't. ### Regression? _No response_ ### Known Workarounds _No response_ ### Configuration _No response_ ### Other information _No response_
Author: sagimar
Assignees: -
Labels: `area-System.Text.Json`, `untriaged`
Milestone: -
eiriktsarpalis commented 1 year ago

Thanks for helping with triage @MihaZupan. We'll consider this in a future release. In the meantime you might want to consider authoring a custom JsonConverter<Uri> that addresses your use case.