From 987ceb8618dd7eb0369c45c5d7a0aa05ccebb5ff Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Sun, 10 Jul 2022 18:40:28 +0100 Subject: [PATCH 1/7] Fix contract customization being ignored over prexisting JsonIgnoreCondition settings --- .../Object/ObjectDefaultConverter.cs | 12 +- ...ParameterizedConstructorConverter.Large.cs | 2 +- ...ParameterizedConstructorConverter.Small.cs | 2 +- ...ctWithParameterizedConstructorConverter.cs | 6 +- .../Metadata/JsonParameterInfo.cs | 4 +- .../Metadata/JsonPropertyInfo.cs | 202 +++-------- .../Metadata/JsonPropertyInfoOfT.cs | 227 +++++++++---- .../Metadata/ReflectionJsonTypeInfoOfT.cs | 4 +- ...nTypeInfoResolverTests.JsonPropertyInfo.cs | 319 +++++++++++++++++- 9 files changed, 526 insertions(+), 252 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs index 9871d76f83d5b0..679c0bc5b1c4d4 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs @@ -200,7 +200,7 @@ internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, if (state.Current.PropertyState < StackFramePropertyState.ReadValue) { - if (!jsonPropertyInfo.CanDeserialize) + if (!jsonPropertyInfo.HasSetter) { if (!reader.TrySkip()) { @@ -289,7 +289,7 @@ internal sealed override bool OnTryWrite( for (int i = 0; i < properties.Count; i++) { JsonPropertyInfo jsonPropertyInfo = properties[i].Value; - if (jsonPropertyInfo.CanSerialize) + if (jsonPropertyInfo.HasGetter) { // Remember the current property for JsonPath support if an exception is thrown. state.Current.JsonPropertyInfo = jsonPropertyInfo; @@ -305,7 +305,7 @@ internal sealed override bool OnTryWrite( // Write extension data after the normal properties. JsonPropertyInfo? extensionDataProperty = jsonTypeInfo.ExtensionDataProperty; - if (extensionDataProperty?.CanSerialize == true) + if (extensionDataProperty?.HasGetter == true) { // Remember the current property for JsonPath support if an exception is thrown. state.Current.JsonPropertyInfo = extensionDataProperty; @@ -339,7 +339,7 @@ internal sealed override bool OnTryWrite( while (state.Current.EnumeratorIndex < propertyList.Count) { JsonPropertyInfo jsonPropertyInfo = propertyList[state.Current.EnumeratorIndex].Value; - if (jsonPropertyInfo.CanSerialize) + if (jsonPropertyInfo.HasGetter) { state.Current.JsonPropertyInfo = jsonPropertyInfo; state.Current.NumberHandling = jsonPropertyInfo.EffectiveNumberHandling; @@ -368,7 +368,7 @@ internal sealed override bool OnTryWrite( if (state.Current.EnumeratorIndex == propertyList.Count) { JsonPropertyInfo? extensionDataProperty = jsonTypeInfo.ExtensionDataProperty; - if (extensionDataProperty?.CanSerialize == true) + if (extensionDataProperty?.HasGetter == true) { // Remember the current property for JsonPath support if an exception is thrown. state.Current.JsonPropertyInfo = extensionDataProperty; @@ -415,7 +415,7 @@ protected static void ReadPropertyValue( bool useExtensionProperty) { // Skip the property if not found. - if (!jsonPropertyInfo.CanDeserialize) + if (!jsonPropertyInfo.HasSetter) { reader.Skip(); } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs index ba190f1bf1c1b4..a369e8a349bcf6 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs @@ -21,7 +21,7 @@ protected sealed override bool ReadAndCacheConstructorArgument(ref ReadStack sta bool success = jsonParameterInfo.ConverterBase.TryReadAsObject(ref reader, jsonParameterInfo.Options!, ref state, out object? arg); - if (success && !(arg == null && jsonParameterInfo.IgnoreDefaultValuesOnRead)) + if (success && !(arg == null && jsonParameterInfo.IgnoreNullTokensOnRead)) { ((object[])state.Current.CtorArgumentState!.Arguments)[jsonParameterInfo.ClrInfo.Position] = arg!; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs index f206adba75f2d2..913198dd364199 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs @@ -67,7 +67,7 @@ private static bool TryRead( bool success = converter.TryRead(ref reader, info.PropertyType, info.Options!, ref state, out TArg? value); - arg = value == null && jsonParameterInfo.IgnoreDefaultValuesOnRead + arg = value == null && jsonParameterInfo.IgnoreNullTokensOnRead ? (TArg?)info.DefaultValue! // Use default value specified on parameter, if any. : value!; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs index 4fa80afa6c8066..95809800844089 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs @@ -287,7 +287,7 @@ private void ReadConstructorArguments(ref ReadStack state, ref Utf8JsonReader re out _, createExtensionProperty: false); - if (jsonPropertyInfo.CanDeserialize) + if (jsonPropertyInfo.HasSetter) { ArgumentState argumentState = state.Current.CtorArgumentState!; @@ -452,7 +452,7 @@ private static bool HandlePropertyWithContinuation( { if (state.Current.PropertyState < StackFramePropertyState.ReadValue) { - if (!jsonPropertyInfo.CanDeserialize) + if (!jsonPropertyInfo.HasSetter) { if (!reader.TrySkip()) { @@ -486,7 +486,7 @@ private static bool HandlePropertyWithContinuation( } } - Debug.Assert(jsonPropertyInfo.CanDeserialize); + Debug.Assert(jsonPropertyInfo.HasSetter); // Ensure that the cache has enough capacity to add this property. diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs index 76866852d2bc5d..e5342b24c0791c 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs @@ -20,7 +20,7 @@ internal abstract class JsonParameterInfo // The default value of the parameter. This is `DefaultValue` of the `ParameterInfo`, if specified, or the CLR `default` for the `ParameterType`. public object? DefaultValue { get; private protected set; } - public bool IgnoreDefaultValuesOnRead { get; private set; } + public bool IgnoreNullTokensOnRead { get; private set; } // Options can be referenced here since all JsonPropertyInfos originate from a JsonTypeInfo that is cached on JsonSerializerOptions. public JsonSerializerOptions? Options { get; set; } // initialized in Init method @@ -62,7 +62,7 @@ public virtual void Initialize(JsonParameterInfoValues parameterInfo, JsonProper PropertyType = matchingProperty.PropertyType; NameAsUtf8Bytes = matchingProperty.NameAsUtf8Bytes!; ConverterBase = matchingProperty.EffectiveConverter; - IgnoreDefaultValuesOnRead = matchingProperty.IgnoreDefaultValuesOnRead; + IgnoreNullTokensOnRead = matchingProperty.IgnoreNullTokensOnRead; NumberHandling = matchingProperty.EffectiveNumberHandling; MatchingPropertyCanBeNull = matchingProperty.PropertyTypeCanBeNull; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs index 70ab27004493cc..9a9d2a3359d2ac 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs @@ -53,6 +53,7 @@ public JsonConverter? CustomConverter set { VerifyMutable(); + DetermineEffectiveConverter(value); _customConverter = value; } } @@ -68,7 +69,11 @@ public JsonConverter? CustomConverter public Func? Get { get => _untypedGet; - set => SetGetter(value); + set + { + VerifyMutable(); + SetGetter(value); + } } /// @@ -80,7 +85,14 @@ public JsonConverter? CustomConverter public Action? Set { get => _untypedSet; - set => SetSetter(value); + set + { + VerifyMutable(); + SetSetter(value); + // Invalidate any JsonIgnore configuration if delegate set manually by user + IgnoreNullTokensOnRead = false; + _ignoreCondition = null; + } } private protected Func? _untypedGet; @@ -106,32 +118,29 @@ public JsonConverter? CustomConverter set { VerifyMutable(); - _shouldSerialize = value; - // By default we will go through faster path (not using delegate) and use IgnoreCondition - // If user sets it explicitly we always go through delegate + SetShouldSerialize(value); + // Invalidate any JsonIgnore configuration if delegate set manually by user + IgnoreDefaultValuesOnWrite = false; _ignoreCondition = null; - _shouldSerializeIsExplicitlySet = true; } } + private protected Func? _shouldSerialize; + private protected abstract void SetShouldSerialize(Delegate? predicate); + internal JsonIgnoreCondition? IgnoreCondition { get => _ignoreCondition; set { Debug.Assert(!_isConfigured); - + ConfigureIgnoreCondition(value); _ignoreCondition = value; - _shouldSerialize = value != null ? GetShouldSerializeForIgnoreCondition(value.Value) : null; - _shouldSerializeIsExplicitlySet = false; } } - private Func? _shouldSerialize; - private bool _shouldSerializeIsExplicitlySet; private JsonIgnoreCondition? _ignoreCondition; - - private protected abstract Func GetShouldSerializeForIgnoreCondition(JsonIgnoreCondition condition); + private protected abstract void ConfigureIgnoreCondition(JsonIgnoreCondition? ignoreCondition); /// /// Gets or sets a custom attribute provider for the current property. @@ -199,8 +208,8 @@ internal static JsonPropertyInfo GetPropertyPlaceholder() JsonPropertyInfo info = new JsonPropertyInfo(typeof(object), declaringTypeInfo: null, options: null!); Debug.Assert(!info.IsForTypeInfo); - Debug.Assert(!info.CanDeserialize); - Debug.Assert(!info.CanSerialize); + Debug.Assert(!info.HasSetter); + Debug.Assert(!info.HasGetter); info.Name = string.Empty; @@ -220,6 +229,7 @@ private protected void VerifyMutable() } } + internal bool IsConfigured => _isConfigured; private volatile bool _isConfigured; internal void EnsureConfigured() @@ -246,13 +256,12 @@ internal void Configure() CacheNameAsUtf8BytesAndEscapedNameSection(); } - if (IsIgnored) + if (EffectiveConverter is null) { - return; + Debug.Assert(CustomConverter is null); + DetermineEffectiveConverter(customConverter: null); } - DetermineEffectiveConverter(); - if (IsForTypeInfo) { DetermineNumberHandlingForTypeInfo(); @@ -260,17 +269,10 @@ internal void Configure() else { DetermineNumberHandlingForProperty(); - - if (!IsIgnored) - { - DetermineIgnoreCondition(IgnoreCondition); - } - - DetermineSerializationCapabilities(IgnoreCondition); } } - private protected abstract void DetermineEffectiveConverter(); + private protected abstract void DetermineEffectiveConverter(JsonConverter? customConverter); private protected abstract void DetermineMemberAccessors(MemberInfo memberInfo); private void DeterminePoliciesFromMember(MemberInfo memberInfo) @@ -315,118 +317,6 @@ internal void CacheNameAsUtf8BytesAndEscapedNameSection() EscapedNameSection = JsonHelpers.GetEscapedPropertyNameSection(NameAsUtf8Bytes, Options.Encoder); } - internal void DetermineSerializationCapabilities(JsonIgnoreCondition? ignoreCondition) - { - if (IsIgnored) - { - CanSerialize = false; - CanDeserialize = false; - return; - } - - Debug.Assert(MemberType == MemberTypes.Property || MemberType == MemberTypes.Field || MemberType == default); - - if ((ConverterStrategy & (ConverterStrategy.Enumerable | ConverterStrategy.Dictionary)) == 0) - { - Debug.Assert(ignoreCondition != JsonIgnoreCondition.Always); - - // Three possible values for ignoreCondition: - // null = JsonIgnore was not placed on this property, global IgnoreReadOnlyProperties/Fields wins - // WhenNull = only ignore when null, global IgnoreReadOnlyProperties/Fields loses - // Never = never ignore (always include), global IgnoreReadOnlyProperties/Fields loses - bool serializeReadOnlyProperty = ignoreCondition != null || (MemberType == MemberTypes.Property - ? !Options.IgnoreReadOnlyProperties - : !Options.IgnoreReadOnlyFields); - - // We serialize if there is a getter + not ignoring readonly properties. - CanSerialize = HasGetter && (HasSetter || serializeReadOnlyProperty || _shouldSerializeIsExplicitlySet); - - // We deserialize if there is a setter. - CanDeserialize = HasSetter; - } - else - { - if (HasGetter) - { - Debug.Assert(EffectiveConverter != null); - - CanSerialize = true; - - if (HasSetter) - { - CanDeserialize = true; - } - } - } - } - - internal void DetermineIgnoreCondition(JsonIgnoreCondition? ignoreCondition) - { - if (_shouldSerializeIsExplicitlySet) - { - Debug.Assert(ignoreCondition == null); -#pragma warning disable SYSLIB0020 // JsonSerializerOptions.IgnoreNullValues is obsolete - if (Options.IgnoreNullValues) -#pragma warning restore SYSLIB0020 - { - Debug.Assert(Options.DefaultIgnoreCondition == JsonIgnoreCondition.Never); - if (PropertyTypeCanBeNull) - { - IgnoreDefaultValuesOnRead = true; - } - } - - return; - } - - if (ignoreCondition != null) - { - // This is not true for CodeGen scenarios since we do not cache this as of yet. - // Debug.Assert(MemberInfo != null); - Debug.Assert(ignoreCondition != JsonIgnoreCondition.Always); - - if (ignoreCondition == JsonIgnoreCondition.WhenWritingDefault) - { - IgnoreDefaultValuesOnWrite = true; - } - else if (ignoreCondition == JsonIgnoreCondition.WhenWritingNull) - { - if (PropertyTypeCanBeNull) - { - IgnoreDefaultValuesOnWrite = true; - } - else - { - ThrowHelper.ThrowInvalidOperationException_IgnoreConditionOnValueTypeInvalid(MemberName!, DeclaringType); - } - } - } -#pragma warning disable SYSLIB0020 // JsonSerializerOptions.IgnoreNullValues is obsolete - else if (Options.IgnoreNullValues) - { - Debug.Assert(Options.DefaultIgnoreCondition == JsonIgnoreCondition.Never); - if (PropertyTypeCanBeNull) - { - IgnoreDefaultValuesOnRead = true; - IgnoreDefaultValuesOnWrite = true; - } - } - else if (Options.DefaultIgnoreCondition == JsonIgnoreCondition.WhenWritingNull) - { - Debug.Assert(!Options.IgnoreNullValues); - if (PropertyTypeCanBeNull) - { - IgnoreDefaultValuesOnWrite = true; - } - } - else if (Options.DefaultIgnoreCondition == JsonIgnoreCondition.WhenWritingDefault) - { - Debug.Assert(!Options.IgnoreNullValues); - IgnoreDefaultValuesOnWrite = true; - } -#pragma warning restore SYSLIB0020 - } - internal void DetermineNumberHandlingForTypeInfo() { if (DeclaringTypeNumberHandling != null && DeclaringTypeNumberHandling != JsonNumberHandling.Strict && !EffectiveConverter.IsInternalConverter) @@ -524,8 +414,8 @@ internal string GetDebugInfo(int indent = 0) sb.AppendLine($"{ind} NameAsUtf8.Length: {(NameAsUtf8Bytes?.Length ?? -1)},"); sb.AppendLine($"{ind} IsConfigured: {_isConfigured},"); sb.AppendLine($"{ind} IsIgnored: {IsIgnored},"); - sb.AppendLine($"{ind} CanSerialize: {CanSerialize},"); - sb.AppendLine($"{ind} CanDeserialize: {CanDeserialize},"); + sb.AppendLine($"{ind} HasGetter: {HasGetter},"); + sb.AppendLine($"{ind} HasSetter: {HasSetter},"); sb.AppendLine($"{ind}}}"); return sb.ToString(); @@ -535,7 +425,7 @@ internal string GetDebugInfo(int indent = 0) internal bool HasGetter => _untypedGet is not null; internal bool HasSetter => _untypedSet is not null; - internal void InitializeUsingMemberReflection(MemberInfo memberInfo) + internal void InitializeUsingMemberReflection(MemberInfo memberInfo, JsonConverter? customConverter, JsonIgnoreCondition? ignoreCondition) { Debug.Assert(AttributeProvider == null); @@ -559,19 +449,37 @@ internal void InitializeUsingMemberReflection(MemberInfo memberInfo) break; } + CustomConverter = customConverter; DeterminePoliciesFromMember(memberInfo); DeterminePropertyNameFromMember(memberInfo); - if (!IsIgnored) + if (ignoreCondition != JsonIgnoreCondition.Always) { DetermineMemberAccessors(memberInfo); } + // NB setting the ignore condition must follow converter & getter/setter + // configuration in order for access policies to be applied correctly. + IgnoreCondition = ignoreCondition; IsExtensionData = memberInfo.GetCustomAttribute(inherit: false) != null; } - internal bool IgnoreDefaultValuesOnRead { get; private set; } - internal bool IgnoreDefaultValuesOnWrite { get; private set; } + internal bool IgnoreNullTokensOnRead { get; private protected set; } + internal bool IgnoreDefaultValuesOnWrite { get; private protected set; } + + internal bool IgnoreReadOnlyMember + { + get + { + Debug.Assert(MemberType == MemberTypes.Property || MemberType == MemberTypes.Field || MemberType == default); + return MemberType switch + { + MemberTypes.Property => Options.IgnoreReadOnlyProperties, + MemberTypes.Field => Options.IgnoreReadOnlyFields, + _ => false, + }; + } + } /// /// True if the corresponding cref="JsonTypeInfo.PropertyInfoForTypeInfo"/> is this instance. @@ -773,10 +681,6 @@ internal JsonTypeInfo JsonTypeInfo internal abstract void SetExtensionDictionaryAsObject(object obj, object? extensionDict); - internal bool CanSerialize { get; private set; } - - internal bool CanDeserialize { get; private set; } - internal bool IsIgnored => _ignoreCondition == JsonIgnoreCondition.Always; /// diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs index faa58bd81fc1e0..d9bf8e644352e4 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs @@ -37,8 +37,7 @@ internal JsonPropertyInfo(Type declaringType, JsonTypeInfo? declaringTypeInfo, J private protected override void SetGetter(Delegate? getter) { Debug.Assert(getter is null or Func or Func); - - VerifyMutable(); + Debug.Assert(!IsConfigured); if (getter is null) { @@ -61,8 +60,7 @@ private protected override void SetGetter(Delegate? getter) private protected override void SetSetter(Delegate? setter) { Debug.Assert(setter is null or Action or Action); - - VerifyMutable(); + Debug.Assert(!IsConfigured); if (setter is null) { @@ -82,6 +80,37 @@ private protected override void SetSetter(Delegate? setter) } } + internal new Func? ShouldSerialize + { + get => _shouldSerializeTyped; + set => SetShouldSerialize(value); + } + + private Func? _shouldSerializeTyped; + + private protected override void SetShouldSerialize(Delegate? predicate) + { + Debug.Assert(predicate is null or Func or Func); + Debug.Assert(!IsConfigured); + + if (predicate is null) + { + _shouldSerializeTyped = null; + _shouldSerialize = null; + } + else if (predicate is Func typedPredicate) + { + _shouldSerializeTyped = typedPredicate; + _shouldSerialize = typedPredicate is Func untypedPredicate ? untypedPredicate : (obj, value) => typedPredicate(obj, (T)value!); + } + else + { + Func untypedPredicate = (Func)predicate; + _shouldSerializeTyped = (obj, value) => untypedPredicate(obj, value); + _shouldSerialize = untypedPredicate; + } + } + internal override object? DefaultValue => default(T); internal override bool PropertyTypeCanBeNull => default(T) is null; @@ -94,43 +123,37 @@ private protected override void DetermineMemberAccessors(MemberInfo memberInfo) switch (memberInfo) { case PropertyInfo propertyInfo: - { - bool useNonPublicAccessors = propertyInfo.GetCustomAttribute(inherit: false) != null; - - MethodInfo? getMethod = propertyInfo.GetMethod; - if (getMethod != null && (getMethod.IsPublic || useNonPublicAccessors)) - { - Get = Options.MemberAccessorStrategy.CreatePropertyGetter(propertyInfo); - } + bool useNonPublicAccessors = propertyInfo.GetCustomAttribute(inherit: false) != null; - MethodInfo? setMethod = propertyInfo.SetMethod; - if (setMethod != null && (setMethod.IsPublic || useNonPublicAccessors)) - { - Set = Options.MemberAccessorStrategy.CreatePropertySetter(propertyInfo); - } - - break; + MethodInfo? getMethod = propertyInfo.GetMethod; + if (getMethod != null && (getMethod.IsPublic || useNonPublicAccessors)) + { + Get = Options.MemberAccessorStrategy.CreatePropertyGetter(propertyInfo); } - case FieldInfo fieldInfo: + MethodInfo? setMethod = propertyInfo.SetMethod; + if (setMethod != null && (setMethod.IsPublic || useNonPublicAccessors)) { - Debug.Assert(fieldInfo.IsPublic); + Set = Options.MemberAccessorStrategy.CreatePropertySetter(propertyInfo); + } - Get = Options.MemberAccessorStrategy.CreateFieldGetter(fieldInfo); + break; - if (!fieldInfo.IsInitOnly) - { - Set = Options.MemberAccessorStrategy.CreateFieldSetter(fieldInfo); - } + case FieldInfo fieldInfo: + Debug.Assert(fieldInfo.IsPublic); - break; - } + Get = Options.MemberAccessorStrategy.CreateFieldGetter(fieldInfo); - default: + if (!fieldInfo.IsInitOnly) { - Debug.Fail($"Invalid MemberInfo type: {memberInfo.MemberType}"); - break; + Set = Options.MemberAccessorStrategy.CreateFieldSetter(fieldInfo); } + + break; + + default: + Debug.Fail($"Invalid MemberInfo type: {memberInfo.MemberType}"); + break; } } @@ -168,20 +191,21 @@ internal JsonPropertyInfo(JsonPropertyInfoValues propertyInfo, JsonSerializer DefaultConverterForType = propertyInfo.PropertyTypeInfo?.Converter as JsonConverter; CustomConverter = propertyInfo.Converter; - IgnoreCondition = propertyInfo.IgnoreCondition; - if (IgnoreCondition != JsonIgnoreCondition.Always) + if (propertyInfo.IgnoreCondition != JsonIgnoreCondition.Always) { Get = propertyInfo.Getter!; Set = propertyInfo.Setter; } + // NB setting the ignore condition must follow converter & getter/setter + // configuration in order for access policies to be applied correctly. + IgnoreCondition = propertyInfo.IgnoreCondition; JsonTypeInfo = propertyInfo.PropertyTypeInfo; NumberHandling = propertyInfo.NumberHandling; } - private protected override void DetermineEffectiveConverter() + private protected override void DetermineEffectiveConverter(JsonConverter? customConverter) { - JsonConverter? customConverter = CustomConverter; if (customConverter != null) { customConverter = Options.ExpandConverterFactory(customConverter, PropertyType); @@ -229,23 +253,13 @@ value is not null && if (IgnoreDefaultValuesOnWrite) { - if (default(T) is null) + // Fast path `ShouldSerialize` check when using JsonIgnoreCondition.WhenWritingNull/Default configuration + if (IsDefaultValue(value)) { - if (value is null) - { - return true; - } - } - else - { - if (EqualityComparer.Default.Equals(default, value)) - { - return true; - } + return true; } } - - if (ShouldSerialize?.Invoke(obj, value) == false) + else if (ShouldSerialize?.Invoke(obj, value) == false) { // We return true here. // False means that there is not enough data. @@ -326,7 +340,7 @@ internal override bool ReadJsonAndSetMember(object obj, ref ReadStack state, ref ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(TypedEffectiveConverter.TypeToConvert); } - if (!IgnoreDefaultValuesOnRead) + if (!IgnoreNullTokensOnRead) { T? value = default; Set!(obj, value!); @@ -339,7 +353,7 @@ internal override bool ReadJsonAndSetMember(object obj, ref ReadStack state, ref // CanUseDirectReadOrWrite == false when using streams Debug.Assert(!state.IsContinuation); - if (!isNullToken || !IgnoreDefaultValuesOnRead || default(T) is not null) + if (!isNullToken || !IgnoreNullTokensOnRead || default(T) is not null) { // Optimize for internal converters by avoiding the extra call to TryRead. T? fastValue = TypedEffectiveConverter.Read(ref reader, PropertyType, Options); @@ -351,7 +365,7 @@ internal override bool ReadJsonAndSetMember(object obj, ref ReadStack state, ref else { success = true; - if (!isNullToken || !IgnoreDefaultValuesOnRead || default(T) is not null || state.IsContinuation) + if (!isNullToken || !IgnoreNullTokensOnRead || default(T) is not null || state.IsContinuation) { success = TypedEffectiveConverter.TryRead(ref reader, PropertyType, Options, ref state, out T? value); if (success) @@ -406,40 +420,115 @@ internal override void SetExtensionDictionaryAsObject(object obj, object? extens Set!(obj, typedValue); } - private protected override Func GetShouldSerializeForIgnoreCondition(JsonIgnoreCondition ignoreCondition) + private protected override void ConfigureIgnoreCondition(JsonIgnoreCondition? ignoreCondition) { + DetermineSerializationCapabilities(ignoreCondition); + switch (ignoreCondition) { - case JsonIgnoreCondition.Always: return ShouldSerializeIgnoreConditionAlways; - case JsonIgnoreCondition.Never: return ShouldSerializeIgnoreConditionNever; + case JsonIgnoreCondition.Never: + break; + + case JsonIgnoreCondition.Always: + ShouldSerialize = ShouldSerializeIgnoreConditionAlways; + break; + case JsonIgnoreCondition.WhenWritingNull: - if (!PropertyTypeCanBeNull) + if (PropertyTypeCanBeNull) + { + ShouldSerialize = ShouldSerializeIgnoreWhenWritingDefault; + IgnoreDefaultValuesOnWrite = true; + } + else { - return ShouldSerializeIgnoreConditionNever; + ThrowHelper.ThrowInvalidOperationException_IgnoreConditionOnValueTypeInvalid(MemberName!, DeclaringType); } + break; - goto case JsonIgnoreCondition.WhenWritingDefault; case JsonIgnoreCondition.WhenWritingDefault: + ShouldSerialize = ShouldSerializeIgnoreWhenWritingDefault; + IgnoreDefaultValuesOnWrite = true; + break; + + case not null: + Debug.Fail($"Unknown value of JsonIgnoreCondition '{ignoreCondition}'"); + break; + + case null: +#pragma warning disable SYSLIB0020 // JsonSerializerOptions.IgnoreNullValues is obsolete + if (Options.IgnoreNullValues) +#pragma warning restore SYSLIB0020 { - return ShouldSerializeIgnoreConditionWhenWritingDefaultPropertyTypeEqualsTypeToConvert; + Debug.Assert(Options.DefaultIgnoreCondition == JsonIgnoreCondition.Never); + if (PropertyTypeCanBeNull) + { + ShouldSerialize = ShouldSerializeIgnoreWhenWritingDefault; + IgnoreDefaultValuesOnWrite = true; + IgnoreNullTokensOnRead = true; + } } - default: - Debug.Fail($"Unknown value of JsonIgnoreCondition '{ignoreCondition}'"); - return null!; + else if (Options.DefaultIgnoreCondition == JsonIgnoreCondition.WhenWritingNull) + { + if (PropertyTypeCanBeNull) + { + ShouldSerialize = ShouldSerializeIgnoreWhenWritingDefault; + IgnoreDefaultValuesOnWrite = true; + } + } + else if (Options.DefaultIgnoreCondition == JsonIgnoreCondition.WhenWritingDefault) + { + ShouldSerialize = ShouldSerializeIgnoreWhenWritingDefault; + IgnoreDefaultValuesOnWrite = true; + } + + break; + } + + static bool ShouldSerializeIgnoreConditionAlways(object _, T? value) => false; + static bool ShouldSerializeIgnoreWhenWritingDefault(object _, T? value) + { + return default(T) is null ? value is not null : !EqualityComparer.Default.Equals(default, value); } } - private static bool ShouldSerializeIgnoreConditionAlways(object obj, object? value) => false; - private static bool ShouldSerializeIgnoreConditionNever(object obj, object? value) => true; - private static bool ShouldSerializeIgnoreConditionWhenWritingDefaultPropertyTypeEqualsTypeToConvert(object obj, object? value) + private static bool IsDefaultValue(T? value) { - if (value == null) + return default(T) is null ? value is null : EqualityComparer.Default.Equals(default, value); + } + + private void DetermineSerializationCapabilities(JsonIgnoreCondition? ignoreCondition) + { + if (ignoreCondition == JsonIgnoreCondition.Always) { - return false; + Debug.Assert(Get == null && Set == null, "Getters or Setters should not be set when JsonIgnoreCondition.Always is specified."); + return; } - T typedValue = (T)value; - return !EqualityComparer.Default.Equals(default, typedValue); + Debug.Assert(TypedEffectiveConverter != null, "Must have calculated the effective converter strategy."); + if ((ConverterStrategy & (ConverterStrategy.Enumerable | ConverterStrategy.Dictionary)) == 0) + { + // We serialize if there is a getter + not ignoring readonly properties. + if (Get != null && Set == null) + { + // Three possible values for ignoreCondition: + // null = JsonIgnore was not placed on this property, global IgnoreReadOnlyProperties/Fields wins + // WhenNull = only ignore when null, global IgnoreReadOnlyProperties/Fields loses + // Never = never ignore (always include), global IgnoreReadOnlyProperties/Fields loses + bool serializeReadOnlyProperty = ignoreCondition != null || !IgnoreReadOnlyMember; + if (!serializeReadOnlyProperty) + { + Get = null; + } + } + } + else + { + if (Get == null && Set != null) + { + // Collection properties with setters only are not supported. + Set = null; + } + } } } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs index a0efc0629a26d4..f7b91f3127fc15 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs @@ -209,9 +209,7 @@ private void CacheMember( } JsonPropertyInfo jsonPropertyInfo = CreatePropertyUsingReflection(typeToConvert, converter); - jsonPropertyInfo.IgnoreCondition = ignoreCondition; - jsonPropertyInfo.CustomConverter = customConverter; - jsonPropertyInfo.InitializeUsingMemberReflection(memberInfo); + jsonPropertyInfo.InitializeUsingMemberReflection(memberInfo, customConverter, ignoreCondition); return jsonPropertyInfo; } diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs index 3642e9459f6571..fdc97d735ef0a0 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs @@ -810,7 +810,7 @@ private class TestClassWithNumberAndIgnoreConditionOnProperty } [Fact] - public static void DefaultIgnoreConditionFromOptionsDoesntFlowToShouldSerializePropertyAndOverrideIsRespected() + public static void DefaultIgnoreConditionFromOptionsFlowsToShouldSerializePropertyAndOverrideIsRespected() { TestClassWithNumber obj = new() { @@ -823,7 +823,8 @@ public static void DefaultIgnoreConditionFromOptionsDoesntFlowToShouldSerializeP if (ti.Type == typeof(TestClassWithNumber)) { Assert.Equal(1, ti.Properties.Count); - Assert.Null(ti.Properties[0].ShouldSerialize); + Assert.NotNull(ti.Properties[0].ShouldSerialize); + Assert.False(ti.Properties[0].ShouldSerialize(obj, 0)); ti.Properties[0].ShouldSerialize = (o, val) => { Assert.Same(obj, o); @@ -1019,41 +1020,83 @@ public static void JsonIgnoreConditionIsCorrectlyTranslatedToShouldSerializeDele static void TestJsonIgnoreConditionDelegate(JsonIgnoreCondition defaultIgnoreCondition, JsonIgnoreCondition? ignoreConditionOnProperty, JsonPropertyInfo property, ModifyJsonIgnore modify) { - // defaultIgnoreCondition is not taken into accound, we might expect null if defaultIgnoreCondition == ignoreConditionOnProperty switch (ignoreConditionOnProperty) { - case null: - Assert.Null(property.ShouldSerialize); - break; case JsonIgnoreCondition.Always: Assert.NotNull(property.ShouldSerialize); Assert.False(property.ShouldSerialize(null, null)); Assert.False(property.ShouldSerialize(null, "")); Assert.False(property.ShouldSerialize(null, "asd")); + Assert.Throws(() => property.ShouldSerialize(null, 0)); Assert.Null(property.Get); Assert.Null(property.Set); break; case JsonIgnoreCondition.WhenWritingDefault: - Assert.NotNull(property.ShouldSerialize); - Assert.False(property.ShouldSerialize(null, 0)); - Assert.True(property.ShouldSerialize(null, 1)); - Assert.True(property.ShouldSerialize(null, -1)); + TestIntIgnoreWhenWritingDefault(); break; case JsonIgnoreCondition.WhenWritingNull: - Assert.NotNull(property.ShouldSerialize); - Assert.False(property.ShouldSerialize(null, null)); - Assert.True(property.ShouldSerialize(null, "")); - Assert.True(property.ShouldSerialize(null, "asd")); + TestStringIgnoreWhenWritingDefault(); break; case JsonIgnoreCondition.Never: - Assert.NotNull(property.ShouldSerialize); - Assert.True(property.ShouldSerialize(null, null)); - Assert.True(property.ShouldSerialize(null, "")); - Assert.True(property.ShouldSerialize(null, "asd")); + Assert.Null(property.ShouldSerialize); break; + + case null: + switch (defaultIgnoreCondition) + { + case JsonIgnoreCondition.WhenWritingDefault: + if (property.PropertyType == typeof(int)) + { + TestIntIgnoreWhenWritingDefault(); + } + else + { + Assert.Equal(typeof(string), property.PropertyType); + TestStringIgnoreWhenWritingDefault(); + } + break; + + case JsonIgnoreCondition.WhenWritingNull: + if (property.PropertyType == typeof(int)) + { + Assert.Null(property.ShouldSerialize); + } + else + { + Assert.Equal(typeof(string), property.PropertyType); + TestStringIgnoreWhenWritingDefault(); + } + break; + + default: + Assert.Null(property.ShouldSerialize); + break; + } + + break; + } + + void TestIntIgnoreWhenWritingDefault() + { + Assert.NotNull(property.ShouldSerialize); + Assert.False(property.ShouldSerialize(null, 0)); + Assert.True(property.ShouldSerialize(null, 1)); + Assert.True(property.ShouldSerialize(null, -1)); + Assert.Throws(() => property.ShouldSerialize(null, null)); + Assert.Throws(() => property.ShouldSerialize(null, "string")); } + void TestStringIgnoreWhenWritingDefault() + { + Assert.NotNull(property.ShouldSerialize); + Assert.False(property.ShouldSerialize(null, null)); + Assert.True(property.ShouldSerialize(null, "")); + Assert.True(property.ShouldSerialize(null, "asd")); + Assert.Throws(() => property.ShouldSerialize(null, 0)); + } + + if (modify != ModifyJsonIgnore.DontModify && ignoreConditionOnProperty == JsonIgnoreCondition.Always) { property.Get = (o) => ((TestClassWithEveryPossibleJsonIgnore)o).AlwaysProperty; @@ -1444,5 +1487,245 @@ public class ClassWithFieldsAndProperties public string Field; public int Property { get; set; } } + + [Fact] + public static void CanEnableDeserializationOnAlwaysIgnoreProperty() + { + var options = new JsonSerializerOptions + { + TypeInfoResolver = new DefaultJsonTypeInfoResolver + { + Modifiers = + { + jsonTypeInfo => + { + if (jsonTypeInfo.Type != typeof(ClassWithJsonIgnoreAlwaysProperty)) + { + return; + } + + Assert.Equal(1, jsonTypeInfo.Properties.Count); + JsonPropertyInfo jsonPropertyInfo = jsonTypeInfo.Properties[0]; + + Assert.NotNull(jsonPropertyInfo.ShouldSerialize); + Assert.Null(jsonPropertyInfo.Get); + Assert.Null(jsonPropertyInfo.Set); + + jsonPropertyInfo.Set = (obj, value) => ((ClassWithJsonIgnoreAlwaysProperty)obj).Value = (int)value; + } + } + } + }; + + ClassWithJsonIgnoreAlwaysProperty value = JsonSerializer.Deserialize("""{"Value":42}""", options); + Assert.Equal(42, value.Value); + } + + public class ClassWithJsonIgnoreAlwaysProperty + { + [JsonIgnore] + public int Value { get; set; } + } + + [Theory] + [MemberData(nameof(ClassWithSetterOnlyCollectionProperty_SupportedWhenSetManually_MemberData))] + public static void ClassWithSetterOnlyCollectionProperty_SupportedWhenSetManually(T value, string json) + { + _ = value; // parameter not used + + string fullJson = $@"{{""Value"":{json}}}"; + + // sanity check: deserialization is not supported using the default contract + ClassWithSetterOnlyProperty result = JsonSerializer.Deserialize>(fullJson); + Assert.Null(result.GetValue()); + + var options = new JsonSerializerOptions + { + TypeInfoResolver = new DefaultJsonTypeInfoResolver + { + Modifiers = + { + jsonTypeInfo => + { + if (jsonTypeInfo.Type != typeof(ClassWithSetterOnlyProperty)) + { + return; + } + + Assert.Equal(1, jsonTypeInfo.Properties.Count); + + JsonPropertyInfo jpi = jsonTypeInfo.Properties[0]; + Assert.Null(jpi.Set); + jpi.Set = (obj, value) => ((ClassWithSetterOnlyProperty)obj)._value = (T)value; + } + } + } + }; + + result = JsonSerializer.Deserialize>(fullJson, options); + Assert.NotNull(result.GetValue()); + } + + public static IEnumerable ClassWithSetterOnlyCollectionProperty_SupportedWhenSetManually_MemberData() + { + yield return Wrap(new List(), "[1,2,3]"); + yield return Wrap(new Dictionary(), """{"key":"value"}"""); + + static object[] Wrap(T value, string json) => new object[] { value, json }; + } + + public class ClassWithSetterOnlyProperty + { + internal T _value; + public T Value { set => _value = value; } + public T GetValue() => _value; + } + + [Fact] + public static void ClassWithReadOnlyMembers_SupportedWhenSetManually() + { + var value = new ClassWithReadOnlyMembers(); + + // Sanity check -- default contract does not support serialization + var options = new JsonSerializerOptions { IgnoreReadOnlyFields = true, IgnoreReadOnlyProperties = true }; + string json = JsonSerializer.Serialize(value, options); + Assert.Equal("{}", json); + + // Use a constract resolver that updates getters manually + options = new JsonSerializerOptions + { + IgnoreReadOnlyFields = true, + IgnoreReadOnlyProperties = true, + + TypeInfoResolver = new DefaultJsonTypeInfoResolver + { + Modifiers = + { + jsonTypeInfo => + { + if (jsonTypeInfo.Type != typeof(ClassWithReadOnlyMembers)) + { + return; + } + + Assert.Equal(2, jsonTypeInfo.Properties.Count); + + JsonPropertyInfo jpi = jsonTypeInfo.Properties[0]; + Assert.Equal("Field", jpi.Name); // JsonPropertyOrder should ensure correct ordering of properties. + Assert.Null(jpi.Get); + jpi.Get = obj => ((ClassWithReadOnlyMembers)obj).Field; + + jpi = jsonTypeInfo.Properties[1]; + Assert.Equal("Property", jpi.Name); // JsonPropertyOrder should ensure correct ordering of properties. + Assert.Null(jpi.Get); + jpi.Get = obj => ((ClassWithReadOnlyMembers)obj).Property; + } + } + } + }; + + json = JsonSerializer.Serialize(value, options); + Assert.Equal("""{"Field":1,"Property":2}""", json); + } + + public class ClassWithReadOnlyMembers + { + [JsonInclude, JsonPropertyOrder(0)] + public readonly int Field = 1; + [JsonPropertyOrder(1)] + public int Property => 2; + } + + [Fact] + public static void IgnoreNullValues_Serialization_CanBeInvalidatedByCustomContracts() + { + var value = new ClassWithNullableProperty(); + + // Sanity check -- property is ignored using default contract + var options = new JsonSerializerOptions { IgnoreNullValues = true }; + string json = JsonSerializer.Serialize(value, options); + Assert.Equal("{}", json); + + // Property can be re-enabled using a custom contract + options = new JsonSerializerOptions + { + IgnoreNullValues = true, + TypeInfoResolver = new DefaultJsonTypeInfoResolver + { + Modifiers = + { + jsonTypeInfo => + { + if (jsonTypeInfo.Type != typeof(ClassWithNullableProperty)) + { + return; + } + + Assert.Equal(1, jsonTypeInfo.Properties.Count); + + JsonPropertyInfo jpi = jsonTypeInfo.Properties[0]; + Assert.NotNull(jpi.ShouldSerialize); + Assert.False(jpi.ShouldSerialize(value, null)); + Assert.NotNull(jpi.Get); + + jpi.ShouldSerialize = null; + } + } + } + }; + + json = JsonSerializer.Serialize(value, options); + Assert.Equal("""{"Value":null}""", json); + } + + [Fact] + public static void IgnoreNullValues_Deserialization_CanBeInvalidatedByCustomContracts() + { + string json = """{"Value":null}"""; + + // Sanity check -- property is ignored using default contract + var options = new JsonSerializerOptions { IgnoreNullValues = true }; + ClassWithNullableProperty value = JsonSerializer.Deserialize(json, options); + Assert.Null(value.Value); + + // Property can be re-enabled using a custom contract + options = new JsonSerializerOptions + { + IgnoreNullValues = true, + TypeInfoResolver = new DefaultJsonTypeInfoResolver + { + Modifiers = + { + jsonTypeInfo => + { + if (jsonTypeInfo.Type != typeof(ClassWithNullableProperty)) + { + return; + } + + Assert.Equal(1, jsonTypeInfo.Properties.Count); + + JsonPropertyInfo jpi = jsonTypeInfo.Properties[0]; + Assert.NotNull(jpi.Set); + jpi.Set = (obj, value) => ((ClassWithNullableProperty)obj).Value = (string)value; + } + } + } + }; + + value = JsonSerializer.Deserialize(json, options); + Assert.Equal("NULL", value.Value); + } + + public class ClassWithNullableProperty + { + private string? _value; + + public string? Value + { + get => _value; + set => _value = value ?? "NULL"; + } + } } } From 4f3aea3c7a9113aac071080450259b3025916a74 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Mon, 11 Jul 2022 10:15:47 +0100 Subject: [PATCH 2/7] Update src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs --- .../DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs index fdc97d735ef0a0..a3d72d18945b05 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs @@ -1095,8 +1095,6 @@ void TestStringIgnoreWhenWritingDefault() Assert.True(property.ShouldSerialize(null, "asd")); Assert.Throws(() => property.ShouldSerialize(null, 0)); } - - if (modify != ModifyJsonIgnore.DontModify && ignoreConditionOnProperty == JsonIgnoreCondition.Always) { property.Get = (o) => ((TestClassWithEveryPossibleJsonIgnore)o).AlwaysProperty; From 83018d75997b0e54bb5f6772a877ef5ac5628309 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Mon, 11 Jul 2022 14:29:48 +0100 Subject: [PATCH 3/7] Revert to having `DefaultIgnoreCondition`/`IgnoreNullValues` settings being applied post-configuration. --- .../Metadata/JsonPropertyInfo.cs | 37 ++++++++++++- .../Metadata/JsonPropertyInfoOfT.cs | 34 ++---------- ...nTypeInfoResolverTests.JsonPropertyInfo.cs | 53 +++++-------------- 3 files changed, 52 insertions(+), 72 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs index 9a9d2a3359d2ac..8c98e0c90e139f 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs @@ -91,12 +91,13 @@ public JsonConverter? CustomConverter SetSetter(value); // Invalidate any JsonIgnore configuration if delegate set manually by user IgnoreNullTokensOnRead = false; - _ignoreCondition = null; + _isUserSpecifiedSetter = true; } } private protected Func? _untypedGet; private protected Action? _untypedSet; + private bool _isUserSpecifiedSetter; private protected abstract void SetGetter(Delegate? getter); private protected abstract void SetSetter(Delegate? setter); @@ -121,7 +122,6 @@ public JsonConverter? CustomConverter SetShouldSerialize(value); // Invalidate any JsonIgnore configuration if delegate set manually by user IgnoreDefaultValuesOnWrite = false; - _ignoreCondition = null; } } @@ -269,6 +269,7 @@ internal void Configure() else { DetermineNumberHandlingForProperty(); + DetermineIgnoreCondition(); } } @@ -317,6 +318,38 @@ internal void CacheNameAsUtf8BytesAndEscapedNameSection() EscapedNameSection = JsonHelpers.GetEscapedPropertyNameSection(NameAsUtf8Bytes, Options.Encoder); } + internal void DetermineIgnoreCondition() + { + if (_ignoreCondition != null) + { + // Do not apply global policy if already configured on the property level. + return; + } + +#pragma warning disable SYSLIB0020 // JsonSerializerOptions.IgnoreNullValues is obsolete + if (Options.IgnoreNullValues) +#pragma warning restore SYSLIB0020 + { + Debug.Assert(Options.DefaultIgnoreCondition == JsonIgnoreCondition.Never); + if (PropertyTypeCanBeNull) + { + IgnoreNullTokensOnRead = !_isUserSpecifiedSetter; + IgnoreDefaultValuesOnWrite = ShouldSerialize is null; + } + } + else if (Options.DefaultIgnoreCondition == JsonIgnoreCondition.WhenWritingNull) + { + if (PropertyTypeCanBeNull) + { + IgnoreDefaultValuesOnWrite = ShouldSerialize is null; + } + } + else if (Options.DefaultIgnoreCondition == JsonIgnoreCondition.WhenWritingDefault) + { + IgnoreDefaultValuesOnWrite = ShouldSerialize is null; + } + } + internal void DetermineNumberHandlingForTypeInfo() { if (DeclaringTypeNumberHandling != null && DeclaringTypeNumberHandling != JsonNumberHandling.Strict && !EffectiveConverter.IsInternalConverter) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs index d9bf8e644352e4..42d422fcefc996 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs @@ -426,6 +426,9 @@ private protected override void ConfigureIgnoreCondition(JsonIgnoreCondition? ig switch (ignoreCondition) { + case null: + break; + case JsonIgnoreCondition.Never: break; @@ -450,38 +453,9 @@ private protected override void ConfigureIgnoreCondition(JsonIgnoreCondition? ig IgnoreDefaultValuesOnWrite = true; break; - case not null: + default: Debug.Fail($"Unknown value of JsonIgnoreCondition '{ignoreCondition}'"); break; - - case null: -#pragma warning disable SYSLIB0020 // JsonSerializerOptions.IgnoreNullValues is obsolete - if (Options.IgnoreNullValues) -#pragma warning restore SYSLIB0020 - { - Debug.Assert(Options.DefaultIgnoreCondition == JsonIgnoreCondition.Never); - if (PropertyTypeCanBeNull) - { - ShouldSerialize = ShouldSerializeIgnoreWhenWritingDefault; - IgnoreDefaultValuesOnWrite = true; - IgnoreNullTokensOnRead = true; - } - } - else if (Options.DefaultIgnoreCondition == JsonIgnoreCondition.WhenWritingNull) - { - if (PropertyTypeCanBeNull) - { - ShouldSerialize = ShouldSerializeIgnoreWhenWritingDefault; - IgnoreDefaultValuesOnWrite = true; - } - } - else if (Options.DefaultIgnoreCondition == JsonIgnoreCondition.WhenWritingDefault) - { - ShouldSerialize = ShouldSerializeIgnoreWhenWritingDefault; - IgnoreDefaultValuesOnWrite = true; - } - - break; } static bool ShouldSerializeIgnoreConditionAlways(object _, T? value) => false; diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs index a3d72d18945b05..33b815b22f3129 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs @@ -810,7 +810,7 @@ private class TestClassWithNumberAndIgnoreConditionOnProperty } [Fact] - public static void DefaultIgnoreConditionFromOptionsFlowsToShouldSerializePropertyAndOverrideIsRespected() + public static void DefaultIgnoreConditionFromOptionsDoesntFlowToShouldSerializePropertyAndOverrideIsRespected() { TestClassWithNumber obj = new() { @@ -823,8 +823,7 @@ public static void DefaultIgnoreConditionFromOptionsFlowsToShouldSerializeProper if (ti.Type == typeof(TestClassWithNumber)) { Assert.Equal(1, ti.Properties.Count); - Assert.NotNull(ti.Properties[0].ShouldSerialize); - Assert.False(ti.Properties[0].ShouldSerialize(obj, 0)); + Assert.Null(ti.Properties[0].ShouldSerialize); ti.Properties[0].ShouldSerialize = (o, val) => { Assert.Same(obj, o); @@ -1020,8 +1019,13 @@ public static void JsonIgnoreConditionIsCorrectlyTranslatedToShouldSerializeDele static void TestJsonIgnoreConditionDelegate(JsonIgnoreCondition defaultIgnoreCondition, JsonIgnoreCondition? ignoreConditionOnProperty, JsonPropertyInfo property, ModifyJsonIgnore modify) { + // defaultIgnoreCondition is not taken into account, we might expect null if defaultIgnoreCondition == ignoreConditionOnProperty switch (ignoreConditionOnProperty) { + case null: + Assert.Null(property.ShouldSerialize); + break; + case JsonIgnoreCondition.Always: Assert.NotNull(property.ShouldSerialize); Assert.False(property.ShouldSerialize(null, null)); @@ -1041,40 +1045,6 @@ static void TestJsonIgnoreConditionDelegate(JsonIgnoreCondition defaultIgnoreCon case JsonIgnoreCondition.Never: Assert.Null(property.ShouldSerialize); break; - - case null: - switch (defaultIgnoreCondition) - { - case JsonIgnoreCondition.WhenWritingDefault: - if (property.PropertyType == typeof(int)) - { - TestIntIgnoreWhenWritingDefault(); - } - else - { - Assert.Equal(typeof(string), property.PropertyType); - TestStringIgnoreWhenWritingDefault(); - } - break; - - case JsonIgnoreCondition.WhenWritingNull: - if (property.PropertyType == typeof(int)) - { - Assert.Null(property.ShouldSerialize); - } - else - { - Assert.Equal(typeof(string), property.PropertyType); - TestStringIgnoreWhenWritingDefault(); - } - break; - - default: - Assert.Null(property.ShouldSerialize); - break; - } - - break; } void TestIntIgnoreWhenWritingDefault() @@ -1662,11 +1632,14 @@ public static void IgnoreNullValues_Serialization_CanBeInvalidatedByCustomContra Assert.Equal(1, jsonTypeInfo.Properties.Count); JsonPropertyInfo jpi = jsonTypeInfo.Properties[0]; - Assert.NotNull(jpi.ShouldSerialize); - Assert.False(jpi.ShouldSerialize(value, null)); + Assert.Null(jpi.ShouldSerialize); Assert.NotNull(jpi.Get); - jpi.ShouldSerialize = null; + // Because null `ShouldSerialize` gets overridden by + // `DefaultIgnoreCondition`/`IgnoreNullValues` post-configuration, + // we need to explicitly set a delegate that always returns true. + // This is a design quirk we might want to consider changing. + jpi.ShouldSerialize = (_,value) => true; } } } From 3422f74447e75019858008b58976a2833d3b8217 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Mon, 11 Jul 2022 15:00:57 +0100 Subject: [PATCH 4/7] Revert `JsonIgnoreCondition.Never` mapping --- .../Json/Serialization/Metadata/JsonPropertyInfoOfT.cs | 2 ++ .../DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs | 7 ++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs index 42d422fcefc996..ee275baa93d88f 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs @@ -430,6 +430,7 @@ private protected override void ConfigureIgnoreCondition(JsonIgnoreCondition? ig break; case JsonIgnoreCondition.Never: + ShouldSerialize = ShouldSerializeIgnoreConditionNever; break; case JsonIgnoreCondition.Always: @@ -458,6 +459,7 @@ private protected override void ConfigureIgnoreCondition(JsonIgnoreCondition? ig break; } + static bool ShouldSerializeIgnoreConditionNever(object _, T? value) => true; static bool ShouldSerializeIgnoreConditionAlways(object _, T? value) => false; static bool ShouldSerializeIgnoreWhenWritingDefault(object _, T? value) { diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs index 33b815b22f3129..5704d1fd66aa16 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs @@ -1043,7 +1043,11 @@ static void TestJsonIgnoreConditionDelegate(JsonIgnoreCondition defaultIgnoreCon TestStringIgnoreWhenWritingDefault(); break; case JsonIgnoreCondition.Never: - Assert.Null(property.ShouldSerialize); + Assert.NotNull(property.ShouldSerialize); + Assert.True(property.ShouldSerialize(null, null)); + Assert.True(property.ShouldSerialize(null, "")); + Assert.True(property.ShouldSerialize(null, "asd")); + Assert.Throws(() => property.ShouldSerialize(null, 0)); break; } @@ -1065,6 +1069,7 @@ void TestStringIgnoreWhenWritingDefault() Assert.True(property.ShouldSerialize(null, "asd")); Assert.Throws(() => property.ShouldSerialize(null, 0)); } + if (modify != ModifyJsonIgnore.DontModify && ignoreConditionOnProperty == JsonIgnoreCondition.Always) { property.Get = (o) => ((TestClassWithEveryPossibleJsonIgnore)o).AlwaysProperty; From 6979890d83dc2bf40bd72fecb8f365e28270ef23 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Mon, 11 Jul 2022 19:21:14 +0100 Subject: [PATCH 5/7] Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs --- .../Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs index ee275baa93d88f..3f9497b8f56535 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs @@ -101,7 +101,7 @@ private protected override void SetShouldSerialize(Delegate? predicate) else if (predicate is Func typedPredicate) { _shouldSerializeTyped = typedPredicate; - _shouldSerialize = typedPredicate is Func untypedPredicate ? untypedPredicate : (obj, value) => typedPredicate(obj, (T)value!); + _shouldSerialize = typedPredicate is Func untypedPredicate ? untypedPredicate : (obj, value) => typedPredicate(obj, (T?)value); } else { From 33299cf817e5256222ba725d1f3bd63c5a94dc3f Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Mon, 11 Jul 2022 19:24:37 +0100 Subject: [PATCH 6/7] Remove no longer needed test helpers --- ...nTypeInfoResolverTests.JsonPropertyInfo.cs | 32 +++++++------------ 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs index 5704d1fd66aa16..c45b2a2e217dd0 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs @@ -1037,10 +1037,19 @@ static void TestJsonIgnoreConditionDelegate(JsonIgnoreCondition defaultIgnoreCon Assert.Null(property.Set); break; case JsonIgnoreCondition.WhenWritingDefault: - TestIntIgnoreWhenWritingDefault(); + Assert.NotNull(property.ShouldSerialize); + Assert.False(property.ShouldSerialize(null, 0)); + Assert.True(property.ShouldSerialize(null, 1)); + Assert.True(property.ShouldSerialize(null, -1)); + Assert.Throws(() => property.ShouldSerialize(null, null)); + Assert.Throws(() => property.ShouldSerialize(null, "string")); break; case JsonIgnoreCondition.WhenWritingNull: - TestStringIgnoreWhenWritingDefault(); + Assert.NotNull(property.ShouldSerialize); + Assert.False(property.ShouldSerialize(null, null)); + Assert.True(property.ShouldSerialize(null, "")); + Assert.True(property.ShouldSerialize(null, "asd")); + Assert.Throws(() => property.ShouldSerialize(null, 0)); break; case JsonIgnoreCondition.Never: Assert.NotNull(property.ShouldSerialize); @@ -1051,25 +1060,6 @@ static void TestJsonIgnoreConditionDelegate(JsonIgnoreCondition defaultIgnoreCon break; } - void TestIntIgnoreWhenWritingDefault() - { - Assert.NotNull(property.ShouldSerialize); - Assert.False(property.ShouldSerialize(null, 0)); - Assert.True(property.ShouldSerialize(null, 1)); - Assert.True(property.ShouldSerialize(null, -1)); - Assert.Throws(() => property.ShouldSerialize(null, null)); - Assert.Throws(() => property.ShouldSerialize(null, "string")); - } - - void TestStringIgnoreWhenWritingDefault() - { - Assert.NotNull(property.ShouldSerialize); - Assert.False(property.ShouldSerialize(null, null)); - Assert.True(property.ShouldSerialize(null, "")); - Assert.True(property.ShouldSerialize(null, "asd")); - Assert.Throws(() => property.ShouldSerialize(null, 0)); - } - if (modify != ModifyJsonIgnore.DontModify && ignoreConditionOnProperty == JsonIgnoreCondition.Always) { property.Get = (o) => ((TestClassWithEveryPossibleJsonIgnore)o).AlwaysProperty; From 73028d3f88b04d39345afd08969f218dab991072 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Mon, 11 Jul 2022 19:33:57 +0100 Subject: [PATCH 7/7] Remove unnecessary invalidator --- .../System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs index 8c98e0c90e139f..04f78e1baffc80 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs @@ -89,8 +89,6 @@ public JsonConverter? CustomConverter { VerifyMutable(); SetSetter(value); - // Invalidate any JsonIgnore configuration if delegate set manually by user - IgnoreNullTokensOnRead = false; _isUserSpecifiedSetter = true; } }