JsonConverter inconsistency when deserializing into property without public getter

[Georgiks]

Description

With given code

public class PrivateGetter<T>
{
    public T Property { private get; set; }

    public override string ToString()
    {
        return Property is null ? "null" : Property.ToString();
    }
}

public class PublicGetter<T>
{
    public T Property { get; set; }

    public override string ToString()
    {
        return Property is null ? "null" : Property.ToString();
    }
}

var jsonListOfInts = @"
{
    ""Property"": [ 1 ]
}
";

var jsonInt = @"
{
    ""Property"": 1
}
";
Console.WriteLine(JsonSerializer.Deserialize<PrivateGetter<List<int>>>(jsonListOfInts));
Console.WriteLine(JsonSerializer.Deserialize<PublicGetter<List<int>>>(jsonListOfInts));

Console.WriteLine(JsonSerializer.Deserialize<PrivateGetter<int>>(jsonInt));
Console.WriteLine(JsonSerializer.Deserialize<PublicGetter<int>>(jsonInt));

Output:

null
System.Collections.Generic.List`1[System.Int32]
1
1

When deserializing a JSON with a List<> property with a non-public getter, the value is not deserialized and property is null.
However when serializing and int the same way, a value of 1 is (correctly) assigned.
I expect that the behaviour of deserializing into property with non-public getter is independent of the property type.

Unfortunatelly the documentation site https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-immutability does not specify whether the behaviour is to allow deserializing into property without public getter or not, therefore it is hard to guess whether the case with int is wrong or the List<> one.

Configuration

.NET Core 3.1

Regression?

Other information

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

With given code

public class PrivateGetter<T>
{
    public T Property { private get; set; }

    public override string ToString()
    {
        return Property is null ? "null" : Property.ToString();
    }
}

public class PublicGetter<T>
{
    public T Property { get; set; }

    public override string ToString()
    {
        return Property is null ? "null" : Property.ToString();
    }
}

var jsonListOfInts = @"
{
    ""Property"": [ 1 ]
}
";

var jsonInt = @"
{
    ""Property"": 1
}
";
Console.WriteLine(JsonSerializer.Deserialize<PrivateGetter<List<int>>>(jsonListOfInts));
Console.WriteLine(JsonSerializer.Deserialize<PublicGetter<List<int>>>(jsonListOfInts));

Console.WriteLine(JsonSerializer.Deserialize<PrivateGetter<int>>(jsonInt));
Console.WriteLine(JsonSerializer.Deserialize<PublicGetter<int>>(jsonInt));

Output:

null
System.Collections.Generic.List`1[System.Int32]
1
1

When deserializing a JSON with a List<> property with private getter, the value is not deserialized and property is null.
However when serializing and int the same way, a value of 1 is correctly assigned.
I expect that the behaviour of deserializing into property with non-public getter is independent on the property type.

Unfortunatelly the documentation site https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-immutability does not specify whether the behaviour is to allow deserializing into property without public getter or not, therefore it is hard to guess whether the case with int is wrong or the List<> one.

Configuration

.NET Core 3.1

Regression?

Other information

Author:	Georgiks
Assignees:	-
Labels:	area-System.Text.Json, untriaged
Milestone:	-

[eiriktsarpalis]

I can reproduce -- the issue is certainly surprising although I was able to force deserialization for the List case by attaching a JsonInclude attribute to the property.

That being said, the private getter/public setter combination is unusual, and we generally recommend against using it. We should still fix this and ensure relevant test coverage is added for that particular combination.

[eiriktsarpalis]

Doing a bit more digging -- the root cause appears to be this method which seems to applying different logic depending on whether the property type is a collection or not. Not sure why though, I couldn't find any comments justifying the split.

@layomia is this by design? should we be making the change to support setter-only deserialization?

[eiriktsarpalis]

We seem to have a test that validates the behavior as reported:

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

Lines 1003 to 1013 in ec2d25c

	public async Task NoGetter()
	{
	ClassWithNoGetter objWithNoGetter = await JsonSerializerWrapperForString.DeserializeWrapper<ClassWithNoGetter>(
	@"{""MyString"":""Hello"",""MyIntArray"":[0],""MyIntList"":[0]}");
	Assert.Equal("Hello", objWithNoGetter.GetMyString());
	// Currently we don't support setters without getters.
	Assert.Equal(0, objWithNoGetter.GetMyIntArray().Length);
	Assert.Equal(0, objWithNoGetter.GetMyIntList().Count);
	}

and for value types:

runtime/src/libraries/System.Text.Json/tests/Common/PropertyVisibilityTests.NonPublicAccessors.cs

Lines 25 to 26 in ec2d25c

	Assert.Equal(2f, obj.GetMyFloat);
	Assert.Equal(new Uri("https://microsoft.com"), obj.MyUri);

Again, I don't have context as to why we're doing this, but likely we're closing this as by-design behavior.

[Georgiks]

Understood, thanks anyway.

[eiriktsarpalis]

After private discussion with @layomia we have concluded that we should change the DetermineSerializationCapabilities method to not distinguish between collection and object/value converters.

That being said, it is a fix that probably doesn't meet the bug bar for RC1 since a) it affects a relatively rare corner case and b) it can be easily worked around by adding a JsonInclude attribute to the property. Making changes here carries a nonzero probability of introducing unanticipated breaks, so I'm moving this to 7.0.0.

[dvdvorle]

Just documenting a real-life use case here to make sure this makes it into 7.0.0

I'm using a write-only property for refactoring purposes such as property renames and the like, where the object is persisted as json.

[Obsolete("Don't use this, only for data-migration purposes.")]
public List<EnvironmentHistoryItem> EnvironmentHistory
{
  set => EnvironmentHistoryV2 = new EnvironmentHistory { Items = value };
}

public EnvironmentHistory EnvironmentHistoryV2 { get; set; } = new EnvironmentHistory();

I didn't expect this bug, took me a couple of hours to find it (and this github issue), and Newtonsoft also deserializes this correctly.

The JsonInclude workaround only applies if you have a private getter btw, if you have no getter at all it won't work, even with the JsonInclude attribute.

[eiriktsarpalis]

We won't have time to work on this for .NET 7, moving to Future.

[eiriktsarpalis]

FWIW we passed an opportunity to fix this behavior when refactoring for .NET 7 work. Even though the current behavior is inconsistent, we felt it would be too much of breaking change to justify fixing. We might revisit in the future assuming there is demand.

[layomia]

This overlaps with feature #78556. The major scenario for modifying/populating properties (rather than replacing them with new instances) is for collections. FYI @krwq.