From 9ac6a39f3cb72e02c2a1298b07c4676a0ee92377 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20O=C3=9Fwald?= <1410947+matz3@users.noreply.github.com> Date: Thu, 21 Apr 2022 13:57:04 +0200 Subject: [PATCH] [FIX] FileSystem Adapter: Use native fs.copy / Skip writing when resource is unchanged (#370) * Always resolve fsBasePath to absolute path for later comparison * Always transform stream into buffer if write path equals source path Co-authored-by: Merlin Beutlberger --- lib/Resource.js | 26 +++++++- lib/adapters/FileSystem.js | 43 +++++++++++-- test/lib/Resource.js | 88 ++++++++++++++++++++++++--- test/lib/adapters/FileSystem_write.js | 29 +++++++++ 4 files changed, 170 insertions(+), 16 deletions(-) diff --git a/lib/Resource.js b/lib/Resource.js index ecdd96f8..5f2a8094 100644 --- a/lib/Resource.js +++ b/lib/Resource.js @@ -38,9 +38,10 @@ 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 {object} [parameters.source] Experimental, internal parameter. Do not use * @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, source, project}) { if (!path) { throw new Error("Cannot create Resource: path parameter missing"); } @@ -53,6 +54,11 @@ class Resource { this._path = path; this._name = this._getNameFromPath(path); this._project = project; // Experimental, internal parameter + this._source = source; // Experimental, internal parameter + if (this._source) { + // Indicator for adapters like FileSystem to detect whether a resource has been changed + this._source.modified = false; + } this._statInfo = statInfo || { // TODO isFile: fnTrue, isDirectory: fnFalse, @@ -113,6 +119,9 @@ class Resource { * @param {Buffer} buffer Buffer instance */ setBuffer(buffer) { + if (this._source && !this._source.modified) { + this._source.modified = true; + } this._createStream = null; // if (this._stream) { // TODO this may cause strange issues // this._stream.destroy(); @@ -196,6 +205,9 @@ class Resource { callback for dynamic creation of a readable stream */ setStream(stream) { + if (this._source && !this._source.modified) { + this._source.modified = true; + } this._buffer = null; // if (this._stream) { // TODO this may cause strange issues // this._stream.destroy(); @@ -323,6 +335,10 @@ class Resource { return tree; } + getSource() { + return this._source || {}; + } + /** * Returns the content as stream. * @@ -361,7 +377,15 @@ class Resource { }); contentStream.on("end", () => { const buffer = Buffer.concat(buffers); + let modified; + if (this._source) { + modified = this._source.modified; + } this.setBuffer(buffer); + // Modified flag should be reset as the resource hasn't been modified from the outside + if (this._source) { + this._source.modified = modified; + } this._buffering = null; resolve(buffer); }); diff --git a/lib/adapters/FileSystem.js b/lib/adapters/FileSystem.js index ae1d1fa4..de61ccb0 100644 --- a/lib/adapters/FileSystem.js +++ b/lib/adapters/FileSystem.js @@ -1,12 +1,17 @@ const log = require("@ui5/logger").getLogger("resources:adapters:FileSystem"); const path = require("path"); +const {promisify} = require("util"); const fs = require("graceful-fs"); +const copyFile = promisify(fs.copyFile); +const chmod = promisify(fs.chmod); const globby = require("globby"); const makeDir = require("make-dir"); const {PassThrough} = require("stream"); const Resource = require("../Resource"); const AbstractAdapter = require("./AbstractAdapter"); +const READ_ONLY_MODE = 0o444; + /** * File system resource adapter * @@ -26,7 +31,7 @@ class FileSystem extends AbstractAdapter { */ constructor({virBasePath, project, fsBasePath, excludes}) { super({virBasePath, project, excludes}); - this._fsBasePath = fsBasePath; + this._fsBasePath = path.resolve(fsBasePath); } /** @@ -59,6 +64,10 @@ class FileSystem extends AbstractAdapter { project: this._project, statInfo: stat, path: this._virBaseDir, + source: { + adapter: "FileSystem", + fsPath: this._fsBasePath + }, createStream: () => { return fs.createReadStream(this._fsBasePath); } @@ -91,6 +100,10 @@ class FileSystem extends AbstractAdapter { project: this._project, statInfo: stat, path: virPath, + source: { + adapter: "FileSystem", + fsPath: fsPath + }, createStream: () => { return fs.createReadStream(fsPath); } @@ -157,7 +170,10 @@ class FileSystem extends AbstractAdapter { project: this._project, statInfo: stat, path: virPath, - fsPath + source: { + adapter: "FileSystem", + fsPath + } }; if (!stat.isDirectory()) { @@ -201,13 +217,30 @@ class FileSystem extends AbstractAdapter { const fsPath = path.join(this._fsBasePath, relPath); const dirPath = path.dirname(fsPath); + await makeDir(dirPath, {fs}); + + const resourceSource = resource.getSource(); + if (!resourceSource.modified && resourceSource.adapter === "FileSystem" && resourceSource.fsPath) { + // fs.copyFile can be used when the resource is from FS and hasn't been modified + // In addition, nothing needs to be done when src === dest + if (resourceSource.fsPath === fsPath) { + log.verbose("Skip writing to %s (Resource hasn't been modified)", fsPath); + } else { + log.verbose("Copying resource from %s to %s", resourceSource.fsPath, fsPath); + await copyFile(resourceSource.fsPath, fsPath); + if (readOnly) { + await chmod(fsPath, READ_ONLY_MODE); + } + } + return; + } + log.verbose("Writing to %s", fsPath); - await makeDir(dirPath, {fs}); return new Promise((resolve, reject) => { let contentStream; - if (drain || readOnly) { + if ((drain || readOnly) && resourceSource.fsPath !== fsPath) { // Stream will be drained contentStream = resource.getStream(); @@ -233,7 +266,7 @@ class FileSystem extends AbstractAdapter { const writeOptions = {}; if (readOnly) { - writeOptions.mode = 0o444; // read only + writeOptions.mode = READ_ONLY_MODE; } const write = fs.createWriteStream(fsPath, writeOptions); diff --git a/test/lib/Resource.js b/test/lib/Resource.js index f61c5d52..cd8e59bb 100644 --- a/test/lib/Resource.js +++ b/test/lib/Resource.js @@ -130,18 +130,36 @@ test("Resource: getStream throwing an error", (t) => { }); }); -test("Resource: setString", (t) => { - t.plan(1); - +test("Resource: setString", async (t) => { const resource = new Resource({ path: "my/path/to/resource", + source: {} // Needs to be passed in order to get the "modified" state }); + t.is(resource.getSource().modified, false); + resource.setString("Content"); - return resource.getString().then(function(value) { - t.is(value, "Content", "String set"); + t.is(resource.getSource().modified, true); + + const value = await resource.getString(); + t.is(value, "Content", "String set"); +}); + +test("Resource: setBuffer", async (t) => { + const resource = new Resource({ + path: "my/path/to/resource", + source: {} // Needs to be passed in order to get the "modified" state }); + + t.is(resource.getSource().modified, false); + + resource.setBuffer(Buffer.from("Content")); + + t.is(resource.getSource().modified, true); + + const value = await resource.getString(); + t.is(value, "Content", "String set"); }); test("Resource: size modification", async (t) => { @@ -204,12 +222,14 @@ test("Resource: size modification", async (t) => { t.is(await streamResource.getSize(), 23, "size for streamResource read again"); }); -test("Resource: setStream", (t) => { - t.plan(1); - +test("Resource: setStream (Stream)", async (t) => { const resource = new Resource({ path: "my/path/to/resource", + source: {} // Needs to be passed in order to get the "modified" state }); + + t.is(resource.getSource().modified, false); + const stream = new Stream.Readable(); stream._read = function() {}; stream.push("I am a "); @@ -219,9 +239,34 @@ test("Resource: setStream", (t) => { resource.setStream(stream); - return resource.getString().then(function(value) { - t.is(value, "I am a readable stream!", "Stream set correctly"); + t.is(resource.getSource().modified, true); + + const value = await resource.getString(); + t.is(value, "I am a readable stream!", "Stream set correctly"); +}); + +test("Resource: setStream (Create stream callback)", async (t) => { + const resource = new Resource({ + path: "my/path/to/resource", + source: {} // Needs to be passed in order to get the "modified" state }); + + t.is(resource.getSource().modified, false); + + resource.setStream(() => { + const stream = new Stream.Readable(); + stream._read = function() {}; + stream.push("I am a "); + stream.push("readable "); + stream.push("stream!"); + stream.push(null); + return stream; + }); + + t.is(resource.getSource().modified, true); + + const value = await resource.getString(); + t.is(value, "I am a readable stream!", "Stream set correctly"); }); test("Resource: clone resource with buffer", (t) => { @@ -330,6 +375,29 @@ test("getBuffer from Stream content: Subsequent content requests should not thro await t.notThrowsAsync(p2); }); +test("Resource: constructor with stream", async (t) => { + const stream = new Stream.Readable(); + stream._read = function() {}; + stream.push("I am a "); + stream.push("readable "); + stream.push("stream!"); + stream.push(null); + + const resource = new Resource({ + path: "my/path/to/resource", + stream, + source: {} // Needs to be passed in order to get the "modified" state + }); + + t.is(resource.getSource().modified, false); + + const value = await resource.getString(); + t.is(value, "I am a readable stream!"); + + // modified should still be false although setBuffer is called internally + t.is(resource.getSource().modified, false); +}); + test("integration stat - resource size", async (t) => { const fsPath = path.join("test", "fixtures", "application.a", "webapp", "index.html"); const statInfo = await stat(fsPath); diff --git a/test/lib/adapters/FileSystem_write.js b/test/lib/adapters/FileSystem_write.js index 3c15be84..5bac557c 100644 --- a/test/lib/adapters/FileSystem_write.js +++ b/test/lib/adapters/FileSystem_write.js @@ -78,6 +78,35 @@ test("Write resource in drain mode", async (t) => { // Write resource content to another readerWriter await readerWriters.dest.write(resource, {drain: true}); + t.notThrows(() => { + assert.fileEqual(destFsPath, "./test/fixtures/application.a/webapp/index.html"); + }); + + // Should not fail as resource hasn't been modified + await t.notThrowsAsync(resource.getBuffer()); +}); + +test("Write modified resource in drain mode", async (t) => { + const readerWriters = t.context.readerWriters; + const destFsPath = path.join(t.context.tmpDirPath, "index.html"); + + // Get resource from one readerWriter + const resource = await readerWriters.source.byPath("/app/index.html"); + + resource.setString(` + + + Application A + + + + +` + ); + + // Write resource content to another readerWriter + await readerWriters.dest.write(resource, {drain: true}); + t.notThrows(() => { assert.fileEqual(destFsPath, "./test/fixtures/application.a/webapp/index.html"); });