[BREAKING] Clone resources when writing in and reading from the Memory (#448)

BREAKING CHANGE:
Resources stored in the adapters can not be modified by reference anymore. All modifications need to be persisted by using the #write method in order to be reflected in the adapter.

Author: flovogt

lib/Resource.js

@@ -58,7 +58,7 @@ class Resource {
 		this._source = source; // Experimental, internal parameter
 		if (this._source) {
 			// Indicator for adapters like FileSystem to detect whether a resource has been changed
-			this._source.modified = false;
+			this._source.modified = this._source.modified || false;
 		}
 		this.__project = project; // Two underscores since "_project" was widely used in UI5 Tooling 2.0
 
@@ -314,7 +314,7 @@ class Resource {
 		const options = {
 			path: this._path,
 			statInfo: clone(this._statInfo),
-			source: this._source
+			source: clone(this._source)
 		};
 
 		if (this._stream) {
@@ -325,6 +325,10 @@ class Resource {
 			options.buffer = this._buffer;
 		}
 
+		if (this.__project) {
+			options.project = this.__project;
+		}
+
 		return options;
 	}
 

lib/adapters/Memory.js

@@ -27,6 +27,24 @@ class Memory extends AbstractAdapter {
 		this._virDirs = Object.create(null); // map full of directories
 	}
 
+	/**
+	 * Matches and returns resources from a given map (either _virFiles or _virDirs).
+	 *
+	 * @private
+	 * @param {string[]} patterns
+	 * @param {object} resourceMap
+	 * @returns {Promise<module:@ui5/fs.Resource[]>}
+	 */
+	async _matchPatterns(patterns, resourceMap) {
+		const resourcePaths = Object.keys(resourceMap);
+		const matchedPaths = micromatch(resourcePaths, patterns, {
+			dot: true
+		});
+		return Promise.all(matchedPaths.map((virPath) => {
+			return resourceMap[virPath] && resourceMap[virPath].clone();
+		}));
+	}
+
 	/**
 	 * Locate resources by glob.
 	 *
@@ -55,22 +73,11 @@ class Memory extends AbstractAdapter {
 			];
 		}
 
-		const filePaths = Object.keys(this._virFiles);
-		const matchedFilePaths = micromatch(filePaths, patterns, {
-			dot: true
-		});
-		let matchedResources = matchedFilePaths.map((virPath) => {
-			return this._virFiles[virPath];
-		});
+		let matchedResources = await this._matchPatterns(patterns, this._virFiles);
 
 		if (!options.nodir) {
-			const dirPaths = Object.keys(this._virDirs);
-			const matchedDirs = micromatch(dirPaths, patterns, {
-				dot: true
-			});
-			matchedResources = matchedResources.concat(matchedDirs.map((virPath) => {
-				return this._virDirs[virPath];
-			}));
+			const matchedDirs = await this._matchPatterns(patterns, this._virDirs);
+			matchedResources = matchedResources.concat(matchedDirs);
 		}
 
 		return matchedResources;
@@ -85,28 +92,25 @@ class Memory extends AbstractAdapter {
 	 * @param {@ui5/fs/tracing.Trace} trace Trace instance
 	 * @returns {Promise<@ui5/fs/Resource>} Promise resolving to a single resource
 	 */
-	_byPath(virPath, options, trace) {
+	async _byPath(virPath, options, trace) {
 		if (this.isPathExcluded(virPath)) {
-			return Promise.resolve(null);
+			return null;
+		}
+		if (!virPath.startsWith(this._virBasePath) && virPath !== this._virBaseDir) {
+			// Neither starts with basePath, nor equals baseDirectory
+			return null;
 		}
-		return new Promise((resolve, reject) => {
-			if (!virPath.startsWith(this._virBasePath) && virPath !== this._virBaseDir) {
-				// Neither starts with basePath, nor equals baseDirectory
-				resolve(null);
-				return;
-			}
 
-			const relPath = virPath.substr(this._virBasePath.length);
-			trace.pathCall();
+		const relPath = virPath.substr(this._virBasePath.length);
+		trace.pathCall();
 
-			const resource = this._virFiles[relPath];
+		const resource = this._virFiles[relPath];
 
-			if (!resource || (options.nodir && resource.getStatInfo().isDirectory())) {
-				resolve(null);
-			} else {
-				resolve(resource);
-			}
-		});
+		if (!resource || (options.nodir && resource.getStatInfo().isDirectory())) {
+			return null;
+		} else {
+			return await resource.clone();
+		}
 	}
 
 	/**
@@ -119,42 +123,39 @@ class Memory extends AbstractAdapter {
 	async _write(resource) {
 		resource = await this._migrateResource(resource);
 		super._write(resource);
-		return new Promise((resolve, reject) => {
-			const relPath = resource.getPath().substr(this._virBasePath.length);
-			log.silly("Writing to virtual path %s", resource.getPath());
-			this._virFiles[relPath] = resource;
-
-			// Add virtual directories for all path segments of the written resource
-			// TODO: Add tests for all this
-			const pathSegments = relPath.split("/");
-			pathSegments.pop(); // Remove last segment representing the resource itself
+		const relPath = resource.getPath().substr(this._virBasePath.length);
+		log.silly("Writing to virtual path %s", resource.getPath());
+		this._virFiles[relPath] = await resource.clone();
 
-			pathSegments.forEach((segment, i) => {
-				if (i >= 1) {
-					segment = pathSegments[i - 1] + "/" + segment;
-				}
-				pathSegments[i] = segment;
-			});
+		// Add virtual directories for all path segments of the written resource
+		// TODO: Add tests for all this
+		const pathSegments = relPath.split("/");
+		pathSegments.pop(); // Remove last segment representing the resource itself
 
-			for (let i = pathSegments.length - 1; i >= 0; i--) {
-				const segment = pathSegments[i];
-				if (!this._virDirs[segment]) {
-					this._virDirs[segment] = this._createResource({
-						project: this._project,
-						source: {
-							adapter: "Memory"
-						},
-						statInfo: { // TODO: make closer to fs stat info
-							isDirectory: function() {
-								return true;
-							}
-						},
-						path: this._virBasePath + segment
-					});
-				}
+		pathSegments.forEach((segment, i) => {
+			if (i >= 1) {
+				segment = pathSegments[i - 1] + "/" + segment;
 			}
-			resolve();
+			pathSegments[i] = segment;
 		});
+
+		for (let i = pathSegments.length - 1; i >= 0; i--) {
+			const segment = pathSegments[i];
+			if (!this._virDirs[segment]) {
+				this._virDirs[segment] = this._createResource({
+					project: this._project,
+					source: {
+						adapter: "Memory"
+					},
+					statInfo: { // TODO: make closer to fs stat info
+						isDirectory: function() {
+							return true;
+						}
+					},
+					path: this._virBasePath + segment
+				});
+			}
+		}
 	}
 }
 

test/lib/Resource.js

@@ -316,6 +316,71 @@ test("Resource: clone resource with stream", async (t) => {
 	t.is(clonedResourceContent, "Content", "Cloned resource has correct content string");
 });
 
+test("Resource: clone resource with source", async (t) => {
+	t.plan(4);
+
+	const resource = new Resource({
+		path: "my/path/to/resource",
+		source: {
+			adapter: "FileSystem",
+			fsPath: "/resources/my.js"
+		}
+	});
+
+	const clonedResource = await resource.clone();
+
+	t.not(resource.getSource(), clonedResource.getSource());
+	t.deepEqual(clonedResource.getSource(), resource.getSource());
+
+	// Change existing resource and clone
+	resource.setString("New Content");
+
+	const clonedResource2 = await resource.clone();
+
+	t.not(clonedResource.getSource(), resource.getSource());
+	t.deepEqual(clonedResource2.getSource(), resource.getSource());
+});
+
+test("Resource: clone resource with project", async (t) => {
+	t.plan(2);
+
+	const myProject = {
+		name: "my project"
+	};
+
+	const resourceOptions = {
+		path: "my/path/to/resource",
+		project: myProject
+	};
+
+	const resource = new Resource({
+		path: "my/path/to/resource",
+		project: myProject
+	});
+
+	const clonedResource = await resource.clone();
+	t.pass("Resource cloned");
+
+	const clonedResourceProject = await clonedResource.getProject();
+	t.is(clonedResourceProject, resourceOptions.project, "Cloned resource should have same " +
+		"project reference as the original resource");
+});
+
+test("Resource: create resource with modified source", (t) => {
+	t.plan(1);
+
+	const resource = new Resource({
+		path: "my/path/to/resource",
+		source: {
+			adapter: "FileSystem",
+			fsPath: "/resources/my.js",
+			modified: true
+		}
+	});
+
+	t.true(resource.getSource().modified, "Modified flag is still true");
+});
+
 test("getStream with createStream callback content: Subsequent content requests should throw error due " +
 		"to drained content", async (t) => {
 	const resource = createBasicResource();

test/lib/adapters/AbstractAdapter.js

@@ -1,9 +1,11 @@
 import test from "ava";
 import AbstractAdapter from "../../../lib/adapters/AbstractAdapter.js";
+import {createResource} from "../../../lib/resourceFactory.js";
 
-class MyAbstractAdapter extends AbstractAdapter {}
+class MyAbstractAdapter extends AbstractAdapter { }
 
 test("_migrateResource", async (t) => {
+	// Any JS object which might be a kind of resource
 	const resource = {
 		_path: "/test.js"
 	};
@@ -16,3 +18,25 @@ test("_migrateResource", async (t) => {
 
 	t.is(migratedResource.getPath(), "/test.js");
 });
+
+test("Write resource with another project than provided in the adapter", (t) => {
+	const resource = createResource({
+		path: "/test.js",
+		project: {
+			getName: () => "test.lib",
+			getVersion: () => "2.0.0"
+		}
+	});
+
+	const writer = new MyAbstractAdapter({
+		virBasePath: "/",
+		project: {
+			getName: () => "test.lib1",
+			getVersion: () => "2.0.0"
+		}
+	});
+
+	const error = t.throws(() => writer._write(resource));
+	t.is(error.message,
+		"Unable to write resource associated with project test.lib into adapter of project test.lib1: /test.js");
+});

test/lib/adapters/Memory_read.js

@@ -605,3 +605,59 @@ test("static excludes: glob with negated directory exclude, not excluding resour
 
 	t.is(resources.length, 4, "Found two resources and two directories");
 });
+
+test("byPath returns new resource", async (t) => {
+	const originalResource = createResource({
+		path: "/app/index.html",
+		string: "test"
+	});
+
+	const memoryAdapter = createAdapter({virBasePath: "/"});
+
+	await memoryAdapter.write(originalResource);
+
+	const returnedResource = await memoryAdapter.byPath("/app/index.html");
+
+	t.deepEqual(returnedResource, originalResource,
+		"Returned resource should be deep equal to original resource");
+	t.not(returnedResource, originalResource,
+		"Returned resource should not have same reference as original resource");
+
+	const anotherReturnedResource = await memoryAdapter.byPath("/app/index.html");
+
+	t.deepEqual(anotherReturnedResource, originalResource,
+		"Returned resource should be deep equal to original resource");
+	t.not(anotherReturnedResource, originalResource,
+		"Returned resource should not have same reference as original resource");
+
+	t.not(returnedResource, anotherReturnedResource,
+		"Both returned resources should not have same reference");
+});
+
+test("byGlob returns new resources", async (t) => {
+	const originalResource = createResource({
+		path: "/app/index.html",
+		string: "test"
+	});
+
+	const memoryAdapter = createAdapter({virBasePath: "/"});
+
+	await memoryAdapter.write(originalResource);
+
+	const [returnedResource] = await memoryAdapter.byGlob("/**");
+
+	t.deepEqual(returnedResource, originalResource,
+		"Returned resource should be deep equal to the original resource");
+	t.not(returnedResource, originalResource,
+		"Returned resource should not have same reference as the original resource");
+
+	const [anotherReturnedResource] = await memoryAdapter.byGlob("/**");
+
+	t.deepEqual(anotherReturnedResource, originalResource,
+		"Another returned resource should be deep equal to the original resource");
+	t.not(anotherReturnedResource, originalResource,
+		"Another returned resource should not have same reference as the original resource");
+
+	t.not(returnedResource, anotherReturnedResource,
+		"Both returned resources should not have same reference");
+});