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

[API Proposal]: Add json option to fallback to parsing value from string if type implements IParsable/ISpanParsable and enable it by default #88206

Closed
DoctorKrolic opened this issue Jun 29, 2023 · 4 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json

Comments

@DoctorKrolic
Copy link
Contributor

Background and motivation

Currently json parsing already handles some string parsing scenarios for some tipes, e.g. TimeSpan:

var json = """
    {
        "Delay": "00:05:00"
    }
    """;

var obj = JsonSerializer.Deserialize<MyClass>(json);
Console.WriteLine(obj.Delay);

class MyClass
{
    public TimeSpan Delay { get; set; }
}

This code doesn't throw and outputs 00:05:00 as expected.

However, in more general case, where I have my own object, that implements IParsable/ISpanParsable the same trick fails:

using System.Diagnostics.CodeAnalysis;
using System.Text.Json;

var json1 = """
    {
        "MyValue": {
            "Value": 2
        }
    }
    """;

var json2 = """
    {
        "MyValue": "2"
    }
    """;

var mc1 = JsonSerializer.Deserialize<MyClass1>(json1);
var mc2 = JsonSerializer.Deserialize<MyClass1>(json2);

Console.WriteLine(mc1!.MyValue.Value);
Console.WriteLine(mc2!.MyValue.Value);

class MyClass1
{
    public required MyClass MyValue { get; set; }
}

class MyClass : IParsable<MyClass>
{
    public int Value { get; set; }

    public static MyClass Parse(string s, IFormatProvider? provider)
        => new() { Value = int.Parse(s, provider) };

    public static bool TryParse([NotNullWhen(true)] string? s, IFormatProvider? provider, [MaybeNullWhen(false)] out MyClass result)
    {
        if (!int.TryParse(s, out var val))
        {
            result = default;
            return false;
        }

        result = new() { Value = val };
        return true;
    }
}

I expect 2 outputs of 2 to the console, but actually get a parse error in the second case.

Such capability will also enable currenctly unsupported parsing scenarios when type has several constructors and there is no parameterless one among them. In such cases if type implements IParsable/ISpanParsable and in the given json corresponding proeprty is a string, JsonSerializer can fallback to this code path as well.

API Proposal

Instead of treating TimeSpan and similar build-in types as "special" during json parsing, we can have JsonSerializerOptionsFlag, that controlls fallbacks to string parsing and set it to true by default:

namespace System.Text.Json;

public sealed class JsonSerializerOptions
{
    public bool FallbackToParsingFromString { get; set; } = true; // I believe name can be improved
}

This way we keep previous behavior and enable new scenarios with custom parsable types.

API Usage

Since the flag should be on by default, my code example from above should start to work. However, I can disable this flag to get the error again:

using System.Diagnostics.CodeAnalysis;
using System.Text.Json;

var json1 = """
    {
        "MyValue": {
            "Value": 2
        }
    }
    """;

var json2 = """
    {
        "MyValue": "2"
    }
    """;

var options = new JsonSerializerOptions
{
    FallbackToParsingFromString = false
};

var mc1 = JsonSerializer.Deserialize<MyClass1>(json1, options); // Still works
var mc2 = JsonSerializer.Deserialize<MyClass1>(json2, options); // Exception thrown here

Console.WriteLine(mc1!.MyValue.Value);
Console.WriteLine(mc2!.MyValue.Value);

class MyClass1
{
    public required MyClass MyValue { get; set; }
}

class MyClass : IParsable<MyClass>
{
    public int Value { get; set; }

    public static MyClass Parse(string s, IFormatProvider? provider)
        => new() { Value = int.Parse(s, provider) };

    public static bool TryParse([NotNullWhen(true)] string? s, IFormatProvider? provider, [MaybeNullWhen(false)] out MyClass result)
    {
        if (!int.TryParse(s, out var val))
        {
            result = default;
            return false;
        }

        result = new() { Value = val };
        return true;
    }
}

Alternative Designs

  1. Maybe there shouldn't be a flag for this at all? Just change existing behaviour to always fallback to parsing from string if applicable
  2. If (1) is accepted, should there be a flag to allow only parsing from string? Not sure when this can actually be helpful though
  3. Alternative to (1) and (2): instead of flag make an enum option with values like OnlyFullObject, CanFallbackToStringParse, OnlyAllowStringParse (not final names ofc.)

Risks

No response

@DoctorKrolic DoctorKrolic added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 29, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 29, 2023
@ghost
Copy link

ghost commented Jun 29, 2023

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Currently json parsing already handles some string parsing scenarios for some tipes, e.g. TimeSpan:

var json = """
    {
        "Delay": "00:05:00"
    }
    """;

var obj = JsonSerializer.Deserialize<MyClass>(json);
Console.WriteLine(obj.Delay);

class MyClass
{
    public TimeSpan Delay { get; set; }
}

This code doesn't throw and outputs 00:05:00 as expected.

However, in more general case, where I have my own object, that implements IParsable/ISpanParsable the same trick fails:

using System.Diagnostics.CodeAnalysis;
using System.Text.Json;

var json1 = """
    {
        "MyValue": {
            "Value": 2
        }
    }
    """;

var json2 = """
    {
        "MyValue": "2"
    }
    """;

var mc1 = JsonSerializer.Deserialize<MyClass1>(json1);
var mc2 = JsonSerializer.Deserialize<MyClass1>(json2);

Console.WriteLine(mc1!.MyValue.Value);
Console.WriteLine(mc2!.MyValue.Value);

class MyClass1
{
    public required MyClass MyValue { get; set; }
}

class MyClass : IParsable<MyClass>
{
    public int Value { get; set; }

    public static MyClass Parse(string s, IFormatProvider? provider)
        => new() { Value = int.Parse(s, provider) };

    public static bool TryParse([NotNullWhen(true)] string? s, IFormatProvider? provider, [MaybeNullWhen(false)] out MyClass result)
    {
        if (!int.TryParse(s, out var val))
        {
            result = default;
            return false;
        }

        result = new() { Value = val };
        return true;
    }
}

I expect 2 outputs of 2 to the console, but actually get a parse error in the second case.

Such capability will also enable currenctly unsupported parsing scenarios when type has several constructors and there is no parameterless one among them. In such cases if type implements IParsable/ISpanParsable and in the given json corresponding proeprty is a string, JsonSerializer can fallback to this code path as well.

API Proposal

Instead of treating TimeSpan and similar build-in types as "special" during json parsing, we can have JsonSerializerOptionsFlag, that controlls fallbacks to string parsing and set it to true by default:

namespace System.Text.Json;

public sealed class JsonSerializerOptions
{
    public bool FallbackToParsingFromString { get; set; } = true; // I believe name can be improved
}

This way we keep previous behavior and enable new scenarios with custom parsable types.

API Usage

Since the flag should be on by default, my code example from above should start to work. However, I can disable this flag to get the error again:

using System.Diagnostics.CodeAnalysis;
using System.Text.Json;

var json1 = """
    {
        "MyValue": {
            "Value": 2
        }
    }
    """;

var json2 = """
    {
        "MyValue": "2"
    }
    """;

var options = new JsonSerializerOptions
{
    FallbackToParsingFromString = false
};

var mc1 = JsonSerializer.Deserialize<MyClass1>(json1, options); // Still works
var mc2 = JsonSerializer.Deserialize<MyClass1>(json2, options); // Exception thrown here

Console.WriteLine(mc1!.MyValue.Value);
Console.WriteLine(mc2!.MyValue.Value);

class MyClass1
{
    public required MyClass MyValue { get; set; }
}

class MyClass : IParsable<MyClass>
{
    public int Value { get; set; }

    public static MyClass Parse(string s, IFormatProvider? provider)
        => new() { Value = int.Parse(s, provider) };

    public static bool TryParse([NotNullWhen(true)] string? s, IFormatProvider? provider, [MaybeNullWhen(false)] out MyClass result)
    {
        if (!int.TryParse(s, out var val))
        {
            result = default;
            return false;
        }

        result = new() { Value = val };
        return true;
    }
}

Alternative Designs

  1. Maybe there shouldn't be a flag for this at all? Just change existing behaviour to always fallback to parsing from string if applicable
  2. If (1) is accepted, should there be a flag to allow only parsing from string? Not sure when this can actually be helpful though
  3. Alternative to (1) and (2): instead of flag make an enum option with values like OnlyFullObject, CanFallbackToStringParse, OnlyAllowStringParse (not final names ofc.)

Risks

No response

Author: DoctorKrolic
Assignees: -
Labels:

api-suggestion, area-System.Text.Json, untriaged

Milestone: -

@neon-sunset
Copy link
Contributor

What about IUtf8SpanParsable since STJ focuses on avoiding transcoding the data unless necessary?

@gregsdennis
Copy link
Contributor

Would this automatically support numeric types encoded as strings as well? I'm not sure that's desirable default behavior. (It certainly isn't for my applications, but others have requested it.)

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Jun 30, 2023

Duplicate of #86529. We should instead focus on IUtf8SpanParseable/IUtf8SpanFormattable which is covered by #84305.

Echoing @gregsdennis's comment though I don't think we should default to these interfaces if implemented, since it's not likely that they will have been implemented with JSON serialization in mind. In the future it's more likely we could rely purpose-built interfaces such as

public interface IJsonSerializable<T>
{
    static abstract void Serialize(Utf8JsonWriter writer, T? value);
}

public interface IJsonDeserializable<T>
{
    static abstract T? Serialize(ref Utf8JsonReader reader);
}

or simply

public interface IJsonSerializable<T>
{
    static abstract JsonConverter<T> GetConverter(JsonSerializerOptions options);
}

@eiriktsarpalis eiriktsarpalis closed this as not planned Won't fix, can't repro, duplicate, stale Jun 30, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jun 30, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json
Projects
None yet
Development

No branches or pull requests

4 participants