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

Adding Json aot/trimmer-safe overloads to [Typed]Results #46252

Closed
brunolins16 opened this issue Jan 24, 2023 · 7 comments · Fixed by #46008
Closed

Adding Json aot/trimmer-safe overloads to [Typed]Results #46252

brunolins16 opened this issue Jan 24, 2023 · 7 comments · Fixed by #46008
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Milestone

Comments

@brunolins16
Copy link
Member

brunolins16 commented Jan 24, 2023

Background and Motivation

Both JsonSerializer and Http[Request/Response]Extensions provide AOT/Trimmer-safe overloads, with JsonTypeInfo or JsonSerializerContext, however, when producing a Json response using the static [Typed]Results class only overloads that take JsonSerializerOptions are available.

My suggestion is introduced new overload, following the same available in the JsonSerializer and Http[Request/Response]Extensions and mark the current overload with RUC/RDC attributes.

Proposed API

namespace Microsoft.AspNetCore.Http;

public static class TypedResults
{
     public static JsonHttpResult<TValue> Json<TValue>(TValue? data, JsonSerializerOptions? options = null, string? contentType = null, int? statusCode = null)
+    public static JsonHttpResult<TValue> Json<TValue>(TValue? data, JsonSerializerContext jsonContext, string? contentType = null, int? statusCode = null) {}
+    public static JsonHttpResult<TValue> Json<TValue>(TValue? data, JsonTypeInfo jsonTypeInfo, string? contentType = null, int? statusCode = null)
}

public static class Results
{
     public static IResult Json<TValue>(TValue? data, JsonSerializerOptions? options = null, string? contentType = null, int? statusCode = null)
+    public static IResult Json<TValue>(TValue? data, JsonSerializerContext jsonSerializerContext, string? contentType = null, int? statusCode = null) {}
+    public static IResult Json<TValue>(TValue? data, JsonTypeInfo jsonTypeInfo, string? contentType = null, int? statusCode = null){}

     public static IResult Json(object? data, JsonSerializerOptions? options = null, string? contentType = null, int? statusCode = null)
+    public static IResult Json(object? data, JsonSerializerContext jsonSerializerContext, string? contentType = null, int? statusCode = null){}
+    public static IResult Json(object? data, JsonTypeInfo jsonTypeInfo, string? contentType = null, int? statusCode = null){}
}

Usage Examples

using System.Text.Json.Serialization;

var app = WebApplication.Create(args);

// Using context
app.MapGet("/", () => TypedResults.Json(new Sample(), SampleJsonContext.Default));

// Using typeInfo
app.MapGet("/", () => TypedResults.Json(new Sample(), SampleJsonContext.Default.Sample));

app.Run();

public class Sample
{ }

[JsonSerializable(typeof(Sample))]
public partial class SampleJsonContext : JsonSerializerContext
{ }

Alternative Designs

Should MVC controller base be updated as well?

Risks

[Type]Results already have few overloads for the Json methods, that introducing those new overloads will cause ambiguity when calling something like Json(new Sample(), null, contentType, statusCode) but it is similar to what JsonSerializer method have today

@brunolins16 brunolins16 added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jan 24, 2023
@brunolins16 brunolins16 added this to the .NET 8 Planning milestone Jan 24, 2023
@ghost
Copy link

ghost commented Jan 24, 2023

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@brunolins16 brunolins16 self-assigned this Jan 24, 2023
@brunolins16 brunolins16 added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 25, 2023
@ghost
Copy link

ghost commented Jan 25, 2023

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@halter73
Copy link
Member

halter73 commented Jan 30, 2023

API Review Notes:

  • Why is JsonTypeInfo not JsonTypeInfo<TValue> like it is for WriteAsJsonAsync. Let's stay consistent with WriteAsJsonAsync and SerializeAsync and include the generic parameter.

API Approved!

namespace Microsoft.AspNetCore.Http;

public static class TypedResults
{
     public static JsonHttpResult<TValue> Json<TValue>(TValue? data, JsonSerializerOptions? options = null, string? contentType = null, int? statusCode = null)
+    public static JsonHttpResult<TValue> Json<TValue>(TValue? data, JsonSerializerContext jsonContext, string? contentType = null, int? statusCode = null) {}
+    public static JsonHttpResult<TValue> Json<TValue>(TValue? data, JsonTypeInfo<TValue> jsonTypeInfo, string? contentType = null, int? statusCode = null)
}

public static class Results
{
     public static IResult Json<TValue>(TValue? data, JsonSerializerOptions? options = null, string? contentType = null, int? statusCode = null)
+    public static IResult Json<TValue>(TValue? data, JsonSerializerContext jsonSerializerContext, string? contentType = null, int? statusCode = null) {}
+    public static IResult Json<TValue>(TValue? data, JsonTypeInfo<TValue> jsonTypeInfo, string? contentType = null, int? statusCode = null){}

     public static IResult Json(object? data, JsonSerializerOptions? options = null, string? contentType = null, int? statusCode = null)

// EDIT: The commented API was originally approved. The line below with the Type type argument now replaces it.
// +    public static IResult Json(object? data, JsonSerializerContext jsonSerializerContext, string? contentType = null, int? statusCode = null){}
+    public static IResult Json(object? data, Type type, JsonSerializerContext jsonSerializerContext, string? contentType = null, int? statusCode = null){}
+    public static IResult Json(object? data, JsonTypeInfo jsonTypeInfo, string? contentType = null, int? statusCode = null){}
}

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jan 30, 2023
@brunolins16
Copy link
Member Author

brunolins16 commented Jan 30, 2023

@halter73

I was updating the PR with the approved API and I noticed that we have one inconsistency between this API and JsonSerializer (or HttpReponseExtensions) where when the non-generic method takes a JsonSerializerContext we always take a type as well while in the proposed api it is not there.

I believe we have two options:

  1. Add the type parameter:
-    public static IResult Json(object? data, JsonSerializerContext jsonSerializerContext, string? contentType = null, int? statusCode = null){}
+    public static IResult Json(object? data, Type type, JsonSerializerContext jsonSerializerContext, string? contentType = null, int? statusCode = null){}
  1. Remove this overload
-    public static IResult Json(object? data, JsonSerializerContext jsonSerializerContext, string? contentType = null, int? statusCode = null){}

I would say option 1 is more consistent with what we have today, however, JsonHttpResult is a generic class. I will let you decide.

@brunolins16
Copy link
Member Author

however, JsonHttpResult is a generic class

Just to clarify. today we create a JsonHttpResult<object> for the non-generic overloads.

@brunolins16
Copy link
Member Author

I have updated my PR to:

namespace Microsoft.AspNetCore.Http;

public static class TypedResults
{
     public static JsonHttpResult<TValue> Json<TValue>(TValue? data, JsonSerializerOptions? options = null, string? contentType = null, int? statusCode = null)
+    public static JsonHttpResult<TValue> Json<TValue>(TValue? data, JsonSerializerContext context, string? contentType = null, int? statusCode = null) {}
+    public static JsonHttpResult<TValue> Json<TValue>(TValue? data, JsonTypeInfo<TValue> jsonTypeInfo, string? contentType = null, int? statusCode = null)
}

public static class Results
{
     public static IResult Json<TValue>(TValue? data, JsonSerializerOptions? options = null, string? contentType = null, int? statusCode = null)
+    public static IResult Json<TValue>(TValue? data, JsonSerializerContext context, string? contentType = null, int? statusCode = null) {}
+    public static IResult Json<TValue>(TValue? data, JsonTypeInfo<TValue> jsonTypeInfo, string? contentType = null, int? statusCode = null){}

     public static IResult Json(object? data, JsonSerializerOptions? options = null, string? contentType = null, int? statusCode = null)
+    public static IResult Json(object? data, Type type, JsonSerializerContext context, string? contentType = null, int? statusCode = null){}
+    public static IResult Json(object? data, JsonTypeInfo jsonTypeInfo, string? contentType = null, int? statusCode = null){}
}

@halter73
Copy link
Member

Nice catch @brunolins16. The goal here was to align with JsonSerializer.SerializeAsync and WriteAsJsonAsync, and in both cases the non-generic overloads that take the JsonSerializerContext take object? value, Type inputType, JsonSerializerContext context, ... in that order like your updated API proposal.

The updated API is approved! I'll make a note of it in the meeting email. Thanks.

@brunolins16 brunolins16 moved this from In Progress to Done in [.NET 8] Web Frameworks Feb 1, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 3, 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-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants