fix(common): set HttpResponseBase.statusText to an empty string by default #57580
Conversation
…fault Previously, at the time of HTTP/2 support introduction in browsers, the Fetch Standard at https://fetch.spec.whatwg.org/#concept-response-status-message specified that the statusText would always be 'OK'. On the initial closing date of issue #23334, this was changed to "Unless stated otherwise, it is the empty byte sequence". In practice, this lead to errors being logged as e.g. "500 OK". Now, when there is no statusText set in an underlying response, the default is an empty string. BREAKING CHANGE: The statusText on HttpResponseBase no longer defaults to 'OK' Any code that depends on statusText being 'OK' in situations where there is none set needs to be updated to expect an empty string. Note that this may apply to use-cases like displaying and logging HTTP response errors. Fixes #23334
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
It should probably be considered to replace custom status text entirely, and just retrieving the standard status text descriptions from the current HTTP standard and embedding them into Angular. But this is out of scope for this PR, imo. |
|
CLA signed. |
|
This is a breaking change that isn't worth introducing considering that the breakages can be subtle and not trivial to migrate. |
|
That's odd, but sure. Guess you'll keep having "500 OK" in your |
|
I do think this is a meaningful change to make so I wanted to reopen, but the repo has since been deleted. I can't say if this has a chance of landing as there is indeed a risk of breakages, likely in a subtle way that may not be caught in tests due to the nature of this change. |
| @@ -93,9 +93,6 @@ export class TestRequest { | |||
| statusText ||= 'OK'; | |||
There was a problem hiding this comment.
There's an OK here as well that may need to be audited.
|
I'm open the re-evaluate the breaking-ness of the change. Maybe a TGP could give us a hint of how breaking that would be ? |
|
The property is literally documented as "do not rely on this". Only really bad code actually relies on this status text being OK when the status code isn't 200. So, the breaking change is technically one but in reality, this shouldn't matter. I just tend to be pessimistic on this stuff, considering the amount of such really bad code I've seen in the past. In case you want me to make a follow-up PR (including @JoostK's comment on that one missed occurrence and, if wanted, potentially fixing up unit tests), do tell. |
|
An alternative would be to drop that property entirely. Another alternative, instead of leaving it empty when it is, is to retrieve the "would-be proper value" for all status codes defined RFC 9110. Many changes that could be made. |
I don't think this is the main concern, it's more that some procedures may not handle an empty string properly. I do agree that would still be bad code, indeed, and the comment already calling out that the property shouldn't be depended upon is a nice touch. If you'd still like to see this change being made then please do go ahead; getting a green PR is the first step towards making this change. One alternative approach might be to just deprecate the property entirely, without going through the breaking change. With HTTP/2 not supporting this concept at all you could argue that the status text is a relic of the past, not worth keeping around. Let me bring this up with the team, and perhaps hold off on resuming work on a PR until I can get back to you. |
|
Okay, let's go ahead with deprecating the property in its entirety, not changing it to an empty string by default. |
|
This will mean code changes are still needed for stuff like the error text containing stuff like "500 OK", but I suppose it will just mean removing the usage of statusText in that part entirely? |
|
If it's possible to remove the usages, please do so. Except for the tests of the property itself, those can/should remain until the property is actually removed. |
|
Follow-up PR: #58080 |
…eprecate HttpResponseBase.statusText Since HTTP/2, responses no longer contain a status text besides the status code, which caused our default value of 'OK' to be used in HttpErrorResponse.message. As discussed in angular#57580, we are opting to not change the default value to match the fetch spec (https://fetch.spec.whatwg.org/#concept-response-status-message), instead, we are deprecating the statusText field, and not using it in our own code. DEPRECATED: HttpResponseBase.statusText This field is usually populated with OK by default, and overwritten by what a remote HTTP server responds with on the status line. Since HTTP/2, we aren't guaranteed to have a value for this on the status line, meaning this value gets stuck at OK. If you need this field for display purposes, it is recommended to retrieve a selection from RFC9110 (or future RFCs) yourself. For other purposes, an HttpResponseCode enum is also provided at common/http. Fixes angular#23334
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #23334
What is the new behavior?
The new behaviour
Does this PR introduce a breaking change?
The statusText on HttpResponseBase no longer defaults to 'OK'.
Any code that depends on statusText being 'OK' in situations where there is none set needs to be updated to expect an empty string. Note that this may apply to use-cases like displaying and logging HTTP response errors.
Other information