dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.46k stars 10.03k forks source link

Way to generate 4xx response when non-nullable model property setter throws ArgumentNullException #54197

Open mikernet opened 8 months ago

mikernet commented 8 months ago

Not sure how to categorize this issue, but it comes down this: what is the best way to (or how can I) make it so that if a model binder (whether that is request body JSON deserializion or form fields or whatever) gets an ArgumentNullException when attempting to set a non-nullable model property, MVC/web api/minimal API will return a 4xx response code? For example:

public class SomeDTO
{
    private string _someValue = string.Empty;

    public string SomeValue
    {
        get => _someValue;
        set => _someValue = value ?? throw new ArgumentNullException(nameof(value));
    }
}

JSON Payload:

{
    "SomeValue" = null
}

Right now, deserialization of this DTO causes a 500 internal server error response, which is not really appropriate given the circumstances. This is problematic when projects use something like NullGuard.Fody or RuntimeNullables.Fody where non-nullable reference type properties get automatic null checks injected into the setters. It should not be required to remove these null checks on DTOs.

I would like to provide a low-impact RuntimeNullables.AspNet package that users can add, which will provide an extension method that can be called on startup configuration such as app.UseNullableModelPropertyHandling() or a similar approach that will provide the desired behavior in this situation, which is to return a 4xx reponse with a message indicating what property had been invalidly attempted to be set to a null value, but I don't know what approach would be ideal for this.

Minimal APIs appear to pose additional constraints on approaches given the source generators and interceptors.

Is there a way for me to accomplish this?

mikernet commented 8 months ago

Or, alternatively, could the built-in model binders handle this situation more appropriately so I don't need to provide that package at all?

captainsafia commented 8 months ago

what is the best way to (or how can I) make it so that if a model binder (whether that is request body JSON deserializion or form fields or whatever) gets an ArgumentNullException when attempting to set a non-nullable model property, MVC/web api/minimal API will return a 4xx response code?

Am I understanding correctly that the gist of what you are trying to do is return 4xx status code when a required property is not set?

I don't quite grok why are you are including ?? throw new ArgumentNullException(nameof(value)); in your setters given requiredness checks in minimal APIs and MVC will handle this.

A repro would be helpful here so I can poke around at the API you are trying to create.

mikernet commented 8 months ago

@captainsafia Tools like NullGuard.Fody and our RuntimeNullables library automatically inject null checks that throw ArgumentNullException into non-nullable method parameters (including set_PropertyName(value) prop setter methods), effectively turning nullable annotations into runtime checks. The code demonstrated for the DTO that throws ANE when value is null is an example of the equivalent code that is generated by these tools.

Classes can be opted out of this injection, but there are several benefits to being able to leave them in and still have a well-behaved application. It's not an internal server error when a null value is passed to a non-nullable property that throws ANE, it should be a bad request.

I am looking for a way to achieve this.

mikernet commented 8 months ago

All I need to make this work is a way to detect if ANE is thrown during model binding so I can return a different response, but I need to be able to ascertain that it was during model binding and not during other parts of the endpoint execution. Is there a relatively simple and unobtrusive way for me to do that?

captainsafia commented 8 months ago

Tools like NullGuard.Fody and our RuntimeNullables library automatically inject null checks that throw ArgumentNullException into non-nullable method parameters (including set_PropertyName(value) prop setter methods), effectively turning nullable annotations into runtime checks. The code demonstrated for the DTO that throws ANE when value is null is an example of the equivalent code that is generated by these tools.

Ah -- got it! You're system is applying a stricter set of checks than what minimal APIs/MVC do by default.

My first instinct is to recommend using some sort of exception middleware and catching the ArgumentNullException and pre-processing it from there. That's probably the earliest and most common point in the stack where you can do something like this. Have you looked into that approach?

mikernet commented 8 months ago

Yup, that's right :)

The issue with the middleware approach is that I can't differentiate between ANE thrown during model binding, and ANE thrown during execution of the endpoint (or maybe I can, but I'm not sure how). If a developer bug causes ANE to be thrown during execution of the actual endpoint, it should be a 500 internal server error.

captainsafia commented 8 months ago

Yes, sorry, you mentioned this constraint in your original message but I hadn't groked it while I was processing the rest of the message.

I don't have any great ideas for how to work around this outside of maybe throwing a custom exception type -- although that's not fantastic.

Is there a chance you might be able to provide a simple repro project with the tool that you are using? I can dig around to see if there might be an alternative solution.