[FIX] ui5Framework: Prevent install of libraries within workspace (#589)

JIRA: CPOUI5FOUNDATION-643

Author: matz3

lib/graph/helpers/ui5Framework.js

@@ -208,6 +208,60 @@ const utils = {
 			);
 		}
 	},
+	/**
+	 * This logic needs to stay in sync with the dependency definitions for the
+	 * sapui5/distribution-metadata package.
+	 *
+	 * @param {@ui5/project/specifications/Project} project
+	 */
+	async getFrameworkLibraryDependencies(project) {
+		let dependencies = [];
+		let optionalDependencies = [];
+
+		if (project.getId().startsWith("@sapui5/")) {
+			project.getFrameworkDependencies().forEach((dependency) => {
+				if (dependency.optional) {
+					// Add optional dependency to optionalDependencies
+					optionalDependencies.push(dependency.name);
+				} else if (!dependency.development) {
+					// Add non-development dependency to dependencies
+					dependencies.push(dependency.name);
+				}
+			});
+		} else if (project.getId().startsWith("@openui5/")) {
+			const packageResource = await project.getRootReader().byPath("/package.json");
+			const packageInfo = JSON.parse(await packageResource.getString());
+
+			dependencies = Object.keys(
+				packageInfo.dependencies || {}
+			).map(($) => $.replace("@openui5/", "")); // @sapui5 dependencies must not be defined in package.json
+			optionalDependencies = Object.keys(
+				packageInfo.devDependencies || {}
+			).map(($) => $.replace("@openui5/", "")); // @sapui5 dependencies must not be defined in package.json
+		}
+
+		return {dependencies, optionalDependencies};
+	},
+	async getWorkspaceFrameworkLibraryMetadata({workspace, projectGraph}) {
+		const libraryMetadata = Object.create(null);
+		const ui5Modules = await workspace.getModules();
+		for (const ui5Module of ui5Modules) {
+			const {project} = await ui5Module.getSpecifications();
+			// Only framework projects that are not already part of the projectGraph should be handled.
+			// Otherwise they would be available twice which is checked
+			// after installing via checkForDuplicateFrameworkProjects
+			if (project?.isFrameworkProject?.() && !projectGraph.getProject(project.getName())) {
+				const metadata = libraryMetadata[project.getName()] = Object.create(null);
+				metadata.id = project.getId();
+				metadata.path = project.getRootPath();
+				metadata.version = project.getVersion();
+				const {dependencies, optionalDependencies} = await utils.getFrameworkLibraryDependencies(project);
+				metadata.dependencies = dependencies;
+				metadata.optionalDependencies = optionalDependencies;
+			}
+		}
+		return libraryMetadata;
+	},
 	ProjectProcessor
 };
 
@@ -232,26 +286,31 @@ export default {
 	 *   Promise resolving with the given graph instance to allow method chaining
 	 */
 	enrichProjectGraph: async function(projectGraph, options = {}) {
+		const {workspace} = options;
 		const rootProject = projectGraph.getRoot();
 		const frameworkName = rootProject.getFrameworkName();
 		const frameworkVersion = rootProject.getFrameworkVersion();
 
-		// It is allowed omit the framework version in ui5.yaml and only provide one via the override
+		// It is allowed to omit the framework version in ui5.yaml and only provide one via the override
 		// This is a common use case for framework libraries, which generally should not define a
 		// framework version in their respective ui5.yaml
 		let version = options.versionOverride || frameworkVersion;
 
 		if (rootProject.isFrameworkProject() && !version) {
 			// If the root project is a framework project and no framework version is defined,
-			// all framework dependencies must already be part of the graph
-			rootProject.getFrameworkDependencies().forEach((dep) => {
-				if (utils.shouldIncludeDependency(dep) && !projectGraph.getProject(dep.name)) {
-					throw new Error(
-						`Missing framework dependency ${dep.name} for framework project ${rootProject.getName()}`);
-				}
+			// all framework dependencies must either be already part of the graph or part of the workspace.
+			// A mixed setup of framework deps within the graph AND from the workspace is currently not supported.
+
+			const someDependencyMissing = rootProject.getFrameworkDependencies().some((dep) => {
+				return utils.shouldIncludeDependency(dep) && !projectGraph.getProject(dep.name);
 			});
-			// All framework dependencies are already present in the graph
-			return projectGraph;
+
+			// If all dependencies are available there is nothing else to do here.
+			// In case of a workspace setup, the resolver will be created below without a version and
+			// will succeed in case no library needs to be actually installed.
+			if (!someDependencyMissing) {
+				return projectGraph;
+			}
 		}
 
 
@@ -267,12 +326,6 @@ export default {
 			);
 		}
 
-		if (!version) {
-			throw new Error(
-				`No framework version defined for root project ${rootProject.getName()}`
-			);
-		}
-
 		let Resolver;
 		if (frameworkName === "OpenUI5") {
 			Resolver = (await import("../../ui5Framework/Openui5Resolver.js")).default;
@@ -296,9 +349,20 @@ export default {
 			return projectGraph;
 		}
 
-		log.info(`Using ${frameworkName} version: ${version}`);
+		if (version) {
+			log.info(`Using ${frameworkName} version: ${version}`);
+		}
+
+		let providedLibraryMetadata;
+		if (workspace) {
+			providedLibraryMetadata = await utils.getWorkspaceFrameworkLibraryMetadata({
+				workspace, projectGraph
+			});
+		}
 
-		const resolver = new Resolver({cwd: rootProject.getRootPath(), version});
+		// Note: version might be undefined here and the Resolver will throw an error when calling
+		// #install and it can't be resolved via the provided library metadata
+		const resolver = new Resolver({cwd: rootProject.getRootPath(), version, providedLibraryMetadata});
 
 		let startTime;
 		if (log.isLevelEnabled("verbose")) {
@@ -322,7 +386,7 @@ export default {
 		const projectProcessor = new utils.ProjectProcessor({
 			libraryMetadata,
 			graph: frameworkGraph,
-			workspace: options.workspace
+			workspace
 		});
 
 		await Promise.all(referencedLibraries.map(async (libName) => {

lib/ui5Framework/AbstractResolver.js

@@ -27,29 +27,33 @@ const VERSION_RANGE_REGEXP = /^(0|[1-9]\d*)\.(0|[1-9]\d*)$/;
  * @hideconstructor
  */
 class AbstractResolver {
+	/* eslint-disable max-len */
 	/**
 	 * @param {*} options options
-	 * @param {string} options.version Framework version to use
+	 * @param {string} [options.version] Framework version to use. When omitted, all libraries need to be available
+	 * via <code>providedLibraryMetadata</code> parameter. Otherwise an error is thrown.
 	 * @param {string} [options.cwd=process.cwd()] Working directory to resolve configurations like .npmrc
 	 * @param {string} [options.ui5HomeDir="~/.ui5"] UI5 home directory location. This will be used to store packages,
 	 * metadata and configuration used by the resolvers. Relative to `process.cwd()`
+	 * @param {object.<string, @ui5/project/ui5Framework/AbstractResolver~LibraryMetadataEntry>} [options.providedLibraryMetadata]
+	 * Resolver skips installing listed libraries and uses the dependency information to resolve their dependencies.
+	 * <code>version</code> can be omitted in case all libraries can be resolved via the <code>providedLibraryMetadata</code>.
+	 * Otherwise an error is thrown.
 	 */
-	constructor({cwd, version, ui5HomeDir}) {
+	/* eslint-enable max-len */
+	constructor({cwd, version, ui5HomeDir, providedLibraryMetadata}) {
 		if (new.target === AbstractResolver) {
 			throw new TypeError("Class 'AbstractResolver' is abstract");
 		}
 
-		if (!version) {
-			throw new Error(`AbstractResolver: Missing parameter "version"`);
-		}
-
 		// In some CI environments, the homedir might be set explicitly to a relative
 		// path (e.g. "./"), but tooling requires an absolute path
 		this._ui5HomeDir = path.resolve(
 			ui5HomeDir || path.join(os.homedir(), ".ui5")
 		);
 		this._cwd = cwd ? path.resolve(cwd) : process.cwd();
 		this._version = version;
+		this._providedLibraryMetadata = providedLibraryMetadata;
 	}
 
 	async _processLibrary(libraryName, libraryMetadata, errors) {
@@ -62,7 +66,23 @@ class AbstractResolver {
 
 		log.verbose("Processing " + libraryName);
 
-		const promises = await this.handleLibrary(libraryName);
+		let promises;
+		const providedLibraryMetadata = this._providedLibraryMetadata?.[libraryName];
+		if (providedLibraryMetadata) {
+			log.verbose(`Skipping install for ${libraryName} (provided)`);
+			promises = {
+				// Use existing metadata if library is provided from outside (e.g. workspace)
+				metadata: Promise.resolve(providedLibraryMetadata),
+				// Provided libraries are already "installed"
+				install: Promise.resolve({
+					pkgPath: providedLibraryMetadata.path
+				})
+			};
+		} else if (!this._version) {
+			throw new Error(`Unable to install library ${libraryName}. No framework version provided.`);
+		} else {
+			promises = await this.handleLibrary(libraryName);
+		}
 
 		const [metadata, {pkgPath}] = await Promise.all([
 			promises.metadata.then((metadata) =>
@@ -143,7 +163,6 @@ class AbstractResolver {
 	 *   Object containing all installed libraries with library name as key
 	 */
 
-	/* eslint-enable max-len */
 	/**
 	 * Installs the provided libraries and their dependencies
 	 *

test/lib/graph/helpers/ui5Framework.integration.js

@@ -135,7 +135,8 @@ test.afterEach.always((t) => {
 
 function defineTest(testName, {
 	frameworkName,
-	verbose = false
+	verbose = false,
+	librariesInWorkspace = null
 }) {
 	const npmScope = frameworkName === "SAPUI5" ? "@sapui5" : "@openui5";
 
@@ -410,7 +411,66 @@ function defineTest(testName, {
 		const provider = new DependencyTreeProvider({dependencyTree});
 		const projectGraph = await projectGraphBuilder(provider);
 
-		await ui5Framework.enrichProjectGraph(projectGraph);
+		if (librariesInWorkspace) {
+			const projectNameMap = new Map();
+			const moduleIdMap = new Map();
+			librariesInWorkspace.forEach((libName) => {
+				const libraryDistMetadata = distributionMetadata.libraries[libName];
+				const module = {
+					getSpecifications: sinon.stub().resolves({
+						project: {
+							getName: sinon.stub().returns(libName),
+							getVersion: sinon.stub().returns("1.76.0-SNAPSHOT"),
+							getRootPath: sinon.stub().returns(path.join(fakeBaseDir, "workspace", libName)),
+							isFrameworkProject: sinon.stub().returns(true),
+							getId: sinon.stub().returns(libraryDistMetadata.npmPackageName),
+							getRootReader: sinon.stub().returns({
+								byPath: sinon.stub().resolves({
+									getString: sinon.stub().resolves(JSON.stringify({dependencies: {}}))
+								})
+							}),
+							getFrameworkDependencies: sinon.stub().callsFake(() => {
+								const deps = [];
+								libraryDistMetadata.dependencies.forEach((dep) => {
+									deps.push({name: dep});
+								});
+								libraryDistMetadata.optionalDependencies.forEach((optDep) => {
+									deps.push({name: optDep, optional: true});
+								});
+								return deps;
+							}),
+							isDeprecated: sinon.stub().returns(false),
+							isSapInternal: sinon.stub().returns(false),
+							getAllowSapInternal: sinon.stub().returns(false),
+						}
+					}),
+					getVersion: sinon.stub().returns("1.76.0-SNAPSHOT"),
+					getPath: sinon.stub().returns(path.join(fakeBaseDir, "workspace", libName)),
+				};
+				projectNameMap.set(libName, module);
+				moduleIdMap.set(libraryDistMetadata.npmPackageName, module);
+			});
+
+			const getModuleByProjectName = sinon.stub().callsFake(
+				async (projectName) => projectNameMap.get(projectName)
+			);
+			const getModules = sinon.stub().callsFake(
+				async () => {
+					const sortedMap = new Map([...moduleIdMap].sort((a, b) => String(a[0]).localeCompare(b[0])));
+					return Array.from(sortedMap.values());
+				}
+			);
+
+			const workspace = {
+				getName: sinon.stub().returns("test"),
+				getModules,
+				getModuleByProjectName
+			};
+
+			await ui5Framework.enrichProjectGraph(projectGraph, {workspace});
+		} else {
+			await ui5Framework.enrichProjectGraph(projectGraph);
+		}
 
 		const callbackStub = sinon.stub().resolves();
 		await projectGraph.traverseDepthFirst(callbackStub);
@@ -465,6 +525,15 @@ defineTest("ui5Framework helper should enhance project graph with UI5 framework
 	verbose: true
 });
 
+defineTest("ui5Framework helper should not cause install of libraries within workspace", {
+	frameworkName: "SAPUI5",
+	librariesInWorkspace: ["sap.ui.lib1", "sap.ui.lib2", "sap.ui.lib8"]
+});
+defineTest("ui5Framework helper should not cause install of libraries within workspace", {
+	frameworkName: "OpenUI5",
+	librariesInWorkspace: ["sap.ui.lib1", "sap.ui.lib2", "sap.ui.lib8"]
+});
+
 function defineErrorTest(testName, {
 	frameworkName,
 	failExtract = false,
@@ -723,8 +792,8 @@ test.serial("ui5Framework translator should not try to install anything when no
 	t.is(pacote.manifest.callCount, 0, "No manifest should be requested");
 });
 
-test.serial("ui5Framework translator should throw an error when framework version is not defined", async (t) => {
-	const {ui5Framework, projectGraphBuilder} = t.context;
+test.serial("ui5Framework helper shouldn't throw when framework version and libraries are not provided", async (t) => {
+	const {ui5Framework, projectGraphBuilder, logStub} = t.context;
 
 	const dependencyTree = {
 		id: "test-id",
@@ -745,13 +814,28 @@ test.serial("ui5Framework translator should throw an error when framework versio
 	const provider = new DependencyTreeProvider({dependencyTree});
 	const projectGraph = await projectGraphBuilder(provider);
 
-	await t.throwsAsync(async () => {
-		await ui5Framework.enrichProjectGraph(projectGraph);
-	}, {message: `No framework version defined for root project test-project`}, "Correct error message");
+	await ui5Framework.enrichProjectGraph(projectGraph);
+
+	t.is(logStub.verbose.callCount, 5);
+	t.deepEqual(logStub.verbose.getCall(0).args, [
+		"Configuration for module test-id has been supplied directly"
+	]);
+	t.deepEqual(logStub.verbose.getCall(1).args, [
+		"Module test-id contains project test-project"
+	]);
+	t.deepEqual(logStub.verbose.getCall(2).args, [
+		"Root project test-project qualified as application project for project graph"
+	]);
+	t.deepEqual(logStub.verbose.getCall(3).args, [
+		"Project test-project has no framework dependencies"
+	]);
+	t.deepEqual(logStub.verbose.getCall(4).args, [
+		"No SAPUI5 libraries referenced in project test-project or in any of its dependencies"
+	]);
 });
 
 test.serial(
-	"SAPUI5: ui5Framework translator should throw error when using a library that is not part of the dist metadata",
+	"SAPUI5: ui5Framework helper should throw error when using a library that is not part of the dist metadata",
 	async (t) => {
 		const {sinon, ui5Framework, Installer, projectGraphBuilder} = t.context;