Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] manifestCreator: 'embeds' should list all components #575

Merged
merged 5 commits into from
Feb 26, 2021
Merged

[FIX] manifestCreator: 'embeds' should list all components #575

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac88f72
[FEATURE] manifestCreator: embeds field
tobiasso85 Jan 27, 2021
fed9475
Added test for smaller descriptor version
tobiasso85 Feb 2, 2021
d56dd1d
remove async method signature for methods which do
tobiasso85 Feb 16, 2021
f7ae8bc
remove async method signature for methods which do
tobiasso85 Feb 16, 2021
9cd1313
Added TODO for descriptorVersion parameter
tobiasso85 Feb 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 10 additions & 59 deletions lib/processors/manifestCreator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,75 +177,31 @@ async function createManifest(libraryResource, libBundle, descriptorVersion, _in
function sectionVersion(candidateVersion) {
// _version property for nested sections became optional with AppDescriptor V5
if ( descriptorVersion.compare(APP_DESCRIPTOR_V5) < 0 ) {
return candidateVersion;
return candidateVersion.toString();
}
// return undefined
}

async function createSapApp() {
async function isEmbeddedByLibrary(componentPath, libraryPathPrefix) {
function createSapApp() {
function hasManifest(componentPath, libraryPathPrefix) {
const manifestPath = componentPath + "/manifest.json";

const manifestResource = libBundle.findResource(manifestPath.substring(libraryPathPrefix.length));
if ( manifestResource == null ) {
log.verbose(" component has no accompanying manifest.json, don't list it as 'embedded'");
return false;
}
const manifestString = await manifestResource.getString();
let manifest;
try {
manifest = JSON.parse(manifestString);
} catch (err) {
log.error(
" component '%s': failed to read the component's manifest.json, " +
"it won't be listed as 'embedded'.\nError details: %s", componentPath, err.stack);
return false;
}
let embeddedBy;
if (manifest && manifest["sap.app"]) {
embeddedBy = manifest["sap.app"]["embeddedBy"];
}
if (typeof embeddedBy === "undefined") {
log.verbose(" component doesn't declare 'sap.app/embeddedBy', don't list it as 'embedded'");
return false;
}
if (typeof embeddedBy !== "string") {
log.error(
" component '%s': property 'sap.app/embeddedBy' is of type '%s' (expected 'string'), " +
"it won't be listed as 'embedded'", componentPath, typeof embeddedBy
);
return false;
}
if ( !embeddedBy.length ) {
log.error(
" component '%s': property 'sap.app/embeddedBy' has an empty string value (which is invalid), " +
"it won't be listed as 'embedded'", componentPath
);
return false;
}
let resolvedEmbeddedBy = posixPath.resolve(componentPath, embeddedBy);
if ( resolvedEmbeddedBy && !resolvedEmbeddedBy.endsWith("/") ) {
resolvedEmbeddedBy = resolvedEmbeddedBy + "/";
}
if ( libraryPathPrefix === resolvedEmbeddedBy ) {
log.verbose(" component's 'sap.app/embeddedBy' property points to library, list it as 'embedded'");
return true;
} else {
log.verbose(
" component's 'sap.app/embeddedBy' points to '%s', don't list it as 'embedded'", resolvedEmbeddedBy
);
return false;
}
return true;
}

async function findEmbeddedComponents() {
function findEmbeddedComponents() {
const result = [];
const prefix = libraryResource.getPath().slice(0, - ".library".length);
const prefix = posixPath.dirname(libraryResource.getPath()) + "/";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better 👍🏻

const components = libBundle.getResources(/^\/(?:[^/]+\/)*Component\.js$/);
for (const comp of components) {
const componentPath = comp.getPath().slice(0, - "Component.js".length - 1);
const componentPath = posixPath.dirname(comp.getPath());
log.verbose("checking component at %s", componentPath);
if ( componentPath.startsWith(prefix) && await isEmbeddedByLibrary(componentPath, prefix) ) {
if ( componentPath.startsWith(prefix) && hasManifest(componentPath, prefix) ) {
result.push( componentPath.substring(prefix.length) );
} else if ( prefix === "/resources/sap/apf/" ) {
log.verbose("Package %s contains both '*.library' and 'Component.js'. " +
Expand Down Expand Up @@ -358,7 +314,7 @@ async function createManifest(libraryResource, libBundle, descriptorVersion, _in
_version: sectionVersion(APP_DESCRIPTOR_V3_SECTION_SAP_APP),
id: library.getName(),
type: "library",
embeds: await findEmbeddedComponents(),
embeds: findEmbeddedComponents(),
i18n,
applicationVersion: {
version: isValid(library.getVersion()) ? library.getVersion() : getProjectVersion()
Expand Down Expand Up @@ -429,11 +385,6 @@ async function createManifest(libraryResource, libBundle, descriptorVersion, _in

function createSapUI5() {
function getUI5Version() {
let ui5Version;
if ( ui5Version != null ) {
return ui5Version;
}
Comment on lines -432 to -435
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder where this code came from. It doesn't make sense, but maybe it needs to be fixed rather than being removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code taken from the Java implementation had a member variable ui5version which could be set from the oustide via parameter. This is not the case here anymore so it can be removed.


const dummy = new Dependency({
libraryName: [{
_: "sap.ui.core"
Expand Down Expand Up @@ -689,7 +640,7 @@ async function createManifest(libraryResource, libBundle, descriptorVersion, _in

return {
"_version": descriptorVersion.toString(),
"sap.app": await createSapApp(),
"sap.app": createSapApp(),
"sap.ui": createSapUi(),
"sap.ui5": createSapUI5(),
"sap.fiori": createSapFiori(),
Expand Down
91 changes: 57 additions & 34 deletions test/lib/processors/manifestCreator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,38 @@ test.serial("default manifest creation with special characters small app descrip
t.is(await result.getString(), expectedManifestContentSmallVersionString, "Correct result returned");
});

test.serial("default manifest creation with special characters very small app descriptor version", async (t) => {
const {manifestCreator, errorLogStub} = t.context;
const prefix = "/resources/sap/ui/mine/";
const libraryResource = {
getPath: () => {
return prefix + ".library";
},
getString: async () => {
return libraryContent;
},
_project: {
dependencies: [{
metadata: {
name: "sap.ui.core"
}
}]
}
};
t.is(errorLogStub.callCount, 0);

const options = {descriptorVersion: new Version("1.1.0")};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why the API requires a semver version instance instead of a plain string.
AFAICS the whole processor is not marked as public API so we should be able to do a cleanup.

As this is not related to the actual PR of fixing the embeds behavior, I'd be in favor of creating a separate PR.
Or is there a relation from the version handling change (toString) to the embeds change? At least I could not find any hint. Maybe to increase code coverage?

Copy link
Contributor Author

@tobiasso85 tobiasso85 Feb 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inside the manifestCreator the SemVer object is used for version comparison, as api parameter (descriptorVersion) a string definitely makes sense and can be done in a separate PR.
This test/fix was made to increase the test coverage. During a normal run such a small version is not used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see #556 where we already discussed making the manifestCreator public and that we would re-check the API before doing so.

const result = await manifestCreator({libraryResource, resources: [], options});
const expectedManifestContentSmallVersion = expectedManifestContentObject();
expectedManifestContentSmallVersion["_version"] = "1.1.0";
expectedManifestContentSmallVersion["sap.app"]["_version"] = "1.2.0";
expectedManifestContentSmallVersion["sap.ui"]["_version"] = "1.1.0";
expectedManifestContentSmallVersion["sap.ui5"]["_version"] = "1.1.0";
expectedManifestContentSmallVersion["sap.app"]["i18n"] = "i18n/i18n.properties";
const sResult = await result.getString();
t.deepEqual(JSON.parse(sResult), expectedManifestContentSmallVersion, "Correct result returned");
});

test.serial("manifest creation for sap/apf", async (t) => {
const {manifestCreator, errorLogStub, verboseLogStub} = t.context;

Expand Down Expand Up @@ -696,7 +728,7 @@ test.serial("manifest creation with embedded component", async (t) => {
"checking component at %s", "/resources/sap/lib1/component1"
]);
t.deepEqual(verboseLogStub.getCall(1).args, [
" component's 'sap.app/embeddedBy' property points to library, list it as 'embedded'"
" sap.app/id taken from .library: '%s'", "sap.lib1"
]);
});

Expand All @@ -708,7 +740,9 @@ test.serial("manifest creation with embedded component (Missing 'embeddedBy')",
"sap.app": {
"id": "sap.lib1",
"type": "library",
"embeds": [],
"embeds": [
"component1"
],
"applicationVersion": {
"version": "1.0.0"
},
Expand Down Expand Up @@ -780,7 +814,7 @@ test.serial("manifest creation with embedded component (Missing 'embeddedBy')",
"checking component at %s", "/resources/sap/lib1/component1"
]);
t.deepEqual(verboseLogStub.getCall(1).args, [
" component doesn't declare 'sap.app/embeddedBy', don't list it as 'embedded'"
" sap.app/id taken from .library: '%s'", "sap.lib1"
]);
});

Expand All @@ -792,7 +826,9 @@ test.serial("manifest creation with embedded component ('embeddedBy' doesn't poi
"sap.app": {
"id": "sap.lib1",
"type": "library",
"embeds": [],
"embeds": [
"component1"
],
"applicationVersion": {
"version": "1.0.0"
},
Expand Down Expand Up @@ -868,8 +904,7 @@ test.serial("manifest creation with embedded component ('embeddedBy' doesn't poi
"checking component at %s", "/resources/sap/lib1/component1"
]);
t.deepEqual(verboseLogStub.getCall(1).args, [
" component's 'sap.app/embeddedBy' points to '%s', don't list it as 'embedded'",
"/resources/sap/lib1/foo/"
" sap.app/id taken from .library: '%s'", "sap.lib1"
]);
});

Expand All @@ -881,7 +916,9 @@ test.serial("manifest creation with embedded component ('embeddedBy' absolute pa
"sap.app": {
"id": "sap.lib1",
"type": "library",
"embeds": [],
"embeds": [
"component1"
],
"applicationVersion": {
"version": "1.0.0"
},
Expand Down Expand Up @@ -957,8 +994,7 @@ test.serial("manifest creation with embedded component ('embeddedBy' absolute pa
"checking component at %s", "/resources/sap/lib1/component1"
]);
t.deepEqual(verboseLogStub.getCall(1).args, [
" component's 'sap.app/embeddedBy' points to '%s', don't list it as 'embedded'",
"/"
" sap.app/id taken from .library: '%s'", "sap.lib1"
]);
});

Expand All @@ -970,7 +1006,9 @@ test.serial("manifest creation with embedded component ('embeddedBy' empty strin
"sap.app": {
"id": "sap.lib1",
"type": "library",
"embeds": [],
"embeds": [
"component1"
],
"applicationVersion": {
"version": "1.0.0"
},
Expand Down Expand Up @@ -1039,12 +1077,7 @@ test.serial("manifest creation with embedded component ('embeddedBy' empty strin
});
t.is(await result.getString(), expectedManifestContent, "Correct result returned");

t.is(errorLogStub.callCount, 1);
t.deepEqual(errorLogStub.getCall(0).args, [
" component '%s': property 'sap.app/embeddedBy' has an empty string value (which is invalid), " +
"it won't be listed as 'embedded'",
"/resources/sap/lib1/component1"
]);
t.is(errorLogStub.callCount, 0);
});

test.serial("manifest creation with embedded component ('embeddedBy' object)", async (t) => {
Expand All @@ -1055,7 +1088,9 @@ test.serial("manifest creation with embedded component ('embeddedBy' object)", a
"sap.app": {
"id": "sap.lib1",
"type": "library",
"embeds": [],
"embeds": [
"component1"
],
"applicationVersion": {
"version": "1.0.0"
},
Expand Down Expand Up @@ -1126,13 +1161,7 @@ test.serial("manifest creation with embedded component ('embeddedBy' object)", a
});
t.is(await result.getString(), expectedManifestContent, "Correct result returned");

t.is(errorLogStub.callCount, 1);
t.deepEqual(errorLogStub.getCall(0).args, [
" component '%s': property 'sap.app/embeddedBy' is of type '%s' (expected 'string'), " +
"it won't be listed as 'embedded'",
"/resources/sap/lib1/component1",
"object"
]);
t.is(errorLogStub.callCount, 0);
});

test.serial("manifest creation with embedded component (no manifest.json)", async (t) => {
Expand Down Expand Up @@ -1219,7 +1248,9 @@ test.serial("manifest creation with embedded component (invalid manifest.json)",
"sap.app": {
"id": "sap.lib1",
"type": "library",
"embeds": [],
"embeds": [
"component1"
],
"applicationVersion": {
"version": "1.0.0"
},
Expand Down Expand Up @@ -1284,15 +1315,7 @@ test.serial("manifest creation with embedded component (invalid manifest.json)",
});
t.is(await result.getString(), expectedManifestContent, "Correct result returned");

t.is(errorLogStub.callCount, 1);
t.is(errorLogStub.getCall(0).args.length, 3);
t.deepEqual(errorLogStub.getCall(0).args.slice(0, 2), [
" component '%s': failed to read the component's manifest.json, " +
"it won't be listed as 'embedded'.\n" +
"Error details: %s",
"/resources/sap/lib1/component1"
]);
t.true(errorLogStub.getCall(0).args[2].startsWith("SyntaxError: Unexpected token"));
t.is(errorLogStub.callCount, 0);
});

test.serial("manifest creation for invalid .library content", async (t) => {
Expand Down