dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.59k stars 10.06k forks source link

API analyzers should support undeclared 200 status codes #15385

Open jawn opened 5 years ago

jawn commented 5 years ago

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 https://github.com/aspnet/AspNetCore.Docs/issues/15269#event-2741659999

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

pranavkm commented 5 years ago

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.

jawn commented 5 years ago

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 commented 5 years ago

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.

ghost commented 3 years ago

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 commented 3 years ago

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.

ghost commented 2 years ago

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.

kjkrum commented 2 years ago

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 commented 2 years ago

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.