Web: Fixes handling native fetch response that can result in unexpected errors#52154
Web: Fixes handling native fetch response that can result in unexpected errors#52154
Conversation
kimlisa
force-pushed
the
lisa/fix-reading-json
branch
2 times, most recently
from
a4286a4 to
bfcd7e8
Compare
kimlisa
force-pushed
the
lisa/fix-reading-json
branch
from
bfcd7e8 to
db852fc
Compare
|
Can we add a test case which would have caught the regression? |
bl-nero
left a comment
bl-nero
left a comment
There was a problem hiding this comment.
Approving with a small caveat.
| // to read. | ||
| const contentType = response.headers?.get('content-type'); | ||
| if (!contentType || !contentType.includes('application/json')) { | ||
| return response; |
There was a problem hiding this comment.
I'm wondering if using https://developer.mozilla.org/en-US/docs/Web/API/Response/text wouldn't be a more "symmetrical" solution to returning JSON if the body is a JSON — that is, unless in our case, an empty body also throws instead of resolving to an empty string.
There was a problem hiding this comment.
your comment saved us. because it led to a discussion about typing the return value... where the linter caught more regressions 🙏
what happened was, previous api.fetch had a auto derived type Promise<Response>, after the change, the type changed to Promise<any>, which is why the linter didn't catch bugs
what i ended up doing was revert the api.fetch to its previous implementation, which is to return the native fetch response object (and explicitly typed it), and moved the json processing to another function call.
the reason for the revert was that in some areas of our code, the caller expects and handles the native fetch response (eg: here, here and here)
public-teleport-github-review-bot
Bot
removed request for
Joerger,
kiosion and
ravicious
kimlisa
force-pushed
the
lisa/fix-reading-json
branch
2 times, most recently
from
a42b23b to
8181876
Compare
kimlisa
changed the title
kimlisa
force-pushed
the
lisa/fix-reading-json
branch
from
8181876 to
57cec68
Compare
…ed errors (gravitational#52154) * Web: fix reading JSON from an empty response * Revert api.fetch to native fetch return type
4 participants
fixes regression introduced by #51134
some of the callsite to
api.fetchexpects and handles native fetch response, explained more herealso some of our api responses does not return a body like here, and if we try to
response.json()it throws an error, even though the request itself was successfulpreviously, the
api.fetchdefinition was just returning native fetch call but ^ PR changed it to also read JSONwe should ideally be returning OK json response, but some of it has slipped and i don't think there is too many
example of error on a success fetch:
changelog: Fixed broken
Download Metadata Filebutton from the SAML enrolling resource flow in the web UI.changelog: Fixed broken
Refreshbutton in the Access Monitoring reports page in the web UI.changelog: Fixed broken
Download app.zipmenu item in the Integrations list dropdown menu for Microsoft Teams in the web UI.changelog: Fixed
Unexpected end of JSON inputerror in an otherwise successful web API call.