Skip to content

Commit

Permalink
chore(cli): some errors are not being caught when fetching notices (#…
Browse files Browse the repository at this point in the history
…19112)

This PR handles two additional error scenarios:

1. When there is an error in the _request_. An example is DNS name resolution.
2. When `https.get` itself throws an error instead of sending the error to a registered callback function. This one is unlikely to happen (the only thing I've found that would cause this is failure to parse the URL), but I'm just taking the opportunity to close all the gaps.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
otaviomacedo authored Feb 23, 2022
1 parent 631fc29 commit 4e82d36
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 22 deletions.
53 changes: 31 additions & 22 deletions packages/aws-cdk/lib/notices.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,31 +106,40 @@ export interface NoticeDataSource {
export class WebsiteNoticeDataSource implements NoticeDataSource {
fetch(): Promise<Notice[]> {
return new Promise((resolve) => {
https.get('https://cli.cdk.dev-tools.aws.dev/notices.json', res => {
if (res.statusCode === 200) {
res.setEncoding('utf8');
let rawData = '';
res.on('data', (chunk) => {
rawData += chunk;
});
res.on('end', () => {
try {
const data = JSON.parse(rawData).notices as Notice[];
resolve(data ?? []);
} catch (e) {
debug(`Failed to parse notices: ${e}`);
try {
const req = https.get('https://cli.cdk.dev-tools.aws.dev/notices.json', res => {
if (res.statusCode === 200) {
res.setEncoding('utf8');
let rawData = '';
res.on('data', (chunk) => {
rawData += chunk;
});
res.on('end', () => {
try {
const data = JSON.parse(rawData).notices as Notice[];
resolve(data ?? []);
} catch (e) {
debug(`Failed to parse notices: ${e}`);
resolve([]);
}
});
res.on('error', e => {
debug(`Failed to fetch notices: ${e}`);
resolve([]);
}
});
res.on('error', e => {
debug(`Failed to fetch notices: ${e}`);
});
} else {
debug(`Failed to fetch notices. Status code: ${res.statusCode}`);
resolve([]);
});
} else {
debug(`Failed to fetch notices. Status code: ${res.statusCode}`);
}
});
req.on('error', e => {
debug(`Error on request: ${e}`);
resolve([]);
}
});
});
} catch (e) {
debug(`HTTPS 'get' call threw an error: ${e}`);
resolve([]);
}
});
}
}
Expand Down
22 changes: 22 additions & 0 deletions packages/aws-cdk/test/notices.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as https from 'https';
import * as os from 'os';
import * as path from 'path';
import * as fs from 'fs-extra';
Expand Down Expand Up @@ -211,6 +212,27 @@ describe('cli notices', () => {
expect(result).toEqual([]);
});

test('returns empty array when HTTPS call throws', async () => {
const mockGet = jest.spyOn(https, 'get')
.mockImplementation(() => { throw new Error('No connection'); });

const result = await dataSource.fetch();

expect(result).toEqual([]);

mockGet.mockRestore();
});

test('returns empty array when the request has an error', async () => {
nock('https://cli.cdk.dev-tools.aws.dev')
.get('/notices.json')
.replyWithError('DNS resolution failed');

const result = await dataSource.fetch();

expect(result).toEqual([]);
});

function mockCall(statusCode: number, body: any): Promise<Notice[]> {
nock('https://cli.cdk.dev-tools.aws.dev')
.get('/notices.json')
Expand Down

0 comments on commit 4e82d36

Please sign in to comment.