From 5b5f43e4fb9971238345638e4d8d1b44d5e88c01 Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Thu, 2 Dec 2021 23:34:18 +0100 Subject: [PATCH 01/15] [FEATURE] Introduce ReaderFilter --- lib/AbstractReader.js | 10 ++++ lib/ReaderFilter.js | 104 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+) create mode 100644 lib/ReaderFilter.js diff --git a/lib/AbstractReader.js b/lib/AbstractReader.js index 059ad4d1..6475d5e0 100644 --- a/lib/AbstractReader.js +++ b/lib/AbstractReader.js @@ -70,6 +70,16 @@ class AbstractReader { }); } + filter({resourceTagCollection, matchMode, filters}) { + const ReaderFilter = require("./ReaderFilter"); + return new ReaderFilter({ + reader: this, + resourceTagCollection, + matchMode, + filters + }); + } + /** * Locates resources by one or more glob patterns. * diff --git a/lib/ReaderFilter.js b/lib/ReaderFilter.js new file mode 100644 index 00000000..d406e1b2 --- /dev/null +++ b/lib/ReaderFilter.js @@ -0,0 +1,104 @@ +const AbstractReader = require("./AbstractReader"); + +/** + * Resource Locator ReaderCollection + * + * @public + * @memberof module:@ui5/fs + * @augments module:@ui5/fs.AbstractReader + */ +class ReaderFilter extends AbstractReader { + /** + * The constructor. + * + * @param {object} parameters Parameters + * @param {module:@ui5/fs.AbstractReader} parameters.reader A resource reader + * @param {module:@ui5/fs.ResourceTagCollection} parameters.resourceTagCollection + * Resource tag collection to apply filters onto + * @param {object[]} parameters.filters Filters + * @param {string} parameters.matchMode Whether to match "any", "all" or "none" + */ + constructor({reader, resourceTagCollection, filters, matchMode}) { + super(); + if (!reader) { + throw new Error(`Missing parameter "reader"`); + } + if (!resourceTagCollection) { + throw new Error(`Missing parameter "resourceTagCollection"`); + } + if (!filters || !filters.length) { + throw new Error(`Missing parameter "filters"`); + } + if (!matchMode) { + throw new Error(`Missing parameter "matchMode"`); + } + this._reader = reader; + this._resourceTagCollection = resourceTagCollection; + this._filters = filters; + + this._matchMode = matchMode; + } + + /** + * Locates resources by glob. + * + * @private + * @param {string|string[]} pattern glob pattern as string or an array of + * glob patterns for virtual directory structure + * @param {object} options glob options + * @param {module:@ui5/fs.tracing.Trace} trace Trace instance + * @returns {Promise} Promise resolving to list of resources + */ + async _byGlob(pattern, options, trace) { + const result = await this._reader._byGlob(pattern, options, trace); + return result.filter(this._filterResource.bind(this)); + } + + /** + * Locates resources by path. + * + * @private + * @param {string} virPath Virtual path + * @param {object} options Options + * @param {module:@ui5/fs.tracing.Trace} trace Trace instance + * @returns {Promise} Promise resolving to a single resource + */ + async _byPath(virPath, options, trace) { + const result = await this._reader._byPath(virPath, options, trace); + if (result) { + if (!this._filterResource(result)) { + return null; + } + } + return result; + } + + _filterResource(resource) { + let filterFunction; + let testEquality = true; + switch (this._matchMode) { + case "any": + filterFunction = "some"; + break; + case "all": + filterFunction = "every"; + break; + case "none": + filterFunction = "every"; + testEquality = false; + break; + default: + throw Error(`Unknown match mode ${this._matchMode}`); + } + return this._filters[filterFunction](({tag, value: filterValue}) => { + const tagValue = this._resourceTagCollection.getTag(resource, tag); + if (testEquality) { + return tagValue === filterValue; + } else { + return tagValue !== filterValue; + } + }); + } +} + +module.exports = ReaderFilter; From b4a20db51f55a6207c8cdf9ffab7d269c0899cb8 Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Thu, 2 Dec 2021 23:34:59 +0100 Subject: [PATCH 02/15] [INTERNAL] ResourceTagCollection: Allow "super collections" --- lib/ResourceTagCollection.js | 26 ++++++- test/lib/ResourceTagCollection.js | 108 ++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 1 deletion(-) diff --git a/lib/ResourceTagCollection.js b/lib/ResourceTagCollection.js index e12ec958..43abfb2d 100644 --- a/lib/ResourceTagCollection.js +++ b/lib/ResourceTagCollection.js @@ -2,10 +2,18 @@ const tagNamespaceRegExp = new RegExp("^[a-z][a-z0-9]*$"); // part before the co const tagNameRegExp = new RegExp("^[A-Z][A-Za-z0-9]+$"); // part after the colon class ResourceTagCollection { - constructor({allowedTags}) { + constructor({allowedTags, superCollection}) { if (!allowedTags || !allowedTags.length) { throw new Error(`Missing parameter 'allowedTags'`); } + + if (superCollection) { + this._superCollection = superCollection; + this._superTags = this._superCollection.getAcceptedTags(); + } else { + this._superTags = []; + } + // No validation of tag names here since we might remove/ignore // this parameter in the future and generally allow all tags this._allowedTags = Object.freeze(allowedTags); @@ -13,6 +21,10 @@ class ResourceTagCollection { } setTag(resource, tag, value = true) { + if (this._superTags.includes(tag)) { + return this._superCollection.setTag(resource, tag, value); + } + this._validateResource(resource); this._validateTag(tag); this._validateValue(value); @@ -25,6 +37,10 @@ class ResourceTagCollection { } clearTag(resource, tag) { + if (this._superTags.includes(tag)) { + return this._superCollection.clearTag(resource, tag); + } + this._validateResource(resource); this._validateTag(tag); @@ -35,6 +51,10 @@ class ResourceTagCollection { } getTag(resource, tag) { + if (this._superTags.includes(tag)) { + return this._superCollection.getTag(resource, tag); + } + this._validateResource(resource); this._validateTag(tag); @@ -44,6 +64,10 @@ class ResourceTagCollection { } } + getAcceptedTags() { + return [...this._allowedTags, ...this._superTags]; + } + _validateResource(resource) { const path = resource.getPath(); if (!path) { diff --git a/test/lib/ResourceTagCollection.js b/test/lib/ResourceTagCollection.js index b7d1617a..737e2a2e 100644 --- a/test/lib/ResourceTagCollection.js +++ b/test/lib/ResourceTagCollection.js @@ -121,6 +121,114 @@ test("clearTag", (t) => { "_validateTag called with correct arguments"); }); +test("superCollection: setTag", (t) => { + const resource = new Resource({ + path: "/some/path" + }); + const superTagCollection = new ResourceTagCollection({ + allowedTags: ["abc:MySuperTag"], + }); + const tagCollection = new ResourceTagCollection({ + allowedTags: ["abc:MyTag"], + superCollection: superTagCollection + }); + + const validateResourceSpy = sinon.spy(superTagCollection, "_validateResource"); + const validateTagSpy = sinon.spy(superTagCollection, "_validateTag"); + const validateValueSpy = sinon.spy(superTagCollection, "_validateValue"); + + tagCollection.setTag(resource, "abc:MySuperTag", "my super value"); + tagCollection.setTag(resource, "abc:MyTag", "my value"); + + t.deepEqual(superTagCollection._pathTags, { + "/some/path": { + "abc:MySuperTag": "my super value" + } + }, "Super tag correctly stored"); + t.deepEqual(tagCollection._pathTags, { + "/some/path": { + "abc:MyTag": "my value" + } + }, "Non-super tag correctly stored"); + + t.is(validateResourceSpy.callCount, 1, "_validateResource called once"); + t.is(validateResourceSpy.getCall(0).args[0], resource, + "_validateResource called with correct arguments"); + + t.is(validateTagSpy.callCount, 1, "_validateTag called once"); + t.is(validateTagSpy.getCall(0).args[0], "abc:MySuperTag", + "_validateTag called with correct arguments"); + + t.is(validateValueSpy.callCount, 1, "_validateValue called once"); + t.is(validateValueSpy.getCall(0).args[0], "my super value", + "_validateValue called with correct arguments"); +}); + +test("superCollection: getTag", (t) => { + const resource = new Resource({ + path: "/some/path" + }); + const superTagCollection = new ResourceTagCollection({ + allowedTags: ["abc:MySuperTag"], + }); + const tagCollection = new ResourceTagCollection({ + allowedTags: ["abc:MyTag"], + superCollection: superTagCollection + }); + + tagCollection.setTag(resource, "abc:MySuperTag", 456); + tagCollection.setTag(resource, "abc:MyTag", 123); + + const validateResourceSpy = sinon.spy(superTagCollection, "_validateResource"); + const validateTagSpy = sinon.spy(superTagCollection, "_validateTag"); + + const value = tagCollection.getTag(resource, "abc:MySuperTag"); + + t.is(value, 456, "Got correct tag value"); + + t.is(validateResourceSpy.callCount, 1, "_validateResource called once"); + t.is(validateResourceSpy.getCall(0).args[0], resource, + "_validateResource called with correct arguments"); + + t.is(validateTagSpy.callCount, 1, "_validateTag called once"); + t.is(validateTagSpy.getCall(0).args[0], "abc:MySuperTag", + "_validateTag called with correct arguments"); +}); + +test("superCollection: clearTag", (t) => { + const resource = new Resource({ + path: "/some/path" + }); + const superTagCollection = new ResourceTagCollection({ + allowedTags: ["abc:MySuperTag"], + }); + const tagCollection = new ResourceTagCollection({ + allowedTags: ["abc:MyTag"], + superCollection: superTagCollection + }); + + tagCollection.setTag(resource, "abc:MySuperTag", 123); + + const validateResourceSpy = sinon.spy(superTagCollection, "_validateResource"); + const validateTagSpy = sinon.spy(superTagCollection, "_validateTag"); + + tagCollection.clearTag(resource, "abc:MySuperTag"); + + t.deepEqual(superTagCollection._pathTags, { + "/some/path": { + "abc:MySuperTag": undefined + } + }, "Tag value set to undefined"); + + t.is(validateResourceSpy.callCount, 1, "_validateResource called once"); + t.is(validateResourceSpy.getCall(0).args[0], resource, + "_validateResource called with correct arguments"); + + t.is(validateTagSpy.callCount, 1, "_validateTag called once"); + t.is(validateTagSpy.getCall(0).args[0], "abc:MySuperTag", + "_validateTag called with correct arguments"); +}); + test("_validateTag: Not in list of allowed tags", (t) => { const tagCollection = new ResourceTagCollection({ allowedTags: ["abc:MyTag"] From b3993d8fa2e72bf8fe9dd68acf2e1e0c9465d3ed Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Fri, 3 Dec 2021 00:32:13 +0100 Subject: [PATCH 03/15] [INTERNAL] Rework ReaderFilter to accept a simple filter callback --- lib/AbstractReader.js | 6 ++-- lib/ReaderFilter.js | 54 +++++--------------------------- test/lib/ReaderFilter.js | 66 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 50 deletions(-) create mode 100644 test/lib/ReaderFilter.js diff --git a/lib/AbstractReader.js b/lib/AbstractReader.js index 6475d5e0..5f8fb9fc 100644 --- a/lib/AbstractReader.js +++ b/lib/AbstractReader.js @@ -70,13 +70,11 @@ class AbstractReader { }); } - filter({resourceTagCollection, matchMode, filters}) { + filter(callback) { const ReaderFilter = require("./ReaderFilter"); return new ReaderFilter({ reader: this, - resourceTagCollection, - matchMode, - filters + filterCallback: callback }); } diff --git a/lib/ReaderFilter.js b/lib/ReaderFilter.js index d406e1b2..da18707b 100644 --- a/lib/ReaderFilter.js +++ b/lib/ReaderFilter.js @@ -13,30 +13,19 @@ class ReaderFilter extends AbstractReader { * * @param {object} parameters Parameters * @param {module:@ui5/fs.AbstractReader} parameters.reader A resource reader - * @param {module:@ui5/fs.ResourceTagCollection} parameters.resourceTagCollection - * Resource tag collection to apply filters onto - * @param {object[]} parameters.filters Filters - * @param {string} parameters.matchMode Whether to match "any", "all" or "none" + * @param {Function} parameters.filterCallback + * Filter function. Should return true for items to keep and false otherwise */ - constructor({reader, resourceTagCollection, filters, matchMode}) { + constructor({reader, filterCallback}) { super(); if (!reader) { throw new Error(`Missing parameter "reader"`); } - if (!resourceTagCollection) { - throw new Error(`Missing parameter "resourceTagCollection"`); - } - if (!filters || !filters.length) { - throw new Error(`Missing parameter "filters"`); - } - if (!matchMode) { - throw new Error(`Missing parameter "matchMode"`); + if (!filterCallback) { + throw new Error(`Missing parameter "filterCallback"`); } this._reader = reader; - this._resourceTagCollection = resourceTagCollection; - this._filters = filters; - - this._matchMode = matchMode; + this._filterCallback = filterCallback; } /** @@ -51,7 +40,7 @@ class ReaderFilter extends AbstractReader { */ async _byGlob(pattern, options, trace) { const result = await this._reader._byGlob(pattern, options, trace); - return result.filter(this._filterResource.bind(this)); + return result.filter(this._filterCallback); } /** @@ -66,39 +55,12 @@ class ReaderFilter extends AbstractReader { async _byPath(virPath, options, trace) { const result = await this._reader._byPath(virPath, options, trace); if (result) { - if (!this._filterResource(result)) { + if (!this._filterCallback(result)) { return null; } } return result; } - - _filterResource(resource) { - let filterFunction; - let testEquality = true; - switch (this._matchMode) { - case "any": - filterFunction = "some"; - break; - case "all": - filterFunction = "every"; - break; - case "none": - filterFunction = "every"; - testEquality = false; - break; - default: - throw Error(`Unknown match mode ${this._matchMode}`); - } - return this._filters[filterFunction](({tag, value: filterValue}) => { - const tagValue = this._resourceTagCollection.getTag(resource, tag); - if (testEquality) { - return tagValue === filterValue; - } else { - return tagValue !== filterValue; - } - }); - } } module.exports = ReaderFilter; diff --git a/test/lib/ReaderFilter.js b/test/lib/ReaderFilter.js new file mode 100644 index 00000000..3bef2a68 --- /dev/null +++ b/test/lib/ReaderFilter.js @@ -0,0 +1,66 @@ +const test = require("ava"); +const sinon = require("sinon"); +const ReaderFilter = require("../../lib/ReaderFilter"); + +test("_byGlob: Basic filter", async (t) => { + const abstractReader = { + _byGlob: sinon.stub().returns(Promise.resolve(["resource a", "resource b"])) + }; + const trace = { + collection: sinon.spy() + }; + const readerCollection = new ReaderFilter({ + reader: abstractReader, + filterCallback: function(resource) { + if (resource === "resource a") { + return false; + } + return true; + } + }); + + const resources = await readerCollection._byGlob("anyPattern", {}, trace); + t.deepEqual(resources, ["resource b"], "Correct resource in result"); +}); + +test("_byPath: Negative filter", async (t) => { + const abstractReader = { + _byPath: sinon.stub().returns(Promise.resolve("resource a")) + }; + const trace = { + collection: sinon.spy() + }; + const readerCollection = new ReaderFilter({ + reader: abstractReader, + filterCallback: function(resource) { + if (resource === "resource a") { + return false; + } + return true; + } + }); + + const resources = await readerCollection._byPath("anyPattern", {}, trace); + t.deepEqual(resources, null, "Correct empty in result"); +}); + +test("_byPath: Positive filter", async (t) => { + const abstractReader = { + _byPath: sinon.stub().returns(Promise.resolve("resource b")) + }; + const trace = { + collection: sinon.spy() + }; + const readerCollection = new ReaderFilter({ + reader: abstractReader, + filterCallback: function(resource) { + if (resource === "resource a") { + return false; + } + return true; + } + }); + + const resources = await readerCollection._byPath("anyPattern", {}, trace); + t.deepEqual(resources, "resource b", "Correct resource in result"); +}); From 46a121b7a86278112997f8f563da968cc77ffdfb Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Fri, 3 Dec 2021 15:46:37 +0100 Subject: [PATCH 04/15] [INTERNAL] ReaderFilter: Update JSDoc --- lib/ReaderFilter.js | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/lib/ReaderFilter.js b/lib/ReaderFilter.js index da18707b..7dc354d3 100644 --- a/lib/ReaderFilter.js +++ b/lib/ReaderFilter.js @@ -1,7 +1,7 @@ const AbstractReader = require("./AbstractReader"); /** - * Resource Locator ReaderCollection + * Transparently apply filters on resource readers by wrapping them. * * @public * @memberof module:@ui5/fs @@ -9,12 +9,21 @@ const AbstractReader = require("./AbstractReader"); */ class ReaderFilter extends AbstractReader { /** - * The constructor. + * Filter callback + * + * @public + * @callback module:@ui5/fs.ReaderFilter~filterCallback + * @param {module:@ui5/fs.Resource} resource Resource to test + * @returns {boolean} Return true to keep the resource and false to disregard it + */ + + /** + * Constructor * * @param {object} parameters Parameters - * @param {module:@ui5/fs.AbstractReader} parameters.reader A resource reader - * @param {Function} parameters.filterCallback - * Filter function. Should return true for items to keep and false otherwise + * @param {module:@ui5/fs.AbstractReader} parameters.reader The resource reader to wrap + * @param {module:@ui5/fs.ReaderFilter~filterCallback} parameters.filterCallback + * Filter function. Will be called for every resource read through this reader. */ constructor({reader, filterCallback}) { super(); From 00220d90829070a61d162870f72d171dbfaf307e Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Mon, 6 Dec 2021 12:51:33 +0100 Subject: [PATCH 05/15] [INTERNAL] Apply suggestions from code review Co-authored-by: KlattG <57760635+KlattG@users.noreply.github.com> --- lib/ReaderFilter.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ReaderFilter.js b/lib/ReaderFilter.js index 7dc354d3..aade9065 100644 --- a/lib/ReaderFilter.js +++ b/lib/ReaderFilter.js @@ -14,7 +14,7 @@ class ReaderFilter extends AbstractReader { * @public * @callback module:@ui5/fs.ReaderFilter~filterCallback * @param {module:@ui5/fs.Resource} resource Resource to test - * @returns {boolean} Return true to keep the resource and false to disregard it + * @returns {boolean} Return true to keep the resource, or false to disregard it */ /** From 2afde49a117681f09bdf71a06e53dfc189de0bb5 Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Fri, 17 Dec 2021 14:07:35 +0100 Subject: [PATCH 06/15] [FEATURE] Introduce ReaderTransformer --- lib/AbstractReader.js | 10 ++- lib/ReaderFilter.js | 16 ++-- lib/ReaderTransformer.js | 84 ++++++++++++++++++ test/lib/ReaderFilter.js | 14 +-- test/lib/ReaderTransformer.js | 157 ++++++++++++++++++++++++++++++++++ 5 files changed, 265 insertions(+), 16 deletions(-) create mode 100644 lib/ReaderTransformer.js create mode 100644 test/lib/ReaderTransformer.js diff --git a/lib/AbstractReader.js b/lib/AbstractReader.js index 5f8fb9fc..58c3b887 100644 --- a/lib/AbstractReader.js +++ b/lib/AbstractReader.js @@ -74,7 +74,15 @@ class AbstractReader { const ReaderFilter = require("./ReaderFilter"); return new ReaderFilter({ reader: this, - filterCallback: callback + callback + }); + } + + transformer(callback) { + const ReaderTransformer = require("./ReaderTransformer"); + return new ReaderTransformer({ + reader: this, + callback }); } diff --git a/lib/ReaderFilter.js b/lib/ReaderFilter.js index aade9065..0085ca56 100644 --- a/lib/ReaderFilter.js +++ b/lib/ReaderFilter.js @@ -12,7 +12,7 @@ class ReaderFilter extends AbstractReader { * Filter callback * * @public - * @callback module:@ui5/fs.ReaderFilter~filterCallback + * @callback module:@ui5/fs.ReaderFilter~callback * @param {module:@ui5/fs.Resource} resource Resource to test * @returns {boolean} Return true to keep the resource, or false to disregard it */ @@ -22,19 +22,19 @@ class ReaderFilter extends AbstractReader { * * @param {object} parameters Parameters * @param {module:@ui5/fs.AbstractReader} parameters.reader The resource reader to wrap - * @param {module:@ui5/fs.ReaderFilter~filterCallback} parameters.filterCallback + * @param {module:@ui5/fs.ReaderFilter~callback} parameters.callback * Filter function. Will be called for every resource read through this reader. */ - constructor({reader, filterCallback}) { + constructor({reader, callback}) { super(); if (!reader) { throw new Error(`Missing parameter "reader"`); } - if (!filterCallback) { - throw new Error(`Missing parameter "filterCallback"`); + if (!callback) { + throw new Error(`Missing parameter "callback"`); } this._reader = reader; - this._filterCallback = filterCallback; + this._callback = callback; } /** @@ -49,7 +49,7 @@ class ReaderFilter extends AbstractReader { */ async _byGlob(pattern, options, trace) { const result = await this._reader._byGlob(pattern, options, trace); - return result.filter(this._filterCallback); + return result.filter(this._callback); } /** @@ -64,7 +64,7 @@ class ReaderFilter extends AbstractReader { async _byPath(virPath, options, trace) { const result = await this._reader._byPath(virPath, options, trace); if (result) { - if (!this._filterCallback(result)) { + if (!this._callback(result)) { return null; } } diff --git a/lib/ReaderTransformer.js b/lib/ReaderTransformer.js new file mode 100644 index 00000000..2431a846 --- /dev/null +++ b/lib/ReaderTransformer.js @@ -0,0 +1,84 @@ +const AbstractReader = require("./AbstractReader"); + +/** + * Transparently apply filters on resource readers by wrapping them. + * + * @public + * @memberof module:@ui5/fs + * @augments module:@ui5/fs.AbstractReader + */ +class ReaderTransformer extends AbstractReader { + /** + * Filter callback + * + * @public + * @callback module:@ui5/fs.ReaderTransformer~callback + * @param {module:@ui5/fs.Resource} resource Resource to transform + * @returns {Promise|undefined} Optional promise resolving once the resource has been transformed + */ + + /** + * Constructor + * + * @param {object} parameters Parameters + * @param {module:@ui5/fs.AbstractReader} parameters.reader The resource reader to wrap + * @param {module:@ui5/fs.ReaderTransformer~callback} parameters.callback + * Filter function. Will be called for every resource read through this reader. + */ + constructor({reader, callback}) { + super(); + if (!reader) { + throw new Error(`Missing parameter "reader"`); + } + if (!callback) { + throw new Error(`Missing parameter "callback"`); + } + this._reader = reader; + this._callback = callback; + } + + /** + * Locates resources by glob. + * + * @private + * @param {string|string[]} pattern glob pattern as string or an array of + * glob patterns for virtual directory structure + * @param {object} options glob options + * @param {module:@ui5/fs.tracing.Trace} trace Trace instance + * @returns {Promise} Promise resolving to list of resources + */ + async _byGlob(pattern, options, trace) { + const result = await this._reader._byGlob(pattern, options, trace); + return Promise.all(result.map(async (resource) => { + resource = await resource.clone(); + const res = this._callback(resource); + if (res && res instanceof Promise) { + await res; + } + return resource; + })); + } + + /** + * Locates resources by path. + * + * @private + * @param {string} virPath Virtual path + * @param {object} options Options + * @param {module:@ui5/fs.tracing.Trace} trace Trace instance + * @returns {Promise} Promise resolving to a single resource + */ + async _byPath(virPath, options, trace) { + let resource = await this._reader._byPath(virPath, options, trace); + if (resource) { + resource = await resource.clone(); + const res = this._callback(resource); + if (res && res instanceof Promise) { + await res; + } + } + return resource; + } +} + +module.exports = ReaderTransformer; diff --git a/test/lib/ReaderFilter.js b/test/lib/ReaderFilter.js index 3bef2a68..3c9864ea 100644 --- a/test/lib/ReaderFilter.js +++ b/test/lib/ReaderFilter.js @@ -11,7 +11,7 @@ test("_byGlob: Basic filter", async (t) => { }; const readerCollection = new ReaderFilter({ reader: abstractReader, - filterCallback: function(resource) { + callback: function(resource) { if (resource === "resource a") { return false; } @@ -32,7 +32,7 @@ test("_byPath: Negative filter", async (t) => { }; const readerCollection = new ReaderFilter({ reader: abstractReader, - filterCallback: function(resource) { + callback: function(resource) { if (resource === "resource a") { return false; } @@ -40,8 +40,8 @@ test("_byPath: Negative filter", async (t) => { } }); - const resources = await readerCollection._byPath("anyPattern", {}, trace); - t.deepEqual(resources, null, "Correct empty in result"); + const resource = await readerCollection._byPath("anyPattern", {}, trace); + t.deepEqual(resource, null, "Correct empty result"); }); test("_byPath: Positive filter", async (t) => { @@ -53,7 +53,7 @@ test("_byPath: Positive filter", async (t) => { }; const readerCollection = new ReaderFilter({ reader: abstractReader, - filterCallback: function(resource) { + callback: function(resource) { if (resource === "resource a") { return false; } @@ -61,6 +61,6 @@ test("_byPath: Positive filter", async (t) => { } }); - const resources = await readerCollection._byPath("anyPattern", {}, trace); - t.deepEqual(resources, "resource b", "Correct resource in result"); + const resource = await readerCollection._byPath("anyPattern", {}, trace); + t.deepEqual(resource, "resource b", "Correct resource in result"); }); diff --git a/test/lib/ReaderTransformer.js b/test/lib/ReaderTransformer.js new file mode 100644 index 00000000..b2026edc --- /dev/null +++ b/test/lib/ReaderTransformer.js @@ -0,0 +1,157 @@ +const test = require("ava"); +const sinon = require("sinon"); +const ReaderTransformer = require("../../lib/ReaderTransformer"); + +function getDummyResource(name) { + return { + name, + clone: function() { + return getDummyResource(name); + } + }; +} + +test("_byGlob: Basic transformation", async (t) => { + const resourceA = getDummyResource("resource a"); + const resourceB = getDummyResource("resource b"); + const abstractReader = { + _byGlob: sinon.stub().returns(Promise.resolve([resourceA, resourceB])) + }; + const trace = { + collection: sinon.spy() + }; + const readerCollection = new ReaderTransformer({ + reader: abstractReader, + callback: function(resource) { + if (resource.name === "resource a") { + resource.name = "transformed resource a"; + } + } + }); + + const resources = await readerCollection._byGlob("anyPattern", {}, trace); + t.deepEqual(resources.length, 2, "Still two resources in result set"); + t.deepEqual(resources[0].name, "transformed resource a", "resource a has been transformed in result"); + t.deepEqual(resources[1].name, "resource b", "resource b has not been transformed"); + t.not(resources[1], resourceB, "resource b instance has been cloned"); + t.deepEqual(resourceA.name, "resource a", "Original resource a has not been transformed"); +}); + +test("_byPath: Basic transformation", async (t) => { + const resourceA = getDummyResource("resource a"); + const abstractReader = { + _byPath: sinon.stub().returns(resourceA) + }; + const trace = { + collection: sinon.spy() + }; + const readerCollection = new ReaderTransformer({ + reader: abstractReader, + callback: function(resource) { + resource.name = "transformed resource a"; + } + }); + + const resource = await readerCollection._byPath("anyPattern", {}, trace); + + t.deepEqual(resource.name, "transformed resource a", "resource a has been transformed in result"); + t.deepEqual(resourceA.name, "resource a", "Original resource a has not been transformed"); +}); + +test("_byPath: No transformation", async (t) => { + const resourceB = getDummyResource("resource b"); + const abstractReader = { + _byPath: sinon.stub().returns(resourceB) + }; + const trace = { + collection: sinon.spy() + }; + const readerCollection = new ReaderTransformer({ + reader: abstractReader, + callback: function(resource) { + return; + } + }); + + const resource = await readerCollection._byPath("anyPattern", {}, trace); + t.deepEqual(resource.name, "resource b", "Correct resource in result"); + t.not(resource, resourceB, "resource b instance has been cloned"); +}); + + +test("async _byGlob: Basic transformation", async (t) => { + const resourceA = getDummyResource("resource a"); + const resourceB = getDummyResource("resource b"); + const abstractReader = { + _byGlob: sinon.stub().returns(Promise.resolve([resourceA, resourceB])) + }; + const trace = { + collection: sinon.spy() + }; + const readerCollection = new ReaderTransformer({ + reader: abstractReader, + callback: function(resource) { + return new Promise((resolve) => { + setTimeout(() => { + if (resource.name === "resource a") { + resource.name = "transformed resource a"; + } + resolve(); + }, 10); + }); + } + }); + + const resources = await readerCollection._byGlob("anyPattern", {}, trace); + t.deepEqual(resources.length, 2, "Still two resources in result set"); + t.deepEqual(resources[0].name, "transformed resource a", "resource a has been transformed in result"); + t.deepEqual(resources[1].name, "resource b", "resource b has not been transformed"); + t.not(resources[1], resourceB, "resource b instance has been cloned"); + t.deepEqual(resourceA.name, "resource a", "Original resource a has not been transformed"); +}); + +test("async _byPath: Basic transformation", async (t) => { + const resourceA = getDummyResource("resource a"); + const abstractReader = { + _byPath: sinon.stub().returns(resourceA) + }; + const trace = { + collection: sinon.spy() + }; + const readerCollection = new ReaderTransformer({ + reader: abstractReader, + callback: function(resource) { + return new Promise((resolve) => { + setTimeout(() => { + resource.name = "transformed resource a"; + resolve(); + }, 10); + }); + } + }); + + const resource = await readerCollection._byPath("anyPattern", {}, trace); + + t.deepEqual(resource.name, "transformed resource a", "resource a has been transformed in result"); + t.deepEqual(resourceA.name, "resource a", "Original resource a has not been transformed"); +}); + +test("async _byPath: No transformation", async (t) => { + const resourceB = getDummyResource("resource b"); + const abstractReader = { + _byPath: sinon.stub().returns(resourceB) + }; + const trace = { + collection: sinon.spy() + }; + const readerCollection = new ReaderTransformer({ + reader: abstractReader, + callback: async function(resource) { + return; + } + }); + + const resource = await readerCollection._byPath("anyPattern", {}, trace); + t.deepEqual(resource.name, "resource b", "Correct resource in result"); + t.not(resource, resourceB, "resource b instance has been cloned"); +}); From ecd7bd600c8735ffd946ebadd5bd2a82734a4168 Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Fri, 17 Dec 2021 14:08:37 +0100 Subject: [PATCH 07/15] [INTERNAL] Move resource content into separate instance This should make resource clone operations cheaper since the content doesn't need to be read and cloned immediately --- lib/Resource.js | 189 +++++---------------- lib/ResourceContent.js | 232 ++++++++++++++++++++++++++ test/lib/Resource.js | 24 +-- test/lib/adapters/FileSystem_write.js | 2 +- 4 files changed, 287 insertions(+), 160 deletions(-) create mode 100644 lib/ResourceContent.js diff --git a/lib/Resource.js b/lib/Resource.js index ecdd96f8..fb0c0923 100644 --- a/lib/Resource.js +++ b/lib/Resource.js @@ -1,6 +1,6 @@ -const stream = require("stream"); const clone = require("clone"); -const path = require("path"); +const nodePath = require("path"); +const ResourceContent = require("./ResourceContent"); const fnTrue = () => true; const fnFalse = () => false; @@ -38,16 +38,19 @@ class Resource { * stream of the content of this resource (cannot be used in conjunction with parameters buffer, * string or stream). * In some cases this is the most memory-efficient way to supply resource content + * @param {module:@ui5/fs.ResourceContent} [parameters.contentInstance] Resource content instance * @param {object} [parameters.project] Experimental, internal parameter. Do not use */ - constructor({path, statInfo, buffer, string, createStream, stream, project}) { + constructor({path, statInfo, buffer, string, createStream, stream, contentInstance, project}) { if (!path) { throw new Error("Cannot create Resource: path parameter missing"); } - if (buffer && createStream || buffer && string || string && createStream || buffer && stream || - string && stream || createStream && stream) { + if (buffer && createStream || buffer && string || buffer && stream || buffer && contentInstance || + string && createStream || string && stream || string && contentInstance || + stream && createStream || stream && contentInstance || + contentInstance && createStream) { throw new Error("Cannot create Resource: Please set only one content parameter. " + - "Buffer, string, stream or createStream"); + "Buffer, string, stream, createStream or contentInstance"); } this._path = path; @@ -71,14 +74,14 @@ class Resource { birthtime: new Date() }; - if (createStream) { - this._createStream = createStream; - } else if (stream) { - this._stream = stream; - } else if (buffer) { - this.setBuffer(buffer); - } else if (typeof string === "string" || string instanceof String) { - this.setString(string); + if (contentInstance) { + this._content = contentInstance; + } else { + try { + this._content = new ResourceContent({buffer, string, createStream, stream}); + } catch (err) { + throw new Error(`Failed to create content for resource ${this._path}: ${err.message}`); + } } // Tracing: @@ -92,17 +95,10 @@ class Resource { * @returns {Promise} Promise resolving with a buffer of the resource content. */ async getBuffer() { - if (this._contentDrained) { - throw new Error(`Content of Resource ${this._path} has been drained. ` + - "This might be caused by requesting resource content after a content stream has been " + - "requested and no new content (e.g. a new stream) has been set."); - } - if (this._buffer) { - return this._buffer; - } else if (this._createStream || this._stream) { - return this._getBufferFromStream(); - } else { - throw new Error(`Resource ${this._path} has no content`); + try { + return await this._content.getBuffer(); + } catch (err) { + throw new Error(`Failed to get buffer for resource ${this._path}: ${err.message}`); } } @@ -113,14 +109,7 @@ class Resource { * @param {Buffer} buffer Buffer instance */ setBuffer(buffer) { - this._createStream = null; - // if (this._stream) { // TODO this may cause strange issues - // this._stream.destroy(); - // } - this._stream = null; - this._buffer = buffer; - this._contentDrained = false; - this._streamDrained = false; + this._content = new ResourceContent({buffer}); } /** @@ -129,13 +118,12 @@ class Resource { * @public * @returns {Promise} Promise resolving with the resource content. */ - getString() { - if (this._contentDrained) { - return Promise.reject(new Error(`Content of Resource ${this._path} has been drained. ` + - "This might be caused by requesting resource content after a content stream has been " + - "requested and no new content (e.g. a new stream) has been set.")); + async getString() { + try { + return await this._content.getString(); + } catch (err) { + throw new Error(`Failed to get string for resource ${this._path}: ${err.message}`); } - return this.getBuffer().then((buffer) => buffer.toString()); } /** @@ -145,7 +133,7 @@ class Resource { * @param {string} string Resource content */ setString(string) { - this.setBuffer(Buffer.from(string, "utf8")); + this._content = new ResourceContent({string}); } /** @@ -160,32 +148,11 @@ class Resource { * @returns {stream.Readable} Readable stream for the resource content. */ getStream() { - if (this._contentDrained) { - throw new Error(`Content of Resource ${this._path} has been drained. ` + - "This might be caused by requesting resource content after a content stream has been " + - "requested and no new content (e.g. a new stream) has been set."); - } - let contentStream; - if (this._buffer) { - const bufferStream = new stream.PassThrough(); - bufferStream.end(this._buffer); - contentStream = bufferStream; - } else if (this._createStream || this._stream) { - contentStream = this._getStream(); - } - if (!contentStream) { - throw new Error(`Resource ${this._path} has no content`); + try { + return this._content.getStream(); + } catch (err) { + throw new Error(`Failed to get stream for resource ${this._path}: ${err.message}`); } - // If a stream instance is being returned, it will typically get drained be the consumer. - // In that case, further content access will result in a "Content stream has been drained" error. - // However, depending on the execution environment, a resources content stream might have been - // transformed into a buffer. In that case further content access is possible as a buffer can't be - // drained. - // To prevent unexpected "Content stream has been drained" errors caused by changing environments, we flag - // the resource content as "drained" every time a stream is requested. Even if actually a buffer or - // createStream callback is being used. - this._contentDrained = true; - return contentStream; } /** @@ -196,19 +163,13 @@ class Resource { callback for dynamic creation of a readable stream */ setStream(stream) { - this._buffer = null; - // if (this._stream) { // TODO this may cause strange issues - // this._stream.destroy(); - // } + const options = {}; if (typeof stream === "function") { - this._createStream = stream; - this._stream = null; + options.createStream = stream; } else { - this._stream = stream; - this._createStream = null; + options.stream = stream; } - this._contentDrained = false; - this._streamDrained = false; + this._content = new ResourceContent(options); } /** @@ -253,16 +214,11 @@ class Resource { * @returns {Promise} size in bytes, 0 if there is no content yet */ async getSize() { - // if resource does not have any content it should have 0 bytes - if (!this._buffer && !this._createStream && !this._stream) { - return 0; - } - const buffer = await this.getBuffer(); - return buffer.byteLength; + return this._content.getSize(); } _getNameFromPath(virPath) { - return path.posix.basename(virPath); + return nodePath.posix.basename(virPath); } /** @@ -280,30 +236,14 @@ class Resource { * @public * @returns {Promise} Promise resolving with the clone */ - clone() { + async clone() { const options = { path: this._path, - statInfo: clone(this._statInfo) - }; - - const addContentOption = () => { - if (this._stream) { - return this._getBufferFromStream().then(function(buffer) { - options.buffer = buffer; - }); - } else { - if (this._createStream) { - options.createStream = this._createStream; - } else if (this._buffer) { - options.buffer = this._buffer; - } - return Promise.resolve(); - } + statInfo: clone(this._statInfo), + contentInstance: this._content }; - return addContentOption().then(() => { - return new Resource(options); - }); + return new Resource(options); } /** @@ -322,51 +262,6 @@ class Resource { return tree; } - - /** - * Returns the content as stream. - * - * @private - * @returns {stream.Readable} Readable stream - */ - _getStream() { - if (this._streamDrained) { - throw new Error(`Content stream of Resource ${this._path} is flagged as drained.`); - } - if (this._createStream) { - return this._createStream(); - } - this._streamDrained = true; - return this._stream; - } - - /** - * Converts the buffer into a stream. - * - * @private - * @returns {Promise} Promise resolving with buffer. - */ - _getBufferFromStream() { - if (this._buffering) { // Prevent simultaneous buffering, causing unexpected access to drained stream - return this._buffering; - } - return this._buffering = new Promise((resolve, reject) => { - const contentStream = this._getStream(); - const buffers = []; - contentStream.on("data", (data) => { - buffers.push(data); - }); - contentStream.on("error", (err) => { - reject(err); - }); - contentStream.on("end", () => { - const buffer = Buffer.concat(buffers); - this.setBuffer(buffer); - this._buffering = null; - resolve(buffer); - }); - }); - } } module.exports = Resource; diff --git a/lib/ResourceContent.js b/lib/ResourceContent.js new file mode 100644 index 00000000..584be4f7 --- /dev/null +++ b/lib/ResourceContent.js @@ -0,0 +1,232 @@ +const stream = require("stream"); + +/** + * Content of a resource + * + * @private + * @memberof module:@ui5/fs + */ +class ResourceContent { + /** + * + * + * @public + * @param {object} parameters Parameters + * [fs.Stats]{@link https://nodejs.org/api/fs.html#fs_class_fs_stats} or similar object + * @param {Buffer} [parameters.buffer] Content of this resources as a Buffer instance + * (cannot be used in conjunction with parameters string, stream or createStream) + * @param {string} [parameters.string] Content of this resources as a string + * (cannot be used in conjunction with parameters buffer, stream or createStream) + * @param {Stream} [parameters.stream] Readable stream of the content of this resource + * (cannot be used in conjunction with parameters buffer, string or createStream) + * @param {module:@ui5/fs.Resource~createStream} [parameters.createStream] Function callback that returns a readable + * stream of the content of this resource (cannot be used in conjunction with parameters buffer, + * string or stream). + * In some cases this is the most memory-efficient way to supply resource content + */ + constructor({buffer, string, createStream, stream}) { + if (buffer && createStream || buffer && string || string && createStream || buffer && stream || + string && stream || createStream && stream) { + throw new Error("Cannot create ResourceContent: Please set only one content parameter. " + + "Buffer, string, stream or createStream"); + } + + if (createStream) { + this._createStream = createStream; + } else if (stream) { + this._stream = stream; + } else if (buffer) { + this._setBuffer(buffer); + } else if (typeof string === "string" || string instanceof String) { + this._setString(string); + } + } + + /** + * Gets a buffer with the resource content. + * + * @public + * @returns {Promise} Promise resolving with a buffer of the resource content. + */ + async getBuffer() { + if (this._contentDrained) { + throw new Error(`Content stream has been drained. ` + + "This might be caused by requesting resource content after a content stream has been " + + "requested and no new content (e.g. a new stream) has been set."); + } + if (this._buffer) { + return this._buffer; + } else if (this._createStream || this._stream) { + return this._getBufferFromStream(); + } else { + throw new Error(`No content available`); + } + } + + /** + * Sets a Buffer as content. + * + * @public + * @param {Buffer} buffer Buffer instance + */ + _setBuffer(buffer) { + this._createStream = null; + // if (this._stream) { // TODO this may cause strange issues + // this._stream.destroy(); + // } + this._stream = null; + this._buffer = buffer; + this._contentDrained = false; + this._streamDrained = false; + } + + /** + * Gets a string with the resource content. + * + * @public + * @returns {Promise} Promise resolving with the resource content. + */ + async getString() { + if (this._contentDrained) { + throw new Error(`Content stream has been drained. ` + + "This might be caused by requesting resource content after a content stream has been " + + "requested and no new content (e.g. a new stream) has been set."); + } + return this.getBuffer().then((buffer) => buffer.toString()); + } + + /** + * Sets a String as content + * + * @public + * @param {string} string Resource content + */ + _setString(string) { + this._setBuffer(Buffer.from(string, "utf8")); + } + + /** + * Gets a readable stream for the resource content. + * + * Repetitive calls of this function are only possible if new content has been set in the meantime (through + * [setStream]{@link module:@ui5/fs.Resource#setStream}, [setBuffer]{@link module:@ui5/fs.Resource#setBuffer} + * or [setString]{@link module:@ui5/fs.Resource#setString}). This + * is to prevent consumers from accessing drained streams. + * + * @public + * @returns {stream.Readable} Readable stream for the resource content. + */ + getStream() { + if (this._contentDrained) { + throw new Error(`Content stream has been drained. ` + + "This might be caused by requesting resource content after a content stream has been " + + "requested and no new content (e.g. a new stream) has been set."); + } + let contentStream; + if (this._buffer) { + const bufferStream = new stream.PassThrough(); + bufferStream.end(this._buffer); + contentStream = bufferStream; + } else if (this._createStream || this._stream) { + contentStream = this._getStream(); + } + if (!contentStream) { + throw new Error(`No content available`); + } + // If a stream instance is being returned, it will typically get drained by the consumer. + // In that case, further content access will result in a "Content stream has been drained" error. + // However, depending on the execution environment, a resources content stream might have been + // transformed into a buffer. In that case further content access is possible as a buffer can't be + // drained. + // To prevent unexpected "Content stream has been drained" errors caused by changing environments, we flag + // the resource content as "drained" every time a stream is requested. Even if actually a buffer or + // createStream callback is being used. + this._contentDrained = true; + return contentStream; + } + + /** + * Sets a readable stream as content. + * + * @public + * @param {stream.Readable|module:@ui5/fs.Resource~createStream} stream Readable stream of the resource content or + callback for dynamic creation of a readable stream + */ + _setStream(stream) { + this._buffer = null; + // if (this._stream) { // TODO this may cause strange issues + // this._stream.destroy(); + // } + if (typeof stream === "function") { + this._createStream = stream; + this._stream = null; + } else { + this._stream = stream; + this._createStream = null; + } + this._contentDrained = false; + this._streamDrained = false; + } + + /** + * Size in bytes allocated by the underlying buffer. + * + * @see {TypedArray#byteLength} + * @returns {Promise} size in bytes, 0 if there is no content yet + */ + async getSize() { + // if resource does not have any content it should have 0 bytes + if (!this._buffer && !this._createStream && !this._stream) { + return 0; + } + const buffer = await this.getBuffer(); + return buffer.byteLength; + } + + /** + * Returns the content as stream. + * + * @private + * @returns {stream.Readable} Readable stream + */ + _getStream() { + if (this._streamDrained) { + throw new Error(`Content stream is flagged as drained.`); + } + if (this._createStream) { + return this._createStream(); + } + this._streamDrained = true; + return this._stream; + } + + /** + * Converts the buffer into a stream. + * + * @private + * @returns {Promise} Promise resolving with buffer. + */ + _getBufferFromStream() { + if (this._buffering) { // Prevent simultaneous buffering, causing unexpected access to drained stream + return this._buffering; + } + return this._buffering = new Promise((resolve, reject) => { + const contentStream = this._getStream(); + const buffers = []; + contentStream.on("data", (data) => { + buffers.push(data); + }); + contentStream.on("error", (err) => { + reject(err); + }); + contentStream.on("end", () => { + const buffer = Buffer.concat(buffers); + this._setBuffer(buffer); + this._buffering = null; + resolve(buffer); + }); + }); + } +} + +module.exports = ResourceContent; diff --git a/test/lib/Resource.js b/test/lib/Resource.js index f61c5d52..d90d6dea 100644 --- a/test/lib/Resource.js +++ b/test/lib/Resource.js @@ -60,7 +60,7 @@ test("Resource: constructor with duplicated content parameter", (t) => { }, { instanceOf: Error, message: "Cannot create Resource: Please set only one content parameter. " + - "Buffer, string, stream or createStream" + "Buffer, string, stream, createStream or contentInstance" }); }); @@ -72,7 +72,7 @@ test("Resource: getBuffer with throwing an error", (t) => { }); return resource.getBuffer().catch(function(error) { - t.is(error.message, "Resource my/path/to/resource has no content", + t.is(error.message, "Failed to get buffer for resource my/path/to/resource: No content available", "getBuffer called w/o having a resource content provided"); }); }); @@ -126,7 +126,7 @@ test("Resource: getStream throwing an error", (t) => { resource.getStream(); }, { instanceOf: Error, - message: "Resource my/path/to/resource has no content" + message: "Failed to get stream for resource my/path/to/resource: No content available" }); }); @@ -267,9 +267,9 @@ test("getStream with createStream callback content: Subsequent content requests resource.getStream(); t.throws(() => { resource.getStream(); - }, {message: /Content of Resource \/app\/index.html has been drained/}); - await t.throwsAsync(resource.getBuffer(), {message: /Content of Resource \/app\/index.html has been drained/}); - await t.throwsAsync(resource.getString(), {message: /Content of Resource \/app\/index.html has been drained/}); + }, {message: /Content stream has been drained/}); + await t.throwsAsync(resource.getBuffer(), {message: /Content stream has been drained/}); + await t.throwsAsync(resource.getString(), {message: /Content stream has been drained/}); }); test("getStream with Buffer content: Subsequent content requests should throw error due to drained " + @@ -279,9 +279,9 @@ test("getStream with Buffer content: Subsequent content requests should throw er resource.getStream(); t.throws(() => { resource.getStream(); - }, {message: /Content of Resource \/app\/index.html has been drained/}); - await t.throwsAsync(resource.getBuffer(), {message: /Content of Resource \/app\/index.html has been drained/}); - await t.throwsAsync(resource.getString(), {message: /Content of Resource \/app\/index.html has been drained/}); + }, {message: /Content stream has been drained/}); + await t.throwsAsync(resource.getBuffer(), {message: /Content stream has been drained/}); + await t.throwsAsync(resource.getString(), {message: /Content stream has been drained/}); }); test("getStream with Stream content: Subsequent content requests should throw error due to drained " + @@ -301,9 +301,9 @@ test("getStream with Stream content: Subsequent content requests should throw er resource.getStream(); t.throws(() => { resource.getStream(); - }, {message: /Content of Resource \/app\/index.html has been drained/}); - await t.throwsAsync(resource.getBuffer(), {message: /Content of Resource \/app\/index.html has been drained/}); - await t.throwsAsync(resource.getString(), {message: /Content of Resource \/app\/index.html has been drained/}); + }, {message: /Content stream has been drained/}); + await t.throwsAsync(resource.getBuffer(), {message: /Content stream has been drained/}); + await t.throwsAsync(resource.getString(), {message: /Content stream has been drained/}); }); test("getBuffer from Stream content: Subsequent content requests should not throw error due to drained " + diff --git a/test/lib/adapters/FileSystem_write.js b/test/lib/adapters/FileSystem_write.js index 3c15be84..a69f86fc 100644 --- a/test/lib/adapters/FileSystem_write.js +++ b/test/lib/adapters/FileSystem_write.js @@ -82,7 +82,7 @@ test("Write resource in drain mode", async (t) => { assert.fileEqual(destFsPath, "./test/fixtures/application.a/webapp/index.html"); }); await t.throwsAsync(resource.getBuffer(), - {message: /Content of Resource \/app\/index.html has been drained/}); + {message: /Failed to get buffer for resource \/app\/index.html: Content stream has been drained/}); }); test("Writing with readOnly and drain options set should fail", async (t) => { From a48a3ac84f3701b4d2d40171b2b695aa945fb4b4 Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Fri, 17 Dec 2021 14:28:47 +0100 Subject: [PATCH 08/15] [INTERNAL] Move Reader and Transformer to 'readers' subdir --- index.js | 10 ++++++++++ lib/AbstractReader.js | 8 ++++---- lib/{ReaderFilter.js => readers/Filter.js} | 10 +++++----- .../Transformer.js} | 12 ++++++------ test/lib/{ReaderFilter.js => readers/Filter.js} | 8 ++++---- .../Transformer.js} | 14 +++++++------- 6 files changed, 36 insertions(+), 26 deletions(-) rename lib/{ReaderFilter.js => readers/Filter.js} (88%) rename lib/{ReaderTransformer.js => readers/Transformer.js} (87%) rename test/lib/{ReaderFilter.js => readers/Filter.js} (88%) rename test/lib/{ReaderTransformer.js => readers/Transformer.js} (92%) diff --git a/index.js b/index.js index b1b1d920..23338ac8 100644 --- a/index.js +++ b/index.js @@ -22,6 +22,16 @@ module.exports = { */ Memory: "./lib/adapters/Memory" }, + readers: { + /** + * @type {typeof import('./lib/readers/Filter')} + */ + Filter: "./lib/readers/Filter", + /** + * @type {typeof import('./lib/readers/Transformer')} + */ + Transformer: "./lib/readers/Transformer", + }, /** * @type {typeof import('./lib/AbstractReader')} */ diff --git a/lib/AbstractReader.js b/lib/AbstractReader.js index 58c3b887..ee0aac16 100644 --- a/lib/AbstractReader.js +++ b/lib/AbstractReader.js @@ -71,16 +71,16 @@ class AbstractReader { } filter(callback) { - const ReaderFilter = require("./ReaderFilter"); - return new ReaderFilter({ + const Filter = require("./readers/Filter"); + return new Filter({ reader: this, callback }); } transformer(callback) { - const ReaderTransformer = require("./ReaderTransformer"); - return new ReaderTransformer({ + const Transformer = require("./readers/Transformer"); + return new Transformer({ reader: this, callback }); diff --git a/lib/ReaderFilter.js b/lib/readers/Filter.js similarity index 88% rename from lib/ReaderFilter.js rename to lib/readers/Filter.js index 0085ca56..79951b6b 100644 --- a/lib/ReaderFilter.js +++ b/lib/readers/Filter.js @@ -1,4 +1,4 @@ -const AbstractReader = require("./AbstractReader"); +const AbstractReader = require("../AbstractReader"); /** * Transparently apply filters on resource readers by wrapping them. @@ -7,12 +7,12 @@ const AbstractReader = require("./AbstractReader"); * @memberof module:@ui5/fs * @augments module:@ui5/fs.AbstractReader */ -class ReaderFilter extends AbstractReader { +class Filter extends AbstractReader { /** * Filter callback * * @public - * @callback module:@ui5/fs.ReaderFilter~callback + * @callback module:@ui5/fs.readers.Filter~callback * @param {module:@ui5/fs.Resource} resource Resource to test * @returns {boolean} Return true to keep the resource, or false to disregard it */ @@ -22,7 +22,7 @@ class ReaderFilter extends AbstractReader { * * @param {object} parameters Parameters * @param {module:@ui5/fs.AbstractReader} parameters.reader The resource reader to wrap - * @param {module:@ui5/fs.ReaderFilter~callback} parameters.callback + * @param {module:@ui5/fs.readers.Filter~callback} parameters.callback * Filter function. Will be called for every resource read through this reader. */ constructor({reader, callback}) { @@ -72,4 +72,4 @@ class ReaderFilter extends AbstractReader { } } -module.exports = ReaderFilter; +module.exports = Filter; diff --git a/lib/ReaderTransformer.js b/lib/readers/Transformer.js similarity index 87% rename from lib/ReaderTransformer.js rename to lib/readers/Transformer.js index 2431a846..d5586e42 100644 --- a/lib/ReaderTransformer.js +++ b/lib/readers/Transformer.js @@ -1,18 +1,18 @@ -const AbstractReader = require("./AbstractReader"); +const AbstractReader = require("../AbstractReader"); /** * Transparently apply filters on resource readers by wrapping them. * * @public - * @memberof module:@ui5/fs + * @memberof module:@ui5/fs.readers * @augments module:@ui5/fs.AbstractReader */ -class ReaderTransformer extends AbstractReader { +class Transformer extends AbstractReader { /** * Filter callback * * @public - * @callback module:@ui5/fs.ReaderTransformer~callback + * @callback module:@ui5/fs.readers.Transformer~callback * @param {module:@ui5/fs.Resource} resource Resource to transform * @returns {Promise|undefined} Optional promise resolving once the resource has been transformed */ @@ -22,7 +22,7 @@ class ReaderTransformer extends AbstractReader { * * @param {object} parameters Parameters * @param {module:@ui5/fs.AbstractReader} parameters.reader The resource reader to wrap - * @param {module:@ui5/fs.ReaderTransformer~callback} parameters.callback + * @param {module:@ui5/fs.readers.Transformer~callback} parameters.callback * Filter function. Will be called for every resource read through this reader. */ constructor({reader, callback}) { @@ -81,4 +81,4 @@ class ReaderTransformer extends AbstractReader { } } -module.exports = ReaderTransformer; +module.exports = Transformer; diff --git a/test/lib/ReaderFilter.js b/test/lib/readers/Filter.js similarity index 88% rename from test/lib/ReaderFilter.js rename to test/lib/readers/Filter.js index 3c9864ea..f3487bdc 100644 --- a/test/lib/ReaderFilter.js +++ b/test/lib/readers/Filter.js @@ -1,6 +1,6 @@ const test = require("ava"); const sinon = require("sinon"); -const ReaderFilter = require("../../lib/ReaderFilter"); +const Filter = require("../../../lib/readers/Filter"); test("_byGlob: Basic filter", async (t) => { const abstractReader = { @@ -9,7 +9,7 @@ test("_byGlob: Basic filter", async (t) => { const trace = { collection: sinon.spy() }; - const readerCollection = new ReaderFilter({ + const readerCollection = new Filter({ reader: abstractReader, callback: function(resource) { if (resource === "resource a") { @@ -30,7 +30,7 @@ test("_byPath: Negative filter", async (t) => { const trace = { collection: sinon.spy() }; - const readerCollection = new ReaderFilter({ + const readerCollection = new Filter({ reader: abstractReader, callback: function(resource) { if (resource === "resource a") { @@ -51,7 +51,7 @@ test("_byPath: Positive filter", async (t) => { const trace = { collection: sinon.spy() }; - const readerCollection = new ReaderFilter({ + const readerCollection = new Filter({ reader: abstractReader, callback: function(resource) { if (resource === "resource a") { diff --git a/test/lib/ReaderTransformer.js b/test/lib/readers/Transformer.js similarity index 92% rename from test/lib/ReaderTransformer.js rename to test/lib/readers/Transformer.js index b2026edc..d4525307 100644 --- a/test/lib/ReaderTransformer.js +++ b/test/lib/readers/Transformer.js @@ -1,6 +1,6 @@ const test = require("ava"); const sinon = require("sinon"); -const ReaderTransformer = require("../../lib/ReaderTransformer"); +const Transformer = require("../../../lib/readers/Transformer"); function getDummyResource(name) { return { @@ -20,7 +20,7 @@ test("_byGlob: Basic transformation", async (t) => { const trace = { collection: sinon.spy() }; - const readerCollection = new ReaderTransformer({ + const readerCollection = new Transformer({ reader: abstractReader, callback: function(resource) { if (resource.name === "resource a") { @@ -45,7 +45,7 @@ test("_byPath: Basic transformation", async (t) => { const trace = { collection: sinon.spy() }; - const readerCollection = new ReaderTransformer({ + const readerCollection = new Transformer({ reader: abstractReader, callback: function(resource) { resource.name = "transformed resource a"; @@ -66,7 +66,7 @@ test("_byPath: No transformation", async (t) => { const trace = { collection: sinon.spy() }; - const readerCollection = new ReaderTransformer({ + const readerCollection = new Transformer({ reader: abstractReader, callback: function(resource) { return; @@ -88,7 +88,7 @@ test("async _byGlob: Basic transformation", async (t) => { const trace = { collection: sinon.spy() }; - const readerCollection = new ReaderTransformer({ + const readerCollection = new Transformer({ reader: abstractReader, callback: function(resource) { return new Promise((resolve) => { @@ -118,7 +118,7 @@ test("async _byPath: Basic transformation", async (t) => { const trace = { collection: sinon.spy() }; - const readerCollection = new ReaderTransformer({ + const readerCollection = new Transformer({ reader: abstractReader, callback: function(resource) { return new Promise((resolve) => { @@ -144,7 +144,7 @@ test("async _byPath: No transformation", async (t) => { const trace = { collection: sinon.spy() }; - const readerCollection = new ReaderTransformer({ + const readerCollection = new Transformer({ reader: abstractReader, callback: async function(resource) { return; From d0d27020ca58b49ec88797ad28af940ebe12c2c2 Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Tue, 21 Dec 2021 13:07:50 +0100 Subject: [PATCH 09/15] Revert "[INTERNAL] Move resource content into separate instance" No major benefits since streams need to be drained into a buffer anyways. This reverts commit ecd7bd600c8735ffd946ebadd5bd2a82734a4168. --- lib/Resource.js | 189 ++++++++++++++++----- lib/ResourceContent.js | 232 -------------------------- test/lib/Resource.js | 24 +-- test/lib/adapters/FileSystem_write.js | 2 +- 4 files changed, 160 insertions(+), 287 deletions(-) delete mode 100644 lib/ResourceContent.js diff --git a/lib/Resource.js b/lib/Resource.js index fb0c0923..ecdd96f8 100644 --- a/lib/Resource.js +++ b/lib/Resource.js @@ -1,6 +1,6 @@ +const stream = require("stream"); const clone = require("clone"); -const nodePath = require("path"); -const ResourceContent = require("./ResourceContent"); +const path = require("path"); const fnTrue = () => true; const fnFalse = () => false; @@ -38,19 +38,16 @@ class Resource { * stream of the content of this resource (cannot be used in conjunction with parameters buffer, * string or stream). * In some cases this is the most memory-efficient way to supply resource content - * @param {module:@ui5/fs.ResourceContent} [parameters.contentInstance] Resource content instance * @param {object} [parameters.project] Experimental, internal parameter. Do not use */ - constructor({path, statInfo, buffer, string, createStream, stream, contentInstance, project}) { + constructor({path, statInfo, buffer, string, createStream, stream, project}) { if (!path) { throw new Error("Cannot create Resource: path parameter missing"); } - if (buffer && createStream || buffer && string || buffer && stream || buffer && contentInstance || - string && createStream || string && stream || string && contentInstance || - stream && createStream || stream && contentInstance || - contentInstance && createStream) { + if (buffer && createStream || buffer && string || string && createStream || buffer && stream || + string && stream || createStream && stream) { throw new Error("Cannot create Resource: Please set only one content parameter. " + - "Buffer, string, stream, createStream or contentInstance"); + "Buffer, string, stream or createStream"); } this._path = path; @@ -74,14 +71,14 @@ class Resource { birthtime: new Date() }; - if (contentInstance) { - this._content = contentInstance; - } else { - try { - this._content = new ResourceContent({buffer, string, createStream, stream}); - } catch (err) { - throw new Error(`Failed to create content for resource ${this._path}: ${err.message}`); - } + if (createStream) { + this._createStream = createStream; + } else if (stream) { + this._stream = stream; + } else if (buffer) { + this.setBuffer(buffer); + } else if (typeof string === "string" || string instanceof String) { + this.setString(string); } // Tracing: @@ -95,10 +92,17 @@ class Resource { * @returns {Promise} Promise resolving with a buffer of the resource content. */ async getBuffer() { - try { - return await this._content.getBuffer(); - } catch (err) { - throw new Error(`Failed to get buffer for resource ${this._path}: ${err.message}`); + if (this._contentDrained) { + throw new Error(`Content of Resource ${this._path} has been drained. ` + + "This might be caused by requesting resource content after a content stream has been " + + "requested and no new content (e.g. a new stream) has been set."); + } + if (this._buffer) { + return this._buffer; + } else if (this._createStream || this._stream) { + return this._getBufferFromStream(); + } else { + throw new Error(`Resource ${this._path} has no content`); } } @@ -109,7 +113,14 @@ class Resource { * @param {Buffer} buffer Buffer instance */ setBuffer(buffer) { - this._content = new ResourceContent({buffer}); + this._createStream = null; + // if (this._stream) { // TODO this may cause strange issues + // this._stream.destroy(); + // } + this._stream = null; + this._buffer = buffer; + this._contentDrained = false; + this._streamDrained = false; } /** @@ -118,12 +129,13 @@ class Resource { * @public * @returns {Promise} Promise resolving with the resource content. */ - async getString() { - try { - return await this._content.getString(); - } catch (err) { - throw new Error(`Failed to get string for resource ${this._path}: ${err.message}`); + getString() { + if (this._contentDrained) { + return Promise.reject(new Error(`Content of Resource ${this._path} has been drained. ` + + "This might be caused by requesting resource content after a content stream has been " + + "requested and no new content (e.g. a new stream) has been set.")); } + return this.getBuffer().then((buffer) => buffer.toString()); } /** @@ -133,7 +145,7 @@ class Resource { * @param {string} string Resource content */ setString(string) { - this._content = new ResourceContent({string}); + this.setBuffer(Buffer.from(string, "utf8")); } /** @@ -148,11 +160,32 @@ class Resource { * @returns {stream.Readable} Readable stream for the resource content. */ getStream() { - try { - return this._content.getStream(); - } catch (err) { - throw new Error(`Failed to get stream for resource ${this._path}: ${err.message}`); + if (this._contentDrained) { + throw new Error(`Content of Resource ${this._path} has been drained. ` + + "This might be caused by requesting resource content after a content stream has been " + + "requested and no new content (e.g. a new stream) has been set."); + } + let contentStream; + if (this._buffer) { + const bufferStream = new stream.PassThrough(); + bufferStream.end(this._buffer); + contentStream = bufferStream; + } else if (this._createStream || this._stream) { + contentStream = this._getStream(); + } + if (!contentStream) { + throw new Error(`Resource ${this._path} has no content`); } + // If a stream instance is being returned, it will typically get drained be the consumer. + // In that case, further content access will result in a "Content stream has been drained" error. + // However, depending on the execution environment, a resources content stream might have been + // transformed into a buffer. In that case further content access is possible as a buffer can't be + // drained. + // To prevent unexpected "Content stream has been drained" errors caused by changing environments, we flag + // the resource content as "drained" every time a stream is requested. Even if actually a buffer or + // createStream callback is being used. + this._contentDrained = true; + return contentStream; } /** @@ -163,13 +196,19 @@ class Resource { callback for dynamic creation of a readable stream */ setStream(stream) { - const options = {}; + this._buffer = null; + // if (this._stream) { // TODO this may cause strange issues + // this._stream.destroy(); + // } if (typeof stream === "function") { - options.createStream = stream; + this._createStream = stream; + this._stream = null; } else { - options.stream = stream; + this._stream = stream; + this._createStream = null; } - this._content = new ResourceContent(options); + this._contentDrained = false; + this._streamDrained = false; } /** @@ -214,11 +253,16 @@ class Resource { * @returns {Promise} size in bytes, 0 if there is no content yet */ async getSize() { - return this._content.getSize(); + // if resource does not have any content it should have 0 bytes + if (!this._buffer && !this._createStream && !this._stream) { + return 0; + } + const buffer = await this.getBuffer(); + return buffer.byteLength; } _getNameFromPath(virPath) { - return nodePath.posix.basename(virPath); + return path.posix.basename(virPath); } /** @@ -236,14 +280,30 @@ class Resource { * @public * @returns {Promise} Promise resolving with the clone */ - async clone() { + clone() { const options = { path: this._path, - statInfo: clone(this._statInfo), - contentInstance: this._content + statInfo: clone(this._statInfo) + }; + + const addContentOption = () => { + if (this._stream) { + return this._getBufferFromStream().then(function(buffer) { + options.buffer = buffer; + }); + } else { + if (this._createStream) { + options.createStream = this._createStream; + } else if (this._buffer) { + options.buffer = this._buffer; + } + return Promise.resolve(); + } }; - return new Resource(options); + return addContentOption().then(() => { + return new Resource(options); + }); } /** @@ -262,6 +322,51 @@ class Resource { return tree; } + + /** + * Returns the content as stream. + * + * @private + * @returns {stream.Readable} Readable stream + */ + _getStream() { + if (this._streamDrained) { + throw new Error(`Content stream of Resource ${this._path} is flagged as drained.`); + } + if (this._createStream) { + return this._createStream(); + } + this._streamDrained = true; + return this._stream; + } + + /** + * Converts the buffer into a stream. + * + * @private + * @returns {Promise} Promise resolving with buffer. + */ + _getBufferFromStream() { + if (this._buffering) { // Prevent simultaneous buffering, causing unexpected access to drained stream + return this._buffering; + } + return this._buffering = new Promise((resolve, reject) => { + const contentStream = this._getStream(); + const buffers = []; + contentStream.on("data", (data) => { + buffers.push(data); + }); + contentStream.on("error", (err) => { + reject(err); + }); + contentStream.on("end", () => { + const buffer = Buffer.concat(buffers); + this.setBuffer(buffer); + this._buffering = null; + resolve(buffer); + }); + }); + } } module.exports = Resource; diff --git a/lib/ResourceContent.js b/lib/ResourceContent.js deleted file mode 100644 index 584be4f7..00000000 --- a/lib/ResourceContent.js +++ /dev/null @@ -1,232 +0,0 @@ -const stream = require("stream"); - -/** - * Content of a resource - * - * @private - * @memberof module:@ui5/fs - */ -class ResourceContent { - /** - * - * - * @public - * @param {object} parameters Parameters - * [fs.Stats]{@link https://nodejs.org/api/fs.html#fs_class_fs_stats} or similar object - * @param {Buffer} [parameters.buffer] Content of this resources as a Buffer instance - * (cannot be used in conjunction with parameters string, stream or createStream) - * @param {string} [parameters.string] Content of this resources as a string - * (cannot be used in conjunction with parameters buffer, stream or createStream) - * @param {Stream} [parameters.stream] Readable stream of the content of this resource - * (cannot be used in conjunction with parameters buffer, string or createStream) - * @param {module:@ui5/fs.Resource~createStream} [parameters.createStream] Function callback that returns a readable - * stream of the content of this resource (cannot be used in conjunction with parameters buffer, - * string or stream). - * In some cases this is the most memory-efficient way to supply resource content - */ - constructor({buffer, string, createStream, stream}) { - if (buffer && createStream || buffer && string || string && createStream || buffer && stream || - string && stream || createStream && stream) { - throw new Error("Cannot create ResourceContent: Please set only one content parameter. " + - "Buffer, string, stream or createStream"); - } - - if (createStream) { - this._createStream = createStream; - } else if (stream) { - this._stream = stream; - } else if (buffer) { - this._setBuffer(buffer); - } else if (typeof string === "string" || string instanceof String) { - this._setString(string); - } - } - - /** - * Gets a buffer with the resource content. - * - * @public - * @returns {Promise} Promise resolving with a buffer of the resource content. - */ - async getBuffer() { - if (this._contentDrained) { - throw new Error(`Content stream has been drained. ` + - "This might be caused by requesting resource content after a content stream has been " + - "requested and no new content (e.g. a new stream) has been set."); - } - if (this._buffer) { - return this._buffer; - } else if (this._createStream || this._stream) { - return this._getBufferFromStream(); - } else { - throw new Error(`No content available`); - } - } - - /** - * Sets a Buffer as content. - * - * @public - * @param {Buffer} buffer Buffer instance - */ - _setBuffer(buffer) { - this._createStream = null; - // if (this._stream) { // TODO this may cause strange issues - // this._stream.destroy(); - // } - this._stream = null; - this._buffer = buffer; - this._contentDrained = false; - this._streamDrained = false; - } - - /** - * Gets a string with the resource content. - * - * @public - * @returns {Promise} Promise resolving with the resource content. - */ - async getString() { - if (this._contentDrained) { - throw new Error(`Content stream has been drained. ` + - "This might be caused by requesting resource content after a content stream has been " + - "requested and no new content (e.g. a new stream) has been set."); - } - return this.getBuffer().then((buffer) => buffer.toString()); - } - - /** - * Sets a String as content - * - * @public - * @param {string} string Resource content - */ - _setString(string) { - this._setBuffer(Buffer.from(string, "utf8")); - } - - /** - * Gets a readable stream for the resource content. - * - * Repetitive calls of this function are only possible if new content has been set in the meantime (through - * [setStream]{@link module:@ui5/fs.Resource#setStream}, [setBuffer]{@link module:@ui5/fs.Resource#setBuffer} - * or [setString]{@link module:@ui5/fs.Resource#setString}). This - * is to prevent consumers from accessing drained streams. - * - * @public - * @returns {stream.Readable} Readable stream for the resource content. - */ - getStream() { - if (this._contentDrained) { - throw new Error(`Content stream has been drained. ` + - "This might be caused by requesting resource content after a content stream has been " + - "requested and no new content (e.g. a new stream) has been set."); - } - let contentStream; - if (this._buffer) { - const bufferStream = new stream.PassThrough(); - bufferStream.end(this._buffer); - contentStream = bufferStream; - } else if (this._createStream || this._stream) { - contentStream = this._getStream(); - } - if (!contentStream) { - throw new Error(`No content available`); - } - // If a stream instance is being returned, it will typically get drained by the consumer. - // In that case, further content access will result in a "Content stream has been drained" error. - // However, depending on the execution environment, a resources content stream might have been - // transformed into a buffer. In that case further content access is possible as a buffer can't be - // drained. - // To prevent unexpected "Content stream has been drained" errors caused by changing environments, we flag - // the resource content as "drained" every time a stream is requested. Even if actually a buffer or - // createStream callback is being used. - this._contentDrained = true; - return contentStream; - } - - /** - * Sets a readable stream as content. - * - * @public - * @param {stream.Readable|module:@ui5/fs.Resource~createStream} stream Readable stream of the resource content or - callback for dynamic creation of a readable stream - */ - _setStream(stream) { - this._buffer = null; - // if (this._stream) { // TODO this may cause strange issues - // this._stream.destroy(); - // } - if (typeof stream === "function") { - this._createStream = stream; - this._stream = null; - } else { - this._stream = stream; - this._createStream = null; - } - this._contentDrained = false; - this._streamDrained = false; - } - - /** - * Size in bytes allocated by the underlying buffer. - * - * @see {TypedArray#byteLength} - * @returns {Promise} size in bytes, 0 if there is no content yet - */ - async getSize() { - // if resource does not have any content it should have 0 bytes - if (!this._buffer && !this._createStream && !this._stream) { - return 0; - } - const buffer = await this.getBuffer(); - return buffer.byteLength; - } - - /** - * Returns the content as stream. - * - * @private - * @returns {stream.Readable} Readable stream - */ - _getStream() { - if (this._streamDrained) { - throw new Error(`Content stream is flagged as drained.`); - } - if (this._createStream) { - return this._createStream(); - } - this._streamDrained = true; - return this._stream; - } - - /** - * Converts the buffer into a stream. - * - * @private - * @returns {Promise} Promise resolving with buffer. - */ - _getBufferFromStream() { - if (this._buffering) { // Prevent simultaneous buffering, causing unexpected access to drained stream - return this._buffering; - } - return this._buffering = new Promise((resolve, reject) => { - const contentStream = this._getStream(); - const buffers = []; - contentStream.on("data", (data) => { - buffers.push(data); - }); - contentStream.on("error", (err) => { - reject(err); - }); - contentStream.on("end", () => { - const buffer = Buffer.concat(buffers); - this._setBuffer(buffer); - this._buffering = null; - resolve(buffer); - }); - }); - } -} - -module.exports = ResourceContent; diff --git a/test/lib/Resource.js b/test/lib/Resource.js index d90d6dea..f61c5d52 100644 --- a/test/lib/Resource.js +++ b/test/lib/Resource.js @@ -60,7 +60,7 @@ test("Resource: constructor with duplicated content parameter", (t) => { }, { instanceOf: Error, message: "Cannot create Resource: Please set only one content parameter. " + - "Buffer, string, stream, createStream or contentInstance" + "Buffer, string, stream or createStream" }); }); @@ -72,7 +72,7 @@ test("Resource: getBuffer with throwing an error", (t) => { }); return resource.getBuffer().catch(function(error) { - t.is(error.message, "Failed to get buffer for resource my/path/to/resource: No content available", + t.is(error.message, "Resource my/path/to/resource has no content", "getBuffer called w/o having a resource content provided"); }); }); @@ -126,7 +126,7 @@ test("Resource: getStream throwing an error", (t) => { resource.getStream(); }, { instanceOf: Error, - message: "Failed to get stream for resource my/path/to/resource: No content available" + message: "Resource my/path/to/resource has no content" }); }); @@ -267,9 +267,9 @@ test("getStream with createStream callback content: Subsequent content requests resource.getStream(); t.throws(() => { resource.getStream(); - }, {message: /Content stream has been drained/}); - await t.throwsAsync(resource.getBuffer(), {message: /Content stream has been drained/}); - await t.throwsAsync(resource.getString(), {message: /Content stream has been drained/}); + }, {message: /Content of Resource \/app\/index.html has been drained/}); + await t.throwsAsync(resource.getBuffer(), {message: /Content of Resource \/app\/index.html has been drained/}); + await t.throwsAsync(resource.getString(), {message: /Content of Resource \/app\/index.html has been drained/}); }); test("getStream with Buffer content: Subsequent content requests should throw error due to drained " + @@ -279,9 +279,9 @@ test("getStream with Buffer content: Subsequent content requests should throw er resource.getStream(); t.throws(() => { resource.getStream(); - }, {message: /Content stream has been drained/}); - await t.throwsAsync(resource.getBuffer(), {message: /Content stream has been drained/}); - await t.throwsAsync(resource.getString(), {message: /Content stream has been drained/}); + }, {message: /Content of Resource \/app\/index.html has been drained/}); + await t.throwsAsync(resource.getBuffer(), {message: /Content of Resource \/app\/index.html has been drained/}); + await t.throwsAsync(resource.getString(), {message: /Content of Resource \/app\/index.html has been drained/}); }); test("getStream with Stream content: Subsequent content requests should throw error due to drained " + @@ -301,9 +301,9 @@ test("getStream with Stream content: Subsequent content requests should throw er resource.getStream(); t.throws(() => { resource.getStream(); - }, {message: /Content stream has been drained/}); - await t.throwsAsync(resource.getBuffer(), {message: /Content stream has been drained/}); - await t.throwsAsync(resource.getString(), {message: /Content stream has been drained/}); + }, {message: /Content of Resource \/app\/index.html has been drained/}); + await t.throwsAsync(resource.getBuffer(), {message: /Content of Resource \/app\/index.html has been drained/}); + await t.throwsAsync(resource.getString(), {message: /Content of Resource \/app\/index.html has been drained/}); }); test("getBuffer from Stream content: Subsequent content requests should not throw error due to drained " + diff --git a/test/lib/adapters/FileSystem_write.js b/test/lib/adapters/FileSystem_write.js index a69f86fc..3c15be84 100644 --- a/test/lib/adapters/FileSystem_write.js +++ b/test/lib/adapters/FileSystem_write.js @@ -82,7 +82,7 @@ test("Write resource in drain mode", async (t) => { assert.fileEqual(destFsPath, "./test/fixtures/application.a/webapp/index.html"); }); await t.throwsAsync(resource.getBuffer(), - {message: /Failed to get buffer for resource \/app\/index.html: Content stream has been drained/}); + {message: /Content of Resource \/app\/index.html has been drained/}); }); test("Writing with readOnly and drain options set should fail", async (t) => { From ee6861477d1aa920fb7531411406ad22c0cadb5a Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Wed, 22 Dec 2021 16:31:49 +0100 Subject: [PATCH 10/15] [INTERNAL] ResourceTagCollection: Allow to pass resource path instead of instance We don't need more information anyways --- lib/ResourceTagCollection.js | 30 +++++++++++------------ test/lib/ResourceTagCollection.js | 40 +++++++++++++++---------------- 2 files changed, 35 insertions(+), 35 deletions(-) diff --git a/lib/ResourceTagCollection.js b/lib/ResourceTagCollection.js index 43abfb2d..182939eb 100644 --- a/lib/ResourceTagCollection.js +++ b/lib/ResourceTagCollection.js @@ -20,45 +20,42 @@ class ResourceTagCollection { this._pathTags = {}; } - setTag(resource, tag, value = true) { + setTag(resourcePath, tag, value = true) { if (this._superTags.includes(tag)) { - return this._superCollection.setTag(resource, tag, value); + return this._superCollection.setTag(resourcePath, tag, value); } - this._validateResource(resource); + resourcePath = this._getPath(resourcePath); this._validateTag(tag); this._validateValue(value); - const resourcePath = resource.getPath(); if (!this._pathTags[resourcePath]) { this._pathTags[resourcePath] = {}; } this._pathTags[resourcePath][tag] = value; } - clearTag(resource, tag) { + clearTag(resourcePath, tag) { if (this._superTags.includes(tag)) { - return this._superCollection.clearTag(resource, tag); + return this._superCollection.clearTag(resourcePath, tag); } - this._validateResource(resource); + resourcePath = this._getPath(resourcePath); this._validateTag(tag); - const resourcePath = resource.getPath(); if (this._pathTags[resourcePath]) { this._pathTags[resourcePath][tag] = undefined; } } - getTag(resource, tag) { + getTag(resourcePath, tag) { if (this._superTags.includes(tag)) { - return this._superCollection.getTag(resource, tag); + return this._superCollection.getTag(resourcePath, tag); } - this._validateResource(resource); + resourcePath = this._getPath(resourcePath); this._validateTag(tag); - const resourcePath = resource.getPath(); if (this._pathTags[resourcePath]) { return this._pathTags[resourcePath][tag]; } @@ -68,11 +65,14 @@ class ResourceTagCollection { return [...this._allowedTags, ...this._superTags]; } - _validateResource(resource) { - const path = resource.getPath(); - if (!path) { + _getPath(resourcePath) { + if (typeof resourcePath !== "string") { + resourcePath = resourcePath.getPath(); + } + if (!resourcePath) { throw new Error(`Invalid Resource: Resource path must not be empty`); } + return resourcePath; } _validateTag(tag) { diff --git a/test/lib/ResourceTagCollection.js b/test/lib/ResourceTagCollection.js index 737e2a2e..43c9646d 100644 --- a/test/lib/ResourceTagCollection.js +++ b/test/lib/ResourceTagCollection.js @@ -25,7 +25,7 @@ test("setTag", (t) => { allowedTags: ["abc:MyTag"] }); - const validateResourceSpy = sinon.spy(tagCollection, "_validateResource"); + const validateResourceSpy = sinon.spy(tagCollection, "_getPath"); const validateTagSpy = sinon.spy(tagCollection, "_validateTag"); const validateValueSpy = sinon.spy(tagCollection, "_validateValue"); @@ -37,9 +37,9 @@ test("setTag", (t) => { } }, "Tag correctly stored"); - t.is(validateResourceSpy.callCount, 1, "_validateResource called once"); + t.is(validateResourceSpy.callCount, 1, "_getPath called once"); t.is(validateResourceSpy.getCall(0).args[0], resource, - "_validateResource called with correct arguments"); + "_getPath called with correct arguments"); t.is(validateTagSpy.callCount, 1, "_validateTag called once"); t.is(validateTagSpy.getCall(0).args[0], "abc:MyTag", @@ -75,16 +75,16 @@ test("getTag", (t) => { }); tagCollection.setTag(resource, "abc:MyTag", 123); - const validateResourceSpy = sinon.spy(tagCollection, "_validateResource"); + const validateResourceSpy = sinon.spy(tagCollection, "_getPath"); const validateTagSpy = sinon.spy(tagCollection, "_validateTag"); const value = tagCollection.getTag(resource, "abc:MyTag"); t.is(value, 123, "Got correct tag value"); - t.is(validateResourceSpy.callCount, 1, "_validateResource called once"); + t.is(validateResourceSpy.callCount, 1, "_getPath called once"); t.is(validateResourceSpy.getCall(0).args[0], resource, - "_validateResource called with correct arguments"); + "_getPath called with correct arguments"); t.is(validateTagSpy.callCount, 1, "_validateTag called once"); t.is(validateTagSpy.getCall(0).args[0], "abc:MyTag", @@ -101,7 +101,7 @@ test("clearTag", (t) => { tagCollection.setTag(resource, "abc:MyTag", 123); - const validateResourceSpy = sinon.spy(tagCollection, "_validateResource"); + const validateResourceSpy = sinon.spy(tagCollection, "_getPath"); const validateTagSpy = sinon.spy(tagCollection, "_validateTag"); tagCollection.clearTag(resource, "abc:MyTag"); @@ -112,9 +112,9 @@ test("clearTag", (t) => { } }, "Tag value set to undefined"); - t.is(validateResourceSpy.callCount, 1, "_validateResource called once"); + t.is(validateResourceSpy.callCount, 1, "_getPath called once"); t.is(validateResourceSpy.getCall(0).args[0], resource, - "_validateResource called with correct arguments"); + "_getPath called with correct arguments"); t.is(validateTagSpy.callCount, 1, "_validateTag called once"); t.is(validateTagSpy.getCall(0).args[0], "abc:MyTag", @@ -133,7 +133,7 @@ test("superCollection: setTag", (t) => { superCollection: superTagCollection }); - const validateResourceSpy = sinon.spy(superTagCollection, "_validateResource"); + const validateResourceSpy = sinon.spy(superTagCollection, "_getPath"); const validateTagSpy = sinon.spy(superTagCollection, "_validateTag"); const validateValueSpy = sinon.spy(superTagCollection, "_validateValue"); @@ -151,9 +151,9 @@ test("superCollection: setTag", (t) => { } }, "Non-super tag correctly stored"); - t.is(validateResourceSpy.callCount, 1, "_validateResource called once"); + t.is(validateResourceSpy.callCount, 1, "_getPath called once"); t.is(validateResourceSpy.getCall(0).args[0], resource, - "_validateResource called with correct arguments"); + "_getPath called with correct arguments"); t.is(validateTagSpy.callCount, 1, "_validateTag called once"); t.is(validateTagSpy.getCall(0).args[0], "abc:MySuperTag", @@ -179,16 +179,16 @@ test("superCollection: getTag", (t) => { tagCollection.setTag(resource, "abc:MySuperTag", 456); tagCollection.setTag(resource, "abc:MyTag", 123); - const validateResourceSpy = sinon.spy(superTagCollection, "_validateResource"); + const validateResourceSpy = sinon.spy(superTagCollection, "_getPath"); const validateTagSpy = sinon.spy(superTagCollection, "_validateTag"); const value = tagCollection.getTag(resource, "abc:MySuperTag"); t.is(value, 456, "Got correct tag value"); - t.is(validateResourceSpy.callCount, 1, "_validateResource called once"); + t.is(validateResourceSpy.callCount, 1, "_getPath called once"); t.is(validateResourceSpy.getCall(0).args[0], resource, - "_validateResource called with correct arguments"); + "_getPath called with correct arguments"); t.is(validateTagSpy.callCount, 1, "_validateTag called once"); t.is(validateTagSpy.getCall(0).args[0], "abc:MySuperTag", @@ -209,7 +209,7 @@ test("superCollection: clearTag", (t) => { tagCollection.setTag(resource, "abc:MySuperTag", 123); - const validateResourceSpy = sinon.spy(superTagCollection, "_validateResource"); + const validateResourceSpy = sinon.spy(superTagCollection, "_getPath"); const validateTagSpy = sinon.spy(superTagCollection, "_validateTag"); tagCollection.clearTag(resource, "abc:MySuperTag"); @@ -220,9 +220,9 @@ test("superCollection: clearTag", (t) => { } }, "Tag value set to undefined"); - t.is(validateResourceSpy.callCount, 1, "_validateResource called once"); + t.is(validateResourceSpy.callCount, 1, "_getPath called once"); t.is(validateResourceSpy.getCall(0).args[0], resource, - "_validateResource called with correct arguments"); + "_getPath called with correct arguments"); t.is(validateTagSpy.callCount, 1, "_validateTag called once"); t.is(validateTagSpy.getCall(0).args[0], "abc:MySuperTag", @@ -387,12 +387,12 @@ test("_validateValue: Invalid value null", (t) => { }); }); -test("_validateResource: Empty path", (t) => { +test("_getPath: Empty path", (t) => { const tagCollection = new ResourceTagCollection({ allowedTags: ["abc:MyTag"] }); t.throws(() => { - tagCollection._validateResource({ + tagCollection._getPath({ getPath: () => "" }); }, { From 9453e4829f503c74fd9985b1633b64057153615d Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Wed, 22 Dec 2021 17:10:20 +0100 Subject: [PATCH 11/15] [INTERNAL] Transformer: Callback can request resource for modification --- lib/AbstractReader.js | 15 ++++ lib/readers/Filter.js | 2 +- lib/readers/Transformer.js | 41 +++++++---- test/lib/readers/Transformer.js | 119 ++++++-------------------------- 4 files changed, 65 insertions(+), 112 deletions(-) diff --git a/lib/AbstractReader.js b/lib/AbstractReader.js index ee0aac16..4ed995bf 100644 --- a/lib/AbstractReader.js +++ b/lib/AbstractReader.js @@ -70,6 +70,14 @@ class AbstractReader { }); } + + /** + * Create a [Filter-Reader]{@link module:@ui5/fs.readers.Filter} from the current reader + * + * @param {module:@ui5/fs.readers.Filter~callback} callback + * Filter function. Will be called for every resource passed through this reader. + * @returns {module:@ui5/fs.reader.Transformer} T + */ filter(callback) { const Filter = require("./readers/Filter"); return new Filter({ @@ -78,6 +86,13 @@ class AbstractReader { }); } + /** + * Create a [Transform-Reader]{@link module:@ui5/fs.readers.Transform} from the current reader + * + * @param {module:@ui5/fs.readers.Transformer~callback} callback + * Callback to check and eventually transform any resource passed through the reader + * @returns {module:@ui5/fs.reader.Transformer} T + */ transformer(callback) { const Transformer = require("./readers/Transformer"); return new Transformer({ diff --git a/lib/readers/Filter.js b/lib/readers/Filter.js index 79951b6b..a9f60416 100644 --- a/lib/readers/Filter.js +++ b/lib/readers/Filter.js @@ -1,7 +1,7 @@ const AbstractReader = require("../AbstractReader"); /** - * Transparently apply filters on resource readers by wrapping them. + * A reader that allows dynamic filtering of resources passed through it * * @public * @memberof module:@ui5/fs diff --git a/lib/readers/Transformer.js b/lib/readers/Transformer.js index d5586e42..ecdc818c 100644 --- a/lib/readers/Transformer.js +++ b/lib/readers/Transformer.js @@ -1,7 +1,7 @@ const AbstractReader = require("../AbstractReader"); /** - * Transparently apply filters on resource readers by wrapping them. + * A reader that allows modification of all resources passed through it. * * @public * @memberof module:@ui5/fs.readers @@ -9,12 +9,24 @@ const AbstractReader = require("../AbstractReader"); */ class Transformer extends AbstractReader { /** - * Filter callback + * Callback to check and eventually transform a resource * * @public * @callback module:@ui5/fs.readers.Transformer~callback - * @param {module:@ui5/fs.Resource} resource Resource to transform - * @returns {Promise|undefined} Optional promise resolving once the resource has been transformed + * @param {module:@ui5/fs.Resource} resourcePath Path of the resource to process. + * This can be used to decide whether the resource should be transformed + * @param {module:@ui5/fs.readers.Transformer~getResource} + * Function to retrieve the given resource instance in order to transform it + * @returns {Promise} Promise resolving once the transformation is done + */ + + /** + * Callback to retrieve a resource for modification. This will create a clone of the original + * resource which then takes its place in the result set of the reader + * + * @public + * @callback module:@ui5/fs.readers.Transformer~getResource + * @returns {Promise} Promise resolving to the resource */ /** @@ -50,12 +62,12 @@ class Transformer extends AbstractReader { async _byGlob(pattern, options, trace) { const result = await this._reader._byGlob(pattern, options, trace); return Promise.all(result.map(async (resource) => { - resource = await resource.clone(); - const res = this._callback(resource); - if (res && res instanceof Promise) { - await res; - } - return resource; + let resourceClone; + await this._callback(resource.getPath(), async function() { + resourceClone = resourceClone || await resource.clone(); + return resourceClone; + }); + return resourceClone || resource; })); } @@ -71,11 +83,10 @@ class Transformer extends AbstractReader { async _byPath(virPath, options, trace) { let resource = await this._reader._byPath(virPath, options, trace); if (resource) { - resource = await resource.clone(); - const res = this._callback(resource); - if (res && res instanceof Promise) { - await res; - } + await this._callback(resource.getPath(), async function() { + resource = await resource.clone(); + return resource; + }); } return resource; } diff --git a/test/lib/readers/Transformer.js b/test/lib/readers/Transformer.js index d4525307..330fc80d 100644 --- a/test/lib/readers/Transformer.js +++ b/test/lib/readers/Transformer.js @@ -5,6 +5,9 @@ const Transformer = require("../../../lib/readers/Transformer"); function getDummyResource(name) { return { name, + getPath: function() { + return `/resources/${name}`; + }, clone: function() { return getDummyResource(name); } @@ -12,8 +15,8 @@ function getDummyResource(name) { } test("_byGlob: Basic transformation", async (t) => { - const resourceA = getDummyResource("resource a"); - const resourceB = getDummyResource("resource b"); + const resourceA = getDummyResource("resource.a"); + const resourceB = getDummyResource("resource.b"); const abstractReader = { _byGlob: sinon.stub().returns(Promise.resolve([resourceA, resourceB])) }; @@ -22,23 +25,24 @@ test("_byGlob: Basic transformation", async (t) => { }; const readerCollection = new Transformer({ reader: abstractReader, - callback: function(resource) { - if (resource.name === "resource a") { - resource.name = "transformed resource a"; + callback: async function(resourcePath, getClonedResource) { + if (resourcePath === "/resources/resource.a") { + const resource = await getClonedResource(); + resource.name = "transformed resource.a"; } } }); const resources = await readerCollection._byGlob("anyPattern", {}, trace); t.deepEqual(resources.length, 2, "Still two resources in result set"); - t.deepEqual(resources[0].name, "transformed resource a", "resource a has been transformed in result"); - t.deepEqual(resources[1].name, "resource b", "resource b has not been transformed"); - t.not(resources[1], resourceB, "resource b instance has been cloned"); - t.deepEqual(resourceA.name, "resource a", "Original resource a has not been transformed"); + t.deepEqual(resources[0].name, "transformed resource.a", "resource.a has been transformed in result"); + t.deepEqual(resources[1].name, "resource.b", "resource.b has not been transformed"); + t.is(resources[1], resourceB, "resource.b instance has not been cloned"); + t.deepEqual(resourceA.name, "resource.a", "Original resource.a has not been transformed"); }); test("_byPath: Basic transformation", async (t) => { - const resourceA = getDummyResource("resource a"); + const resourceA = getDummyResource("resource.a"); const abstractReader = { _byPath: sinon.stub().returns(resourceA) }; @@ -47,19 +51,20 @@ test("_byPath: Basic transformation", async (t) => { }; const readerCollection = new Transformer({ reader: abstractReader, - callback: function(resource) { - resource.name = "transformed resource a"; + callback: async function(resourcePath, getClonedResource) { + const resource = await getClonedResource(); + resource.name = "transformed resource.a"; } }); const resource = await readerCollection._byPath("anyPattern", {}, trace); - t.deepEqual(resource.name, "transformed resource a", "resource a has been transformed in result"); - t.deepEqual(resourceA.name, "resource a", "Original resource a has not been transformed"); + t.deepEqual(resource.name, "transformed resource.a", "resource.a has been transformed in result"); + t.deepEqual(resourceA.name, "resource.a", "Original resource.a has not been transformed"); }); test("_byPath: No transformation", async (t) => { - const resourceB = getDummyResource("resource b"); + const resourceB = getDummyResource("resource.b"); const abstractReader = { _byPath: sinon.stub().returns(resourceB) }; @@ -68,90 +73,12 @@ test("_byPath: No transformation", async (t) => { }; const readerCollection = new Transformer({ reader: abstractReader, - callback: function(resource) { + callback: async function(resourcePath, getClonedResource) { return; } }); const resource = await readerCollection._byPath("anyPattern", {}, trace); - t.deepEqual(resource.name, "resource b", "Correct resource in result"); - t.not(resource, resourceB, "resource b instance has been cloned"); -}); - - -test("async _byGlob: Basic transformation", async (t) => { - const resourceA = getDummyResource("resource a"); - const resourceB = getDummyResource("resource b"); - const abstractReader = { - _byGlob: sinon.stub().returns(Promise.resolve([resourceA, resourceB])) - }; - const trace = { - collection: sinon.spy() - }; - const readerCollection = new Transformer({ - reader: abstractReader, - callback: function(resource) { - return new Promise((resolve) => { - setTimeout(() => { - if (resource.name === "resource a") { - resource.name = "transformed resource a"; - } - resolve(); - }, 10); - }); - } - }); - - const resources = await readerCollection._byGlob("anyPattern", {}, trace); - t.deepEqual(resources.length, 2, "Still two resources in result set"); - t.deepEqual(resources[0].name, "transformed resource a", "resource a has been transformed in result"); - t.deepEqual(resources[1].name, "resource b", "resource b has not been transformed"); - t.not(resources[1], resourceB, "resource b instance has been cloned"); - t.deepEqual(resourceA.name, "resource a", "Original resource a has not been transformed"); -}); - -test("async _byPath: Basic transformation", async (t) => { - const resourceA = getDummyResource("resource a"); - const abstractReader = { - _byPath: sinon.stub().returns(resourceA) - }; - const trace = { - collection: sinon.spy() - }; - const readerCollection = new Transformer({ - reader: abstractReader, - callback: function(resource) { - return new Promise((resolve) => { - setTimeout(() => { - resource.name = "transformed resource a"; - resolve(); - }, 10); - }); - } - }); - - const resource = await readerCollection._byPath("anyPattern", {}, trace); - - t.deepEqual(resource.name, "transformed resource a", "resource a has been transformed in result"); - t.deepEqual(resourceA.name, "resource a", "Original resource a has not been transformed"); -}); - -test("async _byPath: No transformation", async (t) => { - const resourceB = getDummyResource("resource b"); - const abstractReader = { - _byPath: sinon.stub().returns(resourceB) - }; - const trace = { - collection: sinon.spy() - }; - const readerCollection = new Transformer({ - reader: abstractReader, - callback: async function(resource) { - return; - } - }); - - const resource = await readerCollection._byPath("anyPattern", {}, trace); - t.deepEqual(resource.name, "resource b", "Correct resource in result"); - t.not(resource, resourceB, "resource b instance has been cloned"); + t.deepEqual(resource.name, "resource.b", "Correct resource in result"); + t.is(resource, resourceB, "resource.b instance has not been cloned"); }); From 5ff40f5afb84f9f8097f42c0883a54ee29005cf2 Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Wed, 22 Dec 2021 17:14:00 +0100 Subject: [PATCH 12/15] [INTERNAL] Filter: Simplify if condition --- lib/readers/Filter.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/readers/Filter.js b/lib/readers/Filter.js index a9f60416..27ba36bb 100644 --- a/lib/readers/Filter.js +++ b/lib/readers/Filter.js @@ -63,10 +63,8 @@ class Filter extends AbstractReader { */ async _byPath(virPath, options, trace) { const result = await this._reader._byPath(virPath, options, trace); - if (result) { - if (!this._callback(result)) { - return null; - } + if (result && !this._callback(result)) { + return null; } return result; } From 3bb27639088c44f28c97931536c62c9e5e73f6e8 Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Thu, 23 Dec 2021 10:24:53 +0100 Subject: [PATCH 13/15] [INTERNAL] Transformer: Make sure to only clone resource once Add tests for the same --- lib/readers/Transformer.js | 11 +++++++---- test/lib/readers/Transformer.js | 15 +++++++++------ 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/lib/readers/Transformer.js b/lib/readers/Transformer.js index ecdc818c..cc19d6f1 100644 --- a/lib/readers/Transformer.js +++ b/lib/readers/Transformer.js @@ -64,6 +64,7 @@ class Transformer extends AbstractReader { return Promise.all(result.map(async (resource) => { let resourceClone; await this._callback(resource.getPath(), async function() { + // Make sure to only clone once resourceClone = resourceClone || await resource.clone(); return resourceClone; }); @@ -81,14 +82,16 @@ class Transformer extends AbstractReader { * @returns {Promise} Promise resolving to a single resource */ async _byPath(virPath, options, trace) { - let resource = await this._reader._byPath(virPath, options, trace); + const resource = await this._reader._byPath(virPath, options, trace); + let resourceClone; if (resource) { await this._callback(resource.getPath(), async function() { - resource = await resource.clone(); - return resource; + // Make sure to only clone once + resourceClone = resourceClone || await resource.clone(); + return resourceClone; }); } - return resource; + return resourceClone || resource; } } diff --git a/test/lib/readers/Transformer.js b/test/lib/readers/Transformer.js index 330fc80d..6111f587 100644 --- a/test/lib/readers/Transformer.js +++ b/test/lib/readers/Transformer.js @@ -4,7 +4,7 @@ const Transformer = require("../../../lib/readers/Transformer"); function getDummyResource(name) { return { - name, + name, // arbitrary attribute to change getPath: function() { return `/resources/${name}`; }, @@ -25,10 +25,11 @@ test("_byGlob: Basic transformation", async (t) => { }; const readerCollection = new Transformer({ reader: abstractReader, - callback: async function(resourcePath, getClonedResource) { + callback: async function(resourcePath, getResource) { if (resourcePath === "/resources/resource.a") { - const resource = await getClonedResource(); + const resource = await getResource(); resource.name = "transformed resource.a"; + await getResource(); // additional call should not lead to additional clone } } }); @@ -51,9 +52,11 @@ test("_byPath: Basic transformation", async (t) => { }; const readerCollection = new Transformer({ reader: abstractReader, - callback: async function(resourcePath, getClonedResource) { - const resource = await getClonedResource(); + callback: async function(resourcePath, getResource) { + const resource = await getResource(); resource.name = "transformed resource.a"; + + await getResource(); // additional call should not lead to additional clone } }); @@ -73,7 +76,7 @@ test("_byPath: No transformation", async (t) => { }; const readerCollection = new Transformer({ reader: abstractReader, - callback: async function(resourcePath, getClonedResource) { + callback: async function(resourcePath, getResource) { return; } }); From db72383735c431d1e31009bdcce7ea537568ccf0 Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Thu, 23 Dec 2021 13:39:35 +0100 Subject: [PATCH 14/15] [INTERNAL] JSDoc: Make Filter and Transform public --- index.js | 5 +++++ lib/AbstractReader.js | 2 ++ 2 files changed, 7 insertions(+) diff --git a/index.js b/index.js index 23338ac8..21360bc5 100644 --- a/index.js +++ b/index.js @@ -22,6 +22,11 @@ module.exports = { */ Memory: "./lib/adapters/Memory" }, + /** + * @public + * @alias module:@ui5/fs.readers + * @namespace + */ readers: { /** * @type {typeof import('./lib/readers/Filter')} diff --git a/lib/AbstractReader.js b/lib/AbstractReader.js index 4ed995bf..5770c9cb 100644 --- a/lib/AbstractReader.js +++ b/lib/AbstractReader.js @@ -74,6 +74,7 @@ class AbstractReader { /** * Create a [Filter-Reader]{@link module:@ui5/fs.readers.Filter} from the current reader * + * @public * @param {module:@ui5/fs.readers.Filter~callback} callback * Filter function. Will be called for every resource passed through this reader. * @returns {module:@ui5/fs.reader.Transformer} T @@ -89,6 +90,7 @@ class AbstractReader { /** * Create a [Transform-Reader]{@link module:@ui5/fs.readers.Transform} from the current reader * + * @public * @param {module:@ui5/fs.readers.Transformer~callback} callback * Callback to check and eventually transform any resource passed through the reader * @returns {module:@ui5/fs.reader.Transformer} T From 4fca2aa36a2e5ec6f44f7691da37095abcb37532 Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Thu, 13 Jan 2022 11:14:13 +0100 Subject: [PATCH 15/15] [INTERNAL] Apply UA review --- lib/AbstractReader.js | 4 ++-- lib/readers/Filter.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/AbstractReader.js b/lib/AbstractReader.js index 5770c9cb..b0a6aa00 100644 --- a/lib/AbstractReader.js +++ b/lib/AbstractReader.js @@ -77,7 +77,7 @@ class AbstractReader { * @public * @param {module:@ui5/fs.readers.Filter~callback} callback * Filter function. Will be called for every resource passed through this reader. - * @returns {module:@ui5/fs.reader.Transformer} T + * @returns {module:@ui5/fs.reader.Filter} Filter instance */ filter(callback) { const Filter = require("./readers/Filter"); @@ -93,7 +93,7 @@ class AbstractReader { * @public * @param {module:@ui5/fs.readers.Transformer~callback} callback * Callback to check and eventually transform any resource passed through the reader - * @returns {module:@ui5/fs.reader.Transformer} T + * @returns {module:@ui5/fs.reader.Transformer} Transformer instance */ transformer(callback) { const Transformer = require("./readers/Transformer"); diff --git a/lib/readers/Filter.js b/lib/readers/Filter.js index 27ba36bb..4108ede7 100644 --- a/lib/readers/Filter.js +++ b/lib/readers/Filter.js @@ -14,7 +14,7 @@ class Filter extends AbstractReader { * @public * @callback module:@ui5/fs.readers.Filter~callback * @param {module:@ui5/fs.Resource} resource Resource to test - * @returns {boolean} Return true to keep the resource, or false to disregard it + * @returns {boolean} Whether to keep the resource */ /**