From 846e9290ef29aadc1ad18203003983181cd9c23a Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Mon, 30 Dec 2019 14:17:34 +0100 Subject: [PATCH] [BREAKING] LibraryFormatter: Ignore manifest.json of nested apps If a library contains a nested application project or similar, do not assume that projects manifest.json to be relevant for the library itself in case it does not feature a manifest.json. Instead try to identify such situations by checking whether the .library file and the manifest.json are on the same directory level. BREAKING CHANGE: If a library contains both, a manifest.json and .library file, they must either be located in the same directory or the manifest.json is ignored. In cases where the manifest.json is located on a higher level or different directory on the same level than a .library file, an exception is thrown. --- lib/types/library/LibraryFormatter.js | 106 +++++++++++++++---- test/lib/types/library/LibraryFormatter.js | 114 ++++++++++++++++++++- 2 files changed, 194 insertions(+), 26 deletions(-) diff --git a/lib/types/library/LibraryFormatter.js b/lib/types/library/LibraryFormatter.js index 8916169ec..66bdceade 100644 --- a/lib/types/library/LibraryFormatter.js +++ b/lib/types/library/LibraryFormatter.js @@ -11,7 +11,7 @@ const AbstractUi5Formatter = require("../AbstractUi5Formatter"); class LibraryFormatter extends AbstractUi5Formatter { /** * Formats and validates the project - * + * * @returns {Promise} */ async format() { @@ -67,16 +67,20 @@ class LibraryFormatter extends AbstractUi5Formatter { * @throws {Error} if namespace can not be determined */ async getNamespace() { - let libraryId; - let fsNamespacePath; + // Trigger both reads asynchronously + const pManifest = this.getManifest(); + const pDotLibrary = this.getDotLibrary(); + + let manifestPath; + let manifestId; try { - const {content: manifest, fsPath} = await this.getManifest(); + const {content: manifest, fsPath} = await pManifest; // check for a proper sap.app/id in manifest.json to determine namespace if (manifest["sap.app"] && manifest["sap.app"].id) { log.verbose(`Found namespace information for project ${this._project.metadata.name} in ` + `manifest.json`); - libraryId = manifest["sap.app"] && manifest["sap.app"].id; - fsNamespacePath = path.dirname(fsPath); + manifestId = manifest["sap.app"] && manifest["sap.app"].id; + manifestPath = path.dirname(fsPath); } else { log.verbose( `No "sap.app" ID configuration found in manifest.json of project ${this._project.metadata.name}`); @@ -84,30 +88,88 @@ class LibraryFormatter extends AbstractUi5Formatter { } catch (err) { log.verbose(`Namespace resolution from manifest.json failed for project ` + `${this._project.metadata.name}: ${err.message}`); - log.verbose(`Falling back to .library file...`); } - if (!libraryId) { - try { - const {content: dotLibrary, fsPath} = await this.getDotLibrary(); - if (dotLibrary && dotLibrary.library && dotLibrary.library.name) { - log.verbose(`Found namespace information for project ${this._project.metadata.name} in ` + - `.library file`); - libraryId = dotLibrary.library.name; - fsNamespacePath = path.dirname(fsPath); - } else { - throw new Error( - `No library name found in .library of project ${this._project.metadata.name}`); + let dotLibraryPath; + let dotLibraryId; + try { + const {content: dotLibrary, fsPath} = await pDotLibrary; + if (dotLibrary && dotLibrary.library && dotLibrary.library.name) { + log.verbose(`Found namespace information for project ${this._project.metadata.name} in ` + + `.library file`); + dotLibraryId = dotLibrary.library.name; + dotLibraryPath = path.dirname(fsPath); + } else { + throw new Error( + `No library name found in .library of project ${this._project.metadata.name}`); + } + } catch (err) { + log.verbose(`Namespace resolution from .library failed for project ` + + `${this._project.metadata.name}: ${err.message}`); + } + + let libraryId; + let fsNamespacePath; + if (manifestId && dotLibraryId) { + // Both files present + // => check whether they are on the same level + const manifestDepth = manifestPath.split(path.sep).length; + const dotLibraryDepth = dotLibraryPath.split(path.sep).length; + + if (manifestDepth < dotLibraryDepth) { + // We see the .library file as the "leading" file of a library + // Therefore, a manifest.json on a higher level is something we do not except + throw new Error(`Failed to detect namespace for project ${this._project.metadata.name}: ` + + `Found a manifest.json on a higher directory level than the .library file. ` + + `It should be on the same or a lower level. ` + + `Note that a manifest.json on a lower level will be ignored.\n` + + ` manifest.json path: ${path.join(manifestPath, "manifest.json")}\n` + + ` is higher than\n` + + ` .library path: ${path.join(dotLibraryPath, ".library")}`); + } + if (manifestDepth === dotLibraryDepth) { + if (manifestPath !== dotLibraryPath) { + // This just should not happen in your project + throw new Error(`Failed to detect namespace for project ${this._project.metadata.name}: ` + + `Found a manifest.json on the same directory level but in a different directory ` + + `than the .library file. They should be in the same directory.\n` + + ` manifest.json path: ${path.join(manifestPath, "manifest.json")}\n` + + ` is different to\n` + + ` .library path: ${path.join(dotLibraryPath, ".library")}`); } - } catch (err) { - log.verbose(`Namespace resolution from .library failed for project ` + - `${this._project.metadata.name}: ${err.message}`); - log.verbose("Falling back to library.js file path..."); + // Typical scenario if both files are present + log.verbose(`Found a manifest.json and a .library file on the same level for ` + + `project ${this._project.metadata.name}.`); + log.verbose(`Resolving namespace of project ${this._project.metadata.name} from manifest.json...`); + libraryId = manifestId; + fsNamespacePath = manifestPath; + } else { + // Typical scenario: Some nested component has a manifest.json but the library itself only + // features a .library. => Ignore the manifest.json + log.verbose(`Ignoring manifest.json found on a lower level than the .library file of ` + + `project ${this._project.metadata.name}.`); + log.verbose(`Resolving namespace of project ${this._project.metadata.name} from .library...`); + libraryId = dotLibraryId; + fsNamespacePath = dotLibraryPath; } + } else if (manifestId) { + // Only manifest available + log.verbose(`Resolving namespace of project ${this._project.metadata.name} from manifest.json...`); + libraryId = manifestId; + fsNamespacePath = manifestPath; + } else if (dotLibraryId) { + // Only .library available + log.verbose(`Resolving namespace of project ${this._project.metadata.name} from .library...`); + libraryId = dotLibraryId; + fsNamespacePath = dotLibraryPath; + } else { + log.verbose(`Failed to resolve namespace of project ${this._project.metadata.name} from manifest.json ` + + `or .library file. Falling back to library.js file path...`); } let namespace; if (libraryId) { + // Maven placeholders can only exist in manifest.json or .library configuration if (this.hasMavenPlaceholder(libraryId)) { try { libraryId = await this.resolveMavenPlaceholder(libraryId); diff --git a/test/lib/types/library/LibraryFormatter.js b/test/lib/types/library/LibraryFormatter.js index 9d63bc858..f11f32455 100644 --- a/test/lib/types/library/LibraryFormatter.js +++ b/test/lib/types/library/LibraryFormatter.js @@ -162,8 +162,8 @@ test.serial("format: copyright retrieval fails", async (t) => { t.deepEqual(myProject.metadata.copyright, libraryETree.metadata.copyright, "Copyright was not altered"); - t.is(loggerVerboseSpy.callCount, 4, "calls to verbose"); - t.is(loggerVerboseSpy.getCall(3).args[0], "my-pony", "message from rejection"); + t.is(loggerVerboseSpy.callCount, 7, "calls to verbose"); + t.is(loggerVerboseSpy.getCall(6).args[0], "my-pony", "message from rejection"); mock.stop("@ui5/logger"); }); @@ -237,7 +237,7 @@ test.serial("format: namespace resolution fails", async (t) => { t.deepEqual(globbyStub.getCall(0).args[0], "**/manifest.json", "First glob is for manifest.json files"); t.deepEqual(globbyStub.getCall(1).args[0], "**/.library", "Second glob is for .library files"); t.deepEqual(globbyStub.getCall(2).args[0], "**/library.js", "Third glob for library.js files"); - t.deepEqual(loggerVerboseSpy.callCount, 6, "7 calls to log.verbose should be done"); + t.deepEqual(loggerVerboseSpy.callCount, 5, "5 calls to log.verbose should be done"); const logVerboseCalls = loggerVerboseSpy.getCalls().map((call) => call.args[0]); t.true(logVerboseCalls.includes( @@ -497,7 +497,7 @@ test("getCopyright: no copyright available", async (t) => { "Rejected with correct error message"); }); -test("getNamespace: from manifest.json", async (t) => { +test("getNamespace: from manifest.json with .library on same level", async (t) => { const myProject = clone(libraryETree); const libraryFormatter = new LibraryFormatter({project: myProject}); @@ -509,13 +509,55 @@ test("getNamespace: from manifest.json", async (t) => { }, fsPath: path.normalize("/some/path/mani-pony/manifest.json") // normalize for windows }); + sinon.stub(libraryFormatter, "getDotLibrary").resolves({ + content: { + library: {name: "dot-pony"} + }, + fsPath: path.normalize("/some/path/mani-pony/.library") // normalize for windows + }); const getSourceBasePathStub = sinon.stub(libraryFormatter, "getSourceBasePath").returns("/some/path/"); const res = await libraryFormatter.getNamespace(); + t.deepEqual(getSourceBasePathStub.callCount, 1, + "getSourceBasePath got called once"); t.deepEqual(getSourceBasePathStub.getCall(0).args[0], true, "getSourceBasePath called with correct argument"); t.deepEqual(res, "mani-pony", "Returned correct namespace"); }); +test("getNamespace: from manifest.json with .library on same level but different directory", async (t) => { + const myProject = clone(libraryETree); + + const manifestFsPath = path.normalize("/some/path/mani-pony/manifest.json"); // normalize for windows + const dotLibraryFsPath = path.normalize("/some/path/different-pony/.library"); + + const libraryFormatter = new LibraryFormatter({project: myProject}); + sinon.stub(libraryFormatter, "getManifest").resolves({ + content: { + "sap.app": { + id: "mani-pony" + } + }, + fsPath: manifestFsPath + }); + sinon.stub(libraryFormatter, "getDotLibrary").resolves({ + content: { + library: {name: "dot-pony"} + }, + fsPath: dotLibraryFsPath + }); + sinon.stub(libraryFormatter, "getSourceBasePath").returns("/some/path/"); + + const err = await t.throwsAsync(libraryFormatter.getNamespace()); + + t.deepEqual(err.message, + `Failed to detect namespace for project library.e: Found a manifest.json on the same directory level ` + + `but in a different directory than the .library file. They should be in the same directory.\n` + + ` manifest.json path: ${manifestFsPath}\n` + + ` is different to\n` + + ` .library path: ${dotLibraryFsPath}`, + "Rejected with correct error message"); +}); + test("getNamespace: from manifest.json with not matching file path", async (t) => { const myProject = clone(libraryETree); @@ -528,6 +570,12 @@ test("getNamespace: from manifest.json with not matching file path", async (t) = }, fsPath: path.normalize("/some/path/different/namespace/manifest.json") // normalize for windows }); + sinon.stub(libraryFormatter, "getDotLibrary").resolves({ + content: { + library: {name: "dot-pony"} + }, + fsPath: path.normalize("/some/path/different/namespace/.library") // normalize for windows + }); sinon.stub(libraryFormatter, "getSourceBasePath").returns("/some/path/"); const err = await t.throwsAsync(libraryFormatter.getNamespace()); @@ -585,6 +633,64 @@ test("getNamespace: from .library", async (t) => { t.deepEqual(res, "dot-pony", "Returned correct namespace"); }); +test("getNamespace: from .library with ignored manifest.json on lower level", async (t) => { + const myProject = clone(libraryETree); + + const libraryFormatter = new LibraryFormatter({project: myProject}); + sinon.stub(libraryFormatter, "getManifest").resolves({ + content: { + "sap.app": { + id: "mani-pony" + } + }, + fsPath: path.normalize("/some/path/namespace/somedir/manifest.json") // normalize for windows + }); + sinon.stub(libraryFormatter, "getDotLibrary").resolves({ + content: { + library: {name: "dot-pony"} + }, + fsPath: path.normalize("/some/path/dot-pony/.library") // normalize for windows + }); + sinon.stub(libraryFormatter, "getSourceBasePath").returns("/some/path/"); + const res = await libraryFormatter.getNamespace(); + t.deepEqual(res, "dot-pony", "Returned correct namespace"); +}); + +test("getNamespace: manifest.json on higher level than .library", async (t) => { + const myProject = clone(libraryETree); + + const manifestFsPath = path.normalize("/some/path/namespace/manifest.json"); // normalize for windows + const dotLibraryFsPath = path.normalize("/some/path/namespace/morenamespace/.library"); + + const libraryFormatter = new LibraryFormatter({project: myProject}); + sinon.stub(libraryFormatter, "getManifest").resolves({ + content: { + "sap.app": { + id: "mani-pony" + } + }, + fsPath: manifestFsPath + }); + sinon.stub(libraryFormatter, "getDotLibrary").resolves({ + content: { + library: {name: "dot-pony"} + }, + fsPath: dotLibraryFsPath + }); + sinon.stub(libraryFormatter, "getSourceBasePath").returns("/some/path/"); + const err = await t.throwsAsync(libraryFormatter.getNamespace()); + + t.deepEqual(err.message, + `Failed to detect namespace for project library.e: ` + + `Found a manifest.json on a higher directory level than the .library file. ` + + `It should be on the same or a lower level. ` + + `Note that a manifest.json on a lower level will be ignored.\n` + + ` manifest.json path: ${manifestFsPath}\n` + + ` is higher than\n` + + ` .library path: ${dotLibraryFsPath}`, + "Rejected with correct error message"); +}); + test("getNamespace: from .library with maven placeholder", async (t) => { const myProject = clone(libraryETree);