Address edge scenarios with JsonSerializer's property visibility

[layomia]

Contributes to fixing #37640. Will leave the issue open until the fix is confirmed in app compat. cc @DotNetAppCompatFeiWang

#36936 made a change so that the serializer ignores virtual properties when a derived property that hid them is ignored (with [JsonIgnore]) and there's a JSON property name collision.

The failing scenario in #37640 makes it clear that we need to ignore virtual properties when a derived property that hid them is ignored, whether or not there's a property name collision. This is to avoid serializing content that the caller explicitly marked to be ignored.

[layomia]

Following up on #37710 (comment).

[steveharter]

Are the current semantics in this PR consistent with Newtonsoft?

[layomia]

Are the current semantics in this PR consistent with Newtonsoft?

With the latest commit, we are largely aligned with Newtonsoft.Json behavior.

• If an override of a virtual property is [JsonIgnore]d, then the property is ignored as well.

runtime/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs

Lines 386 to 387 in 766572f

	string serialized = JsonSerializer.Serialize(new DerivedClass_With_IgnoredOverride());
	Assert.Equal(@"{}", serialized);

• If a property that hides a virtual or non-virtual property is [JsonIgnore]d, and there's no property name collision, then the hidden property is included for (de)serialization.

runtime/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs

Lines 395 to 396 in 766572f

	serialized = JsonSerializer.Serialize(new DerivedClass_With_Ignored_NewProperty());
	Assert.Equal(@"{""MyProp"":false}", serialized);

• If a property that hides a virtual or non-virtual property is [JsonIgnore]d, and there's a property name collision between a non-ignored property and the hidden property, then the hidden property is ignored, while the non-ignored property is included for (de)serialization

runtime/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs

Lines 410 to 411 in 766572f

	serialized = JsonSerializer.Serialize(new DerivedClass_With_NewProperty_And_ConflictingPropertyName());
	Assert.Equal(@"{""MyProp"":null}", serialized);

wrt. how we differ - in Newtonsoft.Json, property name conflicts at different type-hierarchy levels that are not caused by deriving or the new keyword are allowed. So, properties on more derived types win. In System.Text.Json, property name conflicts are only valid when caused by deriving or the new keyword.

runtime/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs

Line 427 in 766572f

Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(new DerivedClass_WithConflictingPropertyName()));

[steveharter]

wrt. how we differ - in Newtonsoft.Json, property name conflicts at different type-hierarchy levels that are not caused by deriving or the new keyword are allowed. So, properties on more derived types win. In System.Text.Json, property name conflicts are only valid when caused by deriving or the new keyword.

This means all CLR naming "conflicts" are OK, but naming conflicts through [JsonPropertyName] are not?

[steveharter]

Is this necessary (I didn't see any use of nullability attributes)?

[layomia]

No - will remove. I used a nullability attribute in a helper in an earlier iteration of this change.

[layomia]

This means all CLR naming "conflicts" are OK, but naming conflicts through [JsonPropertyName] are not?

Yes. [JsonPropertyName] and also JsonNamingPolicy.

[DotNetAppCompatFeiWang]

cc @dotnet-actwx-bot