From 4963f3f304e8c1f2470e1a52f51df928ebbc5076 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Wed, 15 Nov 2017 12:18:14 -0600 Subject: [PATCH 01/23] Add modality endpoints --- api/api.py | 10 +++ api/handlers/modalityhandler.py | 113 ++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+) create mode 100644 api/handlers/modalityhandler.py diff --git a/api/api.py b/api/api.py index db7a9dc70..634bf91fc 100644 --- a/api/api.py +++ b/api/api.py @@ -9,6 +9,7 @@ from .handlers.devicehandler import DeviceHandler from .handlers.grouphandler import GroupHandler from .handlers.listhandler import FileListHandler, NotesListHandler, PermissionsListHandler, TagsListHandler +from .handlers.modalityhandler import ModalityHandler from .handlers.refererhandler import AnalysesHandler from .handlers.reporthandler import ReportHandler from .handlers.resolvehandler import ResolveHandler @@ -177,6 +178,15 @@ def prefix(path, routes): ]), + # Modalities + + route( '/modalities', ModalityHandler, h='get_all', m=['GET']), + route( '/modalities', ModalityHandler, m=['POST']), + prefix('/modalities', [ + route('/', ModalityHandler, m=['GET', 'PUT', 'DELETE']), + ]), + + # Site route('//rules', RulesHandler, m=['GET', 'POST']), diff --git a/api/handlers/modalityhandler.py b/api/handlers/modalityhandler.py new file mode 100644 index 000000000..c89f17379 --- /dev/null +++ b/api/handlers/modalityhandler.py @@ -0,0 +1,113 @@ +import datetime as dt + +from ..web import base +from .. import config +from .. import util +from ..auth import require_drone, require_login, require_superuser +from ..dao import containerstorage, APINotFoundException +from ..validators import validate_data + +log = config.log + + +class ModalityHandler(base.RequestHandler): + + def __init__(self, request=None, response=None): + super(ModalityHandler, self).__init__(request, response) + self.storage = containerstorage.ContainerStorage('modalities', use_object_id=False) + + @require_login + def get(self, modality_name): + return self.storage.get_container(modality_name) + + @require_login + def get_all(self): + return self.storage.get_all_el(None, None, None) + + @require_superuser + def post(self): + payload = self.request.json_body + # Clean this up when validate_data method is fixed to use new schemas + # POST unnecessary, used to avoid run-time modification of schema + #validate_data(payload, 'modality.json', 'input', 'POST', optional=True) + + result = self.storage.create_el(payload) + if result.acknowledged: + return {'_id': result.inserted_id} + else: + self.abort(400, 'Modality not inserted') + + @require_superuser + def put(self, modality_name): + payload = self.request.json_body + # Clean this up when validate_data method is fixed to use new schemas + # POST unnecessary, used to avoid run-time modification of schema + #validate_data(payload, 'modality.json', 'input', 'POST', optional=True) + + result = self.storage.update_el(modality_name, payload) + if result.matched_count == 1: + return {'modified': result.modified_count} + else: + raise APINotFoundException('Modality with name {} not found, modality not updated'.format(modality_name)) + + @require_superuser + def delete(self, modality_name): + result = self.storage.delete_el(modality_name) + if result.deleted_count == 1: + return {'deleted': result.deleted_count} + else: + raise APINotFoundException('Modality with name {} not found, modality not deleted'.format(modality_name)) + + @staticmethod + def check_classification(modality_name, classification_map): + """ + Given a modality name and a proposed classification map, + ensure: + - that a modality exists with that name and has a classification + map + - all keys in the classification_map exist in the + `classifications` map on the modality object + - all the values in the arrays in the classification_map + exist in the modality's classifications map + + For example: + Modality = { + "_id" = "Example_modality", + "classifications": { + "Example1": ["Blue", "Green"] + "Example2": ["one", "two"] + } + } + + Returns True: + classification_map = { + "Example1": ["Blue"], + "custom": ["anything"] + } + + Returns False: + classification_map = { + "Example1": ["Red"], # "Red" is not allowed + "Example2": ["one", "two"] + } + """ + try: + modality = containerstorage.ContainerStorage('modalities', use_object_id=False).get_container(modality_name) + except APINotFoundException: + if classification_map.keys() == ['custom']: + # for unknown modalities allow only list of custom values + return True + else: + return False + + classifications = modality.get('classifications') + + for k,array in classification_map.iteritems(): + if k == 'custom': + # any unique value is allowed in custom list + continue + possible_values = classifications.get(k, []) + if not set(array).issubset(set(possible_values)): + return False + + return True From f16381ef02caff9bbd932d0c36df4aad4cb1e3f8 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Wed, 15 Nov 2017 13:14:23 -0600 Subject: [PATCH 02/23] Remove unused imports --- api/handlers/modalityhandler.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/api/handlers/modalityhandler.py b/api/handlers/modalityhandler.py index c89f17379..235f930d1 100644 --- a/api/handlers/modalityhandler.py +++ b/api/handlers/modalityhandler.py @@ -1,11 +1,8 @@ -import datetime as dt - from ..web import base from .. import config -from .. import util -from ..auth import require_drone, require_login, require_superuser +from ..auth import require_login, require_superuser from ..dao import containerstorage, APINotFoundException -from ..validators import validate_data +#from ..validators import validate_data log = config.log From f2e881e2a5c76690c15025b8b578d876b35947a4 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Thu, 16 Nov 2017 15:28:26 -0600 Subject: [PATCH 03/23] Case-insensitive formatting --- api/handlers/modalityhandler.py | 84 ++++++++++++++++++++++++++++----- 1 file changed, 71 insertions(+), 13 deletions(-) diff --git a/api/handlers/modalityhandler.py b/api/handlers/modalityhandler.py index 235f930d1..f987fb90f 100644 --- a/api/handlers/modalityhandler.py +++ b/api/handlers/modalityhandler.py @@ -1,11 +1,18 @@ from ..web import base from .. import config from ..auth import require_login, require_superuser -from ..dao import containerstorage, APINotFoundException +from ..dao import containerstorage, APINotFoundException, APIValidationException #from ..validators import validate_data log = config.log +class APIClassificationException(APIValidationException): + def __init__(self, modality, errors): + + error_msg = 'Classification does not match format for modality {}. Unallowable key-value pairs: {}'.format(modality, errors) + + super(APIValidationException, self).__init__(error_msg) + self.errors = errors class ModalityHandler(base.RequestHandler): @@ -56,7 +63,7 @@ def delete(self, modality_name): raise APINotFoundException('Modality with name {} not found, modality not deleted'.format(modality_name)) @staticmethod - def check_classification(modality_name, classification_map): + def check_and_format_classification(modality_name, classification_map): """ Given a modality name and a proposed classification map, ensure: @@ -67,6 +74,8 @@ def check_classification(modality_name, classification_map): - all the values in the arrays in the classification_map exist in the modality's classifications map + And then return a map with the keys and values properly formatted + For example: Modality = { "_id" = "Example_modality", @@ -76,13 +85,13 @@ def check_classification(modality_name, classification_map): } } - Returns True: + Returns properly formatted classification map: classification_map = { "Example1": ["Blue"], "custom": ["anything"] } - Returns False: + Raises APIClassificationException: classification_map = { "Example1": ["Red"], # "Red" is not allowed "Example2": ["one", "two"] @@ -90,21 +99,70 @@ def check_classification(modality_name, classification_map): """ try: modality = containerstorage.ContainerStorage('modalities', use_object_id=False).get_container(modality_name) - except APINotFoundException: + except APINotFoundException as e: if classification_map.keys() == ['custom']: # for unknown modalities allow only list of custom values - return True + return classification_map else: - return False + raise e - classifications = modality.get('classifications') + classifications = modality.get('classifications', {}) + + formatted_map = {} # the formatted map that will be returned + bad_kvs = [] # a list of errors to report, formatted like ['k:v', 'k:v', 'k:v'] for k,array in classification_map.iteritems(): if k == 'custom': # any unique value is allowed in custom list - continue - possible_values = classifications.get(k, []) - if not set(array).issubset(set(possible_values)): - return False + formatted_map[k] = array + + else: + for v in array: + + allowed, fk, fv = case_insensitive_search(classifications, k, v) + + if allowed: + if fk in formatted_map: + formatted_map[fk].append(fv) + else: + formatted_map[fk] = [fv] + + else: + bad_kvs.append(k+':'+v) + + if bad_kvs: + raise APIClassificationException(modality_name, bad_kvs) + + return formatted_map + + def case_insensitive_search(classifications, proposed_key, proposed_value): + """ + Looks for value in given classification map, returning: + + 1) found - a boolean that is true if the proposed_value was found, false if not + 2) key_name - the key name of the classification list where it was found + 3) value - the formatted string that should be saved to the file's classification + + NOTE: If the proposed_value was not found, key_name and value will be None + + This function is used mainly to preserve the existing stylization of the strings stored on + the modalities set classifications. + """ + + for k in classifications.keys(): + if k.lower() == proposed_key.lower(): + for v in classifications[k]: + if v.lower() == proposed_value.lower(): + + # Both key and value were found + return True, k, v + + # Key was found but not value + return False, None, None + + # key was not found + return False, None, None + + + - return True From a19b7eb14b767ba0ce24aa33539ee411ef6f8875 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Thu, 16 Nov 2017 16:48:42 -0600 Subject: [PATCH 04/23] Add classification editing endpoint --- api/dao/liststorage.py | 46 ++++++++ api/handlers/listhandler.py | 19 +++- api/handlers/modalityhandler.py | 180 ++++++++++++++++---------------- 3 files changed, 155 insertions(+), 90 deletions(-) diff --git a/api/dao/liststorage.py b/api/dao/liststorage.py index 12d38ca61..7a0d6737c 100644 --- a/api/dao/liststorage.py +++ b/api/dao/liststorage.py @@ -8,6 +8,7 @@ from .. import config from .. import util from ..jobs import rules +from ..handlers.modalityhandler import check_and_format_classification from .containerstorage import SessionStorage, AcquisitionStorage log = config.log @@ -217,6 +218,51 @@ def modify_info(self, _id, query_params, payload): return self.dbc.update_one(query, update) + def modify_classification(self, _id, query_params, payload): + update = {} + modality = self.get_container(_id)['files'][0].get('modality') #TODO: make this more reliable if the file isn't there + add_payload = payload.get('add') + delete_payload = payload.get('delete') + replace_payload = payload.get('replace') + + if (add_payload or delete_payload) and replace_payload is not None: + raise APIStorageException('Cannot add or delete AND replace classification fields.') + + if replace_payload is not None: + replace_payload = check_and_format_classification(modality, replace_payload) + update = { + '$set': { + self.list_name + '.$.classification': util.mongo_sanitize_fields(replace_payload) + } + } + + else: + if add_payload: + add_payload = check_and_format_classification(modality, add_payload) + + update['$addToSet'] = {} + for k,v in add_payload.iteritems(): + update['$addToSet'][self.list_name + '.$.classification.' + k] = {'$each': v} + if delete_payload: + delete_payload = check_and_format_classification(modality, delete_payload) + + # TODO: Test to make sure $pull succeeds when key does not exist + update['$pullAll'] = {} + for k,v in delete_payload.iteritems(): + update['$pullAll'][self.list_name + '.$.classification.' + k] = v + + if self.use_object_id: + _id = bson.objectid.ObjectId(_id) + query = {'_id': _id } + query[self.list_name] = {'$elemMatch': query_params} + + if not update.get('$set'): + update['$set'] = {'modified': datetime.datetime.utcnow()} + else: + update['$set']['modified'] = datetime.datetime.utcnow() + + return self.dbc.update_one(query, update) + class StringListStorage(ListStorage): """ diff --git a/api/handlers/listhandler.py b/api/handlers/listhandler.py index 5f731a7e9..8c68eb37a 100644 --- a/api/handlers/listhandler.py +++ b/api/handlers/listhandler.py @@ -539,8 +539,23 @@ def modify_info(self, cont_name, list_name, **kwargs): # abort if the query of the update wasn't able to find any matching documents if result.matched_count == 0: self.abort(404, 'Element not updated in list {} of container {} {}'.format(storage.list_name, storage.cont_name, _id)) - else: - return {'modified':result.modified_count} + + def modify_classification(self, cont_name, list_name, **kwargs): + _id = kwargs.pop('cid') + permchecker, storage, _, _, _ = self._initialize_request(cont_name, list_name, _id, query_params=kwargs) + + payload = self.request.json_body + + validators.validate_data(payload, 'classification_update.json', 'input', 'POST') + + try: + permchecker(noop)('PUT', _id=_id, query_params=kwargs, payload=payload) + result = storage.modify_classification(_id, kwargs, payload) + except APIStorageException as e: + self.abort(400, e.message) + # abort if the query of the update wasn't able to find any matching documents + if result.matched_count == 0: + self.abort(404, 'Element not updated in list {} of container {} {}'.format(storage.list_name, storage.cont_name, _id)) def post(self, cont_name, list_name, **kwargs): _id = kwargs.pop('cid') diff --git a/api/handlers/modalityhandler.py b/api/handlers/modalityhandler.py index f987fb90f..1e0a6073c 100644 --- a/api/handlers/modalityhandler.py +++ b/api/handlers/modalityhandler.py @@ -6,13 +6,6 @@ log = config.log -class APIClassificationException(APIValidationException): - def __init__(self, modality, errors): - - error_msg = 'Classification does not match format for modality {}. Unallowable key-value pairs: {}'.format(modality, errors) - - super(APIValidationException, self).__init__(error_msg) - self.errors = errors class ModalityHandler(base.RequestHandler): @@ -62,106 +55,117 @@ def delete(self, modality_name): else: raise APINotFoundException('Modality with name {} not found, modality not deleted'.format(modality_name)) - @staticmethod - def check_and_format_classification(modality_name, classification_map): - """ - Given a modality name and a proposed classification map, - ensure: - - that a modality exists with that name and has a classification - map - - all keys in the classification_map exist in the - `classifications` map on the modality object - - all the values in the arrays in the classification_map - exist in the modality's classifications map - - And then return a map with the keys and values properly formatted - - For example: - Modality = { - "_id" = "Example_modality", - "classifications": { - "Example1": ["Blue", "Green"] - "Example2": ["one", "two"] - } - } - Returns properly formatted classification map: - classification_map = { - "Example1": ["Blue"], - "custom": ["anything"] - } +class APIClassificationException(APIValidationException): + def __init__(self, modality, errors): - Raises APIClassificationException: - classification_map = { - "Example1": ["Red"], # "Red" is not allowed - "Example2": ["one", "two"] - } - """ - try: - modality = containerstorage.ContainerStorage('modalities', use_object_id=False).get_container(modality_name) - except APINotFoundException as e: - if classification_map.keys() == ['custom']: - # for unknown modalities allow only list of custom values - return classification_map - else: - raise e + error_msg = 'Classification does not match format for modality {}. Unallowable key-value pairs: {}'.format(modality, errors) - classifications = modality.get('classifications', {}) + super(APIClassificationException, self).__init__(error_msg) + self.errors = errors - formatted_map = {} # the formatted map that will be returned - bad_kvs = [] # a list of errors to report, formatted like ['k:v', 'k:v', 'k:v'] - for k,array in classification_map.iteritems(): - if k == 'custom': - # any unique value is allowed in custom list - formatted_map[k] = array +def case_insensitive_search(classifications, proposed_key, proposed_value): + """ + Looks for value in given classification map, returning: - else: - for v in array: + 1) found - a boolean that is true if the proposed_value was found, false if not + 2) key_name - the key name of the classification list where it was found + 3) value - the formatted string that should be saved to the file's classification - allowed, fk, fv = case_insensitive_search(classifications, k, v) + NOTE: If the proposed_value was not found, key_name and value will be None - if allowed: - if fk in formatted_map: - formatted_map[fk].append(fv) - else: - formatted_map[fk] = [fv] + This function is used mainly to preserve the existing stylization of the strings stored on + the modalities set classifications. + """ - else: - bad_kvs.append(k+':'+v) + for k in classifications.iterkeys(): + if k.lower() == proposed_key.lower(): + for v in classifications[k]: + if v.lower() == proposed_value.lower(): + + # Both key and value were found + return True, k, v + + # Key was found but not value + return False, None, None + + # key was not found + return False, None, None - if bad_kvs: - raise APIClassificationException(modality_name, bad_kvs) - return formatted_map +def check_and_format_classification(modality_name, classification_map): + """ + Given a modality name and a proposed classification map, + ensure: + - that a modality exists with that name and has a classification + map + - all keys in the classification_map exist in the + `classifications` map on the modality object + - all the values in the arrays in the classification_map + exist in the modality's classifications map - def case_insensitive_search(classifications, proposed_key, proposed_value): - """ - Looks for value in given classification map, returning: + And then return a map with the keys and values properly formatted - 1) found - a boolean that is true if the proposed_value was found, false if not - 2) key_name - the key name of the classification list where it was found - 3) value - the formatted string that should be saved to the file's classification + For example: + Modality = { + "_id" = "Example_modality", + "classifications": { + "Example1": ["Blue", "Green"] + "Example2": ["one", "two"] + } + } + + Returns properly formatted classification map: + classification_map = { + "Example1": ["Blue"], + "custom": ["anything"] + } + + Raises APIClassificationException: + classification_map = { + "Example1": ["Red"], # "Red" is not allowed + "Example2": ["one", "two"] + } + """ + try: + modality = containerstorage.ContainerStorage('modalities', use_object_id=False).get_container(modality_name) + except APINotFoundException as e: + if classification_map.keys() == ['custom']: + # for unknown modalities allow only list of custom values + return classification_map + else: + raise e - NOTE: If the proposed_value was not found, key_name and value will be None + classifications = modality.get('classifications', {}) - This function is used mainly to preserve the existing stylization of the strings stored on - the modalities set classifications. - """ + formatted_map = {} # the formatted map that will be returned + bad_kvs = [] # a list of errors to report, formatted like ['k:v', 'k:v', 'k:v'] + + for k,array in classification_map.iteritems(): + if k == 'custom': + # any unique value is allowed in custom list + formatted_map[k] = array + + else: + for v in array: + + allowed, fk, fv = case_insensitive_search(classifications, k, v) + + if allowed: + if fk in formatted_map: + formatted_map[fk].append(fv) + else: + formatted_map[fk] = [fv] - for k in classifications.keys(): - if k.lower() == proposed_key.lower(): - for v in classifications[k]: - if v.lower() == proposed_value.lower(): + else: + bad_kvs.append(k+':'+v) - # Both key and value were found - return True, k, v + if bad_kvs: + raise APIClassificationException(modality_name, bad_kvs) - # Key was found but not value - return False, None, None + return formatted_map - # key was not found - return False, None, None From 1ad7a7835e16a1e02f83e018a426de61cd6bd28c Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Fri, 17 Nov 2017 13:01:11 -0600 Subject: [PATCH 05/23] Add routes and db update --- api/api.py | 15 ++++---- api/handlers/modalityhandler.py | 16 ++++----- bin/database.py | 64 ++++++++++++++++++++++++++++++++- 3 files changed, 79 insertions(+), 16 deletions(-) diff --git a/api/api.py b/api/api.py index 634bf91fc..d6ee70b24 100644 --- a/api/api.py +++ b/api/api.py @@ -278,13 +278,14 @@ def prefix(path, routes): route('/', TagsListHandler, m=['POST']), route('//', TagsListHandler, m=['GET', 'PUT', 'DELETE']), - route('/packfile-start', FileListHandler, h='packfile_start', m=['POST']), - route('/packfile', FileListHandler, h='packfile', m=['POST']), - route('/packfile-end', FileListHandler, h='packfile_end'), - route('/', FileListHandler, m=['POST']), - route('//', FileListHandler, m=['GET', 'PUT', 'DELETE']), - route('///info', FileListHandler, h='get_info', m=['GET']), - route('///info', FileListHandler, h='modify_info', m=['POST']), + route('/packfile-start', FileListHandler, h='packfile_start', m=['POST']), + route('/packfile', FileListHandler, h='packfile', m=['POST']), + route('/packfile-end', FileListHandler, h='packfile_end'), + route('/', FileListHandler, m=['POST']), + route('//', FileListHandler, m=['GET', 'PUT', 'DELETE']), + route('///info', FileListHandler, h='get_info', m=['GET']), + route('///info', FileListHandler, h='modify_info', m=['POST']), + route('///classification', FileListHandler, h='modify_classification', m=['POST']), route( '//analyses', AnalysesHandler, h='get_all', m=['GET']), route( '/analyses', AnalysesHandler, h='get_all', m=['GET']), diff --git a/api/handlers/modalityhandler.py b/api/handlers/modalityhandler.py index 1e0a6073c..51670feba 100644 --- a/api/handlers/modalityhandler.py +++ b/api/handlers/modalityhandler.py @@ -1,6 +1,6 @@ from ..web import base from .. import config -from ..auth import require_login, require_superuser +from ..auth import require_login, require_admin from ..dao import containerstorage, APINotFoundException, APIValidationException #from ..validators import validate_data @@ -21,7 +21,7 @@ def get(self, modality_name): def get_all(self): return self.storage.get_all_el(None, None, None) - @require_superuser + @require_admin def post(self): payload = self.request.json_body # Clean this up when validate_data method is fixed to use new schemas @@ -34,7 +34,7 @@ def post(self): else: self.abort(400, 'Modality not inserted') - @require_superuser + @require_admin def put(self, modality_name): payload = self.request.json_body # Clean this up when validate_data method is fixed to use new schemas @@ -47,7 +47,7 @@ def put(self, modality_name): else: raise APINotFoundException('Modality with name {} not found, modality not updated'.format(modality_name)) - @require_superuser + @require_admin def delete(self, modality_name): result = self.storage.delete_el(modality_name) if result.deleted_count == 1: @@ -118,7 +118,7 @@ def check_and_format_classification(modality_name, classification_map): Returns properly formatted classification map: classification_map = { - "Example1": ["Blue"], + "Example1": ["blue"], "custom": ["anything"] } @@ -131,7 +131,7 @@ def check_and_format_classification(modality_name, classification_map): try: modality = containerstorage.ContainerStorage('modalities', use_object_id=False).get_container(modality_name) except APINotFoundException as e: - if classification_map.keys() == ['custom']: + if classification_map.keys() == ['Custom']: # for unknown modalities allow only list of custom values return classification_map else: @@ -143,9 +143,9 @@ def check_and_format_classification(modality_name, classification_map): bad_kvs = [] # a list of errors to report, formatted like ['k:v', 'k:v', 'k:v'] for k,array in classification_map.iteritems(): - if k == 'custom': + if k.lower() == 'custom': # any unique value is allowed in custom list - formatted_map[k] = array + formatted_map['Custom'] = array else: for v in array: diff --git a/bin/database.py b/bin/database.py index 375c4b76f..32fa284c4 100755 --- a/bin/database.py +++ b/bin/database.py @@ -22,7 +22,7 @@ from api.types import Origin from api.jobs import batch -CURRENT_DATABASE_VERSION = 42 # An int that is bumped when a new schema change is made +CURRENT_DATABASE_VERSION = 43 # An int that is bumped when a new schema change is made def get_db_version(): @@ -1360,6 +1360,68 @@ def upgrade_to_42(): process_cursor(cursor, upgrade_to_42_closure, context=cont_name) +def upgrade_to_43_closure(cont, cont_name): + """ + if the file has a modality, we try to find a matching classification + key and value for each measurement in the modality's classification map + + if there is no modality or the modality cannot be found in the modalities + collection, all measurements are added to the custom key + """ + + files = cont['files'] + for f in cont['files']: + modality = f.get('modality') + measurements = f['measurements'] + modality_container = None + + if modality: + modality_container = config.db.modalities.find_one({'_id': modality}) + + if modality_container: + classification = {} + m_class = modality_container.get('classifications', {}) + + for m in measurements: + found = False + for k, v_array in m_class.iteritems(): + for v in v_array: + if v.lower() == m.lower(): + found = True + if classification.get(k): + classification[k].append(v) + else: + classification[k] = [v] + if not found: + if classification.get('Custom'): + classification['Custom'].append(m) + else: + classification['Custom'] = [m] + + else: + classification = {'Custom': measurements} + + f.pop('measurements') + f['classification'] = classification + + + config.db[cont_name].update_one({'_id': cont['_id']}, {'$set': {'files': files}}) + + return True + + +def upgrade_to_43(): + """ + Update classification for all files with existing measurements field + """ + + + for cont_name in ['groups', 'projects', 'collections', 'sessions', 'acquisitions', 'analyses']: + + cursor = config.db[cont_name].find({'files.measurements': {'$exists': True }}) + process_cursor(cursor, upgrade_to_39_closure, context=cont_name) + + ### ### BEGIN RESERVED UPGRADE SECTION ### From 2b2604f74babf74a5c11253afb6347cd39ceb4d7 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Fri, 17 Nov 2017 13:12:34 -0600 Subject: [PATCH 06/23] Update json schemas and examples --- swagger/examples/file_info_list.json | 4 +- swagger/examples/output/acquisition-list.json | 6 +-- swagger/examples/output/acquisition.json | 2 +- swagger/examples/output/analysis.json | 4 +- swagger/schemas/definitions/file.json | 50 +++++++++---------- swagger/schemas/definitions/modality.json | 30 +++++++++++ swagger/schemas/mongo/file.json | 6 +-- 7 files changed, 64 insertions(+), 38 deletions(-) create mode 100644 swagger/schemas/definitions/modality.json diff --git a/swagger/examples/file_info_list.json b/swagger/examples/file_info_list.json index 70992e72e..34837d9af 100755 --- a/swagger/examples/file_info_list.json +++ b/swagger/examples/file_info_list.json @@ -8,7 +8,7 @@ "hash": "v0-sha384-12188e00a26650b2baa3f0195337dcf504f4362bb2136eef0cdbefb57159356b1355a0402fca0ab5ab081f21c305e5c2", "name": "cortical_surface_right_hemisphere.obj", "tags": [], - "measurements": [], + "classification": {}, "modified": "2016-10-18T15:26:35.701000+00:00", "modality": null, "size": 21804112, @@ -23,7 +23,7 @@ "hash": "v0-sha384-12188e00a26650b2baa3f0195337dcf504f4362bb2136eef0cdbefb57159356b1355a0402fca0ab5ab081f21c305e5c2", "name": "cortical_surface_right_hemisphere.obj", "tags": [], - "measurements": [], + "classification": {}, "modified": "2016-10-18T17:45:17.776000+00:00", "modality": null, "info": {}, diff --git a/swagger/examples/output/acquisition-list.json b/swagger/examples/output/acquisition-list.json index 1b29d562c..e452cf6a1 100644 --- a/swagger/examples/output/acquisition-list.json +++ b/swagger/examples/output/acquisition-list.json @@ -9,7 +9,7 @@ "name": "Admin Import" }, "mimetype": "application/zip", - "measurements": [], + "classification": {}, "hash": "v0-sha384-dd3c97bfe0ad1fcba75ae6718c6e81038c59af4f447f5db194d52732efa4f955b28455db02eb64cad3e4e55f11e3679f", "name": "4784_1_1_localizer_dicom.zip", "tags": [], @@ -48,7 +48,7 @@ "name": "Admin Import" }, "mimetype": "application/zip", - "measurements": [], + "classification": {}, "hash": "v0-sha384-ca055fb36845db86e4278cf6e185f8674d11a96f4b29af27e401fc495cc82ef6b53a5729c3757713064649dc71c8c725", "name": "4784_3_1_t1_dicom.zip", "tags": [], @@ -87,7 +87,7 @@ "name": "Admin Import" }, "mimetype": "application/zip", - "measurements": [], + "classification": {}, "hash": "v0-sha384-537e42b1dd8f1feef9844fbfb4f60461361e71cafa7055556097e9d0b9f7fac68c8f234ed126af9412bd43a548948847", "name": "4784_5_1_fmri_dicom.zip", "tags": [], diff --git a/swagger/examples/output/acquisition.json b/swagger/examples/output/acquisition.json index fef6ba738..945dbb8ee 100644 --- a/swagger/examples/output/acquisition.json +++ b/swagger/examples/output/acquisition.json @@ -8,7 +8,7 @@ "name": "Admin Import" }, "mimetype": "application/zip", - "measurements": [], + "classification": {}, "hash": "v0-sha384-dd3c97bfe0ad1fcba75ae6718c6e81038c59af4f447f5db194d52732efa4f955b28455db02eb64cad3e4e55f11e3679f", "name": "4784_1_1_localizer_dicom.zip", "tags": [], diff --git a/swagger/examples/output/analysis.json b/swagger/examples/output/analysis.json index bb0c4caae..0c4d700f3 100644 --- a/swagger/examples/output/analysis.json +++ b/swagger/examples/output/analysis.json @@ -8,7 +8,7 @@ "hash": "v0-sha384-12188e00a26650b2baa3f0195337dcf504f4362bb2136eef0cdbefb57159356b1355a0402fca0ab5ab081f21c305e5c2", "name": "cortical_surface_right_hemisphere.obj", "tags": [], - "measurements": [], + "classification": {}, "modified": "2016-10-18T15:26:35.701000+00:00", "modality": null, "input": true, @@ -24,7 +24,7 @@ "hash": "v0-sha384-12188e00a26650b2baa3f0195337dcf504f4362bb2136eef0cdbefb57159356b1355a0402fca0ab5ab081f21c305e5c2", "name": "cortical_surface_right_hemisphere.obj", "tags": [], - "measurements": [], + "classification": {}, "modified": "2016-10-18T17:45:17.776000+00:00", "modality": null, "output": true, diff --git a/swagger/schemas/definitions/file.json b/swagger/schemas/definitions/file.json index f6cc81590..1edf9a777 100644 --- a/swagger/schemas/definitions/file.json +++ b/swagger/schemas/definitions/file.json @@ -5,10 +5,8 @@ "file-type": { "type": "string" }, "mimetype": { "type": "string" }, "modality": { "type": "string" }, - "measurements": { - "items": { "type": "string"}, - "type": "array", - "uniqueItems": true + "classification": { + "type": "object", }, "tags": { "items": { "type": "string"}, @@ -36,7 +34,7 @@ "additionalProperties":false }, "hash":{ - "type":"string", + "type":"string", "minLength":106, "maxLength":106 }, @@ -53,39 +51,39 @@ {"type":"null"} ] }, - "measurements": {"$ref":"#/definitions/measurements"}, - "tags": {"$ref":"#/definitions/tags"}, - "info": {"$ref":"common.json#/definitions/info"}, - "origin": {"$ref":"#/definitions/file-origin"}, - "hash": {"$ref":"#/definitions/hash"}, - "created": {"$ref":"created-modified.json#/definitions/created"}, - "modified": {"$ref":"created-modified.json#/definitions/modified"}, - "size": {"$ref":"#/definitions/size"}, - "info_exists": {"type": "boolean"}, - "input": {"type":"boolean"}, - "output": {"type":"boolean"} + "classification": {"$ref":"#/definitions/classification"}, + "tags": {"$ref":"#/definitions/tags"}, + "info": {"$ref":"common.json#/definitions/info"}, + "origin": {"$ref":"#/definitions/file-origin"}, + "hash": {"$ref":"#/definitions/hash"}, + "created": {"$ref":"created-modified.json#/definitions/created"}, + "modified": {"$ref":"created-modified.json#/definitions/modified"}, + "size": {"$ref":"#/definitions/size"}, + "info_exists": {"type": "boolean"}, + "input": {"type":"boolean"}, + "output": {"type":"boolean"} }, "additionalProperties": false }, "file-input":{ "type": "object", "properties": { - "name": {"$ref":"#/definitions/name"}, - "type": {"$ref":"#/definitions/file-type"}, - "mimetype": {"$ref":"#/definitions/mimetype"}, - "modality": {"$ref":"#/definitions/modality"}, - "measurements": {"$ref":"#/definitions/measurements"}, - "tags": {"$ref":"#/definitions/tags"}, - "info": {"$ref":"common.json#/definitions/info"} + "name": {"$ref":"#/definitions/name"}, + "type": {"$ref":"#/definitions/file-type"}, + "mimetype": {"$ref":"#/definitions/mimetype"}, + "modality": {"$ref":"#/definitions/modality"}, + "classification": {"$ref":"#/definitions/classification"}, + "tags": {"$ref":"#/definitions/tags"}, + "info": {"$ref":"common.json#/definitions/info"} }, "additionalProperties": false }, "file-update":{ "type": "object", "properties": { - "type": {"$ref":"#/definitions/file-type"}, - "modality": {"$ref":"#/definitions/modality"}, - "measurements": {"$ref":"#/definitions/measurements"} + "type": {"$ref":"#/definitions/file-type"}, + "modality": {"$ref":"#/definitions/modality"}, + "classification": {"$ref":"#/definitions/classification"} }, "additionalProperties": false }, diff --git a/swagger/schemas/definitions/modality.json b/swagger/schemas/definitions/modality.json new file mode 100644 index 000000000..cc8cab0bf --- /dev/null +++ b/swagger/schemas/definitions/modality.json @@ -0,0 +1,30 @@ +{ + "$schema": "http://json-schema.org/draft-04/schema#", + "type": "object", + "definitions": { + "_id": { + "maxLength": 64, + "minLength": 2, + "pattern": "^[0-9a-zA-Z_-]*$" + }, + "classifications": { + "type": "object", + "patternProperties": { + "^[0-9a-zA-Z_-]*$":{ + "type": "array", + "items": { + "type": "string" + } + }, + }, + }, + "modality":{ + "type": "object", + "properties": { + "_id": {"$ref":"#/definitions/_id"}, + "classifications": {"$ref":"#/definitions/classifications"} + }, + "additionalProperties": false + } + } +} diff --git a/swagger/schemas/mongo/file.json b/swagger/schemas/mongo/file.json index 4e077d85a..fbf3336c9 100644 --- a/swagger/schemas/mongo/file.json +++ b/swagger/schemas/mongo/file.json @@ -10,10 +10,8 @@ "size": { "type": "integer" }, "hash": { "type": "string" }, "modality": { "type": "string" }, - "measurements": { - "items": { "type": "string"}, - "type": "array", - "uniqueItems": true + "classification": { + "type": "object", }, "tags": { "items": { "type": "string"}, From f09c9b681926c9c690184e660c0e76051afc6fc3 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Fri, 17 Nov 2017 13:25:52 -0600 Subject: [PATCH 07/23] Appease config.py --- api/config.py | 1 + 1 file changed, 1 insertion(+) diff --git a/api/config.py b/api/config.py index c2bbaf696..8d93c2073 100644 --- a/api/config.py +++ b/api/config.py @@ -163,6 +163,7 @@ def apply_env_variables(config): 'group-new.json', 'group-update.json', 'info_update.json', + 'modality.json', 'note.json', 'packfile.json', 'permission.json', From c3b2cc295b94ed24260973e25e95f9f7e4b628f5 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Mon, 27 Nov 2017 14:20:13 -0600 Subject: [PATCH 08/23] Fix closure reference after rebase --- bin/database.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/database.py b/bin/database.py index 32fa284c4..7bf356098 100755 --- a/bin/database.py +++ b/bin/database.py @@ -1419,7 +1419,7 @@ def upgrade_to_43(): for cont_name in ['groups', 'projects', 'collections', 'sessions', 'acquisitions', 'analyses']: cursor = config.db[cont_name].find({'files.measurements': {'$exists': True }}) - process_cursor(cursor, upgrade_to_39_closure, context=cont_name) + process_cursor(cursor, upgrade_to_40_closure, context=cont_name) ### From 782dda5dffb4faa4a74e62884d949fd9b5693cb4 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Mon, 27 Nov 2017 15:09:14 -0600 Subject: [PATCH 09/23] Clean up a few measurement references --- api/handlers/modalityhandler.py | 3 ++- api/jobs/queue.py | 2 +- api/jobs/rules.py | 19 +++++++++++-------- api/placer.py | 2 +- api/upload.py | 2 +- swagger/schemas/definitions/rule.json | 4 ++-- 6 files changed, 18 insertions(+), 14 deletions(-) diff --git a/api/handlers/modalityhandler.py b/api/handlers/modalityhandler.py index 51670feba..aca57fd1a 100644 --- a/api/handlers/modalityhandler.py +++ b/api/handlers/modalityhandler.py @@ -1,7 +1,8 @@ from ..web import base from .. import config from ..auth import require_login, require_admin -from ..dao import containerstorage, APINotFoundException, APIValidationException +from ..dao import containerstorage +from ..web.errors import APINotFoundException, APIValidationException #from ..validators import validate_data log = config.log diff --git a/api/jobs/queue.py b/api/jobs/queue.py index 89ea8cc10..885924837 100644 --- a/api/jobs/queue.py +++ b/api/jobs/queue.py @@ -231,7 +231,7 @@ def enqueue_job(job_map, origin, perm_check_uid=None): cr = create_containerreference_from_filereference(inputs[x]) # Whitelist file fields passed to gear to those that are scientific-relevant - whitelisted_keys = ['info', 'tags', 'measurements', 'mimetype', 'type', 'modality', 'size'] + whitelisted_keys = ['info', 'tags', 'classification', 'mimetype', 'type', 'modality', 'size'] obj_projection = { key: obj.get(key) for key in whitelisted_keys } config_['inputs'][x] = { diff --git a/api/jobs/rules.py b/api/jobs/rules.py index b111baf59..0027c001d 100644 --- a/api/jobs/rules.py +++ b/api/jobs/rules.py @@ -1,5 +1,6 @@ import fnmatch import re +import itertools from .. import config from ..types import Origin @@ -33,7 +34,7 @@ # 'value': '*.dcm' # }, # { -# 'type': 'file.measurements', # Match any of the file's measurements +# 'type': 'file.classification', # Match any of the file's classification # 'value': 'diffusion' # }, # { @@ -41,7 +42,7 @@ # 'value': 'bvec' # }, # { -# 'type': 'container.has-measurement', # Match the container having any file (including this one) with this measurement +# 'type': 'container.has-classification', # Match the container having any file (including this one) with this classification # 'value': 'functional' # } # ] @@ -86,10 +87,11 @@ def match(value): elif match_type == 'file.name': return match(file_['name']) - # Match any of the file's measurements - elif match_type == 'file.measurements': + # Match any of the file's classification + elif match_type == 'file.classification': if match_param: - return any(match(value) for value in file_.get('measurements', [])) + classification_values = list(itertools.chain.from_iterable(file_.get('classification', {}).itervalues())) + return any(match(value) for value in classification_values) else: return False @@ -102,11 +104,12 @@ def match(value): return False - # Match the container having any file (including this one) with this measurement - elif match_type == 'container.has-measurement': + # Match the container having any file (including this one) with this classification + elif match_type == 'container.has-classification': if match_param: for c_file in container['files']: - if any(match(value) for value in c_file.get('measurements', [])): + classification_values = list(itertools.chain.from_iterable(c_file.get('classification', {}).itervalues())) + if any(match(value) for value in classification_values): return True return False diff --git a/api/placer.py b/api/placer.py index 77aa4eacf..14c1ee616 100644 --- a/api/placer.py +++ b/api/placer.py @@ -567,7 +567,7 @@ def finalize(self): # OPPORTUNITY: packfile endpoint could be extended someday to take additional metadata. 'modality': None, - 'measurements': [], + 'classification': {}, 'tags': [], 'info': {}, diff --git a/api/upload.py b/api/upload.py index 75de2e470..916e39c4a 100644 --- a/api/upload.py +++ b/api/upload.py @@ -127,7 +127,7 @@ def process_upload(request, strategy, container_type=None, id_=None, origin=None 'type': None, 'modality': None, - 'measurements': [], + 'classification': {}, 'tags': [], 'info': {} } diff --git a/swagger/schemas/definitions/rule.json b/swagger/schemas/definitions/rule.json index 3e2585774..d2cb57bbb 100644 --- a/swagger/schemas/definitions/rule.json +++ b/swagger/schemas/definitions/rule.json @@ -11,9 +11,9 @@ "enum": [ "file.type", "file.name", - "file.measurements", + "file.classification", "container.has-type", - "container.has-measurement" + "container.has-classification" ] }, "value": { "type": "string" }, From 4e6bf0eec00fc5512cf4826016cc6ca48658217a Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Tue, 28 Nov 2017 13:58:52 -0600 Subject: [PATCH 10/23] Add rule upgrade --- bin/database.py | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/bin/database.py b/bin/database.py index 7bf356098..3863b7f99 100755 --- a/bin/database.py +++ b/bin/database.py @@ -1360,7 +1360,7 @@ def upgrade_to_42(): process_cursor(cursor, upgrade_to_42_closure, context=cont_name) -def upgrade_to_43_closure(cont, cont_name): +def upgrade_files_to_43(cont, cont_name): """ if the file has a modality, we try to find a matching classification key and value for each measurement in the modality's classification map @@ -1409,6 +1409,21 @@ def upgrade_to_43_closure(cont, cont_name): return True +def upgrade_rules_to_43(rule): + + def adjust_type(r): + if r['type'] == 'file.measurement': + r['type'] = 'file.classification' + elif r['type'] == 'container.has-measurement': + r['type'] = 'container.has-classification' + + for r in rule.get('any', []): + adjust_type(r) + + for r in rule.get('all', []): + adjust_type(r) + + config.db.project_rules.replace_one({'_id': rule['_id']}, rule) def upgrade_to_43(): """ @@ -1419,7 +1434,17 @@ def upgrade_to_43(): for cont_name in ['groups', 'projects', 'collections', 'sessions', 'acquisitions', 'analyses']: cursor = config.db[cont_name].find({'files.measurements': {'$exists': True }}) - process_cursor(cursor, upgrade_to_40_closure, context=cont_name) + process_cursor(cursor, upgrade_files_to_43, context=cont_name) + + + cursor = config.db.project_rules.find({'$or': [ + {'all.type': {'$in': ['file.measurement', 'container.has-measurement']}}, + {'any.type': {'$in': ['file.measurement', 'container.has-measurement']}} + ]}) + process_cursor(cursor, upgrade_rules_to_43) + + + ### From 9c559ce45fe110bf330c37206f42d3cac008f78c Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Thu, 30 Nov 2017 14:11:07 -0600 Subject: [PATCH 11/23] Remove references to measurements --- api/handlers/collectionshandler.py | 3 +-- api/handlers/containerhandler.py | 17 ----------------- api/handlers/dataexplorerhandler.py | 6 +++--- 3 files changed, 4 insertions(+), 22 deletions(-) diff --git a/api/handlers/collectionshandler.py b/api/handlers/collectionshandler.py index 703b277cc..13f2bbf47 100644 --- a/api/handlers/collectionshandler.py +++ b/api/handlers/collectionshandler.py @@ -168,8 +168,7 @@ def get_sessions(self, cid): sessions = list(containerstorage.SessionStorage().get_all_el(query, None, projection)) self._filter_all_permissions(sessions, self.uid) - if self.is_true('measurements'): - self._add_session_measurements(sessions) + for sess in sessions: sess = self.handle_origin(sess) return sessions diff --git a/api/handlers/containerhandler.py b/api/handlers/containerhandler.py index 2bc5c5609..fe5d7b08d 100644 --- a/api/handlers/containerhandler.py +++ b/api/handlers/containerhandler.py @@ -346,10 +346,6 @@ def get_all(self, cont_name, par_cont_name=None, par_id=None): # the "count" flag add a count for each container returned if self.is_true('counts'): self._add_results_counts(results, cont_name) - # the "measurements" flag applies only to query for sessions - # and add a list of the measurements in the child acquisitions - if cont_name == 'sessions' and self.is_true('measurements'): - self._add_session_measurements(results) modified_results = [] for result in results: @@ -620,19 +616,6 @@ def _get_validators(self): payload_validator = validators.from_schema_path(payload_schema_uri) return mongo_validator, payload_validator - def _add_session_measurements(self, results): - session_measurements = config.db.acquisitions.aggregate([ - {'$match': {'session': {'$in': [sess['_id'] for sess in results]}}}, - {'$project': { '_id': '$session', 'files':1 }}, - {'$unwind': '$files'}, - {'$project': { '_id': '$_id', 'files.measurements': 1}}, - {'$unwind': '$files.measurements'}, - {'$group': {'_id': '$_id', 'measurements': {'$addToSet': '$files.measurements'}}} - ]) - session_measurements = {sess['_id']: sess['measurements'] for sess in session_measurements} - for sess in results: - sess['measurements'] = session_measurements.get(sess['_id'], None) - def _get_parent_container(self, payload): if not self.config.get('parent_storage'): return None, None diff --git a/api/handlers/dataexplorerhandler.py b/api/handlers/dataexplorerhandler.py index 1e47702e6..4ad946829 100644 --- a/api/handlers/dataexplorerhandler.py +++ b/api/handlers/dataexplorerhandler.py @@ -206,9 +206,9 @@ "filter": {"term": {"container_type": "file"}}, "aggs": { - "file.measurements" : { + "file.classification" : { "terms" : { - "field" : "file.measurements.raw", + "field" : "file.classification.raw", "size" : 15, "missing": "null" } @@ -278,7 +278,7 @@ SOURCE_FILE = SOURCE_ANALYSIS + [ "file.created", - "file.measurements", + "file.classification", "file.name", "file.size", "file.type", From b12413422675978a4ca6bacdd24ef00311fa2db5 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Thu, 30 Nov 2017 15:29:03 -0600 Subject: [PATCH 12/23] Clean up tests --- swagger/schemas/mongo/file.json | 4 +--- .../python/test_containers.py | 4 ++-- tests/integration_tests/python/test_rules.py | 18 +++++++++--------- tests/unit_tests/python/test_rules.py | 10 +++++----- 4 files changed, 17 insertions(+), 19 deletions(-) diff --git a/swagger/schemas/mongo/file.json b/swagger/schemas/mongo/file.json index fbf3336c9..d4ae60135 100644 --- a/swagger/schemas/mongo/file.json +++ b/swagger/schemas/mongo/file.json @@ -10,9 +10,7 @@ "size": { "type": "integer" }, "hash": { "type": "string" }, "modality": { "type": "string" }, - "classification": { - "type": "object", - }, + "classification": { "type": "object" }, "tags": { "items": { "type": "string"}, "type": "array", diff --git a/tests/integration_tests/python/test_containers.py b/tests/integration_tests/python/test_containers.py index 792481a7d..a5e72e4b2 100644 --- a/tests/integration_tests/python/test_containers.py +++ b/tests/integration_tests/python/test_containers.py @@ -612,7 +612,7 @@ def test_edit_file_attributes(data_builder, as_admin, file_form): payload = { 'type': 'new type', 'modality': 'new modality', - 'measurements': ['measurement'] + 'classification': {'custom': ['measurement']} } assert as_admin.put('/projects/' + project + '/files/' + file_name, json=payload).ok @@ -622,7 +622,7 @@ def test_edit_file_attributes(data_builder, as_admin, file_form): file_object = r.json() assert file_object['type'] == payload['type'] - assert file_object['measurements'] == payload['measurements'] + assert file_object['classification'] == payload['classification'] assert file_object['modality'] == payload['modality'] diff --git a/tests/integration_tests/python/test_rules.py b/tests/integration_tests/python/test_rules.py index 0616e3311..4636124cb 100644 --- a/tests/integration_tests/python/test_rules.py +++ b/tests/integration_tests/python/test_rules.py @@ -42,7 +42,7 @@ def test_site_rules(randstr, data_builder, as_admin, as_user, as_public): 'name': 'invalid-regex-rule', 'any': [], 'all': [ - {'type': 'file.measurements', 'value': invalid_pattern, 'regex': True}, + {'type': 'file.classification', 'value': invalid_pattern, 'regex': True}, ] }) assert r.status_code == 422 @@ -101,7 +101,7 @@ def test_site_rules(randstr, data_builder, as_admin, as_user, as_public): # attempt to modify site rule with invalid regex r = as_admin.put('/site/rules/' + rule_id, json={ 'all': [ - {'type': 'file.measurements', 'value': invalid_pattern, 'regex': True}, + {'type': 'file.classification', 'value': invalid_pattern, 'regex': True}, ] }) assert r.status_code == 422 @@ -359,10 +359,10 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a # NOTE this is a legacy rule r = as_admin.post('/projects/' + project + '/rules', json={ 'alg': gear_name, - 'name': 'txt-job-trigger-rule-with-measurement', + 'name': 'txt-job-trigger-rule-with-classification', 'any': [ - {'type': 'container.has-measurement', 'value': 'functional'}, - {'type': 'container.has-measurement', 'value': 'anatomical'} + {'type': 'container.has-classification', 'value': 'functional'}, + {'type': 'container.has-classification', 'value': 'anatomical'} ], 'all': [ {'type': 'file.type', 'value': 'text'}, @@ -379,14 +379,14 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a gear_jobs = [job for job in api_db.jobs.find({'gear_id': gear_2})] assert len(gear_jobs) == 1 # still 1 from before - # update test2.csv's metadata to include a valid measurement to spawn job + # update test2.csv's metadata to include a valid classification to spawn job metadata = { 'project':{ 'label': 'rule project', 'files': [ { 'name': 'test2.csv', - 'measurements': ['functional'] + 'classification': {'intent': ['functional']} } ] } @@ -398,7 +398,7 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a ) assert r.ok - # Ensure file without type or measurements does not cause issues with rule evalution + # Ensure file without type or classification does not cause issues with rule evalution # upload file that matches only part of rule r = as_admin.post('/projects/' + project + '/files', files=file_form('test3.notreal')) assert r.ok @@ -417,7 +417,7 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a # NOTE this is a legacy rule r = as_admin.post('/projects/' + project + '/rules', json={ 'alg': gear_name, - 'name': 'file-measurement-regex', + 'name': 'file-classification-regex', 'any': [], 'all': [ {'type': 'file.name', 'value': 'test\d+\.(csv|txt)', 'regex': True}, diff --git a/tests/unit_tests/python/test_rules.py b/tests/unit_tests/python/test_rules.py index 8dca05532..0cedfd73e 100644 --- a/tests/unit_tests/python/test_rules.py +++ b/tests/unit_tests/python/test_rules.py @@ -90,8 +90,8 @@ def test_eval_match_file_name_match_regex(): result = rules.eval_match(*args) assert result == False -def test_eval_match_file_measurements(): - part = rulePart(match_type='file.measurements', file_={'measurements': ['a', 'diffusion', 'b'] }) +def test_eval_match_file_classification(): + part = rulePart(match_type='file.classification', file_={'classification': {'intent': ['a', 'diffusion', 'b'] }}) args = part.gen(match_param='diffusion') result = rules.eval_match(*args) @@ -101,13 +101,13 @@ def test_eval_match_file_measurements(): result = rules.eval_match(*args) assert result == False - # Check match returns false without raising when not given a file.measurement + # Check match returns false without raising when not given a file.classification args = part.gen(match_param='', file_={'a': 'b'}, container={'a': 'b'}) result = rules.eval_match(*args) assert result == False -def test_eval_match_file_measurements_regex(): - part = rulePart(match_type='file.measurements', file_={'measurements': ['diffusion']}, regex=True) +def test_eval_match_file_classification_regex(): + part = rulePart(match_type='file.classification', file_={'classification': {'intent': ['diffusion']}}, regex=True) args = part.gen(match_param='test|diffusion') result = rules.eval_match(*args) From 95e46dbceb5afa1dd16b3abb87c032cf6bc49fcb Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Thu, 30 Nov 2017 17:10:30 -0600 Subject: [PATCH 13/23] Add modality tests --- api/dao/basecontainerstorage.py | 12 ++++ api/handlers/modalityhandler.py | 15 ++--- .../python/test_classification.py | 60 +++++++++++++++++++ tests/integration_tests/python/test_rules.py | 3 +- 4 files changed, 77 insertions(+), 13 deletions(-) create mode 100644 tests/integration_tests/python/test_classification.py diff --git a/api/dao/basecontainerstorage.py b/api/dao/basecontainerstorage.py index 9484833f4..8d3d260b3 100644 --- a/api/dao/basecontainerstorage.py +++ b/api/dao/basecontainerstorage.py @@ -188,8 +188,20 @@ def update_el(self, _id, payload, unset_payload=None, recursive=False, r_payload raise APIStorageException(e.message) if recursive and r_payload is not None: containerutil.propagate_changes(self.cont_name, _id, {}, {'$set': util.mongo_dict(r_payload)}) + + config.log.warning(update) return self.dbc.update_one({'_id': _id}, update) + def replace_el(self, _id, payload): + if self.use_object_id: + try: + _id = bson.ObjectId(_id) + except bson.errors.InvalidId as e: + raise APIStorageException(e.message) + payload['_id'] = _id + return self.dbc.replace_one({'_id': _id}, payload) + + def delete_el(self, _id): if self.use_object_id: try: diff --git a/api/handlers/modalityhandler.py b/api/handlers/modalityhandler.py index aca57fd1a..a0671b5d9 100644 --- a/api/handlers/modalityhandler.py +++ b/api/handlers/modalityhandler.py @@ -30,10 +30,7 @@ def post(self): #validate_data(payload, 'modality.json', 'input', 'POST', optional=True) result = self.storage.create_el(payload) - if result.acknowledged: - return {'_id': result.inserted_id} - else: - self.abort(400, 'Modality not inserted') + return {'_id': result.inserted_id} @require_admin def put(self, modality_name): @@ -42,18 +39,14 @@ def put(self, modality_name): # POST unnecessary, used to avoid run-time modification of schema #validate_data(payload, 'modality.json', 'input', 'POST', optional=True) - result = self.storage.update_el(modality_name, payload) - if result.matched_count == 1: - return {'modified': result.modified_count} - else: + result = self.storage.replace_el(modality_name, payload) + if result.matched_count != 1: raise APINotFoundException('Modality with name {} not found, modality not updated'.format(modality_name)) @require_admin def delete(self, modality_name): result = self.storage.delete_el(modality_name) - if result.deleted_count == 1: - return {'deleted': result.deleted_count} - else: + if result.deleted_count != 1: raise APINotFoundException('Modality with name {} not found, modality not deleted'.format(modality_name)) diff --git a/tests/integration_tests/python/test_classification.py b/tests/integration_tests/python/test_classification.py new file mode 100644 index 000000000..a8e2bf184 --- /dev/null +++ b/tests/integration_tests/python/test_classification.py @@ -0,0 +1,60 @@ +def test_modalities(data_builder, as_admin, as_user): + + payload = { + '_id': 'MR', + 'classification': { + 'Intent': ["Structural", "Functional", "Localizer"], + 'Contrast': ["B0", "B1", "T1", "T2"] + } + } + + # test adding new modality + r = as_admin.post('/modalities', json=payload) + assert r.ok + assert r.json()['_id'] == payload['_id'] + modality1 = payload['_id'] + + # get specific modality + r = as_user.get('/modalities/' + modality1) + assert r.ok + assert r.json() == payload + + # try replacing existing modality via POST + r = as_admin.post('/modalities', json=payload) + assert r.status_code == 409 + + # list modalities as non-admin + r = as_user.get('/modalities') + assert r.ok + modalities = r.json() + assert len(modalities) == 1 + assert modalities[0]['_id'] == modality1 + + # replace existing modality + update = { + 'classification': { + 'Intent': ["new", "stuff"] + } + } + r = as_admin.put('/modalities/' + modality1, json=update) + assert r.ok + r = as_admin.get('/modalities/' + modality1) + assert r.ok + assert r.json()['classification'] == update['classification'] + + # try to replace missing modality + r = as_admin.put('/modalities/' + 'madeup', json=update) + assert r.status_code == 404 + + # delete modality + r = as_admin.delete('/modalities/' + modality1) + assert r.ok + + # try to delete missing modality + r = as_admin.delete('/modalities/' + modality1) + assert r.status_code == 404 + + + + + diff --git a/tests/integration_tests/python/test_rules.py b/tests/integration_tests/python/test_rules.py index 4636124cb..439fc6102 100644 --- a/tests/integration_tests/python/test_rules.py +++ b/tests/integration_tests/python/test_rules.py @@ -438,8 +438,6 @@ def test_rules(randstr, data_builder, file_form, as_root, as_admin, with_user, a r = as_admin.delete('/projects/' + project + '/rules/' + rule3) assert r.ok - # TODO add and test 'new-style' rules - def test_disabled_rules(randstr, data_builder, api_db, as_admin, file_form): # Create gear, project and *disabled* rule triggering on any csv (once enabled) @@ -477,3 +475,4 @@ def test_disabled_rules(randstr, data_builder, api_db, as_admin, file_form): assert len(gear_jobs) == 1 assert len(gear_jobs[0]['inputs']) == 1 assert gear_jobs[0]['inputs'][0]['name'] == 'test2.csv' + From 0d0452c517d5637c7a1114cf39ebb19cf0cee082 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Thu, 7 Dec 2017 12:37:40 -0600 Subject: [PATCH 14/23] Add additional tests, modify a few schemas --- api/config.py | 1 + api/dao/liststorage.py | 50 +++--- api/handlers/listhandler.py | 11 +- api/handlers/modalityhandler.py | 7 +- swagger/schemas/definitions/file.json | 4 +- .../python/test_classification.py | 170 ++++++++++++++++++ 6 files changed, 209 insertions(+), 34 deletions(-) diff --git a/api/config.py b/api/config.py index 8d93c2073..bf8716318 100644 --- a/api/config.py +++ b/api/config.py @@ -155,6 +155,7 @@ def apply_env_variables(config): 'analysis-job.json', 'analysis-update.json', 'avatars.json', + 'classification_update.json', 'collection.json', 'collection-update.json', 'device.json', diff --git a/api/dao/liststorage.py b/api/dao/liststorage.py index 7a0d6737c..df0eff9be 100644 --- a/api/dao/liststorage.py +++ b/api/dao/liststorage.py @@ -1,5 +1,6 @@ import bson.errors import bson.objectid +import copy import datetime import pymongo @@ -219,7 +220,13 @@ def modify_info(self, _id, query_params, payload): return self.dbc.update_one(query, update) def modify_classification(self, _id, query_params, payload): - update = {} + update = {'$set': {'modified': datetime.datetime.utcnow()}} + + if self.use_object_id: + _id = bson.objectid.ObjectId(_id) + query = {'_id': _id } + query[self.list_name] = {'$elemMatch': query_params} + modality = self.get_container(_id)['files'][0].get('modality') #TODO: make this more reliable if the file isn't there add_payload = payload.get('add') delete_payload = payload.get('delete') @@ -230,38 +237,41 @@ def modify_classification(self, _id, query_params, payload): if replace_payload is not None: replace_payload = check_and_format_classification(modality, replace_payload) - update = { - '$set': { - self.list_name + '.$.classification': util.mongo_sanitize_fields(replace_payload) - } - } + + r_update = copy.deepcopy(update) + r_update['$set'][self.list_name + '.$.classification'] = util.mongo_sanitize_fields(replace_payload) + + result = self.dbc.update_one(query, r_update) + if result.matched_count == 0: + raise APINotFoundException('File not found in container {} {}.'.format(self.cont_name, _id)) else: if add_payload: add_payload = check_and_format_classification(modality, add_payload) - update['$addToSet'] = {} + a_update = copy.deepcopy(update) + a_update['$addToSet'] = {} for k,v in add_payload.iteritems(): - update['$addToSet'][self.list_name + '.$.classification.' + k] = {'$each': v} + a_update['$addToSet'][self.list_name + '.$.classification.' + k] = {'$each': v} + + result = self.dbc.update_one(query, a_update) + if result.matched_count == 0: + raise APINotFoundException('File not found in container {} {}.'.format(self.cont_name, _id)) + + if delete_payload: delete_payload = check_and_format_classification(modality, delete_payload) # TODO: Test to make sure $pull succeeds when key does not exist - update['$pullAll'] = {} + d_update = copy.deepcopy(update) + d_update['$pullAll'] = {} for k,v in delete_payload.iteritems(): - update['$pullAll'][self.list_name + '.$.classification.' + k] = v + d_update['$pullAll'][self.list_name + '.$.classification.' + k] = v - if self.use_object_id: - _id = bson.objectid.ObjectId(_id) - query = {'_id': _id } - query[self.list_name] = {'$elemMatch': query_params} + result = self.dbc.update_one(query, d_update) + if result.matched_count == 0: + raise APINotFoundException('File not found in container {} {}.'.format(self.cont_name, _id)) - if not update.get('$set'): - update['$set'] = {'modified': datetime.datetime.utcnow()} - else: - update['$set']['modified'] = datetime.datetime.utcnow() - - return self.dbc.update_one(query, update) class StringListStorage(ListStorage): diff --git a/api/handlers/listhandler.py b/api/handlers/listhandler.py index 8c68eb37a..24c990f41 100644 --- a/api/handlers/listhandler.py +++ b/api/handlers/listhandler.py @@ -548,14 +548,9 @@ def modify_classification(self, cont_name, list_name, **kwargs): validators.validate_data(payload, 'classification_update.json', 'input', 'POST') - try: - permchecker(noop)('PUT', _id=_id, query_params=kwargs, payload=payload) - result = storage.modify_classification(_id, kwargs, payload) - except APIStorageException as e: - self.abort(400, e.message) - # abort if the query of the update wasn't able to find any matching documents - if result.matched_count == 0: - self.abort(404, 'Element not updated in list {} of container {} {}'.format(storage.list_name, storage.cont_name, _id)) + permchecker(noop)('PUT', _id=_id, query_params=kwargs, payload=payload) + result = storage.modify_classification(_id, kwargs, payload) + def post(self, cont_name, list_name, **kwargs): _id = kwargs.pop('cid') diff --git a/api/handlers/modalityhandler.py b/api/handlers/modalityhandler.py index a0671b5d9..87a081395 100644 --- a/api/handlers/modalityhandler.py +++ b/api/handlers/modalityhandler.py @@ -56,7 +56,7 @@ def __init__(self, modality, errors): error_msg = 'Classification does not match format for modality {}. Unallowable key-value pairs: {}'.format(modality, errors) super(APIClassificationException, self).__init__(error_msg) - self.errors = errors + self.errors = str(errors) def case_insensitive_search(classifications, proposed_key, proposed_value): @@ -131,7 +131,7 @@ def check_and_format_classification(modality_name, classification_map): else: raise e - classifications = modality.get('classifications', {}) + classifications = modality.get('classification', {}) formatted_map = {} # the formatted map that will be returned bad_kvs = [] # a list of errors to report, formatted like ['k:v', 'k:v', 'k:v'] @@ -156,7 +156,8 @@ def check_and_format_classification(modality_name, classification_map): bad_kvs.append(k+':'+v) if bad_kvs: - raise APIClassificationException(modality_name, bad_kvs) + raise Exception(bad_kvs) + #raise APIClassificationException(modality_name, bad_kvs) return formatted_map diff --git a/swagger/schemas/definitions/file.json b/swagger/schemas/definitions/file.json index 1edf9a777..a54cb7f74 100644 --- a/swagger/schemas/definitions/file.json +++ b/swagger/schemas/definitions/file.json @@ -82,9 +82,7 @@ "type": "object", "properties": { "type": {"$ref":"#/definitions/file-type"}, - "modality": {"$ref":"#/definitions/modality"}, - "classification": {"$ref":"#/definitions/classification"} - }, + "modality": {"$ref":"#/definitions/modality"} }, "additionalProperties": false }, "file-output":{ diff --git a/tests/integration_tests/python/test_classification.py b/tests/integration_tests/python/test_classification.py index a8e2bf184..3a75988f5 100644 --- a/tests/integration_tests/python/test_classification.py +++ b/tests/integration_tests/python/test_classification.py @@ -55,6 +55,176 @@ def test_modalities(data_builder, as_admin, as_user): assert r.status_code == 404 +def test_edit_file_classification(data_builder, as_admin, as_user, file_form): + ## Setup + # Add file + project = data_builder.create_project() + file_name = 'test_file.txt' + + r = as_admin.post('/projects/' + project + '/files', files=file_form(file_name)) + assert r.ok + + r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + assert r.ok + assert r.json()['classification'] == {} + + + # add modality information + payload = { + '_id': 'MR', + 'classification': { + 'Intent': ["Structural", "Functional", "Localizer"], + 'Contrast': ["B0", "B1", "T1", "T2"] + } + } + + r = as_admin.post('/modalities', json=payload) + assert r.ok + assert r.json()['_id'] == payload['_id'] + modality1 = payload['_id'] + + # Add modality to file + r = as_admin.put('/projects/' + project + '/files/' + file_name, json={ + 'modality': 'MR' + }) + + + ## Classification editing + + # Send improper payload + r = as_admin.post('/projects/' + project + '/files/' + file_name + '/classification', json={ + 'delete': ['this', 'is'], + 'replace': {'not_going': 'to_happen'} + }) + assert r.status_code == 400 + + # Send improper payload + r = as_admin.post('/projects/' + project + '/files/' + file_name + '/classification', json={ + 'delete': ['should', 'be', 'a', 'map'] + }) + assert r.status_code == 400 + + # Send improper payload + r = as_admin.post('/projects/' + project + '/files/' + file_name + '/classification', json={ + 'set': 'cannot do this' + }) + assert r.status_code == 400 + + # Attempt full replace of classification + file_cls = { + 'Intent': ['Structural'], + 'Contrast': ['B1', 'T1'], + 'Custom': ['Custom Value'] + } + + + r = as_admin.post('/projects/' + project + '/files/' + file_name + '/classification', json={ + 'replace': file_cls + }) + assert r.ok + + r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + assert r.ok + assert r.json()['classification'] == file_cls + + + # Use 'add' to add new key to list + r = as_admin.post('/projects/' + project + '/files/' + file_name + '/classification', json={ + 'add': {'Intent': ['Functional']} + }) + assert r.ok + + file_cls['Intent'].append('Functional') + r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + assert r.ok + assert r.json()['classification'] == file_cls + + + # Remove item from list + r = as_admin.post('/projects/' + project + '/files/' + file_name + '/classification', json={ + 'delete': {'Intent': ['Structural'], + 'Contrast': ['B1']} + }) + assert r.ok + + file_cls['Intent'] = ['Functional'] + file_cls['Contrast'] = ['T1'] + r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + assert r.ok + assert r.json()['classification'] == file_cls + + # Add and delete from same list + r = as_admin.post('/projects/' + project + '/files/' + file_name + '/classification', json={ + 'add': {'Intent': ['Localizer']}, + 'delete': {'Intent': ['Functional']} + }) + assert r.ok + + file_cls['Intent'] = ['Localizer'] + r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + assert r.ok + assert r.json()['classification'] == file_cls + + # Use 'delete' on keys that do not exist + r = as_admin.post('/projects/' + project + '/files/' + file_name + '/classification', json={ + 'delete': {'Intent': ['Structural', 'Functional']} + }) + assert r.ok + + r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + assert r.ok + assert r.json()['classification'] == file_cls + + # Use 'add' on keys that already exist + r = as_admin.post('/projects/' + project + '/files/' + file_name + '/classification', json={ + 'add': {'Intent': ['Localizer']} + }) + assert r.ok + + r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + assert r.ok + assert r.json()['classification'] == file_cls + + # Ensure lowercase gets formatted in correct format via modality's classification + r = as_admin.post('/projects/' + project + '/files/' + file_name + '/classification', json={ + 'add': {'contrast': ['t2', 'b0'], 'custom': ['lowercase']} + }) + assert r.ok + + file_cls['Contrast'].extend(['T2', 'B0']) + file_cls['Custom'].append('lowercase') + r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + assert r.ok + assert r.json()['classification'] == file_cls + + + # Use 'replace' to set file classification to {} + r = as_admin.post('/projects/' + project + '/files/' + file_name + '/classification', json={ + 'replace': {} + }) + assert r.ok + + r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + assert r.ok + assert r.json()['classification'] == {} + + # Attempt to add to nonexistent file + r = as_admin.post('/projects/' + project + '/files/' + 'madeup.txt' + '/classification', json={ + 'add': {'Intent': ['Localizer']} + }) + assert r.status_code == 404 + + # Attempt to delete from nonexistent file + r = as_admin.post('/projects/' + project + '/files/' + 'madeup.txt' + '/classification', json={ + 'delete': {'Intent': ['Localizer']} + }) + assert r.status_code == 404 + + # Attempt to replae nonexistent file + r = as_admin.post('/projects/' + project + '/files/' + 'madeup.txt' + '/classification', json={ + 'replace': {'Intent': ['Localizer']} + }) + assert r.status_code == 404 From a5c80f3950d5560df511e17fe4cec84a7d8bafd4 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Thu, 7 Dec 2017 13:39:30 -0600 Subject: [PATCH 15/23] Pylint --- api/handlers/listhandler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/handlers/listhandler.py b/api/handlers/listhandler.py index 24c990f41..7d217a3c2 100644 --- a/api/handlers/listhandler.py +++ b/api/handlers/listhandler.py @@ -549,7 +549,7 @@ def modify_classification(self, cont_name, list_name, **kwargs): validators.validate_data(payload, 'classification_update.json', 'input', 'POST') permchecker(noop)('PUT', _id=_id, query_params=kwargs, payload=payload) - result = storage.modify_classification(_id, kwargs, payload) + storage.modify_classification(_id, kwargs, payload) def post(self, cont_name, list_name, **kwargs): From 9240a3c90d489c3002a4bde1170e6920cb4cb44c Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Thu, 7 Dec 2017 14:24:58 -0600 Subject: [PATCH 16/23] Improve 404 message, remove redundant checks --- api/dao/liststorage.py | 13 +++---------- api/handlers/listhandler.py | 2 +- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/api/dao/liststorage.py b/api/dao/liststorage.py index df0eff9be..ffff2ac4b 100644 --- a/api/dao/liststorage.py +++ b/api/dao/liststorage.py @@ -241,9 +241,7 @@ def modify_classification(self, _id, query_params, payload): r_update = copy.deepcopy(update) r_update['$set'][self.list_name + '.$.classification'] = util.mongo_sanitize_fields(replace_payload) - result = self.dbc.update_one(query, r_update) - if result.matched_count == 0: - raise APINotFoundException('File not found in container {} {}.'.format(self.cont_name, _id)) + self.dbc.update_one(query, r_update) else: if add_payload: @@ -254,10 +252,7 @@ def modify_classification(self, _id, query_params, payload): for k,v in add_payload.iteritems(): a_update['$addToSet'][self.list_name + '.$.classification.' + k] = {'$each': v} - result = self.dbc.update_one(query, a_update) - if result.matched_count == 0: - raise APINotFoundException('File not found in container {} {}.'.format(self.cont_name, _id)) - + self.dbc.update_one(query, a_update) if delete_payload: delete_payload = check_and_format_classification(modality, delete_payload) @@ -268,9 +263,7 @@ def modify_classification(self, _id, query_params, payload): for k,v in delete_payload.iteritems(): d_update['$pullAll'][self.list_name + '.$.classification.' + k] = v - result = self.dbc.update_one(query, d_update) - if result.matched_count == 0: - raise APINotFoundException('File not found in container {} {}.'.format(self.cont_name, _id)) + self.dbc.update_one(query, d_update) diff --git a/api/handlers/listhandler.py b/api/handlers/listhandler.py index 7d217a3c2..534caec92 100644 --- a/api/handlers/listhandler.py +++ b/api/handlers/listhandler.py @@ -195,7 +195,7 @@ def _initialize_request(self, cont_name, list_name, _id, query_params=None): else: permchecker = permchecker(self, container) else: - self.abort(404, 'Element {} not found in container {}'.format(_id, storage.cont_name)) + self.abort(404, 'Element {} not found in {} {}'.format(query_params.values()[0], containerutil.singularize(storage.cont_name), _id)) mongo_schema_uri = validators.schema_uri('mongo', conf.get('storage_schema_file')) mongo_validator = validators.decorator_from_schema_path(mongo_schema_uri) From 2f893cc5b463c9b1eb9b1831b3a77e03a3b00ea4 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Thu, 7 Dec 2017 15:37:22 -0600 Subject: [PATCH 17/23] Add test to unaccepted keys --- api/handlers/modalityhandler.py | 5 ++--- .../python/test_classification.py | 22 +++++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/api/handlers/modalityhandler.py b/api/handlers/modalityhandler.py index 87a081395..84cfbd15f 100644 --- a/api/handlers/modalityhandler.py +++ b/api/handlers/modalityhandler.py @@ -56,7 +56,7 @@ def __init__(self, modality, errors): error_msg = 'Classification does not match format for modality {}. Unallowable key-value pairs: {}'.format(modality, errors) super(APIClassificationException, self).__init__(error_msg) - self.errors = str(errors) + self.errors = {'unaccepted_keys': errors} def case_insensitive_search(classifications, proposed_key, proposed_value): @@ -156,8 +156,7 @@ def check_and_format_classification(modality_name, classification_map): bad_kvs.append(k+':'+v) if bad_kvs: - raise Exception(bad_kvs) - #raise APIClassificationException(modality_name, bad_kvs) + raise APIClassificationException(modality_name, bad_kvs) return formatted_map diff --git a/tests/integration_tests/python/test_classification.py b/tests/integration_tests/python/test_classification.py index 3a75988f5..d80db1ede 100644 --- a/tests/integration_tests/python/test_classification.py +++ b/tests/integration_tests/python/test_classification.py @@ -199,6 +199,28 @@ def test_edit_file_classification(data_builder, as_admin, as_user, file_form): assert r.ok assert r.json()['classification'] == file_cls + # Ensure lowercase gets formatted in correct format via modality's classification + r = as_admin.post('/projects/' + project + '/files/' + file_name + '/classification', json={ + 'delete': {'contrast': ['t2'], 'custom': ['lowercase']} + }) + assert r.ok + + file_cls['Contrast'] = ['T1', 'B0'] + file_cls['Custom'] = ['Custom Value'] + r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + assert r.ok + assert r.json()['classification'] == file_cls + + # Try to replace with bad key names and values + r = as_admin.post('/projects/' + project + '/files/' + file_name + '/classification', json={ + 'replace': { + 'made-up': ['fake'], + 'Intent': ['not real'] + } + }) + assert r.status_code == 422 + assert r.json()['malformed_keys'] == ['made-up:fake', 'Intent:not real'] + # Use 'replace' to set file classification to {} r = as_admin.post('/projects/' + project + '/files/' + file_name + '/classification', json={ From 3fef7670e907e66243de12d400d1ac5f2eb36a4e Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Thu, 7 Dec 2017 15:44:44 -0600 Subject: [PATCH 18/23] Fix key change --- tests/integration_tests/python/test_classification.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration_tests/python/test_classification.py b/tests/integration_tests/python/test_classification.py index d80db1ede..17a908335 100644 --- a/tests/integration_tests/python/test_classification.py +++ b/tests/integration_tests/python/test_classification.py @@ -219,7 +219,7 @@ def test_edit_file_classification(data_builder, as_admin, as_user, file_form): } }) assert r.status_code == 422 - assert r.json()['malformed_keys'] == ['made-up:fake', 'Intent:not real'] + assert r.json()['unaccepted_keys'] == ['made-up:fake', 'Intent:not real'] # Use 'replace' to set file classification to {} From fb9abecdf02112a6818a47c31b9a2051f00e496b Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Wed, 13 Dec 2017 13:50:33 -0600 Subject: [PATCH 19/23] More tests/modifications --- api/handlers/modalityhandler.py | 10 +++++-- .../python/test_classification.py | 30 +++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/api/handlers/modalityhandler.py b/api/handlers/modalityhandler.py index 84cfbd15f..d9e9916ce 100644 --- a/api/handlers/modalityhandler.py +++ b/api/handlers/modalityhandler.py @@ -53,7 +53,10 @@ def delete(self, modality_name): class APIClassificationException(APIValidationException): def __init__(self, modality, errors): - error_msg = 'Classification does not match format for modality {}. Unallowable key-value pairs: {}'.format(modality, errors) + if not modality: + error_msg = 'Unknown modality. Classification must be set under "custom" key' + else: + error_msg = 'Classification does not match format for modality {}. Unallowable key-value pairs: {}'.format(modality, errors) super(APIClassificationException, self).__init__(error_msg) self.errors = {'unaccepted_keys': errors} @@ -125,11 +128,12 @@ def check_and_format_classification(modality_name, classification_map): try: modality = containerstorage.ContainerStorage('modalities', use_object_id=False).get_container(modality_name) except APINotFoundException as e: - if classification_map.keys() == ['Custom']: + keys = classification_map.keys() + if len(keys) == 1 and keys[0].lower() == 'custom': # for unknown modalities allow only list of custom values return classification_map else: - raise e + raise APIClassificationException(None, []) classifications = modality.get('classification', {}) diff --git a/tests/integration_tests/python/test_classification.py b/tests/integration_tests/python/test_classification.py index 17a908335..7bad23ae4 100644 --- a/tests/integration_tests/python/test_classification.py +++ b/tests/integration_tests/python/test_classification.py @@ -232,6 +232,36 @@ def test_edit_file_classification(data_builder, as_admin, as_user, file_form): assert r.ok assert r.json()['classification'] == {} + # Add Custom field for unknown modality + r = as_admin.put('/projects/' + project + '/files/' + file_name, json={ + 'modality': 'new unknown' + }) + + file_cls = { + 'Custom': ['Custom Value'] + } + + # allows custom fields + r = as_admin.post('/projects/' + project + '/files/' + file_name + '/classification', json={ + 'replace': file_cls + }) + assert r.ok + + r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + assert r.ok + assert r.json()['classification'] == file_cls + + # does not allow non-custom fields + file_cls = { + 'Intent': ['Structural'] + } + + r = as_admin.post('/projects/' + project + '/files/' + file_name + '/classification', json={ + 'replace': file_cls + }) + assert r.status_code == 422 + + # Attempt to add to nonexistent file r = as_admin.post('/projects/' + project + '/files/' + 'madeup.txt' + '/classification', json={ 'add': {'Intent': ['Localizer']} From 6a38109abbfa113b659487dfe5cd1505141d94d0 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Fri, 19 Jan 2018 15:35:30 -0600 Subject: [PATCH 20/23] Update docs for classification --- api/config.py | 73 ------------------- api/handlers/listhandler.py | 2 +- api/handlers/modalityhandler.py | 2 +- api/jobs/queue.py | 11 +++ swagger/examples/input/analysis.json | 6 +- .../examples/input/classification-update.json | 6 ++ swagger/examples/input/file.json | 2 +- swagger/examples/input/modality.json | 8 ++ swagger/examples/input/project-template.json | 12 +-- swagger/examples/input/rule-new.json | 2 +- swagger/examples/output/rule-list.json | 6 +- swagger/schemas/definitions/modality.json | 4 +- .../schemas/input/classification-update.json | 18 +++++ 13 files changed, 61 insertions(+), 91 deletions(-) create mode 100644 swagger/examples/input/classification-update.json create mode 100644 swagger/examples/input/modality.json create mode 100644 swagger/schemas/input/classification-update.json diff --git a/api/config.py b/api/config.py index bf8716318..999cc7767 100644 --- a/api/config.py +++ b/api/config.py @@ -1,6 +1,5 @@ import os import copy -import glob import json import logging import pymongo @@ -132,78 +131,6 @@ def apply_env_variables(config): # validate the lists of json schemas schema_path = os.path.join(os.path.dirname(os.path.abspath(__file__)), '../swagger/schemas') -expected_mongo_schemas = set([ - 'acquisition.json', - 'analysis.json', - 'collection.json', - 'container.json', - 'file.json', - 'group.json', - 'note.json', - 'permission.json', - 'project.json', - 'session.json', - 'subject.json', - 'user.json', - 'avatars.json', - 'tag.json' -]) -expected_input_schemas = set([ - 'acquisition.json', - 'acquisition-update.json', - 'analysis.json', - 'analysis-job.json', - 'analysis-update.json', - 'avatars.json', - 'classification_update.json', - 'collection.json', - 'collection-update.json', - 'device.json', - 'file.json', - 'file-update.json', - 'group-new.json', - 'group-update.json', - 'info_update.json', - 'modality.json', - 'note.json', - 'packfile.json', - 'permission.json', - 'project.json', - 'project-template.json', - 'project-update.json', - 'rule-new.json', - 'rule-update.json', - 'session.json', - 'session-update.json', - 'subject.json', - 'user-new.json', - 'user-update.json', - 'download.json', - 'tag.json', - 'enginemetadata.json', - 'labelupload.json', - 'uidupload.json', - 'uidmatchupload.json' -]) -mongo_schemas = set() -input_schemas = set() - -# check that the lists of schemas are correct -for schema_filepath in glob.glob(schema_path + '/mongo/*.json'): - schema_file = os.path.basename(schema_filepath) - mongo_schemas.add(schema_file) - with open(schema_filepath, 'rU') as f: - pass - -assert mongo_schemas == expected_mongo_schemas, '{} is different from {}'.format(mongo_schemas, expected_mongo_schemas) - -for schema_filepath in glob.glob(schema_path + '/input/*.json'): - schema_file = os.path.basename(schema_filepath) - input_schemas.add(schema_file) - with open(schema_filepath, 'rU') as f: - pass - -assert input_schemas == expected_input_schemas, '{} is different from {}'.format(input_schemas, expected_input_schemas) def create_or_recreate_ttl_index(coll_name, index_name, ttl): if coll_name in db.collection_names(): diff --git a/api/handlers/listhandler.py b/api/handlers/listhandler.py index 534caec92..6331b0139 100644 --- a/api/handlers/listhandler.py +++ b/api/handlers/listhandler.py @@ -546,7 +546,7 @@ def modify_classification(self, cont_name, list_name, **kwargs): payload = self.request.json_body - validators.validate_data(payload, 'classification_update.json', 'input', 'POST') + validators.validate_data(payload, 'classification-update.json', 'input', 'POST') permchecker(noop)('PUT', _id=_id, query_params=kwargs, payload=payload) storage.modify_classification(_id, kwargs, payload) diff --git a/api/handlers/modalityhandler.py b/api/handlers/modalityhandler.py index d9e9916ce..b5baee3aa 100644 --- a/api/handlers/modalityhandler.py +++ b/api/handlers/modalityhandler.py @@ -127,7 +127,7 @@ def check_and_format_classification(modality_name, classification_map): """ try: modality = containerstorage.ContainerStorage('modalities', use_object_id=False).get_container(modality_name) - except APINotFoundException as e: + except APINotFoundException: keys = classification_map.keys() if len(keys) == 1 and keys[0].lower() == 'custom': # for unknown modalities allow only list of custom values diff --git a/api/jobs/queue.py b/api/jobs/queue.py index 885924837..1df08e47b 100644 --- a/api/jobs/queue.py +++ b/api/jobs/queue.py @@ -234,6 +234,17 @@ def enqueue_job(job_map, origin, perm_check_uid=None): whitelisted_keys = ['info', 'tags', 'classification', 'mimetype', 'type', 'modality', 'size'] obj_projection = { key: obj.get(key) for key in whitelisted_keys } + ### + # recreate `measurements` list on object + # Can be removed when `classification` key has been adopted everywhere + + obj_projection['measurements'] = [] + if obj_projection.get('classification'): + for v in obj_projection['classification'].itervalues(): + obj_projection.extend(v) + # + ### + config_['inputs'][x] = { 'base': 'file', 'hierarchy': cr.__dict__, diff --git a/swagger/examples/input/analysis.json b/swagger/examples/input/analysis.json index 57a68f4fe..2e1330774 100644 --- a/swagger/examples/input/analysis.json +++ b/swagger/examples/input/analysis.json @@ -8,7 +8,7 @@ "hash": "v0-sha384-12188e00a26650b2baa3f0195337dcf504f4362bb2136eef0cdbefb57159356b1355a0402fca0ab5ab081f21c305e5c2", "name": "cortical_surface_right_hemisphere.obj", "tags": [], - "measurements": [], + "classification": {}, "modified": "2016-10-18T15:26:35.701000+00:00", "modality": null, "input": true, @@ -25,7 +25,7 @@ "hash": "v0-sha384-12188e00a26650b2baa3f0195337dcf504f4362bb2136eef0cdbefb57159356b1355a0402fca0ab5ab081f21c305e5c2", "name": "cortical_surface_right_hemisphere.obj", "tags": [], - "measurements": [], + "classification": {}, "modified": "2016-10-18T17:45:17.776000+00:00", "modality": null, "output": true, @@ -43,5 +43,5 @@ "description": "This is an analysis description", "job": "54759eb3c090d83494e2d804", "label": "Analysis label", - "user": "canakgun@flywheel.io" + "user": "user_1" } diff --git a/swagger/examples/input/classification-update.json b/swagger/examples/input/classification-update.json new file mode 100644 index 000000000..4c632e016 --- /dev/null +++ b/swagger/examples/input/classification-update.json @@ -0,0 +1,6 @@ +{ + "replace": { + "Contrast": ["B0", "Diffusion"], + "Intent": ["Structural"] + } +} diff --git a/swagger/examples/input/file.json b/swagger/examples/input/file.json index 798e3edc0..cefaf9fdf 100644 --- a/swagger/examples/input/file.json +++ b/swagger/examples/input/file.json @@ -2,7 +2,7 @@ "name": "4784_1_1_localizer_dicom.zip", "type": "dicom", "mimetype": "application/zip", - "measurements": [], + "classification": {}, "tags": [], "info": {} } diff --git a/swagger/examples/input/modality.json b/swagger/examples/input/modality.json new file mode 100644 index 000000000..19aebadf6 --- /dev/null +++ b/swagger/examples/input/modality.json @@ -0,0 +1,8 @@ +{ + "_id": "MR", + "classifications": { + "Contrast": ["B0", "B1", "T1", "T2", "T2*", "PD", "MT", "ASL", "Perfusion", "Diffusion", "Spectroscopy", "Susceptibility", "Velocity", "Fingerprinting"], + "Intent": ["Structural", "Functional", "Localizer", "Shim", "Calibration"], + "Features": ["Quantitative", "Multi-Shell", "Multi-Echo", "Multi-Flip", "Multi-Band", "Steady-State", "3D", "Compressed-Sensing", "Eddy-Current-Corrected", "Fieldmap-Corrected", "Gradient-Unwarped", "Motion-Corrected", "Physio-Corrected"] + } +} diff --git a/swagger/examples/input/project-template.json b/swagger/examples/input/project-template.json index a185b97dd..abd5353df 100644 --- a/swagger/examples/input/project-template.json +++ b/swagger/examples/input/project-template.json @@ -15,11 +15,11 @@ "$schema": "http://json-schema.org/draft-04/schema#", "type": "object", "properties": { - "measurement": { + "classification": { "type": "string", "pattern": "^[aA]natomical$" } }, - "required": ["measurement"] + "required": ["classification"] }, "minimum": 2 }, @@ -28,11 +28,11 @@ "$schema": "http://json-schema.org/draft-04/schema#", "type": "object", "properties": { - "measurement": { + "classification": { "type": "string", "pattern": "^functional$" } }, - "required": ["measurement"] + "required": ["classification"] }, "minimum": 1 }, @@ -41,13 +41,13 @@ "$schema": "http://json-schema.org/draft-04/schema#", "type": "object", "properties": { - "measurement": { "enum": ["Localizer"] }, + "classification": { "enum": ["Localizer"] }, "label": { "type": "string", "pattern": "t1" } }, - "required": ["label", "measurement"] + "required": ["label", "classification"] }, "minimum": 1 } diff --git a/swagger/examples/input/rule-new.json b/swagger/examples/input/rule-new.json index f730cd4c8..66b147e54 100644 --- a/swagger/examples/input/rule-new.json +++ b/swagger/examples/input/rule-new.json @@ -6,7 +6,7 @@ "all": [ { "regex": true, - "type": "file.measurements", + "type": "file.classification", "value": "^(?!non-image).+$" }, { diff --git a/swagger/examples/output/rule-list.json b/swagger/examples/output/rule-list.json index eb6126a22..d7248d76b 100644 --- a/swagger/examples/output/rule-list.json +++ b/swagger/examples/output/rule-list.json @@ -4,7 +4,7 @@ "all": [ { "regex": true, - "type": "file.measurements", + "type": "file.classification", "value": "^(?!non-image).+$" }, { @@ -25,7 +25,7 @@ }, { "regex": true, - "type": "file.measurements", + "type": "file.classification", "value": "^(?!non-image).+$" } ], @@ -42,7 +42,7 @@ }, { "regex": true, - "type": "file.measurements", + "type": "file.classification", "value": "(functional|anatomy_t1w|anatomy_t2w)" } ], diff --git a/swagger/schemas/definitions/modality.json b/swagger/schemas/definitions/modality.json index cc8cab0bf..bb84ab294 100644 --- a/swagger/schemas/definitions/modality.json +++ b/swagger/schemas/definitions/modality.json @@ -15,8 +15,8 @@ "items": { "type": "string" } - }, - }, + } + } }, "modality":{ "type": "object", diff --git a/swagger/schemas/input/classification-update.json b/swagger/schemas/input/classification-update.json new file mode 100644 index 000000000..fc5d017c1 --- /dev/null +++ b/swagger/schemas/input/classification-update.json @@ -0,0 +1,18 @@ +{ + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "Helper endpoint for editing a file's classification key", + "type": "object", + "oneOf": [ + { + "properties": { + "add": {"$ref":"../definitions/file.json#/definitions/classification"}, + "delete": {"$ref":"../definitions/file.json#/definitions/classification"} + }, "additionalProperties": false + }, + { + "properties": { + "replace": {"$ref":"../definitions/file.json#/definitions/classification"} + }, "additionalProperties": false + } + ] +} From 19e127266546ff9e896ede85e8e9e6a7a94c09bd Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Fri, 19 Jan 2018 16:20:00 -0600 Subject: [PATCH 21/23] Remove dm2 logic from placer, allow measurement --- api/placer.py | 31 ++++++++++--------------------- 1 file changed, 10 insertions(+), 21 deletions(-) diff --git a/api/placer.py b/api/placer.py index 14c1ee616..6a3f24adb 100644 --- a/api/placer.py +++ b/api/placer.py @@ -263,27 +263,16 @@ def check(self): validators.validate_data(self.metadata, 'enginemetadata.json', 'input', 'POST', optional=True) ### - # Remove when switch to dmv2 is complete across all gears - c_metadata = self.metadata.get(self.container_type, {}) # pragma: no cover - if self.context.get('job_id') and c_metadata and not c_metadata.get('files', []): # pragma: no cover - job = Job.get(self.context.get('job_id')) - input_names = [{'name': v.name} for v in job.inputs.itervalues()] - - measurement = self.metadata.get(self.container_type, {}).pop('measurement', None) - info = self.metadata.get(self.container_type,{}).pop('metadata', None) - modality = self.metadata.get(self.container_type, {}).pop('instrument', None) - if measurement or info or modality: - files_ = self.metadata[self.container_type].get('files', []) - files_ += input_names - for f in files_: - if measurement: - f['measurements'] = [measurement] - if info: - f['info'] = info - if modality: - f['modality'] = modality - - self.metadata[self.container_type]['files'] = files_ + # Shuttle `measurements` key into `classification` on files + ### + + if self.metadata.get(self.container_type, {}): # pragma: no cover + + for f in self.metadata[self.container_type].get('files', []): + + if 'measurements' in f: + m = f.pop('measurements') + f['classification'] = {'Custom': m} ### def process_file_field(self, field, file_attrs): From 64cc6f7562cdb558819ac27d4eeb935fb0e3e1bc Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Wed, 24 Jan 2018 17:14:41 -0600 Subject: [PATCH 22/23] Clean up tests, add db upgrade test --- api/dao/hierarchy.py | 10 +- api/dao/liststorage.py | 36 ++++-- bin/database.py | 22 ++++ .../python/test_containers.py | 6 + .../integration_tests/python/test_upgrades.py | 116 ++++++++++++++++++ 5 files changed, 176 insertions(+), 14 deletions(-) diff --git a/api/dao/hierarchy.py b/api/dao/hierarchy.py index e3bce623a..e64832bc0 100644 --- a/api/dao/hierarchy.py +++ b/api/dao/hierarchy.py @@ -144,7 +144,15 @@ def check_req(cont, req_k, req_v): """ Return True if container satisfies specific requirement. """ - cont_v = cont.get(req_k) + + # If looking at classification, translate to list rather than dictionary + if req_k == 'classification': + cont_v = [] + for v in cont.get('classification', {}).itervalues(): + cont_v.extend(v) + else: + cont_v = cont.get(req_k) + if cont_v: if isinstance(req_v, dict): for k,v in req_v.iteritems(): diff --git a/api/dao/liststorage.py b/api/dao/liststorage.py index ffff2ac4b..1d0d16e85 100644 --- a/api/dao/liststorage.py +++ b/api/dao/liststorage.py @@ -2,7 +2,6 @@ import bson.objectid import copy import datetime -import pymongo from ..web.errors import APIStorageException, APIConflictException, APINotFoundException from . import consistencychecker @@ -117,12 +116,23 @@ def _get_el(self, _id, query_params): if result and result.get(self.list_name): return result.get(self.list_name)[0] + def _update_session_compliance(self, _id): + if self.cont_name in ['sessions', 'acquisitions']: + if self.cont_name == 'sessions': + session_id = _id + else: + session_id = AcquisitionStorage().get_container(_id).get('session') + SessionStorage().recalc_session_compliance(session_id) + class FileStorage(ListStorage): def __init__(self, cont_name): super(FileStorage,self).__init__(cont_name, 'files', use_object_id=True) + def _create_jobs(self, container_before): + container_after = self.get_container(container_before['_id']) + return rules.create_jobs(config.db, container_before, container_after, self.cont_name) def _update_el(self, _id, query_params, payload, exclude_params): container_before = self.get_container(_id) @@ -147,11 +157,9 @@ def _update_el(self, _id, query_params, payload, exclude_params): '$set': mod_elem } - container_after = self.dbc.find_one_and_update(query, update, return_document=pymongo.collection.ReturnDocument.AFTER) - if not container_after: - raise APINotFoundException('Could not find and modify {} {}. file not updated'.format(_id, self.cont_name)) - - jobs_spawned = rules.create_jobs(config.db, container_before, container_after, self.cont_name) + self.dbc.find_one_and_update(query, update) + self._update_session_compliance(_id) + jobs_spawned = self._create_jobs(container_before) return { 'modified': 1, @@ -164,12 +172,7 @@ def _delete_el(self, _id, query_params): if f['name'] == query_params['name']: f['deleted'] = datetime.datetime.utcnow() result = self.dbc.update_one({'_id': _id}, {'$set': {'files': files, 'modified': datetime.datetime.utcnow()}}) - if self.cont_name in ['sessions', 'acquisitions']: - if self.cont_name == 'sessions': - session_id = _id - else: - session_id = AcquisitionStorage().get_container(_id).get('session') - SessionStorage().recalc_session_compliance(session_id) + self._update_session_compliance(_id) return result def _get_el(self, _id, query_params): @@ -217,9 +220,13 @@ def modify_info(self, _id, query_params, payload): else: update['$set']['modified'] = datetime.datetime.utcnow() - return self.dbc.update_one(query, update) + result = self.dbc.update_one(query, update) + self._update_session_compliance(_id) + return result + def modify_classification(self, _id, query_params, payload): + container_before = self.get_container(_id) update = {'$set': {'modified': datetime.datetime.utcnow()}} if self.use_object_id: @@ -265,6 +272,9 @@ def modify_classification(self, _id, query_params, payload): self.dbc.update_one(query, d_update) + self._update_session_compliance(_id) + self._create_jobs(container_before) + class StringListStorage(ListStorage): diff --git a/bin/database.py b/bin/database.py index 3863b7f99..e5c34b441 100755 --- a/bin/database.py +++ b/bin/database.py @@ -1425,6 +1425,25 @@ def adjust_type(r): config.db.project_rules.replace_one({'_id': rule['_id']}, rule) + return True + +def upgrade_templates_to_43(project): + """ + Set any measurements keys to classification + """ + + template = project['template'] + + for a in template.get('acquisitions', []): + for f in a.get('files', []): + if 'measurements' in f: + cl = f.pop('measurements') + f['classification'] = cl + + config.db.projects.update_one({'_id': project['_id']}, {'$set': {'template': template}}) + + return True + def upgrade_to_43(): """ Update classification for all files with existing measurements field @@ -1443,6 +1462,9 @@ def upgrade_to_43(): ]}) process_cursor(cursor, upgrade_rules_to_43) + cursor = config.db.projects.find({'template': {'$exists': True }}) + process_cursor(cursor, upgrade_templates_to_43) + diff --git a/tests/integration_tests/python/test_containers.py b/tests/integration_tests/python/test_containers.py index a5e72e4b2..f41a9e6fb 100644 --- a/tests/integration_tests/python/test_containers.py +++ b/tests/integration_tests/python/test_containers.py @@ -63,6 +63,8 @@ def test_project_template(data_builder, file_form, as_admin): assert as_admin.post('/acquisitions/' + acquisition_2 + '/files', files=file_form('non-compliant.txt')).ok assert as_admin.post('/acquisitions/' + acquisition_2 + '/files', files=file_form('compliant1.csv')).ok assert as_admin.post('/acquisitions/' + acquisition_2 + '/files', files=file_form('compliant2.csv')).ok + assert as_admin.post('/acquisitions/' + acquisition_2 + '/files/compliant1.csv/classification', json={'add': {'custom': ['diffusion']}}) + assert as_admin.post('/acquisitions/' + acquisition_2 + '/files/compliant2.csv/classification', json={'add': {'custom': ['diffusion']}}) # test the session before setting the template r = as_admin.get('/sessions/' + session) @@ -79,6 +81,7 @@ def test_project_template(data_builder, file_form, as_admin): 'files': [{ 'minimum': 2, 'mimetype': 'text/csv', + 'classification': 'diffusion' }] }] }) @@ -95,6 +98,7 @@ def test_project_template(data_builder, file_form, as_admin): 'files': [{ 'minimum': 2, 'mimetype': 'text/csv', + 'classification': 'diffusion' }] }] }) @@ -152,6 +156,8 @@ def satisfies_template(): assert as_admin.delete('/acquisitions/' + acquisition_2 + '/files/compliant2.csv').ok assert not satisfies_template() assert as_admin.post('/acquisitions/' + acquisition_2 + '/files', files=file_form('compliant2.csv')).ok + assert not satisfies_template() + assert as_admin.post('/acquisitions/' + acquisition_2 + '/files/compliant2.csv/classification', json={'add': {'custom': ['diffusion']}}) # acquisitions.minimum assert satisfies_template() diff --git a/tests/integration_tests/python/test_upgrades.py b/tests/integration_tests/python/test_upgrades.py index 4d0b51a05..055a98b4e 100644 --- a/tests/integration_tests/python/test_upgrades.py +++ b/tests/integration_tests/python/test_upgrades.py @@ -2,6 +2,7 @@ import sys import bson +import copy import pytest @@ -32,3 +33,118 @@ def test_42(data_builder, api_db, as_admin, database): # Verify archived was removed when false as well session_data = as_admin.get('/sessions/' + session2).json() assert 'archived' not in session_data + + +def test_43(data_builder, randstr, api_db, as_admin, database, file_form): + + # Set up files with measurements + + assert True + + containers = [ + ('collections', data_builder.create_collection()), + ('projects', data_builder.create_project()), + ('sessions', data_builder.create_session()), + ('acquisitions', data_builder.create_acquisition()) + ] + + for c in containers: + assert as_admin.post('/{}/{}/files'.format(c[0], c[1]), files=file_form('test.csv')).ok + assert as_admin.post('/{}/{}/files'.format(c[0], c[1]), files=file_form('test2.csv')).ok + api_db[c[0]].update_one({'_id': bson.ObjectId(c[1])}, + {'$set': { # Mangoes ... + 'files.0.measurements': ['diffusion', 'functional'], + 'files.1.measurements': ['diffusion', 'functional'] + }}) + + + # Set up rules referencing measurements + + rule = { + 'all' : [ + {'type' : 'file.measurement', 'value' : 'diffusion'}, + {'type' : 'container.has-measurement', 'value' : 'tests', 'regex': True} + ], + 'any' : [ + {'type' : 'file.measurement', 'value' : 'diffusion'}, + {'type' : 'container.has-measurement', 'value' : 'tests', 'regex': True} + ], + 'name' : 'Run dcm2niix on dicom', + 'alg' : 'dcm2niix', + 'project_id' : 'site' + } + + api_db.project_rules.insert(copy.deepcopy(rule)) + api_db.project_rules.insert(copy.deepcopy(rule)) + + + # Set up session templates referencing measurements + + t_project1 = data_builder.create_project() + t_project2 = data_builder.create_project() + + template = { + 'session': {'subject': {'code': '^compliant$'}}, + 'acquisitions': [{ + 'minimum': 1, + 'files': [{ + 'minimum': 2, + 'measurements': 'diffusion' + }] + }] + } + + assert as_admin.post('/projects/' + t_project1 + '/template', json=template).ok + assert as_admin.post('/projects/' + t_project2 + '/template', json=template).ok + + + ### RUN UPGRADE + + database.upgrade_to_43() + + #### + + + # Ensure files were updated + for c in containers: + files = as_admin.get('/{}/{}'.format(c[0], c[1])).json()['files'] + for f in files: + assert f['classification'] == {'Custom': ['diffusion', 'functional']} + + + # Ensure rules were updated + rule_after = { + 'all' : [ + {'type' : 'file.classification', 'value' : 'diffusion'}, + {'type' : 'container.has-classification', 'value' : 'tests', 'regex': True} + ], + 'any' : [ + {'type' : 'file.classification', 'value' : 'diffusion'}, + {'type' : 'container.has-classification', 'value' : 'tests', 'regex': True} + ], + 'name' : 'Run dcm2niix on dicom', + 'alg' : 'dcm2niix' + } + + rules = as_admin.get('/site/rules').json() + for r in rules: + r.pop('_id') + assert r == rule_after + + + # Ensure templates were updated + template_after = { + 'session': {'subject': {'code': '^compliant$'}}, + 'acquisitions': [{ + 'minimum': 1, + 'files': [{ + 'minimum': 2, + 'classification': 'diffusion' + }] + }] + } + for p in [t_project1, t_project2]: + assert as_admin.get('/projects/' + p).json()['template'] == template_after + + + From db0f6c91396d6fda057efe5b89bc07965779d4c7 Mon Sep 17 00:00:00 2001 From: Megan Henning Date: Mon, 12 Mar 2018 20:17:09 -0500 Subject: [PATCH 23/23] Small schema updates --- swagger/examples/input/analysis.json | 2 +- swagger/schemas/definitions/file.json | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/swagger/examples/input/analysis.json b/swagger/examples/input/analysis.json index 2e1330774..4efde2a56 100644 --- a/swagger/examples/input/analysis.json +++ b/swagger/examples/input/analysis.json @@ -43,5 +43,5 @@ "description": "This is an analysis description", "job": "54759eb3c090d83494e2d804", "label": "Analysis label", - "user": "user_1" + "user": "canakgun@flywheel.io" } diff --git a/swagger/schemas/definitions/file.json b/swagger/schemas/definitions/file.json index a54cb7f74..0ef2526c4 100644 --- a/swagger/schemas/definitions/file.json +++ b/swagger/schemas/definitions/file.json @@ -5,9 +5,7 @@ "file-type": { "type": "string" }, "mimetype": { "type": "string" }, "modality": { "type": "string" }, - "classification": { - "type": "object", - }, + "classification": { "type": "object" }, "tags": { "items": { "type": "string"}, "type": "array", @@ -82,7 +80,9 @@ "type": "object", "properties": { "type": {"$ref":"#/definitions/file-type"}, - "modality": {"$ref":"#/definitions/modality"} }, + "modality": {"$ref":"#/definitions/modality"}, + "classification": {"$ref":"#/definitions/classification"} + }, "additionalProperties": false }, "file-output":{