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)