Skip to content

IResult support in MVC #40639

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

Closed
Tracked by #33403
brunolins16 opened this issue Mar 10, 2022 · 5 comments · Fixed by #40710
Closed
Tracked by #33403

IResult support in MVC #40639

brunolins16 opened this issue Mar 10, 2022 · 5 comments · Fixed by #40710
Assignees
Labels
Docs This issue tracks updating documentation feature-model-binding old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Milestone

Comments

@brunolins16
Copy link
Member

brunolins16 commented Mar 10, 2022

Background and Motivation

Today, with the new Microsoft.AspNetCore.Http.Results class is very easy to get confused and implement a Controller Action similar with that, or any other variation of the methods exposed by this class:

[HttpGet]
public IResult Get()
{
    return Results.Ok();
}

Also, is very easy to use an Microsoft.AspNetCore.Http.IResult custom implementation as a return of a controller action, like this:

public class MyCustomResult : IResult
{
    public Task ExecuteAsync(HttpContext httpContext) => Task.CompletedTask;
}
[HttpGet]
public IResult Get()
{
    return new MyCustomResult();
}

Both examples will not throw exception but will serialize the class. For the first example the action will return the following payload:

{
  "value": null,
  "statusCode": 200,
  "contentType": null
}

Proposed API

The proposal is the creation of a new public action result that will store an instance of the IResult and call the IResult.ExecuteAsync method instead instead of using the IActionResultExecutor to write the Http Response. With this, we will miss all the content-negotiation available in MVC through the ObjectResultExecutor but we will have the correct serialization and all the other components, like Middleware, Filters etc., will work.

+namespace Microsoft.AspNetCore.Mvc;

+public class HttpResultsActionResult : ActionResult
+{
+    public IResult Result { get; }
+    public MinimalActionResult(IResult result){}
+    public override Task ExecuteResultAsync(ActionContext context){}
+}

The controller's action can return a Microsoft.AspNetCore.Http.IResult that will be converted to the new HttpResultsActionResult when the ActionResultTypeMapper.Convert is invoked. Eg.:

[HttpGet]
public IResult Get() => Results.Ok(new Todo() { Id = 1, Title = "Sample todo"});

The sample action will produce the following payload:

{
  "id": 1,
  "title":  "Sample todo"
}

Usage Examples

Controller's action using the HttpResultsActionResult

    [HttpGet()]
    public ActionResult Get(int id)
    {
        var result = Results.Ok(new Contact() { ContactId = id });
        return new HttpResultsActionResult(result);
    }

Result's filter using the HttpResultsActionResult

public class HttpResultResultFilter : IResultFilter
{
    public void OnResultExecuted(ResultExecutedContext context)
    {
        if (context is { Result: HttpResultsActionResult { Result: IResult httpResult } })
        {
            // Do something after the result executes.
        }        
    }
}

If the proposal #40656 is approved, the results instance could be further inspected, eg.:

public class HttpResultResultFilter : IResultFilter
{
    public void OnResultExecuted(ResultExecutedContext context)
    {
        if (context is
            {
                Result: HttpResultsActionResult
                { Result: IStatusCodeHttpResult { StatusCode: StatusCodes.Status200OK } }
            })
        {
            // Do something after the result executes.
        }      
    }
}

Alternative Designs

The new type could have a private constructor, that could avoid undesired instantiation

+namespace Microsoft.AspNetCore.Mvc;

+public class HttpResultsActionResult : ActionResult
+{
+    public IResult Result { get; }
+    public override Task ExecuteResultAsync(ActionContext context){}
+}

Or the new action result could be internal instead, that will make this proposal irrelevant, but the feature will be available, however, will not be possible to create Result Filters as shown in the example.

@brunolins16 brunolins16 self-assigned this Mar 10, 2022
@brunolins16 brunolins16 added old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels feature-model-binding Cost:S labels Mar 10, 2022
@brunolins16 brunolins16 added this to the .NET 7 Planning milestone Mar 10, 2022
@ghost
Copy link

ghost commented Mar 10, 2022

Thanks for contacting us.

We're moving this issue to the .NET 7 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 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 15, 2022
@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 Mar 15, 2022
@ghost
Copy link

ghost commented Mar 15, 2022

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

API Review Notes

  • Should the new type and constructor be public?
    • Yes for the type, because it could be useful for filters
    • Yes for the constructor. Even though no one should really call it, this is expected for ActionResult implementations.
  • What about renaming the type not to include "Result" twice?
+namespace Microsoft.AspNetCore.Mvc;

+public class HttpActionResult : ActionResult
+{
+    public IResult Result { get; }
+    public HttpActionResult(IResult result){}
+    public override Task ExecuteResultAsync(ActionContext context){}
+}

@brunolins16
Copy link
Member Author

Here are few name ideas for the next review meeting:

  • HttpActionResult
  • HttpApiResult
  • HttpResult
  • HttpResultActionResult
  • MinimalHttpResult
  • MinimalApiResult
  • MinimalActionResult
  • SimpleHttpResult
  • SimpleApiResult
  • SimpleActionResult
  • ApiResult
  • ApiActionResult

@halter73
Copy link
Member

API Review:

After considering the many additional proposed names, we like the originally proposed HttpActionResult even if it's similar to IHttpActionResult the most.

That said, we don't expect people to new these up since there's already an implicit conversion effectively when returned from Actions. We will wait for developer feedback to see if the use case of inspecting IResults in action filters is important enough to add new public API.

@halter73 halter73 removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Mar 28, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 1, 2022
@brunolins16 brunolins16 added the Docs This issue tracks updating documentation label Jul 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Docs This issue tracks updating documentation feature-model-binding old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants