From c99d6d3a19fbb6c197b449dfd6cb8acc48837dba Mon Sep 17 00:00:00 2001
From: Merlin Beutlberger <m.beutlberger@sap.com>
Date: Tue, 18 Dec 2018 15:17:09 +0100
Subject: [PATCH] [FIX] npm translator: Fix handling of indirect dependency
 cycles

---
 lib/translators/npm.js      | 128 ++++++------
 test/lib/translators/npm.js | 388 ++++++++++++++++++++++++++++++++----
 2 files changed, 422 insertions(+), 94 deletions(-)

diff --git a/lib/translators/npm.js b/lib/translators/npm.js
index 58e5322eb..5bf7fc335 100644
--- a/lib/translators/npm.js
+++ b/lib/translators/npm.js
@@ -106,7 +106,11 @@ class NpmTranslator {
 					const pendingModules = Object.keys(this.debugUnresolvedProjects).map((key) => {
 						return this.debugUnresolvedProjects[key].moduleName;
 					});
-					log.silly(`${pendingModules.length} resolutions left: ${pendingModules.join(", ")}`);
+					if (pendingModules.length) {
+						log.silly(`${pendingModules.length} resolutions left: ${pendingModules.join(", ")}`);
+					} else {
+						log.silly("All modules resolved.");
+					}
 				}
 
 				return [{
@@ -183,69 +187,85 @@ class NpmTranslator {
 		});
 	}
 
-	readProject({modulePath, moduleName, parentPath}) {
-		if (this.projectCache[modulePath]) {
-			const cache = this.projectCache[modulePath];
-			// Check whether modules has already been processed in the current subtree (indicates a loop)
-			if (parentPath.indexOf(`:${moduleName}:`) !== -1) {
-				log.verbose(`Deduping project ${moduleName} with parent path ${parentPath}`);
-				// This is a loop => abort further processing
-				if (this.includeDeduped) {
-					// Add module marked as deduped
-					return cache.pPkg.then((pkg) => {
-						return {
-							id: moduleName,
-							version: pkg.version,
-							path: modulePath,
-							dependencies: [],
-							deduped: true
-						};
-					});
-				} else {
-					// Ignore this dependency
-					return Promise.resolve(null);
-				}
+	async readProject({modulePath, moduleName, parentPath}) {
+		let {pPkg} = this.projectCache[modulePath] || {};
+		if (!pPkg) {
+			pPkg = readPkg(modulePath).catch((err) => {
+				// Failed to read package
+				// If dependency shim is available, fake the package
+
+				/* Disabled shimming until shim-plugin is available
+				const id = path.basename(modulePath);
+				if (pkgDependenciesShims[id]) {
+					const dependencies = JSON.parse(JSON.stringify(pkgDependenciesShims[id]));
+					return { // Fake package.json content
+						name: id,
+						dependencies,
+						version: "",
+						ui5: {
+							dependencies: Object.keys(dependencies)
+						}
+					};
+				}*/
+				throw err;
+			});
+			this.projectCache[modulePath] = {
+				pPkg
+			};
+		}
+
+		// Check whether module has already been processed in the current subtree (indicates a loop)
+		if (parentPath.indexOf(`:${moduleName}:`) !== -1) {
+			log.verbose(`Deduping project ${moduleName} with parent path ${parentPath}`);
+			// This is a loop => abort further processing
+			if (!this.includeDeduped) {
+				// Ignore this dependency
+				return null;
 			} else {
-				if (log.isLevelEnabled("silly")) {
-					log.silly(
-						`${parentPath.match(/([^:]*):$/)[1]} is waiting for ${moduleName} (waiting via cache 🗄 )...`);
-				}
-				return cache.pProject;
+				// Add module marked as deduped
+				const pkg = await pPkg;
+
+				return {
+					id: moduleName,
+					version: pkg.version,
+					path: modulePath,
+					dependencies: [],
+					deduped: true
+				};
 			}
 		}
+
+		// Check whether project has already been processed
+		// Note: We can only cache already *processed* projects, not the promise waiting for the processing to complete
+		//	Otherwise cyclic dependencies might wait for each other, emptying the event loop
+		// Note 2: Currently caching can't be used at all. If a cached dependency has an indirect dependency to the
+		//	requesting module, a circular reference would be created
+		/*
+		if (cachedProject) {
+			if (log.isLevelEnabled("silly")) {
+				log.silly(`${parentPath.match(/([^:]*):$/)[1]} retrieved already ` +
+					`resolved project ${moduleName} from cache 🗄 `);
+			}
+			return cachedProject;
+		}*/
 		if (log.isLevelEnabled("silly")) {
 			log.silly(`${parentPath.match(/([^:]*):$/)[1]} is waiting for ${moduleName}...`);
 		}
 
-		const pPkg = readPkg(modulePath).catch((err) => {
-			// Failed to read package
-			// If dependency shim is available, fake the package
-
-			/* Disabled shimming until shim-plugin is available
-			const id = path.basename(modulePath);
-			if (pkgDependenciesShims[id]) {
-				const dependencies = JSON.parse(JSON.stringify(pkgDependenciesShims[id]));
-				return { // Fake package.json content
-					name: id,
-					dependencies,
-					version: "",
-					ui5: {
-						dependencies: Object.keys(dependencies)
-					}
-				};
-			}*/
-			throw err;
-		});
-
-		const pProject = pPkg.then((pkg) => {
+		return pPkg.then((pkg) => {
 			return this.processPkg({
 				name: moduleName,
-				pkg: pkg,
+				pkg,
 				path: modulePath
 			}, parentPath).then((projects) => {
 				// Flatten the array of project arrays (yes, because collections)
 				return Array.prototype.concat.apply([], projects.filter((p) => p !== null));
-			});
+			})/*
+			// Currently no project caching, see above
+			.then((projects) => {
+				this.projectCache[modulePath].cachedProject = projects;
+				return projects;
+			})*/;
 		}, () => {
 			// Failed to read package. Create a project anyway
 			return [{
@@ -255,12 +275,6 @@ class NpmTranslator {
 				dependencies: []
 			}];
 		});
-
-		this.projectCache[modulePath] = {
-			pPkg,
-			pProject
-		};
-		return pProject;
 	}
 
 	/* Returns path to a module
diff --git a/test/lib/translators/npm.js b/test/lib/translators/npm.js
index 906042215..031ad4e02 100644
--- a/test/lib/translators/npm.js
+++ b/test/lib/translators/npm.js
@@ -56,7 +56,7 @@ test("AppG: project with npm 'optionalDependencies' should not fail if optional
 	});
 });
 
-test("AppCycle: cyclic dev deps", (t) => {
+test("AppCycleA: cyclic dev deps", (t) => {
 	const applicationCycleAPath = path.join(cycleDepsBasePath, "application.cycle.a");
 
 	return npmTranslator.generateDependencyTree(applicationCycleAPath, {includeDeduped: false}).then((parsedTree) => {
@@ -64,7 +64,7 @@ test("AppCycle: cyclic dev deps", (t) => {
 	});
 });
 
-test("AppCycle: cyclic dev deps - include deduped", (t) => {
+test("AppCycleA: cyclic dev deps - include deduped", (t) => {
 	const applicationCycleAPath = path.join(cycleDepsBasePath, "application.cycle.a");
 
 	return npmTranslator.generateDependencyTree(applicationCycleAPath, {includeDeduped: true}).then((parsedTree) => {
@@ -72,56 +72,28 @@ test("AppCycle: cyclic dev deps - include deduped", (t) => {
 	});
 });
 
-test("AppCycle: cyclic npm deps - Cycle via devDependency on second level", (t) => {
-	const cycleDepsBasePath = path.join(__dirname, "..", "..", "fixtures", "cyclic-deps", "node_modules");
+test("AppCycleB: cyclic npm deps - Cycle via devDependency on second level", (t) => {
 	const applicationCycleBPath = path.join(cycleDepsBasePath, "application.cycle.b");
-
-	const applicationCycleB = {
-		id: "application.cycle.b",
-		version: "1.0.0",
-		path: applicationCycleBPath,
-		dependencies: []
-	};
-
-	const moduleD = {
-		id: "module.d",
-		version: "1.0.0",
-		path: path.join(cycleDepsBasePath, "module.d"),
-		dependencies: []
-	};
-
-	const moduleE = {
-		id: "module.e",
-		version: "1.0.0",
-		path: path.join(cycleDepsBasePath, "module.e"),
-		dependencies: []
-	};
-
-	applicationCycleB.dependencies.push(moduleD, moduleE);
-	moduleD.dependencies.push(moduleE);
-	moduleE.dependencies.push(moduleD);
-
-	const applicationCycleTree = applicationCycleB;
 	return npmTranslator.generateDependencyTree(applicationCycleBPath).then((parsedTree) => {
-		t.deepEqual(parsedTree, applicationCycleTree, "Parsed correctly");
+		t.deepEqual(parsedTree, applicationCycleBTree, "Parsed correctly");
 	});
 });
 
-test("AppCycle: cyclic npm deps - Cycle on third level (one indirection)", (t) => {
+test("AppCycleC: cyclic npm deps - Cycle on third level (one indirection)", (t) => {
 	const applicationCycleCPath = path.join(cycleDepsBasePath, "application.cycle.c");
 	return npmTranslator.generateDependencyTree(applicationCycleCPath, {includeDeduped: false}).then((parsedTree) => {
 		t.deepEqual(parsedTree, applicationCycleCTree, "Parsed correctly");
 	});
 });
 
-test("AppCycle: cyclic npm deps - Cycle on third level (one indirection) - include deduped", (t) => {
+test("AppCycleC: cyclic npm deps - Cycle on third level (one indirection) - include deduped", (t) => {
 	const applicationCycleCPath = path.join(cycleDepsBasePath, "application.cycle.c");
 	return npmTranslator.generateDependencyTree(applicationCycleCPath, {includeDeduped: true}).then((parsedTree) => {
 		t.deepEqual(parsedTree, applicationCycleCTreeIncDeduped, "Parsed correctly");
 	});
 });
 
-test("AppCycle: cyclic npm deps - Cycles everywhere", (t) => {
+test("AppCycleD: cyclic npm deps - Cycles everywhere", (t) => {
 	const applicationCycleDPath = path.join(cycleDepsBasePath, "application.cycle.d");
 	return npmTranslator.generateDependencyTree(applicationCycleDPath, {includeDeduped: true}).then((parsedTree) => {
 		t.deepEqual(parsedTree, applicationCycleDTree, "Parsed correctly");
@@ -397,6 +369,47 @@ const applicationCycleATreeIncDeduped = {
 	]
 };
 
+const applicationCycleBTree = {
+	"id": "application.cycle.b",
+	"version": "1.0.0",
+	"path": path.join(cycleDepsBasePath, "application.cycle.b"),
+	"dependencies": [
+		{
+			"id": "module.d",
+			"version": "1.0.0",
+			"path": path.join(cycleDepsBasePath, "module.d"),
+			"dependencies": [
+				{
+					"id": "module.e",
+					"version": "1.0.0",
+					"path": path.join(cycleDepsBasePath, "module.e"),
+					"dependencies": []
+				}
+			]
+		},
+		{
+			"id": "module.e",
+			"version": "1.0.0",
+			"path": path.join(cycleDepsBasePath, "module.e"),
+			"dependencies": [
+				{
+					"id": "module.d",
+					"version": "1.0.0",
+					"path": path.join(cycleDepsBasePath, "module.d"),
+					"dependencies": [
+						{
+							"id": "module.e",
+							"version": "1.0.0",
+							"path": path.join(cycleDepsBasePath, "module.e"),
+							"dependencies": []
+						}
+					]
+				}
+			]
+		}
+	]
+};
+
 const applicationCycleCTree = {
 	"id": "application.cycle.c",
 	"version": "1.0.0",
@@ -500,7 +513,8 @@ const applicationCycleCTreeIncDeduped = {
 		},
 		{
 			"id": "module.g",
-			"version": "1.0.0", "path": path.join(cycleDepsBasePath, "module.g"),
+			"version": "1.0.0",
+			"path": path.join(cycleDepsBasePath, "module.g"),
 			"dependencies": [
 				{
 					"id": "module.a",
@@ -535,4 +549,304 @@ const applicationCycleCTreeIncDeduped = {
 	]
 };
 
-const applicationCycleDTree = {};
+const applicationCycleDTree = {
+	"id": "application.cycle.d",
+	"version": "1.0.0",
+	"path": path.join(cycleDepsBasePath, "application.cycle.d"),
+	"dependencies": [
+		{
+			"id": "module.h",
+			"version": "1.0.0",
+			"path": path.join(cycleDepsBasePath, "module.h"),
+			"dependencies": [
+				{
+					"id": "module.i",
+					"version": "1.0.0",
+					"path": path.join(cycleDepsBasePath, "module.i"),
+					"dependencies": [
+						{
+							"id": "module.k",
+							"version": "1.0.0",
+							"path": path.join(cycleDepsBasePath, "module.k"),
+							"dependencies": [
+								{
+									"id": "module.h",
+									"version": "1.0.0",
+									"path": path.join(cycleDepsBasePath, "module.h"),
+									"dependencies": [],
+									"deduped": true
+								},
+								{
+									"id": "module.i",
+									"version": "1.0.0",
+									"path": path.join(cycleDepsBasePath, "module.i"),
+									"dependencies": [],
+									"deduped": true
+								},
+								{
+									"id": "module.j",
+									"version": "1.0.0",
+									"path": path.join(cycleDepsBasePath, "module.j"),
+									"dependencies": [
+										{
+											"id": "module.i",
+											"version": "1.0.0",
+											"path": path.join(cycleDepsBasePath, "module.i"),
+											"dependencies": [],
+											"deduped": true
+										}
+									]
+								}
+							]
+						}
+					]
+				},
+				{
+					"id": "module.j",
+					"version": "1.0.0",
+					"path": path.join(cycleDepsBasePath, "module.j"),
+					"dependencies": [
+						{
+							"id": "module.i",
+							"version": "1.0.0",
+							"path": path.join(cycleDepsBasePath, "module.i"),
+							"dependencies": [
+								{
+									"id": "module.k",
+									"version": "1.0.0",
+									"path": path.join(cycleDepsBasePath, "module.k"),
+									"dependencies": [
+										{
+											"id": "module.h",
+											"version": "1.0.0",
+											"path": path.join(cycleDepsBasePath, "module.h"),
+											"dependencies": [],
+											"deduped": true
+										},
+										{
+											"id": "module.i",
+											"version": "1.0.0",
+											"path": path.join(cycleDepsBasePath, "module.i"),
+											"dependencies": [],
+											"deduped": true
+										},
+										{
+											"id": "module.j",
+											"version": "1.0.0",
+											"path": path.join(cycleDepsBasePath, "module.j"),
+											"dependencies": [],
+											"deduped": true
+										}
+									]
+								}
+							]
+						}
+					]
+				}
+			]
+		},
+		{
+			"id": "module.i",
+			"version": "1.0.0",
+			"path": path.join(cycleDepsBasePath, "module.i"),
+			"dependencies": [
+				{
+					"id": "module.k",
+					"version": "1.0.0",
+					"path": path.join(cycleDepsBasePath, "module.k"),
+					"dependencies": [
+						{
+							"id": "module.h",
+							"version": "1.0.0",
+							"path": path.join(cycleDepsBasePath, "module.h"),
+							"dependencies": [
+								{
+									"id": "module.i",
+									"version": "1.0.0",
+									"path": path.join(cycleDepsBasePath, "module.i"),
+									"dependencies": [],
+									"deduped": true
+								},
+								{
+									"id": "module.j",
+									"version": "1.0.0",
+									"path": path.join(cycleDepsBasePath, "module.j"),
+									"dependencies": [
+										{
+											"id": "module.i",
+											"version": "1.0.0",
+											"path": path.join(cycleDepsBasePath, "module.i"),
+											"dependencies": [],
+											"deduped": true
+										}
+									]
+								}
+							]
+						},
+						{
+							"id": "module.i",
+							"version": "1.0.0",
+							"path": path.join(cycleDepsBasePath, "module.i"),
+							"dependencies": [],
+							"deduped": true
+						},
+						{
+							"id": "module.j",
+							"version": "1.0.0",
+							"path": path.join(cycleDepsBasePath, "module.j"),
+							"dependencies": [
+								{
+									"id": "module.i",
+									"version": "1.0.0",
+									"path": path.join(cycleDepsBasePath, "module.i"),
+									"dependencies": [],
+									"deduped": true
+								}
+							]
+						}
+					]
+				}
+			]
+		},
+		{
+			"id": "module.j",
+			"version": "1.0.0",
+			"path": path.join(cycleDepsBasePath, "module.j"),
+			"dependencies": [
+				{
+					"id": "module.i",
+					"version": "1.0.0",
+					"path": path.join(cycleDepsBasePath, "module.i"),
+					"dependencies": [
+						{
+							"id": "module.k",
+							"version": "1.0.0",
+							"path": path.join(cycleDepsBasePath, "module.k"),
+							"dependencies": [
+								{
+									"id": "module.h",
+									"version": "1.0.0",
+									"path": path.join(cycleDepsBasePath, "module.h"),
+									"dependencies": [
+										{
+											"id": "module.i",
+											"version": "1.0.0",
+											"path": path.join(cycleDepsBasePath, "module.i"),
+											"dependencies": [],
+											"deduped": true
+										},
+										{
+											"id": "module.j",
+											"version": "1.0.0",
+											"path": path.join(cycleDepsBasePath, "module.j"),
+											"dependencies": [],
+											"deduped": true
+										}
+									]
+								},
+								{
+									"id": "module.i",
+									"version": "1.0.0",
+									"path": path.join(cycleDepsBasePath, "module.i"),
+									"dependencies": [],
+									"deduped": true
+								},
+								{
+									"id": "module.j",
+									"version": "1.0.0",
+									"path": path.join(cycleDepsBasePath, "module.j"),
+									"dependencies": [],
+									"deduped": true
+								}
+							]
+						}
+					]
+				}
+			]
+		},
+		{
+			"id": "module.k",
+			"version": "1.0.0",
+			"path": path.join(cycleDepsBasePath, "module.k"),
+			"dependencies": [
+				{
+					"id": "module.h",
+					"version": "1.0.0",
+					"path": path.join(cycleDepsBasePath, "module.h"),
+					"dependencies": [
+						{
+							"id": "module.i",
+							"version": "1.0.0",
+							"path": path.join(cycleDepsBasePath, "module.i"),
+							"dependencies": [
+								{
+									"id": "module.k",
+									"version": "1.0.0",
+									"path": path.join(cycleDepsBasePath, "module.k"),
+									"dependencies": [],
+									"deduped": true
+								}
+							]
+						},
+						{
+							"id": "module.j",
+							"version": "1.0.0",
+							"path": path.join(cycleDepsBasePath, "module.j"),
+							"dependencies": [
+								{
+									"id": "module.i",
+									"version": "1.0.0",
+									"path": path.join(cycleDepsBasePath, "module.i"),
+									"dependencies": [
+										{
+											"id": "module.k",
+											"version": "1.0.0",
+											"path": path.join(cycleDepsBasePath, "module.k"),
+											"dependencies": [],
+											"deduped": true
+										}
+									]
+								}
+							]
+						}
+					]
+				},
+				{
+					"id": "module.i",
+					"version": "1.0.0",
+					"path": path.join(cycleDepsBasePath, "module.i"),
+					"dependencies": [
+						{
+							"id": "module.k",
+							"version": "1.0.0",
+							"path": path.join(cycleDepsBasePath, "module.k"),
+							"dependencies": [],
+							"deduped": true
+						}
+					]
+				},
+				{
+					"id": "module.j",
+					"version": "1.0.0",
+					"path": path.join(cycleDepsBasePath, "module.j"),
+					"dependencies": [
+						{
+							"id": "module.i",
+							"version": "1.0.0",
+							"path": path.join(cycleDepsBasePath, "module.i"),
+							"dependencies": [
+								{
+									"id": "module.k",
+									"version": "1.0.0",
+									"path": path.join(cycleDepsBasePath, "module.k"),
+									"dependencies": [],
+									"deduped": true
+								}
+							]
+						}
+					]
+				}
+			]
+		}
+	]
+};