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

Json serializer code generator doesn't support internal constructors #77016

Closed
Tracked by #77019
dotMorten opened this issue Oct 13, 2022 · 10 comments
Closed
Tracked by #77019

Json serializer code generator doesn't support internal constructors #77016

dotMorten opened this issue Oct 13, 2022 · 10 comments
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@dotMorten
Copy link

dotMorten commented Oct 13, 2022

Description

If I have a public class that I want to be able to deserialize, but I don't want that class publicly constructable, I can no longer deserialize it, even though the generated deserializer code has access to the internal constructor.

This issue prevents moving from DataContractJsonSerializer to S.T.J.

Reproduction Steps

In a console app, add the following code:

using System.Text.Json;
using System.Text.Json.Serialization;

string json = "{\"name\":\"Hello World\"}";

var obj = JsonSerializer.Deserialize(json, MyDataModelSerializationContext.Default.MyDataModel);

[JsonSerializable(typeof(MyDataModel))]
[JsonSourceGenerationOptions(PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase)]
internal partial class MyDataModelSerializationContext : JsonSerializerContext
{
}

public class MyDataModel
{
    // [JsonConstructor] Won't make a difference
    internal MyDataModel() { }
    public string? Name { get; set; }
}

Expected behavior

Object deserializes.

Actual behavior

Exception thrown on deserialize:

System.NotSupportedException: 'Deserialization of types without a parameterless constructor, a singular parameterized constructor, or a parameterized constructor annotated with 'JsonConstructorAttribute' is not supported. Type 'MyDataModel'. Path: $ | LineNumber: 0 | BytePositionInLine: 1.'
image

Looking at the generated code, you'll note the ObjectCreator is null.
image
Compare that to when the constructor is declared public:
image

Related

A similar issue is hit with a data model like this:

internal class MyDataModel
{
    [JsonPropertyName("name")]
    public string? Name { get; internal set; }
}

In this case for this internal class, a setter isn't generated for the Name property and during serialization the value is never defined. Granted in this case there's no need to mark it internal since the entire class is already internal - however even if the class is public, I can't restrict the setter from public abuse and only allow the deserializer to set it.

Regression?

No response

Known Workarounds

Not really. If I make the entire class internal (and make the constructor public albeit it is effectively internal now), it works again. However I do want my class to be publicly accessible

Configuration

.net 6.0.401

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 13, 2022
@ghost
Copy link

ghost commented Oct 13, 2022

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

If I have a public class that I want to be able to deserialize, but I don't want that class publicly constructable, I can no longer deserialize it, even though the generated deserializer code has access to the internal constructor.

Reproduction Steps

In a console app, add the following code:

using System.Text.Json;
using System.Text.Json.Serialization;

string json = "{\"name\":\"Hello World\"}";

var obj = JsonSerializer.Deserialize(json, MyDataModelSerializationContext.Default.MyDataModel);

[JsonSerializable(typeof(MyDataModel))]
[JsonSourceGenerationOptions(PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase)]
internal partial class MyDataModelSerializationContext : JsonSerializerContext
{
}

public class MyDataModel
{
    // [JsonConstructor] Won't make a difference
    internal MyDataModel() { }
    public string? Name { get; set; }
}

Expected behavior

Object deserializes.

Actual behavior

Exception thrown on deserialize:

System.NotSupportedException: 'Deserialization of types without a parameterless constructor, a singular parameterized constructor, or a parameterized constructor annotated with 'JsonConstructorAttribute' is not supported. Type 'MyDataModel'. Path: $ | LineNumber: 0 | BytePositionInLine: 1.'
image

Looking at the generated code, you'll note the ObjectCreator is null.
image
Compare that to when the constructor is declared public:
image

Regression?

No response

Known Workarounds

Not really. If I make the entire class internal (and make the constructor public albeit it is effectively internal now), it works again. However I do want my class to be publicly accessible

Configuration

.net 6.0.401

Other information

No response

Author: dotMorten
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Oct 13, 2022
@layomia layomia added this to the Future milestone Oct 13, 2022
@layomia
Copy link
Contributor

layomia commented Oct 13, 2022

Triage: this can be considered for .NET 8.0.

@eiriktsarpalis
Copy link
Member

FWIW this is by design and consistent with the behavior of the reflection-based serializer. A notable exception is internal getter/setter support via the JsonIncludeAttribute -- we might consider extending this to constructors (or extend the JsonConstructorAttribute so that internal constructors can be annotated) but obviously there will be restrictions when it comes to the source generator.

@eiriktsarpalis eiriktsarpalis added the enhancement Product code improvement that does NOT require public API changes/additions label Oct 13, 2022
@dotMorten
Copy link
Author

dotMorten commented Oct 13, 2022

FWIW this is by design and consistent with the behavior of the reflection-based serializer.

I'm literally porting from the reflection based DataContractSerializer and it works just fine with these internal classes and properties. If we don't have parity I find it hard for us to move and start supporting full trimming in our assemblies. Perhaps there could at least be a setting on JsonSourceGenerationOptions to support internal members? As it stands right now, this is a blocker for us wrt being able to move.

@eiriktsarpalis
Copy link
Member

Is making the constructors public an option?

@dotMorten
Copy link
Author

No :(

@jkotas
Copy link
Member

jkotas commented Dec 2, 2022

EF Core is running into similar set of issues with source-generating equivalent of private reflection. Once you start looking at this, we should talk about the possible solutions.

@eiriktsarpalis
Copy link
Member

What might be a way to offer a source-generating equivalent of private reflection? I suppose it might be possible to generate member accessor stubs inside the type declaration itself provided it lives in the same project, but is there any other way you have in mind?

@jkotas
Copy link
Member

jkotas commented Dec 3, 2022

What might be a way to offer a source-generating equivalent of private reflection?

One extreme is to make JIT pattern-match reflection calls with constant inputs and replace them with direct invocations. For example, if you wrote typeof(MyType).GetMethod("MyMethod", BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance, new Type[] { typeof(string), typeof(int) }).Invoke(thisArg, new object[] { stringArg, intArg }), the JIT can replace it with thisArg.MyMethod(stringArg, intArg) in theory. This would be a very complex and fragile pattern match. I do not think we want to do that.

Other extreme is to emit IL (either via an extra IL .dll or via IL rewriting). It is possible to suppress visibility checks in IL today using IgnoresAccessChecksToAttribute. https://gist.github.com/jkotas/ddfeb9ec47bff90b4249534c4cd0761a shows a small demo using Fody. Extra IL file or IL rewriting comes with a bunch of problems that are very hard to solve (e.g. IL rewriting breaks edit-and-continue). I do not think we want to do that.

So the idea is that we may be able to come up with something viable (e.g. new runtime API) between these two extremes.

@eiriktsarpalis
Copy link
Member

Closing in favor of the more general user story tracked in #87431

@ghost ghost locked as resolved and limited conversation to collaborators Jul 13, 2023
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
Projects
None yet
Development

No branches or pull requests

5 participants