Bugfix: Rework collection matching logic so it properly matches combined collection/subcollection mapping.

With functional test to verify it works end-to-end with tool execution and unit tests to verify different kinds of collection combinations.

lib/galaxy/dataset_collections/matching.py

@@ -61,7 +61,7 @@ class MatchingCollections( object ):
         matching_collections = MatchingCollections()
         for input_key, to_match in collections_to_match.iteritems():
             hdca = to_match.hdca
-            subcollection_type = to_match = to_match.subcollection_type
+            subcollection_type = to_match.subcollection_type
             collection_type_description = collection_type_descriptions.for_collection_type( hdca.collection.collection_type )
             matching_collections.__attempt_add_to_match( input_key, hdca, collection_type_description, subcollection_type )
 

lib/galaxy/dataset_collections/structure.py

@@ -18,19 +18,15 @@ leaf = Leaf()
 
 class Tree( object ):
 
-    def __init__( self, dataset_collection, collection_type_description, leaf_subcollection_type ):
+    def __init__( self, dataset_collection, collection_type_description ):
         self.collection_type_description = collection_type_description
-        self.leaf_subcollection_type = leaf_subcollection_type  # collection_type to trim tree at...
         children = []
         for element in dataset_collection.elements:
-            child_collection = element.child_collection
-            if child_collection:
+            if collection_type_description.has_subcollections():
+                child_collection = element.child_collection
                 subcollection_type_description = collection_type_description.subcollection_type_description()  # Type description of children
-                if subcollection_type_description.can_match_type( leaf_subcollection_type ):
-                    children.append( ( element.element_identifier, leaf  ) )
-                else:
-                    children.append( ( element.element_identifier, Tree( child_collection, collection_type_description=subcollection_type_description, leaf_subcollection_type=leaf_subcollection_type )  ) )
-            elif element.hda:
+                children.append( ( element.element_identifier, Tree( child_collection, collection_type_description=subcollection_type_description )  ) )
+            else:
                 children.append( ( element.element_identifier, leaf ) )
 
         self.children = children
@@ -56,7 +52,6 @@ class Tree( object ):
 
     def can_match( self, other_structure ):
         if not self.collection_type_description.can_match_type( other_structure.collection_type_description ):
-            # TODO: generalize
             return False
 
         if len( self.children ) != len( other_structure.children ):
@@ -99,4 +94,7 @@ def dict_map( func, input_dict ):
 
 
 def get_structure( dataset_collection_instance, collection_type_description, leaf_subcollection_type=None ):
-    return Tree( dataset_collection_instance.collection, collection_type_description, leaf_subcollection_type=leaf_subcollection_type )
+    if leaf_subcollection_type:
+        collection_type_description = collection_type_description.effective_collection_type_description( leaf_subcollection_type )
+
+    return Tree( dataset_collection_instance.collection, collection_type_description )

lib/galaxy/dataset_collections/type_description.py

@@ -15,9 +15,9 @@ class CollectionTypeDescription( object ):
     """ Abstraction over dataset collection type that ties together string
     reprentation in database/model with type registry.
 
-
-    >>> nested_type_description = CollectionTypeDescription( "list:paired", None )
-    >>> paired_type_description = CollectionTypeDescription( "paired", None )
+    >>> factory = CollectionTypeDescriptionFactory( None )
+    >>> nested_type_description = factory.for_collection_type( "list:paired" )
+    >>> paired_type_description = factory.for_collection_type( "paired" )
     >>> nested_type_description.has_subcollections_of_type( "list" )
     False
     >>> nested_type_description.has_subcollections_of_type( "list:paired" )
@@ -34,6 +34,10 @@ class CollectionTypeDescription( object ):
     'paired'
     >>> nested_type_description.rank_collection_type()
     'list'
+    >>> nested_type_description.effective_collection_type( paired_type_description )
+    'list'
+    >>> nested_type_description.effective_collection_type_description( paired_type_description ).collection_type
+    'list'
     """
 
     def __init__( self, collection_type, collection_type_description_factory ):
@@ -41,6 +45,19 @@ class CollectionTypeDescription( object ):
         self.collection_type_description_factory = collection_type_description_factory
         self.__has_subcollections = self.collection_type.find( ":" ) > 0
 
+    def effective_collection_type_description( self, subcollection_type ):
+        effective_collection_type = self.effective_collection_type( subcollection_type )
+        return self.collection_type_description_factory.for_collection_type( effective_collection_type )
+
+    def effective_collection_type( self, subcollection_type ):
+        if hasattr( subcollection_type, 'collection_type' ):
+            subcollection_type = subcollection_type.collection_type
+
+        if not self.has_subcollections_of_type( subcollection_type ):
+            raise ValueError( "Cannot compute effective subcollection type of %s over %s" % ( subcollection_type, self ) )
+
+        return self.collection_type[ :-( len( subcollection_type ) + 1 ) ]
+
     def has_subcollections_of_type( self, other_collection_type ):
         """ Take in another type (either flat string or another
         CollectionTypeDescription) and determine if this collection contains

test/api/helpers.py

@@ -291,12 +291,10 @@ class DatasetCollectionPopulator( object ):
         return element_identifiers
 
     def list_identifiers( self, history_id, contents=None ):
-        hda1, hda2, hda3 = self.__datasets( history_id, count=3, contents=contents )
-        element_identifiers = [
-            dict( name="data1", src="hda", id=hda1[ "id" ] ),
-            dict( name="data2", src="hda", id=hda2[ "id" ] ),
-            dict( name="data3", src="hda", id=hda3[ "id" ] ),
-        ]
+        count = 3 if not contents else len( contents )
+        hdas = self.__datasets( history_id, count=count, contents=contents )
+        hda_to_identifier = lambda ( i, hda ): dict( name="data%d" % ( i + 1 ), src="hda", id=hda[ "id" ] )
+        element_identifiers = map( hda_to_identifier, enumerate( hdas ) )
         return element_identifiers
 
     def __create( self, payload ):

test/api/test_tools.py

@@ -279,6 +279,27 @@ class ToolsTestCase( api.ApiTestCase ):
         assert output1_content.strip() == "123\n456", output1_content
         assert output2_content.strip() == "789\n0ab", output2_content
 
+    @skip_without_tool( "collection_mixed_param" )
+    def test_combined_mapping_and_subcollection_mapping( self ):
+        history_id = self.dataset_populator.new_history()
+        nested_list_id = self.__build_nested_list( history_id )
+        create_response = self.dataset_collection_populator.create_list_in_history( history_id, contents=["xxx", "yyy"] )
+        list_id = create_response.json()[ "id" ]
+        inputs = {
+            "f1|__collection_multirun__": "%s|paired" % nested_list_id,
+            "f2|__collection_multirun__": list_id,
+        }
+        self.dataset_populator.wait_for_history( history_id, assert_ok=True )
+        outputs = self._run_and_get_outputs( "collection_mixed_param", history_id, inputs )
+        assert len( outputs ), 2
+        self.dataset_populator.wait_for_history( history_id, assert_ok=True )
+        output1 = outputs[ 0 ]
+        output2 = outputs[ 1 ]
+        output1_content = self._get_content( history_id, dataset=output1 )
+        output2_content = self._get_content( history_id, dataset=output2 )
+        assert output1_content.strip() == "123\n456\nxxx", output1_content
+        assert output2_content.strip() == "789\n0ab\nyyy", output2_content
+
     def _cat1_outputs( self, history_id, inputs ):
         return self._run_outputs( self._run_cat1( history_id, inputs ) )
 

test/unit/dataset_collections/test_matching.py

@@ -0,0 +1,141 @@
+from galaxy.dataset_collections import (
+    type_description,
+    registry,
+    matching,
+)
+
+TYPE_REGISTRY = registry.DatasetCollectionTypesRegistry( None )
+TYPE_DESCRIPTION_FACTORY = type_description.CollectionTypeDescriptionFactory( TYPE_REGISTRY )
+
+
+def test_pairs_match():
+    assert_can_match( pair_instance(), pair_instance() )
+
+
+def test_lists_of_same_cardinality_match():
+    assert_can_match( list_instance(), list_instance() )
+
+
+def test_nested_lists_match():
+    nested_list = list_instance(
+        elements=[
+            pair_element("data1"),
+            pair_element("data2"),
+            pair_element("data3"),
+        ],
+        collection_type="list:paired",
+    )
+    assert_can_match( nested_list, nested_list )
+
+
+def test_different_types_cannot_match():
+    assert_cannot_match( list_instance(), pair_instance() )
+    assert_cannot_match( pair_instance(), list_instance() )
+
+
+def test_lists_of_different_cardinality_do_not_match():
+    list_1 = list_instance( ids=[ "data1", "data2" ] )
+    list_2 = list_instance( ids=[ "data1", "data2", "data3" ] )
+    assert_cannot_match( list_1, list_2 )
+    assert_cannot_match( list_2, list_1 )
+
+
+def test_valid_collection_subcollection_matching():
+    flat_list = list_instance( ids=[ "data1", "data2", "data3" ] )
+    nested_list = list_instance(
+        elements=[
+            pair_element("data11"),
+            pair_element("data21"),
+            pair_element("data31"),
+        ],
+        collection_type="list:paired",
+    )
+    assert_cannot_match( flat_list, nested_list )
+    assert_cannot_match( nested_list, flat_list )
+    assert_can_match( ( nested_list, "paired" ), flat_list )
+
+
+def assert_can_match( *items ):
+    to_match = build_collections_to_match( *items )
+    matching.MatchingCollections.for_collections( to_match, TYPE_DESCRIPTION_FACTORY )
+
+
+def assert_cannot_match( *items ):
+    to_match = build_collections_to_match( *items )
+    threw_exception = False
+    try:
+        matching.MatchingCollections.for_collections( to_match, TYPE_DESCRIPTION_FACTORY )
+    except Exception:
+        threw_exception = True
+    assert threw_exception
+
+
+def build_collections_to_match( *items ):
+    to_match = matching.CollectionsToMatch()
+
+    for i, item in enumerate( items ):
+        if isinstance( item, tuple ):
+            collection_instance, subcollection_type = item
+        else:
+            collection_instance, subcollection_type = item, None
+        to_match.add( "input_%d" % i, collection_instance, subcollection_type )
+    return to_match
+
+
+def pair_element( element_identifier ):
+    return collection_element( element_identifier, pair_instance().collection )
+
+
+def pair_instance( ):
+    paired_collection_instance = collection_instance( collection_type="paired", elements=[
+        hda_element( "left" ),
+        hda_element( "right" ),
+    ] )
+    return paired_collection_instance
+
+
+def list_instance( collection_type="list", elements=None, ids=None ):
+    if not elements:
+        if ids is None:
+            ids = [ "data1", "data2" ]
+        elements = map(hda_element, ids)
+    list_collection_instance = collection_instance(
+        collection_type=collection_type,
+        elements=elements
+    )
+    return list_collection_instance
+
+
+class MockCollectionInstance( object ):
+
+    def __init__( self, collection_type, elements ):
+        self.collection = MockCollection( collection_type, elements )
+
+
+class MockCollection( object ):
+
+    def __init__( self, collection_type, elements ):
+        self.collection_type = collection_type
+        self.elements = elements
+
+
+class MockCollectionElement( object ):
+
+    def __init__( self, element_identifier, collection ):
+        self.element_identifier = element_identifier
+        self.child_collection = collection
+        self.hda = None
+
+
+class MockHDAElement( object ):
+
+    def __init__( self, element_identifier ):
+        self.element_identifier = element_identifier
+        self.child_collection = False
+        self.hda = object()
+
+
+collection_instance = MockCollectionInstance
+collection = MockCollection
+collection_element = MockCollectionElement
+hda_element = MockHDAElement