Skip to content

Fix test failures and proper fix this time #18

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.

Sign up for GitHub

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

Merged
eiriktsarpalis merged 5 commits into from
Jun 22, 2022
Merged

Fix test failures and proper fix this time #18

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9cf6c41
Fix test failures and proper fix this time
krwq Jun 22, 2022
1283ff6
reinstate ActiveIssueAttribute
eiriktsarpalis Jun 22, 2022
3bd6b1c
PR feedback and adjust couple of tests which don't set TypeInfoResolver
krwq Jun 22, 2022
f1ca39e
fix IAsyncEnumerable tests
eiriktsarpalis Jun 22, 2022
28b67b0
Lock JsonSerializerOptions in JsonTypeInfo.EnsureConfigured()
eiriktsarpalis Jun 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...raries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Helpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ public static partial class JsonSerializer
var reader = new Utf8JsonReader(utf8Json, isFinalBlock: true, readerState);

ReadStack state = default;
jsonTypeInfo.Options.VerifyTypeInfoResolverIsSet();
jsonTypeInfo.EnsureConfigured();
Copy link
Collaborator

@eiriktsarpalis eiriktsarpalis Jun 22, 2022

Choose a reason for hiding this comment

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

Hmm, could the verification logic live inside EnsureConfigured? Or could this break other places where configuration is happening?

Copy link
Owner Author

@krwq krwq Jun 22, 2022

Choose a reason for hiding this comment

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

I'm unsure this is safe, I think yes but currently this check is not matching all callsites

state.Initialize(jsonTypeInfo);

Expand Down
3 changes: 3 additions & 0 deletions ...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,7 @@ private static async IAsyncEnumerable<TValue> CreateAsyncEnumerableDeserializer<
JsonTypeInfo<Queue<TValue>> queueTypeInfo,
[EnumeratorCancellation] CancellationToken cancellationToken)
{
queueTypeInfo.Options.VerifyTypeInfoResolverIsSet();
queueTypeInfo.EnsureConfigured();
JsonSerializerOptions options = queueTypeInfo.Options;
var bufferState = new ReadBufferState(options.DefaultBufferSize);
Expand Down Expand Up @@ -467,6 +468,7 @@ private static async IAsyncEnumerable<TValue> CreateAsyncEnumerableDeserializer<
JsonSerializerOptions options = jsonTypeInfo.Options;
var bufferState = new ReadBufferState(options.DefaultBufferSize);
ReadStack readStack = default;
jsonTypeInfo.Options.VerifyTypeInfoResolverIsSet();
jsonTypeInfo.EnsureConfigured();
readStack.Initialize(jsonTypeInfo, supportContinuation: true);
JsonConverter converter = readStack.Current.JsonPropertyInfo!.EffectiveConverter;
Expand Down Expand Up @@ -498,6 +500,7 @@ private static async IAsyncEnumerable<TValue> CreateAsyncEnumerableDeserializer<
JsonSerializerOptions options = jsonTypeInfo.Options;
var bufferState = new ReadBufferState(options.DefaultBufferSize);
ReadStack readStack = default;
jsonTypeInfo.Options.VerifyTypeInfoResolverIsSet();
jsonTypeInfo.EnsureConfigured();
readStack.Initialize(jsonTypeInfo, supportContinuation: true);
JsonConverter converter = readStack.Current.JsonPropertyInfo!.EffectiveConverter;
Expand Down
1 change: 1 addition & 0 deletions ...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,7 @@ public static partial class JsonSerializer

private static TValue? ReadFromSpan<TValue>(ReadOnlySpan<char> json, JsonTypeInfo jsonTypeInfo)
{
jsonTypeInfo.Options.VerifyTypeInfoResolverIsSet();
jsonTypeInfo.EnsureConfigured();
byte[]? tempArray = null;

Expand Down
1 change: 1 addition & 0 deletions ...System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Utf8JsonReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ public static partial class JsonSerializer
private static TValue? Read<TValue>(ref Utf8JsonReader reader, JsonTypeInfo jsonTypeInfo)
{
ReadStack state = default;
jsonTypeInfo.Options.VerifyTypeInfoResolverIsSet();
jsonTypeInfo.EnsureConfigured();
state.Initialize(jsonTypeInfo);

Expand Down
1 change: 1 addition & 0 deletions ...aries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Helpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ jsonTypeInfo is not JsonTypeInfo<TValue> ||
"Incorrect method called. WriteUsingGeneratedSerializer() should have been called instead.");

WriteStack state = default;
jsonTypeInfo.Options.VerifyTypeInfoResolverIsSet();
jsonTypeInfo.EnsureConfigured();
state.Initialize(jsonTypeInfo, supportContinuation: false, supportAsync: false);

Expand Down
2 changes: 2 additions & 0 deletions ...raries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Stream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ private static async Task WriteStreamAsync<TValue>(
using (var writer = new Utf8JsonWriter(bufferWriter, writerOptions))
{
WriteStack state = new WriteStack { CancellationToken = cancellationToken };
jsonTypeInfo.Options.VerifyTypeInfoResolverIsSet();
jsonTypeInfo.EnsureConfigured();
JsonConverter converter = state.Initialize(jsonTypeInfo, supportContinuation: true, supportAsync: true);

Expand Down Expand Up @@ -390,6 +391,7 @@ private static void WriteStream<TValue>(
using (var writer = new Utf8JsonWriter(bufferWriter, writerOptions))
{
WriteStack state = default;
jsonTypeInfo.Options.VerifyTypeInfoResolverIsSet();
jsonTypeInfo.EnsureConfigured();
JsonConverter converter = state.Initialize(jsonTypeInfo, supportContinuation: true, supportAsync: false);

Expand Down
15 changes: 9 additions & 6 deletions src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -655,12 +655,6 @@ internal void InitializeForReflectionSerializer()
private JsonTypeInfo? GetTypeInfoInternal(Type type)
{
IJsonTypeInfoResolver? resolver = _effectiveJsonTypeInfoResolver ?? _typeInfoResolver;

if (resolver == null)
{
ThrowHelper.ThrowInvalidOperationException_JsonTypeInfoUsedButTypeInfoResolverNotSet();
}

JsonTypeInfo? info = resolver?.GetTypeInfo(type, this);

if (info != null)
Expand Down Expand Up @@ -728,6 +722,15 @@ internal void VerifyMutable()
}
}

internal void VerifyTypeInfoResolverIsSet()
{
IJsonTypeInfoResolver? resolver = _effectiveJsonTypeInfoResolver ?? _typeInfoResolver;
Copy link
Collaborator

@eiriktsarpalis eiriktsarpalis Jun 22, 2022

Choose a reason for hiding this comment

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

Hmmm, I think we should instead simply check if the options instance is mutable. What should happen if for example we have:

var options = new JsonSerializerOptions();
var resolver = new DefaultJsonTypeInfoResolver();

JsonTypeInfo jti = resolver.GetTypeInfo(typeof(MyEnum), options); 

options.Resolver = resolver;
options.Converter.Add(new JsonStringEnumConverter());

// jti now has inconsistent configuration relative to its encapsulated options instance

Copy link
Collaborator

@eiriktsarpalis eiriktsarpalis Jun 22, 2022

Choose a reason for hiding this comment

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

And the more I think about it, I think we should make it impossible for JsonTypeInfo to encapsulate mutable options. In other words,

JsonTypeInfo.CreateJsonTypeInfo(typeof(int), new JsonSerializerOptions()); // InvalidOperationException

Copy link
Collaborator

@eiriktsarpalis eiriktsarpalis Jun 22, 2022

Choose a reason for hiding this comment

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

So if we make that check there, we remove the need to do it in the (hot path) source gen serialization methods.

Copy link
Owner Author

@krwq krwq Jun 22, 2022

Choose a reason for hiding this comment

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

the place where we've originally hit this had locked options

if (resolver == null)
{
ThrowHelper.ThrowInvalidOperationException_JsonTypeInfoUsedButTypeInfoResolverNotSet();
}
}

private sealed class ConverterList : ConfigurationList<JsonConverter>
{
private readonly JsonSerializerOptions _options;
Expand Down
1 change: 0 additions & 1 deletion ...n/tests/System.Text.Json.Tests/Serialization/CustomConverterTests/CustomConverterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,6 @@ void RunTest<TConverterReturn>()
}

[ActiveIssue("https://github.com/dotnet/runtime/issues/66232", TargetFrameworkMonikers.NetFramework)]
[ActiveIssue("https://github.com/dotnet/runtime/issues/66371", typeof(PlatformDetection), nameof(PlatformDetection.IsMonoInterpreter))]
[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public static void GetConverter_Poco_WriteThrowsNotSupportedException()
{
Expand Down
61 changes: 61 additions & 0 deletions ...xt.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverMultiContextTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,67 @@ public async Task JsonSerializerSerializeWithTypeInfoOfT<T>(T testObj, string ex
Assert.Equal(expectedJson, json);
}

[Fact]
public async Task SerializationWithJsonTypeInfoWithoutSettingTypeInfoResolverThrows()
{
JsonSerializerOptions o = new();
DefaultJsonTypeInfoResolver r = new();
// note: TypeInfoResolver not set
JsonTypeInfo<SomeClass> ti = (JsonTypeInfo<SomeClass>)r.GetTypeInfo(typeof(SomeClass), o);
SomeClass obj = new()
{
ObjProp = "test",
IntProp = 42,
};

// TODO: reasses if this is expected behavior
await Assert.ThrowsAsync<InvalidOperationException>(() => Serializer.SerializeWrapper(obj, ti));
}

[Fact]
public async Task DeserializationWithJsonTypeInfoWithoutSettingTypeInfoResolverThrows()
{
JsonSerializerOptions o = new();
DefaultJsonTypeInfoResolver r = new();
// note: TypeInfoResolver not set
JsonTypeInfo<SomeClass> ti = (JsonTypeInfo<SomeClass>)r.GetTypeInfo(typeof(SomeClass), o);

// TODO: reasses if this is expected behavior
string json = """{"ObjProp":"test","IntProp":42}""";
await Assert.ThrowsAsync<InvalidOperationException>(() => Serializer.DeserializeWrapper(json, ti));
}

[Fact]
public async Task SerializationWithJsonTypeInfoWhenTypeInfoResolverSetIsPossible()
{
JsonSerializerOptions o = new();
DefaultJsonTypeInfoResolver r = new();
o.TypeInfoResolver = r;
JsonTypeInfo<SomeClass> ti = (JsonTypeInfo<SomeClass>)r.GetTypeInfo(typeof(SomeClass), o);
SomeClass obj = new()
{
ObjProp = "test",
IntProp = 42,
};

string json = await Serializer.SerializeWrapper(obj, ti);
Assert.Equal("""{"ObjProp":"test","IntProp":42}""", json);
}

[Fact]
public async Task DeserializationWithJsonTypeInfoWhenTypeInfoResolverSetIsPossible()
{
JsonSerializerOptions o = new();
DefaultJsonTypeInfoResolver r = new();
o.TypeInfoResolver = r;
JsonTypeInfo<SomeClass> ti = (JsonTypeInfo<SomeClass>)r.GetTypeInfo(typeof(SomeClass), o);
string json = """{"ObjProp":"test","IntProp":42}""";
SomeClass deserialized = await Serializer.DeserializeWrapper(json, ti);
Assert.IsType<JsonElement>(deserialized.ObjProp);
Assert.Equal("test", ((JsonElement)deserialized.ObjProp).GetString());
Assert.Equal(42, deserialized.IntProp);
}

public static IEnumerable<object[]> JsonSerializerSerializeWithTypeInfoOfT_TestData()
{
yield return new object[] { "value", @"""value""" };
Expand Down
74 changes: 0 additions & 74 deletions ...ts/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
using System.Text;
using System.Text.Json.Serialization.Metadata;
using System.Threading.Tasks;
using Microsoft.DotNet.RemoteExecutor;
using Xunit;

namespace System.Text.Json.Serialization.Tests
Expand Down Expand Up @@ -179,79 +178,6 @@ public static void ModifiersAreCalledAndModifyTypeInfos()
Assert.True(secondModifierCalled);
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public static void StaticInitialization_SerializationWithJsonTypeInfoWithoutSettingTypeInfoResolverThrows()
{
RemoteExecutor.Invoke(static () =>
{
JsonSerializerOptions o = new();
DefaultJsonTypeInfoResolver r = new();
// note: TypeInfoResolver not set
JsonTypeInfo<SomeClass> ti = (JsonTypeInfo<SomeClass>)r.GetTypeInfo(typeof(SomeClass), o);
SomeClass obj = new()
{
ObjProp = "test",
IntProp = 42,
};

// TODO: reasses if this is expected behavior
Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(obj, ti));
}).Dispose();
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public static void StaticInitialization_DeserializationWithJsonTypeInfoWithoutSettingTypeInfoResolverThrows()
{
RemoteExecutor.Invoke(static () =>
{
JsonSerializerOptions o = new();
DefaultJsonTypeInfoResolver r = new();
// note: TypeInfoResolver not set
JsonTypeInfo<SomeClass> ti = (JsonTypeInfo<SomeClass>)r.GetTypeInfo(typeof(SomeClass), o);

// TODO: reasses if this is expected behavior
string json = """{"ObjProp":"test","IntProp":42}""";
Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize(json, ti));
}).Dispose();
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public static void StaticInitialization_SerializationWithJsonTypeInfoWhenTypeInfoResolverSetIsPossible()
{
RemoteExecutor.Invoke(static () =>
{
JsonSerializerOptions o = new();
DefaultJsonTypeInfoResolver r = new();
o.TypeInfoResolver = r;
JsonTypeInfo<SomeClass> ti = (JsonTypeInfo<SomeClass>)r.GetTypeInfo(typeof(SomeClass), o);
SomeClass obj = new()
{
ObjProp = "test",
IntProp = 42,
};

string json = JsonSerializer.Serialize(obj, ti);
Assert.Equal("""{"ObjProp":"test","IntProp":42}""", json);
}).Dispose();
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public static void StaticInitialization_DeserializationWithJsonTypeInfoWhenTypeInfoResolverSetIsPossible()
{
RemoteExecutor.Invoke(static () =>
{
JsonSerializerOptions o = new();
DefaultJsonTypeInfoResolver r = new();
o.TypeInfoResolver = r;
JsonTypeInfo<SomeClass> ti = (JsonTypeInfo<SomeClass>)r.GetTypeInfo(typeof(SomeClass), o);
string json = """{"ObjProp":"test","IntProp":42}""";
SomeClass deserialized = JsonSerializer.Deserialize(json, ti);
Assert.IsType<JsonElement>(deserialized.ObjProp);
Assert.Equal("test", ((JsonElement)deserialized.ObjProp).GetString());
Assert.Equal(42, deserialized.IntProp);
}).Dispose();
}

private static void InvokeGeneric(Type type, string methodName, params object[] args)
{
typeof(DefaultJsonTypeInfoResolverTests)
Expand Down