Skip to content

Improve error messages for implicit and explicit FromBody attribute in Minimal API #35086 #35188

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.

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 12 commits into from
Aug 17, 2021
Merged
19 changes: 14 additions & 5 deletions src/Http/Http.Extensions/src/HttpRequestJsonExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@
using System;
using System.Diagnostics.CodeAnalysis;
using System.IO;
using System.Net.Http;
using System.Text;
using System.Text.Json;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Http.Features;
using Microsoft.AspNetCore.Http.Json;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Options;
Expand Down Expand Up @@ -59,7 +61,7 @@ public static class HttpRequestJsonExtensions

if (!request.HasJsonContentType(out var charset))
{
throw CreateContentTypeError(request);
throw CreateContentTypeError(request, null);
}

options ??= ResolveSerializerOptions(request.HttpContext);
Expand Down Expand Up @@ -124,7 +126,7 @@ public static class HttpRequestJsonExtensions

if (!request.HasJsonContentType(out var charset))
{
throw CreateContentTypeError(request);
throw CreateContentTypeError(request, type);
}

options ??= ResolveSerializerOptions(request.HttpContext);
Expand All @@ -135,7 +137,7 @@ public static class HttpRequestJsonExtensions
try
{
return await JsonSerializer.DeserializeAsync(inputStream, type, options, cancellationToken);
}
}
finally
{
if (usesTranscodingStream)
Expand Down Expand Up @@ -192,8 +194,15 @@ private static JsonSerializerOptions ResolveSerializerOptions(HttpContext httpCo
return httpContext.RequestServices?.GetService<IOptions<JsonOptions>>()?.Value?.SerializerOptions ?? JsonOptions.DefaultSerializerOptions;
}

private static InvalidOperationException CreateContentTypeError(HttpRequest request)
{
private static InvalidOperationException CreateContentTypeError(HttpRequest request, Type? type)
{
var feature = request.HttpContext.Features.Get<IHttpRequestBodyDetectionFeature>();

//if there is no content-type and the content-length is zero, it might be safe to assume that the parameter is a service that the user forgot to register with the DI
if (feature?.CanHaveBody == false)
{
return new InvalidOperationException($"We failed to infer or bind '{type?.FullName}' parameter. Was this a Service parameter that you forgot to register ?");
}
return new InvalidOperationException($"Unable to read the request as JSON because the request content type '{request.ContentType}' is not a known JSON content type.");
}

Expand Down
37 changes: 32 additions & 5 deletions src/Http/Http.Extensions/src/RequestDelegateFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Linq.Expressions;
using System.Reflection;
using System.Security.Claims;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Http.Features;
Expand Down Expand Up @@ -194,6 +195,20 @@ private static Expression[] CreateArguments(ParameterInfo[]? parameters, Factory
args[i] = CreateArgument(parameters[i], factoryContext);
}

if(factoryContext.HasAnotherBodyParameter)
{
var errorMessage = new StringBuilder();
errorMessage.Append($"Failure to infer one or more parameters. Below is the list of parameters we found: \n\n");
errorMessage.Append(string.Format("{0,6} {1,15}\n\n", "Parameter", "Source"));

foreach (KeyValuePair<string, string> kv in factoryContext.TrackedParameters)
{
errorMessage.Append(string.Format("{0,6} {1,15:N0}\n", kv.Key, kv.Value));
}
//Could also throw a custom exception message if needed.
throw new InvalidOperationException(errorMessage.ToString());
}

return args;
}

Expand All @@ -208,6 +223,7 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext

if (parameterCustomAttributes.OfType<IFromRouteMetadata>().FirstOrDefault() is { } routeAttribute)
{
factoryContext.TrackedParameters.Add(parameter.Name, "Route Attribute");
if (factoryContext.RouteParameters is { } routeParams && !routeParams.Contains(parameter.Name, StringComparer.OrdinalIgnoreCase))
{
throw new InvalidOperationException($"{parameter.Name} is not a route paramter.");
Expand All @@ -217,18 +233,22 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext
}
else if (parameterCustomAttributes.OfType<IFromQueryMetadata>().FirstOrDefault() is { } queryAttribute)
{
factoryContext.TrackedParameters.Add(parameter.Name, "Query Attribute");
return BindParameterFromProperty(parameter, QueryExpr, queryAttribute.Name ?? parameter.Name, factoryContext);
}
else if (parameterCustomAttributes.OfType<IFromHeaderMetadata>().FirstOrDefault() is { } headerAttribute)
{
factoryContext.TrackedParameters.Add(parameter.Name, "Header Attribute");
return BindParameterFromProperty(parameter, HeadersExpr, headerAttribute.Name ?? parameter.Name, factoryContext);
}
else if (parameterCustomAttributes.OfType<IFromBodyMetadata>().FirstOrDefault() is { } bodyAttribute)
{
factoryContext.TrackedParameters.Add(parameter.Name, "Body Attribute");
return BindParameterFromBody(parameter, bodyAttribute.AllowEmpty, factoryContext);
}
else if (parameter.CustomAttributes.Any(a => typeof(IFromServiceMetadata).IsAssignableFrom(a.AttributeType)))
{
factoryContext.TrackedParameters.Add(parameter.Name, "From Services");
return BindParameterFromService(parameter);
}
else if (parameter.ParameterType == typeof(HttpContext))
Expand Down Expand Up @@ -257,9 +277,11 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext
// to query string in this case
if (factoryContext.RouteParameters is { } routeParams && routeParams.Contains(parameter.Name, StringComparer.OrdinalIgnoreCase))
{
factoryContext.TrackedParameters.Add(parameter.Name, "Route Parameter");
return BindParameterFromProperty(parameter, RouteValuesExpr, parameter.Name, factoryContext);
}

factoryContext.TrackedParameters.Add(parameter.Name, "Query String Parameter");
return BindParameterFromRouteValueOrQueryString(parameter, parameter.Name, factoryContext);
}
else
Expand All @@ -268,6 +290,7 @@ private static Expression CreateArgument(ParameterInfo parameter, FactoryContext
{
if (serviceProviderIsService.IsService(parameter.ParameterType))
{
factoryContext.TrackedParameters.Add(parameter.Name, "Service Parameter");
return Expression.Call(GetRequiredServiceMethod.MakeGenericMethod(parameter.ParameterType), RequestServicesExpr);
}
}
Expand Down Expand Up @@ -489,9 +512,6 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall,
{
object? bodyValue = defaultBodyValue;

var feature = httpContext.Features.Get<IHttpRequestBodyDetectionFeature>();
if (feature?.CanHaveBody == true)
{
try
{
bodyValue = await httpContext.Request.ReadFromJsonAsync(bodyType);
Expand All @@ -503,11 +523,11 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall,
}
catch (InvalidDataException ex)
{

Log.RequestBodyInvalidDataException(httpContext, ex);
httpContext.Response.StatusCode = 400;
return;
}
}

await invoker(target, httpContext, bodyValue);
};
Expand Down Expand Up @@ -711,7 +731,10 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al
{
if (factoryContext.JsonRequestBodyType is not null)
{
throw new InvalidOperationException("Action cannot have more than one FromBody attribute.");
factoryContext.HasAnotherBodyParameter = true;
#pragma warning disable CS8604 // Possible null reference argument.
factoryContext.TrackedParameters.Add(parameter.Name, "We failed to infer this parameter. Did you forget to inject it as a Service?");
#pragma warning restore CS8604 // Possible null reference argument.
}

var nullability = NullabilityContext.Create(parameter);
Expand Down Expand Up @@ -954,6 +977,10 @@ private class FactoryContext

public bool UsingTempSourceString { get; set; }
public List<(ParameterExpression, Expression)> CheckParams { get; } = new();

public Dictionary<string, string> TrackedParameters { get; } = new();

public bool HasAnotherBodyParameter { get; set; }
}

private static partial class Log
Expand Down