Skip to content

Polish xml merge #553

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

Merged
merged 1 commit into from
Jun 12, 2015
Merged

Polish xml merge #553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions lib/definitions/platform.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ interface IPlatformData {
targetedOS?: string[];
configurationFileName?: string;
configurationFilePath?: string;
mergeXmlConfig?: any[];
}

interface IPlatformsData {
Expand Down
3 changes: 2 additions & 1 deletion lib/services/android-project-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ class AndroidProjectService implements IPlatformProjectService {
],
frameworkFilesExtensions: [".jar", ".dat", ".so"],
configurationFileName: "AndroidManifest.xml",
configurationFilePath: path.join(this.$projectData.platformsDir, "android", "AndroidManifest.xml")
configurationFilePath: path.join(this.$projectData.platformsDir, "android", "AndroidManifest.xml"),
mergeXmlConfig: [{ "nodename": "manifest", "attrname": "*" }]
};
}

Expand Down
34 changes: 28 additions & 6 deletions lib/services/plugins-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import semver = require("semver");
import Future = require("fibers/future");
import constants = require("./../constants");
let xmlmerge = require("xmlmerge-js");
let DOMParser = require('xmldom').DOMParser;

export class PluginsService implements IPluginsService {
private static INSTALL_COMMAND_NAME = "install";
Expand Down Expand Up @@ -77,12 +78,22 @@ export class PluginsService implements IPluginsService {
shelljs.cp("-R", pluginData.fullPath, pluginDestinationPath);

let pluginPlatformsFolderPath = path.join(pluginDestinationPath, pluginData.name, "platforms", platform);
let pluginConfigurationFilePath = path.join(pluginPlatformsFolderPath, platformData.configurationFileName);
let pluginConfigurationFilePath = path.join(pluginPlatformsFolderPath, platformData.configurationFileName);
let configurationFilePath = platformData.configurationFilePath;

if(this.$fs.exists(pluginConfigurationFilePath).wait()) {
// Validate plugin configuration file
let pluginConfigurationFileContent = this.$fs.readText(pluginConfigurationFilePath).wait();
let configurationFileContent = this.$fs.readText(platformData.configurationFilePath).wait();
let resultXml = this.mergeXml(pluginConfigurationFileContent, configurationFileContent).wait();
this.$fs.writeFile(platformData.configurationFilePath, resultXml).wait();
this.validateXml(pluginConfigurationFileContent, pluginConfigurationFilePath);

// Validate configuration file
let configurationFileContent = this.$fs.readText(configurationFilePath).wait();
this.validateXml(configurationFileContent, configurationFilePath);

// Merge xml
let resultXml = this.mergeXml(configurationFileContent, pluginConfigurationFileContent, platformData.mergeXmlConfig || []).wait();
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't you verify the resultXml as well? This way we will not save incorrect xml, which the user will have to modify manually after that.

this.validateXml(resultXml);
this.$fs.writeFile(configurationFilePath, resultXml).wait();
}

if(this.$fs.exists(pluginPlatformsFolderPath).wait()) {
Expand Down Expand Up @@ -209,11 +220,11 @@ export class PluginsService implements IPluginsService {
}).future<string>()();
}

private mergeXml(xml1: string, xml2: string): IFuture<string> {
private mergeXml(xml1: string, xml2: string, config: any[]): IFuture<string> {
let future = new Future<string>();

try {
xmlmerge.merge(xml1, xml2, "", (mergedXml: string) => {
xmlmerge.merge(xml1, xml2, config, (mergedXml: string) => {
future.return(mergedXml);
});
} catch(err) {
Expand All @@ -222,5 +233,16 @@ export class PluginsService implements IPluginsService {

return future;
}

private validateXml(xml: string, xmlFilePath?: string): void {
let doc = new DOMParser({
locator: {},
errorHandler: (level: any, msg: string) => {
let errorMessage = xmlFilePath ? `Invalid xml file ${xmlFilePath}.` : `Invalid xml ${xml}.`;
this.$errors.fail(errorMessage + ` Additional technical information: ${msg}.` )
}
});
doc.parseFromString(xml, 'text/xml');
}
}
$injector.register("pluginsService", PluginsService);
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
"winreg": "0.0.12",
"ws": "0.7.1",
"xcode": "https://github.com/NativeScript/node-xcode/archive/NativeScript-0.9.tar.gz",
"xmldom": "0.1.19",
"xmlhttprequest": "https://github.com/telerik/node-XMLHttpRequest/tarball/master",
"xmlmerge-js": "0.2.4",
"yargs": "1.2.2"
Expand Down
165 changes: 164 additions & 1 deletion test/plugins-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import ErrorsLib = require("../lib/common/errors");
import ProjectHelperLib = require("../lib/common/project-helper");
import PlatformsDataLib = require("../lib/platforms-data");
import ProjectDataServiceLib = require("../lib/services/project-data-service");
import helpers = require("../lib/common/helpers");
import os = require("os");

import PluginsServiceLib = require("../lib/services/plugins-service");
import AddPluginCommandLib = require("../lib/commands/plugin/add-plugin");
Expand Down Expand Up @@ -124,6 +126,30 @@ function addPluginWhenExpectingToFail(testInjector: IInjector, plugin: string, e
assert.isTrue(isErrorThrown);
}

function createAndroidManifestFile(projectFolder: string, fs:IFileSystem): void {
let manifest = '<?xml version="1.0" encoding="UTF-8"?>' +
'<manifest xmlns:android="http://schemas.android.com/apk/res/android" package="com.example.android.basiccontactables" android:versionCode="1" android:versionName="1.0" >' +
'<uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE"/>' +
'<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"/>' +
'<uses-permission android:name="android.permission.INTERNET"/>' +
'<application android:allowBackup="true" android:icon="@drawable/ic_launcher" android:label="@string/app_name" android:theme="@style/Theme.Sample" >' +
'<activity android:name="com.example.android.basiccontactables.MainActivity" android:label="@string/app_name" android:launchMode="singleTop">' +
'<meta-data android:name="android.app.searchable" android:resource="@xml/searchable" />' +
'<intent-filter>' +
'<action android:name="android.intent.action.SEARCH" />' +
'</intent-filter>' +
'<intent-filter>' +
'<action android:name="android.intent.action.MAIN" />' +
'</intent-filter>' +
'</activity>' +
'</application>' +
'</manifest>';

fs.createDirectory(path.join(projectFolder, "platforms")).wait();
fs.createDirectory(path.join(projectFolder, "platforms", "android")).wait();
fs.writeFile(path.join(projectFolder, "platforms", "android", "AndroidManifest.xml"), manifest).wait();
}

describe("Plugins service", () => {
let testInjector: IInjector;
beforeEach(() => {
Expand Down Expand Up @@ -256,7 +282,7 @@ describe("Plugins service", () => {
let packageJsonContent = fs.readJson(path.join(projectFolder, "package.json")).wait();
let actualDependencies = packageJsonContent.dependencies;
let expectedDependencies = {
"plugin1": "^1.0.0"
"plugin1": "^1.0.3"
};
assert.deepEqual(actualDependencies, expectedDependencies);
});
Expand Down Expand Up @@ -419,4 +445,141 @@ describe("Plugins service", () => {
commandsService.tryExecuteCommand("plugin|add", [pluginFolderPath]).wait();
});
});

describe("merge xmls tests", () => {
let testInjector: IInjector;
beforeEach(() => {
testInjector = createTestInjector();
testInjector.registerCommand("plugin|add", AddPluginCommandLib.AddPluginCommand);
});
it("fails if the plugin contains incorrect xml", () => {
let pluginName = "mySamplePlugin";
let projectFolder = createProjectFile(testInjector);
let pluginFolderPath = path.join(projectFolder, pluginName);
let pluginJsonData = {
"name": pluginName,
"version": "0.0.1",
"nativescript": {
"platforms": {
"android": "0.10.0"
}
}
};
let fs = testInjector.resolve("fs");
fs.writeJson(path.join(pluginFolderPath, "package.json"), pluginJsonData).wait();

// Adds AndroidManifest.xml file in platforms/android folder
createAndroidManifestFile(projectFolder, fs);

// Mock plugins service
let pluginsService = testInjector.resolve("pluginsService");
pluginsService.getAllInstalledPlugins = () => {
return (() => {
return [{
name: ""
}];
}).future<IPluginData[]>()();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a merge stopper, but you may use future.fromResult to cut some code

}

let appDestinationDirectoryPath = path.join(projectFolder, "platforms", "android");

// Mock platformsData
let platformsData = testInjector.resolve("platformsData");
platformsData.getPlatformData = (platform: string) => {
return {
appDestinationDirectoryPath: appDestinationDirectoryPath,
frameworkPackageName: "tns-android",
configurationFileName: "AndroidManifest.xml"
}
}

// Ensure the pluginDestinationPath folder exists
let pluginPlatformsDirPath = path.join(appDestinationDirectoryPath, "app", "tns_modules", pluginName, "platforms", "android");
fs.ensureDirectoryExists(pluginPlatformsDirPath).wait();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a need to use the actual FS? Can we mock the FS instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to use the actual, real fs because I really create platforms folder


// Creates invalid plugin's AndroidManifest.xml file
let xml = '<?xml version="1.0" encoding="UTF-8"?>' +
'<manifest xmlns:android="http://schemas.android.com/apk/res/android" package="com.example.android.basiccontactables" android:versionCode="1" android:versionName="1.0" >' +
'<uses-permission android:name="android.permission.READ_CONTACTS"/>';
let pluginConfigurationFilePath = path.join(pluginPlatformsDirPath, "AndroidManifest.xml");
fs.writeFile(pluginConfigurationFilePath, xml).wait();

// Expected error message. The assertion happens in mockBeginCommand
let expectedErrorMessage = `Exception: Invalid xml file ${pluginConfigurationFilePath}. Additional technical information: element parse error: Exception: Invalid xml file ` +
`${pluginConfigurationFilePath}. Additional technical information: unclosed xml attribute` +
`\n@#[line:1,col:39].` +
`\n@#[line:1,col:39].`;
mockBeginCommand(testInjector, expectedErrorMessage);

let commandsService = testInjector.resolve(CommandsServiceLib.CommandsService);
commandsService.tryExecuteCommand("plugin|add", [pluginFolderPath]).wait();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we executing the command itself? Can't we test a service instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually because I have already created mocks and want to reuse them 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Please reconsider leaving the test as-is. Currently you are testing the entire command infrastructure and the plugins service at the same time, which is not the correct way of writing unit tests.

});
it("merges AndroidManifest.xml and produces correct xml", () => {
let pluginName = "mySamplePlugin";
let projectFolder = createProjectFile(testInjector);
let pluginFolderPath = path.join(projectFolder, pluginName);
let pluginJsonData = {
"name": pluginName,
"version": "0.0.1",
"nativescript": {
"platforms": {
"android": "0.10.0"
}
}
};
let fs = testInjector.resolve("fs");
fs.writeJson(path.join(pluginFolderPath, "package.json"), pluginJsonData).wait();

// Adds AndroidManifest.xml file in platforms/android folder
createAndroidManifestFile(projectFolder, fs);

// Mock plugins service
let pluginsService = testInjector.resolve("pluginsService");
pluginsService.getAllInstalledPlugins = () => {
return (() => {
return [{
name: ""
}];
}).future<IPluginData[]>()();
}

let appDestinationDirectoryPath = path.join(projectFolder, "platforms", "android");

// Mock platformsData
let platformsData = testInjector.resolve("platformsData");
platformsData.getPlatformData = (platform: string) => {
return {
appDestinationDirectoryPath: appDestinationDirectoryPath,
frameworkPackageName: "tns-android",
configurationFileName: "AndroidManifest.xml",
configurationFilePath: path.join(appDestinationDirectoryPath, "AndroidManifest.xml"),
mergeXmlConfig: [{ "nodename": "manifest", "attrname": "*" }]
}
}

// Ensure the pluginDestinationPath folder exists
let pluginPlatformsDirPath = path.join(appDestinationDirectoryPath, "app", "tns_modules", pluginName, "platforms", "android");
fs.ensureDirectoryExists(pluginPlatformsDirPath).wait();

// Creates valid plugin's AndroidManifest.xml file
let xml = '<?xml version="1.0" encoding="UTF-8"?>' +
'<manifest xmlns:android="http://schemas.android.com/apk/res/android" package="com.example.android.basiccontactables" android:versionCode="1" android:versionName="1.0" >' +
'<uses-permission android:name="android.permission.READ_CONTACTS"/>' +
'</manifest>';
let pluginConfigurationFilePath = path.join(pluginPlatformsDirPath, "AndroidManifest.xml");
fs.writeFile(pluginConfigurationFilePath, xml).wait();

pluginsService.add(pluginFolderPath).wait();

let expectedXml = '<?xmlversion="1.0"encoding="UTF-8"?><manifestxmlns:android="http://schemas.android.com/apk/res/android"package="com.example.android.basiccontactables"android:versionCode="1"android:versionName="1.0"><uses-permissionandroid:name="android.permission.READ_EXTERNAL_STORAGE"/><uses-permissionandroid:name="android.permission.WRITE_EXTERNAL_STORAGE"/><uses-permissionandroid:name="android.permission.INTERNET"/><applicationandroid:allowBackup="true"android:icon="@drawable/ic_launcher"android:label="@string/app_name"android:theme="@style/Theme.Sample"><activityandroid:name="com.example.android.basiccontactables.MainActivity"android:label="@string/app_name"android:launchMode="singleTop"><meta-dataandroid:name="android.app.searchable"android:resource="@xml/searchable"/><intent-filter><actionandroid:name="android.intent.action.SEARCH"/></intent-filter><intent-filter><actionandroid:name="android.intent.action.MAIN"/></intent-filter></activity></application><uses-permissionandroid:name="android.permission.READ_CONTACTS"/></manifest>';
expectedXml = helpers.stringReplaceAll(expectedXml, os.EOL, "");
expectedXml = helpers.stringReplaceAll(expectedXml, " ", "");

let actualXml = fs.readText(path.join(appDestinationDirectoryPath, "AndroidManifest.xml")).wait();
actualXml = helpers.stringReplaceAll(actualXml, "\n", "");
actualXml = helpers.stringReplaceAll(actualXml, " ", "");

assert.equal(expectedXml, actualXml);
});
});
});