Convert to System.Text.Json #73
Conversation
highlyunavailable
force-pushed
the
master
branch
from
3e73f57
to
56aa891
Compare
highlyunavailable
force-pushed
the
master
branch
from
56aa891
to
d32c8e7
Compare
There was a problem hiding this comment.
@highlyunavailable Thank you for submitting this PR, it's excellent work.
Please see my comment on the version of System.Text.Json used in this PR. It worries me that its current adoption is too low which can cause troubles for our consumers. I think we should wait for wider adoption of the version 5.0.1 or higher before merging this PR.
| @@ -22,7 +22,7 @@ | |||
| using System.Net.Http; | |||
| using System.Net.Http.Headers; | |||
| using System.Security.Cryptography.X509Certificates; | |||
| using Newtonsoft.Json; | |||
| using System.Text.Json; | |||
There was a problem hiding this comment.
It seems that System.Text.Json is not used in this file.
| @@ -79,45 +80,32 @@ public override int GetHashCode() | |||
| } | |||
|
|
|||
| [Obsolete("The Legacy ACL system has been deprecated, please use Token, Role and Policy instead.")] | |||
| public class ACLTypeConverter : JsonConverter | |||
| public class ACLTypeConverter : JsonConverter<ACLType> | |||
There was a problem hiding this comment.
I think it would be good to also make all our custom JsonConverters to be internal so we reduce our public API surface.
| @@ -17,9 +17,10 @@ | |||
| // ----------------------------------------------------------------------- | |||
|
|
|||
| using System; | |||
| using System.Text.Json; | |||
There was a problem hiding this comment.
I think that System.Text.Json is not needed in this file
| switch (type) | ||
| { | ||
| case "client": | ||
| return ACLType.Client; | ||
| case "management": | ||
| return ACLType.Management; | ||
| default: | ||
| throw new ArgumentOutOfRangeException("serializer", type, | ||
| throw new ArgumentOutOfRangeException(nameof(reader), type, |
There was a problem hiding this comment.
I think that ArgumentOutOfRangeException takes the name of an argument that caused the exception, so it should be rather: throw new ArgumentOutOfRangeException(nameof(type), type,...
| @@ -22,7 +22,7 @@ | |||
| using System.Net.Http; | |||
| using System.Net.Http.Headers; | |||
| using System.Security.Cryptography.X509Certificates; | |||
| using Newtonsoft.Json; | |||
| using System.Text.Json; | |||
There was a problem hiding this comment.
System.Text.Json is not needed for this file.
| <Reference Include="System.Net.Http" /> | ||
| <Reference Include="System.Net.Http.WebRequest" /> | ||
| <Reference Include="Microsoft.CSharp" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="System.Text.Json" Version="5.0.1" /> |
There was a problem hiding this comment.
Version 5.0.1 is fairly new. The download stats on nuget.org show that the most popular version is still 4.7.2. Unfortunately, version 4.7.2 cannot be used for consuldotnet because it is missing some necessary functionalities (e.g. conditional JsonIgnore attribute).
I think that because of the slow adoption of new versions of the System.Text.Json library, we shouldn't take that dependency yet. If we did that while the version 4.7.2 is the most commonly used one we would make our consuldotnet library incompatible with majority of the codebases out there.
| @@ -44,7 +45,7 @@ public interface IAgentEndpoint | |||
| string NodeName { get; } | |||
| Task<string> GetNodeName(CancellationToken ct = default(CancellationToken)); | |||
| Task PassTTL(string checkID, string note, CancellationToken ct = default(CancellationToken)); | |||
| Task<QueryResult<Dictionary<string, Dictionary<string, dynamic>>>> Self(CancellationToken ct = default(CancellationToken)); | |||
| Task<QueryResult<Dictionary<string, Dictionary<string, JsonElement>>>> Self(CancellationToken ct = default(CancellationToken)); | |||
There was a problem hiding this comment.
We need to be really careful with changing the public API to avoid runtime failures when diamond dependency issue occurs.
I think that in this case the best would be to keep using the dynamic "type". To make it compile we could simply add an explicit package reference to Microsoft.CSharp Version="4.3.0". It will not by any real dependency addition as this package is currently referenced transitively via Newtonsoft.Json.
Convert all uses of Newtonsoft.Json to System.Text.Json
Closes #43
This converts the library to use System.Text.Json instead of Newtonsoft.Json to remove the dependency on that. All tests pass locally, but this is such a fundamental change that there may be unseen issues. This may also cause another problem: would System.Text.Json need to be ILMerged like Newtonsoft.Json? The original reason that it was being ilmerged was to stop dependency issues between common libraries, but I'm not sure if .net still has this problem with the new JSON libs.
There is also a breaking change in here: All references to
dynamicare gone (it was used in the agentSelfAPI) as compilation was failing due to needing the Microsoft.CSharp library referenced. Instead, rawJsonElements are being returned instead wheredynamicwas being used.