From 4bee04b70f3fbffbb00158c7825cc9319d8f6570 Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Tue, 18 Aug 2020 12:03:14 +0000 Subject: [PATCH 1/8] #140: Fix DB order filter from an import error --- common/database/filters.py | 1 + common/database/helpers.py | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/common/database/filters.py b/common/database/filters.py index 2604dd41..74f411af 100644 --- a/common/database/filters.py +++ b/common/database/filters.py @@ -8,6 +8,7 @@ ) from common.exceptions import FilterError +from sqlalchemy import asc, desc class DatabaseWhereFilter(WhereFilter): def __init__(self, field, value, operation): diff --git a/common/database/helpers.py b/common/database/helpers.py index 57c57480..b06c5313 100644 --- a/common/database/helpers.py +++ b/common/database/helpers.py @@ -3,7 +3,6 @@ from abc import ABC, abstractmethod from functools import wraps -from sqlalchemy import asc, desc from sqlalchemy.orm import aliased from common.exceptions import ( From 78ef98e33809c65902299413c5c200ba34fa518c Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Tue, 18 Aug 2020 12:12:42 +0000 Subject: [PATCH 2/8] #140: Move FilterOrderHandler into its own file - This will remove circular imports in a later change - Also helps to tidy up the code base by separating the filter handler from the abstract classes of the filters --- common/database/helpers.py | 2 +- common/filter_order_handler.py | 35 ++++++++++++++++++++++++++++++++++ common/filters.py | 31 ------------------------------ common/icat/filters.py | 1 - common/icat/helpers.py | 2 +- 5 files changed, 37 insertions(+), 34 deletions(-) create mode 100644 common/filter_order_handler.py diff --git a/common/database/helpers.py b/common/database/helpers.py index b06c5313..f4e0d217 100644 --- a/common/database/helpers.py +++ b/common/database/helpers.py @@ -24,7 +24,7 @@ SESSION, ) from common.session_manager import session_manager -from common.filters import FilterOrderHandler +from common.filter_order_handler import FilterOrderHandler from common.config import config backend_type = config.get_backend_type() diff --git a/common/filter_order_handler.py b/common/filter_order_handler.py new file mode 100644 index 00000000..aa799216 --- /dev/null +++ b/common/filter_order_handler.py @@ -0,0 +1,35 @@ +class FilterOrderHandler(object): + """ + The FilterOrderHandler takes in filters, sorts them according to the order of + operations, then applies them. + """ + + def __init__(self): + self.filters = [] + + def add_filter(self, filter): + self.filters.append(filter) + + def add_filters(self, filters): + self.filters.extend(filters) + + def sort_filters(self): + """ + Sorts the filters according to the order of operations + """ + self.filters.sort(key=lambda x: x.precedence) + + def apply_filters(self, query): + """ + Given a query apply the filters the handler has in the correct order. + :param query: The query to have filters applied to + """ + self.sort_filters() + + """ + if any(isinstance(filter, PythonICATOrderFilter) for filter in self.filters): + PythonICATOrderFilter.result_order = [] + """ + + for filter in self.filters: + filter.apply_filter(query) diff --git a/common/filters.py b/common/filters.py index 61af31a4..8ab93a0d 100644 --- a/common/filters.py +++ b/common/filters.py @@ -81,34 +81,3 @@ class IncludeFilter(QueryFilter): def __init__(self, included_filters): self.included_filters = included_filters["include"] - - -class FilterOrderHandler(object): - """ - The FilterOrderHandler takes in filters, sorts them according to the order of - operations, then applies them. - """ - - def __init__(self): - self.filters = [] - - def add_filter(self, filter): - self.filters.append(filter) - - def add_filters(self, filters): - self.filters.extend(filters) - - def sort_filters(self): - """ - Sorts the filters according to the order of operations - """ - self.filters.sort(key=lambda x: x.precedence) - - def apply_filters(self, query): - """ - Given a query apply the filters the handler has in the correct order. - :param query: The query to have filters applied to - """ - self.sort_filters() - for filter in self.filters: - filter.apply_filter(query) diff --git a/common/icat/filters.py b/common/icat/filters.py index ff4c3341..bec8c392 100644 --- a/common/icat/filters.py +++ b/common/icat/filters.py @@ -19,7 +19,6 @@ def __init__(self, field, value, operation): super().__init__(field, value, operation) def apply_filter(self, query): - if self.operation == "eq": where_filter = create_condition(self.field, "=", self.value) elif self.operation == "like": diff --git a/common/icat/helpers.py b/common/icat/helpers.py index c3af6ae6..89c6a7f8 100644 --- a/common/icat/helpers.py +++ b/common/icat/helpers.py @@ -10,7 +10,7 @@ MissingRecordError, PythonICATError, ) -from common.filters import FilterOrderHandler +from common.filter_order_handler import FilterOrderHandler from common.constants import Constants From 578ac214b56e5086483ca5f0a13fca0aad036234 Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Tue, 18 Aug 2020 13:56:08 +0000 Subject: [PATCH 3/8] #140: Basic implementation of ICAT order filter - If there's more than one order filter in a request, the others get overwritten due to the way setOrder() works in Python ICAT --- common/icat/filters.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/common/icat/filters.py b/common/icat/filters.py index bec8c392..f92bcd72 100644 --- a/common/icat/filters.py +++ b/common/icat/filters.py @@ -54,10 +54,22 @@ def apply_filter(self, query): class PythonICATOrderFilter(OrderFilter): def __init__(self, field, direction): - super().__init__(field, direction) + # Python ICAT doesn't automatically uppercase the direction, errors otherwise + super().__init__(field, direction.upper()) def apply_filter(self, query): - pass + result_order = [(self.field, self.direction)] + log.debug("Result Order: %s", result_order) + + try: + log.info("Adding order filter") + query.setOrder(PythonICATOrderFilter.result_order) + except ValueError: + raise FilterError( + "Order Filter Error: Either an invalid attribute(s) or attribute(s)" + " contains 1-many relationship" + ) + class PythonICATSkipFilter(SkipFilter): From 19a286b794a6d1e1373bdce4b0711f60ece34a88 Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Tue, 18 Aug 2020 14:34:31 +0000 Subject: [PATCH 4/8] #140: Remove unused imports --- src/resources/entities/entity_endpoint.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/resources/entities/entity_endpoint.py b/src/resources/entities/entity_endpoint.py index 81795beb..cd742d49 100644 --- a/src/resources/entities/entity_endpoint.py +++ b/src/resources/entities/entity_endpoint.py @@ -1,16 +1,6 @@ from flask import request from flask_restful import Resource -from common.database.helpers import ( - get_rows_by_filter, - create_rows_from_json, - patch_entities, - get_row_by_id, - delete_row_by_id, - update_row_from_id, - get_filtered_row_count, - get_first_filtered_row, -) from common.helpers import ( get_session_id_from_auth_header, get_filters_from_query_string, From 51924f8bf0efac50a2b93f3f2978f54608512db8 Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Tue, 18 Aug 2020 16:34:21 +0000 Subject: [PATCH 5/8] #140: Move create_condition() to PythonICATWhereFilter - This is to remove a circular dependency that I found while implementing an order filter - Also added a couple of __init__.py files which didn't previously exist --- common/database/__init__.py | 0 common/icat/__init__.py | 0 common/icat/filters.py | 41 +++++++++++++++++++++++++++++-------- common/icat/helpers.py | 26 ++--------------------- 4 files changed, 35 insertions(+), 32 deletions(-) create mode 100644 common/database/__init__.py create mode 100644 common/icat/__init__.py diff --git a/common/database/__init__.py b/common/database/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/common/icat/__init__.py b/common/icat/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/common/icat/filters.py b/common/icat/filters.py index f92bcd72..cb2e7689 100644 --- a/common/icat/filters.py +++ b/common/icat/filters.py @@ -9,7 +9,6 @@ IncludeFilter, ) from common.exceptions import FilterError -from common.icat.helpers import create_condition log = logging.getLogger() @@ -19,30 +18,56 @@ def __init__(self, field, value, operation): super().__init__(field, value, operation) def apply_filter(self, query): + log.info("Creating condition for ICAT where filter") if self.operation == "eq": - where_filter = create_condition(self.field, "=", self.value) + where_filter = self.create_condition(self.field, "=", self.value) elif self.operation == "like": - where_filter = create_condition(self.field, "like", self.value) + where_filter = self.create_condition(self.field, "like", self.value) elif self.operation == "lt": - where_filter = create_condition(self.field, "<", self.value) + where_filter = self.create_condition(self.field, "<", self.value) elif self.operation == "lte": - where_filter = create_condition(self.field, "<=", self.value) + where_filter = self.create_condition(self.field, "<=", self.value) elif self.operation == "gt": - where_filter = create_condition(self.field, ">", self.value) + where_filter = self.create_condition(self.field, ">", self.value) elif self.operation == "gte": - where_filter = create_condition(self.field, ">=", self.value) + where_filter = self.create_condition(self.field, ">=", self.value) elif self.operation == "in": - where_filter = create_condition(self.field, "in", tuple(self.value)) + where_filter = self.create_condition(self.field, "in", tuple(self.value)) else: raise FilterError(f"Bad operation given to where filter: {self.operation}") + log.debug("ICAT Where Filter: %s", where_filter) try: + log.info("Adding ICAT where filter to query") query.addConditions(where_filter) except ValueError: raise FilterError( "Something went wrong when adding WHERE filter to ICAT query" ) + @staticmethod + def create_condition(attribute_name, operator, value): + """ + Construct and return a Python dictionary containing conditions to be used in a + Query object + + :param attribute_name: Attribute name to search + :type attribute_name: :class:`str` + :param operator: Operator to use when filtering the data + :type operator: :class:`str` + :param value: What ICAT will use to filter the data + :type value: :class:`str` or :class:`tuple` (when using an IN expression) + :return: Condition (of type :class:`dict`) ready to be added to a Python ICAT + Query object + """ + + conditions = {} + # Removing quote marks when doing conditions with IN expressions + jpql_value = f"{value}" if isinstance(value, tuple) else f"'{value}'" + conditions[attribute_name] = f"{operator} {jpql_value}" + + return conditions + class PythonICATDistinctFieldFilter(DistinctFieldFilter): def __init__(self, fields): diff --git a/common/icat/helpers.py b/common/icat/helpers.py index 89c6a7f8..a3859a23 100644 --- a/common/icat/helpers.py +++ b/common/icat/helpers.py @@ -12,6 +12,7 @@ ) from common.filter_order_handler import FilterOrderHandler from common.constants import Constants +from common.icat.filters import PythonICATLimitFilter, PythonICATWhereFilter log = logging.getLogger() @@ -206,29 +207,6 @@ def get_python_icat_entity_name(client, database_table_name): return python_icat_entity_name -def create_condition(attribute_name, operator, value): - """ - Construct and return a Python dictionary containing conditions to be used in a - Query object - - :param attribute_name: Attribute name to search - :type attribute_name: :class:`str` - :param operator: Operator to use when filtering the data - :type operator: :class:`str` - :param value: What ICAT will use to filter the data - :type value: :class:`str` or :class:`tuple` (when using an IN expression) - :return: Condition (of type :class:`dict`) ready to be added to a Python ICAT Query - object - """ - - conditions = {} - # Removing quote marks when doing conditions with IN expressions - jpql_value = f"{value}" if isinstance(value, tuple) else f"'{value}'" - conditions[attribute_name] = f"{operator} {jpql_value}" - - return conditions - - def str_to_datetime_object(icat_attribute, data): """ Where data is stored as dates in ICAT (which this function determines), convert @@ -321,7 +299,7 @@ def get_entity_by_id(client, table_name, id_, return_json_formattable_data): """ # Set query condition for the selected ID - id_condition = create_condition("id", "=", id_) + id_condition = PythonICATWhereFilter.create_condition("id", "=", id_) selected_entity_name = get_python_icat_entity_name(client, table_name) From aec81f1f27d72655005d4cdd0d5135b2fd54ff92 Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Wed, 19 Aug 2020 10:58:57 +0000 Subject: [PATCH 6/8] #140: Allow multiple order filters to be used in conjunction --- common/filter_order_handler.py | 6 +----- common/icat/filters.py | 16 ++++++++-------- common/icat/helpers.py | 15 ++++++++++++++- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/common/filter_order_handler.py b/common/filter_order_handler.py index aa799216..cfcba3e2 100644 --- a/common/filter_order_handler.py +++ b/common/filter_order_handler.py @@ -22,14 +22,10 @@ def sort_filters(self): def apply_filters(self, query): """ Given a query apply the filters the handler has in the correct order. + :param query: The query to have filters applied to """ self.sort_filters() - - """ - if any(isinstance(filter, PythonICATOrderFilter) for filter in self.filters): - PythonICATOrderFilter.result_order = [] - """ for filter in self.filters: filter.apply_filter(query) diff --git a/common/icat/filters.py b/common/icat/filters.py index cb2e7689..2594aed4 100644 --- a/common/icat/filters.py +++ b/common/icat/filters.py @@ -78,23 +78,23 @@ def apply_filter(self, query): class PythonICATOrderFilter(OrderFilter): + result_order = [] + def __init__(self, field, direction): # Python ICAT doesn't automatically uppercase the direction, errors otherwise super().__init__(field, direction.upper()) def apply_filter(self, query): - result_order = [(self.field, self.direction)] - log.debug("Result Order: %s", result_order) + PythonICATOrderFilter.result_order.append((self.field, self.direction)) + log.debug("Result Order: %s", PythonICATOrderFilter.result_order) try: log.info("Adding order filter") query.setOrder(PythonICATOrderFilter.result_order) - except ValueError: - raise FilterError( - "Order Filter Error: Either an invalid attribute(s) or attribute(s)" - " contains 1-many relationship" - ) - + except ValueError as e: + # Typically either invalid attribute(s) or attribute(s) contains 1-many + # relationship + raise FilterError(e) class PythonICATSkipFilter(SkipFilter): diff --git a/common/icat/helpers.py b/common/icat/helpers.py index a3859a23..0edb60ee 100644 --- a/common/icat/helpers.py +++ b/common/icat/helpers.py @@ -12,7 +12,7 @@ ) from common.filter_order_handler import FilterOrderHandler from common.constants import Constants -from common.icat.filters import PythonICATLimitFilter, PythonICATWhereFilter +from common.icat.filters import PythonICATOrderFilter, PythonICATWhereFilter log = logging.getLogger() @@ -360,10 +360,15 @@ def update_entity_by_id(client, table_name, id_, new_data): def get_entity_with_filters(client, table_name, filters): + """ + TODO - Add docstring + """ selected_entity_name = get_python_icat_entity_name(client, table_name) query = construct_icat_query(client, selected_entity_name) + filter_handler = FilterOrderHandler() filter_handler.add_filters(filters) + manage_order_filters(filter_handler.filters) filter_handler.apply_filters(query) data = execute_icat_query(client, query, True) @@ -372,3 +377,11 @@ def get_entity_with_filters(client, table_name, filters): raise MissingRecordError("No results found") else: return data + + +def manage_order_filters(filters): + """ + TODO - Add docstring + """ + if any(isinstance(filter, PythonICATOrderFilter) for filter in filters): + PythonICATOrderFilter.result_order = [] From 182d05472e11ed0be7f0c837cb4cdd7dea21ac34 Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Wed, 19 Aug 2020 11:36:46 +0000 Subject: [PATCH 7/8] #140: Add docstrings --- common/icat/filters.py | 1 + common/icat/helpers.py | 22 ++++++++++++++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/common/icat/filters.py b/common/icat/filters.py index 2594aed4..191e012d 100644 --- a/common/icat/filters.py +++ b/common/icat/filters.py @@ -78,6 +78,7 @@ def apply_filter(self, query): class PythonICATOrderFilter(OrderFilter): + # Used to append the order tuples across all filters in a single request result_order = [] def __init__(self, field, direction): diff --git a/common/icat/helpers.py b/common/icat/helpers.py index 0edb60ee..789817bf 100644 --- a/common/icat/helpers.py +++ b/common/icat/helpers.py @@ -361,8 +361,18 @@ def update_entity_by_id(client, table_name, id_, new_data): def get_entity_with_filters(client, table_name, filters): """ - TODO - Add docstring + Gets all the records of a given entity, based on the filters provided in the request + + :param client: ICAT client containing an authenticated user + :type client: :class:`icat.client.Client` + :param table_name: Table name to extract which entity to use + :type table_name: :class:`str` + :param filters: The list of filters to be applied to the request + :type filters: List of specific implementations :class:`QueryFilter` + :return: The list of records of the given entity, using the filters to restrict the + result of the query """ + selected_entity_name = get_python_icat_entity_name(client, table_name) query = construct_icat_query(client, selected_entity_name) @@ -381,7 +391,15 @@ def get_entity_with_filters(client, table_name, filters): def manage_order_filters(filters): """ - TODO - Add docstring + Checks if any order filters have been added to the request and resets the variable + used to manage which attribute(s) to use for sorting results. + + A reset is required because Python ICAT overwrites (as opposed to appending to it) + the query's order list every time one is added to the query. + + :param filters: The list of filters to be applied to the request + :type filters: List of specific implementations :class:`QueryFilter` """ + if any(isinstance(filter, PythonICATOrderFilter) for filter in filters): PythonICATOrderFilter.result_order = [] From 8c1dcd7e68146b31e950156499ce92010eecaa9f Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Wed, 26 Aug 2020 15:15:22 +0000 Subject: [PATCH 8/8] #140: Make suggested changes as per PR comments --- common/database/filters.py | 1 + common/icat/helpers.py | 4 ++-- 2 files changed, 3 insertions(+), 2 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/icat/helpers.py b/common/icat/helpers.py index 789817bf..c4678621 100644 --- a/common/icat/helpers.py +++ b/common/icat/helpers.py @@ -378,7 +378,7 @@ def get_entity_with_filters(client, table_name, filters): filter_handler = FilterOrderHandler() filter_handler.add_filters(filters) - manage_order_filters(filter_handler.filters) + clear_order_filters(filter_handler.filters) filter_handler.apply_filters(query) data = execute_icat_query(client, query, True) @@ -389,7 +389,7 @@ def get_entity_with_filters(client, table_name, filters): return data -def manage_order_filters(filters): +def clear_order_filters(filters): """ Checks if any order filters have been added to the request and resets the variable used to manage which attribute(s) to use for sorting results.