Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add totalSize and totalCount values to VFS stats #66

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add totalSize and totalCount values to VFS stats #66

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
27766ca
Add total values to stats
Aug 2, 2022
ef7b81b
Fix typo in unit tests
Aug 3, 2022
991c893
Fix spaces between properties and values
Aug 4, 2022
1c65dac
Use du package for totalSize value in stat
Aug 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 34 additions & 15 deletions __tests__/adapters/vfs/system.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,21 +50,6 @@ describe('VFS System adapter', () => {
.toBe(true);
});

test('#stat', () => {
const realPath = path.join(core.configuration.tempPath, 'jest/test');

return expect(request('stat', 'home:/test', createOptions()))
.resolves
.toMatchObject({
filename: 'test',
path: realPath,
size: 0,
isFile: true,
isDirectory: false,
mime: 'application/octet-stream'
});
});

test('#copy', () => {
return expect(request('copy', 'home:/test', 'home:/test-copy', createOptions()))
.resolves
Expand Down Expand Up @@ -95,6 +80,40 @@ describe('VFS System adapter', () => {
.toBe(true);
});

test('#stat - file', () => {
const realPath = path.join(core.configuration.tempPath, 'jest/test');

return expect(request('stat', 'home:/test', createOptions()))
.resolves
.toMatchObject({
filename: 'test',
path: realPath,
size: 0,
isFile: true,
isDirectory: false,
mime: 'application/octet-stream',
totalCount:null,
andersevenrud marked this conversation as resolved.
Show resolved Hide resolved
totalSize:null
});
});

test('#stat - directory', () => {
const realPath = path.join(core.configuration.tempPath, 'jest/test-directory');

return expect(request('stat', 'home:/test-directory', createOptions()))
.resolves
.toMatchObject({
filename: 'test-directory',
path: realPath,
size: 4096,
isFile: false,
isDirectory: true,
mime: null,
totalCount:0,
andersevenrud marked this conversation as resolved.
Show resolved Hide resolved
totalSize:0
});
});

test('#readfile', () => {
return expect(request('readfile', 'home:/test', createOptions()))
.resolves
Expand Down
38 changes: 27 additions & 11 deletions src/adapters/vfs/system.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,20 +36,36 @@ const chokidar = require('chokidar');
/*
* Creates an object readable by client
*/
const createFileIter = (core, realRoot, file) => {
const createFileIter = (core, realRoot, file, options = {}) => {
const filename = path.basename(file);
const realPath = path.join(realRoot, filename);
const {mime} = core.make('osjs/vfs');

const createStat = stat => ({
isDirectory: stat.isDirectory(),
isFile: stat.isFile(),
mime: stat.isFile() ? mime(realPath) : null,
size: stat.size,
path: file,
filename,
stat
});
const createStat = async stat => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method of figuring out the total size of a directory seems very inefficient, especially when there's a lot of files.

Maybe it would be better looking into spawning a system process to do this in a native way ? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I searched, du can be used liked this:

const child = spawn('sh', ['-c', 'du -s ${realPath} | cut -f1']);

But the problem is that du seems to have considerable delay, especially for large directories.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

especially for large directories.

But I would assume it's faster than doing it with node, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I'm not 100% sure, but I assume that this wouldn't be cross-compatible with users hoping to run OS.js locally on Windows. IMHO native system processes shouldn't be included without a fallback for Windows

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a du npm package that handles Windows etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In new commit du package is used for totalSize. @andersevenrud

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking about our own vfs system adapter, which is a cloud object storage.
Since totalSize is not provided in APIs, I have to iterate through files which as you said, gets lots of time. Is there any other solution?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I can't think of a good solution to that, especially if there's pagination involved :/

let totalSize = 0;
let files = [];
if (stat.isDirectory()) {
files = await fs.readdir(realPath);
if(!options.showHiddenFiles) {
files = files.filter(f => f.substr(0, 1) !== '.');
}
const promises = files.map(f => fs.stat(path.join(realPath, f)));
const allPromises = await Promise.all(promises);
allPromises.map(s => totalSize += s.size);
}

return ({
isDirectory: stat.isDirectory(),
isFile: stat.isFile(),
mime: stat.isFile() ? mime(realPath) : null,
size: stat.size,
path: file,
totalCount: stat.isDirectory() ? files.length : null,
totalSize: stat.isDirectory() ? totalSize : null,
filename,
stat
});
};

return fs.stat(realPath)
.then(createStat)
Expand Down Expand Up @@ -196,7 +212,7 @@ module.exports = (core) => {
Promise.resolve(getRealPath(core, options.session, vfs.mount, file))
.then(realPath => {
return fs.access(realPath, fs.F_OK)
.then(() => createFileIter(core, path.dirname(realPath), realPath));
.then(() => createFileIter(core, path.dirname(realPath), realPath, options));
}),

/**
Expand Down