Skip to content

Add helper methods on ControllerBase to return ProblemDetails #12298

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Aug 5, 2019
Merged

Add helper methods on ControllerBase to return ProblemDetails #12298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac429f8
Add helper methods on ControllerBase to return ProblemDetails
pranavkm Jul 18, 2019
fbb4bda
Add a factory
pranavkm Jul 18, 2019
8b1f527
Fixup ref assemblies
pranavkm Jul 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ protected ControllerBase() { }
public Microsoft.AspNetCore.Mvc.ModelBinding.IModelBinderFactory ModelBinderFactory { get { throw null; } set { } }
public Microsoft.AspNetCore.Mvc.ModelBinding.ModelStateDictionary ModelState { get { throw null; } }
public Microsoft.AspNetCore.Mvc.ModelBinding.Validation.IObjectModelValidator ObjectValidator { get { throw null; } set { } }
public Microsoft.AspNetCore.Mvc.Infrastructure.ProblemDetailsFactory ProblemDetailsFactory { get { throw null; } set { } }
public Microsoft.AspNetCore.Http.HttpRequest Request { get { throw null; } }
public Microsoft.AspNetCore.Http.HttpResponse Response { get { throw null; } }
public Microsoft.AspNetCore.Routing.RouteData RouteData { get { throw null; } }
Expand Down Expand Up @@ -445,6 +446,8 @@ protected ControllerBase() { }
[Microsoft.AspNetCore.Mvc.NonActionAttribute]
public virtual Microsoft.AspNetCore.Mvc.PhysicalFileResult PhysicalFile(string physicalPath, string contentType, string fileDownloadName, System.DateTimeOffset? lastModified, Microsoft.Net.Http.Headers.EntityTagHeaderValue entityTag, bool enableRangeProcessing) { throw null; }
[Microsoft.AspNetCore.Mvc.NonActionAttribute]
public virtual Microsoft.AspNetCore.Mvc.ObjectResult Problem(string detail = null, string instance = null, int? statusCode = default(int?), string title = null, string type = null) { throw null; }
[Microsoft.AspNetCore.Mvc.NonActionAttribute]
public virtual Microsoft.AspNetCore.Mvc.RedirectResult Redirect(string url) { throw null; }
[Microsoft.AspNetCore.Mvc.NonActionAttribute]
public virtual Microsoft.AspNetCore.Mvc.RedirectResult RedirectPermanent(string url) { throw null; }
Expand Down Expand Up @@ -586,6 +589,8 @@ protected ControllerBase() { }
public virtual Microsoft.AspNetCore.Mvc.ActionResult ValidationProblem([Microsoft.AspNetCore.Mvc.Infrastructure.ActionResultObjectValueAttribute]Microsoft.AspNetCore.Mvc.ModelBinding.ModelStateDictionary modelStateDictionary) { throw null; }
[Microsoft.AspNetCore.Mvc.NonActionAttribute]
public virtual Microsoft.AspNetCore.Mvc.ActionResult ValidationProblem([Microsoft.AspNetCore.Mvc.Infrastructure.ActionResultObjectValueAttribute]Microsoft.AspNetCore.Mvc.ValidationProblemDetails descriptor) { throw null; }
[Microsoft.AspNetCore.Mvc.NonActionAttribute]
public virtual Microsoft.AspNetCore.Mvc.ActionResult ValidationProblem(string detail = null, string instance = null, int? statusCode = default(int?), string title = null, string type = null, [Microsoft.AspNetCore.Mvc.Infrastructure.ActionResultObjectValueAttribute]Microsoft.AspNetCore.Mvc.ModelBinding.ModelStateDictionary modelStateDictionary = null) { throw null; }
}
public partial class ControllerContext : Microsoft.AspNetCore.Mvc.ActionContext
{
Expand Down Expand Up @@ -2438,6 +2443,12 @@ public FileMetadata() { }
public long Length { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } }
}
}
public abstract partial class ProblemDetailsFactory
{
protected ProblemDetailsFactory() { }
public abstract Microsoft.AspNetCore.Mvc.ProblemDetails CreateProblemDetails(Microsoft.AspNetCore.Http.HttpContext httpContext, int? statusCode = default(int?), string title = null, string type = null, string detail = null, string instance = null);
public abstract Microsoft.AspNetCore.Mvc.ValidationProblemDetails CreateValidationProblemDetails(Microsoft.AspNetCore.Http.HttpContext httpContext, Microsoft.AspNetCore.Mvc.ModelBinding.ModelStateDictionary modelStateDictionary, int? statusCode = default(int?), string title = null, string type = null, string detail = null, string instance = null);
}
public partial class RedirectResultExecutor : Microsoft.AspNetCore.Mvc.Infrastructure.IActionResultExecutor<Microsoft.AspNetCore.Mvc.RedirectResult>
{
public RedirectResultExecutor(Microsoft.Extensions.Logging.ILoggerFactory loggerFactory, Microsoft.AspNetCore.Mvc.Routing.IUrlHelperFactory urlHelperFactory) { }
Expand Down
111 changes: 98 additions & 13 deletions src/Mvc/Mvc.Core/src/ControllerBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
using Microsoft.AspNetCore.Mvc.Routing;
using Microsoft.AspNetCore.Routing;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Options;
using Microsoft.Net.Http.Headers;

namespace Microsoft.AspNetCore.Mvc
Expand All @@ -31,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.
Expand Down Expand Up @@ -189,6 +191,28 @@ public IObjectModelValidator ObjectValidator
}
}

public ProblemDetailsFactory ProblemDetailsFactory
{
get
{
if (_problemDetailsFactory == null)
{
_problemDetailsFactory = HttpContext?.RequestServices?.GetRequiredService<ProblemDetailsFactory>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

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>
Expand Down Expand Up @@ -1821,6 +1845,34 @@ public virtual ConflictObjectResult Conflict([ActionResultObjectValue] object er
public virtual ConflictObjectResult Conflict([ActionResultObjectValue] ModelStateDictionary modelState)
=> new ConflictObjectResult(modelState);

/// <summary>
/// Creates an <see cref="ObjectResult"/> that produces a <see cref="ProblemDetails"/> response.
/// </summary>
/// <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 = ProblemDetailsFactory.CreateProblemDetails(
HttpContext,
statusCode: statusCode ?? 500,
title: title,
type: type,
detail: detail,
instance: instance);

return new ObjectResult(problemDetails);
}

/// <summary>
/// Creates an <see cref="BadRequestObjectResult"/> that produces a <see cref="StatusCodes.Status400BadRequest"/> response.
/// </summary>
Expand All @@ -1837,31 +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);
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);

/// <summary>
/// Creates an <see cref="ActionResult"/> that produces a <see cref="StatusCodes.Status400BadRequest"/> response
/// with a <see cref="ValidationProblemDetails"/> value.
/// </summary>
/// <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="ActionResult"/> for the response.</returns>
[NonAction]
public virtual ActionResult ValidationProblem(
string detail = null,
string instance = null,
int? statusCode = null,
string title = null,
string type = null,
[ActionResultObjectValue] ModelStateDictionary modelStateDictionary = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
var validationProblem = new ValidationProblemDetails(ModelState);
return new BadRequestObjectResult(validationProblem);
modelStateDictionary ??= ModelState;

var validationProblem = ProblemDetailsFactory.CreateValidationProblemDetails(
HttpContext,
modelStateDictionary,
statusCode: statusCode,
title: title,
type: type,
detail: detail,
instance: instance);

if (validationProblem.Status == 400)
{
// For compatibility with 2.x, continue producing BadRequestObjectResult instances if the status code is 400.
return new BadRequestObjectResult(validationProblem);
}

return new ObjectResult(validationProblem);
}

/// <summary>
Expand Down
66 changes: 27 additions & 39 deletions src/Mvc/Mvc.Core/src/DependencyInjection/ApiBehaviorOptionsSetup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,16 @@
// 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;
using Microsoft.Extensions.Options;

namespace Microsoft.Extensions.DependencyInjection
{
internal class ApiBehaviorOptionsSetup :
IConfigureOptions<ApiBehaviorOptions>,
IPostConfigureOptions<ApiBehaviorOptions>
internal class ApiBehaviorOptionsSetup : IConfigureOptions<ApiBehaviorOptions>
{
internal static readonly Func<ActionContext, IActionResult> DefaultFactory = DefaultInvalidModelStateResponse;
internal static readonly Func<ActionContext, IActionResult> ProblemDetailsFactory =
ProblemDetailsInvalidModelStateResponse;
private ProblemDetailsFactory _problemDetailsFactory;

public void Configure(ApiBehaviorOptions options)
{
Expand All @@ -25,20 +20,34 @@ public void Configure(ApiBehaviorOptions options)
throw new ArgumentNullException(nameof(options));
}

options.InvalidModelStateResponseFactory = DefaultFactory;
options.InvalidModelStateResponseFactory = context =>
{
// 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);
};

ConfigureClientErrorMapping(options);
}

public void PostConfigure(string name, ApiBehaviorOptions options)
internal static IActionResult ProblemDetailsInvalidModelStateResponse(ProblemDetailsFactory problemDetailsFactory, ActionContext context)
{
// We want to use problem details factory only if
// (a) it has not been opted out of (SuppressMapClientErrors = true)
// (b) a different factory was configured
if (!options.SuppressMapClientErrors &&
object.ReferenceEquals(options.InvalidModelStateResponseFactory, DefaultFactory))
var problemDetails = problemDetailsFactory.CreateValidationProblemDetails(context.HttpContext, context.ModelState);
ObjectResult result;
if (problemDetails.Status == 400)
{
options.InvalidModelStateResponseFactory = ProblemDetailsFactory;
// 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
Expand Down Expand Up @@ -91,33 +100,12 @@ internal static void ConfigureClientErrorMapping(ApiBehaviorOptions options)
Link = "https://tools.ietf.org/html/rfc4918#section-11.2",
Title = Resources.ApiConventions_Title_422,
};
}

private static IActionResult DefaultInvalidModelStateResponse(ActionContext context)
{
var result = new BadRequestObjectResult(context.ModelState);

result.ContentTypes.Add("application/json");
result.ContentTypes.Add("application/xml");

return result;
}

internal static IActionResult ProblemDetailsInvalidModelStateResponse(ActionContext context)
{
var problemDetails = new ValidationProblemDetails(context.ModelState)
options.ClientErrorMapping[500] = new ClientErrorData
{
Status = StatusCodes.Status400BadRequest,
Link = "https://tools.ietf.org/html/rfc7231#section-6.6.1",
Title = Resources.ApiConventions_Title_500,
};

ProblemDetailsClientErrorFactory.SetTraceId(context, problemDetails);

var result = new BadRequestObjectResult(problemDetails);

result.ContentTypes.Add("application/problem+json");
result.ContentTypes.Add("application/problem+xml");

return result;
}
}
}
3 changes: 1 addition & 2 deletions src/Mvc/Mvc.Core/src/DependencyInjection/MvcCoreServiceCollectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,6 @@ internal static void AddMvcCoreServices(IServiceCollection services)
ServiceDescriptor.Transient<IPostConfigureOptions<MvcOptions>, MvcCoreMvcOptionsSetup>());
services.TryAddEnumerable(
ServiceDescriptor.Transient<IConfigureOptions<ApiBehaviorOptions>, ApiBehaviorOptionsSetup>());
services.TryAddEnumerable(
ServiceDescriptor.Transient<IPostConfigureOptions<ApiBehaviorOptions>, ApiBehaviorOptionsSetup>());
services.TryAddEnumerable(
ServiceDescriptor.Transient<IConfigureOptions<RouteOptions>, MvcCoreRouteOptionsSetup>());

Expand Down Expand Up @@ -260,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
Expand Down
Loading