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

Honor converters for underlying types of Nullable<T> specified with JsonConverterAttribute #32006

Merged
merged 14 commits into from
Feb 29, 2020

Conversation

CodeBlanch
Copy link
Contributor

Let's say we have a converter Int32Converter which knows how to convert typeof(int).

This works:

var options = new JsonSerializerOptions();
options.Converters.Add(new Int32Converter());

{
    int? myInt = JsonSerializer.Deserialize<int?>("null", options);
    Assert.False(myInt.HasValue);
}

{
    int? myInt = JsonSerializer.Deserialize<int?>("1", options);
    Assert.Equal(1, myInt.Value);
}

But this doesn't work:

private class Int32Class
{
    [JsonConverter(typeof(Int32Converter))]
    public int? MyInt { get; set; }
}

{
    Int32Class myIntClass = JsonSerializer.Deserialize<Int32Class>(@"{""MyInt"":null}");
    Assert.False(myIntClass.MyInt.HasValue);
}

{
    Int32Class myIntClass = JsonSerializer.Deserialize<Int32Class>(@"{""MyInt"":1}");
    Assert.Equal(1, myIntClass.MyInt.Value);
}

How the code works is it will first attempt the declared type. If it can't find a converter, and if it's a Nullable type, it will attempt to find a converter for the underlying type. But there's an exception thrown in the attribute case on the first pass that prevents the second phase from working.

This bug makes it a real pain to support Nullable in converters. Example, see: JsonTimeSpanConverter

To make it work today you have to handle Nullable in CanConvert & CreateConverter even though the engine won't call the converter for null. Fixing the bug enables all converters to also be Nullable converters, for free.

@steveharter
Copy link
Member

steveharter commented Feb 10, 2020

@layomia is there an issue for this?

@CodeBlanch
Copy link
Contributor Author

@steveharter FYI I synced up and tested against your latest changes, fix seems to still be needed.

@steveharter
Copy link
Member

@steveharter FYI I synced up and tested against your latest changes, fix seems to still be needed.

Yes after I added that comment a few minutes later I updated it to reflect that it wouldn't be fixed.

We should take your fix IMO. I'd like to here from one of the other reviewers as well.

@jozkee
Copy link
Member

jozkee commented Feb 13, 2020

So the fix is about fallingback on the options.Converters list since the JsonConverterAttribute does not support using the underlying type of the nullable?

That doesn't sounds very intuitive and would cause a silent breaking change on the hierarchy that we currently have for converter registration https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-converters-how-to#converter-registration-precedence

Consider the following example:

// Only allows numbers; otherwise throws.
private class MyFirstIntConverter : JsonConverter<int>
{
    public override int Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        if (reader.TokenType == JsonTokenType.Number)
        {
            return reader.GetInt32();
        }

        throw new JsonException();
    }

    public override void Write(Utf8JsonWriter writer, int value, JsonSerializerOptions options)
    {
        writer.WriteNumberValue(value);
    }
}

// Allows numbers and strings; otherwise throws.
private class MySecondIntConverter : JsonConverter<int>
{
    public override int Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
         if (reader.TokenType == JsonTokenType.Number)
        {
            return reader.GetInt32();
        }
        else if (reader.TokenType == JsonTokenType.String)
        {
            return Convert.ToInt32(reader.GetString());
        }

        throw new JsonException();
    }

    public override void Write(Utf8JsonWriter writer, int value, JsonSerializerOptions options)
    {
        writer.WriteNumberValue(value);
    }
}


private class Int32Class
{
    // Register the first converter on the property.
    [JsonConverter(typeof(MyFirstIntConverter))]
    public int? MyInt { get; set; }
}

public void TestConverterOnNullable()
{
    var options = new JsonSerializerOptions();
    // Register the second converter on the global options.
    options.Converters.Add(new MySecondIntConverter());

    Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<int?>(@"{""MyInt"":""100000""}", options));
    Assert.True(false, "You are not supposed to reach this");
}

Instead, we should avoid throwing when looking at the JsonConverterAttribute for nullables and do as we do for the options.Converters and try to use the converter form the attribute, if it matches the underlying type int on int?.

@CodeBlanch
Copy link
Contributor Author

@jozkee Interesting! No, it shouldn't be falling back to the options.Converters. I think something broke in @steveharter's refactor.

It used to flow back out to:

if (converter == null)
{
// No converter. We'll check below if there's a converter for the non-nullable type e.g.
// one that implements JsonConverter<int>, given the type is typeof(int?).
type = nullableUnderlyingType;
}

Which would then retry for the underlying type and pull the correct converter off the attribute.

I took a stab at fixing it but there's a lot of changes to digest. @steveharter Any ideas?

I'm going to change the test to use a converter for a made-up type so it doesn't fall back to one of the built-in converts and succeed when it should fail (like it's doing now).

@CodeBlanch
Copy link
Contributor Author

Update... looks like the second pass using the default converts was actually intentional in the new design. JsonValueConverterNullableFactory is now responsible for handling Nullable<> converters. BUT. the way it was falling to it (out of GetConverterFromAttribute), the specific converter requested on the attribute was lost. I changed the fix so instead of doing the fall-through, it is now stitched up to call the JsonValueConverterNullableFactory directly when it determines it needs to make a Nullable<> version of a converter specified via an attribute. Tricky one!

Copy link
Contributor

@layomia layomia left a comment

Choose a reason for hiding this comment

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

Looks good, thanks. A couple comments; then we can take this PR

  • Can you move your tests to a new CustomConverterTests.NullableTypes.cs file? The current locations don't seem specific.
  • Can you add tests for @jozkee's example above, showing that a converter handling the underlying type placed on a nullable type or property wins over one added at runtime?

@CodeBlanch
Copy link
Contributor Author

@layomia Updated. NullableCustomValueTypeChoosesAttributeOverOptions is the test added covering @jozkee's case explicitly.

@layomia
Copy link
Contributor

layomia commented Feb 28, 2020

Running CI again and updating the PR name to reflect the fix.

@layomia layomia changed the title System.Text.Json: JsonConverterAttribute with Nullable<T> throws exception Honor converters for underlying types of Nullable<T> specified with JsonConverterAttribute Feb 28, 2020
@joperezr
Copy link
Member

The issue that this build is hitting is #32926, which has been fixed already by #32927. Re-running CI won't get the fix, since instead we need to rebase this PR on top of master again to get that fix in. That should fix the build issue.

@joperezr joperezr mentioned this pull request Feb 28, 2020
@layomia layomia added this to the 5.0 milestone Feb 29, 2020
Copy link
Contributor

@layomia layomia left a comment

Choose a reason for hiding this comment

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

Thanks, @CodeBlanch!

@layomia layomia merged commit 17e2cae into dotnet:master Feb 29, 2020
@CodeBlanch CodeBlanch deleted the JsonConverterAttribute+NullableType branch February 29, 2020 01:43
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants