From 77e279a43a70011054466fd0170ebd485c248882 Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 11 Aug 2021 21:56:10 -0700 Subject: [PATCH] fix: move pathological nest fixing link to PlaceDep When a dependency graph cycles back on itself incompatibly like this: ``` a@1 -> b@1 b@1 -> a@2 a@2 -> b@2 b@2 -> a@1 ``` We would find ourselves unable to handle the conflict via nesting. For example: ``` root +-- a@1 -> b@1 +-- b@1 -> a@2 +-- a@2 -> b@2 +-- b@2 -> a@1 +-- a@1 -> b@1 +-- b@1 -> a@2 +-- a@2 -> b@2 +-- b@2 -> a@1 (cycling forever) ``` In order to address this, we create a link when such a cycle is detected. ``` root +-- a@1 -> b@1 +-- b@1 -> a@2 +-- a@2 -> b@2 +-- b@2 -> a@1 +-- a@1 -> b@1 +-- b@1 -> link to root/node_modules/b@1 ``` Prior to the recent refactor to move much of the dependency placement logic out of Arborist.buildIdealTree and into the PlaceDep class, this link was created right at the moment when a new dependency was created in a temp tree. However, if we feed that Link object into the PlaceDep flow, it will (correctly) see that the Link does not match the Node further up the tree, and attempt to replace it. Compounding the problem (and why it appeared in `npm dedupe` and not `npm install`) is the fact that explicitly named updates are _always_ treated as a "problem edge", so that they can be re-evaluated. So, rather than creating a Node to be tested against the tree, it was creating a Link object, and then attempting to replace the Link's target with the Link itself, which caused some havoc. This patch moves the loop detection and remediating Link creation into the PlaceDep class, which is the more proper place for it, as that class owns the "put deps into the tree" logic, and this is clearly a "put deps into the tree" type of situation. Via: @ParadoxInfinite Close: npm/cli#3632 Close: #308 Fix: npm/cli#3565 PR-URL: https://github.com/npm/arborist/pull/309 Credit: @isaacs Close: #309 Reviewed-by: @nlf --- lib/arborist/build-ideal-tree.js | 11 --- lib/place-dep.js | 17 +++- .../arborist/build-ideal-tree.js.test.cjs | 44 +++++---- tap-snapshots/test/place-dep.js.test.cjs | 90 +++++++++++++++++++ test/arborist/build-ideal-tree.js | 7 +- test/place-dep.js | 28 ++++++ 6 files changed, 159 insertions(+), 38 deletions(-) diff --git a/lib/arborist/build-ideal-tree.js b/lib/arborist/build-ideal-tree.js index 7ef42289d..679d52582 100644 --- a/lib/arborist/build-ideal-tree.js +++ b/lib/arborist/build-ideal-tree.js @@ -982,7 +982,6 @@ This is a one-time fix-up, please be patient... // Note that the virtual root will also have virtual copies of the // targets of any child Links, so that they resolve appropriately. const parent = parent_ || this[_virtualRoot](edge.from) - const realParent = edge.peer ? edge.from.resolveParent : edge.from const spec = npa.resolve(edge.name, edge.spec, edge.from.path) const first = await this[_nodeFromSpec](edge.name, spec, parent, edge) @@ -1013,16 +1012,6 @@ This is a one-time fix-up, please be patient... required.has(secondEdge.from) && secondEdge.type !== 'peerOptional')) required.add(node) - // handle otherwise unresolvable dependency nesting loops by - // creating a symbolic link - // a1 -> b1 -> a2 -> b2 -> a1 -> ... - // instead of nesting forever, when the loop occurs, create - // a symbolic link to the earlier instance - for (let p = edge.from.resolveParent; p; p = p.resolveParent) { - if (p.matches(node) && !p.isTop) - return new Link({ parent: realParent, target: p }) - } - // keep track of the thing that caused this node to be included. const src = parent.sourceReference this[_peerSetSource].set(node, src) diff --git a/lib/place-dep.js b/lib/place-dep.js index 913b2ba6c..c0023e74a 100644 --- a/lib/place-dep.js +++ b/lib/place-dep.js @@ -16,6 +16,7 @@ const { } = CanPlaceDep const debug = require('./debug.js') +const Link = require('./link.js') const gatherDepSet = require('./gather-dep-set.js') const peerEntrySets = require('./peer-entry-sets.js') @@ -256,6 +257,20 @@ class PlaceDep { return } + // we were told to place it here in the target, so either it does not + // already exist in the tree, OR it's shadowed. + // handle otherwise unresolvable dependency nesting loops by + // creating a symbolic link + // a1 -> b1 -> a2 -> b2 -> a1 -> ... + // instead of nesting forever, when the loop occurs, create + // a symbolic link to the earlier instance + for (let p = target; p; p = p.resolveParent) { + if (p.matches(dep) && !p.isTop) { + this.placed = new Link({ parent: target, target: p }) + return + } + } + // XXX if we are replacing SOME of a peer entry group, we will need to // remove any that are not being replaced and will now be invalid, and // re-evaluate them deeper into the tree. @@ -268,7 +283,7 @@ class PlaceDep { integrity: dep.integrity, legacyPeerDeps: this.legacyPeerDeps, error: dep.errors[0], - ...(dep.isLink ? { target: dep.target, realpath: dep.target.path } : {}), + ...(dep.isLink ? { target: dep.target, realpath: dep.realpath } : {}), }) this.oldDep = target.children.get(this.name) diff --git a/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs b/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs index 1bb295bc0..3e9558824 100644 --- a/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs +++ b/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs @@ -76495,7 +76495,7 @@ ArboristNode { } ` -exports[`test/arborist/build-ideal-tree.js TAP pathologically nested dependency cycle > expect resolving Promise 1`] = ` +exports[`test/arborist/build-ideal-tree.js TAP pathologically nested dependency cycle > must match snapshot 1`] = ` ArboristNode { "children": Map { "@isaacs/pathological-dep-nesting-a" => ArboristNode { @@ -76549,27 +76549,6 @@ ArboristNode { "@isaacs/pathological-dep-nesting-b" => ArboristNode { "children": Map { "@isaacs/pathological-dep-nesting-a" => ArboristNode { - "children": Map { - "@isaacs/pathological-dep-nesting-b" => ArboristLink { - "edgesIn": Set { - EdgeIn { - "from": "node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-a", - "name": "@isaacs/pathological-dep-nesting-b", - "spec": "1", - "type": "prod", - }, - }, - "location": "node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-a/node_modules/@isaacs/pathological-dep-nesting-b", - "name": "@isaacs/pathological-dep-nesting-b", - "path": "{CWD}/test/fixtures/pathological-dep-nesting-cycle/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-a/node_modules/@isaacs/pathological-dep-nesting-b", - "realpath": "{CWD}/test/fixtures/pathological-dep-nesting-cycle/node_modules/@isaacs/pathological-dep-nesting-b", - "resolved": "file:../../../../../../../..", - "target": ArboristNode { - "location": "node_modules/@isaacs/pathological-dep-nesting-b", - }, - "version": "1.0.0", - }, - }, "edgesIn": Set { EdgeIn { "from": "node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b", @@ -76582,7 +76561,7 @@ ArboristNode { "@isaacs/pathological-dep-nesting-b" => EdgeOut { "name": "@isaacs/pathological-dep-nesting-b", "spec": "1", - "to": "node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-a/node_modules/@isaacs/pathological-dep-nesting-b", + "to": "node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b", "type": "prod", }, }, @@ -76592,6 +76571,25 @@ ArboristNode { "resolved": "https://registry.npmjs.org/@isaacs/pathological-dep-nesting-a/-/pathological-dep-nesting-a-1.0.0.tgz", "version": "1.0.0", }, + "@isaacs/pathological-dep-nesting-b" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-a", + "name": "@isaacs/pathological-dep-nesting-b", + "spec": "1", + "type": "prod", + }, + }, + "location": "node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b", + "name": "@isaacs/pathological-dep-nesting-b", + "path": "{CWD}/test/fixtures/pathological-dep-nesting-cycle/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b", + "realpath": "{CWD}/test/fixtures/pathological-dep-nesting-cycle/node_modules/@isaacs/pathological-dep-nesting-b", + "resolved": "file:../../../../..", + "target": ArboristNode { + "location": "node_modules/@isaacs/pathological-dep-nesting-b", + }, + "version": "1.0.0", + }, }, "edgesIn": Set { EdgeIn { diff --git a/tap-snapshots/test/place-dep.js.test.cjs b/tap-snapshots/test/place-dep.js.test.cjs index 4e6053fdb..b819d4dc3 100644 --- a/tap-snapshots/test/place-dep.js.test.cjs +++ b/tap-snapshots/test/place-dep.js.test.cjs @@ -4369,6 +4369,96 @@ exports[`test/place-dep.js TAP placement tests nest peer set under dependent nod Array [] ` +exports[`test/place-dep.js TAP placement tests pathologically nested dependency cycle > changes to tree 1`] = ` +--- expected ++++ actual +@@ -118,13 +118,6 @@ + "spec": "2", + "from": "node_modules/b/node_modules/a", + }, +- EdgeIn { +- "type": "prod", +- "name": "b", +- "spec": "1", +- "error": "INVALID", +- "from": "node_modules/b/node_modules/b/node_modules/a", +- }, + }, + "children": Map { + "a" => ArboristNode { +@@ -141,8 +134,7 @@ + "type": "prod", + "name": "b", + "spec": "1", +- "to": "node_modules/b/node_modules/b", +- "error": "INVALID", ++ "to": "node_modules/b/node_modules/b/node_modules/b", + }, + }, + "edgesIn": Set { +@@ -154,6 +146,29 @@ + }, + }, + }, ++ "b" => ArboristLink { ++ "name": "b", ++ "version": "1.0.0", ++ "location": "node_modules/b/node_modules/b/node_modules/b", ++ "path": "/some/path/node_modules/b/node_modules/b/node_modules/b", ++ "realpath": "/some/path/node_modules/b", ++ "resolved": "file:../../..", ++ "extraneous": true, ++ "dev": true, ++ "optional": true, ++ "peer": true, ++ "edgesIn": Set { ++ EdgeIn { ++ "type": "prod", ++ "name": "b", ++ "spec": "1", ++ "from": "node_modules/b/node_modules/b/node_modules/a", ++ }, ++ }, ++ "target": ArboristNode { ++ "location": "node_modules/b", ++ }, ++ }, + }, + }, + }, + +` + +exports[`test/place-dep.js TAP placement tests pathologically nested dependency cycle > placements 1`] = ` +Array [ + Object { + "canPlace": Symbol(OK), + "canPlaceSelf": Symbol(OK), + "checks": Map { + "node_modules/b/node_modules/b/node_modules/a" => Array [ + Symbol(OK), + Symbol(OK), + ], + "node_modules/b/node_modules/b" => Array [ + Symbol(OK), + Symbol(OK), + ], + "node_modules/b" => Array [ + Symbol(CONFLICT), + Symbol(CONFLICT), + ], + }, + "dep": "b@1.0.0", + "edge": "{ node_modules/b/node_modules/b/node_modules/a prod b@1 }", + "placed": "node_modules/b/node_modules/b/node_modules/b", + }, +] +` + +exports[`test/place-dep.js TAP placement tests pathologically nested dependency cycle > warnings 1`] = ` +Array [] +` + exports[`test/place-dep.js TAP placement tests peer all the way down, conflict but not ours > changes to tree 1`] = ` --- expected +++ actual diff --git a/test/arborist/build-ideal-tree.js b/test/arborist/build-ideal-tree.js index cec5fc06f..d19366b1d 100644 --- a/test/arborist/build-ideal-tree.js +++ b/test/arborist/build-ideal-tree.js @@ -818,9 +818,10 @@ t.test('update global space single dep', t => { }) // if we get this wrong, it'll spin forever and use up all the memory -t.test('pathologically nested dependency cycle', t => - t.resolveMatchSnapshot(printIdeal( - resolve(fixtures, 'pathological-dep-nesting-cycle')))) +t.test('pathologically nested dependency cycle', async t => { + t.matchSnapshot(await printIdeal( + resolve(fixtures, 'pathological-dep-nesting-cycle'))) +}) t.test('resolve file deps from cwd', t => { const cwd = process.cwd() diff --git a/test/place-dep.js b/test/place-dep.js index 0ee4b279d..fc799b807 100644 --- a/test/place-dep.js +++ b/test/place-dep.js @@ -605,6 +605,34 @@ t.test('placement tests', t => { }, }) + // pathologically nested dep cycle + // a1 -> b1 -> a2 -> b2 -> a1 + runTest('pathologically nested dependency cycle', { + tree: new Node({ + path, + pkg: { dependencies: { a: '1' }}, + children: [ + { pkg: { name: 'a', version: '1.0.0', dependencies: { b: '1' }}}, + { + pkg: { name: 'b', version: '1.0.0', dependencies: { a: '2' }}, + children: [ + { pkg: { name: 'a', version: '2.0.0', dependencies: { b: '2' }}}, + { + pkg: { name: 'b', version: '2.0.0', dependencies: { a: '1' }}, + children: [ + { pkg: { name: 'a', version: '1.0.0', dependencies: { b: '1' }}}, + ], + }, + ], + }, + ], + }), + dep: new Node({ + pkg: { name: 'b', version: '1.0.0', dependencies: { a: '2' }}, + }), + nodeLoc: 'node_modules/b/node_modules/b/node_modules/a', + }) + // peer dep shenanigans runTest('basic placement of a production dep with peer deps', { tree: new Node({