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.

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

Fix STJ support for System.Reflection.NullabilityInfoContext.IsSupported = false #102852

Merged
merged 4 commits into from
May 30, 2024
Merged

Fix STJ support for System.Reflection.NullabilityInfoContext.IsSupported = false #102852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cc4d9f4
Fix STJ support for System.Reflection.NullabilityInfoContext.IsSuppor…
eiriktsarpalis May 29, 2024
d51ae07
Update src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Se…
jozkee May 30, 2024
79ae309
Bring back exception in cases where RespectNullability has been turne…
eiriktsarpalis May 30, 2024
ec5eb17
Update src/libraries/System.Text.Json/src/System/Text/Json/Serializat…
eiriktsarpalis May 30, 2024
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
39 changes: 26 additions & 13 deletions ...t.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.Helpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ namespace System.Text.Json.Serialization.Metadata
{
public partial class DefaultJsonTypeInfoResolver
{
private static readonly bool s_isNullabilityInfoContextSupported =
AppContext.TryGetSwitch("System.Reflection.NullabilityInfoContext.IsSupported", out bool isSupported) ? isSupported : true;

internal static MemberAccessor MemberAccessor
{
[RequiresUnreferencedCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
Expand Down Expand Up @@ -71,7 +74,10 @@ private static JsonTypeInfo CreateTypeInfoCore(Type type, JsonConverter converte

if (typeInfo.Kind is JsonTypeInfoKind.Object)
{
NullabilityInfoContext nullabilityCtx = new();
// If the System.Reflection.NullabilityInfoContext.IsSupported feature switch has been disabled,
// we want to avoid resolving nullability information for properties and parameters unless the
// user has explicitly opted into nullability enforcement in which case an exception will be surfaced.
NullabilityInfoContext? nullabilityCtx = s_isNullabilityInfoContextSupported || options.RespectNullableAnnotations ? new() : null;

if (converter.ConstructorIsParameterized)
{
Expand All @@ -91,7 +97,7 @@ private static JsonTypeInfo CreateTypeInfoCore(Type type, JsonConverter converte

[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
[RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
private static void PopulateProperties(JsonTypeInfo typeInfo, NullabilityInfoContext nullabilityCtx)
private static void PopulateProperties(JsonTypeInfo typeInfo, NullabilityInfoContext? nullabilityCtx)
{
Debug.Assert(!typeInfo.IsReadOnly);
Debug.Assert(typeInfo.Kind is JsonTypeInfoKind.Object);
Expand Down Expand Up @@ -158,7 +164,7 @@ private static void PopulateProperties(JsonTypeInfo typeInfo, NullabilityInfoCon
private static void AddMembersDeclaredBySuperType(
JsonTypeInfo typeInfo,
Type currentType,
NullabilityInfoContext nullabilityCtx,
NullabilityInfoContext? nullabilityCtx,
bool constructorHasSetsRequiredMembersAttribute,
ref JsonTypeInfo.PropertyHierarchyResolutionState state)
{
Expand Down Expand Up @@ -219,7 +225,7 @@ private static void AddMember(
JsonTypeInfo typeInfo,
Type typeToConvert,
MemberInfo memberInfo,
NullabilityInfoContext nullabilityCtx,
NullabilityInfoContext? nullabilityCtx,
bool shouldCheckForRequiredKeyword,
bool hasJsonIncludeAttribute,
ref JsonTypeInfo.PropertyHierarchyResolutionState state)
Expand All @@ -241,7 +247,7 @@ private static void AddMember(
JsonTypeInfo typeInfo,
Type typeToConvert,
MemberInfo memberInfo,
NullabilityInfoContext nullabilityCtx,
NullabilityInfoContext? nullabilityCtx,
JsonSerializerOptions options,
bool shouldCheckForRequiredKeyword,
bool hasJsonIncludeAttribute)
Expand Down Expand Up @@ -301,7 +307,7 @@ private static bool PropertyIsOverriddenAndIgnored(PropertyInfo propertyInfo, Di

[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
[RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
private static void PopulateParameterInfoValues(JsonTypeInfo typeInfo, NullabilityInfoContext nullabilityCtx)
private static void PopulateParameterInfoValues(JsonTypeInfo typeInfo, NullabilityInfoContext? nullabilityCtx)
{
Debug.Assert(typeInfo.Converter.ConstructorInfo != null);
ParameterInfo[] parameters = typeInfo.Converter.ConstructorInfo.GetParameters();
Expand All @@ -326,9 +332,7 @@ private static void PopulateParameterInfoValues(JsonTypeInfo typeInfo, Nullabili
Position = reflectionInfo.Position,
HasDefaultValue = reflectionInfo.HasDefaultValue,
DefaultValue = reflectionInfo.GetDefaultValue(),
IsNullable =
reflectionInfo.ParameterType.IsNullableType() &&
DetermineParameterNullability(reflectionInfo, nullabilityCtx) is not NullabilityState.NotNull,
IsNullable = DetermineParameterNullability(reflectionInfo, nullabilityCtx) is not NullabilityState.NotNull,
};

jsonParameters[i] = jsonInfo;
Expand All @@ -344,7 +348,7 @@ private static void PopulatePropertyInfo(
MemberInfo memberInfo,
JsonConverter? customConverter,
JsonIgnoreCondition? ignoreCondition,
NullabilityInfoContext nullabilityCtx,
NullabilityInfoContext? nullabilityCtx,
bool shouldCheckForRequiredKeyword,
bool hasJsonIncludeAttribute)
{
Expand Down Expand Up @@ -487,9 +491,9 @@ internal static void DeterminePropertyAccessors<T>(JsonPropertyInfo<T> jsonPrope

[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
[RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
private static void DeterminePropertyNullability(JsonPropertyInfo propertyInfo, MemberInfo memberInfo, NullabilityInfoContext nullabilityCtx)
private static void DeterminePropertyNullability(JsonPropertyInfo propertyInfo, MemberInfo memberInfo, NullabilityInfoContext? nullabilityCtx)
{
if (!propertyInfo.PropertyTypeCanBeNull)
if (!propertyInfo.PropertyTypeCanBeNull || nullabilityCtx is null)
{
return;
}
Expand All @@ -511,8 +515,17 @@ private static void DeterminePropertyNullability(JsonPropertyInfo propertyInfo,

[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
[RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
private static NullabilityState DetermineParameterNullability(ParameterInfo parameterInfo, NullabilityInfoContext nullabilityCtx)
private static NullabilityState DetermineParameterNullability(ParameterInfo parameterInfo, NullabilityInfoContext? nullabilityCtx)
{
if (!parameterInfo.ParameterType.IsNullableType())
{
return NullabilityState.NotNull;
}

if (nullabilityCtx is null)
{
return NullabilityState.Unknown;
}
#if NET8_0
// Workaround for https://github.com/dotnet/runtime/issues/92487
// The fix has been incorporated into .NET 9 (and the polyfilled implementations in netfx).
Expand Down
54 changes: 54 additions & 0 deletions src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -960,6 +960,60 @@ public static void Options_RespectNullableAnnotationsDefault_FeatureSwitch(bool?
}, arg, options).Dispose();
}

[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public static void Options_NullabilityInfoFeatureSwitchDisabled_ReportsPropertiesAsNullable()
{
var options = new RemoteInvokeOptions()
{
RuntimeConfigurationOptions =
{
["System.Reflection.NullabilityInfoContext.IsSupported"] = false
}
};

RemoteExecutor.Invoke(static () =>
{
var value = new NullableAnnotationsTests.NotNullablePropertyClass();
string expectedJson = """{"Property":null}""";

Assert.Null(value.Property);
string json = JsonSerializer.Serialize(value);
Assert.Equal(expectedJson, json);
value = JsonSerializer.Deserialize<NullableAnnotationsTests.NotNullablePropertyClass>(json);
Assert.Null(value.Property);
jozkee marked this conversation as resolved.
Show resolved Hide resolved

}, options).Dispose();
}

[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public static void Options_NullabilityInfoFeatureSwitchDisabled_RespectNullabilityAnnotationsEnabled_ThrowsInvalidOperationException()
{
var options = new RemoteInvokeOptions()
{
RuntimeConfigurationOptions =
{
["System.Reflection.NullabilityInfoContext.IsSupported"] = false
}
};

RemoteExecutor.Invoke(static () =>
{
var jsonOptions = new JsonSerializerOptions { RespectNullableAnnotations = true };
var value = new NullableAnnotationsTests.NotNullablePropertyClass();
string expectedJson = """{"Property":null}""";
InvalidOperationException ex;

ex = Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(value, jsonOptions));
Assert.Contains("System.Reflection.NullabilityInfoContext.IsSupported", ex.Message);

ex = Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize<NullableAnnotationsTests.NotNullablePropertyClass>(expectedJson, jsonOptions));
Assert.Contains("System.Reflection.NullabilityInfoContext.IsSupported", ex.Message);

}, options).Dispose();
}

private static void GenericObjectOrJsonElementConverterTestHelper<T>(string converterName, object objectValue, string stringValue)
{
var options = new JsonSerializerOptions();
Expand Down