Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add JsonIncludeAttribute & support for non-public accessors #34675

Merged
merged 4 commits into from
Apr 14, 2020

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Apr 8, 2020

Fixes #29743.
Fixes #34453 (since I'm in this code).

New API approved in #34558.

@layomia layomia added this to the 5.0 milestone Apr 8, 2020
@layomia layomia self-assigned this Apr 8, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@layomia layomia changed the title Add JsonIncludeAttribute & support for non-public property accessors Add JsonIncludeAttribute & support for non-public accessors Apr 8, 2020

namespace System.Text.Json.Serialization.Tests
{
public static partial class PropertyVisibilityTests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this PR assumes that the property is still public, correct?(meaning the setter or getter can be non-public, but not both). Do we have tests with a private property where both setter and getter are private to verify?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I added some tests showing that non-public properties (with various accessor visibility levels) are not included:

[Fact]
public static void NonPublicMembersAreNotIncluded()
{
Assert.Equal("{}", JsonSerializer.Serialize(new ClassWithNonPublicProperties()));
string json = @"{""MyInt"":1,""MyString"":""Hello"",""MyFloat"":2,""MyDouble"":3}";
var obj = JsonSerializer.Deserialize<ClassWithNonPublicProperties>(json);
Assert.Equal(0, obj.MyInt);
Assert.Null(obj.MyString);
Assert.Equal(0, obj.GetMyFloat);
Assert.Equal(0, obj.GetMyDouble);
}
private class ClassWithNonPublicProperties
{
internal int MyInt { get; set; }
internal string MyString { get; private set; }
internal float MyFloat { private get; set; }
private double MyDouble { get; set; }
internal float GetMyFloat => MyFloat;
internal double GetMyDouble => MyDouble;
}

These tests ensure that we throw IOE when [JsonInclude] is used on a non-public properties:

[Theory]
[InlineData(typeof(ClassWithPrivateProperty_WithJsonIncludeProperty))]
[InlineData(typeof(ClassWithInternalProperty_WithJsonIncludeProperty))]
[InlineData(typeof(ClassWithProtectedProperty_WithJsonIncludeProperty))]
public static void NonPublicProperty_WithJsonInclude_Invalid(Type type)
{
InvalidOperationException ex = Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize("", type));
string exAsStr = ex.ToString();
Assert.Contains("MyString", exAsStr);
Assert.Contains(type.ToString(), exAsStr);
Assert.Contains("JsonIncludeAttribute", exAsStr);
ex = Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(Activator.CreateInstance(type), type));
exAsStr = ex.ToString();
Assert.Contains("MyString", exAsStr);
Assert.Contains(type.ToString(), exAsStr);
Assert.Contains("JsonIncludeAttribute", exAsStr);
}
private class ClassWithPrivateProperty_WithJsonIncludeProperty
{
[JsonInclude]
private string MyString { get; set; }
}
private class ClassWithInternalProperty_WithJsonIncludeProperty
{
[JsonInclude]
internal string MyString { get; }
}
private class ClassWithProtectedProperty_WithJsonIncludeProperty
{
[JsonInclude]
protected string MyString { get; private set; }
}

@layomia layomia merged commit b3e791d into dotnet:master Apr 14, 2020
watfordjc added a commit to watfordjc/csharp-stream-controller that referenced this pull request Jul 8, 2020
Collection properties should be read only
https://docs.microsoft.com/en-gb/visualstudio/code-quality/ca2227

Suppress FxCopAnalyzers warning CA2227 as System.Text.Json.JsonSerializer.Deserialize in .NET Core 3.1 cannot deserialise to read-only properties.

There are two related issues that will allow System.Text.Json.JsonSerializer.Deserialize to deserialise to read-only properties in .NET Core 5.0:
1) Pull Request that includes support for non-public accessors: dotnet/runtime#34675
2) Issue regarding, among other things, adding to collections during deserialisation if the collection property has no setter: dotnet/runtime#30258
danjagnow added a commit to blizzard-net/warcraft that referenced this pull request Jul 23, 2020
Changed private setters to public in model classes for consistency and because System.Text.Json does not currently support them.  See dotnet/runtime#34675 for details, because this will be supported in .NET 5.
@TanvirArjel
Copy link

TanvirArjel commented Sep 16, 2020

@layomia Is this available on .NET 5.0 RC1, maybe not. My Project has been updated to .NET 5.0 RC1 but it's not working yet.

public class DataTableParamsDto
{
    public int Draw { get; set; }

    public int Start { get; set; }

    public int Length { get; set; }

    public List<ColumnRequestItem> Columns { get; private set; } // Not working

    public List<OrderRequestItem> Order { get; private set; } // Not working

    public SearchRequestItem Search { get; private set; } // Not working
}

@layomia
Copy link
Contributor Author

layomia commented Sep 17, 2020

@TanvirArjel the feature is in RC1, but it requires explicitly opting in with JsonIncludeAttribute:

public class DataTableParamsDto
{
    public int Draw { get; set; }

    public int Start { get; set; }

    public int Length { get; set; }

    [JsonInclude]
    public List<ColumnRequestItem> Columns { get; private set; }

    [JsonInclude]
    public List<OrderRequestItem> Order { get; private set; }

    [JsonInclude]
    public SearchRequestItem Search { get; private set; }
}

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
@layomia layomia deleted the non_public_prop_accessors branch May 1, 2021 00:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CreatePropertyInfoForClassInfo run twice? JsonSerializer should support private setters as an opt-in feature
4 participants