diff --git a/packages/openneuro-server/src/datalad/dataset.ts b/packages/openneuro-server/src/datalad/dataset.ts index 61de4be26..bdd5ebdc6 100644 --- a/packages/openneuro-server/src/datalad/dataset.ts +++ b/packages/openneuro-server/src/datalad/dataset.ts @@ -377,6 +377,8 @@ export const deleteFiles = (datasetId, files, user) => { .del(filesUrl(datasetId)) .set("Cookie", generateDataladCookie(config)(user)) .set("Accept", "application/json") + // Fail with an error instead of hanging indefinitely if a worker stalls + .timeout({ response: 60000, deadline: 300000 }) .send({ filenames }) .then(() => filenames) } diff --git a/services/datalad/datalad_service/tasks/files.py b/services/datalad/datalad_service/tasks/files.py index 9599d4113..7337996d5 100644 --- a/services/datalad/datalad_service/tasks/files.py +++ b/services/datalad/datalad_service/tasks/files.py @@ -1,5 +1,7 @@ import json import logging +import os +import shutil import subprocess from urllib.parse import urlparse, parse_qs @@ -13,6 +15,37 @@ from datalad_service.config import AWS_S3_PUBLIC_BUCKET +def target_path(dataset_root, path): + """Resolve a dataset relative path, rejecting anything outside of the dataset.""" + # Normalize without resolving symlinks, annexed files are symlinks into .git/annex + target = os.path.normpath(os.path.join(dataset_root, path)) + if target == dataset_root or not target.startswith(dataset_root + os.sep): + raise ValueError(f'"{path}" is not a path within this dataset') + if os.path.relpath(target, dataset_root).split(os.sep)[0] == '.git': + raise ValueError(f'"{path}" is a git internal path and cannot be removed') + return target + + +def remove_from_worktree(dataset_root, target): + """Delete one file or directory and any parent directories it leaves empty.""" + try: + if os.path.isdir(target) and not os.path.islink(target): + shutil.rmtree(target) + else: + os.remove(target) + except FileNotFoundError: + # Already gone, the index update below is all that is needed + pass + # Git does not track directories, so drop any that are now empty + parent = os.path.dirname(target) + while parent != dataset_root: + try: + os.rmdir(parent) + except OSError: + break + parent = os.path.dirname(parent) + + async def remove_files(store, dataset, paths, name=None, email=None, cookies=None): dataset_path = store.get_dataset_path(dataset) repo = pygit2.Repository(dataset_path) @@ -20,9 +53,17 @@ async def remove_files(store, dataset, paths, name=None, email=None, cookies=Non author = pygit2.Signature(name, email) else: author = None + dataset_root = os.path.realpath(dataset_path) + # Validate every path before changing anything + targets = [target_path(dataset_root, path) for path in paths] repo.index.remove_all(paths) repo.index.write() - repo.checkout_index() + # Remove the requested paths from the working tree directly. repo.checkout_index() + # would do this too, but it diffs the entire index against the entire working tree, + # making any delete cost time proportional to the size of the dataset instead of to + # the number of paths being deleted. + for target in targets: + remove_from_worktree(dataset_root, target) await git_commit_index(repo, author, message='[OpenNeuro] Files removed') diff --git a/services/datalad/tests/test_files.py b/services/datalad/tests/test_files.py index c2eac753e..197db0870 100644 --- a/services/datalad/tests/test_files.py +++ b/services/datalad/tests/test_files.py @@ -2,9 +2,11 @@ import falcon from falcon import testing import json +import pygit2 +import pytest from datalad.api import Dataset -from datalad_service.tasks.files import parse_s3_annex_url +from datalad_service.tasks.files import parse_s3_annex_url, target_path class FileWrapper: @@ -372,6 +374,112 @@ def test_delete_nested_file(client, new_dataset): assert json.loads(response.content)['deleted'] == ['derivatives/LICENSE', 'CHANGES'] +def test_delete_file_removes_from_working_tree(client, new_dataset): + ds_id = os.path.basename(new_dataset.path) + response = client.simulate_delete( + f'/datasets/{ds_id}/files', body='{ "filenames": ["CHANGES"] }' + ) + assert response.status == falcon.HTTP_OK + # Removed from the working tree and the index, not just the commit + assert not os.path.exists(os.path.join(new_dataset.path, 'CHANGES')) + repo = pygit2.Repository(new_dataset.path) + index_paths = [entry.path for entry in repo.index] + assert 'CHANGES' not in index_paths + # Untouched files are still present in both + assert os.path.exists(os.path.join(new_dataset.path, 'dataset_description.json')) + assert 'dataset_description.json' in index_paths + + +def test_delete_directory(client, new_dataset): + ds_id = os.path.basename(new_dataset.path) + for filename in ('derivatives:LICENSE', 'derivatives:sub-01:stats.tsv'): + response = client.simulate_post( + f'/datasets/{ds_id}/files/{filename}', body='test content' + ) + assert response.status == falcon.HTTP_OK + response = client.simulate_post( + f'/datasets/{ds_id}/draft', params={'validate': 'false'} + ) + assert response.status == falcon.HTTP_OK + response = client.simulate_delete( + f'/datasets/{ds_id}/files', body='{ "filenames": ["derivatives"] }' + ) + assert response.status == falcon.HTTP_OK + assert json.loads(response.content)['deleted'] == ['derivatives'] + assert not os.path.exists(os.path.join(new_dataset.path, 'derivatives')) + repo = pygit2.Repository(new_dataset.path) + assert not [ + entry.path for entry in repo.index if entry.path.startswith('derivatives/') + ] + + +def test_delete_prunes_emptied_directories(client, new_dataset): + """Deleting the last file in a directory should not leave the directory behind.""" + ds_id = os.path.basename(new_dataset.path) + response = client.simulate_post( + f'/datasets/{ds_id}/files/sub-01:anat:sub-01_T1w.json', body='{}' + ) + assert response.status == falcon.HTTP_OK + response = client.simulate_post( + f'/datasets/{ds_id}/draft', params={'validate': 'false'} + ) + assert response.status == falcon.HTTP_OK + response = client.simulate_delete( + f'/datasets/{ds_id}/files', + body='{ "filenames": ["sub-01:anat:sub-01_T1w.json"] }', + ) + assert response.status == falcon.HTTP_OK + assert not os.path.exists(os.path.join(new_dataset.path, 'sub-01')) + + +def test_delete_leaves_untracked_files_alone(client, new_dataset): + """Only the requested paths are removed, unrelated untracked files survive.""" + ds_id = os.path.basename(new_dataset.path) + untracked = os.path.join(new_dataset.path, 'untracked.txt') + with open(untracked, 'w') as f: + f.write('not committed') + response = client.simulate_delete( + f'/datasets/{ds_id}/files', body='{ "filenames": ["CHANGES"] }' + ) + assert response.status == falcon.HTTP_OK + assert os.path.exists(untracked) + + +def test_delete_does_not_rewrite_unrequested_paths(client, new_dataset): + """A delete only touches the paths it was asked to delete. + + A full checkout of the index would also restore any other tracked file missing from + the working tree, so an unrelated delete could rewrite arbitrary amounts of a large + dataset as a side effect. + """ + ds_id = os.path.basename(new_dataset.path) + missing = os.path.join(new_dataset.path, 'dataset_description.json') + os.remove(missing) + response = client.simulate_delete( + f'/datasets/{ds_id}/files', body='{ "filenames": ["CHANGES"] }' + ) + assert response.status == falcon.HTTP_OK + assert not os.path.exists(os.path.join(new_dataset.path, 'CHANGES')) + assert not os.path.exists(missing) + + +def test_target_path_rejects_paths_outside_dataset(tmp_path): + dataset_root = str(tmp_path) + with pytest.raises(ValueError): + target_path(dataset_root, '../outside.txt') + with pytest.raises(ValueError): + target_path(dataset_root, 'sub-01/../../outside.txt') + with pytest.raises(ValueError): + target_path(dataset_root, '/etc/passwd') + with pytest.raises(ValueError): + target_path(dataset_root, '.') + with pytest.raises(ValueError): + target_path(dataset_root, '.git/config') + assert target_path(dataset_root, 'sub-01/anat/sub-01_T1w.nii.gz') == os.path.join( + dataset_root, 'sub-01/anat/sub-01_T1w.nii.gz' + ) + + def test_delete_non_existing_file(client, new_dataset): ds_id = os.path.basename(new_dataset.path) response = client.simulate_delete(