aspnet / Mvc

[Archived] ASP.NET Core MVC is a model view controller framework for building dynamic web sites with clean separation of concerns, including the merged MVC, Web API, and Web Pages w/ Razor. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
5.62k stars 2.14k forks source link

Suggestions on Web API action results metadata #6558

Closed CharlBest closed 6 years ago

CharlBest commented 7 years ago

Core 2.1 planning suggestion/contribution So I watched the ASP.NET Community Standup (July 11th, 2017) YouTube video a while ago and saw @DamianEdwards talk about the new things in Core 2.1 and any feedback from the community.

I currently have a setup with an Angular 4.3.0 project (Typescript) consuming a Web API project (ASP.NET Core 1.1.2) so it is a very opinionated setup/configuration but still broad as in normal Javascript client consuming c# rest service.

It was really important to me that everything tries and be strongly typed. So: This is how all my controllers are decorated: image

I then use an extension called Typewriter link (Creator: @frhagn) and basically it creates .ts (Typescript) files from C# classes. I automatically regenerates new .ts files if I change my C# file. Had a problem once in a very big C# project I worked on and it could handle the amount of models. Anyway the point is he spoke in the video about auto code generation. I'm currently watching my controllers and models.

Example of controller template: image

Example of models template: image

Result for controller: image

The result of the models in Typescript is as follow: image

I then bundle all of these auto generated bits in a single file and dump them into my Angular project via a gulp file in Visual Studio and set the script to run on build via the task runner.

Conclusion: What I realized was that I actually don't spend as much time jumping between the projects. I code my entire endpoint and then as soon as I'm in my web app (Angular) I have all the return types in my intellisense: image Also I wrote something to read the ModelStateDictionary if the response status in wrong (400 - 500) to get generic form validation working with my rest endpoint responses. I really like the workflow and just thought I'd share it with you guys. This is all in response to the video saying you guys are thinking of improving web API specifically ActionResult metadata, swagger, spec first creation, code generated endpoints and the model binder.

Sorry if I typed to much or if this is totally useless. Haha. Love the work you are doing on .NET Core :)

rynowak commented 7 years ago

👏 👀 💯

I think this is pretty amazing. Doing what you're doing is something we want to make even easier and better in 2.1. Seeing work like this makes the evidence even stronger that we should invest in this area. We want everyone to feel as smart as you are 👍

/cc @davidfowl @DamianEdwards

CharlBest commented 7 years ago

Hahaha thanks @rynowak. I'm not smart, just a normal dev playing around and getting the "best" tech combinations working nicely. But thanks :) Lets see if it helps or drives some discussion.

billpratt commented 7 years ago

I think having a notion of IActionResult<T> is a very nice to have. I probably don't have to explain why having a strongly typed return type is a good thing. :) I would like to point out that libraries like Swashbuckle could also benefit from this when generating swagger documentation. Java Spring has a ResponseEntity<T> return type, found here, for API controllers and works pretty well.

rynowak commented 7 years ago

This actually something we're evaluating for 2.1. https://github.com/rynowak/KodKod/tree/master/src/KodKod.Runtime/Mvc

billpratt commented 7 years ago

@rynowak Nice! I would be happy to review, discuss or contribute in any way. This is something I have been wanting to see for some time.

rynowak commented 7 years ago

Let me throw some ideas at you @CharlBest and @billpratt

What's the value to you if we did the following?

  1. MVC includes swagger by default - let's suppose that the default F5 experience opens and experience like http://petstore.swagger.io/ by default
  2. Idiomatic MVC code generates good swagger

This means some enhancements to the programming model, like ActionResult<T> and also us making some (configurable) assumptions about the kinds of things you action might do.

For instance if you take an example like: http://petstore.swagger.io/#/pet/getPetById - this would be the equivalent code and it should it give something very similar to that example.

[HttpGet("pet/{id}")]
public ActionResult<Pet> GetByIdAsync(int id)
{
  ...
}

For instance we would infer the following responses:

This is something we can't/don't do today - but there are pretty easy conventions to understand, and they fall into the reasonable code probably does this already kind of bucket.


So what's the point of all of this? Enterprising users already can do this using the totally non-obvious attributes we provide 😆.

Well we hope to have everyone fall into the pit of success by default. If we did any of the above we will make sure that it's configurable and/or replaceable. The existing attributes will still work.

We'd also probably take this a step further with:

  1. Generate a swagger document at build time using a dotnet CLI tool

and in the future with:

  1. Start building for codegen kinds of technology for clients - what if you could have something like a project-to-project reference but it generates a strongly typed client? This would require a lot of moving parts and isn't something we can get to soon.

I'm interesting any and all brainstorming about how you'd use this, alternate ideas, and what else would be useful.

billpratt commented 7 years ago

I really like the idea of MVC having out of the box support/generation for Swagger. As a future enhancement I could see a "Enable Swagger Documentation" checkbox on File->New or even a right-click "Enable Swagger Documentation" to an existing project similar to adding Docker support. I still receive comments on my blog post Swagger and ASP.NET Web API, which was written in 2015, that makes me believe that enabling Swagger support it's not as intuitive as it could be. Also, this could help with issues like this.

I think making assumptions like 200, 400 and 404 responses is a really nice to have though I agree it should be configurable and easy to disable. Maybe even adding a 500 in the mix since that is another very common response code.

I would consider utilizing the Swashbuckle project instead of creating something new. I would hate to see all of that hard work go to waste if ASP.NET Core offered a replacement. I would like to see ASP.NET Core provide great support to make it easier for projects like Swashbuckle.

billpratt commented 7 years ago

Something else to discuss...

One nice feature of returning just an IActionResult is the ability to return one type on 200 success and a string message when returning error response codes. I am curious how this would work.

rynowak commented 7 years ago

One nice feature of returning just an IActionResult is the ability to return one type on 200 success and a string message when returning error response codes. I am curious how this would work.

Did you take a look at https://github.com/rynowak/KodKod/blob/master/src/KodKod.Runtime/Mvc/ActionResult.cs?

This is convertible from T or any IActionResult

billpratt commented 7 years ago

I did take a look at that class. I see now what you mean. I like it!

billpratt commented 7 years ago

@rynowak What are your thoughts on using Swashbuckle? Maybe include the maintainers of that project into the discussion?

billpratt commented 7 years ago

@rynowak anything I can do to help with this?

CharlBest commented 7 years ago

I also really like the idea of having ActionResult<T> It makes sense. And off course I like the future idea of automatically creating strongly typed clients even if its just the models/view models to consume somewhere in some other language (like Typescript)

rynowak commented 7 years ago

Sorry for the lack of update here. We're still in the planning/prototyping phase for what our 2.1 investments will be. Assigning to myself and @pranavkm for tracking.

We're very very likely to do some kind of investments here on things like ActionResult<T> and more on making idiomatic MVC generate good swagger. Putting the integration in the box is something we're still trying to figure out. It's definitely our desire to use Swashbuckle for this.


A few things that I'd like feedback on:

We want to provide a set of (customizable) conventions that attempt to categorize your action methods and attempt to deduce what kinds of non-success responses are possible. What kinds of conventions are useful here? There's an example in this post https://github.com/aspnet/Mvc/issues/6558#issuecomment-320564363.

In addition we want to add an object model for https://tools.ietf.org/html/rfc7807 in this box, and retrofit some stuff to use it (if you didn't tell us to do it differently). The effect would be that we'd start using this for non-200 responses by default (customizable). There would ultimately be a way for you to plug in and configure what data you want to send back for a non-200, but this would be the default. This part seems necessary because to have a good swagger experience by default we need to describe errors. Does this seem useful? What do you think about today's experience for non-200's? Do you define your own schema for this today, and how useful is it for you to configure what the framework sends back?

@pranavkm anything else?

pranavkm commented 7 years ago

That seems to cover most of what we've discussed so far. I just started work on the object model for errors and I haven't come across anything in particular that needs to be called out in addition to what you already said.

billpratt commented 7 years ago

What kinds of conventions are useful here? There's an example in this post #6558 (comment). For instance we would infer the following responses:

  • 200 type Pet

Why not infer a 200 for any method defined here: https://tools.ietf.org/html/rfc7231#section-6.3.1

Make sense

Why not infer for any HttpGet attribute? This would work for both actions with and without parameters ie Get() vs Get(string id). In regards to 'id' parameter, I would be careful not force devs into using 'id'. I typically use a more descriptive name like 'productId' or 'countryCode'.

Also, inferring a 500 response for all methods would be useful

billpratt commented 7 years ago

In addition we want to add an object model for https://tools.ietf.org/html/rfc7807 in this box, and retrofit some stuff to use it (if you didn't tell us to do it differently). The effect would be that we'd start using this for non-200 responses by default (customizable). There would ultimately be a way for you to plug in and configure what data you want to send back for a non-200, but this would be the default. This part seems necessary because to have a good swagger experience by default we need to describe errors. Does this seem useful? What do you think about today's experience for non-200's? Do you define your own schema for this today, and how useful is it for you to configure what the framework sends back?

I really like this idea. I think having a default is nice but make it very easy to plug in your own.

For me, I try to be cautious what information I expose to callers when returning a non 2XX response. For example, I wouldn't want to expose a stack trace when a 500 is returned. I typically would return a more generic message like "Internal Server Error" or "An error has occurred" with maybe a correlation id. The actual exception is tracked internally via logging and APM.

pranavkm commented 7 years ago

@billpratt we have a PR to get some of the ground work for RFC 7807 here - https://github.com/aspnet/Mvc/pull/6695. Feel free to chime.

rynowak commented 7 years ago

We're still not yet ready to post an officially 2.1 upcoming changes kind of announcement, but I wanted to point out to everyone here that we've started work on some of the things we're already clear about in the API area:

See: https://github.com/aspnet/Mvc/projects (description) https://github.com/aspnet/Mvc/projects/5 (work items)

@billpratt @khellang @CharlBest @domaindrivendev

I'm interested in any and all feedback on this set of items. Are we missing anything? Any more details that seem important to capture? Feel free to use the issues to provide feedback as well

aspnet-hello commented 6 years ago

We periodically close 'discussion' issues that have not been updated in a long period of time.

We apologize if this causes any inconvenience. We ask that if you are still encountering an issue, please log a new issue with updated information and we will investigate.