Fix contract customization being silently ignored over pre-existing JsonIgnoreCondition configuration

[eiriktsarpalis]

When populating JsonPropertyInfo metadata, the default System.Text.Json contract resolvers will consult the value of the JsonIgnoreCondition setting to determine the value of the Get, Set and ShouldSerialize properties in the property metadata. While these values are mapped correctly when surfaced to the user, the JsonPropertyInfo implementation will also store a copy of the original JsonIgnoreCondition setting 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:

1. Changes the implementation so that Get, Set and ShouldSerialize delegates 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.
2. Moves the ignore policy resolution logic to the metadata creation phase, and away from the final Configure phase. This includes consultation of JsonSerializerOptions.DefaultIgnoreCondition and IgnoreNullValue settings which can now be unset by a custom contract on a type-by-type basis. For completeness, the JsonSerializerOptions.DefaultIgnoreCondition and IgnoreNullValue settings will now also map to ShouldSerialize.
3. Makes the ShouldSerialize delegate implementation strongly typed in the style of Get and Set. This should prevent some unnecessary boxing when serializing properties with value types.
4. JsonIgnoreCondition.Never now maps to a null value for ShouldSerialize, giving it an equivalent representation to a null ignore condition.

Fix #71886.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

---

When populating JsonPropertyInfo metadata, the default System.Text.Json contract resolvers will consult the value of the JsonIgnoreCondition setting to determine the value of the Get, Set and ShouldSerialize properties in the property metadata. While these values are mapped correctly when surfaced to the user, the JsonPropertyInfo implementation will also store a copy of the original JsonIgnoreCondition setting 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:

1. Changes the implementation so that Get, Set and ShouldSerialize delegates 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.
2. Moves the ignore policy resolution logic to the metadata creation phase, and away from the final Configure phase. This includes consultation of JsonSerializerOptions.DefaultIgnoreCondition and IgnoreNullValue settings which can now be unset by a custom contract on a type-by-type basis. For completeness, the JsonSerializerOptions.DefaultIgnoreCondition and IgnoreNullValue settings will now also map to ShouldSerialize.
3. Makes the ShouldSerialize delegate implementation strongly typed in the style of Get and Set. This should prevent some unnecessary boxing when serializing properties with value types.
4. JsonIgnoreCondition.Never now maps to a null value for ShouldSerialize, giving it an equivalent representation to a null ignore condition.

Fix #71886.

Author:	eiriktsarpalis
Assignees:	eiriktsarpalis
Labels:	area-System.Text.Json
Milestone:	-

[krwq]

I'd expect ShouldSerialize=null bring default behavior back and not always serialize

[eiriktsarpalis]

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.

[krwq]

Here is the scenario which will be really weird if we go with null meaning something else than default:

(btw this is currently also broken because of unrelated issue: DetermineIgnoreCondition early returns when _shouldSerializeIsExplicitlySet which we should add extra null check to fix)

        [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;
                }
            }
        }

[eiriktsarpalis]

Here is the scenario which will be really weird if we go with null meaning something else than default:

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 DefaultIgnoreCondition on null ShouldSerialize values is that from a contract resolver's perspective it no longer becomes clear whether null values will get ignored or not. They can query all the JsonSerializerOptions flags to make that determination but that would involve correctly replicating the logic we currently have in DetermineIgnoreCondition. A recommendation to just apply a predicate always returning true to ensure that a null ShouldSerialize predicate remains null seems counterintuitive and counterproductive since you're now forcing redundant delegate invocations.

[eiriktsarpalis]

@krwq I believe I have addressed all your feedback and concerns. Could you take another look please?

[krwq]

LGTM

[jeffhandley]

Failures in "runtime (Libraries Test Run release coreclr windows x64 Debug)" appear to be:

• Test failure: System.Net.Http.Functional.Tests - Assertion failed: ClientCertificates. Expected enumerable cloned value. #71233
• Multiple calls of QuicConnection.CloseAsync lead to Debug.Assert #56851