Skip to content
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.

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

Ensure metadata originating from converters is surfaced while JsonTypeInfo is mutable. #72483

Merged
merged 1 commit into from
Jul 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/src/Resources/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@
<value>The JSON property name for '{0}.{1}' cannot be null.</value>
</data>
<data name="SerializationDataExtensionPropertyInvalid" xml:space="preserve">
<value>The data extension property '{0}.{1}' is invalid. It must implement 'IDictionary&lt;string, JsonElement&gt;' or 'IDictionary&lt;string, object&gt;', or be 'JsonObject'.</value>
<value>The extension data property '{0}.{1}' is invalid. It must implement 'IDictionary&lt;string, JsonElement&gt;' or 'IDictionary&lt;string, object&gt;', or be 'JsonObject'.</value>
</data>
<data name="SerializationDuplicateTypeAttribute" xml:space="preserve">
<value>The type '{0}' cannot have more than one member that has the attribute '{1}'.</value>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@

using System.Diagnostics;
using System.Text.Json.Nodes;
using System.Text.Json.Serialization.Metadata;

namespace System.Text.Json.Serialization.Converters
{
internal sealed class JsonObjectConverter : JsonConverter<JsonObject>
{
internal override object CreateObject(JsonSerializerOptions options)
internal override void ConfigureJsonTypeInfo(JsonTypeInfo jsonTypeInfo, JsonSerializerOptions options)
{
return new JsonObject(options.GetNodeOptions());
jsonTypeInfo.CreateObjectForExtensionDataProperty = () => new JsonObject(options.GetNodeOptions());
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
}

internal override void ReadElementAndSetProperty(
Expand All @@ -27,7 +28,7 @@ internal override void ReadElementAndSetProperty(
JsonObject jObject = (JsonObject)obj;

Debug.Assert(value == null || value is JsonNode);
JsonNode? jNodeValue = (JsonNode?)value;
JsonNode? jNodeValue = value;
Copy link
Member

Choose a reason for hiding this comment

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

nit: consider removing assert above


jObject[propertyName] = jNodeValue;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ internal sealed override bool OnTryRead(ref Utf8JsonReader reader, Type typeToCo
{
Debug.Assert(jsonPropertyInfo == state.Current.JsonTypeInfo.ExtensionDataProperty);
state.Current.JsonPropertyNameAsString = dataExtKey;
JsonSerializer.CreateDataExtensionProperty(obj, jsonPropertyInfo, options);
JsonSerializer.CreateExtensionDataProperty(obj, jsonPropertyInfo, options);
}

ReadPropertyValue(obj, ref state, ref tempReader, jsonPropertyInfo, useExtensionProperty);
Expand Down Expand Up @@ -190,7 +190,7 @@ internal sealed override bool OnTryRead(ref Utf8JsonReader reader, Type typeToCo
{
Debug.Assert(jsonPropertyInfo == state.Current.JsonTypeInfo.ExtensionDataProperty);

JsonSerializer.CreateDataExtensionProperty(obj, jsonPropertyInfo, options);
JsonSerializer.CreateExtensionDataProperty(obj, jsonPropertyInfo, options);
object extDictionary = jsonPropertyInfo.GetValueAsObject(obj)!;

if (extDictionary is IDictionary<string, JsonElement> dict)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,6 @@ internal JsonConverter() { }
/// </summary>
internal bool RequiresReadAhead { get; set; }

/// <summary>
/// Used to support JsonObject as an extension property in a loosely-typed, trimmable manner.
/// </summary>
internal virtual object CreateObject(JsonSerializerOptions options)
{
throw new InvalidOperationException(SR.NodeJsonObjectCustomConverterNotAllowedOnExtensionProperty);
}

/// <summary>
/// Used to support JsonObject as an extension property in a loosely-typed, trimmable manner.
/// </summary>
Expand All @@ -64,7 +56,9 @@ internal virtual void ReadElementAndSetProperty(
JsonSerializerOptions options,
ref ReadStack state)
{
throw new InvalidOperationException(SR.NodeJsonObjectCustomConverterNotAllowedOnExtensionProperty);
Debug.Fail("Should not be reachable.");
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved

throw new InvalidOperationException();
}

internal abstract JsonParameterInfo CreateJsonParameterInfo();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ internal static JsonPropertyInfo LookupProperty(
if (createExtensionProperty)
{
Debug.Assert(obj != null, "obj is null");
CreateDataExtensionProperty(obj, dataExtProperty, options);
CreateExtensionDataProperty(obj, dataExtProperty, options);
}

jsonPropertyInfo = dataExtProperty;
Expand Down Expand Up @@ -98,7 +98,7 @@ internal static ReadOnlySpan<byte> GetPropertyName(
return unescapedPropertyName;
}

internal static void CreateDataExtensionProperty(
internal static void CreateExtensionDataProperty(
object obj,
JsonPropertyInfo jsonPropertyInfo,
JsonSerializerOptions options)
Expand Down Expand Up @@ -130,18 +130,15 @@ internal static void CreateDataExtensionProperty(
// Avoid a reference to the JsonNode type for trimming
if (jsonPropertyInfo.PropertyType.FullName == JsonTypeInfo.JsonObjectTypeName)
{
extensionData = jsonPropertyInfo.EffectiveConverter.CreateObject(options);
ThrowHelper.ThrowInvalidOperationException_NodeJsonObjectCustomConverterNotAllowedOnExtensionProperty();
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
ThrowHelper.ThrowNotSupportedException_SerializationNotSupported(jsonPropertyInfo.PropertyType);
}
}
else
{
extensionData = createObjectForExtensionDataProp();
}

extensionData = createObjectForExtensionDataProp();
jsonPropertyInfo.SetExtensionDictionaryAsObject(obj, extensionData);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public Func<object>? CreateObject
private protected abstract void SetCreateObject(Delegate? createObject);
private protected Func<object>? _createObject;

internal Func<object>? CreateObjectForExtensionDataProperty { get; private protected set; }
internal Func<object>? CreateObjectForExtensionDataProperty { get; set; }

/// <summary>
/// Gets or sets a callback to be invoked before serialization occurs.
Expand Down Expand Up @@ -523,7 +523,7 @@ void ConfigureLocked()
}
}

internal virtual void Configure()
internal void Configure()
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
{
Debug.Assert(Monitor.IsEntered(_configureLock), "Configure called directly, use EnsureConfigured which locks this method");

Expand All @@ -539,8 +539,6 @@ internal virtual void Configure()
Debug.Assert(PropertyInfoForTypeInfo.ConverterStrategy == Converter.ConverterStrategy,
$"ConverterStrategy from PropertyInfoForTypeInfo.ConverterStrategy ({PropertyInfoForTypeInfo.ConverterStrategy}) does not match converter's ({Converter.ConverterStrategy})");

converter.ConfigureJsonTypeInfo(this, Options);

if (Kind == JsonTypeInfoKind.Object)
{
InitializePropertyCache();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,16 +35,10 @@ internal ReflectionJsonTypeInfo(JsonConverter converter, JsonSerializerOptions o
}

CreateObjectForExtensionDataProperty = createObject;
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "The ctor is marked as RequiresUnreferencedCode")]
[UnconditionalSuppressMessage("AotAnalysis", "IL3050:RequiresDynamicCode",
Justification = "The ctor is marked RequiresDynamicCode.")]
internal override void Configure()
{
base.Configure();
Converter.ConfigureJsonTypeInfoUsingReflection(this, Options);
// Plug in any converter configuration -- should be run last.
converter.ConfigureJsonTypeInfo(this, options);
converter.ConfigureJsonTypeInfoUsingReflection(this, options);
}

[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,15 @@ public SourceGenJsonTypeInfo(JsonConverter converter, JsonSerializerOptions opti
{
PopulatePolymorphismMetadata();
MapInterfaceTypesToCallbacks();

// Plug in any converter configuration -- should be run last.
converter.ConfigureJsonTypeInfo(this, options);
}

/// <summary>
/// Creates serialization metadata for an object.
/// </summary>
public SourceGenJsonTypeInfo(JsonSerializerOptions options, JsonObjectInfoValues<T> objectInfo) : this(GetConverter(objectInfo), options)
public SourceGenJsonTypeInfo(JsonSerializerOptions options, JsonObjectInfoValues<T> objectInfo) : base(GetConverter(objectInfo), options)
{
if (objectInfo.ObjectWithParameterizedConstructorCreator != null)
{
Expand All @@ -43,6 +46,11 @@ public SourceGenJsonTypeInfo(JsonSerializerOptions options, JsonObjectInfoValues
PropInitFunc = objectInfo.PropertyMetadataInitializer;
SerializeHandler = objectInfo.SerializeHandler;
NumberHandling = objectInfo.NumberHandling;
PopulatePolymorphismMetadata();
MapInterfaceTypesToCallbacks();
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved

// Plug in any converter configuration -- should be run last.
Converter.ConfigureJsonTypeInfo(this, options);
}

/// <summary>
Expand All @@ -54,7 +62,7 @@ public SourceGenJsonTypeInfo(
Func<JsonConverter<T>> converterCreator,
object? createObjectWithArgs = null,
object? addFunc = null)
: this(new JsonMetadataServicesConverter<T>(converterCreator()), options)
: base(new JsonMetadataServicesConverter<T>(converterCreator()), options)
{
if (collectionInfo is null)
{
Expand All @@ -69,6 +77,11 @@ public SourceGenJsonTypeInfo(
CreateObjectWithArgs = createObjectWithArgs;
AddMethodDelegate = addFunc;
CreateObject = collectionInfo.ObjectCreator;
PopulatePolymorphismMetadata();
MapInterfaceTypesToCallbacks();

// Plug in any converter configuration -- should be run last.
Converter.ConfigureJsonTypeInfo(this, options);
}

private static JsonConverter GetConverter(JsonObjectInfoValues<T> objectInfo)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,12 @@ public static void ThrowInvalidOperationException_SerializationDataExtensionProp
throw new InvalidOperationException(SR.Format(SR.SerializationDataExtensionPropertyInvalid, jsonPropertyInfo.PropertyType, jsonPropertyInfo.MemberName));
}

[DoesNotReturn]
public static void ThrowInvalidOperationException_NodeJsonObjectCustomConverterNotAllowedOnExtensionProperty()
{
throw new InvalidOperationException(SR.NodeJsonObjectCustomConverterNotAllowedOnExtensionProperty);
}

[DoesNotReturn]
public static void ThrowNotSupportedException(ref ReadStack state, in Utf8JsonReader reader, NotSupportedException ex)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
Expand Down Expand Up @@ -942,6 +943,24 @@ public static void JsonConstructorAttributeIsOverriddenAndPropertiesAreSetWhenCr
Assert.True(deserialized.E);
}


[Theory]
[InlineData(typeof(ICollection<string>))]
[InlineData(typeof(IList))]
[InlineData(typeof(IList<bool>))]
[InlineData(typeof(IDictionary))]
[InlineData(typeof(IDictionary<string, bool>))]
[InlineData(typeof(ISet<Guid>))]
public static void AbstractCollectionMetadata_SurfacesCreateObjectWhereApplicable(Type type)
{
var options = new JsonSerializerOptions();
var resolver = new DefaultJsonTypeInfoResolver();

JsonTypeInfo metadata = resolver.GetTypeInfo(type, options);
Assert.NotNull(metadata.CreateObject);
Assert.IsAssignableFrom(type, metadata.CreateObject());
}

private class ClassWithLargeParameterizedConstructor
{
public int A { get; set; }
Expand Down