Skip to content

API Analyzer doesn't recognize ControllerBase.ValidationProblem. #6061

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
ghost opened this issue Dec 20, 2018 · 7 comments · Fixed by #39255
Closed

API Analyzer doesn't recognize ControllerBase.ValidationProblem. #6061

ghost opened this issue Dec 20, 2018 · 7 comments · Fixed by #39255
Assignees
Labels
affected-few This issue impacts only small number of customers analyzer Indicates an issue which is related to analyzer experience bug This issue describes a behavior which is not expected - a bug. feature-openapi investigate old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels severity-minor This label is used by an internal tool
Milestone

Comments

@ghost
Copy link

ghost commented Dec 20, 2018

Describe the bug

The Web API Analyzer doesn't recognize ControllerBase.ValidationProblem() and does not prompt a codefix to add a [ProducesResponseType] attribute like it does for ControllerBase.BadRequest().

To Reproduce

Steps to reproduce the behavior:

  1. Using ASP.NET Core 2.2
  2. Install Microsoft.AspNetCore.Mvc.Api.Analyzers 2.2.0
  3. Add return ValidationProblem(); to a controller marked with [ApiController].

Expected behavior

Analyzer catches the issue and underlines return ValidationProblem();
Codefix prompt to add appropriate attributes to action.

Additional context

Using ASP.NET Core 2.2 and Microsoft.AspNetCore.Mvc.Api.Analyzers 2.2.0. Since the default ModelStateInvalidFilter uses ValidationProblemDetails to build a 400 response object, it makes sense that people may want to do the same for validation problems within actions so that the response format is consistent in both cases. Alternatively or additionally, there should be a way to opt into ControllerBase.BadRequest(ModelState) using ValidationProblemDetails to build its response object for consistency with ModelStateInvalidFilter.

@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Dec 20, 2018
@mkArtakMSFT
Copy link
Member

Thanks for contacting us, @webwizgordon.
@pranavkm, can you please look into this? Thanks!

@khellang
Copy link
Member

khellang commented Jan 3, 2019

ApiControllerBase, is that a custom base class in your app? Sounds like the same issue as #4410?

@ghost
Copy link
Author

ghost commented Jan 9, 2019

Sorry, that was a typo. It should be ControllerBase i.e. the built-in base class for API controllers. I've updated the original comment.

@khellang
Copy link
Member

khellang commented Jan 9, 2019

@ghost
Copy link
Author

ghost commented Jan 9, 2019

Is it intentional that ValidationProblem returns ActionResult instead of a more specific result type? Every other method in ControllerBase returns a specific result type.

@khellang
Copy link
Member

khellang commented Jan 9, 2019

Is it intentional that ValidationProblem returns ActionResult instead of a more specific result type?

I'm not sure. I tried to find the PR adding the method to see whether it had been discussed. @pranavkm probably knows.

I'm guessing it might be because some people like to use 422 instead of 400, so you could override the method and return a different result.

@pranavkm
Copy link
Contributor

pranavkm commented Jan 9, 2019

I'm guessing it might be because some people like to use 422 instead of 400, so you could override the method and return a different result.

Yes, that was the intent.

There are other issues, such as #4950, where the analyzer could infer the status code of an ActionResult returning method by diving into the method call, at least in fairly simple scenarios. Perhaps that would be the fix here.

@mkArtakMSFT mkArtakMSFT modified the milestone: 3.0.0-preview5 Feb 28, 2019
@mkArtakMSFT mkArtakMSFT added this to the Backlog milestone Jul 3, 2019
@mkArtakMSFT mkArtakMSFT added bug This issue describes a behavior which is not expected - a bug. PRI: 3 - Optional and removed investigate labels Jul 3, 2019
@javiercn javiercn added affected-few This issue impacts only small number of customers severity-minor This label is used by an internal tool labels Feb 19, 2021 — with ASP.NET Core Issue Ranking
@javiercn javiercn added analyzer Indicates an issue which is related to analyzer experience feature-openapi labels Apr 18, 2021
@mkArtakMSFT mkArtakMSFT added old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Oct 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-few This issue impacts only small number of customers analyzer Indicates an issue which is related to analyzer experience bug This issue describes a behavior which is not expected - a bug. feature-openapi investigate old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels severity-minor This label is used by an internal tool
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants