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.

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

Adding file size to views #3539

Merged
merged 28 commits into from
Apr 19, 2018
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
3650cc7
adding file size to model
ashleytqy Apr 14, 2018
b864f5a
make file size optional pproperty
ckilcrease Apr 14, 2018
3315dbd
only get filesize for files and notebooks
ckilcrease Apr 15, 2018
388d636
starting out the sort function
ashleytqy Apr 15, 2018
38acc78
add file size text
ashleytqy Apr 15, 2018
59b7c66
revert chages
ashleytqy Apr 15, 2018
86c45a1
fix sort function
ashleytqy Apr 15, 2018
a299be7
fix sorting function foreal
ashleytqy Apr 15, 2018
6475eaa
empty function to format filesize
ashleytqy Apr 15, 2018
d6c7f2e
change column width, flip order
ashleytqy Apr 17, 2018
42c3c02
converting filesize to readable format
ckilcrease Apr 17, 2018
8c96ad5
cleanup
ashleytqy Apr 17, 2018
d19f970
fixed filesize
ckilcrease Apr 17, 2018
a47a188
Merge branch 'filesize' of https://github.com/nyu-ossd-s18/notebook i…
ckilcrease Apr 17, 2018
ff8e1cc
only get filesize for files and notebooks
ckilcrease Apr 15, 2018
abe05de
change column width, flip order
ashleytqy Apr 17, 2018
13933f3
converting filesize to readable format
ckilcrease Apr 17, 2018
acc2056
fixed filesize
ckilcrease Apr 17, 2018
2e9ba4a
use pretty-bytes
ashleytqy Apr 18, 2018
8cc9734
merged changes
ckilcrease Apr 18, 2018
06214ae
remove inline css
ckilcrease Apr 18, 2018
d35ac8b
right align file size column
ashleytqy Apr 18, 2018
dd608ad
add MIT licence
ashleytqy Apr 18, 2018
577cbe5
update api description
ashleytqy Apr 18, 2018
dee58e0
use text() not html()
ashleytqy Apr 18, 2018
174e724
get file size in base model
ckilcrease Apr 18, 2018
192e3fe
remove es6 syntax
ckilcrease Apr 18, 2018
160754d
None -> null for JSON API
takluyver Apr 19, 2018
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
4 changes: 4 additions & 0 deletions notebook/services/api/api.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,10 @@ definitions:
type: string
description: Last modified timestamp
format: dateTime
size:
type: integer
Copy link
Member

Choose a reason for hiding this comment

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

Integer or None ? If size can't be found, probably say that in the description at least.

description: "The size of the file or directory."
format: bytes
Copy link
Member

Choose a reason for hiding this comment

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

I think this should just go in the description. Swagger specifies a fixed list of formats for things like dateTime. From the spec's point of view, this is just a number - I don't think it has a way of declaring a unit.

mimetype:
type: string
description: "The mimetype of a file. If content is not null, and type is 'file', this will contain the mimetype of the file, otherwise this will be null."
Expand Down
17 changes: 16 additions & 1 deletion notebook/services/contents/filemanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,15 @@ def is_hidden(self, path):
os_path = self._get_os_path(path=path)
return is_hidden(os_path, self.root_dir)

def _get_file_size(self, path):
try:
# size of file
size = os.path.getsize(path)
Copy link
Member

Choose a reason for hiding this comment

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

This will do a new stat on the file, which is best avoided for performance reasons. We already get a stat object in _base_model() - perhaps we can use the size from that, and remove it again for directories. It's a bit of an ugly trick, but for a big directory, it could make a difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

@takluyver thank you -- I've updated so that it uses file size from st_size (retrieved from stat) in _base_model(), but let me know if this isn't what you meant

except (ValueError, OSError):
self.log.warning('Unable to get size.')
size = None
return size

def file_exists(self, path):
"""Returns True if the file exists, else returns False.

Expand Down Expand Up @@ -270,6 +279,7 @@ def _base_model(self, path):
model['content'] = None
model['format'] = None
model['mimetype'] = None

try:
model['writable'] = os.access(os_path, os.W_OK)
except OSError:
Expand Down Expand Up @@ -333,6 +343,7 @@ def _dir_model(self, path, content=True):

return model


def _file_model(self, path, content=True, format=None):
"""Build a model for a file

Expand All @@ -348,6 +359,7 @@ def _file_model(self, path, content=True, format=None):

os_path = self._get_os_path(path)
model['mimetype'] = mimetypes.guess_type(os_path)[0]
model['size'] = self._get_file_size(os_path)

if content:
content, format = self._read_file(os_path, format)
Expand All @@ -373,13 +385,16 @@ def _notebook_model(self, path, content=True):
"""
model = self._base_model(path)
model['type'] = 'notebook'
os_path = self._get_os_path(path)
model['size'] = self._get_file_size(os_path)

if content:
os_path = self._get_os_path(path)
nb = self._read_notebook(os_path, as_version=4)
self.mark_trusted_cells(nb, path)
model['content'] = nb
model['format'] = 'json'
self.validate_notebook_model(model)

return model

def get(self, path, content=True, type=None, format=None):
Expand Down
28 changes: 28 additions & 0 deletions notebook/static/base/js/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -1038,7 +1038,34 @@ define([
return (order == 1) ? 1 : -1;
}
};

// source: https://github.com/sindresorhus/pretty-bytes
Copy link
Member

Choose a reason for hiding this comment

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

I'm OK with this since it's one fairly small function, and it's unlikely to change. But we should include a copy of the license in the comment.

https://github.com/sindresorhus/pretty-bytes/blob/master/license

var format_filesize = function(num) {
if (num === undefined)
return;

var UNITS = ['B', 'kB', 'MB', 'GB', 'TB', 'PB', 'EB', 'ZB', 'YB'];

if (!Number.isFinite(num)) {
console.error(`Expected a finite number, got ${typeof num}: ${num}`);
}

var neg = num < 0;

if (neg) {
num = -num;
}

if (num < 1) {
return (neg ? '-' : '') + num + ' B';
}

var exponent = Math.min(Math.floor(Math.log10(num) / 3), UNITS.length - 1);
var numStr = Number((num / Math.pow(1000, exponent)).toPrecision(3));
var unit = UNITS[exponent];

return (neg ? '-' : '') + numStr + ' ' + unit;
}

// javascript stores text as utf16 and string indices use "code units",
// which stores high-codepoint characters as "surrogate pairs",
Expand Down Expand Up @@ -1180,6 +1207,7 @@ define([
parse_b64_data_uri: parse_b64_data_uri,
time: time,
format_datetime: format_datetime,
format_filesize: format_filesize,
datetime_sort_helper: datetime_sort_helper,
dnd_contain_file: dnd_contain_file,
js_idx_to_char_idx: js_idx_to_char_idx,
Expand Down
33 changes: 32 additions & 1 deletion notebook/static/tree/js/notebooklist.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,34 @@ define([
});
}

function size_sorter(ascending) {
var order = ascending ? 1 : 0;
// directories have file size of undefined
return (function(a, b) {
if (a.size === undefined) {
return (ascending) ? -1 : 1;
}

if (b.size === undefined) {
return (ascending) ? 1 : -1;
}

if (a.size > b.size) {
return (ascending) ? -1 : 1;
}

if (b.size > a.size) {
return (ascending) ? 1 : -1;
}

return 0;
});
}

var sort_functions = {
'sort-name': name_sorter,
'last-modified': modified_sorter
'last-modified': modified_sorter,
'file-size': size_sorter
};

var NotebookList = function (selector, options) {
Expand Down Expand Up @@ -520,6 +545,11 @@ define([
.addClass("item_name")
.appendTo(link);

$("<span/>")
.addClass("file_size")
.addClass("pull-right")
.appendTo(item);

$("<span/>")
.addClass("item_modified")
.addClass("pull-right")
Expand Down Expand Up @@ -835,6 +865,7 @@ define([
// Add in the date that the file was last modified
item.find(".item_modified").text(utils.format_datetime(model.last_modified));
item.find(".item_modified").attr("title", moment(model.last_modified).format("YYYY-MM-DD HH:mm"));
item.find(".file_size").html(utils.format_filesize(model.size) || "&nbsp;");
Copy link
Member

Choose a reason for hiding this comment

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

the use of .html() should be a red flag. From a security standpoint it would not pass an audit. We had security issues before because of HTML usage.

If somewhere where to replace you API to return cannot find <size> for file <file> the you get scrip injection in your page, and your client is compromised.

If you've seen use of .html() in other place of the javascript where .text() should/could be used, please open an issue.

Copy link
Contributor Author

@ashleytqy ashleytqy Apr 18, 2018

Choose a reason for hiding this comment

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

re: using .html() – we're using it so that &nbsp; renders because .text() doesn't display an empty string. do you have any suggestions to how to display a &nbsp; without using .html?

it's also used here, which i'm assuming the author also wanted to use the &nbsp; character

$('#counter-select-all').html(checked===0 ? '&nbsp;' : checked);

Copy link
Member

Choose a reason for hiding this comment

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

Good question, there are several way to approach that. A simple one would be to use .html() just for &nbsp;:

if (cfoo){
	$(...).text(foo); 
} else {
    $(...).html("&nbsp");
}

That reduce the use of html so it's one less risk.

Thinking about it more, &nbsp; is also just an escape sequence for unicode character Non Breaking Space, which you can enter with your keyboard using some clever keyboard combinaison | | (nbsp), not like | |(space), check the source, on mac it's Alt Space. That's 1) annoying to type 2) annoying to see when reading code. That's not optimal. After thinking a few minutes more you can realize that hopefully most programming language allow you to have escape sequence, knowing that Non-Breaking space is unicode U+00A0 you can go with :

.text(checked===0 ? '\xA0' : checked); 

If you want to even avoid using jQuery you can use the innerText attribute and assign to it.

With your gained knowledge you can even open an issue for $('#counter-select-all').html(checked===0 ? '&nbsp;' : checked); to be fixed.

As an exercise – if you like the fun of hacking – get in a shoes of an attacker and try to modify the server side to not return the size as an integer but a string, and attempt to have a script injection that display a message in the frontend.

Copy link
Member

Choose a reason for hiding this comment

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

Stepping back a bit from unicode discussions: do we actually need a non-breaking space at all? I think the layout should still work if the element has no text.

Copy link
Contributor Author

@ashleytqy ashleytqy Apr 18, 2018

Choose a reason for hiding this comment

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

I fixed it with using \xA0 (thanks @Carreau !) . Without text, the span element wrapping it doesn't seem to adhere to the width it's given.

};


Expand Down
4 changes: 4 additions & 0 deletions notebook/static/tree/less/tree.less
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,10 @@ ul.breadcrumb {
margin-left: @dashboard_lr_pad;
}

.file_size{
width: 65px;
}

[dir="rtl"] .item_modified.pull-right{
.pull-left();
}
Expand Down
6 changes: 6 additions & 0 deletions notebook/templates/tree.html
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,12 @@
{% endfor %}
</ul>
</div>
<div id="file_size" class="pull-right sort_button">
<span class="btn btn-xs btn-default sort-action" id="file-size">
{% trans %}File size{% endtrans %}
<i class="fa"></i>
</span>
</div>
<div id="last_modified" class="pull-right sort_button">
<span class="btn btn-xs btn-default sort-action" id="last-modified">
{% trans %}Last Modified{% endtrans %}
Expand Down