[26.0] Fix validation of certain classes of text validators in tools.#22280
Conversation
Optional text params with regex (or other) validators would reject None/"" in request format even though the runtime correctly skips validation for empty optional params. Add optional flag to validator chain so None/"" short-circuit before reaching static validators. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
strictify() and _fill_default_for() in convert.py ignored
parameter.default_value for optional text params, hardcoding None
for absent values. Additionally both used `or` which swallowed
empty string defaults due to Python falsiness.
This caused inconsistent behavior for absent params on tools with
value="" across input formats:
| Input | optional (no default) | optional value="" | optional value="" + regex |
|-------|----------------------|-------------------|--------------------------|
| {} | None | "" | "" |
| None | None | None | None |
| "" | "" | "" | "" |
| valid | value | value | value |
| bad | value | value | fails |
All rows now consistent across legacy, 21.01, and request formats.
Test tools added:
- gx_text_optional_with_empty_default (optional text, value="", no validator)
- gx_text_optional_with_empty_default_regex_validation (the wild case)
- gx_text_optional_regex_validation updated to have no default
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Haven't looked at the code yet (and I don't understand Claude's table), but I think that in your example |
|
@nsoranzo I cannot comprehend that as a reasonable way to interpret these tools but you can and Galaxy can and that is enough - this "fixes" the validation work for your interpretation. |
|
It's a bit of an undefined thing isn't it? Optional should mean null/None is allowed, but how should we apply a regex to null/None anyway? An empty string is allowed on required text parameters, why would the optional make a difference here? |
|
I don't want to argue about whether this makes sense - I don't agree with y'all but I don't think I should have to right? I'm fixing a bug and making it work the way y'all seem to think it should? |
|
I think we all want this to work well ? I agree with you ? I don't think |
|
@mvdbeek I promised you this report at today's meeting - it took 30% of a 5-hour usage window but I got it. The scope of the problem is not small - I think it is too late to fix this without a profile version change. I have a PR forthcoming that hardens and tests the JSON schema generated from our models against parameter_specification - the pattern is capturable there and works fine. I wrote a whole separate pipeline in TypeScript that includes meta-model validation for 100% of parameter_specification.xml coverage and 95% JSON schema coverage that matches Galaxy's generation in Python - this is all handled in there. The purpose of having a parameter specification is exactly to catch these sort of oddities and document them and ensure all of our handling around these things works fine and it does. I wish the tool XML was more intuitive to me - but @nsoranzo thinks it makes sense and he is smarter than me - I think we just work around the disagreement and let the implementation describe the expected behavior for now. Perhaps a place to start the pushback would be to implement a linter that looks at optional text parameters with regex validators and ensure the regex validator accepts the empty string? Because while @nsoranzo might think this makes sense as a syntax he would probably agree it would read clearer if <validator type="regex">[0-9a-zA-Z,: _.-]+</validator>was <validator type="regex">[0-9a-zA-Z,: _.-]*</validator>IWC Workflows Failing Validation: Optional Text Params with Regex ValidatorsCommit: Problem: Optional text parameters with regex validators reject Fix: When Example 1: MultiQC —
|
| Category | Failing Workflows | Primary Culprit Tool |
|---|---|---|
| VGP assembly | 10 | compleasm, gfastats, hifiasm, genomescope, yahs |
| Amplicon | 6 | multiqc |
| Epigenetics | 6 | multiqc, cutadapt |
| SARS-CoV-2 | 6 | multiqc, bcftools_consensus, bcftools_annotate |
| Microbiome | 8 | multiqc, bakta, omark |
| Transcriptomics | 2 | multiqc, cutadapt |
| Variant calling | 3 | multiqc, bwa_mem, bcftools_norm |
| scRNAseq | 2 | multiqc |
| Bacterial genomics | 2 | multiqc, bakta |
| Genome annotation | 3 | multiqc, helixer, gffread |
| Metabolomics | 2 | multiqc, xcms |
| Proteomics | 1 | multiqc, pepquery2 |
| Virology | 3 | multiqc |
| Read preprocessing | 1 | multiqc, cutadapt |
Total: 57/120 IWC workflows fail without the fix.
|
arg, ok, i guess this is probably also all because we just don't discriminate "" from null in client. It's good to have those examples, thank you! |
|
I get the "arg" trust me - want me to implement the linter? |
|
To give a bit more details on why I think our current syntax makes sense to me: it feels natural to me that Note that validators for parameters with This keeps the regex simpler as well. |
|
This PR was merged without a "kind/" label, please correct. |
This is causing issues in the workflow work - a couple tools use this pattern that the tool request API was just rejecting:
Rejecting empty values is the right thing to do with that regex - but Galaxy has a bug I guess that lets it through and so we just need to do that forever I think.
Random Claude Table:
All rows now consistent across legacy, 21.01, and request formats.
How to test the changes?
(Select all options that apply)
License