Let JsonException bubble through problem details in minimal APIs#66519
Let JsonException bubble through problem details in minimal APIs#66519Youssef1313 wants to merge 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates minimal API JSON body binding to emit validation-style Problem Details when System.Text.Json throws a JsonException (e.g., missing required properties), aligning the behavior more closely with MVC/controller model binding.
Changes:
- Write
HttpValidationProblemDetailsviaIProblemDetailsServicewhen JSON body deserialization throwsJsonExceptioninRequestDelegateFactory. - Add a functional test validating consistent Problem Details output for both a minimal endpoint and an MVC
[ApiController]endpoint. - Extend the
SimpleWebSiteWithWebApplicationBuildertest app with endpoints/models to reproduce therequiredproperty scenario.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Mvc/test/WebSites/SimpleWebSiteWithWebApplicationBuilder/Program.cs | Adds ProblemDetails services + new endpoints/model to exercise missing required property behavior. |
| src/Mvc/test/Mvc.FunctionalTests/SimpleWithWebApplicationBuilderTests.cs | Adds a functional test asserting Problem Details JSON for both minimal + MVC endpoints. |
| src/Http/Http.Extensions/src/RequestDelegateFactory.cs | Emits HttpValidationProblemDetails on JsonException during minimal API JSON body binding. |
mikekistler
left a comment
There was a problem hiding this comment.
@Youssef1313 Thank you for this contribution!
I will let the engineering team weigh in on the details of this change but I fully support the effort to solve this problem.
|
@BrennanConroy @halter73 @javiercn Can I get a review here please? Thanks. |
| Exception = ex, | ||
| ProblemDetails = new HttpValidationProblemDetails(errors) | ||
| { | ||
| Status = StatusCodes.Status400BadRequest, |
There was a problem hiding this comment.
While looking around, I noticed we don't set the Status on the problem details in the validation filter
https://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.
Hmm, looks like this will eventually get applied anyways when it reaches to:
This is assuming that the DefaultProblemDetailsWriter is 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!
| } | ||
|
|
||
| [Theory] | ||
| [InlineData("/post-required-minimal")] |
There was a problem hiding this comment.
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
https://github.com/dotnet/aspnetcore/blob/main/src/Http/Http.Extensions/test/RequestDelegateGenerator/RequestDelegateCreationTests.JsonBody.cs
so it tests both RDF and RDG
Fixes #64139