From 5663a29a5af71983649727de0c802c19de5805f9 Mon Sep 17 00:00:00 2001 From: Nuwan Goonasekera <2070605+nuwang@users.noreply.github.com> Date: Thu, 11 Jun 2026 23:45:26 +0530 Subject: [PATCH 01/10] Add direct-download redirects for object store datasets Downloading a dataset from a remote (S3-like) object store blocks a web thread while the whole object is pulled into the local cache before any bytes are sent, causing 504 timeouts on large files and making objects larger than the cache undownloadable. Add an opt-in `enable_direct_download` flag per object store backend. When set, plain whole-file downloads are served by redirecting the client (302) to a short-lived presigned URL generated by the backing store, removing Galaxy from the download path entirely. - ObjectStore.get_direct_download_url() gates on the flag and forwards content-disposition/content-type into the presigned URL so the client receives the right filename; implemented for boto3, s3, and azure. - DatasetsService.display() returns a redirect for non-archive whole-file download requests; previews, chunked display, extra-files access, and composite/archived downloads continue to stream as before. - The single-file-vs-archive decision is shared with the existing streamed-download path via Data.is_archive_download(). Refs galaxyproject/galaxy#19255 --- .../packages/api-client/src/schema/schema.ts | 5 + .../sample/object_store_conf.sample.yml | 21 ++- lib/galaxy/datatypes/data.py | 36 +++-- lib/galaxy/objectstore/__init__.py | 47 +++++++ lib/galaxy/objectstore/azure_blob.py | 7 +- lib/galaxy/objectstore/cloud.py | 2 +- .../examples/boto3_direct_download.xml | 7 + .../examples/boto3_direct_download.yml | 18 +++ lib/galaxy/objectstore/irods.py | 2 +- lib/galaxy/objectstore/pithos.py | 2 +- lib/galaxy/objectstore/rucio.py | 4 +- lib/galaxy/objectstore/s3.py | 12 +- lib/galaxy/objectstore/s3_boto3.py | 18 ++- lib/galaxy/webapps/galaxy/api/datasets.py | 14 ++ .../webapps/galaxy/services/datasets.py | 40 ++++++ .../test_direct_download_redirect.py | 128 ++++++++++++++++++ test/unit/objectstore/test_objectstore.py | 59 ++++++++ .../galaxy/services/test_datasets_service.py | 27 ++++ 18 files changed, 423 insertions(+), 26 deletions(-) create mode 100644 lib/galaxy/objectstore/examples/boto3_direct_download.xml create mode 100644 lib/galaxy/objectstore/examples/boto3_direct_download.yml create mode 100644 test/integration/objectstore/test_direct_download_redirect.py create mode 100644 test/unit/webapps/galaxy/services/test_datasets_service.py diff --git a/client/packages/api-client/src/schema/schema.ts b/client/packages/api-client/src/schema/schema.ts index c617af63579..5df50293e03 100644 --- a/client/packages/api-client/src/schema/schema.ts +++ b/client/packages/api-client/src/schema/schema.ts @@ -9126,6 +9126,11 @@ export interface components { description?: string | null; /** Device */ device?: string | null; + /** + * Enable Direct Download + * @default false + */ + enable_direct_download: boolean; /** Name */ name?: string | null; /** Object Expires After Days */ diff --git a/lib/galaxy/config/sample/object_store_conf.sample.yml b/lib/galaxy/config/sample/object_store_conf.sample.yml index 2b0fba0c128..cc973d3f6ed 100644 --- a/lib/galaxy/config/sample/object_store_conf.sample.yml +++ b/lib/galaxy/config/sample/object_store_conf.sample.yml @@ -110,7 +110,7 @@ backends: name: Scratch Storage description: > This data storage is fast and meant for exploratory analysis and methods development. Data stored here is not backed up - and automatically purged after a month. + and automatically purged after a month. badges: - type: faster - type: less_stable @@ -160,7 +160,7 @@ backends: # suitable just for AWS services (aws_s3 & cloud), one is # more suited for non-AWS S3 compatible services (generic_s3), # and finally boto3 gracefully handles either scenario. -# +# # boto3 is built on the newest and most widely used Python client # outside of Galaxy. It has advanced transfer options and is likely # the client you should use for new setup. generic_s3 and aws_s3 @@ -181,6 +181,12 @@ auth: secret_key: ... bucket: name: unique_bucket_name_all_lowercase +# When true, dataset downloads are served by redirecting the client to a short-lived presigned URL +# generated by this bucket, instead of streaming the bytes through Galaxy's cache. This removes Galaxy +# from the download path entirely (avoiding cache pull-through timeouts for large objects), but the +# bucket endpoint must be reachable by clients, and anyone with the URL can fetch the object until it +# expires (~1 hour). Defaults to false. +# enable_direct_download: true connection: # not strictly needed but more of the API works with this. region: us-east-1 transfer: @@ -221,6 +227,9 @@ bucket: name: unique_bucket_name_all_lowercase use_reduced_redundancy: false max_chunk_size: 250 +# Redirect dataset downloads to a short-lived presigned URL served by the bucket instead of streaming +# through Galaxy's cache. See the boto3 example above for the tradeoffs. Defaults to false. +# enable_direct_download: true connection: # not strictly needed but more of the API works with this. region: us-east-1 cache: @@ -246,7 +255,7 @@ connection: # The domain of the Onezone service (e.g. datahub.egi.eu), or its IP address for # devel instances (see above). The minimal supported Onezone version is 21.02.4. onezone_domain: datahub.egi.eu - # Allows connection to Onedata servers that do not present trusted SSL certificates. + # Allows connection to Onedata servers that do not present trusted SSL certificates. # SHOULD NOT be used unless you really know what you are doing. disable_tls_certificate_validation: false space: @@ -343,6 +352,9 @@ bucket: name: unique_bucket_name_all_lowercase use_reduced_redundancy: false max_chunk_size: 250 +# Redirect dataset downloads to a short-lived presigned URL served by the bucket instead of streaming +# through Galaxy's cache. See the boto3 example above for the tradeoffs. Defaults to false. +# enable_direct_download: true connection: host: swift.example.org port: 6000 @@ -367,6 +379,9 @@ auth: container: name: unique_container_name max_chunk_size: 250 +# Redirect dataset downloads to a short-lived SAS URL served by the container instead of streaming +# through Galaxy's cache. See the boto3 example above for the tradeoffs. Defaults to false. +# enable_direct_download: true cache: path: database/object_store_cache_azure size: 1000 diff --git a/lib/galaxy/datatypes/data.py b/lib/galaxy/datatypes/data.py index 96a2c5deff3..da8a1c4432f 100644 --- a/lib/galaxy/datatypes/data.py +++ b/lib/galaxy/datatypes/data.py @@ -481,29 +481,43 @@ class Data(metaclass=DataMeta): file_paths.append(dataset.get_file_name()) return zip(file_paths, rel_paths) - def _serve_file_download(self, headers, data, trans, to_ext, file_size, **kwd): - composite_extensions = trans.app.datatypes_registry.get_composite_extensions() + def is_archive_download(self, datatypes_registry, extension) -> bool: + """Whether downloading a dataset of this `extension` is served as a multi-file archive (zip). + + Composite/bundled datatypes are zipped on the fly rather than served as the single stored + object, so such downloads cannot be satisfied by a direct link to the backing store. + """ + composite_extensions = datatypes_registry.get_composite_extensions() composite_extensions.append("html") # for archiving composite datatypes composite_extensions.append("tool_markdown") # basically should act as an HTML datatype in this capacity composite_extensions.append("data_manager_json") # for downloading bundles if bundled. composite_extensions.append("directory") # for downloading directories. composite_extensions.append("zarr") # for downloading zarr directories. + return extension in composite_extensions - if data.extension in composite_extensions: + def download_content_disposition(self, dataset, to_ext, **kwd) -> str: + """Build the Content-Disposition header value used when downloading `dataset`. + + Shared so direct (e.g. presigned URL) downloads receive the same filename as streamed ones. + """ + filename = self._download_filename( + dataset, + to_ext, + hdca=kwd.get("hdca"), + element_identifier=kwd.get("element_identifier"), + filename_pattern=kwd.get("filename_pattern"), + ) + return to_content_disposition(filename) + + def _serve_file_download(self, headers, data, trans, to_ext, file_size, **kwd): + if self.is_archive_download(trans.app.datatypes_registry, data.extension): return self._archive_composite_dataset(trans, data, headers, do_action=kwd.get("do_action", "zip")) else: headers["Content-Length"] = str(file_size) - filename = self._download_filename( - data, - to_ext, - hdca=kwd.get("hdca"), - element_identifier=kwd.get("element_identifier"), - filename_pattern=kwd.get("filename_pattern"), - ) headers["content-type"] = ( "application/octet-stream" # force octet-stream so Safari doesn't append mime extensions to filename ) - headers["Content-Disposition"] = to_content_disposition(filename) + headers["Content-Disposition"] = self.download_content_disposition(data, to_ext, **kwd) return open(data.get_file_name(), "rb"), headers def _serve_binary_file_contents_as_text(self, trans, data, headers, file_size, max_peek_size): diff --git a/lib/galaxy/objectstore/__init__.py b/lib/galaxy/objectstore/__init__.py index 3e9a56ec216..0135465cf96 100644 --- a/lib/galaxy/objectstore/__init__.py +++ b/lib/galaxy/objectstore/__init__.py @@ -319,6 +319,19 @@ class ObjectStore(metaclass=abc.ABCMeta): """ raise NotImplementedError() + @abc.abstractmethod + def get_direct_download_url( + self, obj, content_disposition: Optional[str] = None, content_type: Optional[str] = None + ) -> Optional[str]: + """Return a URL a client can be redirected to in order to download `obj` directly from the backing store. + + Returns None unless the concrete store supports direct access *and* the admin has opted in via the + ``enable_direct_download`` configuration flag. ``content_disposition`` and ``content_type``, when + supported by the backend, are baked into the URL so the client receives the right download filename + and content type. + """ + raise NotImplementedError() + @abc.abstractmethod def get_concrete_store_name(self, obj): """Return a display name or title of the objectstore corresponding to obj. @@ -673,6 +686,17 @@ class BaseObjectStore(ObjectStore): obj_dir=obj_dir, ) + def get_direct_download_url( + self, obj, content_disposition: Optional[str] = None, content_type: Optional[str] = None + ) -> Optional[str]: + return self._invoke( + "get_direct_download_url", obj, content_disposition=content_disposition, content_type=content_type + ) + + def _get_direct_download_url(self, obj, content_disposition=None, content_type=None) -> Optional[str]: + # Stores that don't support direct download (or haven't opted in) get this no-op default. + return None + def get_concrete_store_name(self, obj): return self._invoke("get_concrete_store_name", obj) @@ -701,6 +725,13 @@ class BaseObjectStore(ObjectStore): private = asbool(config_xml.attrib.get("private", DEFAULT_PRIVATE)) return private + @classmethod + def parse_enable_direct_download_from_config_xml(clazz, config_xml): + enable_direct_download = False + if config_xml is not None: + enable_direct_download = asbool(config_xml.attrib.get("enable_direct_download", False)) + return enable_direct_download + @classmethod def parse_badges_from_config_xml(clazz, badges_xml): badges = [] @@ -758,6 +789,10 @@ class ConcreteObjectStore(BaseObjectStore): self.quota_source = quota_config.get("source", DEFAULT_QUOTA_SOURCE) self.quota_enabled = quota_config.get("enabled", DEFAULT_QUOTA_ENABLED) self.device_id = config_dict.get("device", None) + # Allow clients to download this store's datasets directly from the backing store (e.g. via a + # presigned URL) instead of streaming through Galaxy. Opt-in; only meaningful for stores whose + # _get_object_url returns a usable URL. + self.enable_direct_download = asbool(config_dict.get("enable_direct_download", False)) self.badges = read_badges(config_dict) def to_dict(self): @@ -772,6 +807,7 @@ class ConcreteObjectStore(BaseObjectStore): } rval["badges"] = self._get_concrete_store_badges(None) rval["device"] = self.device_id + rval["enable_direct_download"] = self.enable_direct_download rval["object_expires_after_days"] = self.object_expires_after_days return rval @@ -784,9 +820,15 @@ class ConcreteObjectStore(BaseObjectStore): quota=QuotaModel(source=self.quota_source, enabled=self.quota_enabled), badges=self._get_concrete_store_badges(None), device=self.device_id, + enable_direct_download=self.enable_direct_download, object_expires_after_days=self.object_expires_after_days, ) + def _get_direct_download_url(self, obj, content_disposition=None, content_type=None) -> Optional[str]: + if not self.enable_direct_download: + return None + return self._get_object_url(obj, content_disposition=content_disposition, content_type=content_type) + def _get_concrete_store_badges(self, obj) -> list[BadgeDict]: return serialize_badges( self.badges, @@ -1255,6 +1297,10 @@ class NestedObjectStore(BaseObjectStore): """For the first backend that has this `obj`, get its URL.""" return self._call_method("_get_object_url", obj, None, False, **kwargs) + def _get_direct_download_url(self, obj, **kwargs) -> Optional[str]: + """For the first backend that has this `obj`, get its direct download URL.""" + return self._call_method("_get_direct_download_url", obj, None, False, **kwargs) + def _get_concrete_store_name(self, obj): return self._call_method("_get_concrete_store_name", obj, None, False) @@ -1779,6 +1825,7 @@ class ConcreteObjectStoreModel(BaseModel): quota: QuotaModel badges: list[BadgeDict] device: str | None = None + enable_direct_download: bool = False object_expires_after_days: int | None = None diff --git a/lib/galaxy/objectstore/azure_blob.py b/lib/galaxy/objectstore/azure_blob.py index 30b61b0249f..eb16ea82877 100644 --- a/lib/galaxy/objectstore/azure_blob.py +++ b/lib/galaxy/objectstore/azure_blob.py @@ -85,6 +85,9 @@ def parse_config_xml(config_xml): "transfer": transfer_dict, "extra_dirs": extra_dirs, "private": CachingConcreteObjectStore.parse_private_from_config_xml(config_xml), + "enable_direct_download": CachingConcreteObjectStore.parse_enable_direct_download_from_config_xml( + config_xml + ), } name = config_xml.attrib.get("name", None) if name is not None: @@ -311,7 +314,7 @@ class AzureBlobObjectStore(CachingConcreteObjectStore): log.exception("Could not delete blob '%s' from Azure", rel_path) return False - def _get_object_url(self, obj, **kwargs): + def _get_object_url(self, obj, content_disposition=None, content_type=None, **kwargs): if self._exists(obj, **kwargs): rel_path = self._construct_path(obj, **kwargs) try: @@ -324,6 +327,8 @@ class AzureBlobObjectStore(CachingConcreteObjectStore): blob_name=rel_path, permission=BlobSasPermissions(read=True), expiry=now() + timedelta(hours=1), + content_disposition=content_disposition, + content_type=content_type, ) return f"{url}?{token}" except AzureHttpError: diff --git a/lib/galaxy/objectstore/cloud.py b/lib/galaxy/objectstore/cloud.py index b1baf80dc7a..f37fba51128 100644 --- a/lib/galaxy/objectstore/cloud.py +++ b/lib/galaxy/objectstore/cloud.py @@ -323,7 +323,7 @@ class Cloud(CachingConcreteObjectStore, UsesAxel): log.exception("Could not delete key '%s' from cloud", rel_path) return False - def _get_object_url(self, obj, **kwargs): + def _get_object_url(self, obj, content_disposition=None, content_type=None, **kwargs): if self._exists(obj, **kwargs): rel_path = self._construct_path(obj, **kwargs) try: diff --git a/lib/galaxy/objectstore/examples/boto3_direct_download.xml b/lib/galaxy/objectstore/examples/boto3_direct_download.xml new file mode 100644 index 00000000000..12f5d7adb07 --- /dev/null +++ b/lib/galaxy/objectstore/examples/boto3_direct_download.xml @@ -0,0 +1,7 @@ + + + + + + + diff --git a/lib/galaxy/objectstore/examples/boto3_direct_download.yml b/lib/galaxy/objectstore/examples/boto3_direct_download.yml new file mode 100644 index 00000000000..117b8eb0635 --- /dev/null +++ b/lib/galaxy/objectstore/examples/boto3_direct_download.yml @@ -0,0 +1,18 @@ +type: boto3 +enable_direct_download: true +auth: + access_key: access_moo + secret_key: secret_cow + +bucket: + name: unique_bucket_name_all_lowercase + +cache: + path: database/object_store_cache + size: 1000 + +extra_dirs: +- type: job_work + path: database/job_working_directory_s3 +- type: temp + path: database/tmp_s3 diff --git a/lib/galaxy/objectstore/irods.py b/lib/galaxy/objectstore/irods.py index 7c0fe5d0150..5386c2be7ab 100644 --- a/lib/galaxy/objectstore/irods.py +++ b/lib/galaxy/objectstore/irods.py @@ -590,7 +590,7 @@ class IRODSObjectStore(CachingConcreteObjectStore): return False # Unlike S3, url is not really applicable to iRODS - def _get_object_url(self, obj, **kwargs): + def _get_object_url(self, obj, content_disposition=None, content_type=None, **kwargs): if self._exists(obj, **kwargs): rel_path = self._construct_path(obj, **kwargs) diff --git a/lib/galaxy/objectstore/pithos.py b/lib/galaxy/objectstore/pithos.py index 2e1bfba3862..26fe30501da 100644 --- a/lib/galaxy/objectstore/pithos.py +++ b/lib/galaxy/objectstore/pithos.py @@ -240,7 +240,7 @@ class PithosObjectStore(CachingConcreteObjectStore): log.exception(f"Could not delete path '{path}' from Pithos") return False - def _get_object_url(self, obj, **kwargs): + def _get_object_url(self, obj, content_disposition=None, content_type=None, **kwargs): """ :returns: URL for direct access, None if no object """ diff --git a/lib/galaxy/objectstore/rucio.py b/lib/galaxy/objectstore/rucio.py index 0894ec65914..4a8a17bee17 100644 --- a/lib/galaxy/objectstore/rucio.py +++ b/lib/galaxy/objectstore/rucio.py @@ -598,7 +598,9 @@ class RucioObjectStore(CachingConcreteObjectStore): log.debug("rucio _get_store_usage_percent, not implemented yet") return 0.0 - def _get_object_url(self, obj, extra_dir=None, extra_dir_at_root=False, alt_name=None): + def _get_object_url( + self, obj, extra_dir=None, extra_dir_at_root=False, alt_name=None, content_disposition=None, content_type=None + ): log.debug("rucio _get_object_url") return None diff --git a/lib/galaxy/objectstore/s3.py b/lib/galaxy/objectstore/s3.py index e321ebdb324..ac42372053b 100644 --- a/lib/galaxy/objectstore/s3.py +++ b/lib/galaxy/objectstore/s3.py @@ -105,6 +105,9 @@ def parse_config_xml(config_xml): "cache": cache_dict, "extra_dirs": extra_dirs, "private": CachingConcreteObjectStore.parse_private_from_config_xml(config_xml), + "enable_direct_download": CachingConcreteObjectStore.parse_enable_direct_download_from_config_xml( + config_xml + ), } name = config_xml.attrib.get("name", None) if name is not None: @@ -408,12 +411,17 @@ class S3ObjectStore(CachingConcreteObjectStore, CloudConfigMixin, UsesAxel): def _download_directory_into_cache(self, rel_path, cache_path): download_directory(self._bucket, rel_path, cache_path) - def _get_object_url(self, obj, **kwargs): + def _get_object_url(self, obj, content_disposition=None, content_type=None, **kwargs): if self._exists(obj, **kwargs): rel_path = self._construct_path(obj, **kwargs) try: key = Key(self._bucket, rel_path) - return key.generate_url(expires_in=86400) # 24hrs + response_headers = {} + if content_disposition is not None: + response_headers["response-content-disposition"] = content_disposition + if content_type is not None: + response_headers["response-content-type"] = content_type + return key.generate_url(expires_in=86400, response_headers=response_headers or None) # 24hrs except S3ResponseError: log.exception("Trouble generating URL for dataset '%s'", rel_path) return None diff --git a/lib/galaxy/objectstore/s3_boto3.py b/lib/galaxy/objectstore/s3_boto3.py index 030fb0b6019..103cee1543a 100644 --- a/lib/galaxy/objectstore/s3_boto3.py +++ b/lib/galaxy/objectstore/s3_boto3.py @@ -126,6 +126,9 @@ def parse_config_xml(config_xml): "cache": cache_dict, "extra_dirs": extra_dirs, "private": CachingConcreteObjectStore.parse_private_from_config_xml(config_xml), + "enable_direct_download": CachingConcreteObjectStore.parse_enable_direct_download_from_config_xml( + config_xml + ), } name = config_xml.attrib.get("name", None) if name is not None: @@ -386,16 +389,21 @@ class S3ObjectStore(CachingConcreteObjectStore): with self._atomic_download(local_file_path) as tmp: self._client.download_file(self.bucket, key, tmp) - def _get_object_url(self, obj, **kwargs): + def _get_object_url(self, obj, content_disposition=None, content_type=None, **kwargs): try: if self._exists(obj, **kwargs): rel_path = self._construct_path(obj, **kwargs) + params = { + "Bucket": self.bucket, + "Key": rel_path, + } + if content_disposition is not None: + params["ResponseContentDisposition"] = content_disposition + if content_type is not None: + params["ResponseContentType"] = content_type url = self._client.generate_presigned_url( ClientMethod="get_object", - Params={ - "Bucket": self.bucket, - "Key": rel_path, - }, + Params=params, ExpiresIn=3600, HttpMethod="GET", ) diff --git a/lib/galaxy/webapps/galaxy/api/datasets.py b/lib/galaxy/webapps/galaxy/api/datasets.py index c68d1ec45ae..37b69ac91cb 100644 --- a/lib/galaxy/webapps/galaxy/api/datasets.py +++ b/lib/galaxy/webapps/galaxy/api/datasets.py @@ -21,6 +21,7 @@ from fastapi import ( Request, ) from starlette.responses import ( + RedirectResponse, Response, StreamingResponse, ) @@ -69,6 +70,7 @@ from galaxy.webapps.galaxy.services.datasets import ( DatasetTextContentDetails, DeleteDatasetBatchPayload, DeleteDatasetBatchResult, + DirectDownloadUrl, RequestDataType, UpdateObjectStoreIdPayload, ) @@ -77,6 +79,14 @@ log = logging.getLogger(__name__) router = Router(tags=["datasets"]) +DIRECT_DOWNLOAD_REDIRECT_RESPONSE = { + "description": ( + "Redirect to a URL serving the dataset directly from the backing object store. " + "Only returned for whole-file downloads when the dataset's object store has " + "`enable_direct_download` set." + ), +} + DatasetIDPathParam = Annotated[ DecodedDatabaseIdField, Path(..., description="The encoded database identifier of the dataset.") ] @@ -300,6 +310,7 @@ class FastAPIDatasets: summary="Displays (preview) or downloads dataset content.", tags=["histories"], response_class=StreamingResponse, + responses={302: DIRECT_DOWNLOAD_REDIRECT_RESPONSE}, ) @router.head( "/api/histories/{history_id}/contents/{history_content_id}/display", @@ -327,6 +338,7 @@ class FastAPIDatasets: "/api/datasets/{history_content_id}/display", summary="Displays (preview) or downloads dataset content.", response_class=StreamingResponse, + responses={302: DIRECT_DOWNLOAD_REDIRECT_RESPONSE}, ) @router.head( "/api/datasets/{history_content_id}/display", @@ -373,6 +385,8 @@ class FastAPIDatasets: ck_size=ck_size, **extra_params, ) + if isinstance(display_data, DirectDownloadUrl): + return RedirectResponse(display_data.url, status_code=302, headers=headers) if isinstance(display_data, IOBase): file_name = getattr(display_data, "name", None) if file_name: diff --git a/lib/galaxy/webapps/galaxy/services/datasets.py b/lib/galaxy/webapps/galaxy/services/datasets.py index e304177aeec..444756f169b 100644 --- a/lib/galaxy/webapps/galaxy/services/datasets.py +++ b/lib/galaxy/webapps/galaxy/services/datasets.py @@ -4,6 +4,7 @@ API operations on the contents of a history dataset. import logging import os +from dataclasses import dataclass from enum import Enum from typing import ( Any, @@ -94,6 +95,26 @@ log = logging.getLogger(__name__) DEFAULT_LIMIT = 500 +@dataclass(frozen=True) +class DirectDownloadUrl: + """Sentinel returned by `DatasetsService.display` to signal a redirect to a backing-store URL.""" + + url: str + + +def is_direct_download_candidate(filename, to_ext, raw, offset, ck_size, is_archive) -> bool: + """Whether a display request is a plain whole-file download eligible for a direct backing-store link. + + Excludes extra-files access, chunked display, datatype-processed previews, and archived/composite + downloads -- only a request for the single stored object's bytes can be served directly. + """ + if filename or offset is not None or ck_size is not None: + return False + if is_archive: + return False + return raw or to_ext is not None + + class RequestDataType(str, Enum): """Particular pieces of information that can be requested for a dataset.""" @@ -633,6 +654,22 @@ class DatasetsService(ServiceBase, UsesVisualizationMixin): return rval + def _direct_download_url(self, trans, dataset_instance, filename, to_ext, raw, offset, ck_size) -> Optional[str]: + """Return a backing-store URL the client can be redirected to, or None to stream as usual.""" + datatype = dataset_instance.datatype + is_archive = datatype.is_archive_download(trans.app.datatypes_registry, dataset_instance.extension) + if not is_direct_download_candidate(filename, to_ext, raw, offset, ck_size, is_archive): + return None + content_disposition = None + content_type = None + if to_ext is not None: + # Match the filename/content-type a streamed download would produce. + content_disposition = datatype.download_content_disposition(dataset_instance, to_ext) + content_type = "application/octet-stream" + return trans.app.object_store.get_direct_download_url( + dataset_instance.dataset, content_disposition=content_disposition, content_type=content_type + ) + def display( self, trans: ProvidesHistoryContext, @@ -662,6 +699,9 @@ class DatasetsService(ServiceBase, UsesVisualizationMixin): if filename and filename.startswith("/"): # Path needs to relative to extra files path filename = filename.lstrip("/") + direct_url = self._direct_download_url(trans, dataset_instance, filename, to_ext, raw, offset, ck_size) + if direct_url is not None: + return DirectDownloadUrl(direct_url), headers if raw: if filename and filename != "index": object_store = trans.app.object_store diff --git a/test/integration/objectstore/test_direct_download_redirect.py b/test/integration/objectstore/test_direct_download_redirect.py new file mode 100644 index 00000000000..b6462ed49d1 --- /dev/null +++ b/test/integration/objectstore/test_direct_download_redirect.py @@ -0,0 +1,128 @@ +"""Integration test for direct-download redirects (presigned URLs) from a remote object store. + +Datasets stored in a backing object store with ``enable_direct_download`` set should be served via a +302 redirect to a URL the client fetches directly from the store, instead of being pulled through +Galaxy's cache. Uses a boto3 object store backed by a disposable minio container. +""" + +import os +import string + +import requests + +from galaxy_test.base.populators import DatasetPopulator +from galaxy_test.driver import integration_util +from galaxy_test.driver.integration_util import docker_rm +from ._base import ( + BaseObjectStoreIntegrationTestCase, + files_count, + OBJECT_STORE_ACCESS_KEY, + OBJECT_STORE_HOST, + OBJECT_STORE_PORT, + OBJECT_STORE_SECRET_KEY, + start_minio, +) + +BOTO3_DIRECT_DOWNLOAD_CONFIG = string.Template(""" + + + + + + + + +""") + + +@integration_util.skip_unless_docker() +class TestDirectDownloadRedirectIntegration(BaseObjectStoreIntegrationTestCase): + object_store_cache_path: str + + @classmethod + def setUpClass(cls): + cls.container_name = f"{cls.__name__}_container" + start_minio(cls.container_name) + super().setUpClass() + + @classmethod + def tearDownClass(cls): + docker_rm(cls.container_name) + super().tearDownClass() + + @classmethod + def handle_galaxy_config_kwds(cls, config): + super().handle_galaxy_config_kwds(config) + temp_directory = cls._test_driver.mkdtemp() + cls.object_stores_parent = temp_directory + cls.object_store_cache_path = os.path.join(temp_directory, "object_store_cache") + config_path = os.path.join(temp_directory, "object_store_conf.xml") + config["object_store_store_by"] = "uuid" + with open(config_path, "w") as f: + f.write( + BOTO3_DIRECT_DOWNLOAD_CONFIG.safe_substitute( + { + "temp_directory": temp_directory, + "host": OBJECT_STORE_HOST, + "port": OBJECT_STORE_PORT, + "access_key": OBJECT_STORE_ACCESS_KEY, + "secret_key": OBJECT_STORE_SECRET_KEY, + } + ) + ) + config["object_store_config_file"] = config_path + + def setUp(self): + super().setUp() + self.dataset_populator = DatasetPopulator(self.galaxy_interactor) + + def _display_url(self, hda_id, **params): + return self._api_url(f"datasets/{hda_id}/display", params=params, use_key=True) + + def test_download_redirects_to_presigned_url(self): + history_id = self.dataset_populator.new_history() + hda = self.dataset_populator.new_dataset(history_id, content="123", wait=True) + + # Clear the cache so we can prove the download is served without pulling the object back in. + self._reset_cache() + assert files_count(self.object_store_cache_path) == 0 + + url = self._display_url(hda["id"], to_ext="txt") + response = requests.get(url, allow_redirects=False) + assert response.status_code == 302 + location = response.headers["Location"] + assert OBJECT_STORE_HOST in location + # The presigned URL carries the download filename so the client gets a sensible name. + assert "response-content-disposition" in location.lower() + + # The redirect target is fetchable directly from the object store and holds the data. + direct_response = requests.get(location) + direct_response.raise_for_status() + assert direct_response.content == b"123\n" + + # Galaxy served the download without pulling the object into its cache. + assert files_count(self.object_store_cache_path) == 0 + + def test_raw_download_redirects(self): + history_id = self.dataset_populator.new_history() + hda = self.dataset_populator.new_dataset(history_id, content="raw-bytes", wait=True) + + url = self._display_url(hda["id"], raw="True") + response = requests.get(url, allow_redirects=False) + assert response.status_code == 302 + assert OBJECT_STORE_HOST in response.headers["Location"] + + def test_preview_is_not_redirected(self): + history_id = self.dataset_populator.new_history() + hda = self.dataset_populator.new_dataset(history_id, content="hello", wait=True) + + # A preview (not a download) is processed by the datatype and streamed through Galaxy. + url = self._display_url(hda["id"], preview="True") + response = requests.get(url, allow_redirects=False) + assert response.status_code == 200 + assert "hello" in response.text + + def _reset_cache(self): + for root, _, files in os.walk(self.object_store_cache_path): + for file_ in files: + os.remove(os.path.join(root, file_)) diff --git a/test/unit/objectstore/test_objectstore.py b/test/unit/objectstore/test_objectstore.py index 3704f967250..b047f2aa59f 100644 --- a/test/unit/objectstore/test_objectstore.py +++ b/test/unit/objectstore/test_objectstore.py @@ -676,6 +676,9 @@ def test_config_parse_boto3(): # defaults to AWS assert object_store.endpoint_url is None + # direct download (presigned URL redirects) is opt-in + assert object_store.enable_direct_download is False + cache_target = object_store.cache_target assert cache_target.size == 1000 assert cache_target.path == "database/object_store_cache" @@ -703,6 +706,62 @@ def test_config_parse_boto3(): assert len(extra_dirs) == 2 +@patch_object_stores_to_skip_initialize +def test_config_parse_enable_direct_download(): + for config_str in [get_example("boto3_direct_download.xml"), get_example("boto3_direct_download.yml")]: + with TestConfig(config_str) as (directory, object_store): + assert object_store.enable_direct_download is True + + as_dict = object_store.to_dict() + _assert_key_has_value(as_dict, "enable_direct_download", True) + + model = object_store.to_model("the_object_store_id") + assert model.enable_direct_download is True + + +@patch_object_stores_to_skip_initialize +def test_get_direct_download_url_returns_presigned_url_when_enabled(): + with TestConfig(get_example("boto3_direct_download.yml")) as (directory, object_store): + object_store._client = MagicMock() + object_store._client.generate_presigned_url.return_value = "https://s3.example.org/signed" + with patch.object(object_store, "_exists", return_value=True): + url = object_store.get_direct_download_url(MockDataset(1)) + assert url == "https://s3.example.org/signed" + + +@patch_object_stores_to_skip_initialize +def test_get_direct_download_url_returns_none_when_disabled(): + with TestConfig(get_example("boto3_simple.yml")) as (directory, object_store): + object_store._client = MagicMock() + with patch.object(object_store, "_exists", return_value=True): + url = object_store.get_direct_download_url(MockDataset(1)) + assert url is None + object_store._client.generate_presigned_url.assert_not_called() + + +@patch_object_stores_to_skip_initialize +def test_get_direct_download_url_forwards_content_disposition(): + with TestConfig(get_example("boto3_direct_download.yml")) as (directory, object_store): + object_store._client = MagicMock() + object_store._client.generate_presigned_url.return_value = "https://s3.example.org/signed" + with patch.object(object_store, "_exists", return_value=True): + object_store.get_direct_download_url( + MockDataset(1), + content_disposition='attachment; filename="Galaxy1-[data].txt"', + content_type="application/octet-stream", + ) + _, call_kwargs = object_store._client.generate_presigned_url.call_args + params = call_kwargs["Params"] + assert params["ResponseContentDisposition"] == 'attachment; filename="Galaxy1-[data].txt"' + assert params["ResponseContentType"] == "application/octet-stream" + + +def test_get_direct_download_url_disk_store_returns_none(): + with TestConfig(DISK_TEST_CONFIG) as (directory, object_store): + url = object_store.get_direct_download_url(MockDataset(1)) + assert url is None + + @patch_object_stores_to_skip_initialize def test_config_parse_boto3_custom_connection(): for config_str in [get_example("boto3_custom_connection.xml"), get_example("boto3_custom_connection.yml")]: diff --git a/test/unit/webapps/galaxy/services/test_datasets_service.py b/test/unit/webapps/galaxy/services/test_datasets_service.py new file mode 100644 index 00000000000..a03f289f3cb --- /dev/null +++ b/test/unit/webapps/galaxy/services/test_datasets_service.py @@ -0,0 +1,27 @@ +import pytest + +from galaxy.webapps.galaxy.services.datasets import is_direct_download_candidate + + +@pytest.mark.parametrize( + ("filename", "to_ext", "raw", "offset", "ck_size", "is_archive", "expected"), + [ + # plain download (floppy disk / bioblend) -> candidate + (None, "data", False, None, None, False, True), + # raw byte access of the main file -> candidate + (None, None, True, None, None, False, True), + # preview / display (no to_ext, not raw) -> not a candidate + (None, None, False, None, None, False, False), + # extra-files access -> not a candidate + ("index.html", None, True, None, None, False, False), + ("subfile", "data", False, None, None, False, False), + # chunked display -> not a candidate + (None, "data", False, 0, None, False, False), + (None, "data", False, None, 1024, False, False), + # composite/archived datatypes are zipped through Galaxy -> not a candidate + (None, "data", False, None, None, True, False), + (None, None, True, None, None, True, False), + ], +) +def test_is_direct_download_candidate(filename, to_ext, raw, offset, ck_size, is_archive, expected): + assert is_direct_download_candidate(filename, to_ext, raw, offset, ck_size, is_archive) is expected From c678e5b17f08abca898f86adec0b7055f255da49 Mon Sep 17 00:00:00 2001 From: Nuwan Goonasekera <2070605+nuwang@users.noreply.github.com> Date: Sun, 14 Jun 2026 13:29:32 +0530 Subject: [PATCH 02/10] Fix CI: mypy typing and regenerate client API schema - objectstore: suppress attr-defined on the dynamically dispatched _get_object_url call from ConcreteObjectStore._get_direct_download_url, and give NestedObjectStore._get_direct_download_url an explicit signature matching the base (mypy override compatibility). - client: properly regenerate schema.ts so enable_direct_download lands on both ConcreteObjectStoreModel and its subclass UserConcreteObjectStoreModel (keeping SelectableObjectStore types compatible), and include the 302 responses for the display endpoints. --- .../packages/api-client/src/schema/schema.ts | 19 +++++++++++++++++++ lib/galaxy/objectstore/__init__.py | 17 ++++++++++++++--- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/client/packages/api-client/src/schema/schema.ts b/client/packages/api-client/src/schema/schema.ts index 5df50293e03..90604b66b68 100644 --- a/client/packages/api-client/src/schema/schema.ts +++ b/client/packages/api-client/src/schema/schema.ts @@ -25306,6 +25306,11 @@ export interface components { description?: string | null; /** Device */ device?: string | null; + /** + * Enable Direct Download + * @default false + */ + enable_direct_download: boolean; /** Hidden */ hidden: boolean; /** Name */ @@ -33755,6 +33760,13 @@ export interface operations { }; content?: never; }; + /** @description Redirect to a URL serving the dataset directly from the backing object store. Only returned for whole-file downloads when the dataset's object store has `enable_direct_download` set. */ + 302: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; /** @description Request Error */ "4XX": { headers: { @@ -38900,6 +38912,13 @@ export interface operations { }; content?: never; }; + /** @description Redirect to a URL serving the dataset directly from the backing object store. Only returned for whole-file downloads when the dataset's object store has `enable_direct_download` set. */ + 302: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; /** @description Request Error */ "4XX": { headers: { diff --git a/lib/galaxy/objectstore/__init__.py b/lib/galaxy/objectstore/__init__.py index 0135465cf96..29c64882858 100644 --- a/lib/galaxy/objectstore/__init__.py +++ b/lib/galaxy/objectstore/__init__.py @@ -827,7 +827,11 @@ class ConcreteObjectStore(BaseObjectStore): def _get_direct_download_url(self, obj, content_disposition=None, content_type=None) -> Optional[str]: if not self.enable_direct_download: return None - return self._get_object_url(obj, content_disposition=content_disposition, content_type=content_type) + # _get_object_url is resolved via dynamic dispatch on each concrete backend; it is not + # declared on ConcreteObjectStore so static analysis can't see it here. + return self._get_object_url( # type: ignore[attr-defined] + obj, content_disposition=content_disposition, content_type=content_type + ) def _get_concrete_store_badges(self, obj) -> list[BadgeDict]: return serialize_badges( @@ -1297,9 +1301,16 @@ class NestedObjectStore(BaseObjectStore): """For the first backend that has this `obj`, get its URL.""" return self._call_method("_get_object_url", obj, None, False, **kwargs) - def _get_direct_download_url(self, obj, **kwargs) -> Optional[str]: + def _get_direct_download_url(self, obj, content_disposition=None, content_type=None) -> Optional[str]: """For the first backend that has this `obj`, get its direct download URL.""" - return self._call_method("_get_direct_download_url", obj, None, False, **kwargs) + return self._call_method( + "_get_direct_download_url", + obj, + None, + False, + content_disposition=content_disposition, + content_type=content_type, + ) def _get_concrete_store_name(self, obj): return self._call_method("_get_concrete_store_name", obj, None, False) From 3f06fcf4ca781acb700f6070c04be7c68048b0e6 Mon Sep 17 00:00:00 2001 From: Nuwan Goonasekera <2070605+nuwang@users.noreply.github.com> Date: Sun, 14 Jun 2026 14:52:16 +0530 Subject: [PATCH 03/10] Make enable_direct_download model field optional Rendering it as a required boolean forced every client object-store test fixture and mock to supply the field. Make ConcreteObjectStoreModel's enable_direct_download Optional so it is generated as an optional property (boolean | null) on both the model and its UserConcreteObjectStoreModel subclass; to_model still reports the real boolean value. --- client/packages/api-client/src/schema/schema.ts | 14 ++++---------- lib/galaxy/objectstore/__init__.py | 2 +- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/client/packages/api-client/src/schema/schema.ts b/client/packages/api-client/src/schema/schema.ts index 90604b66b68..740c8bdc1ab 100644 --- a/client/packages/api-client/src/schema/schema.ts +++ b/client/packages/api-client/src/schema/schema.ts @@ -9126,11 +9126,8 @@ export interface components { description?: string | null; /** Device */ device?: string | null; - /** - * Enable Direct Download - * @default false - */ - enable_direct_download: boolean; + /** Enable Direct Download */ + enable_direct_download?: boolean | null; /** Name */ name?: string | null; /** Object Expires After Days */ @@ -25306,11 +25303,8 @@ export interface components { description?: string | null; /** Device */ device?: string | null; - /** - * Enable Direct Download - * @default false - */ - enable_direct_download: boolean; + /** Enable Direct Download */ + enable_direct_download?: boolean | null; /** Hidden */ hidden: boolean; /** Name */ diff --git a/lib/galaxy/objectstore/__init__.py b/lib/galaxy/objectstore/__init__.py index 29c64882858..b8160fdacf6 100644 --- a/lib/galaxy/objectstore/__init__.py +++ b/lib/galaxy/objectstore/__init__.py @@ -1836,7 +1836,7 @@ class ConcreteObjectStoreModel(BaseModel): quota: QuotaModel badges: list[BadgeDict] device: str | None = None - enable_direct_download: bool = False + enable_direct_download: bool | None = None object_expires_after_days: int | None = None From 6779e15e6e96ec71b98cdc5327cdaf5cb36b7444 Mon Sep 17 00:00:00 2001 From: Nuwan Goonasekera <2070605+nuwang@users.noreply.github.com> Date: Sun, 14 Jun 2026 15:20:29 +0530 Subject: [PATCH 04/10] Fix remaining mypy errors from full lib+test type check - annotate the headers dict in DatasetsService.display - declare container_name on the tee-stream integration test class --- lib/galaxy/webapps/galaxy/services/datasets.py | 2 +- test/integration/objectstore/test_direct_download_redirect.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/webapps/galaxy/services/datasets.py b/lib/galaxy/webapps/galaxy/services/datasets.py index 444756f169b..a1fa8f203a5 100644 --- a/lib/galaxy/webapps/galaxy/services/datasets.py +++ b/lib/galaxy/webapps/galaxy/services/datasets.py @@ -690,7 +690,7 @@ class DatasetsService(ServiceBase, UsesVisualizationMixin): some point in the future without warning. Generally, data should be processed by its datatype prior to display (the default if raw is unspecified or explicitly false. """ - headers = {} + headers: dict[str, str] = {} rval: Any = "" try: dataset_manager = self.dataset_manager_by_type[hda_ldda] diff --git a/test/integration/objectstore/test_direct_download_redirect.py b/test/integration/objectstore/test_direct_download_redirect.py index b6462ed49d1..cffaa6b8dec 100644 --- a/test/integration/objectstore/test_direct_download_redirect.py +++ b/test/integration/objectstore/test_direct_download_redirect.py @@ -37,6 +37,7 @@ BOTO3_DIRECT_DOWNLOAD_CONFIG = string.Template(""" @integration_util.skip_unless_docker() class TestDirectDownloadRedirectIntegration(BaseObjectStoreIntegrationTestCase): + container_name: str object_store_cache_path: str @classmethod From 8324eb268f3b84cce68018d77145dd3ab40107a2 Mon Sep 17 00:00:00 2001 From: Nuwan Goonasekera <2070605+nuwang@users.noreply.github.com> Date: Mon, 15 Jun 2026 11:52:27 +0530 Subject: [PATCH 05/10] Serve download redirects from a dedicated /download route Redirecting /display to a presigned object-store URL was a non-obvious breaking change: curl does not follow redirects by default and requests does not follow them for HEAD, and it only happened for certain object stores. Instead, leave /display untouched (it always streams through Galaxy) and add an explicit /download route whose contract is that clients must follow a 302. - New GET/HEAD /api/datasets/{id}/download and /api/histories/{history_id}/contents/{id}/download. GET redirects (302) to a presigned URL when the backend supports it, otherwise streams the whole file (zipping composite datatypes) exactly as before. HEAD answers from object-store metadata (size, filename) without redirecting or pulling into cache, so HEAD clients still get the size. - Revert the redirect from /display; drop the DirectDownloadUrl sentinel. - Point the web UI download actions and the HDA download_url field at the new route. (bioblend will move to it in a follow-up in its own repo.) Refs galaxyproject/galaxy#19255 --- .../packages/api-client/src/schema/schema.ts | 262 +++++++++++++++++- .../src/components/Dataset/DatasetDisplay.vue | 4 +- client/src/components/Dataset/DatasetView.vue | 2 +- .../Content/Dataset/DatasetActions.vue | 2 +- .../Content/Dataset/DatasetDownload.test.ts | 4 +- .../Content/Dataset/DatasetDownload.vue | 2 +- lib/galaxy/managers/hdas.py | 2 +- lib/galaxy/webapps/galaxy/api/datasets.py | 63 ++++- .../webapps/galaxy/services/datasets.py | 57 +++- .../test_direct_download_redirect.py | 44 ++- 10 files changed, 394 insertions(+), 48 deletions(-) diff --git a/client/packages/api-client/src/schema/schema.ts b/client/packages/api-client/src/schema/schema.ts index 740c8bdc1ab..4d43cdad53a 100644 --- a/client/packages/api-client/src/schema/schema.ts +++ b/client/packages/api-client/src/schema/schema.ts @@ -983,6 +983,30 @@ export interface paths { patch?: never; trace?: never; }; + "/api/datasets/{history_content_id}/download": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + /** + * Downloads the dataset, redirecting to the object store when possible. + * @description Downloads the whole dataset file. Clients must follow the 302 redirect this route may return. + */ + get: operations["download_api_datasets__history_content_id__download_get"]; + put?: never; + post?: never; + delete?: never; + options?: never; + /** + * Returns download metadata (size, filename) for the dataset. + * @description Downloads the whole dataset file. Clients must follow the 302 redirect this route may return. + */ + head: operations["download_api_datasets__history_content_id__download_head"]; + patch?: never; + trace?: never; + }; "/api/datasets/{history_content_id}/metadata_file": { parameters: { query?: never; @@ -2529,6 +2553,30 @@ export interface paths { patch?: never; trace?: never; }; + "/api/histories/{history_id}/contents/{history_content_id}/download": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + /** + * Downloads the dataset, redirecting to the object store when possible. + * @description Downloads the whole dataset file. Clients must follow the 302 redirect this route may return. + */ + get: operations["history_contents_download_api_histories__history_id__contents__history_content_id__download_get"]; + put?: never; + post?: never; + delete?: never; + options?: never; + /** + * Returns download metadata (size, filename) for the dataset. + * @description Downloads the whole dataset file. Clients must follow the 302 redirect this route may return. + */ + head: operations["history_contents_download_api_histories__history_id__contents__history_content_id__download_head"]; + patch?: never; + trace?: never; + }; "/api/histories/{history_id}/contents/{history_content_id}/extra_files": { parameters: { query?: never; @@ -33754,13 +33802,6 @@ export interface operations { }; content?: never; }; - /** @description Redirect to a URL serving the dataset directly from the backing object store. Only returned for whole-file downloads when the dataset's object store has `enable_direct_download` set. */ - 302: { - headers: { - [name: string]: unknown; - }; - content?: never; - }; /** @description Request Error */ "4XX": { headers: { @@ -33838,6 +33879,105 @@ export interface operations { }; }; }; + download_api_datasets__history_content_id__download_get: { + parameters: { + query?: { + /** @description The file extension when downloading the display data. Use the value `data` to let the server infer it from the data type. */ + to_ext?: string | null; + }; + header?: { + /** @description The user ID that will be used to effectively make this API call. Only admins and designated users can make API calls on behalf of other users. */ + "run-as"?: string | null; + }; + path: { + /** @description The ID of the History Dataset. */ + history_content_id: string; + }; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description Successful Response */ + 200: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; + /** @description Redirect to a URL serving the dataset directly from the backing object store. Only returned for whole-file downloads when the dataset's object store has `enable_direct_download` set. */ + 302: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; + /** @description Request Error */ + "4XX": { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["MessageExceptionModel"]; + }; + }; + /** @description Server Error */ + "5XX": { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["MessageExceptionModel"]; + }; + }; + }; + }; + download_api_datasets__history_content_id__download_head: { + parameters: { + query?: { + /** @description The file extension when downloading the display data. Use the value `data` to let the server infer it from the data type. */ + to_ext?: string | null; + }; + header?: { + /** @description The user ID that will be used to effectively make this API call. Only admins and designated users can make API calls on behalf of other users. */ + "run-as"?: string | null; + }; + path: { + /** @description The ID of the History Dataset. */ + history_content_id: string; + }; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description Successful Response */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": unknown; + }; + }; + /** @description Request Error */ + "4XX": { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["MessageExceptionModel"]; + }; + }; + /** @description Server Error */ + "5XX": { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["MessageExceptionModel"]; + }; + }; + }; + }; datasets__get_metadata_file: { parameters: { query: { @@ -38906,13 +39046,6 @@ export interface operations { }; content?: never; }; - /** @description Redirect to a URL serving the dataset directly from the backing object store. Only returned for whole-file downloads when the dataset's object store has `enable_direct_download` set. */ - 302: { - headers: { - [name: string]: unknown; - }; - content?: never; - }; /** @description Request Error */ "4XX": { headers: { @@ -38991,6 +39124,107 @@ export interface operations { }; }; }; + history_contents_download_api_histories__history_id__contents__history_content_id__download_get: { + parameters: { + query?: { + /** @description The file extension when downloading the display data. Use the value `data` to let the server infer it from the data type. */ + to_ext?: string | null; + }; + header?: { + /** @description The user ID that will be used to effectively make this API call. Only admins and designated users can make API calls on behalf of other users. */ + "run-as"?: string | null; + }; + path: { + /** @description The ID of the History Dataset. */ + history_content_id: string; + history_id: string | null; + }; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description Successful Response */ + 200: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; + /** @description Redirect to a URL serving the dataset directly from the backing object store. Only returned for whole-file downloads when the dataset's object store has `enable_direct_download` set. */ + 302: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; + /** @description Request Error */ + "4XX": { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["MessageExceptionModel"]; + }; + }; + /** @description Server Error */ + "5XX": { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["MessageExceptionModel"]; + }; + }; + }; + }; + history_contents_download_api_histories__history_id__contents__history_content_id__download_head: { + parameters: { + query?: { + /** @description The file extension when downloading the display data. Use the value `data` to let the server infer it from the data type. */ + to_ext?: string | null; + }; + header?: { + /** @description The user ID that will be used to effectively make this API call. Only admins and designated users can make API calls on behalf of other users. */ + "run-as"?: string | null; + }; + path: { + /** @description The ID of the History Dataset. */ + history_content_id: string; + history_id: string | null; + }; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description Successful Response */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": unknown; + }; + }; + /** @description Request Error */ + "4XX": { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["MessageExceptionModel"]; + }; + }; + /** @description Server Error */ + "5XX": { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["MessageExceptionModel"]; + }; + }; + }; + }; extra_files_history_api_histories__history_id__contents__history_content_id__extra_files_get: { parameters: { query?: never; diff --git a/client/src/components/Dataset/DatasetDisplay.vue b/client/src/components/Dataset/DatasetDisplay.vue index 566b2562459..b153420dcf8 100644 --- a/client/src/components/Dataset/DatasetDisplay.vue +++ b/client/src/components/Dataset/DatasetDisplay.vue @@ -38,7 +38,9 @@ const { isAdmin } = storeToRefs(useUserStore()); const dataset = computed(() => getDataset(props.datasetId)); const datasetUrl = computed(() => `/datasets/${props.datasetId}/display/`); -const downloadUrl = computed(() => withPrefix(`${datasetUrl.value}?to_ext=${dataset.value?.file_ext}`)); +const downloadUrl = computed(() => + withPrefix(`/api/datasets/${props.datasetId}/download?to_ext=${dataset.value?.file_ext}`), +); const isLoading = computed(() => isLoadingDataset(props.datasetId)); const previewUrl = computed(() => `${datasetUrl.value}?preview=True`); diff --git a/client/src/components/Dataset/DatasetView.vue b/client/src/components/Dataset/DatasetView.vue index ae640df68ef..624cc973d21 100644 --- a/client/src/components/Dataset/DatasetView.vue +++ b/client/src/components/Dataset/DatasetView.vue @@ -43,7 +43,7 @@ const iframeLoading = ref(true); const dataset = computed(() => datasetStore.getDataset(props.datasetId)); const loadError = computed(() => datasetStore.getDatasetError(props.datasetId)); -const downloadUrl = computed(() => withPrefix(`/datasets/${props.datasetId}/display`)); +const downloadUrl = computed(() => withPrefix(`/api/datasets/${props.datasetId}/download`)); const headerState = computed(() => (headerCollapsed.value ? "closed" : "open")); // Track datatype loading state diff --git a/client/src/components/History/Content/Dataset/DatasetActions.vue b/client/src/components/History/Content/Dataset/DatasetActions.vue index 5316771f614..b5526b8387b 100644 --- a/client/src/components/History/Content/Dataset/DatasetActions.vue +++ b/client/src/components/History/Content/Dataset/DatasetActions.vue @@ -58,7 +58,7 @@ const rerunUrl = computed(() => { return prependPath(props.itemUrls.rerun!); }); const downloadUrl = computed(() => { - return prependPath(`api/datasets/${props.item.id}/display?to_ext=${props.item.extension}`); + return prependPath(`api/datasets/${props.item.id}/download?to_ext=${props.item.extension}`); }); function onCopyLink() { diff --git a/client/src/components/History/Content/Dataset/DatasetDownload.test.ts b/client/src/components/History/Content/Dataset/DatasetDownload.test.ts index 4b2ad4bd579..ad01e545215 100644 --- a/client/src/components/History/Content/Dataset/DatasetDownload.test.ts +++ b/client/src/components/History/Content/Dataset/DatasetDownload.test.ts @@ -38,9 +38,9 @@ describe("DatasetDownload", () => { expect(foundItems).toBe(false); await wrapper.trigger("click"); const emitted = wrapper.emitted()["on-download"]; - expect(emitted?.[0]?.[0]).toBe(`/api/datasets/item_id/display?to_ext=ext`); + expect(emitted?.[0]?.[0]).toBe(`/api/datasets/item_id/download?to_ext=ext`); expect(emitted?.[1]?.[0]).toBe(`/api/datasets/item_id/metadata_file?metadata_file=a`); expect(emitted?.[2]?.[0]).toBe(`/api/datasets/item_id/metadata_file?metadata_file=b`); - expect(emitted?.[3]?.[0]).toBe(`/api/datasets/item_id/display?to_ext=ext`); + expect(emitted?.[3]?.[0]).toBe(`/api/datasets/item_id/download?to_ext=ext`); }); }); diff --git a/client/src/components/History/Content/Dataset/DatasetDownload.vue b/client/src/components/History/Content/Dataset/DatasetDownload.vue index ffa4cdaf132..4ae94776374 100644 --- a/client/src/components/History/Content/Dataset/DatasetDownload.vue +++ b/client/src/components/History/Content/Dataset/DatasetDownload.vue @@ -26,7 +26,7 @@ const metaDownloadUrl = computed(() => { return prependPath(`api/datasets/${props.item.id}/metadata_file?metadata_file=`); }); const downloadUrl = computed(() => { - return prependPath(`api/datasets/${props.item.id}/display?to_ext=${props.item.extension}`); + return prependPath(`api/datasets/${props.item.id}/download?to_ext=${props.item.extension}`); }); const downloadTitle = computed(() => { const size = props.item.file_size; diff --git a/lib/galaxy/managers/hdas.py b/lib/galaxy/managers/hdas.py index deb52357270..44189e6d487 100644 --- a/lib/galaxy/managers/hdas.py +++ b/lib/galaxy/managers/hdas.py @@ -628,7 +628,7 @@ class HDASerializer( # datasets._UnflattenedMetadataDatasetAssociationSerialize ), # TODO: backwards compat: need to go away "download_url": lambda item, key, **context: self.url_for( - "history_contents_display", + "history_contents_download", history_id=self.app.security.encode_id(item.history.id), history_content_id=self.app.security.encode_id(item.id), context=context, diff --git a/lib/galaxy/webapps/galaxy/api/datasets.py b/lib/galaxy/webapps/galaxy/api/datasets.py index 37b69ac91cb..1cd0a3c427a 100644 --- a/lib/galaxy/webapps/galaxy/api/datasets.py +++ b/lib/galaxy/webapps/galaxy/api/datasets.py @@ -70,7 +70,6 @@ from galaxy.webapps.galaxy.services.datasets import ( DatasetTextContentDetails, DeleteDatasetBatchPayload, DeleteDatasetBatchResult, - DirectDownloadUrl, RequestDataType, UpdateObjectStoreIdPayload, ) @@ -310,7 +309,6 @@ class FastAPIDatasets: summary="Displays (preview) or downloads dataset content.", tags=["histories"], response_class=StreamingResponse, - responses={302: DIRECT_DOWNLOAD_REDIRECT_RESPONSE}, ) @router.head( "/api/histories/{history_id}/contents/{history_content_id}/display", @@ -338,7 +336,6 @@ class FastAPIDatasets: "/api/datasets/{history_content_id}/display", summary="Displays (preview) or downloads dataset content.", response_class=StreamingResponse, - responses={302: DIRECT_DOWNLOAD_REDIRECT_RESPONSE}, ) @router.head( "/api/datasets/{history_content_id}/display", @@ -359,6 +356,64 @@ class FastAPIDatasets: """Streams the dataset for download or the contents preview to be displayed in a browser.""" return self._display(request, trans, history_content_id, preview, filename, to_ext, raw, offset, ck_size) + @router.get( + "/api/histories/{history_id}/contents/{history_content_id}/download", + name="history_contents_download", + summary="Downloads the dataset, redirecting to the object store when possible.", + tags=["histories"], + response_class=StreamingResponse, + responses={302: DIRECT_DOWNLOAD_REDIRECT_RESPONSE}, + ) + @router.head( + "/api/histories/{history_id}/contents/{history_content_id}/download", + name="history_contents_download", + summary="Returns download metadata (size, filename) for the dataset.", + tags=["histories"], + ) + def download_history_content( + self, + request: Request, + history_content_id: HistoryDatasetIDPathParam, + history_id: Optional[HistoryIDPathParam] = None, + trans=DependsOnTrans, + to_ext: Optional[str] = ToExtQueryParam, + ): + """Downloads the whole dataset file. Clients must follow the 302 redirect this route may return.""" + return self._download(request, trans, history_content_id, to_ext) + + @router.get( + "/api/datasets/{history_content_id}/download", + summary="Downloads the dataset, redirecting to the object store when possible.", + response_class=StreamingResponse, + responses={302: DIRECT_DOWNLOAD_REDIRECT_RESPONSE}, + ) + @router.head( + "/api/datasets/{history_content_id}/download", + summary="Returns download metadata (size, filename) for the dataset.", + ) + def download( + self, + request: Request, + history_content_id: HistoryDatasetIDPathParam, + trans=DependsOnTrans, + to_ext: Optional[str] = ToExtQueryParam, + ): + """Downloads the whole dataset file. Clients must follow the 302 redirect this route may return.""" + return self._download(request, trans, history_content_id, to_ext) + + def _download(self, request: Request, trans, dataset_id: DecodedDatabaseIdField, to_ext: Optional[str]): + # Default to_ext to "data" so the route always behaves as a whole-file download (server infers + # the extension from the datatype) rather than a preview. + to_ext = to_ext or "data" + if request.method == "HEAD": + headers = self.service.download_head_headers(trans, dataset_id, to_ext) + return Response(status_code=200, headers=headers) + url = self.service.direct_download_url(trans, dataset_id, to_ext) + if url is not None: + return RedirectResponse(url, status_code=302) + # No direct URL available: stream the file through Galaxy, exactly as a /display download would. + return self._display(request, trans, dataset_id, preview=False, filename=None, to_ext=to_ext, raw=False) + def _display( self, request: Request, @@ -385,8 +440,6 @@ class FastAPIDatasets: ck_size=ck_size, **extra_params, ) - if isinstance(display_data, DirectDownloadUrl): - return RedirectResponse(display_data.url, status_code=302, headers=headers) if isinstance(display_data, IOBase): file_name = getattr(display_data, "name", None) if file_name: diff --git a/lib/galaxy/webapps/galaxy/services/datasets.py b/lib/galaxy/webapps/galaxy/services/datasets.py index a1fa8f203a5..ea2011e4631 100644 --- a/lib/galaxy/webapps/galaxy/services/datasets.py +++ b/lib/galaxy/webapps/galaxy/services/datasets.py @@ -4,7 +4,6 @@ API operations on the contents of a history dataset. import logging import os -from dataclasses import dataclass from enum import Enum from typing import ( Any, @@ -95,13 +94,6 @@ log = logging.getLogger(__name__) DEFAULT_LIMIT = 500 -@dataclass(frozen=True) -class DirectDownloadUrl: - """Sentinel returned by `DatasetsService.display` to signal a redirect to a backing-store URL.""" - - url: str - - def is_direct_download_candidate(filename, to_ext, raw, offset, ck_size, is_archive) -> bool: """Whether a display request is a plain whole-file download eligible for a direct backing-store link. @@ -654,11 +646,23 @@ class DatasetsService(ServiceBase, UsesVisualizationMixin): return rval - def _direct_download_url(self, trans, dataset_instance, filename, to_ext, raw, offset, ck_size) -> Optional[str]: - """Return a backing-store URL the client can be redirected to, or None to stream as usual.""" + def direct_download_url( + self, + trans: ProvidesHistoryContext, + dataset_id: DecodedDatabaseIdField, + to_ext: Optional[str] = None, + hda_ldda: DatasetSourceType = DatasetSourceType.hda, + ) -> Optional[str]: + """Return a backing-store URL a whole-file download can be redirected to, or None to stream. + + Used by the dedicated download route; the regular display route never redirects. + """ + dataset_manager = self.dataset_manager_by_type[hda_ldda] + dataset_instance = dataset_manager.get_accessible(dataset_id, trans.user) + dataset_manager.ensure_dataset_on_disk(trans, dataset_instance) datatype = dataset_instance.datatype is_archive = datatype.is_archive_download(trans.app.datatypes_registry, dataset_instance.extension) - if not is_direct_download_candidate(filename, to_ext, raw, offset, ck_size, is_archive): + if not is_direct_download_candidate(None, to_ext, False, None, None, is_archive): return None content_disposition = None content_type = None @@ -670,6 +674,34 @@ class DatasetsService(ServiceBase, UsesVisualizationMixin): dataset_instance.dataset, content_disposition=content_disposition, content_type=content_type ) + def download_head_headers( + self, + trans: ProvidesHistoryContext, + dataset_id: DecodedDatabaseIdField, + to_ext: Optional[str] = None, + hda_ldda: DatasetSourceType = DatasetSourceType.hda, + ) -> dict[str, str]: + """Build response headers for a HEAD download request from object-store metadata. + + Answers without redirecting or pulling the object into cache, so clients (which may not follow + redirects on HEAD) can learn the size and filename of a download. + """ + dataset_manager = self.dataset_manager_by_type[hda_ldda] + dataset_instance = dataset_manager.get_accessible(dataset_id, trans.user) + dataset_manager.ensure_dataset_on_disk(trans, dataset_instance) + datatype = dataset_instance.datatype + headers = { + "content-type": "application/octet-stream", + "Content-Disposition": datatype.download_content_disposition(dataset_instance, to_ext), + "accept-ranges": "bytes", + } + # Composite/archived downloads are zipped on the fly, so their size is not known up front. + if not datatype.is_archive_download(trans.app.datatypes_registry, dataset_instance.extension): + size = trans.app.object_store.size(dataset_instance.dataset) + if size: + headers["Content-Length"] = str(size) + return headers + def display( self, trans: ProvidesHistoryContext, @@ -699,9 +731,6 @@ class DatasetsService(ServiceBase, UsesVisualizationMixin): if filename and filename.startswith("/"): # Path needs to relative to extra files path filename = filename.lstrip("/") - direct_url = self._direct_download_url(trans, dataset_instance, filename, to_ext, raw, offset, ck_size) - if direct_url is not None: - return DirectDownloadUrl(direct_url), headers if raw: if filename and filename != "index": object_store = trans.app.object_store diff --git a/test/integration/objectstore/test_direct_download_redirect.py b/test/integration/objectstore/test_direct_download_redirect.py index cffaa6b8dec..f09229b6a00 100644 --- a/test/integration/objectstore/test_direct_download_redirect.py +++ b/test/integration/objectstore/test_direct_download_redirect.py @@ -1,8 +1,9 @@ """Integration test for direct-download redirects (presigned URLs) from a remote object store. -Datasets stored in a backing object store with ``enable_direct_download`` set should be served via a -302 redirect to a URL the client fetches directly from the store, instead of being pulled through -Galaxy's cache. Uses a boto3 object store backed by a disposable minio container. +Whole-file downloads of datasets stored in a backing object store with ``enable_direct_download`` set +are served from the dedicated ``/download`` route via a 302 redirect to a URL the client fetches +directly from the store, instead of being pulled through Galaxy's cache. The ``/display`` route keeps +streaming through Galaxy (no redirect). Uses a boto3 object store backed by a disposable minio container. """ import os @@ -77,10 +78,13 @@ class TestDirectDownloadRedirectIntegration(BaseObjectStoreIntegrationTestCase): super().setUp() self.dataset_populator = DatasetPopulator(self.galaxy_interactor) + def _download_url(self, hda_id, **params): + return self._api_url(f"datasets/{hda_id}/download", params=params, use_key=True) + def _display_url(self, hda_id, **params): return self._api_url(f"datasets/{hda_id}/display", params=params, use_key=True) - def test_download_redirects_to_presigned_url(self): + def test_download_route_redirects_to_presigned_url(self): history_id = self.dataset_populator.new_history() hda = self.dataset_populator.new_dataset(history_id, content="123", wait=True) @@ -88,7 +92,7 @@ class TestDirectDownloadRedirectIntegration(BaseObjectStoreIntegrationTestCase): self._reset_cache() assert files_count(self.object_store_cache_path) == 0 - url = self._display_url(hda["id"], to_ext="txt") + url = self._download_url(hda["id"], to_ext="txt") response = requests.get(url, allow_redirects=False) assert response.status_code == 302 location = response.headers["Location"] @@ -104,15 +108,39 @@ class TestDirectDownloadRedirectIntegration(BaseObjectStoreIntegrationTestCase): # Galaxy served the download without pulling the object into its cache. assert files_count(self.object_store_cache_path) == 0 - def test_raw_download_redirects(self): + def test_download_route_redirects_with_inferred_extension(self): history_id = self.dataset_populator.new_history() - hda = self.dataset_populator.new_dataset(history_id, content="raw-bytes", wait=True) + hda = self.dataset_populator.new_dataset(history_id, content="123", wait=True) - url = self._display_url(hda["id"], raw="True") + # No to_ext: the download route infers it and still redirects for a single-file dataset. + url = self._download_url(hda["id"]) response = requests.get(url, allow_redirects=False) assert response.status_code == 302 assert OBJECT_STORE_HOST in response.headers["Location"] + def test_head_download_returns_metadata_without_redirect(self): + history_id = self.dataset_populator.new_history() + hda = self.dataset_populator.new_dataset(history_id, content="123", wait=True) + + self._reset_cache() + url = self._download_url(hda["id"], to_ext="txt") + response = requests.head(url, allow_redirects=False) + # HEAD answers from object-store metadata: 200, real size, no redirect, no cache pull. + assert response.status_code == 200 + assert "Location" not in response.headers + assert response.headers["Content-Length"] == str(len(b"123\n")) + assert files_count(self.object_store_cache_path) == 0 + + def test_display_does_not_redirect(self): + history_id = self.dataset_populator.new_history() + hda = self.dataset_populator.new_dataset(history_id, content="display-me", wait=True) + + # The legacy /display route is unchanged: it streams through Galaxy, never redirects. + url = self._display_url(hda["id"], to_ext="txt") + response = requests.get(url, allow_redirects=False) + assert response.status_code == 200 + assert "display-me" in response.text + def test_preview_is_not_redirected(self): history_id = self.dataset_populator.new_history() hda = self.dataset_populator.new_dataset(history_id, content="hello", wait=True) From 3ad2edff2627b4e618c9799beed7c5202b931094 Mon Sep 17 00:00:00 2001 From: Nuwan Goonasekera <2070605+nuwang@users.noreply.github.com> Date: Mon, 15 Jun 2026 16:39:56 +0530 Subject: [PATCH 06/10] Always return a 302 from the dataset /download route Make /download redirect for every GET, not only when an object-store presigned URL is available: when there is nothing to offload it now redirects to the streaming /display route instead of streaming directly. This gives clients a single, uniform contract (always follow the redirect) regardless of the backing object store, so a client developed against a disk-backed instance behaves the same against S3. HEAD still answers from object-store metadata with a 200 (no redirect), since clients such as requests do not follow redirects on HEAD. --- lib/galaxy/webapps/galaxy/api/datasets.py | 18 ++++++++++++++---- lib/galaxy_test/api/test_datasets.py | 16 ++++++++++++++++ 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/api/datasets.py b/lib/galaxy/webapps/galaxy/api/datasets.py index 1cd0a3c427a..9eff1b477ff 100644 --- a/lib/galaxy/webapps/galaxy/api/datasets.py +++ b/lib/galaxy/webapps/galaxy/api/datasets.py @@ -12,6 +12,7 @@ from typing import ( Annotated, cast, ) +from urllib.parse import urlencode from fastapi import ( Body, @@ -406,13 +407,22 @@ class FastAPIDatasets: # the extension from the datatype) rather than a preview. to_ext = to_ext or "data" if request.method == "HEAD": + # HEAD answers from object-store metadata without redirecting -- clients (e.g. requests) do + # not follow redirects on HEAD, so a 302 here would hide the size/filename from them. headers = self.service.download_head_headers(trans, dataset_id, to_ext) return Response(status_code=200, headers=headers) url = self.service.direct_download_url(trans, dataset_id, to_ext) - if url is not None: - return RedirectResponse(url, status_code=302) - # No direct URL available: stream the file through Galaxy, exactly as a /display download would. - return self._display(request, trans, dataset_id, preview=False, filename=None, to_ext=to_ext, raw=False) + if url is None: + # No object-store offload: redirect to the streaming display route. Every download is a 302 + # so clients implement redirect-following uniformly, regardless of the backing object store. + # A relative location resolves to the sibling /display route (same dataset, same auth/prefix). + query = {"to_ext": to_ext} + api_key = request.query_params.get("key") + if api_key: + # Preserve a query-string API key across the redirect (header/cookie auth carries itself). + query["key"] = api_key + url = f"display?{urlencode(query)}" + return RedirectResponse(url, status_code=302) def _display( self, diff --git a/lib/galaxy_test/api/test_datasets.py b/lib/galaxy_test/api/test_datasets.py index 579361d9f45..57e0ad23360 100644 --- a/lib/galaxy_test/api/test_datasets.py +++ b/lib/galaxy_test/api/test_datasets.py @@ -3,6 +3,8 @@ import zipfile from io import BytesIO from urllib.parse import quote +import requests + from galaxy.model.unittest_utils.store_fixtures import ( deferred_hda_model_store_dict, one_hda_model_store_dict, @@ -342,6 +344,20 @@ class TestDatasetsApi(ApiTestCase): self._assert_status_code_is(display_response, 200) assert display_response.text == contents + def test_download_always_redirects(self, history_id): + content = "download-me\n" + hda = self.dataset_populator.new_dataset(history_id, content=content, wait=True) + download_url = self._api_url(f"datasets/{hda['id']}/download", {"to_ext": "txt"}, use_key=True) + # The /download route always returns a 302 so every client follows redirects uniformly; for a + # disk object store (no presigned URL) it points back at the streaming /display route. + no_follow = requests.get(download_url, allow_redirects=False) + assert no_follow.status_code == 302 + assert "display" in no_follow.headers["location"] + # Following the redirect yields the dataset content. + followed = requests.get(download_url) + followed.raise_for_status() + assert "download-me" in followed.text + def test_display_preview_binary_as_text_uses_text_plain(self, history_id): # Regression test for https://github.com/galaxyproject/galaxy/issues/22395 # When previewing an unknown / binary dataset as text the response must use From b95cf7a7edb4e2a57ed924fbfe668060ad9d23a3 Mon Sep 17 00:00:00 2001 From: Nuwan Goonasekera <2070605+nuwang@users.noreply.github.com> Date: Mon, 15 Jun 2026 16:53:15 +0530 Subject: [PATCH 07/10] Build the download fallback redirect via url_builder Resolve the streaming /display route by name through trans.url_builder (the established pattern, e.g. group_roles) instead of hand-building a relative path. This produces a qualified, prefix-aware URL and is robust to route changes; query params (to_ext, and a carried-through API key) are encoded by the builder. --- lib/galaxy/webapps/galaxy/api/datasets.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/api/datasets.py b/lib/galaxy/webapps/galaxy/api/datasets.py index 9eff1b477ff..12f845fddf9 100644 --- a/lib/galaxy/webapps/galaxy/api/datasets.py +++ b/lib/galaxy/webapps/galaxy/api/datasets.py @@ -12,7 +12,6 @@ from typing import ( Annotated, cast, ) -from urllib.parse import urlencode from fastapi import ( Body, @@ -415,13 +414,17 @@ class FastAPIDatasets: if url is None: # No object-store offload: redirect to the streaming display route. Every download is a 302 # so clients implement redirect-following uniformly, regardless of the backing object store. - # A relative location resolves to the sibling /display route (same dataset, same auth/prefix). - query = {"to_ext": to_ext} + query_params = {"to_ext": to_ext} api_key = request.query_params.get("key") if api_key: # Preserve a query-string API key across the redirect (header/cookie auth carries itself). - query["key"] = api_key - url = f"display?{urlencode(query)}" + query_params["key"] = api_key + url = trans.url_builder( + "display", + history_content_id=trans.security.encode_id(dataset_id), + qualified=True, + query_params=query_params, + ) return RedirectResponse(url, status_code=302) def _display( From 42dfcdd5f5495321592256baf6c2d57c22b0a5f9 Mon Sep 17 00:00:00 2001 From: Nuwan Goonasekera <2070605+nuwang@users.noreply.github.com> Date: Mon, 15 Jun 2026 17:14:43 +0530 Subject: [PATCH 08/10] Drop redundant API-key carry-through on the download redirect Shipped clients authenticate the download with the x-api-key header (bioblend) or the session cookie (UI), both of which carry automatically across the same-origin redirect to /display, so re-attaching a query string key was unnecessary (and a key in a redirect Location is best avoided). The download route forwards only to_ext to /display -- other display params (preview, offset, ck_size) are intentionally not honored so the route always downloads the whole file. The test authenticates via the header like bioblend and asserts to_ext is preserved across the redirect and that auth survives it (200). --- lib/galaxy/webapps/galaxy/api/datasets.py | 8 ++------ lib/galaxy_test/api/test_datasets.py | 19 +++++++++++++------ 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/api/datasets.py b/lib/galaxy/webapps/galaxy/api/datasets.py index 12f845fddf9..a725150c378 100644 --- a/lib/galaxy/webapps/galaxy/api/datasets.py +++ b/lib/galaxy/webapps/galaxy/api/datasets.py @@ -414,16 +414,12 @@ class FastAPIDatasets: if url is None: # No object-store offload: redirect to the streaming display route. Every download is a 302 # so clients implement redirect-following uniformly, regardless of the backing object store. - query_params = {"to_ext": to_ext} - api_key = request.query_params.get("key") - if api_key: - # Preserve a query-string API key across the redirect (header/cookie auth carries itself). - query_params["key"] = api_key + # Auth (x-api-key header, session cookie) carries itself across this same-origin redirect. url = trans.url_builder( "display", history_content_id=trans.security.encode_id(dataset_id), qualified=True, - query_params=query_params, + query_params={"to_ext": to_ext}, ) return RedirectResponse(url, status_code=302) diff --git a/lib/galaxy_test/api/test_datasets.py b/lib/galaxy_test/api/test_datasets.py index 57e0ad23360..124e14cfc3b 100644 --- a/lib/galaxy_test/api/test_datasets.py +++ b/lib/galaxy_test/api/test_datasets.py @@ -347,15 +347,22 @@ class TestDatasetsApi(ApiTestCase): def test_download_always_redirects(self, history_id): content = "download-me\n" hda = self.dataset_populator.new_dataset(history_id, content=content, wait=True) - download_url = self._api_url(f"datasets/{hda['id']}/download", {"to_ext": "txt"}, use_key=True) + # Authenticate via the x-api-key header (as bioblend does); it carries across the redirect. + download_url = self._api_url(f"datasets/{hda['id']}/download", {"to_ext": "txt"}) + headers = {"x-api-key": self.galaxy_interactor.api_key} # The /download route always returns a 302 so every client follows redirects uniformly; for a # disk object store (no presigned URL) it points back at the streaming /display route. - no_follow = requests.get(download_url, allow_redirects=False) + no_follow = requests.get(download_url, headers=headers, allow_redirects=False) assert no_follow.status_code == 302 - assert "display" in no_follow.headers["location"] - # Following the redirect yields the dataset content. - followed = requests.get(download_url) - followed.raise_for_status() + location = no_follow.headers["location"] + assert "display" in location + # The to_ext query param is carried through to the streaming route by the redirect. + assert "to_ext=txt" in location + # The client resends the x-api-key header on this same-origin redirect, so /display + # authenticates and serves the data. A 200 here (with only the header, no cookie) is the + # proof that auth survived the redirect -- /display would return 401 otherwise. + followed = requests.get(download_url, headers=headers) + assert followed.status_code == 200 assert "download-me" in followed.text def test_display_preview_binary_as_text_uses_text_plain(self, history_id): From 05c726109cc94fc5688f4eb0c39cfa2b0b767a26 Mon Sep 17 00:00:00 2001 From: Nuwan Goonasekera <2070605+nuwang@users.noreply.github.com> Date: Wed, 8 Jul 2026 01:25:43 +0530 Subject: [PATCH 09/10] Use modern X | None syntax after dev migration --- lib/galaxy/webapps/galaxy/api/datasets.py | 8 ++++---- lib/galaxy/webapps/galaxy/services/datasets.py | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/api/datasets.py b/lib/galaxy/webapps/galaxy/api/datasets.py index a725150c378..74dc747c8b6 100644 --- a/lib/galaxy/webapps/galaxy/api/datasets.py +++ b/lib/galaxy/webapps/galaxy/api/datasets.py @@ -374,9 +374,9 @@ class FastAPIDatasets: self, request: Request, history_content_id: HistoryDatasetIDPathParam, - history_id: Optional[HistoryIDPathParam] = None, + history_id: HistoryIDPathParam | None = None, trans=DependsOnTrans, - to_ext: Optional[str] = ToExtQueryParam, + to_ext: str | None = ToExtQueryParam, ): """Downloads the whole dataset file. Clients must follow the 302 redirect this route may return.""" return self._download(request, trans, history_content_id, to_ext) @@ -396,12 +396,12 @@ class FastAPIDatasets: request: Request, history_content_id: HistoryDatasetIDPathParam, trans=DependsOnTrans, - to_ext: Optional[str] = ToExtQueryParam, + to_ext: str | None = ToExtQueryParam, ): """Downloads the whole dataset file. Clients must follow the 302 redirect this route may return.""" return self._download(request, trans, history_content_id, to_ext) - def _download(self, request: Request, trans, dataset_id: DecodedDatabaseIdField, to_ext: Optional[str]): + def _download(self, request: Request, trans, dataset_id: DecodedDatabaseIdField, to_ext: str | None): # Default to_ext to "data" so the route always behaves as a whole-file download (server infers # the extension from the datatype) rather than a preview. to_ext = to_ext or "data" diff --git a/lib/galaxy/webapps/galaxy/services/datasets.py b/lib/galaxy/webapps/galaxy/services/datasets.py index ea2011e4631..d4b1da43ad0 100644 --- a/lib/galaxy/webapps/galaxy/services/datasets.py +++ b/lib/galaxy/webapps/galaxy/services/datasets.py @@ -650,9 +650,9 @@ class DatasetsService(ServiceBase, UsesVisualizationMixin): self, trans: ProvidesHistoryContext, dataset_id: DecodedDatabaseIdField, - to_ext: Optional[str] = None, + to_ext: str | None = None, hda_ldda: DatasetSourceType = DatasetSourceType.hda, - ) -> Optional[str]: + ) -> str | None: """Return a backing-store URL a whole-file download can be redirected to, or None to stream. Used by the dedicated download route; the regular display route never redirects. @@ -678,7 +678,7 @@ class DatasetsService(ServiceBase, UsesVisualizationMixin): self, trans: ProvidesHistoryContext, dataset_id: DecodedDatabaseIdField, - to_ext: Optional[str] = None, + to_ext: str | None = None, hda_ldda: DatasetSourceType = DatasetSourceType.hda, ) -> dict[str, str]: """Build response headers for a HEAD download request from object-store metadata. From 439f5d25ea3541fdecc6cd2850638c15762baeb4 Mon Sep 17 00:00:00 2001 From: Nuwan Goonasekera <2070605+nuwang@users.noreply.github.com> Date: Wed, 8 Jul 2026 02:02:45 +0530 Subject: [PATCH 10/10] Modernize objectstore Optional annotations for ruff UP045 --- lib/galaxy/objectstore/__init__.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/galaxy/objectstore/__init__.py b/lib/galaxy/objectstore/__init__.py index b8160fdacf6..e3cdce0293c 100644 --- a/lib/galaxy/objectstore/__init__.py +++ b/lib/galaxy/objectstore/__init__.py @@ -321,8 +321,8 @@ class ObjectStore(metaclass=abc.ABCMeta): @abc.abstractmethod def get_direct_download_url( - self, obj, content_disposition: Optional[str] = None, content_type: Optional[str] = None - ) -> Optional[str]: + self, obj, content_disposition: str | None = None, content_type: str | None = None + ) -> str | None: """Return a URL a client can be redirected to in order to download `obj` directly from the backing store. Returns None unless the concrete store supports direct access *and* the admin has opted in via the @@ -687,13 +687,13 @@ class BaseObjectStore(ObjectStore): ) def get_direct_download_url( - self, obj, content_disposition: Optional[str] = None, content_type: Optional[str] = None - ) -> Optional[str]: + self, obj, content_disposition: str | None = None, content_type: str | None = None + ) -> str | None: return self._invoke( "get_direct_download_url", obj, content_disposition=content_disposition, content_type=content_type ) - def _get_direct_download_url(self, obj, content_disposition=None, content_type=None) -> Optional[str]: + def _get_direct_download_url(self, obj, content_disposition=None, content_type=None) -> str | None: # Stores that don't support direct download (or haven't opted in) get this no-op default. return None @@ -824,7 +824,7 @@ class ConcreteObjectStore(BaseObjectStore): object_expires_after_days=self.object_expires_after_days, ) - def _get_direct_download_url(self, obj, content_disposition=None, content_type=None) -> Optional[str]: + def _get_direct_download_url(self, obj, content_disposition=None, content_type=None) -> str | None: if not self.enable_direct_download: return None # _get_object_url is resolved via dynamic dispatch on each concrete backend; it is not @@ -1301,7 +1301,7 @@ class NestedObjectStore(BaseObjectStore): """For the first backend that has this `obj`, get its URL.""" return self._call_method("_get_object_url", obj, None, False, **kwargs) - def _get_direct_download_url(self, obj, content_disposition=None, content_type=None) -> Optional[str]: + def _get_direct_download_url(self, obj, content_disposition=None, content_type=None) -> str | None: """For the first backend that has this `obj`, get its direct download URL.""" return self._call_method( "_get_direct_download_url",