From 30bee115fec47897d94f6030e0521b66c9552a75 Mon Sep 17 00:00:00 2001 From: Aileen Nowak Date: Thu, 31 Aug 2017 16:39:37 +0700 Subject: [PATCH] Used `got` to handle requests for image-size (#8892) refs #8589, refs #8868 - swap `request` with `got` in `getImageSizeFromUrl` util - less handling for request cases e.g. timeouts, follow redirects --- core/server/utils/image-size-from-url.js | 154 ++++----- .../unit/server_helpers/ghost_head_spec.js | 312 +++++++++--------- .../unit/utils/image-size-from-url_spec.js | 72 +++- package.json | 1 + yarn.lock | 102 +++++- 5 files changed, 383 insertions(+), 258 deletions(-) diff --git a/core/server/utils/image-size-from-url.js b/core/server/utils/image-size-from-url.js index caffe01f1d..1f746a84c0 100644 --- a/core/server/utils/image-size-from-url.js +++ b/core/server/utils/image-size-from-url.js @@ -14,17 +14,14 @@ // we add the protocol to the incomplete one and use urlFor() to get the absolute URL. // If the request fails or image-size is not able to read the file, we reject with error. -var sizeOf = require('image-size'), - url = require('url'), - Promise = require('bluebird'), - http = require('http'), - https = require('https'), - config = require('../config'), - utils = require('../utils'), - errors = require('../errors'), - dimensions, - request, - requestHandler; +var sizeOf = require('image-size'), + url = require('url'), + Promise = require('bluebird'), + got = require('got'), + config = require('../config'), + utils = require('../utils'), + errors = require('../errors'), + dimensions; /** * @description read image dimensions from URL @@ -32,90 +29,75 @@ var sizeOf = require('image-size'), * @returns {Promise} imageObject or error */ module.exports.getImageSizeFromUrl = function getImageSizeFromUrl(imagePath) { - return new Promise(function imageSizeRequest(resolve, reject) { - var imageObject = {}, - options, - timeout = config.get('times:getImageSizeTimeoutInMS') || 10000; + var imageObject = {}, + requestOptions, + timeout = config.get('times:getImageSizeTimeoutInMS') || 10000; - imageObject.url = imagePath; + imageObject.url = imagePath; - // check if we got an url without any protocol - if (imagePath.indexOf('http') === -1) { - // our gravatar urls start with '//' in that case add 'http:' - if (imagePath.indexOf('//') === 0) { - // it's a gravatar url - imagePath = 'http:' + imagePath; - } else { - // get absolute url for image - imagePath = utils.url.urlFor('image', {image: imagePath}, true); - } + // check if we got an url without any protocol + if (imagePath.indexOf('http') === -1) { + // our gravatar urls start with '//' in that case add 'http:' + if (imagePath.indexOf('//') === 0) { + // it's a gravatar url + imagePath = 'http:' + imagePath; + } else { + // get absolute url for image + imagePath = utils.url.urlFor('image', {image: imagePath}, true); } + } - options = url.parse(imagePath); + imagePath = url.parse(imagePath); + requestOptions = { + headers: { + 'User-Agent': 'Mozilla/5.0' + }, + timeout: timeout, + encoding: null + }; - requestHandler = imagePath.indexOf('https') === 0 ? https : http; - options.headers = {'User-Agent': 'Mozilla/5.0'}; + return got( + imagePath, + requestOptions + ).then(function (response) { + try { + // response.body contains the Buffer. Using the Buffer rather than an URL + // requires to use sizeOf synchronously. See https://github.com/image-size/image-size#asynchronous + dimensions = sizeOf(response.body); - request = requestHandler.get(options, function (res) { - var chunks = []; + imageObject.width = dimensions.width; + imageObject.height = dimensions.height; - res.on('data', function (chunk) { - chunks.push(chunk); - }); - - res.on('end', function () { - if (res.statusCode === 200) { - try { - dimensions = sizeOf(Buffer.concat(chunks)); - - imageObject.width = dimensions.width; - imageObject.height = dimensions.height; - - return resolve(imageObject); - } catch (err) { - return reject(new errors.InternalServerError({ - code: 'IMAGE_SIZE', - err: err, - context: imagePath - })); - } - } else if (res.statusCode === 404) { - return reject(new errors.NotFoundError({ - message: 'Image not found.', - code: 'IMAGE_SIZE', - statusCode: res.statusCode, - context: imagePath - })); - } else { - return reject(new errors.InternalServerError({ - message: 'Unknown Request error.', - code: 'IMAGE_SIZE', - statusCode: res.statusCode, - context: imagePath - })); - } - }); - }).on('socket', function (socket) { - if (timeout) { - socket.setTimeout(timeout); - - /** - * https://nodejs.org/api/http.html - * "...if a callback is assigned to the Server's 'timeout' event, timeouts must be handled explicitly" - * - * socket.destroy will jump to the error listener - */ - socket.on('timeout', function () { - request.abort(); - socket.destroy(new Error('Request timed out.')); - }); - } - }).on('error', function (err) { - return reject(new errors.InternalServerError({ + return Promise.resolve(imageObject); + } catch (err) { + return Promise.reject(new errors.InternalServerError({ code: 'IMAGE_SIZE', err: err, - context: imagePath + context: imagePath.href })); - }); + } + }).catch(function (err) { + if (err.statusCode === 404) { + return Promise.reject(new errors.NotFoundError({ + message: 'Image not found.', + code: 'IMAGE_SIZE', + statusCode: err.statusCode, + context: err.url || imagePath.href || imagePath + })); + } else if (err.code === 'ETIMEDOUT') { + return Promise.reject(new errors.InternalServerError({ + message: 'Request timed out.', + code: 'IMAGE_SIZE', + statusCode: err.statusCode, + context: err.url || imagePath.href || imagePath + })); + } else { + return Promise.reject(new errors.InternalServerError({ + message: 'Unknown Request error.', + code: 'IMAGE_SIZE', + statusCode: err.statusCode, + context: err.url || imagePath.href || imagePath + })); + } }); }; diff --git a/core/test/unit/server_helpers/ghost_head_spec.js b/core/test/unit/server_helpers/ghost_head_spec.js index d8e62a74af..2f77cdab40 100644 --- a/core/test/unit/server_helpers/ghost_head_spec.js +++ b/core/test/unit/server_helpers/ghost_head_spec.js @@ -42,7 +42,7 @@ describe('{{ghost_head}} helper', function () { return localSettingsCache[key]; }); - configUtils.set('url', 'http://localhost:82832/'); + configUtils.set('url', 'http://localhost:65530/'); }); it('returns meta tag string on paginated index page without structured data and schema', function (done) { @@ -52,9 +52,9 @@ describe('{{ghost_head}} helper', function () { ).then(function (rendered) { should.exist(rendered); rendered.string.should.match(//); - rendered.string.should.match(//); + rendered.string.should.match(//); rendered.string.should.match(//); - rendered.string.should.match(//); + rendered.string.should.match(//); rendered.string.should.not.match(//); @@ -70,28 +70,28 @@ describe('{{ghost_head}} helper', function () { ).then(function (rendered) { should.exist(rendered); rendered.string.should.match(//); - rendered.string.should.match(//); + rendered.string.should.match(//); rendered.string.should.match(//); rendered.string.should.match(//); rendered.string.should.match(//); rendered.string.should.match(//); rendered.string.should.match(//); rendered.string.should.match(//); - rendered.string.should.match(//); - rendered.string.should.match(//); + rendered.string.should.match(//); + rendered.string.should.match(//); rendered.string.should.match(//); rendered.string.should.match(//); rendered.string.should.match(//); - rendered.string.should.match(//); - rendered.string.should.match(//); + rendered.string.should.match(//); + rendered.string.should.match(//); rendered.string.should.match(//); - rendered.string.should.match(//); + rendered.string.should.match(//); rendered.string.should.match(/