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

JsonSerializer should support field as well as properties #876

Closed
YohDeadfall opened this issue Mar 30, 2019 · 34 comments · Fixed by #36986
Closed

JsonSerializer should support field as well as properties #876

YohDeadfall opened this issue Mar 30, 2019 · 34 comments · Fixed by #36986
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Milestone

Comments

@YohDeadfall
Copy link
Contributor

YohDeadfall commented Mar 30, 2019

Updated by @layomia: I'm pasting the API approved in #34558. Please see that issue for review notes and further discussion.

namespace System.Text.Json
{
    public partial class JsonSerializerOptions
    {
        public bool IgnoreReadOnlyFields { get; set; }
        public bool IncludeFields { get; set; }
    }
}
namespace System.Text.Json.Serialization
{
    [AttributeUsage(AttributeTargets.Property |
                    Attributes.Field, AllowMultiple = false)]
    public sealed class JsonIncludeAttribute : JsonAttribute
    {
        public JsonIncludeAttribute();
    }
}
Original proposal (click to view)

There is no way to serialize and deserialize fields using JsonSerializer.

While public fields are generally not recommended, they are used in .NET itself (see value tuples) and by users.

This feature was scoped out of v1 (3.x) due to lack of time, as we prioritized supporting simple POCOs with public properties.

We elected to have an opt-in model for field support because it would be a breaking change to support them by default, and also because public fields are generally not recommended. Other serializers, including Newtonsoft.Json, Utf8Json, and Jil, support this feature by default.

@karelz
Copy link
Member

karelz commented Mar 30, 2019

Sounds good to me

@YohDeadfall
Copy link
Contributor Author

@ahsonkhan Should this be implemented via emitting IL or via reflection accessors?

@ahsonkhan
Copy link
Member

Should this be implemented via emitting IL or via reflection accessors?

It would depend on which approach yields better performance. I believe IL emit is the way to go (with a fallback to a reflection-based code path if necessary). @steveharter can provide additional guidance, or correct me if I am mistaken.

@ahsonkhan
Copy link
Member

Context from a user's issue where fields not being supported causing a pain-point:
dotnet/aspnetcore#11210

Describe the bug

When I use below code in razor page to post a http request. The deserialized object is not correct. There is no exception happened.

billInfo = await Http.PostJsonAsync<BillInfo>("api/TMobileBill", filePath);

I then changed to use Http.PostAsync to get the response string, then deserialize it by using Newtonsoft library. It works fine.

        var myContent = Newtonsoft.Json.JsonConvert.SerializeObject(filePath);
        var buffer = System.Text.Encoding.UTF8.GetBytes(myContent);
        var byteContent = new ByteArrayContent(buffer);
        byteContent.Headers.ContentType = new System.Net.Http.Headers.MediaTypeWithQualityHeaderValue("application/json");
        var response = await Http.PostAsync("api/TMobileBill", byteContent);
        var responseStr = await response.Content.ReadAsStringAsync();
        billInfo = Newtonsoft.Json.JsonConvert.DeserializeObject<BillInfo>(responseStr);
        StateHasChanged();

The response json string in my problem is:

{"period":"May 09, 2019 - Jun 08, 2019","dueDate":"Jul 01, 2019","totalAmount":326.03,"billInfoPerPerson":[{"name":"A B","phonesInfo":[{"phoneNumber":"(425) 111-1111","planHost":false,"usage":4.84,"regularCharge":30.0,"otherCharge":0.0,"totalCharge":0.0},{"phoneNumber":"(425) 222-2222","planHost":false,"usage":5.48,"regularCharge":30.0,"otherCharge":0.0,"totalCharge":0.0}],"totalAmount":60.0},{"name":"C D","phonesInfo":[{"phoneNumber":"(425) 333-3333","planHost":false,"usage":1.08,"regularCharge":20.0,"otherCharge":0.0,"totalCharge":0.0},{"phoneNumber":"(425) 444-4444","planHost":false,"usage":1.47,"regularCharge":20.0,"otherCharge":0.0,"totalCharge":0.0}],"totalAmount":40.0},{"name":"E F","phonesInfo":[{"phoneNumber":"(425) 555-5555","planHost":false,"usage":1.05,"regularCharge":20.0,"otherCharge":28.03,"totalCharge":0.0},{"phoneNumber":"(425) 666-6666","planHost":false,"usage":2.15,"regularCharge":30.0,"otherCharge":0.0,"totalCharge":0.0}],"totalAmount":78.03},{"name":"G H","phonesInfo":[{"phoneNumber":"(425) 777-7777","planHost":false,"usage":1.52,"regularCharge":20.0,"otherCharge":17.5,"totalCharge":0.0},{"phoneNumber":"(425) 888-8888","planHost":true,"usage":5.27,"regularCharge":30.0,"otherCharge":20.5,"totalCharge":0.0}],"totalAmount":88.0},{"name":"I J","phonesInfo":[{"phoneNumber":"(425) 999-9999","planHost":false,"usage":3.67,"regularCharge":30.0,"otherCharge":0.0,"totalCharge":0.0},{"phoneNumber":"(425) 000-0000","planHost":false,"usage":3.05,"regularCharge":30.0,"otherCharge":0.0,"totalCharge":0.0}],"totalAmount":60.0}],"warning":"There are $3 account service fee. Make sure it is expected. "}

The corresponding class is:

    public class PersonBillInfo
    {
        public string Name;
        public List<PhoneBillInfo> PhonesInfo;
        public float TotalAmount;
    }

    public class PhoneBillInfo
    {
        public string PhoneNumber;
        public bool PlanHost;
        public float Usage;
        public float RegularCharge;
        public float OtherCharge;
        public float TotalCharge;
    }

    public class BillInfo
    {
        public string Period;
        public string DueDate;
        public float TotalAmount;
        public IEnumerable<PersonBillInfo> BillInfoPerPerson;
        public string Warning;
    }

To Reproduce

Steps to reproduce the behavior:

  1. Using this version of ASP.NET Core 'SDK 3.0.100-preview6-012264'
  2. Implement a controller action to respond the json string as above.
  3. From razor page, use below code to call the controller action.
var billInfo = Http.PostJsonAsync<BillInfo>("api/TMobileBill", filePath);
  1. Evaluate the billInfo object. All the members of billInfo object are null.

Expected behavior

The members of billInfo object have values.

@YohDeadfall
Copy link
Contributor Author

@steveharter Without dotnet/corefx#38233 I'm not able to implement this one. Later it would be possible to access fields without code emit, but it's work in progress by @GrabYourPitchforks.

@Timovzl
Copy link

Timovzl commented Aug 5, 2019

Should this be implemented via emitting IL or via reflection accessors?

IL emit also has the advantage of letting us write to readonly fields, something I have not been able to achieve with expressions or, if memory serves, with reflection.

In case anyone trips on the idea, note that, while writing to readonly fields may seem questionable at first glance, it actually makes perfect sense during deserialization. After all, deserialization is an alternate way to construct a fully populated object. It is comparable to using a specific constructor that takes a parameter for each field - a perfect using case for assigning our readonly fields. The fact that deserialization is often not implemented by using such a constructor does not change the fact that we are trying to constructor a fully populated object. In other words, being able to assign readonly fields during deserialization is valuable and makes sense.

@ahsonkhan
Copy link
Member

From @MarkStega in https://github.com/dotnet/corefx/issues/41374

It appears that System.Text.Json.Deserialize does not properly handle an array of POCO's inside another POCO.

Build the following console application:

using Newtonsoft.Json;
using System;

namespace ConsoleApp1
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine("Hello Json!");

            string json = "{\"pStatus\":0,\"pError\":\"\",\"pResult\":[{\"processIdentifier\":"
                + "\"ANES - 2019 - 9955\",\"associatedProcessIdentifier\":\"\",\"dashboard\":\" "
                + "| SPD_Version | 5 | SPD_AnesthesiaType | Choice | SPD_BloodProducts ||\"},"
                + "{\"processIdentifier\":\"ANES - 2019 - 9964\",\"associatedProcessIdentifier\":\"\","
                + "\"dashboard\":\" | SPD_Version | 5 | SPD_AnesthesiaType | Choice | SPD_BloodProducts ||\""
                + "}]}";

            var newton = Newtonsoft.Json.JsonConvert.DeserializeObject<ServiceResult<Process_DD[]>>(json);

            var stjo = new System.Text.Json.JsonSerializerOptions();
            stjo.PropertyNameCaseInsensitive = true;
            var stj = System.Text.Json.JsonSerializer.Deserialize<ServiceResult<Process_DD[]>>(json, stjo);
        }
    }

    public class ServiceResult<ServiceResultType>
    {
        public enum eStatus { OK, Error };
        public eStatus pStatus { get; set; }
        public string pError { get; set; }
        public ServiceResultType pResult { get; set; }

        public ServiceResult() { }
        public ServiceResult(
            ServiceResultType p_Result,
            eStatus p_Status = eStatus.OK,
            string p_Error = "")
        {
            pStatus = p_Status;
            pError = p_Error;
            pResult = p_Result;
        }
    }

    public class Process_DD
    {
        public string ProcessIdentifier;
        public string AssociatedProcessIdentifier;
        public string Dashboard;

        public Process_DD()
        {
        }
        public Process_DD(string processIdentifier, string associatedProcessIdentifier, string dashboard)
        {
            this.ProcessIdentifier = processIdentifier;
            this.AssociatedProcessIdentifier = associatedProcessIdentifier;
            this.Dashboard = dashboard;
        }
    }

}

using the following csproj:

&lt;Project Sdk="Microsoft.NET.Sdk"&gt;

  &lt;PropertyGroup&gt;
    &lt;OutputType&gt;Exe</OutputType&gt;
    &lt;TargetFramework&gt;netcoreapp3.0&lt;/TargetFramework&gt;
  &lt;/PropertyGroup&gt;

  &lt;ItemGroup&gt;
    &lt;PackageReference Include="Newtonsoft.Json" Version="12.0.2" /&gt;
  &lt;/ItemGroup&gt;

&lt;/Project&gt;

Debug the program and step through both deserializations. Note that the Newtonsoft result has appropriate values for the two array elements of type Process_DD. The System.Text.Json result has null values for the properties.

C:\Users\ms>dotnet --info
.NET Core SDK (reflecting any global.json):
Version: 3.0.100
Commit: 04339c3a26

Runtime Environment:
OS Name: Windows
OS Version: 10.0.18362
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\3.0.100\

Host (useful for support):
Version: 3.0.0
Commit: 7d57652f33

.NET Core SDKs installed:
2.1.802 [C:\Program Files\dotnet\sdk]
2.2.402 [C:\Program Files\dotnet\sdk]
3.0.100 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
Microsoft.AspNetCore.All 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.All 2.2.7 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.App 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 2.2.7 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 3.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.NETCore.App 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.2.7 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 3.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.WindowsDesktop.App 3.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

To install additional .NET Core runtimes or SDKs:
https://aka.ms/dotnet-download

@ahsonkhan
Copy link
Member

From https://github.com/dotnet/corefx/issues/41420 by @codexguy:

To reproduce:
Take the sample default ASPNET Core 3.0 app (API-based template), change the WeatherForecast class to include a ValueTuple:

public class WeatherForecast
{

...

    public (int x, int y) Location { get; set; }
}

Change the Get method to populate Location with arbitrary values. Run the project: JSON returned has an empty location. Serializing ValueTuple's is possible with Newtonsoft (being unable to do so wrecks existing code I have that uses ValueTuples successfully with Core 2.1, moving to Core 3.0).

@layomia
Copy link
Contributor

layomia commented Dec 14, 2019

From @kaamil1984 in #552:

Can we have tuple support in System.Text.Json.Serializer?

General

Is there any chance, that we can have tuple support in System.Text.Json.Serializer?

I would like to serialize and deserialize object like this:

(int Result, string SuccessMessage, string ErrorMessage) MyTuple;

@layomia layomia transferred this issue from dotnet/corefx Dec 14, 2019
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Dec 14, 2019
@layomia layomia added this to the 5.0 milestone Dec 14, 2019
@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Dec 14, 2019
@marcuslindblom
Copy link

Any progress here or what is the prefered workaround for this issue?

@YohDeadfall
Copy link
Contributor Author

Previously it was blocked by the team due to optimizations. Recently pinged @ahsonkhanon this, but have received no answer.

Anyway I'm going to move the PR to dotnet/runtime in a few days.

@marcuslindblom
Copy link

@YohDeadfall does this address named tuple serialization as well? I use Newtonsoft Json for now but it serialize this (string Landscape, string Portrait) Images; as Item1 and Item2.

@YohDeadfall
Copy link
Contributor Author

Don't think that named tuples worth support because names available only in very limited number of cases and in most of them it won't work well. Names provided by TupleElementNamesAttribute and, as you know, attributes cannot be applied to variables. Therefore, names can be serialized when a value tuple is a part of another object, but this probably means a bad design.

andy-a-o added a commit to losol/eventuras that referenced this issue Jan 5, 2020
The issue was, ValueTuples aren't serialized properly. Here's the link dotnet/runtime#876.
For now, we shouldn't use ValueTuples for DTOs at all, instead we need to create DTO classes for each part of the response.

+ Integration test.
losolio pushed a commit to losol/eventuras that referenced this issue Jan 5, 2020
The issue was, ValueTuples aren't serialized properly. Here's the link dotnet/runtime#876.
For now, we shouldn't use ValueTuples for DTOs at all, instead we need to create DTO classes for each part of the response.

+ Integration test.
@marcuslindblom
Copy link

@YohDeadfall Ok, I see. Do I understand you correct if I could use TupleElementAttribute if I have a model like this?

public class Home {
    public string Heading { get;set; }
    public ArtDirection ArtDirection { get;set; }
  }

  public class ArtDirection {
    public string Alt { get; set; }
    public (string Landscape, string Portrait, string Fallback) Images { get; set; }
  }

@YohDeadfall
Copy link
Contributor Author

Yes, it is only one possible way to get names of tuple members, but in other cases it isn't possible (when you pass a tuple directly to the serializer). Therefore, supporting TupleElementNamesAttribute would cause different behavior in different cases and misunderstanding by users. The final decision on this should be made by Microsoft team.

It's better to have a class/struct for that kind of scenario.

/cc @ahsonkhan @steveharter @layomia

@ahsonkhan
Copy link
Member

From @newpost in #1519

[ApiController]
    [Route("[controller]")]
    public class DefaultController : ControllerBase
    {
        [HttpGet]
        public ValueTuple<int, int, int> Index()
        {
            return new ValueTuple<int,int,int>(1,2,3);
        }
    }

actual output: {}.
expected output:{"item1":1,"item2":2,"item3":3}

@ahsonkhan
Copy link
Member

I mention a workaround for ValueTuples (up to 3) based on custom converters if folks need it.
#1519 (comment)

@ahsonkhan
Copy link
Member

From @Greypuma in #32294

I was trying to serialize an object into json. I get an empty string. below is the code i used.
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Text.Json;
using System.Text.Json.Serialization;
using Newtonsoft.Json;

namespace test
{
public class AdbEmulator
{
//needs to be public otherwise wont work
public string name;
internal string attached;

}
public class AndroidJson
{
    public AndroidJson()
    {
        AddData();
    }
    public void AddData()
    {
        AdbEmulator adb1 = new AdbEmulator() { name = "emulator-5554", attached = "ll" };
        emulators.Add(adb1);
        string jsonAdb = System.Text.Json.JsonSerializer.Serialize<AdbEmulator>(adb1);
        string newtonjson = Newtonsoft.Json.JsonConvert.SerializeObject(adb1);
    }
}

Program.cs
static void Main(string[] args)
{ AndroidJson aj = new AndroidJson(); }

Output
jsonAdb = {}
newtonjson ={"name":"emulator-5554"}

I'm not sure why newtonsoft works and the microsoft one does not.
i'v'e tried the code in an dotnet core 3.1 project and windows application ( including the nuget package for that project).

@rafaelbenavent
Copy link

Any update on this?

@layomia
Copy link
Contributor

layomia commented Mar 25, 2020

From @leung85 in #34001:

core version 3.1.102

 public class TestDict
    {
        public Dictionary<string, string> Test = new Dictionary<string, string>(){
            { "09:00", "In"},
            { "18:00", "Out"}
        };
    }

 var option = new JsonSerializerOptions();
            option.WriteIndented = true;
            string configJson = System.Text.Json.JsonSerializer.Serialize(new TestDict(), option);

the configJson return {},
But it can be serialized if just use Dictionary<string, string> directly.

@layomia
Copy link
Contributor

layomia commented Apr 13, 2020

@YohDeadfall, are you still interested in implementing this feature? API & behavior spec approved in #34558:

namespace System.Text.Json
{
    public partial class JsonSerializerOptions
    {
        public bool IgnoreReadOnlyFields { get; set; }
        public bool IncludeFields { get; set; }
    }
}
namespace System.Text.Json.Serialization
{
    [AttributeUsage(AttributeTargets.Property |
                    Attributes.Field, AllowMultiple = false)]
    public sealed class JsonIncludeAttribute : JsonAttribute
    {
        public JsonIncludeAttribute();
    }
}

@YohDeadfall
Copy link
Contributor Author

Will take a look and implement. Probably will resurrect the closed PR with the requested API above.

@mnns
Copy link

mnns commented May 7, 2020

Spent a lot of time understanding why I don't see the fields in my output. Anyway, using 3.1, still don't see this option. Any news? thanks

@YohDeadfall
Copy link
Contributor Author

It should be .NET 5 feature.

@layomia layomia added the api-approved API was approved in API review, it can be implemented label May 11, 2020
@layomia
Copy link
Contributor

layomia commented May 15, 2020

@YohDeadfall, can you take another look soon? Field support is one of the top priorities for System.Text.Json in 5.0, and we'd like to get the feature in a preview for folks to try.

@YohDeadfall
Copy link
Contributor Author

Planned to reopen the PR on this weekend. Working on this won't be problematic, but the snake case naming policy will since there is a stuck discussion.

@layomia
Copy link
Contributor

layomia commented May 15, 2020

Planned to reopen the PR on this weekend.

Sounds good 👍

but the snake case naming policy will since there is a stuck discussion.

We should address snake case separately from this issue - #782 (comment).

@YohDeadfall
Copy link
Contributor Author

While it isn't too late, there are a few Ignore properties in JsonSerializerOptions. Should IncludeFields be renamed to IgnoreFields with the default value true for consistency?

@layomia
Copy link
Contributor

layomia commented Jun 2, 2020

Should IncludeFields be renamed to IgnoreFields with the default value true for consistency?

This was considered, but we chose IncludeFields so that the default value would be false for consistency with other options (where CLR default is used).

@jessicah
Copy link

jessicah commented Nov 1, 2020

Could we have the JsonInclude also apply at the struct level?

@YohDeadfall
Copy link
Contributor Author

What did you mean? It's not working for structs or something else?

@ghost ghost locked as resolved and limited conversation to collaborators Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Projects
No open projects