Skip to content

Commit dcf382d

Browse files
committed
Check structure of octokit errors instead of instanceof
1 parent bad50cd commit dcf382d

File tree

4 files changed

+132
-10
lines changed

4 files changed

+132
-10
lines changed

src/errors/index.ts

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15+
import {GraphqlResponseError} from '@octokit/graphql';
1516
import {RequestError} from '@octokit/request-error';
1617
import {RequestError as RequestErrorBody} from '@octokit/types';
1718

@@ -91,3 +92,56 @@ export class FileNotFoundError extends Error {
9192
this.name = FileNotFoundError.name;
9293
}
9394
}
95+
96+
/**
97+
* Type guard to check if an error is an Octokit RequestError.
98+
*
99+
* This function checks the structure of the error object to determine if it matches
100+
* the shape of a RequestError. It should be favored instead of `instanceof` checks,
101+
* especially in scenarios where the prototype chain might not be reliable, such as when
102+
* dealing with different versions of a package or when the error object might have been
103+
* modified.
104+
*
105+
* @param error The error object to check.
106+
* @returns A boolean indicating whether the error is a RequestError.
107+
*/
108+
export function isOctokitRequestError(error: unknown): error is RequestError {
109+
if (typeof error === 'object' && error !== null) {
110+
const e = error as RequestError;
111+
return (
112+
e.name === 'HttpError' &&
113+
typeof e.status === 'number' &&
114+
typeof e.request === 'object'
115+
);
116+
}
117+
return false;
118+
}
119+
120+
/**
121+
* Type guard to check if an error is an Octokit GraphqlResponseError.
122+
*
123+
* This function checks the structure of the error object to determine if it matches
124+
* the shape of a GraphqlResponseError. It should be favored instead of `instanceof` checks,
125+
* especially in scenarios where the prototype chain might not be reliable, such as when
126+
* dealing with different versions of a package or when the error object might have been
127+
* modified.
128+
*
129+
* @param error The error object to check.
130+
* @returns A boolean indicating whether the error is a GraphqlResponseError.
131+
*/
132+
export function isOctokitGraphqlResponseError(
133+
error: unknown
134+
): error is GraphqlResponseError<unknown> {
135+
if (typeof error === 'object' && error !== null) {
136+
const e = error as GraphqlResponseError<unknown>;
137+
return (
138+
typeof e.request === 'object' &&
139+
typeof e.headers === 'object' &&
140+
typeof e.response === 'object' &&
141+
typeof e.name === 'string' &&
142+
Array.isArray(e.errors) &&
143+
e.data !== undefined
144+
);
145+
}
146+
return false;
147+
}

src/github.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,12 @@ import {Commit} from './commit';
2222
import {Octokit} from '@octokit/rest';
2323
import {request} from '@octokit/request';
2424
import {graphql} from '@octokit/graphql';
25-
import {RequestError} from '@octokit/request-error';
2625
import {
2726
GitHubAPIError,
2827
DuplicateReleaseError,
2928
FileNotFoundError,
3029
ConfigurationError,
30+
isOctokitRequestError,
3131
} from './errors';
3232

3333
const MAX_ISSUE_BODY_SIZE = 65536;
@@ -1590,7 +1590,7 @@ export class GitHub {
15901590
};
15911591
},
15921592
e => {
1593-
if (e instanceof RequestError) {
1593+
if (isOctokitRequestError(e)) {
15941594
if (
15951595
e.status === 422 &&
15961596
GitHubAPIError.parseErrors(e).some(error => {
@@ -1754,7 +1754,7 @@ export class GitHub {
17541754
this.logger.debug(`SHA for branch: ${sha}`);
17551755
return sha;
17561756
} catch (e) {
1757-
if (e instanceof RequestError && e.status === 404) {
1757+
if (isOctokitRequestError(e) && e.status === 404) {
17581758
this.logger.debug(`Branch: ${branchName} does not exist`);
17591759
return undefined;
17601760
}
@@ -2079,7 +2079,7 @@ const wrapAsync = <T extends Array<any>, V>(
20792079
if (errorHandler) {
20802080
errorHandler(e as GitHubAPIError);
20812081
}
2082-
if (e instanceof RequestError) {
2082+
if (isOctokitRequestError(e)) {
20832083
throw new GitHubAPIError(e);
20842084
}
20852085
throw e;

src/manifest.ts

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@ import {
3939
DuplicateReleaseError,
4040
FileNotFoundError,
4141
ConfigurationError,
42+
isOctokitRequestError,
43+
isOctokitGraphqlResponseError,
4244
} from './errors';
4345
import {ManifestPlugin} from './plugin';
4446
import {
@@ -47,8 +49,6 @@ import {
4749
} from './util/pull-request-overflow-handler';
4850
import {signoffCommitMessage} from './util/signoff-commit-message';
4951
import {CommitExclude} from './util/commit-exclude';
50-
import {RequestError} from '@octokit/request-error';
51-
import {GraphqlResponseError} from '@octokit/graphql';
5252

5353
type ExtraJsonFile = {
5454
type: 'json';
@@ -1185,10 +1185,9 @@ export class Manifest {
11851185
//
11861186
// Error mentioned here: https://docs.github.com/en/code-security/code-scanning/troubleshooting-code-scanning/resource-not-accessible-by-integration
11871187
if (
1188-
err &&
1189-
((err instanceof RequestError && err.status === 403) ||
1190-
(err instanceof GraphqlResponseError &&
1191-
err.errors?.find(e => e.type === 'FORBIDDEN')))
1188+
(isOctokitRequestError(err) && err.status === 403) ||
1189+
(isOctokitGraphqlResponseError(err) &&
1190+
err.errors?.find(e => e.type === 'FORBIDDEN'))
11921191
) {
11931192
await this.throwIfChangesBranchesRaceConditionDetected(pullRequests);
11941193
createdReleases = await runReleaseProcess();

test/errors/index.ts

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
import {expect} from 'chai';
2+
import {GraphqlResponseError} from '@octokit/graphql';
3+
import {
4+
isOctokitGraphqlResponseError,
5+
isOctokitRequestError,
6+
} from '../../src/errors';
7+
import {RequestError} from '@octokit/request-error';
8+
9+
describe('Errors', () => {
10+
describe('isOctokitRequestError', () => {
11+
it('should return true for valid RequestError', () => {
12+
const error = new RequestError('Not Found', 404, {
13+
request: {method: 'GET', url: '/foo/bar', headers: {}},
14+
headers: {},
15+
});
16+
expect(isOctokitRequestError(error)).to.be.true;
17+
});
18+
19+
it('should return false for invalid RequestError', () => {
20+
const error = {
21+
name: 'SomeOtherError',
22+
status: 500,
23+
request: 'invalid_request_object',
24+
};
25+
expect(isOctokitRequestError(error)).to.be.false;
26+
});
27+
});
28+
29+
describe('isOctokitGraphqlResponseError', () => {
30+
it('should return true for valid GraphqlResponseError', () => {
31+
const error = new GraphqlResponseError(
32+
{
33+
method: 'GET',
34+
url: '/foo/bar',
35+
},
36+
{},
37+
{
38+
data: {},
39+
errors: [
40+
{
41+
type: 'FORBIDDEN',
42+
message: 'Resource not accessible by integration',
43+
path: ['foo'],
44+
extensions: {},
45+
locations: [
46+
{
47+
line: 123,
48+
column: 456,
49+
},
50+
],
51+
},
52+
],
53+
}
54+
);
55+
expect(isOctokitGraphqlResponseError(error)).to.be.true;
56+
});
57+
58+
it('should return false for invalid GraphqlResponseError', () => {
59+
const error = {
60+
request: {},
61+
headers: {},
62+
response: {},
63+
name: 'SomeOtherError',
64+
errors: [],
65+
};
66+
expect(isOctokitGraphqlResponseError(error)).to.be.false;
67+
});
68+
});
69+
});

0 commit comments

Comments
 (0)