From 054c585f29b21d95642b0ae30a9a15eb914efc82 Mon Sep 17 00:00:00 2001 From: Adam Date: Thu, 22 Aug 2019 12:54:20 -0700 Subject: [PATCH 1/6] functions/imagemagick: robustness and idiomatic improvements --- functions/imagemagick/index.js | 13 ++-- functions/imagemagick/test/index.test.js | 88 +++++++++++++++--------- 2 files changed, 63 insertions(+), 38 deletions(-) diff --git a/functions/imagemagick/index.js b/functions/imagemagick/index.js index 2eb8c732ac..1721a549a0 100644 --- a/functions/imagemagick/index.js +++ b/functions/imagemagick/index.js @@ -21,9 +21,9 @@ const fs = require('fs'); const {promisify} = require('util'); const path = require('path'); const {Storage} = require('@google-cloud/storage'); -const storage = new Storage(); -const vision = require('@google-cloud/vision').v1p1beta1; +const vision = require('@google-cloud/vision'); +const storage = new Storage(); const client = new vision.ImageAnnotatorClient(); const {BLURRED_BUCKET_NAME} = process.env; @@ -45,6 +45,7 @@ exports.blurOffensiveImages = async event => { const detections = result.safeSearchAnnotation || {}; if ( + // Levels are defined in https://cloud.google.com/vision/docs/reference/rpc/google.cloud.vision.v1#google.cloud.vision.v1.Likelihood detections.adult === 'VERY_LIKELY' || detections.violence === 'VERY_LIKELY' ) { @@ -55,7 +56,7 @@ exports.blurOffensiveImages = async event => { } } catch (err) { console.error(`Failed to analyze ${file.name}.`, err); - return Promise.reject(err); + throw err; } }; // [END functions_imagemagick_analyze] @@ -71,8 +72,7 @@ const blurImage = async (file, blurredBucketName) => { console.log(`Downloaded ${file.name} to ${tempLocalPath}.`); } catch (err) { - console.error('File download failed.', err); - return Promise.reject(err); + throw new Error(`File download failed: ${err}`); } await new Promise((resolve, reject) => { @@ -98,8 +98,7 @@ const blurImage = async (file, blurredBucketName) => { await blurredBucket.upload(tempLocalPath, {destination: file.name}); console.log(`Uploaded blurred image to: ${gcsPath}`); } catch (err) { - console.error(`Unable to upload blurred image to ${gcsPath}:`, err); - return Promise.reject(err); + throw new Error(`Unable to upload blurred image to ${gcsPath}: ${err}`); } // Delete the temporary file. diff --git a/functions/imagemagick/test/index.test.js b/functions/imagemagick/test/index.test.js index b4c93a741e..d7f8d9d98c 100644 --- a/functions/imagemagick/test/index.test.js +++ b/functions/imagemagick/test/index.test.js @@ -31,40 +31,64 @@ requestRetry = requestRetry.defaults({ retryDelay: 1000, }); -const BUCKET_NAME = process.env.FUNCTIONS_BUCKET; -const {BLURRED_BUCKET_NAME} = process.env; const safeFileName = 'bicycle.jpg'; const offensiveFileName = 'zombie.jpg'; - +const BUCKET_NAME = process.env.FUNCTIONS_BUCKET; +const {BLURRED_BUCKET_NAME} = process.env; +const blurredBucket = storage.bucket(BLURRED_BUCKET_NAME); const cwd = path.join(__dirname, '..'); -const blurredBucket = storage.bucket(BLURRED_BUCKET_NAME); +// Successfully generated images require cleanup. +let cleanupRequired = false; describe('functions/imagemagick tests', () => { - const startFF = port => { - return execPromise( - `functions-framework --target=blurOffensiveImages --signature-type=event --port=${port}`, - {timeout: 15000, shell: true, cwd} - ); - }; - - const stopFF = async ffProc => { - try { - return await ffProc; - } catch (err) { - // Timeouts always cause errors on Linux, so catch them - if (err.name && err.name === 'ChildProcessError') { - const {stdout, stderr} = err; - return {stdout, stderr}; + let startFF, stopFF; + + before(() => { + startFF = port => { + return execPromise( + `functions-framework --target=blurOffensiveImages --signature-type=event --port=${port}`, + {timeout: 15000, shell: true, cwd} + ); + }; + + stopFF = async ffProc => { + try { + return await ffProc; + } catch (err) { + // Timeouts always cause errors on Linux, so catch them + if (err.name && err.name === 'ChildProcessError') { + const {stdout, stderr} = err; + return {stdout, stderr}; + } + + throw err; } + }; + }); + + before(async () => { + let exists = await storage + .bucket(BUCKET_NAME) + .file(offensiveFileName) + .exists(); + if (!exists[0]) { + throw Error(`Missing required file: gs://${BUCKET_NAME}/${offensiveFileName}`); + } - throw err; + exists = await storage + .bucket(BUCKET_NAME) + .file(safeFileName) + .exists(); + if (!exists[0]) { + throw Error(`Missing required file: gs://${BUCKET_NAME}/${safeFileName}`); } - }; + }); + - beforeEach(tools.stubConsole); - afterEach(tools.restoreConsole); +// beforeEach(tools.stubConsole); +// afterEach(tools.restoreConsole); it('blurOffensiveImages detects safe images using Cloud Vision', async () => { const PORT = 8080; @@ -81,7 +105,6 @@ describe('functions/imagemagick tests', () => { }); const {stdout} = await stopFF(ffProc); - assert.ok(stdout.includes(`Detected ${safeFileName} as OK.`)); }); @@ -108,16 +131,19 @@ describe('functions/imagemagick tests', () => { ) ); - assert.ok( - storage - .bucket(BLURRED_BUCKET_NAME) - .file(offensiveFileName) - .exists(), - 'File uploaded' - ); + const exists = storage + .bucket(BLURRED_BUCKET_NAME) + .file(offensiveFileName) + .exists() + + assert.ok(exists, 'File uploaded'); + cleanupRequired |= exists; }); after(async () => { + if (!cleanupRequired) { + return; + } try { await blurredBucket.file(offensiveFileName).delete(); } catch (err) { From aa5551c7bb55eec3cbff9df165daae4f8d850bf0 Mon Sep 17 00:00:00 2001 From: Adam Date: Thu, 22 Aug 2019 14:38:15 -0700 Subject: [PATCH 2/6] functions/imagemagick: lint fixes --- functions/imagemagick/test/index.test.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/functions/imagemagick/test/index.test.js b/functions/imagemagick/test/index.test.js index d7f8d9d98c..71d5a8edb9 100644 --- a/functions/imagemagick/test/index.test.js +++ b/functions/imagemagick/test/index.test.js @@ -31,7 +31,6 @@ requestRetry = requestRetry.defaults({ retryDelay: 1000, }); - const safeFileName = 'bicycle.jpg'; const offensiveFileName = 'zombie.jpg'; const BUCKET_NAME = process.env.FUNCTIONS_BUCKET; @@ -74,7 +73,9 @@ describe('functions/imagemagick tests', () => { .file(offensiveFileName) .exists(); if (!exists[0]) { - throw Error(`Missing required file: gs://${BUCKET_NAME}/${offensiveFileName}`); + throw Error( + `Missing required file: gs://${BUCKET_NAME}/${offensiveFileName}` + ); } exists = await storage @@ -86,9 +87,8 @@ describe('functions/imagemagick tests', () => { } }); - -// beforeEach(tools.stubConsole); -// afterEach(tools.restoreConsole); + beforeEach(tools.stubConsole); + afterEach(tools.restoreConsole); it('blurOffensiveImages detects safe images using Cloud Vision', async () => { const PORT = 8080; @@ -134,7 +134,7 @@ describe('functions/imagemagick tests', () => { const exists = storage .bucket(BLURRED_BUCKET_NAME) .file(offensiveFileName) - .exists() + .exists(); assert.ok(exists, 'File uploaded'); cleanupRequired |= exists; From 6f57503fab90235c503f555375c11fcd74715dfe Mon Sep 17 00:00:00 2001 From: Adam Date: Mon, 26 Aug 2019 08:53:46 -0700 Subject: [PATCH 3/6] functions/imagemagick: reflow require statements to group storage --- functions/imagemagick/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/functions/imagemagick/index.js b/functions/imagemagick/index.js index 1721a549a0..0bfad113be 100644 --- a/functions/imagemagick/index.js +++ b/functions/imagemagick/index.js @@ -20,9 +20,9 @@ const gm = require('gm').subClass({imageMagick: true}); const fs = require('fs'); const {promisify} = require('util'); const path = require('path'); -const {Storage} = require('@google-cloud/storage'); const vision = require('@google-cloud/vision'); +const {Storage} = require('@google-cloud/storage'); const storage = new Storage(); const client = new vision.ImageAnnotatorClient(); From 8bb8a015d8643ccc0b7954f57c1ac6eebb675612 Mon Sep 17 00:00:00 2001 From: Adam Date: Mon, 26 Aug 2019 08:55:18 -0700 Subject: [PATCH 4/6] functions/imagemagick: nodejs style and comment feedback --- functions/imagemagick/index.js | 2 +- functions/imagemagick/test/index.test.js | 13 ++++++------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/functions/imagemagick/index.js b/functions/imagemagick/index.js index 0bfad113be..838c27ced8 100644 --- a/functions/imagemagick/index.js +++ b/functions/imagemagick/index.js @@ -45,7 +45,7 @@ exports.blurOffensiveImages = async event => { const detections = result.safeSearchAnnotation || {}; if ( - // Levels are defined in https://cloud.google.com/vision/docs/reference/rpc/google.cloud.vision.v1#google.cloud.vision.v1.Likelihood + // Levels are defined in https://cloud.google.com/vision/docs/reference/rest/v1/AnnotateImageResponse#likelihood detections.adult === 'VERY_LIKELY' || detections.violence === 'VERY_LIKELY' ) { diff --git a/functions/imagemagick/test/index.test.js b/functions/imagemagick/test/index.test.js index 71d5a8edb9..ebfbbe5d51 100644 --- a/functions/imagemagick/test/index.test.js +++ b/functions/imagemagick/test/index.test.js @@ -38,11 +38,10 @@ const {BLURRED_BUCKET_NAME} = process.env; const blurredBucket = storage.bucket(BLURRED_BUCKET_NAME); const cwd = path.join(__dirname, '..'); -// Successfully generated images require cleanup. -let cleanupRequired = false; - describe('functions/imagemagick tests', () => { let startFF, stopFF; + // Successfully generated images require cleanup. + let cleanupRequired = false; before(() => { startFF = port => { @@ -68,21 +67,21 @@ describe('functions/imagemagick tests', () => { }); before(async () => { - let exists = await storage + let [exists] = await storage .bucket(BUCKET_NAME) .file(offensiveFileName) .exists(); - if (!exists[0]) { + if (!exists) { throw Error( `Missing required file: gs://${BUCKET_NAME}/${offensiveFileName}` ); } - exists = await storage + [exists] = await storage .bucket(BUCKET_NAME) .file(safeFileName) .exists(); - if (!exists[0]) { + if (!exists) { throw Error(`Missing required file: gs://${BUCKET_NAME}/${safeFileName}`); } }); From 1011648b2a8d644ec8b785748a2492bab12f39eb Mon Sep 17 00:00:00 2001 From: Adam Ross Date: Mon, 26 Aug 2019 09:58:39 -0700 Subject: [PATCH 5/6] functions/imagemagick: improve test coverage, add missing file test --- functions/imagemagick/test/index.test.js | 81 +++++++++++++++--------- 1 file changed, 50 insertions(+), 31 deletions(-) diff --git a/functions/imagemagick/test/index.test.js b/functions/imagemagick/test/index.test.js index ebfbbe5d51..d85943ee38 100644 --- a/functions/imagemagick/test/index.test.js +++ b/functions/imagemagick/test/index.test.js @@ -31,18 +31,38 @@ requestRetry = requestRetry.defaults({ retryDelay: 1000, }); -const safeFileName = 'bicycle.jpg'; -const offensiveFileName = 'zombie.jpg'; const BUCKET_NAME = process.env.FUNCTIONS_BUCKET; const {BLURRED_BUCKET_NAME} = process.env; const blurredBucket = storage.bucket(BLURRED_BUCKET_NAME); const cwd = path.join(__dirname, '..'); -describe('functions/imagemagick tests', () => { +const testFiles = { + safe: 'bicycle.jpg', + offensive: 'zombie.jpg', +}; + +describe('functions/imagemagick tests', async () => { let startFF, stopFF; // Successfully generated images require cleanup. let cleanupRequired = false; + before(async () => { + let exists; + + const names = Object.keys(testFiles); + for (let i = 0; i < names.length; i++) { + [exists] = await storage + .bucket(BUCKET_NAME) + .file(testFiles[names[i]]) + .exists(); + if (!exists) { + throw Error( + `Missing required file: gs://${BUCKET_NAME}/${testFiles[names[i]]}` + ); + } + } + }); + before(() => { startFF = port => { return execPromise( @@ -66,26 +86,6 @@ describe('functions/imagemagick tests', () => { }; }); - before(async () => { - let [exists] = await storage - .bucket(BUCKET_NAME) - .file(offensiveFileName) - .exists(); - if (!exists) { - throw Error( - `Missing required file: gs://${BUCKET_NAME}/${offensiveFileName}` - ); - } - - [exists] = await storage - .bucket(BUCKET_NAME) - .file(safeFileName) - .exists(); - if (!exists) { - throw Error(`Missing required file: gs://${BUCKET_NAME}/${safeFileName}`); - } - }); - beforeEach(tools.stubConsole); afterEach(tools.restoreConsole); @@ -98,13 +98,13 @@ describe('functions/imagemagick tests', () => { body: { data: { bucket: BUCKET_NAME, - name: safeFileName, + name: testFiles.safe, }, }, }); const {stdout} = await stopFF(ffProc); - assert.ok(stdout.includes(`Detected ${safeFileName} as OK.`)); + assert.ok(stdout.includes(`Detected ${testFiles.safe} as OK.`)); }); it('blurOffensiveImages successfully blurs offensive images', async () => { @@ -116,35 +116,54 @@ describe('functions/imagemagick tests', () => { body: { data: { bucket: BUCKET_NAME, - name: offensiveFileName, + name: testFiles.offensive, }, }, }); const {stdout} = await stopFF(ffProc); - assert.ok(stdout.includes(`Blurred image: ${offensiveFileName}`)); + assert.ok(stdout.includes(`Blurred image: ${testFiles.offensive}`)); assert.ok( stdout.includes( - `Uploaded blurred image to: gs://${BLURRED_BUCKET_NAME}/${offensiveFileName}` + `Uploaded blurred image to: gs://${BLURRED_BUCKET_NAME}/${testFiles.offensive}` ) ); - const exists = storage + const exists = await storage .bucket(BLURRED_BUCKET_NAME) - .file(offensiveFileName) + .file(testFiles.offensive) .exists(); assert.ok(exists, 'File uploaded'); cleanupRequired |= exists; }); + it('blurOffensiveImages detects missing images as safe using Cloud Vision', async () => { + const PORT = 8082; + const ffProc = startFF(PORT); + const missingFileName = 'file-does-not-exist.jpg'; + + await requestRetry({ + url: `http://localhost:${PORT}/blurOffensiveImages`, + body: { + data: { + bucket: BUCKET_NAME, + name: missingFileName, + }, + }, + }); + + const {stdout} = await stopFF(ffProc); + assert.ok(stdout.includes(`Detected ${missingFileName} as OK.`)); + }); + after(async () => { if (!cleanupRequired) { return; } try { - await blurredBucket.file(offensiveFileName).delete(); + await blurredBucket.file(testFiles.offensive).delete(); } catch (err) { console.log('Error deleting uploaded file:', err); } From a08a2dd935f6498c9bc778955f79e4ee00f6677e Mon Sep 17 00:00:00 2001 From: Adam Ross Date: Tue, 27 Aug 2019 15:41:23 -0700 Subject: [PATCH 6/6] functions/imagemagick: allow cleanup to fail gracefully --- functions/imagemagick/test/index.test.js | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/functions/imagemagick/test/index.test.js b/functions/imagemagick/test/index.test.js index d85943ee38..f960245a65 100644 --- a/functions/imagemagick/test/index.test.js +++ b/functions/imagemagick/test/index.test.js @@ -43,8 +43,6 @@ const testFiles = { describe('functions/imagemagick tests', async () => { let startFF, stopFF; - // Successfully generated images require cleanup. - let cleanupRequired = false; before(async () => { let exists; @@ -134,9 +132,7 @@ describe('functions/imagemagick tests', async () => { .bucket(BLURRED_BUCKET_NAME) .file(testFiles.offensive) .exists(); - assert.ok(exists, 'File uploaded'); - cleanupRequired |= exists; }); it('blurOffensiveImages detects missing images as safe using Cloud Vision', async () => { @@ -159,13 +155,10 @@ describe('functions/imagemagick tests', async () => { }); after(async () => { - if (!cleanupRequired) { - return; - } try { await blurredBucket.file(testFiles.offensive).delete(); } catch (err) { - console.log('Error deleting uploaded file:', err); + console.log('Error deleting uploaded file:', err.message); } }); });