Skip to content

Commit

Permalink
Ensure source generated metadata properties are read-only.
Browse files Browse the repository at this point in the history
  • Loading branch information
eiriktsarpalis committed Oct 11, 2022
1 parent 20d4e30 commit 66ad2e8
Show file tree
Hide file tree
Showing 8 changed files with 112 additions and 25 deletions.
32 changes: 15 additions & 17 deletions src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1010,6 +1010,8 @@ private static string GenerateFastPathFuncForType(TypeGenerationSpec typeGenSpec

return $@"
// Intentionally not a static method because we create a delegate to it. Invoking delegates to instance
// methods is almost as fast as virtual calls. Static methods need to go through a shuffle thunk.
private void {serializeMethodName}({Utf8JsonWriterTypeRef} {WriterVarName}, {valueTypeRef} {ValueVarName})
{{
{GetEarlyNullCheckSource(emitNullCheck)}
Expand Down Expand Up @@ -1085,16 +1087,19 @@ private static string GenerateForType(TypeGenerationSpec typeMetadata, string me
/// </summary>
public {typeInfoPropertyTypeRef} {typeFriendlyName}
{{
get => _{typeFriendlyName} ??= {typeMetadata.CreateTypeInfoMethodName}({OptionsInstanceVariableName});
get => _{typeFriendlyName} ??= {typeMetadata.CreateTypeInfoMethodName}({OptionsInstanceVariableName}, makeReadOnly: true);
}}

// Intentionally not a static method because we create a delegate to it. Invoking delegates to instance
// methods is almost as fast as virtual calls. Static methods need to go through a shuffle thunk.
private {typeInfoPropertyTypeRef} {typeMetadata.CreateTypeInfoMethodName}({JsonSerializerOptionsTypeRef} {OptionsLocalVariableName})
private {typeInfoPropertyTypeRef} {typeMetadata.CreateTypeInfoMethodName}({JsonSerializerOptionsTypeRef} {OptionsLocalVariableName}, bool makeReadOnly)
{{
{typeInfoPropertyTypeRef}? {JsonTypeInfoReturnValueLocalVariableName} = null;
{WrapWithCheckForCustomConverter(metadataInitSource, typeCompilableName)}

if (makeReadOnly)
{{
{JsonMetadataServicesTypeRef}.MakeReadOnly({JsonTypeInfoReturnValueLocalVariableName});
}}

return {JsonTypeInfoReturnValueLocalVariableName};
}}
{additionalSource}";
Expand Down Expand Up @@ -1271,30 +1276,23 @@ private string GetGetTypeInfoImplementation()
// Explicit IJsonTypeInfoResolver implementation
sb.AppendLine();
sb.Append(@$"{JsonTypeInfoTypeRef}? {JsonTypeInfoResolverTypeRef}.GetTypeInfo({TypeTypeRef} type, {JsonSerializerOptionsTypeRef} {OptionsLocalVariableName})
{{
if ({OptionsInstanceVariableName} == {OptionsLocalVariableName})
{{
return this.GetTypeInfo(type);
}}
else
{{");
{{");
// TODO (https://github.com/dotnet/runtime/issues/52218): Make this Dictionary-lookup-based if root-serializable type count > 64.
foreach (TypeGenerationSpec metadata in types)
{
if (metadata.ClassType != ClassType.TypeUnsupportedBySourceGen)
{
sb.Append($@"
if (type == typeof({metadata.TypeRef}))
{{
return {metadata.CreateTypeInfoMethodName}({OptionsLocalVariableName});
}}
if (type == typeof({metadata.TypeRef}))
{{
return {metadata.CreateTypeInfoMethodName}({OptionsLocalVariableName}, makeReadOnly: false);
}}
");
}
}

sb.Append($@"
return null;
}}
return null;
}}
");

Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Text.Json/ref/System.Text.Json.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1082,6 +1082,7 @@ public static partial class JsonMetadataServices
public static System.Text.Json.Serialization.JsonConverter<System.Text.Json.Nodes.JsonNode> JsonNodeConverter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<System.Text.Json.Nodes.JsonObject> JsonObjectConverter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<System.Text.Json.Nodes.JsonValue> JsonValueConverter { get { throw null; } }
public static void MakeReadOnly(System.Text.Json.Serialization.Metadata.JsonTypeInfo jsonTypeInfo) { throw null; }
public static System.Text.Json.Serialization.JsonConverter<object?> ObjectConverter { get { throw null; } }
[System.CLSCompliantAttribute(false)]
public static System.Text.Json.Serialization.JsonConverter<sbyte> SByteConverter { get { throw null; } }
Expand Down
6 changes: 3 additions & 3 deletions src/libraries/System.Text.Json/src/Resources/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -247,10 +247,10 @@
<value>Cannot add callbacks to the 'Modifiers' property after the resolver has been used for the first time.</value>
</data>
<data name="TypeInfoImmutable" xml:space="preserve">
<value>JsonTypeInfo cannot be changed after first usage.</value>
<value>This JsonTypeInfo instance is marked read-only or has already been used in serialization or deserialization.</value>
</data>
<data name="PropertyInfoImmutable" xml:space="preserve">
<value>JsonPropertyInfo cannot be changed after first usage.</value>
<value>This JsonTypeInfo instance is marked read-only or has already been used in serialization or deserialization.</value>
</data>
<data name="MaxDepthMustBePositive" xml:space="preserve">
<value>Max depth must be positive.</value>
Expand Down Expand Up @@ -337,7 +337,7 @@
<value>The JSON object contains a trailing comma at the end which is not supported in this mode. Change the reader options.</value>
</data>
<data name="SerializerOptionsReadOnly" xml:space="preserve">
<value>Serializer options cannot be changed once serialization or deserialization has occurred.</value>
<value>This JsonSerializerOptions instance is read-only or has already been used in serialization or deserialization.</value>
</data>
<data name="StreamNotWritable" xml:space="preserve">
<value>Stream is not writable.</value>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,5 +94,20 @@ public static JsonTypeInfo<T> CreateValueInfo<T>(JsonSerializerOptions options,
JsonTypeInfo<T> info = new SourceGenJsonTypeInfo<T>(converter, options);
return info;
}

/// <summary>
/// Marks the provided <see cref="JsonTypeInfo"/> instance as locked for further modification.
/// </summary>
/// <param name="jsonTypeInfo">The metadata instance to lock for modification.</param>
/// <exception cref="ArgumentNullException">Thrown when <paramref name="jsonTypeInfo"/> is null.</exception>
public static void MakeReadOnly(JsonTypeInfo jsonTypeInfo)
{
if (jsonTypeInfo is null)
{
ThrowHelper.ThrowArgumentNullException(nameof(jsonTypeInfo));
}

jsonTypeInfo.IsReadOnly = true;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ internal static JsonPropertyInfo GetPropertyPlaceholder()

private protected void VerifyMutable()
{
if (_isConfigured)
if (ParentTypeInfo?.IsReadOnly == true)
{
ThrowHelper.ThrowInvalidOperationException_PropertyInfoImmutable();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ internal JsonTypeInfo(Type type, JsonConverter converter, JsonSerializerOptions

internal void VerifyMutable()
{
if (_isConfigured)
if (IsReadOnly)
{
ThrowHelper.ThrowInvalidOperationException_TypeInfoImmutable();
}
Expand All @@ -489,6 +489,8 @@ internal void VerifyMutable()

internal bool IsConfigured => _isConfigured;

internal bool IsReadOnly { get; set; }

internal void EnsureConfigured()
{
Debug.Assert(!Monitor.IsEntered(_configureLock), "recursive locking detected.");
Expand All @@ -511,6 +513,7 @@ void ConfigureLocked()
{
Configure();

IsReadOnly = true;
_isConfigured = true;
}
catch (Exception e)
Expand Down Expand Up @@ -693,6 +696,7 @@ public static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions o
/// <returns>A blank <see cref="JsonPropertyInfo"/> instance.</returns>
/// <exception cref="ArgumentNullException"><paramref name="propertyType"/> or <paramref name="name"/> is null.</exception>
/// <exception cref="ArgumentException"><paramref name="propertyType"/> cannot be used for serialization.</exception>
/// <exception cref="InvalidOperationException">The <see cref="JsonTypeInfo"/> instance has been locked for further modification.</exception>
[RequiresUnreferencedCode(MetadataFactoryRequiresUnreferencedCode)]
[RequiresDynamicCode(MetadataFactoryRequiresUnreferencedCode)]
public JsonPropertyInfo CreateJsonPropertyInfo(Type propertyType, string name)
Expand All @@ -712,6 +716,7 @@ public JsonPropertyInfo CreateJsonPropertyInfo(Type propertyType, string name)
ThrowHelper.ThrowArgumentException_CannotSerializeInvalidType(nameof(propertyType), propertyType, Type, name);
}

VerifyMutable();
JsonPropertyInfo propertyInfo = CreatePropertyUsingReflection(propertyType);
propertyInfo.Name = name;

Expand Down Expand Up @@ -748,6 +753,8 @@ internal void CacheMember(JsonPropertyInfo jsonPropertyInfo, JsonPropertyDiction
Debug.Assert(jsonPropertyInfo.MemberName != null, "MemberName can be null in custom JsonPropertyInfo instances and should never be passed in this method");
string memberName = jsonPropertyInfo.MemberName;

jsonPropertyInfo.EnsureChildOf(this);

if (jsonPropertyInfo.IsExtensionData)
{
if (ExtensionDataProperty != null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,66 @@ public static void VariousNestingAndVisibilityLevelsAreSupported()
Assert.NotNull(NestedPublicContext.NestedProtectedInternalClass.Default);
}

[Fact]
public static void PropertyMetadataIsImmutable()
{
JsonTypeInfo<Person> typeInfo = PersonJsonContext.Default.Person;

Assert.Throws<InvalidOperationException>(() => typeInfo.CreateObject = null);
Assert.Throws<InvalidOperationException>(() => typeInfo.OnDeserializing = obj => { });
Assert.Throws<InvalidOperationException>(() => typeInfo.Properties.Clear());

JsonPropertyInfo propertyInfo = typeInfo.Properties[0];
Assert.Throws<InvalidOperationException>(() => propertyInfo.Name = "differentName");
Assert.Throws<InvalidOperationException>(() => propertyInfo.NumberHandling = JsonNumberHandling.AllowReadingFromString);
Assert.Throws<InvalidOperationException>(() => propertyInfo.IsRequired = true);
Assert.Throws<InvalidOperationException>(() => propertyInfo.Order = -1);
}

[Fact]
public static void JsonSerializerContext_GetTypeInfo_MetadataIsImmutable()
{
JsonTypeInfo<Person> typeInfo = (JsonTypeInfo<Person>)PersonJsonContext.Default.GetTypeInfo(typeof(Person));

Assert.Throws<InvalidOperationException>(() => typeInfo.CreateObject = null);
Assert.Throws<InvalidOperationException>(() => typeInfo.OnDeserializing = obj => { });
Assert.Throws<InvalidOperationException>(() => typeInfo.Properties.Clear());

JsonPropertyInfo propertyInfo = typeInfo.Properties[0];
Assert.Throws<InvalidOperationException>(() => propertyInfo.Name = "differentName");
Assert.Throws<InvalidOperationException>(() => propertyInfo.NumberHandling = JsonNumberHandling.AllowReadingFromString);
Assert.Throws<InvalidOperationException>(() => propertyInfo.IsRequired = true);
Assert.Throws<InvalidOperationException>(() => propertyInfo.Order = -1);
}

[Fact]
public static void IJsonTypeInfoResolver_GetTypeInfo_MetadataIsMutable()
{
IJsonTypeInfoResolver resolver = PersonJsonContext.Default;
JsonTypeInfo<Person> typeInfo = (JsonTypeInfo<Person>)resolver.GetTypeInfo(typeof(Person), PersonJsonContext.Default.Options);

Assert.NotSame(typeInfo, PersonJsonContext.Default.Person);

JsonTypeInfo<Person> typeInfo2 = (JsonTypeInfo<Person>)resolver.GetTypeInfo(typeof(Person), PersonJsonContext.Default.Options);
Assert.NotSame(typeInfo, typeInfo2);

typeInfo.CreateObject = null;
typeInfo.OnDeserializing = obj => { };

JsonPropertyInfo propertyInfo = typeInfo.Properties[0];
propertyInfo.Name = "differentName";
propertyInfo.NumberHandling = JsonNumberHandling.AllowReadingFromString;
propertyInfo.IsRequired = true;
propertyInfo.Order = -1;

typeInfo.Properties.Clear();
Assert.Equal(0, typeInfo.Properties.Count);

// Changes should not impact other metadata instances
Assert.Equal(2, typeInfo2.Properties.Count);
Assert.Equal(2, PersonJsonContext.Default.Person.Properties.Count);
}

[Fact]
public static void VariousGenericsAreSupported()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -400,17 +400,23 @@ private static void TestTypeInfoImmutability<T>(JsonTypeInfo<T> typeInfo)
Assert.Equal(typeof(T), typeInfo.Type);
Assert.True(typeInfo.Converter.CanConvert(typeof(T)));

JsonPropertyInfo prop = typeInfo.CreateJsonPropertyInfo(typeof(string), "foo");
Assert.True(typeInfo.Properties.IsReadOnly);
Assert.Throws<InvalidOperationException>(() => untyped.CreateObject = untyped.CreateObject);
Assert.Throws<InvalidOperationException>(() => typeInfo.CreateObject = typeInfo.CreateObject);
Assert.Throws<InvalidOperationException>(() => typeInfo.NumberHandling = typeInfo.NumberHandling);
Assert.Throws<InvalidOperationException>(() => typeInfo.CreateJsonPropertyInfo(typeof(string), "foo"));
Assert.Throws<InvalidOperationException>(() => typeInfo.Properties.Clear());
Assert.Throws<InvalidOperationException>(() => typeInfo.Properties.Add(prop));
Assert.Throws<InvalidOperationException>(() => typeInfo.Properties.Insert(0, prop));
Assert.Throws<InvalidOperationException>(() => typeInfo.PolymorphismOptions = null);
Assert.Throws<InvalidOperationException>(() => typeInfo.PolymorphismOptions = new());

if (typeInfo.Properties.Count > 0)
{
JsonPropertyInfo prop = typeInfo.Properties[0];
Assert.Throws<InvalidOperationException>(() => typeInfo.Properties.Add(prop));
Assert.Throws<InvalidOperationException>(() => typeInfo.Properties.Insert(0, prop));
Assert.Throws<InvalidOperationException>(() => typeInfo.Properties.RemoveAt(0));
}

if (typeInfo.PolymorphismOptions is JsonPolymorphismOptions jpo)
{
Assert.True(jpo.DerivedTypes.IsReadOnly);
Expand Down

0 comments on commit 66ad2e8

Please sign in to comment.