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

Blocking Issue: System.Text.Json and non-public members #31511

Closed
ghost opened this issue Nov 18, 2019 · 53 comments
Closed

Blocking Issue: System.Text.Json and non-public members #31511

ghost opened this issue Nov 18, 2019 · 53 comments
Assignees
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Text.Json design-discussion Ongoing discussion about design without consensus
Milestone

Comments

@ghost
Copy link

ghost commented Nov 18, 2019

Hello,

Why do the Serializers and Deserializers in the System.Text.Json namespace not want to deal with non-public class members? Often times, we need to have non-public class properties/fields serialized or deserialized (or even one-way serialized/deserialized).

This is a blocking issue because Newtonsoft Json does this correctly. This makes porting code that used Newtonsoft to the System.Text.Json serializers impossible.

@Clockwork-Muse
Copy link
Contributor

Having non-public data members be part of the serialization contract implies that you can't use only public information to reconstruct one, which is often problematic. It's also conceptually nonsensical - if it's part of the serialization contract, that by definition is part of (somewhere) a public contract.

@ghost
Copy link
Author

ghost commented Nov 18, 2019

Not necessarily @Clockwork-Muse. You would have internal/private members that are necessary to the class's function on rehydration (states for example) that you don't want to expose to other consumers of the class -- and hence would be private/internal to that class. Think of the classes holding data for a Workflow system.

I think your misunderstanding comes from the assumption that you if you persist to Json, you sure MUST be sharing it with some other code and hence only public/public is allowable. That is so not true! You persist to Json for a LOT of reasons, one of them being intermediate state persistence of objects in large applications.

@etreff-tillster
Copy link

I'll add to this. We use private properties extensively because we need to transform strings or objects on serialization/deserialization. An example is with CultureInfo for our UI. We want to set what language the user has selected, but CultureInfo is not serializable. We transform the CultureInfo object to use its code instead. Of course we can do this with converters, but that also implies a bit of behind the scenes magic.

A better example is transformation of DateTimeOffset objects. We can't necessarily use a converter for this because we may only need a specific DateTimeOffset to be formatted differently. Putting business logic into a model is bad, but putting business logic into a converter is even worse.

@Clockwork-Muse
Copy link
Contributor

@etreff-tillster - What are you doing to DateTimeOffsets? You should (barring exceptional circumstances) be sending the "raw", standard ISO format, value, which will be done for you, and then handling the transformation on the client side, if necessary.

@etreff-tillster
Copy link

@etreff-tillster - What are you doing to DateTimeOffsets? You should (barring exceptional circumstances) be sending the "raw", standard ISO format, value, which will be done for you, and then handling the transformation on the client side, if necessary.

Our company has multiple offices, and each of us develop different subsystems in different languages. We are enforcing ISO format in our newer systems since our team is the owner of our side of the project, but we need to support legacy systems that have been sending DateTimeOffsets over in a different format. Our legacy systems are very organic in nature, so unfortunately it isn't rare to have DateTimeOffsets, GUIDs, and enums all come in in different formats on different sub-objects. Another use case is that we allow clients to specify times for events in certain configuration files. Our clients are not tech savvy, so editing an ISO format timestamp is out of the question.

DateTimeOffset is just an example, though. There are other cases where we want to save multiple complex objects into a single JSON field ($ or | concatenated for example).

Most importantly, we also need to send out data, and we cannot control what standard the other side uses. It's all fine to tell me to use ISO, but the APIs I am consuming don't care what I want.

There are a few ways to work around all of this, each with their own performance impacts.

  1. You add support for private member serialization.
  2. You add support in JsonProperty to accept a JsonConverter type to use specifically on that member. This type would need to accept as an input the object being serialized and the current member so that we would be able to combine the values of multiple members to create the serialization value for that member. Similar would need to be done for deserialization.
  3. Add something similar to OnDeserializing/OnSerializing, except you pass in a context that lets us define properties that may not necessarily exist on the class. This may not work really well with deserialization due to a forward-only reader (assuming I understand this correctly).

In an ideal world it would be nice to be able to define a model that just serializes and deserializes correctly without any transformation of data, but unfortunately we do not live in an ideal world and we need to be cautious of public property bloat on our model objects.

@Clockwork-Muse
Copy link
Contributor

... 2. Can be done already via the [JsonConverter](https://docs.microsoft.com/en-us/dotnet/api/system.text.json.serialization.jsonconverterattribute?view=netcore-3.1) attribute. You'd probably want to do one for the entire type if you needed individual member serialization to be dependent on each other.

Everything still reads like a converter, though - I honestly don't see the benefit of having to keep a hidden backing field in sync, as opposed to just serializing it out to the expected form at write-time.

Another use case is that we allow clients to specify times for events in certain configuration files. Our clients are not tech savvy, so editing an ISO format timestamp is out of the question.

... If your clients can't be expected to understand and follow a simple text format, I wouldn't trust them to manually edit a config file, either. I'd give them some simple tool to write it for them.

@etreff-tillster
Copy link

@Clockwork-Muse

We have yet to migrate to 3.1, so I was not aware that was in 3.1. We will be migrating in January after the holidays, so that will be a great step to allowing us to migrate away from Json.NET without breaking changes.

I don't disagree with your points, but I want you to keep in mind that many companies are not progressive. Many companies always try to make a minimum viable product, and making a UI or tool to edit configuration for them is never part of that. I can't begin to describe how much of an uphill battle it was to get the sign off to use structured logging in addition to a rolling text file.

I understand the reluctance to implement this. I really do. It is a balance between keeping code clean, keeping it fast, keeping with good programming paradigms, and getting the widest adaptation as possible. Unfortunately the world does not move as fast as .NET Core and .NET 5 do, so in order to get the largest adaptation you are going to need to make some concessions, especially around areas that are exposed to third parties such as serialization mechanisms. I'm not posting here as a want. I'm posting here out of need. I would love nothing more than to rewrite all of our models, but it is a fight on multiple fronts with Product, Management, and our old technology. This kind of fight is not the minority.

@JandosHk
Copy link

EF Core 3.0+ materializes entities directly to backing fields. This is to avoid accidentally executing business logic in property setters, which they may have. STJ must do the same.

@layomia
Copy link
Contributor

layomia commented Jun 22, 2021

Linking #38327.

@layomia
Copy link
Contributor

layomia commented Jul 22, 2021

Triage - we didn't get to this in .NET 6.0 but can consider it in .NET 7.0.

@layomia layomia modified the milestones: 6.0.0, 7.0.0 Jul 22, 2021
@layomia
Copy link
Contributor

layomia commented Jul 22, 2021

From @swtrse in #38327:

In general, a constructor decorated with the [JsonConstructor] attribute should be able to be a non-public constructor.

This would address cases where there is a special internal constructor that is solely used for json deserializing and that should not be called from user code.

Category: Design

@WhitWaldo
Copy link

WhitWaldo commented Apr 27, 2022

I know this thread was started talking about non-public members, but since #38327 was merged into it, I've wound up here.

Just to jump in with some additional feedback on why I'd like to see this myself. We primarily use JSON to store data in intermediate serialized storage of POCOs and occasionally to simplify communication between services.

We primarily rely on guiding developers on the "right" way to instantiate a given class via the constructors. If the developer can only use these four constructors to instantiate that class coupled with some init setters, we can save ourselves a lot of downstream business logic handling the possibility that properties weren't populated as anticipated.

By making a public parameterless constructor available, even if we mark in the documentation that it's not suitable for developer consumption, it's now there in the Intellisense options and we're relying on the developers having internal object knowledge in order to set the fields properly.

Rather, as we did in Json.NET, I'd really like to see the return of a private parameterless constructor be used for deserialization. It's private, so developers cannot use it themselves (without reflection, which they're not going to use), but it's there just for System.Text.Json to utilize for this purpose.

For me at least, if the .NET 7 STJ release came with nothing but this one ask, I'd be thrilled about it.

Thank you for your continued consideration.

@OskarKlintrot
Copy link

OskarKlintrot commented Apr 27, 2022

it's now there in the Intellisense options

To hide it from intellisense you can use [EditorBrowsable(EditorBrowsableState.Never)]. Not a perfect solution but it's handy sometimes. It will only hide it though if it's a package you consume.

@WhitWaldo
Copy link

I've looked for an attribute like that to use as a workaround in the meantime. Thank you @OskarKlintrot !

@brianlagunas
Copy link

Support for private/internal constructors and properties/fields are also required for my scenario. Please implement this.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Jun 23, 2022

.NET 7 Preview 6 will include support for contract customisation, which should enable support for private field serialization for those who need it. Here is a functional test demonstrating how this can be done:

[Fact]
public static void FieldContractResolverScenario()
{
var options = new JsonSerializerOptions { TypeInfoResolver = new FieldContractResolver() };
var value = FieldContractResolver.TestClass.Create("str", 42, true);
string json = JsonSerializer.Serialize(value, options);
Assert.Equal("""{"_string":"str","_int":42,"_bool":true}""", json);
FieldContractResolver.TestClass result = JsonSerializer.Deserialize<FieldContractResolver.TestClass>(json, options);
Assert.Equal(value, result);
}
internal class FieldContractResolver : DefaultJsonTypeInfoResolver
{
public class TestClass
{
private string _string;
private int _int;
private bool _bool;
public static TestClass Create(string @string, int @int, bool @bool)
=> new TestClass { _string = @string, _int = @int, _bool = @bool };
// Should be ignored by the serializer
public bool Boolean
{
get => _bool;
set => throw new NotSupportedException();
}
public override int GetHashCode() => (_string, _int, _bool).GetHashCode();
public override bool Equals(object? other)
=> other is TestClass tc && (_string, _int, _bool) == (tc._string, tc._int, tc._bool);
}
public override JsonTypeInfo GetTypeInfo(Type type, JsonSerializerOptions options)
{
JsonTypeInfo jsonTypeInfo = base.GetTypeInfo(type, options);
if (jsonTypeInfo.Kind == JsonTypeInfoKind.Object)
{
jsonTypeInfo.Properties.Clear();
foreach (FieldInfo field in type.GetFields(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic))
{
JsonPropertyInfo jsonPropertyInfo = jsonTypeInfo.CreateJsonPropertyInfo(field.FieldType, field.Name);
jsonPropertyInfo.Get = field.GetValue;
jsonPropertyInfo.Set = field.SetValue;
jsonTypeInfo.Properties.Add(jsonPropertyInfo);
}
}
return jsonTypeInfo;
}
}

@TanvirArjel
Copy link

@eiriktsarpalis Will FieldContractResolver work for a private constructor too?

@eiriktsarpalis
Copy link
Member

It should be possible to use private parameterless constructors using the JsonTypeInfo.CreateObject delegate.

Parameterized constructor metadata has been cut from .NET 7, so it won't be possible to define custom ones.

@Romfos
Copy link

Romfos commented Jun 24, 2022

Thank you for workaround, but [JsonInclude] support would be nice for private properties :(

@TanvirArjel
Copy link

TanvirArjel commented Jun 24, 2022

@eiriktsarpalis Thanks but when will [JsonInclude] and [JsonConstructor] support for the private properties and constructors arrive as you described here #31511 (comment) ?

@eiriktsarpalis
Copy link
Member

TBH, it is unlikely we would have the time to work on this for .NET 7 at this stage.

@msynk
Copy link

msynk commented Jul 17, 2022

I think if you make the type itself non-public (internal for example) and change all of its properties to public, somehow the problem can be solved (if you want to hide the type itself and not some of its members ofc).

@eiriktsarpalis
Copy link
Member

Regarding support for internal member serialization, the release of the contract customization feature in .NET 7 Preview 6 should make it possible to achieve via a custom contract resolver. Here is a functional test demonstrating how it could be implemented:

[Fact]
public static void FieldContractResolverScenario()
{
var options = new JsonSerializerOptions { TypeInfoResolver = new FieldContractResolver() };
var value = FieldContractResolver.TestClass.Create("str", 42, true);
string json = JsonSerializer.Serialize(value, options);
Assert.Equal("""{"_string":"str","_int":42,"_bool":true}""", json);
FieldContractResolver.TestClass result = JsonSerializer.Deserialize<FieldContractResolver.TestClass>(json, options);
Assert.Equal(value, result);
}
internal class FieldContractResolver : DefaultJsonTypeInfoResolver
{
public class TestClass
{
private string _string;
private int _int;
private bool _bool;
public static TestClass Create(string @string, int @int, bool @bool)
=> new TestClass { _string = @string, _int = @int, _bool = @bool };
// Should be ignored by the serializer
public bool Boolean
{
get => _bool;
set => throw new NotSupportedException();
}
public override int GetHashCode() => (_string, _int, _bool).GetHashCode();
public override bool Equals(object? other)
=> other is TestClass tc && (_string, _int, _bool) == (tc._string, tc._int, tc._bool);
}
public override JsonTypeInfo GetTypeInfo(Type type, JsonSerializerOptions options)
{
JsonTypeInfo jsonTypeInfo = base.GetTypeInfo(type, options);
if (jsonTypeInfo.Kind == JsonTypeInfoKind.Object)
{
jsonTypeInfo.Properties.Clear();
foreach (FieldInfo field in type.GetFields(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic))
{
JsonPropertyInfo jsonPropertyInfo = jsonTypeInfo.CreateJsonPropertyInfo(field.FieldType, field.Name);
jsonPropertyInfo.Get = field.GetValue;
jsonPropertyInfo.Set = field.SetValue;
jsonTypeInfo.Properties.Add(jsonPropertyInfo);
}
}
return jsonTypeInfo;
}
}

@eiriktsarpalis eiriktsarpalis modified the milestones: 8.0.0, Future Jul 22, 2022
@eiriktsarpalis
Copy link
Member

Per my earlier comment, adding support for non-public members should be achievable via contract customization starting in .NET 7. We're not planning on adding out-of-the-box support for this, but it should still be possible for third-party extension libraries to ship contract resolvers offering this functionality.

@eiriktsarpalis eiriktsarpalis closed this as not planned Won't fix, can't repro, duplicate, stale Sep 2, 2022
@WhitWaldo
Copy link

@eiriktsarpalis Given that other threads regarding private constructions were closed in favor of this one, can you speak to whether or not support for private constructors is a possibility on any post-.NET 7 roadmaps?

@TanvirArjel
Copy link

TanvirArjel commented Sep 2, 2022

@eiriktsarpalis

Here is a sample employee class:

public class Employee
{
    private Employee()
    {
    }

    private Employee(int id, string name)
    {
       Id = id;
       Name = name;
    }

    public int Id { get; set; }

    public string Name { get; set; }

    public static Employee Create(int id, string name)
    {
        Employee employee = new Employee(id, name);
        return employee;
    }
}

Here is my serialization and deserialization code:

private static void Main(string[] args)
{
    JsonSerializerOptions options = new JsonSerializerOptions 
    { 
      TypeInfoResolver = new PrivateConstructorContractResolver() 
    };

    Employee employee = Employee.Create(1, "Tanvir");
    string json = JsonSerializer.Serialize(employee);
    Employee desrializedEmployee = JsonSerializer.Deserialize<Employee>(json, options);
}

Now as you said:

It should be possible to use private parameterless constructors using the JsonTypeInfo.CreateObject delegate.

Please write the code for the PrivateConstructorContractResolver so that the above deserialization works:

public class PrivateConstructorContractResolver : DefaultJsonTypeInfoResolver
{

}

@TanvirArjel
Copy link

@eiriktsarpalis I am eagerly waiting for your response.

@eiriktsarpalis
Copy link
Member

@eiriktsarpalis Given that other threads regarding private constructions were closed in favor of this one, can you speak to whether or not support for private constructors is a possibility on any post-.NET 7 roadmaps?

In .NET 7 it should be possible to specify private parameterless constructors via contract customization using the JsonTypeInfo.CreateObject delegate. This issue tracks parameterized delegate support via contract customization (which should also unblock private constructor support).

Please write the code for the PrivateConstructorContractResolver so that the above deserialization works:

Per my comments above, parameterized constructor support is not supported yet via the contract customization layer. It should still be possible to get your example to work using the parameterless constructor:

public class PrivateConstructorContractResolver : DefaultJsonTypeInfoResolver
{
    public override JsonTypeInfo GetTypeInfo(Type type, JsonSerializerOptions options)
    {
          JsonTypeInfo typeInfo = base.GetTypeInfo(type, options);
          if (typeInfo.Type == typeof(Employee))
          {
                typeInfo.CreateObject = () => Employee.Create(); // alternative factory calling into the parameterless ctor
          }

          return typeInfo;
    }
}

@TanvirArjel
Copy link

TanvirArjel commented Sep 2, 2022

@eiriktsarpalis What you have written here is tightly coupled to the Employee type so that I have to write contract resolver for every type. Secondly, if i can expose an public parameterless factory method then what's wrong to have public parameterless constructor? I cannot have public parameterless constructor nor factory method. Please write the contract resolver in such a way that it works for every type.

@TanvirArjel
Copy link

TanvirArjel commented Sep 2, 2022

Here is my generic contract resolver who is working fine.

public class PrivateConstructorContractResolver : DefaultJsonTypeInfoResolver
{
   public override JsonTypeInfo GetTypeInfo(Type type, JsonSerializerOptions options)
   {
       JsonTypeInfo jsonTypeInfo = base.GetTypeInfo(type, options);

       if (jsonTypeInfo.Kind == JsonTypeInfoKind.Object)
       {
         jsonTypeInfo.CreateObject = () =>
         {
            return Activator.CreateInstance(type, true);
         };
       }

       return jsonTypeInfo;
   }
}

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Sep 2, 2022

Note that a blanket assignment to CreateObject might invalidate otherwise valid JSON contracts. I would recommend scoping to the specific types you are interested in customizing, or perhaps use reflection to identify types that don't contain public constructors:

if (jsonTypeInfo.Kind == JsonTypeInfoKind.Object && jsonTypeInfo.CreateObject is null)
{
     // CreateObject can be null either because we have no public constructor or because a parameterized constructor is being used internally
     if (jsonTypeInfo.Type.GetConstructors(BindingFlags.Public | BindingFlags.Instance).Length == 0)
     {
          // The type doesn't have public constructors
          jsonTypeInfo.CreateObject = () => Activator.CreateInstance(jsonTypeInfo.Type, true);
     }
}

@Clockwork-Muse
Copy link
Contributor

.... Or writing a source generator to generate per-object registrations, especially since that would enable you to sidestep the need to call Activator.CreateInstance(...).

@ghost ghost locked as resolved and limited conversation to collaborators Oct 2, 2022
@eiriktsarpalis
Copy link
Member

FWIW the .NET 8 release will include support for opting into specific non-public members via the JsonIncludeAttribute:

#88452

Obviously this still not permit blanket inclusion of non-public members, that can only be achieved using contract customization.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Text.Json design-discussion Ongoing discussion about design without consensus
Projects
None yet
Development

No branches or pull requests