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 private setters as an opt-in feature #29743

Closed
YohDeadfall opened this issue Jun 3, 2019 · 22 comments · Fixed by #34675
Closed

JsonSerializer should support private setters as an opt-in feature #29743

YohDeadfall opened this issue Jun 3, 2019 · 22 comments · Fixed by #34675
Assignees
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions json-functionality-doc Missing JSON specific functionality that needs documenting
Milestone

Comments

@YohDeadfall
Copy link
Contributor

YohDeadfall commented Jun 3, 2019

Updated by @layomia:
There's a lot of overlap between this feature and field support wrt. public API proposal. See #34558 for combined API proposal and discussion.


Non-public setters for public properties are not used when deserializing. Other serializers have this feature. As a workaround, user's have to write custom converter's for such POCOs, which isn't trivial in case of a complex object. This feature provides an opt-in for the serializer to use non-public setters.

Enabling non-public getter usage is included for parity with Newtonsoft.Json which supports this when the JsonProperty attribute is used, and to prevent complicating the API surface if this is ever desired in the future.

This feature was scoped out of v1 (3.x), as the objective was to support simple POCOs. We elected to make this feature opt-in due to security concerns with non-public support.

This feature does not include support for non-public properties.

A related feature that allows deserialization of immutable objects through parameterized constructors was recently added in #33444.

@mungojam
Copy link

Or add support for parameters in constructors which Json.Net supports. That would then support immutable classes without the need to have any setters, private or not

@drmathias
Copy link

Deserialisation with constructor parameters would cover both readonly properties and properties with private setters. Private setter deserialisation would be nice too, especially to avoid writing constructors with massive parameter lists.

@devhl-labs
Copy link

Can we consider making this a 3.0 milestone? A library with private or internal setters can't deserialize anything without it. If you deserialize something from a source which you do not control, it makes sense to have private or internal setters.

@Yucked
Copy link

Yucked commented Jul 28, 2019

I spent couple weeks trying to debug everything on my side just to be led to this issue. Any ETA on this?

@JosepBalague
Copy link

Failing in 4.6.0-preview8.19405.3 with internal setters:

[JsonPropertyName("FolderName")] public string FolderName { get; internal set; }

Works:
[JsonPropertyName("FolderName")] public string FolderName { get; set; }

Cheers

@devhl-labs
Copy link

devhl-labs commented Aug 22, 2019 via email

@Daniel-Glucksman
Copy link

Me too!

@silkfire
Copy link

@steveharter @ahsonkhan Any more info on this issue? Milestone 3.1 perhaps?

@daniel-white
Copy link

I'd be happy to write this and add a PR. Love immutable request objects.

@imperugo
Copy link

imperugo commented Oct 30, 2019

Immutable is a plus, but IMHO it must me reached without attributes.

use [JsonPropertyName("FolderName")] doesn't look a good option to me.

@layomia

This comment has been minimized.

@chrisdpratt
Copy link

As far as I'm concerned, this makes System.Text.Json unusable in its current state. Thankfully, we can keep using JSON.NET, but I just wanted to give this a bump, and really encourage this be made a top development priority.

@ahsonkhan
Copy link
Member

Moving to 5.0.

@AlexeiScherbakov
Copy link

Please add also list properties (with type List) and only getter

@johncrim
Copy link

johncrim commented Nov 22, 2019

As I commented on dotnet/corefx#42515 - the feature to deserialize into existing sub-objects (which I described in dotnet/corefx#42515) is not the same as supporting private setters. I think the ability to deserialize into existing sub-objects is more important than private setters, but both features are important.

The desired behavior with dotnet/corefx#42515 is that the existing sub-object be deserialized into (the sub-object is obtained from the getter), instead of requiring that the deserializer create a new sub-object, deserialize into it, and then pass it into the parent object setter (whether private or internal, non-existent should also work).

Doing it this way allows developers to set default values before deserializing, to handle values that aren't present in the JSON. It also allows developers to create less mutable objects, and use non-nullable reference types for properties that should never be null.

oschwald referenced this issue in maxmind/GeoIP2-dotnet Nov 22, 2019
This was abandoned for the time being due to the requirement that all
of the setters be made public. Related issues upstream:

https://github.com/dotnet/corefx/issues/38163
https://github.com/dotnet/corefx/issues/40399

The second would be preferable.
TechPreacher referenced this issue in TechPreacher/IotHubSynchronizer Dec 10, 2019
This branch is currently blocked by the JsonSerializer not being able to deserialize into objects with private setters. (https://github.com/dotnet/corefx/issues/38163)
@ForeverZer0
Copy link

Completely agree with @chrisdpratt about the library being rather unusable in many regards due to this lack of a feature(s). Honestly, I am a bit surprised that this was included in a release with this basic feature missing, perhaps due to deadlines and time constraints. The API looks very promising, look forward to giving it another try when it is more complete, though do a feel a bit annoyed that it is being touted as a viable replacement of existing libraries in its current state.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@bnayae
Copy link

bnayae commented Mar 16, 2020

work around:
use Obsolete attribute in order to prevent direct code access (serialization is using reflection therefore ignore it)

    private string _microServiceVersion = string.Empty;
    public string MicroServiceVersion
    {
        get => _microServiceVersion;
        [Obsolete("for serialization", true)]
        set => _microServiceVersion = value; 
    }

@chrisdpratt
Copy link

@bnayae That's not a workaround. The point is to not expose a setter at all.

About the best "workaround" I've found for this is using interfaces. All my public methods return the interface, which only exposes a getter, and then the actual implementation has a setter for the sake of serialization. It doesn't stop someone from casting to the concrete type to access the setter, but using an interface is at least explicit that it should be treated as readonly.

@Felk
Copy link

Felk commented Jun 25, 2020

Will this also work for init-only properties?

I could not get init properties to work with the .NET 5 preview 6, so I assumed that they simply aren't supported yet in the preview build. Therefore unfortunately I was unable to test how System.Text.Json interacts with them myself.

@montyclt
Copy link

This will support init modifier of C# 9?

@steveharter
Copy link
Member

steveharter commented Aug 4, 2020

I could not get init properties to work with the .NET 5 preview 6, so I assumed that they simply aren't supported yet in the preview build. Therefore unfortunately I was unable to test how System.Text.Json interacts with them myself.

Are you referring to using a record type and having the serializer calling the appropriate constructor? If so, that was fixed in preview 8 via #38539.

If you just are using the init modifier directly on a property (not a record type) that should have worked in preview 6 since the runtime doesn't prevent reflection or IL Emit'd code from calling the setter AFAIK.

@Felk
Copy link

Felk commented Aug 4, 2020

If you just are using the init modifier directly on a property (not a record type) that should have worked in preview 6 since the runtime doesn't prevent reflection or IL Emit'd code from calling the setter AFAIK.

I was running into this issue which has since been resolved: https://stackoverflow.com/q/62648189/3688648

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions json-functionality-doc Missing JSON specific functionality that needs documenting
Projects
None yet
Development

Successfully merging a pull request may close this issue.