Merge pull request #219 from ral-facilities/bugfix/fix-distinct-filte…

…r-#141

Fix Distinct Filter on ICAT Backend

datagateway_api/common/icat/filters.py

@@ -117,22 +117,30 @@ def __init__(self, fields):
     def apply_filter(self, query):
         try:
             log.info("Adding ICAT distinct filter to ICAT query")
-            if (
-                query.aggregate == "COUNT"
-                or query.aggregate == "AVG"
-                or query.aggregate == "SUM"
-            ):
+            log.debug("Fields for distinct filter: %s", self.fields)
+
+            # These aggregate keywords not currently used in the API, but conditional
+            # present in case they're used in the future
+            if query.aggregate == "AVG" or query.aggregate == "SUM":
                 # Distinct can be combined with other aggregate functions
                 query.setAggregate(f"{query.aggregate}:DISTINCT")
+            elif query.aggregate == "COUNT":
+                # When count and distinct keywords are used together when selecting
+                # multiple attributes, Python ICAT will always throw an error on query
+                # execution (more info:
+                # https://github.com/icatproject/python-icat/issues/76). This appears to
+                # be a JPQL limitation, something that cannot be fixed in Python ICAT.
+                # As a result, the API will get the distinct results and manually
+                # perform `len()` on the list, using `manual_count` as a flag to
+                # recognise this situation
+                query.setAggregate("DISTINCT")
+                log.debug("Manual count flag enabled")
+                query.manual_count = True
             else:
                 query.setAggregate("DISTINCT")
 
-            # Using where filters to identify which fields to apply distinct too
-            for field in self.fields:
-                where_filter = PythonICATWhereFilter(field, "null", "ne")
-                where_filter.apply_filter(query)
+            query.setAttributes(self.fields)
 
-            log.debug("Fields for distinct filter: %s", self.fields)
         except ValueError as e:
             raise FilterError(e)
 

datagateway_api/common/icat/helpers.py

@@ -553,6 +553,7 @@ def get_facility_cycles_for_instrument(
 
     query_aggregate = "COUNT:DISTINCT" if count_query else "DISTINCT"
     query = ICATQuery(client, "FacilityCycle", aggregate=query_aggregate)
+    query.isis_endpoint = True
 
     instrument_id_check = PythonICATWhereFilter(
         "facility.instruments.id", instrument_id, "eq",
@@ -633,6 +634,7 @@ def get_investigations_for_instrument_in_facility_cycle(
 
     query_aggregate = "COUNT:DISTINCT" if count_query else "DISTINCT"
     query = ICATQuery(client, "Investigation", aggregate=query_aggregate)
+    query.isis_endpoint = True
 
     instrument_id_check = PythonICATWhereFilter(
         "facility.instruments.id", instrument_id, "eq",

pyproject.toml

@@ -17,7 +17,7 @@ Flask-Cors = "3.0.9"
 apispec = "3.3.0"
 flask-swagger-ui = "3.25.0"
 PyYAML = "5.4"
-python-icat = "0.17.0"
+python-icat = "0.18.1"
 suds-community = "^0.8.4"
 Flask-SQLAlchemy = "^2.4.4"
 requests = "^2.25.1"

test/icat/endpoints/test_count_with_filters_icat.py

@@ -2,24 +2,44 @@
 
 
 class TestICATCountWithFilters:
-    @pytest.mark.usefixtures("single_investigation_test_data")
+    @pytest.mark.parametrize(
+        "query_params, expected_result",
+        [
+            pytest.param(
+                '?where={"title": {"like": "Test data for the Python ICAT Backend on'
+                ' DataGateway API"}}',
+                5,
+                id="Filter on test data",
+            ),
+            pytest.param(
+                '?where={"title": {"like": "Test data for the Python ICAT Backend on'
+                ' DataGateway API"}}&distinct=["startDate"]',
+                1,
+                id="Distinct test data",
+            ),
+        ],
+    )
+    @pytest.mark.usefixtures("multiple_investigation_test_data")
     def test_valid_count_with_filters(
-        self, flask_test_app_icat, valid_icat_credentials_header,
+        self,
+        flask_test_app_icat,
+        valid_icat_credentials_header,
+        query_params,
+        expected_result,
     ):
         test_response = flask_test_app_icat.get(
-            '/investigations/count?where={"title": {"like": "Test data for the Python'
-            ' ICAT Backend on DataGateway API"}}',
+            f"/investigations/count{query_params}",
             headers=valid_icat_credentials_header,
         )
 
-        assert test_response.json == 1
+        assert test_response.json == expected_result
 
     def test_valid_no_results_count_with_filters(
         self, flask_test_app_icat, valid_icat_credentials_header,
     ):
         test_response = flask_test_app_icat.get(
-            '/investigations/count?where={"title": {"like": "This filter should cause a'
-            '404 for testing purposes..."}}',
+            '/investigations/count?where={"title": {"like": "This filter should cause 0'
+            ' results to be found for testing purposes..."}}',
             headers=valid_icat_credentials_header,
         )
 

test/icat/filters/test_distinct_filter.py

@@ -5,12 +5,19 @@
 
 
 class TestICATDistinctFilter:
-    def test_valid_str_field_input(self, icat_query):
-        test_filter = PythonICATDistinctFieldFilter("name")
+    @pytest.mark.parametrize(
+        "attribute_name",
+        [
+            pytest.param("name", id="Attribute for own entity"),
+            pytest.param("investigationUsers.role", id="Related attribute name"),
+        ],
+    )
+    def test_valid_str_field_input(self, icat_query, attribute_name):
+        test_filter = PythonICATDistinctFieldFilter(attribute_name)
         test_filter.apply_filter(icat_query)
 
         assert (
-            icat_query.conditions == {"name": "!= null"}
+            icat_query.attributes == [attribute_name]
             and icat_query.aggregate == "DISTINCT"
         )
 
@@ -19,8 +26,7 @@ def test_valid_list_fields_input(self, icat_query):
         test_filter.apply_filter(icat_query)
 
         assert (
-            icat_query.conditions
-            == {"doi": "!= null", "name": "!= null", "title": "!= null"}
+            icat_query.attributes == ["doi", "name", "title"]
             and icat_query.aggregate == "DISTINCT"
         )
 
@@ -35,11 +41,30 @@ def test_distinct_aggregate_added(self, icat_query):
 
         assert icat_query.aggregate == "DISTINCT"
 
-    @pytest.mark.parametrize("existing_aggregate", ["COUNT", "AVG", "SUM"])
-    def test_existing_aggregate_appended(self, icat_query, existing_aggregate):
+    @pytest.mark.parametrize(
+        "existing_aggregate, expected_instance_aggregate",
+        [
+            pytest.param(
+                "COUNT", "DISTINCT", id="Existing count aggregate (count endpoints)",
+            ),
+            pytest.param("AVG", "AVG:DISTINCT", id="Existing avg aggregate"),
+            pytest.param("SUM", "SUM:DISTINCT", id="Existing sum aggregate"),
+        ],
+    )
+    def test_existing_aggregate_on_query(
+        self, icat_query, existing_aggregate, expected_instance_aggregate,
+    ):
         icat_query.setAggregate(existing_aggregate)
 
         test_filter = PythonICATDistinctFieldFilter("name")
         test_filter.apply_filter(icat_query)
 
-        assert icat_query.aggregate == f"{existing_aggregate}:DISTINCT"
+        assert icat_query.aggregate == expected_instance_aggregate
+
+    def test_manual_count_flag(self, icat_query):
+        icat_query.setAggregate("COUNT")
+
+        test_filter = PythonICATDistinctFieldFilter("name")
+        test_filter.apply_filter(icat_query)
+
+        assert icat_query.manual_count