fix(gatsby-plugin-sharp): strip non related args when hashing resizing args (#12129)

oorestisime · pieh · commit da0b6221bb7f · 2019-03-27T14:54:28.000+01:00
So continuing on the work to make gatsby-plugin-sharp friendly on third-party packages (oorestisime/gatsby-remark-rehype-images#2) we now face this issue #12080 TL;DR images generated from transformer-sharp or and other plugins are the same (same sha256) but with different hash (depends on the available options). The problem is that all the plugins leak their options into the hash generation and even ones that are unrelated to the resizing processing (base64, generateTraceSVG, traceSVG etc) I've been discussing this with @pieh for the last 2-3 hours :D Proposed solution was to sanitize the args based on a whitelist, which is implemented on the PR. Which leads to the first question: - is it ok if after this release _all_ image hashes are changing? By doing so it can also potentially help lazy image processing @wardpeet by getting rid of unrelated options when persisting jobs. Anyway current state of the PR is that it kind of works, i ve tested this in websites using my plugin and other that not. The issue here is that there are some differences in the number of images created when this modification is on or not. I also verified that public folder doesn't contain duplicate images: `find public -regex ".*\.\(jpg\|webp\|png\|jpeg\)" -type f -exec sha256sum {} \; | cut -d ' ' -f 1 |sort| uniq -d | wc -l`
diff --git a/packages/gatsby-plugin-sharp/package.json b/packages/gatsby-plugin-sharp/package.json
@@ -10,7 +10,6 @@
     "@babel/runtime": "^7.0.0",
     "async": "^2.1.2",
     "bluebird": "^3.5.0",
-    "fs-exists-cached": "^1.0.0",
     "fs-extra": "^7.0.0",
     "imagemin": "^6.0.0",
     "imagemin-mozjpeg": "^8.0.0",
diff --git a/packages/gatsby-plugin-sharp/src/__tests__/__snapshots__/index.js.snap b/packages/gatsby-plugin-sharp/src/__tests__/__snapshots__/index.js.snap
@@ -15,14 +15,14 @@ Object {
   "aspectRatio": 2.0661764705882355,
   "base64": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABQAAAAKCAYAAAC0VX7mAAAACXBIWXMAABYlAAAWJQFJUiTwAAABAklEQVQoz61R7WrCQBDM+7+Cv1qhlIogoUWMoH8KrfgOLVXRKDV4d7kkl6/pbsxpqBIo9mBuZ7N3k51bB7TKsrwZVsexyS2rKerwFhYFVI1DnkMQOHIuCaLB+ayuwXlMaApXgvMoxjTU8MIQE4rMx8SnNffUkU/qM/bbiPAeRfgw5n8tB+QgO1kmUvwROdm0kYHmUNoe+NoU2wZTdVjQH7Isg9YRpFIXor8vpGmKjb/FQQgslit8fi3wvQ/OHXLhxRvDHY7Qd5+x3e2udmNzTUN4fZvhoddH566L+8cn9AZuVcvpLR3e9kEAISWEkDAmbRXkGCcJ1r4PRRNPEkPu9Kn2Azs0CqJYU3JKAAAAAElFTkSuQmCC",
   "density": 144,
-  "originalImg": "/static/1234/c81ba/test.png",
+  "originalImg": "/static/1234/e681d/test.png",
   "originalName": undefined,
   "presentationHeight": 68,
   "presentationWidth": 141,
   "sizes": "(max-width: 141px) 100vw, 141px",
-  "src": "/static/1234/c81ba/test.png",
-  "srcSet": "/static/1234/e4832/test.png 200w,
-/static/1234/c81ba/test.png 281w",
+  "src": "/static/1234/e681d/test.png",
+  "srcSet": "/static/1234/9ec3c/test.png 200w,
+/static/1234/e681d/test.png 281w",
   "srcSetType": "image/png",
   "tracedSVG": undefined,
 }
@@ -33,14 +33,14 @@ Object {
   "aspectRatio": 2.0661764705882355,
   "base64": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABQAAAAKCAYAAAC0VX7mAAAACXBIWXMAABYlAAAWJQFJUiTwAAABAklEQVQoz61R7WrCQBDM+7+Cv1qhlIogoUWMoH8KrfgOLVXRKDV4d7kkl6/pbsxpqBIo9mBuZ7N3k51bB7TKsrwZVsexyS2rKerwFhYFVI1DnkMQOHIuCaLB+ayuwXlMaApXgvMoxjTU8MIQE4rMx8SnNffUkU/qM/bbiPAeRfgw5n8tB+QgO1kmUvwROdm0kYHmUNoe+NoU2wZTdVjQH7Isg9YRpFIXor8vpGmKjb/FQQgslit8fi3wvQ/OHXLhxRvDHY7Qd5+x3e2udmNzTUN4fZvhoddH566L+8cn9AZuVcvpLR3e9kEAISWEkDAmbRXkGCcJ1r4PRRNPEkPu9Kn2Azs0CqJYU3JKAAAAAElFTkSuQmCC",
   "density": 144,
-  "originalImg": "/static/1234/ec7d1/test.png",
+  "originalImg": "/static/1234/e681d/test.png",
   "originalName": undefined,
   "presentationHeight": 136,
   "presentationWidth": 281,
   "sizes": "(max-width: 281px) 100vw, 281px",
-  "src": "/static/1234/ec7d1/test.png",
-  "srcSet": "/static/1234/dc107/test.png 200w,
-/static/1234/ec7d1/test.png 281w",
+  "src": "/static/1234/e681d/test.png",
+  "srcSet": "/static/1234/9ec3c/test.png 200w,
+/static/1234/e681d/test.png 281w",
   "srcSetType": "image/png",
   "tracedSVG": undefined,
 }
@@ -51,13 +51,13 @@ Object {
   "aspectRatio": 1,
   "base64": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABQAAAAUCAIAAAAC64paAAAACXBIWXMAAAsSAAALEgHS3X78AAABKklEQVQ4y8WUy07DMBBFrwMpr4aybjepUeqIFd/CAgmJsuIHgWXVL+Gx4C3xCdQJnpm0UDJNAQnVuouMM8dzY4+DcoA/C/8Fe9bv4ABMMpQOZc5yFPqfwEUQY499jHoY9/BsKQyTRTNMyzu8WAwTbBnI2DY438XbPr0qFsFCvlrkMTERYFgRL3HYUvh5OMdpQqkbBtPC9NDiINQPCZM6LGVvUzIpwNch4V5EGxHS/DeYtjfHZbcyXB8yGfaPimcafLEMHqmw2L5OsWkUUuY6Ee5V28WUP25TXjwPS3iWUFnfcFQPFna98rlmSGL4IOZucQvgT76Po53qeOTYTtp4qpFKe/qM29PhJsVVl3SXsluntLfS7rPvn10MP1Aau+lKBuA9I/kV/AyW6gNTIWTuA/r8lQAAAABJRU5ErkJggg==",
   "density": 72,
-  "originalImg": "/static/1234/197d2/test.png",
+  "originalImg": "/static/1234/c0399/test.png",
   "originalName": undefined,
   "presentationHeight": 100,
   "presentationWidth": 100,
   "sizes": "(max-width: 100px) 100vw, 100px",
-  "src": "/static/1234/197d2/test.png",
-  "srcSet": "/static/1234/197d2/test.png 100w",
+  "src": "/static/1234/c0399/test.png",
+  "srcSet": "/static/1234/c0399/test.png 100w",
   "srcSetType": "image/png",
   "tracedSVG": undefined,
 }
@@ -144,8 +144,8 @@ Object {
   "base64": undefined,
   "height": 100,
   "originalName": undefined,
-  "src": "/static/1234/24ae5/test.png",
-  "srcSet": "/static/1234/24ae5/test.png 1x",
+  "src": "/static/1234/c0399/test.png",
+  "srcSet": "/static/1234/c0399/test.png 1x",
   "tracedSVG": "data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' width='100' height='100'%3e%3cpath d='M41 24c-18 7-24 29-11 43 15 17 44 8 46-15 1-19-17-34-35-28' fill='red' fill-rule='evenodd'/%3e%3c/svg%3e",
   "width": 100,
 }
@@ -156,15 +156,15 @@ Object {
   "aspectRatio": 1,
   "base64": undefined,
   "density": 72,
-  "originalImg": "/static/1234/9dd55/test.png",
+  "originalImg": "/static/1234/c0399/test.png",
   "originalName": undefined,
   "presentationHeight": 100,
   "presentationWidth": 100,
   "sizes": "(max-width: 100px) 100vw, 100px",
-  "src": "/static/1234/9dd55/test.png",
-  "srcSet": "/static/1234/a516e/test.png 25w,
-/static/1234/208be/test.png 50w,
-/static/1234/9dd55/test.png 100w",
+  "src": "/static/1234/c0399/test.png",
+  "srcSet": "/static/1234/0f0dc/test.png 25w,
+/static/1234/bc08f/test.png 50w,
+/static/1234/c0399/test.png 100w",
   "srcSetType": "image/png",
   "tracedSVG": "data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' width='100' height='100'%3e%3cpath d='M41 24c-18 7-24 29-11 43 15 17 44 8 46-15 1-19-17-34-35-28' fill='red' fill-rule='evenodd'/%3e%3c/svg%3e",
 }
diff --git a/packages/gatsby-plugin-sharp/src/__tests__/process-file.js b/packages/gatsby-plugin-sharp/src/__tests__/process-file.js
@@ -0,0 +1,94 @@
+const { createArgsDigest } = require(`../process-file`)
+
+describe(`createArgsDigest`, () => {
+  const defaultArgsBaseline = {
+    toFormat: `jpg`,
+    width: 500,
+    height: 500,
+    cropFocus: 17,
+    quality: 50,
+    pngCompressionLevel: 4,
+    jpegProgressive: false,
+    grayscale: false,
+    rotate: 0,
+    duotone: null,
+  }
+
+  describe(`changes hash if used args are different`, () => {
+    const testHashDifferent = (label, change, extraBaselineOptions = {}) => {
+      it(label, () => {
+        const argsBaseline = {
+          ...defaultArgsBaseline,
+          ...extraBaselineOptions,
+        }
+        const baselineHash = createArgsDigest(argsBaseline)
+        const outputHash = createArgsDigest({
+          ...defaultArgsBaseline,
+          ...change,
+        })
+        expect(baselineHash).not.toBe(outputHash)
+      })
+    }
+
+    testHashDifferent(`width change`, { width: defaultArgsBaseline.width + 1 })
+    testHashDifferent(`height change`, {
+      height: defaultArgsBaseline.height + 1,
+    })
+    testHashDifferent(`cropFocus change`, {
+      cropFocus: defaultArgsBaseline.cropFocus + 1,
+    })
+    testHashDifferent(`format change`, { toFormat: `png` })
+    testHashDifferent(`jpegProgressive change`, {
+      jpegProgressive: !defaultArgsBaseline.jpegProgressive,
+    })
+    testHashDifferent(`grayscale change`, {
+      grayscale: !defaultArgsBaseline.grayscale,
+    })
+    testHashDifferent(`rotate change`, {
+      rotate: defaultArgsBaseline.rotate + 1,
+    })
+    testHashDifferent(`dutone change`, {
+      duotone: {
+        highlight: `#ff0000`,
+        shadow: `#000000`,
+      },
+    })
+  })
+
+  describe(`doesn't change hash if not used options are different`, () => {
+    const testHashEqual = (label, change, extraBaselineOptions = {}) => {
+      it(label, () => {
+        const argsBaseline = {
+          ...defaultArgsBaseline,
+          ...extraBaselineOptions,
+        }
+        const baselineHash = createArgsDigest(argsBaseline)
+        const outputHash = createArgsDigest({
+          ...defaultArgsBaseline,
+          ...change,
+        })
+        expect(baselineHash).toBe(outputHash)
+      })
+    }
+
+    testHashEqual(`unrelated argument`, { foo: `bar` })
+
+    testHashEqual(`png option change when using jpg`, {
+      pngCompressionLevel: defaultArgsBaseline.pngCompressionLevel + 1,
+    })
+    testHashEqual(
+      `jpg option change when using png`,
+      {
+        toFormat: `png`,
+        jpegProgressive: !defaultArgsBaseline.jpegProgressive,
+      },
+      {
+        toFormat: `png`,
+      }
+    )
+    describe(`not used arguments`, () => {
+      testHashEqual(`maxWidth`, { maxWidth: 500 })
+      testHashEqual(`base64`, { base64: true })
+    })
+  })
+})
diff --git a/packages/gatsby-plugin-sharp/src/index.js b/packages/gatsby-plugin-sharp/src/index.js
@@ -25,6 +25,7 @@ const _ = require(`lodash`)
 const fs = require(`fs-extra`)
 const path = require(`path`)
 const { scheduleJob } = require(`./scheduler`)
+const { createArgsDigest } = require(`./process-file`)
 
 const imageSizeCache = new Map()
 const getImageSize = file => {
@@ -148,33 +149,12 @@ const healOptions = (
 
 function queueImageResizing({ file, args = {}, reporter }) {
   const options = healOptions(pluginOptions, args, file.extension)
-  // Filter out false args, and args not for this extension and put width at
-  // end (for the file path)
-  const pairedArgs = _.toPairs(args)
-  let filteredArgs
-  // Remove non-true arguments
-  filteredArgs = _.filter(pairedArgs, arg => arg[1])
-  // Remove pathPrefix
-  filteredArgs = _.filter(filteredArgs, arg => arg[0] !== `pathPrefix`)
-  filteredArgs = _.filter(filteredArgs, arg => {
-    if (file.extension.match(/^jp*/)) {
-      return !_.includes(arg[0], `png`)
-    } else if (file.extension.match(/^png/)) {
-      return !arg[0].match(/^jp*/)
-    }
-    return true
-  })
-  const sortedArgs = _.sortBy(filteredArgs, arg => arg[0] === `width`)
-  const fileExtension = options.toFormat ? options.toFormat : file.extension
-
-  const argsDigest = crypto
-    .createHash(`md5`)
-    .update(JSON.stringify(sortedArgs))
-    .digest(`hex`)
-
-  const argsDigestShort = argsDigest.substr(argsDigest.length - 5)
+  if (!options.toFormat) {
+    options.toFormat = file.extension
+  }
 
-  const imgSrc = `/${file.name}.${fileExtension}`
+  const argsDigestShort = createArgsDigest(options)
+  const imgSrc = `/${file.name}.${options.toFormat}`
   const dirPath = path.join(
     process.cwd(),
     `public`,
@@ -212,7 +192,7 @@ function queueImageResizing({ file, args = {}, reporter }) {
   }
 
   // encode the file name for URL
-  const encodedImgSrc = `/${encodeURIComponent(file.name)}.${fileExtension}`
+  const encodedImgSrc = `/${encodeURIComponent(file.name)}.${options.toFormat}`
 
   // Prefix the image src.
   const digestDirPrefix = `${file.internal.contentDigest}/${argsDigestShort}`
diff --git a/packages/gatsby-plugin-sharp/src/process-file.js b/packages/gatsby-plugin-sharp/src/process-file.js
@@ -6,6 +6,8 @@ const imagemin = require(`imagemin`)
 const imageminMozjpeg = require(`imagemin-mozjpeg`)
 const imageminPngquant = require(`imagemin-pngquant`)
 const imageminWebp = require(`imagemin-webp`)
+const _ = require(`lodash`)
+const crypto = require(`crypto`)
 
 // Try to enable the use of SIMD instructions. Seems to provide a smallish
 // speedup on resizing heavy loads (~10%). Sharp disables this feature by
@@ -24,7 +26,49 @@ try {
   // doesn't support cpu-core-count utility.
 }
 
-module.exports = (file, transforms, options = {}) => {
+/**
+ * List of arguments used by `processFile` function.
+ * This is used to generate args hash using only
+ * arguments that affect output of that function.
+ */
+const argsWhitelist = [
+  `height`,
+  `width`,
+  `cropFocus`,
+  `toFormat`,
+  `pngCompressionLevel`,
+  `quality`,
+  `jpegProgressive`,
+  `grayscale`,
+  `rotate`,
+  `duotone`,
+]
+
+/**
+ * @typedef {Object} TransformArgs
+ * @property {number} height
+ * @property {number} width
+ * @property {number} cropFocus
+ * @property {string} toFormat
+ * @property {number} pngCompressionLevel
+ * @property {number} quality
+ * @property {boolean} jpegProgressive
+ * @property {boolean} grayscale
+ * @property {number} rotate
+ * @property {object} duotone
+ */
+
+/**+
+ * @typedef {Object} Transform
+ * @property {string} outputPath
+ * @property {TransformArgs} args
+ */
+
+/**
+ * @param {String} file
+ * @param {Transform[]} transforms
+ */
+exports.processFile = (file, transforms, options = {}) => {
   let pipeline
   try {
     pipeline = sharp(file)
@@ -169,3 +213,26 @@ const compressWebP = (pipeline, outputPath, options) =>
       })
       .then(imageminBuffer => fs.writeFile(outputPath, imageminBuffer))
   )
+
+exports.createArgsDigest = args => {
+  const filtered = _.pickBy(args, (value, key) => {
+    // remove falsy
+    if (!value) return false
+    if (args.toFormat.match(/^jp*/) && _.includes(key, `png`)) {
+      return false
+    } else if (args.toFormat.match(/^png/) && key.match(/^jp*/)) {
+      return false
+    }
+    // after initial processing - get rid of unknown/unneeded fields
+    return argsWhitelist.includes(key)
+  })
+
+  const argsDigest = crypto
+    .createHash(`md5`)
+    .update(JSON.stringify(filtered, Object.keys(filtered).sort()))
+    .digest(`hex`)
+
+  const argsDigestShort = argsDigest.substr(argsDigest.length - 5)
+
+  return argsDigestShort
+}
diff --git a/packages/gatsby-plugin-sharp/src/scheduler.js b/packages/gatsby-plugin-sharp/src/scheduler.js
@@ -1,8 +1,8 @@
 const _ = require(`lodash`)
 const ProgressBar = require(`progress`)
-const existsSync = require(`fs-exists-cached`).sync
+const { existsSync } = require(`fs`)
 const queue = require(`async/queue`)
-const processFile = require(`./process-file`)
+const { processFile } = require(`./process-file`)
 
 const toProcess = {}
 let totalJobs = 0
