Merge pull request #13930 from strapi/fix/manually-resize-image-on-upload

Do not override width and height after uploading image

packages/core/upload/server/services/__tests__/upload/formatFileInfo.test.js

@@ -1,6 +1,6 @@
 'use strict';
 
-const uploadService = require('../upload')({});
+const uploadService = require('../../upload')({});
 
 describe('Upload service', () => {
   beforeAll(() => {

packages/core/upload/server/services/__tests__/upload/uploadImage.test.js

@@ -0,0 +1,89 @@
+'use strict';
+
+const path = require('path');
+const fs = require('fs');
+const fse = require('fs-extra');
+const _ = require('lodash');
+const uploadService = require('../../upload')({});
+
+const testFilePath = path.join(__dirname, './image.png');
+const tmpWorkingDirectory = path.join(__dirname, './tmp');
+
+function mockUploadProvider(uploadFunc, props) {
+  const { responsiveDimensions = false } = props || {};
+
+  const default_config = {
+    plugin: {
+      upload: {
+        breakpoints: {
+          large: 1000,
+          medium: 750,
+        },
+      },
+    },
+  };
+
+  global.strapi = {
+    config: {
+      get: (path, defaultValue) => _.get(default_config, path, defaultValue),
+    },
+    plugins: {
+      upload: {
+        services: {
+          provider: {
+            upload: uploadFunc,
+          },
+          upload: {
+            getSettings: () => ({ responsiveDimensions }),
+          },
+          'image-manipulation': require('../../image-manipulation')(),
+        },
+      },
+    },
+  };
+}
+
+const getFileData = () => ({
+  alternativeText: 'image.png',
+  caption: 'image.png',
+  ext: '.png',
+  folder: null,
+  folderPath: '/',
+  getStream: () => fs.createReadStream(testFilePath),
+  hash: 'image_d9b4f84424',
+  height: 1000,
+  size: 4,
+  width: 1500,
+  tmpWorkingDirectory,
+});
+
+describe('Upload image', () => {
+  beforeAll(async () => {
+    // Create tmp directory if it does not exist
+    await fse.mkdir(tmpWorkingDirectory);
+  });
+
+  afterAll(async () => {
+    // Remove tmp directory
+    await fse.remove(tmpWorkingDirectory);
+  });
+
+  test('Upload with thubmnail', async () => {
+    let fileData = getFileData();
+    const upload = jest.fn();
+    mockUploadProvider(upload);
+
+    await uploadService.uploadImage(fileData);
+    expect(upload).toHaveBeenCalledTimes(2);
+  });
+
+  test('Upload with responsive formats', async () => {
+    let fileData = getFileData();
+    const upload = jest.fn();
+    mockUploadProvider(upload, { responsiveDimensions: true });
+
+    await uploadService.uploadImage(fileData);
+    // 1 for the original image, 1 for thumbnail, 2 for the responsive formats
+    expect(upload).toHaveBeenCalledTimes(4);
+  });
+});

packages/core/upload/server/services/upload.js

@@ -159,50 +159,74 @@ module.exports = ({ strapi }) => ({
     return uploadedFiles;
   },
 
-  async uploadFileAndPersist(fileData, { user } = {}) {
+  /**
-    const config = strapi.config.get('plugin.upload');
+   * When uploading an image, an additional thubmnail is generated.
-
+   * Also, if there are responsive formats defined, another set of images will be generated too.
+   *
+   * @param {*} fileData
+   */
+  async uploadImage(fileData) {
     const {
       getDimensions,
       generateThumbnail,
       generateResponsiveFormats,
-      isImage,
       isOptimizableImage,
     } = getService('image-manipulation');
+
+    // Store width and height of the original image
+    const { width, height } = await getDimensions(fileData);
+
+    // Make sure this is assigned before calling upload
+    // That way it can mutate the width and height
+    _.assign(fileData, {
+      width,
+      height,
+    });
+
+    // Upload image
     await getService('provider').upload(fileData);
 
-    if (await isImage(fileData)) {
+    // Generate thumbnail and responsive formats
-      if (await isOptimizableImage(fileData)) {
+    if (await isOptimizableImage(fileData)) {
-        const thumbnailFile = await generateThumbnail(fileData);
+      const thumbnailFile = await generateThumbnail(fileData);
-        if (thumbnailFile) {
+      if (thumbnailFile) {
-          await getService('provider').upload(thumbnailFile);
+        await getService('provider').upload(thumbnailFile);
-          _.set(fileData, 'formats.thumbnail', thumbnailFile);
+        _.set(fileData, 'formats.thumbnail', thumbnailFile);
-        }
-
-        const formats = await generateResponsiveFormats(fileData);
-        if (Array.isArray(formats) && formats.length > 0) {
-          for (const format of formats) {
-            if (!format) continue;
-
-            const { key, file } = format;
-
-            await getService('provider').upload(file);
-
-            _.set(fileData, ['formats', key], file);
-          }
-        }
       }
 
-      const { width, height } = await getDimensions(fileData);
+      const formats = await generateResponsiveFormats(fileData);
+      if (Array.isArray(formats) && formats.length > 0) {
+        for (const format of formats) {
+          if (!format) continue;
 
-      _.assign(fileData, {
+          const { key, file } = format;
-        width,
+
-        height,
+          await getService('provider').upload(file);
-      });
+
+          _.set(fileData, ['formats', key], file);
+        }
+      }
+    }
+  },
+
+  /**
+   * Upload a file. If it is an image it will generate a thumbnail
+   * and responsive formats (if enabled).
+   */
+  async uploadFileAndPersist(fileData, { user } = {}) {
+    const config = strapi.config.get('plugin.upload');
+
+    const { isImage } = getService('image-manipulation');
+
+    if (await isImage(fileData)) {
+      await this.uploadImage(fileData);
+    } else {
+      await getService('provider').upload(fileData);
     }
 
     _.set(fileData, 'provider', config.provider);
 
+    // Persist file(s)
     return this.add(fileData, { user });
   },
 
@@ -230,9 +254,7 @@ module.exports = ({ strapi }) => ({
   async replace(id, { data, file }, { user } = {}) {
     const config = strapi.config.get('plugin.upload');
 
-    const { getDimensions, generateThumbnail, generateResponsiveFormats } = getService(
+    const { isImage } = getService('image-manipulation');
-      'image-manipulation'
-    );
 
     const dbFile = await this.findOne(id);
     if (!dbFile) {
@@ -267,42 +289,15 @@ module.exports = ({ strapi }) => ({
         }
       }
 
-      await getService('provider').upload(fileData);
-
       // clear old formats
       _.set(fileData, 'formats', {});
 
-      const { isImage, isOptimizableImage } = getService('image-manipulation');
-
       if (await isImage(fileData)) {
-        if (await isOptimizableImage(fileData)) {
+        await this.uploadImage(fileData);
-          const thumbnailFile = await generateThumbnail(fileData);
+      } else {
-          if (thumbnailFile) {
+        await getService('provider').upload(fileData);
-            await getService('provider').upload(thumbnailFile);
-            _.set(fileData, 'formats.thumbnail', thumbnailFile);
-          }
-
-          const formats = await generateResponsiveFormats(fileData);
-          if (Array.isArray(formats) && formats.length > 0) {
-            for (const format of formats) {
-              if (!format) continue;
-
-              const { key, file } = format;
-
-              await getService('provider').upload(file);
-
-              _.set(fileData, ['formats', key], file);
-            }
-          }
-        }
-
-        const { width, height } = await getDimensions(fileData);
-
-        _.assign(fileData, {
-          width,
-          height,
-        });
       }
+
       _.set(fileData, 'provider', config.provider);
     } finally {
       // delete temporary folder