From b1728493138df08271b860bd09c0db805c1fda02 Mon Sep 17 00:00:00 2001 From: John Davis Date: Tue, 24 Oct 2023 09:36:34 -0400 Subject: [PATCH] Do not reuse util.listify Move function to managers.base; rename. Do not reuse listify; implement standalone function. Add test case for tuples. (See PR code review for discussion) --- lib/galaxy/managers/base.py | 24 ++++++++++++++++++++++-- lib/galaxy/managers/histories.py | 4 ++-- lib/galaxy/managers/sharable.py | 12 +++++------- lib/galaxy/managers/users.py | 6 +++--- lib/galaxy/util/__init__.py | 11 ----------- test/unit/app/managers/test_base.py | 20 ++++++++++++++++++++ test/unit/util/test_utils.py | 17 ----------------- 7 files changed, 52 insertions(+), 42 deletions(-) create mode 100644 test/unit/app/managers/test_base.py diff --git a/lib/galaxy/managers/base.py b/lib/galaxy/managers/base.py index 1b413310b6c..77e538003d5 100644 --- a/lib/galaxy/managers/base.py +++ b/lib/galaxy/managers/base.py @@ -68,7 +68,6 @@ from galaxy.structured_app import ( BasicSharedApp, MinimalManagerApp, ) -from galaxy.util import munge_lists from galaxy.web import url_for as gx_url_for log = logging.getLogger(__name__) @@ -462,7 +461,7 @@ class ModelManager(Generic[U]): if not ids: return [] ids_filter = parsed_filter("orm", self.model_class.__table__.c.id.in_(ids)) - found = self.list(filters=munge_lists(ids_filter, filters), **kwargs) + found = self.list(filters=combine_lists(ids_filter, filters), **kwargs) # TODO: this does not order by the original 'ids' array # ...could use get (supposedly since found are in the session, the db won't be hit twice) @@ -1364,3 +1363,24 @@ class StorageCleanerManager(Protocol): def cleanup_items(self, user: model.User, item_ids: Set[int]) -> StorageItemsCleanupResult: """Purges the given list of items by ID. The items must be owned by the user.""" raise NotImplementedError + + +def combine_lists(listA: Any, listB: Any) -> List: + """ + Combine two lists into a single list. + + Arguments can be None, non-lists, or lists. If an argument is None, it will + not be included in the returned list. If both arguments are None, an empty + list will be returned. + """ + + def make_list(item): + # Check for None explicitly: __bool__ may be overwritten. + if item is None: + return [] + elif isinstance(item, (list, tuple)): + return list(item) + else: + return [item] + + return make_list(listA) + make_list(listB) diff --git a/lib/galaxy/managers/histories.py b/lib/galaxy/managers/histories.py index 22166843f34..7fef5a46c04 100644 --- a/lib/galaxy/managers/histories.py +++ b/lib/galaxy/managers/histories.py @@ -37,6 +37,7 @@ from galaxy.managers import ( sharable, ) from galaxy.managers.base import ( + combine_lists, ModelDeserializingError, Serializer, SortableManager, @@ -60,7 +61,6 @@ from galaxy.schema.storage_cleaner import ( ) from galaxy.security.validate_user_input import validate_preferred_object_store_id from galaxy.structured_app import MinimalManagerApp -from galaxy.util import munge_lists log = logging.getLogger(__name__) @@ -136,7 +136,7 @@ class HistoryManager(sharable.SharableModelManager, deletable.PurgableManagerMix if self.user_manager.is_anonymous(user): return None if (not current_history or current_history.deleted) else current_history desc_update_time = desc(self.model_class.update_time) - filters = munge_lists(filters, self.model_class.user_id == user.id) + filters = combine_lists(filters, self.model_class.user_id == user.id) # TODO: normalize this return value return self.query(filters=filters, order_by=desc_update_time, limit=1, **kwargs).first() diff --git a/lib/galaxy/managers/sharable.py b/lib/galaxy/managers/sharable.py index 6eadd811028..244b3208159 100644 --- a/lib/galaxy/managers/sharable.py +++ b/lib/galaxy/managers/sharable.py @@ -34,6 +34,7 @@ from galaxy.managers import ( taggable, users, ) +from galaxy.managers.base import combine_lists from galaxy.model import ( User, UserShareAssociation, @@ -45,10 +46,7 @@ from galaxy.schema.schema import ( SharingOptions, ) from galaxy.structured_app import MinimalManagerApp -from galaxy.util import ( - munge_lists, - ready_name_for_url, -) +from galaxy.util import ready_name_for_url from galaxy.util.hash_util import md5_hash_str if TYPE_CHECKING: @@ -86,7 +84,7 @@ class SharableModelManager( `user`. """ user_filter = self.model_class.table.c.user_id == user.id - filters = munge_lists(user_filter, kwargs.get("filters", None)) + filters = combine_lists(user_filter, kwargs.get("filters", None)) return self.list(filters=filters, **kwargs) # .... owned/accessible interfaces @@ -157,7 +155,7 @@ class SharableModelManager( Return a query for all published items. """ published_filter = self.model_class.table.c.published == true() - filters = munge_lists(published_filter, filters) + filters = combine_lists(published_filter, filters) return self.query(filters=filters, **kwargs) def list_published(self, filters=None, **kwargs): @@ -165,7 +163,7 @@ class SharableModelManager( Return a list of all published items. """ published_filter = self.model_class.table.c.published == true() - filters = munge_lists(published_filter, filters) + filters = combine_lists(published_filter, filters) return self.list(filters=filters, **kwargs) # .... user sharing diff --git a/lib/galaxy/managers/users.py b/lib/galaxy/managers/users.py index f2aca660ec8..65ef2a7919f 100644 --- a/lib/galaxy/managers/users.py +++ b/lib/galaxy/managers/users.py @@ -37,6 +37,7 @@ from galaxy.managers import ( base, deletable, ) +from galaxy.managers.base import combine_lists from galaxy.model import ( User, UserAddress, @@ -54,7 +55,6 @@ from galaxy.structured_app import ( BasicSharedApp, MinimalManagerApp, ) -from galaxy.util import munge_lists from galaxy.util.hash_util import new_secure_hash_v2 from galaxy.web import url_for @@ -268,7 +268,7 @@ class UserManager(base.ModelManager, deletable.PurgableManagerMixin): """ Find a user by their email. """ - filters = munge_lists(self.model_class.email == email, filters) + filters = combine_lists(self.model_class.email == email, filters) try: # TODO: use one_or_none return super().one(filters=filters, **kwargs) @@ -322,7 +322,7 @@ class UserManager(base.ModelManager, deletable.PurgableManagerMixin): Return a list of admin Users. """ admin_emails = self.app.config.admin_users_list - filters = munge_lists(self.model_class.email.in_(admin_emails), filters) + filters = combine_lists(self.model_class.email.in_(admin_emails), filters) return super().list(filters=filters, **kwargs) def error_unless_admin(self, user, msg="Administrators only", **kwargs): diff --git a/lib/galaxy/util/__init__.py b/lib/galaxy/util/__init__.py index 451895fc981..afeb7307e54 100644 --- a/lib/galaxy/util/__init__.py +++ b/lib/galaxy/util/__init__.py @@ -1882,14 +1882,3 @@ def enum_values(enum_class): Values are in member definition order. """ return [value.value for value in enum_class.__members__.values()] - - -def munge_lists(listA: Any, listB: Any) -> List: - """ - Combine two lists into a single list. - - Arguments can be None, non-lists, or lists. If an argument is None, it will - not be included in the returned list. If both arguments are None, an empty - list will be returned. - """ - return listify(listA) + listify(listB) diff --git a/test/unit/app/managers/test_base.py b/test/unit/app/managers/test_base.py new file mode 100644 index 00000000000..7c03c87daa3 --- /dev/null +++ b/test/unit/app/managers/test_base.py @@ -0,0 +1,20 @@ +from galaxy.managers.base import combine_lists + + +class NotFalsy: + """Class requires explicit check for None""" + + def __bool__(self): + raise Exception("not implemented") + + +def test_combine_lists(): + foo, bar = NotFalsy(), NotFalsy() + assert combine_lists(foo, None) == [foo] + assert combine_lists(None, foo) == [foo] + assert combine_lists(foo, bar) == [foo, bar] + assert combine_lists([foo, bar], None) == [foo, bar] + assert combine_lists(None, [foo, bar]) == [foo, bar] + assert combine_lists((foo, bar), None) == [foo, bar] + assert combine_lists(None, (foo, bar)) == [foo, bar] + assert combine_lists(None, None) == [] diff --git a/test/unit/util/test_utils.py b/test/unit/util/test_utils.py index a6ea82dcdb2..34d7cb95093 100644 --- a/test/unit/util/test_utils.py +++ b/test/unit/util/test_utils.py @@ -149,20 +149,3 @@ def test_enum_values(): B = "b" assert util.enum_values(Stuff) == ["a", "c", "b"] - - -class NotFalsy: - """Class requires explicit check for None""" - - def __bool__(self): - raise Exception("not implemented") - - -def test_munge_lists(): - foo, bar = NotFalsy(), NotFalsy() - assert util.munge_lists(foo, None) == [foo] - assert util.munge_lists(None, foo) == [foo] - assert util.munge_lists(foo, bar) == [foo, bar] - assert util.munge_lists([foo, bar], None) == [foo, bar] - assert util.munge_lists(None, [foo, bar]) == [foo, bar] - assert util.munge_lists(None, None) == []