From bd237cd5863352faba908bbcf39529cd176601a0 Mon Sep 17 00:00:00 2001 From: carlfeberhard Date: Thu, 14 Jan 2016 12:10:53 -0500 Subject: [PATCH] History contents manager: add tests, fix order_by defaulting, clarify filter getter name --- lib/galaxy/managers/containers.py | 6 +- lib/galaxy/managers/history_contents.py | 36 ++++---- .../managers/test_HistoryContentsManager.py | 90 ++++++++++++++++++- 3 files changed, 111 insertions(+), 21 deletions(-) diff --git a/lib/galaxy/managers/containers.py b/lib/galaxy/managers/containers.py index 340655aff16..cac49647b99 100644 --- a/lib/galaxy/managers/containers.py +++ b/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 ): diff --git a/lib/galaxy/managers/history_contents.py b/lib/galaxy/managers/history_contents.py index cec9bf5f4d2..420e25760e6 100644 --- a/lib/galaxy/managers/history_contents.py +++ b/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 [] diff --git a/test/unit/managers/test_HistoryContentsManager.py b/test/unit/managers/test_HistoryContentsManager.py index cb0e3593558..91957a0dc1c 100644 --- a/test/unit/managers/test_HistoryContentsManager.py +++ b/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__':