Clean up API error-distinguishing logic #4896

gnprice · 2021-07-14T05:32:48Z

This is what I ended up with after my comment #4754 (review) :

[The] existing code [in apiErrors] should change, I think, to handle this properly. I'll sketch out a version of how that could look, and either push some draft commits to this branch or send a PR.

It was kind of a tangle, and now I hope it's clearer. In particular, I think it's now a lot more straightforward to think of the set of possible errors on an API request as completely partitioned into meaningful subcategories, and to write conditionals on those.

The isClientError predicate goes away in this branch, replaced by a direct instanceof check at its former callsite. I think the same thing can be done with isServerError and isNetworkRequestFailedError in #4754; they can become instanceof checks referring to the new error classes added here.

The last few commits in this branch are some cleanups to the API result types, which aren't needed for anything else in this branch. I wrote those on the way toward trying to make interpretApiResponse return an ApiSuccessResponse instead of mixed, but the change to actually do that is still a draft.

This inherits P1 priority from #4754 .

Plus a TODO comment for something the new jsdoc says that isn't yet true. We'll fix that later in this series.

chrisbobbe

Thanks for this cleanup, @gnprice! See a few tiny comments below; otherwise, please merge at will.

chrisbobbe · 2021-07-14T16:15:53Z

src/api/apiFetch.js

    Sentry.addBreadcrumb({
      category: 'api',
      level: 'info',
-      data: { route, params, httpStatus: response.status, json },
+      data: { route, params, httpStatus, data },


api errors [nfc]: Rely on error object in logging code.

nit: Breadcrumbs sent to Sentry will look different after this commit, so I think that makes it non-NFC.

Mmm, indeed, good catch.

I think actually the name should ideally say something like "result" or "response" -- neither "json" nor "data" is real clear that this is the data the server sent back in the body, rather than say the data that we sent in the request. I'll pick a name like that.

chrisbobbe · 2021-07-14T16:25:21Z

src/api/apiFetch.js

@@ -37,12 +37,13 @@ export const apiFetch = async (
  params: $Diff<$Exact<RequestOptions>, {| headers: mixed |}>,
 ) => fetch(new URL(`/${apiVersion}/${route}`, auth.realm).toString(), getFetchParams(auth, params));

+/** (Caller beware! Return type is the magic `empty`.) */


fetch having a type of any in the libdef RN provides

Yeah; it'll be nice to get facebook/react-native@6651b7c59, looks like in RN v0.65.

Ah indeed, neat!

chrisbobbe · 2021-07-14T16:38:22Z

src/api/apiErrors.js

@@ -11,7 +21,7 @@ import * as logging from '../utils/logging';
 * See docs: https://zulip.com/api/rest-error-handling
 */
 // TODO we currently raise these in more situations; fix that.
-export class ApiError extends Error {
+export class ApiError extends RequestError {
  code: ApiErrorCode;


Could also have a commit that makes ApiError and ServerError read-only, right?

Sure, yeah.

I guess when I did that on the base class:

export class RequestError extends Error { +httpStatus: number | void; +data: mixed;

it was just because that's necessary in order for the subclasses to give more-specific types for those properties. But the same thing would be appropriate on the subclasses' properties too.

For one of these cases, there's a nice new thing the data can be instead. For the other, we could modify the test case to have some status code that's neither 5xx nor 4xx. But later in this series we're going to eliminate all the non-4xx cases of ApiError anyway, so we don't bother.

We got an error, but of that whole error what this number specifically is is the HTTP status code.

This sets up the next refactoring; we're going to use a try/catch at this callsite.

The high-order bit about this particular property, relative to the others in this blob, is that it represents the data the server sent back to us in reply to the request. So give it a name that says that. A variety of names are possible, but "response" is the term we use in the types ApiResponse, ApiResponseSuccess, and so on, which correspond exactly to the data we expect to see here. So use that.

This matches how the data will flow when we use a try/catch, coming up next.

And use it for the example that motivated adding it.

This is NFC in the case where we reach `makeErrorFromApi` and throw an exception it returns. If there's an exception somewhere else -- which would be a bug in this code -- then this change means that we now reach the logging code that got moved inside this new `catch` block. That seems all to the good. And in fact, one of our tests demonstrates that case! It does so by artificially making `fetch` itself fail. That does seem like a bug (an existing one) in this code; we'll be fixing it later in this series.

This is just the definition of the `ok` property, by spec: https://fetch.spec.whatwg.org/#dom-response-ok

This is the type Flow was already inferring here, as a result of, ultimately, `fetch` having a type of `any` in the libdef RN provides. Make it a little more explicit that that's happening.

This gives us a better shot at seeing all in one place how we interpret the server's response and what the possible cases are.

This was getting lumped in at the bottom with the case of a malformed error blob.

We now only throw an ApiError in the first place when the code is 4xx.

Saying `instanceof ApiError` seems like a cleaner interface for the API code to present, now that it means the same thing.

This can indeed have additional properties.

We've had this as an exact object type, but the real type is inexact. When spreading it, though, we always just want to incorporate the properties it specifically has. The way to spell that is `$Exact<ApiResponseSuccess>`. This has no effect at the moment because the type is exact in the first place, but it'll let us change it to inexact and keep getting the intended results.

This may indeed have additional properties.

gnprice · 2021-07-15T00:00:55Z

Thanks for the review! Merged, with those changes.

gnprice added 3 commits July 13, 2021 19:43

api errors [nfc]: Simplify logic in the well-formed case.

16b044c

api errors: On 5xx response, don't try to interpret JSON.

2f4db11

api errors [nfc]: Give ApiError some more helpful jsdoc.

23f9c9e

Plus a TODO comment for something the new jsdoc says that isn't yet true. We'll fix that later in this series.

gnprice added the P1 high-priority label Jul 14, 2021

gnprice requested a review from chrisbobbe July 14, 2021 05:32

gnprice mentioned this pull request Jul 14, 2021

Time out initial fetch and go to account-picker screen. #4754

Merged

chrisbobbe reviewed Jul 14, 2021

View reviewed changes

gnprice added 23 commits July 14, 2021 16:58

api errors [nfc]: Mark ApiError's properties as read-only.

9b0cf23

api errors: Make a distinct ServerError class, and Server5xxError.

e62f600

api errors [nfc]: Add RequestError as base class.

9c59f90

api errors: Fix "HTTP status" terminology in an error message.

b74bf2f

We got an error, but of that whole error what this number specifically is is the HTTP status code.

api errors: Make a distinct MalformedResponseError class.

be8ac20

api errors: Put HTTP status in message for MalformedResponseError.

f27fe03

api errors [nfc]: Call makeErrorFromApi sooner.

0986397

This sets up the next refactoring; we're going to use a try/catch at this callsite.

api errors [nfc]: Rely on error object in logging code.

13d6838

This matches how the data will flow when we use a try/catch, coming up next.

logging: Add logging.info, below "warn" and "error".

a4321b7

And use it for the example that motivated adding it.

api errors [nfc]: Unpack response.ok in terms of status.

0595a40

This is just the definition of the `ok` property, by spec: https://fetch.spec.whatwg.org/#dom-response-ok

api types [nfc]: Make a bogus empty explicit.

4ef0f06

This is the type Flow was already inferring here, as a result of, ultimately, `fetch` having a type of `any` in the libdef RN provides. Make it a little more explicit that that's happening.

api errors [nfc]: Put success and failure logic together.

a870379

This gives us a better shot at seeing all in one place how we interpret the server's response and what the possible cases are.

api errors: Add NetworkError.

21c4b03

api errors [nfc]: Make the 2xx-yet-no-JSON case explicit.

fb632bb

This was getting lumped in at the bottom with the case of a malformed error blob.

api errors: Add UnexpectedHttpStatusError.

05cef99

api errors [nfc]: Simplify isClientError.

fb5cd6e

We now only throw an ApiError in the first place when the code is 4xx.

api errors [nfc]: Inline away isClientError.

fad6ddb

Saying `instanceof ApiError` seems like a cleaner interface for the API code to present, now that it means the same thing.

api types: Fix an ApiResponse to ApiResponseSuccess.

f5b956e

api types: Make ApiResponse an inexact object type.

3a6be04

This can indeed have additional properties.

gnprice added 2 commits July 14, 2021 16:59

api types: Make ApiResponseSuccess an inexact object type.

a56287b

This may indeed have additional properties.

api types: Mark ApiResponse and friends as read-only.

3756025

gnprice force-pushed the pr-api-errors branch from 375dad8 to 3756025 Compare July 15, 2021 00:00

gnprice merged commit 3756025 into zulip:master Jul 15, 2021

gnprice deleted the pr-api-errors branch July 15, 2021 00:01

gnprice mentioned this pull request Jul 24, 2021

Flow: Switch to new "Types-First" mode. #4907

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up API error-distinguishing logic #4896

Clean up API error-distinguishing logic #4896

gnprice commented Jul 14, 2021

chrisbobbe left a comment

chrisbobbe Jul 14, 2021

gnprice Jul 14, 2021

chrisbobbe Jul 14, 2021

gnprice Jul 14, 2021

chrisbobbe Jul 14, 2021

gnprice Jul 14, 2021

gnprice commented Jul 15, 2021

Clean up API error-distinguishing logic #4896

Clean up API error-distinguishing logic #4896

Conversation

gnprice commented Jul 14, 2021

chrisbobbe left a comment

Choose a reason for hiding this comment

chrisbobbe Jul 14, 2021

Choose a reason for hiding this comment

gnprice Jul 14, 2021

Choose a reason for hiding this comment

chrisbobbe Jul 14, 2021

Choose a reason for hiding this comment

gnprice Jul 14, 2021

Choose a reason for hiding this comment

chrisbobbe Jul 14, 2021

Choose a reason for hiding this comment

gnprice Jul 14, 2021

Choose a reason for hiding this comment

gnprice commented Jul 15, 2021