From 93eb4686670299b75e8e164ba878a4106374b30c Mon Sep 17 00:00:00 2001
From: Matthew Richards <matthew.richards@stfc.ac.uk>
Date: Thu, 20 Aug 2020 15:07:34 +0000
Subject: [PATCH]  #139: Allow a request to have both limit and skip filters

- The skip filter will be merged into the limit filter, with the skip filter being removed from the filter handler
- There is a bug when using a skip filter on it's own, as it'll always return a 404
---
 common/database/filters.py     |  1 +
 common/filter_order_handler.py |  3 +++
 common/icat/filters.py         | 26 ++++++++++++++++++++---
 common/icat/helpers.py         | 39 +++++++++++++++++++++++++++++++++-
 4 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/common/database/filters.py b/common/database/filters.py
index 74f411af..6822355e 100644
--- a/common/database/filters.py
+++ b/common/database/filters.py
@@ -10,6 +10,7 @@
 
 from sqlalchemy import asc, desc
 
+
 class DatabaseWhereFilter(WhereFilter):
     def __init__(self, field, value, operation):
         super().__init__(field, value, operation)
diff --git a/common/filter_order_handler.py b/common/filter_order_handler.py
index aa799216..92a63721 100644
--- a/common/filter_order_handler.py
+++ b/common/filter_order_handler.py
@@ -13,6 +13,9 @@ def add_filter(self, filter):
     def add_filters(self, filters):
         self.filters.extend(filters)
 
+    def remove_filter(self, filter):
+        self.filters.remove(filter)
+
     def sort_filters(self):
         """
         Sorts the filters according to the order of operations
diff --git a/common/icat/filters.py b/common/icat/filters.py
index cb2e7689..7215adb9 100644
--- a/common/icat/filters.py
+++ b/common/icat/filters.py
@@ -96,21 +96,41 @@ def apply_filter(self, query):
             )
 
 
-
 class PythonICATSkipFilter(SkipFilter):
     def __init__(self, skip_value):
         super().__init__(skip_value)
 
     def apply_filter(self, query):
-        pass
+        icat_set_limit(query, self.skip_value, False)
 
 
 class PythonICATLimitFilter(LimitFilter):
     def __init__(self, limit_value):
         super().__init__(limit_value)
+        self.skip_value = 0
 
     def apply_filter(self, query):
-        pass
+        icat_set_limit(query, self.skip_value, self.limit_value)
+
+
+def icat_set_limit(query, skip_number, limit_number):
+    """
+    Add limit (utilising skip and count) to an ICAT query
+
+    :param query: ICAT Query object to execute within Python ICAT
+    :type query: :class:`icat.query.Query`
+    :param skip_number:
+    :type skip_number: :class:`int`
+    :param limit_number:
+    :type limit_number: :class:`int
+    :raises FilterError: If the tuple is not of two elements, or the elements aren't of
+        the valid type
+    """
+    try:
+        query.setLimit((skip_number, limit_number))
+    except TypeError as e:
+        # Not a two element tuple as managed by Python ICAT's setLimit()
+        raise FilterError(e)
 
 
 class PythonICATIncludeFilter(IncludeFilter):
diff --git a/common/icat/helpers.py b/common/icat/helpers.py
index a3859a23..459eafb8 100644
--- a/common/icat/helpers.py
+++ b/common/icat/helpers.py
@@ -12,7 +12,11 @@
 )
 from common.filter_order_handler import FilterOrderHandler
 from common.constants import Constants
-from common.icat.filters import PythonICATLimitFilter, PythonICATWhereFilter
+from common.icat.filters import (
+    PythonICATLimitFilter,
+    PythonICATWhereFilter,
+    PythonICATSkipFilter,
+)
 
 
 log = logging.getLogger()
@@ -364,7 +368,9 @@ def get_entity_with_filters(client, table_name, filters):
     query = construct_icat_query(client, selected_entity_name)
     filter_handler = FilterOrderHandler()
     filter_handler.add_filters(filters)
+    merge_limit_skip_filters(filter_handler)
     filter_handler.apply_filters(query)
+    
 
     data = execute_icat_query(client, query, True)
 
@@ -372,3 +378,34 @@ def get_entity_with_filters(client, table_name, filters):
         raise MissingRecordError("No results found")
     else:
         return data
+
+
+def merge_limit_skip_filters(filter_handler):
+    """
+    When there are both limit and skip filters in a request, merge them into the limit
+    filter and remove the skip filter from `filter_handler`
+
+    :param filter_handler: The filter handler to apply the filters
+    :param filters: The filters to be applied
+    """
+
+    if any(
+        isinstance(filter, PythonICATSkipFilter) for filter in filter_handler.filters
+    ) and any(
+        isinstance(filter, PythonICATLimitFilter) for filter in filter_handler.filters
+    ):
+        # Merge skip and limit filter into a single limit filter
+        for filter in filter_handler.filters:
+            if isinstance(filter, PythonICATSkipFilter):
+                skip_filter = filter
+                request_skip_value = filter.skip_value
+
+            if isinstance(filter, PythonICATLimitFilter):
+                limit_filter = filter
+
+        if skip_filter and limit_filter:
+            log.info("Merging skip filter with limit filter")
+            limit_filter.skip_value = skip_filter.skip_value
+            log.info("Removing skip filter from list of filters")
+            filter_handler.remove_filter(skip_filter)
+            log.debug("Filters: %s", filter_handler.filters)