Merge pull request #240 from ral-facilities/bugfix/join-specs-order-#238

Allow One-Many Related Orderings to not miss out blank results

.github/workflows/ci-build.yml

@@ -81,7 +81,7 @@ jobs:
 
     - name: Add dummy data to icatdb
       run: |
-        poetry run python -m util.icat_db_generator -s 4 -y 3
+        poetry run python -m util.icat_db_generator
 
     # Run Nox tests session, saves and uploads a coverage report to codecov
     - name: Run Nox tests session

README.md

@@ -764,11 +764,13 @@ Within the repository, there are some useful files which can help with using the
 ## Database Generator
 There is a tool to generate mock data into ICAT's database. It is located in
 `util/icat_db_generator.py`. By default it will generate 20 years worth of data (approx
-70,000 entities). The script makes use of `random` and `Faker` and is seeded with a seed
-of 1. The seed and number of years of data generated can be changed by using the arg
-flags `-s` or `--seed` for the seed, and `-y` or `--years` for the number of years. For
-example: `python -m util.icat_db_generator -s 4 -y 10` Would set the seed to 4 and
-generate 10 years of data.
+70,000 entities). The default arguments will match the data on SciGateway Preprod and
+therefore this is usually a good starting point. The script makes use of `random` and
+`Faker` and is seeded with a seed of 1. The seed and number of years of data generated
+can be changed by using the arg flags `-s` or `--seed` for the seed, and `-y` or
+`--years` for the number of years. For example:
+`python -m util.icat_db_generator -s 4 -y 10` Would set the seed to 4 and generate 10
+years of data.
 
 This uses code from the API's Database Backend, so a suitable `db_url` should be
 configured in `config.json`.

datagateway_api/common/filter_order_handler.py

@@ -82,6 +82,7 @@ def clear_python_icat_order_filters(self):
             for icat_filter in self.filters
         ):
             PythonICATOrderFilter.result_order = []
+            PythonICATOrderFilter.join_specs = {}
 
     def manage_icat_filters(self, filters, query):
         """

datagateway_api/common/icat/filters.py

@@ -148,6 +148,9 @@ def apply_filter(self, query):
 class PythonICATOrderFilter(OrderFilter):
     # Used to append the order tuples across all filters in a single request
     result_order = []
+    # When 1-many relationships occur, entities should be joined using a LEFT JOIN to
+    # prevent disappearing results when sorting on DataGateway's table view
+    join_specs = {}
 
     def __init__(self, field, direction):
         # Python ICAT doesn't automatically uppercase the direction, errors otherwise
@@ -161,10 +164,36 @@ def apply_filter(self, query):
             log.info("Adding order filter (for %s)", self.field)
             query.setOrder(PythonICATOrderFilter.result_order)
         except ValueError as e:
-            # Typically either invalid attribute(s) or attribute(s) contains 1-many
-            # relationship
+            # Typically invalid attribute(s)
             raise FilterError(e)
 
+        split_fields = self.field.split(".")
+        for field_pointer in range(len(split_fields)):
+            # Looking for plural entities but not field names
+            # This is to avoid adding JOINs to field names such as job's argument field
+            if (
+                split_fields[field_pointer].endswith("s")
+                and split_fields[field_pointer] != split_fields[-1]
+            ):
+                # Length minus 1 is used to omit field names, same reason as above
+                for join_field_pointer in range(field_pointer, len(split_fields) - 1):
+                    join_field_list = split_fields[
+                        field_pointer : join_field_pointer + 1
+                    ]
+                    join_field_str = ".".join(join_field_list)
+
+                    PythonICATOrderFilter.join_specs[join_field_str] = "LEFT JOIN"
+
+                log.debug(
+                    "Setting query join specs: %s", PythonICATOrderFilter.join_specs,
+                )
+                try:
+                    query.setJoinSpecs(PythonICATOrderFilter.join_specs)
+                except (TypeError, ValueError) as e:
+                    raise FilterError(e)
+
+                break
+
 
 class PythonICATSkipFilter(SkipFilter):
     def __init__(self, skip_value):

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.18.1"
+python-icat = "0.19.0"
 suds-community = "^0.8.4"
 py-object-pool = "^1.1"
 cachetools = "^4.2.1"

test/icat/filters/test_order_filter.py

@@ -30,6 +30,50 @@ def test_result_order_appended(self, icat_query):
 
         filter_handler.clear_python_icat_order_filters()
 
+    def test_join_specs_added(self, icat_query):
+        pid_filter = PythonICATOrderFilter("studyInvestigations.study.pid", "ASC")
+        name_filter = PythonICATOrderFilter(
+            "investigationInstruments.instrument.name", "DESC",
+        )
+
+        filter_handler = FilterOrderHandler()
+        filter_handler.add_filters([pid_filter, name_filter])
+        filter_handler.apply_filters(icat_query)
+
+        assert PythonICATOrderFilter.join_specs == {
+            "studyInvestigations": "LEFT JOIN",
+            "studyInvestigations.study": "LEFT JOIN",
+            "investigationInstruments": "LEFT JOIN",
+            "investigationInstruments.instrument": "LEFT JOIN",
+        }
+
+        filter_handler.clear_python_icat_order_filters()
+
+    def test_valid_one_many_related_ordering(self, icat_query):
+        pid_filter = PythonICATOrderFilter("studyInvestigations.study.pid", "DESC")
+        filter_handler = FilterOrderHandler()
+        filter_handler.add_filter(pid_filter)
+        filter_handler.apply_filters(icat_query)
+
+        assert icat_query.join_specs == {
+            "studyInvestigations": "LEFT JOIN",
+            "studyInvestigations.study": "LEFT JOIN",
+        }
+
+        filter_handler.clear_python_icat_order_filters()
+
+    def test_invalid_one_many_related_ordering(self, icat_query):
+        pid_filter = PythonICATOrderFilter("studyInvestigations.study.pid", "DESC")
+        filter_handler = FilterOrderHandler()
+        filter_handler.add_filter(pid_filter)
+
+        PythonICATOrderFilter.join_specs["testEntities"] = "LEFT JOIN"
+
+        with pytest.raises(FilterError):
+            filter_handler.apply_filters(icat_query)
+
+        filter_handler.clear_python_icat_order_filters()
+
     def test_filter_applied_to_query(self, icat_query):
         test_filter = PythonICATOrderFilter("id", "DESC")
 

test/icat/test_query.py

@@ -166,13 +166,13 @@ def test_invalid_query_creation(self, icat_client):
                         " API 0",
                         "facility": {
                             "createId": "user",
-                            "createTime": "2011-01-29 06:19:43+00:00",
+                            "createTime": "2002-11-27 06:20:36+00:00",
                             "daysUntilRelease": 10,
                             "description": "Lorem ipsum light source",
                             "fullName": None,
                             "id": 1,
                             "modId": "user",
-                            "modTime": "2008-10-15 12:05:09+00:00",
+                            "modTime": "2005-04-30 19:41:49+00:00",
                             "name": "LILS",
                             "url": None,
                         },