Added field support to JSON serializer

[YohDeadfall]

Fixes #876.

/cc @steveharter @ahsonkhan

[jkotas]

We need to figure out how to deal with Json serialization breaking changes before accepting new features like this one that can break existing working code. More discussion in #1198 .

[YohDeadfall]

Speaking about breakage, I see that the serializer now supports private properties by default, but this don't work for fields well. Am not sure that turning on private member serialization by default is a good idea because:

• some properties can be for internal usage only (can be workarounded by JsonIgnoreAttribute),
• non compiler generated fields must be marked always as ignored.

Changing the code to ignore non-public fields by default would lead to misbehavior and errors due to assumption that all members should be processed the same.

A test case which demonstrates the problem is for indexers.

[bartonjs]

This is going to need an opt-in (shouldn't be on by default).

[YohDeadfall]

Did you mean fields or private properties?

[Drawaes]

I know for me if you start serializing my private properties that is a potential data leak out of services and it would cause big problems (without opting in of course)

[bartonjs]

I meant fields (this PR). Private properties, too, really. The opt-in either needs to be a configuration parameter, or if you have to put an attribute on each serialized field/private-property then that's fine, too. (At a quick glance I didn't see this depending on a special per-field property, and didn't see a config setting being added to the ref.cs)

[YohDeadfall]

@bartonjs, why public field serialization should not behave as property serialization?

@Drawaes Agreed, private member serialization hasn't been a good idea ever.

[bartonjs]

why public field serialization should not behave as property serialization?

Because serialization is a persistence format, I'm very strongly of the belief that any change in the output from version to version requires an opt-in. If we used to not serialize fields, we shouldn't serialize them by default.

[YohDeadfall]

Strongly disagreed even if it's a breaking change. The feature won't affect to many users if will affect at all since public fields is a bad design. Another point is that other serializers support this by default and they do not separate fields and properties.

Otherwise, it's sad to see that the young serializer has received not enough attention and was released to early.

For non-public members opened issue #2242.

[layomia]

@YohDeadfall, as discussed above, this feature should not be on by default. New public options need to be designed and API approved, along with a spec detailing the behavior. For example, see the spec for the reference handling feature.

If you're interested, the next step to move this feature forward is to provide this spec. We can help review it and move it through API review, then we can move forward with an implementation.

A solution is to add a boolean property on JsonSerializerOptions, say IncludeFields, which defaults to false and is applied "globally" on (de)serialization. In addition, the spec should consider a few key questions:

• Which of the existing property-level attributes should be applied to fields? Should JsonExtensionData apply to fields?
• Which of the options in JsonSerializerOptions should apply to fields? Should PropertyNamingPolicy, PropertyNameCaseInsensitive apply to fields, or do we need new options?
• Should readonly fields be supported for deserialization? Should there be an option to ignore readonly fields for serialization (an equivalent to IgnoreReadOnlyProperties)?
• How should JSON property name collisions be handled? This is discussed further in JSON Serialization Name Collision with hidden properties #30964 (comment).

Looking into Newtonsoft.Json behavior for precedent is a good idea. We expect fields and properties to have the same pattern of behavior, e.g. collections should be handled in the same way.

---

Another thing to think about is more granular support for this feature. This doesn't need to be included in the spec, but I'd appreciate any thoughts you have.

In addition to a globally applied boolean IncludeFields option on JsonSerializerOptions, are scenarios requiring more granularity common enough for us to

• add a field-level [JsonIncludeField] attribute to indicate that a particular field should be included? This would be helpful if one wanted to turn off the feature globally and include only a few fields.
  • should [JsonExcludeField] be added as well for the inverse scenario?
  • should such attributes be generalized to [JsonInclude] and [JsonExclude] to accommodate other scenarios where one might want to cherry-pick inclusion (e.g. properties with private setters, readonly properties etc.)
  • should these attributes be applied recursively? e.g, include this field and all fields in its object graph
  • the functionality of such attributes might be covered with the JsonIgnoreCondition.Always and JsonIgnoreCondition.Never options being proposed in conditional per-property ignore semantics, but it would be good to think about how such options might be applied to this feature.
• add a class/struct-level attribute to indicate that fields in the POCO should be included (again, helpful if global option is false)?
  • should the attribute be recursively applied to the object graphs of all members or restricted to the members alone?

[YohDeadfall]

Looking into Newtonsoft.Json behavior for precedent is a good idea. We expect fields and properties to have the same pattern of behavior, e.g. collections should be handled in the same way.

For your info, Utf8Json behaves the same and all many other serializers including that which made by Microsoft and included into .NET. So the opt-in strategy would be a breaking change in mind and would cause problems when switching from something other to System.Text.Json. As I know Microsoft made an option to allow devs to continue using Newtonsoft.JSON instead of System.Text.Json in ASP.NET Core.

If you're interested, the next step to move this feature forward is to provide this spec. I can help review it and move it through API review, then we can move forward with an implementation.

Okay, let's go through API review process... What should be done for that?

---

I'm also thinking about more granular support for this feature. This doesn't need to be included in the spec, but I'd appreciate any thoughts you have.

This remembers me DataContractSerializer and all its attribute... Why not to reuse them? There are ShouldSerializeSomething methods and so on also.

[layomia]

@YohDeadfall

So the opt-in strategy would be a breaking change in mind and would cause problems when switching from something other to System.Text.Json.

We acknowledge this pain point, but prioritize not having such significant breaking changes between versions of System.Text.Json. Let's move forward with designing this as an opt-in feature, and we can revisit this decision later on. Does that sound good?

Microsoft made an option to allow devs to continue using Newtonsoft.JSON instead of System.Text.Json in ASP.NET Core

This will remain an option in 5.0, and ASP.NET Core has an easy way for you to plug in any serializer you want. For those who stick with System.Text.Json as the default, ASP.NET Core provides an easy way to configure options for the serializer which can opt in to field support.

Okay, let's go through API review process... What should be done for that?

The next step is to write a spec that contains an API proposal and addresses the above questions. When it is ready, create a PR like this #354, and we'll review it.

This remembers me DataContractSerializer and all its attribute... Why not to reuse them?

This is an option. This will be considered in depth, likely in the context of conditional per-property ignore semantics.

[stephentoub]

The next step is to write a spec that contains an API proposal and addresses the above questions. When it is ready, create a PR like this #354, and we'll review it.

@layomia, so this PR should be closed for now?

[layomia]

so this PR should be closed for now?

Yes, closing this for now until a spec is fully fleshed out: #32711.

cc @YohDeadfall

[YohDeadfall]

@layomia, it will take a bit more time than I thought. Anyway, am working on it, and here is a first question.

In JSON an object contains key-value pairs which represents properties and corresponding values. The serializer was designed keeping this in mind and works with properties only for now. Due to these two reasons everywhere in code we see properties (i.e. JsonPropertyInfo.PropertyInfo and so on).

Should we rename all parameters, fields and properties of type PropertyInfo to something having member as a part of the name? Without that the code looks like a mess, but with it PR goes to be larger.

The second question is about tests. Does the PR has enough of them?

[YohDeadfall]

A thought on tests. Having a text template to generate class/struct for visibility and fields would improve code coverage and remove duplication.