Add helper methods on ControllerBase to return ProblemDetails #12298

pranavkm · 2019-07-18T03:01:17Z

Introduce ControllerBase.Problem and ValidationProblem overload
that accepts optional parameters
Consistently use ClientErrorData when generating ProblemDetails
Clean-up InvalidModelStateResponseFactory initialization.

Description

Add APIs to return a ProblemDetails instance from a ControllerBase. Fixes bugs where instances of ProblemDetails created in different parts of MVC had different (missing) content.

Customer Impact

This is a new API so customers aren't impacted until they use it.

Regression?

No

Risk

Low.

src/Mvc/Mvc.Core/src/DependencyInjection/ApiBehaviorOptionsSetup.cs

pranavkm · 2019-07-18T03:07:19Z

src/Mvc/Mvc.Core/src/DependencyInjection/ApiBehaviorOptionsSetup.cs

+                    Status = StatusCodes.Status400BadRequest,
+                };
+
+                if (options.ClientErrorMapping.TryGetValue(400, out var clientErrorData))


This is the new bit. ValidationProblemDetails returned by the ModelStateInvalidFilter should now have a "type": "https://tools.ietf.org/html/rfc7231#section-6.5.1" (or whatever Link the user configured).

I realized I'm missing a test to verify we use the user configured value. I'll add one

pranavkm · 2019-07-18T03:09:56Z

src/Mvc/test/WebSites/BasicWebSite/StartupWithCustomInvalidModelStateFactory.cs

                options.InvalidModelStateResponseFactory = context =>
                {
-                    var result = (BadRequestObjectResult)previous(context);


This test was really janky. This broke because it expected a vanilla dictionary but now gets a ProblemDetails. We wouldn't recommend users to write something like this.

pranavkm · 2019-07-18T03:13:08Z

/cc @khellang

src/Mvc/Mvc.Core/src/ControllerBase.cs

rynowak · 2019-07-18T03:25:49Z

src/Mvc/Mvc.Core/src/ControllerBase.cs

+
+            if (problemDetails.Title is null || problemDetails.Type is null)
+            {
+                var options = HttpContext.RequestServices.GetRequiredService<IOptions<ApiBehaviorOptions>>().Value;


I'm a little anxious about putting a service here without a way to mock it. I don't have a great solution other than doing things like making this a filter.

Most of the other places we interact with services, you can mock them easily. https://github.com/aspnet/AspNetCore/blob/f7e3eaa7e324b99aeb496a68414cd093acaae802/src/Mvc/Mvc.Core/src/ControllerBase.cs#L171

Does it seem reasonable to make a new service ProblemDetailsFactory, and then add that as a property, and then call it from here?

rynowak · 2019-07-18T03:26:33Z

src/Mvc/Mvc.Core/src/ControllerBase.cs

@@ -1849,6 +1898,8 @@ public virtual ActionResult ValidationProblem([ActionResultObjectValue] ModelSta
            }

            var validationProblem = new ValidationProblemDetails(modelStateDictionary);
+            ApplyProblemDetailsDefaults(validationProblem, statusCode: 400);


Is this broken today? We don't apply the defaults?

rynowak · 2019-07-18T03:27:32Z

src/Mvc/Mvc.Core/src/ControllerBase.cs

+            string instance = null,
+            string title = null,
+            string type = null,
+            [ActionResultObjectValue] ModelStateDictionary modelStateDictionary = null)


Do we need this as a parameter? is there really a use case for passing it in explicitly?

We have this for BadObjectResult and another ValidationProblem. Doesn't seem like it would hurt.

rynowak · 2019-07-18T03:28:06Z

src/Mvc/Mvc.Core/src/ControllerBase.cs

+                validationProblem.Title = title;
+            }
+
+            ApplyProblemDetailsDefaults(validationProblem, statusCode: 400);


For the people that really want this to be a 422 - what do they do?

ValidationProblem returns the non-specific ActionResult return type. We expect you to override and copy this block of code if you'd like it to get this behavior. It's a few lines of code to copy.

rynowak · 2019-07-18T03:30:10Z

src/Mvc/Mvc.Core/test/ControllerBaseTest.cs

+            var controller = new TestableController
+            {
+                ControllerContext = context,
+            };


See I don't feel like this amount of setup is reasonable for someone who wants to unit test a controller.

khellang · 2019-07-18T20:08:47Z

src/Mvc/Mvc.Core/src/DependencyInjection/ApiBehaviorOptionsSetup.cs

-                options.InvalidModelStateResponseFactory = ProblemDetailsFactory;
+                var problemDetails = new ValidationProblemDetails(context.ModelState)
+                {
+                    Status = StatusCodes.Status400BadRequest,


This could be removed, ref. #8688

khellang · 2019-07-20T22:16:00Z

src/Mvc/Mvc.Core/src/ControllerBase.cs

+            {
+                if (_problemDetailsFactory == null)
+                {
+                    _problemDetailsFactory = HttpContext?.RequestServices?.GetRequiredService<ProblemDetailsFactory>();


This could end up being null which would lead to a NRE below.

It's expected to only ever be null in test scenarios. Basically following the same pattern as https://github.com/aspnet/AspNetCore/pull/12298/files#diff-8aa2822b0a420b6f8a04153ca4312519R172

Wouldn't it be better to check if it's null and throw a better exception then? 🤔

@khellang I'd rather keep the code comparable to the other properties for now. I did venture at using nullable types at one point and that forces some cleaning up some of these.

I'm not saying you should change this property, I'm saying you might want to guard against null at the call site. Haven't checked usage of the other properties, but I'd consider those potential bugs as well if not properly guarded against null.

pranavkm · 2019-07-22T16:34:18Z

src/Mvc/Mvc.Core/src/Infrastructure/ProblemDetailsFactory.cs

+        /// <param name="detail">The value for <see cref="ProblemDetails.Detail" />.</param>
+        /// <param name="instance">The value for <see cref="ProblemDetails.Instance" />.</param>
+        /// <returns>The <see cref="ValidationProblemDetails"/> instance.</returns>
+        public abstract ValidationProblemDetails CreateValidationProblemDetails(


Going back to an earlier question about what would you do if you needed to consistently return 422 for ValidationProblem - this would be the API to override this. This is called by InvalidModelStateFilter and ControllerBase.ValidationProblem

* Introduce ControllerBase.Problem and ValidationProblem overload that accepts optional parameters * Consistently use ClientErrorData when generating ProblemDetails * Clean-up InvalidModelStateResponseFactory initialization. Fixes #8537

netcore-jroger

useful.

pranavkm commented Jul 18, 2019

View reviewed changes

rynowak reviewed Jul 18, 2019

View reviewed changes

src/Mvc/Mvc.Core/src/ControllerBase.cs Show resolved Hide resolved

rynowak reviewed Jul 18, 2019

View reviewed changes

Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jul 18, 2019

khellang reviewed Jul 18, 2019

View reviewed changes

khellang mentioned this pull request Jul 19, 2019

Don't explicitly set ValidationProblemDetails status code #8688

Closed

pranavkm force-pushed the prkrishn/problem-details-helper branch from f7e3eaa to 8e8f88a Compare July 19, 2019 20:20

khellang reviewed Jul 20, 2019

View reviewed changes

pranavkm commented Jul 22, 2019

View reviewed changes

pranavkm added 3 commits July 23, 2019 11:03

Add a factory

fbb4bda

Fixup ref assemblies

8b1f527

pranavkm force-pushed the prkrishn/problem-details-helper branch from 126150c to 8b1f527 Compare July 23, 2019 18:11

mkArtakMSFT added the tell-mode Indicates a PR which is being merged during tell-mode label Jul 25, 2019

pranavkm added ask-mode This issue / PR is a patch candidate which we will bar-check internally before patching it. and removed tell-mode Indicates a PR which is being merged during tell-mode labels Jul 30, 2019

rynowak approved these changes Jul 31, 2019

View reviewed changes

pranavkm changed the base branch from master to release/3.0 August 4, 2019 02:09

pranavkm merged commit 709b390 into release/3.0 Aug 5, 2019

ghost deleted the prkrishn/problem-details-helper branch August 5, 2019 18:28

pranavkm mentioned this pull request Aug 5, 2019

Validate ProblemDetails scenarios #12886

Closed

netcore-jroger reviewed Sep 5, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add helper methods on ControllerBase to return ProblemDetails #12298

Add helper methods on ControllerBase to return ProblemDetails #12298

pranavkm commented Jul 18, 2019 •

edited

Loading

pranavkm Jul 18, 2019

pranavkm Jul 18, 2019

rynowak Jul 18, 2019

pranavkm Jul 18, 2019

pranavkm commented Jul 18, 2019

rynowak Jul 18, 2019 •

edited

Loading

pranavkm Jul 18, 2019

rynowak Jul 18, 2019

pranavkm Jul 18, 2019

rynowak Jul 18, 2019

pranavkm Jul 22, 2019

rynowak Jul 18, 2019

pranavkm Jul 18, 2019

rynowak Jul 18, 2019

khellang Jul 18, 2019

khellang Jul 20, 2019

pranavkm Jul 22, 2019

khellang Jul 22, 2019

pranavkm Jul 31, 2019

khellang Jul 31, 2019

pranavkm Jul 22, 2019

netcore-jroger left a comment

Add helper methods on ControllerBase to return ProblemDetails #12298

Add helper methods on ControllerBase to return ProblemDetails #12298

Conversation

pranavkm commented Jul 18, 2019 • edited Loading

Description

Customer Impact

Regression?

Risk

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm commented Jul 18, 2019

rynowak Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netcore-jroger left a comment

Choose a reason for hiding this comment

pranavkm commented Jul 18, 2019 •

edited

Loading

rynowak Jul 18, 2019 •

edited

Loading