From 0f172f1d65e7741680059f638af1d9e6326b380a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thibault=20Bruy=C3=A8re?= Date: Mon, 4 Dec 2023 17:00:22 +0100 Subject: [PATCH] Fix memoization of bodyJson The old cacheBodyJson was not clean It used to replace the getBodyJson method which is very dangerous Instead, add a kwargs named "memoizedBodyJson" to the function call --- .../server/api/annotation.py | 41 ++++++++++------- .../server/api/connections.py | 44 +++++++++++-------- .../server/helpers/proxiedModel.py | 40 +++-------------- 3 files changed, 55 insertions(+), 70 deletions(-) diff --git a/devops/girder/plugins/AnnotationPlugin/upenncontrast_annotation/server/api/annotation.py b/devops/girder/plugins/AnnotationPlugin/upenncontrast_annotation/server/api/annotation.py index 43c0e633..d0ad292b 100644 --- a/devops/girder/plugins/AnnotationPlugin/upenncontrast_annotation/server/api/annotation.py +++ b/devops/girder/plugins/AnnotationPlugin/upenncontrast_annotation/server/api/annotation.py @@ -3,7 +3,7 @@ from girder.api.rest import Resource, loadmodel from girder.constants import AccessType from girder.exceptions import AccessException, RestException -from ..helpers.proxiedModel import recordable, cacheBodyJson +from ..helpers.proxiedModel import recordable, memoizeBodyJson from ..models.annotation import Annotation as AnnotationModel from bson.objectid import ObjectId @@ -12,11 +12,11 @@ # Helper functions to get dataset ID for recordable endpoints def getDatasetIdFromAnnotationInBody(self: 'Annotation', *args, **kwargs): - annotation = self.getBodyJson() + annotation = kwargs["memoizedBodyJson"] return annotation['datasetId'] def getDatasetIdFromAnnotationListInBody(self: 'Annotation', *args, **kwargs): - annotations = self.getBodyJson() + annotations = kwargs["memoizedBodyJson"] return None if len(annotations) <= 0 else annotations[0]['datasetId'] def getDatasetIdFromLoadedAnnotation(self: 'Annotation', *args, **kwargs): @@ -24,7 +24,7 @@ def getDatasetIdFromLoadedAnnotation(self: 'Annotation', *args, **kwargs): return annotation['datasetId'] def getDatasetIdFromAnnotationIdListInBody(self: 'Annotation', *args, **kwargs): - annotationStringIds = self.getBodyJson() + annotationStringIds = kwargs["memoizedBodyJson"] query = { '_id': { '$in': [ObjectId(stringId) for stringId in annotationStringIds] }, } @@ -57,25 +57,27 @@ def __init__(self): # TODO(performance): use objectId whenever possible # TODO: error handling and documentation - @cacheBodyJson @access.user @describeRoute(Description("Create a new annotation").param('body', 'Annotation Object', paramType='body')) + @memoizeBodyJson @recordable('Create an annotation', getDatasetIdFromAnnotationInBody) - def create(self, params): + def create(self, params, *args, **kwargs): + bodyJson = kwargs["memoizedBodyJson"] currentUser = self.getCurrentUser() if not currentUser: raise AccessException('User not found', 'currentUser') - return self._annotationModel.create(currentUser, self.getBodyJson()) + return self._annotationModel.create(currentUser, bodyJson) - @cacheBodyJson @access.user @describeRoute(Description("Create multiple new annotations").param('body', 'Annotation Object List', paramType='body')) + @memoizeBodyJson @recordable('Create multiple annotations', getDatasetIdFromAnnotationListInBody) - def createMultiple(self, params): + def createMultiple(self, params, *args, **kwargs): + bodyJson = kwargs["memoizedBodyJson"] currentUser = self.getCurrentUser() if not currentUser: raise AccessException('User not found', 'currentUser') - return [self._annotationModel.create(currentUser, annotation) for annotation in self.getBodyJson()] + return [self._annotationModel.create(currentUser, annotation) for annotation in bodyJson] @describeRoute(Description("Delete an existing annotation").param('id', 'The annotation\'s Id', paramType='path').errorResponse('ID was invalid.') .errorResponse('Write access was denied for the annotation.', 403)) @@ -85,13 +87,14 @@ def createMultiple(self, params): def delete(self, upenn_annotation, params): self._annotationModel.delete(upenn_annotation) - @cacheBodyJson @access.user @describeRoute(Description("Delete all annotations in the id list") .param('body', 'A list of all annotation ids to delete.', paramType='body')) + @memoizeBodyJson @recordable('Delete multiple annotations', getDatasetIdFromAnnotationIdListInBody) - def deleteMultiple(self, params): - stringIds = [stringId for stringId in self.getBodyJson()] + def deleteMultiple(self, params, *args, **kwargs): + bodyJson = kwargs["memoizedBodyJson"] + stringIds = [stringId for stringId in bodyJson] self._annotationModel.deleteMultiple(stringIds) @describeRoute(Description("Update an existing annotation") @@ -103,9 +106,11 @@ def deleteMultiple(self, params): .errorResponse("Validation Error: JSON doesn't follow schema.")) @access.user @loadmodel(model='upenn_annotation', plugin='upenncontrast_annotation', level=AccessType.WRITE) + @memoizeBodyJson @recordable('Update an annotation', getDatasetIdFromLoadedAnnotation) - def update(self, upenn_annotation, params): - upenn_annotation.update(self.getBodyJson()) + def update(self, upenn_annotation, params, *args, **kwargs): + bodyJson = kwargs["memoizedBodyJson"] + upenn_annotation.update(bodyJson) self._annotationModel.update(upenn_annotation) @access.user @@ -141,8 +146,10 @@ def get(self, upenn_annotation, params): @describeRoute(Description("Compute annotations from a worker tool") .param('datasetId', 'The dataset Id', required=False) .param('body', 'A JSON object containing the worker tool', paramType='body')) - def compute(self, params): + @memoizeBodyJson + def compute(self, params, *args, **kwargs): + bodyJson = kwargs["memoizedBodyJson"] datasetId = params.get('datasetId', None) if not datasetId: raise RestException(code=400, message="Missing datasetId parameter") - return self._annotationModel.compute(datasetId, self.getBodyJson(), self.getCurrentUser()) + return self._annotationModel.compute(datasetId, bodyJson, self.getCurrentUser()) diff --git a/devops/girder/plugins/AnnotationPlugin/upenncontrast_annotation/server/api/connections.py b/devops/girder/plugins/AnnotationPlugin/upenncontrast_annotation/server/api/connections.py index 5f3a4632..773204a0 100644 --- a/devops/girder/plugins/AnnotationPlugin/upenncontrast_annotation/server/api/connections.py +++ b/devops/girder/plugins/AnnotationPlugin/upenncontrast_annotation/server/api/connections.py @@ -3,7 +3,7 @@ from girder.api.rest import Resource, loadmodel from girder.constants import AccessType from girder.exceptions import AccessException -from ..helpers.proxiedModel import recordable, cacheBodyJson +from ..helpers.proxiedModel import recordable, memoizeBodyJson from ..models.connections import AnnotationConnection as ConnectionModel from ..models.annotation import Annotation as AnnotationModel @@ -13,11 +13,11 @@ # Helper functions to get dataset ID for recordable endpoints def getDatasetIdFromConnectionInBody(self: 'AnnotationConnection', *args, **kwargs): - connection = self.getBodyJson() + connection = kwargs["memoizedBodyJson"] return connection['datasetId'] def getDatasetIdFromConnectionListInBody(self: 'AnnotationConnection', *args, **kwargs): - connections = self.getBodyJson() + connections = kwargs["memoizedBodyJson"] return None if len(connections) <= 0 else connections[0]['datasetId'] def getDatasetIdFromLoadedConnection(self: 'AnnotationConnection', *args, **kwargs): @@ -25,7 +25,7 @@ def getDatasetIdFromLoadedConnection(self: 'AnnotationConnection', *args, **kwar return connection['datasetId'] def getDatasetIdFromConnectionIdListInBody(self: 'AnnotationConnection', *args, **kwargs): - connectionStringIds = self.getBodyJson() + connectionStringIds = kwargs["memoizedBodyJson"] query = { '_id': { '$in': [ObjectId(stringId) for stringId in connectionStringIds] }, } @@ -34,7 +34,7 @@ def getDatasetIdFromConnectionIdListInBody(self: 'AnnotationConnection', *args, return None if connection is None else connection['datasetId'] def getDatasetIdFromInfoInBody(self: 'AnnotationConnection', *args, **kwargs): - info = self.getBodyJson() + info = kwargs["memoizedBodyJson"] annotationsIdsToConnect = info['annotationsIds'] annotationModel: AnnotationModel = AnnotationModel() for stringId in annotationsIdsToConnect: @@ -67,25 +67,27 @@ def __init__(self): # TODO: creation date, update date, creatorId # TODO: error handling and documentation - @cacheBodyJson @access.user @describeRoute(Description("Create a new connection").param('body', 'Connection Object', paramType='body')) + @memoizeBodyJson @recordable('Create a connection', getDatasetIdFromConnectionInBody) - def create(self, params): + def create(self, params, *args, **kwargs): + bodyJson = kwargs["memoizedBodyJson"] currentUser = self.getCurrentUser() if not currentUser: raise AccessException('User not found', 'currentUser') - return self._connectionModel.create(currentUser, self.getBodyJson()) + return self._connectionModel.create(currentUser, bodyJson) - @cacheBodyJson @access.user @describeRoute(Description("Create multiple new connections").param('body', 'Connection Object List', paramType='body')) + @memoizeBodyJson @recordable('Create multiple connections', getDatasetIdFromConnectionListInBody) - def multipleCreate(self, params): + def multipleCreate(self, params, *args, **kwargs): + bodyJson = kwargs["memoizedBodyJson"] currentUser = self.getCurrentUser() if not currentUser: raise AccessException('User not found', 'currentUser') - return [self._connectionModel.create(currentUser, connection) for connection in self.getBodyJson()] + return [self._connectionModel.create(currentUser, connection) for connection in bodyJson] @describeRoute(Description("Delete an existing connection").param('id', 'The connection\'s Id', paramType='path').errorResponse('ID was invalid.') .errorResponse('Write access was denied for the connection.', 403)) @@ -95,13 +97,14 @@ def multipleCreate(self, params): def delete(self, annotation_connection, params): self._connectionModel.remove(annotation_connection) - @cacheBodyJson @access.user @describeRoute(Description("Delete all annotation connections in the id list") .param('body', 'A list of all annotation connection ids to delete.', paramType='body')) + @memoizeBodyJson @recordable('Delete multiple connections', getDatasetIdFromConnectionIdListInBody) - def deleteMultiple(self, params): - stringIds = [stringId for stringId in self.getBodyJson()] + def deleteMultiple(self, params, *args, **kwargs): + bodyJson = kwargs["memoizedBodyJson"] + stringIds = [stringId for stringId in bodyJson] self._connectionModel.deleteMultiple(stringIds) @describeRoute(Description("Update an existing connection") @@ -113,9 +116,11 @@ def deleteMultiple(self, params): .errorResponse("Validation Error: JSON doesn't follow schema.")) @access.user @loadmodel(model='annotation_connection', plugin='upenncontrast_annotation', level=AccessType.WRITE) + @memoizeBodyJson @recordable('Update a connection', getDatasetIdFromLoadedConnection) - def update(self, connection, params): - connection.update(self.getBodyJson()) + def update(self, connection, params, *args, **kwargs): + bodyJson = kwargs["memoizedBodyJson"] + connection.update(bodyJson) self._connectionModel.update(connection) @access.user @@ -156,12 +161,13 @@ def get(self, annotation_connection): return annotation_connection - @cacheBodyJson @access.user @describeRoute(Description("Create connections between annotations").param('body', 'Connection Object', paramType='body')) + @memoizeBodyJson @recordable('Create connections with nearest', getDatasetIdFromInfoInBody) - def connectToNearest(self, params): + def connectToNearest(self, params, *args, **kwargs): + bodyJson = kwargs["memoizedBodyJson"] currentUser = self.getCurrentUser() if not currentUser: raise AccessException('User not found', 'currentUser') - return self._connectionModel.connectToNearest(user=currentUser, info=self.getBodyJson()) + return self._connectionModel.connectToNearest(user=currentUser, info=bodyJson) diff --git a/devops/girder/plugins/AnnotationPlugin/upenncontrast_annotation/server/helpers/proxiedModel.py b/devops/girder/plugins/AnnotationPlugin/upenncontrast_annotation/server/helpers/proxiedModel.py index 1479100d..8511c091 100644 --- a/devops/girder/plugins/AnnotationPlugin/upenncontrast_annotation/server/helpers/proxiedModel.py +++ b/devops/girder/plugins/AnnotationPlugin/upenncontrast_annotation/server/helpers/proxiedModel.py @@ -10,51 +10,23 @@ from functools import wraps from bson.objectid import ObjectId -def cacheBodyJson(func): +def memoizeBodyJson(func): ''' A decorator on rest.Resource methods to cache the result of self.getBodyJson() This is usefull when some decorators and the decorated function use it - For example, when using @recordable - Use this decorator before any other decorator, for example: + For example, when using @recordable with a findDatasetIdFn that uses bodyJson + + Use this decorator before any other decorator using memoizedBodyJson: ``` - @cacheBodyJson + @memoizeBodyJson @recordable('Foo', bar) def f(self, *args, **kwargs): pass ``` - instead of - ``` - @recordable('Foo', bar) - @cacheBodyJson - def f(self, *args, **kwargs): - pass - ``` ''' @wraps(func) def wrapped(self: rest.Resource, *args, **kwargs): - # Get the result of getBodyJson - error = None - bodyJson = None - try: - bodyJson = self.getBodyJson() - except rest.RestException as err: - error = err - - # Create memoised version of the method - def getCachedBodyJson(self): - return deepcopy(bodyJson) - def raiseBodyJsonError(self): - raise error - newFunction = getCachedBodyJson if error is None else raiseBodyJsonError - newMethod = MethodType(newFunction, self) - - # Wrap the function call - originalMethod = self.getBodyJson - try: - self.getBodyJson = newMethod - return func(self, *args, **kwargs) - finally: - self.getBodyJson = originalMethod + return func(self, *args, **kwargs, memoizedBodyJson=self.getBodyJson()) return wrapped