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.09k stars 9.9k forks source link

Please improve error message on deserialization problem #50439

Closed macias closed 8 months ago

macias commented 12 months ago

Is there an existing issue for this?

Is your feature request related to a problem? Please describe the problem.

Consider such type:

public sealed class CategorySaveRequest
{
    public required List<Category> Categories { get; set; } 
}

I could force C# compiler to accept null as Categories. When doing pure serialization/deserialization it does not cause problem, but when I send a corrupted (with null) data to controller, with method like:

public  ValueTask<Result<CategorySaveResponse>> SaveCategoriesAsync([FromBody] CategorySaveRequest request)

I will get an error:

{"type":"https://tools.ietf.org/html/rfc7231#section-6.5.1","title":"One or more validation errors occurred.","status":400,"traceId":"00-04c1e53cf1ace50dfdea3e139768c874-15202f87d156e377-00","errors":{"Categories":["The Categories field is required."]}}

The error message indicates that the code knows perfectly well which type to instantiate, however pointing out only member name causes the message to be more vague than someone would like to receive (just imagine 10 request types with Categories member).

Describe the solution you'd like

Please enrich message error, something along the lines:

{"type":"https://tools.ietf.org/html/rfc7231#section-6.5.1","title":"One or more validation errors occurred.","status":400,"traceId":"00-04c1e53cf1ace50dfdea3e139768c874-15202f87d156e377-00","errors":{"Categories":["The Categories property of type CategorySaveRequest is required."]}}

Side note: term "field" in original message is incorrect, but this issue is not so important.

Additional context

No response

captainsafia commented 12 months ago

@macias The error message that you see here is controlled by MVC's model binding layer. What version of .NET are you running? There might be some APIs to help you here.

Also, a full repro with a sample code + your .NET version would be helpful.

ghost commented 12 months ago

Hi @macias. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

macias commented 12 months ago

@captainsafia The framework is NET 7.0. Sample code -- please take a look here https://codeberg.org/macias/Demo/src/branch/master/AspNetVagueErrorDeserialization

This is a bit modified template Blazor WebAsm project. Please run it, open web page for it, click on "Fetch data" menu (from left pane), open DevTools pane in the browser, and then click on button "POST". You will get error message saying:

{"type":"https://tools.ietf.org/html/rfc7231#section-6.5.1","title":"One or more validation errors occurred.","status":400,"traceId":"00-f1f32066307a46f841dd1dbbd037fdff-997ff44e913b8e82-00","errors":{"Summary":["The Summary field is required."]}}

So, as my report states, the "parent" type name is missing here.

captainsafia commented 9 months ago

@macias You can modify the behavior of the generated response by configuring InvalidModelStateResponseFactory via the ConfigureApiBehaviorOptions extension method. You should be able to modify the keys and values for the error message from there.

You can also try returning a custom ValidationProblemResult from the action with your desired values but the former option might be more generic.

macias commented 9 months ago

@captainsafia thank you for the reply. This is useful of course but my point is to make it provide information out of the box. Or in other words, the defaults for any library (IMHO) should be useful, informative one, so there would be no need for tweaking right from the start. And this case falls into this category.

IMHO this issue is not resolved.

captainsafia commented 9 months ago

@macias Yep, I understand your original intent. The challenge here is that it's a matter of opinion whether the format desired above should be standard out-of-the-box. The fact that we provide an API for customization here gives us the flexibility to enable people to what's right for their use case without taking on the burden of implementing the one right approach inbox.

When possible, we always try to point people to the customization toggles that are available, instead of modifying the behavior of the framework, which can be breaking in some cases.

macias commented 9 months ago

Just to clarify, this is not about format, but coordinates. Maybe an analogy, you compile C# code, and you get this error:

Cannot assign int to variable x.

How helpful is it? Or would it be better:

File X, line 5, column 17: Cannot assign int to variable x.

?

Same here. The coordinates are missing, so as in the first example I have to actually hunt down occurrences, instead of put a finger immediately at the problematic piece. This day and night experience.

captainsafia commented 9 months ago

Yep, my point being is that the difference between:

{"Categories":["The Categories field is required."]}}

and

{"Categories":["The Categories property of type CategorySaveRequest is required."]}}

from a framework perspective is in the formatting of the error message which can be controlled by the InvalidModelStateResponseFactory API.

ghost commented 8 months ago

This issue has been resolved and has not had any activity for 1 day. It will be closed for housekeeping purposes.

See our Issue Management Policies for more information.