feat: Phasing out Newtonsoft.Json in favour of System.Text.Json#1046
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1046 +/- ##
==========================================
+ Coverage 32.04% 32.06% +0.01%
==========================================
Files 2802 2803 +1
Lines 111115 111147 +32
Branches 6492 6495 +3
==========================================
+ Hits 35607 35637 +30
- Misses 73643 73647 +4
+ Partials 1865 1863 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
nandan-bhat
left a comment
There was a problem hiding this comment.
Left a few inline comments on the Newtonsoft to System.Text.Json migration. Overall it looks solid and the converter/UserInfo test coverage is good. My main worry is the SignupUserResponse private field handling, plus a couple of spots where the behaviour quietly changes between the two libraries.
| [JsonProperty("_id")] | ||
| [JsonInclude] | ||
| [JsonPropertyName("_id")] | ||
| private string _id; // Standard connection |
There was a problem hiding this comment.
These backing fields are private and rely on [JsonInclude] to get populated. System.Text.Json does not treat [JsonInclude] on private fields the same way Newtonsoft did. Depending on the runtime it either ignores them or throws when it builds the contract. If they get ignored, Id will always be null because none of _id, id or user_id ever get set.
I could not find any test that deserializes SignupUserResponse, so this would slip through. Can we add a test that feeds _id, id and user_id payloads separately and checks Id resolves in each case? If the attribute does not work on net462 / netstandard2.0, we may need the custom TypeInfoResolver approach that ManagementApi already uses, or make these members non private.
There was a problem hiding this comment.
This is fixed with a custom serialiser in commit
| /// </summary> | ||
| [JsonProperty("user_metadata")] | ||
| [JsonPropertyName("user_metadata")] | ||
| public dynamic UserMetadata { get; set; } |
There was a problem hiding this comment.
Heads up: UserMetadata has no converter, so under System.Text.Json this deserializes to a JsonElement instead of the JObject Newtonsoft used to give. Any consumer doing dynamic md = response.UserMetadata; md.someField will now throw RuntimeBinderException. We kept the old shape for UserInfo.AdditionalClaims through the obsolete shim, but this one changes silently. At the very least we should list it as a breaking change.
There was a problem hiding this comment.
Documented the behaviour. We will also call this out as a breaking change on release.
The field is dynamic which already means there is no guarantee on the type (structure). However, since we are knowingly breaking this type, we can ensure we call this out explicitly.
| /// </summary> | ||
| [JsonProperty("request_language")] | ||
| [JsonPropertyName("request_language")] | ||
| public object RequestLanguage { get; set; } |
There was a problem hiding this comment.
Same story as UserMetadata. This was a JValue under Newtonsoft and becomes a JsonElement now, so a cast like (string)response.RequestLanguage will throw InvalidCastException. Worth calling out as a behaviour change, or exposing it as JsonElement on purpose.
| var extraDataProp = props.FirstOrDefault(p => p.PropertyType == typeof(Dictionary<string, string>)); | ||
|
|
||
| foreach (var jp in JObject.Load(reader).Properties()) | ||
| foreach (var property in doc.RootElement.EnumerateObject()) |
There was a problem hiding this comment.
EnumerateObject() throws InvalidOperationException when the content is valid JSON but not an object (for example 123, a plain string, or an array). That is not a JsonException, so the try/catch fallback in ApiError.Parse will not catch it and it escapes.
The old Newtonsoft path threw a JsonException here, so the fallback used to kick in and return the raw content. Can we add a guard like if (doc.RootElement.ValueKind != JsonValueKind.Object) and return early, plus a test for something like ApiError.Parse("123")?
| case JsonTokenType.Number: | ||
| return Add(Epoch, TimeSpan.FromSeconds(reader.GetInt64())); | ||
| case JsonTokenType.String: | ||
| return reader.GetDateTime(); |
There was a problem hiding this comment.
reader.GetDateTime() only accepts ISO 8601 style strings and throws FormatException on anything else. The old converter was lenient and just returned the raw value. So for any non ISO date string this now throws instead of degrading. Auth0 normally sends epoch or ISO here so the real risk is low, but a DateTime.TryParse fallback (or returning null) would keep the old leniency.
| <!-- Package Dependencies --> | ||
| <ItemGroup> | ||
| <PackageReference Include="Newtonsoft.Json" Version="13.0.4" /> | ||
| <PackageReference Include="System.Text.Json" Version="8.0.5" /> |
There was a problem hiding this comment.
Small consistency thing: Core pins System.Text.Json to 8.0.5 while AuthenticationApi and ManagementApi are on 9.0.9. NuGet will unify to 9.0.9 at runtime so it builds fine, but the different floor here is easy to trip over later, and 8.0.x is a separate servicing line for security patches. Can we bump this to 9.0.9 to match?
| namespace Auth0.Core.Serialization; | ||
|
|
||
| internal class StringOrStringArrayJsonConverter : JsonConverter | ||
| internal class StringOrStringArrayJsonConverter : JsonConverter<object?> |
There was a problem hiding this comment.
Nit: I could not find any [JsonConverter(typeof(StringOrStringArrayJsonConverter))] usage in src, so this looks unused. If it is dead code we could drop it along with its tests instead of porting it over. Feel free to ignore if it is kept on purpose for something planned.
Changes
This PR phases out Newtonsoft.Json in favour of System.Text.Json (STJ) across
Auth0.CoreandAuth0.AuthenticationApi, bringing them in line withAuth0.ManagementApi(already on STJ) and removing the Newtonsoft dependency from the serialization path. The
migration is designed for zero-to-minimum breaking changes for consumers.
Auth0.Core
Auth0JsonSerializerOptions.Default— a shared internal options singleton(
PropertyNameCaseInsensitive = true,DefaultIgnoreCondition = WhenWritingNull) thatreproduces Newtonsoft's case-insensitive matching and
NullValueHandling.Ignore.ApiErrorConverteras an STJJsonConverter<ApiError>.StringOrObjectAsStringConverterandStringOrStringArrayJsonConvertertoJsonConverter<T>. These converters areinternal, so the base-type change is not partof the public contract.
Auth0.AuthenticationApi
HttpClientAuthenticationConnectionto STJ(
[JsonProperty]→[JsonPropertyName], Newtonsoft[JsonIgnore]→ STJ[JsonIgnore],serialize/deserialize via the shared options). Serialization uses
body.GetType()sopolymorphic/derived properties are still emitted.
Public API impact (deprecation, not removal)
UserInfo.AdditionalClaims(IDictionary<string, JToken>) is kept but marked[Obsolete]and[JsonIgnore]. It is now a computed shim that reads from a newAdditionalClaimsJsonproperty. Existing consumer code that readsAdditionalClaimscontinues to compile and still returns
JTokens.UserInfo.AdditionalClaimsJson(IDictionary<string, JsonElement>) as the real[JsonExtensionData]sink (additive; defaults to an empty dictionary so the obsoleteaccessor is null-safe).
are attribute-only.
Intentional behavioural delta
StringOrObjectAsStringConverternow emits compact JSON where Newtonsoft emittedpretty-printed output (e.g.
{"innerValue":"test"}instead of the indented form). This isasserted explicitly in the tests.
References
Testing
Checklist