System.Text.Json fast path serialization not working with combined contexts

[eiriktsarpalis]

EDIT: See API proposal at the bottom of this post.

The internal logic governing use of fast path serialization is dependent on JsonSerializerOptions encapsulating a JsonSerializerContext instance. Consider:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonMetadataServicesConverter.cs

Lines 72 to 80 in 7d2be1e

	if (!state.SupportContinuation &&
	jsonTypeInfo is JsonTypeInfo<T> info &&
	info.SerializeHandler != null &&
	!state.CurrentContainsMetadata && // Do not use the fast path if state needs to write metadata.
	info.Options.SerializerContext?.CanUseSerializationLogic == true)
	{
	info.SerializeHandler(writer, value);
	return true;
	}

or

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/SourceGenJsonTypeInfoOfT.cs

Lines 130 to 134 in 7d2be1e

	if (SerializeHandler != null && Options.SerializerContext?.CanUseSerializationLogic == true)
	{
	ThrowOnDeserialize = true;
	return;
	}

or

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs

Lines 452 to 455 in 26da83d

	if (converter.ConstructorIsParameterized)
	{
	InitializeConstructorParameters(GetParameterInfoValues(), sourceGenMode: Options.SerializerContext != null);
	}

The introduction of contract customization has made it possible for source generated metadata to be associated with options instances not attached to a JsonSerializerContext. This can result in inconsistency, consider:

[JsonSourceGenerationOptions(GenerationMode = JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(HighLowTemps))]
public partial class SerializationContext : JsonSerializerContext {}

public class HighLowTemps
{
    public int High { get; set; }
    public int Low { get; set; }
}

[Fact]
public static void CombiningContexts_FastPathSerialization()
{
    var value = new HighLowTemps { High = 40, Low = 30 };
    string expectedJson = """{"High":40,"Low":30}""";

    // Sanity check -- default context
    var options = new JsonSerializerOptions { TypeInfoResolver = SerializationContext.Default };
    string json = JsonSerializer.Serialize(value, options);
    Assert.Equal(expectedJson, json);

    // Using a trivial combined context
    options = new JsonSerializerOptions { TypeInfoResolver = JsonTypeInfoResolver.Combine(SerializationContext.Default) };
    json = JsonSerializer.Serialize(value, options);  // 'JsonSerializerContext' '<null>' did not provide property metadata
                                                      // for type 'System.Text.Json.SourceGeneration.Tests.HighLowTemps'.
    Assert.Equal(expectedJson, json);
}

In order to get the issue fixed, it is necessary for source gen JsonTypeInfo<T> to encapsulate a reference to their originating JsonSerializerContext.

API Proposal

namespace System.Text.Json.Serialization.Metadata;

public class JsonTypeInfo
{
    public IJsonTypeInfoResolver? OriginatingResolver { get; set; } = null;
}

[ghost]

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

---

The internal logic governing use of fast path serialization is dependent on JsonSerializerOptions encapsulating a JsonSerializerContext instance. Consider:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonMetadataServicesConverter.cs

Lines 72 to 80 in 7d2be1e

	if (!state.SupportContinuation &&
	jsonTypeInfo is JsonTypeInfo<T> info &&
	info.SerializeHandler != null &&
	!state.CurrentContainsMetadata && // Do not use the fast path if state needs to write metadata.
	info.Options.SerializerContext?.CanUseSerializationLogic == true)
	{
	info.SerializeHandler(writer, value);
	return true;
	}

or

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/SourceGenJsonTypeInfoOfT.cs

Lines 130 to 134 in 7d2be1e

	if (SerializeHandler != null && Options.SerializerContext?.CanUseSerializationLogic == true)
	{
	ThrowOnDeserialize = true;
	return;
	}

The introduction of contract customization has made it possible for source generated metadata to be associated with options instances not attached to a JsonSerializerContext. This can result in inconsistency, consider:

[JsonSourceGenerationOptions(GenerationMode = JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(HighLowTemps))]
public partial class SerializationContext : JsonSerializerContext {}

public class HighLowTemps
{
    public int High { get; set; }
    public int Low { get; set; }
}

[Fact]
public static void CombiningContexts_FastPathSerialization()
{
    var value = new HighLowTemps { High = 40, Low = 30 };
    string expectedJson = """{"High":40,"Low":30}""";

    // Sanity check -- default context
    var options = new JsonSerializerOptions { TypeInfoResolver = SerializationContext.Default };
    string json = JsonSerializer.Serialize(value, options);
    Assert.Equal(expectedJson, json);

    // Using a trivial combined context
    options = new JsonSerializerOptions { TypeInfoResolver = JsonTypeInfoResolver.Combine(SerializationContext.Default) };
    json = JsonSerializer.Serialize(value, options);  // 'JsonSerializerContext' '<null>' did not provide property metadata
                                                      // for type 'System.Text.Json.SourceGeneration.Tests.HighLowTemps'.
    Assert.Equal(expectedJson, json);
}

In order to get the issue fixed, it is necessary for source gen JsonTypeInfo<T> to encapsulate a reference to their originating JsonSerializerContext. IIUC this is not possible without updating the JsonMetadataServices APIs:

namespace System.Text.Json.Serialization.Metadata;

public partial class JsonObjectInfoValues<T>
{
     public JsonSerializerContext? JsonSerializerContext { get; init; }
}

public partial class JsonCollectionInfoValues<T>
{
     public JsonSerializerContext? JsonSerializerContext { get; init; }
}

public partial class JsonCollectionInfoValues<T>
{
     public JsonSerializerContext? JsonSerializerContext { get; init; }
}

public partial static class JsonMetadataServices
{
    public static JsonTypeInfo<T> CreateValueInfo<T>(JsonSerializerOptions options, JsonConverter converter); // existing overload
    public static JsonTypeInfo<T> CreateValueInfo<T>(JsonSerializerOptions options, JsonConverter converter, JsonSerializerContext? context);
}

Author:	eiriktsarpalis
Assignees:	-
Labels:	area-System.Text.Json
Milestone:	8.0.0

[krwq]

I'm aware of the issue although I'd take slightly different approach to the fix. Rather than trying to preserve this logic I'd focus on what info.Options.SerializerContext?.CanUseSerializationLogic == true is doing.

IMO following approach should be taken:

• we change implementation of IJsonTypeInfoResolver.GetTypeInfo for context to compare options with generated options and only if those are equivalent only then we generate SerializeHandler for type info. We need to take into consideration what happens when TypeInfoResolver is not context in those cases
• SerializeHandler is always used (minus perhaps some some compat layer so that code generated in previous versions continue working)

also I'm not sure if we can ever claim SerializeHandler will work correctly when customization happens - i.e. say we generated a SerializeHandler for struct with int and then we customize int so it always serializes as string. Basically to do the right thing we need to make sure that all inner classes used in SerializeHandler are handled by this context and just partially or handler would need to somehow have access to int metadata (and technically it actually does have it with options.GetTypeInfo(typeof(int))).

We should figure out if this even makes sense fixing considering number of edge case scenarios fixing this could introduce. The only moment where we can currently safely use fast path is what we're currently doing (when context is assigned directly as TypeInfoResolver)

[bartonjs]

Video

Looks good as proposed. Based on discussion it seems like EditorBrowsable(Never) is justified, since it's mostly distracting to callers outside of the serializer/sourcegen dichotomy.

namespace System.Text.Json.Serialization.Metadata;

public class JsonTypeInfo
{
    [EditorBrowsable(EditorBrowsableState.Never)]
    public IJsonTypeInfoResolver? OriginatingResolver { get; set; } = null;
}