Skip to content

[26.0] Raise error when API client sends invalid parameter keys #22277

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mvdbeek merged 2 commits into from
Mar 28, 2026
Merged

[26.0] Raise error when API client sends invalid parameter keys #22277

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec0df9d
Raise error when API client sends invalid parameter keys
mvdbeek Mar 27, 2026
79d98b1
Make exception more generic
mvdbeek Mar 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions lib/galaxy/tools/parameters/wrapped.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
Union,
)

from galaxy.exceptions import RequestParameterInvalidException
from galaxy.tools.parameters.basic import (
DataCollectionToolParameter,
DataToolParameter,
Expand Down Expand Up @@ -201,6 +202,13 @@ def process_key(incoming_key: str, incoming_value: Any, d: dict[str, Any]):
# Section / Conditional
input_name = key_parts[0]
subdict = d.get(input_name, {})
if not isinstance(subdict, dict):
raise RequestParameterInvalidException(
f"Parameter '{incoming_key}' uses prefix '{input_name}' as a nested group, "
f"but '{input_name}' was already assigned the plain value {subdict}. "
f"Use the full prefixed parameter name (e.g. '<conditional_name>|<input_name>') "
f"instead of the unprefixed input name."
)
Copy link
Copy Markdown
Member

@jmchilton jmchilton Mar 27, 2026

Choose a reason for hiding this comment

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

It says in the comment above this could be a section and not just a conditional. Also - where is the evidence that this is how subdict became not a subdict. It feel likes Claude walking one potential value through the logic and hitting one kind of error and then generalizing. I'm fine with replacing the error type but this error message feels wrong or unsupported to me. Maybe if I spend more time with it - I would feel differently though.

Copy link
Copy Markdown
Member Author

@mvdbeek mvdbeek Mar 27, 2026

Choose a reason for hiding this comment

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

It says in the comment above this could be a section and not just a conditional. Also - where is the evidence that this is how subdict became not a subdict.

The payload is in the PR description, I deleted the unit test that seemed like nonsense. I did struggle finding a better message here. Yes, it doesn't need to be a conditional, any invalid parameter that conflicts with a valid input is going to fail this.

Copy link
Copy Markdown
Member Author

@mvdbeek mvdbeek Mar 27, 2026

Choose a reason for hiding this comment

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

I assume validating against the schema is going to solve that problem entirely so I thought we can let that one slide 😆

Copy link
Copy Markdown
Member

@jmchilton jmchilton Mar 27, 2026

Choose a reason for hiding this comment

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

Scene cut to how that work is going 😭😭😓...

Screenshot 2026-03-27 at 10 32 52 AM

Copy link
Copy Markdown
Member Author

@mvdbeek mvdbeek Mar 27, 2026

Choose a reason for hiding this comment

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

I mean i don't have all the context but this doesn't seem to be entirely right ?
Screenshot 2026-03-27 at 18 21 59

Copy link
Copy Markdown
Member

@jmchilton jmchilton Mar 27, 2026

Choose a reason for hiding this comment

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

The context is totally unrelated I just wanted to scream into the void - it is workflow state - how we do the maybe it is encoded or not thing - I don't know how it could be consistent but hopefully I'll prove myself wrong.

d[input_name] = subdict
process_key("|".join(key_parts[1:]), incoming_value=incoming_value, d=subdict)

Expand Down
Loading