From aedf76889bbddf31f80d5223b14f2d2be652cc4a Mon Sep 17 00:00:00 2001 From: sethvincent Date: Mon, 26 Mar 2018 14:53:41 -0400 Subject: [PATCH 1/5] aws.downloadS3File: use pump to catch errors in both streams, add tests --- packages/common/aws.js | 23 ++++++++++++++--------- packages/common/tests/aws.js | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 9 deletions(-) diff --git a/packages/common/aws.js b/packages/common/aws.js index 08ef45fcd2f..71a955b92d8 100644 --- a/packages/common/aws.js +++ b/packages/common/aws.js @@ -10,6 +10,7 @@ const log = require('./log'); const string = require('./string'); const { inTestMode, randomString, testAwsClient } = require('./test-utils'); const promiseRetry = require('promise-retry'); +const pump = require('pump'); const region = exports.region = process.env.AWS_DEFAULT_REGION || 'us-east-1'; if (region) { @@ -151,18 +152,22 @@ exports.promiseS3Upload = (params) => { /** * Downloads the given s3Obj to the given filename in a streaming manner - * @param s3Obj The parameters to send to S3 getObject call - * @param filename The output filename + * + * @param {Object} s3Obj - The parameters to send to S3 getObject call + * @param {string} filepath - The filepath of the file that is downloaded + * @returns {Promise} - returns filename if successful */ -exports.downloadS3File = (s3Obj, filename) => { +exports.downloadS3File = (s3Obj, filepath) => { const s3 = exports.s3(); - const file = fs.createWriteStream(filename); + const fileWriteStream = fs.createWriteStream(filepath); + return new Promise((resolve, reject) => { - s3.getObject(s3Obj) - .createReadStream() - .pipe(file) - .on('finish', () => resolve(filename)) - .on('error', reject); + const objectReadStream = s3.getObject(s3Obj).createReadStream(); + + pump(objectReadStream, fileWriteStream, (err) => { + if (err) reject(err); + else resolve(filepath); + }); }); }; diff --git a/packages/common/tests/aws.js b/packages/common/tests/aws.js index 6bdee2a6838..98eb01fe6f4 100644 --- a/packages/common/tests/aws.js +++ b/packages/common/tests/aws.js @@ -1,5 +1,8 @@ 'use strict'; +const fs = require('fs'); +const path = require('path'); +const { tmpdir } = require('os'); const test = require('ava'); const aws = require('../aws'); const { randomString } = require('../test-utils'); @@ -43,3 +46,36 @@ test('listS3ObjectsV2 handles truncated case', async (t) => { return aws.recursivelyDeleteS3Bucket(Bucket); }); + +test('downloadS3File rejects promise if key not found', async (t) => { + const Bucket = randomString(); + await aws.s3().createBucket({ Bucket }).promise(); + + try { + await aws.downloadS3File({ Bucket, Key: 'not-gonna-find-it' }, '/tmp/wut'); + } + catch (err) { + t.is(err.message, 'The specified key does not exist.'); + } +}); + +test('downloadS3File resolves filepath if key is found', async (t) => { + const Bucket = randomString(); + const Key = 'example'; + const Body = 'example'; + + await aws.s3().createBucket({ Bucket }).promise(); + await aws.s3().putObject({ Bucket, Key: Key, Body: Body }).promise(); + + const params = { Bucket, Key: Key }; + const filepath = await aws.downloadS3File(params, path.join(tmpdir(), 'example')); + + const result = await new Promise((resolve, reject) => { + fs.readFile(filepath, 'utf-8', (err, data) => { + if (err) reject(err); + else resolve(data); + }); + }); + + t.is(result, Body); +}); From d42ec577285f50ee51862c68d90ae826e6d200f1 Mon Sep 17 00:00:00 2001 From: sethvincent Date: Mon, 26 Mar 2018 14:58:50 -0400 Subject: [PATCH 2/5] eslint-ratchet update --- .eslint-ratchet-high-water-mark | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.eslint-ratchet-high-water-mark b/.eslint-ratchet-high-water-mark index a4ea6e7a770..e8c19528942 100644 --- a/.eslint-ratchet-high-water-mark +++ b/.eslint-ratchet-high-water-mark @@ -1 +1 @@ -2059 +1530 From 23888b592efb1d932616a17211d009cdd241cbed Mon Sep 17 00:00:00 2001 From: sethvincent Date: Mon, 26 Mar 2018 15:02:45 -0400 Subject: [PATCH 3/5] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 393a813295c..2351eb624db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [Unreleased] ### Fixed +- **CUMULUS-331:** Fix aws.downloadS3File to handle non-existent key - Using test ftp provider for discover-granules testing [CUMULUS-427] - **CUMULUS-304: "Add AWS API throttling to pdr-status-check task"** Added concurrency limit on SFN API calls. The default concurrency is 10 and is configurable through Lambda environment variable CONCURRENCY. - **CUMULUS-271: "Empty response body from rules PUT endpoint"** Added the updated rule to response body. From 345488dff8b831c838e66ae71e5198040ca0341e Mon Sep 17 00:00:00 2001 From: sethvincent Date: Mon, 26 Mar 2018 15:57:56 -0400 Subject: [PATCH 4/5] fix eslint-ratchet --- .eslint-ratchet-high-water-mark | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.eslint-ratchet-high-water-mark b/.eslint-ratchet-high-water-mark index e8c19528942..9adbd23923b 100644 --- a/.eslint-ratchet-high-water-mark +++ b/.eslint-ratchet-high-water-mark @@ -1 +1 @@ -1530 +2042 From 96fda69eb71155c09f79cced38e6078704991ed8 Mon Sep 17 00:00:00 2001 From: sethvincent Date: Mon, 26 Mar 2018 16:14:12 -0400 Subject: [PATCH 5/5] fix eslint-ratchet --- .eslint-ratchet-high-water-mark | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.eslint-ratchet-high-water-mark b/.eslint-ratchet-high-water-mark index 9adbd23923b..65bb5e1ec8c 100644 --- a/.eslint-ratchet-high-water-mark +++ b/.eslint-ratchet-high-water-mark @@ -1 +1 @@ -2042 +2051