From 223264ab3686a0f6f89943d816d2f07dcea49acf Mon Sep 17 00:00:00 2001 From: Gary Xue <105693507+gxueatlassian@users.noreply.github.com> Date: Mon, 18 Dec 2023 13:23:42 +1100 Subject: [PATCH] ARC-2759 skip the error "Commit not found by sha..." if on last try (#2618) --- src/config/feature-flags.ts | 3 +- src/github/client/github-client-errors.ts | 7 + .../client/github-client-interceptors.test.ts | 20 +- .../client/github-client-interceptors.ts | 14 +- src/sqs/push.ts | 2 +- src/transforms/push.test.ts | 222 ++++++++++++------ src/transforms/push.ts | 14 +- 7 files changed, 205 insertions(+), 77 deletions(-) diff --git a/src/config/feature-flags.ts b/src/config/feature-flags.ts index cb59ce4ead..53d8516266 100644 --- a/src/config/feature-flags.ts +++ b/src/config/feature-flags.ts @@ -26,7 +26,8 @@ export enum BooleanFlags { USE_CUSTOM_ROOT_CA_BUNDLE = "use-custom-root-ca-bundle", GENERATE_CORE_HEAP_DUMPS_ON_LOW_MEM = "generate-core-heap-dumps-on-low-mem", USE_RATELIMIT_ON_JIRA_CLIENT = "use-ratelimit-on-jira-client", - SKIP_PROCESS_QUEUE_IF_ISSUE_NOT_FOUND = "skip-process-queue-when-issue-not-exists" + SKIP_PROCESS_QUEUE_IF_ISSUE_NOT_FOUND = "skip-process-queue-when-issue-not-exists", + SKIP_COMMIT_IF_SHA_NOT_FOUND_ON_LAST_TRY = "skip-commit-if-sha-not-found-on-last-try" } export enum StringFlags { diff --git a/src/github/client/github-client-errors.ts b/src/github/client/github-client-errors.ts index 7c8662cadc..6c66c1fd9a 100644 --- a/src/github/client/github-client-errors.ts +++ b/src/github/client/github-client-errors.ts @@ -86,6 +86,13 @@ export class GithubClientNotFoundError extends GithubClientError { } } +export class GithubClientCommitNotFoundBySHAError extends GithubClientError { + constructor(cause: AxiosError) { + super("Commit not found by sha", cause); + this.uiErrorCode = "RESOURCE_NOT_FOUND"; + } +} + /** * Type for errors section in GraphQL response diff --git a/src/github/client/github-client-interceptors.test.ts b/src/github/client/github-client-interceptors.test.ts index d650db10b7..722361b86d 100644 --- a/src/github/client/github-client-interceptors.test.ts +++ b/src/github/client/github-client-interceptors.test.ts @@ -3,7 +3,9 @@ import { getLogger } from "config/logger"; import { GithubClientBlockedIpError, GithubClientInvalidPermissionsError, - GithubClientNotFoundError, GithubClientSSOLoginError + GithubClientNotFoundError, + GithubClientSSOLoginError, + GithubClientCommitNotFoundBySHAError } from "~/src/github/client/github-client-errors"; describe("github-client-interceptors", () => { @@ -66,4 +68,20 @@ describe("github-client-interceptors", () => { } expect(error!).toBeInstanceOf(GithubClientSSOLoginError); }); + + it("correctly maps commits not found by sha error", async () => { + gheNock.get("/").reply(422, { + "message": "No commit found for SHA: whatever the sha is", + "documentation_url": "https://docs.github.com" + }); + + let error: Error; + const client = await createAnonymousClient(gheUrl, jiraHost, { trigger: "test" }, getLogger("test")); + try { + await client.getPage(1000); + } catch (err: unknown) { + error = err as Error; + } + expect(error!).toBeInstanceOf(GithubClientCommitNotFoundBySHAError); + }); }); diff --git a/src/github/client/github-client-interceptors.ts b/src/github/client/github-client-interceptors.ts index e5623e43fb..ccae216a9b 100644 --- a/src/github/client/github-client-interceptors.ts +++ b/src/github/client/github-client-interceptors.ts @@ -5,7 +5,8 @@ import { GithubClientInvalidPermissionsError, GithubClientRateLimitingError, GithubClientNotFoundError, - GithubClientSSOLoginError + GithubClientSSOLoginError, + GithubClientCommitNotFoundBySHAError } from "./github-client-errors"; import Logger from "bunyan"; import { statsd } from "config/statsd"; @@ -166,6 +167,17 @@ export const handleFailedRequest = (rootLogger: Logger) => return Promise.reject(mappedError); } + if (status === 422) { + if ((String(err.response?.data?.message) || "").toLocaleLowerCase().includes("no commit found for sha")) { + const mappedError = new GithubClientCommitNotFoundBySHAError(err); + logger.warn({ + err: mappedError, + remote: response.data.message + }, "commit not found by sha"); + return Promise.reject(mappedError); + } + } + const isWarning = status && (status >= 300 && status < 500 && status !== 400); if (isWarning) { diff --git a/src/sqs/push.ts b/src/sqs/push.ts index 152172ac75..c7b8582049 100644 --- a/src/sqs/push.ts +++ b/src/sqs/push.ts @@ -30,5 +30,5 @@ export const pushQueueMessageHandler: MessageHandler = subTrigger: "push" }; const gitHubInstallationClient = await createInstallationClient(installationId, jiraHost, metrics, context.log, payload.gitHubAppConfig?.gitHubAppId); - await processPush(gitHubInstallationClient, payload, context.log); + await processPush(gitHubInstallationClient, context, payload, context.log); }; diff --git a/src/transforms/push.test.ts b/src/transforms/push.test.ts index 8f3fd1910f..96a8921ed2 100644 --- a/src/transforms/push.test.ts +++ b/src/transforms/push.test.ts @@ -5,6 +5,7 @@ import { GitHubCommit, GitHubRepository } from "interfaces/github"; import { shouldSendAll, booleanFlag, BooleanFlags, numberFlag, NumberFlags } from "config/feature-flags"; import { getLogger } from "config/logger"; import { GitHubInstallationClient } from "../github/client/github-installation-client"; +import { GithubClientCommitNotFoundBySHAError } from "../github/client/github-client-errors"; import { DatabaseStateCreator, CreatorResult } from "test/utils/database-state-creator"; jest.mock("../sqs/queues"); @@ -12,6 +13,21 @@ jest.mock("config/feature-flags"); const logger = getLogger("test"); describe("Enqueue push", () => { + + let db: CreatorResult; + let issueKey: string; + let sha: string; + beforeEach(async () => { + db = await new DatabaseStateCreator() + .forCloud() + .create(); + when(shouldSendAll).calledWith("commits", expect.anything(), expect.anything()).mockResolvedValue(false); + when(numberFlag).calledWith(NumberFlags.SKIP_PROCESS_QUEUE_IF_ISSUE_NOT_FOUND_TIMEOUT, expect.anything(), expect.anything()) + .mockResolvedValue(10000); + issueKey = `KEY-${new Date().getTime()}`; + sha = `sha-${issueKey}`; + }); + it("should push GitHubAppConfig to payload", async () => { await enqueuePush({ installation: { id: 123, node_id: 456 }, @@ -105,21 +121,72 @@ describe("Enqueue push", () => { }), 0, expect.anything()); }); - describe("Skipping msg when issue not exist", () => { + describe("Maybe skipp commit when ff is on and commimt sha on not found", () => { + it("when ff is OFF, should throw error for the problematic commits if 422 and even on last try", async () => { + + when(booleanFlag).calledWith(BooleanFlags.SKIP_COMMIT_IF_SHA_NOT_FOUND_ON_LAST_TRY, expect.anything()) + .mockResolvedValue(false); + + githubUserTokenNock(db.subscription.gitHubInstallationId); + githubUserTokenNock(db.subscription.gitHubInstallationId); + mockGitHubCommitRestApi(); + mockGitHubCommitRestApi422("invalid-sha"); + + const pushPayload = getPushPayload(); + pushPayload.shas.push({ + id: "invalid-sha", + issueKeys: [ issueKey ] + }); + + await expect(async () => { + await processPush(getGitHubClient(), getContext({ lastAttempt: true }), pushPayload, logger); + }).rejects.toThrow(GithubClientCommitNotFoundBySHAError); + }); + + it("should throw error for the problematic commits if 422 but NOT on last try", async () => { + + when(booleanFlag).calledWith(BooleanFlags.SKIP_COMMIT_IF_SHA_NOT_FOUND_ON_LAST_TRY, expect.anything()) + .mockResolvedValue(true); + + githubUserTokenNock(db.subscription.gitHubInstallationId); + githubUserTokenNock(db.subscription.gitHubInstallationId); + mockGitHubCommitRestApi(); + mockGitHubCommitRestApi422("invalid-sha"); + + const pushPayload = getPushPayload(); + pushPayload.shas.push({ + id: "invalid-sha", + issueKeys: [ issueKey ] + }); + + await expect(async () => { + await processPush(getGitHubClient(), getContext({ lastAttempt: false }), pushPayload, logger); + }).rejects.toThrow(GithubClientCommitNotFoundBySHAError); + }); - let db: CreatorResult; - let issueKey; - let sha; - beforeEach(async () => { - db = await new DatabaseStateCreator() - .forCloud() - .create(); - when(shouldSendAll).calledWith("commits", expect.anything(), expect.anything()).mockResolvedValue(false); - when(numberFlag).calledWith(NumberFlags.SKIP_PROCESS_QUEUE_IF_ISSUE_NOT_FOUND_TIMEOUT, expect.anything(), expect.anything()) - .mockResolvedValue(10000); - issueKey = `KEY-${new Date().getTime()}`; - sha = `sha-${issueKey}`; + it("should success skip the problematic commits if 422 on last try", async () => { + + when(booleanFlag).calledWith(BooleanFlags.SKIP_COMMIT_IF_SHA_NOT_FOUND_ON_LAST_TRY, expect.anything()) + .mockResolvedValue(true); + + githubUserTokenNock(db.subscription.gitHubInstallationId); + githubUserTokenNock(db.subscription.gitHubInstallationId); + mockGitHubCommitRestApi(); + mockGitHubCommitRestApi422("invalid-sha"); + + mockJiraDevInfoAcceptUpdate(); + + const pushPayload = getPushPayload(); + pushPayload.shas.push({ + id: "invalid-sha", + issueKeys: [ issueKey ] + }); + + await processPush(getGitHubClient(), getContext({ lastAttempt: true }), pushPayload, logger); }); + }); + + describe("Skipping msg when issue not exist", () => { describe("Use redis to avoid overload jira", () => { it("should reuse status from redis and only call jira once for same issue-key", async () => { @@ -128,8 +195,8 @@ describe("Enqueue push", () => { mockIssueNotExists(); - await processPush(getGitHubClient(), getPushPayload(), logger); - await processPush(getGitHubClient(), getPushPayload(), logger); + await processPush(getGitHubClient(), getContext(), getPushPayload(), logger); + await processPush(getGitHubClient(), getContext(), getPushPayload(), logger); }); }); @@ -140,7 +207,7 @@ describe("Enqueue push", () => { mockIssueNotExists(); - await processPush(getGitHubClient(), getPushPayload(), logger); + await processPush(getGitHubClient(), getContext(), getPushPayload(), logger); }); @@ -156,7 +223,7 @@ describe("Enqueue push", () => { mockJiraDevInfoAcceptUpdate(); - await processPush(getGitHubClient(), getPushPayload(), logger); + await processPush(getGitHubClient(), getContext(), getPushPayload(), logger); }); @@ -170,66 +237,81 @@ describe("Enqueue push", () => { mockJiraDevInfoAcceptUpdate(); - await processPush(getGitHubClient(), getPushPayload(), logger); + await processPush(getGitHubClient(), getContext(), getPushPayload(), logger); }); + }); - const mockJiraDevInfoAcceptUpdate = () => { - jiraNock.post("/rest/devinfo/0.10/bulk", (reqBody) => { - return reqBody.repositories[0].commits.flatMap(c => c.issueKeys).some(ck => ck === issueKey); - }).reply(202, ""); - }; - - const mockGitHubCommitRestApi = () => { - githubNock - .get("/repos/org1/repo1/commits/" + sha) - .reply(200, { - files: [], - sha - }); - }; - - const getPushPayload = () => { - return { - jiraHost, - installationId: db.subscription.gitHubInstallationId, - gitHubAppConfig: undefined, - webhookId: "aaa", - repository: { - owner: { login: "org1" }, - name: "repo1" - } as GitHubRepository, - shas: [{ - id: sha, - issueKeys: [issueKey] - }] - }; + const mockJiraDevInfoAcceptUpdate = () => { + jiraNock.post("/rest/devinfo/0.10/bulk", (reqBody) => { + return reqBody.repositories[0].commits.flatMap(c => c.issueKeys).some(ck => ck === issueKey); + }).reply(202, ""); + }; + + const mockGitHubCommitRestApi = () => { + githubNock + .get("/repos/org1/repo1/commits/" + sha) + .reply(200, { + files: [], + sha + }); + }; + + const mockGitHubCommitRestApi422 = (sha: string) => { + githubNock + .get("/repos/org1/repo1/commits/" + sha) + .reply(422, { + message: `No commit found for SHA: ${sha}`, + documentation_url: "https://docs.github.com" + }); + }; + + const getPushPayload = () => { + return { + jiraHost, + installationId: db.subscription.gitHubInstallationId, + gitHubAppConfig: undefined, + webhookId: "aaa", + repository: { + owner: { login: "org1" }, + name: "repo1" + } as GitHubRepository, + shas: [{ + id: sha, + issueKeys: [issueKey] + }] }; + }; - const mockIssueExists = () => { - jiraNock.get(`/rest/api/latest/issue/${issueKey}`) - .query({ fields: "summary" }).reply(200, {}); - }; + const mockIssueExists = () => { + jiraNock.get(`/rest/api/latest/issue/${issueKey}`) + .query({ fields: "summary" }).reply(200, {}); + }; - const mockIssueNotExists = () => { - jiraNock.get(`/rest/api/latest/issue/${issueKey}`) - .query({ fields: "summary" }).reply(404, ""); - }; + const mockIssueNotExists = () => { + jiraNock.get(`/rest/api/latest/issue/${issueKey}`) + .query({ fields: "summary" }).reply(404, ""); + }; - const getGitHubClient = () => { - return new GitHubInstallationClient({ - appId: 2, - githubBaseUrl: "https://api.github.com", - installationId: db.subscription.gitHubInstallationId - }, { - apiUrl: "https://api.github.com", - baseUrl: "https://github.com", - graphqlUrl: "https://api.github.com/graphql", - hostname: "https://github.com", - apiKeyConfig: undefined, - proxyBaseUrl: undefined - }, jiraHost, { trigger: "test" }, logger, undefined); + const getGitHubClient = () => { + return new GitHubInstallationClient({ + appId: 2, + githubBaseUrl: "https://api.github.com", + installationId: db.subscription.gitHubInstallationId + }, { + apiUrl: "https://api.github.com", + baseUrl: "https://github.com", + graphqlUrl: "https://api.github.com/graphql", + hostname: "https://github.com", + apiKeyConfig: undefined, + proxyBaseUrl: undefined + }, jiraHost, { trigger: "test" }, logger, undefined); + }; + + const getContext = (override?: any): any => { + return { + ...override }; - }); + }; }); diff --git a/src/transforms/push.ts b/src/transforms/push.ts index 11912477db..0345cd3580 100644 --- a/src/transforms/push.ts +++ b/src/transforms/push.ts @@ -1,5 +1,6 @@ import Logger from "bunyan"; import { uniq } from "lodash"; +import { createHashWithSharedSecret } from "utils/encryption"; import { Subscription } from "models/subscription"; import { getJiraClient, JiraClient } from "../jira/client/jira-client"; import { JiraClientError } from "../jira/client/axios"; @@ -8,8 +9,9 @@ import { emitWebhookProcessedMetrics } from "utils/webhook-utils"; import { JiraCommit, JiraCommitFile, JiraCommitFileChangeTypeEnum } from "interfaces/jira"; import { isBlocked, shouldSendAll, booleanFlag, BooleanFlags } from "config/feature-flags"; import { sqsQueues } from "../sqs/queues"; -import { GitHubAppConfig, PushQueueMessagePayload } from "~/src/sqs/sqs.types"; +import { GitHubAppConfig, PushQueueMessagePayload, SQSMessageContext } from "~/src/sqs/sqs.types"; import { GitHubInstallationClient } from "../github/client/github-installation-client"; +import { GithubClientCommitNotFoundBySHAError } from "../github/client/github-client-errors"; import { compact, isEmpty } from "lodash"; import { GithubCommitFile, GitHubPushData } from "interfaces/github"; import { transformRepositoryDevInfoBulk } from "~/src/transforms/transform-repository"; @@ -87,7 +89,7 @@ export const createJobData = async (payload: GitHubPushData, jiraHost: string, l export const enqueuePush = async (payload: GitHubPushData, jiraHost: string, logger: Logger, gitHubAppConfig?: GitHubAppConfig) => await sqsQueues.push.sendMessage(await createJobData(payload, jiraHost, logger, gitHubAppConfig), 0, logger); -export const processPush = async (github: GitHubInstallationClient, payload: PushQueueMessagePayload, rootLogger: Logger) => { +export const processPush = async (github: GitHubInstallationClient, context: SQSMessageContext, payload: PushQueueMessagePayload, rootLogger: Logger) => { const { repository, repository: { owner, name: repo }, @@ -143,10 +145,10 @@ export const processPush = async (github: GitHubInstallationClient, payload: Pus const recentShas = shas.slice(0, MAX_COMMIT_HISTORY); const invalidIssueKeys = await tryGetInvalidIssueKeys(recentShas, subscription, jiraClient, log); + const shouldSkipCommitIfShaNotFound = context.lastAttempt && await booleanFlag(BooleanFlags.SKIP_COMMIT_IF_SHA_NOT_FOUND_ON_LAST_TRY, jiraHost); const commitPromises: Promise[] = recentShas.map(async (sha): Promise => { try { - if (await booleanFlag(BooleanFlags.SKIP_PROCESS_QUEUE_IF_ISSUE_NOT_FOUND, jiraHost)) { if (sha.issueKeys.every(k => invalidIssueKeys.includes(k))) { log.info("Issue key not found on jira, skip processing commits"); @@ -194,6 +196,12 @@ export const processPush = async (github: GitHubInstallationClient, payload: Pus flags: isMergeCommit ? ["MERGE_COMMIT"] : undefined }; } catch (err: unknown) { + + if (shouldSkipCommitIfShaNotFound && err instanceof GithubClientCommitNotFoundBySHAError) { + log.warn({ err, commitSha: createHashWithSharedSecret(sha.id) }, "Skip for commit not found by sha error on last try"); + return null; + } + log.warn({ err }, "Failed to fetch data from GitHub"); throw err; }