Merge pull request #13299 from mvdbeek/disable_required_metadata_filter_for_now

[22.01] Limit required element validation to safe elements

lib/galaxy/datatypes/binary.py

@@ -2535,8 +2535,11 @@ class RData(CompressedArchive):
     VERSION_2_PREFIX = b'RDX2\nX\n'
     VERSION_3_PREFIX = b'RDX3\nX\n'
     file_ext = 'rdata'
+    check_required_metadata = True
 
-    MetadataElement(name="version", default=None, desc="serialisation version", param=MetadataParameter, readonly=True, visible=False, optional=False)
+    # Tools may in the past have output rdata files that are actually RDS files, and so parsing the version fails,
+    # that is why we have to keep optional="True".
+    MetadataElement(name="version", default=None, desc="serialisation version", param=MetadataParameter, readonly=True, visible=False, optional=True)
 
     def set_meta(self, dataset, overwrite=True, **kwd):
         super().set_meta(dataset, overwrite=overwrite, **kwd)
@@ -2591,6 +2594,7 @@ class RDS(CompressedArchive):
     '3.5.0'
     """
     file_ext = 'rds'
+    check_required_metadata = True
 
     MetadataElement(name="version", default=None, desc="serialisation version", param=MetadataParameter, readonly=True, visible=False, optional=False)
     MetadataElement(name="rversion", default=None, desc="R version", param=MetadataParameter, readonly=True, visible=False, optional=False)

lib/galaxy/datatypes/data.py

@@ -243,7 +243,10 @@ class Data(metaclass=DataMeta):
                 continue
             if not check and len(skip) == 0 and dataset.metadata.spec[key].get("optional"):
                 continue  # we skip check for optional and nonrequested values here
-            if not dataset.metadata.element_is_set(key):
+            if not dataset.metadata.element_is_set(key) and (check or dataset.metadata.spec[key].check_required_metadata):
+                # FIXME: Optional metadata isn't always properly annotated,
+                # so skip check if check_required_metadata is false on the datatype that defined the metadata element.
+                # See https://github.com/galaxyproject/tools-iuc/issues/4367
                 return key
         return False
 

lib/galaxy/datatypes/interval.py

@@ -379,6 +379,7 @@ class Bed(Interval):
     file_ext = "bed"
     data_sources = {"data": "tabix", "index": "bigwig", "feature_search": "fli"}
     track_type = Interval.track_type
+    check_required_metadata = True
 
     column_names = ['Chrom', 'Start', 'End', 'Name', 'Score', 'Strand', 'ThickStart', 'ThickEnd', 'ItemRGB', 'BlockCount', 'BlockSizes', 'BlockStarts']
 

lib/galaxy/model/metadata.py

@@ -166,13 +166,12 @@ class MetadataCollection(Mapping):
         :returns: True if the value differes from the no_value
                   False if its equal of if no metadata with the name is specified
         """
+        meta_val = self[name]
         try:
-            meta_val = self[name]
+            meta_spec = self.parent.metadata.spec[name]
         except KeyError:
-            log.debug(f"no metadata with name {name} found")
+            log.debug(f"No metadata element with name '{name}' found")
             return False
-
-        meta_spec = self.parent.metadata.spec[name]
         return meta_val != meta_spec.no_value
 
     def get_metadata_parameter(self, name, **kwd):
@@ -373,6 +372,11 @@ class MetadataElementSpec:
         self.param = param(self)
         # add spec element to the spec
         datatype.metadata_spec.append(self)
+        # Should we validate that non-optional elements have been set ?
+        # (The answer is yes, but not all datatypes control optionality appropriately at this point.)
+        # This allows us to check that inherited MetadataElement instances from datatypes that set
+        # check_required_metadata have been reviewed and considered really required.
+        self.check_required_metadata = datatype.__dict__.get('check_required_metadata', False)
 
     def get(self, name, default=None):
         return self.__dict__.get(name, default)

test/unit/data/datatypes/test_check_required.py

@@ -0,0 +1,42 @@
+from galaxy.datatypes.data import Data
+from galaxy.datatypes.metadata import MetadataElement
+from galaxy.model import HistoryDatasetAssociation
+from galaxy.model.unittest_utils import GalaxyDataTestApp
+
+
+class CheckRequiredFalse(Data):
+    ext = 'false'
+    MetadataElement(name="columns", desc="Number of columns")
+
+
+class CheckRequiredTrue(Data):
+    ext = 'true'
+    check_required_metadata = True
+    MetadataElement(name="columns", desc="Number of columns")
+
+
+class CheckRequiredInherited(CheckRequiredTrue):
+    ext = 'inherited'
+    MetadataElement(name="something")
+
+
+def test_check_required_metadata_false():
+    app = GalaxyDataTestApp()
+    app.datatypes_registry.datatypes_by_extension['false'] = CheckRequiredFalse
+    hda = HistoryDatasetAssociation(sa_session=app.model.session, extension='false')
+    assert not hda.metadata.spec['columns'].check_required_metadata
+
+
+def test_check_required_metadata_true():
+    app = GalaxyDataTestApp()
+    app.datatypes_registry.datatypes_by_extension['true'] = CheckRequiredTrue
+    hda = HistoryDatasetAssociation(sa_session=app.model.session, extension='true')
+    assert hda.metadata.spec['columns'].check_required_metadata
+
+
+def test_check_required_metadata_inherited():
+    app = GalaxyDataTestApp()
+    app.datatypes_registry.datatypes_by_extension['inherited'] = CheckRequiredInherited
+    hda = HistoryDatasetAssociation(sa_session=app.model.session, extension='inherited')
+    assert hda.metadata.spec['columns'].check_required_metadata
+    assert not hda.metadata.spec['something'].check_required_metadata