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.

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

Add opt-in for converters to handle null #35826

Merged
merged 11 commits into from
May 8, 2020
2 changes: 2 additions & 0 deletions src/libraries/System.Text.Json/ref/System.Text.Json.cs
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,8 @@ public abstract partial class JsonConverter<T> : System.Text.Json.Serialization.
{
protected internal JsonConverter() { }
public override bool CanConvert(System.Type typeToConvert) { throw null; }
public virtual bool HandleNull { get { throw null; } }
[return: System.Diagnostics.CodeAnalysis.MaybeNull]
jozkee marked this conversation as resolved.
Show resolved Hide resolved
public abstract T Read(ref System.Text.Json.Utf8JsonReader reader, System.Type typeToConvert, System.Text.Json.JsonSerializerOptions options);
public abstract void Write(System.Text.Json.Utf8JsonWriter writer, T value, System.Text.Json.JsonSerializerOptions options);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ internal sealed override bool OnTryRead(
// Read the value and add.
reader.ReadWithVerify();
TValue element = elementConverter.Read(ref reader, typeof(TValue), options);
Add(element, options, ref state);
Add(element!, options, ref state);
jozkee marked this conversation as resolved.
Show resolved Hide resolved
}
}
else
Expand All @@ -129,7 +129,7 @@ internal sealed override bool OnTryRead(

// Get the value from the converter and add it.
elementConverter.TryRead(ref reader, typeof(TValue), options, ref state, out TValue element);
Add(element, options, ref state);
Add(element!, options, ref state);
}
}
}
Expand Down Expand Up @@ -247,7 +247,7 @@ internal sealed override bool OnTryRead(
return false;
}

Add(element, options, ref state);
Add(element!, options, ref state);
state.Current.EndElement();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using System.Diagnostics;

namespace System.Text.Json.Serialization.Converters
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ internal override bool OnTryRead(

// Obtain the CLR value from the JSON and apply to the object.
TElement element = elementConverter.Read(ref reader, elementConverter.TypeToConvert, options);
Add(element, ref state);
Add(element!, ref state);
}
}
else
Expand All @@ -83,7 +83,7 @@ internal override bool OnTryRead(

// Get the value from the converter and add it.
elementConverter.TryRead(ref reader, typeof(TElement), options, ref state, out TElement element);
Add(element, ref state);
Add(element!, ref state);
}
}
}
Expand Down Expand Up @@ -184,7 +184,7 @@ internal override bool OnTryRead(
return false;
}

Add(element, ref state);
Add(element!, ref state);

// No need to set PropertyState to TryRead since we're done with this element now.
state.Current.EndElement();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,28 +33,28 @@ protected override bool ReadAndCacheConstructorArgument(ref ReadStack state, ref
success = ((JsonParameterInfo<TArg0>)jsonParameterInfo).ReadJsonTyped(ref state, ref reader, out TArg0 arg0);
if (success)
{
arguments.Arg0 = arg0;
arguments.Arg0 = arg0!;
}
break;
case 1:
success = ((JsonParameterInfo<TArg1>)jsonParameterInfo).ReadJsonTyped(ref state, ref reader, out TArg1 arg1);
if (success)
{
arguments.Arg1 = arg1;
arguments.Arg1 = arg1!;
}
break;
case 2:
success = ((JsonParameterInfo<TArg2>)jsonParameterInfo).ReadJsonTyped(ref state, ref reader, out TArg2 arg2);
if (success)
{
arguments.Arg2 = arg2;
arguments.Arg2 = arg2!;
}
break;
case 3:
success = ((JsonParameterInfo<TArg3>)jsonParameterInfo).ReadJsonTyped(ref state, ref reader, out TArg3 arg3);
if (success)
{
arguments.Arg3 = arg3;
arguments.Arg3 = arg3!;
}
break;
default:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ internal override bool OnTryRead(
ThrowHelper.ThrowJsonException();
}

value = new KeyValuePair<TKey, TValue>(k, v);
value = new KeyValuePair<TKey, TValue>(k!, v!);
return true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ public NullableConverter(JsonConverter<T> converter)

public override T? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
// We do not check _converter.HandleNull, as the underlying struct cannot be null.
// A custom converter for some type T? can handle null.
if (reader.TokenType == JsonTokenType.Null)
{
return null;
Expand All @@ -30,6 +32,8 @@ public override void Write(Utf8JsonWriter writer, T? value, JsonSerializerOption
{
if (!value.HasValue)
{
// We do not check _converter.HandleNull, as the underlying struct cannot be null.
// A custom converter for some type T? can handle null.
writer.WriteNullValue();
}
else
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,6 @@ internal JsonConverter() { }

internal abstract Type? ElementType { get; }

/// <summary>
/// Cached value of ShouldHandleNullValue. It is cached since the converter should never
/// change the value depending on state and because it may contain non-trival logic.
/// </summary>
internal bool HandleNullValue { get; set; }

/// <summary>
/// Cached value of TypeToConvert.IsValueType, which is an expensive call.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Diagnostics.CodeAnalysis;

namespace System.Text.Json.Serialization
{
public partial class JsonConverter<T>
Expand All @@ -14,6 +16,7 @@ public partial class JsonConverter<T>
return ReadCore(ref reader, options, ref state);
}

[return: MaybeNull]
internal T ReadCore(
ref Utf8JsonReader reader,
JsonSerializerOptions options,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ protected internal JsonConverter()
// In the future, this will be check for !IsSealed (and excluding value types).
CanBePolymorphic = TypeToConvert == typeof(object);
IsValueType = TypeToConvert.IsValueType;
HandleNullValue = ShouldHandleNullValue;
HandleNull = IsValueType;
layomia marked this conversation as resolved.
Show resolved Hide resolved
CanBeNull = !IsValueType || Nullable.GetUnderlyingType(TypeToConvert) != null;
IsInternalConverter = GetType().Assembly == typeof(JsonConverter).Assembly;
CanUseDirectReadOrWrite = !CanBePolymorphic && IsInternalConverter && ClassType == ClassType.Value;
}
Expand Down Expand Up @@ -54,10 +55,19 @@ internal override sealed JsonParameterInfo CreateJsonParameterInfo()

internal override Type? ElementType => null;

// Allow a converter that can't be null to return a null value representation, such as JsonElement or Nullable<>.
// In other cases, this will likely cause an JsonException in the converter.
// Do not call this directly; it is cached in HandleNullValue.
internal virtual bool ShouldHandleNullValue => IsValueType;
/// <summary>
/// Indicates whether <see langword="null"/> should be passed to the converter on serialization,
/// and whether <see cref="JsonTokenType.Null"/> should be passed on deserialization.
/// </summary>
/// <remarks>
/// The default value is <see langword="true"/> for converters for value types, and <see langword="false"/> for converters for reference types.
/// </remarks>
public virtual bool HandleNull { get; }

/// <summary>
/// Can <see langword="null"/> be assigned to <see cref="TypeToConvert"/>?
/// </summary>
internal bool CanBeNull { get; }

/// <summary>
/// Is the converter built-in.
Expand All @@ -79,7 +89,7 @@ internal virtual bool OnTryWrite(Utf8JsonWriter writer, T value, JsonSerializerO
}

// Provide a default implementation for value converters.
internal virtual bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, [MaybeNullWhen(false)] out T value)
internal virtual bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, [MaybeNull] out T value)
{
value = Read(ref reader, typeToConvert, options);
return true;
Expand All @@ -95,19 +105,25 @@ internal virtual bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, J
/// <param name="typeToConvert">The <see cref="Type"/> being converted.</param>
/// <param name="options">The <see cref="JsonSerializerOptions"/> being used.</param>
/// <returns>The value that was converted.</returns>
[return: MaybeNull]
public abstract T Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options);

internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, out T value)
internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, [MaybeNull] out T value)
{
if (ClassType == ClassType.Value)
{
// A value converter should never be within a continuation.
Debug.Assert(!state.IsContinuation);

// For perf and converter simplicity, handle null here instead of forwarding to the converter.
if (reader.TokenType == JsonTokenType.Null && !HandleNullValue)
if (reader.TokenType == JsonTokenType.Null && !HandleNull)
{
value = default!;
if (!CanBeNull)
{
ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(TypeToConvert);
}

value = default;
return true;
}

Expand Down Expand Up @@ -147,10 +163,15 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali
// For performance, only perform validation on internal converters on debug builds.
if (IsInternalConverter)
{
if (reader.TokenType == JsonTokenType.Null && !HandleNullValue && !wasContinuation)
if (reader.TokenType == JsonTokenType.Null && !HandleNull && !wasContinuation)
{
if (!CanBeNull)
{
ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(TypeToConvert);
}

// For perf and converter simplicity, handle null here instead of forwarding to the converter.
value = default!;
value = default;
success = true;
}
else
Expand All @@ -164,9 +185,14 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali
if (!wasContinuation)
{
// For perf and converter simplicity, handle null here instead of forwarding to the converter.
if (reader.TokenType == JsonTokenType.Null && !HandleNullValue)
if (reader.TokenType == JsonTokenType.Null && !HandleNull)
{
value = default!;
if (!CanBeNull)
{
ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(TypeToConvert);
}

value = default;
state.Pop(true);
return true;
}
Expand Down Expand Up @@ -213,7 +239,20 @@ internal bool TryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions opt
{
if (value == null)
{
writer.WriteNullValue();
if (!HandleNull)
{
writer.WriteNullValue();
}
else
{
Debug.Assert(ClassType == ClassType.Value);
Debug.Assert(!state.IsContinuation);

int originalPropertyDepth = writer.CurrentDepth;
Write(writer, value, options);
VerifyWrite(originalPropertyDepth, writer);
}

return true;
}

Expand All @@ -236,15 +275,12 @@ internal bool TryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions opt
}
}
}
else
else if (value == null && !HandleNull)
{
// We do not pass null values to converters unless HandleNullValue is true. Null values for properties were
// We do not pass null values to converters unless HandleNull is true. Null values for properties were
// already handled in GetMemberAndWriteJson() so we don't need to check for IgnoreNullValues here.
if (value == null && !HandleNullValue)
{
writer.WriteNullValue();
return true;
}
writer.WriteNullValue();
return true;
}

if (ClassType == ClassType.Value)
Expand All @@ -255,7 +291,6 @@ internal bool TryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions opt

Write(writer, value, options);
VerifyWrite(originalPropertyDepth, writer);

return true;
}

Expand Down Expand Up @@ -299,48 +334,30 @@ internal bool TryWriteDataExtensionProperty(Utf8JsonWriter writer, T value, Json
ThrowHelper.ThrowJsonException_SerializerCycleDetected(options.EffectiveMaxDepth);
}

bool success;
JsonDictionaryConverter<T> dictionaryConverter = (JsonDictionaryConverter<T>)this;

if (ClassType == ClassType.Value)
layomia marked this conversation as resolved.
Show resolved Hide resolved
{
Debug.Assert(!state.IsContinuation);

int originalPropertyDepth = writer.CurrentDepth;
bool isContinuation = state.IsContinuation;
bool success;

// Ignore the naming policy for extension data.
state.Current.IgnoreDictionaryKeyPolicy = true;
state.Push();

success = dictionaryConverter.OnWriteResume(writer, value, options, ref state);
if (success)
{
VerifyWrite(originalPropertyDepth, writer);
}
}
else
if (!isContinuation)
{
bool isContinuation = state.IsContinuation;

state.Push();

if (!isContinuation)
{
Debug.Assert(state.Current.OriginalDepth == 0);
state.Current.OriginalDepth = writer.CurrentDepth;
}

// Ignore the naming policy for extension data.
state.Current.IgnoreDictionaryKeyPolicy = true;
Debug.Assert(state.Current.OriginalDepth == 0);
state.Current.OriginalDepth = writer.CurrentDepth;
}

success = dictionaryConverter.OnWriteResume(writer, value, options, ref state);
if (success)
{
VerifyWrite(state.Current.OriginalDepth, writer);
}
// Ignore the naming policy for extension data.
state.Current.IgnoreDictionaryKeyPolicy = true;

state.Pop(success);
success = dictionaryConverter.OnWriteResume(writer, value, options, ref state);
if (success)
{
VerifyWrite(state.Current.OriginalDepth, writer);
}

state.Pop(success);

return success;
}

Expand Down
Loading