From ae79b84c1872ae68fe7272617f37333216391f14 Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Wed, 28 Feb 2018 12:28:58 -0600 Subject: [PATCH 1/4] Engine uploads convert ':' to '/' --- api/placer.py | 4 +- .../integration_tests/python/test_uploads.py | 48 +++++++++++++++---- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/api/placer.py b/api/placer.py index 2e1215092..cfe5a163f 100644 --- a/api/placer.py +++ b/api/placer.py @@ -299,6 +299,8 @@ def process_file_field(self, field, file_attrs): if not job_ticket['success']: file_attrs['from_failed_job'] = True + file_attrs['name'] = file_attrs['name'].replace(':', '/') + self.save_file(field, file_attrs) self.saved.append(file_attrs) @@ -318,7 +320,7 @@ def finalize(self): file_mds = self.metadata.get(self.container_type, {}).get('files', []) saved_file_names = [x.get('name') for x in self.saved] for file_md in file_mds: - if file_md['name'] not in saved_file_names: + if file_md['name'].replace(':', '/') not in saved_file_names: self.save_file(None, file_md) # save file_attrs update only self.saved.append(file_md) diff --git a/tests/integration_tests/python/test_uploads.py b/tests/integration_tests/python/test_uploads.py index 607ec86e8..d556057ca 100644 --- a/tests/integration_tests/python/test_uploads.py +++ b/tests/integration_tests/python/test_uploads.py @@ -701,10 +701,23 @@ def test_acquisition_engine_upload(data_builder, file_form, as_root): ) assert r.status_code == 404 + metadata['acquisition']['files'] = [ + { + 'name': 'one.csv', + 'type': 'engine type 0', + 'info': {'test': 'f0'} + }, + { + 'name': 'folder:two.csv', + 'type': 'engine type 1', + 'info': {'test': 'f1'} + } + ] + # engine upload r = as_root.post('/engine', params={'level': 'acquisition', 'id': acquisition, 'job': job}, - files=file_form('one.csv', 'two.csv', meta=metadata) + files=file_form('one.csv', 'folder:two.csv', meta=metadata) ) assert r.ok @@ -734,7 +747,7 @@ def test_acquisition_engine_upload(data_builder, file_form, as_root): assert a_timestamp == m_timestamp for mf in metadata['acquisition']['files']: - f = find_file_in_array(mf['name'], a['files']) + f = find_file_in_array(mf['name'].replace(':', '/'), a['files']) assert mf is not None assert f['type'] == mf['type'] assert f['info'] == mf['info'] @@ -764,6 +777,11 @@ def test_session_engine_upload(data_builder, file_form, as_root): 'name': 'two.csv', 'type': 'engine type 1', 'info': {'test': 'f1'} + }, + { + 'name': 'folder:three.csv', + 'type': 'engine type 2', + 'info': {'test': 'f2'} } ] } @@ -771,7 +789,7 @@ def test_session_engine_upload(data_builder, file_form, as_root): r = as_root.post('/engine', params={'level': 'session', 'id': session}, - files=file_form('one.csv', 'two.csv', meta=metadata) + files=file_form('one.csv', 'two.csv', 'folder:three.csv', meta=metadata) ) assert r.ok @@ -794,7 +812,11 @@ def test_session_engine_upload(data_builder, file_form, as_root): assert s_timestamp == m_timestamp for f in s['files']: - mf = find_file_in_array(f['name'], metadata['session']['files']) + if '/' in f['name']: + assert f['name'] == 'folder/three.csv' + mf = find_file_in_array(f['name'].replace('/', ':'), metadata['session']['files']) + else: + mf = find_file_in_array(f['name'], metadata['session']['files']) assert mf is not None assert f['type'] == mf['type'] assert f['info'] == mf['info'] @@ -816,6 +838,11 @@ def test_project_engine_upload(data_builder, file_form, as_root): 'name': 'two.csv', 'type': 'engine type 1', 'info': {'test': 'f1'} + }, + { + 'name': 'folder:three.csv', + 'type': 'engine type 2', + 'info': {'test': 'f2'} } ] } @@ -823,7 +850,7 @@ def test_project_engine_upload(data_builder, file_form, as_root): r = as_root.post('/engine', params={'level': 'project', 'id': project}, - files=file_form('one.csv', 'two.csv', meta=metadata) + files=file_form('one.csv', 'two.csv', 'folder:three.csv', meta=metadata) ) assert r.ok @@ -835,7 +862,11 @@ def test_project_engine_upload(data_builder, file_form, as_root): assert p['info'] == metadata['project']['info'] for f in p['files']: - mf = find_file_in_array(f['name'], metadata['project']['files']) + if '/' in f['name']: + assert f['name'] == 'folder/three.csv' + mf = find_file_in_array(f['name'].replace('/', ':'), metadata['project']['files']) + else: + mf = find_file_in_array(f['name'], metadata['project']['files']) assert mf is not None assert f['type'] == mf['type'] assert f['info'] == mf['info'] @@ -843,7 +874,8 @@ def test_project_engine_upload(data_builder, file_form, as_root): def test_acquisition_file_only_engine_upload(data_builder, file_form, as_root): acquisition = data_builder.create_acquisition() - file_names = ['one.csv', 'two.csv'] + file_names = ['one.csv', 'folder:two.csv'] + expected_file_names = ['one.csv', 'folder/two.csv'] r = as_root.post('/engine', params={'level': 'acquisition', 'id': acquisition}, @@ -853,7 +885,7 @@ def test_acquisition_file_only_engine_upload(data_builder, file_form, as_root): r = as_root.get('/acquisitions/' + acquisition) assert r.ok - assert set(f['name'] for f in r.json()['files']) == set(file_names) + assert set(f['name'] for f in r.json()['files']) == set(expected_file_names) def test_acquisition_subsequent_file_engine_upload(data_builder, file_form, as_root): From 9f3ad43689ac1edf7e26ea46a3bce68e7898dc90 Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Wed, 28 Feb 2018 16:06:02 -0600 Subject: [PATCH 2/4] Match on metadata fname basename on engine upload --- api/placer.py | 7 ++--- .../integration_tests/python/test_uploads.py | 31 +++++++------------ 2 files changed, 14 insertions(+), 24 deletions(-) diff --git a/api/placer.py b/api/placer.py index cfe5a163f..5f2068f4e 100644 --- a/api/placer.py +++ b/api/placer.py @@ -2,6 +2,7 @@ import copy import datetime import dateutil +import os import pymongo import uuid import zipfile @@ -289,7 +290,7 @@ def process_file_field(self, field, file_attrs): file_mds = self.metadata.get(self.container_type, {}).get('files', []) for file_md in file_mds: - if file_md['name'] == file_attrs['name']: + if os.path.basename(file_md['name']) == file_attrs['name']: file_attrs.update(file_md) break @@ -299,8 +300,6 @@ def process_file_field(self, field, file_attrs): if not job_ticket['success']: file_attrs['from_failed_job'] = True - file_attrs['name'] = file_attrs['name'].replace(':', '/') - self.save_file(field, file_attrs) self.saved.append(file_attrs) @@ -320,7 +319,7 @@ def finalize(self): file_mds = self.metadata.get(self.container_type, {}).get('files', []) saved_file_names = [x.get('name') for x in self.saved] for file_md in file_mds: - if file_md['name'].replace(':', '/') not in saved_file_names: + if file_md['name'] not in saved_file_names: self.save_file(None, file_md) # save file_attrs update only self.saved.append(file_md) diff --git a/tests/integration_tests/python/test_uploads.py b/tests/integration_tests/python/test_uploads.py index d556057ca..3acaf0ac1 100644 --- a/tests/integration_tests/python/test_uploads.py +++ b/tests/integration_tests/python/test_uploads.py @@ -708,7 +708,7 @@ def test_acquisition_engine_upload(data_builder, file_form, as_root): 'info': {'test': 'f0'} }, { - 'name': 'folder:two.csv', + 'name': 'folder/two.csv', 'type': 'engine type 1', 'info': {'test': 'f1'} } @@ -717,7 +717,7 @@ def test_acquisition_engine_upload(data_builder, file_form, as_root): # engine upload r = as_root.post('/engine', params={'level': 'acquisition', 'id': acquisition, 'job': job}, - files=file_form('one.csv', 'folder:two.csv', meta=metadata) + files=file_form('one.csv', 'two.csv', meta=metadata) ) assert r.ok @@ -747,7 +747,7 @@ def test_acquisition_engine_upload(data_builder, file_form, as_root): assert a_timestamp == m_timestamp for mf in metadata['acquisition']['files']: - f = find_file_in_array(mf['name'].replace(':', '/'), a['files']) + f = find_file_in_array(mf['name'], a['files']) assert mf is not None assert f['type'] == mf['type'] assert f['info'] == mf['info'] @@ -779,7 +779,7 @@ def test_session_engine_upload(data_builder, file_form, as_root): 'info': {'test': 'f1'} }, { - 'name': 'folder:three.csv', + 'name': 'folder/three.csv', 'type': 'engine type 2', 'info': {'test': 'f2'} } @@ -789,7 +789,7 @@ def test_session_engine_upload(data_builder, file_form, as_root): r = as_root.post('/engine', params={'level': 'session', 'id': session}, - files=file_form('one.csv', 'two.csv', 'folder:three.csv', meta=metadata) + files=file_form('one.csv', 'two.csv', 'three.csv', meta=metadata) ) assert r.ok @@ -812,11 +812,7 @@ def test_session_engine_upload(data_builder, file_form, as_root): assert s_timestamp == m_timestamp for f in s['files']: - if '/' in f['name']: - assert f['name'] == 'folder/three.csv' - mf = find_file_in_array(f['name'].replace('/', ':'), metadata['session']['files']) - else: - mf = find_file_in_array(f['name'], metadata['session']['files']) + mf = find_file_in_array(f['name'], metadata['session']['files']) assert mf is not None assert f['type'] == mf['type'] assert f['info'] == mf['info'] @@ -840,7 +836,7 @@ def test_project_engine_upload(data_builder, file_form, as_root): 'info': {'test': 'f1'} }, { - 'name': 'folder:three.csv', + 'name': 'folder/three.csv', 'type': 'engine type 2', 'info': {'test': 'f2'} } @@ -850,7 +846,7 @@ def test_project_engine_upload(data_builder, file_form, as_root): r = as_root.post('/engine', params={'level': 'project', 'id': project}, - files=file_form('one.csv', 'two.csv', 'folder:three.csv', meta=metadata) + files=file_form('one.csv', 'two.csv', 'three.csv', meta=metadata) ) assert r.ok @@ -862,11 +858,7 @@ def test_project_engine_upload(data_builder, file_form, as_root): assert p['info'] == metadata['project']['info'] for f in p['files']: - if '/' in f['name']: - assert f['name'] == 'folder/three.csv' - mf = find_file_in_array(f['name'].replace('/', ':'), metadata['project']['files']) - else: - mf = find_file_in_array(f['name'], metadata['project']['files']) + mf = find_file_in_array(f['name'], metadata['project']['files']) assert mf is not None assert f['type'] == mf['type'] assert f['info'] == mf['info'] @@ -874,8 +866,7 @@ def test_project_engine_upload(data_builder, file_form, as_root): def test_acquisition_file_only_engine_upload(data_builder, file_form, as_root): acquisition = data_builder.create_acquisition() - file_names = ['one.csv', 'folder:two.csv'] - expected_file_names = ['one.csv', 'folder/two.csv'] + file_names = ['one.csv', 'two.csv'] r = as_root.post('/engine', params={'level': 'acquisition', 'id': acquisition}, @@ -885,7 +876,7 @@ def test_acquisition_file_only_engine_upload(data_builder, file_form, as_root): r = as_root.get('/acquisitions/' + acquisition) assert r.ok - assert set(f['name'] for f in r.json()['files']) == set(expected_file_names) + assert set(f['name'] for f in r.json()['files']) == set(file_names) def test_acquisition_subsequent_file_engine_upload(data_builder, file_form, as_root): From 6342cc483f06de66fbbfe4d9eb6321dde666b5cd Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Mon, 19 Mar 2018 13:38:21 -0500 Subject: [PATCH 3/4] One to one matching of metadata to file form --- api/files.py | 5 ++++- api/placer.py | 3 +-- tests/integration_tests/python/test_uploads.py | 14 +++++++++----- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/api/files.py b/api/files.py index 5b2bfaeee..b438e7613 100644 --- a/api/files.py +++ b/api/files.py @@ -122,10 +122,13 @@ class SingleFileFieldStorage(cgi.FieldStorage): def make_file(self, binary=None): # Sanitize form's filename (read: prevent malicious escapes, bad characters, etc) - self.filename = fs.path.basename(self.filename) self.hasher = hashlib.new(DEFAULT_HASH_ALG) if not isinstance(self.filename, unicode): self.filename = six.u(self.filename) + + # If the filepath doesn't exist, make it + if not file_system.exists(fs.path.dirname(self.filename)) and self.filename: + file_system.makedirs(fs.path.dirname(self.filename)) self.open_file = file_system.open(self.filename, 'wb') return self.open_file diff --git a/api/placer.py b/api/placer.py index 5f2068f4e..2e1215092 100644 --- a/api/placer.py +++ b/api/placer.py @@ -2,7 +2,6 @@ import copy import datetime import dateutil -import os import pymongo import uuid import zipfile @@ -290,7 +289,7 @@ def process_file_field(self, field, file_attrs): file_mds = self.metadata.get(self.container_type, {}).get('files', []) for file_md in file_mds: - if os.path.basename(file_md['name']) == file_attrs['name']: + if file_md['name'] == file_attrs['name']: file_attrs.update(file_md) break diff --git a/tests/integration_tests/python/test_uploads.py b/tests/integration_tests/python/test_uploads.py index 3acaf0ac1..ad001f17a 100644 --- a/tests/integration_tests/python/test_uploads.py +++ b/tests/integration_tests/python/test_uploads.py @@ -708,7 +708,12 @@ def test_acquisition_engine_upload(data_builder, file_form, as_root): 'info': {'test': 'f0'} }, { - 'name': 'folder/two.csv', + 'name': 'folderA/two.csv', + 'type': 'engine type 1', + 'info': {'test': 'f1'} + }, + { + 'name': 'folderB/two.csv', 'type': 'engine type 1', 'info': {'test': 'f1'} } @@ -717,7 +722,7 @@ def test_acquisition_engine_upload(data_builder, file_form, as_root): # engine upload r = as_root.post('/engine', params={'level': 'acquisition', 'id': acquisition, 'job': job}, - files=file_form('one.csv', 'two.csv', meta=metadata) + files=file_form('one.csv', 'folderA/two.csv', 'folderB/two.csv', meta=metadata) ) assert r.ok @@ -752,7 +757,6 @@ def test_acquisition_engine_upload(data_builder, file_form, as_root): assert f['type'] == mf['type'] assert f['info'] == mf['info'] - def test_session_engine_upload(data_builder, file_form, as_root): project = data_builder.create_project() session = data_builder.create_session() @@ -789,7 +793,7 @@ def test_session_engine_upload(data_builder, file_form, as_root): r = as_root.post('/engine', params={'level': 'session', 'id': session}, - files=file_form('one.csv', 'two.csv', 'three.csv', meta=metadata) + files=file_form('one.csv', 'two.csv', 'folder/three.csv', meta=metadata) ) assert r.ok @@ -846,7 +850,7 @@ def test_project_engine_upload(data_builder, file_form, as_root): r = as_root.post('/engine', params={'level': 'project', 'id': project}, - files=file_form('one.csv', 'two.csv', 'three.csv', meta=metadata) + files=file_form('one.csv', 'two.csv', 'folder/three.csv', meta=metadata) ) assert r.ok From 6bdfcb4ff257b76c49fa0fdc8a79f86595acf90d Mon Sep 17 00:00:00 2001 From: Harsha Kethineni Date: Mon, 19 Mar 2018 16:16:37 -0500 Subject: [PATCH 4/4] route file download and info to contain slashes --- api/api.py | 6 +-- api/handlers/listhandler.py | 2 +- .../python/test_access_log.py | 2 +- .../python/test_containers.py | 40 +++++++++---------- 4 files changed, 25 insertions(+), 25 deletions(-) diff --git a/api/api.py b/api/api.py index db7a9dc70..31db2048e 100644 --- a/api/api.py +++ b/api/api.py @@ -41,7 +41,7 @@ 'tag': '[^/]{1,32}', # Filename - 'fname': '[^/]+', + 'fname': '.+', # Note ID 'nid': '[0-9a-f]{24}', @@ -272,9 +272,9 @@ def prefix(path, routes): route('/packfile', FileListHandler, h='packfile', m=['POST']), route('/packfile-end', FileListHandler, h='packfile_end'), route('/', FileListHandler, m=['POST']), + route('//info/', FileListHandler, h='get_info', m=['GET']), + route('//info/', FileListHandler, h='modify_info', 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( '//analyses', AnalysesHandler, h='get_all', m=['GET']), route( '/analyses', AnalysesHandler, h='get_all', m=['GET']), diff --git a/api/handlers/listhandler.py b/api/handlers/listhandler.py index 54d36da41..d6f531499 100644 --- a/api/handlers/listhandler.py +++ b/api/handlers/listhandler.py @@ -467,7 +467,7 @@ def get(self, cont_name, list_name, **kwargs): self.response.headers['Content-Type'] = str(fileinfo.get('mimetype', 'application/octet-stream')) else: self.response.headers['Content-Type'] = 'application/octet-stream' - self.response.headers['Content-Disposition'] = 'attachment; filename="' + filename + '"' + self.response.headers['Content-Disposition'] = 'attachment; filename="' + os.path.basename(filename) + '"' else: self.response.status = 206 if len(ranges) > 1: diff --git a/tests/integration_tests/python/test_access_log.py b/tests/integration_tests/python/test_access_log.py index b690b1bae..31a538c9d 100644 --- a/tests/integration_tests/python/test_access_log.py +++ b/tests/integration_tests/python/test_access_log.py @@ -256,7 +256,7 @@ def test_access_log_succeeds(data_builder, as_admin, log_db): log_records_count_before = log_db.access_log.count({}) - r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + r = as_admin.get('/projects/' + project + '/files/info/' + file_name) assert r.ok assert r.json()['name'] == file_name diff --git a/tests/integration_tests/python/test_containers.py b/tests/integration_tests/python/test_containers.py index dc11cfda1..00bd3d92f 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): assert as_admin.put('/projects/' + project + '/files/' + file_name, json=payload).ok - r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + r = as_admin.get('/projects/' + project + '/files/info/' + file_name) assert r.ok file_object = r.json() @@ -786,33 +786,33 @@ def test_edit_file_info(data_builder, as_admin, file_form): # Assert getting file info 404s properly - r = as_admin.get('/projects/' + project + '/files/' + 'not_real.txt' + '/info') + r = as_admin.get('/projects/' + project + '/files/info/' + 'not_real.txt') assert r.status_code == 404 - r = as_admin.get('/projects/' + '000000000000000000000000' + '/files/' + 'not_real.txt' + '/info') + r = as_admin.get('/projects/' + '000000000000000000000000' + '/files/info/' + 'not_real.txt') assert r.status_code == 404 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') + r = as_admin.get('/projects/' + project + '/files/info/' + file_name) assert r.ok assert r.json()['info'] == {} # Send improper payload - r = as_admin.post('/projects/' + project + '/files/' + file_name + '/info', json={ + r = as_admin.post('/projects/' + project + '/files/info/' + file_name, json={ 'delete': ['map'], 'replace': {'not_going': 'to_happen'} }) assert r.status_code == 400 # Send improper payload - r = as_admin.post('/projects/' + project + '/files/' + file_name + '/info', json={ + r = as_admin.post('/projects/' + project + '/files/info/' + file_name, json={ 'delete': {'a': 'map'}, }) assert r.status_code == 400 # Send improper payload - r = as_admin.post('/projects/' + project + '/files/' + file_name + '/info', json={ + r = as_admin.post('/projects/' + project + '/files/info/' + file_name, json={ 'set': 'cannot do this', }) assert r.status_code == 400 @@ -828,60 +828,60 @@ def test_edit_file_info(data_builder, as_admin, file_form): } - r = as_admin.post('/projects/' + project + '/files/' + file_name + '/info', json={ + r = as_admin.post('/projects/' + project + '/files/info/' + file_name, json={ 'replace': file_info }) assert r.ok - r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + r = as_admin.get('/projects/' + project + '/files/info/' + file_name) assert r.ok assert r.json()['info'] == file_info # Use 'set' to add new key - r = as_admin.post('/projects/' + project + '/files/' + file_name + '/info', json={ + r = as_admin.post('/projects/' + project + '/files/info/' + file_name, json={ 'set': {'map': 'no longer a map'} }) assert r.ok file_info['map'] = 'no longer a map' - r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + r = as_admin.get('/projects/' + project + '/files/info/' + file_name) assert r.ok assert r.json()['info'] == file_info # Use 'set' to do full replace of "map" key - r = as_admin.post('/projects/' + project + '/files/' + file_name + '/info', json={ + r = as_admin.post('/projects/' + project + '/files/info/' + file_name, json={ 'set': {'map': 'no longer a map'} }) assert r.ok file_info['map'] = 'no longer a map' - r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + r = as_admin.get('/projects/' + project + '/files/info/' + file_name) assert r.ok assert r.json()['info'] == file_info # Use 'delete' to unset "map" key - r = as_admin.post('/projects/' + project + '/files/' + file_name + '/info', json={ + r = as_admin.post('/projects/' + project + '/files/info/' + file_name, json={ 'delete': ['map', 'a'] }) assert r.ok file_info.pop('map') file_info.pop('a') - r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + r = as_admin.get('/projects/' + project + '/files/info/' + file_name) assert r.ok assert r.json()['info'] == file_info # Use 'delete' on keys that do not exist - r = as_admin.post('/projects/' + project + '/files/' + file_name + '/info', json={ + r = as_admin.post('/projects/' + project + '/files/info/' + file_name, json={ 'delete': ['madeup', 'keys'] }) assert r.ok - r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + r = as_admin.get('/projects/' + project + '/files/info/' + file_name) assert r.ok assert r.json()['info'] == file_info @@ -895,7 +895,7 @@ def test_edit_file_info(data_builder, as_admin, file_form): # Add reserved key and ensure it is returned BIDS_map = {'BIDS':{'project_label': 'TEST'}} - r = as_admin.post('/projects/' + project + '/files/' + file_name + '/info', json={ + r = as_admin.post('/projects/' + project + '/files/info/' + file_name, json={ 'set': BIDS_map }) assert r.ok @@ -909,12 +909,12 @@ def test_edit_file_info(data_builder, as_admin, file_form): # Use 'replace' to set file info to {} - r = as_admin.post('/projects/' + project + '/files/' + file_name + '/info', json={ + r = as_admin.post('/projects/' + project + '/files/info/' + file_name, json={ 'replace': {} }) assert r.ok - r = as_admin.get('/projects/' + project + '/files/' + file_name + '/info') + r = as_admin.get('/projects/' + project + '/files/info/' + file_name) assert r.ok assert r.json()['info'] == {}