Repository: lirantal/lockfile-lint
Pull request #22 (2019-10-13 20:12:12)

Adds more user friendly error messages in ParseLockfile
Closes: #16

<!--- Provide a general summary of your changes in the Title above -->

## Description

Added enhanced and more informative error messages in `ParseLockfile.js`. The following error types are supported:

#### NO_OPTIONS:

Programatically generated the scenario below, this isn't something that would happen normally:

```
$ lockfile-lint -p __tests__/fixtures/yarn-only-http.lock --allowed-hosts yarn --validate-https
ABORTING lockfile lint process due to error exceptions

Did not recive options for lockfile path or type.

Error at new ParsingError (stack trace...)

error: command failed with exit code 1
```

#### NO_PARSER_FOR_TYPE:

```
$ lockfile-lint -p __tests__/fixtures/yarn-only-http.lock --allowed-hosts yarn --validate-https --type pnpm
ABORTING lockfile lint process due to error exceptions

Unable to find relevant lockfile parser for type "pnpm", the currently available options are npm,yarn.

Error at new ParsingError (stack trace...)

error: command failed with exit code 1
```

#### NO_PARSER_FOR_PATH:

```
$ lockfile-lint -p __tests__/fixtures/yarn-only-http.lock --allowed-hosts yarn --validate-https
ABORTING lockfile lint process due to error exceptions

Unable to find relevant lockfile parser for "__tests__/fixtures/yarn-only-http.lock", consider passing the --type option.

Error at new ParsingError (stack trace...)

error: command failed with exit code 1
```

#### READ_FAILED:

```
$ lockfile-lint -p __tests__/fixtures/non-existing-yarn.lock --allowed-hosts yarn --validate-https --type yarn
ABORTING lockfile lint process due to error exceptions

Unable to read lockfile "__tests__/fixtures/non-existing-yarn.lock"

Error: ENOENT: no such file or directory, open '__tests__/fixtures/non-existing-yarn.lock'
    at Object.openSync (fs.js:443:3)
    at Object.readFileSync (fs.js:343:35) (stack trace...)

error: command failed with exit code 1
```

#### PARSE_NPMLOCKFILE_FAILED:

```
$ lockfile-lint -p __tests__/fixtures/bad-package-lock.json --allowed-hosts yarn --validate-https --type npm
ABORTING lockfile lint process due to error exceptions

Unable to parse npm lockfile "__tests__/fixtures/bad-package-lock.json"

SyntaxError: Unexpected token m in JSON at position 23 (stack trace...)

error: command failed with exit code 1

```

#### PARSE_YARNLOCKFILE_FAILED

```
$ lockfile-lint -p __tests__/fixtures/bad-yarn.lock --allowed-hosts yarn --validate-https --type yarn
ABORTING lockfile lint process due to error exceptions

Unable to parse yarn lockfile "__tests__/fixtures/bad-yarn.lock"

SyntaxError: Unknown token: { line: 1, col: 0, type: 'INVALID', value: undefined } 1:0 in lockfile (stack trace...)

error: command failed with exit code 1

```

#### More Details

These error types are defined in `packages/lockfile-lint-api/src/common/ParsingError.js` and are used in `packages/lockfile-lint-api/src/ParseLockfile.js`.

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->

- [x] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Related Issue

#16 

## Motivation and Context

More descriptive/informative error messages when parsing a lockfile fails.

## How Has This Been Tested?

Added/Updated unit tests for all scenarios.


## Checklist:

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [x] I have updated the documentation (if required).
- [x] I have read the **CONTRIBUTING** document.
- [x] I have added tests to cover my changes.
- [x] All new and existing tests passed.
- [x] I added a picture of a cute animal cause it's fun

![giphy-cat-type](https://user-images.githubusercontent.com/5598081/66721765-8c299900-ee0e-11e9-8991-cad45aea4e4a.gif)



emimuresan: Not sure why the tests in TravisCI are failing with:

```
Expected substring: "expecting options object"
lockfile-lint-api:     Received message:   "Did not recive options for lockfile path or type"
```

I modified `parseLockfile.test`, so instead of "expecting options object" it actually should be "Did not recive options for lockfile path or type", and that is what it asserts on in the test file.
lirantal: @emimuresan I'm unable to reproduce the build failure locally either and I also tried resetting the build cache which didn't help. What about if you just open a new PR with the exact same code? I think you can just use a new branch to trigger a new PR
lirantal: @emimuresan maybe you can rebase with master? checking out locally I see that I'm finding the test file being unstaged. This is probably why it's not getting through to the CI.
codecov-io: # [Codecov](https://codecov.io/gh/lirantal/lockfile-lint/pull/22?src=pr&el=h1) Report
> Merging [#22](https://codecov.io/gh/lirantal/lockfile-lint/pull/22?src=pr&el=desc) into [master](https://codecov.io/gh/lirantal/lockfile-lint/commit/ff32753afa5881b06fff6cefa1f19662d561f468?src=pr&el=desc) will **increase** coverage by `3.16%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/lirantal/lockfile-lint/pull/22/graphs/tree.svg?width=650&token=P36VJa7ksa&height=150&src=pr)](https://codecov.io/gh/lirantal/lockfile-lint/pull/22?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #22      +/-   ##
==========================================
+ Coverage    96.1%   99.26%   +3.16%     
==========================================
  Files           9        8       -1     
  Lines         154      136      -18     
  Branches       21       22       +1     
==========================================
- Hits          148      135      -13     
+ Misses          6        1       -5
```


| [Impacted Files](https://codecov.io/gh/lirantal/lockfile-lint/pull/22?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...kages/lockfile-lint-api/src/common/PackageError.js](https://codecov.io/gh/lirantal/lockfile-lint/pull/22/diff?src=pr&el=tree#diff-cGFja2FnZXMvbG9ja2ZpbGUtbGludC1hcGkvc3JjL2NvbW1vbi9QYWNrYWdlRXJyb3IuanM=) | `100% <ø> (ø)` | :arrow_up: |
| [packages/lockfile-lint-api/src/ParseLockfile.js](https://codecov.io/gh/lirantal/lockfile-lint/pull/22/diff?src=pr&el=tree#diff-cGFja2FnZXMvbG9ja2ZpbGUtbGludC1hcGkvc3JjL1BhcnNlTG9ja2ZpbGUuanM=) | `100% <100%> (+2.63%)` | :arrow_up: |
| [packages/lockfile-lint-api/src/common/constants.js](https://codecov.io/gh/lirantal/lockfile-lint/pull/22/diff?src=pr&el=tree#diff-cGFja2FnZXMvbG9ja2ZpbGUtbGludC1hcGkvc3JjL2NvbW1vbi9jb25zdGFudHMuanM=) | `100% <100%> (ø)` | |
| [...s/lockfile-lint-api/src/validators/ValidateHost.js](https://codecov.io/gh/lirantal/lockfile-lint/pull/22/diff?src=pr&el=tree#diff-cGFja2FnZXMvbG9ja2ZpbGUtbGludC1hcGkvc3JjL3ZhbGlkYXRvcnMvVmFsaWRhdGVIb3N0Lmpz) | `100% <100%> (ø)` | :arrow_up: |
| [...kages/lockfile-lint-api/src/common/ParsingError.js](https://codecov.io/gh/lirantal/lockfile-lint/pull/22/diff?src=pr&el=tree#diff-cGFja2FnZXMvbG9ja2ZpbGUtbGludC1hcGkvc3JjL2NvbW1vbi9QYXJzaW5nRXJyb3IuanM=) | `100% <100%> (ø)` | |
| [packages/lockfile-lint/src/validators/index.js](https://codecov.io/gh/lirantal/lockfile-lint/pull/22/diff?src=pr&el=tree#diff-cGFja2FnZXMvbG9ja2ZpbGUtbGludC9zcmMvdmFsaWRhdG9ycy9pbmRleC5qcw==) | | |
| [packages/lockfile-lint/src/cli.js](https://codecov.io/gh/lirantal/lockfile-lint/pull/22/diff?src=pr&el=tree#diff-cGFja2FnZXMvbG9ja2ZpbGUtbGludC9zcmMvY2xpLmpz) | | |
| [packages/lockfile-lint/src/main.js](https://codecov.io/gh/lirantal/lockfile-lint/pull/22/diff?src=pr&el=tree#diff-cGFja2FnZXMvbG9ja2ZpbGUtbGludC9zcmMvbWFpbi5qcw==) | | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/lirantal/lockfile-lint/pull/22?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/lirantal/lockfile-lint/pull/22?src=pr&el=footer). Last update [ff32753...8e9d43c](https://codecov.io/gh/lirantal/lockfile-lint/pull/22?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

emimuresan: > @emimuresan maybe you can rebase with master? checking out locally I see that I'm finding the test file being unstaged. This is probably why it's not getting through to the CI.

Yep, it was strange. Upon further investigation i discovered that there were 2 duplicate test files for ParseLockfile, i checked on master and it was the same:
```
parseLockFile.test.js
parseLockfile.test.js
```

I deleted `parseLockFile.test.js` in this PR. Hopefully it should be ok in CI now.

lirantal: Great work here @emimuresan ✨, thank you ❤️

Patch:
diff --git a/packages/lockfile-lint-api/src/ParseLockfile.js b/packages/lockfile-lint-api/src/ParseLockfile.js
index 26784be..d0bbb8f 100644
--- a/packages/lockfile-lint-api/src/ParseLockfile.js
+++ b/packages/lockfile-lint-api/src/ParseLockfile.js
@@ -4,6 +4,15 @@
 const fs = require('fs')
 const path = require('path')
 const yarnLockfileParser = require('@yarnpkg/lockfile')
+const {ParsingError, ERROR_MESSAGES} = require('./common/ParsingError')
+const {
+  NO_OPTIONS,
+  NO_PARSER_FOR_PATH,
+  NO_PARSER_FOR_TYPE,
+  READ_FAILED,
+  PARSE_NPMLOCKFILE_FAILED,
+  PARSE_YARNLOCKFILE_FAILED
+} = ERROR_MESSAGES
 
 class ParseLockfile {
   /**
@@ -14,7 +23,7 @@ class ParseLockfile {
    */
   constructor (options) {
     if (!options || typeof options !== 'object') {
-      throw new Error('expecting options object')
+      throw new ParsingError(NO_OPTIONS)
     }
 
     this.options = {}
@@ -22,6 +31,14 @@ class ParseLockfile {
     this.options.lockfileType = options.lockfileType
   }
 
+  /**
+   * Checks if lockfile type option was provided
+   * @return boolean
+   */
+  isLockfileTypeGiven () {
+    return typeof this.options.lockfileType === 'string' && this.options.lockfileType
+  }
+
   /**
    * Synchronously parses a lockfile
    * @return {object} parsed file
@@ -29,11 +46,20 @@ class ParseLockfile {
   parseSync () {
     const lockfileParser = this.resolvePkgMgrForLockfile()
     if (!lockfileParser) {
-      throw new Error('unable to find relevant lockfile parser')
+      if (this.isLockfileTypeGiven()) {
+        throw new ParsingError(NO_PARSER_FOR_TYPE, this.options.lockfileType)
+      }
+      throw new ParsingError(NO_PARSER_FOR_PATH, this.options.lockfilePath)
+    }
+
+    let file
+    try {
+      // eslint-disable-next-line security/detect-non-literal-fs-filename
+      file = fs.readFileSync(this.options.lockfilePath, 'utf8')
+    } catch (error) {
+      throw new ParsingError(READ_FAILED, this.options.lockfilePath, error)
     }
 
-    // eslint-disable-next-line security/detect-non-literal-fs-filename
-    const file = fs.readFileSync(this.options.lockfilePath, 'utf8')
     return lockfileParser.call(this, file)
   }
 
@@ -46,7 +72,7 @@ class ParseLockfile {
     }
 
     let resolver
-    if (typeof this.options.lockfileType === 'string' && this.options.lockfileType) {
+    if (this.isLockfileTypeGiven()) {
       resolver = lockfileResolversByPackageManager[this.options.lockfileType]
     }
 
@@ -70,17 +96,27 @@ class ParseLockfile {
   }
 
   parseYarnLockfile (lockfileBuffer) {
-    return yarnLockfileParser.parse(lockfileBuffer)
+    let parsedFile
+    try {
+      parsedFile = yarnLockfileParser.parse(lockfileBuffer)
+    } catch (error) {
+      throw new ParsingError(PARSE_YARNLOCKFILE_FAILED, this.options.lockfilePath, error)
+    }
+    return parsedFile
   }
 
   parseNpmLockfile (lockfileBuffer) {
-    const packageJsonParsed = JSON.parse(lockfileBuffer)
-
-    // transform original format of npm's package-json
-    // to match yarns so we have a unified format to validate
-    // against
-    const npmDepsTree = packageJsonParsed.dependencies
-    const flattenedDepTree = this._flattenNpmDepsTree(npmDepsTree)
+    let flattenedDepTree
+    try {
+      const packageJsonParsed = JSON.parse(lockfileBuffer)
+
+      // transform original format of npm's package-json to match yarns
+      // so we have a unified format to validate against
+      const npmDepsTree = packageJsonParsed.dependencies
+      flattenedDepTree = this._flattenNpmDepsTree(npmDepsTree)
+    } catch (error) {
+      throw new ParsingError(PARSE_NPMLOCKFILE_FAILED, this.options.lockfilePath, error)
+    }
 
     return {
       type: 'success',
diff --git a/packages/lockfile-lint-api/src/common/PackageError.js b/packages/lockfile-lint-api/src/common/PackageError.js
index 04fefa1..c94fef8 100644
--- a/packages/lockfile-lint-api/src/common/PackageError.js
+++ b/packages/lockfile-lint-api/src/common/PackageError.js
@@ -4,7 +4,7 @@ module.exports = class PackageError extends Error {
   /**
    * constructor
    * @param {string} packageName - the name of the package where the error occured
-   * @param {string} error - the original error object
+   * @param {Error} error - the original error object
    */
   constructor (packageName = '', error = {}) {
     super()
diff --git a/packages/lockfile-lint-api/src/common/ParsingError.js b/packages/lockfile-lint-api/src/common/ParsingError.js
new file mode 100644
index 0000000..e8dcc36
--- /dev/null
+++ b/packages/lockfile-lint-api/src/common/ParsingError.js
@@ -0,0 +1,34 @@
+'use strict'
+
+const {LOCKFILE_TYPES} = require('./constants')
+
+const ERROR_MESSAGES = {
+  NO_OPTIONS: () => 'Did not receive options for lockfile path or type',
+  NO_PARSER_FOR_TYPE: type =>
+    `Unable to find relevant lockfile parser for type "${type}", the currently available options are ${LOCKFILE_TYPES}.`,
+  NO_PARSER_FOR_PATH: path =>
+    `Unable to find relevant lockfile parser for "${path}", consider passing the --type option.`,
+  READ_FAILED: path => `Unable to read lockfile "${path}"`,
+  PARSE_NPMLOCKFILE_FAILED: path => `Unable to parse npm lockfile "${path}"`,
+  PARSE_YARNLOCKFILE_FAILED: path => `Unable to parse yarn lockfile "${path}"`
+}
+
+class ParsingError extends Error {
+  /**
+   * constructor
+   * @param {string} errorKey - the key corresponding to the error message
+   * @param {string} relatedValue - the value related to the error, to be used in the error message
+   * @param {Error} error - the original error (if one exists)
+   */
+  constructor (errorFn, relatedValue = '', error = {}) {
+    super()
+    this.name = 'ParsingError'
+    this.message = typeof errorFn === 'function' ? errorFn(relatedValue) : 'INVALID ERROR KEY'
+    this.stack = error.stack || new Error().stack
+  }
+}
+
+module.exports = {
+  ParsingError,
+  ERROR_MESSAGES
+}
diff --git a/packages/lockfile-lint-api/src/common/constants.js b/packages/lockfile-lint-api/src/common/constants.js
new file mode 100644
index 0000000..e85c509
--- /dev/null
+++ b/packages/lockfile-lint-api/src/common/constants.js
@@ -0,0 +1,10 @@
+'use strict'
+
+module.exports = {
+  LOCKFILE_TYPES: ['npm', 'yarn'], // currently supported types
+  REGISTRY: {
+    npm: 'registry.npmjs.org',
+    yarn: 'registry.yarnpkg.com',
+    verdaccio: 'registry.verdaccio.org'
+  }
+}
diff --git a/packages/lockfile-lint-api/src/validators/ValidateHost.js b/packages/lockfile-lint-api/src/validators/ValidateHost.js
index ae80d55..7064830 100644
--- a/packages/lockfile-lint-api/src/validators/ValidateHost.js
+++ b/packages/lockfile-lint-api/src/validators/ValidateHost.js
@@ -2,12 +2,7 @@
 
 const {URL} = require('url')
 const PackageError = require('../common/PackageError')
-
-const REGISTRY = {
-  npm: 'registry.npmjs.org',
-  yarn: 'registry.yarnpkg.com',
-  verdaccio: 'registry.verdaccio.org'
-}
+const {REGISTRY} = require('../common/constants')
 
 module.exports = class ValidateHost {
   constructor ({packages} = {}) {


Test patch:
diff --git a/packages/lockfile-lint-api/__tests__/__fixtures__/bad-package-lock.json b/packages/lockfile-lint-api/__tests__/__fixtures__/bad-package-lock.json
new file mode 100644
index 0000000..dc0c882
--- /dev/null
+++ b/packages/lockfile-lint-api/__tests__/__fixtures__/bad-package-lock.json
@@ -0,0 +1,3 @@
+{
+    "cat-generator": meow
+}
\ No newline at end of file
diff --git a/packages/lockfile-lint-api/__tests__/__fixtures__/bad-yarn.lock b/packages/lockfile-lint-api/__tests__/__fixtures__/bad-yarn.lock
new file mode 100644
index 0000000..dc0c882
--- /dev/null
+++ b/packages/lockfile-lint-api/__tests__/__fixtures__/bad-yarn.lock
@@ -0,0 +1,3 @@
+{
+    "cat-generator": meow
+}
\ No newline at end of file
diff --git a/packages/lockfile-lint-api/__tests__/parseLockFile.test.js b/packages/lockfile-lint-api/__tests__/parseLockFile.test.js
deleted file mode 100644
index 6bc6f6a..0000000
--- a/packages/lockfile-lint-api/__tests__/parseLockFile.test.js
+++ /dev/null
@@ -1,18 +0,0 @@
-/* eslint-disable no-new */
-'use strict'
-
-const ParseLockfile = require('../src/ParseLockfile')
-
-describe('ParseLockfile', () => {
-  it('instantiating a parser with invalid object throws an error', () => {
-    expect(() => {
-      new ParseLockfile()
-    }).toThrowError('expecting options object')
-  })
-
-  it('instantiating a parser with string type throws an error', () => {
-    expect(() => {
-      new ParseLockfile('/path/to/file')
-    }).toThrowError('expecting options object')
-  })
-})
diff --git a/packages/lockfile-lint-api/__tests__/parseLockfile.test.js b/packages/lockfile-lint-api/__tests__/parseLockfile.test.js
index 6bc6f6a..c235a21 100644
--- a/packages/lockfile-lint-api/__tests__/parseLockfile.test.js
+++ b/packages/lockfile-lint-api/__tests__/parseLockfile.test.js
@@ -4,15 +4,46 @@
 const ParseLockfile = require('../src/ParseLockfile')
 
 describe('ParseLockfile', () => {
+  it('instantiating a parser correctly works', () => {
+    expect(() => {
+      new ParseLockfile({lockfilePath: '/path/to/file', lockfileType: 'npm'})
+    }).not.toThrowError()
+  })
+
   it('instantiating a parser with invalid object throws an error', () => {
     expect(() => {
       new ParseLockfile()
-    }).toThrowError('expecting options object')
+    }).toThrowError('Did not receive options for lockfile path or type')
   })
 
   it('instantiating a parser with string type throws an error', () => {
     expect(() => {
       new ParseLockfile('/path/to/file')
-    }).toThrowError('expecting options object')
+    }).toThrowError('Did not receive options for lockfile path or type')
+  })
+
+  it('using a parser with an invalid option type throws an error', () => {
+    const parser = new ParseLockfile({lockfilePath: '/path/to/file', lockfileType: 'meow'})
+    expect(() => {
+      parser.parseSync()
+    }).toThrowError(
+      'Unable to find relevant lockfile parser for type "meow", the currently available options are npm,yarn.'
+    )
+  })
+
+  it('using a parser with a path option for a lockfile of unknown type throws an error', () => {
+    const parser = new ParseLockfile({lockfilePath: '/path/to/file'})
+    expect(() => {
+      parser.parseSync()
+    }).toThrowError(
+      'Unable to find relevant lockfile parser for "/path/to/file", consider passing the --type option.'
+    )
+  })
+
+  it('using a parser with a path option for a lockfile that does not exist throws an error', () => {
+    const parser = new ParseLockfile({lockfilePath: '/path/to/file', lockfileType: 'yarn'})
+    expect(() => {
+      parser.parseSync()
+    }).toThrowError('Unable to read lockfile "/path/to/file"')
   })
 })
diff --git a/packages/lockfile-lint-api/__tests__/parseNpmLockfile.test.js b/packages/lockfile-lint-api/__tests__/parseNpmLockfile.test.js
index b9e9e17..d9b1fa6 100644
--- a/packages/lockfile-lint-api/__tests__/parseNpmLockfile.test.js
+++ b/packages/lockfile-lint-api/__tests__/parseNpmLockfile.test.js
@@ -42,4 +42,16 @@ describe('ParseLockfile Npm', () => {
       })
     )
   })
+
+  it('parsing an npm lockfile with invalid content throws an error', () => {
+    const mockNpmLockfilePath = path.join(__dirname, './__fixtures__/bad-package-lock.json')
+    const options = {
+      lockfilePath: mockNpmLockfilePath,
+      lockfileType: 'npm'
+    }
+    const parser = new ParseLockfile(options)
+    expect(() => parser.parseSync()).toThrowError(
+      `Unable to parse npm lockfile "${mockNpmLockfilePath}"`
+    )
+  })
 })
diff --git a/packages/lockfile-lint-api/__tests__/parseYarnLockfile.test.js b/packages/lockfile-lint-api/__tests__/parseYarnLockfile.test.js
index 645dab6..db253c0 100644
--- a/packages/lockfile-lint-api/__tests__/parseYarnLockfile.test.js
+++ b/packages/lockfile-lint-api/__tests__/parseYarnLockfile.test.js
@@ -23,4 +23,16 @@ describe('ParseLockfile Yarn', () => {
       })
     )
   })
+
+  it('parsing a yarn lockfile with invalid content throws an error', () => {
+    const mockYarnLockfilePath = path.join(__dirname, './__fixtures__/bad-yarn.lock')
+    const options = {
+      lockfilePath: mockYarnLockfilePath,
+      lockfileType: 'yarn'
+    }
+    const parser = new ParseLockfile(options)
+    expect(() => parser.parseSync()).toThrowError(
+      `Unable to parse yarn lockfile "${mockYarnLockfilePath}"`
+    )
+  })
 })