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

[API Proposal]: JsonSourceGenerationOptions should support ReferenceHandler #107597

Open
petro2050 opened this issue Sep 10, 2024 · 11 comments
Open
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@petro2050
Copy link

petro2050 commented Sep 10, 2024

Background and motivation

Provide a way to specify a reference handler using compile-time configuration.

API Proposal

The ReferenceHandler type is a class and therefore it cannot be specified in literal annotations. If we wanted to add support for that it would require introducing a proxy enum type, in the style of JsonKnownEnumPolicy. However that would leave out support for user-defined reference handlers in case that matters:

namespace System.Text.Json.Serialization;

public enum KnownReferenceHandler // Naming follows the ReferenceHandler type which misses a Json prefix
{ 
     IgnoreCycles = 1,
     Preserve = 2,
     Unspecified = 0,
}

public partial class JsonSourceGenerationOptionsAttribute
{
    public JsonKnownReferenceHandler ReferenceHandler { get; set; }
}

API Usage

[JsonSourceGenerationOptions(WriteIndented = true, ReferenceHandler = KnownReferenceHandler.IgnoreCycles)]
[JsonSerializable(typeof(WeatherForecast))]
partial class SourceGenerationContext : JsonSerializerContext;

Alternative Designs

No response

Risks

No response

@petro2050 petro2050 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Sep 10, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Sep 10, 2024
Copy link
Contributor

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

@eiriktsarpalis
Copy link
Member

The ReferenceHandler type is a class and therefore it cannot be specified in literal annotations. If we wanted to add support for that it would required introducing a proxy enum type, in the style of JsonKnownEnumPolicy. However that would leave out support for user-defined reference handlers in case that matters:

namespace System.Text.Json.Serialization;

public enum JsonKnownReferenceHandler 
{ 
     IgnoreCycles = 1,
     Preserve = 2,
     Unspecified = 0,
}

public partial class JsonSourceGenerationOptionsAttribute
{
    public JsonKnownReferenceHandler ReferenceHandler { get; set; }
}

Would that cover your use case?

@huoyaoyuan
Copy link
Member

huoyaoyuan commented Sep 10, 2024

User-defined handler type could be specified with typeof in attributes.

BTW, is it actionable to support user-defined naming style in SG mode? Since generated metadata needs to materialize the type info, the serialized field names can no longer be generated as constants.

@eiriktsarpalis
Copy link
Member

User-defined handler type could be specified with typeof in attributes.

The implementation types are not public I'm afraid: https://learn.microsoft.com/en-us/dotnet/api/system.text.json.serialization.referencehandler?view=net-8.0

BTW, is it actionable to support user-defined naming style in SG mode? Since generated metadata needs to materialize the type info, the serialized field names can no longer be generated as constants.

I'm not sure I follow the question, could you give an example?

@petro2050
Copy link
Author

@eiriktsarpalis JsonKnownReferenceHandler would cover my case.

Just wondering if this can be prioritized. I'm trying to switch to AOT, but Serialize<TValue>(TValue value, JsonSerializerOptions? options = null) (which I'm using today) requires unreferenced/dynamic code, so I'd like to pass a JsonTypeInfo<T> to a shared serializer project that gets referenced by multiple other projects. In other words, I'm looking for a generic solution. I'd also like to avoid passing the context itself since JsonTypeInfo<T> provides type safety.

definition:
public static string Serialize<T>(T value, JsonTypeInfo<T> typeInfo);

usage:
JsonHelper.Serialize(car, ProjectXContext.Default.Car);

@huoyaoyuan
Copy link
Member

BTW, is it actionable to support user-defined naming style in SG mode? Since generated metadata needs to materialize the type info, the serialized field names can no longer be generated as constants.

I'm not sure I follow the question, could you give an example?

For this code snippet:

public class MyType
{
    public string SomeProperty { get; set; }
}

[JsonSourceGenerationOptions(PropertyNamingPolicy = JsonKnownNamingPolicy.SnakeCaseLower)]
[JsonSerializable(typeof(MyType))]
public partial class MyContext : JsonSerializerContext { }

Currently there will be PropName_some_property = JsonEncodedText.Encode("some_property"); generated, which executes the known naming policy at compile time. However for custom naming policy, it can't be executed so.

It was desired because snake case wasn't even a built-in naming policy in previous versions.

@eiriktsarpalis
Copy link
Member

Just wondering if this can be prioritized.

It's still possible to use reference handling with the source generator, but it requires slightly more boilerplate. This works with Native AOT:

JsonSerializer.Serialize(new MyPoco("John", 30), MyContext.Custom.MyPoco); // {"$id":"1","Name":"John","Age":30}

record MyPoco(string Name, int Age);

[JsonSerializable(typeof(MyPoco))]
partial class MyContext : JsonSerializerContext
{
    public static MyContext Custom { get; } = new(new JsonSerializerOptions 
    { 
        ReferenceHandler = ReferenceHandler.Preserve,
        WriteIndented = true,
    });
}

I'm not sure I follow the question, could you give an example?

For this code snippet:

I see, the naming policy is run at compile time as a performance optimization for the fast-path serializer but it doesn't need to do so. Assuming we had an API where either the naming policies or reference handlers were exposed as named types (they are not today) we might conceivably have an API that lets users nominate their own implementations (as is the case with the JsonSourceGenerationOptionsAttribute.Converters property today), but I'm not super convinced we want to do this.

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Sep 11, 2024
@eiriktsarpalis eiriktsarpalis added this to the Future milestone Sep 11, 2024
@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Sep 11, 2024
@eiriktsarpalis eiriktsarpalis self-assigned this Sep 11, 2024
@petro2050
Copy link
Author

Thanks for the code example. I went with your recommendation to unblock myself for now. It'd still be great to override the default behavior in the future (through JsonSourceGenerationOptions) instead of going through a custom implementation.

@terrajobst
Copy link
Member

terrajobst commented Sep 17, 2024

Video

  • Looks good as proposed
namespace System.Text.Json.Serialization;

public enum KnownReferenceHandler // Naming follows the ReferenceHandler type which misses a Json prefix
{ 
    IgnoreCycles = 1,
    Preserve = 2,
    Unspecified = 0,
}

public partial class JsonSourceGenerationOptionsAttribute
{
    public KnownReferenceHandler ReferenceHandler { get; set; }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Sep 17, 2024
@eiriktsarpalis eiriktsarpalis added the help wanted [up-for-grabs] Good issue for external contributors label Sep 17, 2024
@PranavSenthilnathan
Copy link
Member

I created a PR with the approved api shape, but adding this option seems like it's setting a precedent to keep adding more options to JsonSourceGenerationOptionsAttribute. That approach makes sense for JsonKnownNamingPolicy because the codegen that STJ does uses this value, so it needs to be a compile time constant. For example:

    internal partial class SourceGenerationContext
    {
        private static readonly global::System.Text.Json.JsonEncodedText EncodedPropName_6D792D737472696E67 = global::System.Text.Json.JsonEncodedText.Encode("my-property-name");
    }

The reference handler also has codegen, but it's very minor - specifically it is just used to construct the object initializer for the default options:

        [global::System.CodeDom.Compiler.GeneratedCodeAttribute("System.Text.Json.SourceGeneration", "42.42.42.42")]
        internal partial class ContextWithPreserveReference
        {
            private readonly static global::System.Text.Json.JsonSerializerOptions s_defaultOptions = new()
            {
                ReferenceHandler = global::System.Text.Json.Serialization.ReferenceHandler.Preserve,
            };
            ...
            public static global::System.Text.Json.SourceGeneration.Tests.JsonSerializerContextTests.ContextWithPreserveReference Default { get; } = new global::System.Text.Json.SourceGeneration.Tests.JsonSerializerContextTests.ContextWithPreserveReference(new global::System.Text.Json.JsonSerializerOptions(s_defaultOptions));
            ...
        }

The default option doesn't need to be a compile time constant - it just needs to be instantiated before the context singleton is created. So an alternative to the approved API you could consider is to provide an extension point to allow the user to specify their default options. I'm sure there are a few options to do this, but maybe something like this would work (using static partial here since default options and singleton are static):

    static partial void UpdateDefaults(JsonSerializerOptions defaultOptions);

The idea being that the user can implement this if they want by mutating the default options parameter and we use the mutated value to create the singleton context.

@eiriktsarpalis
Copy link
Member

The default option doesn't need to be a compile time constant - it just needs to be instantiated before the context singleton is created. So an alternative to the approved API you could consider is to provide an extension point to allow the user to specify their default options.

Don't the existing APIs of JsonSerializerOptions (specifically its copy constructor) already provide equivalent functionality? The scope of this issue specifically is providing compile time configuration support.

@eiriktsarpalis eiriktsarpalis removed their assignment Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

5 participants