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

Improve feature parity between JsonSerializerOptions and JsonSourceGenerationOptionsAttribute #57321

Closed
ithline opened this issue Aug 12, 2021 · 10 comments · Fixed by #88753
Closed
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json blocking Marks issues that we want to fast track in order to unblock other important work partner-impact This issue impacts a partner who needs to be kept updated source-generator Indicates an issue with a source generator feature
Milestone

Comments

@ithline
Copy link

ithline commented Aug 12, 2021

EDIT See #57321 (comment) for an API proposal.

Original Proposal

Currently source generator for JSON serialization in `System.Text.Json` creates a static property named `Default` that receives default `JsonSerializerOptions` defined by the source generator. These can be modified by using `JsonSourceGenerationOptionsAttribute`, but several options are missing and cannot be set, e.g. `PropertyNameCaseInsensitive`, `NumberHandling`, `DictionaryKeyPolicy`...

It would be really helpful if these missing options could be added to the JsonSourceGenerationOptionsAttribute or at least disable generating of the Default property, so we could provide our own configuration for the default instance.

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Aug 12, 2021
@ghost
Copy link

ghost commented Aug 12, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Currently source generator for JSON serialization in System.Text.Json creates a static property named Default that receives default JsonSerializerOptions defined by the source generator. These can be modified by using JsonSourceGenerationOptionsAttribute, but several options are missing and cannot be set, e.g. PropertyNameCaseInsensitive, NumberHandling, DictionaryKeyPolicy...

It would be really helpful if these missing options could be added to the JsonSourceGenerationOptionsAttribute or at least disable generating of the Default property, so we could provide our own configuration for the default instance.

Author: Ithline
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@layomia
Copy link
Contributor

layomia commented Aug 12, 2021

Have you tried using the generated ctor on your context type that takes a JsonSerializerOptions instance?

e.g.

[JsonSerializable(typeof(Foo))]
internal partial sealed class MyContext : JsonSerializerContext { }

JsonSerializerOptions options = new() { ... };
MyContext context = new(options);

JsonSerializer.Serialize(foo, context.Foo);

@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Aug 12, 2021
@layomia layomia added this to the 6.0.0 milestone Aug 12, 2021
@ithline
Copy link
Author

ithline commented Aug 13, 2021

Yes, that can be used as a workaround, but the generator still forces public API of my custom JsonSerializerContext type. I work with a third party API that needs some specific options set to work and having a Default property with basically invalid settings is not a good design. Everyone on the team would need to be aware not to use this property but instead use some other instance.

Ideally even the constructors would not be automatically generated and left to the developer.
I use need metadata generation to get rid of trimming warnings and don't need the sped up pre-generated serializer.

@eiriktsarpalis
Copy link
Member

@layomia given that there's a workaround, does this meet the bar for 6.0.0?

@eiriktsarpalis eiriktsarpalis added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Aug 16, 2021
@eiriktsarpalis eiriktsarpalis modified the milestones: 6.0.0, 7.0.0 Aug 16, 2021
@eiriktsarpalis
Copy link
Member

Moving to 7.0.0, we need to think about API design for this particular use case and we have a 6.0 workaround via the generated constructors.

@layomia layomia added the source-generator Indicates an issue with a source generator feature label Sep 22, 2021
@layomia layomia modified the milestones: 7.0.0, Future Jan 22, 2022
@zachcwillson
Copy link

Hey folks,

Would love to see this added so we can avoid [JsonPropertyName] attributes that are required for deserializing

{
    "property": "x"
}

into

public sealed record Poco
{
    public string Property { get; set; }
}

Without needing to do any additional work.

Currently we have to:

public sealed record Poco
{
    [JsonPropertyName("property")]
    public string Property { get; set; }
}

@eiriktsarpalis
Copy link
Member

Have you tried using the JsonSourceGenerationOptionsAttribute?

string json = JsonSerializer.Serialize(new Poco { Property = "prop" }, MyContext.Default.Poco);
Console.WriteLine(json); // {"property":"prop"}

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

public sealed record Poco
{
    public string Property { get; set; }
}

@zachcwillson
Copy link

zachcwillson commented May 23, 2023

Deleted my previous comment, I got it working with JsonKnownNamingPolicy.CamelCase. However, it would be great to be also able to specify other options.

@eiriktsarpalis eiriktsarpalis changed the title Allow specifying custom JsonSerializerOptions for Default instance of generated JsonSerializerContext Add more JsonSerializerOptions settings to JsonSourceGenerationOptionsAttribute Jun 15, 2023
@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Jul 7, 2023

Background and Motivation

We should close the gap between JsonSerializerOptions and JsonSourceGenerationOptionsAttribute so that users can configure the Default JsonSerializerContext without needing to create instances from scratch.

Currently specifying options beyond what is offered by JsonSourceGenerationOptionsAttribute requires boilerplate:

// can't use ContextWithAllOptionsSet.Default, need to use a custom instance intead
var options = new JsonSourceGenerationOptions(JsonSerializerDefaults.Web) { WriteIndented = true };
var customContext = new ContextWithAllOptionsSet(options);

[JsonSerializable(typeof(PersonStruct))]
public partial class ContextWithAllOptionsSet : JsonSerializerContext
{ }

API Proposal

The proposed new properties/constructors are taken directly from the JsonSerializerOptions class.

namespace System.Text.Json.Serialization;

public partial class JsonSourceGenerationOptionsAttribute
{
    public JsonSourceGenerationOptionsAttribute();
+   public JsonSourceGenerationOptionsAttribute(JsonSerializerDefaults defaults);

+   public bool AllowTrailingCommas { get; set; }
+   public Type[]? Converters { get; set; }
+   public int DefaultBufferSize { get; set; }
    public JsonIgnoreCondition DefaultIgnoreCondition { get; set; }
+   public JsonKnownNamingPolicy DictionaryKeyPolicy { get; set; }
    public bool IgnoreReadOnlyFields { get; set; }
    public bool IgnoreReadOnlyProperties { get; set; }
    public bool IncludeFields { get; set; }
+   public int MaxDepth { get; set; }
+   public JsonNumberHandling NumberHandling { get; set; }
+   public JsonObjectCreationHandling PreferredObjectCreationHandling { get; set; }
+   public bool PropertyNameCaseInsensitive { get; set; }
    public JsonKnownNamingPolicy PropertyNamingPolicy { get; set; }
+   public JsonCommentHandling ReadCommentHandling { get; set; }
+   public JsonUnknownTypeHandling UnknownTypeHandling { get; set; }
+   public JsonUnmappedMemberHandling UnmappedMemberHandling { get; set; }
    public bool WriteIndented { get; set; }
}

Note that the above leaves out the Encoder and ReferenceHandler properties from JsonSerializerOptions as they are reference types and cannot be specified on attribute annotations. We can consider adding support for these in the future: common values can be represented via proxy enum types as is the case of JsonKnownNamingPolicy being used instead of the JsonNamingPolicy class.

API Usage

The following declaration

[JsonSourceGenerationOptions(JsonSerializerDefaults.Web,
    AllowTrailingCommas = true,
    Converters = new[] { typeof(JsonStringEnumConverter<BindingFlags>), typeof(JsonStringEnumConverter<JsonIgnoreCondition>) },
    DefaultBufferSize = 128,
    DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingDefault,
    DictionaryKeyPolicy = JsonKnownNamingPolicy.SnakeCaseUpper,
    IgnoreReadOnlyFields = true,
    IgnoreReadOnlyProperties = true,
    IncludeFields = true,
    MaxDepth = 1024,
    NumberHandling = JsonNumberHandling.WriteAsString,
    PreferredObjectCreationHandling = JsonObjectCreationHandling.Replace,
    PropertyNameCaseInsensitive = true,
    PropertyNamingPolicy = JsonKnownNamingPolicy.KebabCaseUpper,
    ReadCommentHandling = JsonCommentHandling.Skip,
    UnknownTypeHandling = JsonUnknownTypeHandling.JsonNode,
    UnmappedMemberHandling = JsonUnmappedMemberHandling.Disallow,
    WriteIndented = true)]
[JsonSerializable(typeof(PersonStruct))]
public partial class ContextWithAllOptionsSet : JsonSerializerContext
{ }

Will result in the following default options instance being generated

private readonly static global::System.Text.Json.JsonSerializerOptions s_defaultOptions = new(global::System.Text.Json.JsonSerializerDefaults.Web)
{
    AllowTrailingCommas = true,
    Converters =
    {
        new global::System.Text.Json.Serialization.JsonStringEnumConverter<global::System.Reflection.BindingFlags>(),
        new global::System.Text.Json.Serialization.JsonStringEnumConverter<global::System.Text.Json.Serialization.JsonIgnoreCondition>(),
    },
    DefaultBufferSize = 128,
    DefaultIgnoreCondition = global::System.Text.Json.Serialization.JsonIgnoreCondition.WhenWritingDefault,
    DictionaryKeyPolicy = global::System.Text.Json.JsonNamingPolicy.SnakeCaseUpper,
    IgnoreReadOnlyFields = true,
    IgnoreReadOnlyProperties = true,
    IncludeFields = true,
    MaxDepth = 1024,
    NumberHandling = global::System.Text.Json.Serialization.JsonNumberHandling.WriteAsString,
    PreferredObjectCreationHandling = global::System.Text.Json.Serialization.JsonObjectCreationHandling.Replace,
    PropertyNameCaseInsensitive = true,
    PropertyNamingPolicy = global::System.Text.Json.JsonNamingPolicy.KebabCaseUpper,
    ReadCommentHandling = global::System.Text.Json.JsonCommentHandling.Skip,
    UnknownTypeHandling = global::System.Text.Json.Serialization.JsonUnknownTypeHandling.JsonNode,
    UnmappedMemberHandling = global::System.Text.Json.Serialization.JsonUnmappedMemberHandling.Disallow,
    WriteIndented = true,
};

@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 8.0.0 Jul 7, 2023
@eiriktsarpalis eiriktsarpalis added blocking Marks issues that we want to fast track in order to unblock other important work 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 Jul 7, 2023
@eiriktsarpalis eiriktsarpalis added the partner-impact This issue impacts a partner who needs to be kept updated label Jul 7, 2023
@eiriktsarpalis eiriktsarpalis changed the title Add more JsonSerializerOptions settings to JsonSourceGenerationOptionsAttribute Improve feature parity between JsonSerializerOptions and JsonSourceGenerationOptionsAttribute Jul 7, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 12, 2023
@terrajobst
Copy link
Member

terrajobst commented Jul 13, 2023

Video

  • We considered exposing the constructor parameter as a property but we don't believe anyone should need that
namespace System.Text.Json.Serialization;

public partial class JsonSourceGenerationOptionsAttribute
{
    public JsonSourceGenerationOptionsAttribute();
+   public JsonSourceGenerationOptionsAttribute(JsonSerializerDefaults defaults);

+   public bool AllowTrailingCommas { get; set; }
+   public Type[]? Converters { get; set; }
+   public int DefaultBufferSize { get; set; }
    public JsonIgnoreCondition DefaultIgnoreCondition { get; set; }
+   public JsonKnownNamingPolicy DictionaryKeyPolicy { get; set; }
    public bool IgnoreReadOnlyFields { get; set; }
    public bool IgnoreReadOnlyProperties { get; set; }
    public bool IncludeFields { get; set; }
+   public int MaxDepth { get; set; }
+   public JsonNumberHandling NumberHandling { get; set; }
+   public JsonObjectCreationHandling PreferredObjectCreationHandling { get; set; }
+   public bool PropertyNameCaseInsensitive { get; set; }
    public JsonKnownNamingPolicy PropertyNamingPolicy { get; set; }
+   public JsonCommentHandling ReadCommentHandling { get; set; }
+   public JsonUnknownTypeHandling UnknownTypeHandling { get; set; }
+   public JsonUnmappedMemberHandling UnmappedMemberHandling { get; set; }
    public bool WriteIndented { 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 Jul 13, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 14, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2023
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 blocking Marks issues that we want to fast track in order to unblock other important work partner-impact This issue impacts a partner who needs to be kept updated source-generator Indicates an issue with a source generator feature
Projects
None yet
5 participants