Feature: Ability to automatically skip null checking for requests DTO in ASP.NET Core

[rodion-m]

First of all thank you very much for a great library!
Here is a case:

1. Create an ASP.NET Core project and add RuntimeNullables package
2. Create endpoint and request data class:

[Post]
public IActionResult Auth(AuthRequest request)
{
    //logic
}

public record AuthRequest(string Something);

3. Send a request to this gateway:

{ "Something": null }

4. Server returned 500 error (cause Something cannot be null), but had to return 400 (Bad Request). So, as you see this is kind of a bug actually.

Of course we can add [NullChecks(false)] attribute to the request class and the bug will disappear, but what if we forget to do that? Again 500? And it's actually not so convenient to add NullChecks to requests explicitly all times.
I suggest to add an ability to automatically skip null checking for requests DTO in ASP.NET Core. Let's discuss an implementation here.

PS. It's interesting that NullGuard.Fody just crashed my app at all when I sent an incorrect request like in the example, but RuntimeNullables only throws an exception and generates 500 (and also customizable). So, I fully migrated from NullGuard.Fody to RuntimeNullables. Thanks again for your work!

[mikernet]

I don't know how I feel about "implicitly" removing null checks from some classes because they are used as an action method parameter. That seems a little too magical and unexpected for me. If null checks are implicitly added everywhere then I think it is necessary to be explicit in removing them otherwise things become unclear. I don't really like the idea of complicating the reasoning required to figure out where null checks are being applied any more than it already is. I would find it unexpected that I need to null check request.Something even though I have declared it as non-nullable while using this library.

The more appropriate resolution here IMO is to fix ASP.NET's response code when an ArgumentNullException is thrown during model deserialization. Given the extensibility points ASP.NET provides I would be somewhat surprised if this wasn't easily achievable.

[mikernet]

If you add [NullChecks(false)] then how are you validating the value in the model? You would either need to null check it manually, in which case you can just make the property nullable, or you need to put a [Required] annotation on it. I might be open to something involving removing null checks on properties that have [Required] applied to them, given that it signals you want to use validation to ensure the value is not null and not a normal null check.

[mikernet]

That would fix the "bug" and also give you proper model validation, but it would also require remembering to explicitly annotate each non-null property with [Required] to get that behavior so less than ideal IMO.

Really I think the optimal solution is to add a RuntimeNullables.AspNet package that has a simple config extension method to setup ASP.NET to properly handle ArgumentNullException during DTO deserialization.

[rodion-m]

That would fix the "bug" and also give you proper model validation, but it would also require remembering to explicitly annotate each non-null property with [Required] to get that behavior so less than ideal IMO.

Really I think the optimal solution is to add a RuntimeNullables.AspNet package that has a simple config extension method to setup ASP.NET to properly handle ArgumentNullException during DTO deserialization.

Hi @mikernet ! Did you use ASP.NET Core 5+ with enabled nullable references types? It validates non-nullable fields automatically, so you don't need to specify [Required] attribute explicitly. So, a server just returns 400 with an error description. But with your library server became return 500, as I mentioned before.

I don't ask to ignore null-checking for all request classes, I just suggest to consider some mechanism that will allow to exclude specific classes from null checking. For example I usually derive all requests from IServerRequest interface - it'll be cool if we can define that all classes, derived from IServerRequest will not include a null checking in the constructor.

[mikernet]

it'll be cool if we can define that all classes, derived from IServerRequest will not include a null checking in the constructor

How is that different than applying [NullChecks(false)]?

[mikernet]

Oh I see, you're saying you already derive it from that interface so it wouldn't require an extra step.

it'll be cool if we can define that all classes, derived from IServerRequest

My concern here is that in addition to creating conditions that would make it difficult to ascertain what is null checked when looking at a code file, this adds layers of complexity for relatively minor gain (IMO). For example, what happens if you say ISomething should have null checks and ISomethingElse shouldn't and a class inherits both? Or what if you say a base class should have null checks but then a subclass implements an interface that says it shouldn't?

I still think fixing or tapping into ASP.NET's deserialization mechanism is the clean way to accomplish this without any of the headaches above.

[rodion-m]

I still think fixing or tapping into ASP.NET's deserialization mechanism is the clean way to accomplish this without any of the headaches above.

Well, I don't want to make some request's fields nullable, because they are not. And it's great that I have ability to make it not nullable, since ASP.NET automatically check them after serialization.

For example, what happens if you say ISomething should have null checks and ISomethingElse shouldn't and a class inherits both?

Well, it's easy actually - if one of the ignored interfaces is implemented then the tool will ignore null checking injection from these constructors.

Or what if you say a base class should have null checks but then a subclass implements an interface that says it shouldn't?

I'm not sure about popularity of this case (multi-inheritance for a request), but even if someone will use it, I believe that the tool should ignore null checking exactly for a class that implements an ignored interface (IServerRequest):

interface INoNullChecking {}

record BaseClass(string Something); //null checking is enabled

record DerivedClass(string Something, string Something2) 
    : BaseClass(Something), INoNullChecking; // null checking in this ctor is disabled

So in this case, if we pass to DerivedClass "a", null! - it'll work fine, but null, null will throw an exception from BaseClass's ctor. Again, it's a strange rare case, I don't know who will do like that.

[mikernet]

FYI: I have an issue open on aspnetcore to address this issue: dotnet/aspnetcore#54197