-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Let JsonException bubble through problem details in minimal APIs #66519
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -291,4 +291,16 @@ public async Task FileUpload_Fails_WithoutAntiforgeryToken() | |
| // Assert | ||
| await response.AssertStatusCodeAsync(HttpStatusCode.BadRequest); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData("/post-required-minimal")] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should keep RDF tests in the RDF area https://github.com/dotnet/aspnetcore/tree/main/src/Http/Http.Extensions/test Should also check that RDG behavior matches probably would put the test in |
||
| [InlineData("/post-required-mvc")] | ||
| public async Task PostWithRequiredProperty(string endpoint) | ||
| { | ||
| var response = await Client.PostAsJsonAsync(endpoint, new { }); | ||
| var responseString = await response.Content.ReadAsStringAsync(); | ||
| Assert.Matches(""" | ||
| {"type":"https://tools.ietf.org/html/rfc9110#section-15.5.1","title":"One or more validation errors occurred\.","status":400,"errors":{"\$":\["JSON deserialization for type 'ModelWithRequiredProperty' was missing required properties including: 'prop'\."]},"traceId":".+?"} | ||
| """, responseString); | ||
|
Youssef1313 marked this conversation as resolved.
|
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While looking around, I noticed we don't set the
Statuson the problem details in the validation filterhttps://source.dot.net/#Microsoft.AspNetCore.Routing/[ValidationEndpointFilterFactory.cs](https://source.dot.net/#Microsoft.AspNetCore.Routing/ValidationEndpointFilterFactory.cs,99),99
Should probably do that, either here or a separate change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, looks like this will eventually get applied anyways when it reaches to:
aspnetcore/src/Http/Http.Extensions/src/DefaultProblemDetailsWriter.cs
Line 56 in 05434a9
aspnetcore/src/Shared/ProblemDetails/ProblemDetailsDefaults.cs
Line 179 in 05434a9
This is assuming that the
DefaultProblemDetailsWriteris used and not any other custom writer that's written and registered by the user. For the case of a custom implementation, I'm not sure yet, I'll try to come up with a test to demonstrate how the behavior can be different. I assume we will need to also have some consistency there between what MVC does and what minimal API does.Thanks for pointing that out @BrennanConroy!