Improved error handling during theme file checks

refs https://github.com/TryGhost/Team/issues/864

- It was unclear what was behind a generic "ValidationError" trown by gscan when investigating the issue
- The changes improve:
-  the error handling is now more granular - catching the prolbe one level closer to the acutal prolbem
- error messages describe a little better the stage they fail in
- when the error is constructed we included the "err" parameter to have the stack trace available in the logs

Author: naz

lib/checker.js

@@ -1,6 +1,7 @@
 const Promise = require('bluebird');
 const _ = require('lodash');
 const requireDir = require('require-dir');
+const {errors} = require('ghost-ignition');
 const readTheme = require('./read-theme');
 const versions = require('./utils').versions;
 
@@ -14,6 +15,7 @@ const checks = requireDir('./checks');
  * @param {string} themePath
  * @param {Object} options
  * @param {string} [options.checkVersion] version to check the theme against
+ * @param {string} [options.themeName] name of the checked theme
  * @returns {Promise<Object>}
  */
 const checker = function checkAll(themePath, options = {}) {
@@ -32,6 +34,15 @@ const checker = function checkAll(themePath, options = {}) {
             return Promise.reduce(_.values(checks), function (theme, check) {
                 return check(theme, options, themePath);
             }, theme);
+        })
+        .catch((error) => {
+            throw new errors.ValidationError({
+                message: 'Failed theme files check',
+                help: 'Your theme file structure is corrupted or contains errors',
+                errorDetails: error.message,
+                context: options.themeName,
+                err: error
+            });
         });
 };
 

lib/index.js

@@ -31,20 +31,26 @@ const checkZip = async function checkZip(path, options) {
 
     try {
         const {path: extractedZipPath} = await readZip(zip);
-        const theme = await check(extractedZipPath, options);
+        const theme = await check(extractedZipPath, Object.assign({themeName: zip.name}, options));
 
         if (options.keepExtractedDir) {
             return theme;
         } else {
-            return fs.remove(zip.origPath).then(() => theme);
+            await fs.remove(zip.origPath);
+            return theme;
         }
     } catch (error) {
-        throw new errors.ValidationError({
-            message: 'Failed to read zip file',
-            help: 'Your zip file might be corrupted, try unzipping and zipping again.',
-            errorDetails: error.message,
-            context: zip.name
-        });
+        if (!errors.utils.isIgnitionError(error)) {
+            throw new errors.ValidationError({
+                message: 'Failed to check zip file',
+                help: 'Your zip file might be corrupted, try unzipping and zipping again.',
+                errorDetails: error.message,
+                context: zip.name,
+                err: error
+            });
+        }
+
+        throw error;
     }
 };
 

lib/read-zip.js

@@ -4,6 +4,7 @@ const Promise = require('bluebird');
 const os = require('os');
 const glob = require('glob');
 const {extract} = require('@tryghost/zip');
+const {errors} = require('ghost-ignition');
 const uuid = require('uuid');
 const _ = require('lodash');
 
@@ -38,7 +39,14 @@ const readZip = (zip) => {
             return zip;
         }).catch((err) => {
             debug('Zip extraction error', err);
-            throw err;
+
+            throw new errors.ValidationError({
+                message: 'Failed to read zip file',
+                help: 'Your zip file might be corrupted, try unzipping and zipping again.',
+                errorDetails: err.message,
+                context: zip.name,
+                err: err
+            });
         });
 };
 

test/general.test.js

@@ -35,5 +35,25 @@ describe('Check zip', function () {
                 });
         });
     });
+
+    describe('throws errors', function () {
+        it('non existing file', async function () {
+            try {
+                await checkZip(themePath('030-assets/do_not_exist.zip'));
+                should.fail(checkZip, 'Should have errored');
+            } catch (err) {
+                should.exist(err);
+
+                should.exist(err.errorType);
+                should.equal(err.errorType, 'ValidationError');
+
+                should.exist(err.message);
+                should.equal(err.message, 'Failed to read zip file');
+
+                should.exist(err.help);
+                should.equal(err.help, 'Your zip file might be corrupted, try unzipping and zipping again.');
+            }
+        });
+    });
 });