jezzsantos / saastack

A comprehensive codebase template for starting your real-world, fully featured SaaS web products. On the .NET platform
The Unlicense
36 stars 9 forks source link

'required' properties in the Request DTO are throwing exceptions #34

Closed jezzsantos closed 4 months ago

jezzsantos commented 4 months ago

A typical Reequest DTO might look like this:

namespace Infrastructure.Web.Api.Operations.Shared.Cars;

[Route("/cars", OperationMethod.Post, AccessType.Token)]
[Authorize(Roles.Tenant_Member, Features.Tenant_PaidTrial)]
public class RegisterCarRequest : TenantedRequest<GetCarResponse>
{
    public required string Jurisdiction { get; set; }

    public required string Make { get; set; }

    public required string Model { get; set; }

    public required string NumberPlate { get; set; }

    public required int Year { get; set; }
}

Notice the use of the required keyword, since this project (and all projects) have <Nullable>enable</Nullable>.

This works at the C# code level, but these DTO objects are populated by ASPNET at runtime due to minimal API registrations like this:

var carsapiGroup = app.MapGroup(string.Empty)
                .WithTags("CarsApi")
                .RequireCors("__DefaultCorsPolicy")
                .AddEndpointFilter<global::Infrastructure.Web.Api.Common.Endpoints.MultiTenancyFilter>()
                .AddEndpointFilter<global::Infrastructure.Web.Api.Common.Endpoints.ApiUsageFilter>()
                .AddEndpointFilter<global::Infrastructure.Web.Api.Common.Endpoints.RequestCorrelationFilter>()
                .AddEndpointFilter<global::Infrastructure.Web.Api.Common.Endpoints.ContentNegotiationFilter>();

.... other methods
            carsapiGroup.MapGet("/cars/{Id}",
                async (global::MediatR.IMediator mediator, [global::Microsoft.AspNetCore.Http.AsParameters] global::Infrastructure.Web.Api.Operations.Shared.Cars.GetCarRequest request) =>
                     await mediator.Send(request, global::System.Threading.CancellationToken.None))
                .RequireAuthorization("Token")
                .RequireCallerAuthorization("POLICY:{|Features|:{|Tenant|:[|basic_features|]},|Roles|:{|Tenant|:[|org_member|]}}")
                .WithOpenApi(op =>
                    {
                        op.OperationId = "GetCar";
                        op.Description = "(request type: GetCarRequest)";
                        op.Responses.Clear();
                        return op;
                    });

            carsapiGroup.MapPost("/cars",
                async (global::MediatR.IMediator mediator, global::Infrastructure.Web.Api.Operations.Shared.Cars.RegisterCarRequest request) =>
                     await mediator.Send(request, global::System.Threading.CancellationToken.None))
                .RequireAuthorization("Token")
                .RequireCallerAuthorization("POLICY:{|Features|:{|Tenant|:[|paidtrial_features|]},|Roles|:{|Tenant|:[|org_member|]}}")
                .WithOpenApi(op =>
                    {
                        op.OperationId = "RegisterCar";
                        op.Description = "(request type: RegisterCarRequest)";
                        op.Responses.Clear();
                        return op;
                    });

The problem comes when we submit either of these requests in any HTTP client tool, like this, with an invalid or incomplete request:

POST https://localhost:5001/cars
Accept: application/json
Content-Type: application/json

{
  "Make": ""
}

Which is clearly missing the required properties like: Make, Model and Year, then we should end up with a HTTP - 400 Validation, not an HTTP - 500!

But, instead, we get one of these HTTP - 500 responses, because APNET (internally) struggles to handle the required keyword when there is no data for that property.

In fact two different exceptions, depending on a couple of things:

This exception is from a GET request that we map to use the [AsParameters] on the request object, where the required property is missing from the URL:

Microsoft.AspNetCore.Http.BadHttpRequestException: Required parameter "string RequiredField" was not provided from query string.
   at Microsoft.AspNetCore.Http.RequestDelegateFactory.Log.RequiredParameterNotProvided(HttpContext httpContext, String parameterTypeName, String parameterName, String source, Boolean shouldThrow)
   at lambda_method274(Closure, Object, HttpContext)
   at Infrastructure.Web.Hosting.Common.Extensions.WebApplicationExtensions.<>c.<<EnableEventingPropagation>b__4_1>d.MoveNext() in C:\Projects\github\jezzsantos\saastack\src\Infrastructure.Web.Hosting.Common\Extensions\WebApplicationExtensions.cs:line 159
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at Infrastructure.Web.Hosting.Common.Pipeline.MultiTenancyMiddleware.InvokeAsync(HttpContext context, ITenancyContext tenancyContext, ICallerContextFactory callerContextFactory, ITenantDetective tenantDetective) in C:\Projects\github\jezzsantos\saastack\src\Infrastructure.Web.Hosting.Common\Pipeline\MultiTenancyMiddleware.cs:line 55
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddlewareImpl.<Invoke>g__Awaited|10_0(ExceptionHandlerMiddlewareImpl middleware, HttpContext context, Task task)

see also: https://github.com/dotnet/aspnetcore/issues/52881#issuecomment-1871456841

This exception is from POST request, where the required property is missing from the body of the request :

"type": "https://tools.ietf.org/html/rfc7231#section-6.6.1",
  "title": "An unexpected error occurred",
  "status": 500,
  "detail": "Failed to read parameter \"RegisterCarRequest request\" from the request body as JSON.",
  "instance": "https://localhost:5001/cars",
  "exception": "Microsoft.AspNetCore.Http.BadHttpRequestException: Failed to read parameter \"RegisterCarRequest request\" from the request body as JSON.\r\n ---> System.Text.Json.JsonException: JSON deserialization for type 'Infrastructure.Web.Api.Operations.Shared.Cars.RegisterCarRequest' was missing required properties, including the following: jurisdiction, model, numberPlate, year\r\n   at System.Text.Json.ThrowHelper.ThrowJsonException_JsonRequiredPropertyMissing(JsonTypeInfo parent, BitArray requiredPropertiesSet)\r\n   at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)\r\n   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value, Boolean& isPopulatedValue)\r\n   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)\r\n   at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1.ContinueDeserialize(ReadBufferState& bufferState, JsonReaderState& jsonReaderState, ReadStack& readStack)\r\n   at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1.DeserializeAsync(Stream utf8Json, CancellationToken cancellationToken)\r\n   at System.Text.Json.Serialization.Metadata.JsonTypeInfo`1.DeserializeAsObjectAsync(Stream utf8Json, CancellationToken cancellationToken)\r\n   at Microsoft.AspNetCore.Http.HttpRequestJsonExtensions.ReadFromJsonAsync(HttpRequest request, JsonTypeInfo jsonTypeInfo, CancellationToken cancellationToken)\r\n   at Microsoft.AspNetCore.Http.HttpRequestJsonExtensions.ReadFromJsonAsync(HttpRequest request, JsonTypeInfo jsonTypeInfo, CancellationToken cancellationToken)\r\n   at Microsoft.AspNetCore.Http.RequestDelegateFactory.<HandleRequestBodyAndCompileRequestDelegateForJson>g__TryReadBodyAsync|102_0(HttpContext httpContext, Type bodyType, String parameterTypeName, String parameterName, Boolean allowEmptyRequestBody, Boolean throwOnBadRequest, JsonTypeInfo jsonTypeInfo)\r\n   --- End of inner exception stack trace ---\r\n   at Microsoft.AspNetCore.Http.RequestDelegateFactory.Log.InvalidJsonRequestBody(HttpContext httpContext, String parameterTypeName, String parameterName, Exception exception, Boolean shouldThrow)\r\n   at Microsoft.AspNetCore.Http.RequestDelegateFactory.<HandleRequestBodyAndCompileRequestDelegateForJson>g__TryReadBodyAsync|102_0(HttpContext httpContext, Type bodyType, String parameterTypeName, String parameterName, Boolean allowEmptyRequestBody, Boolean throwOnBadRequest, JsonTypeInfo jsonTypeInfo)\r\n   at Microsoft.AspNetCore.Http.RequestDelegateFactory.<>c__DisplayClass102_2.<<HandleRequestBodyAndCompileRequestDelegateForJson>b__2>d.MoveNext()\r\n--- End of stack trace from previous location ---\r\n   at Infrastructure.Web.Hosting.Common.Extensions.WebApplicationExtensions.<>c.<<EnableEventingPropagation>b__4_1>d.MoveNext() in C:\\Projects\\github\\jezzsantos\\saastack\\src\\Infrastructure.Web.Hosting.Common\\Extensions\\WebApplicationExtensions.cs:line 159\r\n--- End of stack trace from previous location ---\r\n   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)\r\n   at Infrastructure.Web.Hosting.Common.Pipeline.MultiTenancyMiddleware.InvokeAsync(HttpContext context, ITenancyContext tenancyContext, ICallerContextFactory callerContextFactory, ITenantDetective tenantDetective) in C:\\Projects\\github\\jezzsantos\\saastack\\src\\Infrastructure.Web.Hosting.Common\\Pipeline\\MultiTenancyMiddleware.cs:line 55\r\n   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)\r\n   at Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddlewareImpl.<Invoke>g__Awaited|10_0(ExceptionHandlerMiddlewareImpl middleware, HttpContext context, Task task)"
}

see also this: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/required-properties as to why this is being honored

Solution

We need to resolve this, one way or the other, since an inbound HTTP may be missing any data, and we want the appropriate HTTP response in all these cases:

  1. GET /resource/{Id} when the Id property is null/missing from the request -> HTTP 404 NotFound (since no effective route can be matched)
  2. GET /resource/{Id} when another property (i.e. Make) is missing from the request -> HTTP 400 BadRequest (since the validator kicks in)
  3. POST /resource/{Id} when the Id property is null/missing from the request -> HTTP 404 NotFound (since no effective route can be matched)
  4. POST /resource/{Id} when another property (i.e. Make) is missing from the request -> HTTP 400 BadRequest (since the validator kicks in)

At present, the only workable solution is this:

  1. Forbid the usage of the required keyword in all Request DTOs, and turn off nullability (i.e. <nullable>disabled</nullable> or use #pragma CS8618, either for each class or for the whole assembly).
  2. Also, for all GET requests, we would also need to make all parameters in the request DTO be string? to bypass the issues with the [AsParameters] exception (since GET requests do not support bodies).

In either GET or POST requests parameters like Id that are used in the route path can be declared as string or string? it makes no difference. However, in all GET, DELETE requests, all properties of the requestDTO must be string?.

jezzsantos commented 4 months ago

Since both [AsParameters] and the System.Text.Json deserializer both act upon whether a property is using the required modifier, AND both throw (albeit different) exceptions if the data is not present, AND neither behavior can be overridden or disabled, we are left with really only two known options to move forward:

I believe we favour the first option (first) to minimize the amount of customization we are making to the runtime, since these things may be fixed in subsequent versions of ASPNET