Skip to content

Commit

Permalink
fix(ng-dev): avoid runtime error when pull request does not have stat…
Browse files Browse the repository at this point in the history
…us/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.
  • Loading branch information
devversion committed Mar 17, 2022
1 parent 660672d commit 2c7dece
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 37 deletions.
21 changes: 10 additions & 11 deletions github-actions/slash-commands/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: [
Expand All @@ -54834,7 +54834,7 @@ var require_fetch_pull_request = __commonJS({
})
]
})
},
}),
message: typed_graphqlify_1.types.string
}
}
Expand Down Expand Up @@ -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":
Expand Down Expand Up @@ -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":
Expand Down
71 changes: 45 additions & 26 deletions ng-dev/pr/common/fetch-pull-request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -41,7 +42,7 @@ export const PR_SCHEMA = {
nodes: [
{
commit: {
statusCheckRollup: {
statusCheckRollup: optional({
state: graphqlTypes.custom<StatusState>(),
contexts: params(
{last: 100},
Expand All @@ -51,7 +52,7 @@ export const PR_SCHEMA = {
CheckRun: {
__typename: graphqlTypes.constant('CheckRun'),
status: graphqlTypes.custom<CheckStatusState>(),
conclusion: graphqlTypes.custom<CheckConclusionState>(),
conclusion: graphqlTypes.custom<CheckConclusionState | null>(),
name: graphqlTypes.string,
},
StatusContext: {
Expand All @@ -63,7 +64,7 @@ export const PR_SCHEMA = {
],
},
),
},
}),
message: graphqlTypes.string,
},
},
Expand Down Expand Up @@ -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,
Expand All @@ -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),
};
Expand All @@ -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':
Expand All @@ -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':
Expand Down

0 comments on commit 2c7dece

Please sign in to comment.