From ebacc42da5c32ccf15cd61f5c8c069a4cd85e689 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Sun, 3 Jan 2021 18:47:48 +0100
Subject: [PATCH 01/30] add the possibility to negate the regex validator

---
 lib/galaxy/tools/parameters/validation.py | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 9b4d86e226d..aaa82060610 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -35,9 +35,9 @@ class Validator:
         param elem the validator element
         return an object of a Validator subclass that corresponds to the type attribute of the validator element
         """
-        type = elem.get('type', None)
-        assert type is not None, "Required 'type' attribute missing from validator"
-        return validator_types[type].from_element(param, elem)
+        _type = elem.get('type', None)
+        assert _type is not None, "Required 'type' attribute missing from validator"
+        return validator_types[_type].from_element(param, elem)
 
     def validate(self, value, trans=None):
         """
@@ -74,19 +74,21 @@ class RegexValidator(Validator):
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message'), elem.text)
+        return cls(elem.get('message'), elem.text, elem.get('negate', 'false'))
 
-    def __init__(self, message, expression):
+    def __init__(self, message, expression, negate):
         self.message = message
         # Compile later. RE objects used to not be thread safe. Not sure about
         # the sre module.
         self.expression = expression
+        self.invert = util.asbool(negate)
 
     def validate(self, value, trans=None):
         if not isinstance(value, list):
             value = [value]
         for val in value:
-            if re.match(self.expression, val or '') is None:
+            match = re.match(self.expression, val or '')
+            if (not self.invert and match is None) or (self.invert and match is not None):
                 raise ValueError(self.message)
 
 

From 8107e78e4ff09a02892574964643c874851c0b46 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Wed, 6 Jan 2021 12:12:43 +0100
Subject: [PATCH 02/30] tmp

---
 lib/galaxy/tools/parameters/validation.py | 138 +++++++++++-----------
 1 file changed, 66 insertions(+), 72 deletions(-)

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index aaa82060610..2cf162bd715 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -1,6 +1,7 @@
 """
 Classes related to parameter validation.
 """
+import abc
 import logging
 import re
 
@@ -13,7 +14,7 @@ from galaxy import (
 log = logging.getLogger(__name__)
 
 
-class Validator:
+class Validator(abc.ABC):
     """
     A validator checks that a value meets some conditions OR raises ValueError
     """
@@ -39,13 +40,26 @@ class Validator:
         assert _type is not None, "Required 'type' attribute missing from validator"
         return validator_types[_type].from_element(param, elem)
 
-    def validate(self, value, trans=None):
+    def __init__(self, message, negate=False):
+        self.message = message
+        self.negate = util.asbool(negate)
+        super().__init__()
+
+    @abc.abstractmethod
+    def validate(self, value, trans=None, message=None):
         """
         validate a value
 
         return None if positive validation, otherwise a ValueError is raised
         """
-        raise TypeError("Abstract Method")
+        if message is None:
+            message = self.message
+        log.error("validate %s %s" % (value, self.negate))
+        if (not self.negate and value) or (self.negate and not value):
+            return
+        else:
+            raise ValueError(message)
+        pass
 
 
 class RegexValidator(Validator):
@@ -77,7 +91,7 @@ class RegexValidator(Validator):
         return cls(elem.get('message'), elem.text, elem.get('negate', 'false'))
 
     def __init__(self, message, expression, negate):
-        self.message = message
+        super().__init__(message, negate)
         # Compile later. RE objects used to not be thread safe. Not sure about
         # the sre module.
         self.expression = expression
@@ -88,8 +102,7 @@ class RegexValidator(Validator):
             value = [value]
         for val in value:
             match = re.match(self.expression, val or '')
-            if (not self.invert and match is None) or (self.invert and match is not None):
-                raise ValueError(self.message)
+            super().validate(match is not None, trans)
 
 
 class ExpressionValidator(Validator):
@@ -116,7 +129,8 @@ class ExpressionValidator(Validator):
         return cls(elem.get('message'), elem.text, elem.get('substitute_value_in_message'))
 
     def __init__(self, message, expression, substitute_value_in_message):
-        self.message = message
+        super().__init__(message)
+        # TODO 
         self.substitute_value_in_message = substitute_value_in_message
         # Save compiled expression, code objects are thread safe (right?)
         self.expression = compile(expression, '<string>', 'eval')
@@ -129,9 +143,11 @@ class ExpressionValidator(Validator):
             evalresult = eval(self.expression, dict(value=value))
         except Exception:
             log.debug(f"Validator {self.expression} could not be evaluated on {str(value)}", exc_info=True)
-            raise ValueError(message)
+            super().validate(false, trans, message)
         if not(evalresult):
-            raise ValueError(message)
+            super().validate(false, trans, message)
+        else:
+            super().validate(true, trans, message)
 
 
 class InRangeValidator(Validator):
@@ -159,11 +175,12 @@ class InRangeValidator(Validator):
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message', None), elem.get('min'),
-                   elem.get('max'), elem.get('exclude_min', 'false'),
-                   elem.get('exclude_max', 'false'))
+        return cls(elem.get('message', None), elem.get('min', '-inf'),
+                   elem.get('max', 'inf'), elem.get('exclude_min', 'false'),
+                   elem.get('exclude_max', 'false'),
+                   elem.get('negate', 'false'))
 
-    def __init__(self, message, range_min, range_max, exclude_min=False, exclude_max=False):
+    def __init__(self, message, range_min, range_max, exclude_min, exclude_max, negate):
         """
         When the optional exclude_min and exclude_max attributes are set
         to true, the range excludes the end points (i.e., min < value < max),
@@ -171,9 +188,11 @@ class InRangeValidator(Validator):
         (1.e., min <= value <= max).  Combinations of exclude_min and exclude_max
         values are allowed.
         """
-        self.min = float(range_min if range_min is not None else '-inf')
+        super().__init__(message or f"Value must be {op1} {self_min_str} and {op2} {self_max_str}", negate)
+
+        self.min = float(range_min)
         self.exclude_min = util.asbool(exclude_min)
-        self.max = float(range_max if range_max is not None else 'inf')
+        self.max = float(range_max)
         self.exclude_max = util.asbool(exclude_max)
         assert self.min <= self.max, 'min must be less than or equal to max'
         # Remove unneeded 0s and decimal from floats to make message pretty.
@@ -185,21 +204,16 @@ class InRangeValidator(Validator):
             op1 = '>'
         if self.exclude_max:
             op2 = '<'
-        self.message = message or f"Value must be {op1} {self_min_str} and {op2} {self_max_str}"
 
     def validate(self, value, trans=None):
         if self.exclude_min:
-            if not self.min < float(value):
-                raise ValueError(self.message)
+            super().validate(self.min < float(value), trans)
         else:
-            if not self.min <= float(value):
-                raise ValueError(self.message)
+            super().validate(self.min <= float(value), trans)
         if self.exclude_max:
-            if not float(value) < self.max:
-                raise ValueError(self.message)
+            super().validate(float(value) < self.max, trans)
         else:
-            if not float(value) <= self.max:
-                raise ValueError(self.message)
+            super().validate(float(value) <= self.max, trans)
 
 
 class LengthValidator(Validator):
@@ -230,7 +244,7 @@ class LengthValidator(Validator):
         return cls(elem.get('message', None), elem.get('min', None), elem.get('max', None))
 
     def __init__(self, message, length_min, length_max):
-        self.message = message
+        super().__init__(message)
         if length_min is not None:
             length_min = int(length_min)
         if length_max is not None:
@@ -240,9 +254,11 @@ class LengthValidator(Validator):
 
     def validate(self, value, trans=None):
         if self.min is not None and len(value) < self.min:
-            raise ValueError(self.message or ("Must have length of at least %d" % self.min))
-        if self.max is not None and len(value) > self.max:
-            raise ValueError(self.message or ("Must have length no more than %d" % self.max))
+            super().validate(false, trans, self.message or ("Must have length of at least %d" % self.min))
+        elif self.max is not None and len(value) > self.max:
+            super().validate(false, trans, self.message or ("Must have length no more than %d" % self.max))
+        else:
+            super().validate(true, trans)
 
 
 class DatasetOkValidator(Validator):
@@ -250,9 +266,6 @@ class DatasetOkValidator(Validator):
     Validator that checks if a dataset is in an 'ok' state
     """
 
-    def __init__(self, message=None):
-        self.message = message
-
     @classmethod
     def from_element(cls, param, elem):
         return cls(elem.get('message', None))
@@ -267,9 +280,6 @@ class DatasetOkValidator(Validator):
 class DatasetEmptyValidator(Validator):
     """Validator that checks if a dataset has a positive file size."""
 
-    def __init__(self, message=None):
-        self.message = message
-
     @classmethod
     def from_element(cls, param, elem):
         return cls(elem.get('message', None))
@@ -285,9 +295,6 @@ class DatasetEmptyValidator(Validator):
 class DatasetExtraFilesPathEmptyValidator(Validator):
     """Validator that checks if a dataset's extra_files_path exists and is not empty."""
 
-    def __init__(self, message=None):
-        self.message = message
-
     @classmethod
     def from_element(cls, param, elem):
         return cls(elem.get('message', None))
@@ -306,15 +313,15 @@ class MetadataValidator(Validator):
     """
     requires_dataset_metadata = True
 
-    def __init__(self, message=None, check="", skip=""):
-        self.message = message
-        self.check = check.split(",")
-        self.skip = skip.split(",")
-
     @classmethod
     def from_element(cls, param, elem):
         return cls(message=elem.get('message', None), check=elem.get('check', ""), skip=elem.get('skip', ""))
 
+    def __init__(self, message=None, check="", skip=""):
+        super().__init__(message)
+        self.check = check.split(",")
+        self.skip = skip.split(",")
+
     def validate(self, value, trans=None):
         if value:
             if not isinstance(value, model.DatasetInstance):
@@ -331,15 +338,9 @@ class UnspecifiedBuildValidator(Validator):
     """
     requires_dataset_metadata = True
 
-    def __init__(self, message=None):
-        if message is None:
-            self.message = "Unspecified genome build, click the pencil icon in the history item to set the genome build"
-        else:
-            self.message = message
-
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message', None))
+        return cls(elem.get('message', "Unspecified genome build, click the pencil icon in the history item to set the genome build"))
 
     def validate(self, value, trans=None):
         # if value is None, we cannot validate
@@ -354,34 +355,26 @@ class UnspecifiedBuildValidator(Validator):
 class NoOptionsValidator(Validator):
     """Validator that checks for empty select list"""
 
-    def __init__(self, message=None):
-        self.message = message
-
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message', None))
+        return cls(elem.get('message', "No options available for selection"))
 
     def validate(self, value, trans=None):
         if value is None:
-            if self.message is None:
-                self.message = "No options available for selection"
             raise ValueError(self.message)
 
 
 class EmptyTextfieldValidator(Validator):
     """Validator that checks for empty text field"""
 
-    def __init__(self, message=None):
-        self.message = message
-
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message', None))
+        return cls(elem.get('message', "Field requires a value"))
 
     def validate(self, value, trans=None):
         if value == '':
             if self.message is None:
-                self.message = "Field requires a value"
+                self.message = "
             raise ValueError(self.message)
 
 
@@ -408,8 +401,8 @@ class MetadataInFileColumnValidator(Validator):
         return cls(filename, metadata_name, metadata_column, message, line_startswith, split)
 
     def __init__(self, filename, metadata_name, metadata_column, message="Value for metadata not found.", line_startswith=None, split="\t"):
+        super().__init__(message)
         self.metadata_name = metadata_name
-        self.message = message
         self.valid_values = []
         for line in open(filename):
             if line_startswith is None or line.startswith(line_startswith):
@@ -448,7 +441,7 @@ class ValueInDataTableColumnValidator(Validator):
         return cls(tool_data_table, column, message, line_startswith)
 
     def __init__(self, tool_data_table, column, message="Value not found.", line_startswith=None):
-        self.message = message
+        super().__init__(message)
         self.valid_values = []
         self._data_table_content_version = None
         self._tool_data_table = tool_data_table
@@ -518,8 +511,8 @@ class MetadataInDataTableColumnValidator(Validator):
         return cls(tool_data_table, metadata_name, metadata_column, message, line_startswith)
 
     def __init__(self, tool_data_table, metadata_name, metadata_column, message="Value for metadata not found.", line_startswith=None):
+        super().__init__(message)
         self.metadata_name = metadata_name
-        self.message = message
         self.valid_values = []
         self._data_table_content_version = None
         self._tool_data_table = tool_data_table
@@ -567,23 +560,23 @@ class MetadataNotInDataTableColumnValidator(MetadataInDataTableColumnValidator):
 
 class MetadataInRangeValidator(InRangeValidator):
     """
-    Validator that ensures metadata is in a specified range
+    validator that ensures metadata is in a specified range
     """
-    requires_dataset_metadata = True
+    requires_dataset_metadata = true
 
     @classmethod
     def from_element(cls, param, elem):
-        metadata_name = elem.get('metadata_name', None)
+        metadata_name = elem.get('metadata_name', none)
         assert metadata_name, "dataset_metadata_in_range validator requires metadata_name attribute."
         metadata_name = metadata_name.strip()
-        return cls(metadata_name,
-                   elem.get('message', None), elem.get('min'),
-                   elem.get('max'), elem.get('exclude_min', 'false'),
-                   elem.get('exclude_max', 'false'))
+        return cls(metadata_name, elem.get('message', None),
+                   elem.get('min'), elem.get('max'),
+                   elem.get('exclude_min', 'false'), elem.get('exclude_max', 'false'),
+                   elem.get('negate', 'false'))
 
-    def __init__(self, metadata_name, message, range_min, range_max, exclude_min=False, exclude_max=False):
+    def __init__(self, metadata_name, message, range_min, range_max, exclude_min, exclude_max, negate):
         self.metadata_name = metadata_name
-        super().__init__(message, range_min, range_max, exclude_min, exclude_max)
+        super().__init__(message, range_min, range_max, exclude_min, exclude_max, negate)
 
     def validate(self, value, trans=None):
         if value:
@@ -596,6 +589,7 @@ class MetadataInRangeValidator(InRangeValidator):
             except ValueError:
                 raise ValueError(f'{self.metadata_name} must be a float or an integer')
             super().validate(value_to_check, trans)
+        super().validate(true, trans)
 
 
 validator_types = dict(

From 8a8e4398965c776198841352ed7bfffbbd5c4ee8 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Mon, 15 Feb 2021 17:03:22 +0100
Subject: [PATCH 03/30] tmp

---
 lib/galaxy/tools/parameters/validation.py | 407 ++++++++++++++++++----
 1 file changed, 341 insertions(+), 66 deletions(-)

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 2cf162bd715..b147331de8d 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -41,6 +41,7 @@ class Validator(abc.ABC):
         return validator_types[_type].from_element(param, elem)
 
     def __init__(self, message, negate=False):
+        log.error("INIT msg %s neg %s" % (message, negate))
         self.message = message
         self.negate = util.asbool(negate)
         super().__init__()
@@ -52,12 +53,14 @@ class Validator(abc.ABC):
 
         return None if positive validation, otherwise a ValueError is raised
         """
+        log.error("VAL value %s" % value)
         if message is None:
             message = self.message
         log.error("validate %s %s" % (value, self.negate))
         if (not self.negate and value) or (self.negate and not value):
             return
         else:
+            # TODO message often makes not sense if negate=True
             raise ValueError(message)
         pass
 
@@ -84,6 +87,26 @@ class RegexValidator(Validator):
     Traceback (most recent call last):
         ...
     ValueError: Not gonna happen
+    >>>
+    >>> p = ToolParameter.build(None, XML('''
+    ... <param name="blah" type="text" value="10">
+    ...     <validator type="regex" message="Not gonna happen" negate="true">[Ff]oo</validator>
+    ... </param>
+    ... '''))
+    >>> t = p.validate("Foo")
+    Traceback (most recent call last):
+        ...
+    ValueError: Not gonna happen
+    >>> t = p.validate("foo")
+    Traceback (most recent call last):
+        ...
+    ValueError: Not gonna happen
+    >>> t = p.validate("Fop")
+    >>> t = p.validate(["Foo", "foo"])
+    Traceback (most recent call last):
+        ...
+    ValueError: Not gonna happen
+    >>> t = p.validate(["Fop", "Fop"])
     """
 
     @classmethod
@@ -95,7 +118,6 @@ class RegexValidator(Validator):
         # Compile later. RE objects used to not be thread safe. Not sure about
         # the sre module.
         self.expression = expression
-        self.invert = util.asbool(negate)
 
     def validate(self, value, trans=None):
         if not isinstance(value, list):
@@ -122,14 +144,29 @@ class ExpressionValidator(Validator):
     Traceback (most recent call last):
         ...
     ValueError: Not gonna happen
+    >>>
+    >>> p = ToolParameter.build(None, XML('''
+    ... <param name="blah" type="text" value="10">
+    ...     <validator type="expression" message="Not gonna happen" negate="true">value.lower() == "foo"</validator>
+    ... </param>
+    ... '''))
+    >>> t = p.validate("Foo")
+    Traceback (most recent call last):
+        ...
+    ValueError: Not gonna happen
+    >>> t = p.validate("foo")
+    Traceback (most recent call last):
+        ...
+    ValueError: Not gonna happen
+    >>> t = p.validate("Fop")
     """
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message'), elem.text, elem.get('substitute_value_in_message'))
+        return cls(elem.get('message'), elem.text, elem.get('substitute_value_in_message'), elem.get('negate', 'false'))
 
-    def __init__(self, message, expression, substitute_value_in_message):
-        super().__init__(message)
+    def __init__(self, message, expression, substitute_value_in_message, negate):
+        super().__init__(message, negate)
         # TODO 
         self.substitute_value_in_message = substitute_value_in_message
         # Save compiled expression, code objects are thread safe (right?)
@@ -143,13 +180,14 @@ class ExpressionValidator(Validator):
             evalresult = eval(self.expression, dict(value=value))
         except Exception:
             log.debug(f"Validator {self.expression} could not be evaluated on {str(value)}", exc_info=True)
-            super().validate(false, trans, message)
+            super().validate(False, trans, message)
         if not(evalresult):
-            super().validate(false, trans, message)
+            super().validate(False, trans, message)
         else:
-            super().validate(true, trans, message)
+            super().validate(True, trans, message)
 
 
+# TODO This could be a subclass of ExpressionValidator
 class InRangeValidator(Validator):
     """
     Validator that ensures a number is in a specified range
@@ -171,6 +209,22 @@ class InRangeValidator(Validator):
     Traceback (most recent call last):
         ...
     ValueError: Not gonna happen
+    >>>
+    >>> p = ToolParameter.build(None, XML('''
+    ... <param name="blah" type="integer" value="10">
+    ...     <validator type="in_range" message="Not gonna happen" min="10" exclude_min="true" max="20" negate="true"/>
+    ... </param>
+    ... '''))
+    >>> t = p.validate(10)
+    >>> t = p.validate(15)
+    Traceback (most recent call last):
+        ...
+    ValueError: Not gonna happen
+    >>> t = p.validate(20)
+    Traceback (most recent call last):
+        ...
+    ValueError: Not gonna happen
+    >>> t = p.validate(21)
     """
 
     @classmethod
@@ -188,8 +242,6 @@ class InRangeValidator(Validator):
         (1.e., min <= value <= max).  Combinations of exclude_min and exclude_max
         values are allowed.
         """
-        super().__init__(message or f"Value must be {op1} {self_min_str} and {op2} {self_max_str}", negate)
-
         self.min = float(range_min)
         self.exclude_min = util.asbool(exclude_min)
         self.max = float(range_max)
@@ -204,18 +256,21 @@ class InRangeValidator(Validator):
             op1 = '>'
         if self.exclude_max:
             op2 = '<'
+        super().__init__(message or f"Value must be {op1} {self_min_str} and {op2} {self_max_str}", negate)
 
     def validate(self, value, trans=None):
         if self.exclude_min:
-            super().validate(self.min < float(value), trans)
+            mincmp = self.min.__lt__
         else:
-            super().validate(self.min <= float(value), trans)
+            mincmp = self.min.__le__
         if self.exclude_max:
-            super().validate(float(value) < self.max, trans)
+            maxcmp = self.max.__gt__
         else:
-            super().validate(float(value) <= self.max, trans)
+            maxcmp = self.max.__ge__
+        super().validate(mincmp(float(value)) and maxcmp(float(value)), trans)
 
 
+# TODO This could be a subclass of InRangeValidator
 class LengthValidator(Validator):
     """
     Validator that ensures the length of the provided string (value) is in a specific range
@@ -232,128 +287,345 @@ class LengthValidator(Validator):
     >>> t = p.validate("f")
     Traceback (most recent call last):
         ...
-    ValueError: Must have length of at least 2
+    ValueError: Must have length of at least 2 and at most 8
     >>> t = p.validate("foobarbaz")
     Traceback (most recent call last):
         ...
-    ValueError: Must have length no more than 8
+    ValueError: Must have length of at least 2 and at most 8
+    >>>
+    >>> p = ToolParameter.build(None, XML('''
+    ... <param name="blah" type="text" value="foobar">
+    ...     <validator type="length" min="2" max="8" negate="true"/>
+    ... </param>
+    ... '''))
+    >>> t = p.validate("foo")
+    Traceback (most recent call last):
+        ...
+    ValueError: Must have length of at least 2 and at most 8
+    >>> t = p.validate("bar")
+    Traceback (most recent call last):
+        ...
+    ValueError: Must have length of at least 2 and at most 8
+    >>> t = p.validate("f")
+    >>> t = p.validate("foobarbaz")
     """
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message', None), elem.get('min', None), elem.get('max', None))
+        return cls(elem.get('message', None), elem.get('min', None), elem.get('max', None), elem.get('negate', 'false'))
 
-    def __init__(self, message, length_min, length_max):
-        super().__init__(message)
+    def __init__(self, message, length_min, length_max, negate):
+        super().__init__(message, negate)
         if length_min is not None:
-            length_min = int(length_min)
-        if length_max is not None:
-            length_max = int(length_max)
-        self.min = length_min
-        self.max = length_max
-
-    def validate(self, value, trans=None):
-        if self.min is not None and len(value) < self.min:
-            super().validate(false, trans, self.message or ("Must have length of at least %d" % self.min))
-        elif self.max is not None and len(value) > self.max:
-            super().validate(false, trans, self.message or ("Must have length no more than %d" % self.max))
+            self.min = int(length_min)
         else:
-            super().validate(true, trans)
+            self.min = float('-inf')
+        if length_max is not None:
+            self.max = int(length_max)
+        else:
+            self.max = float('inf')
+    
+    def validate(self, value, trans=None):
+        super().validate(self.min <= len(value) <= self.max, trans, self.message or ("Must have length of at least %d and at most %s" % (self.min, self.max)))
 
 
 class DatasetOkValidator(Validator):
     """
     Validator that checks if a dataset is in an 'ok' state
+
+    >>> from galaxy.datatypes.registry import example_datatype_registry_for_sample
+    >>> from galaxy.model import History, HistoryDatasetAssociation, set_datatypes_registry
+    >>> from galaxy.model.mapping import init
+    >>> from galaxy.util import XML
+    >>> from galaxy.tools.parameters.basic import ToolParameter
+    >>> 
+    >>> sa_session = init("/tmp", "sqlite:///:memory:", create_tables=True).session
+    >>> hist = History()
+    >>> sa_session.add(hist)
+    >>> sa_session.flush()
+    >>> set_datatypes_registry(example_datatype_registry_for_sample())
+    >>> ok_hda = hist.add_dataset(HistoryDatasetAssociation(id=1, extension='interval', create_dataset=True, sa_session=sa_session))
+    >>> ok_hda.set_dataset_state(model.Dataset.states.OK)
+    >>> notok_hda = hist.add_dataset(HistoryDatasetAssociation(id=2, extension='interval', create_dataset=True, sa_session=sa_session))
+    >>> # TODO I do not get 100% why for state!=OK the validator is called 
+    >>> # TODO because DataToolParameter.validate.do_validate calls the validator only of state=OK
+    >>> # TODO in this light I wonder about the use of this validator at all....
+    >>> notok_hda.set_dataset_state(model.Dataset.states.EMPTY)
+    >>> 
+    >>> p = ToolParameter.build(None, XML('''
+    ... <param name="blah" type="data">
+    ...     <validator type="dataset_ok_validator"/>
+    ... </param>
+    ... '''))
+    >>> t = p.validate(ok_hda)
+    >>> t = p.validate(notok_hda)
+    Traceback (most recent call last):
+        ...
+    ValueError: The selected dataset is still being generated, select another dataset or wait until it is completed
+    >>>
+    >>> p = ToolParameter.build(None, XML('''
+    ... <param name="blah" type="data">
+    ...     <validator type="dataset_ok_validator" negate="true"/>
+    ... </param>
+    ... '''))
+    >>> t = p.validate(ok_hda)
+    Traceback (most recent call last):
+        ...
+    ValueError: The selected dataset is still being generated, select another dataset or wait until it is completed
+    >>> t = p.validate(notok_hda)
     """
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message', None))
+        return cls(elem.get('message', None), elem.get('negate', 'false'))
 
     def validate(self, value, trans=None):
-        if value and value.state != model.Dataset.states.OK:
-            if self.message is None:
-                self.message = "The selected dataset is still being generated, select another dataset or wait until it is completed"
-            raise ValueError(self.message)
+        # TODO all Dataset Validators should be able to handle lists, or? 
+        if self.message is None:
+            self.message = "The selected dataset is still being generated, select another dataset or wait until it is completed"
+        super().validate(value and value.state == model.Dataset.states.OK)
 
 
 class DatasetEmptyValidator(Validator):
-    """Validator that checks if a dataset has a positive file size."""
+    """
+    Validator that checks if a dataset has a positive file size.
+    
+    >>> from galaxy.datatypes.registry import example_datatype_registry_for_sample
+    >>> from galaxy.model import History, HistoryDatasetAssociation, set_datatypes_registry
+    >>> from galaxy.model.mapping import init
+    >>> from galaxy.util import XML
+    >>> from galaxy.tools.parameters.basic import ToolParameter
+    >>> 
+    >>> sa_session = init("/tmp", "sqlite:///:memory:", create_tables=True).session
+    >>> hist = History()
+    >>> sa_session.add(hist)
+    >>> sa_session.flush()
+    >>> set_datatypes_registry(example_datatype_registry_for_sample())
+    >>> empty_hda = hist.add_dataset(HistoryDatasetAssociation(id=1, extension='interval', create_dataset=True, sa_session=sa_session))
+    >>> empty_hda.dataset.file_size = 0
+    >>> full_hda = hist.add_dataset(HistoryDatasetAssociation(id=2, extension='interval', create_dataset=True, sa_session=sa_session))
+    >>> full_hda.dataset.file_size = 1
+    >>> 
+    >>> p = ToolParameter.build(None, XML('''
+    ... <param name="blah" type="data">
+    ...     <validator type="empty_dataset"/>
+    ... </param>
+    ... '''))
+    >>> t = p.validate(full_hda)
+    >>> t = p.validate(empty_hda)
+    Traceback (most recent call last):
+        ...
+    ValueError: The selected dataset is empty, this tool expects non-empty files.
+    >>> 
+    >>> p = ToolParameter.build(None, XML('''
+    ... <param name="blah" type="data">
+    ...     <validator type="empty_dataset" negate="true"/>
+    ... </param>
+    ... '''))
+    >>> t = p.validate(full_hda)
+    Traceback (most recent call last):
+        ...
+    ValueError: The selected dataset is empty, this tool expects non-empty files.
+    >>> t = p.validate(empty_hda)
+    """
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message', None))
+        return cls(elem.get('message', "The selected dataset is empty, this tool expects non-empty files."), elem.get('negate', 'false'))
 
     def validate(self, value, trans=None):
-        if value:
-            if value.get_size() == 0:
-                if self.message is None:
-                    self.message = "The selected dataset is empty, this tool expects non-empty files."
-                raise ValueError(self.message)
+        super().validate(not(value and value.get_size() == 0))
 
 
 class DatasetExtraFilesPathEmptyValidator(Validator):
-    """Validator that checks if a dataset's extra_files_path exists and is not empty."""
+    """
+    Validator that checks if a dataset's extra_files_path exists and is not empty.
+    
+    >>> from galaxy.datatypes.registry import example_datatype_registry_for_sample
+    >>> from galaxy.model import History, HistoryDatasetAssociation, set_datatypes_registry
+    >>> from galaxy.model.mapping import init
+    >>> from galaxy.util import XML
+    >>> from galaxy.tools.parameters.basic import ToolParameter
+    >>> 
+    >>> sa_session = init("/tmp", "sqlite:///:memory:", create_tables=True).session
+    >>> hist = History()
+    >>> sa_session.add(hist)
+    >>> sa_session.flush()
+    >>> set_datatypes_registry(example_datatype_registry_for_sample())
+    >>> has_extra_hda = hist.add_dataset(HistoryDatasetAssociation(id=1, extension='interval', create_dataset=True, sa_session=sa_session))
+    >>> has_extra_hda.dataset.file_size = 10
+    >>> has_extra_hda.dataset.total_size = 15
+    >>> has_no_extra_hda = hist.add_dataset(HistoryDatasetAssociation(id=2, extension='interval', create_dataset=True, sa_session=sa_session))
+    >>> has_no_extra_hda.dataset.file_size = 10
+    >>> has_no_extra_hda.dataset.total_size = 10
+    >>> 
+    >>> p = ToolParameter.build(None, XML('''
+    ... <param name="blah" type="data">
+    ...     <validator type="empty_extra_files_path"/>
+    ... </param>
+    ... '''))
+    >>> t = p.validate(has_extra_hda)
+    >>> t = p.validate(has_no_extra_hda)
+    Traceback (most recent call last):
+        ...
+    ValueError: The selected dataset's extra_files_path directory is empty or does not exist, this tool expects non-empty extra_files_path directories associated with the selected input.
+    >>> 
+    >>> p = ToolParameter.build(None, XML('''
+    ... <param name="blah" type="data">
+    ...     <validator type="empty_extra_files_path" negate="true"/>
+    ... </param>
+    ... '''))
+    >>> t = p.validate(has_extra_hda)
+    Traceback (most recent call last):
+        ...
+    ValueError: The selected dataset's extra_files_path directory is empty or does not exist, this tool expects non-empty extra_files_path directories associated with the selected input.
+    >>> t = p.validate(has_no_extra_hda)
+    """
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message', None))
+        return cls(elem.get('message', "The selected dataset's extra_files_path directory is empty or does not exist, this tool expects non-empty extra_files_path directories associated with the selected input."), elem.get('negate', 'false'))
 
     def validate(self, value, trans=None):
-        if value:
-            if value.get_total_size() == value.get_size():
-                if self.message is None:
-                    self.message = "The selected dataset's extra_files_path directory is empty or does not exist, this tool expects non-empty extra_files_path directories associated with the selected input."
-                raise ValueError(self.message)
+        super().validate(not(value and value.get_total_size() == value.get_size()))
 
 
 class MetadataValidator(Validator):
     """
     Validator that checks for missing metadata
+
+    >>> from galaxy.datatypes.registry import example_datatype_registry_for_sample
+    >>> from galaxy.model import History, HistoryDatasetAssociation, set_datatypes_registry
+    >>> from galaxy.model.mapping import init
+    >>> from galaxy.util import XML
+    >>> from galaxy.tools.parameters.basic import ToolParameter
+    >>> 
+    >>> sa_session = init("/tmp", "sqlite:///:memory:", create_tables=True).session
+    >>> hist = History()
+    >>> sa_session.add(hist)
+    >>> sa_session.flush()
+    >>> set_datatypes_registry(example_datatype_registry_for_sample())
+    >>> hda = hist.add_dataset(HistoryDatasetAssociation(id=1, extension='interval', create_dataset=True, sa_session=sa_session))
+    >>> hda.set_dataset_state(model.Dataset.states.OK)
+    >>> # TODO I did not find a way to remove a metadata from the hda, therefore I used two parameters, ideas?
+    >>> p = ToolParameter.build(None, XML('''
+    ... <param name="blah" type="data">
+    ...     <validator type="metadata" check="dbkey" skip="absent_metadata"/>
+    ... </param>
+    ... '''))
+    >>> p2 = ToolParameter.build(None, XML('''
+    ... <param name="blah" type="data">
+    ...     <validator type="metadata" check="absent_metadata" skip="dbkey"/>
+    ... </param>
+    ... '''))
+    >>> t = p.validate(hda)
+    >>> t = p2.validate(hda)
+    Traceback (most recent call last):
+        ...
+    ValueError: Metadata missing, click the pencil icon in the history item to edit / save the metadata attributes
+    >>> p = ToolParameter.build(None, XML('''
+    ... <param name="blah" type="data">
+    ...     <validator type="metadata" check="dbkey" skip="absent_metadata" negate="true"/>
+    ... </param>
+    ... '''))
+    >>> p2 = ToolParameter.build(None, XML('''
+    ... <param name="blah" type="data">
+    ...     <validator type="metadata" check="absent_metadata" skip="dbkey" negate="true"/>
+    ... </param>
+    ... '''))
+    >>> t = p.validate(hda)
+    Traceback (most recent call last):
+        ...
+    ValueError: Metadata missing, click the pencil icon in the history item to edit / save the metadata attributes
+    >>> t = p2.validate(hda)
     """
     requires_dataset_metadata = True
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(message=elem.get('message', None), check=elem.get('check', ""), skip=elem.get('skip', ""))
+        return cls(message=elem.get('message', "Metadata missing, click the pencil icon in the history item to edit / save the metadata attributes"),
+                   check=elem.get('check', ""), 
+                   skip=elem.get('skip', ""),
+                   negate=elem.get('negate', 'false'))
 
-    def __init__(self, message=None, check="", skip=""):
-        super().__init__(message)
+    def __init__(self, message=None, check="", skip="", negate='false'):
+        log.error("MetadataValidator")
+        super().__init__(message, negate)
         self.check = check.split(",")
         self.skip = skip.split(",")
 
     def validate(self, value, trans=None):
-        if value:
-            if not isinstance(value, model.DatasetInstance):
-                raise ValueError('A non-dataset value was provided.')
-            if value.missing_meta(check=self.check, skip=self.skip):
-                if self.message is None:
-                    self.message = "Metadata missing, click the pencil icon in the history item to edit / save the metadata attributes"
-                raise ValueError(self.message)
+        log.error("VAL value %s" % value)
+        
+        # TODO why this validator checks for isinstance(value, model.DatasetInstance)
+        super().validate( value and isinstance(value, model.DatasetInstance) and not value.missing_meta(check=self.check, skip=self.skip) )
 
 
 class UnspecifiedBuildValidator(Validator):
     """
     Validator that checks for dbkey not equal to '?'
+
+    >>> from galaxy.datatypes.registry import example_datatype_registry_for_sample
+    >>> from galaxy.model import History, HistoryDatasetAssociation, set_datatypes_registry
+    >>> from galaxy.model.mapping import init
+    >>> from galaxy.util import XML
+    >>> from galaxy.tools.parameters.basic import ToolParameter
+    >>> 
+    >>> sa_session = init("/tmp", "sqlite:///:memory:", create_tables=True).session
+    >>> hist = History()
+    >>> sa_session.add(hist)
+    >>> sa_session.flush()
+    >>> set_datatypes_registry(example_datatype_registry_for_sample())
+    >>> has_dbkey_hda = hist.add_dataset(HistoryDatasetAssociation(id=1, extension='interval', create_dataset=True, sa_session=sa_session))
+    >>> has_dbkey_hda.set_dataset_state(model.Dataset.states.OK)
+    >>> has_dbkey_hda.metadata.dbkey = 'hg19'
+    >>> has_no_dbkey_hda = hist.add_dataset(HistoryDatasetAssociation(id=2, extension='interval', create_dataset=True, sa_session=sa_session))
+    >>> has_no_dbkey_hda.set_dataset_state(model.Dataset.states.OK)
+    >>> 
+    >>> p = ToolParameter.build(None, XML('''
+    ... <param name="blah" type="data">
+    ...     <validator type="unspecified_build"/>
+    ... </param>
+    ... '''))
+    >>> t = p.validate(has_dbkey_hda)
+    >>> t = p.validate(has_no_dbkey_hda)
+    Traceback (most recent call last):
+        ...
+    ValueError: Unspecified genome build, click the pencil icon in the history item to set the genome build
+    >>> 
+    >>> p = ToolParameter.build(None, XML('''
+    ... <param name="blah" type="data">
+    ...     <validator type="unspecified_build" negate="true"/>
+    ... </param>
+    ... '''))
+    >>> t = p.validate(has_dbkey_hda)
+    Traceback (most recent call last):
+        ...
+    ValueError: Unspecified genome build, click the pencil icon in the history item to set the genome build
+    >>> t = p.validate(has_no_dbkey_hda)
     """
     requires_dataset_metadata = True
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message', "Unspecified genome build, click the pencil icon in the history item to set the genome build"))
+        return cls(elem.get('message', "Unspecified genome build, click the pencil icon in the history item to set the genome build"),
+                   elem.get('negate', 'false'))
 
     def validate(self, value, trans=None):
         # if value is None, we cannot validate
         if value:
             dbkey = value.metadata.dbkey
+            # TODO can dbkey really be a list?
             if isinstance(dbkey, list):
                 dbkey = dbkey[0]
-            if dbkey == '?':
-                raise ValueError(self.message)
+            super().validate(dbkey != '?')            
 
 
 class NoOptionsValidator(Validator):
-    """Validator that checks for empty select list"""
+    """
+    Validator that checks for empty select list
+
+    """
 
     @classmethod
     def from_element(cls, param, elem):
@@ -365,7 +637,10 @@ class NoOptionsValidator(Validator):
 
 
 class EmptyTextfieldValidator(Validator):
-    """Validator that checks for empty text field"""
+    """
+    Validator that checks for empty text field
+    
+    """
 
     @classmethod
     def from_element(cls, param, elem):
@@ -374,7 +649,7 @@ class EmptyTextfieldValidator(Validator):
     def validate(self, value, trans=None):
         if value == '':
             if self.message is None:
-                self.message = "
+                self.message = ""
             raise ValueError(self.message)
 
 
@@ -562,7 +837,7 @@ class MetadataInRangeValidator(InRangeValidator):
     """
     validator that ensures metadata is in a specified range
     """
-    requires_dataset_metadata = true
+    requires_dataset_metadata = True
 
     @classmethod
     def from_element(cls, param, elem):

From d8dd833dd68fbad7ae42f58c84bc279c741a93b6 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Wed, 17 Feb 2021 11:50:04 +0100
Subject: [PATCH 04/30] add defaults

---
 lib/galaxy/tools/parameters/validation.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index b147331de8d..a711a16aef8 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -234,7 +234,7 @@ class InRangeValidator(Validator):
                    elem.get('exclude_max', 'false'),
                    elem.get('negate', 'false'))
 
-    def __init__(self, message, range_min, range_max, exclude_min, exclude_max, negate):
+    def __init__(self, message, range_min, range_max, exclude_min=False, exclude_max=False, negate=False):
         """
         When the optional exclude_min and exclude_max attributes are set
         to true, the range excludes the end points (i.e., min < value < max),

From a4b14a10711c43bc508791ff83eeadf750550bd2 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Thu, 25 Feb 2021 18:39:18 +0100
Subject: [PATCH 05/30] do not validate if `value is None` for data set
 validators

---
 lib/galaxy/tools/parameters/validation.py | 65 ++++++++++++-----------
 1 file changed, 34 insertions(+), 31 deletions(-)

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index a711a16aef8..5a7d5cd384b 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -167,7 +167,7 @@ class ExpressionValidator(Validator):
 
     def __init__(self, message, expression, substitute_value_in_message, negate):
         super().__init__(message, negate)
-        # TODO 
+        # TODO
         self.substitute_value_in_message = substitute_value_in_message
         # Save compiled expression, code objects are thread safe (right?)
         self.expression = compile(expression, '<string>', 'eval')
@@ -324,7 +324,7 @@ class LengthValidator(Validator):
             self.max = int(length_max)
         else:
             self.max = float('inf')
-    
+
     def validate(self, value, trans=None):
         super().validate(self.min <= len(value) <= self.max, trans, self.message or ("Must have length of at least %d and at most %s" % (self.min, self.max)))
 
@@ -338,7 +338,7 @@ class DatasetOkValidator(Validator):
     >>> from galaxy.model.mapping import init
     >>> from galaxy.util import XML
     >>> from galaxy.tools.parameters.basic import ToolParameter
-    >>> 
+    >>>
     >>> sa_session = init("/tmp", "sqlite:///:memory:", create_tables=True).session
     >>> hist = History()
     >>> sa_session.add(hist)
@@ -347,11 +347,11 @@ class DatasetOkValidator(Validator):
     >>> ok_hda = hist.add_dataset(HistoryDatasetAssociation(id=1, extension='interval', create_dataset=True, sa_session=sa_session))
     >>> ok_hda.set_dataset_state(model.Dataset.states.OK)
     >>> notok_hda = hist.add_dataset(HistoryDatasetAssociation(id=2, extension='interval', create_dataset=True, sa_session=sa_session))
-    >>> # TODO I do not get 100% why for state!=OK the validator is called 
+    >>> # TODO I do not get 100% why for state!=OK the validator is called
     >>> # TODO because DataToolParameter.validate.do_validate calls the validator only of state=OK
     >>> # TODO in this light I wonder about the use of this validator at all....
     >>> notok_hda.set_dataset_state(model.Dataset.states.EMPTY)
-    >>> 
+    >>>
     >>> p = ToolParameter.build(None, XML('''
     ... <param name="blah" type="data">
     ...     <validator type="dataset_ok_validator"/>
@@ -380,22 +380,23 @@ class DatasetOkValidator(Validator):
         return cls(elem.get('message', None), elem.get('negate', 'false'))
 
     def validate(self, value, trans=None):
-        # TODO all Dataset Validators should be able to handle lists, or? 
-        if self.message is None:
-            self.message = "The selected dataset is still being generated, select another dataset or wait until it is completed"
-        super().validate(value and value.state == model.Dataset.states.OK)
+        if value:
+            # TODO all Dataset Validators should be able to handle lists, or?
+            if self.message is None:
+                self.message = "The selected dataset is still being generated, select another dataset or wait until it is completed"
+            super().validate(value.state == model.Dataset.states.OK)
 
 
 class DatasetEmptyValidator(Validator):
     """
     Validator that checks if a dataset has a positive file size.
-    
+   
     >>> from galaxy.datatypes.registry import example_datatype_registry_for_sample
     >>> from galaxy.model import History, HistoryDatasetAssociation, set_datatypes_registry
     >>> from galaxy.model.mapping import init
     >>> from galaxy.util import XML
     >>> from galaxy.tools.parameters.basic import ToolParameter
-    >>> 
+    >>>
     >>> sa_session = init("/tmp", "sqlite:///:memory:", create_tables=True).session
     >>> hist = History()
     >>> sa_session.add(hist)
@@ -405,7 +406,7 @@ class DatasetEmptyValidator(Validator):
     >>> empty_hda.dataset.file_size = 0
     >>> full_hda = hist.add_dataset(HistoryDatasetAssociation(id=2, extension='interval', create_dataset=True, sa_session=sa_session))
     >>> full_hda.dataset.file_size = 1
-    >>> 
+    >>>
     >>> p = ToolParameter.build(None, XML('''
     ... <param name="blah" type="data">
     ...     <validator type="empty_dataset"/>
@@ -416,7 +417,7 @@ class DatasetEmptyValidator(Validator):
     Traceback (most recent call last):
         ...
     ValueError: The selected dataset is empty, this tool expects non-empty files.
-    >>> 
+    >>>
     >>> p = ToolParameter.build(None, XML('''
     ... <param name="blah" type="data">
     ...     <validator type="empty_dataset" negate="true"/>
@@ -434,19 +435,20 @@ class DatasetEmptyValidator(Validator):
         return cls(elem.get('message', "The selected dataset is empty, this tool expects non-empty files."), elem.get('negate', 'false'))
 
     def validate(self, value, trans=None):
-        super().validate(not(value and value.get_size() == 0))
+        if value:
+            super().validate(value.get_size() != 0)
 
 
 class DatasetExtraFilesPathEmptyValidator(Validator):
     """
     Validator that checks if a dataset's extra_files_path exists and is not empty.
-    
+   
     >>> from galaxy.datatypes.registry import example_datatype_registry_for_sample
     >>> from galaxy.model import History, HistoryDatasetAssociation, set_datatypes_registry
     >>> from galaxy.model.mapping import init
     >>> from galaxy.util import XML
     >>> from galaxy.tools.parameters.basic import ToolParameter
-    >>> 
+    >>>
     >>> sa_session = init("/tmp", "sqlite:///:memory:", create_tables=True).session
     >>> hist = History()
     >>> sa_session.add(hist)
@@ -458,7 +460,7 @@ class DatasetExtraFilesPathEmptyValidator(Validator):
     >>> has_no_extra_hda = hist.add_dataset(HistoryDatasetAssociation(id=2, extension='interval', create_dataset=True, sa_session=sa_session))
     >>> has_no_extra_hda.dataset.file_size = 10
     >>> has_no_extra_hda.dataset.total_size = 10
-    >>> 
+    >>>
     >>> p = ToolParameter.build(None, XML('''
     ... <param name="blah" type="data">
     ...     <validator type="empty_extra_files_path"/>
@@ -469,7 +471,7 @@ class DatasetExtraFilesPathEmptyValidator(Validator):
     Traceback (most recent call last):
         ...
     ValueError: The selected dataset's extra_files_path directory is empty or does not exist, this tool expects non-empty extra_files_path directories associated with the selected input.
-    >>> 
+    >>>
     >>> p = ToolParameter.build(None, XML('''
     ... <param name="blah" type="data">
     ...     <validator type="empty_extra_files_path" negate="true"/>
@@ -487,7 +489,8 @@ class DatasetExtraFilesPathEmptyValidator(Validator):
         return cls(elem.get('message', "The selected dataset's extra_files_path directory is empty or does not exist, this tool expects non-empty extra_files_path directories associated with the selected input."), elem.get('negate', 'false'))
 
     def validate(self, value, trans=None):
-        super().validate(not(value and value.get_total_size() == value.get_size()))
+        if value:
+            super().validate(value.get_total_size() != value.get_size())
 
 
 class MetadataValidator(Validator):
@@ -499,7 +502,7 @@ class MetadataValidator(Validator):
     >>> from galaxy.model.mapping import init
     >>> from galaxy.util import XML
     >>> from galaxy.tools.parameters.basic import ToolParameter
-    >>> 
+    >>>
     >>> sa_session = init("/tmp", "sqlite:///:memory:", create_tables=True).session
     >>> hist = History()
     >>> sa_session.add(hist)
@@ -544,7 +547,7 @@ class MetadataValidator(Validator):
     @classmethod
     def from_element(cls, param, elem):
         return cls(message=elem.get('message', "Metadata missing, click the pencil icon in the history item to edit / save the metadata attributes"),
-                   check=elem.get('check', ""), 
+                   check=elem.get('check', ""),
                    skip=elem.get('skip', ""),
                    negate=elem.get('negate', 'false'))
 
@@ -556,9 +559,9 @@ class MetadataValidator(Validator):
 
     def validate(self, value, trans=None):
         log.error("VAL value %s" % value)
-        
-        # TODO why this validator checks for isinstance(value, model.DatasetInstance)
-        super().validate( value and isinstance(value, model.DatasetInstance) and not value.missing_meta(check=self.check, skip=self.skip) )
+        if value:
+            # TODO why this validator checks for isinstance(value, model.DatasetInstance)
+            super().validate(isinstance(value, model.DatasetInstance) and not value.missing_meta(check=self.check, skip=self.skip))
 
 
 class UnspecifiedBuildValidator(Validator):
@@ -570,7 +573,7 @@ class UnspecifiedBuildValidator(Validator):
     >>> from galaxy.model.mapping import init
     >>> from galaxy.util import XML
     >>> from galaxy.tools.parameters.basic import ToolParameter
-    >>> 
+    >>>
     >>> sa_session = init("/tmp", "sqlite:///:memory:", create_tables=True).session
     >>> hist = History()
     >>> sa_session.add(hist)
@@ -581,7 +584,7 @@ class UnspecifiedBuildValidator(Validator):
     >>> has_dbkey_hda.metadata.dbkey = 'hg19'
     >>> has_no_dbkey_hda = hist.add_dataset(HistoryDatasetAssociation(id=2, extension='interval', create_dataset=True, sa_session=sa_session))
     >>> has_no_dbkey_hda.set_dataset_state(model.Dataset.states.OK)
-    >>> 
+    >>>
     >>> p = ToolParameter.build(None, XML('''
     ... <param name="blah" type="data">
     ...     <validator type="unspecified_build"/>
@@ -592,7 +595,7 @@ class UnspecifiedBuildValidator(Validator):
     Traceback (most recent call last):
         ...
     ValueError: Unspecified genome build, click the pencil icon in the history item to set the genome build
-    >>> 
+    >>>
     >>> p = ToolParameter.build(None, XML('''
     ... <param name="blah" type="data">
     ...     <validator type="unspecified_build" negate="true"/>
@@ -618,7 +621,7 @@ class UnspecifiedBuildValidator(Validator):
             # TODO can dbkey really be a list?
             if isinstance(dbkey, list):
                 dbkey = dbkey[0]
-            super().validate(dbkey != '?')            
+            super().validate(dbkey != '?')           
 
 
 class NoOptionsValidator(Validator):
@@ -639,7 +642,7 @@ class NoOptionsValidator(Validator):
 class EmptyTextfieldValidator(Validator):
     """
     Validator that checks for empty text field
-    
+   
     """
 
     @classmethod
@@ -841,7 +844,7 @@ class MetadataInRangeValidator(InRangeValidator):
 
     @classmethod
     def from_element(cls, param, elem):
-        metadata_name = elem.get('metadata_name', none)
+        metadata_name = elem.get('metadata_name', None)
         assert metadata_name, "dataset_metadata_in_range validator requires metadata_name attribute."
         metadata_name = metadata_name.strip()
         return cls(metadata_name, elem.get('message', None),
@@ -864,7 +867,7 @@ class MetadataInRangeValidator(InRangeValidator):
             except ValueError:
                 raise ValueError(f'{self.metadata_name} must be a float or an integer')
             super().validate(value_to_check, trans)
-        super().validate(true, trans)
+        super().validate(True, trans)
 
 
 validator_types = dict(

From 8bda57ed8b1c5980caab4563feefd12e47b5d531 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Fri, 26 Feb 2021 11:21:14 +0100
Subject: [PATCH 06/30] mod NoOptionsValidator

---
 lib/galaxy/tools/parameters/validation.py | 37 ++++++++++++++++++-----
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 5a7d5cd384b..cfe28fb90b7 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -390,7 +390,7 @@ class DatasetOkValidator(Validator):
 class DatasetEmptyValidator(Validator):
     """
     Validator that checks if a dataset has a positive file size.
-   
+
     >>> from galaxy.datatypes.registry import example_datatype_registry_for_sample
     >>> from galaxy.model import History, HistoryDatasetAssociation, set_datatypes_registry
     >>> from galaxy.model.mapping import init
@@ -442,7 +442,7 @@ class DatasetEmptyValidator(Validator):
 class DatasetExtraFilesPathEmptyValidator(Validator):
     """
     Validator that checks if a dataset's extra_files_path exists and is not empty.
-   
+
     >>> from galaxy.datatypes.registry import example_datatype_registry_for_sample
     >>> from galaxy.model import History, HistoryDatasetAssociation, set_datatypes_registry
     >>> from galaxy.model.mapping import init
@@ -621,28 +621,51 @@ class UnspecifiedBuildValidator(Validator):
             # TODO can dbkey really be a list?
             if isinstance(dbkey, list):
                 dbkey = dbkey[0]
-            super().validate(dbkey != '?')           
+            super().validate(dbkey != '?')
 
 
 class NoOptionsValidator(Validator):
     """
     Validator that checks for empty select list
 
+    >>> from galaxy.util import XML
+    >>> from galaxy.tools.parameters.basic import ToolParameter
+    >>> p = ToolParameter.build(None, XML('''
+    ... <param name="index" type="select" label="Select reference genome" help="If your genome of interest is not listed, contact the Galaxy team">
+    ...     <options from_data_table="bowtie2_indexes"/>
+    ...     <validator type="no_options" message="No indexes are available for the selected input dataset"/>
+    ... </param>
+    ... '''))
+    >>> t = p.validate('foo')
+    >>> t = p.validate(None)
+    Traceback (most recent call last):
+        ...
+    ValueError: No options available for selection
+    >>>
+    >>> p = ToolParameter.build(None, XML('''
+    ... <param name="index" type="select" label="Select reference genome" help="If your genome of interest is not listed, contact the Galaxy team">
+    ...     <options from_data_table="bowtie2_indexes"/>
+    ...     <validator type="no_options" message="No indexes are available for the selected input dataset" negate="true"/>
+    ... </param>
+    ... '''))
+    >>> t = p.validate('foo')
+    Traceback (most recent call last):
+        ...
+    ValueError: No options available for selection
+    >>> t = p.validate(None)
     """
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message', "No options available for selection"))
+        return cls(elem.get('message', "No options available for selection"), elem.get('negate', 'false'))
 
     def validate(self, value, trans=None):
-        if value is None:
-            raise ValueError(self.message)
+        super().validate( value is None )
 
 
 class EmptyTextfieldValidator(Validator):
     """
     Validator that checks for empty text field
-   
     """
 
     @classmethod

From 9dcc4310fec91a9fe149d3ceea7bb7551f1f7d88 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Fri, 26 Feb 2021 11:57:56 +0100
Subject: [PATCH 07/30] fix InRangeValidator

min/max can be initialized with None
---
 lib/galaxy/tools/parameters/validation.py | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index cfe28fb90b7..16b9954eb1c 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -229,8 +229,8 @@ class InRangeValidator(Validator):
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message', None), elem.get('min', '-inf'),
-                   elem.get('max', 'inf'), elem.get('exclude_min', 'false'),
+        return cls(elem.get('message', None), elem.get('min'),
+                   elem.get('max'), elem.get('exclude_min', 'false'),
                    elem.get('exclude_max', 'false'),
                    elem.get('negate', 'false'))
 
@@ -242,9 +242,9 @@ class InRangeValidator(Validator):
         (1.e., min <= value <= max).  Combinations of exclude_min and exclude_max
         values are allowed.
         """
-        self.min = float(range_min)
+        self.min = float(range_min if range_min is not None else '-inf')
         self.exclude_min = util.asbool(exclude_min)
-        self.max = float(range_max)
+        self.max = float(range_max if range_max is not None else 'inf')
         self.exclude_max = util.asbool(exclude_max)
         assert self.min <= self.max, 'min must be less than or equal to max'
         # Remove unneeded 0s and decimal from floats to make message pretty.
@@ -631,7 +631,7 @@ class NoOptionsValidator(Validator):
     >>> from galaxy.util import XML
     >>> from galaxy.tools.parameters.basic import ToolParameter
     >>> p = ToolParameter.build(None, XML('''
-    ... <param name="index" type="select" label="Select reference genome" help="If your genome of interest is not listed, contact the Galaxy team">
+    ... <param name="index" type="select" label="Select reference genome">
     ...     <options from_data_table="bowtie2_indexes"/>
     ...     <validator type="no_options" message="No indexes are available for the selected input dataset"/>
     ... </param>
@@ -643,7 +643,7 @@ class NoOptionsValidator(Validator):
     ValueError: No options available for selection
     >>>
     >>> p = ToolParameter.build(None, XML('''
-    ... <param name="index" type="select" label="Select reference genome" help="If your genome of interest is not listed, contact the Galaxy team">
+    ... <param name="index" type="select" label="Select reference genome">
     ...     <options from_data_table="bowtie2_indexes"/>
     ...     <validator type="no_options" message="No indexes are available for the selected input dataset" negate="true"/>
     ... </param>
@@ -660,7 +660,7 @@ class NoOptionsValidator(Validator):
         return cls(elem.get('message', "No options available for selection"), elem.get('negate', 'false'))
 
     def validate(self, value, trans=None):
-        super().validate( value is None )
+        super().validate(value is None)
 
 
 class EmptyTextfieldValidator(Validator):

From e987ec4ca1a21d04ba66df25f1714594081e6286 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Fri, 26 Feb 2021 12:54:29 +0100
Subject: [PATCH 08/30] fix noOptions

---
 lib/galaxy/tools/parameters/validation.py | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 16b9954eb1c..7749b45e02d 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -41,7 +41,6 @@ class Validator(abc.ABC):
         return validator_types[_type].from_element(param, elem)
 
     def __init__(self, message, negate=False):
-        log.error("INIT msg %s neg %s" % (message, negate))
         self.message = message
         self.negate = util.asbool(negate)
         super().__init__()
@@ -552,7 +551,6 @@ class MetadataValidator(Validator):
                    negate=elem.get('negate', 'false'))
 
     def __init__(self, message=None, check="", skip="", negate='false'):
-        log.error("MetadataValidator")
         super().__init__(message, negate)
         self.check = check.split(",")
         self.skip = skip.split(",")
@@ -632,7 +630,6 @@ class NoOptionsValidator(Validator):
     >>> from galaxy.tools.parameters.basic import ToolParameter
     >>> p = ToolParameter.build(None, XML('''
     ... <param name="index" type="select" label="Select reference genome">
-    ...     <options from_data_table="bowtie2_indexes"/>
     ...     <validator type="no_options" message="No indexes are available for the selected input dataset"/>
     ... </param>
     ... '''))
@@ -660,7 +657,7 @@ class NoOptionsValidator(Validator):
         return cls(elem.get('message', "No options available for selection"), elem.get('negate', 'false'))
 
     def validate(self, value, trans=None):
-        super().validate(value is None)
+        super().validate(value is not None)
 
 
 class EmptyTextfieldValidator(Validator):

From 554802c8033dedf0b45739017b72c6f6e4d0d890 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Mon, 15 Feb 2021 17:07:22 +0100
Subject: [PATCH 09/30] unset dataset size needs to be checked against None

---
 lib/galaxy/model/__init__.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py
index 28f153b5115..e351ef7403a 100644
--- a/lib/galaxy/model/__init__.py
+++ b/lib/galaxy/model/__init__.py
@@ -2664,7 +2664,7 @@ class Dataset(StorableObject, RepresentById, _HasTable):
 
     def get_size(self, nice_size=False):
         """Returns the size of the data on disk"""
-        if self.file_size:
+        if self.file_size is not None:
             if nice_size:
                 return galaxy.util.nice_size(self.file_size)
             else:
@@ -2682,7 +2682,7 @@ class Dataset(StorableObject, RepresentById, _HasTable):
         calls to get_total_size or set_total_size - potentially avoiding both a database flush and check against
         the file system.
         """
-        if not self.file_size:
+        if self.file_size is not None:
             self.file_size = self._calculate_size()
             if no_extra_files:
                 self.total_size = self.file_size

From e2ecb33cc871f3698569d2e6627e0ff729374a4d Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Fri, 26 Feb 2021 13:36:18 +0100
Subject: [PATCH 10/30] fix nooptions check

---
 lib/galaxy/tools/parameters/validation.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 7749b45e02d..126e2aaf63b 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -630,7 +630,7 @@ class NoOptionsValidator(Validator):
     >>> from galaxy.tools.parameters.basic import ToolParameter
     >>> p = ToolParameter.build(None, XML('''
     ... <param name="index" type="select" label="Select reference genome">
-    ...     <validator type="no_options" message="No indexes are available for the selected input dataset"/>
+    ...     <validator type="no_options" message="No options available for selection"/>
     ... </param>
     ... '''))
     >>> t = p.validate('foo')
@@ -642,7 +642,7 @@ class NoOptionsValidator(Validator):
     >>> p = ToolParameter.build(None, XML('''
     ... <param name="index" type="select" label="Select reference genome">
     ...     <options from_data_table="bowtie2_indexes"/>
-    ...     <validator type="no_options" message="No indexes are available for the selected input dataset" negate="true"/>
+    ...     <validator type="no_options" negate="true"/>
     ... </param>
     ... '''))
     >>> t = p.validate('foo')

From a323517021bf7a87f6921e8f5c809def144af35f Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Mon, 19 Jul 2021 17:57:05 +0200
Subject: [PATCH 11/30] fix unit test for NoOptionsValidator and finish
 EmptyTextfieldValidator

---
 .../tools/parameters/dynamic_options.py       |  3 ++
 lib/galaxy/tools/parameters/validation.py     | 36 ++++++++++++++++---
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/lib/galaxy/tools/parameters/dynamic_options.py b/lib/galaxy/tools/parameters/dynamic_options.py
index a0614a47f5a..759457f4a6a 100644
--- a/lib/galaxy/tools/parameters/dynamic_options.py
+++ b/lib/galaxy/tools/parameters/dynamic_options.py
@@ -586,6 +586,9 @@ class DynamicOptions:
     @property
     def tool_data_table(self):
         if self.tool_data_table_name:
+            # this is needed for the validator unit tests and should not happen in real life
+            if self.tool_param.tool is None:
+                return None
             tool_data_table = self.tool_param.tool.app.tool_data_tables.get(self.tool_data_table_name, None)
             if tool_data_table:
                 # Column definitions are optional, but if provided override those from the table
diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 126e2aaf63b..565460dcf81 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -663,17 +663,43 @@ class NoOptionsValidator(Validator):
 class EmptyTextfieldValidator(Validator):
     """
     Validator that checks for empty text field
+
+    >>> from galaxy.util import XML
+    >>> from galaxy.tools.parameters.basic import ToolParameter
+    >>> p = ToolParameter.build(None, XML('''
+    ... <param name="blah" type="text" value="">
+    ...     <validator type="empty_field"/>
+    ... </param>
+    ... '''))
+    >>> t = p.validate("")
+    Traceback (most recent call last):
+        ...
+    ValueError: Field requires a value
+    >>> p = ToolParameter.build(None, XML('''
+    ... <param name="blah" type="text" value="">
+    ...     <validator type="empty_field" negate="true"/>
+    ... </param>
+    ... '''))
+    >>> t = p.validate("foo")
+    Traceback (most recent call last):
+        ...
+    ValueError: Field must not set a value
+    >>> t = p.validate("")
     """
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message', "Field requires a value"))
+        negate = elem.get('negate', 'false')
+        if negate == 'false':
+            message = elem.get('message', "Field requires a value")
+        else:
+            message = elem.get('message', "Field must not set a value")
+        return cls(message, negate)
 
     def validate(self, value, trans=None):
-        if value == '':
-            if self.message is None:
-                self.message = ""
-            raise ValueError(self.message)
+        if self.message is None:
+            self.message = ""
+        super().validate(value != '')
 
 
 class MetadataInFileColumnValidator(Validator):

From 16a7b4007518fa23f6b86d2279d7020fa79c401d Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Wed, 21 Jul 2021 13:08:50 +0200
Subject: [PATCH 12/30] Revert "unset dataset size needs to be checked against
 None"

This reverts commit e5a9800e8558b51db65791333849114cad18516b.
---
 lib/galaxy/model/__init__.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py
index e351ef7403a..28f153b5115 100644
--- a/lib/galaxy/model/__init__.py
+++ b/lib/galaxy/model/__init__.py
@@ -2664,7 +2664,7 @@ class Dataset(StorableObject, RepresentById, _HasTable):
 
     def get_size(self, nice_size=False):
         """Returns the size of the data on disk"""
-        if self.file_size is not None:
+        if self.file_size:
             if nice_size:
                 return galaxy.util.nice_size(self.file_size)
             else:
@@ -2682,7 +2682,7 @@ class Dataset(StorableObject, RepresentById, _HasTable):
         calls to get_total_size or set_total_size - potentially avoiding both a database flush and check against
         the file system.
         """
-        if self.file_size is not None:
+        if not self.file_size:
             self.file_size = self._calculate_size()
             if no_extra_files:
                 self.total_size = self.file_size

From 988b16fcc068e4397a4947a239f7def7385dcd54 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Wed, 21 Jul 2021 13:36:07 +0200
Subject: [PATCH 13/30] fix DatasetEmptyValidator doctests

without the change to check the data set size (e5a9800e85 reverted in d20ae267fd)
---
 lib/galaxy/tools/parameters/validation.py | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 565460dcf81..1f3ab74618f 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -391,7 +391,7 @@ class DatasetEmptyValidator(Validator):
     Validator that checks if a dataset has a positive file size.
 
     >>> from galaxy.datatypes.registry import example_datatype_registry_for_sample
-    >>> from galaxy.model import History, HistoryDatasetAssociation, set_datatypes_registry
+    >>> from galaxy.model import Dataset, History, HistoryDatasetAssociation, set_datatypes_registry
     >>> from galaxy.model.mapping import init
     >>> from galaxy.util import XML
     >>> from galaxy.tools.parameters.basic import ToolParameter
@@ -401,10 +401,11 @@ class DatasetEmptyValidator(Validator):
     >>> sa_session.add(hist)
     >>> sa_session.flush()
     >>> set_datatypes_registry(example_datatype_registry_for_sample())
-    >>> empty_hda = hist.add_dataset(HistoryDatasetAssociation(id=1, extension='interval', create_dataset=True, sa_session=sa_session))
-    >>> empty_hda.dataset.file_size = 0
-    >>> full_hda = hist.add_dataset(HistoryDatasetAssociation(id=2, extension='interval', create_dataset=True, sa_session=sa_session))
-    >>> full_hda.dataset.file_size = 1
+    >>> # TODO is there a better way than hardcoding 'test-data/'
+    >>> empty_dataset = Dataset(external_filename="test-data/empty.txt")
+    >>> empty_hda = hist.add_dataset(HistoryDatasetAssociation(id=1, extension='interval', dataset=empty_dataset, sa_session=sa_session))
+    >>> full_dataset = Dataset(external_filename="test-data/1.tabular")
+    >>> full_hda = hist.add_dataset(HistoryDatasetAssociation(id=2, extension='interval', dataset=full_dataset, sa_session=sa_session))
     >>>
     >>> p = ToolParameter.build(None, XML('''
     ... <param name="blah" type="data">

From 1983c952c1782133ff75839a1d861f2a46796e58 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Wed, 21 Jul 2021 15:34:19 +0200
Subject: [PATCH 14/30] fix failing framework test for MetadataInRangeValidator

---
 lib/galaxy/tools/parameters/validation.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 1f3ab74618f..054ae2838db 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -266,6 +266,7 @@ class InRangeValidator(Validator):
             maxcmp = self.max.__gt__
         else:
             maxcmp = self.max.__ge__
+        log.error(f"{value} min {mincmp(float(value))} max {maxcmp(float(value))}")
         super().validate(mincmp(float(value)) and maxcmp(float(value)), trans)
 
 
@@ -914,7 +915,6 @@ class MetadataInRangeValidator(InRangeValidator):
             except ValueError:
                 raise ValueError(f'{self.metadata_name} must be a float or an integer')
             super().validate(value_to_check, trans)
-        super().validate(True, trans)
 
 
 validator_types = dict(

From a4be8acd51c33d42e8ff6f18c494c296dee741c5 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Wed, 21 Jul 2021 20:15:37 +0200
Subject: [PATCH 15/30] ValueInDataTableColumnValidator fix, test and negate

- add possibility to negate
- add test tool (doc test seems to complicated)
- fix access to fields: was
`self.valid_values.append(fields[self._column]` needed to be `self.valid_values.append(fields[self._metadata_column])`
---
 lib/galaxy/tools/parameters/validation.py     | 24 +++++++------
 .../tools/validation_value_in_datatable.xml   | 35 +++++++++++++++++++
 2 files changed, 49 insertions(+), 10 deletions(-)
 create mode 100644 test/functional/tools/validation_value_in_datatable.xml

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 054ae2838db..648c4fc5eba 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -55,6 +55,7 @@ class Validator(abc.ABC):
         log.error("VAL value %s" % value)
         if message is None:
             message = self.message
+        # TODO allow for a placeholder in message
         log.error("validate %s %s" % (value, self.negate))
         if (not self.negate and value) or (self.negate and not value):
             return
@@ -764,10 +765,11 @@ class ValueInDataTableColumnValidator(Validator):
         line_startswith = elem.get("line_startswith", None)
         if line_startswith:
             line_startswith = line_startswith.strip()
-        return cls(tool_data_table, column, message, line_startswith)
+        negate = elem.get('negate', 'false')
+        return cls(tool_data_table, column, message, line_startswith, negate)
 
-    def __init__(self, tool_data_table, column, message="Value not found.", line_startswith=None):
-        super().__init__(message)
+    def __init__(self, tool_data_table, column, message="Value not found.", line_startswith=None, negate='false'):
+        super().__init__(message, negate)
         self.valid_values = []
         self._data_table_content_version = None
         self._tool_data_table = tool_data_table
@@ -781,17 +783,17 @@ class ValueInDataTableColumnValidator(Validator):
         self.valid_values = []
         for fields in data_fields:
             if self._column < len(fields):
-                self.valid_values.append(fields[self._metadata_column])
+                self.valid_values.append(fields[self._column])
 
     def validate(self, value, trans=None):
+        log.error(f"VALUE {value}")
+        log.error(f"valid_values {self.valid_values}")
         if not value:
             return
         if not self._tool_data_table.is_current_version(self._data_table_content_version):
             log.debug('ValueInDataTableColumnValidator: values are out of sync with data table (%s), updating validator.', self._tool_data_table.name)
             self._load_values()
-        if value in self.valid_values:
-            return
-        raise ValueError(self.message)
+        super().validate(value in self.valid_values, trans)
 
 
 class ValueNotInDataTableColumnValidator(ValueInDataTableColumnValidator):
@@ -799,12 +801,12 @@ class ValueNotInDataTableColumnValidator(ValueInDataTableColumnValidator):
     Validator that checks if a value is NOT in a tool data table column.
     """
 
-    def __init__(self, tool_data_table, metadata_column, message="Value already present.", line_startswith=None):
-        super().__init__(tool_data_table, metadata_column, message, line_startswith)
+    def __init__(self, tool_data_table, metadata_column, message="Value already present.", line_startswith=None, negate='false'):
+        super().__init__(tool_data_table, metadata_column, message, line_startswith, negate)
 
     def validate(self, value, trans=None):
         try:
-            super(ValueInDataTableColumnValidator, self).validate(value, trans)
+            super().validate(value, trans)
         except ValueError:
             return
         else:
@@ -887,6 +889,8 @@ class MetadataNotInDataTableColumnValidator(MetadataInDataTableColumnValidator):
 class MetadataInRangeValidator(InRangeValidator):
     """
     validator that ensures metadata is in a specified range
+
+    note: this is covered in a framework test (validation_metadata_in_range)
     """
     requires_dataset_metadata = True
 
diff --git a/test/functional/tools/validation_value_in_datatable.xml b/test/functional/tools/validation_value_in_datatable.xml
new file mode 100644
index 00000000000..aebf11cf50e
--- /dev/null
+++ b/test/functional/tools/validation_value_in_datatable.xml
@@ -0,0 +1,35 @@
+<tool id="validation_value_in_datatable" name="validation_value_in_datatable" profile="19.05" version="0.1">
+    <command><![CDATA[
+echo 'Hello World' > out1
+    ]]></command>
+    <inputs>
+        <!-- test value_in_data_table validator with and without negation-->
+        <param name="value" type="text">
+            <validator type="value_in_data_table" table_name="test_fasta_indexes" metadata_column="1"/>
+        </param>
+        <param name="value_neg" type="text">
+            <validator type="value_in_data_table" table_name="test_fasta_indexes" metadata_column="1" negate="true"/>
+        </param>
+        <!-- also test deprecated value_not_in_data_table validator-->
+         <param name="value_neg_deprecated" type="text">
+            <validator type="value_not_in_data_table" table_name="test_fasta_indexes" metadata_column="1"/>
+        </param>
+    </inputs>
+    <outputs>
+        <data name="out_file1" format="txt" from_work_dir="out1"/>
+    </outputs>
+    <tests>
+        <test expect_failure="false">
+            <param name="value" value="hg19"/>
+            <param name="value_neg" value="wrongvalue"/>
+            <param name="value_neg_deprecated" value="wrongvalue"/>
+        </test>
+        <test expect_failure="true">
+            <param name="value" value="wrongvalue"/>
+            <param name="value_neg" value="hg19"/>
+            <param name="value_deprecated" value="hg19"/>
+        </test>
+    </tests>
+    <help>
+    </help>
+</tool>

From 7a1dd0ae4abf39e790a9bf46595d0d0b25351c4d Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Wed, 21 Jul 2021 21:01:46 +0200
Subject: [PATCH 16/30] MetadataInFileColumnValidator,
 MetadataInDataTableColumnValidator test, negate

- add tests
- implement negate attribute
---
 lib/galaxy/tools/parameters/validation.py     | 48 ++++++++++++-------
 .../validation_dataset_metadata_in_file.xml   | 30 ++++++++++++
 .../validation_metadata_in_datatable.xml      | 36 ++++++++++++++
 .../tools/validation_value_in_datatable.xml   |  2 +-
 4 files changed, 98 insertions(+), 18 deletions(-)
 create mode 100644 test/functional/tools/validation_dataset_metadata_in_file.xml
 create mode 100644 test/functional/tools/validation_metadata_in_datatable.xml

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 648c4fc5eba..b9509ade6a3 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -50,6 +50,8 @@ class Validator(abc.ABC):
         """
         validate a value
 
+        TODO assert bool value and document how to implement in derived classes
+
         return None if positive validation, otherwise a ValueError is raised
         """
         log.error("VAL value %s" % value)
@@ -167,7 +169,7 @@ class ExpressionValidator(Validator):
 
     def __init__(self, message, expression, substitute_value_in_message, negate):
         super().__init__(message, negate)
-        # TODO
+        # TODO document substitute_value_in_message and use in all
         self.substitute_value_in_message = substitute_value_in_message
         # Save compiled expression, code objects are thread safe (right?)
         self.expression = compile(expression, '<string>', 'eval')
@@ -708,6 +710,9 @@ class EmptyTextfieldValidator(Validator):
 class MetadataInFileColumnValidator(Validator):
     """
     Validator that checks if the value for a dataset's metadata item exists in a file.
+
+    TODO deprecate
+    note: this is covered in a framework test ()
     """
     requires_dataset_metadata = True
 
@@ -725,10 +730,11 @@ class MetadataInFileColumnValidator(Validator):
         line_startswith = elem.get("line_startswith", None)
         if line_startswith:
             line_startswith = line_startswith.strip()
-        return cls(filename, metadata_name, metadata_column, message, line_startswith, split)
+        negate = elem.get('negate', 'false')
+        return cls(filename, metadata_name, metadata_column, message, line_startswith, split, negate)
 
-    def __init__(self, filename, metadata_name, metadata_column, message="Value for metadata not found.", line_startswith=None, split="\t"):
-        super().__init__(message)
+    def __init__(self, filename, metadata_name, metadata_column, message="Value for metadata not found.", line_startswith=None, split="\t", negate="false"):
+        super().__init__(message, negate)
         self.metadata_name = metadata_name
         self.valid_values = []
         for line in open(filename):
@@ -740,15 +746,14 @@ class MetadataInFileColumnValidator(Validator):
     def validate(self, value, trans=None):
         if not value:
             return
-        if hasattr(value, "metadata"):
-            if value.metadata.spec[self.metadata_name].param.to_string(value.metadata.get(self.metadata_name)) in self.valid_values:
-                return
-        raise ValueError(self.message)
+        super().validate(hasattr(value, "metadata") and value.metadata.spec[self.metadata_name].param.to_string(value.metadata.get(self.metadata_name)) in self.valid_values)
 
 
 class ValueInDataTableColumnValidator(Validator):
     """
     Validator that checks if a value is in a tool data table column.
+
+    note: this is covered in a framework test (validation_value_in_datatable)
     """
 
     @classmethod
@@ -762,6 +767,7 @@ class ValueInDataTableColumnValidator(Validator):
         except ValueError:
             pass
         message = elem.get("message", f"Value was not found in {table_name}.")
+        # TODO deprecate line_startswith .. not used in all *InDataTableColumnValidator validators
         line_startswith = elem.get("line_startswith", None)
         if line_startswith:
             line_startswith = line_startswith.strip()
@@ -799,6 +805,8 @@ class ValueInDataTableColumnValidator(Validator):
 class ValueNotInDataTableColumnValidator(ValueInDataTableColumnValidator):
     """
     Validator that checks if a value is NOT in a tool data table column.
+
+    note: this is covered in a framework test (validation_value_in_datatable)
     """
 
     def __init__(self, tool_data_table, metadata_column, message="Value already present.", line_startswith=None, negate='false'):
@@ -816,6 +824,9 @@ class ValueNotInDataTableColumnValidator(ValueInDataTableColumnValidator):
 class MetadataInDataTableColumnValidator(Validator):
     """
     Validator that checks if the value for a dataset's metadata item exists in a file.
+
+    TODO Could be derived from ValueInDataTableColumnValidator
+    note: this is covered in a framework test (validation_metadata_in_datatable)
     """
     requires_dataset_metadata = True
 
@@ -827,6 +838,7 @@ class MetadataInDataTableColumnValidator(Validator):
         metadata_name = elem.get("metadata_name", None)
         if metadata_name:
             metadata_name = metadata_name.strip()
+        # TODO rename to column
         metadata_column = elem.get("metadata_column", 0)
         try:
             metadata_column = int(metadata_column)
@@ -836,10 +848,11 @@ class MetadataInDataTableColumnValidator(Validator):
         line_startswith = elem.get("line_startswith", None)
         if line_startswith:
             line_startswith = line_startswith.strip()
-        return cls(tool_data_table, metadata_name, metadata_column, message, line_startswith)
+        negate = elem.get('negate', 'false')
+        return cls(tool_data_table, metadata_name, metadata_column, message, line_startswith, negate)
 
-    def __init__(self, tool_data_table, metadata_name, metadata_column, message="Value for metadata not found.", line_startswith=None):
-        super().__init__(message)
+    def __init__(self, tool_data_table, metadata_name, metadata_column, message="Value for metadata not found.", line_startswith=None, negate="false"):
+        super().__init__(message, negate)
         self.metadata_name = metadata_name
         self.valid_values = []
         self._data_table_content_version = None
@@ -863,23 +876,24 @@ class MetadataInDataTableColumnValidator(Validator):
             if not self._tool_data_table.is_current_version(self._data_table_content_version):
                 log.debug('MetadataInDataTableColumnValidator values are out of sync with data table (%s), updating validator.', self._tool_data_table.name)
                 self._load_values()
-            if value.metadata.spec[self.metadata_name].param.to_string(value.metadata.get(self.metadata_name)) in self.valid_values:
-                return
-        raise ValueError(self.message)
+        # TODO instead of `and` call super().validate 2x using a better error message for the case that there is no metadata
+        super().validate(hasattr(value, "metadata") and value.metadata.spec[self.metadata_name].param.to_string(value.metadata.get(self.metadata_name)) in self.valid_values, trans)
 
 
 class MetadataNotInDataTableColumnValidator(MetadataInDataTableColumnValidator):
     """
     Validator that checks if the value for a dataset's metadata item doesn't exists in a file.
+
+    note: this is covered in a framework test (validation_metadata_in_datatable)
     """
     requires_dataset_metadata = True
 
-    def __init__(self, tool_data_table, metadata_name, metadata_column, message="Value for metadata not found.", line_startswith=None):
-        super(MetadataInDataTableColumnValidator, self).__init__(tool_data_table, metadata_name, metadata_column, message, line_startswith)
+    def __init__(self, tool_data_table, metadata_name, metadata_column, message="Value for metadata not found.", line_startswith=None, negate="false"):
+        super().__init__(tool_data_table, metadata_name, metadata_column, message, line_startswith, negate)
 
     def validate(self, value, trans=None):
         try:
-            super(MetadataInDataTableColumnValidator, self).validate(value, trans)
+            super().validate(value, trans)
         except ValueError:
             return
         else:
diff --git a/test/functional/tools/validation_dataset_metadata_in_file.xml b/test/functional/tools/validation_dataset_metadata_in_file.xml
new file mode 100644
index 00000000000..e44706126cb
--- /dev/null
+++ b/test/functional/tools/validation_dataset_metadata_in_file.xml
@@ -0,0 +1,30 @@
+<tool id="validation_dataset_metadata_in_file" name="validation_dataset_metadata_in_file" profile="19.05" version="0.1">
+    <command><![CDATA[
+echo 'Hello World' > out1
+    ]]></command>
+    <inputs>
+        <!-- test dataset_metadata_in_file validator with and without negation
+             the test also uses the data table fasta_indexes.loc, but it could be any file in the tool data dir -->
+        <param name="value" type="data">
+            <validator type="dataset_metadata_in_file" filename="fasta_indexes.loc" metadata_name="dbkey" metadata_column="1"/>
+        </param>
+        <param name="value_neg" type="data">
+            <validator type="dataset_metadata_in_file" filename="fasta_indexes.loc" metadata_name="dbkey" metadata_column="1" negate="true"/>
+        </param>
+    </inputs>
+    <outputs>
+        <data name="out_file1" format="txt" from_work_dir="out1"/>
+    </outputs>
+    <tests>
+        <test expect_failure="false">
+            <param name="value" value="1.fasta" dbkey="hg19"/> <!-- hg19 is in the test data table-->
+            <param name="value_neg" value="1.fasta" dbkey="hg38"/>
+        </test>
+        <test expect_failure="true">
+            <param name="value" value="1.fasta" dbkey="hg38"/> <!-- hg19 is in the test data table-->
+            <param name="value_neg" value="1.fasta" dbkey="hg19"/>
+        </test>
+    </tests>
+    <help>
+    </help>
+</tool>
diff --git a/test/functional/tools/validation_metadata_in_datatable.xml b/test/functional/tools/validation_metadata_in_datatable.xml
new file mode 100644
index 00000000000..94fee715a46
--- /dev/null
+++ b/test/functional/tools/validation_metadata_in_datatable.xml
@@ -0,0 +1,36 @@
+<tool id="validation_metadata_in_datatable" name="validation_metadata_in_datatable" profile="21.09" version="0.1">
+    <command><![CDATA[
+echo 'Hello World' > out1
+    ]]></command>
+    <inputs>
+        <!-- test dataset_metadata_in_data_table validator with and without negation
+             we simply use dbkey as metadata field here -->
+        <param name="value" type="data">
+            <validator type="dataset_metadata_in_data_table" table_name="test_fasta_indexes" metadata_column="1" metadata_name="dbkey"/>
+        </param>
+        <param name="value_neg" type="text">
+            <validator type="value_in_data_table" table_name="test_fasta_indexes" metadata_column="1" negate="true"/>
+        </param>
+        <!-- also test deprecated dataset_metadata_not_in_data_table validator-->
+        <param name="value_neg_deprecated" type="text">
+            <validator type="dataset_metadata_not_in_data_table" table_name="test_fasta_indexes" metadata_column="1" metadata_name="dbkey"/>
+        </param>
+    </inputs>
+    <outputs>
+        <data name="out_file1" format="txt" from_work_dir="out1"/>
+    </outputs>
+    <tests>
+        <test expect_failure="false">
+            <param name="value" value="1.fasta" dbkey="hg19"/> <!-- hg19 is in the test data table-->
+            <param name="value_neg" value="1.fasta" dbkey="hg38"/>
+            <param name="value_neg_deprecated" value="1.fasta" dbkey="hg38"/>
+        </test>
+        <test expect_failure="true">
+            <param name="value" value="1.fasta" dbkey="hg38"/> <!-- hg38 is not in the test data table-->
+            <param name="value_neg" value="1.fasta" dbkey="hg19"/>
+            <param name="value_neg_deprecated" value="1.fasta" dbkey="hg19"/>
+        </test>
+    </tests>
+    <help>
+    </help>
+</tool>
diff --git a/test/functional/tools/validation_value_in_datatable.xml b/test/functional/tools/validation_value_in_datatable.xml
index aebf11cf50e..86ef401743a 100644
--- a/test/functional/tools/validation_value_in_datatable.xml
+++ b/test/functional/tools/validation_value_in_datatable.xml
@@ -1,4 +1,4 @@
-<tool id="validation_value_in_datatable" name="validation_value_in_datatable" profile="19.05" version="0.1">
+<tool id="validation_value_in_datatable" name="validation_value_in_datatable" profile="21.09" version="0.1">
     <command><![CDATA[
 echo 'Hello World' > out1
     ]]></command>

From eeb4af0328ca6363c4d8a018f90c001f12d44aad Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Wed, 21 Jul 2021 21:38:45 +0200
Subject: [PATCH 17/30] xsd: document new negate attribute of validator

---
 lib/galaxy/tool_util/xsd/galaxy.xsd | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/galaxy/tool_util/xsd/galaxy.xsd b/lib/galaxy/tool_util/xsd/galaxy.xsd
index 578e6067ef1..bfa12aadbfa 100644
--- a/lib/galaxy/tool_util/xsd/galaxy.xsd
+++ b/lib/galaxy/tool_util/xsd/galaxy.xsd
@@ -3742,6 +3742,12 @@ Valid values include: ``expression``, ``regex``, ``in_range``, ``length``,
 The error message displayed on the tool form if validation fails.</xs:documentation>
           </xs:annotation>
         </xs:attribute>
+        <xs:attribute name="negate" type="xs:boolean" default="false">
+          <xs:annotation>
+            <xs:documentation xml:lang="en">
+Negates the result of the validator.</xs:documentation>
+          </xs:annotation>
+        </xs:attribute>
         <xs:attribute name="check" type="xs:string">
           <xs:annotation>
             <xs:documentation xml:lang="en">Comma-seperated list of metadata

From 44061433d1d89456f8fbef59bbd6b6ff053b5224 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Thu, 22 Jul 2021 10:33:54 +0200
Subject: [PATCH 18/30] debug package tests

---
 packages/test.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/packages/test.sh b/packages/test.sh
index 033e1d8d202..15b4f2c5981 100755
--- a/packages/test.sh
+++ b/packages/test.sh
@@ -1,6 +1,6 @@
 #!/bin/bash
 
-set -e
+set -ex
 
 # Don't display the pip progress bar when running under CI
 [ "$CI" = 'true' ] && export PIP_PROGRESS_BAR=off

From abf7098863997e3c502d599ba302a4f0bf4d4443 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Thu, 22 Jul 2021 10:49:02 +0200
Subject: [PATCH 19/30] fix packaging test

by providing test data in the parameter module
---
 lib/galaxy/tools/parameters/test/1.tabular |  1 +
 lib/galaxy/tools/parameters/test/empty.txt |  1 +
 lib/galaxy/tools/parameters/validation.py  | 12 ++++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)
 create mode 120000 lib/galaxy/tools/parameters/test/1.tabular
 create mode 120000 lib/galaxy/tools/parameters/test/empty.txt

diff --git a/lib/galaxy/tools/parameters/test/1.tabular b/lib/galaxy/tools/parameters/test/1.tabular
new file mode 120000
index 00000000000..929b6a0e404
--- /dev/null
+++ b/lib/galaxy/tools/parameters/test/1.tabular
@@ -0,0 +1 @@
+../../../../../test-data/1.tabular
\ No newline at end of file
diff --git a/lib/galaxy/tools/parameters/test/empty.txt b/lib/galaxy/tools/parameters/test/empty.txt
new file mode 120000
index 00000000000..c37ecd1e239
--- /dev/null
+++ b/lib/galaxy/tools/parameters/test/empty.txt
@@ -0,0 +1 @@
+../../../../../test-data/empty.txt
\ No newline at end of file
diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index b9509ade6a3..557ff2599d9 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -3,6 +3,7 @@ Classes related to parameter validation.
 """
 import abc
 import logging
+import os.path
 import re
 
 
@@ -14,6 +15,13 @@ from galaxy import (
 log = logging.getLogger(__name__)
 
 
+def get_test_fname(fname):
+    """Returns test data filename"""
+    path, name = os.path.split(__file__)
+    full_path = os.path.join(path, 'test', fname)
+    return full_path
+
+
 class Validator(abc.ABC):
     """
     A validator checks that a value meets some conditions OR raises ValueError
@@ -406,9 +414,9 @@ class DatasetEmptyValidator(Validator):
     >>> sa_session.flush()
     >>> set_datatypes_registry(example_datatype_registry_for_sample())
     >>> # TODO is there a better way than hardcoding 'test-data/'
-    >>> empty_dataset = Dataset(external_filename="test-data/empty.txt")
+    >>> empty_dataset = Dataset(external_filename=get_test_fname("empty.txt"))
     >>> empty_hda = hist.add_dataset(HistoryDatasetAssociation(id=1, extension='interval', dataset=empty_dataset, sa_session=sa_session))
-    >>> full_dataset = Dataset(external_filename="test-data/1.tabular")
+    >>> full_dataset = Dataset(external_filename=get_test_fname("1.tabular"))
     >>> full_hda = hist.add_dataset(HistoryDatasetAssociation(id=2, extension='interval', dataset=full_dataset, sa_session=sa_session))
     >>>
     >>> p = ToolParameter.build(None, XML('''

From dbe81bf866cc40a3c364920c04cf608890d5a8c7 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Thu, 22 Jul 2021 11:04:24 +0200
Subject: [PATCH 20/30] assert that value is bool in Validator.validate

and add doc on how to use it
---
 lib/galaxy/tools/parameters/validation.py | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 557ff2599d9..47c4da5cd8f 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -58,10 +58,16 @@ class Validator(abc.ABC):
         """
         validate a value
 
-        TODO assert bool value and document how to implement in derived classes
+        needs to be implemented in classes derived from validator.
+        the implementation needs to call `super().validate()`
+        giving value as a bool which should be true if the
+        validation is positive and false otherwise.
+        the Validator.validate function will then negate the value
+        depending on `self.negate`
 
         return None if positive validation, otherwise a ValueError is raised
         """
+        assert isinstance(value, bool), 'value must be boolean'
         log.error("VAL value %s" % value)
         if message is None:
             message = self.message

From 1d7c11e4c0bd268024f7c82f8d3039b02712a45b Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Thu, 22 Jul 2021 11:24:58 +0200
Subject: [PATCH 21/30] deprecate some validators and an attribute

`*notin*` validtors:
- deprecate: should be done via `negate="true"`

`dataset_metadata_in_file` validator
- make deprectated (was already documented as deprecated .. also now
  more clearly documented).

line_startswith:
- removed from all *datatable validators: because it was not implemented
  and also makes no sense
- marked as deprecated since it's used only in the deprecated
  `dataset_metadata_in_file` validator
---
 lib/galaxy/tool_util/linters/inputs.py    |  4 +-
 lib/galaxy/tool_util/xsd/galaxy.xsd       | 46 +++++++++++++----------
 lib/galaxy/tools/parameters/validation.py | 41 ++++++++++----------
 3 files changed, 50 insertions(+), 41 deletions(-)

diff --git a/lib/galaxy/tool_util/linters/inputs.py b/lib/galaxy/tool_util/linters/inputs.py
index f8a843fb11a..437efe5e76f 100644
--- a/lib/galaxy/tool_util/linters/inputs.py
+++ b/lib/galaxy/tool_util/linters/inputs.py
@@ -19,12 +19,12 @@ FILTER_TYPES = [
 
 ATTRIB_VALIDATOR_COMPATIBILITY = {
     "check": ["metadata"],
-    "expression": ["regex"],
+    "expression": ["regex", "substitute_value_in_message"],
     "table_name": ["dataset_metadata_in_data_table", "dataset_metadata_not_in_data_table", "value_in_data_table", "value_not_in_data_table"],
     "filename": ["dataset_metadata_in_file"],
     "metadata_name": ["dataset_metadata_in_data_table", "dataset_metadata_not_in_data_table", "dataset_metadata_in_file"],
     "metadata_column": ["dataset_metadata_in_data_table", "dataset_metadata_not_in_data_table", "value_in_data_table", "value_not_in_data_table", "dataset_metadata_in_file options"],
-    "line_startswith": ["dataset_metadata_in_file", "dataset_metadata_in_data_table", "dataset_metadata_not_in_data_table", "value_in_data_table", "value_not_in_data_table"],
+    "line_startswith": ["dataset_metadata_in_file"],
     "min": ["in_range", "length"],
     "max": ["in_range", "length"],
     "exclude_min": ["in_range"],
diff --git a/lib/galaxy/tool_util/xsd/galaxy.xsd b/lib/galaxy/tool_util/xsd/galaxy.xsd
index bfa12aadbfa..99ecc0f647f 100644
--- a/lib/galaxy/tool_util/xsd/galaxy.xsd
+++ b/lib/galaxy/tool_util/xsd/galaxy.xsd
@@ -3643,10 +3643,13 @@ parameters a ``metadata`` validator is added automatically.
 - ``dataset_ok_validator``: Check if the data set is in state OK.
 - ``dataset_metadata_in_range``: Check if a numeric metadata value is within
 a given range.
-- ``dataset_metadata_in_file``: Check if a metadata value is contained in a
-specific column of another data set.
-- ``dataset_metadata_in_data_table`` (``dataset_metadata_not_in_data_table``):
-Check if a metadata value is contained in a column of a data table.
+- ``dataset_metadata_in_data_table``: Check if a metadata value is contained in a column of a data table.
+
+Deprecated data validators:
+
+- ``dataset_metadata_in_file``: Use data tables with ``dataset_metadata_in_data_table``.
+Check if a metadata value is contained in a specific column of another data set.
+- ``dataset_metadata_not_in_data_table``: Use ``dataset_metadata_in_data_table`` with ``negate="true"``.
 
 ### Validators for textual inputs (``text``, ``select``, ...)
 
@@ -3666,9 +3669,13 @@ For ``text`` inputs the following validators are useful:
 
 - ``length``: Check if the length of the value is within a range.
 - ``empty_field``: Check if the string is not empty
-- ``value_in_data_table`` (``value_not_in_data_table``): Check if the value is
+- ``value_in_data_table``: Check if the value is
 contained in a column of a given data table.
 
+Deprecated:
+
+- ``value_not_in_data_table``: Use ``value_in_data_table`` with ``negate="true"``.
+
 ### Validators for numeric inputs (``integer``, ``float``)
 
 ``in_range``: Check if the value is in a given range.
@@ -3725,15 +3732,16 @@ use in filenames may not contain ``..``.
       <xs:extension base="xs:string">
         <xs:attribute name="type" type="ValidatorType" use="required">
           <xs:annotation>
-            <xs:documentation xml:lang="en"><![CDATA[The list of supported
+            <xs:documentation xml:lang="en"><![CDATA[
+Valid values are: ``expression``, ``regex``, ``in_range``, ``length``,
+``metadata``, ``unspecified_build``, ``no_options``, ``empty_field``,
+``dataset_metadata_in_data_table``, ``value_in_data_table``,
+``dataset_ok_validator``, ``dataset_metadata_in_range``.
+Deprecated validators: ``dataset_metadata_in_file``, ``value_not_in_data_table``, ``dataset_metadata_not_in_data_table``.
+The list of supported
 validators is in the ``validator_types`` dictionary in
 [/lib/galaxy/tools/parameters/validation.py](https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/tools/parameters/validation.py).
-Valid values include: ``expression``, ``regex``, ``in_range``, ``length``,
-``metadata``, ``unspecified_build``, ``no_options``, ``empty_field``,
-``dataset_metadata_in_file``,
-``dataset_metadata_in_data_table``, ``dataset_metadata_not_in_data_table``,
-``value_in_data_table``, ``value_not_in_data_table``,
-``dataset_ok_validator``, ``dataset_metadata_in_range``]]></xs:documentation>
+]]></xs:documentation>
           </xs:annotation>
         </xs:attribute>
         <xs:attribute name="message" type="xs:string">
@@ -3785,13 +3793,6 @@ in ``dataset_metadata_in_data_table``, ``dataset_metadata_not_in_data_table``, `
 This can be an integer index to the column or a column name.</xs:documentation>
           </xs:annotation>
         </xs:attribute>
-        <xs:attribute name="line_startswith" type="xs:string">
-          <xs:annotation>
-            <xs:documentation xml:lang="en">Used to indicate lines in the file
-being used for validation start with a this attribute value.
-For use with validators of type ``dataset_metadata_in_file``, ``dataset_metadata_in_data_table``, ``dataset_metadata_not_in_data_table``, ``value_in_data_table``, ``value_not_in_data_tabl``</xs:documentation>
-          </xs:annotation>
-        </xs:attribute>
         <xs:attribute name="min" type="xs:decimal">
           <xs:annotation>
             <xs:documentation xml:lang="en">When the ``type`` attribute value is
@@ -3831,6 +3832,13 @@ fields to skip if type is ``metadata``. If not specified, all non-optional
 metadata fields will be checked unless ``check`` attribute is specified.</xs:documentation>
           </xs:annotation>
         </xs:attribute>
+        <xs:attribute name="line_startswith" type="xs:string">
+          <xs:annotation>
+            <xs:documentation xml:lang="en">Deprecated. Used to indicate lines in the file
+being used for validation start with a this attribute value.
+For use with validator ``dataset_metadata_in_file``</xs:documentation>
+          </xs:annotation>
+        </xs:attribute>
       </xs:extension>
     </xs:simpleContent>
   </xs:complexType>
diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 47c4da5cd8f..6a18343b016 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -725,8 +725,9 @@ class MetadataInFileColumnValidator(Validator):
     """
     Validator that checks if the value for a dataset's metadata item exists in a file.
 
-    TODO deprecate
-    note: this is covered in a framework test ()
+    Deprecated: DataTables are now the preferred way.
+
+    note: this is covered in a framework test (validation_dataset_metadata_in_file)
     """
     requires_dataset_metadata = True
 
@@ -781,14 +782,10 @@ class ValueInDataTableColumnValidator(Validator):
         except ValueError:
             pass
         message = elem.get("message", f"Value was not found in {table_name}.")
-        # TODO deprecate line_startswith .. not used in all *InDataTableColumnValidator validators
-        line_startswith = elem.get("line_startswith", None)
-        if line_startswith:
-            line_startswith = line_startswith.strip()
         negate = elem.get('negate', 'false')
-        return cls(tool_data_table, column, message, line_startswith, negate)
+        return cls(tool_data_table, column, message, negate)
 
-    def __init__(self, tool_data_table, column, message="Value not found.", line_startswith=None, negate='false'):
+    def __init__(self, tool_data_table, column, message="Value not found.", negate='false'):
         super().__init__(message, negate)
         self.valid_values = []
         self._data_table_content_version = None
@@ -820,11 +817,13 @@ class ValueNotInDataTableColumnValidator(ValueInDataTableColumnValidator):
     """
     Validator that checks if a value is NOT in a tool data table column.
 
+    Deprecated: use now the `negate` attribute
+
     note: this is covered in a framework test (validation_value_in_datatable)
     """
 
-    def __init__(self, tool_data_table, metadata_column, message="Value already present.", line_startswith=None, negate='false'):
-        super().__init__(tool_data_table, metadata_column, message, line_startswith, negate)
+    def __init__(self, tool_data_table, metadata_column, message="Value already present.", negate='false'):
+        super().__init__(tool_data_table, metadata_column, message, negate)
 
     def validate(self, value, trans=None):
         try:
@@ -859,13 +858,10 @@ class MetadataInDataTableColumnValidator(Validator):
         except ValueError:
             pass
         message = elem.get("message", f"Value for metadata {metadata_name} was not found in {table_name}.")
-        line_startswith = elem.get("line_startswith", None)
-        if line_startswith:
-            line_startswith = line_startswith.strip()
         negate = elem.get('negate', 'false')
-        return cls(tool_data_table, metadata_name, metadata_column, message, line_startswith, negate)
+        return cls(tool_data_table, metadata_name, metadata_column, message, negate)
 
-    def __init__(self, tool_data_table, metadata_name, metadata_column, message="Value for metadata not found.", line_startswith=None, negate="false"):
+    def __init__(self, tool_data_table, metadata_name, metadata_column, message="Value for metadata not found.", negate="false"):
         super().__init__(message, negate)
         self.metadata_name = metadata_name
         self.valid_values = []
@@ -898,12 +894,14 @@ class MetadataNotInDataTableColumnValidator(MetadataInDataTableColumnValidator):
     """
     Validator that checks if the value for a dataset's metadata item doesn't exists in a file.
 
+    Deprecated: use now the `negate` attribute
+
     note: this is covered in a framework test (validation_metadata_in_datatable)
     """
     requires_dataset_metadata = True
 
-    def __init__(self, tool_data_table, metadata_name, metadata_column, message="Value for metadata not found.", line_startswith=None, negate="false"):
-        super().__init__(tool_data_table, metadata_name, metadata_column, message, line_startswith, negate)
+    def __init__(self, tool_data_table, metadata_name, metadata_column, message="Value for metadata not found.", negate="false"):
+        super().__init__(tool_data_table, metadata_name, metadata_column, message, negate)
 
     def validate(self, value, trans=None):
         try:
@@ -960,14 +958,17 @@ validator_types = dict(
     empty_field=EmptyTextfieldValidator,
     empty_dataset=DatasetEmptyValidator,
     empty_extra_files_path=DatasetExtraFilesPathEmptyValidator,
-    dataset_metadata_in_file=MetadataInFileColumnValidator,
     dataset_metadata_in_data_table=MetadataInDataTableColumnValidator,
-    dataset_metadata_not_in_data_table=MetadataNotInDataTableColumnValidator,
     dataset_metadata_in_range=MetadataInRangeValidator,
     value_in_data_table=ValueInDataTableColumnValidator,
-    value_not_in_data_table=ValueNotInDataTableColumnValidator,
     dataset_ok_validator=DatasetOkValidator,
 )
+deprecated_validator_types = dict(
+    dataset_metadata_in_file=MetadataInFileColumnValidator,
+    dataset_metadata_not_in_data_table=MetadataNotInDataTableColumnValidator,
+    value_not_in_data_table=ValueNotInDataTableColumnValidator,
+)
+validator_types.update(deprecated_validator_types)
 
 
 def get_suite():

From a0046e43168c238c9b87b2a996e1f8c06503131a Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Thu, 22 Jul 2021 19:53:08 +0200
Subject: [PATCH 22/30] improved default messages from validators

- improved default messages
- allways substitute %s by value, deprecate `substitute_by_value`
- add functional tool tests to the tool_conf file (forgotten before)
  and small fixes
---
 lib/galaxy/tool_util/xsd/galaxy.xsd           |   7 +-
 lib/galaxy/tools/parameters/validation.py     | 170 ++++++++++--------
 test/functional/tools/samples_tool_conf.xml   |   3 +
 .../validation_dataset_metadata_in_file.xml   |  20 ++-
 .../validation_metadata_in_datatable.xml      |  20 ++-
 5 files changed, 129 insertions(+), 91 deletions(-)

diff --git a/lib/galaxy/tool_util/xsd/galaxy.xsd b/lib/galaxy/tool_util/xsd/galaxy.xsd
index 99ecc0f647f..63b8b78c99b 100644
--- a/lib/galaxy/tool_util/xsd/galaxy.xsd
+++ b/lib/galaxy/tool_util/xsd/galaxy.xsd
@@ -3747,7 +3747,7 @@ validators is in the ``validator_types`` dictionary in
         <xs:attribute name="message" type="xs:string">
           <xs:annotation>
             <xs:documentation xml:lang="en">
-The error message displayed on the tool form if validation fails.</xs:documentation>
+The error message displayed on the tool form if validation fails. A placeholder string ``%s`` will be repaced by the ``value``</xs:documentation>
           </xs:annotation>
         </xs:attribute>
         <xs:attribute name="negate" type="xs:boolean" default="false">
@@ -3839,6 +3839,11 @@ being used for validation start with a this attribute value.
 For use with validator ``dataset_metadata_in_file``</xs:documentation>
           </xs:annotation>
         </xs:attribute>
+        <xs:attribute name="substitute_value_in_message" type="xs:string">
+          <xs:annotation>
+            <xs:documentation xml:lang="en">Deprecated. This is now always done.</xs:documentation>
+          </xs:annotation>
+        </xs:attribute>
       </xs:extension>
     </xs:simpleContent>
   </xs:complexType>
diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 6a18343b016..6f94b1304af 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -54,31 +54,34 @@ class Validator(abc.ABC):
         super().__init__()
 
     @abc.abstractmethod
-    def validate(self, value, trans=None, message=None):
+    def validate(self, value, trans=None, message=None, value_to_show=None):
         """
         validate a value
 
         needs to be implemented in classes derived from validator.
         the implementation needs to call `super().validate()`
-        giving value as a bool which should be true if the
-        validation is positive and false otherwise.
+        giving result as a bool (which should be true if the
+        validation is positive and false otherwise) and the value
+        that is validated.
+
         the Validator.validate function will then negate the value
-        depending on `self.negate`
+        depending on `self.negate` and return None if
+        - value is True and negate is False
+        - value is False and negate is True
+        and raise a ValueError otherwise.
 
         return None if positive validation, otherwise a ValueError is raised
         """
         assert isinstance(value, bool), 'value must be boolean'
-        log.error("VAL value %s" % value)
         if message is None:
             message = self.message
-        # TODO allow for a placeholder in message
-        log.error("validate %s %s" % (value, self.negate))
+        if value_to_show and "%s" in message:
+            message = message % value_to_show
         if (not self.negate and value) or (self.negate and not value):
             return
         else:
             # TODO message often makes not sense if negate=True
             raise ValueError(message)
-        pass
 
 
 class RegexValidator(Validator):
@@ -89,7 +92,7 @@ class RegexValidator(Validator):
     >>> from galaxy.tools.parameters.basic import ToolParameter
     >>> p = ToolParameter.build(None, XML('''
     ... <param name="blah" type="text" value="10">
-    ...     <validator type="regex" message="Not gonna happen">[Ff]oo</validator>
+    ...     <validator type="regex">[Ff]oo</validator>
     ... </param>
     ... '''))
     >>> t = p.validate("Foo")
@@ -97,39 +100,41 @@ class RegexValidator(Validator):
     >>> t = p.validate("Fop")
     Traceback (most recent call last):
         ...
-    ValueError: Not gonna happen
+    ValueError: Value 'Fop' does not match regular expression '[Ff]oo'
     >>> t = p.validate(["Foo", "foo"])
     >>> t = p.validate(["Foo", "Fop"])
     Traceback (most recent call last):
         ...
-    ValueError: Not gonna happen
+    ValueError: Value 'Fop' does not match regular expression '[Ff]oo'
     >>>
     >>> p = ToolParameter.build(None, XML('''
     ... <param name="blah" type="text" value="10">
-    ...     <validator type="regex" message="Not gonna happen" negate="true">[Ff]oo</validator>
+    ...     <validator type="regex" negate="true">[Ff]oo</validator>
     ... </param>
     ... '''))
     >>> t = p.validate("Foo")
     Traceback (most recent call last):
         ...
-    ValueError: Not gonna happen
+    ValueError: Value 'Foo' does match regular expression '[Ff]oo'
     >>> t = p.validate("foo")
     Traceback (most recent call last):
         ...
-    ValueError: Not gonna happen
+    ValueError: Value 'foo' does match regular expression '[Ff]oo'
     >>> t = p.validate("Fop")
-    >>> t = p.validate(["Foo", "foo"])
+    >>> t = p.validate(["Fop", "foo"])
     Traceback (most recent call last):
         ...
-    ValueError: Not gonna happen
+    ValueError: Value 'foo' does match regular expression '[Ff]oo'
     >>> t = p.validate(["Fop", "Fop"])
     """
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message'), elem.text, elem.get('negate', 'false'))
+        return cls(elem.get('message', None), elem.text, elem.get('negate', 'false'))
 
     def __init__(self, message, expression, negate):
+        if message is None:
+            message = f"Value '%s' does {'not ' if negate == 'false' else ''}match regular expression '{expression}'"
         super().__init__(message, negate)
         # Compile later. RE objects used to not be thread safe. Not sure about
         # the sre module.
@@ -140,7 +145,7 @@ class RegexValidator(Validator):
             value = [value]
         for val in value:
             match = re.match(self.expression, val or '')
-            super().validate(match is not None, trans)
+            super().validate(match is not None, value_to_show=val)
 
 
 class ExpressionValidator(Validator):
@@ -179,28 +184,22 @@ class ExpressionValidator(Validator):
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message'), elem.text, elem.get('substitute_value_in_message'), elem.get('negate', 'false'))
+        return cls(elem.get('message', None), elem.text, elem.get('negate', 'false'))
 
-    def __init__(self, message, expression, substitute_value_in_message, negate):
+    def __init__(self, message, expression, negate):
+        if message is None:
+            message = f"Value '%s' does not evaluate to {'True' if negate == 'false' else 'False'} for '{expression}'"
         super().__init__(message, negate)
-        # TODO document substitute_value_in_message and use in all
-        self.substitute_value_in_message = substitute_value_in_message
         # Save compiled expression, code objects are thread safe (right?)
         self.expression = compile(expression, '<string>', 'eval')
 
     def validate(self, value, trans=None):
-        message = self.message
-        if self.substitute_value_in_message:
-            message = message % value
         try:
             evalresult = eval(self.expression, dict(value=value))
         except Exception:
-            log.debug(f"Validator {self.expression} could not be evaluated on {str(value)}", exc_info=True)
-            super().validate(False, trans, message)
-        if not(evalresult):
-            super().validate(False, trans, message)
-        else:
-            super().validate(True, trans, message)
+            log.debug(f"Validator '{self.expression}' could not be evaluated on '{str(value)}'", exc_info=True)
+            super().validate(False, value, f"Validator '{self.expression}' could not be evaluated on '%s'")
+        super().validate(evalresult, value_to_show=value)
 
 
 # TODO This could be a subclass of ExpressionValidator
@@ -228,18 +227,18 @@ class InRangeValidator(Validator):
     >>>
     >>> p = ToolParameter.build(None, XML('''
     ... <param name="blah" type="integer" value="10">
-    ...     <validator type="in_range" message="Not gonna happen" min="10" exclude_min="true" max="20" negate="true"/>
+    ...     <validator type="in_range" min="10" exclude_min="true" max="20" negate="true"/>
     ... </param>
     ... '''))
     >>> t = p.validate(10)
     >>> t = p.validate(15)
     Traceback (most recent call last):
         ...
-    ValueError: Not gonna happen
+    ValueError: Value ('15') must not fulfill value > 10 and value <= 20
     >>> t = p.validate(20)
     Traceback (most recent call last):
         ...
-    ValueError: Not gonna happen
+    ValueError: Value ('20') must not fulfill value > 10 and value <= 20
     >>> t = p.validate(21)
     """
 
@@ -272,7 +271,9 @@ class InRangeValidator(Validator):
             op1 = '>'
         if self.exclude_max:
             op2 = '<'
-        super().__init__(message or f"Value must be {op1} {self_min_str} and {op2} {self_max_str}", negate)
+        if message is None:
+            message = f"Value ('%s') must {'not ' if negate == 'true' else ''}fulfill value {op1} {self_min_str} and value {op2} {self_max_str}"
+        super().__init__(message, negate)
 
     def validate(self, value, trans=None):
         if self.exclude_min:
@@ -283,8 +284,7 @@ class InRangeValidator(Validator):
             maxcmp = self.max.__gt__
         else:
             maxcmp = self.max.__ge__
-        log.error(f"{value} min {mincmp(float(value))} max {maxcmp(float(value))}")
-        super().validate(mincmp(float(value)) and maxcmp(float(value)), trans)
+        super().validate(mincmp(float(value)) and maxcmp(float(value)), value_to_show=value)
 
 
 # TODO This could be a subclass of InRangeValidator
@@ -318,11 +318,11 @@ class LengthValidator(Validator):
     >>> t = p.validate("foo")
     Traceback (most recent call last):
         ...
-    ValueError: Must have length of at least 2 and at most 8
+    ValueError: Must not have length of at least 2 and at most 8
     >>> t = p.validate("bar")
     Traceback (most recent call last):
         ...
-    ValueError: Must have length of at least 2 and at most 8
+    ValueError: Must not have length of at least 2 and at most 8
     >>> t = p.validate("f")
     >>> t = p.validate("foobarbaz")
     """
@@ -332,7 +332,6 @@ class LengthValidator(Validator):
         return cls(elem.get('message', None), elem.get('min', None), elem.get('max', None), elem.get('negate', 'false'))
 
     def __init__(self, message, length_min, length_max, negate):
-        super().__init__(message, negate)
         if length_min is not None:
             self.min = int(length_min)
         else:
@@ -341,9 +340,12 @@ class LengthValidator(Validator):
             self.max = int(length_max)
         else:
             self.max = float('inf')
+        if message is None:
+            message = f"Must {'not ' if negate == 'true' else ''}have length of at least {self.min} and at most {self.max}"
+        super().__init__(message, negate)
 
     def validate(self, value, trans=None):
-        super().validate(self.min <= len(value) <= self.max, trans, self.message or ("Must have length of at least %d and at most %s" % (self.min, self.max)))
+        super().validate(self.min <= len(value) <= self.max, trans, value_to_show=value)
 
 
 class DatasetOkValidator(Validator):
@@ -388,19 +390,24 @@ class DatasetOkValidator(Validator):
     >>> t = p.validate(ok_hda)
     Traceback (most recent call last):
         ...
-    ValueError: The selected dataset is still being generated, select another dataset or wait until it is completed
+    ValueError: The selected dataset must not be in state OK
     >>> t = p.validate(notok_hda)
     """
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message', None), elem.get('negate', 'false'))
+        negate = elem.get('negate', 'false')
+        message = elem.get('message', None)
+        if message is None:
+            if negate == 'false':
+                message = "The selected dataset is still being generated, select another dataset or wait until it is completed"
+            else:
+                message = "The selected dataset must not be in state OK"
+        return cls(message, negate)
 
     def validate(self, value, trans=None):
         if value:
             # TODO all Dataset Validators should be able to handle lists, or?
-            if self.message is None:
-                self.message = "The selected dataset is still being generated, select another dataset or wait until it is completed"
             super().validate(value.state == model.Dataset.states.OK)
 
 
@@ -444,13 +451,17 @@ class DatasetEmptyValidator(Validator):
     >>> t = p.validate(full_hda)
     Traceback (most recent call last):
         ...
-    ValueError: The selected dataset is empty, this tool expects non-empty files.
+    ValueError: The selected dataset is non-empty, this tool expects empty files.
     >>> t = p.validate(empty_hda)
     """
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message', "The selected dataset is empty, this tool expects non-empty files."), elem.get('negate', 'false'))
+        message = elem.get('message', None)
+        negate = elem.get('negate', 'false')
+        if not message:
+            message = f"The selected dataset is {'non-' if negate == 'true' else ''}empty, this tool expects {'non-' if negate=='false' else ''}empty files."
+        return cls(message, negate)
 
     def validate(self, value, trans=None):
         if value:
@@ -498,13 +509,17 @@ class DatasetExtraFilesPathEmptyValidator(Validator):
     >>> t = p.validate(has_extra_hda)
     Traceback (most recent call last):
         ...
-    ValueError: The selected dataset's extra_files_path directory is empty or does not exist, this tool expects non-empty extra_files_path directories associated with the selected input.
+    ValueError: The selected dataset's extra_files_path directory is non-empty or does exist, this tool expects empty extra_files_path directories associated with the selected input.
     >>> t = p.validate(has_no_extra_hda)
     """
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message', "The selected dataset's extra_files_path directory is empty or does not exist, this tool expects non-empty extra_files_path directories associated with the selected input."), elem.get('negate', 'false'))
+        message = elem.get('message', None)
+        negate = elem.get('negate', 'false')
+        if not message:
+            message = f"The selected dataset's extra_files_path directory is {'non-' if negate == 'true' else ''}empty or does {'not ' if negate == 'false' else ''}exist, this tool expects {'non-' if negate == 'false' else ''}empty extra_files_path directories associated with the selected input."
+        return cls(message, negate)
 
     def validate(self, value, trans=None):
         if value:
@@ -564,7 +579,11 @@ class MetadataValidator(Validator):
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(message=elem.get('message', "Metadata missing, click the pencil icon in the history item to edit / save the metadata attributes"),
+        message = elem.get('message', None)
+        if not message:
+            # TODO message not useful for negate="true" .. but maybe OK since the validator itself is not useful then
+            message = "Metadata missing, click the pencil icon in the history item to edit / save the metadata attributes"
+        return cls(message=message,
                    check=elem.get('check', ""),
                    skip=elem.get('skip', ""),
                    negate=elem.get('negate', 'false'))
@@ -575,7 +594,6 @@ class MetadataValidator(Validator):
         self.skip = skip.split(",")
 
     def validate(self, value, trans=None):
-        log.error("VAL value %s" % value)
         if value:
             # TODO why this validator checks for isinstance(value, model.DatasetInstance)
             super().validate(isinstance(value, model.DatasetInstance) and not value.missing_meta(check=self.check, skip=self.skip))
@@ -621,15 +639,18 @@ class UnspecifiedBuildValidator(Validator):
     >>> t = p.validate(has_dbkey_hda)
     Traceback (most recent call last):
         ...
-    ValueError: Unspecified genome build, click the pencil icon in the history item to set the genome build
+    ValueError: Specified genome build, click the pencil icon in the history item to remove the genome build
     >>> t = p.validate(has_no_dbkey_hda)
     """
     requires_dataset_metadata = True
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message', "Unspecified genome build, click the pencil icon in the history item to set the genome build"),
-                   elem.get('negate', 'false'))
+        message = elem.get('message', None)
+        negate = elem.get('negate', 'false')
+        if not message:
+            message = f"{'Unspecified' if negate == 'false' else 'Specified'} genome build, click the pencil icon in the history item to {'set' if negate == 'false' else 'remove'} the genome build"
+        return cls(message, negate)
 
     def validate(self, value, trans=None):
         # if value is None, we cannot validate
@@ -667,13 +688,17 @@ class NoOptionsValidator(Validator):
     >>> t = p.validate('foo')
     Traceback (most recent call last):
         ...
-    ValueError: No options available for selection
+    ValueError: Options available for selection
     >>> t = p.validate(None)
     """
 
     @classmethod
     def from_element(cls, param, elem):
-        return cls(elem.get('message', "No options available for selection"), elem.get('negate', 'false'))
+        message = elem.get('message', None)
+        negate = elem.get('negate', 'false')
+        if not message:
+            message = f"{'No options' if negate == 'false' else 'Options'} available for selection"
+        return cls(message, negate)
 
     def validate(self, value, trans=None):
         super().validate(value is not None)
@@ -708,16 +733,16 @@ class EmptyTextfieldValidator(Validator):
 
     @classmethod
     def from_element(cls, param, elem):
+        message = elem.get('message', None)
         negate = elem.get('negate', 'false')
-        if negate == 'false':
-            message = elem.get('message', "Field requires a value")
-        else:
-            message = elem.get('message', "Field must not set a value")
+        if not message:
+            if negate == 'false':
+                message = elem.get('message', "Field requires a value")
+            else:
+                message = elem.get('message', "Field must not set a value")
         return cls(message, negate)
 
     def validate(self, value, trans=None):
-        if self.message is None:
-            self.message = ""
         super().validate(value != '')
 
 
@@ -751,12 +776,12 @@ class MetadataInFileColumnValidator(Validator):
     def __init__(self, filename, metadata_name, metadata_column, message="Value for metadata not found.", line_startswith=None, split="\t", negate="false"):
         super().__init__(message, negate)
         self.metadata_name = metadata_name
-        self.valid_values = []
+        self.valid_values = set()
         for line in open(filename):
             if line_startswith is None or line.startswith(line_startswith):
                 fields = line.split(split)
                 if metadata_column < len(fields):
-                    self.valid_values.append(fields[metadata_column].strip())
+                    self.valid_values.add(fields[metadata_column].strip())
 
     def validate(self, value, trans=None):
         if not value:
@@ -803,14 +828,12 @@ class ValueInDataTableColumnValidator(Validator):
                 self.valid_values.append(fields[self._column])
 
     def validate(self, value, trans=None):
-        log.error(f"VALUE {value}")
-        log.error(f"valid_values {self.valid_values}")
         if not value:
             return
         if not self._tool_data_table.is_current_version(self._data_table_content_version):
             log.debug('ValueInDataTableColumnValidator: values are out of sync with data table (%s), updating validator.', self._tool_data_table.name)
             self._load_values()
-        super().validate(value in self.valid_values, trans)
+        super().validate(value in self.valid_values)
 
 
 class ValueNotInDataTableColumnValidator(ValueInDataTableColumnValidator):
@@ -827,7 +850,7 @@ class ValueNotInDataTableColumnValidator(ValueInDataTableColumnValidator):
 
     def validate(self, value, trans=None):
         try:
-            super().validate(value, trans)
+            super().validate(value)
         except ValueError:
             return
         else:
@@ -925,10 +948,12 @@ class MetadataInRangeValidator(InRangeValidator):
         metadata_name = elem.get('metadata_name', None)
         assert metadata_name, "dataset_metadata_in_range validator requires metadata_name attribute."
         metadata_name = metadata_name.strip()
-        return cls(metadata_name, elem.get('message', None),
-                   elem.get('min'), elem.get('max'),
-                   elem.get('exclude_min', 'false'), elem.get('exclude_max', 'false'),
-                   elem.get('negate', 'false'))
+        ret = cls(metadata_name, elem.get('message', None),
+                  elem.get('min'), elem.get('max'),
+                  elem.get('exclude_min', 'false'), elem.get('exclude_max', 'false'),
+                  elem.get('negate', 'false'))
+        ret.message = "Metadata: " + ret.message
+        return ret
 
     def __init__(self, metadata_name, message, range_min, range_max, exclude_min, exclude_max, negate):
         self.metadata_name = metadata_name
@@ -963,6 +988,7 @@ validator_types = dict(
     value_in_data_table=ValueInDataTableColumnValidator,
     dataset_ok_validator=DatasetOkValidator,
 )
+
 deprecated_validator_types = dict(
     dataset_metadata_in_file=MetadataInFileColumnValidator,
     dataset_metadata_not_in_data_table=MetadataNotInDataTableColumnValidator,
diff --git a/test/functional/tools/samples_tool_conf.xml b/test/functional/tools/samples_tool_conf.xml
index 4c27f1a0c8c..6baa4754863 100644
--- a/test/functional/tools/samples_tool_conf.xml
+++ b/test/functional/tools/samples_tool_conf.xml
@@ -135,6 +135,9 @@
   <tool file="validation_sanitizer.xml" />
   <tool file="validation_repeat.xml" />
   <tool file="validation_metadata_in_range.xml"/>
+  <tool file="validation_metadata_in_datatable.xml"/>
+  <tool file="validation_metadata_in_file.xml"/>
+  <tool file="validation_value_in_datatable.xml"/>
   <tool file="empty_output.xml" />
   <tool file="validation_empty_dataset.xml" />
   <tool file="implicit_conversion.xml" />
diff --git a/test/functional/tools/validation_dataset_metadata_in_file.xml b/test/functional/tools/validation_dataset_metadata_in_file.xml
index e44706126cb..14a27627d52 100644
--- a/test/functional/tools/validation_dataset_metadata_in_file.xml
+++ b/test/functional/tools/validation_dataset_metadata_in_file.xml
@@ -1,28 +1,30 @@
-<tool id="validation_dataset_metadata_in_file" name="validation_dataset_metadata_in_file" profile="19.05" version="0.1">
+<tool id="validation_dataset_metadata_in_file" name="validation_dataset_metadata_in_file" profile="21.09" version="0.1">
     <command><![CDATA[
 echo 'Hello World' > out1
     ]]></command>
     <inputs>
         <!-- test dataset_metadata_in_file validator with and without negation
              the test also uses the data table fasta_indexes.loc, but it could be any file in the tool data dir -->
-        <param name="value" type="data">
+        <param name="value" type="data" format="data">
             <validator type="dataset_metadata_in_file" filename="fasta_indexes.loc" metadata_name="dbkey" metadata_column="1"/>
         </param>
-        <param name="value_neg" type="data">
-            <validator type="dataset_metadata_in_file" filename="fasta_indexes.loc" metadata_name="dbkey" metadata_column="1" negate="true"/>
+        <param name="value_neg" type="data" format="data">
+            <validator type="dataset_metadata_in_file" filename="fasta_indexes.loc" line_startswith="h" metadata_name="dbkey" metadata_column="1" negate="true"/>
         </param>
     </inputs>
     <outputs>
         <data name="out_file1" format="txt" from_work_dir="out1"/>
     </outputs>
     <tests>
+        <!-- hg19 is in the file and hg38 isn't -->
+        <!-- TODO These tests fail if the same test data is used .. no idea why -->
         <test expect_failure="false">
-            <param name="value" value="1.fasta" dbkey="hg19"/> <!-- hg19 is in the test data table-->
-            <param name="value_neg" value="1.fasta" dbkey="hg38"/>
-        </test>
+            <param name="value" value="1.fasta" dbkey="hg19"/> 
+            <param name="value_neg" value="2.fasta" dbkey="hg38"/>
+           </test>
         <test expect_failure="true">
-            <param name="value" value="1.fasta" dbkey="hg38"/> <!-- hg19 is in the test data table-->
-            <param name="value_neg" value="1.fasta" dbkey="hg19"/>
+            <param name="value" value="1.fasta" dbkey="hg38"/>
+            <param name="value_neg" value="2.fasta" dbkey="hg19"/>
         </test>
     </tests>
     <help>
diff --git a/test/functional/tools/validation_metadata_in_datatable.xml b/test/functional/tools/validation_metadata_in_datatable.xml
index 94fee715a46..421a1850c8d 100644
--- a/test/functional/tools/validation_metadata_in_datatable.xml
+++ b/test/functional/tools/validation_metadata_in_datatable.xml
@@ -5,14 +5,14 @@ echo 'Hello World' > out1
     <inputs>
         <!-- test dataset_metadata_in_data_table validator with and without negation
              we simply use dbkey as metadata field here -->
-        <param name="value" type="data">
+        <param name="value" type="data" format="data">
             <validator type="dataset_metadata_in_data_table" table_name="test_fasta_indexes" metadata_column="1" metadata_name="dbkey"/>
         </param>
-        <param name="value_neg" type="text">
-            <validator type="value_in_data_table" table_name="test_fasta_indexes" metadata_column="1" negate="true"/>
+        <param name="value_neg" type="data" format="data">
+            <validator type="dataset_metadata_in_data_table" table_name="test_fasta_indexes" metadata_column="1" metadata_name="dbkey" negate="true"/>
         </param>
         <!-- also test deprecated dataset_metadata_not_in_data_table validator-->
-        <param name="value_neg_deprecated" type="text">
+        <param name="value_neg_deprecated" type="data" format="data">
             <validator type="dataset_metadata_not_in_data_table" table_name="test_fasta_indexes" metadata_column="1" metadata_name="dbkey"/>
         </param>
     </inputs>
@@ -20,15 +20,17 @@ echo 'Hello World' > out1
         <data name="out_file1" format="txt" from_work_dir="out1"/>
     </outputs>
     <tests>
+        <!-- hg19 is in the test data table and hg38 not-->
+        <!-- TODO These tests fail if the same test data is used .. no idea why -->
         <test expect_failure="false">
-            <param name="value" value="1.fasta" dbkey="hg19"/> <!-- hg19 is in the test data table-->
-            <param name="value_neg" value="1.fasta" dbkey="hg38"/>
-            <param name="value_neg_deprecated" value="1.fasta" dbkey="hg38"/>
+            <param name="value" value="1.fasta" dbkey="hg19"/> 
+            <param name="value_neg" value="2.fasta" dbkey="hg38"/>
+            <param name="value_neg_deprecated" value="3.bed" dbkey="hg38"/>
         </test>
         <test expect_failure="true">
             <param name="value" value="1.fasta" dbkey="hg38"/> <!-- hg38 is not in the test data table-->
-            <param name="value_neg" value="1.fasta" dbkey="hg19"/>
-            <param name="value_neg_deprecated" value="1.fasta" dbkey="hg19"/>
+            <param name="value_neg" value="2.fasta" dbkey="hg19"/>
+            <param name="value_neg_deprecated" value="3.bed" dbkey="hg19"/>
         </test>
     </tests>
     <help>

From 246fe4144d7179c9485ed5c1271d0c027d78bffb Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Wed, 28 Jul 2021 12:11:41 +0200
Subject: [PATCH 23/30] make LengthValidator subclass of InRangeValidator

---
 lib/galaxy/tools/parameters/validation.py | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 6f94b1304af..27fc260a4d1 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -287,8 +287,7 @@ class InRangeValidator(Validator):
         super().validate(mincmp(float(value)) and maxcmp(float(value)), value_to_show=value)
 
 
-# TODO This could be a subclass of InRangeValidator
-class LengthValidator(Validator):
+class LengthValidator(InRangeValidator):
     """
     Validator that ensures the length of the provided string (value) is in a specific range
 
@@ -332,20 +331,12 @@ class LengthValidator(Validator):
         return cls(elem.get('message', None), elem.get('min', None), elem.get('max', None), elem.get('negate', 'false'))
 
     def __init__(self, message, length_min, length_max, negate):
-        if length_min is not None:
-            self.min = int(length_min)
-        else:
-            self.min = float('-inf')
-        if length_max is not None:
-            self.max = int(length_max)
-        else:
-            self.max = float('inf')
         if message is None:
-            message = f"Must {'not ' if negate == 'true' else ''}have length of at least {self.min} and at most {self.max}"
-        super().__init__(message, negate)
+            message = f"Must {'not ' if negate == 'true' else ''}have length of at least {length_min} and at most {length_max}"
+        super().__init__(message, range_min=length_min, range_max=length_max, negate=negate)
 
     def validate(self, value, trans=None):
-        super().validate(self.min <= len(value) <= self.max, trans, value_to_show=value)
+        super().validate(len(value), trans)
 
 
 class DatasetOkValidator(Validator):
@@ -426,7 +417,6 @@ class DatasetEmptyValidator(Validator):
     >>> sa_session.add(hist)
     >>> sa_session.flush()
     >>> set_datatypes_registry(example_datatype_registry_for_sample())
-    >>> # TODO is there a better way than hardcoding 'test-data/'
     >>> empty_dataset = Dataset(external_filename=get_test_fname("empty.txt"))
     >>> empty_hda = hist.add_dataset(HistoryDatasetAssociation(id=1, extension='interval', dataset=empty_dataset, sa_session=sa_session))
     >>> full_dataset = Dataset(external_filename=get_test_fname("1.tabular"))
@@ -465,6 +455,7 @@ class DatasetEmptyValidator(Validator):
 
     def validate(self, value, trans=None):
         if value:
+            log.error(f"EMPTY? value {value}")
             super().validate(value.get_size() != 0)
 
 
@@ -782,8 +773,10 @@ class MetadataInFileColumnValidator(Validator):
                 fields = line.split(split)
                 if metadata_column < len(fields):
                     self.valid_values.add(fields[metadata_column].strip())
+        log.error(f"self.valid_values {self.valid_values}")
 
     def validate(self, value, trans=None):
+        log.error(f"validate value {value} against self.valid_values {self.valid_values}")
         if not value:
             return
         super().validate(hasattr(value, "metadata") and value.metadata.spec[self.metadata_name].param.to_string(value.metadata.get(self.metadata_name)) in self.valid_values)

From a60a231b28d3fb6707e791c02c993fb172b75582 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Wed, 28 Jul 2021 14:59:11 +0200
Subject: [PATCH 24/30] remove TODOs from functional tool tests

I guess test data is referred by the file name. This leads to a clash
with the dbkeys.
---
 test/functional/tools/validation_dataset_metadata_in_file.xml | 1 -
 test/functional/tools/validation_metadata_in_datatable.xml    | 1 -
 2 files changed, 2 deletions(-)

diff --git a/test/functional/tools/validation_dataset_metadata_in_file.xml b/test/functional/tools/validation_dataset_metadata_in_file.xml
index 14a27627d52..1192046f640 100644
--- a/test/functional/tools/validation_dataset_metadata_in_file.xml
+++ b/test/functional/tools/validation_dataset_metadata_in_file.xml
@@ -17,7 +17,6 @@ echo 'Hello World' > out1
     </outputs>
     <tests>
         <!-- hg19 is in the file and hg38 isn't -->
-        <!-- TODO These tests fail if the same test data is used .. no idea why -->
         <test expect_failure="false">
             <param name="value" value="1.fasta" dbkey="hg19"/> 
             <param name="value_neg" value="2.fasta" dbkey="hg38"/>
diff --git a/test/functional/tools/validation_metadata_in_datatable.xml b/test/functional/tools/validation_metadata_in_datatable.xml
index 421a1850c8d..7e3e6933c13 100644
--- a/test/functional/tools/validation_metadata_in_datatable.xml
+++ b/test/functional/tools/validation_metadata_in_datatable.xml
@@ -21,7 +21,6 @@ echo 'Hello World' > out1
     </outputs>
     <tests>
         <!-- hg19 is in the test data table and hg38 not-->
-        <!-- TODO These tests fail if the same test data is used .. no idea why -->
         <test expect_failure="false">
             <param name="value" value="1.fasta" dbkey="hg19"/> 
             <param name="value_neg" value="2.fasta" dbkey="hg38"/>

From 95b919aebd9da6a2e41f15773979c7c18d70933e Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Wed, 28 Jul 2021 15:21:26 +0200
Subject: [PATCH 25/30] make InRangeValidator a subclass of ExpressionValidator

---
 lib/galaxy/tools/parameters/validation.py | 44 +++++++++--------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 27fc260a4d1..22c82b3f49b 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -191,9 +191,11 @@ class ExpressionValidator(Validator):
             message = f"Value '%s' does not evaluate to {'True' if negate == 'false' else 'False'} for '{expression}'"
         super().__init__(message, negate)
         # Save compiled expression, code objects are thread safe (right?)
+        log.error(f"ExpressionValidator expression {expression}")
         self.expression = compile(expression, '<string>', 'eval')
 
     def validate(self, value, trans=None):
+        log.error(f"ExpressionValidator.validate value {value} expression {self.expression}")
         try:
             evalresult = eval(self.expression, dict(value=value))
         except Exception:
@@ -202,8 +204,7 @@ class ExpressionValidator(Validator):
         super().validate(evalresult, value_to_show=value)
 
 
-# TODO This could be a subclass of ExpressionValidator
-class InRangeValidator(Validator):
+class InRangeValidator(ExpressionValidator):
     """
     Validator that ensures a number is in a specified range
 
@@ -211,19 +212,19 @@ class InRangeValidator(Validator):
     >>> from galaxy.tools.parameters.basic import ToolParameter
     >>> p = ToolParameter.build(None, XML('''
     ... <param name="blah" type="integer" value="10">
-    ...     <validator type="in_range" message="Not gonna happen" min="10" exclude_min="true" max="20"/>
+    ...     <validator type="in_range" message="Doh!! %s not in range" min="10" exclude_min="true" max="20"/>
     ... </param>
     ... '''))
     >>> t = p.validate(10)
     Traceback (most recent call last):
         ...
-    ValueError: Not gonna happen
+    ValueError: Doh!! 10 not in range
     >>> t = p.validate(15)
     >>> t = p.validate(20)
     >>> t = p.validate(21)
     Traceback (most recent call last):
         ...
-    ValueError: Not gonna happen
+    ValueError: Doh!! 21 not in range
     >>>
     >>> p = ToolParameter.build(None, XML('''
     ... <param name="blah" type="integer" value="10">
@@ -234,11 +235,11 @@ class InRangeValidator(Validator):
     >>> t = p.validate(15)
     Traceback (most recent call last):
         ...
-    ValueError: Value ('15') must not fulfill value > 10 and value <= 20
+    ValueError: Value ('15') must not fulfill float('10') < value <= float('20')
     >>> t = p.validate(20)
     Traceback (most recent call last):
         ...
-    ValueError: Value ('20') must not fulfill value > 10 and value <= 20
+    ValueError: Value ('20') must not fulfill float('10') < value <= float('20')
     >>> t = p.validate(21)
     """
 
@@ -257,34 +258,22 @@ class InRangeValidator(Validator):
         (1.e., min <= value <= max).  Combinations of exclude_min and exclude_max
         values are allowed.
         """
-        self.min = float(range_min if range_min is not None else '-inf')
+        self.min = range_min if range_min is not None else '-inf'
         self.exclude_min = util.asbool(exclude_min)
-        self.max = float(range_max if range_max is not None else 'inf')
+        self.max = range_max if range_max is not None else 'inf'
         self.exclude_max = util.asbool(exclude_max)
-        assert self.min <= self.max, 'min must be less than or equal to max'
+        assert float(self.min) <= float(self.max), 'min must be less than or equal to max'
         # Remove unneeded 0s and decimal from floats to make message pretty.
-        self_min_str = str(self.min).rstrip('0').rstrip('.')
-        self_max_str = str(self.max).rstrip('0').rstrip('.')
-        op1 = '>='
+        op1 = '<='
         op2 = '<='
         if self.exclude_min:
-            op1 = '>'
+            op1 = '<'
         if self.exclude_max:
             op2 = '<'
+        expression = f"float('{self.min}') {op1} value {op2} float('{self.max}')"
         if message is None:
-            message = f"Value ('%s') must {'not ' if negate == 'true' else ''}fulfill value {op1} {self_min_str} and value {op2} {self_max_str}"
-        super().__init__(message, negate)
-
-    def validate(self, value, trans=None):
-        if self.exclude_min:
-            mincmp = self.min.__lt__
-        else:
-            mincmp = self.min.__le__
-        if self.exclude_max:
-            maxcmp = self.max.__gt__
-        else:
-            maxcmp = self.max.__ge__
-        super().validate(mincmp(float(value)) and maxcmp(float(value)), value_to_show=value)
+            message = f"Value ('%s') must {'not ' if negate == 'true' else ''}fulfill {expression}"
+        super().__init__(message, expression, negate)
 
 
 class LengthValidator(InRangeValidator):
@@ -962,6 +951,7 @@ class MetadataInRangeValidator(InRangeValidator):
                 raise ValueError(f'{self.metadata_name} Metadata missing')
             except ValueError:
                 raise ValueError(f'{self.metadata_name} must be a float or an integer')
+            log.error(f"MetadataInRangeValidato.validate value_to_check {value_to_check}")
             super().validate(value_to_check, trans)
 
 

From e99057b18ae4afde37c84602665ee563aa0ed555 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Wed, 28 Jul 2021 15:35:17 +0200
Subject: [PATCH 26/30] make MetadataInDataTableColumnValidator subclass of
 ValueDataTableColumnValidator

---
 lib/galaxy/tools/parameters/validation.py | 36 ++++-------------------
 1 file changed, 5 insertions(+), 31 deletions(-)

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 22c82b3f49b..4da47e96506 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -80,7 +80,6 @@ class Validator(abc.ABC):
         if (not self.negate and value) or (self.negate and not value):
             return
         else:
-            # TODO message often makes not sense if negate=True
             raise ValueError(message)
 
 
@@ -387,7 +386,6 @@ class DatasetOkValidator(Validator):
 
     def validate(self, value, trans=None):
         if value:
-            # TODO all Dataset Validators should be able to handle lists, or?
             super().validate(value.state == model.Dataset.states.OK)
 
 
@@ -444,7 +442,6 @@ class DatasetEmptyValidator(Validator):
 
     def validate(self, value, trans=None):
         if value:
-            log.error(f"EMPTY? value {value}")
             super().validate(value.get_size() != 0)
 
 
@@ -762,13 +759,11 @@ class MetadataInFileColumnValidator(Validator):
                 fields = line.split(split)
                 if metadata_column < len(fields):
                     self.valid_values.add(fields[metadata_column].strip())
-        log.error(f"self.valid_values {self.valid_values}")
 
     def validate(self, value, trans=None):
-        log.error(f"validate value {value} against self.valid_values {self.valid_values}")
         if not value:
             return
-        super().validate(hasattr(value, "metadata") and value.metadata.spec[self.metadata_name].param.to_string(value.metadata.get(self.metadata_name)) in self.valid_values)
+        super().validate(value.metadata.spec[self.metadata_name].param.to_string(value.metadata.get(self.metadata_name)) in self.valid_values)
 
 
 class ValueInDataTableColumnValidator(Validator):
@@ -839,7 +834,7 @@ class ValueNotInDataTableColumnValidator(ValueInDataTableColumnValidator):
             raise ValueError(self.message)
 
 
-class MetadataInDataTableColumnValidator(Validator):
+class MetadataInDataTableColumnValidator(ValueInDataTableColumnValidator):
     """
     Validator that checks if the value for a dataset's metadata item exists in a file.
 
@@ -856,7 +851,7 @@ class MetadataInDataTableColumnValidator(Validator):
         metadata_name = elem.get("metadata_name", None)
         if metadata_name:
             metadata_name = metadata_name.strip()
-        # TODO rename to column
+        # TODO rename to column?
         metadata_column = elem.get("metadata_column", 0)
         try:
             metadata_column = int(metadata_column)
@@ -867,32 +862,11 @@ class MetadataInDataTableColumnValidator(Validator):
         return cls(tool_data_table, metadata_name, metadata_column, message, negate)
 
     def __init__(self, tool_data_table, metadata_name, metadata_column, message="Value for metadata not found.", negate="false"):
-        super().__init__(message, negate)
+        super().__init__(tool_data_table, metadata_column, message, negate)
         self.metadata_name = metadata_name
-        self.valid_values = []
-        self._data_table_content_version = None
-        self._tool_data_table = tool_data_table
-        if isinstance(metadata_column, str):
-            metadata_column = tool_data_table.columns[metadata_column]
-        self._metadata_column = metadata_column
-        self._load_values()
-
-    def _load_values(self):
-        self._data_table_content_version, data_fields = self._tool_data_table.get_version_fields()
-        self.valid_values = []
-        for fields in data_fields:
-            if self._metadata_column < len(fields):
-                self.valid_values.append(fields[self._metadata_column])
 
     def validate(self, value, trans=None):
-        if not value:
-            return
-        if hasattr(value, "metadata"):
-            if not self._tool_data_table.is_current_version(self._data_table_content_version):
-                log.debug('MetadataInDataTableColumnValidator values are out of sync with data table (%s), updating validator.', self._tool_data_table.name)
-                self._load_values()
-        # TODO instead of `and` call super().validate 2x using a better error message for the case that there is no metadata
-        super().validate(hasattr(value, "metadata") and value.metadata.spec[self.metadata_name].param.to_string(value.metadata.get(self.metadata_name)) in self.valid_values, trans)
+        super().validate(value.metadata.spec[self.metadata_name].param.to_string(value.metadata.get(self.metadata_name)), trans)
 
 
 class MetadataNotInDataTableColumnValidator(MetadataInDataTableColumnValidator):

From 5d740b8be336317f7ea83bc665e5342bb86081d5 Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Fri, 10 Sep 2021 14:05:59 +0200
Subject: [PATCH 27/30] fix documentation in dataset_metadata_in_file

and deprecate also the filename attribute
---
 lib/galaxy/tool_util/xsd/galaxy.xsd | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/galaxy/tool_util/xsd/galaxy.xsd b/lib/galaxy/tool_util/xsd/galaxy.xsd
index 63b8b78c99b..b26002e41f9 100644
--- a/lib/galaxy/tool_util/xsd/galaxy.xsd
+++ b/lib/galaxy/tool_util/xsd/galaxy.xsd
@@ -3648,7 +3648,8 @@ a given range.
 Deprecated data validators:
 
 - ``dataset_metadata_in_file``: Use data tables with ``dataset_metadata_in_data_table``.
-Check if a metadata value is contained in a specific column of another data set.
+Check if a metadata value is contained in a specific column of a file in the ``tool_data_path``
+(which is set in Galaxy's config).
 - ``dataset_metadata_not_in_data_table``: Use ``dataset_metadata_in_data_table`` with ``negate="true"``.
 
 ### Validators for textual inputs (``text``, ``select``, ...)
@@ -3775,7 +3776,7 @@ more information.</xs:documentation>
         </xs:attribute>
         <xs:attribute name="filename" type="xs:string">
           <xs:annotation>
-            <xs:documentation xml:lang="en">Tool data filename to check against
+            <xs:documentation xml:lang="en">Deprecated: use ``dataset_metadata_in_data_table``. Tool data filename to check against
 if ``type`` is ``dataset_metadata_in_file``. File should be present Galaxy's
 ``tool-data`` directory.</xs:documentation>
           </xs:annotation>

From c0ac16f1f17806a307e4aca947b606e86dcc720f Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Fri, 10 Sep 2021 14:10:13 +0200
Subject: [PATCH 28/30] remove obsolete TODO

---
 lib/galaxy/tools/parameters/validation.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 4da47e96506..3f91097dce6 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -838,7 +838,6 @@ class MetadataInDataTableColumnValidator(ValueInDataTableColumnValidator):
     """
     Validator that checks if the value for a dataset's metadata item exists in a file.
 
-    TODO Could be derived from ValueInDataTableColumnValidator
     note: this is covered in a framework test (validation_metadata_in_datatable)
     """
     requires_dataset_metadata = True

From df62fe25964e3d1380cf0151b7375e545a69417e Mon Sep 17 00:00:00 2001
From: Matthias Bernt <m.bernt@ufz.de>
Date: Fri, 10 Sep 2021 14:22:37 +0200
Subject: [PATCH 29/30] de-deprecate *not_in* validators

---
 lib/galaxy/tool_util/xsd/galaxy.xsd       | 12 +++++-------
 lib/galaxy/tools/parameters/validation.py | 12 +++++-------
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/lib/galaxy/tool_util/xsd/galaxy.xsd b/lib/galaxy/tool_util/xsd/galaxy.xsd
index b26002e41f9..9217482e4bd 100644
--- a/lib/galaxy/tool_util/xsd/galaxy.xsd
+++ b/lib/galaxy/tool_util/xsd/galaxy.xsd
@@ -3644,13 +3644,13 @@ parameters a ``metadata`` validator is added automatically.
 - ``dataset_metadata_in_range``: Check if a numeric metadata value is within
 a given range.
 - ``dataset_metadata_in_data_table``: Check if a metadata value is contained in a column of a data table.
+- ``dataset_metadata_not_in_data_table``: Equivalent to ``dataset_metadata_in_data_table`` with ``negate="true"``.
 
 Deprecated data validators:
 
 - ``dataset_metadata_in_file``: Use data tables with ``dataset_metadata_in_data_table``.
 Check if a metadata value is contained in a specific column of a file in the ``tool_data_path``
 (which is set in Galaxy's config).
-- ``dataset_metadata_not_in_data_table``: Use ``dataset_metadata_in_data_table`` with ``negate="true"``.
 
 ### Validators for textual inputs (``text``, ``select``, ...)
 
@@ -3672,10 +3672,7 @@ For ``text`` inputs the following validators are useful:
 - ``empty_field``: Check if the string is not empty
 - ``value_in_data_table``: Check if the value is
 contained in a column of a given data table.
-
-Deprecated:
-
-- ``value_not_in_data_table``: Use ``value_in_data_table`` with ``negate="true"``.
+- ``value_not_in_data_table``: Equivalent to ``value_in_data_table`` with ``negate="true"``.
 
 ### Validators for numeric inputs (``integer``, ``float``)
 
@@ -3736,9 +3733,10 @@ use in filenames may not contain ``..``.
             <xs:documentation xml:lang="en"><![CDATA[
 Valid values are: ``expression``, ``regex``, ``in_range``, ``length``,
 ``metadata``, ``unspecified_build``, ``no_options``, ``empty_field``,
-``dataset_metadata_in_data_table``, ``value_in_data_table``,
+``dataset_metadata_in_data_table``, ``dataset_metadata_not_in_data_table``,
+``value_in_data_table``, ``value_not_in_data_table``,
 ``dataset_ok_validator``, ``dataset_metadata_in_range``.
-Deprecated validators: ``dataset_metadata_in_file``, ``value_not_in_data_table``, ``dataset_metadata_not_in_data_table``.
+Deprecated validator: ``dataset_metadata_in_file``.
 The list of supported
 validators is in the ``validator_types`` dictionary in
 [/lib/galaxy/tools/parameters/validation.py](https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/tools/parameters/validation.py).
diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 3f91097dce6..04236235cf4 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -816,8 +816,7 @@ class ValueInDataTableColumnValidator(Validator):
 class ValueNotInDataTableColumnValidator(ValueInDataTableColumnValidator):
     """
     Validator that checks if a value is NOT in a tool data table column.
-
-    Deprecated: use now the `negate` attribute
+    Equivalent to ValueInDataTableColumnValidator with `negate="true"`.
 
     note: this is covered in a framework test (validation_value_in_datatable)
     """
@@ -871,8 +870,7 @@ class MetadataInDataTableColumnValidator(ValueInDataTableColumnValidator):
 class MetadataNotInDataTableColumnValidator(MetadataInDataTableColumnValidator):
     """
     Validator that checks if the value for a dataset's metadata item doesn't exists in a file.
-
-    Deprecated: use now the `negate` attribute
+    Equivalent to MetadataInDataTableColumnValidator with `negate="true"`.
 
     note: this is covered in a framework test (validation_metadata_in_datatable)
     """
@@ -940,15 +938,15 @@ validator_types = dict(
     empty_dataset=DatasetEmptyValidator,
     empty_extra_files_path=DatasetExtraFilesPathEmptyValidator,
     dataset_metadata_in_data_table=MetadataInDataTableColumnValidator,
+    dataset_metadata_not_in_data_table=MetadataNotInDataTableColumnValidator,
     dataset_metadata_in_range=MetadataInRangeValidator,
     value_in_data_table=ValueInDataTableColumnValidator,
+    value_not_in_data_table=ValueNotInDataTableColumnValidator,
     dataset_ok_validator=DatasetOkValidator,
 )
 
 deprecated_validator_types = dict(
-    dataset_metadata_in_file=MetadataInFileColumnValidator,
-    dataset_metadata_not_in_data_table=MetadataNotInDataTableColumnValidator,
-    value_not_in_data_table=ValueNotInDataTableColumnValidator,
+    dataset_metadata_in_file=MetadataInFileColumnValidator
 )
 validator_types.update(deprecated_validator_types)
 

From b14653cb3d6d1b5a44086536fca7c66c640feab5 Mon Sep 17 00:00:00 2001
From: M Bernt <m.bernt@ufz.de>
Date: Fri, 10 Sep 2021 16:29:50 +0200
Subject: [PATCH 30/30] Remove TODO

Co-authored-by: Marius van den Beek <m.vandenbeek@gmail.com>
---
 lib/galaxy/tools/parameters/validation.py | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/lib/galaxy/tools/parameters/validation.py b/lib/galaxy/tools/parameters/validation.py
index 04236235cf4..3b5e322e0d9 100644
--- a/lib/galaxy/tools/parameters/validation.py
+++ b/lib/galaxy/tools/parameters/validation.py
@@ -345,9 +345,6 @@ class DatasetOkValidator(Validator):
     >>> ok_hda = hist.add_dataset(HistoryDatasetAssociation(id=1, extension='interval', create_dataset=True, sa_session=sa_session))
     >>> ok_hda.set_dataset_state(model.Dataset.states.OK)
     >>> notok_hda = hist.add_dataset(HistoryDatasetAssociation(id=2, extension='interval', create_dataset=True, sa_session=sa_session))
-    >>> # TODO I do not get 100% why for state!=OK the validator is called
-    >>> # TODO because DataToolParameter.validate.do_validate calls the validator only of state=OK
-    >>> # TODO in this light I wonder about the use of this validator at all....
     >>> notok_hda.set_dataset_state(model.Dataset.states.EMPTY)
     >>>
     >>> p = ToolParameter.build(None, XML('''