From 7f78cc47f94729540b38c5be23464dac836c314a Mon Sep 17 00:00:00 2001 From: Arash Date: Tue, 13 Jan 2026 16:47:48 +0100 Subject: [PATCH 01/12] Fix credentials list failing to load when associated tools are missing --- .../webapps/galaxy/services/credentials.py | 34 ++++++++++++++----- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/services/credentials.py b/lib/galaxy/webapps/galaxy/services/credentials.py index 9eb53c56150..d0cacd95c4a 100644 --- a/lib/galaxy/webapps/galaxy/services/credentials.py +++ b/lib/galaxy/webapps/galaxy/services/credentials.py @@ -272,14 +272,30 @@ class CredentialsService: for user_credentials, credentials_group, credential in existing_user_credentials: cred_id = user_credentials.id - definition = self._get_credentials_definition( - user, - cast(SOURCE_TYPE, user_credentials.source_type), - user_credentials.source_id, - user_credentials.source_version, - user_credentials.name, - user_credentials.version, - ) + definition = None + try: + definition = self._get_credentials_definition( + user, + cast(SOURCE_TYPE, user_credentials.source_type), + user_credentials.source_id, + user_credentials.source_version, + user_credentials.name, + user_credentials.version, + ) + except ObjectNotFound: + # Tool was removed or is no longer available - create a minimal fallback definition + # using the stored credential data so the UI can still display the credentials + if include_definition: + definition = CredentialsRequirement( + name=user_credentials.name, + version=user_credentials.version, + description="[Tool definition not available]", + label="", + optional=False, + variables=[], + secrets=[], + ) + user_credentials_dict.setdefault( cred_id, { @@ -295,7 +311,7 @@ class CredentialsService: }, ) - if include_definition: + if include_definition and definition: user_credentials_dict[cred_id]["definition"] = { "name": definition.name, "version": definition.version, From 43c7ff46dc3fbc1abe25ac7feb6b17d81543579a Mon Sep 17 00:00:00 2001 From: Arash Date: Fri, 16 Jan 2026 12:51:16 +0100 Subject: [PATCH 02/12] Add test for listing credentials when associated tool is missing --- test/integration/test_credentials.py | 58 ++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/test/integration/test_credentials.py b/test/integration/test_credentials.py index 2b68630d832..a2bc52bc51d 100644 --- a/test/integration/test_credentials.py +++ b/test/integration/test_credentials.py @@ -458,6 +458,64 @@ class TestCredentialsApi(integration_util.IntegrationTestCase, integration_util. vault_ref = self._get_vault_ref(payload, group["id"], secret["name"]) self._check_vault_entry_exists(test_user_email, vault_ref, should_exist=False) + @skip_without_tool(CREDENTIALS_TEST_TOOL) + def test_list_credentials_with_missing_tool(self): + # Create credentials for the test tool + payload = self._build_credentials_payload() + self._provide_user_credentials(payload) + + # Verify credentials exist normally + credentials_list = self._check_credentials_exist() + assert len(credentials_list) == 1 + user_credentials_id = credentials_list[0]["id"] + + # Remove the tool from the toolbox to simulate it being unavailable + toolbox = self._app.toolbox + original_tool = toolbox._tools_by_id.pop(CREDENTIALS_TEST_TOOL) + + try: + # Test 1: List credentials with include_definition=True + response = self._get("/api/users/current/credentials?include_definition=true") + self._assert_status_code_is(response, 200) + credentials_with_definition = response.json() + + assert len(credentials_with_definition) == 1 + credential = credentials_with_definition[0] + + # Check that the credential still has basic information + assert credential["id"] == user_credentials_id + assert credential["source_id"] == CREDENTIALS_TEST_TOOL + assert credential["source_type"] == "tool" + + # Check that the fallback definition was provided + assert "definition" in credential + definition = credential["definition"] + assert definition["name"] == payload["service_credential"]["name"] + assert definition["version"] == payload["service_credential"]["version"] + assert definition["description"] == "[Tool definition not available]" + assert definition["label"] == "" + assert definition["optional"] is False + assert definition["variables"] == [] + assert definition["secrets"] == [] + + # Verify that groups are still present and accessible + assert len(credential["groups"]) > 0 + + # Test 2: List credentials without include_definition + response = self._get("/api/users/current/credentials") + self._assert_status_code_is(response, 200) + credentials_without_definition = response.json() + + assert len(credentials_without_definition) == 1 + credential_no_def = credentials_without_definition[0] + + # Should not have definition field when not requested + assert "definition" not in credential_no_def + assert credential_no_def["id"] == user_credentials_id + assert len(credential_no_def["groups"]) > 0 + finally: + toolbox._tools_by_id[CREDENTIALS_TEST_TOOL] = original_tool + def _provide_user_credentials(self, payload=None, status_code=200): payload = payload or self._build_credentials_payload() response = self._post("/api/users/current/credentials", data=payload, json=True) From c547dcc694d5c64c6b1fda9fac4df7d5bb5cfe6e Mon Sep 17 00:00:00 2001 From: Alireza Heidari Date: Fri, 16 Jan 2026 14:53:31 +0100 Subject: [PATCH 03/12] Use empty description for missing tool credentials fallback --- lib/galaxy/webapps/galaxy/services/credentials.py | 2 +- test/integration/test_credentials.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/services/credentials.py b/lib/galaxy/webapps/galaxy/services/credentials.py index d0cacd95c4a..31260985e1d 100644 --- a/lib/galaxy/webapps/galaxy/services/credentials.py +++ b/lib/galaxy/webapps/galaxy/services/credentials.py @@ -289,7 +289,7 @@ class CredentialsService: definition = CredentialsRequirement( name=user_credentials.name, version=user_credentials.version, - description="[Tool definition not available]", + description="", label="", optional=False, variables=[], diff --git a/test/integration/test_credentials.py b/test/integration/test_credentials.py index a2bc52bc51d..8a9161950d9 100644 --- a/test/integration/test_credentials.py +++ b/test/integration/test_credentials.py @@ -492,7 +492,7 @@ class TestCredentialsApi(integration_util.IntegrationTestCase, integration_util. definition = credential["definition"] assert definition["name"] == payload["service_credential"]["name"] assert definition["version"] == payload["service_credential"]["version"] - assert definition["description"] == "[Tool definition not available]" + assert definition["description"] == "" assert definition["label"] == "" assert definition["optional"] is False assert definition["variables"] == [] From e99661633cdef8b5853d5b158f47c6cf36922c2b Mon Sep 17 00:00:00 2001 From: Alireza Heidari Date: Fri, 16 Jan 2026 14:53:44 +0100 Subject: [PATCH 04/12] Add UI indicators for credentials with missing tools --- .../ServiceCredentialsGroupsList.vue | 72 +++++++++++++++---- 1 file changed, 57 insertions(+), 15 deletions(-) diff --git a/client/src/components/User/Credentials/ServiceCredentialsGroupsList.vue b/client/src/components/User/Credentials/ServiceCredentialsGroupsList.vue index f1f019c2e5e..08594b89550 100644 --- a/client/src/components/User/Credentials/ServiceCredentialsGroupsList.vue +++ b/client/src/components/User/Credentials/ServiceCredentialsGroupsList.vue @@ -18,7 +18,7 @@ * */ -import { faKey, faPencilAlt, faTrash, faWrench } from "@fortawesome/free-solid-svg-icons"; +import { faExclamationTriangle, faKey, faPencilAlt, faTrash, faWrench } from "@fortawesome/free-solid-svg-icons"; import { BModal } from "bootstrap-vue"; import { faCheck } from "font-awesome-6"; import { storeToRefs } from "pinia"; @@ -71,7 +71,7 @@ const props = defineProps(); const { confirm } = useConfirmDialog(); -const { getToolNameById } = useToolStore(); +const { getToolForId, getToolNameById } = useToolStore(); const userToolsServiceCredentialsStore = useUserToolsServiceCredentialsStore(); const { userToolsServicesCurrentGroupIds } = storeToRefs(userToolsServiceCredentialsStore); @@ -111,6 +111,27 @@ const isGroupInUse = computed(() => (group: ServiceCredentialsGroupDetails) => { return false; }); +/** + * Checks if the source tool for a credential group is missing/deleted. + * @param {ServiceCredentialsGroupDetails} group - The credential group to check. + * @returns {boolean} True if the tool is no longer available. + */ +const isToolMissing = computed(() => (group: ServiceCredentialsGroupDetails) => { + return !getToolForId(group.sourceId); +}); + +/** + * Gets the display name for a tool, with a fallback for missing/deleted tools. + * @param {ServiceCredentialsGroupDetails} group - The credential group. + * @returns {string} The tool name or a fallback indicator. + */ +const getToolDisplayName = computed(() => (group: ServiceCredentialsGroupDetails) => { + if (isToolMissing.value(group)) { + return `${group.sourceId} (deleted)`; + } + return getToolNameById(group.sourceId); +}); + /** * Deletes a credential group after user confirmation. * @param {ServiceCredentialsGroupDetails} groupToDelete - The group to delete. @@ -120,7 +141,9 @@ const isGroupInUse = computed(() => (group: ServiceCredentialsGroupDetails) => { async function deleteGroup(groupToDelete: ServiceCredentialsGroupDetails): Promise { let message = `Are you sure you want to delete the credentials group "${groupToDelete.name}"?`; - if (isGroupInUse.value(groupToDelete)) { + if (isToolMissing.value(groupToDelete)) { + message = message.concat(` The associated tool is no longer available.`); + } else if (isGroupInUse.value(groupToDelete)) { message = message.concat(` This group is currently in use by '${getToolNameById(groupToDelete.sourceId)}'.`); } @@ -221,23 +244,40 @@ async function onSaveChanges(): Promise { * @returns {CardBadge[]} Array of badge configurations. */ function getBadgesFor(group: ServiceCredentialsGroupDetails): CardBadge[] { - const badges: CardBadge[] = [ - { - id: `tool-${group.sourceId}`, - icon: faWrench, - title: "This tool is using this credentials group. Click to view.", - label: getToolNameById(group.sourceId), - to: `/root?tool_id=${group.sourceId}&tool_version=${group.sourceVersion}`, - }, - { + const toolMissing = isToolMissing.value(group); + const badges: CardBadge[] = []; + + if (toolMissing) { + badges.push({ + id: `tool-missing-${group.id}`, + icon: faExclamationTriangle, + title: "The tool associated with these credentials is no longer available. You cannot edit or use this group.", + label: "Tool Unavailable", + variant: "warning", + }); + } + + badges.push({ + id: `tool-${group.sourceId}`, + icon: faWrench, + title: toolMissing + ? "This tool is no longer available." + : "This tool is using this credentials group. Click to view.", + label: getToolDisplayName.value(group), + to: toolMissing ? undefined : `/root?tool_id=${group.sourceId}&tool_version=${group.sourceVersion}`, + }); + + if (!toolMissing) { + badges.push({ id: `in-use-${group.id}`, icon: faCheck, title: "This group is currently in use.", label: "In Use", variant: "success", visible: isGroupInUse.value(group), - }, - ]; + }); + } + return badges; } @@ -247,6 +287,7 @@ function getBadgesFor(group: ServiceCredentialsGroupDetails): CardBadge[] { * @returns {CardAction[]} Array of action configurations */ function getPrimaryActions(group: ServiceCredentialsGroupDetails): CardAction[] { + const toolMissing = isToolMissing.value(group); const primaryActions: CardAction[] = [ { id: `delete-${group.id}`, @@ -259,10 +300,11 @@ function getPrimaryActions(group: ServiceCredentialsGroupDetails): CardAction[] { id: `edit-${group.id}`, label: "Edit", - title: "Edit this group", + title: !toolMissing ? "Cannot edit - tool definition not available" : "Edit this group", icon: faPencilAlt, variant: "outline-info", handler: () => editGroup(group), + disabled: toolMissing, }, ]; return primaryActions; From f2d5d5854678239336c1c20c261f720d80ae577a Mon Sep 17 00:00:00 2001 From: Alireza Heidari Date: Fri, 16 Jan 2026 19:05:33 +0100 Subject: [PATCH 05/12] Not show group in use indicator when tool is missing --- .../ServiceCredentialsGroupsList.vue | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/client/src/components/User/Credentials/ServiceCredentialsGroupsList.vue b/client/src/components/User/Credentials/ServiceCredentialsGroupsList.vue index 08594b89550..d6d127afdd4 100644 --- a/client/src/components/User/Credentials/ServiceCredentialsGroupsList.vue +++ b/client/src/components/User/Credentials/ServiceCredentialsGroupsList.vue @@ -95,12 +95,25 @@ const cardTitle = computed(() => (group: ServiceCredentialsGroupDetails) => { return `${group.serviceDefinition.name} (v${group.serviceDefinition.version}) - ${group.name}`; }); +/** + * Checks if the source tool for a credential group is missing/deleted. + * @param {ServiceCredentialsGroupDetails} group - The credential group to check. + * @returns {boolean} True if the tool is no longer available. + */ +const isToolMissing = computed(() => (group: ServiceCredentialsGroupDetails) => { + return !getToolForId(group.sourceId); +}); + /** * Checks if a credential group is currently in use by any tool. * @param {ServiceCredentialsGroupDetails} group - The credential group to check. * @returns {boolean} True if the group is in use. */ const isGroupInUse = computed(() => (group: ServiceCredentialsGroupDetails) => { + if (isToolMissing.value(group)) { + return false; + } + const userToolKey = userToolsServiceCredentialsStore.getUserToolKey(group.sourceId, group.sourceVersion); const userToolService = userToolsServicesCurrentGroupIds.value[userToolKey]; for (const groupId of Object.values(userToolService || {})) { @@ -111,15 +124,6 @@ const isGroupInUse = computed(() => (group: ServiceCredentialsGroupDetails) => { return false; }); -/** - * Checks if the source tool for a credential group is missing/deleted. - * @param {ServiceCredentialsGroupDetails} group - The credential group to check. - * @returns {boolean} True if the tool is no longer available. - */ -const isToolMissing = computed(() => (group: ServiceCredentialsGroupDetails) => { - return !getToolForId(group.sourceId); -}); - /** * Gets the display name for a tool, with a fallback for missing/deleted tools. * @param {ServiceCredentialsGroupDetails} group - The credential group. From 521d256f04d15796eda6d8f7078d9d3f182d09de Mon Sep 17 00:00:00 2001 From: Arash Date: Mon, 19 Jan 2026 11:32:44 +0100 Subject: [PATCH 06/12] Mock toolbox.get_tool to simulate unavailable tool in credentials API tests --- test/integration/test_credentials.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/test/integration/test_credentials.py b/test/integration/test_credentials.py index 8a9161950d9..53110a9cbf5 100644 --- a/test/integration/test_credentials.py +++ b/test/integration/test_credentials.py @@ -1,4 +1,5 @@ from typing import Optional +from unittest.mock import patch from galaxy.model.db.user import get_user_by_email from galaxy.security.vault import UserVaultWrapper @@ -469,11 +470,8 @@ class TestCredentialsApi(integration_util.IntegrationTestCase, integration_util. assert len(credentials_list) == 1 user_credentials_id = credentials_list[0]["id"] - # Remove the tool from the toolbox to simulate it being unavailable - toolbox = self._app.toolbox - original_tool = toolbox._tools_by_id.pop(CREDENTIALS_TEST_TOOL) - - try: + # Mock the toolbox.get_tool method to simulate the tool being unavailable + with patch.object(self._app.toolbox, "get_tool", return_value=None): # Test 1: List credentials with include_definition=True response = self._get("/api/users/current/credentials?include_definition=true") self._assert_status_code_is(response, 200) @@ -513,8 +511,6 @@ class TestCredentialsApi(integration_util.IntegrationTestCase, integration_util. assert "definition" not in credential_no_def assert credential_no_def["id"] == user_credentials_id assert len(credential_no_def["groups"]) > 0 - finally: - toolbox._tools_by_id[CREDENTIALS_TEST_TOOL] = original_tool def _provide_user_credentials(self, payload=None, status_code=200): payload = payload or self._build_credentials_payload() From e7883ef1723c31bd11c674c566782e1773a36344 Mon Sep 17 00:00:00 2001 From: Arash Date: Mon, 19 Jan 2026 14:22:01 +0100 Subject: [PATCH 07/12] Refactor test for unavailable tool by simulating removal and restoration in credentials API tests --- test/integration/test_credentials.py | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/test/integration/test_credentials.py b/test/integration/test_credentials.py index 53110a9cbf5..d3aa13485cc 100644 --- a/test/integration/test_credentials.py +++ b/test/integration/test_credentials.py @@ -1,5 +1,4 @@ from typing import Optional -from unittest.mock import patch from galaxy.model.db.user import get_user_by_email from galaxy.security.vault import UserVaultWrapper @@ -470,8 +469,18 @@ class TestCredentialsApi(integration_util.IntegrationTestCase, integration_util. assert len(credentials_list) == 1 user_credentials_id = credentials_list[0]["id"] - # Mock the toolbox.get_tool method to simulate the tool being unavailable - with patch.object(self._app.toolbox, "get_tool", return_value=None): + # Save the tool reference before removing it + tool = self._app.toolbox.get_tool(CREDENTIALS_TEST_TOOL) + assert tool is not None, f"Tool {CREDENTIALS_TEST_TOOL} should be available before removal" + + try: + # Remove the tool to simulate it being unavailable + # Use remove_from_panel=False to keep restoration simple + self._app.toolbox.remove_tool_by_id(CREDENTIALS_TEST_TOOL, remove_from_panel=False) + + # Verify tool is actually removed + assert self._app.toolbox.get_tool(CREDENTIALS_TEST_TOOL) is None + # Test 1: List credentials with include_definition=True response = self._get("/api/users/current/credentials?include_definition=true") self._assert_status_code_is(response, 200) @@ -511,6 +520,10 @@ class TestCredentialsApi(integration_util.IntegrationTestCase, integration_util. assert "definition" not in credential_no_def assert credential_no_def["id"] == user_credentials_id assert len(credential_no_def["groups"]) > 0 + finally: + # Restore the tool to avoid affecting other tests + if tool is not None: + self._app.toolbox.register_tool(tool) def _provide_user_credentials(self, payload=None, status_code=200): payload = payload or self._build_credentials_payload() From ad8dc528ac673d4df886a505dcaa3e9256603a6a Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 20 Jan 2026 12:42:02 +0100 Subject: [PATCH 08/12] Allow optional token and public_name in RDM configs Updates RDM file source configuration fields to be optional, improving flexibility for cases where token or public_name may not be provided during initialization, like in public instances. --- lib/galaxy/files/sources/_rdm.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/galaxy/files/sources/_rdm.py b/lib/galaxy/files/sources/_rdm.py index 335f7299a15..6ed5c1d1b79 100644 --- a/lib/galaxy/files/sources/_rdm.py +++ b/lib/galaxy/files/sources/_rdm.py @@ -23,13 +23,13 @@ log = logging.getLogger(__name__) class RDMFileSourceTemplateConfiguration(BaseFileSourceTemplateConfiguration): - token: Union[str, TemplateExpansion] - public_name: Union[str, TemplateExpansion] + token: Optional[Union[str, TemplateExpansion]] = None + public_name: Optional[Union[str, TemplateExpansion]] = None class RDMFileSourceConfiguration(BaseFileSourceConfiguration): - token: str - public_name: str + token: Optional[str] = None + public_name: Optional[str] = None class ContainerAndFileIdentifier(NamedTuple): From 9b8849ed8c61304156604f2634d3dd6947881551 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 20 Jan 2026 12:42:58 +0100 Subject: [PATCH 09/12] Normalize repository URLs by removing trailing slash Ensures repository URLs are consistently formatted without a trailing slash to prevent potential issues with URL setup in configs. --- lib/galaxy/files/sources/_rdm.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/files/sources/_rdm.py b/lib/galaxy/files/sources/_rdm.py index 6ed5c1d1b79..2a1e3ae3542 100644 --- a/lib/galaxy/files/sources/_rdm.py +++ b/lib/galaxy/files/sources/_rdm.py @@ -51,7 +51,7 @@ class RDMRepositoryInteractor: """ def __init__(self, repository_url: str, plugin: "RDMFilesSource"): - self._repository_url = repository_url + self._repository_url = self._strip_last_slash(repository_url) self._plugin = plugin @property @@ -138,6 +138,12 @@ class RDMRepositoryInteractor: """ raise NotImplementedError() + def _strip_last_slash(self, url: str) -> str: + """Utility method to strip the last slash from a URL if present.""" + if url.endswith("/"): + return url[:-1] + return url + class RDMFilesSource(BaseFilesSource[RDMFileSourceTemplateConfiguration, RDMFileSourceConfiguration]): """Base class for Research Data Management (RDM) file sources. From 15319d1006c7d4e9938e5c4e809282a2f8cdb8d8 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 20 Jan 2026 12:54:52 +0100 Subject: [PATCH 10/12] Fix error handling to expose issues to the user Replaces generic exceptions with Galaxy exceptions to surface the issues to the user. Those exceptions should be safe to surface since they are proxied from the Dataverse instance. --- lib/galaxy/files/sources/dataverse.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/lib/galaxy/files/sources/dataverse.py b/lib/galaxy/files/sources/dataverse.py index 5154f685612..0e41cd455d8 100644 --- a/lib/galaxy/files/sources/dataverse.py +++ b/lib/galaxy/files/sources/dataverse.py @@ -13,6 +13,7 @@ from typing_extensions import TypedDict from galaxy.exceptions import ( AuthenticationRequired, + MessageException, ObjectNotFound, ) from galaxy.files.models import ( @@ -336,14 +337,14 @@ class DataverseRepositoryInteractor(RDMRepositoryInteractor): collection_payload = self._prepare_collection_data(title, public_name, user_email) collection = self._create_collection(":root", collection_payload, context) if not collection or "data" not in collection or "alias" not in collection["data"]: - raise Exception("Could not create collection in Dataverse or response has an unexpected format.") + raise MessageException("Could not create collection in Dataverse or response has an unexpected format.") collection_alias = collection["data"]["alias"] # Prepare and create the dataset dataset_payload = self._prepare_dataset_data(title, public_name, user_email) dataset = self._create_dataset(collection_alias, dataset_payload, context) if not dataset or "data" not in dataset: - raise Exception("Could not create dataset in Dataverse or response has an unexpected format.") + raise MessageException("Could not create dataset in Dataverse or response has an unexpected format.") dataset["data"]["name"] = title return dataset["data"] @@ -421,14 +422,18 @@ class DataverseRepositoryInteractor(RDMRepositoryInteractor): f"Authentication required to download file from '{download_file_content_url}'. " f"Please provide a valid API token in your user preferences." ) - # TODO: We can only download files from published datasets for now - if e.code in [403, 404]: + if e.code == 403: + # Permission denied: dataset may be unpublished or user lacks access rights + raise ObjectNotFound( + f"Access forbidden when downloading file from '{download_file_content_url}'. " + f"You may not have permission to access this file, or the dataset is not published." + ) + if e.code == 404: raise ObjectNotFound( f"File not found at '{download_file_content_url}'. " f"Please make sure the dataset and file exist and are published." ) - else: - raise + raise def _get_datasets_from_response(self, response: dict) -> list[RemoteDirectory]: rval: list[RemoteDirectory] = [] @@ -494,7 +499,7 @@ class DataverseRepositoryInteractor(RDMRepositoryInteractor): error_message = self._get_response_error_message(response) if response.status_code == 403: self._raise_auth_required(error_message) - raise Exception( + raise MessageException( f"Request to {response.url} failed with status code {response.status_code}: {error_message}" ) From e201d590d26c330ac7bb59d2dff5a09585ee3cc0 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Tue, 20 Jan 2026 12:55:36 +0100 Subject: [PATCH 11/12] Hardens Dataverse file ID parsing for nested PIDs Handles Dataverse file-level persistent identifiers that themselves contain URI schemes and slashes, ensuring correct resolution of files with independent persistent IDs. Enhances parsing logic to support more complex Dataverse path structures and improves robustness when extracting dataset and file identifiers. --- lib/galaxy/files/sources/dataverse.py | 41 +++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 3 deletions(-) diff --git a/lib/galaxy/files/sources/dataverse.py b/lib/galaxy/files/sources/dataverse.py index 0e41cd455d8..81c1ae710d2 100644 --- a/lib/galaxy/files/sources/dataverse.py +++ b/lib/galaxy/files/sources/dataverse.py @@ -106,6 +106,7 @@ class DataverseRDMFilesSource(RDMFilesSource): - doi:10.70122/FK2/AVNCLL (persistent ID) - doi:10.70122/FK2/DIG2DG/AVNCLL (persistent ID) - doi:10.70122/FK2/DIG2DG/id:12345 (database ID) + - doi:10.5072/FK2/doi:10.70122/AVNCLL (persistent ID) - perma:BSC/3ST00L/id:9056 (database ID) """ if not source_path.startswith("/"): @@ -126,8 +127,7 @@ class DataverseRDMFilesSource(RDMFilesSource): f"Invalid source path: '{source_path}'. Expected format: '//'." ) - file_id_part = parts[-1] - dataset_id = "/".join(parts[:-1]) + dataset_id, file_id_part = self._split_dataset_and_file_pid(parts) # The file identifier can be either: # - A persistent ID suffix (e.g., 'AVNCLL' -> full ID is 'doi:10.70122/FK2/DIG2DG/AVNCLL') @@ -135,11 +135,46 @@ class DataverseRDMFilesSource(RDMFilesSource): if file_id_part.startswith("id:"): # Database ID format - keep the 'id:' prefix as the file identifier file_id = file_id_part + elif re.match(r"^[a-zA-Z][a-zA-Z0-9+.-]*:.*", file_id_part): + # Full persistent identifier (e.g. doi:, hdl:, ark:, or custom PID providers). + # Files in Dataverse may have their own independent persistent IDs that are + # not hierarchically related to the dataset persistent ID. + file_id = file_id_part else: - # Persistent ID format - construct full persistent ID + # Dataset-scoped persistent ID suffix - construct full persistent ID file_id = f"{dataset_id}/{file_id_part}" return ContainerAndFileIdentifier(container_id=dataset_id, file_identifier=file_id) + @staticmethod + def _split_dataset_and_file_pid(parts: list[str]) -> tuple[str, str]: + """ + Split a Dataverse source path into dataset ID and file identifier parts. + + Dataverse file-level persistent IDs may themselves contain slashes and are not + necessarily hierarchically related to the dataset persistent ID. For example: + + /doi:10.57745/I8EUTL/doi:10.57745/L7SOAJ + + In this case: + dataset_id = doi:10.57745/I8EUTL + file_id = doi:10.57745/L7SOAJ + + This helper detects such cases by recognizing URI-scheme prefixes in path segments + and grouping them accordingly. + """ + # Default: last segment is the file identifier + file_id_part = parts[-1] + dataset_id = "/".join(parts[:-1]) + + # Heuristic: if the penultimate segment starts a URI scheme (e.g. doi:, hdl:, ark:), + # then the file persistent ID spans the last two segments. + pid_scheme_re = re.compile(r"^[a-zA-Z][a-zA-Z0-9+.-]*:") + if len(parts) >= 3 and pid_scheme_re.match(parts[-2]): + file_id_part = f"{parts[-2]}/{parts[-1]}" + dataset_id = "/".join(parts[:-2]) + + return dataset_id, file_id_part + def get_container_id_from_path(self, source_path: str) -> str: return self.parse_path(source_path, container_id_only=True).container_id From e46f9e789ca9463fbbe07ea4108ce3d4b50862a4 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Tue, 20 Jan 2026 14:11:32 +0100 Subject: [PATCH 12/12] Add database operation tool to convert sample sheets to list collections This tool converts sample sheet collections back to their corresponding non-sample-sheet types by stripping the column_definitions and row column metadata: - sample_sheet -> list - sample_sheet:paired -> list:paired - sample_sheet:paired_or_unpaired -> list:paired_or_unpaired - sample_sheet:record -> list:record Useful when a sample sheet needs to be passed to a tool that expects a regular list collection, or when discarding sample sheet metadata. --- lib/galaxy/config/sample/tool_conf.xml.sample | 1 + .../types/sample_sheet_workbook.py | 19 +++++ lib/galaxy/tools/__init__.py | 62 ++++++++++++++ lib/galaxy/tools/convert_sample_sheet.xml | 52 ++++++++++++ lib/galaxy_test/api/test_tools.py | 83 +++++++++++++++++++ lib/galaxy_test/base/populators.py | 41 +++++++++ test/functional/tools/sample_tool_conf.xml | 1 + 7 files changed, 259 insertions(+) create mode 100644 lib/galaxy/tools/convert_sample_sheet.xml diff --git a/lib/galaxy/config/sample/tool_conf.xml.sample b/lib/galaxy/config/sample/tool_conf.xml.sample index 9b5ae9134c7..d9446a41d93 100644 --- a/lib/galaxy/config/sample/tool_conf.xml.sample +++ b/lib/galaxy/config/sample/tool_conf.xml.sample @@ -53,6 +53,7 @@ + diff --git a/lib/galaxy/model/dataset_collections/types/sample_sheet_workbook.py b/lib/galaxy/model/dataset_collections/types/sample_sheet_workbook.py index 40043c00f40..395cdff3e68 100644 --- a/lib/galaxy/model/dataset_collections/types/sample_sheet_workbook.py +++ b/lib/galaxy/model/dataset_collections/types/sample_sheet_workbook.py @@ -618,5 +618,24 @@ def _list_to_sample_sheet_collection_type(input_collection_type: str) -> SampleS ) +def _sample_sheet_to_list_collection_type(input_collection_type: str) -> str: + """Convert sample_sheet collection types to corresponding list collection types. + + Converts sample_sheet types to list types (e.g., sample_sheet:paired -> list:paired). + """ + if input_collection_type == "sample_sheet": + return "list" + elif input_collection_type == "sample_sheet:paired": + return "list:paired" + elif input_collection_type == "sample_sheet:paired_or_unpaired": + return "list:paired_or_unpaired" + elif input_collection_type == "sample_sheet:record": + return "list:record" + else: + raise RequestParameterInvalidException( + f"Invalid collection type for sample sheet conversion: {input_collection_type}" + ) + + def _prefix_column_to_column_target(column_header: FetchPrefixColumn) -> ColumnTarget: return target_model_by_type(column_header.type) diff --git a/lib/galaxy/tools/__init__.py b/lib/galaxy/tools/__init__.py index 75ba1915045..b44e7cab443 100644 --- a/lib/galaxy/tools/__init__.py +++ b/lib/galaxy/tools/__init__.py @@ -57,6 +57,7 @@ from galaxy.model import ( StoredWorkflow, ) from galaxy.model.dataset_collections.matching import MatchingCollections +from galaxy.model.dataset_collections.types.sample_sheet_workbook import _sample_sheet_to_list_collection_type from galaxy.schema.credentials import CredentialsContext from galaxy.tool_shed.util.repository_util import get_installed_repository from galaxy.tool_shed.util.shed_util_common import set_image_paths @@ -4671,6 +4672,66 @@ class DuplicateFileToCollectionTool(DatabaseOperationTool): ) +class ConvertSampleSheetTool(DatabaseOperationTool): + """Convert a sample sheet collection back to its corresponding non-sample-sheet type. + + This tool strips the sample sheet metadata (column_definitions and row columns) + and converts the collection type from sample_sheet variants to list variants. + """ + + tool_type = "convert_sample_sheet" + require_terminal_states = False + require_dataset_ok = False + + def produce_outputs(self, trans, out_data, output_collections, incoming, history, **kwds): + has_collection = incoming["input"] + if hasattr(has_collection, "element_type"): + # It is a DCE + collection = has_collection.element_object + else: + # It is an HDCA + collection = has_collection.collection + + input_collection_type = collection.collection_type + output_collection_type = _sample_sheet_to_list_collection_type(input_collection_type) + + new_elements: dict[str, Any] = {} + copied_datasets = [] + + def copy_elements(source_collection, target_dict): + for dce in source_collection.elements: + element_identifier = dce.element_identifier + dce_object = dce.element_object + if dce.is_collection: + # Handle nested collections (e.g., paired within sample_sheet:paired) + sub_collection: dict[str, Any] = {} + sub_collection["src"] = "new_collection" + sub_collection["collection_type"] = dce_object.collection_type + sub_elements = {} + for sub_dce in dce_object.elements: + sub_element_identifier = sub_dce.element_identifier + sub_dce_object = sub_dce.element_object + copied_dataset = sub_dce_object.copy(copy_tags=sub_dce_object.tags, flush=False) + sub_elements[sub_element_identifier] = copied_dataset + copied_datasets.append(copied_dataset) + sub_collection["elements"] = sub_elements + target_dict[element_identifier] = sub_collection + else: + copied_dataset = dce_object.copy(copy_tags=dce_object.tags, flush=False) + target_dict[element_identifier] = copied_dataset + copied_datasets.append(copied_dataset) + + copy_elements(collection, new_elements) + self._add_datasets_to_history(history, copied_datasets) + output_collections.create_collection( + next(iter(self.outputs.values())), + "output", + collection_type=output_collection_type, + elements=new_elements, + propagate_hda_tags=False, + ) + + # Populate tool_type to ToolClass mappings TOOL_CLASSES: list[type[Tool]] = [ Tool, @@ -4690,6 +4751,7 @@ TOOL_CLASSES: list[type[Tool]] = [ BuildListCollectionTool, ExtractDatasetCollectionTool, DataDestinationTool, + ConvertSampleSheetTool, ] tool_types = {tool_class.tool_type: tool_class for tool_class in TOOL_CLASSES} diff --git a/lib/galaxy/tools/convert_sample_sheet.xml b/lib/galaxy/tools/convert_sample_sheet.xml new file mode 100644 index 00000000000..0ed0a43a700 --- /dev/null +++ b/lib/galaxy/tools/convert_sample_sheet.xml @@ -0,0 +1,52 @@ + + to list collection + + + + operation_2409 + + + model_operation_macros.xml + + + + + + + + + + + diff --git a/lib/galaxy_test/api/test_tools.py b/lib/galaxy_test/api/test_tools.py index 884a06b48ea..5f3972da947 100644 --- a/lib/galaxy_test/api/test_tools.py +++ b/lib/galaxy_test/api/test_tools.py @@ -751,6 +751,89 @@ class TestToolsApi(ApiTestCase, TestsTools): assert run_response.status_code == 400 assert run_response.json()["err_msg"] == "Dataset collection has no element_index with key 100." + @skip_without_tool("__CONVERT_SAMPLE_SHEET__") + def test_convert_sample_sheet_to_list(self): + with self.dataset_populator.test_history(require_new=False) as history_id: + # Create sample_sheet collection with column_definitions and rows + create_response = self.dataset_collection_populator.create_sample_sheet( + history_id, + contents=[("sample1", "content1"), ("sample2", "content2")], + column_definitions=[ + {"type": "int", "name": "replicate", "optional": False}, + {"type": "string", "name": "treatment", "optional": False}, + ], + rows={"sample1": [1, "control"], "sample2": [2, "treatment"]}, + ) + self._assert_status_code_is(create_response, 200) + sample_sheet_hdca = create_response.json() + assert sample_sheet_hdca["collection_type"] == "sample_sheet" + assert sample_sheet_hdca["column_definitions"] is not None + + # Run convert sample sheet tool + inputs = {"input": {"src": "hdca", "id": sample_sheet_hdca["id"]}} + self.dataset_populator.wait_for_history(history_id, assert_ok=True) + response = self._run("__CONVERT_SAMPLE_SHEET__", history_id, inputs, assert_ok=True) + + # Verify output is a list collection without sample sheet metadata + output_collections = response["output_collections"] + assert len(output_collections) == 1 + self.dataset_populator.wait_for_job(response["jobs"][0]["id"], assert_ok=True) + converted_hdca = self.dataset_populator.get_history_collection_details( + history_id, hid=output_collections[0]["hid"] + ) + assert converted_hdca["collection_type"] == "list" + assert converted_hdca.get("column_definitions") is None + assert len(converted_hdca["elements"]) == 2 + element_identifiers = [e["element_identifier"] for e in converted_hdca["elements"]] + assert "sample1" in element_identifiers + assert "sample2" in element_identifiers + + @skip_without_tool("__CONVERT_SAMPLE_SHEET__") + def test_convert_sample_sheet_paired_to_list_paired(self): + with self.dataset_populator.test_history(require_new=False) as history_id: + # Create sample_sheet:paired collection + pair_identifiers = self.dataset_collection_populator.pair_identifiers(history_id, ["forward", "reverse"]) + element_identifiers = [ + { + "name": "sample1", + "collection_type": "paired", + "src": "new_collection", + "element_identifiers": pair_identifiers, + } + ] + create_response = self.dataset_collection_populator.create_sample_sheet( + history_id, + contents=element_identifiers, + column_definitions=[{"type": "int", "name": "replicate", "default_value": 0, "optional": False}], + rows={"sample1": [42]}, + collection_type="sample_sheet:paired", + ) + self._assert_status_code_is(create_response, 200) + sample_sheet_hdca = create_response.json() + assert sample_sheet_hdca["collection_type"] == "sample_sheet:paired" + assert sample_sheet_hdca["column_definitions"] is not None + + # Run convert sample sheet tool + inputs = {"input": {"src": "hdca", "id": sample_sheet_hdca["id"]}} + self.dataset_populator.wait_for_history(history_id, assert_ok=True) + response = self._run("__CONVERT_SAMPLE_SHEET__", history_id, inputs, assert_ok=True) + + # Verify output is a list:paired collection without sample sheet metadata + output_collections = response["output_collections"] + assert len(output_collections) == 1 + self.dataset_populator.wait_for_job(response["jobs"][0]["id"], assert_ok=True) + converted_hdca = self.dataset_populator.get_history_collection_details( + history_id, hid=output_collections[0]["hid"] + ) + assert converted_hdca["collection_type"] == "list:paired" + assert converted_hdca.get("column_definitions") is None + assert len(converted_hdca["elements"]) == 1 + # Verify nested paired structure is preserved + element = converted_hdca["elements"][0] + assert element["element_type"] == "dataset_collection" + assert element["object"]["collection_type"] == "paired" + assert len(element["object"]["elements"]) == 2 + @skip_without_tool("__FILTER_FAILED_DATASETS__") def test_filter_failed_list(self): with self.dataset_populator.test_history(require_new=False) as history_id: diff --git a/lib/galaxy_test/base/populators.py b/lib/galaxy_test/base/populators.py index f086af42a30..670011c3b0d 100644 --- a/lib/galaxy_test/base/populators.py +++ b/lib/galaxy_test/base/populators.py @@ -3555,6 +3555,47 @@ class BaseDatasetCollectionPopulator: element_identifiers = [hda_to_identifier(i, hda) for (i, hda) in enumerate(hdas)] return element_identifiers + def create_sample_sheet( + self, + history_id: str, + contents: list, + column_definitions: list, + rows: dict, + name: str = "test sample sheet", + collection_type: str = "sample_sheet", + ): + """Create a sample_sheet collection with metadata. + + Args: + history_id: The history ID to create the collection in. + contents: A list of 2-tuples of form (name, dataset_content) for flat sample sheets, + or a list of element identifiers dicts for nested collections. + column_definitions: List of column definition dicts. + rows: Dict mapping element identifiers to row values. + name: Name for the collection. + collection_type: The collection type (sample_sheet, sample_sheet:paired, etc). + + Returns: + Response from creating the collection. + """ + # For flat sample sheets, create element identifiers from contents + if contents and isinstance(contents[0], tuple): + element_identifiers = self.list_identifiers(history_id, contents) + else: + # Assume contents is already element_identifiers for nested collections + element_identifiers = contents + + payload = dict( + name=name, + instance_type="history", + history_id=history_id, + element_identifiers=element_identifiers, + collection_type=collection_type, + column_definitions=column_definitions, + rows=rows, + ) + return self._create_collection(payload) + def __create(self, payload, wait=False): # Create a collection - either from existing datasets using collection creation API # or from direct uploads with the fetch API. Dispatch on "targets" keyword in payload diff --git a/test/functional/tools/sample_tool_conf.xml b/test/functional/tools/sample_tool_conf.xml index e1d5e36e5bc..d164f0d4c5f 100644 --- a/test/functional/tools/sample_tool_conf.xml +++ b/test/functional/tools/sample_tool_conf.xml @@ -338,6 +338,7 @@ +