History contents manager: add tests, fix order_by defaulting, clarify filter getter name

lib/galaxy/managers/containers.py

@@ -65,7 +65,7 @@ class ContainerManagerMixin( object ):
         query = self.session().query( content_class ).filter( container_filter )
         return query
 
-    def _filter_to_contained( self, container, content_class ):
+    def _get_filter_for_contained( self, container, content_class ):
         raise galaxy.exceptions.NotImplemented( 'Abstract class' )
 
     def _content_manager( self, content ):
@@ -81,7 +81,7 @@ class LibraryFolderAsContainerManagerMixin( ContainerManagerMixin ):
     # subcontainer_class = model.LibraryDatasetCollectionAssociation
     order_contents_on = operator.attrgetter( 'create_time' )
 
-    def _filter_to_contained( self, container, content_class ):
+    def _get_filter_for_contained( self, container, content_class ):
         if content_class == self.subcontainer_class:
             return self.subcontainer_class.parent == container
         return self.contained_class.folder == container
@@ -102,7 +102,7 @@ class DatasetCollectionAsContainerManagerMixin( ContainerManagerMixin ):
     subcontainer_class = model.DatasetCollection
     order_contents_on = operator.attrgetter( 'element_index' )
 
-    def _filter_to_contained( self, container, content_class ):
+    def _get_filter_for_contained( self, container, content_class ):
         return content_class.collection == container
 
     def _content_manager( self, content ):

lib/galaxy/managers/history_contents.py

@@ -37,7 +37,7 @@ class HistoryContentsManager( containers.ContainerManagerMixin ):
         """
         Returns non-subcontainer objects within `container`.
         """
-        filter_to_inside_container = self._filter_to_contained( container, self.contained_class )
+        filter_to_inside_container = self._get_filter_for_contained( container, self.contained_class )
         filters = base.munge_lists( filter_to_inside_container, filters )
         return self.contained_manager.list( filters=filters, limit=limit, offset=offset, order_by=order_by, **kwargs )
 
@@ -45,7 +45,7 @@ class HistoryContentsManager( containers.ContainerManagerMixin ):
         """
         Returns only the containers within `container`.
         """
-        filter_to_inside_container = self._filter_to_contained( container, self.subcontainer_class )
+        filter_to_inside_container = self._get_filter_for_contained( container, self.subcontainer_class )
         filters = base.munge_lists( filter_to_inside_container, filters )
         # TODO: collections.DatasetCollectionManager doesn't have the list
         # return self.subcontainer_manager.list( filters=filters, limit=limit, offset=offset, order_by=order_by, **kwargs )
@@ -64,24 +64,22 @@ class HistoryContentsManager( containers.ContainerManagerMixin ):
     def session( self ):
         return self.app.model.context
 
-    # not limited or offset contents query
     def _filter_to_contents_query( self, container, content_class, **kwargs ):
         # TODO: use list (or by_history etc.)
-        container_filter = self._filter_to_contained( container, content_class )
+        container_filter = self._get_filter_for_contained( container, content_class )
         query = self.session().query( content_class ).filter( container_filter )
         return query
 
-    def _filter_to_contained( self, container, content_class ):
+    def _get_filter_for_contained( self, container, content_class ):
         return content_class.history == container
 
-    # limited and offset query
     def _union_of_contents( self, container, filters=None, limit=None, offset=None, order_by=None, **kwargs ):
         """
         Returns a limited and offset list of both types of contents, filtered
         and in some order.
         """
-        order_by = order_by or self.default_order_by
-        order_by = order_by if isinstance( order_by, tuple ) else ( order_by, )
+        order_by = order_by if order_by is not None else self.default_order_by
+        order_by = order_by if isinstance( order_by, ( tuple, list ) ) else ( order_by, )
 
         # TODO: 3 queries and 3 iterations over results - this is undoubtedly better solved in the actual SQL layer
         # via one common table for contents, Some Yonder Resplendent and Fanciful Join, or ORM functionality
@@ -114,11 +112,9 @@ class HistoryContentsManager( containers.ContainerManagerMixin ):
 
         # partition ids into a map of { component_class names -> list of ids } from the above union query
         id_map = dict( (( self.contained_class.__name__, [] ), ( self.subcontainer_class.__name__, [] )) )
-        get_unioned_typestr = lambda c: str( c[ 1 ] )
-        get_unioned_id = lambda c: c[ 2 ]
         for result in contents_results:
-            result_type = get_unioned_typestr( result )
-            contents_id = get_unioned_id( result )
+            result_type = self._get_union_classstr( result )
+            contents_id = self._get_union_id( result )
             if result_type in id_map:
                 id_map[ result_type ].append( contents_id )
             else:
@@ -139,12 +135,14 @@ class HistoryContentsManager( containers.ContainerManagerMixin ):
         contents = []
         # TODO: or as generator?
         for result in contents_results:
-            result_type = get_unioned_typestr( result )
-            contents_id = get_unioned_id( result )
+            result_type = self._get_union_classstr( result )
+            contents_id = self._get_union_id( result )
             content = id_map[ result_type ][ contents_id ]
             contents.append( content )
         return contents
 
+    #: the columns which are common to both subcontainers and non-subcontainers.
+    #  Also the attributes that may be filtered or orderered_by.
     common_columns = (
         "history_id",
         "model_class",
@@ -186,7 +184,7 @@ class HistoryContentsManager( containers.ContainerManagerMixin ):
         columns = self._contents_common_columns( component_class,
             # TODO: should be purgable? fix
             purged=literal( False ),
-            # these are attached instead to the inner collection
+            # these are attached instead to the inner collection joined below
             create_time=model.DatasetCollection.create_time,
             update_time=model.DatasetCollection.update_time
         )
@@ -197,6 +195,14 @@ class HistoryContentsManager( containers.ContainerManagerMixin ):
         subquery = subquery.filter( component_class.history_id == history_id )
         return subquery
 
+    def _get_union_classstr( self, union ):
+        """Return the string name of the class for this row in the union results"""
+        return str( union[ 1 ] )
+
+    def _get_union_id( self, union ):
+        """Return the id for this row in the union results"""
+        return union[ 2 ]
+
     def _by_ids( self, component_class, id_list ):
         if not id_list:
             return []

test/unit/managers/test_HistoryContentsManager.py

@@ -12,6 +12,8 @@ test_utils = imp.load_source( 'test_utils',
 from galaxy import eggs
 eggs.require( 'SQLAlchemy >= 0.4' )
 from sqlalchemy import true
+from sqlalchemy import false
+from sqlalchemy import desc
 from sqlalchemy.sql import text
 
 from base import BaseTestCase
@@ -129,17 +131,19 @@ class HistoryAsContainerTestCase( BaseTestCase, CreatesCollectionsMixin ):
         contents.extend([ self.add_hda_to_history( history, name=( 'hda-' + str( x ) ) ) for x in xrange( 4, 6 ) ])
         contents.append( self.add_list_collection_to_history( history, contents[4:6] ) )
 
-        self.log( "should be filter on deleted" )
+        self.log( "should allow filter on deleted" )
         self.hda_manager.delete( contents[1] )
         self.hda_manager.delete( contents[4] )
         contents[6].deleted = True
         deleted = [ contents[1], contents[4], contents[6] ]
         self.app.model.context.flush()
-        # weird filter language at this low level
-        filters = [ text( 'anon_1.deleted = 1' ) ]
+
+        # TODO: cross db compat?
+        filters = [ text( 'deleted = 1' ) ]
         # for content in self.contents_manager.contents( history, filters=filters ):
         #     print content.hid, content.history_content_type, content.id, content.name
         self.assertEqual( self.contents_manager.contents( history, filters=filters ), deleted )
+
         # even stranger that sqlalx can use the first model in the union (HDA) for columns across the union
         HDA = self.hda_manager.model_class
         self.assertEqual( self.contents_manager.contents( history,
@@ -148,6 +152,86 @@ class HistoryAsContainerTestCase( BaseTestCase, CreatesCollectionsMixin ):
             filters=[ HDA.deleted == true() ], limit=2, offset=1 )
         self.assertEqual( filter_limited_contents, deleted[1:] )
 
+        self.log( "should allow filter on visible" )
+        contents[2].visible = False
+        contents[5].visible = False
+        contents[6].visible = False
+        invisible = [ contents[2], contents[5], contents[6] ]
+        # for content in invisible:
+        #     print content.id, content.__class__.__name__, content
+        self.app.model.context.flush()
+
+        filters = [ text( 'visible = 0' ) ]
+        self.assertEqual( self.contents_manager.contents( history, filters=filters ), invisible )
+        self.assertEqual( self.contents_manager.contents( history,
+            filters=[ HDA.visible == false() ] ), invisible )
+        filter_limited_contents = self.contents_manager.contents( history,
+            filters=[ HDA.visible == false() ], limit=2, offset=1 )
+        self.assertEqual( filter_limited_contents, invisible[1:] )
+
+        self.log( "should allow filtering more than one attribute" )
+        deleted_and_invisible = [ contents[6] ]
+
+        filters = [ text( 'deleted = 1' ), text( 'visible = 0' ) ]
+        self.assertEqual( self.contents_manager.contents( history, filters=filters ), deleted_and_invisible )
+        self.assertEqual( self.contents_manager.contents( history,
+            filters=[ HDA.deleted == true(), HDA.visible == false() ] ), deleted_and_invisible )
+        offset_too_far = self.contents_manager.contents( history,
+            filters=[ HDA.deleted == true(), HDA.visible == false() ], limit=2, offset=1 )
+        self.assertEqual( offset_too_far, [] )
+
+        self.log( "should allow filtering more than one attribute" )
+        deleted_and_invisible = [ contents[6] ]
+        # note the two syntaxes both work
+        self.assertEqual( self.contents_manager.contents( history,
+            filters=[ text( 'deleted = 1' ), text( 'visible = 0' ) ] ), deleted_and_invisible )
+        self.assertEqual( self.contents_manager.contents( history,
+            filters=[ HDA.deleted == true(), HDA.visible == false() ] ), deleted_and_invisible )
+        offset_too_far = self.contents_manager.contents( history,
+            filters=[ HDA.deleted == true(), HDA.visible == false() ], limit=2, offset=1 )
+        self.assertEqual( offset_too_far, [] )
+
+        self.log( "should allow filtering using like" )
+        # find 'hda-4'
+        self.assertEqual( [ contents[4] ],
+            self.contents_manager.contents( history, filters=[ HDA.name.like( '%-4' ) ] ) )
+        # the collections added above have the default name 'test collection'
+        self.assertEqual( self.contents_manager.subcontainers( history ),
+            self.contents_manager.contents( history, filters=[ HDA.name.like( '%collect%' ) ] ) )
+
+    def test_order_by( self ):
+        user2 = self.user_manager.create( **user2_data )
+        history = self.history_manager.create( name='history', user=user2 )
+        contents = []
+        contents.extend([ self.add_hda_to_history( history, name=( 'hda-' + str( x ) ) ) for x in xrange( 3 ) ])
+        contents.append( self.add_list_collection_to_history( history, contents[:3] ) )
+        contents.extend([ self.add_hda_to_history( history, name=( 'hda-' + str( x ) ) ) for x in xrange( 4, 6 ) ])
+        contents.append( self.add_list_collection_to_history( history, contents[4:6] ) )
+
+        self.log( "should default to hid order_by" )
+        self.assertEqual( self.contents_manager.contents( history ), contents )
+
+        self.log( "should allow asc, desc order_by" )
+        self.assertEqual( self.contents_manager.contents( history, order_by=desc( 'hid' ) ), contents[::-1] )
+
+        def get_create_time( item ):
+            create_time = getattr( item, 'create_time', None )
+            if not create_time:
+                create_time = item.collection.create_time
+            return create_time
+
+        self.log( "should allow create_time order_by" )
+        newest_first = sorted( contents, key=get_create_time, reverse=True )
+        results = self.contents_manager.contents( history, order_by=desc( 'create_time' ) )
+        self.assertEqual( newest_first, results )
+
+        self.log( "should allow update_time order_by" )
+        # change the oldest created to update the update_time
+        contents[0].name = 'zany and/or wacky'
+        self.app.model.context.flush()
+        results = self.contents_manager.contents( history, order_by=desc( 'update_time' ) )
+        self.assertEqual( contents[0], results[0] )
+
 
 # =============================================================================
 if __name__ == '__main__':