JsonConverter.Read call inside another converter throws InvalidOperationException: Cannot skip tokens on partial JSON. #74108
Comments
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsDescriptionKey points:
Reproduction StepsMinimal repro testusing System;
using System.IO;
using System.Text;
using System.Text.Json;
using System.Text.Json.Serialization;
using System.Threading.Tasks;
using Xunit;
public class NestedConverterTest
{
[JsonConverter(typeof(Converter))]
private class MyClass
{
public InnerClass? Inner { get; set; }
}
private class InnerClass
{
public string? Value { get; set; }
}
private class Converter : JsonConverter<MyClass>
{
private JsonConverter<InnerClass> GetConverter(JsonSerializerOptions options)
{
return (JsonConverter<InnerClass>)options.GetConverter(typeof(InnerClass));
}
public override MyClass? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
var inner = GetConverter(options).Read(ref reader, typeof(InnerClass), options);
return new MyClass { Inner = inner };
}
public override void Write(Utf8JsonWriter writer, MyClass value, JsonSerializerOptions options)
{
GetConverter(options).Write(writer, value.Inner!, options);
}
}
[Fact]
public async Task ReadBigStreamWithExcessProps()
{
const int count = 1000;
var stream = new MemoryStream();
stream.Write(Encoding.UTF8.GetBytes("["));
for (int i = 0; i < count; i++)
{
if (i != 0)
{
stream.Write(Encoding.UTF8.GetBytes(","));
}
stream.Write(Encoding.UTF8.GetBytes(@"{""Missing"":""missing-value"",""Value"":""value""}"));
}
stream.Write(Encoding.UTF8.GetBytes("]"));
stream.Position = 0;
var result = await JsonSerializer.DeserializeAsync<MyClass[]>(stream);
Assert.Equal(count, result!.Length);
for (int i = 0; i < count; i++)
{
Assert.Equal("value", result[i].Inner?.Value);
}
}
}Expected behaviorShould successfully deserialize as it does with smaller input JSON or when there are no excessive properties in JSON. Actual behaviorFor .NET 6: Regression?No response Known WorkaroundsThe workaround is to use ConfigurationReproduces from .NET 5 to .NET 7 preview 7. Other informationCustom converters are always called with full current JSON entity buffered (see #39795 (comment)), and looks like I think that this use case (to deserialize a part of JSON entity with default converter, inside another custom converter) is quite usual (for example I use it in my custom "known types" converter, tuple converter and others). The question is why not to use Benchmark codeusing System;
using System.Text.Json.Serialization;
using System.Text.Json;
using BenchmarkDotNet.Attributes;
public class NestedConverterBenchmark
{
[Benchmark]
public string WriteCvtWrite()
{
return JsonSerializer.Serialize(_model, _optionsCvt);
}
[Benchmark]
public string WriteSerialize()
{
return JsonSerializer.Serialize(_model, _optionsSerializer);
}
[Benchmark]
public MyClass<InnerClass> ReadCvtRead()
{
return JsonSerializer.Deserialize<MyClass<InnerClass>>(_json, _optionsCvt)!;
}
[Benchmark]
public MyClass<InnerClass> ReadDeserialize()
{
return JsonSerializer.Deserialize<MyClass<InnerClass>>(_json, _optionsSerializer)!;
}
private static readonly JsonSerializerOptions _optionsCvt
= new JsonSerializerOptions { Converters = { new CvtConverter() } };
private static readonly JsonSerializerOptions _optionsSerializer
= new JsonSerializerOptions { Converters = { new SerializeConverter<InnerClass>() } };
private static readonly MyClass<InnerClass> _model
= new MyClass<InnerClass> { Value = new InnerClass { Prop = "prop-value" } };
private static readonly string _json
= @"{""Prop"":""prop-value""}";
public class MyClass<T>
{
public T? Value { get; set; }
}
public class InnerClass
{
public string? Prop { get; set; }
}
private class CvtConverter : JsonConverterFactory
{
public override JsonConverter? CreateConverter(Type typeToConvert, JsonSerializerOptions options)
{
var innerType = typeToConvert.GetGenericArguments()[0];
var innerConverter = options.GetConverter(innerType);
return (JsonConverter)Activator.CreateInstance(
typeof(Impl<>).MakeGenericType(innerType),
innerConverter
)!;
}
public override bool CanConvert(Type typeToConvert)
{
return typeToConvert.IsConstructedGenericType
&& typeToConvert.GetGenericTypeDefinition() == typeof(MyClass<>);
}
private class Impl<T> : JsonConverter<MyClass<T>>
{
private readonly JsonConverter<T> _innerConverter;
public Impl(JsonConverter<T> innerConverter)
{
_innerConverter = innerConverter;
}
public override MyClass<T>? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
return new MyClass<T>
{
Value = _innerConverter.Read(ref reader, typeof(T), options)!
};
}
public override void Write(Utf8JsonWriter writer, MyClass<T> value, JsonSerializerOptions options)
{
_innerConverter.Write(writer, value.Value!, options);
}
}
}
private class SerializeConverter<T> : JsonConverter<MyClass<T>>
{
public override MyClass<T>? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
return new MyClass<T>
{
Value = JsonSerializer.Deserialize<T>(ref reader, options)!
};
}
public override void Write(Utf8JsonWriter writer, MyClass<T> value, JsonSerializerOptions options)
{
JsonSerializer.Serialize(writer, value.Value, options);
}
}
}
|
|
@dlyz do you have a repro project including a sample JSON payload that's failing, type graph, and the custom converters? Per notes above, this doesn't seem like a regression (same behavior since .NET 5.0) so I'm marking 8.0 for now. FWIW we are considering a feature to provide custom async converters which could help with perf - #63795. |
layomia
added
the
needs-author-action
|
This issue has been marked |
Yes, I have it in the collapsed area under |
ghost
added
needs-further-triage
layomia
removed
the
needs-further-triage
|
Not a regression but needs a look in .NET 8. |
|
Just wanted to say thanks to @dlyz - this issue and the benchmark code saved me a huge headache! |
|
It seems that the serializer is failing to read ahead the entire value before passing to the custom converter in the context of collection. We should try to fix this. |
ghost
added
the
in-pr
|
I've pushed #89637 that should fix this for .NET 8. In the meantime you could work around the issue by using the class Converter : JsonConverter<MyClass>
{
public override MyClass? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
var inner = JsonSerializer.Deserialize<InnerClass>(ref reader, options);
return new MyClass { Inner = inner };
}
public override void Write(Utf8JsonWriter writer, MyClass value, JsonSerializerOptions options)
=> JsonSerializer.Serialize(writer, value, options);
}The above will ensure that the right initializations are performed for the |
ghost
removed
the
in-pr
ghost
locked as resolved and limited conversation to collaborators
Description
Key points:
JsonConverterReadmethod we callReadof another converter acquired withJsonSerializerOptions.GetConverter(typeof(InnerClass))InnerClassis anObjectDefaultConverterInnerClasswith properties that are not represented in the InnerClass (excessive properties).Reproduction Steps
Minimal repro test
Expected behavior
Should successfully deserialize as it does with smaller input JSON or when there are no excessive properties in JSON.
Actual behavior
For .NET 6:
Regression?
No response
Known Workarounds
The workaround is to use
JsonSerializer.Deserializeinstead of acquiring a converter fromJsonSerializerOptionsand callingJsonConverter.Read, but this workaround may be significantly slower (see the benchmark below). Workaround may be applied conditionally whenUtf8JsonReader.IsFinalBlockisfalse.Configuration
Reproduces from .NET 5 to .NET 7 preview 7.
Older versions don't work either, but for other reasons.
Other information
Custom converters are always called with full current JSON entity buffered (see #39795 (comment)), and looks like
ObjectDefaultConverteris aware of that and chooses "fast path" (if (!state.SupportContinuation && !state.Current.CanContainMetadata)), butUtf8JsonReader.Skipmethod fails anyway because it checks for_isFinalBlockwhich isfalse.I think that this use case (to deserialize a part of JSON entity with default converter, inside another custom converter) is quite usual (for example I use it in my custom "known types" converter, tuple converter and others). The question is why not to use
JsonSerializer.Deserialize. The answer is in the benchmark below.ReadCvtRead(with nested converter call) is about 1.5 times faster thenReadDeserialize(withJsonSerializer.Deserializecall).Benchmark code
The text was updated successfully, but these errors were encountered: