From 8ff439d60cde9b4c4502bf1c3d600cafd9631475 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Thu, 11 May 2023 15:00:24 +0200 Subject: [PATCH 1/4] Add count support for listing filters Allows to get the total_matches of a query (without offset or limit) to be able to handle pagination in a deterministic way. --- lib/galaxy/managers/base.py | 21 ++++++++++ test/unit/app/managers/test_HistoryManager.py | 38 +++++++++++++++++-- 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/lib/galaxy/managers/base.py b/lib/galaxy/managers/base.py index 829df35cdca..4cfd29c44ec 100644 --- a/lib/galaxy/managers/base.py +++ b/lib/galaxy/managers/base.py @@ -388,6 +388,27 @@ class ModelManager(Generic[U]): items = self._apply_fn_filters_gen(items, fn_filters) return list(self._apply_fn_limit_offset_gen(items, limit, offset)) + def count(self, filters=None, **kwargs): + """ + Returns the number of objects matching the given filters. + + If the filters include functional filters, the count is performed by getting all objects + and then applying the functional filters which may impact performance. + """ + # TODO: requires case sensitivity fix from https://github.com/galaxyproject/galaxy/pull/16036 + orm_filters, fn_filters = self._split_filters(filters) + query = self.query(filters=orm_filters, **kwargs) + if not fn_filters: + # if no fn_filtering required, we can use count + return query.count() + + # TODO: raise an error instead to avoid performance issues? + + # fn filters will change the number of items + items = query.all() + items = self._apply_fn_filters_gen(items, fn_filters) + return len(list(items)) + def _handle_filters_case_sensitivity(self, filters): """Modifies the filters to make them case insensitive if needed.""" if filters is None: diff --git a/test/unit/app/managers/test_HistoryManager.py b/test/unit/app/managers/test_HistoryManager.py index 6be12582246..1a1aa9b237b 100644 --- a/test/unit/app/managers/test_HistoryManager.py +++ b/test/unit/app/managers/test_HistoryManager.py @@ -938,6 +938,38 @@ class TestHistoryFilters(BaseTestCase): found = self.history_manager.list(filters=filters, offset=-1) assert found == deleted_and_annotated - # TODO: eq, ge, le - # def test_ratings( self ): - # pass + def test_count(self): + user2 = self.user_manager.create(**user2_data) + history1 = self.history_manager.create(name="history1", user=user2) + history2 = self.history_manager.create(name="history2", user=user2) + history3 = self.history_manager.create(name="history3", user=user2) + history4 = self.history_manager.create(name="history4", user=user2) + + self.history_manager.delete(history1) + self.history_manager.delete(history2) + self.history_manager.delete(history3) + + test_annotation = "testing" + history2.add_item_annotation(self.trans.sa_session, user2, history2, test_annotation) + self.trans.sa_session.flush() + history3.add_item_annotation(self.trans.sa_session, user2, history3, test_annotation) + self.trans.sa_session.flush() + history3.add_item_annotation(self.trans.sa_session, user2, history4, test_annotation) + self.trans.sa_session.flush() + + all_histories = [history1, history2, history3, history4] + deleted = [history1, history2, history3] + annotated = [history2, history3, history4] + deleted_and_annotated = [history2, history3] + + self.log("no filters should work") + assert self.history_manager.count() == len(all_histories) + self.log("orm filtered should work") + filters = [model.History.deleted == true()] + assert self.history_manager.count(filters=filters) == len(deleted) + self.log("fn filtered should work") + filters = self.filter_parser.parse_filters([("annotation", "has", test_annotation)]) + assert self.history_manager.count(filters=filters) == len(annotated) + self.log("orm and fn filtered should work") + filters = self.filter_parser.parse_filters([("deleted", "eq", "True"), ("annotation", "has", test_annotation)]) + assert self.history_manager.count(filters=filters) == len(deleted_and_annotated) From 09dfcdbb29ef7737de4d3a3c6eb77395bb860c3b Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Fri, 12 May 2023 11:09:10 +0200 Subject: [PATCH 2/4] Do not support functional filters in count The existing functional filters should be either translated to ORM filters or dropped. They won't be supported in count. --- lib/galaxy/managers/base.py | 18 +++++---------- test/unit/app/managers/test_HistoryManager.py | 23 ++++++++++++------- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/lib/galaxy/managers/base.py b/lib/galaxy/managers/base.py index 4cfd29c44ec..f12f28ebe99 100644 --- a/lib/galaxy/managers/base.py +++ b/lib/galaxy/managers/base.py @@ -392,22 +392,16 @@ class ModelManager(Generic[U]): """ Returns the number of objects matching the given filters. - If the filters include functional filters, the count is performed by getting all objects - and then applying the functional filters which may impact performance. + If the filters include functional filters, this function will raise an exception as they might cause + performance issues. """ # TODO: requires case sensitivity fix from https://github.com/galaxyproject/galaxy/pull/16036 orm_filters, fn_filters = self._split_filters(filters) + if fn_filters: + raise exceptions.RequestParameterInvalidException("Counting with functional filters is not supported.") + query = self.query(filters=orm_filters, **kwargs) - if not fn_filters: - # if no fn_filtering required, we can use count - return query.count() - - # TODO: raise an error instead to avoid performance issues? - - # fn filters will change the number of items - items = query.all() - items = self._apply_fn_filters_gen(items, fn_filters) - return len(list(items)) + return query.count() def _handle_filters_case_sensitivity(self, filters): """Modifies the filters to make them case insensitive if needed.""" diff --git a/test/unit/app/managers/test_HistoryManager.py b/test/unit/app/managers/test_HistoryManager.py index 1a1aa9b237b..3d41a16adfc 100644 --- a/test/unit/app/managers/test_HistoryManager.py +++ b/test/unit/app/managers/test_HistoryManager.py @@ -2,6 +2,7 @@ """ from unittest import mock +import pytest import sqlalchemy from sqlalchemy import true @@ -959,17 +960,23 @@ class TestHistoryFilters(BaseTestCase): all_histories = [history1, history2, history3, history4] deleted = [history1, history2, history3] - annotated = [history2, history3, history4] - deleted_and_annotated = [history2, history3] self.log("no filters should work") assert self.history_manager.count() == len(all_histories) self.log("orm filtered should work") filters = [model.History.deleted == true()] assert self.history_manager.count(filters=filters) == len(deleted) - self.log("fn filtered should work") - filters = self.filter_parser.parse_filters([("annotation", "has", test_annotation)]) - assert self.history_manager.count(filters=filters) == len(annotated) - self.log("orm and fn filtered should work") - filters = self.filter_parser.parse_filters([("deleted", "eq", "True"), ("annotation", "has", test_annotation)]) - assert self.history_manager.count(filters=filters) == len(deleted_and_annotated) + + raw_annotation_fn_filter = ("annotation", "has", test_annotation) + self.log("fn filtered is not supported") + with pytest.raises(exceptions.RequestParameterInvalidException) as exc: + filters = self.filter_parser.parse_filters([raw_annotation_fn_filter]) + self.history_manager.count(filters=filters) + assert "not supported" in str(exc) + + raw_deleted_orm_filter = ("deleted", "eq", "True") + self.log("mixin orm and fn filtered is not supported") + with pytest.raises(exceptions.RequestParameterInvalidException) as exc: + filters = self.filter_parser.parse_filters([raw_deleted_orm_filter, raw_annotation_fn_filter]) + self.history_manager.count(filters=filters) + assert "not supported" in str(exc) From 1206926ce37cd20d0cea752040addf3db8124379 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Fri, 12 May 2023 11:26:54 +0200 Subject: [PATCH 3/4] Turn test logs into assert messages This test file is full of them, and it is probably better to split all of them into their own test cases but it will require a bigger refactoring... --- test/unit/app/managers/test_HistoryManager.py | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/test/unit/app/managers/test_HistoryManager.py b/test/unit/app/managers/test_HistoryManager.py index 3d41a16adfc..1ed4c9d3b42 100644 --- a/test/unit/app/managers/test_HistoryManager.py +++ b/test/unit/app/managers/test_HistoryManager.py @@ -961,22 +961,19 @@ class TestHistoryFilters(BaseTestCase): all_histories = [history1, history2, history3, history4] deleted = [history1, history2, history3] - self.log("no filters should work") - assert self.history_manager.count() == len(all_histories) - self.log("orm filtered should work") + assert self.history_manager.count() == len(all_histories), "having no filters should count all histories" filters = [model.History.deleted == true()] - assert self.history_manager.count(filters=filters) == len(deleted) + assert self.history_manager.count(filters=filters) == len(deleted), "counting with orm filters should work" raw_annotation_fn_filter = ("annotation", "has", test_annotation) - self.log("fn filtered is not supported") - with pytest.raises(exceptions.RequestParameterInvalidException) as exc: + # functional filtering is not supported + with pytest.raises(exceptions.RequestParameterInvalidException) as exc_info: filters = self.filter_parser.parse_filters([raw_annotation_fn_filter]) self.history_manager.count(filters=filters) - assert "not supported" in str(exc) + assert "not supported" in str(exc_info) raw_deleted_orm_filter = ("deleted", "eq", "True") - self.log("mixin orm and fn filtered is not supported") - with pytest.raises(exceptions.RequestParameterInvalidException) as exc: + with pytest.raises(exceptions.RequestParameterInvalidException) as exc_info: filters = self.filter_parser.parse_filters([raw_deleted_orm_filter, raw_annotation_fn_filter]) self.history_manager.count(filters=filters) - assert "not supported" in str(exc) + assert "not supported" in str(exc_info) From 0c3ee1a8f97ce11cfe56589e522698b4e8522f34 Mon Sep 17 00:00:00 2001 From: davelopez <46503462+davelopez@users.noreply.github.com> Date: Mon, 15 May 2023 19:00:55 +0200 Subject: [PATCH 4/4] Handle case insensitive filters on count --- lib/galaxy/managers/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/managers/base.py b/lib/galaxy/managers/base.py index f12f28ebe99..57106c91720 100644 --- a/lib/galaxy/managers/base.py +++ b/lib/galaxy/managers/base.py @@ -395,7 +395,7 @@ class ModelManager(Generic[U]): If the filters include functional filters, this function will raise an exception as they might cause performance issues. """ - # TODO: requires case sensitivity fix from https://github.com/galaxyproject/galaxy/pull/16036 + self._handle_filters_case_sensitivity(filters) orm_filters, fn_filters = self._split_filters(filters) if fn_filters: raise exceptions.RequestParameterInvalidException("Counting with functional filters is not supported.")