Skip to content

Commit

Permalink
Fix handling of GHE without merge queue support
Browse files Browse the repository at this point in the history
Summary:
facebook#830 and facebook#906 reported sapling not working for old Github Enterprise versions since their graphql schema doesnt include merge queue.

This adds a query to detect if merge queue is supported in the graphql version. Then, it will issue a different "YourPullRequestsQuery" depending on if merge queue is supported.

I considered a few other approaches:
- Just detect the error message and retry without merge queue field - a little gross to extract from the error
- Use "@include" instead of an entirely separate query - didn't seem to count as a valid query still via graphiql at least (unsure if the query client here would strip out the unused field though)

Test Plan:
Haven't had a chance to test yet, will try and validate that both versions work and that the detection logic works.
  • Loading branch information
alex-statsig committed Jul 25, 2024
1 parent 5f7a862 commit 986844b
Show file tree
Hide file tree
Showing 4 changed files with 234 additions and 12 deletions.
157 changes: 157 additions & 0 deletions addons/isl-server/src/github/generated/graphql.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29334,6 +29334,118 @@ export enum WorkflowState {
DisabledManually = 'DISABLED_MANUALLY'
}

/** One possible value for a given Enum. Enum values are unique values, not a placeholder for a string or numeric value. However an Enum value is returned in a JSON response as a string. */
export type __EnumValue = {
__typename?: '__EnumValue';
name: Scalars['String'];
description?: Maybe<Scalars['String']>;
isDeprecated: Scalars['Boolean'];
deprecationReason?: Maybe<Scalars['String']>;
};

/** Object and Interface types are described by a list of Fields, each of which has a name, potentially a list of arguments, and a return type. */
export type __Field = {
__typename?: '__Field';
name: Scalars['String'];
description?: Maybe<Scalars['String']>;
args: Array<__InputValue>;
type: __Type;
isDeprecated: Scalars['Boolean'];
deprecationReason?: Maybe<Scalars['String']>;
};


/** Object and Interface types are described by a list of Fields, each of which has a name, potentially a list of arguments, and a return type. */
export type __FieldArgsArgs = {
includeDeprecated?: InputMaybe<Scalars['Boolean']>;
};

/** Arguments provided to Fields or Directives and the input fields of an InputObject are represented as Input Values which describe their type and optionally a default value. */
export type __InputValue = {
__typename?: '__InputValue';
name: Scalars['String'];
description?: Maybe<Scalars['String']>;
type: __Type;
/** A GraphQL-formatted string representing the default value for this input value. */
defaultValue?: Maybe<Scalars['String']>;
isDeprecated: Scalars['Boolean'];
deprecationReason?: Maybe<Scalars['String']>;
};

/**
* The fundamental unit of any GraphQL Schema is the type. There are many kinds of types in GraphQL as represented by the `__TypeKind` enum.
*
* Depending on the kind of a type, certain fields describe information about that type. Scalar types provide no information beyond a name, description and optional `specifiedByURL`, while Enum types provide their values. Object and Interface types provide the fields they describe. Abstract types, Union and Interface, provide the Object types possible at runtime. List and NonNull types compose other types.
*/
export type __Type = {
__typename?: '__Type';
kind: __TypeKind;
name?: Maybe<Scalars['String']>;
description?: Maybe<Scalars['String']>;
specifiedByURL?: Maybe<Scalars['String']>;
fields?: Maybe<Array<__Field>>;
interfaces?: Maybe<Array<__Type>>;
possibleTypes?: Maybe<Array<__Type>>;
enumValues?: Maybe<Array<__EnumValue>>;
inputFields?: Maybe<Array<__InputValue>>;
ofType?: Maybe<__Type>;
};


/**
* The fundamental unit of any GraphQL Schema is the type. There are many kinds of types in GraphQL as represented by the `__TypeKind` enum.
*
* Depending on the kind of a type, certain fields describe information about that type. Scalar types provide no information beyond a name, description and optional `specifiedByURL`, while Enum types provide their values. Object and Interface types provide the fields they describe. Abstract types, Union and Interface, provide the Object types possible at runtime. List and NonNull types compose other types.
*/
export type __TypeFieldsArgs = {
includeDeprecated?: InputMaybe<Scalars['Boolean']>;
};


/**
* The fundamental unit of any GraphQL Schema is the type. There are many kinds of types in GraphQL as represented by the `__TypeKind` enum.
*
* Depending on the kind of a type, certain fields describe information about that type. Scalar types provide no information beyond a name, description and optional `specifiedByURL`, while Enum types provide their values. Object and Interface types provide the fields they describe. Abstract types, Union and Interface, provide the Object types possible at runtime. List and NonNull types compose other types.
*/
export type __TypeEnumValuesArgs = {
includeDeprecated?: InputMaybe<Scalars['Boolean']>;
};


/**
* The fundamental unit of any GraphQL Schema is the type. There are many kinds of types in GraphQL as represented by the `__TypeKind` enum.
*
* Depending on the kind of a type, certain fields describe information about that type. Scalar types provide no information beyond a name, description and optional `specifiedByURL`, while Enum types provide their values. Object and Interface types provide the fields they describe. Abstract types, Union and Interface, provide the Object types possible at runtime. List and NonNull types compose other types.
*/
export type __TypeInputFieldsArgs = {
includeDeprecated?: InputMaybe<Scalars['Boolean']>;
};

/** An enum describing what kind of type a given `__Type` is. */
export enum __TypeKind {
/** Indicates this type is a scalar. */
Scalar = 'SCALAR',
/** Indicates this type is an object. `fields` and `interfaces` are valid fields. */
Object = 'OBJECT',
/** Indicates this type is an interface. `fields`, `interfaces`, and `possibleTypes` are valid fields. */
Interface = 'INTERFACE',
/** Indicates this type is a union. `possibleTypes` is a valid field. */
Union = 'UNION',
/** Indicates this type is an enum. `enumValues` is a valid field. */
Enum = 'ENUM',
/** Indicates this type is an input object. `inputFields` is a valid field. */
InputObject = 'INPUT_OBJECT',
/** Indicates this type is a list. `ofType` is a valid field. */
List = 'LIST',
/** Indicates this type is a non-null. `ofType` is a valid field. */
NonNull = 'NON_NULL'
}

export type MergeQueueSupportQueryVariables = Exact<{ [key: string]: never; }>;


export type MergeQueueSupportQueryData = { __typename?: 'Query', __type?: { __typename: '__Type' } | null };

export type CommentParts_CommitComment_ = { __typename?: 'CommitComment', bodyHTML: string, createdAt: string, author?: { __typename?: 'Bot', login: string, avatarUrl: string } | { __typename?: 'EnterpriseUserAccount', login: string, avatarUrl: string } | { __typename?: 'Mannequin', login: string, avatarUrl: string } | { __typename?: 'Organization', login: string, avatarUrl: string } | { __typename?: 'User', login: string, avatarUrl: string } | null };

export type CommentParts_Discussion_ = { __typename?: 'Discussion', bodyHTML: string, createdAt: string, author?: { __typename?: 'Bot', login: string, avatarUrl: string } | { __typename?: 'EnterpriseUserAccount', login: string, avatarUrl: string } | { __typename?: 'Mannequin', login: string, avatarUrl: string } | { __typename?: 'Organization', login: string, avatarUrl: string } | { __typename?: 'User', login: string, avatarUrl: string } | null };
Expand Down Expand Up @@ -29398,6 +29510,14 @@ export type YourPullRequestsQueryVariables = Exact<{

export type YourPullRequestsQueryData = { __typename?: 'Query', search: { __typename?: 'SearchResultItemConnection', nodes?: Array<{ __typename?: 'App' } | { __typename?: 'Discussion' } | { __typename?: 'Issue' } | { __typename?: 'MarketplaceListing' } | { __typename?: 'Organization' } | { __typename: 'PullRequest', number: number, title: string, body: string, state: PullRequestState, isDraft: boolean, url: string, reviewDecision?: PullRequestReviewDecision | null, comments: { __typename?: 'IssueCommentConnection', totalCount: number }, mergeQueueEntry?: { __typename?: 'MergeQueueEntry', estimatedTimeToMerge?: number | null } | null, commits: { __typename?: 'PullRequestCommitConnection', nodes?: Array<{ __typename?: 'PullRequestCommit', commit: { __typename?: 'Commit', statusCheckRollup?: { __typename?: 'StatusCheckRollup', state: StatusState } | null } } | null> | null } } | { __typename?: 'Repository' } | { __typename?: 'User' } | null> | null } };

export type YourPullRequestsWithoutMergeQueueQueryVariables = Exact<{
searchQuery: Scalars['String'];
numToFetch: Scalars['Int'];
}>;


export type YourPullRequestsWithoutMergeQueueQueryData = { __typename?: 'Query', search: { __typename?: 'SearchResultItemConnection', nodes?: Array<{ __typename?: 'App' } | { __typename?: 'Discussion' } | { __typename?: 'Issue' } | { __typename?: 'MarketplaceListing' } | { __typename?: 'Organization' } | { __typename: 'PullRequest', number: number, title: string, body: string, state: PullRequestState, isDraft: boolean, url: string, reviewDecision?: PullRequestReviewDecision | null, comments: { __typename?: 'IssueCommentConnection', totalCount: number }, commits: { __typename?: 'PullRequestCommitConnection', nodes?: Array<{ __typename?: 'PullRequestCommit', commit: { __typename?: 'Commit', statusCheckRollup?: { __typename?: 'StatusCheckRollup', state: StatusState } | null } } | null> | null } } | { __typename?: 'Repository' } | { __typename?: 'User' } | null> | null } };

export const CommentParts = `
fragment CommentParts on Comment {
bodyHTML
Expand All @@ -29420,6 +29540,13 @@ export const ReactionParts = `
}
}
`;
export const MergeQueueSupportQuery = `
query MergeQueueSupportQuery {
__type(name: "MergeQueueEntry") {
__typename
}
}
`;
export const PullRequestCommentsQuery = `
query PullRequestCommentsQuery($url: URI!, $numToFetch: Int!) {
resource(url: $url) {
Expand Down Expand Up @@ -29480,5 +29607,35 @@ export const YourPullRequestsQuery = `
}
}
}
}
`;
export const YourPullRequestsWithoutMergeQueueQuery = `
query YourPullRequestsWithoutMergeQueueQuery($searchQuery: String!, $numToFetch: Int!) {
search(query: $searchQuery, type: ISSUE, first: $numToFetch) {
nodes {
... on PullRequest {
__typename
number
title
body
state
isDraft
url
reviewDecision
comments {
totalCount
}
commits(last: 1) {
nodes {
commit {
statusCheckRollup {
state
}
}
}
}
}
}
}
}
`;
56 changes: 44 additions & 12 deletions addons/isl-server/src/github/githubCodeReviewProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,17 @@
import type {CodeReviewProvider} from '../CodeReviewProvider';
import type {Logger} from '../logger';
import type {
MergeQueueSupportQueryData,
MergeQueueSupportQueryVariables,
PullRequestCommentsQueryData,
PullRequestCommentsQueryVariables,
PullRequestReviewComment,
PullRequestReviewDecision,
ReactionContent,
YourPullRequestsQueryData,
YourPullRequestsQueryVariables,
YourPullRequestsWithoutMergeQueueQueryData,
YourPullRequestsWithoutMergeQueueQueryVariables,
} from './generated/graphql';
import type {
CodeReviewSystem,
Expand All @@ -24,18 +28,18 @@ import type {
Result,
DiffComment,
} from 'isl/src/types';
import type {ParsedDiff} from 'shared/patch/parse';

import {
MergeQueueSupportQuery,
PullRequestCommentsQuery,
PullRequestState,
StatusState,
YourPullRequestsQuery,
YourPullRequestsWithoutMergeQueueQuery,
} from './generated/graphql';
import queryGraphQL from './queryGraphQL';
import {TypedEventEmitter} from 'shared/TypedEventEmitter';
import {debounce} from 'shared/debounce';
import {parsePatch} from 'shared/patch/parse';
import {notEmpty} from 'shared/utils';

export type GitHubDiffSummary = {
Expand All @@ -55,6 +59,7 @@ type GitHubCodeReviewSystem = CodeReviewSystem & {type: 'github'};
export class GitHubCodeReviewProvider implements CodeReviewProvider {
constructor(private codeReviewSystem: GitHubCodeReviewSystem, private logger: Logger) {}
private diffSummaries = new TypedEventEmitter<'data', Map<DiffId, GitHubDiffSummary>>();
private hasMergeQueueSupport: boolean | null = null;

onChangeDiffSummaries(
callback: (result: Result<Map<DiffId, GitHubDiffSummary>>) => unknown,
Expand All @@ -71,20 +76,47 @@ export class GitHubCodeReviewProvider implements CodeReviewProvider {
};
}

private async detectMergeQueueSupport(): Promise<boolean> {
const data = await this.query<MergeQueueSupportQueryData, MergeQueueSupportQueryVariables>(
MergeQueueSupportQuery,
{},
);
return data?.__type != null;
}

private fetchYourPullRequestsGraphQL(
includeMergeQueue: boolean,
): Promise<YourPullRequestsQueryData | undefined> {
const variables = {
// TODO: somehow base this query on the list of DiffIds
// This is not very easy with github's graphql API, which doesn't allow more than 5 "OR"s in a search query.
// But if we used one-query-per-diff we would reach rate limiting too quickly.
searchQuery: `repo:${this.codeReviewSystem.owner}/${this.codeReviewSystem.repo} is:pr author:@me`,
numToFetch: 50,
};
if (includeMergeQueue) {
return this.query<YourPullRequestsQueryData, YourPullRequestsQueryVariables>(
YourPullRequestsQuery,
variables,
);
} else {
return this.query<
YourPullRequestsWithoutMergeQueueQueryData,
YourPullRequestsWithoutMergeQueueQueryVariables
>(YourPullRequestsWithoutMergeQueueQuery, variables);
}
}

triggerDiffSummariesFetch = debounce(
async () => {
try {
if (this.hasMergeQueueSupport == null) {
this.logger.info('detecting if merge queue is supported');
this.hasMergeQueueSupport = (await this.detectMergeQueueSupport()) ?? false;
this.logger.info('set merge queue support to ' + this.hasMergeQueueSupport);
}
this.logger.info('fetching github PR summaries');
const allSummaries = await this.query<
YourPullRequestsQueryData,
YourPullRequestsQueryVariables
>(YourPullRequestsQuery, {
// TODO: somehow base this query on the list of DiffIds
// This is not very easy with github's graphql API, which doesn't allow more than 5 "OR"s in a search query.
// But if we used one-query-per-diff we would reach rate limiting too quickly.
searchQuery: `repo:${this.codeReviewSystem.owner}/${this.codeReviewSystem.repo} is:pr author:@me`,
numToFetch: 50,
});
const allSummaries = await this.fetchYourPullRequestsGraphQL(this.hasMergeQueueSupport);
if (allSummaries?.search.nodes == null) {
this.diffSummaries.emit('data', new Map());
return;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
query MergeQueueSupportQuery {
__type(name: "MergeQueueEntry") {
__typename
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
query YourPullRequestsWithoutMergeQueueQuery($searchQuery: String!, $numToFetch: Int!) {
search(query: $searchQuery, type: ISSUE, first: $numToFetch) {
nodes {
... on PullRequest {
__typename
number
title
body
state
isDraft
url
reviewDecision
comments {
totalCount
}
commits(last: 1) {
nodes {
commit {
statusCheckRollup {
state
}
}
}
}
}
}
}
}

0 comments on commit 986844b

Please sign in to comment.