-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Add support for setting error type via ProducesResponseType attribute #30236
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
Conversation
/// error type. | ||
/// </summary> | ||
/// <value></value> | ||
public bool OverrideDefaultErrorType { get; set; } |
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.
Open to alternative ideas for names here. My creativity puttered and this was the most explicit name that I could come up with.
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.
I think I'll play around with this to get a sense for the bug. Declaring the error behavior here is too far disconnected from the implementation that configures one to make sense.
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.
Another approach is to change the below:
aspnetcore/src/Mvc/Mvc.Core/src/ApplicationModels/ApiBehaviorApplicationModelProvider.cs
Lines 45 to 46 in b98bcea
var defaultErrorType = options.SuppressMapClientErrors ? typeof(void) : typeof(ProblemDetails); |
to read from an option other than SuppressMapClientErrors
.
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
|
|
||
[ProducesResponseType(typeof(void), 401)] | ||
public IActionResult ActionWithVoidTypeAndOverrideDisabled() => Ok(); | ||
|
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.
nit: Extra line
In which situations do you think the old-behavior is useful? From what I understand, the issue is that if an action returns void, the response in swagger is changed from no content to "this operation returns ProblemDetails" |
/// Gets or sets whether or not the default error type should be used when one | ||
/// is expliclty provided as a type in the <see cref="ProducesResponseTypeAttribute"/>. |
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.
If I'm reading this correctly, the flag means the opposite of what the sentence above implies. That is, true
means we use the explicitly-provided one, and false
means we use the global one. Also typo in "explicitly".
/// Gets or sets whether or not the default error type should be used when one | |
/// is expliclty provided as a type in the <see cref="ProducesResponseTypeAttribute"/>. | |
/// Gets or sets whether or not the default error type should be overridden when one | |
/// is explicitly provided as a type in the <see cref="ProducesResponseTypeAttribute"/>. |
Closing in favor of an alternate approach. |
Addresses #7874.
We set the default error type depending on whether or not the user has set the
SuppressMapClientErrors
option (ref). When the option is not configured, we set the default totypeof(ProblemDetails)
.This default gets set via an implicit
ProducesErrorResponseType
attribute on all controllers (ref) and ultimately gets consumed for APIs that produce an error type designated as a client error (ref).Based on the bug reports associated here, this is not the desired behavior. So to work around that I:
OverrideDefaultErrorType
flag to theProducesResponseType
attributeApiResponseTypeProvider
to avoid using the default error type if the above flag is setI chose this approach because: