Add a factory

Author: pranavkm

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

@@ -32,6 +32,7 @@ public abstract class ControllerBase
         private IModelBinderFactory _modelBinderFactory;
         private IObjectModelValidator _objectValidator;
         private IUrlHelper _url;
+        private ProblemDetailsFactory _problemDetailsFactory;
 
         /// <summary>
         /// Gets the <see cref="Http.HttpContext"/> for the executing action.
@@ -190,6 +191,28 @@ public IObjectModelValidator ObjectValidator
             }
         }
 
+        public ProblemDetailsFactory ProblemDetailsFactory
+        {
+            get
+            {
+                if (_problemDetailsFactory == null)
+                {
+                    _problemDetailsFactory = HttpContext?.RequestServices?.GetRequiredService<ProblemDetailsFactory>();
+                }
+
+                return _problemDetailsFactory;
+            }
+            set
+            {
+                if (value == null)
+                {
+                    throw new ArgumentNullException(nameof(value));
+                }
+
+                _problemDetailsFactory = value;
+            }
+        }
+
         /// <summary>
         /// Gets the <see cref="ClaimsPrincipal"/> for user associated with the executing action.
         /// </summary>
@@ -1823,51 +1846,33 @@ public virtual ConflictObjectResult Conflict([ActionResultObjectValue] ModelStat
             => new ConflictObjectResult(modelState);
 
         /// <summary>
-        /// Creates an <see cref="ObjectResult"/> that produces a <see cref="ProblemDetails"/> response with a <c>500</c>
-        /// error status with a <see cref="ProblemDetails" /> value.
+        /// Creates an <see cref="ObjectResult"/> that produces a <see cref="ProblemDetails"/> response.
         /// </summary>
-        /// <param name="title">The value for <see cref="ProblemDetails.Title" />.</param>
-        /// <param name="type">The value for <see cref="ProblemDetails.Type" />.</param>
+        /// <param name="statusCode">The value for <see cref="ProblemDetails.Status" />..</param>
         /// <param name="detail">The value for <see cref="ProblemDetails.Detail" />.</param>
         /// <param name="instance">The value for <see cref="ProblemDetails.Instance" />.</param>
+        /// <param name="title">The value for <see cref="ProblemDetails.Title" />.</param>
+        /// <param name="type">The value for <see cref="ProblemDetails.Type" />.</param>
         /// <returns>The created <see cref="ObjectResult"/> for the response.</returns>
         [NonAction]
         public virtual ObjectResult Problem(
             string detail = null,
             string instance = null,
+            int? statusCode = null,
             string title = null,
             string type = null)
         {
-            var problemDetails = new ProblemDetails
-            {
-                Title = title,
-                Type = type,
-                Detail = detail,
-                Instance = instance,
-            };
-
-            ApplyProblemDetailsDefaults(problemDetails, statusCode: 500);
+            var problemDetails = ProblemDetailsFactory.CreateProblemDetails(
+                HttpContext,
+                statusCode: statusCode ?? 500,
+                title: title,
+                type: type,
+                detail: detail,
+                instance: instance);
 
             return new ObjectResult(problemDetails);
         }
 
-        private void ApplyProblemDetailsDefaults(ProblemDetails problemDetails, int statusCode)
-        {
-            problemDetails.Status = statusCode;
-
-            if (problemDetails.Title is null || problemDetails.Type is null)
-            {
-                var options = HttpContext.RequestServices.GetRequiredService<IOptions<ApiBehaviorOptions>>().Value;
-                if (options.ClientErrorMapping.TryGetValue(statusCode, out var clientErrorData))
-                {
-                    problemDetails.Title ??= clientErrorData.Title;
-                    problemDetails.Type ??= clientErrorData.Link;
-                }
-            }
-
-            ProblemDetailsClientErrorFactory.SetTraceId(ControllerContext, problemDetails);
-        }
-
         /// <summary>
         /// Creates an <see cref="BadRequestObjectResult"/> that produces a <see cref="StatusCodes.Status400BadRequest"/> response.
         /// </summary>
@@ -1884,70 +1889,64 @@ public virtual ActionResult ValidationProblem([ActionResultObjectValue] Validati
         }
 
         /// <summary>
-        /// Creates an <see cref="BadRequestObjectResult"/> that produces a <see cref="StatusCodes.Status400BadRequest"/> response
+        /// Creates an <see cref="ActionResult"/> that produces a <see cref="StatusCodes.Status400BadRequest"/> response
         /// with validation errors from <paramref name="modelStateDictionary"/>.
         /// </summary>
         /// <param name="modelStateDictionary">The <see cref="ModelStateDictionary"/>.</param>
         /// <returns>The created <see cref="BadRequestObjectResult"/> for the response.</returns>
         [NonAction]
         public virtual ActionResult ValidationProblem([ActionResultObjectValue] ModelStateDictionary modelStateDictionary)
-        {
-            if (modelStateDictionary == null)
-            {
-                throw new ArgumentNullException(nameof(modelStateDictionary));
-            }
+             => ValidationProblem(detail: null, modelStateDictionary: modelStateDictionary);
 
-            var validationProblem = new ValidationProblemDetails(modelStateDictionary);
-            ApplyProblemDetailsDefaults(validationProblem, statusCode: 400);
-
-            return new BadRequestObjectResult(validationProblem);
-        }
 
         /// <summary>
-        /// Creates an <see cref="BadRequestObjectResult"/> that produces a <see cref="StatusCodes.Status400BadRequest"/> response
+        /// Creates an <see cref="ActionResult"/> that produces a <see cref="StatusCodes.Status400BadRequest"/> response
         /// with validation errors from <see cref="ModelState"/>.
         /// </summary>
-        /// <returns>The created <see cref="BadRequestObjectResult"/> for the response.</returns>
+        /// <returns>The created <see cref="ActionResult"/> for the response.</returns>
         [NonAction]
-        public virtual ActionResult ValidationProblem() => ValidationProblem(ModelState);
+        public virtual ActionResult ValidationProblem()
+            => ValidationProblem(ModelState);
 
         /// <summary>
-        /// Creates an <see cref="BadRequestObjectResult"/> that produces a <see cref="StatusCodes.Status400BadRequest"/> response
+        /// Creates an <see cref="ActionResult"/> that produces a <see cref="StatusCodes.Status400BadRequest"/> response
         /// with a <see cref="ValidationProblemDetails"/> value.
         /// </summary>
-        /// <param name="title">The value for <see cref="ProblemDetails.Title" />.</param>
-        /// <param name="type">The value for <see cref="ProblemDetails.Type" />.</param>
         /// <param name="detail">The value for <see cref="ProblemDetails.Detail" />.</param>
         /// <param name="instance">The value for <see cref="ProblemDetails.Instance" />.</param>
+        /// <param name="statusCode">The status code.</param>
+        /// <param name="title">The value for <see cref="ProblemDetails.Title" />.</param>
+        /// <param name="type">The value for <see cref="ProblemDetails.Type" />.</param>
         /// <param name="modelStateDictionary">The <see cref="ModelStateDictionary"/>. 
         /// When <see langword="null"/> uses <see cref="ModelState"/>.</param>
-        /// <returns>The created <see cref="BadRequestObjectResult"/> for the response.</returns>
+        /// <returns>The created <see cref="ActionResult"/> for the response.</returns>
         [NonAction]
         public virtual ActionResult ValidationProblem(
-            string detail,
+            string detail = null,
             string instance = null,
+            int? statusCode = null,
             string title = null,
             string type = null,
             [ActionResultObjectValue] ModelStateDictionary modelStateDictionary = null)
         {
             modelStateDictionary ??= ModelState;
 
-            var validationProblem = new ValidationProblemDetails(modelStateDictionary)
-            {
-                Detail = detail,
-                Instance = instance,
-                Type = type,
-            };
+            var validationProblem = ProblemDetailsFactory.CreateValidationProblemDetails(
+                HttpContext,
+                modelStateDictionary,
+                statusCode: statusCode,
+                title: title,
+                type: type,
+                detail: detail,
+                instance: instance);
 
-            if (title != null)
+            if (validationProblem.Status == 400)
             {
-                // ValidationProblemDetails has a Title by default. Do not overwrite it with a null
-                validationProblem.Title = title;
+                // For compatibility with 2.x, continue producing BadRequestObjectResult instances if the status code is 400.
+                return new BadRequestObjectResult(validationProblem);
             }
 
-            ApplyProblemDetailsDefaults(validationProblem, statusCode: 400);
-
-            return new BadRequestObjectResult(validationProblem);
+            return new ObjectResult(validationProblem);
         }
 
         /// <summary>

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

@@ -2,7 +2,6 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
-using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc;
 using Microsoft.AspNetCore.Mvc.Core;
 using Microsoft.AspNetCore.Mvc.Infrastructure;
@@ -12,37 +11,43 @@ namespace Microsoft.Extensions.DependencyInjection
 {
     internal class ApiBehaviorOptionsSetup : IConfigureOptions<ApiBehaviorOptions>
     {
+        private ProblemDetailsFactory _problemDetailsFactory;
+
         public void Configure(ApiBehaviorOptions options)
         {
             if (options == null)
             {
                 throw new ArgumentNullException(nameof(options));
             }
 
-            options.InvalidModelStateResponseFactory = ProblemDetailsInvalidModelStateResponse;
-            ConfigureClientErrorMapping(options);
-
-            IActionResult ProblemDetailsInvalidModelStateResponse(ActionContext context)
+            options.InvalidModelStateResponseFactory = context =>
             {
-                var problemDetails = new ValidationProblemDetails(context.ModelState)
-                {
-                    Status = StatusCodes.Status400BadRequest,
-                };
-
-                if (options.ClientErrorMapping.TryGetValue(400, out var clientErrorData))
-                {
-                    problemDetails.Type = clientErrorData.Link;
-                }
-
-                ProblemDetailsClientErrorFactory.SetTraceId(context, problemDetails);
-
-                var result = new BadRequestObjectResult(problemDetails);
+                // ProblemDetailsFactory depends on the ApiBehaviorOptions instance. We intentionally avoid constructor injecting
+                // it in this options setup to to avoid a DI cycle.
+                _problemDetailsFactory ??= context.HttpContext.RequestServices.GetRequiredService<ProblemDetailsFactory>();
+                return ProblemDetailsInvalidModelStateResponse(_problemDetailsFactory, context);
+            };
 
-                result.ContentTypes.Add("application/problem+json");
-                result.ContentTypes.Add("application/problem+xml");
+            ConfigureClientErrorMapping(options);
+        }
 
-                return result;
+        internal static IActionResult ProblemDetailsInvalidModelStateResponse(ProblemDetailsFactory problemDetailsFactory, ActionContext context)
+        {
+            var problemDetails = problemDetailsFactory.CreateValidationProblemDetails(context.HttpContext, context.ModelState);
+            ObjectResult result;
+            if (problemDetails.Status == 400)
+            {
+                // For compatibility with 2.x, continue producing BadRequestObjectResult instances if the status code is 400.
+                result = new BadRequestObjectResult(problemDetails);
             }
+            else
+            {
+                result = new ObjectResult(problemDetails);
+            }
+            result.ContentTypes.Add("application/problem+json");
+            result.ContentTypes.Add("application/problem+xml");
+
+            return result;
         }
 
         // Internal for unit testing

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

@@ -258,6 +258,7 @@ internal static void AddMvcCoreServices(IServiceCollection services)
             services.TryAddSingleton<IActionResultExecutor<ContentResult>, ContentResultExecutor>();
             services.TryAddSingleton<IActionResultExecutor<JsonResult>, SystemTextJsonResultExecutor>();
             services.TryAddSingleton<IClientErrorFactory, ProblemDetailsClientErrorFactory>();
+            services.TryAddSingleton<ProblemDetailsFactory, DefaultProblemDetailsFactory>();
 
             //
             // Route Handlers

src/Mvc/Mvc.Core/src/Infrastructure/DefaultProblemDetailsFactory.cs

@@ -0,0 +1,97 @@
+// 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.
+
+using System;
+using System.Diagnostics;
+using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.Mvc.ModelBinding;
+using Microsoft.Extensions.Options;
+
+namespace Microsoft.AspNetCore.Mvc.Infrastructure
+{
+    internal sealed class DefaultProblemDetailsFactory : ProblemDetailsFactory
+    {
+        private readonly ApiBehaviorOptions _options;
+
+        public DefaultProblemDetailsFactory(IOptions<ApiBehaviorOptions> options)
+        {
+            _options = options?.Value ?? throw new ArgumentNullException(nameof(options));
+        }
+
+        public override ProblemDetails CreateProblemDetails(
+            HttpContext httpContext,
+            int? statusCode = null,
+            string title = null,
+            string type = null,
+            string detail = null,
+            string instance = null)
+        {
+            statusCode ??= 500;
+
+            var problemDetails = new ProblemDetails
+            {
+                Status = statusCode,
+                Title = title,
+                Type = type,
+                Detail = detail,
+                Instance = instance,
+            };
+
+            ApplyProblemDetailsDefaults(httpContext, problemDetails, statusCode.Value);
+
+            return problemDetails;
+        }
+
+        public override ValidationProblemDetails CreateValidationProblemDetails(
+            HttpContext httpContext,
+            ModelStateDictionary modelStateDictionary,
+            int? statusCode = null,
+            string title = null,
+            string type = null,
+            string detail = null,
+            string instance = null)
+        {
+            if (modelStateDictionary == null)
+            {
+                throw new ArgumentNullException(nameof(modelStateDictionary));
+            }
+
+            statusCode ??= 400;
+
+            var problemDetails = new ValidationProblemDetails(modelStateDictionary)
+            {
+                Status = statusCode,
+                Type = type,
+                Detail = detail,
+                Instance = instance,
+            };
+
+            if (title != null)
+            {
+                // For validation problem details, don't overwrite the default title with null.
+                problemDetails.Title = title;
+            }
+
+            ApplyProblemDetailsDefaults(httpContext, problemDetails, statusCode.Value);
+
+            return problemDetails;
+        }
+
+        private void ApplyProblemDetailsDefaults(HttpContext httpContext, ProblemDetails problemDetails, int statusCode)
+        {
+            problemDetails.Status ??= statusCode;
+
+            if (_options.ClientErrorMapping.TryGetValue(statusCode, out var clientErrorData))
+            {
+                problemDetails.Title ??= clientErrorData.Title;
+                problemDetails.Type ??= clientErrorData.Link;
+            }
+
+            var traceId = Activity.Current?.Id ?? httpContext?.TraceIdentifier;
+            if (traceId != null)
+            {
+                problemDetails.Extensions["traceId"] = traceId;
+            }
+        }
+    }
+}