Ensure correct flags on shrinkwrapped module deps

When running `Arborist.loadVirtual()` to read the tree from the point of a tree that contains a shrinkwrap, we were inappropriately setting all dep flags to `false`, as we would when reading the virtual tree in a project root. This patch updates loadVirtual to ensure that it always sets the proper flags on nodes within the subtree, if the root supplied is not actually the project root. As a result, changes had to be made to the process that inflates old/ancient lockfiles, and reify()'s handling of garbage data needed to be updated as well (since the cases being tested previously would now typically prune the invalid data out as extraneous before getting to the reify step). If a node DOES make it through to reify() in the idealTree, but lacks a version or resolved field, then something is very wrong, and so we now print a warning to the user asking them to re-try the install or delete their lockfile (which will typically fix the problem, since it either is a root dep that's being removed and will be re-added properly, or a metadep problem that can only be fixed with a full tree rebuild anyway).
npm · Feb 11, 2021 · 1bf12bb · 1bf12bb
1 parent 4f2c028
commit 1bf12bb
Show file tree

Hide file tree

Showing 13 changed files with 368 additions and 16 deletions.
diff --git a/lib/arborist/build-ideal-tree.js b/lib/arborist/build-ideal-tree.js
@@ -397,7 +397,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
     // that they're there, and not reinstall the world unnecessarily.
     if (this[_global] && (this[_updateAll] || this[_updateNames].length)) {
       const nm = resolve(this.path, 'node_modules')
-      for (const name of await readdir(nm)) {
+      for (const name of await readdir(nm).catch(() => [])) {
         tree.package.dependencies = tree.package.dependencies || {}
         if (this[_updateAll] || this[_updateNames].includes(name))
           tree.package.dependencies[name] = '*'
@@ -663,6 +663,11 @@ This is a one-time fix-up, please be patient...
       })
     }
     await promiseCallLimit(queue)
+
+    // have to re-calc dep flags, because the nodes don't have edges
+    // until their packages get assigned, so everything looks extraneous
+    calcDepFlags(this.idealTree)
+
     // yes, yes, this isn't the "original" version, but now that it's been
     // upgraded, we need to make sure we don't do the work to upgrade it
     // again, since it's now as new as can be.

diff --git a/lib/arborist/load-virtual.js b/lib/arborist/load-virtual.js
@@ -24,6 +24,7 @@ const loadWorkspacesVirtual = Symbol.for('loadWorkspacesVirtual')
 const flagsSuspect = Symbol.for('flagsSuspect')
 const reCalcDepFlags = Symbol('reCalcDepFlags')
 const checkRootEdges = Symbol('checkRootEdges')
+const rootOptionProvided = Symbol('rootOptionProvided')
 
 const depsToEdges = (type, deps) =>
   Object.entries(deps).map(d => [type, ...d])
@@ -63,6 +64,8 @@ module.exports = cls => class VirtualLoader extends cls {
       root = await this[loadRoot](s),
     } = options
 
+    this[rootOptionProvided] = options.root
+
     await this[loadFromShrinkwrap](s, root)
     return treeCheck(this.virtualTree)
   }
@@ -74,34 +77,41 @@ module.exports = cls => class VirtualLoader extends cls {
   }
 
   async [loadFromShrinkwrap] (s, root) {
-    // root is never any of these things, but might be a brand new
-    // baby Node object that never had its dep flags calculated.
-    root.extraneous = false
-    root.dev = false
-    root.optional = false
-    root.devOptional = false
-    root.peer = false
+    if (!this[rootOptionProvided]) {
+      // root is never any of these things, but might be a brand new
+      // baby Node object that never had its dep flags calculated.
+      root.extraneous = false
+      root.dev = false
+      root.optional = false
+      root.devOptional = false
+      root.peer = false
+    } else
+      this[flagsSuspect] = true
+
     this[checkRootEdges](s, root)
     root.meta = s
     this.virtualTree = root
     const {links, nodes} = this[resolveNodes](s, root)
     await this[resolveLinks](links, nodes)
     this[assignBundles](nodes)
     if (this[flagsSuspect])
-      this[reCalcDepFlags]()
+      this[reCalcDepFlags](nodes.values())
     return root
   }
 
-  [reCalcDepFlags] () {
+  [reCalcDepFlags] (nodes) {
     // reset all dep flags
-    for (const node of this.virtualTree.inventory.values()) {
+    // can't use inventory here, because virtualTree might not be root
+    for (const node of nodes) {
+      if (node.isRoot || node === this[rootOptionProvided])
+        continue
       node.extraneous = true
       node.dev = true
       node.optional = true
       node.devOptional = true
       node.peer = true
     }
-    calcDepFlags(this.virtualTree, true)
+    calcDepFlags(this.virtualTree, !this[rootOptionProvided])
   }
 
   // check the lockfile deps, and see if they match.  if they do not
@@ -237,6 +247,12 @@ module.exports = cls => class VirtualLoader extends cls {
     // shrinkwrap doesn't include package name unless necessary
     if (!sw.name)
       sw.name = nameFromFolder(path)
+
+    const dev = sw.dev
+    const optional = sw.optional
+    const devOptional = dev || optional || sw.devOptional
+    const peer = sw.peer
+
     const node = new Node({
       legacyPeerDeps: this.legacyPeerDeps,
       root: this.virtualTree,
@@ -246,6 +262,10 @@ module.exports = cls => class VirtualLoader extends cls {
       resolved: consistentResolve(sw.resolved, this.path, path),
       pkg: sw,
       hasShrinkwrap: sw.hasShrinkwrap,
+      dev,
+      optional,
+      devOptional,
+      peer,
     })
     // cast to boolean because they're undefined in the lock file when false
     node.extraneous = !!sw.extraneous

diff --git a/lib/arborist/reify.js b/lib/arborist/reify.js
@@ -442,7 +442,8 @@ module.exports = cls => class Reifier extends cls {
     if (this[_trashList].has(node.path))
       return node
 
-    process.emit('time', `reifyNode:${node.location}`)
+    const timer = `reifyNode:${node.location}`
+    process.emit('time', timer)
     this.addTracker('reify', node.name, node.location)
 
     const p = Promise.resolve()
@@ -454,7 +455,7 @@ module.exports = cls => class Reifier extends cls {
     return this[_handleOptionalFailure](node, p)
       .then(() => {
         this.finishTracker('reify', node.name, node.location)
-        process.emit('timeEnd', `reifyNode:${node.location}`)
+        process.emit('timeEnd', timer)
         return node
       })
   }
@@ -474,9 +475,14 @@ module.exports = cls => class Reifier extends cls {
 
     // no idea what this thing is.  remove it from the tree.
     if (!res) {
-      node.parent = null
+      const warning = 'invalid or damaged lockfile detected\n' +
+        'please re-try this operation once it completes\n' +
+        'so that the damage can be corrected, or perform\n' +
+        'a fresh install with no lockfile if the problem persists.'
+      this.log.warn('reify', warning)
       this.log.verbose('reify', 'unrecognized node in tree', node.path)
       node.parent = null
+      node.fsParent = null
       this[_addNodeToTrashList](node)
       return
     }

diff --git a/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js b/tap-snapshots/test-arborist-build-ideal-tree.js-TAP.test.js
@@ -11530,6 +11530,7 @@ ArboristNode {
       "location": "node_modules/@isaacs/peer-dep-cycle-b",
       "name": "@isaacs/peer-dep-cycle-b",
       "path": "{CWD}/test/fixtures/peer-dep-cycle-with-sw/node_modules/@isaacs/peer-dep-cycle-b",
+      "peer": true,
       "resolved": "https://registry.npmjs.org/@isaacs/peer-dep-cycle-b/-/peer-dep-cycle-b-1.0.0.tgz",
       "version": "1.0.0",
     },
@@ -11553,6 +11554,7 @@ ArboristNode {
       "location": "node_modules/@isaacs/peer-dep-cycle-c",
       "name": "@isaacs/peer-dep-cycle-c",
       "path": "{CWD}/test/fixtures/peer-dep-cycle-with-sw/node_modules/@isaacs/peer-dep-cycle-c",
+      "peer": true,
       "resolved": "https://registry.npmjs.org/@isaacs/peer-dep-cycle-c/-/peer-dep-cycle-c-1.0.0.tgz",
       "version": "1.0.0",
     },
@@ -24971,6 +24973,7 @@ ArboristNode {
               },
               "location": "node_modules/tap/node_modules/nyc/node_modules/align-text",
               "name": "align-text",
+              "optional": true,
               "path": "{CWD}/test/fixtures/sax/node_modules/tap/node_modules/nyc/node_modules/align-text",
               "version": "0.1.4",
             },
@@ -27847,6 +27850,7 @@ ArboristNode {
               },
               "location": "node_modules/tap/node_modules/nyc/node_modules/longest",
               "name": "longest",
+              "optional": true,
               "path": "{CWD}/test/fixtures/sax/node_modules/tap/node_modules/nyc/node_modules/longest",
               "version": "1.0.1",
             },
@@ -69947,6 +69951,7 @@ ArboristNode {
       "location": "node_modules/@isaacs/peer-dep-cycle-b",
       "name": "@isaacs/peer-dep-cycle-b",
       "path": "{CWD}/test/fixtures/peer-dep-cycle-nested-with-sw/node_modules/@isaacs/peer-dep-cycle-b",
+      "peer": true,
       "resolved": "https://registry.npmjs.org/@isaacs/peer-dep-cycle-b/-/peer-dep-cycle-b-1.0.0.tgz",
       "version": "1.0.0",
     },
@@ -69970,6 +69975,7 @@ ArboristNode {
       "location": "node_modules/@isaacs/peer-dep-cycle-c",
       "name": "@isaacs/peer-dep-cycle-c",
       "path": "{CWD}/test/fixtures/peer-dep-cycle-nested-with-sw/node_modules/@isaacs/peer-dep-cycle-c",
+      "peer": true,
       "resolved": "https://registry.npmjs.org/@isaacs/peer-dep-cycle-c/-/peer-dep-cycle-c-1.0.0.tgz",
       "version": "1.0.0",
     },
@@ -127712,6 +127718,7 @@ ArboristNode {
       "location": "node_modules/@isaacs/testing-peer-deps-b",
       "name": "@isaacs/testing-peer-deps-b",
       "path": "{CWD}/test/fixtures/testing-peer-deps-nested/node_modules/@isaacs/testing-peer-deps-b",
+      "peer": true,
       "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-deps-b/-/testing-peer-deps-b-1.2.4.tgz",
       "version": "1.2.4",
     },
@@ -127727,6 +127734,7 @@ ArboristNode {
       "location": "node_modules/@isaacs/testing-peer-deps-c",
       "name": "@isaacs/testing-peer-deps-c",
       "path": "{CWD}/test/fixtures/testing-peer-deps-nested/node_modules/@isaacs/testing-peer-deps-c",
+      "peer": true,
       "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-deps-c/-/testing-peer-deps-c-1.2.3.tgz",
       "version": "1.2.3",
     },
@@ -127775,6 +127783,7 @@ ArboristNode {
           "location": "node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-b",
           "name": "@isaacs/testing-peer-deps-b",
           "path": "{CWD}/test/fixtures/testing-peer-deps-nested/node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-b",
+          "peer": true,
           "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-deps-b/-/testing-peer-deps-b-2.0.1.tgz",
           "version": "2.0.1",
         },
@@ -127790,6 +127799,7 @@ ArboristNode {
           "location": "node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-c",
           "name": "@isaacs/testing-peer-deps-c",
           "path": "{CWD}/test/fixtures/testing-peer-deps-nested/node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-c",
+          "peer": true,
           "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-deps-c/-/testing-peer-deps-c-2.0.0.tgz",
           "version": "2.0.0",
         },
@@ -128782,6 +128792,22 @@ ArboristNode {
 }
 `
 
+exports[`test/arborist/build-ideal-tree.js TAP update global when nothing in global > update with empty node_modules 1`] = `
+ArboristNode {
+  "location": "",
+  "name": "empty_nm",
+  "path": "{CWD}/test/arborist/build-ideal-tree-update-global-when-nothing-in-global/empty_nm",
+}
+`
+
+exports[`test/arborist/build-ideal-tree.js TAP update global when nothing in global > update without node_modules 1`] = `
+ArboristNode {
+  "location": "",
+  "name": "no_nm",
+  "path": "{CWD}/test/arborist/build-ideal-tree-update-global-when-nothing-in-global/no_nm",
+}
+`
+
 exports[`test/arborist/build-ideal-tree.js TAP update mkdirp to non-minimist-using version > must match snapshot 1`] = `
 ArboristNode {
   "children": Map {

diff --git a/tap-snapshots/test-arborist-reify.js-TAP.test.js b/tap-snapshots/test-arborist-reify.js-TAP.test.js
@@ -29604,6 +29604,7 @@ ArboristNode {
       "location": "node_modules/@isaacs/testing-peer-deps-b",
       "name": "@isaacs/testing-peer-deps-b",
       "path": "{CWD}/test/arborist/reify-testing-peer-deps-nested/node_modules/@isaacs/testing-peer-deps-b",
+      "peer": true,
       "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-deps-b/-/testing-peer-deps-b-1.2.4.tgz",
       "version": "1.2.4",
     },
@@ -29619,6 +29620,7 @@ ArboristNode {
       "location": "node_modules/@isaacs/testing-peer-deps-c",
       "name": "@isaacs/testing-peer-deps-c",
       "path": "{CWD}/test/arborist/reify-testing-peer-deps-nested/node_modules/@isaacs/testing-peer-deps-c",
+      "peer": true,
       "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-deps-c/-/testing-peer-deps-c-1.2.3.tgz",
       "version": "1.2.3",
     },
@@ -29667,6 +29669,7 @@ ArboristNode {
           "location": "node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-b",
           "name": "@isaacs/testing-peer-deps-b",
           "path": "{CWD}/test/arborist/reify-testing-peer-deps-nested/node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-b",
+          "peer": true,
           "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-deps-b/-/testing-peer-deps-b-2.0.1.tgz",
           "version": "2.0.1",
         },
@@ -29682,6 +29685,7 @@ ArboristNode {
           "location": "node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-c",
           "name": "@isaacs/testing-peer-deps-c",
           "path": "{CWD}/test/arborist/reify-testing-peer-deps-nested/node_modules/@isaacs/testing-peer-deps-d/node_modules/@isaacs/testing-peer-deps-c",
+          "peer": true,
           "resolved": "https://registry.npmjs.org/@isaacs/testing-peer-deps-c/-/testing-peer-deps-c-2.0.0.tgz",
           "version": "2.0.0",
         },
@@ -42960,6 +42964,50 @@ exports[`test/arborist/reify.js TAP update a yarn.lock file > updated yarn lock
 
 `
 
+exports[`test/arborist/reify.js TAP warn and correct if damaged data in lockfile first pass logs > "fixed" lockfile 1`] = `
+{
+  "name": "reify-warn-and-correct-if-damaged-data-in-lockfile",
+  "lockfileVersion": 2,
+  "requires": true,
+  "packages": {
+    "": {
+      "dependencies": {
+        "abbrev": ""
+      }
+    }
+  }
+}
+
+`
+
+exports[`test/arborist/reify.js TAP warn and correct if damaged data in lockfile second pass just does the right thing > actually fixed lockfile 1`] = `
+{
+  "name": "reify-warn-and-correct-if-damaged-data-in-lockfile",
+  "lockfileVersion": 2,
+  "requires": true,
+  "packages": {
+    "": {
+      "dependencies": {
+        "abbrev": ""
+      }
+    },
+    "node_modules/abbrev": {
+      "version": "1.1.1",
+      "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
+      "integrity": "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q=="
+    }
+  },
+  "dependencies": {
+    "abbrev": {
+      "version": "1.1.1",
+      "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
+      "integrity": "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q=="
+    }
+  }
+}
+
+`
+
 exports[`test/arborist/reify.js TAP weirdly broken lockfile without resolved value > expect resolving Promise 1`] = `
 ArboristNode {
   "children": Map {