-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Added field support to JSON serializer #36986
Conversation
Addressing questions from #2192:
I think the renaming done in the current PR is fine i.e. using
I'd be supportive of having a template to do this if you want to take this on in this PR, depending on the implementation. However, for the purpose of driving this feature in, I'd be satisfied with interspersing fields in the existing test objects. Wrt. to coverage for the new code, we should have tests for all serializer features that apply to properties e.g. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. There are a couple new JsonSerializerOptions
properties from #876 (comment) yet to be added. I brought up a few test cases to add. MemberAccessor
review is pending on a fix for the CI failures.
...ies/System.Text.Json/src/System/Text/Json/Serialization/Attributes/JsonConverterAttribute.cs
Outdated
Show resolved
Hide resolved
...ies/System.Text.Json/src/System/Text/Json/Serialization/Attributes/JsonConverterAttribute.cs
Outdated
Show resolved
Hide resolved
@@ -148,6 +154,44 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) | |||
// Non-public properties should not be included for (de)serialization. | |||
} | |||
} | |||
|
|||
foreach (FieldInfo fieldInfo in currentType.GetFields(binding)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs
Outdated
Show resolved
Hide resolved
...ibraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
Outdated
Show resolved
Hide resolved
Ping @YohDeadfall. Marking as ready-for review since we are close to the implementation we want. There's still some feedback above to be addressed. |
Already addressed some of them, but not pushed (running tests). Am trying to narrow how the changes cause CI failures. |
7a018ec
to
457b22e
Compare
@layomia could you help me? After rebasing the PR and installing the latest .NET, am unable to build .NET anymore because of:
This blocks me from fixing the issue with constructors. |
Cleaning up |
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
Outdated
Show resolved
Hide resolved
b84b565
to
4786151
Compare
@YohDeadfall anything blocking progress on this? I can look into the test failures if needed. Preview 7 snaps on Monday, and we are trying to get this in soon. |
Just time blocks it accidentally, nothing more. I aware of the failures and know how to fix them. |
17650a4
to
ee5ef0d
Compare
Looks like CI failures are due to the merge conflicts. |
Are you holding off for green CI before adding tests? |
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs
Show resolved
Hide resolved
Please add the two test classes from the prior PR @ #2192 and be sure to hook them up to |
ee5ef0d
to
e528a0a
Compare
e528a0a
to
70813da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is green 👍 We just need the tests now :)
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionMemberAccessor.cs
Outdated
Show resolved
Hide resolved
...ibraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
Show resolved
Hide resolved
|
||
string memberName = memberInfo.Name; | ||
|
||
// The JsonPropertyNameAttribute or naming policy resulted in a collision. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've made a few changes recently to make sure we have the correct semantics for property name collisions. We should be sure to add tests to cover collisions between field names, and also between property and field names. Basically, we should extend this test suite:
runtime/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs
Lines 14 to 431 in 8fd5364
[Fact] | |
public static void Serialize_NewSlotPublicProperty() | |
{ | |
// Serialize | |
var obj = new ClassWithNewSlotProperty(); | |
string json = JsonSerializer.Serialize(obj); | |
Assert.Equal(@"{""MyString"":""NewDefaultValue""}", json); | |
// Deserialize | |
json = @"{""MyString"":""NewValue""}"; | |
obj = JsonSerializer.Deserialize<ClassWithNewSlotProperty>(json); | |
Assert.Equal("NewValue", ((ClassWithNewSlotProperty)obj).MyString); | |
Assert.Equal("DefaultValue", ((ClassWithInternalProperty)obj).MyString); | |
} | |
[Fact] | |
public static void Serialize_BasePublicProperty_ConflictWithDerivedPrivate() | |
{ | |
// Serialize | |
var obj = new ClassWithNewSlotInternalProperty(); | |
string json = JsonSerializer.Serialize(obj); | |
Assert.Equal(@"{""MyString"":""DefaultValue""}", json); | |
// Deserialize | |
json = @"{""MyString"":""NewValue""}"; | |
obj = JsonSerializer.Deserialize<ClassWithNewSlotInternalProperty>(json); | |
Assert.Equal("NewValue", ((ClassWithPublicProperty)obj).MyString); | |
Assert.Equal("NewDefaultValue", ((ClassWithNewSlotInternalProperty)obj).MyString); | |
} | |
[Fact] | |
public static void Serialize_PublicProperty_ConflictWithPrivateDueAttributes() | |
{ | |
// Serialize | |
var obj = new ClassWithPropertyNamingConflict(); | |
// Newtonsoft.Json throws JsonSerializationException here because | |
// non-public properties are included when [JsonProperty] is placed on them. | |
string json = JsonSerializer.Serialize(obj); | |
Assert.Equal(@"{""MyString"":""DefaultValue""}", json); | |
// Deserialize | |
json = @"{""MyString"":""NewValue""}"; | |
// Newtonsoft.Json throws JsonSerializationException here because | |
// non-public properties are included when [JsonProperty] is placed on them. | |
obj = JsonSerializer.Deserialize<ClassWithPropertyNamingConflict>(json); | |
Assert.Equal("NewValue", obj.MyString); | |
Assert.Equal("ConflictingValue", obj.ConflictingString); | |
} | |
[Fact] | |
public static void Serialize_PublicProperty_ConflictWithPrivateDuePolicy() | |
{ | |
var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; | |
// Serialize | |
var obj = new ClassWithPropertyPolicyConflict(); | |
string json = JsonSerializer.Serialize(obj, options); | |
Assert.Equal(@"{""myString"":""DefaultValue""}", json); | |
// Deserialize | |
json = @"{""myString"":""NewValue""}"; | |
obj = JsonSerializer.Deserialize<ClassWithPropertyPolicyConflict>(json, options); | |
Assert.Equal("NewValue", obj.MyString); | |
Assert.Equal("ConflictingValue", obj.myString); | |
} | |
[Fact] | |
public static void Serialize_NewSlotPublicProperty_ConflictWithBasePublicProperty() | |
{ | |
// Serialize | |
var obj = new ClassWithNewSlotDecimalProperty(); | |
string json = JsonSerializer.Serialize(obj); | |
Assert.Equal(@"{""MyNumeric"":1.5}", json); | |
// Deserialize | |
json = @"{""MyNumeric"":2.5}"; | |
obj = JsonSerializer.Deserialize<ClassWithNewSlotDecimalProperty>(json); | |
Assert.Equal(2.5M, obj.MyNumeric); | |
} | |
[Fact] | |
public static void Serialize_NewSlotPublicProperty_SpecifiedJsonPropertyName() | |
{ | |
// Serialize | |
var obj = new ClassWithNewSlotAttributedDecimalProperty(); | |
string json = JsonSerializer.Serialize(obj); | |
Assert.Contains(@"""MyNewNumeric"":1.5", json); | |
Assert.Contains(@"""MyNumeric"":1", json); | |
// Deserialize | |
json = @"{""MyNewNumeric"":2.5,""MyNumeric"":4}"; | |
obj = JsonSerializer.Deserialize<ClassWithNewSlotAttributedDecimalProperty>(json); | |
Assert.Equal(4, ((ClassWithHiddenByNewSlotIntProperty)obj).MyNumeric); | |
Assert.Equal(2.5M, ((ClassWithNewSlotAttributedDecimalProperty)obj).MyNumeric); | |
} | |
[Fact] | |
public static void Ignore_NonPublicProperty() | |
{ | |
// Serialize | |
var obj = new ClassWithInternalProperty(); | |
string json = JsonSerializer.Serialize(obj); | |
Assert.Equal(@"{}", json); | |
// Deserialize | |
json = @"{""MyString"":""NewValue""}"; | |
obj = JsonSerializer.Deserialize<ClassWithInternalProperty>(json); | |
Assert.Equal("DefaultValue", obj.MyString); | |
} | |
[Fact] | |
public static void Ignore_NewSlotPublicPropertyIgnored() | |
{ | |
// Serialize | |
var obj = new ClassWithIgnoredNewSlotProperty(); | |
string json = JsonSerializer.Serialize(obj); | |
Assert.Equal(@"{}", json); | |
// Deserialize | |
json = @"{""MyString"":""NewValue""}"; | |
obj = JsonSerializer.Deserialize<ClassWithIgnoredNewSlotProperty>(json); | |
Assert.Equal("NewDefaultValue", ((ClassWithIgnoredNewSlotProperty)obj).MyString); | |
Assert.Equal("DefaultValue", ((ClassWithInternalProperty)obj).MyString); | |
} | |
[Fact] | |
public static void Ignore_BasePublicPropertyIgnored_ConflictWithDerivedPrivate() | |
{ | |
// Serialize | |
var obj = new ClassWithIgnoredPublicPropertyAndNewSlotPrivate(); | |
string json = JsonSerializer.Serialize(obj); | |
Assert.Equal(@"{}", json); | |
// Deserialize | |
json = @"{""MyString"":""NewValue""}"; | |
obj = JsonSerializer.Deserialize<ClassWithIgnoredPublicPropertyAndNewSlotPrivate>(json); | |
Assert.Equal("DefaultValue", ((ClassWithIgnoredPublicProperty)obj).MyString); | |
Assert.Equal("NewDefaultValue", ((ClassWithIgnoredPublicPropertyAndNewSlotPrivate)obj).MyString); | |
} | |
[Fact] | |
public static void Ignore_PublicProperty_ConflictWithPrivateDueAttributes() | |
{ | |
// Serialize | |
var obj = new ClassWithIgnoredPropertyNamingConflictPrivate(); | |
string json = JsonSerializer.Serialize(obj); | |
Assert.Equal(@"{}", json); | |
// Newtonsoft.Json has the following output because non-public properties are included when [JsonProperty] is placed on them. | |
// {"MyString":"ConflictingValue"} | |
// Deserialize | |
json = @"{""MyString"":""NewValue""}"; | |
obj = JsonSerializer.Deserialize<ClassWithIgnoredPropertyNamingConflictPrivate>(json); | |
Assert.Equal("DefaultValue", obj.MyString); | |
Assert.Equal("ConflictingValue", obj.ConflictingString); | |
// The output for Newtonsoft.Json is: | |
// obj.ConflictingString = "NewValue" | |
// obj.MyString still equals "DefaultValue" | |
} | |
[Fact] | |
public static void Ignore_PublicProperty_ConflictWithPrivateDuePolicy() | |
{ | |
var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; | |
// Serialize | |
var obj = new ClassWithIgnoredPropertyPolicyConflictPrivate(); | |
string json = JsonSerializer.Serialize(obj, options); | |
Assert.Equal(@"{}", json); | |
// Deserialize | |
json = @"{""myString"":""NewValue""}"; | |
obj = JsonSerializer.Deserialize<ClassWithIgnoredPropertyPolicyConflictPrivate>(json, options); | |
Assert.Equal("DefaultValue", obj.MyString); | |
Assert.Equal("ConflictingValue", obj.myString); | |
} | |
[Fact] | |
public static void Ignore_PublicProperty_ConflictWithPublicDueAttributes() | |
{ | |
// Serialize | |
var obj = new ClassWithIgnoredPropertyNamingConflictPublic(); | |
string json = JsonSerializer.Serialize(obj); | |
Assert.Equal(@"{""MyString"":""ConflictingValue""}", json); | |
// Deserialize | |
json = @"{""MyString"":""NewValue""}"; | |
obj = JsonSerializer.Deserialize<ClassWithIgnoredPropertyNamingConflictPublic>(json); | |
Assert.Equal("DefaultValue", obj.MyString); | |
Assert.Equal("NewValue", obj.ConflictingString); | |
} | |
[Fact] | |
public static void Ignore_PublicProperty_ConflictWithPublicDuePolicy() | |
{ | |
var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; | |
// Serialize | |
var obj = new ClassWithIgnoredPropertyPolicyConflictPublic(); | |
string json = JsonSerializer.Serialize(obj, options); | |
Assert.Equal(@"{""myString"":""ConflictingValue""}", json); | |
// Deserialize | |
json = @"{""myString"":""NewValue""}"; | |
obj = JsonSerializer.Deserialize<ClassWithIgnoredPropertyPolicyConflictPublic>(json, options); | |
Assert.Equal("DefaultValue", obj.MyString); | |
Assert.Equal("NewValue", obj.myString); | |
} | |
[Fact] | |
public static void Throw_PublicProperty_ConflictDueAttributes() | |
{ | |
// Serialize | |
var obj = new ClassWithPropertyNamingConflictWhichThrows(); | |
Assert.Throws<InvalidOperationException>( | |
() => JsonSerializer.Serialize(obj)); | |
// Deserialize | |
string json = @"{""MyString"":""NewValue""}"; | |
Assert.Throws<InvalidOperationException>( | |
() => JsonSerializer.Deserialize<ClassWithPropertyNamingConflictWhichThrows>(json)); | |
} | |
[Fact] | |
public static void Throw_PublicProperty_ConflictDueAttributes_SingleInheritance() | |
{ | |
// Serialize | |
var obj = new ClassInheritedWithPropertyNamingConflictWhichThrows(); | |
Assert.Throws<InvalidOperationException>( | |
() => JsonSerializer.Serialize(obj)); | |
// The output for Newtonsoft.Json is: | |
// {"MyString":"ConflictingValue"} | |
// Conflicts at different type-hierarchy levels that are not caused by | |
// deriving or the new keyword are allowed. Properties on more derived types win. | |
// Deserialize | |
string json = @"{""MyString"":""NewValue""}"; | |
Assert.Throws<InvalidOperationException>( | |
() => JsonSerializer.Deserialize<ClassInheritedWithPropertyNamingConflictWhichThrows>(json)); | |
// The output for Newtonsoft.Json is: | |
// obj.ConflictingString = "NewValue" | |
// obj.MyString still equals "DefaultValue" | |
} | |
[Fact] | |
public static void Throw_PublicProperty_ConflictDueAttributes_DoubleInheritance() | |
{ | |
// Serialize | |
var obj = new ClassTwiceInheritedWithPropertyNamingConflictWhichThrows(); | |
Assert.Throws<InvalidOperationException>( | |
() => JsonSerializer.Serialize(obj)); | |
// The output for Newtonsoft.Json is: | |
// {"MyString":"ConflictingValue"} | |
// Conflicts at different type-hierarchy levels that are not caused by | |
// deriving or the new keyword are allowed. Properties on more derived types win. | |
// Deserialize | |
string json = @"{""MyString"":""NewValue""}"; | |
Assert.Throws<InvalidOperationException>( | |
() => JsonSerializer.Deserialize<ClassTwiceInheritedWithPropertyNamingConflictWhichThrows>(json)); | |
// The output for Newtonsoft.Json is: | |
// obj.ConflictingString = "NewValue" | |
// obj.MyString still equals "DefaultValue" | |
} | |
[Fact] | |
public static void Throw_PublicProperty_ConflictDuePolicy() | |
{ | |
var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; | |
// Serialize | |
var obj = new ClassWithPropertyPolicyConflictWhichThrows(); | |
Assert.Throws<InvalidOperationException>( | |
() => JsonSerializer.Serialize(obj, options)); | |
// Deserialize | |
string json = @"{""MyString"":""NewValue""}"; | |
Assert.Throws<InvalidOperationException>( | |
() => JsonSerializer.Deserialize<ClassWithPropertyPolicyConflictWhichThrows>(json, options)); | |
} | |
[Fact] | |
public static void Throw_PublicProperty_ConflictDuePolicy_SingleInheritance() | |
{ | |
var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; | |
// Serialize | |
var obj = new ClassInheritedWithPropertyPolicyConflictWhichThrows(); | |
Assert.Throws<InvalidOperationException>( | |
() => JsonSerializer.Serialize(obj, options)); | |
// The output for Newtonsoft.Json is: | |
// {"myString":"ConflictingValue"} | |
// Conflicts at different type-hierarchy levels that are not caused by | |
// deriving or the new keyword are allowed. Properties on more derived types win. | |
// Deserialize | |
string json = @"{""MyString"":""NewValue""}"; | |
Assert.Throws<InvalidOperationException>( | |
() => JsonSerializer.Deserialize<ClassInheritedWithPropertyPolicyConflictWhichThrows>(json, options)); | |
// The output for Newtonsoft.Json is: | |
// obj.myString = "NewValue" | |
// obj.MyString still equals "DefaultValue" | |
} | |
[Fact] | |
public static void Throw_PublicProperty_ConflictDuePolicy_DobuleInheritance() | |
{ | |
var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; | |
// Serialize | |
var obj = new ClassTwiceInheritedWithPropertyPolicyConflictWhichThrows(); | |
Assert.Throws<InvalidOperationException>( | |
() => JsonSerializer.Serialize(obj, options)); | |
// The output for Newtonsoft.Json is: | |
// {"myString":"ConflictingValue"} | |
// Conflicts at different type-hierarchy levels that are not caused by | |
// deriving or the new keyword are allowed. Properties on more derived types win. | |
// Deserialize | |
string json = @"{""MyString"":""NewValue""}"; | |
Assert.Throws<InvalidOperationException>( | |
() => JsonSerializer.Deserialize<ClassTwiceInheritedWithPropertyPolicyConflictWhichThrows>(json, options)); | |
// The output for Newtonsoft.Json is: | |
// obj.myString = "NewValue" | |
// obj.MyString still equals "DefaultValue" | |
} | |
[Fact] | |
public static void HiddenPropertiesIgnored_WhenOverridesIgnored() | |
{ | |
string serialized = JsonSerializer.Serialize(new DerivedClass_With_IgnoredOverride()); | |
Assert.Equal(@"{}", serialized); | |
serialized = JsonSerializer.Serialize(new DerivedClass_WithVisibleProperty_Of_DerivedClass_With_IgnoredOverride()); | |
Assert.Equal(@"{""MyProp"":false}", serialized); | |
serialized = JsonSerializer.Serialize(new DerivedClass_With_IgnoredOverride_And_ConflictingPropertyName()); | |
Assert.Equal(@"{""MyProp"":null}", serialized); | |
serialized = JsonSerializer.Serialize(new DerivedClass_With_Ignored_NewProperty()); | |
Assert.Equal(@"{""MyProp"":false}", serialized); | |
serialized = JsonSerializer.Serialize(new DerivedClass_WithConflictingNewMember()); | |
Assert.Equal(@"{""MyProp"":false}", serialized); | |
serialized = JsonSerializer.Serialize(new DerivedClass_WithConflictingNewMember_Of_DifferentType()); | |
Assert.Equal(@"{""MyProp"":0}", serialized); | |
serialized = JsonSerializer.Serialize(new DerivedClass_With_Ignored_ConflictingNewMember()); | |
Assert.Equal(@"{""MyProp"":false}", serialized); | |
serialized = JsonSerializer.Serialize(new DerivedClass_With_Ignored_ConflictingNewMember_Of_DifferentType()); | |
Assert.Equal(@"{""MyProp"":false}", serialized); | |
serialized = JsonSerializer.Serialize(new DerivedClass_With_NewProperty_And_ConflictingPropertyName()); | |
Assert.Equal(@"{""MyProp"":null}", serialized); | |
serialized = JsonSerializer.Serialize(new DerivedClass_With_Ignored_NewProperty_Of_DifferentType()); | |
Assert.Equal(@"{""MyProp"":false}", serialized); | |
serialized = JsonSerializer.Serialize(new DerivedClass_With_Ignored_NewProperty_Of_DifferentType_And_ConflictingPropertyName()); | |
Assert.Equal(@"{""MyProp"":null}", serialized); | |
serialized = JsonSerializer.Serialize(new FurtherDerivedClass_With_ConflictingPropertyName()); | |
Assert.Equal(@"{""MyProp"":null}", serialized); | |
// Here we differ from Newtonsoft.Json, where the output would be | |
// {"MyProp":null} | |
// Conflicts at different type-hierarchy levels that are not caused by | |
// deriving or the new keyword are allowed. Properties on more derived types win. | |
// This is invalid in System.Text.Json. | |
Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(new DerivedClass_WithConflictingPropertyName())); | |
serialized = JsonSerializer.Serialize(new FurtherDerivedClass_With_IgnoredOverride()); | |
Assert.Equal(@"{""MyProp"":null}", serialized); | |
} |
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.cs
Outdated
Show resolved
Hide resolved
9a4f40e
to
c45c32e
Compare
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
Show resolved
Hide resolved
@YohDeadfall, will you have some time soon to check on the failures here? |
Am on vacation till Saturday, so on Sunday will return back and finish the feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM when these comments are addressed.
@@ -14,13 +14,13 @@ namespace System.Text.Json.Serialization | |||
/// During deserializing, when using <see cref="object"/> a "null" JSON value is treated as a <c>null</c> object reference, and when using | |||
/// <see cref="JsonElement"/> a "null" is treated as a JsonElement with <see cref="JsonElement.ValueKind"/> set to <see cref="JsonValueKind.Null"/>. | |||
/// | |||
/// During serializing, the name of the extension data property is not included in the JSON; | |||
/// During serializing, the name of the extension data member is not included in the JSON; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
member
change to property or field here and below.
public sealed class JsonIgnoreAttribute : JsonAttribute | ||
{ | ||
/// <summary> | ||
/// Specifies the condition that must be met before a property will be ignored. | ||
/// Specifies the condition that must be met before a member will be ignored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
property or field
@@ -30,9 +31,11 @@ public static void Read(Type classType, byte[] data) | |||
public static void ReadFromStream(Type classType, byte[] data) | |||
{ | |||
MemoryStream stream = new MemoryStream(data); | |||
var options = new JsonSerializerOptions { IncludeFields = true }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need IncludeFields
on line 48. This is the cause of the CI failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I pushed tests as is before going to vacation. Thought that would have some time, but had other interests there.
{ | ||
ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(propertyInfo, currentType); | ||
ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(fieldInfo, currentType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please test this code path by extending these tests:
Lines 295 to 331 in 3aa66d4
[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; } | |
} | |
} |
@@ -218,6 +218,8 @@ public sealed partial class JsonSerializerOptions | |||
public System.Text.Encodings.Web.JavaScriptEncoder? Encoder { get { throw null; } set { } } | |||
public bool IgnoreNullValues { get { throw null; } set { } } | |||
public bool IgnoreReadOnlyProperties { get { throw null; } set { } } | |||
public bool IgnoreReadOnlyFields { get { throw null; } set { } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test using this option.
@@ -479,7 +481,7 @@ public abstract partial class JsonConverter | |||
internal JsonConverter() { } | |||
public abstract bool CanConvert(System.Type typeToConvert); | |||
} | |||
[System.AttributeUsageAttribute(System.AttributeTargets.Class | System.AttributeTargets.Enum | System.AttributeTargets.Property | System.AttributeTargets.Struct, AllowMultiple=false)] | |||
[System.AttributeUsageAttribute(System.AttributeTargets.Class | System.AttributeTargets.Struct | System.AttributeTargets.Enum | System.AttributeTargets.Property | System.AttributeTargets.Field, AllowMultiple=false)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test using the attribute on a field, along with JsonIgnoreAttribute, JsonIncludeAttribute, JsonExtensionDataAttribute & JsonPropertyName.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some new tests, but not finished yet. For naming they already exist.
2c58c23
to
735f6f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks for your contribution, @YohDeadfall.
...ies/System.Text.Json/src/System/Text/Json/Serialization/Attributes/JsonConverterAttribute.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/tests/Serialization/ExtensionDataTests.cs
Show resolved
Hide resolved
Will address minor clean up. |
By default the AspNet.Core Serializer on 3.0+ is the System.Text.Json, the current implementation of the library doesn't support Fields only Properties even public fields. There is a change that will be part of .net 5.0 that will provide a option to change this behaviour dotnet/runtime#36986
By default the AspNet.Core Serializer on 3.0+ is the System.Text.Json, the current implementation of the library doesn't support Fields only Properties even public fields. There is a change that will be part of .net 5.0 that will provide a option to change this behaviour dotnet/runtime#36986
By default the AspNet.Core Serializer on 3.0+ is the System.Text.Json, the current implementation of the library doesn't support Fields only Properties even public fields. There is a change that will be part of .net 5.0 that will provide a option to change this behaviour dotnet/runtime#36986
By default the AspNet.Core Serializer on 3.0+ is the System.Text.Json, the current implementation of the library doesn't support Fields only Properties even public fields. There is a change that will be part of .net 5.0 that will provide a option to change this behaviour dotnet/runtime#36986
By default the AspNet.Core Serializer on 3.0+ is the System.Text.Json, the current implementation of the library doesn't support Fields only Properties even public fields. There is a change that will be part of .net 5.0 that will provide a option to change this behaviour dotnet/runtime#36986
By default the AspNet.Core Serializer on 3.0+ is the System.Text.Json, the current implementation of the library doesn't support Fields only Properties even public fields. There is a change that will be part of .net 5.0 that will provide a option to change this behaviour dotnet/runtime#36986
By default the AspNet.Core Serializer on 3.0+ is the System.Text.Json, the current implementation of the library doesn't support Fields only Properties even public fields. There is a change that will be part of .net 5.0 that will provide a option to change this behaviour dotnet/runtime#36986
By default the AspNet.Core Serializer on 3.0+ is the System.Text.Json, the current implementation of the library doesn't support Fields only Properties even public fields. There is a change that will be part of .net 5.0 that will provide a option to change this behaviour dotnet/runtime#36986
Fixes #876, continues #2192.
@layomia This is reworked version of what I did before. It has no tests (in progress), but I would like to get your comments as soon as possible. And yep, there were unanswered questions in the previous PR. Hope you will find some time to address them (: