-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix contract customization being silently ignored over pre-existing JsonIgnoreCondition configuration #71908
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
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsWhen populating This PR:
Fix #71886.
|
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs
Show resolved
Hide resolved
....Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs
Outdated
Show resolved
Hide resolved
....Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs
Show resolved
Hide resolved
....Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs
Show resolved
Hide resolved
....Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs
Outdated
Show resolved
Hide resolved
....Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs
Outdated
Show resolved
Hide resolved
....Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs
Outdated
Show resolved
Hide resolved
krwq
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect ShouldSerialize=null bring default behavior back and not always serialize
See rationale in #71908 (comment). I think making this change is fine. |
....Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs
Outdated
Show resolved
Hide resolved
…rialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs
|
Here is the scenario which will be really weird if we go with (btw this is currently also broken because of unrelated issue: DetermineIgnoreCondition early returns when [Fact]
public static void ChangingShouldSerializeToNullUsesIgnoreConditionSpecifiedInOptions()
{
SomeService service = new();
service.WithConfiguredJsonSerializerOptions((options) =>
{
options.DefaultIgnoreCondition = /* read from config file, for simplicity: */ JsonIgnoreCondition.WhenWritingDefault;
if (options.TypeInfoResolver is DefaultJsonTypeInfoResolver resolver)
{
resolver.Modifiers.Add(RestoreIgnoreConditionForLongToDefault);
}
else
{
throw new Exception("resolver is not DefaultJsonTypeInfoResolver");
}
});
ClassWithUlong obj = new();
obj.UlongValue = 0;
Assert.Equal("""{}""", service.SerializeObject(obj));
obj.UlongValue = 44;
Assert.Equal("""{"UlongValue":"44"}""", service.SerializeObject(obj));
obj.UlongValue = ulong.MaxValue;
Assert.Equal($"{{\"UlongValue\":\"{ulong.MaxValue}\"}}", service.SerializeObject(obj));
static void RestoreIgnoreConditionForLongToDefault(JsonTypeInfo typeInfo)
{
foreach (JsonPropertyInfo prop in typeInfo.Properties)
{
prop.ShouldSerialize = null;
// oterwise
//switch (typeInfo.Options.DefaultIgnoreCondition)
//{
// case JsonIgnoreCondition.WhenWritingDefault:
// // ... user has to do mapping themselves
//}
}
}
}
class ClassWithUlong
{
public ulong UlongValue { get; set; }
}
class SomeService
{
private JsonSerializerOptions _options = new()
{
TypeInfoResolver = new DefaultJsonTypeInfoResolver() { Modifiers = { LongAndULongModifier } }
};
public SomeService WithConfiguredJsonSerializerOptions(Action<JsonSerializerOptions> configureOptions)
{
configureOptions(_options);
return this;
}
public string SerializeObject<T>(T obj)
{
return JsonSerializer.Serialize(obj, _options);
}
static void LongAndULongModifier(JsonTypeInfo typeInfo)
{
foreach (JsonPropertyInfo propertyInfo in typeInfo.Properties)
{
if (propertyInfo.PropertyType == typeof(long) || propertyInfo.PropertyType == typeof(ulong))
{
propertyInfo.ShouldSerialize = (obj, val) =>
{
if (val is long longVal)
{
return longVal != long.MaxValue;
}
else if (val is ulong ulongVal)
{
return ulongVal != ulong.MaxValue;
}
else
{
throw new Exception("something is wrong");
}
};
}
}
// some other unrelated changes here
if (typeInfo.Type == typeof(long) || typeInfo.Type == typeof(ulong))
{
typeInfo.NumberHandling = JsonNumberHandling.AllowReadingFromString | JsonNumberHandling.WriteAsString;
}
}
} |
As you're pointing out in the example, the workaround is to explicitly add a mapping from the options instance at hand. Nothing inconsistent about that, different conventions require different handling. As a counterpoint, one problem I see with enforcing |
… being applied post-configuration.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs
Show resolved
Hide resolved
....Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs
Outdated
Show resolved
Hide resolved
|
@krwq I believe I have addressed all your feedback and concerns. Could you take another look please? |
...ibraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs
Outdated
Show resolved
Hide resolved
...ibraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs
Outdated
Show resolved
Hide resolved
…ion/Metadata/JsonPropertyInfoOfT.cs
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs
Show resolved
Hide resolved
...ibraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs
Show resolved
Hide resolved
...ibraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Failures in "runtime (Libraries Test Run release coreclr windows x64 Debug)" appear to be: |
When populating
JsonPropertyInfometadata, the default System.Text.Json contract resolvers will consult the value of theJsonIgnoreConditionsetting to determine the value of theGet,SetandShouldSerializeproperties in the property metadata. While these values are mapped correctly when surfaced to the user, theJsonPropertyInfoimplementation will also store a copy of the originalJsonIgnoreConditionsetting and consult it further during the final configuration stage of the metadata. In certain cases, this can result in user configuration being silently discarded (see #71886 for an example).This PR:
Get,SetandShouldSerializedelegates become the sole source of truth when determining the ignore policy. Any additional internal state used as performance optimization will get invalidated once the user applies relevant custom delegates.Configurephase. This includes consultation ofJsonSerializerOptions.DefaultIgnoreConditionandIgnoreNullValuesettings which can now be unset by a custom contract on a type-by-type basis. For completeness, theJsonSerializerOptions.DefaultIgnoreConditionandIgnoreNullValuesettings will now also map toShouldSerialize.ShouldSerializedelegate implementation strongly typed in the style ofGetandSet. This should prevent some unnecessary boxing when serializing properties with value types.JsonIgnoreCondition.Nevernow maps to anullvalue forShouldSerialize, giving it an equivalent representation to a null ignore condition.Fix #71886.