fix: error handling for empty yarn lock files (#158)

candrews commented 1 year ago

Description

fix error handling for empty yarn lock files (#158)

Currently, running any validator against an empty lock file results in a exception with a stack trace, like this:

$ touch yarn.lock && npx lockfile-lint@4.10.1 --path yarn.lock --type yarn --allowed-hosts yarn npm yarn
 ℹ ABORTING lockfile lint process due to error exceptions 

Unable to parse yarn lockfile "yarn.lock" 

TypeError: undefined is not iterable (cannot read property Symbol(Symbol.iterator))
    at checkSampleContent (/home/candrews/.npm/_npx/456ed9a151c5043a/node_modules/lockfile-lint-api/src/ParseLockfile.js:24:36)
    at yarnParseAndVerify (/home/candrews/.npm/_npx/456ed9a151c5043a/node_modules/lockfile-lint-api/src/ParseLockfile.js:41:49)
    at ParseLockfile.parseYarnLockfile (/home/candrews/.npm/_npx/456ed9a151c5043a/node_modules/lockfile-lint-api/src/ParseLockfile.js:160:20)
    at ParseLockfile.parseSync (/home/candrews/.npm/_npx/456ed9a151c5043a/node_modules/lockfile-lint-api/src/ParseLockfile.js:122:27)
    at ValidateHostManager (/home/candrews/.npm/_npx/456ed9a151c5043a/node_modules/lockfile-lint/src/validators/index.js:49:27)
    at /home/candrews/.npm/_npx/456ed9a151c5043a/node_modules/lockfile-lint/src/main.js:41:28
    at Array.forEach (<anonymous>)
    at Object.runValidators (/home/candrews/.npm/_npx/456ed9a151c5043a/node_modules/lockfile-lint/src/main.js:31:14)
    at Object.<anonymous> (/home/candrews/.npm/_npx/456ed9a151c5043a/node_modules/lockfile-lint/bin/lockfile-lint.js:80:17)
    at Module._compile (node:internal/modules/cjs/loader:1254:14) 

/home/candrews/.npm/_npx/456ed9a151c5043a/node_modules/lockfile-lint/bin/lockfile-lint.js:89
  error('Error: command failed with exit code 1')
  ^

TypeError: error is not a function
    at Object.<anonymous> (/home/candrews/.npm/_npx/456ed9a151c5043a/node_modules/lockfile-lint/bin/lockfile-lint.js:89:3)
    at Module._compile (node:internal/modules/cjs/loader:1254:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1308:10)
    at Module.load (node:internal/modules/cjs/loader:1117:32)
    at Module._load (node:internal/modules/cjs/loader:958:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:23:47

Node.js v18.16.0
[1]

This PR fixes that issue by adding validation to the validators.

Types of changes

[X] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Motivation and Context

Empty lock files are not handled appropriately - this MR fixes that issue.

How Has This Been Tested?

I've added two new unit tests (one for yarn, one for npm).

Screenshots (if appropriate):

n/a

Checklist:

[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

lirantal commented 1 year ago

Thanks in advance! I'm currently traveling but will do my best to get to this in a few days!

lirantal commented 1 year ago

@candrews I'm not entirely sure I get the expected result of this PR/general capability here. Do you want to "fail gracefully" or do you want to introduce a change that empty lockfiles are valid? The test suite names are a bit confusing to me with how I read this original PR's intent :)

candrews commented 1 year ago

Do you want to "fail gracefully" or do you want to introduce a change that empty lockfiles are valid?

npm and yarn both consider empty lock files to be invalid, so I think that failing with an error indicating that the lockfile is invalid makes the most sense.

The test suite names are a bit confusing to me with how I read this original PR's intent :)

The names of the tests that I've added in this PR are:

should fail with an empty yarn lock file
should fail with an empty npm lock file

I think those are reasonably decent names... if you have a suggestion for improving them, I'm happy to implement it #namingthingsishard

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 :tada:

Comparison is base (969ed05) 97.74% compared to head (d386047) 97.75%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #159 +/- ## ========================================== + Coverage 97.74% 97.75% +0.01% ========================================== Files 13 13 Lines 354 356 +2 Branches 77 78 +1 ========================================== + Hits 346 348 +2 Misses 8 8 ``` | [Impacted Files](https://app.codecov.io/gh/lirantal/lockfile-lint/pull/159?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Liran+Tal) | Coverage Δ | | |---|---|---| | [packages/lockfile-lint-api/src/ParseLockfile.js](https://app.codecov.io/gh/lirantal/lockfile-lint/pull/159?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Liran+Tal#diff-cGFja2FnZXMvbG9ja2ZpbGUtbGludC1hcGkvc3JjL1BhcnNlTG9ja2ZpbGUuanM=) | `98.91% <100.00%> (+0.02%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

lirantal commented 1 year ago

Are you sure you've actually tested that?

The tests pass because you're asserting on a string that is part of the error, but the error still throws with a full stack trace. Is this what you expect?

lirantal commented 1 year ago

@candrews making sure you see my above comment

candrews commented 1 year ago

Are you sure you've actually tested that? ![image](https://user-images.githubusercontent.com/316371

Yes, I've run similar command as you've run, and now I've even run the same one: node packages/lockfile-lint/bin/lockfile-lint.js --type yarn --path packages/lockfile-lint/__tests__/fixtures/empty.json --allowed-hosts npm I've also run the test suite.

The tests pass because you're asserting on a string that is part of the error, but the error still throws with a full stack trace. Is this what you expect?

It is what I expected. If you pass a lock file that is invalid (like a file containing just the { character) you also get an error with a stack trace. Therefore, the behavior implemented by this PR is consistent with the existing similar cases.

I'm happy to implement a different approach, though - tell me what you want and I'll do my best to make it happen :)

lirantal commented 1 year ago

Ok then, let's land this :-)

candrews commented 1 year ago

Thank you very much!

lirantal commented 1 year ago

Thank you!

lirantal / lockfile-lint