Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Newtonsoft.Json with System.Text.Json #283

Closed
wants to merge 6 commits into from
Closed

Replace Newtonsoft.Json with System.Text.Json #283

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a63c910
fix #218
luposky May 15, 2020
0e60971
Update JWT.csproj
abatishchev May 16, 2020
55d204d
Update SystemTextJsonSerializer.cs
abatishchev May 16, 2020
b2a1398
code cleanup
luposky May 16, 2020
255db65
comments + fix
luposky May 17, 2020
4f1adb5
Update SystemTextJsonSerializer.cs
abatishchev May 18, 2020
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
24 changes: 24 additions & 0 deletions src/JWT/Builder/JwtHeader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
using Newtonsoft.Json;
#if SYSTEMTEXTJSON
using System.Text.Json.Serialization;
#endif

namespace JWT.Builder
{
Expand All @@ -8,24 +11,45 @@ namespace JWT.Builder
public class JwtHeader
{
[JsonProperty("typ")]
#if SYSTEMTEXTJSON
Copy link
Member

@abatishchev abatishchev May 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Should it be one or another (conditionally)? I would define attribute alias in the top between #if #else #endif

Copy link
Author

Choose a reason for hiding this comment

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

I really don't know .. freedom of choiche is a value ... using netstandard20 I can choose beetween the two types of serializers (like in the asp.net core framework) so why forcing to use System.Text.Json if even microsoft don't force us to do so?

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise "replacing" won't be replacing but rather adding one more dependency, what users of Json.Net won't need hence would rightfully question.
Ideally either serializer would need to go to its own satellite package so users would be free to choose between one or another or maybe something third.
My point is that having both attributes and serializers basically means referencing them all and piling their respective attributes on each of this property.
I can easily imagine Unity folks or someone on .NET 3.5 who's unable to use either and who use DataContractJsonSerializer instead coming here and asking to add a third one.
So instead I would switch from a 3rd party to the 1st party serializer (and hope most of the users would be fine with that).

Copy link
Author

@luposky luposky May 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

yes, I understand ... I leave this choice to you ... I will push another commit with some improvment ...

[JsonPropertyName("typ")]
#endif
public string Type { get; set; }

[JsonProperty("cty")]
#if SYSTEMTEXTJSON
[JsonPropertyName("cty")]
#endif
public string ContentType { get; set; }

[JsonProperty("alg")]
#if SYSTEMTEXTJSON
[JsonPropertyName("alg")]
#endif
public string Algorithm { get; set; }

[JsonProperty("kid")]
#if SYSTEMTEXTJSON
[JsonPropertyName("kid")]
#endif
public string KeyId { get; set; }

[JsonProperty("x5u")]
#if SYSTEMTEXTJSON
[JsonPropertyName("x5u")]
#endif
public string X5u { get; set; }

[JsonProperty("x5c")]
#if SYSTEMTEXTJSON
[JsonPropertyName("x5c")]
#endif
public string X5c { get; set; }

[JsonProperty("x5t")]
#if SYSTEMTEXTJSON
[JsonPropertyName("x5t")]
#endif
public string X5t { get; set; }
}
}
11 changes: 10 additions & 1 deletion src/JWT/JWT.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,16 @@
<GenerateDocumentationFile>true</GenerateDocumentationFile>
</PropertyGroup>

<PropertyGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
abatishchev marked this conversation as resolved.
Show resolved Hide resolved
<DefineConstants>SYSTEMTEXTJSON;$(DefineConstants)</DefineConstants>
</PropertyGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
<PackageReference Include="System.Text.Json" Version="4.7.2" />
</ItemGroup>

<ItemGroup>
<PackageReference Include="Newtonsoft.Json" Version="9.0.1" />
<PackageReference Include="Newtonsoft.Json" Version="9.0.1" />
Copy link
Member

Choose a reason for hiding this comment

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

Should we reference either one or another?

Copy link
Member

Choose a reason for hiding this comment

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

And the another, already existing serializer must be included only if the target framework is lower than netstd2

Copy link
Author

Choose a reason for hiding this comment

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

I don't see any problem in referencing Newtonsoft.Json in netstandard20 ...
I simply like the option to use System.Text.Json ...
maybe in the future with net50 things can be different and we can drop support for Newtonsoft.Json

</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'netstandard1.3' OR '$(TargetFramework)' == 'netstandard2.0'">
Expand All @@ -49,4 +57,5 @@
<ItemGroup>
<PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.0.0" PrivateAssets="All" />
</ItemGroup>

</Project>
60 changes: 60 additions & 0 deletions src/JWT/Serializers/SystemTextJsonSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
#if SYSTEMTEXTJSON
namespace JWT.Serializers
{
using System;
abatishchev marked this conversation as resolved.
Show resolved Hide resolved
using System.Collections.Generic;
using System.Text.Json;
using JWT;

public class SystemTextJsonSerializer : IJsonSerializer
abatishchev marked this conversation as resolved.
Show resolved Hide resolved
{
public string Serialize(object obj)
{
// Implement using favorite JSON serializer
return JsonSerializer.Serialize(obj);
}

public T Deserialize<T>(string json)
{
// Implement using favorite JSON serializer
var data = JsonSerializer.Deserialize<T>(json);
abatishchev marked this conversation as resolved.
Show resolved Hide resolved

if (data is Dictionary<string, object> odata)
Copy link
Member

Choose a reason for hiding this comment

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

Invert the if?

{
var ndata = new Dictionary<string, object>();
luposky marked this conversation as resolved.
Show resolved Hide resolved
foreach (var key in odata.Keys)
{
var value = (JsonElement)odata[key];
switch (value.ValueKind)
{
case JsonValueKind.Undefined:
Copy link
Member

Choose a reason for hiding this comment

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

Please group all cases that do nothing into a single block

break;
case JsonValueKind.Object:
break;
case JsonValueKind.Array:
break;
case JsonValueKind.String:
ndata.Add(key, value.GetString());
Copy link
Member

Choose a reason for hiding this comment

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

And same here: multiple cases - one block of code

break;
case JsonValueKind.Number:
ndata.Add(key, value.GetInt64());
break;
case JsonValueKind.True:
ndata.Add(key, true);
break;
case JsonValueKind.False:
ndata.Add(key, false);
break;
case JsonValueKind.Null:
break;
}
}

if (ndata is T obj) return obj;
abatishchev marked this conversation as resolved.
Show resolved Hide resolved
}

return data;
}
}
}
#endif