Skip to content

Commit

Permalink
fix(download): wait for response end
Browse files Browse the repository at this point in the history 
The `DropboxService` was not handling calls to `https.get` appropriately
when downloading files.  The code was resolving the promise after
receiving a `close` event from the request.  This could result in the
promise resolving before the file had finished downloading. In addition,
the code was not verifying the status code of the response was a
successful code.

Changes:
* Resolve the promise when the request emits an `end` event and not when
  the response emits a `close` event.
* If the response status code is not a successful code then reject the
  promise.
  • Loading branch information
@klieber
klieber committed Nov 19, 2020
1 parent baec14d commit 98a016a
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 20 deletions.
13 changes: 10 additions & 3 deletions lib/provider/dropbox/dropbox-service.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,16 @@ class DropboxService {
await fs.promises.mkdir(target, { recursive: true });

await new Promise((resolve, reject) => {
const req = https.get(link, (res) => res.pipe(fs.createWriteStream(`${target}/${metadata.name}`)));
req.on('close', resolve);
req.on('error', reject);
https
.get(link, (res) => {
if (res.statusCode < 200 || res.statusCode >= 300) {
reject(new Error(`Unable to download file '${path}': ${res.statusCode} ${res.statusMessage}`));
} else {
res.pipe(fs.createWriteStream(`${target}/${metadata.name}`));
res.on('end', resolve);
}
})
.on('error', reject);
});

const fileDate = new Date(metadata.client_modified);
Expand Down
108 changes: 91 additions & 17 deletions lib/provider/dropbox/dropbox-service.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,20 @@ afterEach(() => {
const createReadStream = (data) =>
new Stream.Readable({
read() {
this.push(data);
if (data) {
this.push(data);
}
this.push(null);
}
});

const createHttpResponse = (code, message, data) => {
const response = createReadStream(data);
response.statusCode = code;
response.statusMessage = message;
return response;
};

describe('DropboxService', () => {
describe('download', () => {
test('file is downloaded successfully', async () => {
Expand All @@ -74,7 +83,9 @@ describe('DropboxService', () => {

describe('downloadLargeFile', () => {
test('file is downloaded successfully', async () => {
const writableStream = new Stream.Writable();
const writableStream = new Stream.Writable({
write() {}
});
jest.spyOn(writableStream, 'write');

when(dropboxClient.filesGetTemporaryLink)
Expand All @@ -89,7 +100,7 @@ describe('DropboxService', () => {

const httpEmitter = new EventEmitter();
https.get.mockImplementation((link, callback) => {
callback(createReadStream(DROPBOX_FILE_BINARY));
callback(createHttpResponse(200, 'OK', DROPBOX_FILE_BINARY));
return httpEmitter;
});

Expand All @@ -98,20 +109,89 @@ describe('DropboxService', () => {

when(fs.createWriteStream).calledWith(expectedFilename).mockReturnValue(writableStream);

setTimeout(() => httpEmitter.emit('close'));
const filename = await dropboxService.downloadLargeFile(`${SOURCE_PATH}/${DROPBOX_FILE_NAME}`, TARGET_PATH);

expect(filename).toBe(expectedFilename);
await expect(dropboxService.downloadLargeFile(`${SOURCE_PATH}/${DROPBOX_FILE_NAME}`, TARGET_PATH)).resolves.toBe(
expectedFilename
);
expect(https.get).toHaveBeenCalledWith(FAKE_DOWNLOAD_LINK, expect.any(Function));
expect(fs.promises.mkdir).toHaveBeenCalledWith(TARGET_PATH, { recursive: true });
expect(writableStream.write).toHaveBeenCalledWith(Buffer.from(DROPBOX_FILE_BINARY));
expect(fs.promises.utimes).toHaveBeenCalledWith(expectedFilename, fileDate, fileDate);
});

test('download fails: http error status', async () => {
const writableStream = new Stream.Writable({
write() {}
});
jest.spyOn(writableStream, 'write');

when(dropboxClient.filesGetTemporaryLink)
.calledWith({ path: `${SOURCE_PATH}/${DROPBOX_FILE_NAME}` })
.mockResolvedValue({
link: FAKE_DOWNLOAD_LINK,
metadata: {
name: DROPBOX_FILE_NAME,
client_modified: MODIFY_DATE_STRING
}
});

https.get.mockImplementation((link, callback) => {
callback(createHttpResponse(500, 'Internal Server Error'));
return new EventEmitter();
});

const expectedFilename = `${TARGET_PATH}/${DROPBOX_FILE_NAME}`;

when(fs.createWriteStream).calledWith(expectedFilename).mockReturnValue(writableStream);

await expect(
dropboxService.downloadLargeFile(`${SOURCE_PATH}/${DROPBOX_FILE_NAME}`, TARGET_PATH)
).rejects.toThrowError("Unable to download file './source-path/mockFile.jpg': 500 Internal Server Error");
expect(https.get).toHaveBeenCalledWith(FAKE_DOWNLOAD_LINK, expect.any(Function));
expect(fs.promises.mkdir).toHaveBeenCalledWith(TARGET_PATH, { recursive: true });
expect(writableStream.write).not.toHaveBeenCalled();
expect(fs.promises.utimes).not.toHaveBeenCalled();
});

test('download fails: request error', async () => {
const writableStream = new Stream.Writable({
write() {}
});
jest.spyOn(writableStream, 'write');

when(dropboxClient.filesGetTemporaryLink)
.calledWith({ path: `${SOURCE_PATH}/${DROPBOX_FILE_NAME}` })
.mockResolvedValue({
link: FAKE_DOWNLOAD_LINK,
metadata: {
name: DROPBOX_FILE_NAME,
client_modified: MODIFY_DATE_STRING
}
});

const httpEmitter = new EventEmitter();
https.get.mockImplementation(() => httpEmitter);

const expectedFilename = `${TARGET_PATH}/${DROPBOX_FILE_NAME}`;

when(fs.createWriteStream).calledWith(expectedFilename).mockReturnValue(writableStream);

setTimeout(() => httpEmitter.emit('error', new Error('Unexpected request error.')));

await expect(
dropboxService.downloadLargeFile(`${SOURCE_PATH}/${DROPBOX_FILE_NAME}`, TARGET_PATH)
).rejects.toThrowError('Unexpected request error.');
expect(https.get).toHaveBeenCalledWith(FAKE_DOWNLOAD_LINK, expect.any(Function));
expect(fs.promises.mkdir).toHaveBeenCalledWith(TARGET_PATH, { recursive: true });
expect(writableStream.write).not.toHaveBeenCalled();
expect(fs.promises.utimes).not.toHaveBeenCalled();
});
});

describe('downloadAndVerify', () => {
test('file is downloaded and verified successfully', async () => {
const writableStream = new Stream.Writable();
const writableStream = new Stream.Writable({
write() {}
});
jest.spyOn(writableStream, 'write');

when(dropboxClient.filesGetTemporaryLink)
Expand All @@ -124,23 +204,17 @@ describe('DropboxService', () => {
}
});

const httpEmitter = new EventEmitter();
https.get.mockImplementation((link, callback) => {
callback(createReadStream(DROPBOX_FILE_BINARY));
return httpEmitter;
callback(createHttpResponse(200, 'OK', DROPBOX_FILE_BINARY));
return new EventEmitter();
});

const expectedFilename = `${TARGET_PATH}/${DROPBOX_FILE_NAME}`;

when(fs.createWriteStream).calledWith(expectedFilename).mockReturnValue(writableStream);

setTimeout(() => httpEmitter.emit('close'));

when(fs.createReadStream).calledWith(expectedFilename).mockReturnValue(createReadStream(DROPBOX_FILE_BINARY));

const filename = await dropboxService.downloadAndVerify(DROPBOX_FILE, TARGET_PATH);

expect(filename).toBe(expectedFilename);
await expect(dropboxService.downloadAndVerify(DROPBOX_FILE, TARGET_PATH)).resolves.toBe(expectedFilename);
expect(https.get).toHaveBeenCalledWith(FAKE_DOWNLOAD_LINK, expect.any(Function));
expect(fs.promises.mkdir).toHaveBeenCalledWith(TARGET_PATH, { recursive: true });
expect(writableStream.write).toHaveBeenCalledWith(Buffer.from(DROPBOX_FILE_BINARY));
Expand Down

0 comments on commit 98a016a

Please sign in to comment.