fix(gatsby-source-drupal): end activities if errors are thrown (#25447)

* fix(gatsby-source-drupal): end activities if errors are thrown

* Update packages/gatsby-source-drupal/src/__tests__/index.js

Co-authored-by: Ward Peeters <ward@coding-tech.com>

Co-authored-by: gatsbybot <mathews.kyle+gatsbybot@gmail.com>
Co-authored-by: Ward Peeters <ward@coding-tech.com>

Author: 3 people

packages/gatsby-source-drupal/src/__tests__/index.js

@@ -1,14 +1,14 @@
 jest.mock(`axios`, () => {
   return {
-    get: path => {
+    get: jest.fn(path => {
       const last = path.split(`/`).pop()
       try {
         return { data: require(`./fixtures/${last}.json`) }
       } catch (e) {
         console.log(`Error`, e)
         return null
       }
-    },
+    }),
   }
 })
 
@@ -18,13 +18,16 @@ jest.mock(`gatsby-source-filesystem`, () => {
   }
 })
 
+const normalize = require(`../normalize`)
+const downloadFileSpy = jest.spyOn(normalize, `downloadFile`)
+
 const { createRemoteFileNode } = require(`gatsby-source-filesystem`)
 
 const { sourceNodes } = require(`../gatsby-node`)
 const { handleWebhookUpdate } = require(`../utils`)
 
 describe(`gatsby-source-drupal`, () => {
-  const nodes = {}
+  let nodes = {}
   const createNodeId = id => `generated-id-${id}`
   const baseUrl = `http://fixture`
   const createContentDigest = jest.fn().mockReturnValue(`contentDigest`)
@@ -362,4 +365,95 @@ describe(`gatsby-source-drupal`, () => {
     expect(nodes[createNodeId(`tag-3`)]).toBeDefined()
     expect(nodes[createNodeId(`article-5`)]).toBeDefined()
   })
+
+  describe(`Error handling`, () => {
+    describe(`Does end activities if error is thrown`, () => {
+      const axios = require(`axios`)
+      beforeEach(() => {
+        nodes = {}
+        reporter.activityTimer.mockClear()
+        activity.start.mockClear()
+        activity.end.mockClear()
+        axios.get.mockClear()
+        downloadFileSpy.mockClear()
+      })
+
+      it(`during data fetching`, async () => {
+        axios.get.mockImplementationOnce(() => {
+          throw new Error(`data fetching failed`)
+        })
+        expect.assertions(5)
+
+        try {
+          await sourceNodes(args, { baseUrl })
+        } catch (e) {
+          expect(e).toMatchInlineSnapshot(`[Error: data fetching failed]`)
+        }
+
+        expect(reporter.activityTimer).toHaveBeenCalledTimes(1)
+        expect(reporter.activityTimer).toHaveBeenNthCalledWith(
+          1,
+          `Fetch data from Drupal`
+        )
+
+        expect(activity.start).toHaveBeenCalledTimes(1)
+        expect(activity.end).toHaveBeenCalledTimes(1)
+      })
+
+      it(`during file downloading`, async () => {
+        downloadFileSpy.mockImplementationOnce(() => {
+          throw new Error(`file downloading failed`)
+        })
+
+        expect.assertions(6)
+
+        try {
+          await sourceNodes(args, { baseUrl })
+        } catch (e) {
+          expect(e).toMatchInlineSnapshot(`[Error: file downloading failed]`)
+        }
+
+        expect(reporter.activityTimer).toHaveBeenCalledTimes(2)
+        expect(reporter.activityTimer).toHaveBeenNthCalledWith(
+          1,
+          `Fetch data from Drupal`
+        )
+        expect(reporter.activityTimer).toHaveBeenNthCalledWith(
+          2,
+          `Remote file download`
+        )
+
+        expect(activity.start).toHaveBeenCalledTimes(2)
+        expect(activity.end).toHaveBeenCalledTimes(2)
+      })
+
+      it(`during refresh webhook handling`, async () => {
+        expect.assertions(5)
+
+        try {
+          await sourceNodes(
+            {
+              ...args,
+              webhookBody: {
+                malformattedPayload: true,
+              },
+            },
+            { baseUrl }
+          )
+        } catch (e) {
+          expect(e).toBeTruthy()
+        }
+
+        expect(reporter.activityTimer).toHaveBeenCalledTimes(1)
+        expect(reporter.activityTimer).toHaveBeenNthCalledWith(
+          1,
+          `loading Drupal content changes`,
+          expect.anything()
+        )
+
+        expect(activity.start).toHaveBeenCalledTimes(1)
+        expect(activity.end).toHaveBeenCalledTimes(1)
+      })
+    })
+  })
 })

packages/gatsby-source-drupal/src/gatsby-node.js

@@ -7,6 +7,21 @@ const { handleReferences, handleWebhookUpdate } = require(`./utils`)
 const asyncPool = require(`tiny-async-pool`)
 const bodyParser = require(`body-parser`)
 
+function gracefullyRethrow(activity, error) {
+  // activity.panicOnBuild was implemented at some point in gatsby@2
+  // but plugin can still be used with older version of gatsby core
+  // so need to do some checking here
+  if (activity.panicOnBuild) {
+    activity.panicOnBuild(error)
+  }
+
+  activity.end()
+
+  if (!activity.panicOnBuild) {
+    throw error
+  }
+}
+
 exports.sourceNodes = async (
   {
     actions,
@@ -44,41 +59,45 @@ exports.sourceNodes = async (
     )
     changesActivity.start()
 
-    const { secret, action, id, data } = webhookBody
-    if (pluginOptions.secret && pluginOptions.secret !== secret) {
-      reporter.warn(
-        `The secret in this request did not match your plugin options secret.`
-      )
-      return
-    }
-    if (action === `delete`) {
-      actions.deleteNode({ node: getNode(createNodeId(id)) })
-      reporter.log(`Deleted node: ${id}`)
-      return
-    }
+    try {
+      const { secret, action, id, data } = webhookBody
+      if (pluginOptions.secret && pluginOptions.secret !== secret) {
+        reporter.warn(
+          `The secret in this request did not match your plugin options secret.`
+        )
+        return
+      }
+      if (action === `delete`) {
+        actions.deleteNode({ node: getNode(createNodeId(id)) })
+        reporter.log(`Deleted node: ${id}`)
+        return
+      }
 
-    let nodesToUpdate = data
-    if (!Array.isArray(data)) {
-      nodesToUpdate = [data]
-    }
+      let nodesToUpdate = data
+      if (!Array.isArray(data)) {
+        nodesToUpdate = [data]
+      }
 
-    for (const nodeToUpdate of nodesToUpdate) {
-      await handleWebhookUpdate(
-        {
-          nodeToUpdate,
-          actions,
-          cache,
-          createNodeId,
-          createContentDigest,
-          getCache,
-          getNode,
-          reporter,
-          store,
-        },
-        pluginOptions
-      )
+      for (const nodeToUpdate of nodesToUpdate) {
+        await handleWebhookUpdate(
+          {
+            nodeToUpdate,
+            actions,
+            cache,
+            createNodeId,
+            createContentDigest,
+            getCache,
+            getNode,
+            reporter,
+            store,
+          },
+          pluginOptions
+        )
+      }
+    } catch (e) {
+      gracefullyRethrow(changesActivity, e)
+      return
     }
-
     changesActivity.end()
     return
   }
@@ -118,74 +137,80 @@ exports.sourceNodes = async (
 
   drupalFetchActivity.start()
 
-  const data = await axios.get(`${baseUrl}/${apiBase}`, {
-    auth: basicAuth,
-    headers,
-    params,
-  })
-  const allData = await Promise.all(
-    _.map(data.data.links, async (url, type) => {
-      if (disallowedLinkTypes.includes(type)) return
-      if (!url) return
-      if (!type) return
-      const getNext = async (url, data = []) => {
-        if (typeof url === `object`) {
-          // url can be string or object containing href field
-          url = url.href
-
-          // Apply any filters configured in gatsby-config.js. Filters
-          // can be any valid JSON API filter query string.
-          // See https://www.drupal.org/docs/8/modules/jsonapi/filtering
-          if (typeof filters === `object`) {
-            if (filters.hasOwnProperty(type)) {
-              url = url + `?${filters[type]}`
+  let allData
+  try {
+    const data = await axios.get(`${baseUrl}/${apiBase}`, {
+      auth: basicAuth,
+      headers,
+      params,
+    })
+    allData = await Promise.all(
+      _.map(data.data.links, async (url, type) => {
+        if (disallowedLinkTypes.includes(type)) return
+        if (!url) return
+        if (!type) return
+        const getNext = async (url, data = []) => {
+          if (typeof url === `object`) {
+            // url can be string or object containing href field
+            url = url.href
+
+            // Apply any filters configured in gatsby-config.js. Filters
+            // can be any valid JSON API filter query string.
+            // See https://www.drupal.org/docs/8/modules/jsonapi/filtering
+            if (typeof filters === `object`) {
+              if (filters.hasOwnProperty(type)) {
+                url = url + `?${filters[type]}`
+              }
             }
           }
-        }
 
-        let d
-        try {
-          d = await axios.get(url, {
-            auth: basicAuth,
-            headers,
-            params,
-          })
-        } catch (error) {
-          if (error.response && error.response.status == 405) {
-            // The endpoint doesn't support the GET method, so just skip it.
-            return []
-          } else {
-            console.error(`Failed to fetch ${url}`, error.message)
-            console.log(error.data)
-            throw error
+          let d
+          try {
+            d = await axios.get(url, {
+              auth: basicAuth,
+              headers,
+              params,
+            })
+          } catch (error) {
+            if (error.response && error.response.status == 405) {
+              // The endpoint doesn't support the GET method, so just skip it.
+              return []
+            } else {
+              console.error(`Failed to fetch ${url}`, error.message)
+              console.log(error.data)
+              throw error
+            }
+          }
+          data = data.concat(d.data.data)
+          // Add support for includes. Includes allow entity data to be expanded
+          // based on relationships. The expanded data is exposed as `included`
+          // in the JSON API response.
+          // See https://www.drupal.org/docs/8/modules/jsonapi/includes
+          if (d.data.included) {
+            data = data.concat(d.data.included)
+          }
+          if (d.data.links && d.data.links.next) {
+            data = await getNext(d.data.links.next, data)
           }
-        }
-        data = data.concat(d.data.data)
-        // Add support for includes. Includes allow entity data to be expanded
-        // based on relationships. The expanded data is exposed as `included`
-        // in the JSON API response.
-        // See https://www.drupal.org/docs/8/modules/jsonapi/includes
-        if (d.data.included) {
-          data = data.concat(d.data.included)
-        }
-        if (d.data.links && d.data.links.next) {
-          data = await getNext(d.data.links.next, data)
-        }
 
-        return data
-      }
+          return data
+        }
 
-      const data = await getNext(url)
+        const data = await getNext(url)
 
-      const result = {
-        type,
-        data,
-      }
+        const result = {
+          type,
+          data,
+        }
 
-      // eslint-disable-next-line consistent-return
-      return result
-    })
-  )
+        // eslint-disable-next-line consistent-return
+        return result
+      })
+    )
+  } catch (e) {
+    gracefullyRethrow(drupalFetchActivity, e)
+    return
+  }
 
   drupalFetchActivity.end()
 
@@ -216,17 +241,31 @@ exports.sourceNodes = async (
 
     // Download all files (await for each pool to complete to fix concurrency issues)
     const fileNodes = [...nodes.values()].filter(isFileNode)
+
     if (fileNodes.length) {
       const downloadingFilesActivity = reporter.activityTimer(
         `Remote file download`
       )
       downloadingFilesActivity.start()
-      await asyncPool(concurrentFileRequests, fileNodes, async node => {
-        await downloadFile(
-          { node, store, cache, createNode, createNodeId, getCache, reporter },
-          pluginOptions
-        )
-      })
+      try {
+        await asyncPool(concurrentFileRequests, fileNodes, async node => {
+          await downloadFile(
+            {
+              node,
+              store,
+              cache,
+              createNode,
+              createNodeId,
+              getCache,
+              reporter,
+            },
+            pluginOptions
+          )
+        })
+      } catch (e) {
+        gracefullyRethrow(downloadingFilesActivity, e)
+        return
+      }
       downloadingFilesActivity.end()
     }
   }