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

Allow specifying JsonConverter attribute annotations on properties of unsupported type. #95893

Open
xbotter opened this issue Dec 12, 2023 · 11 comments
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@xbotter
Copy link

xbotter commented Dec 12, 2023

Description

When I tried to add a custom JsonConverter attribute to the Encoding property, an error was thrown.

Reproduction Steps

var model = new Model();
Console.WriteLine(JsonSerializer.Serialize(model));

class Model
{
    [JsonConverter(typeof(EncodingConverter))]
    public Encoding Encoding { get; set; } = Encoding.UTF8;
}

internal class EncodingConverter : JsonConverter<Encoding>
{
    public override Encoding? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        var name = reader.GetString();
        if (name == null)
            return null;
        return Encoding.GetEncoding(name);
    }

    public override void Write(Utf8JsonWriter writer, Encoding value, JsonSerializerOptions options)
    {
        writer.WriteStringValue(value.WebName);
    }
}

Expected behavior

{"Encoding":"utf-8"}

Actual behavior

Unhandled exception. System.InvalidOperationException: The type 'System.ReadOnlySpan1[System.Byte]' of property 'Preamble' on type 'System.Text.Encoding' is invalid for serialization or deserialization because it is a pointer type, is a ref struct, or contains generic parameters that have not been replaced by specific types. at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_CannotSerializeInvalidType(Type typeToConvert, Type declaringType, MemberInfo memberInfo) at System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.CreatePropertyInfo(JsonTypeInfo typeInfo, Type typeToConvert, MemberInfo memberInfo, JsonSerializerOptions options, Boolean shouldCheckForRequiredKeyword, Boolean hasJsonIncludeAttribute) at System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.AddMembersDeclaredBySuperType(JsonTypeInfo typeInfo, Type currentType, Boolean constructorHasSetsRequiredMembersAttribute, PropertyHierarchyResolutionState& state) at System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.PopulateProperties(JsonTypeInfo typeInfo) at System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.CreateTypeInfoCore(Type type, JsonConverter converter, JsonSerializerOptions options) at System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.GetTypeInfo(Type type, JsonSerializerOptions options) at System.Text.Json.JsonSerializerOptions.GetTypeInfoNoCaching(Type type) at System.Text.Json.JsonSerializerOptions.CachingContext.CreateCacheEntry(Type type, CachingContext context) --- End of stack trace from previous location --- at System.Text.Json.JsonSerializerOptions.GetTypeInfoInternal(Type type, Boolean ensureConfigured, Nullable1 ensureNotNull, Boolean resolveIfMutable, Boolean fallBackToNearestAncestorType)
at System.Text.Json.Serialization.Metadata.JsonPropertyInfo.Configure()
at System.Text.Json.Serialization.Metadata.JsonTypeInfo.ConfigureProperties()
at System.Text.Json.Serialization.Metadata.JsonTypeInfo.Configure()
at System.Text.Json.Serialization.Metadata.JsonTypeInfo.g__ConfigureSynchronized|172_0()
at System.Text.Json.JsonSerializerOptions.GetTypeInfoInternal(Type type, Boolean ensureConfigured, Nullable`1 ensureNotNull, Boolean resolveIfMutable, Boolean fallBackToNearestAncestorType)
at System.Text.Json.JsonSerializerOptions.GetTypeInfoForRootType(Type type, Boolean fallBackToNearestAncestorType)
at System.Text.Json.JsonSerializer.GetTypeInfo[T](JsonSerializerOptions options)
at System.Text.Json.JsonSerializer.Serialize[TValue](TValue value, JsonSerializerOptions options)
at Program.

$(String[] args)

Regression?

net6.0 🆗
net7.0 🆗
net8.0 ❌

Known Workarounds

No response

Configuration

No response

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 12, 2023
@ghost
Copy link

ghost commented Dec 12, 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

Description

When I tried to add a custom JsonConverter attribute to the Encoding property, an error was thrown.

Reproduction Steps

var model = new Model();
Console.WriteLine(JsonSerializer.Serialize(model));

class Model
{
    [JsonConverter(typeof(EncodingConverter))]
    public Encoding Encoding { get; set; } = Encoding.UTF8;
}

internal class EncodingConverter : JsonConverter<Encoding>
{
    public override Encoding? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        var name = reader.GetString();
        if (name == null)
            return null;
        return Encoding.GetEncoding(name);
    }

    public override void Write(Utf8JsonWriter writer, Encoding value, JsonSerializerOptions options)
    {
        writer.WriteStringValue(value.WebName);
    }
}

Expected behavior

{"Encoding":"utf-8"}

Actual behavior

Unhandled exception. System.InvalidOperationException: The type 'System.ReadOnlySpan1[System.Byte]' of property 'Preamble' on type 'System.Text.Encoding' is invalid for serialization or deserialization because it is a pointer type, is a ref struct, or contains generic parameters that have not been replaced by specific types. at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_CannotSerializeInvalidType(Type typeToConvert, Type declaringType, MemberInfo memberInfo) at System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.CreatePropertyInfo(JsonTypeInfo typeInfo, Type typeToConvert, MemberInfo memberInfo, JsonSerializerOptions options, Boolean shouldCheckForRequiredKeyword, Boolean hasJsonIncludeAttribute) at System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.AddMembersDeclaredBySuperType(JsonTypeInfo typeInfo, Type currentType, Boolean constructorHasSetsRequiredMembersAttribute, PropertyHierarchyResolutionState& state) at System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.PopulateProperties(JsonTypeInfo typeInfo) at System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.CreateTypeInfoCore(Type type, JsonConverter converter, JsonSerializerOptions options) at System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver.GetTypeInfo(Type type, JsonSerializerOptions options) at System.Text.Json.JsonSerializerOptions.GetTypeInfoNoCaching(Type type) at System.Text.Json.JsonSerializerOptions.CachingContext.CreateCacheEntry(Type type, CachingContext context) --- End of stack trace from previous location --- at System.Text.Json.JsonSerializerOptions.GetTypeInfoInternal(Type type, Boolean ensureConfigured, Nullable1 ensureNotNull, Boolean resolveIfMutable, Boolean fallBackToNearestAncestorType)
at System.Text.Json.Serialization.Metadata.JsonPropertyInfo.Configure()
at System.Text.Json.Serialization.Metadata.JsonTypeInfo.ConfigureProperties()
at System.Text.Json.Serialization.Metadata.JsonTypeInfo.Configure()
at System.Text.Json.Serialization.Metadata.JsonTypeInfo.g__ConfigureSynchronized|172_0()
at System.Text.Json.JsonSerializerOptions.GetTypeInfoInternal(Type type, Boolean ensureConfigured, Nullable`1 ensureNotNull, Boolean resolveIfMutable, Boolean fallBackToNearestAncestorType)
at System.Text.Json.JsonSerializerOptions.GetTypeInfoForRootType(Type type, Boolean fallBackToNearestAncestorType)
at System.Text.Json.JsonSerializer.GetTypeInfo[T](JsonSerializerOptions options)
at System.Text.Json.JsonSerializer.Serialize[TValue](TValue value, JsonSerializerOptions options)
at Program.

$(String[] args)

Regression?

net6.0 🆗
net7.0 🆗
net8.0 ❌

Known Workarounds

No response

Configuration

No response

Other information

No response

Author: xbotter
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@eiriktsarpalis
Copy link
Member

This is an intentional breaking change introduced in .NET 8. TL;DR the serializer walks the entire type graph to determine whether parts of it are being serialized using the fast-path source generator, but because the custom converter is registered at the property level and not the type itself, an InvalidOperationException is thrown.

You can work around the issue by registering your custom converter at the type level like so:

var options = new JsonSerializerOptions { Converters = { new EncodingConverter() } };
var model = new Model();
Console.WriteLine(JsonSerializer.Serialize(model, options)); // {"Encoding":"utf-8"}

class Model
{
    public Encoding Encoding { get; set; } = Encoding.UTF8;
}

@martindevans
Copy link

Unfortunately that doesn't really work for this use case.

The Model is a public class exported from a library (LLamaSharp), which the user might want to serialize. We have various converters which serialize internal parts of the model (note the EncodingConverter is internal).

This means the class can be encoded/decoded and it "just works" without any special configuration of the JsonSerializerOptions by the user. e.g. we can add new properties in the future with converter attributes and it's still compatible.

Is there a way for this to work?

@martindevans
Copy link

Looking at the docs for the breaking change more closely, I'm not certain it covers the issue we're seeing here. It's talking about new deserialization error, however the problem here is a serialization failure introduced by upgrading from net6 to net8.

[Fact]
public void SerializeRoundTripSystemTextJson()
{
    var expected = new ModelParams("abc/123")
    {
        BatchSize = 17,
        ContextSize = 42,
        Seed = 42,
        GpuLayerCount = 111,
        TensorSplits = { [0] = 3 }
    };

    var json = System.Text.Json.JsonSerializer.Serialize(expected); // <-- the error xbotter posted comes from this line
    var actual = System.Text.Json.JsonSerializer.Deserialize<ModelParams>(json)!;

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Dec 12, 2023

Is there a way for this to work?

I would recommend either using a DTO with a plain old string that doesn't encapsulate Encoding instances. Alternatively you could try adding a JsonIgnore attribute on Encoding property and use a satellite string property for serializing the web name. Roughly:

class Model
{
    [JsonIgnore]
    public Encoding Encoding { get; set; } = Encoding.UTF8;
    [JsonPropertyName("Encoding")]
    public string EncodingName { get => Encoding.WebName; set { Encoding = Encoding.GetEncoding(value); } }
}

If you want to hide the satellite from users, you can either 1) mark the field private and also include a JsonIncludeAttribute (which doesn't work for source generators) or 2) add an EditorBrowsable.Never annotation.

Looking at the docs for the breaking change more closely, I'm not certain it covers the issue we're seeing here.

The doc speaks of deserialization failing as one particular symptom of the wider breaking change, but this case still falls under the same umbrella.

@eiriktsarpalis
Copy link
Member

(note the EncodingConverter is internal)

This is unrelated to the conversation at hand, but given that this is a library component exposed to users I though I'd point out that converters for public types should also be made public because otherwise they won't work with source generated serializers.

@martindevans
Copy link

Thanks for your help, I've gone with something similar to the "satellite" property you suggested.

@eiriktsarpalis eiriktsarpalis changed the title JsonSerializer cannot use the JsonConverterAttribute added to Encoding Allow specifying JsonConverter attribute annotations on properties of unsupported type. Dec 12, 2023
@eiriktsarpalis
Copy link
Member

FWIW this particular scenario was never support in the source generator, and it only happened to work for the case of the reflection-based serializer. Leaving the issue open, since it might be the case that we could work around the issue for both implementations in the future.

@eiriktsarpalis eiriktsarpalis added enhancement Product code improvement that does NOT require public API changes/additions and removed untriaged New issue has not been triaged by the area owner labels Dec 12, 2023
@eiriktsarpalis eiriktsarpalis added this to the Future milestone Dec 12, 2023
@xbotter
Copy link
Author

xbotter commented Dec 13, 2023

(note the EncodingConverter is internal)

This is unrelated to the conversation at hand, but given that this is a library component exposed to users I though I'd point out that converters for public types should also be made public because otherwise they won't work with source generated serializers.

Does this mean that if I have a custom type, it is more recommended to declare the custom converter on the type rather than on the property?

@eiriktsarpalis
Copy link
Member

Correct.

@Bazalii
Copy link

Bazalii commented May 10, 2024

What about generic JsonConverters?

Lets say I have a generic class which contains a property with type that is not serializable(Span<T>). I don't want to create a DTO for this model because of perfomance considerations and I also cannot set generic JsonConverter as default converter for my type because I have to explicitly specify type for annotation:

// this is not possible
[JsonConverter(typeof(MyConverter<T>))]
public class MyGeneric<T>
{
    public Span<T> Items { get; }
}

The only way to solve this issue that I see is to use [JsonIgnore] attribute and set [JsonConverter(typeof(MyConverter<concrete-class-here>))] annotation for properties of type MyGeneric in objects that I serialize:

public class MyGeneric<T>
{
    [JsonIgnore]
    public Span<T> Items { get; }
}

public record MethodResponse
{
    [JsonConverter(typeof(MyConverter<string>))]
    public MyGeneric <T> MyGeneric { get; init; }
}

In my opinion, this solution requires useless work and need of having JsonConverter annotation for every property in DTOs that contain property of my type while it is known that MyConverter will always be used for serialization of MyGeneric. And I also have to add [JsonIgnore] attributes for every not serializable property of my type.

Are there any better solutions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

4 participants