Skip to content

Return a clear error for malformed tool ids in guid_to_repository#23141

Open
SAY-5 wants to merge 2 commits into
galaxyproject:devfrom
SAY-5:fix-guid-to-repository-malformed-id
Open

Return a clear error for malformed tool ids in guid_to_repository#23141
SAY-5 wants to merge 2 commits into
galaxyproject:devfrom
SAY-5:fix-guid-to-repository-malformed-id

Conversation

@SAY-5

@SAY-5 SAY-5 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

A TRS tool id whose decoded guid has fewer than four slashes (for example localhost/repos/owner) reaches guid_to_repository, where tool_id.split("/", 5) returns too few values to unpack. The bare ValueError: not enough values to unpack (expected 5, got 3) propagates out of the /api/ga4gh/trs/v2/tools/{id}/versions endpoint as a 500 instead of a request error. Fixes #23139.

This raises RequestParameterInvalidException for malformed ids so the API returns a 400, and reads the owner/name by index so a guid that still carries a trailing version segment keeps working.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
@SAY-5

SAY-5 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Pushed a fix for the mypy failure on the new test: it was passing app=None against the ToolShedApp-typed parameter. Since the malformed-id check runs before app is dereferenced, I cast None to ToolShedApp so the type checker is satisfied while still exercising the error path.

@mvdbeek mvdbeek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The fix looks great but the test doesn't add a lot of value. Can you either remove the test or test the positive cases as well ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

ValueError: not enough values to unpack (expected 5, got 3)

2 participants