Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARC-1429: Return 404 if Jira has been renamed. #1268

Merged
merged 9 commits into from
Jun 16, 2022
18 changes: 17 additions & 1 deletion src/jira/client/axios.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,23 @@ describe("Jira axios instance", () => {
}

expect(error?.status).toEqual(404);
expect(error?.message).toEqual("Error executing Axios Request HTTP 404 - Bad REST path, or Jira instance not found or temporarily suspended.");
expect(error?.message).toEqual("Error executing Axios Request HTTP 404 - Bad REST path, or Jira instance not found, renamed or temporarily suspended.");
});

it("should return 404 from failed request if Jira has been renamed", async () => {
const requestPayload = "TestRequestPayload";
jiraNock.post("/foo/bar", requestPayload).reply(405);
jiraNock.get("/status").reply(302);

let error;
try {
await getAxiosInstance(jiraHost, "secret").post("/foo/bar", requestPayload);
} catch (e) {
error = e;
}

expect(error?.status).toEqual(404);
expect(error?.message).toEqual("Error executing Axios Request HTTP 404 - Bad REST path, or Jira instance not found, renamed or temporarily suspended.");
});

});
7 changes: 5 additions & 2 deletions src/jira/client/axios.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export const getJiraErrorMessages = (status: number) => {
case 403:
return "HTTP 403 - The JWT token used does not correspond to an app that defines the jiraDevelopmentTool module, or the app does not define the 'WRITE' scope";
case 404:
return "HTTP 404 - Bad REST path, or Jira instance not found or temporarily suspended.";
return "HTTP 404 - Bad REST path, or Jira instance not found, renamed or temporarily suspended.";
case 413:
return "HTTP 413 - Data is too large. Submit fewer devinfo entities in each payload.";
case 429:
Expand Down Expand Up @@ -147,13 +147,16 @@ const instrumentRequest = (response) => {
const instrumentFailedRequest = (baseURL: string, logger: Logger) => {
return async (error: AxiosError) => {
instrumentRequest(error?.response);
if (error.response?.status === 503) {
if (error.response?.status === 503 || error.response?.status === 405) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this should be feature-flagged...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we are changing the behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm considering it fine because of the comment below.

try {
await axios.get("/status", { baseURL });
} catch (e) {
if (e.response.status === 503) {
logger.info(`503 from Jira: Jira instance '${baseURL}' has been deactivated, is suspended or does not exist. Returning 404 to our application.`);
error.response.status = 404;
} else if (e.response.status === 302) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and this one too...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but probably not a big deal cause this is error handling

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think it's ok because I'm just adding more logging to an error that is already being thrown. If these conditions aren't met, we will reject it anyways on line 163.

logger.info(`405 from Jira: Jira instance '${baseURL}' has been renamed. Returning 404 to our application.`);
error.response.status = 404;
}
}
}
Expand Down