From a4c14990574bf7a83a09dd687f69fd0a80ae0354 Mon Sep 17 00:00:00 2001 From: "C. Titus Brown" Date: Wed, 7 Sep 2022 07:28:20 -0700 Subject: [PATCH 1/2] clean up zip error handling for bad zip files --- src/sourmash/sbt_storage.py | 9 +++++---- src/sourmash/sourmash_args.py | 10 +++++++++- tests/test_sourmash_args.py | 22 ++++++++++++++++++++++ 3 files changed, 36 insertions(+), 5 deletions(-) diff --git a/src/sourmash/sbt_storage.py b/src/sourmash/sbt_storage.py index 398e11c877..459830443c 100644 --- a/src/sourmash/sbt_storage.py +++ b/src/sourmash/sbt_storage.py @@ -323,10 +323,11 @@ def load(self, path): def close(self): # TODO: this is not ideal; checking for zipfile.fp is looking at # internal implementation details from CPython... - if self.zipfile is not None or self.bufferzip is not None: - self.flush(keep_closed=True) - self.zipfile.close() - self.zipfile = None + if hasattr(self, 'zipfile'): + if self.zipfile is not None or self.bufferzip is not None: + self.flush(keep_closed=True) + self.zipfile.close() + self.zipfile = None def flush(self, *, keep_closed=False): # This is a bit complicated, but we have to deal with new data diff --git a/src/sourmash/sourmash_args.py b/src/sourmash/sourmash_args.py index f13e2c9037..93c7d0e32a 100644 --- a/src/sourmash/sourmash_args.py +++ b/src/sourmash/sourmash_args.py @@ -1112,7 +1112,15 @@ def open(self): if os.path.exists(self.location): do_create = False - storage = ZipStorage(self.location, mode="w") + storage = None + try: + storage = ZipStorage(self.location, mode="w") + except zipfile.BadZipFile: + pass + + if storage is None: + raise ValueError(f"File '{self.location}' cannot be opened as a zip file.") + if not storage.subdir: storage.subdir = 'signatures' diff --git a/tests/test_sourmash_args.py b/tests/test_sourmash_args.py index 844b4510e2..bf98014816 100644 --- a/tests/test_sourmash_args.py +++ b/tests/test_sourmash_args.py @@ -137,6 +137,28 @@ def test_save_signatures_to_location_1_zip(runtmp): assert len(saved) == 2 +def test_save_signatures_to_location_1_zip_bad(runtmp): + # try saving to bad sigfile.zip + sig2 = utils.get_test_data('2.fa.sig') + ss2 = sourmash.load_one_signature(sig2, ksize=31) + sig47 = utils.get_test_data('47.fa.sig') + ss47 = sourmash.load_one_signature(sig47, ksize=31) + + outloc = runtmp.output('foo.zip') + + # create bad zip: + with open(outloc, 'wt') as fp: + pass + + # now check for error + with pytest.raises(ValueError) as exc: + with sourmash_args.SaveSignaturesToLocation(outloc) as save_sig: + pass + + assert 'cannot be opened as a zip file' in str(exc) + + + def test_save_signatures_to_location_1_zip_dup(runtmp): # save to sigfile.zip sig2 = utils.get_test_data('2.fa.sig') From 924fc2433678f8de12a1cad7deced229275793e6 Mon Sep 17 00:00:00 2001 From: "C. Titus Brown" Date: Wed, 7 Sep 2022 07:45:53 -0700 Subject: [PATCH 2/2] comments, spacing --- src/sourmash/sbt_storage.py | 3 +++ tests/test_sourmash_args.py | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/sourmash/sbt_storage.py b/src/sourmash/sbt_storage.py index 459830443c..a22e782d69 100644 --- a/src/sourmash/sbt_storage.py +++ b/src/sourmash/sbt_storage.py @@ -323,6 +323,9 @@ def load(self, path): def close(self): # TODO: this is not ideal; checking for zipfile.fp is looking at # internal implementation details from CPython... + + # might not have self.zipfile if was invalid zipfile and __init__ + # failed. if hasattr(self, 'zipfile'): if self.zipfile is not None or self.bufferzip is not None: self.flush(keep_closed=True) diff --git a/tests/test_sourmash_args.py b/tests/test_sourmash_args.py index bf98014816..2e6bfaab53 100644 --- a/tests/test_sourmash_args.py +++ b/tests/test_sourmash_args.py @@ -158,7 +158,6 @@ def test_save_signatures_to_location_1_zip_bad(runtmp): assert 'cannot be opened as a zip file' in str(exc) - def test_save_signatures_to_location_1_zip_dup(runtmp): # save to sigfile.zip sig2 = utils.get_test_data('2.fa.sig')