Clean up API error-distinguishing logic

jest/jestSetup.js

@@ -153,5 +153,6 @@ jest.mock('../src/utils/logging', () => {
     __esModule: true, // eslint-disable-line id-match
     error: jest.fn().mockImplementation(logging.error),
     warn: jest.fn().mockImplementation(logging.warn),
+    info: jest.fn().mockImplementation(logging.info),
   };
 });

src/api/apiErrors.js

@@ -1,12 +1,36 @@
 /* @flow strict-local */
 import type { ApiErrorCode, ApiResponseErrorData } from './transportTypes';
-import * as logging from '../utils/logging';
 
-/** Runtime class of custom API error types. */
-export class ApiError extends Error {
-  code: ApiErrorCode;
-  data: $ReadOnly<{ ... }>;
-  httpStatus: number;
+/**
+ * Some kind of error from a Zulip API network request.
+ *
+ * See subclasses: {@link ApiError}, {@link NetworkError}, {@link ServerError}.
+ */
+export class RequestError extends Error {
+  +httpStatus: number | void;
+  +data: mixed;
+}
+
+/**
+ * An error returned by the Zulip server API.
+ *
+ * This always represents a situation where the server said there was a
+ * client-side error in the request, giving a 4xx HTTP status code.
+ *
+ * See docs: https://zulip.com/api/rest-error-handling
+ */
+export class ApiError extends RequestError {
+  +code: ApiErrorCode;
+
+  +httpStatus: number;
+
+  /**
+   * This error's data, if any, beyond the properties common to all errors.
+   *
+   * This consists of the properties in the response other than `result`,
+   * `code`, and `msg`.
+   */
+  +data: $ReadOnly<{ ... }>;
 
   constructor(httpStatus: number, data: $ReadOnly<ApiResponseErrorData>) {
     // eslint-disable-next-line no-unused-vars
@@ -19,50 +43,101 @@ export class ApiError extends Error {
 }
 
 /**
- * Given a server response (allegedly) denoting an error, produce an Error to be
- * thrown.
+ * A network-level error that prevented even getting an HTTP response.
+ */
+export class NetworkError extends RequestError {}
+
+/**
+ * Some kind of server-side error in handling the request.
  *
- * If the `data` argument is actually of the form expected from the server, the
- * returned error will be an {@link ApiError}; otherwise it will be a generic
- * Error.
+ * This should always represent either some kind of operational issue on the
+ * server, or a bug in the server where its responses don't agree with the
+ * documented API.
  */
-export const makeErrorFromApi = (httpStatus: number, data: mixed): Error => {
-  // Validate `data`, and construct the resultant error object.
-  if (typeof data === 'object' && data !== null) {
-    const { result, msg, code } = data;
-    if (result === 'error' && typeof msg === 'string') {
-      // If `code` is present, it must be a string.
-      if (code === undefined || typeof code === 'string') {
-        // Default to 'BAD_REQUEST' if `code` is not present.
-        return new ApiError(httpStatus, {
-          ...data,
-          result,
-          msg,
-          code: code ?? 'BAD_REQUEST',
-        });
-      }
-    }
-  }
+export class ServerError extends RequestError {
+  +httpStatus: number;
 
-  // HTTP 5xx errors aren't generally expected to come with JSON data.
-  if (httpStatus >= 500 && httpStatus <= 599) {
-    return new Error(`Network request failed: HTTP error ${httpStatus}`);
+  constructor(msg: string, httpStatus: number) {
+    super(msg);
+    this.httpStatus = httpStatus;
   }
+}
 
-  // Server has responded, but the response is not a valid error-object.
-  // (This should never happen, even on old versions of the Zulip server.)
-  logging.warn(`Bad response from server: ${JSON.stringify(data) ?? 'undefined'}`);
-  return new Error('Server responded with invalid message');
-};
+/**
+ * A server error, acknowledged by the server via a 5xx HTTP status code.
+ */
+export class Server5xxError extends ServerError {
+  constructor(httpStatus: number) {
+    super(`Network request failed: HTTP status ${httpStatus}`, httpStatus);
+  }
+}
 
 /**
- * Is exception caused by a Client Error (4xx)?
+ * An error where the server's response doesn't match the general Zulip API.
  *
- * Client errors are often caused by incorrect parameters given to the backend
- * by the client application.
+ * This means the server's response didn't contain appropriately-shaped JSON
+ * as documented at the page https://zulip.com/api/rest-error-handling .
+ */
+export class MalformedResponseError extends ServerError {
+  constructor(httpStatus: number, data: mixed) {
+    super(`Server responded with invalid message; HTTP status ${httpStatus}`, httpStatus);
+    this.data = data;
+  }
+}
+
+/**
+ * An error where the server gave an HTTP status it should never give.
  *
- * A notable difference between a Server (5xx) and Client (4xx) errors is that
- * a client error will not be resolved by waiting and retrying the same request.
+ * That is, the HTTP status wasn't one that the docs say the server may
+ * give: https://zulip.com/api/rest-error-handling
  */
-export const isClientError = (e: Error): boolean =>
-  e instanceof ApiError && e.httpStatus >= 400 && e.httpStatus <= 499;
+export class UnexpectedHttpStatusError extends ServerError {
+  constructor(httpStatus: number, data: mixed) {
+    super(`Server gave unexpected HTTP status: ${httpStatus}`, httpStatus);
+    this.data = data;
+  }
+}
+
+/**
+ * Return the data on success; otherwise, throw a nice {@link RequestError}.
+ */
+// For the spec this is implementing, see:
+//   https://zulip.com/api/rest-error-handling
+export const interpretApiResponse = (httpStatus: number, data: mixed): mixed => {
+  if (httpStatus >= 200 && httpStatus <= 299) {
+    // Status code says success…
+
+    if (data === undefined) {
+      // … but response couldn't be parsed as JSON.  Seems like a server bug.
+      throw new MalformedResponseError(httpStatus, data);
+    }
+
+    // … and we got a JSON response, too.  So we can return the data.
+    return data;
+  }
+
+  if (httpStatus >= 500 && httpStatus <= 599) {
+    // Server error.  Ignore `data`; it's unlikely to be a well-formed Zulip
+    // API error blob, and its meaning is undefined if it somehow is.
+    throw new Server5xxError(httpStatus);
+  }
+
+  if (httpStatus >= 400 && httpStatus <= 499) {
+    // Client error.  We should have a Zulip API error blob.
+
+    if (typeof data === 'object' && data !== null) {
+      const { result, msg, code = 'BAD_REQUEST' } = data;
+      if (result === 'error' && typeof msg === 'string' && typeof code === 'string') {
+        // Hooray, we have a well-formed Zulip API error blob.  Use that.
+        throw new ApiError(httpStatus, { ...data, result, msg, code });
+      }
+    }
+
+    // No such luck.  Seems like a server bug, then.
+    throw new MalformedResponseError(httpStatus, data);
+  }
+
+  // HTTP status was none of 2xx, 4xx, or 5xx.  That's a server bug --
+  // the API says that shouldn't happen.
+  throw new UnexpectedHttpStatusError(httpStatus, data);
+};

src/api/apiFetch.js

@@ -6,7 +6,13 @@ import { getAuthHeaders } from './transport';
 import { encodeParamsForUrl } from '../utils/url';
 import userAgent from '../utils/userAgent';
 import { networkActivityStart, networkActivityStop } from '../utils/networkActivity';
-import { makeErrorFromApi } from './apiErrors';
+import {
+  interpretApiResponse,
+  MalformedResponseError,
+  NetworkError,
+  RequestError,
+} from './apiErrors';
+import * as logging from '../utils/logging';
 
 const apiVersion = 'api/v1';
 
@@ -36,27 +42,51 @@ 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`.) */
 export const apiCall = async (
   auth: Auth,
   route: string,
   params: $Diff<$Exact<RequestOptions>, {| headers: mixed |}>,
   isSilent: boolean = false,
-) => {
+): Promise<empty> => {
   try {
     networkActivityStart(isSilent);
-    const response = await apiFetch(auth, route, params);
-    const json = await response.json().catch(() => undefined);
-    if (response.ok && json !== undefined) {
-      return json;
+
+    let response = undefined;
+    let json = undefined;
+    try {
+      response = await apiFetch(auth, route, params);
+      json = await response.json().catch(() => undefined);
+    } catch (error) {
+      if (error instanceof TypeError) {
+        // This really is how `fetch` is supposed to signal a network error:
+        //   https://fetch.spec.whatwg.org/#ref-for-concept-network-error⑥⓪
+        throw new NetworkError(error.message);
+      }
+      throw error;
     }
-    // eslint-disable-next-line no-console
-    console.log({ route, params, httpStatus: response.status, json });
+
+    const result = interpretApiResponse(response.status, json);
+    /* $FlowFixMe[incompatible-type] We let the caller pretend this data
+         is whatever it wants it to be. */
+    return result;
+  } catch (errorIllTyped) {
+    const error: mixed = errorIllTyped; // https://github.com/facebook/flow/issues/2470
+
+    const { httpStatus, data } = error instanceof RequestError ? error : {};
+
+    logging.info({ route, params, httpStatus, response: data });
     Sentry.addBreadcrumb({
       category: 'api',
       level: 'info',
-      data: { route, params, httpStatus: response.status, json },
+      data: { route, params, httpStatus, response: data },
     });
-    throw makeErrorFromApi(response.status, json);
+
+    if (error instanceof MalformedResponseError) {
+      logging.warn(`Bad response from server: ${JSON.stringify(data) ?? 'undefined'}`);
+    }
+
+    throw error;
   } finally {
     networkActivityStop(isSilent);
   }

src/api/devFetchApiKey.js

@@ -3,7 +3,7 @@ import type { Auth, ApiResponseSuccess } from './transportTypes';
 import { apiPost } from './apiFetch';
 
 type ApiResponseDevFetchApiKey = {|
-  ...ApiResponseSuccess,
+  ...$Exact<ApiResponseSuccess>,
   api_key: string,
 |};
 

src/api/devListUsers.js

@@ -4,7 +4,7 @@ import type { DevUser } from './apiTypes';
 import { apiGet } from './apiFetch';
 
 type ApiResponseDevListUsers = {|
-  ...ApiResponseSuccess,
+  ...$Exact<ApiResponseSuccess>,
   direct_admins: DevUser[],
   direct_users: DevUser[],
 |};

src/api/fetchApiKey.js

@@ -3,7 +3,7 @@ import type { Auth, ApiResponseSuccess } from './transportTypes';
 import { apiPost } from './apiFetch';
 
 type ApiResponseFetchApiKey = {|
-  ...ApiResponseSuccess,
+  ...$Exact<ApiResponseSuccess>,
   email: string,
   api_key: string,
 |};

src/api/getTopics.js

@@ -4,7 +4,7 @@ import type { Topic } from './apiTypes';
 import { apiGet } from './apiFetch';
 
 type ApiResponseTopics = {|
-  ...ApiResponseSuccess,
+  ...$Exact<ApiResponseSuccess>,
   topics: Topic[],
 |};
 

src/api/messages/getFileTemporaryUrl.js

@@ -3,7 +3,7 @@ import type { Auth, ApiResponseSuccess } from '../transportTypes';
 import { apiGet } from '../apiFetch';
 
 type ApiResponseTempFileUrl = {|
-  ...ApiResponseSuccess,
+  ...$Exact<ApiResponseSuccess>,
   url: string,
 |};
 

src/api/messages/getMessageHistory.js

@@ -4,7 +4,7 @@ import type { MessageSnapshot } from '../apiTypes';
 import { apiGet } from '../apiFetch';
 
 type ApiResponseMessageHistory = {|
-  ...ApiResponseSuccess,
+  ...$Exact<ApiResponseSuccess>,
   message_history: MessageSnapshot[],
 |};
 

src/api/messages/getMessages.js

@@ -8,7 +8,7 @@ import { identityOfAuth } from '../../account/accountMisc';
 import { AvatarURL } from '../../utils/avatar';
 
 type ApiResponseMessages = {|
-  ...ApiResponseSuccess,
+  ...$Exact<ApiResponseSuccess>,
   anchor: number,
   found_anchor?: boolean,
   found_newest?: boolean,

src/api/messages/getRawMessageContent.js

@@ -3,7 +3,7 @@ import type { Auth, ApiResponseSuccess } from '../transportTypes';
 import { apiGet } from '../apiFetch';
 
 type ApiResponseMessageContent = {|
-  ...ApiResponseSuccess,
+  ...$Exact<ApiResponseSuccess>,
   raw_content: string,
 |};
 

src/api/messages/messagesFlags.js

@@ -3,7 +3,7 @@ import type { ApiResponseSuccess, Auth } from '../transportTypes';
 import { apiPost } from '../apiFetch';
 
 export type ApiResponseMessagesFlags = {|
-  ...ApiResponseSuccess,
+  ...$Exact<ApiResponseSuccess>,
   messages: number[],
 |};
 

src/api/pollForEvents.js

@@ -4,7 +4,7 @@ import type { GeneralEvent } from './eventTypes';
 import { apiGet } from './apiFetch';
 
 type ApiResponsePollEvents = {|
-  ...ApiResponseSuccess,
+  ...$Exact<ApiResponseSuccess>,
   events: GeneralEvent[],
 |};
 

src/api/realm/createRealmFilter.js

@@ -3,7 +3,7 @@ import type { Auth, ApiResponseSuccess } from '../transportTypes';
 import { apiPost } from '../apiFetch';
 
 type ApiResponseRealmCreateFilters = {|
-  ...ApiResponseSuccess,
+  ...$Exact<ApiResponseSuccess>,
   id: number,
 |};