Introduce opinionated API defaults. #6807

pranavkm · 2017-09-11T20:49:58Z

Introduce ProblemDescriptionAttribute to enhance some 4xx messages and produce better API description.
Introduce IErrorDescriptionProvider to modify the shape of error response.

pranavkm · 2017-09-11T20:50:26Z

Making sure we're on page with the design before I add tests etc

rynowak · 2017-09-11T22:10:13Z

samples/MvcSandbox/MvcSandbox.csproj

@@ -15,6 +15,8 @@
    <PackageReference Include="Microsoft.Extensions.Configuration.Json" />
    <PackageReference Include="Microsoft.Extensions.Logging.Console" />
    <PackageReference Include="Microsoft.Extensions.Logging.Debug" />
+
+    <PackageReference Include="Swashbuckle.AspNetCore" Version="1.0.0" />


planning to check this in? that needs @Eilon to approve

Nope. That's just to get an idea of what the API description looks like. I'll undo the sample changes before this goes in.

rynowak · 2017-09-11T22:11:28Z

src/Microsoft.AspNetCore.Mvc.ApiExplorer/ProblemAttributeApiDescriptionProvider.cs

+                    }
+                }
+
+                apiDescription.SupportedResponseTypes.Add(CreateProblemResponse(statusCode: 0));


This was for returning a "default" error response.

rynowak · 2017-09-11T22:12:07Z

src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs

@@ -11,6 +11,7 @@
 using Microsoft.AspNetCore.Mvc.ApplicationModels;
 using Microsoft.AspNetCore.Mvc.ApplicationParts;
 using Microsoft.AspNetCore.Mvc.Controllers;
+using Microsoft.AspNetCore.Mvc.ErrorDescription;


Can we not have this?

Sure. .Infrastructure namespace? It doesn't seem top-level worthy.

rynowak · 2017-09-11T22:14:08Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/ProblemDescriptionApplicationModelProvider.cs

+                    var effectiveProblemDescriptionAttribute = (actionProblemDescriptionAttribute ?? controllerProblemDescriptionAttribute);
+                    if (effectiveProblemDescriptionAttribute != null)
+                    {
+                        actionModel.Properties.Add(ProblemDescriptionAttributeKey, effectiveProblemDescriptionAttribute);


You can make this attribute an IFilterMetadata and then use that from the action descriptor. This will do the same thing with less complexity

ProblemDescriptionAttribute is already a filter, I guess I could use that instead.

rynowak · 2017-09-11T22:15:06Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/ProblemDescriptionFilter.cs

+
+namespace Microsoft.AspNetCore.Mvc.Internal
+{
+    public class ProblemDescriptionFilter : IResultFilter


This should be an action filter. Result filters aren't called in some cases

Yup, missed updating it.

rynowak · 2017-09-11T22:16:19Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/ProblemDescriptionFilter.cs

+                };
+            }
+
+            bool BindsIdParameter()


We don't need this check. You already told us its a 404

SteveSandersonMS · 2017-09-12T16:01:42Z

src/Microsoft.AspNetCore.Mvc.ApiExplorer/ProblemAttributeApiDescriptionProvider.cs

+                {
+                    apiDescription.SupportedResponseTypes.Add(CreateProblemResponse(StatusCodes.Status400BadRequest));
+
+                    if (parameters.Any(p => string.Equals("id", p.Name, StringComparison.OrdinalIgnoreCase)))


Would we want to also look for things of the form blahId, i.e.., EndsWith("Id", StringComparison.Ordinal)? I personally always tend to name things customerId, productId, etc.

Also this is kind of English-language centric. How much precidence is there for giving special meaning to id elsewhere? I seem to recall that EF has some conventions around this. But if I'm wrong then maybe the id property name/suffix should be configurable.

Good idea on the EF bit. I'll take a gander at their code.

Agreed, I think we can do ahead and do EndsWith("Id"...) right now.

I'm not worried about extensibility for this class right now, we need to do it but we need to get to a working E2E so we can start on the other parts of this.

+1 for EndsWith("Id"...). Extensibility, possibly in the form of an action parameter attribute, to indicate "this parameter can trigger a 404" would be useful.

SteveSandersonMS · 2017-09-12T16:29:31Z

src/Microsoft.AspNetCore.Mvc.ApiExplorer/ProblemAttributeApiDescriptionProvider.cs

+
+                    if (parameters.Any(p => string.Equals("id", p.Name, StringComparison.OrdinalIgnoreCase)))
+                    {
+                        apiDescription.SupportedResponseTypes.Add(CreateProblemResponse(StatusCodes.Status404NotFound));


Shouldn't it also depend on the HTTP method? If I send a PUT request with an id, I'm not sure what 404 would mean in that case. Does our ApiDescriptionProviderContext have some way to say that certain ApiResponseType values only apply to certain HTTP methods?

Also, what would be the design for how to say your action does not ever return a 404?

Does our ApiDescriptionProviderContext have some way to say that certain ApiResponseType values only apply to certain HTTP methods?

Unfortunately not (and AFAIK neither does Swagger 2.0).

Also, what would be the design for how to say your action does not ever return a 404?

I intentionally left out any way to configure behavior at this point. We can do this in a follow up iteration. But I agree, ideally you want to be able to describe these in some way that affects both the Api description and the filter.

SteveSandersonMS · 2017-09-12T16:36:15Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultErrorDescriptorFactory.cs

+
+namespace Microsoft.AspNetCore.Mvc.ErrorDescription
+{
+    public class DefaultErrorDescriptorFactory : IErrorDescriptionFactory


We have two levels of extensibility here: both 'factory' and within the factory, some 'providers'. Could you clarify why the two-tier system is necessary? I'm missing why it's not enough to have a single collection of providers.

We have the pattern elsewhere in Mvc. Generally, you have a default provider and the user may append additional providers to run after to customize behavior. Doing so you let's you start from a default configured instance. The factory is meant to be a driver for the providers and we don't expect users to replace it.

The factory is what our code calls. The providers are the extensibility

SteveSandersonMS · 2017-09-12T16:44:04Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/ProblemDescriptionFilter.cs

+                    Title = Resources.ProblemDescription_400_Title,
+                });
+
+                context.Result = new BadRequestObjectResult(problem)


Don't we risk losing valuable data here? If the incoming context.Result is a subclass of StatusCodeResult that contains extra information that you'd want to surface to the client, then is it possible that this code is discarding that information here?

Yup. The check needs to be more stringent.

Good point, these probably need to be exact matches. We should be be looking for NotFoundResult also

SteveSandersonMS · 2017-09-12T16:46:46Z

samples/MvcSandbox/Controllers/HomeController.cs

@@ -14,5 +14,9 @@ public IActionResult Index()
        {
            return View();
        }
+
+        [HttpGet("")]
+        [ProblemDescription]


The spec (https://tools.ietf.org/html/rfc7807) uses the term "problem details" rather than "problem description". Might a different name like [ReturnsProblemDetails] be more descriptive?

@rynowak thoughts? We would change the type name too (it's currently named ProblemDescription) if we were to change this.

That's a good point. We might want to rename that class.

The attribute that's here is just an example.

My plan is for us to introduce an [ApiController] attribute, and mostly have the 'problem filter' hidden from users

* Introduce ProblemDescriptionAttribute to enhance some 4xx messages and produce better API description. * Introduce IErrorDescriptionProvider to modify the shape of error response. Fixes #6785, Fixes #6786

pranavkm · 2017-09-13T23:48:39Z

🆙 📅

pranavkm · 2017-09-14T00:03:59Z

src/Microsoft.AspNetCore.Mvc.ApiExplorer/ProblemDetailsApiDescriptionProvider.cs

+                {
+                    apiDescription.SupportedResponseTypes.Add(CreateProblemResponse(StatusCodes.Status400BadRequest));
+
+                    if (parameters.Any(p => p.Name.EndsWith("id", StringComparison.OrdinalIgnoreCase)))


EF attempts to look for Id and {ModelName}Id. We wouldn't be able to do the latter, but I think it would be reasonable to look for the pattern {something}Id (exact case)

pranavkm · 2017-09-14T00:05:17Z

src/Microsoft.AspNetCore.Mvc.ApiExplorer/ProblemDetailsApiDescriptionProvider.cs

+                    }
+                }
+
+                apiDescription.SupportedResponseTypes.Add(CreateProblemResponse(statusCode: 0));


I'll file a bug for this. We need a way to represent default response types

Can you please add a comment and explain what this is?

pranavkm · 2017-09-14T00:31:11Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/ProblemDetailsFilter.cs

+            return new ProblemDetails
+            {
+                Status = notFoundResult.StatusCode,
+                Title = Resources.ProblemDetails_404_Title,


We could look for the value of the id argument and include it as part of the detail.

I think this is fine for now

pranavkm · 2017-09-14T00:31:47Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/ProblemDetailsFilter.cs

+                case BadRequestResult badRequestResult:
+                    problemDetails = GetBadRequestProblemDetails(badRequestResult);
+                    break;
+                case UnsupportedMediaTypeResult unsupportedMediaTypeResult:


This is the only other StatusCodeResult subtype that's built in to the system. Seemed low cost to add it.

Sure. Might want to comment that we don't really expect user code to return this

pranavkm · 2017-09-14T00:32:06Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/ProblemDetailsFilter.cs

+                case StatusCodeResult result when isExactlyStatusCodeResult && result.StatusCode == StatusCodes.Status400BadRequest:
+                    problemDetails = GetBadRequestProblemDetails(result);
+                    break;
+                case StatusCodeResult result when isExactlyStatusCodeResult && result.StatusCode == StatusCodes.Status404NotFound:


overtly complicated code ftw. But I do get 💯 points

This is ugly, but I think it gets the job done. I want to go with something like this for now to unblock us, even if it's super inelegant

rynowak · 2017-09-14T01:40:43Z

src/Microsoft.AspNetCore.Mvc.Core/ControllerBase.cs

@@ -1503,7 +1503,7 @@ public virtual BadRequestObjectResult BadRequest(ModelStateDictionary modelState
        /// </summary>
        /// <returns>The created <see cref="BadRequestObjectResult"/> for the response.</returns>
        [NonAction]
-        public virtual BadRequestObjectResult ValidationProblem(ValidationProblemDescription descriptor)
+        public virtual BadRequestObjectResult ValidationProblem(ValidationProblemDetails descriptor)


Should we return ActionResult here? You can override this but you can't change the result type.

rynowak · 2017-09-14T01:41:49Z

src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IErrorDescriptorProvider.cs

+
+        void OnProvidersExecuting(ErrorDescriptionContext context);
+
+        void OnProvidersExecuted(ErrorDescriptionContext context);


Should we simplify this down to one method? I don't see much value in spreading this project more

rynowak · 2017-09-14T01:42:15Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultErrorDescriptorFactory.cs

+            _providers = providers.OrderBy(p => p.Order).ToArray();
+        }
+
+        public object CreateErrorDescription(ActionDescriptor actionDescriptor, object result)


validate arguments

rynowak · 2017-09-14T01:44:21Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/ProblemDetailsFilter.cs

+            }
+
+            var errorDetails = _factory.CreateErrorDescription(context.ActionDescriptor, problemDetails);
+            context.Result = new BadRequestObjectResult(errorDetails)


This should be plain old object result

rynowak · 2017-09-14T01:48:10Z

src/Microsoft.AspNetCore.Mvc.Core/Resources.resx

+  </data>
+  <data name="ProblemDetails_415_Title" xml:space="preserve">
+    <value>The request entity is in a format that is not supported by the requested resource.</value>
+  </data>


Do these match the expectations from the spec about titles?

rynowak

This is looking a bit better. I have a crazy idea about the result executors that I want to prototype. I think the filter might have a few fatal flaws...

pranavkm · 2017-09-14T22:32:01Z

Expanding on @rynowak's mysterious comment, the plan is to use #6821 and #6822 to provide problem details rather than a single filter. We realized having the single filter was hard to scale. So this PR introduces the api descriptor. We'll retrofit the configuration once we have those pieces.

rynowak · 2017-09-14T22:57:49Z

src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IErrorDescriptorProvider.cs

+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+


`Extra line

rynowak · 2017-09-14T22:58:42Z

src/Microsoft.AspNetCore.Mvc.Core/ProblemDetailsAttribute.cs

+    /// Adds an <see cref="IFilterMetadata"/> that indicates to the framework that the current action conforms to well-known API behavior.
+    /// </summary>
+    [AttributeUsage(AttributeTargets.Method | AttributeTargets.Class, AllowMultiple = false, Inherited = true)]
+    public class ProblemDetailsAttribute : Attribute, IFilterMetadata


This is OK for now, I'm going to remove this when I merge API controller

pranavkm requested a review from rynowak September 11, 2017 20:49

dnfclas added the cla-already-signed label Sep 11, 2017

pranavkm requested a review from SteveSandersonMS September 11, 2017 20:50

rynowak reviewed Sep 11, 2017

View reviewed changes

SteveSandersonMS reviewed Sep 12, 2017

View reviewed changes

Introduce opinionated API defaults.

f7e984e

* Introduce ProblemDescriptionAttribute to enhance some 4xx messages and produce better API description. * Introduce IErrorDescriptionProvider to modify the shape of error response. Fixes #6785, Fixes #6786

pranavkm force-pushed the prkrishn/ead branch from aada4da to 58ed4c1 Compare September 13, 2017 23:45

pranavkm changed the title ~~[Design] Introduce opinionated API defaults.~~ Introduce opinionated API defaults. Sep 13, 2017

pranavkm force-pushed the prkrishn/ead branch from 58ed4c1 to 1c531a6 Compare September 13, 2017 23:45

Changes per PR comments

25aefe0

pranavkm force-pushed the prkrishn/ead branch from 1c531a6 to 25aefe0 Compare September 13, 2017 23:48

pranavkm commented Sep 14, 2017

View reviewed changes

rynowak reviewed Sep 14, 2017

View reviewed changes

Changes per discussion

9782b08

pranavkm force-pushed the prkrishn/ead branch from df39b2a to 9782b08 Compare September 14, 2017 22:28

rynowak reviewed Sep 14, 2017

View reviewed changes

rynowak approved these changes Sep 14, 2017

View reviewed changes

pranavkm closed this Sep 15, 2017

pranavkm deleted the prkrishn/ead branch September 15, 2017 00:07


		void OnProvidersExecuting(ErrorDescriptionContext context);

		void OnProvidersExecuted(ErrorDescriptionContext context);

		// Copyright (c) .NET Foundation. All rights reserved.
		// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

Introduce opinionated API defaults. #6807

Introduce opinionated API defaults. #6807

Uh oh!

Conversation

pranavkm commented Sep 11, 2017

Uh oh!

pranavkm commented Sep 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rynowak Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavkm commented Sep 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rynowak Sep 11, 2017 •

edited

Loading