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

STJ: Using default system converter does not work when writing and provided options #111429

Closed
Mad-Lynx opened this issue Jan 14, 2025 · 4 comments

Comments

@Mad-Lynx
Copy link

Mad-Lynx commented Jan 14, 2025

Description

According to the documentation , to get the default converter we can just use JsonSerializerOptions.Default.GetConverter(typeof(T)).
That works... kinda.
It will return the default converter, but the problem is if that converter derives from JsonResumableConverter then writing would not work.

While writing, we will try to get JsonTypeInfo -

JsonTypeInfo typeInfo = options.GetTypeInfoInternal(typeof(T));

There is small issue:
We will try to do that, from the options, which could have converter for the type T. In most cases that's perfectly fine, and desirable, but we just got the converter from the "Default" options... so I'm not expecting to have the converters there.

The problem is, if we will do:

JsonSerializerOptions.Default.TryGetTypeInfo(typeof(Foo), out var info);

There is no converter for Foo, so default would be LargeObjectWithParameterizedConstructorConverter (or SmallObjectWithParameterizedConstructorConverter) which will have

private protected sealed override ConverterStrategy GetDefaultConverterStrategy() => ConverterStrategy.Object;

And so, JsonTypeInfo will be populated with the Properties

But when we do:

options.TryGetTypeInfo(typeof(Foo), out var info);

Now, there is our converter defined... which derived from standard JsonConverter<T> which:

private protected override ConverterStrategy GetDefaultConverterStrategy() => ConverterStrategy.Value;

So, the JsonTypeInfo will not have any Properties defined.

So long story short:
even if we got the converter from the default options, while doing write, we will try to get list of properties from the current options, which would result in having empty list.

Reproduction Steps

using System.Text.Json;
using System.Text.Json.Serialization;

var options = new JsonSerializerOptions();
options.Converters.Add(new FallbackConverter<int>());
options.Converters.Add(new FallbackConverter<Foo>());

Console.WriteLine($"Int: {JsonSerializer.Serialize(123, options)}");
Console.WriteLine($"Obj: {JsonSerializer.Serialize(new Foo(123), options)}");

record Foo(int Value);

sealed class FallbackConverter<T> : JsonConverter<T>
{
    private readonly static JsonConverter<T> _fallbackConverter = (JsonConverter<T>)JsonSerializerOptions.Default.GetConverter(typeof(T));

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

    public override T Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
        => throw new NotImplementedException();
}

Expected behavior

Regardless if we will define

options.Converters.Add(new FallbackConverter<int>());
options.Converters.Add(new FallbackConverter<Foo>());

or not, the output should be the same:

Int: 123
Obj: {"Value":123}

Actual behavior

Current output is broken:

Int: 123
Obj: {}

Regression?

No.
In previous versions (<net9) that code would throw System.NullReferenceException in OnTryWrite
But the reason was the same - Properties was empty (to be exact PropertyCache was null)

Known Workarounds

None.

Configuration

NET9

Other information

This issue arise when we had to create a custom converter to handle "Read" in a custom manner, but we wanted to keep "Write" as a default behavior.
But we need to keep/pass the options as there are many other types that have to handled differently.

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jan 14, 2025
Copy link
Contributor

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

@eiriktsarpalis
Copy link
Member

I believe this is a duplicate of #50205

@Mad-Lynx
Copy link
Author

Hi @eiriktsarpalis I think the underlying cause is the same, although dunno if that is strictly duplicate.

  1. the other tickets talks about "Read", I'm referring to "Write" (I know ultimately it is JsonTypeInfo issue, but couldn't find the reported issue about writes)
  2. The other one throw the exceptions, which is clearly "you are doing something wrong"... in this case (now in net9, as net7,8 were throwing as well), it's just works... just the data is wrong/empty.
    So if not our tests, we would not pick it up.
  3. The example is taken (almost) from the documentation (there was a read, I'm using write).
    So if it's know issue, we should probably add some disclaimer in the "Use default system converter" section, that this technique works, but... not for complex types 😉

The #50205 was reported quite a while ago... any insides on when the underlying issue could be fixed?

@eiriktsarpalis
Copy link
Member

It's a different symptom of the same underlying bug, namely that certain built-in converters cannot function independently of the JsonSerializerOptions they were originally created for.

The #50205 was reported quite a while ago... any insides on when the underlying issue could be fixed?

It's not something that is currently being prioritized, unfortunately. Please upvote that original issue so that it's most likely to be included in future planning iterations.

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants