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 5 commits
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; }
}
}
17 changes: 13 additions & 4 deletions src/JWT/JWT.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@
<Authors>Alexander Batishchev, John Sheehan, Michael Lehenbauer</Authors>
<PackageTags>jwt;json</PackageTags>
<PackageLicense>CC0-1.0</PackageLicense>
<Version>7.2.0</Version>
<FileVersion>7.0.0.0</FileVersion>
<AssemblyVersion>7.0.0.0</AssemblyVersion>
<Version>8.0.0-beta1</Version>
<FileVersion>8.0.0.0</FileVersion>
<AssemblyVersion>8.0.0.0</AssemblyVersion>
<RootNamespace>JWT</RootNamespace>
<AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>
</PropertyGroup>
Expand All @@ -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>
59 changes: 59 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,59 @@
#if SYSTEMTEXTJSON

using System;
using System.Collections.Generic;
using System.Text.Json;

namespace JWT.Serializers
{
public sealed class SystemTextJsonSerializer : IJsonSerializer
{
public string Serialize(object obj)
{
return JsonSerializer.Serialize(obj);
}

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

// when deserializing a Dictionary<string, object>
// System.Text.Json create JsonElement objects for every value of the dictionary
// but application will expect to have native basic types (not a JsonElement class)
if (!(data is Dictionary<string, object> odata)) return data;

// we need to create another dictionary and fill it with the real values
// only basic types are supported (no complex object allowed, throw a NotSupportedException in these cases)
// number always converted to long (int64) basic type
var ndata = new Dictionary<string, object>();
foreach (var key in odata.Keys)
{
var value = (JsonElement)odata[key];
switch (value.ValueKind)
{
case JsonValueKind.String:
ndata.Add(key, value.GetString());
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.Undefined:
case JsonValueKind.Object:
case JsonValueKind.Array:
case JsonValueKind.Null:
default:
throw new NotSupportedException(nameof(value.ValueKind));
}
}

return ndata is T obj ? obj : data;
Copy link
Member

Choose a reason for hiding this comment

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

Can you please explain this line?

Copy link
Author

Choose a reason for hiding this comment

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

You cannot "return ndata" because ndata is an object of type Dictionary<string, object> not of type T. This expression cast ndata to the type T if possible. If it is not possible, default to return the object previuosly deserialized as is. Compiler know nothing about the relationship between T and Dictionary<string, object>, so this trick (or similar) is required.

}
}
}
#endif