fix: correct parser nested package in workspaces

fernandoalex commented 10 months ago

[x] Tests written and linted
[ ] Documentation written / README.md updated https://snyk.io/docs/snyk-for-node/
[x] Follows CONTRIBUTING agreement
[ ] Commit history is tidy https://git-scm.com/book/en/v2/Git-Branching-Rebasing
[ ] Reviewed by Snyk team

What this does

Looks like the parser is not working as expected for package-locks with "workspaces" with a nested package objects, this fixes this problem.

❯ snyk test
workspacesDeclaration.map is not a function

❯ snyk test -d
...
snyk test <ref *1> { _: [ [Circular *1] ], debug: true } +0ms
  snyk-test Analyzing npm dependencies for .../package-lock.json +0ms
  snyk:run-test Error running test {
  error: TypeError: workspacesDeclaration.map is not a function
      at resolvedToWorkspace (/snapshot/project/dist/cli/webpack:/snyk/node_modules/snyk-nodejs-lockfile-parser/dist/dep-graph-builders/npm-lock-v2/index.js:111:1)
      at getChildNode (/snapshot/project/dist/cli/webpack:/snyk/node_modules/snyk-nodejs-lockfile-parser/dist/dep-graph-builders/npm-lock-v2/index.js:117:1)
      at dfsVisit (/snapshot/project/dist/cli/webpack:/snyk/node_modules/snyk-nodejs-lockfile-parser/dist/dep-graph-builders/npm-lock-v2/index.js:66:1)
      at dfsVisit (/snapshot/project/dist/cli/webpack:/snyk/node_modules/snyk-nodejs-lockfile-parser/dist/dep-graph-builders/npm-lock-v2/index.js:76:1)
      at dfsVisit (/snapshot/project/dist/cli/webpack:/snyk/node_modules/snyk-nodejs-lockfile-parser/dist/dep-graph-builders/npm-lock-v2/index.js:76:1)
      at buildDepGraphNpmLockV2 (/snapshot/project/dist/cli/webpack:/snyk/node_modules/snyk-nodejs-lockfile-parser/dist/dep-graph-builders/npm-lock-v2/index.js:56:1)
      at Object.parseNpmLockV2Project (/snapshot/project/dist/cli/webpack:/snyk/node_modules/snyk-nodejs-lockfile-parser/dist/dep-graph-builders/npm-lock-v2/index.js:19:1)
      at buildDepGraph (/snapshot/project/dist/cli/webpack:/snyk/src/lib/plugins/nodejs-plugin/npm-lock-parser.ts:146:14)
      at Object.parse (/snapshot/project/dist/cli/webpack:/snyk/src/lib/plugins/nodejs-plugin/npm-lock-parser.ts:67:12)
      at Object.inspect (/snapshot/project/dist/cli/webpack:/snyk/src/lib/plugins/nodejs-plugin/index.ts:30:7)
      at Object.inspect (/snapshot/project/dist/cli/webpack:/snyk/src/lib/module-info/index.ts:23:20)
      at Object.getSinglePluginResult (/snapshot/project/dist/cli/webpack:/snyk/src/lib/plugins/get-single-plugin-result.ts:14:47)
      at Object.getDepsFromPlugin (/snapshot/project/dist/cli/webpack:/snyk/src/lib/plugins/get-deps-from-plugin.ts:101:22)
      at assembleLocalPayloads (/snapshot/project/dist/cli/webpack:/snyk/src/lib/snyk-test/run-test.ts:594:18)
      at runTest (/snapshot/project/dist/cli/webpack:/snyk/src/lib/snyk-test/run-test.ts:337:22)
      at test (/snapshot/project/dist/cli/webpack:/snyk/src/cli/commands/test/index.ts:155:13)
      at runCommand (/snapshot/project/dist/cli/webpack:/snyk/src/cli/main.ts:51:25)
      at main (/snapshot/project/dist/cli/webpack:/snyk/src/cli/main.ts:310:11)
      at /snapshot/project/dist/cli/webpack:/snyk/src/cli/index.ts:13:3
      at Object.callHandlingUnexpectedErrors (/snapshot/project/dist/cli/webpack:/snyk/src/lib/unexpected-error.ts:28:5)
...

Notes for the reviewer

removing the change and running

npx test

should show it failing.

More information

Mostly an example for the snyk team about the problem, if it is faster to just implement a fix yourselves please feel free to ignore this PR and close it.

Screenshots

Visuals that may help the reviewer

CLAassistant commented 10 months ago

All committers have signed the CLA.

JamesPatrickGill commented 10 months ago

Hi @fernandoalex 👋 , thanks so much for your change and the test case! Looks good to me. However I do have a question, I am trying to understand how to generate the lockfile with a workspaces format like that found in your test case:

...
     "workspaces": {
        "packages": [ 
          "packages/a",
          "packages/b"
        ]
      },
...

Can you explain how you generated this on your side? I have tried a combination of node and npm versions generating both v2 lockfiles and v3 lockfiles and none of them had this nested packages array. I'm certain I'm just missing something straight forward.

Now for formalities :) If and when we are happy to merge I will ask you to point at a non-main branch inside our repository so that we can run our CI against your change. Your commit will then be merged upstream.

Thanks again 🙏

fernandoalex commented 10 months ago

@JamesPatrickGill thanks for the response. TBH I am not that familiar with npm, I noticed this when a pipeline broke for our developers. Will have a chat with them and will updated here.

Also sorry for the wrong PR (poiting to main branch) looks like I should have read the docs with a bit more care!

I will update the info in here as soon as I get the response from my developer friends, and will correct the PR.

Thanks,

JamesPatrickGill commented 10 months ago

Don't worry it's our problem, I think it is our process that isn't well fleshed out for external contribution.

Once we are happy here I will open an empty branch for you to merge into and that will run the CI and then I will upstream your commit.

Again thanks for this

JamesPatrickGill commented 10 months ago

I decided to make the empty branch now, feel free to point to this branch fix/correct-parser-for-npm-v2-workspaces in the main repo.

fernandoalex commented 10 months ago

Hi @JamesPatrickGill will start by saying that we have unblocked ourselves by just manually removing the the nested packages from workspaces in the package.json file. (In case someone else in the future see this)

I found out a way for the package-lock to be generated with the nested packages.

After the package.json file is created, we add

"workspaces": {
  "packages": [
    "packages/*"
  ]

Instead of a simple array.

After that npm install will generate the package-lock with that nested packages. TBH I really don't know how we got into this situtation, as the only way I manage the replicate the behaviour was manually changing the package file.

I got really confused on why this was working for us (I mean npm), so I did a quick search and having that nested packages: [] is supported (if I and reading the code https://github.com/npm/map-workspaces/blob/ff82968a3dbb78659fb7febfce4841bf58c514de/lib/index.js#L27 ) correctly.

So do you think there is still value in getting this fix in? If you think so I am happy to clean this up a little bit (maybe adding a bit more context) and to continue with the process to merge. Tho I will probably only look into this again over the weekend.

JamesPatrickGill commented 10 months ago

Hmm, how interesting, I was unable to find this anywhere in the docs also so was skeptical but the function you have linked is an amazing find. And I believe the change you have made replicates that function in our parsing logic.

So yes I believe this is still a valuable addition. When you are active here, please point this PR to the branch fix/correct-parser-for-npm-v2-workspaces and I should be able to manage the rest. P.S. any additional context will be much appreciated 😄

Thanks for digging into this and glad you've been unblocked. 🙏

fernandoalex commented 10 months ago

Hello @JamesPatrickGill I found some time today and made the needed changes, added a comment with some context and changed the tests to reflect that this is not only a package v2 thing as I previously thought, I also enable the other tests to be sure I was not breaking something else.

Let me know if anything else is needed.

Thanks!

JamesPatrickGill commented 10 months ago

Currently cleaning up some prettier things here and squashed the commits together. Your contributor status will remain 😁 https://github.com/snyk/nodejs-lockfile-parser/pull/204

snyk / nodejs-lockfile-parser