Deprecate or analyzer-warn HttpClient IsSuccessStatusCode and EnsureSuccessStatusCode

[scalablecory]

HttpResponseMessage.IsSuccessStatusCode and HttpResponseMessage.EnsureSuccessStatusCode are anti-patterns:

These encourage non-defensive design: if an API returns 200 OK every day, and one day returns 202 Accepted -- is my app doing the correct thing by assuming both are an equivalent result?

I see these used fairly often, and it's usually the first thing newbies reach for.
.NET should warn people away from these and encourage testing for explicit status codes.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

[julealgon]

These are on HttpResponseMessage, not on HttpClient.

I don't quite agree that IsSuccessStatusCode is "an anti-pattern", after all it's just a boolean that you can use to check if something is in the 2XX range, which by itself is not wrong.

I could see an argument for EnsureSuccessStatusCode being more problematic, but maybe not necessarily for the same reason as you see it: it promotes using exceptions as control flow which is a bad practice (and slow).

Having said all that, the proper "recommendation" should be "use kiota and generate a proxy from a well-defined open api spec that has the status codes properly mapped".

[scalablecory]

I literally can not remember a time I've seen someone use these in a way that was correct. They are an anti-pattern.

[MihaZupan]

I see these as simple helpers that are often useful for ruling out responses that you know definitely aren't okay. They don't prevent you from doing further validation.
At least in cases where I've used it, I don't care which exact status code was returned - if something did change, I expect deserializing the response content to fail and surface that.

Helper functions like HttpClient.GetStringAsync, GetFromJsonAsync, etc. are worse from this perspective as they do hide information about which status code was returned (they just call EnsureSuccessStatusCode), and yet I don't see us ever obsoleting them.

[scalablecory]

Filed somewhat out of annoyance; I had just fixed a bug hidden by use of this, when an API started returning OK instead of Created.

This isn't the first time I've made code more robust by removing use of this API -- I'd agree those other shortcuts are bad for the same reason.

I understand it's liked because it's simple, and that'll make it hard to want to do anything about it. But that's part of why I consider them so bad. These are so tempting to use, but are usually a bad choice.

[MihaZupan]

The team consensus here is that we don't see a need to obsolete these helpers as they don't prevent the user from doing stricter validation, and are often sufficient.

But we do see value in improving our documentation to:

• Call out where we are using these checks ourselves and you can't control it, i.e. all HttpClient.Get{String/ByteArray/Stream/FromJson}Async helpers - HttpClient.GetStreamAsync raising HttpRequestException dotnet-api-docs#9148.
• Include a note along the lines of "consider checking that HttpResponseMessage.StatusCode exactly matches the expected value instead".
• Consider improving indicates if the HTTP response was successful to include its definition of "successful". it already does in Returns: section.

[MihaZupan]

This is also something where BannedApiAnalyzers could help within your org.

P:System.Net.Http.HttpResponseMessage.IsSuccessStatusCode
M:System.Net.Http.HttpResponseMessage.EnsureSuccessStatusCode