From b4654b19d5f72aa42b93bde5e65ba44e3db80b59 Mon Sep 17 00:00:00 2001 From: Fabien O'Carroll Date: Fri, 14 Dec 2018 11:54:52 +0700 Subject: [PATCH] Cleaned up image manipulator (#10282) no-issue * Added InternalServerError to resizeImage * Added a redirect to original image if sharp is missing * Improved naming - safeMethod -> method * Updated process method to follow same sharp check pattern * Refactor safety wrapper into makeSafe function * Moved generic manipulation error to makeSafe function * Refactored unsafeProcess to use unsafeResizeImage * Removed CRAZY catch --- ghost/image-transform/lib/transform.js | 76 +++++++------------- ghost/image-transform/test/transform.test.js | 60 +++------------- 2 files changed, 34 insertions(+), 102 deletions(-) diff --git a/ghost/image-transform/lib/transform.js b/ghost/image-transform/lib/transform.js index 9051272d7b..f0ecf92c7b 100644 --- a/ghost/image-transform/lib/transform.js +++ b/ghost/image-transform/lib/transform.js @@ -8,60 +8,20 @@ const fs = require('fs-extra'); * We currently can't enable compression or having more config options, because of * https://github.com/lovell/sharp/issues/1360. */ -const process = (options = {}) => { - let sharp, img, originalData, originalSize; - - try { - sharp = require('sharp'); - } catch (err) { - return Promise.reject(new common.errors.InternalServerError({ - message: 'Sharp wasn\'t installed', - code: 'SHARP_INSTALLATION', - err: err - })); - } - - // @NOTE: workaround for Windows as libvips keeps a reference to the input file - // which makes it impossible to fs.unlink() it on cleanup stage - sharp.cache(false); +const unsafeProcess = (options = {}) => { return fs.readFile(options.in) .then((data) => { - originalData = data; - - // @NOTE: have to use constructor with Buffer for sharp to be able to expose size property - img = sharp(data); - }) - .then(() => img.metadata()) - .then((metadata) => { - originalSize = metadata.size; - - if (metadata.width > options.width) { - img.resize(options.width); - } - - // CASE: if you call `rotate` it will automatically remove the orientation (and all other meta data) and rotates - // based on the orientation. It does not rotate if no orientation is set. - img.rotate(); - return img.toBuffer({resolveWithObject: true}); - }) - .then(({data, info}) => { - if (info.size > originalSize) { - return fs.writeFile(options.out, originalData); - } else { - return fs.writeFile(options.out, data); - } - }) - .catch((err) => { - throw new common.errors.InternalServerError({ - message: 'Unable to manipulate image.', - err: err, - code: 'IMAGE_PROCESSING' + return unsafeResizeImage(data, { + width: options.width }); + }) + .then((data) => { + return fs.writeFile(options.out, data); }); }; -const resizeImage = (originalBuffer, {width, height} = {}) => { +const unsafeResizeImage = (originalBuffer, {width, height} = {}) => { const sharp = require('sharp'); return sharp(originalBuffer) .resize(width, height, { @@ -76,12 +36,24 @@ const resizeImage = (originalBuffer, {width, height} = {}) => { }); }; -module.exports.process = process; -module.exports.safeResizeImage = (buffer, options) => { +const makeSafe = fn => (...args) => { try { require('sharp'); - return resizeImage(buffer, options); - } catch (e) { - return Promise.resolve(buffer); + } catch (err) { + return Promise.reject(new common.errors.InternalServerError({ + message: 'Sharp wasn\'t installed', + code: 'SHARP_INSTALLATION', + err: err + })); } + return fn(...args).catch((err) => { + throw new common.errors.InternalServerError({ + message: 'Unable to manipulate image.', + err: err, + code: 'IMAGE_PROCESSING' + }); + }); }; + +module.exports.process = makeSafe(unsafeProcess); +module.exports.resizeImage = makeSafe(unsafeResizeImage); diff --git a/ghost/image-transform/test/transform.test.js b/ghost/image-transform/test/transform.test.js index 939d77983b..7d1931f304 100644 --- a/ghost/image-transform/test/transform.test.js +++ b/ghost/image-transform/test/transform.test.js @@ -20,9 +20,8 @@ describe('lib/image: manipulator', function () { sandbox.stub(fs, 'writeFile').resolves(); sharpInstance = { - metadata: sandbox.stub(), - resize: sandbox.stub(), - rotate: sandbox.stub(), + resize: sandbox.stub().returnsThis(), + rotate: sandbox.stub().returnsThis(), toBuffer: sandbox.stub(), }; @@ -30,23 +29,14 @@ describe('lib/image: manipulator', function () { return sharpInstance; }); - sharp.cache = sandbox.stub(); testUtils.mockNotExistingModule('sharp', sharp); }); it('resize image', function () { - sharpInstance.metadata.resolves({width: 2000, height: 2000}); - sharpInstance.toBuffer.resolves({ - data: 'manipulated', - info: { - size: 42 - } - }); + sharpInstance.toBuffer.resolves('manipulated'); return manipulator.process({width: 1000}) .then(() => { - sharp.cache.calledOnce.should.be.true(); - sharpInstance.metadata.calledOnce.should.be.true(); sharpInstance.resize.calledOnce.should.be.true(); sharpInstance.rotate.calledOnce.should.be.true(); @@ -56,20 +46,14 @@ describe('lib/image: manipulator', function () { }); it('skip resizing if image is too small', function () { - sharpInstance.metadata.resolves({width: 50, height: 50}); - sharpInstance.toBuffer.resolves({ - data: 'manipulated', - info: { - size: 42 - } - }); + sharpInstance.toBuffer.resolves('manipulated'); return manipulator.process({width: 1000}) .then(() => { - sharp.cache.calledOnce.should.be.true(); - sharpInstance.metadata.calledOnce.should.be.true(); - sharpInstance.resize.calledOnce.should.be.false(); - sharpInstance.rotate.calledOnce.should.be.true(); + sharpInstance.resize.calledOnce.should.be.true(); + should.deepEqual(sharpInstance.resize.args[0][2], { + withoutEnlargement: true + }); fs.writeFile.calledOnce.should.be.true(); fs.writeFile.calledWith('manipulated'); @@ -77,22 +61,10 @@ describe('lib/image: manipulator', function () { }); it('uses original image as an output when the size (bytes) is bigger after manipulation', function () { - sharpInstance.metadata.resolves({ - width: 2000, - size: 10 - }); - - sharpInstance.toBuffer.resolves({ - data: 'manipulated', - info: { - size: 42 - } - }); + sharpInstance.toBuffer.resolves('manipulated to a very very very very very very very large size'); return manipulator.process({width: 1000}) .then(() => { - sharp.cache.calledOnce.should.be.true(); - sharpInstance.metadata.calledOnce.should.be.true(); sharpInstance.resize.calledOnce.should.be.true(); sharpInstance.rotate.calledOnce.should.be.true(); sharpInstance.toBuffer.calledOnce.should.be.true(); @@ -103,13 +75,7 @@ describe('lib/image: manipulator', function () { }); it('sharp throws error during processing', function () { - sharpInstance.metadata.resolves({width: 500, height: 500}); - sharpInstance.toBuffer.resolves({ - data: 'manipulated', - info: { - size: 42 - } - }); + sharpInstance.toBuffer.resolves('manipulated'); fs.writeFile.rejects(new Error('whoops')); @@ -120,12 +86,6 @@ describe('lib/image: manipulator', function () { .catch((err) => { (err instanceof common.errors.InternalServerError).should.be.true; err.code.should.eql('IMAGE_PROCESSING'); - sharp.cache.calledOnce.should.be.true; - sharpInstance.metadata.calledOnce.should.be.true(); - sharpInstance.resize.calledOnce.should.be.false(); - sharpInstance.rotate.calledOnce.should.be.true(); - - fs.writeFile.calledOnce.should.be.true(); }); }); });