Skip to content

[release/6.0] Disable fast path serialization for types with properties using custom converters. #58571

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
danmoseley merged 1 commit into from
Sep 3, 2021
Merged

[release/6.0] Disable fast path serialization for types with properties using custom converters. #58571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,7 +790,7 @@ private string GenerateFastPathFuncForObject(TypeGenerationSpec typeGenSpec)
out Dictionary<string, PropertyGenerationSpec>? serializableProperties,
out bool castingRequiredForProps))
{
string exceptionMessage = @$"""Invalid serializable-property configuration specified for type '{typeRef}'. For more information, use 'JsonSourceGenerationMode.Serialization'.""";
string exceptionMessage = @$"""Invalid serializable-property configuration specified for type '{typeRef}'. For more information, see 'JsonSourceGenerationMode.Serialization'.""";
Copy link
Member

@danmoseley danmoseley Sep 3, 2021

Choose a reason for hiding this comment

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

shouldn't this be in \src\libraries\System.Text.Json\gen\Resources\Strings.resx?

Copy link
Member

@danmoseley danmoseley Sep 3, 2021

Choose a reason for hiding this comment

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

doesn't need fixing in this PR, since it's pre existing.

Copy link
Member

@eiriktsarpalis eiriktsarpalis Sep 3, 2021

Choose a reason for hiding this comment

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

This is tracked by #58292. We should fix for 6.


return GenerateFastPathFuncForType(
serializeMethodName,
Expand Down
3 changes: 2 additions & 1 deletion src/libraries/System.Text.Json/gen/TypeGenerationSpec.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,8 @@ private bool FastPathIsSupported()
{
if (property.TypeGenerationSpec.Type.IsObjectType() ||
property.NumberHandling == JsonNumberHandling.AllowNamedFloatingPointLiterals ||
property.NumberHandling == JsonNumberHandling.WriteAsString)
property.NumberHandling == JsonNumberHandling.WriteAsString ||
property.ConverterInstantiationLogic is not null)
{
return false;
}
Expand Down
4 changes: 4 additions & 0 deletions ...ibraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/ContextClasses.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ namespace System.Text.Json.SourceGeneration.Tests
{
public interface ITestContext
{
public JsonSourceGenerationMode JsonSourceGenerationMode { get; }

public JsonTypeInfo<Location> Location { get; }
public JsonTypeInfo<NumberTypes> NumberTypes { get; }
public JsonTypeInfo<RepeatedTypes.Location> RepeatedLocation { get; }
Expand All @@ -31,6 +33,8 @@ public interface ITestContext
public JsonTypeInfo<RealWorldContextTests.ClassWithEnumAndNullable> ClassWithEnumAndNullable { get; }
public JsonTypeInfo<ClassWithCustomConverter> ClassWithCustomConverter { get; }
public JsonTypeInfo<StructWithCustomConverter> StructWithCustomConverter { get; }
public JsonTypeInfo<ClassWithCustomConverterProperty> ClassWithCustomConverterProperty { get; }
public JsonTypeInfo<StructWithCustomConverterProperty> StructWithCustomConverterProperty { get; }
public JsonTypeInfo<ClassWithBadCustomConverter> ClassWithBadCustomConverter { get; }
public JsonTypeInfo<StructWithBadCustomConverter> StructWithBadCustomConverter { get; }
}
Expand Down
5 changes: 5 additions & 0 deletions ...son/tests/System.Text.Json.SourceGeneration.Tests/MetadataAndSerializationContextTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,13 @@ namespace System.Text.Json.SourceGeneration.Tests
[JsonSerializable(typeof(RealWorldContextTests.ClassWithEnumAndNullable))]
[JsonSerializable(typeof(ClassWithCustomConverter))]
[JsonSerializable(typeof(StructWithCustomConverter))]
[JsonSerializable(typeof(ClassWithCustomConverterProperty))]
[JsonSerializable(typeof(StructWithCustomConverterProperty))]
[JsonSerializable(typeof(ClassWithBadCustomConverter))]
[JsonSerializable(typeof(StructWithBadCustomConverter))]
internal partial class MetadataAndSerializationContext : JsonSerializerContext, ITestContext
{
public JsonSourceGenerationMode JsonSourceGenerationMode => JsonSourceGenerationMode.Default;
}

public sealed class MetadataAndSerializationContextTests : RealWorldContextTests
Expand Down Expand Up @@ -64,6 +67,8 @@ public override void EnsureFastPathGeneratedAsExpected()
Assert.NotNull(MetadataAndSerializationContext.Default.ClassWithEnumAndNullable.Serialize);
Assert.NotNull(MetadataAndSerializationContext.Default.ClassWithCustomConverter);
Assert.NotNull(MetadataAndSerializationContext.Default.StructWithCustomConverter);
Assert.NotNull(MetadataAndSerializationContext.Default.ClassWithCustomConverterProperty);
Assert.NotNull(MetadataAndSerializationContext.Default.StructWithCustomConverterProperty);
Assert.Throws<InvalidOperationException>(() => MetadataAndSerializationContext.Default.ClassWithBadCustomConverter);
Assert.Throws<InvalidOperationException>(() => MetadataAndSerializationContext.Default.StructWithBadCustomConverter);
}
Expand Down
8 changes: 8 additions & 0 deletions ...es/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/MetadataContextTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,13 @@ namespace System.Text.Json.SourceGeneration.Tests
[JsonSerializable(typeof(StructWithCustomConverter), GenerationMode = JsonSourceGenerationMode.Metadata)]
[JsonSerializable(typeof(ClassWithCustomConverter), GenerationMode = JsonSourceGenerationMode.Metadata)]
[JsonSerializable(typeof(StructWithCustomConverter), GenerationMode = JsonSourceGenerationMode.Metadata)]
[JsonSerializable(typeof(ClassWithCustomConverterProperty), GenerationMode = JsonSourceGenerationMode.Metadata)]
[JsonSerializable(typeof(StructWithCustomConverterProperty), GenerationMode = JsonSourceGenerationMode.Metadata)]
[JsonSerializable(typeof(ClassWithBadCustomConverter), GenerationMode = JsonSourceGenerationMode.Metadata)]
[JsonSerializable(typeof(StructWithBadCustomConverter), GenerationMode = JsonSourceGenerationMode.Metadata)]
internal partial class MetadataWithPerTypeAttributeContext : JsonSerializerContext, ITestContext
{
public JsonSourceGenerationMode JsonSourceGenerationMode => JsonSourceGenerationMode.Metadata;
}

public sealed class MetadataWithPerTypeAttributeContextTests : RealWorldContextTests
Expand Down Expand Up @@ -91,10 +94,13 @@ public override void EnsureFastPathGeneratedAsExpected()
[JsonSerializable(typeof(RealWorldContextTests.ClassWithEnumAndNullable))]
[JsonSerializable(typeof(ClassWithCustomConverter))]
[JsonSerializable(typeof(StructWithCustomConverter))]
[JsonSerializable(typeof(ClassWithCustomConverterProperty))]
[JsonSerializable(typeof(StructWithCustomConverterProperty))]
[JsonSerializable(typeof(ClassWithBadCustomConverter))]
[JsonSerializable(typeof(StructWithBadCustomConverter))]
internal partial class MetadataContext : JsonSerializerContext, ITestContext
{
public JsonSourceGenerationMode JsonSourceGenerationMode => JsonSourceGenerationMode.Metadata;
}

public sealed class MetadataContextTests : RealWorldContextTests
Expand Down Expand Up @@ -126,6 +132,8 @@ public override void EnsureFastPathGeneratedAsExpected()
Assert.Null(MetadataContext.Default.ClassWithEnumAndNullable.Serialize);
Assert.Null(MetadataContext.Default.ClassWithCustomConverter.Serialize);
Assert.Null(MetadataContext.Default.StructWithCustomConverter.Serialize);
Assert.Null(MetadataContext.Default.ClassWithCustomConverterProperty.Serialize);
Assert.Null(MetadataContext.Default.StructWithCustomConverterProperty.Serialize);
Assert.Throws<InvalidOperationException>(() => MetadataContext.Default.ClassWithBadCustomConverter.Serialize);
Assert.Throws<InvalidOperationException>(() => MetadataContext.Default.StructWithBadCustomConverter.Serialize);
}
Expand Down
5 changes: 5 additions & 0 deletions ...s/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/MixedModeContextTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,13 @@ namespace System.Text.Json.SourceGeneration.Tests
[JsonSerializable(typeof(RealWorldContextTests.ClassWithEnumAndNullable), GenerationMode = JsonSourceGenerationMode.Metadata | JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(ClassWithCustomConverter), GenerationMode = JsonSourceGenerationMode.Metadata | JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(StructWithCustomConverter), GenerationMode = JsonSourceGenerationMode.Metadata | JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(ClassWithCustomConverterProperty), GenerationMode = JsonSourceGenerationMode.Metadata | JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(StructWithCustomConverterProperty), GenerationMode = JsonSourceGenerationMode.Metadata | JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(ClassWithBadCustomConverter), GenerationMode = JsonSourceGenerationMode.Metadata | JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(StructWithBadCustomConverter), GenerationMode = JsonSourceGenerationMode.Metadata | JsonSourceGenerationMode.Serialization)]
internal partial class MixedModeContext : JsonSerializerContext, ITestContext
{
public JsonSourceGenerationMode JsonSourceGenerationMode => JsonSourceGenerationMode.Metadata | JsonSourceGenerationMode.Serialization;
}

public sealed class MixedModeContextTests : RealWorldContextTests
Expand Down Expand Up @@ -62,6 +65,8 @@ public override void EnsureFastPathGeneratedAsExpected()
Assert.NotNull(MixedModeContext.Default.ClassWithEnumAndNullable.Serialize);
Assert.Null(MixedModeContext.Default.ClassWithCustomConverter.Serialize);
Assert.Null(MixedModeContext.Default.StructWithCustomConverter.Serialize);
Assert.Null(MixedModeContext.Default.ClassWithCustomConverterProperty.Serialize);
Assert.Null(MixedModeContext.Default.StructWithCustomConverterProperty.Serialize);
Assert.Throws<InvalidOperationException>(() => MixedModeContext.Default.ClassWithBadCustomConverter.Serialize);
Assert.Throws<InvalidOperationException>(() => MixedModeContext.Default.StructWithBadCustomConverter.Serialize);
}
Expand Down
54 changes: 54 additions & 0 deletions ...s/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/RealWorldContextTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,60 @@ public virtual void RoundTripWithCustomConverter_Struct()
Assert.Equal(42, obj.MyInt);
}

[Fact]
public virtual void RoundtripWithCustomConverterProperty_Class()
{
const string ExpectedJson = "{\"Property\":42}";

ClassWithCustomConverterProperty obj = new()
{
Property = new ClassWithCustomConverterProperty.NestedPoco { Value = 42 }
};

// Types with properties in custom converters do not support fast path serialization.
Assert.True(DefaultContext.ClassWithCustomConverterProperty.Serialize is null);

if (DefaultContext.JsonSourceGenerationMode == JsonSourceGenerationMode.Serialization)
{
Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(obj, DefaultContext.ClassWithCustomConverterProperty));
}
else
{
string json = JsonSerializer.Serialize(obj, DefaultContext.ClassWithCustomConverterProperty);
Assert.Equal(ExpectedJson, json);
}

obj = JsonSerializer.Deserialize<ClassWithCustomConverterProperty>(ExpectedJson);
Assert.Equal(42, obj.Property.Value);
}

[Fact]
public virtual void RoundtripWithCustomConverterProperty_Struct()
{
const string ExpectedJson = "{\"Property\":42}";

StructWithCustomConverterProperty obj = new()
{
Property = new ClassWithCustomConverterProperty.NestedPoco { Value = 42 }
};

// Types with properties in custom converters do not support fast path serialization.
Assert.True(DefaultContext.StructWithCustomConverterProperty.Serialize is null);

if (DefaultContext.JsonSourceGenerationMode == JsonSourceGenerationMode.Serialization)
{
Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(obj, DefaultContext.StructWithCustomConverterProperty));
}
else
{
string json = JsonSerializer.Serialize(obj, DefaultContext.StructWithCustomConverterProperty);
Assert.Equal(ExpectedJson, json);
}

obj = JsonSerializer.Deserialize<StructWithCustomConverterProperty>(ExpectedJson);
Assert.Equal(42, obj.Property.Value);
}

[Fact]
public virtual void BadCustomConverter_Class()
{
Expand Down
15 changes: 13 additions & 2 deletions ...stem.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/SerializationContextTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,13 @@ namespace System.Text.Json.SourceGeneration.Tests
[JsonSerializable(typeof(RealWorldContextTests.ClassWithEnumAndNullable))]
[JsonSerializable(typeof(ClassWithCustomConverter))]
[JsonSerializable(typeof(StructWithCustomConverter))]
[JsonSerializable(typeof(ClassWithCustomConverterProperty))]
[JsonSerializable(typeof(StructWithCustomConverterProperty))]
[JsonSerializable(typeof(ClassWithBadCustomConverter))]
[JsonSerializable(typeof(StructWithBadCustomConverter))]
internal partial class SerializationContext : JsonSerializerContext, ITestContext
{
public JsonSourceGenerationMode JsonSourceGenerationMode => JsonSourceGenerationMode.Serialization;
}

[JsonSerializable(typeof(Location), GenerationMode = JsonSourceGenerationMode.Serialization)]
Expand All @@ -57,12 +60,13 @@ internal partial class SerializationContext : JsonSerializerContext, ITestContex
[JsonSerializable(typeof(RealWorldContextTests.ClassWithEnumAndNullable), GenerationMode = JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(ClassWithCustomConverter), GenerationMode = JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(StructWithCustomConverter), GenerationMode = JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(ClassWithCustomConverter), GenerationMode = JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(StructWithCustomConverter), GenerationMode = JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(ClassWithCustomConverterProperty), GenerationMode = JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(StructWithCustomConverterProperty), GenerationMode = JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(ClassWithBadCustomConverter), GenerationMode = JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(StructWithBadCustomConverter), GenerationMode = JsonSourceGenerationMode.Serialization)]
internal partial class SerializationWithPerTypeAttributeContext : JsonSerializerContext, ITestContext
{
public JsonSourceGenerationMode JsonSourceGenerationMode => JsonSourceGenerationMode.Serialization;
}

[JsonSourceGenerationOptions(PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase)]
Expand All @@ -88,10 +92,13 @@ internal partial class SerializationWithPerTypeAttributeContext : JsonSerializer
[JsonSerializable(typeof(RealWorldContextTests.ClassWithEnumAndNullable), GenerationMode = JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(ClassWithCustomConverter), GenerationMode = JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(StructWithCustomConverter), GenerationMode = JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(ClassWithCustomConverterProperty), GenerationMode = JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(StructWithCustomConverterProperty), GenerationMode = JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(ClassWithBadCustomConverter), GenerationMode = JsonSourceGenerationMode.Serialization)]
[JsonSerializable(typeof(StructWithBadCustomConverter), GenerationMode = JsonSourceGenerationMode.Serialization)]
internal partial class SerializationContextWithCamelCase : JsonSerializerContext, ITestContext
{
public JsonSourceGenerationMode JsonSourceGenerationMode => JsonSourceGenerationMode.Serialization;
}

public class SerializationContextTests : RealWorldContextTests
Expand Down Expand Up @@ -128,6 +135,8 @@ public override void EnsureFastPathGeneratedAsExpected()
Assert.NotNull(SerializationContext.Default.ClassWithEnumAndNullable.Serialize);
Assert.Null(SerializationContext.Default.ClassWithCustomConverter.Serialize);
Assert.Null(SerializationContext.Default.StructWithCustomConverter.Serialize);
Assert.Null(SerializationContext.Default.ClassWithCustomConverterProperty.Serialize);
Assert.Null(SerializationContext.Default.StructWithCustomConverterProperty.Serialize);
Assert.Throws<InvalidOperationException>(() => SerializationContext.Default.ClassWithBadCustomConverter.Serialize);
Assert.Throws<InvalidOperationException>(() => SerializationContext.Default.StructWithBadCustomConverter.Serialize);
}
Expand Down Expand Up @@ -390,6 +399,8 @@ public override void EnsureFastPathGeneratedAsExpected()
Assert.NotNull(SerializationWithPerTypeAttributeContext.Default.ClassWithEnumAndNullable.Serialize);
Assert.Null(SerializationWithPerTypeAttributeContext.Default.ClassWithCustomConverter.Serialize);
Assert.Null(SerializationWithPerTypeAttributeContext.Default.StructWithCustomConverter.Serialize);
Assert.Null(SerializationWithPerTypeAttributeContext.Default.ClassWithCustomConverterProperty.Serialize);
Assert.Null(SerializationWithPerTypeAttributeContext.Default.StructWithCustomConverterProperty.Serialize);
Assert.Throws<InvalidOperationException>(() => SerializationWithPerTypeAttributeContext.Default.ClassWithBadCustomConverter.Serialize);
Assert.Throws<InvalidOperationException>(() => SerializationWithPerTypeAttributeContext.Default.StructWithBadCustomConverter.Serialize);
}
Expand Down
25 changes: 24 additions & 1 deletion src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/TestClasses.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,8 +199,31 @@ public class ClassWithCustomConverter
public int MyInt { get; set; }
}

public class ClassWithCustomConverterProperty
{
[JsonConverter(typeof(NestedPocoCustomConverter))]
public NestedPoco Property { get; set; }

public class NestedPoco
{
public int Value { get; set; }
}

public class NestedPocoCustomConverter : JsonConverter<NestedPoco>
{
public override NestedPoco? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => new NestedPoco { Value = reader.GetInt32() };
public override void Write(Utf8JsonWriter writer, NestedPoco value, JsonSerializerOptions options) => writer.WriteNumberValue(value.Value);
}
}

public struct StructWithCustomConverterProperty
{
[JsonConverter(typeof(ClassWithCustomConverterProperty.NestedPocoCustomConverter))]
public ClassWithCustomConverterProperty.NestedPoco Property { get; set; }
}

[JsonConverter(typeof(CustomConverterForStruct))] // Invalid
public struct ClassWithBadCustomConverter
public class ClassWithBadCustomConverter
{
public int MyInt { get; set; }
}
Expand Down