Skip to content

Commit

Permalink
#3613: fix network error cause tracking (#3645)
Browse files Browse the repository at this point in the history
  • Loading branch information
twschiller authored Jun 12, 2022
1 parent 3bb6296 commit acefed0
Show file tree
Hide file tree
Showing 6 changed files with 135 additions and 5 deletions.
12 changes: 10 additions & 2 deletions src/background/requests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export async function serializableAxiosRequest<T>(

const { data, status, statusText } = await axios(config);

// Firefox won't send response objects from the background page to the content script. So strip out the
// Firefox won't send response objects from the background page to the content script. Strip out the
// potentially sensitive parts of the response (the request, headers, etc.)
return JSON.parse(JSON.stringify({ data, status, statusText }));
}
Expand Down Expand Up @@ -271,7 +271,15 @@ export async function proxyService<TData>(
);
}

return serializableAxiosRequest<TData>(requestConfig);
try {
return await serializableAxiosRequest<TData>(requestConfig);
} catch (error) {
console.debug("Error making request without service config", {
requestConfig,
error,
});
throw error;
}
}

try {
Expand Down
2 changes: 1 addition & 1 deletion src/components/errors/NetworkErrorDetail.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ const NetworkErrorDetail: React.FunctionComponent<{
const cleanResponse = useMemo(() => {
if (error.response) {
const { request, config, data, ...rest } = error.response;
// Don't include request, since we're showing it the other column
// Don't include request or config, since we're showing it the other column
return {
...rest,
data: tryParse(data),
Expand Down
2 changes: 1 addition & 1 deletion src/development/errorsBadge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ let timer: NodeJS.Timeout;

function updateBadge(errorMessage: string | null): void {
void chrome.browserAction.setTitle({
title: errorMessage,
title: errorMessage ?? "Unknown error (no error message provided)",
});
void chrome.browserAction.setBadgeText({
text: counter ? String(counter) : undefined,
Expand Down
29 changes: 29 additions & 0 deletions src/errors/businessErrors.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright (C) 2022 PixieBrix, Inc.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import { BusinessError } from "@/errors/businessErrors";
import { serializeError } from "serialize-error";

describe("BusinessError", () => {
it("records cause", () => {
const cause = new Error("Pylon Error");
const error = new BusinessError("You Must Construct Additional Pylons", {
cause,
});
expect(serializeError(error).cause).toStrictEqual(serializeError(cause));
});
});
90 changes: 90 additions & 0 deletions src/errors/clientRequestErrors.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/*
* Copyright (C) 2022 PixieBrix, Inc.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import { serializeError } from "serialize-error";
import {
ClientRequestError,
RemoteServiceError,
} from "@/errors/clientRequestErrors";
import axios, { AxiosError } from "axios";
import MockAdapter from "axios-mock-adapter";

const axiosMock = new MockAdapter(axios);

afterEach(() => {
axiosMock.reset();
axiosMock.resetHistory();
});

describe("RemoteServiceError", () => {
it("records cause", () => {
const cause = new AxiosError("The tubes are clogged");
// `serializeError` uses the name/message/stack property to determine if it's errorLike
// In practice, AxiosErrors actually have stacks: https://github.com/axios/axios/issues/2387
cause.stack = "";
const error = new RemoteServiceError(
"You Must Construct Additional Pylons",
{ cause }
);
expect(serializeError(error, { useToJSON: false }).cause).toStrictEqual(
serializeError(cause, { useToJSON: false })
);
});

it("keeps request and response", async () => {
axiosMock
.onAny()
.reply(400, { detail: "I'm sorry Dave, I'm afraid I can't do that" });

let cause: AxiosError;

try {
await axios({
url: "https://httpstat.us/400",
method: "get",
});
} catch (error: unknown) {
cause = error as AxiosError;
}

const error = new RemoteServiceError(
"You Must Construct Additional Pylons",
{ cause }
);

const serialized = serializeError(error, { useToJSON: false });

expect((serialized.cause as AxiosError).config).toBeTruthy();
expect((serialized.cause as AxiosError).response).toBeTruthy();
});
});

describe("ClientRequestError", () => {
it("records cause", () => {
const cause = new AxiosError("The tubes are clogged");
cause.stack = "";
// `serializeError` uses the name/message/stack property to determine if it's errorLike
// In practice, AxiosErrors actually have stacks: https://github.com/axios/axios/issues/2387
const error = new ClientRequestError(
"You Must Construct Additional Pylons",
{ cause }
);
expect(serializeError(error, { useToJSON: false }).cause).toStrictEqual(
serializeError(cause, { useToJSON: false })
);
});
});
5 changes: 4 additions & 1 deletion src/errors/clientRequestErrors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,14 @@ import { SerializableAxiosError } from "@/errors/networkErrorHelpers";
*/
export class ClientRequestError extends BusinessError {
override name = "ClientRequestError";
// Specialize the cause type
override readonly cause: SerializableAxiosError;

// eslint-disable-next-line @typescript-eslint/no-useless-constructor -- Required to make the types stricter
constructor(message: string, options: { cause: SerializableAxiosError }) {
super(message, options);
// This assignment seems to be required in Chrome 102 to ensure the cause is serialized by serialize-error
// https://github.com/pixiebrix/pixiebrix-extension/issues/3613
this.cause = options.cause;
}
}

Expand Down

0 comments on commit acefed0

Please sign in to comment.