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

Infrastructure for writing to pluggable Galaxy file sources. #10152

Merged
merged 4 commits into from
Sep 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 17 additions & 10 deletions client/src/components/FilesDialog/FilesDialog.vue
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ export default {
default: "file",
validator: (prop) => ["file", "directory"].includes(prop),
},
requireWritable: {
type: Boolean,
default: false,
},
},
data() {
return {
Expand Down Expand Up @@ -129,16 +133,19 @@ export default {
this.services
.getFileSources()
.then((items) => {
this.items = items.map((item) => {
return {
id: item.id,
label: item.label,
details: item.doc,
isLeaf: false,
url: item.uri_root,
labelTitle: item.uri_root,
};
});
items = items
.filter((item) => !this.requireWritable || item.writable)
.map((item) => {
return {
id: item.id,
label: item.label,
details: item.doc,
isLeaf: false,
url: item.uri_root,
labelTitle: item.uri_root,
};
});
this.items = items;
this.formatRows();
this.optionsShow = true;
this.showTime = false;
Expand Down
10 changes: 10 additions & 0 deletions client/src/mvc/form/form-parameters.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import SelectContent from "mvc/ui/ui-select-content";
import SelectLibrary from "mvc/ui/ui-select-library";
import SelectFtp from "mvc/ui/ui-select-ftp";
import RulesEdit from "mvc/ui/ui-rules-edit";
import FileSource from "mvc/ui/ui-file-source";
import ColorPicker from "mvc/ui/ui-color-picker";
import DataPicker from "mvc/ui/ui-data-picker";

Expand Down Expand Up @@ -37,6 +38,7 @@ export default Backbone.Model.extend({
ftpfile: "_fieldFtp",
upload: "_fieldUpload",
rules: "_fieldRulesEdit",
directory_uri: "_fieldDirectoryUri",
data_dialog: "_fieldDialog",
},

Expand Down Expand Up @@ -238,6 +240,14 @@ export default Backbone.Model.extend({
});
},

_fieldDirectoryUri: function (input_def) {
return new FileSource.View({
id: `field-${input_def.id}`,
onchange: input_def.onchange,
target: input_def.target,
});
},

_fieldRulesEdit: function (input_def) {
return new RulesEdit.View({
id: `field-${input_def.id}`,
Expand Down
100 changes: 100 additions & 0 deletions client/src/mvc/ui/ui-file-source.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import Backbone from "backbone";
import { filesDialog } from "utils/data";
import _l from "utils/localization";
import Ui from "mvc/ui/ui-misc";

var View = Backbone.View.extend({
initialize: function (options) {
this.model = new Backbone.Model();
this.target = options.target;
const props = {
mode: "directory",
requireWritable: true,
};
// create insert edit button
this.browse_button = new Ui.Button({
title: _l("Select"),
icon: "fa fa-edit",
tooltip: _l("Select URI"),
onclick: () => {
filesDialog((uri) => {
this._handleRemoteFilesUri(uri);
}, props);
},
});

// add change event. fires on trigger
this.on("change", () => {
if (options.onchange) {
options.onchange(this.value());
}
});

// create elements
this.setElement(this._template(options));
this.$text = this.$(".ui-uri-preview");
this.$(".ui-file-select-button").append(this.browse_button.$el);
this.listenTo(this.model, "change", this.render, this);
this.render();
},

_handleRemoteFilesUri: function (uri) {
this._setValue(uri);
},

render: function () {
const value = this._value;
if (value) {
if (value.url !== this.$text.text()) {
this.$text.text(value.url);
}
} else {
this.$text.text("select...");
}
},

/** Main Template */
_template: function (options) {
return `
<div class="ui-rules-edit clearfix">
<span class="ui-uri-preview" />
<span class="ui-file-select-button float-left" />
</div>
`;
},

/** Return/Set current value */
value: function (new_value) {
if (new_value !== undefined) {
this._setValue(new_value);
} else {
return this._getValue();
}
},

/** Update input element options */
update: function (input_def) {
this.target = input_def.target;
},

/** Returns current value */
_getValue: function () {
return this._value.url;
},

/** Sets current value */
_setValue: function (new_value) {
if (new_value) {
if (typeof new_value == "string") {
new_value = JSON.parse(new_value);
}
this._value = new_value;
this.model.trigger("error", null);
this.model.trigger("change");
}
},
});

export default {
View: View,
};
4 changes: 2 additions & 2 deletions client/src/mvc/ui/ui-rules-edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ var View = Backbone.View.extend({
tooltip: _l("Edit Rules"),
onclick: () => {
if (view.target) {
view._fetcCollectionAndEdit();
view._fetchCollectionAndEdit();
} else {
view._showRuleEditor(null);
}
Expand Down Expand Up @@ -55,7 +55,7 @@ var View = Backbone.View.extend({
this.collapsible_disabled = true;
},

_fetcCollectionAndEdit: function () {
_fetchCollectionAndEdit: function () {
const view = this;
const url = `${getAppRoot()}api/dataset_collections/${view.target.id}?instance_type=history`;
axios
Expand Down
20 changes: 10 additions & 10 deletions lib/galaxy/files/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def get_file_source_path(self, uri):
if "://" not in uri:
raise exceptions.RequestParameterInvalidException("Invalid uri [%s]" % uri)
scheme, rest = uri.split("://", 1)
if scheme not in self.get_schemas():
if scheme not in self.get_schemes():
raise exceptions.RequestParameterInvalidException("Unsupported URI scheme [%s]" % scheme)

if scheme != "gxfiles":
Expand Down Expand Up @@ -107,30 +107,30 @@ def validate_uri_root(self, uri, user_context):
if not user_ftp_dir or not os.path.exists(user_ftp_dir):
raise exceptions.ObjectNotFound('Your FTP directory does not exist, attempting to upload files to it may cause it to be created.')

def get_file_source(self, id_prefix, schema):
def get_file_source(self, id_prefix, scheme):
for file_source in self._file_sources:
# gxfiles uses prefix to find plugin, other schema are assumed to have
# gxfiles uses prefix to find plugin, other scheme are assumed to have
# at most one file_source.
if schema != file_source.get_schema():
if scheme != file_source.get_scheme():
continue
prefix_match = schema != "gxfiles" or file_source.get_prefix() == id_prefix
prefix_match = scheme != "gxfiles" or file_source.get_prefix() == id_prefix
if prefix_match:
return file_source

def looks_like_uri(self, path_or_uri):
# is this string a URI this object understands how to realize
if path_or_uri.startswith("gx") and "://" in path_or_uri:
for scheme in self.get_schemas():
for scheme in self.get_schemes():
if path_or_uri.startswith("%s://" % scheme):
return True

return False

def get_schemas(self):
schemas = set()
def get_schemes(self):
schemes = set()
for file_source in self._file_sources:
schemas.add(file_source.get_schema())
return schemas
schemes.add(file_source.get_scheme())
return schemes

def plugins_to_dict(self, for_serialization=False, user_context=None):
rval = []
Expand Down
39 changes: 32 additions & 7 deletions lib/galaxy/files/sources/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,27 @@

from galaxy.util.template import fill_template

DEFAULT_SCHEME = "gxfiles"
DEFAULT_WRITABLE = False


@six.add_metaclass(abc.ABCMeta)
class FilesSource(object):
"""
"""

@abc.abstractproperty
@abc.abstractmethod
def get_uri_root(self):
"""Return a prefix for the root (e.g. gxfiles://prefix/)."""

@abc.abstractproperty
def get_schema(self):
@abc.abstractmethod
def get_scheme(self):
"""Return a prefix for the root (e.g. the gxfiles in gxfiles://prefix/path)."""

@abc.abstractmethod
def get_writable(self):
"""Return a boolean indicating if this target is writable."""

# TODO: off-by-default
@abc.abstractmethod
def list(self, source_path="/", recursive=False, user_context=None):
Expand All @@ -29,6 +36,10 @@ def list(self, source_path="/", recursive=False, user_context=None):
def realize_to(self, source_path, native_path, user_context=None):
"""Realize source path (relative to uri root) to local file system path."""

def write_from(self, target_path, native_path, user_context=None):
"""Write file at native path to target_path (relative to uri root).
"""

@abc.abstractmethod
def to_dict(self, for_serialization=False, user_context=None):
"""Return a dictified representation of this FileSource instance.
Expand All @@ -43,13 +54,16 @@ class BaseFilesSource(FilesSource):
def get_prefix(self):
return self.id

def get_schema(self):
def get_scheme(self):
return "gxfiles"

def get_writable(self):
return self.writable

def get_uri_root(self):
prefix = self.get_prefix()
schema = self.get_schema()
root = "%s://" % schema
scheme = self.get_scheme()
root = "%s://" % scheme
if prefix:
root = uri_join(root, prefix)
return root
Expand All @@ -63,7 +77,8 @@ def _parse_common_config_opts(self, kwd):
self.id = kwd.pop("id")
self.label = kwd.pop("label", None) or self.id
self.doc = kwd.pop("doc", None)
self.schema = kwd.pop("schema", "gxfiles")
self.scheme = kwd.pop("scheme", DEFAULT_SCHEME)
self.writable = kwd.pop("writable", DEFAULT_WRITABLE)
# If coming from to_dict, strip API helper values
kwd.pop("uri_root", None)
kwd.pop("type", None)
Expand All @@ -76,6 +91,7 @@ def to_dict(self, for_serialization=False, user_context=None):
"uri_root": self.get_uri_root(),
"label": self.label,
"doc": self.doc,
"writable": self.writable,
}
if for_serialization:
rval.update(self._serialization_props(user_context=user_context))
Expand All @@ -96,6 +112,15 @@ def _serialization_props(self):
Used in to_dict method if for_serialization is True.
"""

def write_from(self, target_path, native_path, user_context=None):
if not self.get_writable():
raise Exception("Cannot write to a non-writable file source.")
self._write_from(target_path, native_path, user_context=user_context)

@abc.abstractmethod
def _write_from(self, target_path, native_path, user_context=None):
pass

def _evaluate_prop(self, prop_val, user_context):
rval = prop_val
if isinstance(prop_val, str) and "$" in prop_val:
Expand Down
4 changes: 4 additions & 0 deletions lib/galaxy/files/sources/_pyfilesystem2.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ def realize_to(self, source_path, native_path, user_context=None):
with open(native_path, 'wb') as write_file:
self._open_fs(user_context=user_context).download(source_path, write_file)

def _write_from(self, target_path, native_path, user_context=None):
with open(native_path, 'rb') as read_file:
self._open_fs(user_context=user_context).upload(target_path, read_file)

def _resource_info_to_dict(self, dir_path, resource_info):
name = resource_info.name
path = os.path.join(dir_path, name)
Expand Down
7 changes: 4 additions & 3 deletions lib/galaxy/files/sources/galaxy.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ def __init__(self, label="FTP Directory", doc="Galaxy User's FTP Directory", roo
root=root,
label=label,
doc=doc,
writable=True,
Copy link
Member

Choose a reason for hiding this comment

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

Oh this is super cool! However, currently, our standard FTP server is usually not configured to allow downloads. We would need a second server with a different directory that only allows downloads and no uploads - is that correct @natefoo? If so, is it time to introduce ftp_download_* options?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would think that would be a very natural place to start dumping large files (history exports, collection zips, etc.), so I think swapping the default here to True makes sense. It can be overridden explicitly with the config file but it probably makes sense to have galaxy.yml options also.

I guess for options we should make ftp_upload_dir an alias for ftp_dir and user_library_dir an alias for user_library_import_dir and then add ftp_dir_writable (default True) and user_library_dir_writable (default False)?

Copy link
Member

Choose a reason for hiding this comment

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

so I think swapping the default here to True makes sense.

Yes for sure. This is IMHO the easiest to set up globally and has the biggest impact.
But then I learned I can not download those datasets from the standard FTP directories :)

ftp_dir_writable

I'm not sure if this is enough. Would like to have @natefoo opinion here. Is it still recommended to make the upload-ftp upload-only? I don't think having two FTP servers running is a big deal. But maybe all of that is not necessary anymore?

Another aspect is that two different directories for up- and download might make also sense from the admin point of view.
We have cron jobs running cleaning old unused FTP uploads ... we probably want to handle "archives" (downloads) differently.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to get @natefoo's insights also. But I will just say none of this prevents you from configuring two FTP directories - the whole thing is configurable now. I just think that is an advanced configuration and shouldn't be the default assumed.

If we're going to make a push toward documenting more advanced configurations - I don't think we should take user library import dir, ftp dir, and add a new fixed named thing like ftp download dir. We should instead work toward just documenting how to configure any of these things you want in a galaxy files configuration - including other directories with other options, etc..

Copy link
Member

Choose a reason for hiding this comment

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

If we're going to make a push toward documenting more advanced configurations - I don't think we should take user library import dir, ftp dir, and add a new fixed named thing like ftp download dir. We should instead work toward just documenting how to configure any of these things you want in a galaxy files configuration - including other directories with other options, etc..

Ok, you convinced me. If we can easily use a second FTP this is not a problem at all. But the default configuration should be secure imho.

Thanks for this PR!

)
posix_kwds.update(kwd)
if "delete_on_realize" not in posix_kwds:
Expand All @@ -22,7 +23,7 @@ def __init__(self, label="FTP Directory", doc="Galaxy User's FTP Directory", roo
def get_prefix(self):
return None

def get_schema(self):
def get_scheme(self):
return "gxftp"


Expand All @@ -42,7 +43,7 @@ def __init__(self, label="Library Import Directory", doc="Galaxy's library impor
def get_prefix(self):
return None

def get_schema(self):
def get_scheme(self):
return "gximport"


Expand All @@ -62,7 +63,7 @@ def __init__(self, label="Library User Import Directory", doc="Galaxy's user lib
def get_prefix(self):
return None

def get_schema(self):
def get_scheme(self):
return "gxuserimport"


Expand Down
Loading