Fixing bugs with data_source tools - part 2

[wm75]

Still discovering a few quirks with data_source and data_source_async tools after #17422

• [x] currently, these tools have to set check_values="false" in their <inputs> tag. Forgetting to do this leads to hard to debug problems, because the default is "true", while this can never make sense for any data_source tool. The proposed solution is to overwrite the setting for the tool class.

• [ ] currently, translation of input params is requested in several places in the code base, i.e., at the controllers level here: https://github.com/galaxyproject/galaxy/blob/2a9c8d05fbd18053d48026fbce280682a5fd0128/lib/galaxy/webapps/galaxy/controllers/tool_runner.py#L86-L87 and here: https://github.com/galaxyproject/galaxy/blob/2a9c8d05fbd18053d48026fbce280682a5fd0128/lib/galaxy/webapps/galaxy/controllers/async.py#L83-L84 and at the tool level, here: https://github.com/galaxyproject/galaxy/blob/2a9c8d05fbd18053d48026fbce280682a5fd0128/lib/galaxy/tools/init.py#L1834-L1835 and here: https://github.com/galaxyproject/galaxy/blob/2a9c8d05fbd18053d48026fbce280682a5fd0128/lib/galaxy/tools/init.py#L2502-L2503

  The controller-level translations are required, but repeated translations at the tool level can break data_source tools. In particular, URL append operations are broken because the same params get added to the URL twice. That's why I'd like to disable the tool-level translations, but I'm worried that this could have side-effects, and really would like feedback first. We could play it safe and disable extra translations only for data_source tools with sth like: if self.input_translator and not isinstance(self, DataSourceTool): just in case input translation has some exotic undocumented use outside data_source tools?

~~- [ ] I need help with an additional bug with this usage pattern where an ampersand is used to join GET request parameters. Unfortunately, that char gets sanitized by Galaxy (to an X by default), but I fail to understand where that happens~~

@mvdbeek maybe you can help me with the open points? :pray:

License

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

[wm75]

Ah, the sanitization issue is solved by using a tool-wide <options sanitize="False" />. Now I know why existing data_source tools include that :-)

That leaves only the multiple translator calls.

[wm75]

Ok, so once more I'm happy with my changes and think they are ready for reviewing.

[martenson]

this commit is breaking any tool that relies on undeclared params, which I suspect could be a pattern that is actually used? I am a bit wary about doing that.

[wm75]

I am a bit wary about doing that.

Was so too, that's why I didn't touch this in my prior PR. However, out of the built-in data source tools only the UCSC ones seem to work currently and I checked and updated these to still work after the change. Any other data source tools not shipping with Galaxy would be easy to fix.

[martenson]

Even if easy to fix, breaking tools is something Galaxy works hard to avoid. We have tool profiles to mitigate that, maybe we could consider applying it here.

[wm75]

Even if easy to fix, breaking tools is something Galaxy works hard to avoid. We have tool profiles to mitigate that, maybe we could consider applying it here.

That's a very good idea, thanks! Implemented it in the latest commit.

[wm75]

The one failing test appears to be unrelated again :-)

[martenson]

Thanks @wm75 !

I believe this PR should be manually tested on Main for the release. I added the item to the PR description.