DefaultApiConventions should define a 204NoContent response for Delete action methods

[cremor]

Describe the bug

The DefaultApiConventions should define a Status204NoContent ResponseType in addition to the currently defined ResponseTypes.

Right now many documentation pages and tutorials return NoContent in their Delete method. When you apply the DefaultApiConventions and add web API analyzers to those APIs, the analyzers will show warnings about the undeclared status code 204.

To Reproduce

Steps to reproduce the behavior:

1. Create a Web API that returns NoContent for a Delete method, e.g. by following this tutorial: Create a web API
2. Apply the [ApiConventionType(typeof(DefaultApiConventions))] attribute to the controller or assembly.
3. Add a reference to Microsoft.AspNetCore.Mvc.Api.Analyzers to the project.
4. Check the warnings.

Expected behavior

A Delete method should be able to return NoContent() with default conventions applied without getting a warning from API analyzers.

Additional context

This doesn't affect Delete methods that return void (or Task) because for some reason ASP.NET Core returns HTTP 200 (with no body) for those (btw, why is this? For Web API 2 it was even documented that void returns 204). But as soon as you want to handle cases like returning NotFound, you'd need to change that to IActionResult (or Task<IActionResult>) and the warning is triggered.

The HTTP/1.1 RFC also lists 204 as a valid response:

If a DELETE method is successfully applied, the origin server SHOULD send a 202 (Accepted) status code if the action will likely succeed but has not yet been enacted, a 204 (No Content) status code if the action has been enacted and no further information is to be supplied, or a 200 (OK) status code if the action has been enacted and the response message includes a representation describing the status.

I could only find one case in the official documentation and templates where a Delete method actually returns some data. That is from an "API Controller with actions, using Entity Framework" scaffolded controller (source code). But I don't think returning the just deleted object makes much sense.

dotnet --info output:

.NET Core SDK (gemäß "global.json"): Version: 2.2.105 Commit: 7cecb35b92

Laufzeitumgebung:
OS Name: Windows
OS Version: 10.0.16299
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\2.2.105\

Host (useful for support):
Version: 2.2.3
Commit: 6b8ad509b6

.NET Core SDKs installed:
2.1.202 [C:\Program Files\dotnet\sdk]
2.1.505 [C:\Program Files\dotnet\sdk]
2.2.105 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
Microsoft.AspNetCore.All 2.1.9 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.All 2.2.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.App 2.1.9 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 2.2.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.NETCore.App 2.0.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.1.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.2.3 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]

To install additional .NET Core runtimes or SDKs:
https://aka.ms/dotnet-download

[pranavkm]

I could only find one case in the official documentation and templates where a Delete method actually returns some data.

We based the pattern for the delete convention based on the scaffolded code for a controller using EF . @glennc, thoughts?

[glennc]

Having the conventions say that a DELETE could return 204, even if it doesn't, seems in-line with the way we talked about the feature. I don't think we're willing to investigate changing the default from 200 for void/Task as that seems like a pretty big breaking change for questionable value, given that 200 is an acceptable response.

[cremor]

@mkArtakMSFT Why did you close this issue without action? @glennc confirmed that the convention should define the HTTP 204 result, so it should be changed, correct?

[cremor]

@mkArtakMSFT @glennc @pranavkm Can someone please reopen this issue?

[ianrathbone]

Is this something that is definitely not going to get implemented as I've also come across this issue today while writing an API.

We were expecting to be returning a 204 for a DELETE as there is no content to return...

[cremor]

Related:
#4950
dotnet/Scaffolding#983

[cremor]

@mkArtakMSFT @glennc @pranavkm As explained in the previous comments, please reopen this issue!

[mkArtakMSFT]

@cremor this is not something we plan to do. You can create your own convention and use that instead of the default one to get the behavior your want by default. Here are the docs to help you get started: https://docs.microsoft.com/en-us/aspnet/core/web-api/advanced/conventions?view=aspnetcore-3.0#create-web-api-conventions

[cremor]

@mkArtakMSFT That's very sad to hear.

I understand that you won't change the status code that is returned for void/Task actions, so we can leave this. It was just a side note in this issue anyway.
But right now the default conventions do not match your documentation as I've explained in the steps to reproduce in my original comment. And expanding the default convention so that it matches the documentation (and the HTTP RFC) is one line of code. Would you also not accept a PR that contains this one line change?

I know that I can create my own conventions. In fact I've done this for all my Web API projects to fix this and other problems with the default conventions (I've also created issues for all the other problems, those have not been closed).

[mkArtakMSFT]

Given that this behavior has already shipped, we can't simply change it as it'll be a breaking change. As for the documentation, we need to make sure that documentation matches the current behavior.
@cremor can you please show where exactly the documentation states mismatching behavior?

[mkArtakMSFT]

@pranavkm we will need to get the documentation fixed.

[cremor]

@mkArtakMSFT

Given that this behavior has already shipped, we can't simply change it as it'll be a breaking change.

How is adding an additional HTTP response code to the default API convention a breaking change? An API convention doesn't have any affect at runtime, or does it?

As I understand it, the only effect that will have is to not show the wrong warning about a NoContent() return in a DELETE method any more (if the project even has API analyzers enabled).

can you please show where exactly the documentation states mismatching behavior?

Here: https://docs.microsoft.com/en-us/aspnet/core/tutorials/first-web-api?view=aspnetcore-2.2&tabs=visual-studio#add-a-deletetodoitem-method

That tutorial says to create a DELETE method like this:

[HttpDelete("{id}")]
public async Task<IActionResult> DeleteTodoItem(long id)
{
    [some code]
    return NoContent();
}

And it even explicitly states:

The DeleteTodoItem response is 204 (No Content).

Note: The documentation only shows this code for versions <= 2.2. For 3.0 it returns an entity in the delete method (but still states that "204 (No Content)" is returned, which is then wrong. I've already reported this at dotnet/AspNetCore.Docs#14914).

[pranavkm]

@cremor API conventions affect the output of ApiExplorer \ Swagger for the controller. Changing the convention to do something different would "silently" affect apps that use the convention.

[cremor]

@pranavkm Good point, I forgot about that. Thanks for the explanation.

I see that this is a change. But is it really a breaking change? It would just add an additional response type, existing ones would be not affected.

Even if it is a breaking change, there are many possibilities:

• You could hide it behind an optional configuration setting like what was done with CompatibilityVersion.Version_2_2 (although I see that that would require a lot of additional code, so it's probably not worth it)
• You could delay the change until the next major version.
• Even minor version updates had breaking changes in the past.

Please also include #9375 in your consideration. Even if you don't think that this issue here is an important enough fix to justify a change, that other issue might be (and they both require the same type of fix).

[pranavkm]

The most likely solution we would go with would be to create a separate convention and have users explicitly opt in. If you'd like we could use #9375 to consider fixing both of these using this approach.

[cremor]

Yes, I think this would be fine. API conventions are opt-in anyway, so a choice doesn't hurt.