fix(project): Route Theme-Library Resources in BuildReader

packages/project/lib/build/BuildReader.js

@@ -14,6 +14,7 @@ class BuildReader extends AbstractReader {
 	#projects;
 	#projectNames;
 	#applicationProjectName;
+	#themeLibraryProjectNames = [];
 	#namespaces = new Map();
 	#buildServerInterface;
 
@@ -46,6 +47,15 @@ class BuildReader extends AbstractReader {
 			if (project.getType() === "application") {
 				this.#applicationProjectName = project.getName();
 			}
+
+			// Theme libraries have no namespace (they can contribute themes to several) and serve
+			// their resources under a path owned by another project, e.g. themelib_sap_horizon
+			// serves /resources/sap/ui/core/themes/sap_horizon/. Namespace matching therefore
+			// routes such a request to the wrong project, so theme libraries are tracked
+			// separately and offered as a routing candidate for theme resource paths.
+			if (project.getType() === "theme-library") {
+				this.#themeLibraryProjectNames.push(project.getName());
+			}
 		}
 	}
 
@@ -66,70 +76,109 @@ class BuildReader extends AbstractReader {
 	/**
 	 * Locates a resource by path
 	 *
-	 * Attempts to determine the appropriate project reader based on the resource path
-	 * and namespace. Falls back to searching all projects if the resource cannot be found.
+	 * Tries candidate readers in priority order (see {@link BuildReader#_getReaderCandidates})
+	 * and returns the first resource found. Each candidate reader is requested from the build server
+	 * only when the preceding ones did not yield the resource, so the minimal set of projects is
+	 * built. The reader for all projects is the last resort.
 	 *
 	 * @public
 	 * @param {string} virPath Virtual path of the resource
 	 * @param {...*} args Additional arguments to pass to the underlying reader's byPath method
 	 * @returns {Promise<@ui5/fs/Resource|null>} Promise resolving to resource or null if not found
 	 */
 	async byPath(virPath, ...args) {
-		const reader = await this._getReaderForResource(virPath);
-		let res = await reader.byPath(virPath, ...args);
-		if (!res) {
-			// Fallback to unspecified projects
-			const allReader = await this.#buildServerInterface.getReaderForProjects(this.#projectNames);
-			res = await allReader.byPath(virPath, ...args);
+		for (const getReader of this._getReaderCandidates(virPath)) {
+			const reader = await getReader();
+			if (!reader) {
+				continue;
+			}
+			const res = await reader.byPath(virPath, ...args);
+			if (res) {
+				return res;
+			}
 		}
-		return res;
+		return null;
 	}
 
 	/**
-	 * Gets the appropriate reader for a resource at the given path
+	 * Builds the ordered list of candidate readers for a resource path
 	 *
-	 * Determines which project(s) might contain the resource based on namespace matching
-	 * and returns a reader for those projects. For single-project readers, returns that
-	 * project's reader directly.
+	 * Each entry is a factory resolving to a reader (or undefined when its strategy does not apply).
+	 * Factories are evaluated lazily by {@link BuildReader#byPath} and requesting a reader from the
+	 * build server may (re)build the associated projects, so ordering minimizes unnecessary builds:
+	 * cheaper and more specific strategies come first, the reader for all projects comes last.
 	 *
 	 * @param {string} virPath Virtual path of the resource
-	 * @returns {Promise<@ui5/fs/AbstractReader>} Promise resolving to appropriate reader
+	 * @returns {Array<function(): Promise<@ui5/fs/AbstractReader|undefined>>} Ordered readers
 	 */
-	async _getReaderForResource(virPath) {
+	_getReaderCandidates(virPath) {
 		if (this.#projects.length === 1) {
 			// Filtering on a single project (typically the root project)
-			return await this.#buildServerInterface.getReaderForProject(this.#projectNames[0]);
-		}
-		// Determine project for resource path
-		const projects = this._getProjectsForResourcePath(virPath);
-		if (projects.length) {
-			return await this.#buildServerInterface.getReaderForProjects(projects);
+			return [
+				() => this.#buildServerInterface.getReaderForProject(this.#projectNames[0]),
+			];
 		}
 
-		// Unable to determine project for resource using path
-		// Fallback 1: Try to find resource in cached readers (if available) to identify the relevant project
-		const cachedReader = this.#buildServerInterface.getCachedReadersForProjects(this.#projectNames);
-		if (cachedReader) {
+		const readers = [];
+
+		// Cached readers hold the results of already-built (fresh) projects and are free to query,
+		// so consult them first: a hit identifies the owning project without building anything.
+		readers.push(async () => {
+			const cachedReader = this.#buildServerInterface.getCachedReadersForProjects(this.#projectNames);
+			if (!cachedReader) {
+				return;
+			}
 			const res = await cachedReader.byPath(virPath);
 			if (res) {
-				// Found resource in one of the cached readers. Assume it still belongs to the associated project
-				return this.#buildServerInterface.getReaderForProject(res.getProject().getName());
+				// Found in a cached reader. Request the project's own reader so a subsequent
+				// invalidation is reflected, assuming the resource still belongs to that project.
+				return await this.#buildServerInterface.getReaderForProject(res.getProject().getName());
 			}
+		});
+
+		// Namespace matches, most specific first. Offered individually so a more specific match is
+		// tried (and its project built) before a less specific one.
+		for (const projectName of this._getProjectsForResourcePath(virPath)) {
+			readers.push(() => this.#buildServerInterface.getReaderForProject(projectName));
 		}
 
-		// Fallback 2: If the root project is of type application, and the request does not start with
-		// /resources/ or /test-resources/, test whether the resource can be found in the root project
+		// Theme libraries serve resources under a path owned by another project's namespace, so the
+		// namespace match above can miss them. When the path looks like a theme resource, offer the
+		// theme libraries as a candidate before falling back to all projects.
+		if (this.#themeLibraryProjectNames.length && this._isThemeResourcePath(virPath)) {
+			readers.push(() =>
+				this.#buildServerInterface.getReaderForProjects(this.#themeLibraryProjectNames));
+		}
+
+		// If the root project is an application and the request does not start with /resources/ or
+		// /test-resources/, the resource may live in the application project itself.
 		if (this.#applicationProjectName && !virPath.startsWith("/resources/") &&
 			!virPath.startsWith("/test-resources/")) {
-			const appReader = await this.#buildServerInterface.getReaderForProject(this.#applicationProjectName);
-			const res = await appReader.byPath(virPath);
-			if (res) {
-				return appReader;
-			}
+			readers.push(() => this.#buildServerInterface.getReaderForProject(this.#applicationProjectName));
 		}
 
-		// Fallback to request a reader for all projects
-		return await this.#buildServerInterface.getReaderForProjects(this.#projectNames);
+		// Last resort: a reader for all projects. This (re)builds every non-fresh project, so it is
+		// only reached when no more specific strategy located the resource.
+		readers.push(() => this.#buildServerInterface.getReaderForProjects(this.#projectNames));
+
+		return readers;
+	}
+
+	/**
+	 * Checks whether a path looks like a theme-library resource
+	 *
+	 * Theme libraries serve their resources under a "themes/<theme-name>/" segment
+	 * (e.g. /resources/sap/ui/core/themes/sap_horizon/library.css), so only such paths
+	 * are routed to theme libraries.
+	 *
+	 * @param {string} virPath Virtual resource path
+	 * @returns {boolean} True if the path is a resource path with a "themes" segment
+	 */
+	_isThemeResourcePath(virPath) {
+		if (!virPath.startsWith("/resources/") && !virPath.startsWith("/test-resources/")) {
+			return false;
+		}
+		return virPath.split("/").includes("themes");
 	}
 
 	/**

packages/project/test/lib/build/BuildReader.js

@@ -49,21 +49,24 @@ test("byPath: returns resource from primary reader", async (t) => {
 	t.is(result, resource);
 });
 
-test("byPath: falls back to all projects when primary reader returns null", async (t) => {
+test("byPath: single project queries only its own reader", async (t) => {
+	// For a single project, #getReaderForProjects short-circuits to the same reader as
+	// #getReaderForProject, so byPath offers only the single project's reader and consults
+	// nothing else when it returns null.
 	const projects = [createMockProject("proj-a", "my/ns")];
-	const resource = {getPath: () => "/resources/my/ns/a.js"};
 	const primaryReader = {byPath: sinon.stub().resolves(null)};
-	const fallbackReader = {byPath: sinon.stub().resolves(resource)};
 	const buildServerInterface = {
 		getReaderForProject: sinon.stub().resolves(primaryReader),
-		getReaderForProjects: sinon.stub().resolves(fallbackReader),
+		getReaderForProjects: sinon.stub().resolves(primaryReader),
 	};
 	const reader = new BuildReader("test", projects, buildServerInterface);
 	const result = await reader.byPath("/resources/my/ns/a.js");
-	t.is(result, resource);
+	t.is(result, null);
+	t.is(buildServerInterface.getReaderForProject.callCount, 1);
+	t.is(buildServerInterface.getReaderForProjects.callCount, 0);
 });
 
-test("_getReaderForResource: final fallback when path doesn't match any namespace", async (t) => {
+test("byPath: final fallback when path doesn't match any namespace", async (t) => {
 	const projects = [
 		createMockProject("proj-a", "ns/a"),
 		createMockProject("proj-b", "ns/b"),
@@ -79,7 +82,7 @@ test("_getReaderForResource: final fallback when path doesn't match any namespac
 	t.true(buildServerInterface.getReaderForProjects.called);
 });
 
-test("_getReaderForResource: uses cached reader to identify project", async (t) => {
+test("byPath: uses cached reader to identify project", async (t) => {
 	const projects = [
 		createMockProject("proj-a", "ns/a"),
 		createMockProject("proj-b", "ns/b"),
@@ -97,7 +100,7 @@ test("_getReaderForResource: uses cached reader to identify project", async (t)
 	t.is(result, foundResource);
 });
 
-test("_getReaderForResource: application fallback for non-resource paths", async (t) => {
+test("byPath: application fallback for non-resource paths", async (t) => {
 	const projects = [
 		createMockProject("my-app", "my/app", "application"),
 		createMockProject("my-lib", "my/lib"),
@@ -114,3 +117,156 @@ test("_getReaderForResource: application fallback for non-resource paths", async
 	const result = await reader.byPath("/index.html");
 	t.is(result, foundResource);
 });
+
+// Integration-style harness modelling a realistic multi-project build server. Each project owns a
+// set of virtual resource paths. Requesting a reader for a project (getReaderForProject /
+// getReaderForProjects) records the project name in `builtProjects` to stand in for the build the
+// BuildServer would trigger for a non-fresh project, so a test can assert which projects a single
+// byPath request would (re)build.
+function createBuildServerHarness(projectResources) {
+	const builtProjects = new Set();
+	const projects = [];
+
+	function createProjectReader(name) {
+		const paths = projectResources[name];
+		const project = {getName: () => name};
+		return {
+			async byPath(virPath) {
+				if (paths.has(virPath)) {
+					return {getPath: () => virPath, getProject: () => project};
+				}
+				return null;
+			},
+			async byGlob() {
+				return [];
+			},
+		};
+	}
+
+	function createCombinedReader(names) {
+		const readers = names.map((name) => createProjectReader(name));
+		return {
+			async byPath(virPath) {
+				for (const reader of readers) {
+					const res = await reader.byPath(virPath);
+					if (res) {
+						return res;
+					}
+				}
+				return null;
+			},
+			async byGlob() {
+				return [];
+			},
+		};
+	}
+
+	const buildServerInterface = {
+		async getReaderForProject(name) {
+			builtProjects.add(name);
+			return createProjectReader(name);
+		},
+		async getReaderForProjects(names) {
+			for (const name of names) {
+				builtProjects.add(name);
+			}
+			return createCombinedReader(names);
+		},
+		// Nothing is fresh in these cold-start scenarios, so no cached reader is available to
+		// identify the owning project without a build.
+		getCachedReadersForProjects() {
+			return undefined;
+		},
+	};
+
+	return {projects, builtProjects, buildServerInterface};
+}
+
+// Regression: a theme library contributes resources under a path that collides with another
+// project's namespace. "themelib.horizon" has no namespace (theme libraries can serve multiple)
+// and provides "/resources/sap/ui/core/themes/sap_horizon/library.css". Walking that path matches
+// the "sap/ui/core" namespace of the sap.ui.core library, which does not own the resource. The
+// request must still resolve from the theme library without requesting a reader for every project,
+// which would (re)build unrelated stale projects such as the sap.m library.
+test("byPath: routes colliding theme-library resource without building unrelated projects", async (t) => {
+	const {builtProjects, buildServerInterface} = createBuildServerHarness({
+		"app.a": new Set(["/index.html"]),
+		"sap.ui.core": new Set([
+			"/resources/sap/ui/core/library.js",
+			// sap.ui.core ships the base theme itself
+			"/resources/sap/ui/core/themes/base/library.css",
+		]),
+		"themelib.horizon": new Set([
+			"/resources/sap/ui/core/themes/sap_horizon/library.css",
+		]),
+		"sap.m": new Set(["/resources/sap/m/library.js"]),
+	});
+	const projects = [
+		createMockProject("app.a", "app/a", "application"),
+		createMockProject("sap.ui.core", "sap/ui/core", "library"),
+		createMockProject("themelib.horizon", null, "theme-library"),
+		createMockProject("sap.m", "sap/m", "library"),
+	];
+	const reader = new BuildReader("test", projects, buildServerInterface);
+
+	const res = await reader.byPath("/resources/sap/ui/core/themes/sap_horizon/library.css");
+
+	t.truthy(res, "Resource is found");
+	t.is(res.getProject().getName(), "themelib.horizon", "Resource resolves from the theme library");
+	t.false(builtProjects.has("sap.m"), "Unrelated library sap.m is not built");
+	t.false(builtProjects.has("app.a"), "Unrelated application is not built");
+});
+
+// The base theme lives in the sap.ui.core library itself, so a request for it must resolve from the
+// namespace-matched library without building any theme library.
+test("byPath: routes base-theme resource to owning library without building theme libraries", async (t) => {
+	const {builtProjects, buildServerInterface} = createBuildServerHarness({
+		"app.a": new Set(["/index.html"]),
+		"sap.ui.core": new Set([
+			"/resources/sap/ui/core/themes/base/library.css",
+		]),
+		"themelib.horizon": new Set([
+			"/resources/sap/ui/core/themes/sap_horizon/library.css",
+		]),
+	});
+	const projects = [
+		createMockProject("app.a", "app/a", "application"),
+		createMockProject("sap.ui.core", "sap/ui/core", "library"),
+		createMockProject("themelib.horizon", null, "theme-library"),
+	];
+	const reader = new BuildReader("test", projects, buildServerInterface);
+
+	const res = await reader.byPath("/resources/sap/ui/core/themes/base/library.css");
+
+	t.truthy(res, "Resource is found");
+	t.is(res.getProject().getName(), "sap.ui.core", "Resource resolves from the owning library");
+	t.false(builtProjects.has("themelib.horizon"), "Theme library is not built for a base-theme request");
+});
+
+// A theme library whose resource path does not collide with any namespace (nothing in the path
+// matches a namespace) must still route to the theme library rather than falling back to all
+// projects.
+test("byPath: routes non-colliding theme-library resource without building unrelated projects", async (t) => {
+	const {builtProjects, buildServerInterface} = createBuildServerHarness({
+		"app.a": new Set(["/index.html"]),
+		"sap.ui.core": new Set(["/resources/sap/ui/core/library.js"]),
+		"theme.library.e": new Set([
+			"/resources/theme/library/e/themes/my_theme/library.css",
+		]),
+		"sap.m": new Set(["/resources/sap/m/library.js"]),
+	});
+	const projects = [
+		createMockProject("app.a", "app/a", "application"),
+		createMockProject("sap.ui.core", "sap/ui/core", "library"),
+		createMockProject("theme.library.e", null, "theme-library"),
+		createMockProject("sap.m", "sap/m", "library"),
+	];
+	const reader = new BuildReader("test", projects, buildServerInterface);
+
+	const res = await reader.byPath("/resources/theme/library/e/themes/my_theme/library.css");
+
+	t.truthy(res, "Resource is found");
+	t.is(res.getProject().getName(), "theme.library.e", "Resource resolves from the theme library");
+	t.false(builtProjects.has("sap.m"), "Unrelated library sap.m is not built");
+	t.false(builtProjects.has("app.a"), "Unrelated application is not built");
+});