From 8f6f2942c0910a8a393a447f2f68d61eb9d4d1af Mon Sep 17 00:00:00 2001 From: Johan Dahlberg Date: Tue, 10 Jan 2017 16:10:52 +0100 Subject: [PATCH 1/7] Remove output dir before starting bcl2fastq --- bcl2fastq/handlers/bcl2fastq_handlers.py | 2 ++ bcl2fastq/lib/bcl2fastq_utils.py | 10 ++++++++++ tests/test_bcl2fastq_handlers.py | 18 +++++++++++++----- tests/test_utils.py | 10 ++++++++-- 4 files changed, 33 insertions(+), 7 deletions(-) diff --git a/bcl2fastq/handlers/bcl2fastq_handlers.py b/bcl2fastq/handlers/bcl2fastq_handlers.py index 574eddc..787d1ea 100644 --- a/bcl2fastq/handlers/bcl2fastq_handlers.py +++ b/bcl2fastq/handlers/bcl2fastq_handlers.py @@ -174,6 +174,8 @@ def post(self, runfolder): create_bcl2fastq_runner(runfolder_config) bcl2fastq_version = job_runner.version() cmd = job_runner.construct_command() + # If the output directory exists, we always want to clear it. + job_runner.delete_output() job_runner.symlink_output_to_unaligned() log_file = self.bcl2fastq_log_file_provider.log_file_path(runfolder) diff --git a/bcl2fastq/lib/bcl2fastq_utils.py b/bcl2fastq/lib/bcl2fastq_utils.py index 1d74fc4..3bb7c85 100644 --- a/bcl2fastq/lib/bcl2fastq_utils.py +++ b/bcl2fastq/lib/bcl2fastq_utils.py @@ -297,6 +297,16 @@ def construct_command(self): """ raise NotImplementedError("Subclasses should implement this!") + def delete_output(self): + """ + Delete the output directory if it exists + :return: None + """ + if os.path.isdir(self.config.output): + print "Found a directory at output path {}, will remove it.".format(self.config.output) + log.info("Found a directory at output path {}, will remove it.".format(self.config.output)) + shutil.rmtree(self.config.output) + def symlink_output_to_unaligned(self): """ Create a symlink from `runfolder/Unaligned` to what has been defined as the output directory. diff --git a/tests/test_bcl2fastq_handlers.py b/tests/test_bcl2fastq_handlers.py index a1367e8..bdc59c6 100644 --- a/tests/test_bcl2fastq_handlers.py +++ b/tests/test_bcl2fastq_handlers.py @@ -2,8 +2,8 @@ from tornado.escape import json_encode import mock -from test_utils import TestUtils, DummyConfig - +from test_utils import TestUtils, DummyConfig, DummyRunnerConfig +import shutil from bcl2fastq.handlers.bcl2fastq_handlers import * from bcl2fastq.lib.bcl2fastq_utils import BCL2Fastq2xRunner, BCL2FastqRunner @@ -32,6 +32,8 @@ def start_api_call_nbr(self): 8: "y*,7i,n*,y*", } + DUMMY_RUNNER_CONF = DummyRunnerConfig(output='/foo/bar') + def get_app(self): dummy_config = DummyConfig() return Application(routes(config=dummy_config)) @@ -49,8 +51,10 @@ def test_start(self): # Use mock to ensure that this will run without # creating the runfolder. with mock.patch.object(os.path, 'isdir', return_value=True), \ + mock.patch.object(shutil, 'rmtree', return_value=None), \ mock.patch.object(Bcl2FastqConfig, 'get_bcl2fastq_version_from_run_parameters', return_value="2.15.2"), \ - mock.patch.object(BCL2FastqRunnerFactory, "create_bcl2fastq_runner", return_value=FakeRunner("2.15.2")), \ + mock.patch.object(BCL2FastqRunnerFactory, "create_bcl2fastq_runner", + return_value=FakeRunner("2.15.2", self.DUMMY_RUNNER_CONF)), \ mock.patch.object(BCL2FastqRunner, 'symlink_output_to_unaligned', return_value=None): body = {"runfolder_input": "/path/to/runfolder"} @@ -70,8 +74,10 @@ def test_start_with_empty_body(self): # Use mock to ensure that this will run without # creating the runfolder. with mock.patch.object(os.path, 'isdir', return_value=True), \ + mock.patch.object(shutil, 'rmtree', return_value=None), \ mock.patch.object(Bcl2FastqConfig, 'get_bcl2fastq_version_from_run_parameters', return_value="2.15.2"), \ - mock.patch.object(BCL2FastqRunnerFactory, "create_bcl2fastq_runner", return_value=FakeRunner("2.15.2")), \ + mock.patch.object(BCL2FastqRunnerFactory, "create_bcl2fastq_runner", + return_value=FakeRunner("2.15.2", self.DUMMY_RUNNER_CONF)), \ mock.patch.object(BCL2FastqRunner, 'symlink_output_to_unaligned', return_value=None): response = self.fetch(self.API_BASE + "/start/150415_D00457_0091_AC6281ANXX", method="POST", body="") @@ -89,10 +95,12 @@ def test_start_providing_samplesheet(self): # Use mock to ensure that this will run without # creating the runfolder. with mock.patch.object(os.path, 'isdir', return_value=True), \ + mock.patch.object(shutil, 'rmtree', return_value=None), \ mock.patch.object(Bcl2FastqConfig, 'get_bcl2fastq_version_from_run_parameters', return_value="2.15.2"), \ mock.patch.object(BCL2FastqRunner, 'symlink_output_to_unaligned', return_value=None), \ mock.patch.object(Bcl2FastqConfig, "write_samplesheet") as ws , \ - mock.patch.object(BCL2FastqRunnerFactory, "create_bcl2fastq_runner", return_value=FakeRunner("2.15.2")): + mock.patch.object(BCL2FastqRunnerFactory, "create_bcl2fastq_runner", + return_value=FakeRunner("2.15.2", self.DUMMY_RUNNER_CONF)): body = {"runfolder_input": "/path/to/runfolder", "samplesheet": TestUtils.DUMMY_SAMPLESHEET_STRING} diff --git a/tests/test_utils.py b/tests/test_utils.py index a5a5c2a..2ede2e1 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1,5 +1,5 @@ -from bcl2fastq.lib.bcl2fastq_utils import BCL2FastqRunner +from bcl2fastq.lib.bcl2fastq_utils import BCL2FastqRunner, Bcl2FastqConfig class TestUtils: @@ -57,9 +57,15 @@ class DummyConfig: def __getitem__(self, key): return TestUtils.DUMMY_CONFIG[key] +class DummyRunnerConfig(Bcl2FastqConfig): + def __init__(self, output): + self.output = output + + class FakeRunner(BCL2FastqRunner): - def __init__(self, dummy_version): + def __init__(self, dummy_version, config): self.dummy_version = dummy_version + self.config = config def version(self): return str(self.dummy_version) From f1c051ddb1d958bd4fa98e1c9c30ef67d72f74ef Mon Sep 17 00:00:00 2001 From: Johan Dahlberg Date: Tue, 10 Jan 2017 16:14:29 +0100 Subject: [PATCH 2/7] Remove forgotten print statment --- bcl2fastq/lib/bcl2fastq_utils.py | 1 - 1 file changed, 1 deletion(-) diff --git a/bcl2fastq/lib/bcl2fastq_utils.py b/bcl2fastq/lib/bcl2fastq_utils.py index 3bb7c85..df1d088 100644 --- a/bcl2fastq/lib/bcl2fastq_utils.py +++ b/bcl2fastq/lib/bcl2fastq_utils.py @@ -303,7 +303,6 @@ def delete_output(self): :return: None """ if os.path.isdir(self.config.output): - print "Found a directory at output path {}, will remove it.".format(self.config.output) log.info("Found a directory at output path {}, will remove it.".format(self.config.output)) shutil.rmtree(self.config.output) From 10616ec51c063436d8b918f6193cfe373b6ee2b4 Mon Sep 17 00:00:00 2001 From: Johan Dahlberg Date: Tue, 10 Jan 2017 16:30:55 +0100 Subject: [PATCH 3/7] bump version to 1.1.2 --- bcl2fastq/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bcl2fastq/__init__.py b/bcl2fastq/__init__.py index a82b376..72f26f5 100644 --- a/bcl2fastq/__init__.py +++ b/bcl2fastq/__init__.py @@ -1 +1 @@ -__version__ = "1.1.1" +__version__ = "1.1.2" From 50812cf78413bbb324835d77c28485bc8073dbe5 Mon Sep 17 00:00:00 2001 From: Johan Dahlberg Date: Mon, 16 Jan 2017 15:39:32 +0100 Subject: [PATCH 4/7] Only allow arbitrary path if it's ok by config --- bcl2fastq/handlers/bcl2fastq_handlers.py | 6 +++- config/app.config | 7 +++++ tests/test_bcl2fastq_handlers.py | 38 ++++++++++++++++++++-- tests/test_utils.py | 40 +++++++++++++----------- tests/tests_bcl2fastq_utils.py | 11 ++++--- 5 files changed, 76 insertions(+), 26 deletions(-) diff --git a/bcl2fastq/handlers/bcl2fastq_handlers.py b/bcl2fastq/handlers/bcl2fastq_handlers.py index 787d1ea..4165441 100644 --- a/bcl2fastq/handlers/bcl2fastq_handlers.py +++ b/bcl2fastq/handlers/bcl2fastq_handlers.py @@ -119,7 +119,11 @@ def create_config_from_request(self, runfolder, request_body): bcl2fastq_version = request_data["bcl2fastq_version"] if "output" in request_data: - output = request_data["output"] + if self.config['allow_arbitrary_output_folder']: + output = request_data["output"] + else: + raise ArteriaUsageException("A output folder was specified by the request, but the configuration " + "does not allow arbitrary output folder to be specified.") if "samplesheet" in request_data: samplesheet = request_data["samplesheet"] diff --git a/config/app.config b/config/app.config index 42fca21..2de35ec 100644 --- a/config/app.config +++ b/config/app.config @@ -34,3 +34,10 @@ runfolder_path: /vagrant/tiny-test-data/ default_output_path: /vagrant/runfolder_output/ bcl2fastq_logs_path: bcl2fastq_logs + +# Setting this to True will allow the bcl2fastq service direct +# the output of bcl2fastq anywhere on the filesystem (restricted +# by normal permissions). If this is set to False (as per default) +# it will only write output to the specified `default_output_path` +allow_arbitrary_output_folder: False + diff --git a/tests/test_bcl2fastq_handlers.py b/tests/test_bcl2fastq_handlers.py index bdc59c6..eb38ba3 100644 --- a/tests/test_bcl2fastq_handlers.py +++ b/tests/test_bcl2fastq_handlers.py @@ -34,9 +34,10 @@ def start_api_call_nbr(self): DUMMY_RUNNER_CONF = DummyRunnerConfig(output='/foo/bar') + dummy_config = DummyConfig() + def get_app(self): - dummy_config = DummyConfig() - return Application(routes(config=dummy_config)) + return Application(routes(config=self.dummy_config)) def test_versions(self): response = self.fetch(self.API_BASE + "/versions") @@ -91,6 +92,39 @@ def test_start_with_empty_body(self): self.assertEqual(json.loads(response.body)["bcl2fastq_version"], "2.15.2") self.assertEqual(json.loads(response.body)["state"], "started") + def test_start_with_disallowed_output_specified(self): + + self.dummy_config.DUMMY_CONFIG['allow_arbitrary_output_folder'] = False + + with mock.patch.object(os.path, 'isdir', return_value=True): + body = {'output': '/foo/bar'} + response = self.fetch(self.API_BASE + "/start/150415_D00457_0091_AC6281ANXX", + method="POST", + body=json_encode(body)) + self.assertEqual(response.code, 500) + self.assertEqual(response.reason, "A output folder was specified by the request, " + "but the configuration does " + "not allow arbitrary output folder to be specified.") + + def test_start_with_allowed_output_specified(self): + + self.dummy_config.DUMMY_CONFIG['allow_arbitrary_output_folder'] = True + + with mock.patch.object(os.path, 'isdir', return_value=True), \ + mock.patch.object(shutil, 'rmtree', return_value=None), \ + mock.patch.object(Bcl2FastqConfig, 'get_bcl2fastq_version_from_run_parameters', return_value="2.15.2"), \ + mock.patch.object(BCL2FastqRunnerFactory, "create_bcl2fastq_runner", + return_value=FakeRunner("2.15.2", self.DUMMY_RUNNER_CONF)), \ + mock.patch.object(BCL2FastqRunner, 'symlink_output_to_unaligned', return_value=None): + body = {'output': '/foo/bar'} + response = self.fetch(self.API_BASE + "/start/150415_D00457_0091_AC6281ANXX", + method="POST", + body=json_encode(body)) + + # Just increment it here so it doesn't break the other tests + self.start_api_call_nbr() + self.assertEqual(response.code, 202) + def test_start_providing_samplesheet(self): # Use mock to ensure that this will run without # creating the runfolder. diff --git a/tests/test_utils.py b/tests/test_utils.py index 2ede2e1..40d5750 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -3,24 +3,6 @@ class TestUtils: - DUMMY_CONFIG = { "runfolder_path": "/data/biotank3/runfolders", - "default_output_path": "test", - "bcl2fastq": - {"versions": - {"2.15.2": - {"class_creation_function": "_get_bcl2fastq2x_runner", - "binary": "/path/to/bcl2fastq"}, - "1.8.4": - {"class_creation_function": "_get_bcl2fastq1x_runner", - "binary": "/path/to/bcl2fastq"}}}, - "machine_type": - {"MiSeq": {"bcl2fastq_version": "1.8.4"}, - "HiSeq X": {"bcl2fastq_version": "2.15.2"}, - "HiSeq 2500": {"bcl2fastq_version": "1.8.4"}, - "HiSeq 4000": {"bcl2fastq_version": "1.8.4"}, - "HiSeq 2000": {"bcl2fastq_version": "1.8.4"}, - "NextSeq 500": {"bcl2fastq_version": "1.8.4"}}, - "bcl2fastq_logs_path": "/tmp/"} DUMMY_SAMPLESHEET_STRING = """[Header],,,,,,,,,,, IEMFileVersion,4,,,,,,,,,, @@ -54,8 +36,28 @@ class TestUtils: class DummyConfig: + DUMMY_CONFIG = { "runfolder_path": "/data/biotank3/runfolders", + "default_output_path": "test", + "bcl2fastq": + {"versions": + {"2.15.2": + {"class_creation_function": "_get_bcl2fastq2x_runner", + "binary": "/path/to/bcl2fastq"}, + "1.8.4": + {"class_creation_function": "_get_bcl2fastq1x_runner", + "binary": "/path/to/bcl2fastq"}}}, + "machine_type": + {"MiSeq": {"bcl2fastq_version": "1.8.4"}, + "HiSeq X": {"bcl2fastq_version": "2.15.2"}, + "HiSeq 2500": {"bcl2fastq_version": "1.8.4"}, + "HiSeq 4000": {"bcl2fastq_version": "1.8.4"}, + "HiSeq 2000": {"bcl2fastq_version": "1.8.4"}, + "NextSeq 500": {"bcl2fastq_version": "1.8.4"}}, + "bcl2fastq_logs_path": "/tmp/", + "allow_arbitrary_output_folder": False} + def __getitem__(self, key): - return TestUtils.DUMMY_CONFIG[key] + return self.DUMMY_CONFIG[key] class DummyRunnerConfig(Bcl2FastqConfig): def __init__(self, output): diff --git a/tests/tests_bcl2fastq_utils.py b/tests/tests_bcl2fastq_utils.py index aed3197..b10819e 100644 --- a/tests/tests_bcl2fastq_utils.py +++ b/tests/tests_bcl2fastq_utils.py @@ -15,10 +15,11 @@ class TestBcl2FastqConfig(unittest.TestCase): test_dir = os.path.dirname(os.path.realpath(__file__)) samplesheet_file = test_dir + "/sampledata/new_samplesheet_example.csv" samplesheet_with_no_tag = test_dir + "/sampledata/no_tag_samplesheet_example.csv" + dummy_config = DummyConfig() def test_get_bcl2fastq_version_from_run_parameters(self): runfolder = TestBcl2FastqConfig.test_dir + "/sampledata/HiSeq-samples/2014-02_13_average_run" - version = Bcl2FastqConfig.get_bcl2fastq_version_from_run_parameters(runfolder, TestUtils.DUMMY_CONFIG) + version = Bcl2FastqConfig.get_bcl2fastq_version_from_run_parameters(runfolder, self.dummy_config) self.assertEqual(version, "1.8.4") def test_is_single_read_true(self): @@ -117,6 +118,8 @@ def test_get_bases_mask_per_lane_from_samplesheet_no_tag(self): class TestBCL2FastqRunnerFactory(unittest.TestCase): + dummy_config = DummyConfig() + def test_create_bcl2fastq1x_runner(self): config = Bcl2FastqConfig( general_config = DUMMY_CONFIG, @@ -124,7 +127,7 @@ def test_create_bcl2fastq1x_runner(self): runfolder_input = "test/runfolder", output = "test/output") - factory = BCL2FastqRunnerFactory(TestUtils.DUMMY_CONFIG) + factory = BCL2FastqRunnerFactory(self.dummy_config) runner = factory.create_bcl2fastq_runner(config) self.assertIsInstance(runner, BCL2Fastq1xRunner) @@ -135,7 +138,7 @@ def test_create_bcl2fastq2x_runner(self): runfolder_input = "test/runfolder", output = "test/output") - factory = BCL2FastqRunnerFactory(TestUtils.DUMMY_CONFIG) + factory = BCL2FastqRunnerFactory(self.dummy_config) runner = factory.create_bcl2fastq_runner(config) self.assertIsInstance(runner, BCL2Fastq2xRunner, msg="runner is: " + str(runner)) @@ -146,7 +149,7 @@ def test_create_invalid_version_runner(self): runfolder_input = "test/runfolder", output = "test/output") - factory = BCL2FastqRunnerFactory(TestUtils.DUMMY_CONFIG) + factory = BCL2FastqRunnerFactory(self.dummy_config) with self.assertRaises(LookupError): factory.create_bcl2fastq_runner(config) From b8b1f6c3f6595070bc935bb9f51606df7165f00c Mon Sep 17 00:00:00 2001 From: Johan Dahlberg Date: Mon, 16 Jan 2017 17:43:21 +0100 Subject: [PATCH 5/7] Output path should not be input path --- bcl2fastq/handlers/bcl2fastq_handlers.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/bcl2fastq/handlers/bcl2fastq_handlers.py b/bcl2fastq/handlers/bcl2fastq_handlers.py index 4165441..8f5ab41 100644 --- a/bcl2fastq/handlers/bcl2fastq_handlers.py +++ b/bcl2fastq/handlers/bcl2fastq_handlers.py @@ -125,6 +125,9 @@ def create_config_from_request(self, runfolder, request_body): raise ArteriaUsageException("A output folder was specified by the request, but the configuration " "does not allow arbitrary output folder to be specified.") + if os.path.abspath(output) == os.path.abspath(runfolder_input): + raise ArteriaUsageException("The specified output path is the same as the input path. This is not allowed!") + if "samplesheet" in request_data: samplesheet = request_data["samplesheet"] From 1cc6ad34ac35851f3d10cdb184f5ee6e45a353ff Mon Sep 17 00:00:00 2001 From: Johan Dahlberg Date: Tue, 17 Jan 2017 13:41:41 +0100 Subject: [PATCH 6/7] Restrict output to specific dirs --- bcl2fastq/handlers/bcl2fastq_handlers.py | 9 +------ bcl2fastq/lib/bcl2fastq_utils.py | 23 ++++++++++++++++-- config/app.config | 9 +++---- tests/test_bcl2fastq_handlers.py | 31 +++++++++++++++--------- tests/test_utils.py | 5 ++-- 5 files changed, 49 insertions(+), 28 deletions(-) diff --git a/bcl2fastq/handlers/bcl2fastq_handlers.py b/bcl2fastq/handlers/bcl2fastq_handlers.py index 8f5ab41..787d1ea 100644 --- a/bcl2fastq/handlers/bcl2fastq_handlers.py +++ b/bcl2fastq/handlers/bcl2fastq_handlers.py @@ -119,14 +119,7 @@ def create_config_from_request(self, runfolder, request_body): bcl2fastq_version = request_data["bcl2fastq_version"] if "output" in request_data: - if self.config['allow_arbitrary_output_folder']: - output = request_data["output"] - else: - raise ArteriaUsageException("A output folder was specified by the request, but the configuration " - "does not allow arbitrary output folder to be specified.") - - if os.path.abspath(output) == os.path.abspath(runfolder_input): - raise ArteriaUsageException("The specified output path is the same as the input path. This is not allowed!") + output = request_data["output"] if "samplesheet" in request_data: samplesheet = request_data["samplesheet"] diff --git a/bcl2fastq/lib/bcl2fastq_utils.py b/bcl2fastq/lib/bcl2fastq_utils.py index df1d088..fe0e7f8 100644 --- a/bcl2fastq/lib/bcl2fastq_utils.py +++ b/bcl2fastq/lib/bcl2fastq_utils.py @@ -48,6 +48,8 @@ def __init__(self, :param nbr_of_cores: number of cores to run bcl2fastq with """ + self.general_config = general_config + self.runfolder_input = runfolder_input self.base_calls_input = runfolder_input + "/Data/Intensities/BaseCalls" @@ -297,12 +299,29 @@ def construct_command(self): """ raise NotImplementedError("Subclasses should implement this!") + def validate_output(self): + + def _parent_dir(d): + return os.path.abspath(os.path.join(d, os.path.pardir)) + + abs_path_of_allowed_dirs = map(os.path.abspath, self.config.general_config['allowed_output_folders']) + is_located_in_parent_dir = _parent_dir(self.config.output) in abs_path_of_allowed_dirs + + if is_located_in_parent_dir: + True + else: + error_string = "Invalid output directory {} was specified." \ + " Allowed dirs were: {}".format(self.config.output, + self.config.general_config['allowed_output_folders']) + log.error(error_string) + raise ArteriaUsageException(error_string) + def delete_output(self): """ - Delete the output directory if it exists + Delete the output directory if it exists and the output path is valid :return: None """ - if os.path.isdir(self.config.output): + if self.validate_output(): log.info("Found a directory at output path {}, will remove it.".format(self.config.output)) shutil.rmtree(self.config.output) diff --git a/config/app.config b/config/app.config index 2de35ec..51b0e28 100644 --- a/config/app.config +++ b/config/app.config @@ -35,9 +35,8 @@ default_output_path: /vagrant/runfolder_output/ bcl2fastq_logs_path: bcl2fastq_logs -# Setting this to True will allow the bcl2fastq service direct -# the output of bcl2fastq anywhere on the filesystem (restricted -# by normal permissions). If this is set to False (as per default) -# it will only write output to the specified `default_output_path` -allow_arbitrary_output_folder: False +# Only folders and child folder of the directories listed here will be valid as output +# directories. +allowed_output_folders: + - /vagrant/runfolder_output/ diff --git a/tests/test_bcl2fastq_handlers.py b/tests/test_bcl2fastq_handlers.py index eb38ba3..c19a4ca 100644 --- a/tests/test_bcl2fastq_handlers.py +++ b/tests/test_bcl2fastq_handlers.py @@ -32,9 +32,8 @@ def start_api_call_nbr(self): 8: "y*,7i,n*,y*", } - DUMMY_RUNNER_CONF = DummyRunnerConfig(output='/foo/bar') - dummy_config = DummyConfig() + DUMMY_RUNNER_CONF = DummyRunnerConfig(output='/foo/bar/runfolder', general_config = dummy_config) def get_app(self): return Application(routes(config=self.dummy_config)) @@ -94,22 +93,32 @@ def test_start_with_empty_body(self): def test_start_with_disallowed_output_specified(self): - self.dummy_config.DUMMY_CONFIG['allow_arbitrary_output_folder'] = False + # TODO Please note that this test is not very good, since the + # what we ideally want to test is the output specifed by the request, + # and that that gets rejected, however since the create_bcl2fastq_runner command + # is mocked away here and that is what returns the invalid output the tests is + # a bit misleading. In the future we probably want to refactor this. + # / JD 20170117 + runner_conf_with_invalid_output = DummyRunnerConfig(output='/not/foo/bar/runfolder', + general_config = self.dummy_config) + with mock.patch.object(os.path, 'isdir', return_value=True), \ + mock.patch.object(shutil, 'rmtree', return_value=None), \ + mock.patch.object(Bcl2FastqConfig, 'get_bcl2fastq_version_from_run_parameters', return_value="2.15.2"), \ + mock.patch.object(BCL2FastqRunnerFactory, "create_bcl2fastq_runner", + return_value=FakeRunner("2.15.2", runner_conf_with_invalid_output)), \ + mock.patch.object(BCL2FastqRunner, 'symlink_output_to_unaligned', return_value=None): - with mock.patch.object(os.path, 'isdir', return_value=True): - body = {'output': '/foo/bar'} + # This output dir is not allowed by the config + body = {'output': '/not/foo/bar'} response = self.fetch(self.API_BASE + "/start/150415_D00457_0091_AC6281ANXX", method="POST", body=json_encode(body)) + self.assertEqual(response.code, 500) - self.assertEqual(response.reason, "A output folder was specified by the request, " - "but the configuration does " - "not allow arbitrary output folder to be specified.") + self.assertEqual(response.reason, "Invalid output directory /not/foo/bar/runfolder was specified." + " Allowed dirs were: ['/foo/bar']") def test_start_with_allowed_output_specified(self): - - self.dummy_config.DUMMY_CONFIG['allow_arbitrary_output_folder'] = True - with mock.patch.object(os.path, 'isdir', return_value=True), \ mock.patch.object(shutil, 'rmtree', return_value=None), \ mock.patch.object(Bcl2FastqConfig, 'get_bcl2fastq_version_from_run_parameters', return_value="2.15.2"), \ diff --git a/tests/test_utils.py b/tests/test_utils.py index 40d5750..fa09c76 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -54,14 +54,15 @@ class DummyConfig: "HiSeq 2000": {"bcl2fastq_version": "1.8.4"}, "NextSeq 500": {"bcl2fastq_version": "1.8.4"}}, "bcl2fastq_logs_path": "/tmp/", - "allow_arbitrary_output_folder": False} + "allowed_output_folders": ['/foo/bar']} def __getitem__(self, key): return self.DUMMY_CONFIG[key] class DummyRunnerConfig(Bcl2FastqConfig): - def __init__(self, output): + def __init__(self, output, general_config): self.output = output + self.general_config = general_config class FakeRunner(BCL2FastqRunner): From c5c3a17783e971fa254df6ba63f8beb6f27b76d7 Mon Sep 17 00:00:00 2001 From: Johan Dahlberg Date: Wed, 18 Jan 2017 11:47:18 +0100 Subject: [PATCH 7/7] Raise instead of returning, and better test --- bcl2fastq/lib/bcl2fastq_utils.py | 10 ++++------ tests/test_bcl2fastq_handlers.py | 8 +++++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/bcl2fastq/lib/bcl2fastq_utils.py b/bcl2fastq/lib/bcl2fastq_utils.py index fe0e7f8..8a0f444 100644 --- a/bcl2fastq/lib/bcl2fastq_utils.py +++ b/bcl2fastq/lib/bcl2fastq_utils.py @@ -307,9 +307,7 @@ def _parent_dir(d): abs_path_of_allowed_dirs = map(os.path.abspath, self.config.general_config['allowed_output_folders']) is_located_in_parent_dir = _parent_dir(self.config.output) in abs_path_of_allowed_dirs - if is_located_in_parent_dir: - True - else: + if not is_located_in_parent_dir: error_string = "Invalid output directory {} was specified." \ " Allowed dirs were: {}".format(self.config.output, self.config.general_config['allowed_output_folders']) @@ -321,9 +319,9 @@ def delete_output(self): Delete the output directory if it exists and the output path is valid :return: None """ - if self.validate_output(): - log.info("Found a directory at output path {}, will remove it.".format(self.config.output)) - shutil.rmtree(self.config.output) + self.validate_output() + log.info("Found a directory at output path {}, will remove it.".format(self.config.output)) + shutil.rmtree(self.config.output) def symlink_output_to_unaligned(self): """ diff --git a/tests/test_bcl2fastq_handlers.py b/tests/test_bcl2fastq_handlers.py index c19a4ca..1eba646 100644 --- a/tests/test_bcl2fastq_handlers.py +++ b/tests/test_bcl2fastq_handlers.py @@ -100,9 +100,9 @@ def test_start_with_disallowed_output_specified(self): # a bit misleading. In the future we probably want to refactor this. # / JD 20170117 runner_conf_with_invalid_output = DummyRunnerConfig(output='/not/foo/bar/runfolder', - general_config = self.dummy_config) + general_config=self.dummy_config) with mock.patch.object(os.path, 'isdir', return_value=True), \ - mock.patch.object(shutil, 'rmtree', return_value=None), \ + mock.patch.object(shutil, 'rmtree', return_value=None) as rmmock, \ mock.patch.object(Bcl2FastqConfig, 'get_bcl2fastq_version_from_run_parameters', return_value="2.15.2"), \ mock.patch.object(BCL2FastqRunnerFactory, "create_bcl2fastq_runner", return_value=FakeRunner("2.15.2", runner_conf_with_invalid_output)), \ @@ -117,10 +117,11 @@ def test_start_with_disallowed_output_specified(self): self.assertEqual(response.code, 500) self.assertEqual(response.reason, "Invalid output directory /not/foo/bar/runfolder was specified." " Allowed dirs were: ['/foo/bar']") + rmmock.assert_not_called() def test_start_with_allowed_output_specified(self): with mock.patch.object(os.path, 'isdir', return_value=True), \ - mock.patch.object(shutil, 'rmtree', return_value=None), \ + mock.patch.object(shutil, 'rmtree', return_value=None) as rmmock, \ mock.patch.object(Bcl2FastqConfig, 'get_bcl2fastq_version_from_run_parameters', return_value="2.15.2"), \ mock.patch.object(BCL2FastqRunnerFactory, "create_bcl2fastq_runner", return_value=FakeRunner("2.15.2", self.DUMMY_RUNNER_CONF)), \ @@ -133,6 +134,7 @@ def test_start_with_allowed_output_specified(self): # Just increment it here so it doesn't break the other tests self.start_api_call_nbr() self.assertEqual(response.code, 202) + rmmock.assert_called_with('/foo/bar/runfolder') def test_start_providing_samplesheet(self): # Use mock to ensure that this will run without