Skip to content

API analyzers should support undeclared 200 status codes #15385

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

Open
Tracked by #27889
jawn opened this issue Oct 24, 2019 · 8 comments
Open
Tracked by #27889

API analyzers should support undeclared 200 status codes #15385

jawn opened this issue Oct 24, 2019 · 8 comments
Labels
affected-few This issue impacts only small number of customers analyzer Indicates an issue which is related to analyzer experience area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-openapi Priority:3 Work that is nice to have severity-blocking This label is used by an internal tool
Milestone

Comments

@jawn
Copy link

jawn commented Oct 24, 2019

Describe the bug

API analyzers should flag an undeclared 200 status code, and provide an option to fix it.

To Reproduce

Steps to reproduce the behavior:

  1. Using ASP.NET Core 3.0 with analyzers enabled
<PropertyGroup>
 <IncludeOpenAPIAnalyzers>true</IncludeOpenAPIAnalyzers>
</PropertyGroup>
  1. Run this code (adapted from this sample with the ProducesResponseType attribute removed)
// GET api/contacts/{guid}
[HttpGet("{id}", Name = "GetById")]
public IActionResult Get(string id)
{
    var contact = _contacts.Get(id);

    if (contact == null)
    {
        return NotFound();
    }

    return Ok(contact);
}

  1. The API analyzer flags the NotFound using a warning (this is expected).
  2. The API analyzer does not flag the return Ok(...); (not expected)

Expected behavior

  1. The API analyzer does flag the return Ok(...) using a warning.
  2. An option to fix this warning is provided, choosing it adds the following attribute
    [ProducesResponseType(typeof(Contact), StatusCodes.Status200OK)]

Why I think this is a bug

From the docs:

The analyzers package notifies you of any controller action that:

  • Returns an undeclared status code.
  • Returns an undeclared success result.

Additional context

Follow up to dotnet/AspNetCore.Docs#15269 (comment)

.NET Core SDK: 3.0.100
.NET Core runtimes installed:
Microsoft.NETCore.App 3.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.WindowsDesktop.App 3.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

@jawn jawn changed the title API analyzer should flag and add ProductResponseType(typeof(...), StatusCodes200OK) API analyzers should support undeclared 200 status codes Oct 24, 2019
@pranavkm
Copy link
Contributor

The 200 Status code is implicit for every action that does not specify any ProducesResponseType attributes. Admittedly it might a bit confusing for it to not flag the response type, but the analyzer inspect those.

@javiercn javiercn added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Oct 24, 2019
@jawn
Copy link
Author

jawn commented Oct 25, 2019

Strictly speaking, this means no warning needs to be generated by the analyzers.

Would it still be possible to provide an optional quick fix to add the strongly typed ProducesResponseType for the Status Code 200?

@pranavkm
Copy link
Contributor

Would it still be possible to provide an optional quick fix to add the strongly typed ProducesResponseType for the Status Code 200?

I think at some point our plan was to provide a code fix to use ActionResult<T> when the action returns an IActionResult and uses Ok, Created etc, but that was never developed. This does seem like an area the analyzer could improve upon.

@javiercn javiercn added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Oct 25, 2019
@javiercn javiercn added this to the Backlog milestone Oct 25, 2019
@javiercn javiercn added affected-few This issue impacts only small number of customers severity-blocking This label is used by an internal tool labels Oct 9, 2020 — with ASP.NET Core Issue Ranking
@mkArtakMSFT mkArtakMSFT modified the milestones: Backlog, 6.0-preview4 Mar 19, 2021
@javiercn javiercn added analyzer Indicates an issue which is related to analyzer experience feature-openapi labels Apr 18, 2021
@ghost
Copy link

ghost commented May 10, 2021

Thanks for contacting us.

We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@ghost
Copy link

ghost commented Jul 13, 2021

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@pranavkm pranavkm 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 19, 2021
@ghost
Copy link

ghost commented Dec 28, 2021

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@rafikiassumani-msft rafikiassumani-msft added the Priority:3 Work that is nice to have label Jan 25, 2022
@kjkrum
Copy link

kjkrum commented Aug 18, 2022

The 200 Status code is implicit for every action that does not specify any ProducesResponseType attributes.

This produces weird behavior. Initially it warns you if you haven't documented another response code like 404. Upon fixing that warning, only then does it warn you that you haven't documented 200. And if you begin to rely on this warning, it will bite you when you have an action that only returns 200.

To maximize trust in and benefit from the analyzer, it should always warn if you don't document 200.

@ghost
Copy link

ghost commented Oct 11, 2022

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@javiercn javiercn removed their assignment Oct 19, 2022
@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jun 20, 2023
@captainsafia captainsafia modified the milestones: .NET 8 Planning, Backlog Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-few This issue impacts only small number of customers analyzer Indicates an issue which is related to analyzer experience area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-openapi Priority:3 Work that is nice to have severity-blocking This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests

7 participants