Open Georgiks opened 3 years ago
Tagging subscribers to this area: @eiriktsarpalis, @layomia See info in area-owners.md if you want to be subscribed.
Author: | Georgiks |
---|---|
Assignees: | - |
Labels: | `area-System.Text.Json`, `untriaged` |
Milestone: | - |
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.
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?
We seem to have a test that validates the behavior as reported: https://github.com/dotnet/runtime/blob/ec2d25c03fb91d8897277f2b732b71f903b87b1a/src/libraries/System.Text.Json/tests/Common/PropertyVisibilityTests.cs#L1003-L1013 and for value types: https://github.com/dotnet/runtime/blob/ec2d25c03fb91d8897277f2b732b71f903b87b1a/src/libraries/System.Text.Json/tests/Common/PropertyVisibilityTests.NonPublicAccessors.cs#L25-L26 Again, I don't have context as to why we're doing this, but likely we're closing this as by-design behavior.
Understood, thanks anyway.
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
.
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.
We won't have time to work on this for .NET 7, moving to Future.
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.
This overlaps with feature https://github.com/dotnet/runtime/issues/78556. The major scenario for modifying/populating properties (rather than replacing them with new instances) is for collections. FYI @krwq.
Description
With given code
Output:
When deserializing a JSON with a
List<>
property with a non-public getter, the value is not deserialized and property isnull
. However when serializing andint
the same way, a value of1
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 theList<>
one.Configuration
.NET Core 3.1
Regression?
Other information