Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Functions: ImageMagick tutorial: Idiomatic & Robustness Improvements #1456

Merged
merged 7 commits into from
Aug 28, 2019
13 changes: 6 additions & 7 deletions functions/imagemagick/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ const gm = require('gm').subClass({imageMagick: true});
const fs = require('fs');
const {promisify} = require('util');
const path = require('path');
const vision = require('@google-cloud/vision');

const {Storage} = require('@google-cloud/storage');
const storage = new Storage();
grayside marked this conversation as resolved.
Show resolved Hide resolved
const vision = require('@google-cloud/vision').v1p1beta1;

const client = new vision.ImageAnnotatorClient();

const {BLURRED_BUCKET_NAME} = process.env;
Expand All @@ -45,6 +45,7 @@ exports.blurOffensiveImages = async event => {
const detections = result.safeSearchAnnotation || {};

if (
// Levels are defined in https://cloud.google.com/vision/docs/reference/rest/v1/AnnotateImageResponse#likelihood
detections.adult === 'VERY_LIKELY' ||
detections.violence === 'VERY_LIKELY'
) {
Expand All @@ -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]
Expand All @@ -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}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this will stringify correctly? Do we not mean to provide the ${err.message}?

}

await new Promise((resolve, reject) => {
Expand All @@ -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}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about err.message.

}

// Delete the temporary file.
Expand Down
122 changes: 83 additions & 39 deletions functions/imagemagick/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,35 +33,58 @@ requestRetry = requestRetry.defaults({

const BUCKET_NAME = process.env.FUNCTIONS_BUCKET;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It would be nice if we just used FUNCTIONS_BUCKET so that we could destruct process.env in the same way:

  • FUNCTIONS_BUCKET
  • BLURRED_FUNCTIONS_BUCKET

Copy link
Collaborator Author

@grayside grayside Aug 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that it's a bit confusing. From what I can see, BUCKET_NAME is used in the same because it makes sense as simply the bucket causing the function trigger. FUNCTIONS_BUCKET is defined in .kokoro/build.sh and has that name so the product testing config is disambiguated.

const {BLURRED_BUCKET_NAME} = process.env;

const safeFileName = 'bicycle.jpg';
const offensiveFileName = 'zombie.jpg';

const cwd = path.join(__dirname, '..');

const blurredBucket = storage.bucket(BLURRED_BUCKET_NAME);
const cwd = path.join(__dirname, '..');

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};
const testFiles = {
safe: 'bicycle.jpg',
offensive: 'zombie.jpg',
};

describe('functions/imagemagick tests', async () => {
let startFF, stopFF;
// Successfully generated images require cleanup.
let cleanupRequired = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just attempt to delete the file, and swallow the error if (and only if) it doesn't exist?

(IIRC that's more in-line with what our other samples do.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bullet-proofed what I found instead of removing it. I can take this approach.


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]]}`
);
}

throw err;
}
};
});

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;
}
};
});

beforeEach(tools.stubConsole);
afterEach(tools.restoreConsole);
Expand All @@ -75,14 +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 () => {
Expand All @@ -94,32 +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}`
)
);

assert.ok(
storage
.bucket(BLURRED_BUCKET_NAME)
.file(offensiveFileName)
.exists(),
'File uploaded'
);
const exists = await storage
.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 () => {
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);
}
Expand Down