Fix MetadataValidator

[bernt-matthias]

If no check or skip attribute is given, self.check and self.skip need to be initialized to None. Otherwise validation with value.missing_meta(check=self.check, skip=self.skip) is called with value.missing_meta(check=[""], skip=[""]).

This led to the stalling observed in #13136

Explanation: each data input parameter automatically gets a MetadataValidator. Since this validator was dysfunctional the job was scheduled anyway, but did not execute.

Appears that the validator might have been dysfunctional for a long time (since check and skip have been introduced here: bernt-matthias@b9e242e)

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 contributions under Galaxy's current license.
• I agree to allow the Galaxy committers to license these and all my past contributions to the core galaxy codebase under the MIT license. If this condition is an issue, uncheck and just let us know why with an e-mail to galaxy-committers@lists.galaxyproject.org.

[mvdbeek]

Can you add a test here ?

[bernt-matthias]

Can you add a test here ?

Also thought that this would be a good idea. Will try.

[nsoranzo]

You can replace all the no_validation="true" with a better fix:

diff --git a/lib/galaxy/datatypes/data.py b/lib/galaxy/datatypes/data.py
index f7bc205d94..f3862a60e4 100644
--- a/lib/galaxy/datatypes/data.py
+++ b/lib/galaxy/datatypes/data.py
@@ -240,7 +240,7 @@ class Data(metaclass=DataMeta):
         for key, value in to_check:
             if key in skip or (not check and dataset.metadata.spec[key].get("optional")):
                 continue  # we skip check for optional and nonrequested values here
-            if not value:
+            if value is None:
                 return True
         return False
 

Values like 0, "" or [] for a metadata should be valid.

[bernt-matthias]

You can replace all the no_validation="true" with a better fix:

Yes :) I just realized this as well. Not sure yet if I will keep it anyway, because removing the default metadata validator makes debugging the tests a bit easier ..

Values like 0, "" or [] for a metadata should be valid.

Hah, just stumbled over the same point. Intuitively I thought that we should check if the value is the no_value provided when initializing the MetadataElement. But it seems that this is not useful in some cases. For instance TabularData has comment_lines with no_value="0", i.e. we can not distinguish the cases where the comment_lines metadata has not been set from the case where it has been set to 0.

[nsoranzo]

You can replace all the no_validation="true" with a better fix:

Yes :) I just realized this as well. Not sure yet if I will keep it anyway, because removing the default metadata validator makes debugging the tests a bit easier ..

I'd not use no_validation (apart from the fact that it has a misleading name, it should be no_metadata_validation) so that issues like these are not missed.

[bernt-matthias]

Values like 0, "" or [] for a metadata should be valid.

Spent quite a bit of time thinking about this. While formally correct the statement is not specific enough to ensure if a metadata element is set or not. In my test the tool did still stall, i.e. the failing set_meta in the upload job was not detected in the subsequent step.

I think the it might be the best to check if the value is equal to no_value that has been introduced here: 584999c

Unfortunately, apart from the commit message of the original commit no_value has not been documented and seems to be misused in some/many datatypes, e.g. there are metadata that have identical no_value and default which makes it impossible to check if metadata has been set. I estimate that there are approx 180 metadata elements in the sources that we would need to check...

Essentially we should "ensure" (at least in the datatype definitions) that no_value is a value that can not be set by the set_meta function of the corresponding data type (we could be really strict and check this in the setter or relaxed and just fix the metdata definitions of the data types). But I'm not sure about valid combinations of default, no_value, and optional.

Besides this I was wondering if the test meta_val != meta_spec.no_value in element_is_set is correct given that both values can be None.

I was also wondering if it wouldn't be better to fail the upload job if metadata setting fails.

[mvdbeek]

Is this ready ?

[bernt-matthias]

With an approval of @nsoranzo yes :)