From 2c7dece49c9a83992899140c4c6e636a926d2bb2 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Thu, 17 Mar 2022 20:17:19 +0100 Subject: [PATCH] fix(ng-dev): avoid runtime error when pull request does not have status/checks Our PR schema was incorrect and did not account for nullable GraphQL fields, causing errors like: ``` TypeError: Cannot read properties of null (reading 'contexts') at getStatusesForPullRequest (/home/douglasparker/Source/ng-cli/ng-dev/pr/common/fetch-pull-request.ts:130:38) at assertSignedCla (/home/douglasparker/Source/ng-cli/ng-dev/pr/common/validation/validations.ts:129:19) at loadAndValidatePullRequest (/home/douglasparker/Source/ng-cli/ng-dev/pr/merge/pull-request.ts:78:5) at processTicksAndRejections (node:internal/process/task_queues:96:5) at async PullRequestMergeTask.merge (/home/douglasparker/Source/ng-cli/ng-dev/pr/merge/task.ts:111:25) at async performMerge (/home/douglasparker/Source/ng-cli/ng-dev/pr/merge/index.ts:46:22) at async mergePullRequest (/home/douglasparker/Source/ng-cli/ng-dev/pr/merge/index.ts:39:9) at async Object.handler (/home/douglasparker/Source/ng-cli/ng-dev/pr/merge/cli.ts:41:3) ``` This commit corrects the typing/schema and fixes the runtime error. --- github-actions/slash-commands/main.js | 21 ++++---- ng-dev/pr/common/fetch-pull-request.ts | 71 ++++++++++++++++---------- 2 files changed, 55 insertions(+), 37 deletions(-) diff --git a/github-actions/slash-commands/main.js b/github-actions/slash-commands/main.js index 49c668282..cb599463c 100644 --- a/github-actions/slash-commands/main.js +++ b/github-actions/slash-commands/main.js @@ -54815,7 +54815,7 @@ var require_fetch_pull_request = __commonJS({ nodes: [ { commit: { - statusCheckRollup: { + statusCheckRollup: (0, typed_graphqlify_1.optional)({ state: typed_graphqlify_1.types.custom(), contexts: (0, typed_graphqlify_1.params)({ last: 100 }, { nodes: [ @@ -54834,7 +54834,7 @@ var require_fetch_pull_request = __commonJS({ }) ] }) - }, + }), message: typed_graphqlify_1.types.string } } @@ -54878,6 +54878,12 @@ var require_fetch_pull_request = __commonJS({ function getStatusesForPullRequest(pullRequest) { const nodes = pullRequest.commits.nodes; const { statusCheckRollup } = nodes[nodes.length - 1].commit; + if (!statusCheckRollup) { + return { + combinedStatus: PullRequestStatus.FAILING, + statuses: [] + }; + } const statuses = statusCheckRollup.contexts.nodes.map((context) => { switch (context.__typename) { case "CheckRun": @@ -54913,15 +54919,8 @@ var require_fetch_pull_request = __commonJS({ } } function normalizeGithubCheckState(conclusion, status) { - switch (status) { - case "COMPLETED": - break; - case "QUEUED": - case "IN_PROGRESS": - case "WAITING": - case "PENDING": - case "REQUESTED": - return PullRequestStatus.PENDING; + if (status !== "COMPLETED") { + return PullRequestStatus.PENDING; } switch (conclusion) { case "ACTION_REQUIRED": diff --git a/ng-dev/pr/common/fetch-pull-request.ts b/ng-dev/pr/common/fetch-pull-request.ts index 7f090555d..67a211ec9 100644 --- a/ng-dev/pr/common/fetch-pull-request.ts +++ b/ng-dev/pr/common/fetch-pull-request.ts @@ -6,16 +6,17 @@ * found in the LICENSE file at https://angular.io/license */ -import {AuthenticatedGitClient} from '../../utils/git/authenticated-git-client'; -import {getPendingPrs, getPr} from '../../utils/github'; -import {params, types as graphqlTypes, onUnion} from 'typed-graphqlify'; import { - MergeableState, CheckConclusionState, - StatusState, - PullRequestState, CheckStatusState, + MergeableState, + PullRequestState, + StatusState, } from '@octokit/graphql-schema'; +import {getPendingPrs, getPr} from '../../utils/github'; +import {types as graphqlTypes, onUnion, optional, params} from 'typed-graphqlify'; + +import {AuthenticatedGitClient} from '../../utils/git/authenticated-git-client'; /** A status for a pull request status or check. */ export enum PullRequestStatus { @@ -41,7 +42,7 @@ export const PR_SCHEMA = { nodes: [ { commit: { - statusCheckRollup: { + statusCheckRollup: optional({ state: graphqlTypes.custom(), contexts: params( {last: 100}, @@ -51,7 +52,7 @@ export const PR_SCHEMA = { CheckRun: { __typename: graphqlTypes.constant('CheckRun'), status: graphqlTypes.custom(), - conclusion: graphqlTypes.custom(), + conclusion: graphqlTypes.custom(), name: graphqlTypes.string, }, StatusContext: { @@ -63,7 +64,7 @@ export const PR_SCHEMA = { ], }, ), - }, + }), message: graphqlTypes.string, }, }, @@ -103,6 +104,16 @@ export const PR_SCHEMA = { export type PullRequestFromGithub = typeof PR_SCHEMA; +/** Type describing the normalized and combined status of a pull request. */ +export type PullRequestStatusInfo = { + combinedStatus: PullRequestStatus; + statuses: { + status: PullRequestStatus; + type: 'check' | 'status'; + name: string; + }[]; +}; + /** Fetches a pull request from Github. Returns null if an error occurred. */ export async function fetchPullRequestFromGithub( git: AuthenticatedGitClient, @@ -119,25 +130,36 @@ export async function fetchPendingPullRequestsFromGithub( } /** - * Gets the statuses for a commit from a pull requeste, using a consistent interface for both - * status and checks results. + * Gets the statuses for a commit from a pull request, using a consistent interface + * for both status and checks results. */ -export function getStatusesForPullRequest(pullRequest: PullRequestFromGithub) { +export function getStatusesForPullRequest( + pullRequest: PullRequestFromGithub, +): PullRequestStatusInfo { const nodes = pullRequest.commits.nodes; /** The combined github status and github checks object. */ const {statusCheckRollup} = nodes[nodes.length - 1].commit; + // If there is no status check rollup (i.e. no status nor checks), we + // consider the pull request status as failing. + if (!statusCheckRollup) { + return { + combinedStatus: PullRequestStatus.FAILING, + statuses: [], + }; + } + const statuses = statusCheckRollup.contexts.nodes.map((context) => { switch (context.__typename) { case 'CheckRun': return { - type: 'check', + type: 'check' as const, name: context.name, status: normalizeGithubCheckState(context.conclusion, context.status), }; case 'StatusContext': return { - type: 'status', + type: 'status' as const, name: context.context, status: normalizeGithubStatusState(context.state), }; @@ -151,7 +173,7 @@ export function getStatusesForPullRequest(pullRequest: PullRequestFromGithub) { } /** Retrieve the normalized PullRequestStatus for the provided github status state. */ -function normalizeGithubStatusState(state: StatusState) { +function normalizeGithubStatusState(state: StatusState): PullRequestStatus { switch (state) { case 'FAILURE': case 'ERROR': @@ -165,19 +187,16 @@ function normalizeGithubStatusState(state: StatusState) { } /** Retrieve the normalized PullRequestStatus for the provided github check state. */ -function normalizeGithubCheckState(conclusion: CheckConclusionState, status: CheckStatusState) { - switch (status) { - case 'COMPLETED': - break; - case 'QUEUED': - case 'IN_PROGRESS': - case 'WAITING': - case 'PENDING': - case 'REQUESTED': - return PullRequestStatus.PENDING; +function normalizeGithubCheckState( + conclusion: CheckConclusionState | null, + status: CheckStatusState, +): PullRequestStatus { + if (status !== 'COMPLETED') { + return PullRequestStatus.PENDING; } - switch (conclusion) { + // If the `status` is completed, a conclusion is guaranteed to be set. + switch (conclusion!) { case 'ACTION_REQUIRED': case 'TIMED_OUT': case 'CANCELLED':