[FIX] byGlob: Re-included exclude patterns do not add resources to th…

…e result set themselves The current handling of glob exclude patterns may lead to resources being added to a result set even though they do not match the provided glob patterns. This can happen if a negated exclude pattern is provided.
SAP · Nov 22, 2019 · 31f56c0 · 31f56c0
1 parent 1f11762
commit 31f56c0
Show file tree

Hide file tree

Showing 5 changed files with 94 additions and 34 deletions.
diff --git a/lib/adapters/AbstractAdapter.js b/lib/adapters/AbstractAdapter.js
@@ -44,43 +44,52 @@ class AbstractAdapter extends AbstractReaderWriter {
 	 * @param {module:@ui5/fs.tracing.Trace} trace Trace instance
 	 * @returns {Promise<module:@ui5/fs.Resource[]>} Promise resolving to list of resources
 	 */
-	_byGlob(virPattern, options = {nodir: true}, trace) {
-		const excludes = this._excludesNegated;
-
+	async _byGlob(virPattern, options = {nodir: true}, trace) {
 		if (!(virPattern instanceof Array)) {
 			virPattern = [virPattern];
 		}
 
-		// Append static exclude patterns
-		virPattern = Array.prototype.concat.apply(virPattern, excludes);
+		let patterns = await Promise.all(virPattern.map(this._normalizePattern, this));
+		patterns = Array.prototype.concat.apply([], patterns);
+		if (patterns.length === 0) {
+			return [];
+		}
 
-		return Promise.all(virPattern.map(this._normalizePattern, this)).then((patterns) => {
-			patterns = Array.prototype.concat.apply([], patterns);
-			if (patterns.length === 0) {
-				return [];
-			}
+		const excludePatterns = await this._getExcludePattern();
 
-			if (!options.nodir) {
-				for (let i = patterns.length - 1; i >= 0; i--) {
-					const idx = this._virBaseDir.indexOf(patterns[i]);
-					if (patterns[i] && idx !== -1 && idx < this._virBaseDir.length) {
-						const subPath = patterns[i];
-						return Promise.resolve([
-							new Resource({
-								project: this.project,
-								statInfo: { // TODO: make closer to fs stat info
-									isDirectory: function() {
-										return true;
-									}
-								},
-								path: subPath
-							})
-						]);
-					}
+		if (!options.nodir) {
+			for (let i = patterns.length - 1; i >= 0; i--) {
+				const idx = this._virBaseDir.indexOf(patterns[i]);
+				if (patterns[i] && idx !== -1 && idx < this._virBaseDir.length) {
+					const subPath = patterns[i];
+					return Promise.resolve([
+						new Resource({
+							project: this.project,
+							statInfo: { // TODO: make closer to fs stat info
+								isDirectory: function() {
+									return true;
+								}
+							},
+							path: subPath
+						})
+					]);
 				}
 			}
-			return this._runGlob(patterns, options, trace);
-		});
+		}
+		return this._runGlob(patterns, {
+			nodir: options.nodir,
+			ignore: excludePatterns
+		}, trace);
+	}
+
+	async _getExcludePattern() {
+		if (this._normalizedExcludes) {
+			return this._normalizedExcludes;
+		}
+		return this._normalizedExcludes = Promise.all(this._excludes.map(this._normalizePattern, this))
+			.then((excludePatterns) => {
+				return Array.prototype.concat.apply([], excludePatterns);
+			});
 	}
 
 	/**

diff --git a/lib/adapters/FileSystem.js b/lib/adapters/FileSystem.js
@@ -35,18 +35,28 @@ class FileSystem extends AbstractAdapter {
 	 * @param {Array} patterns Array of glob patterns
 	 * @param {Object} [options={}] glob options
 	 * @param {boolean} [options.nodir=true] Do not match directories
+	 * @param {boolean} [options.ignore=[]] An array of glob patterns to exclude matches.
 	 * @param {module:@ui5/fs.tracing.Trace} trace Trace instance
 	 * @returns {Promise<module:@ui5/fs.Resource[]>} Promise resolving to list of resources
 	 */
-	async _runGlob(patterns, options = {nodir: true}, trace) {
+	async _runGlob(patterns, options = {}, trace) {
+		if (options.nodir === undefined) {
+			options.nodir = true;
+		}
+		if (options.ignore === undefined) {
+			options.ignore = [];
+		}
 		const opt = {
 			cwd: this._fsBasePath,
 			dot: true,
 			onlyFiles: options.nodir,
+			ignore: options.ignore,
 			followSymbolicLinks: false
 		};
 		trace.globCall();
-
+		if (log.isLevelEnabled("verbose")) {
+			log.verbose(`Glob with patterns ${patterns.join(", ")} and excludes ${options.ignore.join(", ")}`);
+		}
 		const promises = [];
 		if (!opt.onlyFiles && patterns.includes("")) { // Match physical root directory
 			promises.push(new Promise((resolve, reject) => {

diff --git a/lib/adapters/Memory.js b/lib/adapters/Memory.js
@@ -32,10 +32,21 @@ class Memory extends AbstractAdapter {
 	 * @param {Array} patterns array of glob patterns
 	 * @param {Object} [options={}] glob options
 	 * @param {boolean} [options.nodir=true] Do not match directories
+	 * @param {boolean} [options.ignore=[]] An array of glob patterns to exclude matches.
 	 * @param {module:@ui5/fs.tracing.Trace} trace Trace instance
 	 * @returns {Promise<module:@ui5/fs.Resource[]>} Promise resolving to list of resources
 	 */
-	async _runGlob(patterns, options = {nodir: true}, trace) {
+	async _runGlob(patterns, options = {}, trace) {
+		if (options.nodir === undefined) {
+			options.nodir = true;
+		}
+		if (options.ignore === undefined) {
+			options.ignore = [];
+		}
+		trace.globCall();
+		if (log.isLevelEnabled("verbose")) {
+			log.verbose(`Glob with patterns ${patterns.join(", ")} and excludes ${options.ignore.join(", ")}`);
+		}
 		if (patterns[0] === "" && !options.nodir) { // Match virtual root directory
 			return [
 				new Resource({
@@ -52,7 +63,8 @@ class Memory extends AbstractAdapter {
 
 		const filePaths = Object.keys(this._virFiles);
 		const matchedFilePaths = micromatch(filePaths, patterns, {
-			dot: true
+			dot: true,
+			ignore: options.ignore
 		});
 		let matchedResources = matchedFilePaths.map((virPath) => {
 			return this._virFiles[virPath];
@@ -61,7 +73,8 @@ class Memory extends AbstractAdapter {
 		if (!options.nodir) {
 			const dirPaths = Object.keys(this._virDirs);
 			const matchedDirs = micromatch(dirPaths, patterns, {
-				dot: true
+				dot: true,
+				ignore: options.ignore
 			});
 			matchedResources = matchedResources.concat(matchedDirs.map((virPath) => {
 				return this._virDirs[virPath];

diff --git a/test/lib/adapters/FileSystem_read.js b/test/lib/adapters/FileSystem_read.js
@@ -242,6 +242,20 @@ test("static excludes: glob with negated directory exclude, not excluding resour
 	t.deepEqual(resources.length, 2, "Found two resources");
 });
 
+test("static excludes: glob not existing resource should not match any re-included excludes", async (t) => {
+	const srcReader = resourceFactory.createAdapter({
+		fsBasePath: "./test/fixtures/library.l/src",
+		virBasePath: "/resources/",
+		excludes: [
+			"**",
+			"!**/some.js"
+		]
+	});
+
+	const resources = await srcReader.byGlob("/resources/**/does-not-exist", {nodir: true});
+	t.deepEqual(resources.length, 0, "Found no resources");
+});
+
 test("static excludes: byPath exclude everything in sub-directory", async (t) => {
 	const readerWriter = resourceFactory.createAdapter({
 		fsBasePath: "./test/fixtures/application.a/webapp",

diff --git a/test/lib/adapters/Memory_read.js b/test/lib/adapters/Memory_read.js
@@ -416,6 +416,20 @@ test("static excludes: glob with negated directory exclude, not excluding resour
 	t.deepEqual(resources.length, 2, "Found two resources");
 });
 
+test("static excludes: glob not existing resource should not match any re-included excludes", async (t) => {
+	const srcReader = resourceFactory.createAdapter({
+		fsBasePath: "./test/fixtures/library.l/src",
+		virBasePath: "/resources/",
+		excludes: [
+			"**",
+			"!**/some.js"
+		]
+	});
+
+	const resources = await srcReader.byGlob("/resources/**/does-not-exist", {nodir: true});
+	t.deepEqual(resources.length, 0, "Found no resources");
+});
+
 test("static excludes: byPath exclude everything in sub-directory", async (t) => {
 	const readerWriter = resourceFactory.createAdapter({
 		virBasePath: "/resources/app/",