Skip to content
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

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

Closed
jezzsantos opened this issue May 11, 2024 · 1 comment
Closed
Labels
design defect Something does not work the best way, or is missing something

Comments

@jezzsantos
Copy link
Owner

jezzsantos commented May 11, 2024

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: dotnet/aspnetcore#52881 (comment)

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 jezzsantos added the design defect Something does not work the best way, or is missing something label May 11, 2024
@jezzsantos jezzsantos changed the title Nullable 'required' properties in the Request DTO are throwing exceptions 'required' properties in the Request DTO are throwing exceptions May 11, 2024
@jezzsantos
Copy link
Owner Author

jezzsantos commented May 12, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design defect Something does not work the best way, or is missing something
Projects
None yet
Development

No branches or pull requests

1 participant