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]: Allow C# models to be decorated with an attribute to indicate required code generation, not JsonSerializerContext #73297

Open
mikegoatly opened this issue Aug 3, 2022 · 3 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json source-generator Indicates an issue with a source generator feature
Milestone

Comments

@mikegoatly
Copy link

Background and motivation

I've been looking at enabling trimming for some of our projects now we've migrated to .NET 6, and have found the current design for configuring the supported types on a JsonSerializerContext to be quite cumbersome.

Some of our projects have a reasonable, though not excessive, number of models that can be both serialized and deserialized from a third party API. This could be a single instance of the entity, or a list, which means that for every model we want to work with, we need to remember to decorate our context class as:

[JsonSerializable(typeof(Model1))]
[JsonSerializable(typeof(IList<Model1>))]
[JsonSerializable(typeof(Model2))]
[JsonSerializable(typeof(IList<Model2>))]
[JsonSerializable(typeof(Model3))]
[JsonSerializable(typeof(IList<Model3>))]
internal partial class ThirdPartyModelSerializationContext : JsonSerializerContext
{
}

The number of interaction points we have with the API will undoubtedly grow, and every time a new model is required, it's necessary to remember that there's another class somewhere that needs to have its list of attributes updated.

It the model class itself could be decorated with an attribute indicating the context that it needs to be part of, it would be much easier to remember, e.g.:

internal partial class ThirdPartyModelSerializationContext : JsonSerializerContext
{
}

[JsonSerializerContext(typeof(ThirdPartyModelSerializationContext)]
public class Model1
{
   /// ...
}

API Proposal

public sealed class JsonSerializerContextAttribute : JsonAttribute
{
    public JsonSerializerContextAttribute(Type jsonSerializerContextType, params Type[] additionalRootTypes);
}

API Usage

// Use the existing approach for the edge case where a model must *only* be serializable as part of a list and 
// serialization should throw an error if someone tries to serialize an entity outside of a list type
[JsonSerializable(typeof(IList<Model3>))]
internal partial class ThirdPartyModelSerializationContext : JsonSerializerContext
{
}

// A model that only is only ever (de)serialized as itself
[JsonSerializerContext(typeof(ThirdPartyModelSerializationContext)]
public class Model1
{
   /// ...
}

// A model that could be deserialized as itself or a list
[JsonSerializerContext(typeof(ThirdPartyModelSerializationContext), typeof(IList<Model2>)]
public class Model2
{
   /// ...
}

Alternative Designs

No response

Risks

No response

@mikegoatly mikegoatly added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Aug 3, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 3, 2022
@ghost
Copy link

ghost commented Aug 3, 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

Background and motivation

I've been looking at enabling trimming for some of our projects now we've migrated to .NET 6, and have found the current design for configuring the supported types on a JsonSerializerContext to be quite cumbersome.

Some of our projects have a reasonable, though not excessive, number of models that can be both serialized and deserialized from a third party API. This could be a single instance of the entity, or a list, which means that for every model we want to work with, we need to remember to decorate our context class as:

[JsonSerializable(typeof(Model1))]
[JsonSerializable(typeof(IList<Model1>))]
[JsonSerializable(typeof(Model2))]
[JsonSerializable(typeof(IList<Model2>))]
[JsonSerializable(typeof(Model3))]
[JsonSerializable(typeof(IList<Model3>))]
internal partial class ThirdPartyModelSerializationContext : JsonSerializerContext
{
}

The number of interaction points we have with the API will undoubtedly grow, and every time a new model is required, it's necessary to remember that there's another class somewhere that needs to have its list of attributes updated.

It the model class itself could be decorated with an attribute indicating the context that it needs to be part of, it would be much easier to remember, e.g.:

internal partial class ThirdPartyModelSerializationContext : JsonSerializerContext
{
}

[JsonSerializerContext(typeof(ThirdPartyModelSerializationContext)]
public class Model1
{
   /// ...
}

API Proposal

public sealed class JsonSerializerContextAttribute : JsonAttribute
{
    public JsonSerializerContextAttribute(Type jsonSerializerContextType, params Type[] additionalRootTypes);
}

API Usage

// Use the existing approach for the edge case where a model must *only* be serializable as part of a list and 
// serialization should throw an error if someone tries to serialize an entity outside of a list type
[JsonSerializable(typeof(IList<Model3>))]
internal partial class ThirdPartyModelSerializationContext : JsonSerializerContext
{
}

// A model that only is only ever (de)serialized as itself
[JsonSerializerContext(typeof(ThirdPartyModelSerializationContext)]
public class Model1
{
   /// ...
}

// A model that could be deserialized as itself or a list
[JsonSerializerContext(typeof(ThirdPartyModelSerializationContext), typeof(IList<Model2>)]
public class Model2
{
   /// ...
}

Alternative Designs

No response

Risks

No response

Author: mikegoatly
Assignees: -
Labels:

api-suggestion, area-System.Text.Json

Milestone: -

@eiriktsarpalis
Copy link
Member

It might be something we might consider, that being said there are a couple of drawbacks with that approach:

  • The new attribute makes it possible to point to contexts not in the current project, which a source generator cannot support. It shouldn't be a huge issue assuming we detect this and provide a user friendly diagnostic explaining why this cannot work.
  • It might negatively impact performance of the source generator, since it will need to look for and aggregate attributes from more types than just JsonSerializerContext.

cc @layomia @CyrusNajmabadi

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Aug 3, 2022
@eiriktsarpalis eiriktsarpalis added this to the 8.0.0 milestone Aug 3, 2022
@eiriktsarpalis eiriktsarpalis added the source-generator Indicates an issue with a source generator feature label Aug 3, 2022
@Sergio0694
Copy link
Contributor

"It might negatively impact performance of the source generator, since it will need to look for and aggregate attributes from more types"

This can still be quite efficient on Roslyn 4.0 already (use CreateSyntaxProvider (example) filtering types with at least one attribute, then work on filtering those out), but especially with recent Roslyn versions there's now a new API specifically to look attributes up by fully qualified name in a very very fast way, see dotnet/roslyn#54725 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

No branches or pull requests

3 participants