FastEndpoints / FastEndpoints

A light-weight REST API development framework for ASP.NET 6 and newer.
https://fast-endpoints.com
MIT License
4.75k stars 283 forks source link

`required` properties on request models only work for JSON body #669

Closed thorhj closed 7 months ago

thorhj commented 7 months ago

required properties on request models only work for properties that are bound from the request body.

I like to use required in combination with { get; init; } for required properties in my request models. This works fine if the request model is entirely bound from the request body, but if you add required properties that are bound from somewhere else (e.g. query or path parameters), the request will fail with a 400 Bad Request.

Here is a minimal example:

using FastEndpoints;

var builder = WebApplication.CreateBuilder(args);
builder.Services.AddFastEndpoints();

var app = builder.Build();
app.UseFastEndpoints();

app.Run();

public class PatchUserRequest
{
    public required string Name { get; init; }
    public required int Age { get; init; }
}

public class PatchUserEndpoint : Endpoint<PatchUserRequest, string>
{
    public override void Configure()
    {
        Patch("hello/{Name}");
        AllowAnonymous();
    }

    public override async Task HandleAsync(PatchUserRequest req, CancellationToken ct)
    {
        await SendAsync($"Hello, {req.Name}, your age was changed to {req.Age}!", cancellation: ct);
    }
}

In this example I want to bind Name from the path and Age from the request body. Both properties are required. Here is an example request/response:

$ curl --location --request PATCH 'http://localhost:5123/hello/john' --header 'Content-Type: application/json' --data '{"age":2}'
{"statusCode":400,"message":"One or more errors occurred!","errors":{"serializerErrors":["JSON deserialization for type 'PatchUserRequest' was missing required properties, including the following: name"]}}

Now, if I go ahead and remove required from Name, the above request returns 200 OK.

To me it looks like the PatchUserRequest is generated from deserializing the request body, and the remaining properties are then added unto this object. This causes System.Text.Json to fail since there are required properties missing in the request payload. This theory is confirmed by changing the deserializer to use Newtonsoft.Json (which doesn't fail deserialization on a missing required property), in which case the request above returns 200 OK:

app.UseFastEndpoints(c =>
{
    c.Serializer.RequestDeserializer = async (req, tDto, jctx, ct) =>
    {
        using var reader = new StreamReader(req.Body);
        return Newtonsoft.Json.JsonConvert.DeserializeObject(await reader.ReadToEndAsync(), tDto);
    };
});

I think the relevant code is in RequestBinder where the request object is indeed created by deserializing the JSON body.

As mentioned, a workaround is to just not use required on these properties, but this of course causes problems with nullable reference types (Non-nullable property 'Name' must contain a non-null value when exiting constructor. Consider declaring the property as nullable). To avoid this I have to do stupid stuff that I was finally putting to rest with the introduction of required:

public string Name { get; init; } = null!;
dj-nitehawk commented 7 months ago

the logical solution to this conundrum would be to simply throw a [JsonIgnore] attribute on the Name property, but apparently the required keyword seems to take precedence ingnoring the attribute decoration.

so you'll have to resort to a workaround like this:

app.UseFastEndpoints(c => c.Serializer.Options.IgnoreRequiredKeyword())
static class StjExtensions
{
    internal static void IgnoreRequiredKeyword(this JsonSerializerOptions opts)
    {
        opts.TypeInfoResolver = opts.TypeInfoResolver?.WithAddedModifier(
            ti =>
            {
                if (ti.Kind != JsonTypeInfoKind.Object)
                    return;

                for (var i = 0; i < ti.Properties.Count; i++)
                {
                    var pi = ti.Properties[i];
                    if (pi.IsRequired)
                        pi.IsRequired = false;
                }
            });
    }
}

To avoid this I have to do stupid stuff that I was finally putting to rest with the introduction of required

what i usually do is i turn off the warning with an .editorconfig file that disables it for request/response/endpoint classes with a wildcard match. here's an example:

https://github.com/FastEndpoints/FastEndpoints/blob/7533365d3fb8837125cfcb5662a0d3860b7ac2c3/.editorconfig#L20-L21

so yeah, there you go, two options to choose from :-)

thorhj commented 7 months ago

Thanks, I will look into the TypeInfoResolver solution. I don't like removing required from the required properties as that has consequences on other tooling.