Refactor: fetch image dimensions from local file storage #8900

aileen · 2017-08-15T11:09:51Z

don't fetch icon sizes for blog logo within meta data anymore and make it synchronous

Refactors the getImageSizeFromUrl:

Renamed file to image-size.js
Added second fn getImageSizeFromFilePath that reads from local file storage
Added guard in getImageSizeFromUrl that checks if the image should be on local file storage and uses the new fn then instead
Added tests for new fn
Added a fn fetchDimensionsFromBuffer that takes the file buffer and returns an imageObject with dimensions.
Added a new util getLocalFileStoragePath which takes an image URL or filepath and sanitizes it so it's readable for the storage.

TODOS:

make tests work
implement logic in image-dimensions fn / think of a smarter way to differentiate when to read from local file storage and when via request

aileen · 2017-08-31T09:28:42Z

@kirrg001: if this looks good to you, I actually think that it would make sense to try and get this into LTS as well? What do you think?

kirrg001 · 2017-08-31T09:48:42Z

Can please you rebase? 🙏🏽

aileen · 2017-08-31T10:35:02Z

Done @kirrg001!!

core/server/utils/image-size-from-url.js

@@ -29,9 +29,16 @@ var sizeOf = require('image-size'),
 * @returns {Promise<Object>} imageObject or error
 */
 module.exports.getImageSizeFromUrl = function getImageSizeFromUrl(imagePath) {
+<<<<<<< HEAD


core/server/utils/image-size-from-url.js

@@ -1,103 +0,0 @@
-// Supported formats of https://github.com/image-size/image-size:


core/server/data/meta/index.js


-    return Promise.props(getBlogLogo()).then(function (result) {
-        metaData.blog.logo = result;
+    metaData.blog.logo = getBlogLogo();


core/server/utils/image-size.js

+    imagePath = imagePath.replace(new RegExp('^' + utils.url.urlJoin(utils.url.urlFor('home', true), utils.url.getSubdir(), '/', utils.url.STATIC_IMAGE_URL_PREFIX)), '');
+
+    return storage.getStorage()
+        .read({path: imagePath})


core/server/utils/image-size.js

+    imageObject.url = imagePath;
+
+    // storage needs the path without `/content/images/` prefix
+    imagePath = imagePath.replace(new RegExp('^' + utils.url.urlJoin(utils.url.urlFor('home', true), utils.url.getSubdir(), '/', utils.url.STATIC_IMAGE_URL_PREFIX)), '');


core/server/utils/image-size.js

+    config = require('../config'),
+    storage = require('../adapters/storage'),
+    _ = require('lodash'),
+    dimensions,


core/server/utils/image-size.js

+    return got(
+        imagePath,
+        requestOptions
+    ).then(function (response) {


kirrg001 · 2017-09-04T11:54:48Z

I wonder, if it was discussed at any point to rather fetch and store the dimensions in the database when storing the image? (Could be a simple look up table url -> dimensions). I was unable to find a discussion. Because even with this PR, we can run into timeouts which block the frontend request.

Plus the concern i've raised here #8900 (comment), which describes that blogs are using e.g. the s3 storage adapter and not sure the performance is better than querying the dimensions via the s3 lib. Was that considered/tested?

aileen · 2017-09-04T12:08:43Z

I wonder, if it was discussed at any point to rather fetch and store the dimensions in the database when storing the image? (Could be a simple look up table url -> dimensions).

I remember this was being discussed before the very first implementation but then decided to wait for the image optimisation first.

Re: possible timeouts when people are using a custom storage adapter like S3 wasn't discussed. Maybe this should be discussed and considered first before we proceed with this PR?

aileen · 2017-09-05T11:06:10Z

Re: possible timeouts when people are using a custom storage adapter like S3 wasn't discussed. Maybe this should be discussed and considered first before we proceed with this PR?

We decided that reading from the local file storage would overweight the benefits for users who don't use a custom storage adapter. We'll therefore merge this change and are happy for contributions if improvements for custom storage adapters are needed.

core/server/utils/get-local-file-storage-path.js

+ * @description Takes a url or filepath and returns a filepath with is readable
+ * for the local file storage.
+ */
+function getLocalFileStoragePath(imagePath) {


refs TryGhost#8868 - Removed image-size in blog logo fn for meta data and made it synchronous - Renamed `image-size-from-url.js` to `image-size.js` (incl. the test) - Added second fn `getImageSizeFromFilePath` that reads from local file storage - Added guard in `getImageSizeFromUrl` that checks if the image should be on local file storage and uses the new fn then instead - Added a fn `fetchDimensionsFromBuffer` that takes the file buffer and returns an `imageObject` with dimensions. - Added a new util directory in `adapters/storage` and there a new fn `getLocalFileStoragePath` which takes an image URL or filepath and sanitizes it so it's readable for the storage.

ErisDS · 2017-09-05T13:05:43Z

I wonder, if it was discussed at any point to rather fetch and store the dimensions in the database when storing the image? (Could be a simple look up table url -> dimensions). I was unable to find a discussion. Because even with this PR, we can run into timeouts which block the frontend request.

At some point we may move towards using a different cache, but not just yet - there are a couple of other things that need to become clearer with regard to images

Plus the concern i've raised here #8900 (comment), which describes that blogs are using e.g. the s3 storage adapter and not sure the performance is better than querying the dimensions via the s3 lib. Was that considered/tested?

Users with custom storage adapters are welcome to submit improvements that make this work/perform better for them - I think our focus should always be on the default case 😊

no issue Adds redirect test back, which was accidentially removed with PR TryGhost#8900

no issue Adds redirect test back, which was accidentially removed with PR #8900

aileen mentioned this pull request Aug 29, 2017

🙈 Used got to handle requests for image-size #8892

Merged

3 tasks

aileen force-pushed the refactor-image-size-use-local-storage branch 3 times, most recently from 9a10cf0 to ef5a212 Compare August 29, 2017 10:02

aileen changed the title ~~[WIP] 🔥 Refactor: fetch image dimensions from local file storage~~ Refactor: fetch image dimensions from local file storage Aug 29, 2017

aileen assigned kirrg001 Aug 29, 2017

aileen force-pushed the refactor-image-size-use-local-storage branch from ef5a212 to d1f6130 Compare August 31, 2017 10:33

aileen commented Aug 31, 2017

View reviewed changes

kirrg001 reviewed Sep 4, 2017

View reviewed changes

core/server/data/meta/index.js Outdated

return Promise.props(getBlogLogo()).then(function (result) {

metaData.blog.logo = result;

metaData.blog.logo = getBlogLogo();

This comment was marked as abuse.

Sign in to view

kirrg001 reviewed Sep 4, 2017

View reviewed changes

core/server/utils/image-size.js Outdated

config = require('../config'),

storage = require('../adapters/storage'),

_ = require('lodash'),

dimensions,

This comment was marked as abuse.

Sign in to view

kirrg001 reviewed Sep 4, 2017

View reviewed changes

core/server/utils/image-size.js

return got(

imagePath,

requestOptions

).then(function (response) {

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

aileen force-pushed the refactor-image-size-use-local-storage branch from d1f6130 to 8a6547b Compare September 4, 2017 12:02

aileen force-pushed the refactor-image-size-use-local-storage branch 6 times, most recently from 720ef9d to 7364e4c Compare September 5, 2017 10:20

kirrg001 reviewed Sep 5, 2017

View reviewed changes

aileen force-pushed the refactor-image-size-use-local-storage branch from 7364e4c to b9dc7e3 Compare September 5, 2017 11:29

kirrg001 added 2 commits September 5, 2017 13:48

Fix incorrect Promise/Catch handling for fetchDimensionsFromBuffer

a06521e

rename to adapters/storage/utils.js

d526b63

kirrg001 merged commit eef7932 into TryGhost:master Sep 5, 2017

aileen mentioned this pull request Sep 6, 2017

💄 Logging for image size #8978

Merged

aileen added a commit to aileen/Ghost that referenced this pull request Sep 7, 2017

🙈 Add redirect test back for image-size test

31911df

no issue Adds redirect test back, which was accidentially removed with PR TryGhost#8900

aileen mentioned this pull request Sep 7, 2017

🙈 Add redirect test back for image-size test #8984

Merged

ErisDS pushed a commit that referenced this pull request Sep 7, 2017

Add redirect test back for image-size test 🙈 (#8984)

c64c56f

no issue Adds redirect test back, which was accidentially removed with PR #8900

aileen deleted the refactor-image-size-use-local-storage branch October 13, 2017 08:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: fetch image dimensions from local file storage #8900

Refactor: fetch image dimensions from local file storage #8900

aileen commented Aug 15, 2017 •

edited

Loading

aileen commented Aug 31, 2017

kirrg001 commented Aug 31, 2017

aileen commented Aug 31, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

kirrg001 commented Sep 4, 2017 •

edited

Loading

aileen commented Sep 4, 2017 •

edited

Loading

aileen commented Sep 5, 2017

This comment was marked as abuse.

This comment was marked as abuse.

ErisDS commented Sep 5, 2017

		@@ -1,103 +0,0 @@
		// Supported formats of https://github.com/image-size/image-size:

Refactor: fetch image dimensions from local file storage #8900

Refactor: fetch image dimensions from local file storage #8900

Conversation

aileen commented Aug 15, 2017 • edited Loading

TODOS:

aileen commented Aug 31, 2017

kirrg001 commented Aug 31, 2017

aileen commented Aug 31, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

kirrg001 commented Sep 4, 2017 • edited Loading

aileen commented Sep 4, 2017 • edited Loading

aileen commented Sep 5, 2017

This comment was marked as abuse.

This comment was marked as abuse.

ErisDS commented Sep 5, 2017

aileen commented Aug 15, 2017 •

edited

Loading

kirrg001 commented Sep 4, 2017 •

edited

Loading

aileen commented Sep 4, 2017 •

edited

Loading