Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion api/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -243,8 +243,9 @@ def prefix(path, routes):
route('/packfile', FileListHandler, h='packfile', m=['POST']),
route('/packfile-end', FileListHandler, h='packfile_end'),
route('/<list_name:files>', FileListHandler, m=['POST']),
route('/<list_name:files>/<name:{fname}>', FileListHandler, m=['GET', 'DELETE']),
route('/<list_name:files>/<name:{fname}>', FileListHandler, m=['GET', 'PUT', 'DELETE']),
route('/<list_name:files>/<name:{fname}>/info', FileListHandler, h='get_info', m=['GET']),
route('/<list_name:files>/<name:{fname}>/info', FileListHandler, h='modify_info', m=['POST']),

route( '/analyses', AnalysesHandler, m=['POST']),
prefix('/analyses', [
Expand Down
2 changes: 2 additions & 0 deletions api/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,10 @@ def apply_env_variables(config):
'container.json',
'device.json',
'file.json',
'file-update.json',
'group-new.json',
'group-update.json',
'info_update.json',
'note.json',
'packfile.json',
'permission.json',
Expand Down
99 changes: 84 additions & 15 deletions api/dao/liststorage.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
import bson.errors
import bson.objectid
import datetime
import pymongo

from . import APIStorageException, APIConflictException
from . import APIStorageException, APIConflictException, APINotFoundException
from . import consistencychecker
from .. import config
from .. import util
from ..jobs import rules
from .containerstorage import SessionStorage, AcquisitionStorage

log = config.log
Expand Down Expand Up @@ -41,7 +44,6 @@ def get_container(self, _id, query_params=None):
'$elemMatch': query_params
}
projection = {self.list_name + '.$': 1, 'permissions': 1, 'public': 1}
log.debug('query {}'.format(query))
return self.dbc.find_one(query, projection)

def exec_op(self, action, _id=None, query_params=None, payload=None, exclude_params=None):
Expand All @@ -67,24 +69,19 @@ def exec_op(self, action, _id=None, query_params=None, payload=None, exclude_par
raise ValueError('action should be one of GET, POST, PUT, DELETE')

def _create_el(self, _id, payload, exclude_params):
log.debug('payload {}'.format(payload))
query = {'_id': _id}
if exclude_params:
query[self.list_name] = {'$not': {'$elemMatch': exclude_params} }
update = {
'$push': {self.list_name: payload},
'$set': {'modified': datetime.datetime.utcnow()}
}
log.debug('query {}'.format(query))
log.debug('update {}'.format(update))
result = self.dbc.update_one(query, update)
if result.matched_count < 1:
raise APIConflictException('Item already exists in list.')
return result

def _update_el(self, _id, query_params, payload, exclude_params):
log.debug('query_params {}'.format(query_params))
log.debug('payload {}'.format(payload))
mod_elem = {}
for k,v in payload.items():
mod_elem[self.list_name + '.$.' + k] = v
Expand All @@ -100,19 +97,14 @@ def _update_el(self, _id, query_params, payload, exclude_params):
update = {
'$set': mod_elem
}
log.debug('query {}'.format(query))
log.debug('update {}'.format(update))
return self.dbc.update_one(query, update)

def _delete_el(self, _id, query_params):
log.debug('query_params {}'.format(query_params))
query = {'_id': _id}
update = {
'$pull': {self.list_name: query_params},
'$set': { 'modified': datetime.datetime.utcnow()}
}
log.debug('query {}'.format(query))
log.debug('update {}'.format(update))
result = self.dbc.update_one(query, update)
if self.list_name is 'files' and self.cont_name in ['sessions', 'acquisitions']:
if self.cont_name == 'sessions':
Expand All @@ -123,16 +115,93 @@ def _delete_el(self, _id, query_params):
return result

def _get_el(self, _id, query_params):
log.debug('query_params {}'.format(query_params))
query = {'_id': _id, self.list_name: {'$elemMatch': query_params}}
projection = {self.list_name + '.$': 1}
log.debug('query {}'.format(query))
log.debug('projection {}'.format(projection))
result = self.dbc.find_one(query, projection)
if result and result.get(self.list_name):
return result.get(self.list_name)[0]


class FileStorage(ListStorage):

def __init__(self, cont_name):
super(FileStorage,self).__init__(cont_name, 'files', use_object_id=True)


def _update_el(self, _id, query_params, payload, exclude_params):
container_before = self.get_container(_id)
if not container_before:
raise APINotFoundException('Could not find {} {}, file not updated.'.format(
_id, self.cont_name
))

mod_elem = {}
for k,v in payload.items():
mod_elem[self.list_name + '.$.' + k] = v
query = {'_id': _id }
if exclude_params is None:
query[self.list_name] = {'$elemMatch': query_params}
else:
query['$and'] = [
{self.list_name: {'$elemMatch': query_params}},
{self.list_name: {'$not': {'$elemMatch': exclude_params} }}
]
mod_elem['modified'] = datetime.datetime.utcnow()
update = {
'$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)

return {
'modified': 1,
'jobs_triggered': len(jobs_spawned)
}


def modify_info(self, _id, query_params, payload):
update = {}
set_payload = payload.get('set')
delete_payload = payload.get('delete')
replace_payload = payload.get('replace')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: reject the update if both replace and (delete or set) is not None.

I get that the json schema prevents the handler from sending over a payload that is invalid in that manner, but it make sense (to me at least) that the persistent layer should only allow valid mutations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we consider the schemas to be an integral part of the persistence layer? If not, wouldn't we need to add input validation all over the code? Maybe this is different somehow, and I'm not seeing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had a conversation about this very thing :) Our conclusion is that in general, you don't need to re-implement schemas, but the persistence layer isn't an HTTP layer and at minimum shouldn't expose undefined behavior. This is the rare exception (combining set with replace with delete).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kofalt and I briefly discussed this. Since it's not business or schema logic, I added an extra check for other parts of the code that may call this function. I'd rather have a descriptive 500 if someone misused it than a mongo error about key-wise vs full key sets. I do agree that business logic/schema validation belongs at the handler levels.


if (set_payload or delete_payload) and replace_payload is not None:
raise APIStorageException('Cannot set or delete AND replace info fields.')

if replace_payload is not None:
update = {
'$set': {
self.list_name + '.$.info': util.mongo_sanitize_fields(replace_payload)
}
}

else:
if set_payload:
update['$set'] = {}
for k,v in set_payload.items():
update['$set'][self.list_name + '.$.info.' + k] = util.mongo_sanitize_fields(v)
if delete_payload:
update['$unset'] = {}
for k in delete_payload:
update['$unset'][self.list_name + '.$.info.' + k] = ''

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):
"""
This class provides access to string sublists of a mongodb collections elements (called containers).
Expand Down
44 changes: 38 additions & 6 deletions api/handlers/listhandler.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def initialize_list_configurations():
'input_schema_file': 'tag.json'
},
'files': {
'storage': liststorage.ListStorage,
'storage': liststorage.FileStorage,
'permchecker': listauth.default_sublist,
'use_object_id': True,
'storage_schema_file': 'file.json',
Expand Down Expand Up @@ -87,11 +87,14 @@ def initialize_list_configurations():
for cont_name, cont_config in list_container_configurations.iteritems():
for list_name, list_config in cont_config.iteritems():
storage_class = list_config['storage']
storage = storage_class(
cont_name,
list_name,
use_object_id=list_config.get('use_object_id', False)
)
if list_name == 'files':
storage = storage_class(cont_name)
else:
storage = storage_class(
cont_name,
list_name,
use_object_id=list_config.get('use_object_id', False)
)
list_config['storage'] = storage
return list_container_configurations

Expand Down Expand Up @@ -501,6 +504,25 @@ def get(self, cont_name, list_name, **kwargs):
def get_info(self, cont_name, list_name, **kwargs):
return super(FileListHandler,self).get(cont_name, list_name, **kwargs)

def modify_info(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, 'info_update.json', 'input', 'POST')

try:
permchecker(noop)('PUT', _id=_id, query_params=kwargs, payload=payload)
result = storage.modify_info(_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))
else:
return {'modified':result.modified_count}

def post(self, cont_name, list_name, **kwargs):
_id = kwargs.pop('cid')

Expand All @@ -517,6 +539,16 @@ def post(self, cont_name, list_name, **kwargs):

return upload.process_upload(self.request, upload.Strategy.targeted, container_type=cont_name, id_=_id, origin=self.origin)

def put(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, 'file-update.json', 'input', 'PUT')

result = permchecker(storage.exec_op)('PUT', _id=_id, query_params=kwargs, payload=payload)
return result

def delete(self, cont_name, list_name, **kwargs):
# Overriding base class delete to audit action before completion
_id = kwargs.pop('cid')
Expand Down
9 changes: 9 additions & 0 deletions raml/schemas/definitions/file.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,15 @@
},
"additionalProperties": false
},
"file-update":{
"type": "object",
"properties": {
"type": {"$ref":"#/definitions/file-type"},
"modality": {"$ref":"#/definitions/modality"},
"measurements": {"$ref":"#/definitions/measurements"}
},
"additionalProperties": false
},
"file-output":{
"type": "object",
"properties": {
Expand Down
4 changes: 4 additions & 0 deletions raml/schemas/input/file-update.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"$schema": "http://json-schema.org/draft-04/schema#",
"allOf":[{"$ref":"../definitions/file.json#/definitions/file-update"}]
}
25 changes: 25 additions & 0 deletions raml/schemas/input/info_update.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
{
"$schema": "http://json-schema.org/draft-04/schema#",
"description": "Helper endpoint for editing an object's info key",
"type": "object",
"oneOf": [
{
"properties": {
"set": {"type": "object", "minProperties": 1},
"delete": {
"type": "array",
"uniqueItems": true,
"minItems": 1,
"items": {
"type": "string"
}
}
}, "additionalProperties": false
},
{
"properties": {
"replace": {"type": "object"}
}, "additionalProperties": false
}
]
}
Loading