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

Provide opt-in for custom converters to handle null #34439

Closed
layomia opened this issue Apr 2, 2020 · 3 comments · Fixed by #35826
Closed

Provide opt-in for custom converters to handle null #34439

layomia opened this issue Apr 2, 2020 · 3 comments · Fixed by #35826
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Milestone

Comments

@layomia
Copy link
Contributor

layomia commented Apr 2, 2020

Motivation

The serializer does not pass null to custom converters for reference types on serialization and deserialization. Instead, it handles null by returning a null instance on deserialization (or throwing JsonException/returning a default value for primitive value types, depending on if there's an internal converter), and writing null directly with the writer on serialization. This is primarily for performance to skip an extra call to the converter. In addition, we didn't want callers to have to check for null at the start of every Read and Write method override.

However, the serializer does pass null tokens on deserialization to converters for value types. This is to support internal logic for deserializing JsonElement and Nullable<T> instances, where null is a valid token.

null is also passed to converters for value types because it is not a valid CLR value for these types. This way, the converter can determine what to do with this "invalid" token.

This feature provides a way for custom converters to handle null, regardless of if they convert value or reference types. It has to be opt-in, otherwise it would be a breaking change.

Users may want to handle null for various reasons, including validation and setting/writing default values instead of null. Some scenarios:

API Proposal

namespace System.Text.Json.Serialization
{
    public abstract partial class JsonConverter<T> : System.Text.Json.Serialization.JsonConverter
    {
        /// <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.
        /// The default value is <see langword="true"> for converters for value types,
        /// and <see langword="false"> for converters for reference types.
        /// </summary>
        public virtual bool HandleNull { get { throw null; } }
    }
}

Usage

public class Profile
{
    public string FirstName { get; set; }

    public string LastName { get; set; }

    [JsonConverter(typeof(ProfileUriConverter))]
    public Uri ProfileUri { get; set; }
}

public class ProfileUriConverter : JsonConverter<Uri>
{
    public override bool HandleNull { get; } = true;

    public override Uri Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        if (reader.TokeType == JsonTokenType.Null)
        {
            return new Uri("https://mycompany.com");
        }

        string uriString = reader.GetString();
        if (Uri.TryCreate(uriString, UriKind.RelativeOrAbsolute, out Uri? value))
        {
            return value;
        }

        throw new JsonException();
    }

    public override void Write(Utf8JsonWriter writer, Uri value, JsonSerializerOptions options)
    {
        if (value == null)
        {
            value = new Uri("https://mycompany.com");
        }

        writer.WriteStringValue(value.OriginalString);
    }
}

public static void HandleUserProfile()
{
    string json = @"{""FirstName"":""Jet"",""LastName"":""Doe"",""ProfileUri"":null}";

    Profile profile = JsonSerializer.Deserialize<Profile>(json);
    Console.WriteLine(profile.FirstName); // Jet
    Console.WriteLine(profile.LastName); // Doe
    Console.WriteLine(profile.ProfileUri.OrignalString); // https://mycompany.com

    profile.ProfileUri = null;

    json = JsonSerializer.Serialize(profile);
    Console.WriteLine(json); // {"FirstName":"Jet","LastName":"Doe","ProfileUri":"https://mycompany.com"}
}

Notes

  • We can't make HandleNull default to false for value types because that would be a breaking change.
@layomia layomia added this to the 5.0 milestone Apr 2, 2020
@layomia layomia self-assigned this Apr 2, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Apr 2, 2020
@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Apr 2, 2020
@layomia layomia added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review labels Apr 5, 2020
@terrajobst terrajobst added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review labels Apr 7, 2020
@terrajobst
Copy link
Member

terrajobst commented Apr 7, 2020

Video

  • In this proposal the converter is only called when a null value is included in the payload. If the property is missing in the payload, the the convert still won't be called. Does this satisfy the customer request? While this should probably be an independent feature it seems we'll want to make sure the design for both will work well together.
  • Should this be on the non-generic base JsonConverter?
    • We concluded no, because the Read/Write methods are also only on JsonConverter<T>
namespace System.Text.Json.Serialization
{
    public partial class JsonConverter<T>
    {
        public virtual bool HandleNull { get; }
    }
}

@bartonjs
Copy link
Member

Good as proposed, consider nullable annotations on the callback methods.

    public abstract partial class JsonConverter<T> : System.Text.Json.Serialization.JsonConverter
    {
        // Also update the nullable annotations for the serialize (parameter)/deserialize (return) callbacks
        public virtual bool HandleNull { get { throw null; } }
    }

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Apr 30, 2020
@nathan-alden-sr
Copy link

nathan-alden-sr commented Jun 8, 2020

In case anyone needs a solution to this problem specifically to be able to serialize an empty IList<T> as null instead of [], I created a wrapper class. See here.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants