Runtime: JSON Serialization Name Collision with hidden properties

[douglasg14b]

Take Two Classes:

public class ClassA
{
    public List<int> SomeList { get; set; }
}

public class ClassB : ClassA
{
    new public IEnumerable<int> SomeList { get; set; }
}

If ClassB is serialized an exception is thrown:

The JSON property name for 'ClassB.SomeList' collides with another property.

This seems like unwanted behavior.

[ahsonkhan]

We don't fully support polymorphic types currently and the behavior you are seeing is a side effect of that.

Here's the issue tracking that feature: https://github.com/dotnet/corefx/issues/38650 / https://github.com/dotnet/corefx/issues/37787

cc @steveharter if you have any thoughts on this specific case, where we are seeing a property name collision.

[layomia]

From @nhuthan in https://github.com/dotnet/corefx/issues/42692:

Here is simple code:

public class Program
{
  public static void Main()
  {
      var str = "{\"P1\": 123}";
      var obj = JsonSerializer.Deserialize<B>(str);   //--> throw

  }
}

public class A{
  public int P1{get;set;} 
  public int P2{get;set;}
}

public class B: A{
  public new float P1{get;set;}
}

And I got an error:

Unhandled exception. System.InvalidOperationException: The JSON property name for 'B.P1' collides with another property.
   at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_SerializerPropertyNameConflict(JsonClassInfo jsonClassInfo, JsonPropertyInfo jsonPropertyInfo)
   at System.Text.Json.JsonClassInfo..ctor(Type type, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializerOptions.GetOrAddClass(Type classType)
   at System.Text.Json.JsonSerializer.ReadCore(Type returnType, JsonSerializerOptions options, Utf8JsonReader& reader)
   at System.Text.Json.JsonSerializer.ParseCore(String json, Type returnType, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
   at Program.Main()
Command terminated by signal 6

[layomia]

The expectation here is that property name collisions due to a member on a parent being hidden (with the new) should be resolved by the serializer ignoring the member on the parent. e.g. serializing an instance of MyDerivedClass should work fine:

```C#
public class MyClass
{
// This should be ignored.
public int MyNumber { get; set; }
}

public class MyDerivedClass : MyClass
{
// This should be (de)serialized.
new public double MyNumber { get; set; }
}

when field support is added, this should also work fine:

```C#
public class MyClass
{
    // This should be ignored.
    public int MyNumber;
}

public class MyDerivedClass : MyClass
{
    // This should be (de)serialized.
    new public double MyNumber { get; set; }
}

[JsonIgnore] should continue to work as expected (same for permutations with fields):

```C#
public class MyClass
{
// This should be ignored.
[JsonIgnore]
public int MyNumber { get; set; }
}

public class MyDerivedClass : MyClass
{
// This should be (de)serialized.
new public double MyNumber { get; set; }
}

```C#
public class MyClass
{
    // This should be (de)serialized.
    public int MyNumber { get; set; }
}

public class MyDerivedClass : MyClass
{
    // This should be ignored.
    [JsonIgnore]
    new public double MyNumber { get; set; }
}

Property name collisions due to [JsonPropertyName] or JsonNamingPolicy should continue to fail with the collision error:

```C#
public class MyClass
{
public int MyNumber { get; set; }
}

public class MyDerivedClass : MyClass
{
[JsonPropertyName("MyNumber")]
public double MyDouble { get; set; }
}

Newtonsoft.Json does not throw a collision error in such cases and honors the configuration on the derived class (ignores the parent member):

```C#
public class MyClass
{
    // This is ignored by Newtonsoft.Json
    public int MyNumber { get; set; }
}

public class MyDerivedClass : MyClass
{
    // This is (de)serialized by Newtonsoft.Json
    [JsonProperty("MyNumber")]
    public double MyDouble { get; set; }
}

I don't think the presence of a colliding property name (due to [JsonPropertyInfo]/JsonNamingPolicy) on a member of deriving class is enough information for the serializer to ignore a member on a parent. [JsonIgnore] should be used in such cases:

```C#
public class MyClass
{
// This should be ignored
[JsonIgnore]
public int MyNumber { get; set; }
}

public class MyDerivedClass : MyClass
{
// This should be (de)serialized
[JsonPropertyName("MyNumber")]
public double MyDouble { get; set; }
}

`[JsonPropertyName]`/`JsonNamingPolicy` should work as expected on `new` properties, so long as there are no collisions:

```C#
public class MyClass
{
    // This should be (de)serialized
    public int MyNumber { get; set; }
}

public class MyDerivedClass : MyClass
{
    // This should be (de)serialized
    [JsonPropertyName("MyDouble")]
    new public double MyNumber { get; set; }
}

cc @steveharter, @ahsonkhan

The implementation to fix this issue should include tests for all such permutations.

[YohDeadfall]

I've found another case reported by #32106. An interesting thing happens when you ask for properties of type when it has a new slot member. In case of @douglasg14b reflection returns three properties, but in the example below it returns only one from the derived class.

Is it expected behavior, @GrabYourPitchforks @steveharter ?

var case1 = typeof(ClassWithNewSlotPrivateProperty).GetProperties(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic);
// ClassWithNewSlotPrivateProperty.MyString

var case2 = typeof(ClassB).GetProperties(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic);
// ClassB.P1
// ClassA.P1
// ClassA.P2

// Case 1
public class ClassWithPublicProperty
{
    public string MyString { get; set; } = "DefaultValue";
}

public class ClassWithNewSlotPrivateProperty : ClassWithPublicProperty
{
    private new string MyString { get; set; } = "NewDefaultValue";
}


// Case 2
public class ClassA
{
    public List<int> SomeList { get; set; }
}

public class ClassB : ClassA
{
    new public IEnumerable<int> SomeList { get; set; }
}

/cc @jkotas

[NinoFloris]

@YohDeadfall to fully understand your example, you think case 1 should (de)serialize ClassWithPublicProperty.MyString because the derived class new slot property is private, and therefore less visible?

I guess I can agree to having every new slot property with higher or equal visibility being preferred over its parent. Is that the plan? Ultimately it's another thing to keep in mind, always hiding the parent if shadowed irrespective of visibility is simpler. Do you have some examples where it makes sense to go for the more complex logic?

[YohDeadfall]

@NinoFloris, sorry for the late reply. Yes, I think that derived classes should not alter serialization of base class members in a way to hide them. Hiding a parent's member by a derived class isn't a good idea since when something is public then there are some sense behind this, and changing visibility to private highlights architecture problems.

[layomia]

cc @steveharter, @Jozkee

[YohDeadfall]

@layomia this issue should be fixed by #32107. Just need to write one more test and change naming style for new visibility tests as requested.

[steveharter]

I've found another case reported by #32106. An interesting thing happens when you ask for properties of type when it has a new slot member. In case of @douglasg14b reflection returns three properties, but in the example below it returns only one from the derived class.

Is it expected behavior, @GrabYourPitchforks @steveharter

The case2 should only return 2 items, not 3.

If the name and signature of a property matches a derived class, it is considered a duplicate and not returned. In case1 the signature match but in case2 the signatures are different.