From a980a65b8d88d4e48353643d8e6b0681e8993fe3 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Wed, 30 Sep 2020 12:00:41 +0200 Subject: [PATCH 1/3] Fix composite data download from collection We wrongly pass hdca (and element_identifier) into _archive_composite_dataset, which breaks downloading composite datasets from a collection. Fixes https://sentry.galaxyproject.org/sentry/main/issues/290622/: ``` Exception: Unknown parameter type () File "galaxy/web/framework/middleware/sentry.py", line 43, in __call__ iterable = self.application(environ, start_response) File "/cvmfs/main.galaxyproject.org/venv/lib/python3.6/site-packages/paste/recursive.py", line 85, in __call__ return self.application(environ, start_response) File "galaxy/web/framework/middleware/statsd.py", line 33, in __call__ req = self.application(environ, start_response) File "/cvmfs/main.galaxyproject.org/venv/lib/python3.6/site-packages/paste/httpexceptions.py", line 640, in __call__ return self.application(environ, start_response) File "galaxy/web/framework/base.py", line 141, in __call__ return self.handle_request(environ, start_response) File "galaxy/web/framework/base.py", line 220, in handle_request body = method(trans, **kwargs) File "galaxy/webapps/galaxy/controllers/dataset.py", line 186, in display return data.datatype.display_data(trans, data, preview, filename, to_ext, offset=offset, ck_size=ck_size, **kwd) File "galaxy/datatypes/data.py", line 444, in display_data return self._archive_composite_dataset(trans, data, **kwd) File "galaxy/datatypes/data.py", line 266, in _archive_composite_dataset params = util.Params(kwd) File "galaxy/util/__init__.py", line 822, in __init__ self.__dict__[key] = sanitize_param(value) File "galaxy/util/__init__.py", line 596, in sanitize_param raise Exception('Unknown parameter type (%s)' % (type(value))) ``` --- lib/galaxy/datatypes/data.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/datatypes/data.py b/lib/galaxy/datatypes/data.py index 9cf2cc1e429..cdb5ea94284 100644 --- a/lib/galaxy/datatypes/data.py +++ b/lib/galaxy/datatypes/data.py @@ -441,7 +441,7 @@ class Data(metaclass=DataMeta): from galaxy import datatypes # DBTODO REMOVE THIS AT REFACTOR if to_ext or isinstance(data.datatype, datatypes.binary.Binary): # Saving the file, or binary file if data.extension in composite_extensions: - return self._archive_composite_dataset(trans, data, **kwd) + return self._archive_composite_dataset(trans, data, do_action=kwd.get('do_action'), msg=kwd.get('msg')) else: trans.response.headers['Content-Length'] = int(os.stat(data.file_name).st_size) filename = self._download_filename(data, to_ext, hdca=kwd.get("hdca", None), element_identifier=kwd.get("element_identifier", None)) From 5ce78cb5cd71446f3096fb5da24236f155a0c14e Mon Sep 17 00:00:00 2001 From: Nicola Soranzo Date: Wed, 30 Sep 2020 12:21:20 +0100 Subject: [PATCH 2/3] Simplify `Data._archive_composite_dataset()` --- lib/galaxy/datatypes/data.py | 135 +++++++++++++++++------------------ 1 file changed, 64 insertions(+), 71 deletions(-) diff --git a/lib/galaxy/datatypes/data.py b/lib/galaxy/datatypes/data.py index cdb5ea94284..c40b64a0b4b 100644 --- a/lib/galaxy/datatypes/data.py +++ b/lib/galaxy/datatypes/data.py @@ -261,84 +261,77 @@ class Data(metaclass=DataMeta): messagetype = "error" return error, msg, messagetype - def _archive_composite_dataset(self, trans, data=None, **kwd): + def _archive_composite_dataset(self, trans, data, do_action='zip', msg=''): # save a composite object into a compressed archive for downloading - params = util.Params(kwd) outfname = data.name[0:150] outfname = ''.join(c in FILENAME_VALID_CHARS and c or '_' for c in outfname) - if params.do_action is None: - params.do_action = 'zip' # default - msg = util.restore_text(params.get('msg', '')) - if not data: - msg = "You must select at least one dataset" - else: - error = False - try: - if params.do_action == 'zip': - # Can't use mkstemp - the file must not exist first - tmpd = tempfile.mkdtemp(dir=trans.app.config.new_file_path, prefix='gx_composite_archive_') - util.umask_fix_perms(tmpd, trans.app.config.umask, 0o777, trans.app.config.gid) - tmpf = os.path.join(tmpd, 'library_download.' + params.do_action) - archive = zipfile.ZipFile(tmpf, 'w', zipfile.ZIP_DEFLATED, True) + error = False + try: + if do_action == 'zip': + # Can't use mkstemp - the file must not exist first + tmpd = tempfile.mkdtemp(dir=trans.app.config.new_file_path, prefix='gx_composite_archive_') + util.umask_fix_perms(tmpd, trans.app.config.umask, 0o777, trans.app.config.gid) + tmpf = os.path.join(tmpd, 'library_download.' + do_action) + archive = zipfile.ZipFile(tmpf, 'w', zipfile.ZIP_DEFLATED, True) - def zipfile_add(fpath, arcname): - encoded_arcname = arcname.encode('CP437') - try: - archive.write(fpath, encoded_arcname) - except TypeError: - # Despite documenting the need for CP437 encoded arcname, - # python 3 actually needs this to be a unicode string ... - # https://bugs.python.org/issue24110 - archive.write(fpath, arcname) + def zipfile_add(fpath, arcname): + encoded_arcname = arcname.encode('CP437') + try: + archive.write(fpath, encoded_arcname) + except TypeError: + # Despite documenting the need for CP437 encoded arcname, + # python 3 actually needs this to be a unicode string ... + # https://bugs.python.org/issue24110 + archive.write(fpath, arcname) - archive.add = zipfile_add + archive.add = zipfile_add - elif params.do_action == 'tgz': - archive = util.streamball.StreamBall('w|gz') - elif params.do_action == 'tbz': - archive = util.streamball.StreamBall('w|bz2') - except (OSError, zipfile.BadZipFile): - error = True - log.exception("Unable to create archive for download") - msg = "Unable to create archive for %s for download, please report this error" % outfname + elif do_action == 'tgz': + archive = util.streamball.StreamBall('w|gz') + elif do_action == 'tbz': + archive = util.streamball.StreamBall('w|bz2') + except (OSError, zipfile.BadZipFile): + error = True + log.exception("Unable to create archive for download") + msg = "Unable to create archive for %s for download, please report this error" % outfname + if not error: + ext = data.extension + path = data.file_name + efp = data.extra_files_path + # Add any central file to the archive, + + display_name = os.path.splitext(outfname)[0] + if not display_name.endswith(ext): + display_name = '{}_{}'.format(display_name, ext) + + error, msg = self._archive_main_file(archive, display_name, path)[:2] if not error: - ext = data.extension - path = data.file_name - efp = data.extra_files_path - # Add any central file to the archive, - - display_name = os.path.splitext(outfname)[0] - if not display_name.endswith(ext): - display_name = '{}_{}'.format(display_name, ext) - - error, msg = self._archive_main_file(archive, display_name, path)[:2] - if not error: - # Add any child files to the archive, - for fpath, rpath in self.__archive_extra_files_path(extra_files_path=efp): - try: - archive.add(fpath, rpath) - except OSError: - error = True - log.exception("Unable to add %s to temporary library download archive", rpath) - msg = "Unable to create archive for download, please report this error" - continue - if not error: - if params.do_action == 'zip': - archive.close() - tmpfh = open(tmpf, 'rb') - # CANNOT clean up - unlink/rmdir was always failing because file handle retained to return - must rely on a cron job to clean up tmp - trans.response.set_content_type("application/x-zip-compressed") - trans.response.headers["Content-Disposition"] = 'attachment; filename="%s.zip"' % outfname - return tmpfh - else: - trans.response.set_content_type("application/x-tar") - outext = 'tgz' - if params.do_action == 'tbz': - outext = 'tbz' - trans.response.headers["Content-Disposition"] = 'attachment; filename="{}.{}"'.format(outfname, outext) - archive.wsgi_status = trans.response.wsgi_status() - archive.wsgi_headeritems = trans.response.wsgi_headeritems() - return archive.stream + # Add any child files to the archive, + for fpath, rpath in self.__archive_extra_files_path(extra_files_path=efp): + try: + archive.add(fpath, rpath) + except OSError: + error = True + log.exception("Unable to add %s to temporary library download archive", rpath) + msg = "Unable to create archive for download, please report this error" + continue + if not error: + if do_action == 'zip': + archive.close() + tmpfh = open(tmpf, 'rb') + # CANNOT clean up - unlink/rmdir was always failing because file handle retained to return - must rely on a cron job to clean up tmp + trans.response.set_content_type("application/x-zip-compressed") + trans.response.headers["Content-Disposition"] = 'attachment; filename="%s.zip"' % outfname + return tmpfh + else: + trans.response.set_content_type("application/x-tar") + outext = 'tgz' + if do_action == 'tbz': + outext = 'tbz' + trans.response.headers["Content-Disposition"] = 'attachment; filename="{}.{}"'.format(outfname, outext) + archive.wsgi_status = trans.response.wsgi_status() + archive.wsgi_headeritems = trans.response.wsgi_headeritems() + return archive.stream return trans.show_error_message(msg) def __archive_extra_files_path(self, extra_files_path): From 812015421db97b8c91aa84cc539b3f39ccfc5a0f Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Wed, 30 Sep 2020 15:29:19 +0200 Subject: [PATCH 3/3] Drop msg from _archive_composite_dataset signature --- lib/galaxy/datatypes/data.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/datatypes/data.py b/lib/galaxy/datatypes/data.py index c40b64a0b4b..524a7bd5f6c 100644 --- a/lib/galaxy/datatypes/data.py +++ b/lib/galaxy/datatypes/data.py @@ -261,11 +261,12 @@ class Data(metaclass=DataMeta): messagetype = "error" return error, msg, messagetype - def _archive_composite_dataset(self, trans, data, do_action='zip', msg=''): + def _archive_composite_dataset(self, trans, data, do_action='zip'): # save a composite object into a compressed archive for downloading outfname = data.name[0:150] outfname = ''.join(c in FILENAME_VALID_CHARS and c or '_' for c in outfname) error = False + msg = '' try: if do_action == 'zip': # Can't use mkstemp - the file must not exist first @@ -434,7 +435,7 @@ class Data(metaclass=DataMeta): from galaxy import datatypes # DBTODO REMOVE THIS AT REFACTOR if to_ext or isinstance(data.datatype, datatypes.binary.Binary): # Saving the file, or binary file if data.extension in composite_extensions: - return self._archive_composite_dataset(trans, data, do_action=kwd.get('do_action'), msg=kwd.get('msg')) + return self._archive_composite_dataset(trans, data, do_action=kwd.get('do_action')) else: trans.response.headers['Content-Length'] = int(os.stat(data.file_name).st_size) filename = self._download_filename(data, to_ext, hdca=kwd.get("hdca", None), element_identifier=kwd.get("element_identifier", None))