Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Feb 17, 2021

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 to typeof(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:

  • Added an opt-in OverrideDefaultErrorType flag to the ProducesResponseType attribute
  • Added some logic in the ApiResponseTypeProvider to avoid using the default error type if the above flag is set

I chose this approach because:

  • I wanted to reduce the impact of the breaking change so I made the new strategy opt-in instead of opt-out
  • I wanted to localize the setting to the affected attributes

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Feb 17, 2021
/// error type.
/// </summary>
/// <value></value>
public bool OverrideDefaultErrorType { get; set; }
Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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:

var defaultErrorType = options.SuppressMapClientErrors ? typeof(void) : typeof(ProblemDetails);

to read from an option other than SuppressMapClientErrors.

@captainsafia captainsafia marked this pull request as ready for review February 17, 2021 19:10
@captainsafia captainsafia added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Feb 17, 2021
@ghost
Copy link

ghost commented Feb 17, 2021

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:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.


[ProducesResponseType(typeof(void), 401)]
public IActionResult ActionWithVoidTypeAndOverrideDisabled() => Ok();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Extra line

@javiercn
Copy link
Member

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"

Comment on lines +59 to +60
/// 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"/>.
Copy link
Member

@SteveSandersonMS SteveSandersonMS Feb 22, 2021

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".

Suggested change
/// 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"/>.

@captainsafia captainsafia removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Feb 26, 2021
@captainsafia
Copy link
Member Author

Closing in favor of an alternate approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants