diff --git a/src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/JsonHelpers.cs b/src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/JsonHelpers.cs index a95ff4787d550..0fc68616edb4a 100644 --- a/src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/JsonHelpers.cs +++ b/src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/JsonHelpers.cs @@ -23,18 +23,7 @@ internal static JsonTypeInfo GetJsonTypeInfo(Type type, JsonSerializerOptions? o // Resolves JsonTypeInfo metadata using the appropriate JsonSerializerOptions configuration, // following the semantics of the JsonSerializer reflection methods. options ??= s_defaultSerializerOptions; - - if (options.TypeInfoResolver is null) - { - // Public STJ APIs have no way of configuring TypeInfoResolver - // instances in a thread-safe manner. Let STJ do it for us by - // running a simple reflection-based serialization operation. - // TODO remove once https://github.com/dotnet/runtime/issues/89934 is implemented. - JsonSerializer.Deserialize("0"u8, options); - } - - Debug.Assert(options.TypeInfoResolver != null); - Debug.Assert(options.IsReadOnly); + options.MakeReadOnly(populateMissingResolver: true); return options.GetTypeInfo(type); } diff --git a/src/libraries/System.Text.Json/ref/System.Text.Json.cs b/src/libraries/System.Text.Json/ref/System.Text.Json.cs index 251ede78b51b5..0a308dbe6afa9 100644 --- a/src/libraries/System.Text.Json/ref/System.Text.Json.cs +++ b/src/libraries/System.Text.Json/ref/System.Text.Json.cs @@ -402,6 +402,9 @@ public JsonSerializerOptions(System.Text.Json.JsonSerializerOptions options) { } public System.Text.Json.Serialization.JsonConverter GetConverter(System.Type typeToConvert) { throw null; } public System.Text.Json.Serialization.Metadata.JsonTypeInfo GetTypeInfo(System.Type type) { throw null; } public void MakeReadOnly() { } + [System.Diagnostics.CodeAnalysis.RequiresDynamicCodeAttribute("Populating unconfigured TypeInfoResolver properties with the reflection resolver requires runtime code generation.")] + [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Populating unconfigured TypeInfoResolver properties with the reflection resolver requires unreferenced code.")] + public void MakeReadOnly(bool populateMissingResolver) { } public bool TryGetTypeInfo(System.Type type, [System.Diagnostics.CodeAnalysis.NotNullWhenAttribute(true)] out System.Text.Json.Serialization.Metadata.JsonTypeInfo? typeInfo) { throw null; } } public enum JsonTokenType : byte diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Helpers.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Helpers.cs index 5f6435a305936..f654826f5b0b2 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Helpers.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Helpers.cs @@ -34,11 +34,7 @@ private static JsonTypeInfo GetTypeInfo(JsonSerializerOptions? options, Type inp Debug.Assert(inputType != null); options ??= JsonSerializerOptions.Default; - - if (!options.IsConfiguredForJsonSerializer) - { - options.ConfigureForJsonSerializer(); - } + options.MakeReadOnly(populateMissingResolver: true); // In order to improve performance of polymorphic root-level object serialization, // we bypass GetTypeInfoForRootType and cache JsonTypeInfo in a dedicated property. diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs index da2cc4c35c036..8f088a85f4f21 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs @@ -45,11 +45,14 @@ public JsonConverter GetConverter(Type typeToConvert) ThrowHelper.ThrowArgumentNullException(nameof(typeToConvert)); } - if (JsonSerializer.IsReflectionEnabledByDefault && _typeInfoResolver is null) + if (JsonSerializer.IsReflectionEnabledByDefault) { // Backward compatibility -- root & query the default reflection converters // but do not populate the TypeInfoResolver setting. - return DefaultJsonTypeInfoResolver.GetConverterForType(typeToConvert, this); + if (_typeInfoResolver is null) + { + return DefaultJsonTypeInfoResolver.GetConverterForType(typeToConvert, this); + } } return GetConverterInternal(typeToConvert); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs index f421f232c5529..79eb421e57ba5 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs @@ -76,8 +76,6 @@ public static JsonSerializerOptions Default private bool _propertyNameCaseInsensitive; private bool _writeIndented; - private volatile bool _isReadOnly; - /// /// Constructs a new instance. /// @@ -680,18 +678,21 @@ internal bool CanUseFastPathSerializationLogic internal ReferenceHandlingStrategy ReferenceHandlingStrategy = ReferenceHandlingStrategy.None; /// - /// Specifies whether the current instance has been locked for modification. + /// Specifies whether the current instance has been locked for user modification. /// /// /// A instance can be locked either if /// it has been passed to one of the methods, /// has been associated with a instance, - /// or a user explicitly called the method on the instance. + /// or a user explicitly called the methods on the instance. + /// + /// Read-only instances use caching when querying and metadata. /// public bool IsReadOnly => _isReadOnly; + private volatile bool _isReadOnly; /// - /// Locks the current instance for further modification. + /// Marks the current instance as read-only preventing any further user modification. /// /// The instance does not specify a setting. /// This method is idempotent. @@ -706,11 +707,45 @@ public void MakeReadOnly() } /// - /// Configures the instance for use by the JsonSerializer APIs. + /// Marks the current instance as read-only preventing any further user modification. + /// + /// Populates unconfigured properties with the reflection-based default. + /// + /// The instance does not specify a setting. Thrown if is . + /// -OR- + /// The feature switch has been turned off. + /// + /// + /// When is set to , configures the instance following + /// the semantics of the methods accepting parameters. + /// + /// This method is idempotent. + /// + [RequiresUnreferencedCode("Populating unconfigured TypeInfoResolver properties with the reflection resolver requires unreferenced code.")] + [RequiresDynamicCode("Populating unconfigured TypeInfoResolver properties with the reflection resolver requires runtime code generation.")] + public void MakeReadOnly(bool populateMissingResolver) + { + if (populateMissingResolver) + { + if (!_isConfiguredForJsonSerializer) + { + ConfigureForJsonSerializer(); + } + } + else + { + MakeReadOnly(); + } + + Debug.Assert(IsReadOnly); + } + + /// + /// Configures the instance for use by the JsonSerializer APIs, applying reflection-based fallback where applicable. /// [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] - internal void ConfigureForJsonSerializer() + private void ConfigureForJsonSerializer() { if (JsonSerializer.IsReflectionEnabledByDefault) { @@ -734,7 +769,7 @@ internal void ConfigureForJsonSerializer() // A cache has already been created by the source generator. // Repeat the same configuration routine for that options instance, if different. // Invalidate any cache entries that have already been stored. - if (cachingContext.Options != this) + if (cachingContext.Options != this && !cachingContext.Options._isConfiguredForJsonSerializer) { cachingContext.Options.ConfigureForJsonSerializer(); } @@ -751,11 +786,18 @@ internal void ConfigureForJsonSerializer() ThrowHelper.ThrowInvalidOperationException_JsonSerializerIsReflectionDisabled(); } - MakeReadOnly(); + Debug.Assert(_typeInfoResolver != null); + // NB preserve write order. + _isReadOnly = true; _isConfiguredForJsonSerializer = true; } - internal bool IsConfiguredForJsonSerializer => _isConfiguredForJsonSerializer; + /// + /// This flag is supplementary to and is only used to keep track + /// of source-gen reflection fallback (assuming the IsSourceGenReflectionFallbackEnabled feature switch is on). + /// This mode necessitates running the method even + /// for options instances that have been marked as read-only. + /// private volatile bool _isConfiguredForJsonSerializer; // Only populated in .NET 6 compatibility mode encoding reflection fallback in source gen diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs index 4a11d3ceec44a..b0dd581203fa9 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs @@ -176,6 +176,57 @@ public static void NewDefaultOptions_MakeReadOnly_NoTypeInfoResolver_ThrowsInval Assert.True(options.IsReadOnly); } + [Fact] + public static void NewDefaultOptions_MakeReadOnlyWithPopulateParameterFalse_NoTypeInfoResolver_ThrowsInvalidOperation() + { + var options = new JsonSerializerOptions(); + Assert.False(options.IsReadOnly); + + Assert.Throws(() => options.MakeReadOnly(populateMissingResolver: false)); + Assert.Null(options.TypeInfoResolver); + Assert.False(options.IsReadOnly); + } + + [Fact] + public static void NewDefaultOptions_MakeReadOnlyWithPopulateParameterTrue_NoTypeInfoResolver_UsesDefaultTypeInfoResolver() + { + var options = new JsonSerializerOptions(); + Assert.Null(options.TypeInfoResolver); + Assert.False(options.IsReadOnly); + + options.MakeReadOnly(populateMissingResolver: true); + + DefaultJsonTypeInfoResolver resolver = Assert.IsType(options.TypeInfoResolver); + Assert.True(options.IsReadOnly); + + // Method is idempotent + options.MakeReadOnly(populateMissingResolver: false); + Assert.True(options.IsReadOnly); + Assert.Same(resolver, options.TypeInfoResolver); + + options.MakeReadOnly(populateMissingResolver: true); + Assert.True(options.IsReadOnly); + Assert.Same(resolver, options.TypeInfoResolver); + + options.MakeReadOnly(); + Assert.True(options.IsReadOnly); + Assert.Same(resolver, options.TypeInfoResolver); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public static void NewDefaultOptions_MakeReadOnlyWithPopulateParameter_HasTypeInfoResolver_DoesNotReplaceResolver(bool populateMissingResolver) + { + var options = new JsonSerializerOptions { TypeInfoResolver = JsonContext.Default }; + Assert.False(options.IsReadOnly); + + options.MakeReadOnly(populateMissingResolver); + + Assert.True(options.IsReadOnly); + Assert.Same(JsonContext.Default, options.TypeInfoResolver); + } + [Fact] public static void TypeInfoResolverCannotBeSetAfterAddingContext() { @@ -565,10 +616,15 @@ public static void Options_DisablingIsReflectionEnabledByDefaultSwitch_NewOption ex = Assert.Throws(() => JsonSerializer.Deserialize("\"string\"", options)); Assert.Contains("JsonSerializerOptions.TypeInfoResolver", ex.Message); + ex = Assert.Throws(() => options.MakeReadOnly(populateMissingResolver: true)); + Assert.Contains("JsonSerializerOptions.TypeInfoResolver", ex.Message); + Assert.False(options.TryGetTypeInfo(typeof(string), out JsonTypeInfo? typeInfo)); Assert.Null(typeInfo); Assert.False(options.IsReadOnly); // failed operations should not lock the instance + Assert.Empty(options.TypeInfoResolverChain); + Assert.Null(options.TypeInfoResolver); // Can still use reflection via explicit configuration options.TypeInfoResolver = new DefaultJsonTypeInfoResolver(); @@ -973,7 +1029,11 @@ public static void JsonSerializerOptions_Default_IsReadOnly() Assert.Throws(() => resolver.Modifiers.Add(ti => { })); Assert.Throws(() => resolver.Modifiers.Insert(0, ti => { })); - optionsSingleton.MakeReadOnly(); // MakeReadOnly is idempotent. + // MakeReadOnly is idempotent. + optionsSingleton.MakeReadOnly(); + optionsSingleton.MakeReadOnly(populateMissingResolver: false); + optionsSingleton.MakeReadOnly(populateMissingResolver: true); + Assert.Same(resolver, optionsSingleton.TypeInfoResolver); } [Theory] diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/IsReflectionEnabledByDefaultFalse.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/IsReflectionEnabledByDefaultFalse.cs index d126c2cd26e66..28b2d8daa44be 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/IsReflectionEnabledByDefaultFalse.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/TrimmingTests/IsReflectionEnabledByDefaultFalse.cs @@ -44,18 +44,41 @@ public static int Main() { } + // Calling GetConverter should throw NotSupportedException. + try + { + _ = options.GetConverter(typeof(MyPoco)); + return -4; + } + catch (NotSupportedException) + { + } + + // Calling MakeReadOnly(populateMissingResolver: true) should throw InvalidOperationException. + try + { + options.MakeReadOnly(populateMissingResolver: true); + return -5; + } + catch (InvalidOperationException) + { + } + // Serializing with a custom resolver should work as expected. options.TypeInfoResolver = new MyJsonResolver(); + options.MakeReadOnly(populateMissingResolver: true); + options.GetConverter(typeof(MyPoco)); + if (JsonSerializer.Serialize(valueToSerialize, options) != "{\"Value\":42}") { - return -4; + return -6; } // The Default resolver should have been trimmed from the application. Type? reflectionResolver = GetJsonType("System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver"); if (reflectionResolver != null) { - return -5; + return -7; } return 100;