Skip to content

Commit

Permalink
[FIX] FileSystem Adapter: Use native fs.copy / Skip writing when reso…
Browse files Browse the repository at this point in the history 
…urce 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 <[email protected]>
  • Loading branch information
@matz3 @RandomByte
matz3 and RandomByte authored Apr 21, 2022
1 parent 354f694 commit 9ac6a39
Show file tree
Hide file tree
Showing 4 changed files with 170 additions and 16 deletions.
26 changes: 25 additions & 1 deletion lib/Resource.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand All @@ -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,
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -323,6 +335,10 @@ class Resource {
return tree;
}

getSource() {
return this._source || {};
}

/**
* Returns the content as stream.
*
Expand Down Expand Up @@ -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);
});
Expand Down
43 changes: 38 additions & 5 deletions lib/adapters/FileSystem.js
View file
Original file line number Diff line number Diff line change
@@ -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
*
Expand All @@ -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);
}

/**
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -157,7 +170,10 @@ class FileSystem extends AbstractAdapter {
project: this._project,
statInfo: stat,
path: virPath,
fsPath
source: {
adapter: "FileSystem",
fsPath
}
};

if (!stat.isDirectory()) {
Expand Down Expand Up @@ -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();

Expand All @@ -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);
Expand Down
88 changes: 78 additions & 10 deletions test/lib/Resource.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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 ");
Expand All @@ -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) => {
Expand Down Expand Up @@ -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);
Expand Down
29 changes: 29 additions & 0 deletions test/lib/adapters/FileSystem_write.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(`<!DOCTYPE html>
<html>
<head>
<title>Application A</title>
</head>
<body>
</body>
</html>`
);

// 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");
});
Expand Down

0 comments on commit 9ac6a39

Please sign in to comment.