From fd4744ec0e2ffcd33c616d2138ce16bf90202315 Mon Sep 17 00:00:00 2001
From: Kent Pitman <kent_pitman@hms.harvard.edu>
Date: Fri, 17 Dec 2021 10:30:22 -0500
Subject: [PATCH 1/8] First cut at fixing bug.

---
 CHANGELOG.rst                        |   5 +
 poetry.lock                          |   8 +-
 pyproject.toml                       |   8 +-
 submit_cgap/submission.py            |  57 ++++++++++++
 submit_cgap/tests/test_submission.py | 131 +++++++++++++++++++++++++++
 5 files changed, 201 insertions(+), 8 deletions(-)

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index fb64691..c0f2c58 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -7,6 +7,11 @@ Change Log
 ----------
 
 
+1.2.0
+=====
+
+* Better error diagnostics for S3FS-mounted files that are glaciated if CGAP_S3FS_MOUNT_DIR is set.
+
 1.1.1
 =====
 
diff --git a/poetry.lock b/poetry.lock
index 5bbb428..19319e0 100644
--- a/poetry.lock
+++ b/poetry.lock
@@ -490,7 +490,7 @@ yaml = ["PyYAML (>=3.10)"]
 
 [[package]]
 name = "dcicutils"
-version = "3.4.0"
+version = "3.4.2"
 description = "Utility package for interacting with the 4DN Data Portal and other 4DN resources"
 category = "main"
 optional = false
@@ -1166,7 +1166,7 @@ testing = ["pytest (>=4.6)", "pytest-checkdocs (>=2.4)", "pytest-flake8", "pytes
 [metadata]
 lock-version = "1.1"
 python-versions = ">=3.6.1,<3.8"
-content-hash = "35910676b5e0e91aa168b2bf6c2fec63396bf9b524387d852df77605c4c26721"
+content-hash = "90891cdf7a381d7cddfe82c1f2679c173d375043c830a2277837f789ecbdde89"
 
 [metadata.files]
 alabaster = [
@@ -1288,8 +1288,8 @@ coveralls = [
     {file = "coveralls-3.2.0.tar.gz", hash = "sha256:15a987d9df877fff44cd81948c5806ffb6eafb757b3443f737888358e96156ee"},
 ]
 dcicutils = [
-    {file = "dcicutils-3.4.0-py3-none-any.whl", hash = "sha256:a7a8dcc8a50b8f4a599ad7b93027383faabec9af077c8a02e52d348a40f66ae3"},
-    {file = "dcicutils-3.4.0.tar.gz", hash = "sha256:07e719fdbb11f72227a131b8d650c048ecd786bccd7348b09c0e55f65bbccc54"},
+    {file = "dcicutils-3.4.2-py3-none-any.whl", hash = "sha256:78519432cd5a440cc7a87ad7c6d4726e25733b986eba77691da527920cc9138b"},
+    {file = "dcicutils-3.4.2.tar.gz", hash = "sha256:25a7e8d58f45a44d8ca08f5a1dbd20baa2289f8c4ac7ee58e4602c6fd19b69b8"},
 ]
 docker = [
     {file = "docker-4.4.4-py2.py3-none-any.whl", hash = "sha256:f3607d5695be025fa405a12aca2e5df702a57db63790c73b927eb6a94aac60af"},
diff --git a/pyproject.toml b/pyproject.toml
index 5eaaad4..76d29c6 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "submit_cgap"
-version = "1.1.1"
+version = "1.1.1.1b0"  # eventually "1.2.0"
 description = "Support for uploading file submissions to the Clinical Genomics Analysis Platform (CGAP)."
 authors = ["4DN-DCIC Team <support@4dnucleome.org>"]
 license = "MIT"
@@ -46,7 +46,7 @@ include = [
 python = ">=3.6.1,<3.8"
 
 awscli = ">=1.18.174"
-dcicutils = ">=3.4.0"
+dcicutils = ">=3.4.2"
 boto3 = ">=1.16.14,<2"
 requests = ">=2.24.0,<3"
 
@@ -94,6 +94,6 @@ exclude_lines = [
 ]
 
 [build-system]
+requires = ["poetry-core>=1.0.0"]
+build-backend = "poetry.core.masonry.api"
 
-requires = ["poetry>=0.12"]
-build-backend = "poetry.masonry.api"
diff --git a/submit_cgap/submission.py b/submit_cgap/submission.py
index 71f69fb..cc5ddc7 100644
--- a/submit_cgap/submission.py
+++ b/submit_cgap/submission.py
@@ -1,3 +1,4 @@
+import boto3
 import contextlib
 import glob
 import io
@@ -648,9 +649,65 @@ def do_uploads(upload_spec_list, auth, folder=None, no_query=False, subfolders=F
             upload_file_to_uuid(filename=filename, uuid=uuid, auth=auth)
             show("Upload of %s to item %s was successful." % (filename, uuid))
         except Exception as e:
+            maybe_show_s3fs_warnings(filename)
             show("%s: %s" % (e.__class__.__name__, e))
 
 
+# Refs:
+#  * https://docs.aws.amazon.com/AmazonS3/latest/userguide/storage-class-intro.html
+#  * https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html
+#
+# We single out 'available' storage classes as the ones someone would expect to be readily
+# available in short time and not subject to catastrophe, so that an I/O error probably
+# isn't related to the storage class. In practice that's the two standard storage classes
+# plus the intelligent tiering.  Most of the others have a latency issue or are otherwise
+# fragile. In practice, we just want to not overly warn about normal kinds of storage.
+
+S3_AVAILABLE_STORAGE_CLASSES = [
+    'STANDARD', 'STANDARD_IA', 'INTELLIGENT_TIERING'
+]
+
+CGAP_S3FS_MAPPING_PATTERN = re.compile(r"^([^:]+):(.+)$")
+
+
+def check_s3fs_mapped_filename(filename):
+    s3fs_mapping = os.environ.get('CGAP_S3FS_MAPPING')
+    if not s3fs_mapping:
+        # We're not using S3FS mapping, so we have no warnings to show.
+        return
+    m = CGAP_S3FS_MAPPING_PATTERN.match(s3fs_mapping)
+    if not m:
+        # We were trying to use S3FS mapping, but the mapping info is in bad form.
+        show(f"CGAP_F3FS_MAPPING is in improper form: {s3fs_mapping!r}")
+        return
+    mapped_bucket, mapped_dir = m.groups()
+    mapped_dir = mapped_dir.rstrip('/')
+    pattern = f"^(?:{re.escape(mapped_dir)}|{re.escape(os.path.expanduser(mapped_dir))})/(.*)$"
+    m = re.match(pattern, filename)
+    if not m:
+        # Some files might not match our mapping.
+        return
+    mapped_key = m.group(1)
+    return mapped_bucket, mapped_key
+
+
+def maybe_show_s3fs_warnings(filename):
+    mapped_bucket_and_key = check_s3fs_mapped_filename(filename)
+    if not mapped_bucket_and_key:
+        return False
+    # If we get to here, we have a plausibly mapped filename that we might want to warn about.
+    mapped_bucket, mapped_key = mapped_bucket_and_key
+    try:
+        s3 = boto3.client('s3')
+        metadata = s3.head_object(Bucket=mapped_bucket, Key=mapped_key)
+        storage_class = metadata['StorageClass']
+        if not storage_class in S3_AVAILABLE_STORAGE_CLASSES:
+            show(f"The file {filename} is mapped via S3FS to {storage_class} storage.")
+    except Exception as e:
+        # Add some context for an error message we're about to see on the console.
+        show(f"An error occurred while trying to ask S3 about Bucket={mapped_bucket!r}, Key={mapped_key!r}: {e}")
+
+
 def upload_item_data(item_filename, uuid, server, env, no_query=False):
     """
     Given a part_filename, uploads that filename to the Item specified by uuid on the given server.
diff --git a/submit_cgap/tests/test_submission.py b/submit_cgap/tests/test_submission.py
index e20a8a9..7cafb38 100644
--- a/submit_cgap/tests/test_submission.py
+++ b/submit_cgap/tests/test_submission.py
@@ -6,6 +6,7 @@
 
 from dcicutils.qa_utils import (
     override_environ, ignored, ControlledTime, MockFileSystem, local_attrs, raises_regexp, printed_output,
+    MockBoto3, MockBotoS3Client, MockKeysNotImplemented
 )
 from dcicutils.s3_utils import HealthPageKey
 from unittest import mock
@@ -20,6 +21,7 @@
     resolve_server, resume_uploads, script_catch_errors, show_section, submit_any_ingestion,
     upload_file_to_uuid, upload_item_data, PROGRESS_CHECK_INTERVAL,
     get_s3_encrypt_key_id, get_s3_encrypt_key_id_from_health_page,
+    check_s3fs_mapped_filename, maybe_show_s3fs_warnings,
 )
 from ..utils import FakeResponse
 
@@ -2036,3 +2038,132 @@ def mysterious_error(*args, **kwargs):
             # After 1 second to recheck the time...
             '12:02:10 Timed out after 8 tries.',
         ]
+
+
+def test_check_s3fs_mapped_filename():
+
+    raw_mapping_dir = "~/my-uploads"
+    expanded_mapping_dir = os.path.expanduser(raw_mapping_dir)
+    my_bucket = 'some-bucket-of-mine'
+    my_key = "12345/bar.baz"
+    expected = (my_bucket, my_key)
+
+    for mapping_dir in [raw_mapping_dir, expanded_mapping_dir]:
+        for suffix in ["", "/"]:
+            cgap_s3fs_mapping = f'{my_bucket}:{mapping_dir}{suffix}'
+            with override_environ(CGAP_S3FS_MAPPING=cgap_s3fs_mapping):
+                sample_filename = f'{mapping_dir}/{my_key}'
+                print(f"\nCGAP_S3FS_MAPPING={cgap_s3fs_mapping} sample_filename={sample_filename}")
+                parsed = check_s3fs_mapped_filename(sample_filename)
+                print(f"parsed={parsed}")
+                assert parsed == expected
+
+                with printed_output() as printed:
+                    sample_unmapped_filename = 'something.else'
+                    parsed = check_s3fs_mapped_filename(sample_unmapped_filename)
+                    assert not parsed
+                    assert printed.lines == []
+
+                with override_environ(CGAP_S3FS_MAPPING=""):
+                    with printed_output() as printed:
+                        parsed = check_s3fs_mapped_filename(sample_filename)
+                        assert not parsed
+                        assert printed.lines == []
+
+                with override_environ(CGAP_S3FS_MAPPING=None):
+                    with printed_output() as printed:
+                        parsed = check_s3fs_mapped_filename(sample_filename)
+                        assert not parsed
+                        assert printed.lines == []
+
+                bad_cgap_s3fs_mapping = "something with no colon"
+                with override_environ(CGAP_S3FS_MAPPING=bad_cgap_s3fs_mapping):
+                    with printed_output() as printed:
+                        parsed = check_s3fs_mapped_filename(sample_filename)
+                        assert not parsed
+                        assert printed.lines == [f"CGAP_F3FS_MAPPING is in improper form:"
+                                                 f" {repr(bad_cgap_s3fs_mapping)}"]
+
+
+class MockBotoS3ClientWithStorageClass(MockBotoS3Client):
+
+    def head_object(self, Bucket, Key, **kwargs):  # noQA - AWS argument naming style
+        if kwargs != self.other_required_arguments:
+            raise MockKeysNotImplemented("get_object", kwargs.keys())
+
+        pseudo_filename = os.path.join(Bucket, Key)
+
+        if self.s3_files.exists(pseudo_filename):
+            content = self.s3_files.files[pseudo_filename]
+            return {
+                'Bucket': Bucket,
+                'Key': Key,
+                'ETag': self._content_etag(content),
+                'ContentLength': len(content),
+                'StorageClass': self.storage_class,
+                # Numerous others, but this is enough to make the dictionary non-empty and to satisfy some of our tools
+            }
+        else:
+            # I would need to research what specific error is needed here and hwen,
+            # since it might be a 404 (not found) or a 403 (permissions), depending on various details.
+            # For now, just fail in any way since maybe our code doesn't care.
+            raise Exception("Mock File Not Found")
+
+
+def test_maybe_show_s3fs_warnings():
+
+    class MockBotoStandardS3Client(MockBotoS3ClientWithStorageClass):
+        DEFAULT_STORAGE_CLASS = 'STANDARD'
+    mock_boto3_with_standard_s3 = MockBoto3(s3=MockBotoStandardS3Client)
+
+    class MockBotoDeepArchiveS3Client(MockBotoS3ClientWithStorageClass):
+        DEFAULT_STORAGE_CLASS = 'DEEP_ARCHIVE'
+    mock_boto3_with_deep_archive_s3 = MockBoto3(s3=MockBotoDeepArchiveS3Client)
+
+    with mock.patch.object(submission_module, "check_s3fs_mapped_filename") as mock_check:
+
+        mock_boto3 = mock_boto3_with_standard_s3
+        with mock.patch.object(submission_module, "boto3", mock_boto3):
+
+            mock_check.return_value = None
+
+            with printed_output() as printed:
+                maybe_show_s3fs_warnings("some.file")
+                assert printed.lines == []
+
+            mock_check.return_value = ("some-bucket", "some-key")
+
+            with printed_output() as printed:
+                maybe_show_s3fs_warnings("some.file")
+                assert printed.lines == [
+                    "An error occurred while trying to ask S3 about Bucket='some-bucket',"
+                    " Key='some-key': Mock File Not Found"
+                ]
+
+            some_bucket = 'some-bucket'
+            some_key = 'some-key'
+
+            mock_check.return_value = (some_bucket, some_key)
+
+            mock_boto3.client('s3').upload_fileobj(
+                Fileobj=io.BytesIO(b'{"some": "object"}'),
+                Bucket=some_bucket, Key=some_key)
+
+            with printed_output() as printed:
+                maybe_show_s3fs_warnings("some.file")
+                assert printed.lines == []  # some_file is allocated as StorageClass=STANDARD so no warning to offer.
+
+        mock_boto3 = mock_boto3_with_deep_archive_s3
+        with mock.patch.object(submission_module, "boto3", mock_boto3):
+
+            mock_check.return_value = (some_bucket, some_key)
+
+            mock_boto3.client('s3').upload_fileobj(
+                Fileobj=io.BytesIO(b'{"some": "object"}'),
+                Bucket=some_bucket, Key=some_key)
+
+            with printed_output() as printed:
+                maybe_show_s3fs_warnings("some.file")
+                assert printed.lines == [
+                    "The file some.file is mapped via S3FS to DEEP_ARCHIVE storage."
+                ]

From ff499559bacf7972c2a942d2d930ded7cb69613d Mon Sep 17 00:00:00 2001
From: Kent Pitman <kent_pitman@hms.harvard.edu>
Date: Fri, 17 Dec 2021 10:32:11 -0500
Subject: [PATCH 2/8] Simplify some testing with new dcicutils.

---
 submit_cgap/tests/test_submission.py | 29 ++--------------------------
 1 file changed, 2 insertions(+), 27 deletions(-)

diff --git a/submit_cgap/tests/test_submission.py b/submit_cgap/tests/test_submission.py
index 7cafb38..af5c5b0 100644
--- a/submit_cgap/tests/test_submission.py
+++ b/submit_cgap/tests/test_submission.py
@@ -2085,38 +2085,13 @@ def test_check_s3fs_mapped_filename():
                                                  f" {repr(bad_cgap_s3fs_mapping)}"]
 
 
-class MockBotoS3ClientWithStorageClass(MockBotoS3Client):
-
-    def head_object(self, Bucket, Key, **kwargs):  # noQA - AWS argument naming style
-        if kwargs != self.other_required_arguments:
-            raise MockKeysNotImplemented("get_object", kwargs.keys())
-
-        pseudo_filename = os.path.join(Bucket, Key)
-
-        if self.s3_files.exists(pseudo_filename):
-            content = self.s3_files.files[pseudo_filename]
-            return {
-                'Bucket': Bucket,
-                'Key': Key,
-                'ETag': self._content_etag(content),
-                'ContentLength': len(content),
-                'StorageClass': self.storage_class,
-                # Numerous others, but this is enough to make the dictionary non-empty and to satisfy some of our tools
-            }
-        else:
-            # I would need to research what specific error is needed here and hwen,
-            # since it might be a 404 (not found) or a 403 (permissions), depending on various details.
-            # For now, just fail in any way since maybe our code doesn't care.
-            raise Exception("Mock File Not Found")
-
-
 def test_maybe_show_s3fs_warnings():
 
-    class MockBotoStandardS3Client(MockBotoS3ClientWithStorageClass):
+    class MockBotoStandardS3Client(MockBotoS3Client):
         DEFAULT_STORAGE_CLASS = 'STANDARD'
     mock_boto3_with_standard_s3 = MockBoto3(s3=MockBotoStandardS3Client)
 
-    class MockBotoDeepArchiveS3Client(MockBotoS3ClientWithStorageClass):
+    class MockBotoDeepArchiveS3Client(MockBotoS3Client):
         DEFAULT_STORAGE_CLASS = 'DEEP_ARCHIVE'
     mock_boto3_with_deep_archive_s3 = MockBoto3(s3=MockBotoDeepArchiveS3Client)
 

From f032425149e6407b6fb5ccf39ab5dffecb895cba Mon Sep 17 00:00:00 2001
From: Kent Pitman <kent_pitman@hms.harvard.edu>
Date: Fri, 17 Dec 2021 10:53:07 -0500
Subject: [PATCH 3/8] Improved testing.

---
 submit_cgap/submission.py            |  9 +++++--
 submit_cgap/tests/test_submission.py | 38 ++++++++++++++++------------
 2 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/submit_cgap/submission.py b/submit_cgap/submission.py
index cc5ddc7..73e6f6e 100644
--- a/submit_cgap/submission.py
+++ b/submit_cgap/submission.py
@@ -663,7 +663,12 @@ def do_uploads(upload_spec_list, auth, folder=None, no_query=False, subfolders=F
 # plus the intelligent tiering.  Most of the others have a latency issue or are otherwise
 # fragile. In practice, we just want to not overly warn about normal kinds of storage.
 
-S3_AVAILABLE_STORAGE_CLASSES = [
+ALL_S3_STORAGE_CLASSES = [
+    'STANDARD', 'REDUCED_REDUNDANCY', 'STANDARD_IA', 'ONEZONE_IA', 'INTELLIGENT_TIERING',
+    'GLACIER', 'DEEP_ARCHIVE', 'OUTPOSTS', 'GLACIER_IR',
+]
+
+AVAILABLE_S3_STORAGE_CLASSES = [
     'STANDARD', 'STANDARD_IA', 'INTELLIGENT_TIERING'
 ]
 
@@ -701,7 +706,7 @@ def maybe_show_s3fs_warnings(filename):
         s3 = boto3.client('s3')
         metadata = s3.head_object(Bucket=mapped_bucket, Key=mapped_key)
         storage_class = metadata['StorageClass']
-        if not storage_class in S3_AVAILABLE_STORAGE_CLASSES:
+        if not storage_class in AVAILABLE_S3_STORAGE_CLASSES:
             show(f"The file {filename} is mapped via S3FS to {storage_class} storage.")
     except Exception as e:
         # Add some context for an error message we're about to see on the console.
diff --git a/submit_cgap/tests/test_submission.py b/submit_cgap/tests/test_submission.py
index af5c5b0..4b8cda1 100644
--- a/submit_cgap/tests/test_submission.py
+++ b/submit_cgap/tests/test_submission.py
@@ -21,7 +21,7 @@
     resolve_server, resume_uploads, script_catch_errors, show_section, submit_any_ingestion,
     upload_file_to_uuid, upload_item_data, PROGRESS_CHECK_INTERVAL,
     get_s3_encrypt_key_id, get_s3_encrypt_key_id_from_health_page,
-    check_s3fs_mapped_filename, maybe_show_s3fs_warnings,
+    check_s3fs_mapped_filename, maybe_show_s3fs_warnings, ALL_S3_STORAGE_CLASSES, AVAILABLE_S3_STORAGE_CLASSES,
 )
 from ..utils import FakeResponse
 
@@ -2102,14 +2102,16 @@ class MockBotoDeepArchiveS3Client(MockBotoS3Client):
 
             mock_check.return_value = None
 
+            some_file = 'some_upload_dir/some.file'
+
             with printed_output() as printed:
-                maybe_show_s3fs_warnings("some.file")
+                maybe_show_s3fs_warnings(some_file)
                 assert printed.lines == []
 
             mock_check.return_value = ("some-bucket", "some-key")
 
             with printed_output() as printed:
-                maybe_show_s3fs_warnings("some.file")
+                maybe_show_s3fs_warnings(some_file)
                 assert printed.lines == [
                     "An error occurred while trying to ask S3 about Bucket='some-bucket',"
                     " Key='some-key': Mock File Not Found"
@@ -2120,25 +2122,29 @@ class MockBotoDeepArchiveS3Client(MockBotoS3Client):
 
             mock_check.return_value = (some_bucket, some_key)
 
-            mock_boto3.client('s3').upload_fileobj(
+            s3 = mock_boto3.client('s3')
+
+            s3.upload_fileobj(
                 Fileobj=io.BytesIO(b'{"some": "object"}'),
                 Bucket=some_bucket, Key=some_key)
 
             with printed_output() as printed:
-                maybe_show_s3fs_warnings("some.file")
+                maybe_show_s3fs_warnings(some_file)
                 assert printed.lines == []  # some_file is allocated as StorageClass=STANDARD so no warning to offer.
 
-        mock_boto3 = mock_boto3_with_deep_archive_s3
-        with mock.patch.object(submission_module, "boto3", mock_boto3):
+            assert isinstance(s3, MockBotoS3Client)  # We do this for side-effect so that PyCharm will know the type
 
-            mock_check.return_value = (some_bucket, some_key)
+            for storage_class in ALL_S3_STORAGE_CLASSES:
 
-            mock_boto3.client('s3').upload_fileobj(
-                Fileobj=io.BytesIO(b'{"some": "object"}'),
-                Bucket=some_bucket, Key=some_key)
+                s3._set_object_storage_class(f'{some_bucket}/{some_key}', storage_class)
 
-            with printed_output() as printed:
-                maybe_show_s3fs_warnings("some.file")
-                assert printed.lines == [
-                    "The file some.file is mapped via S3FS to DEEP_ARCHIVE storage."
-                ]
+                with printed_output() as printed:
+
+                    maybe_show_s3fs_warnings(some_file)
+
+                    if storage_class in AVAILABLE_S3_STORAGE_CLASSES:
+                        assert printed.lines == []
+                    else:
+                        assert printed.lines == [
+                            f"The file {some_file} is mapped via S3FS to {storage_class} storage."
+                        ]

From bef2e52589f4c09740644be5fab51de98141657a Mon Sep 17 00:00:00 2001
From: Kent Pitman <kent_pitman@hms.harvard.edu>
Date: Mon, 20 Dec 2021 04:56:45 -0500
Subject: [PATCH 4/8] WIP

---
 submit_cgap/submission.py            | 33 ++++++++++++++++++----------
 submit_cgap/tests/test_submission.py |  9 ++++++++
 2 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/submit_cgap/submission.py b/submit_cgap/submission.py
index 73e6f6e..42f3795 100644
--- a/submit_cgap/submission.py
+++ b/submit_cgap/submission.py
@@ -675,25 +675,36 @@ def do_uploads(upload_spec_list, auth, folder=None, no_query=False, subfolders=F
 CGAP_S3FS_MAPPING_PATTERN = re.compile(r"^([^:]+):(.+)$")
 
 
-def check_s3fs_mapped_filename(filename):
-    s3fs_mapping = os.environ.get('CGAP_S3FS_MAPPING')
-    if not s3fs_mapping:
+def check_s3fs_mapped_filename(filename, s3=None):
+    upload_buckets = os.environ.get("UPLOAD_BUCKETS")
+    upload_dir = os.environ.get("UPLOAD_DIR")
+    if not upload_buckets or not upload_dir:
         # We're not using S3FS mapping, so we have no warnings to show.
         return
-    m = CGAP_S3FS_MAPPING_PATTERN.match(s3fs_mapping)
-    if not m:
-        # We were trying to use S3FS mapping, but the mapping info is in bad form.
-        show(f"CGAP_F3FS_MAPPING is in improper form: {s3fs_mapping!r}")
-        return
-    mapped_bucket, mapped_dir = m.groups()
-    mapped_dir = mapped_dir.rstrip('/')
+    mapped_dir = upload_dir.rstrip('/')
     pattern = f"^(?:{re.escape(mapped_dir)}|{re.escape(os.path.expanduser(mapped_dir))})/(.*)$"
     m = re.match(pattern, filename)
     if not m:
         # Some files might not match our mapping.
         return
     mapped_key = m.group(1)
-    return mapped_bucket, mapped_key
+    candidates = bash_enumeration(upload_buckets)
+    for mapped_bucket in candidates:
+        try:
+            s3.head_object(Bucket=mapped_bucket, Key=mapped_key)  # an error means we're failing
+            return mapped_bucket, mapped_key
+        except Exception:
+            pass
+    else:
+        # No suitable match found
+        return
+
+
+STRING_LIST_SEPARATORS = str.maketrans("\n\t,", "   ")
+
+
+def bash_enumeration(string_list):
+    return [x for x in string_list.translate(STRING_LIST_SEPARATORS).split(" ") if x]
 
 
 def maybe_show_s3fs_warnings(filename):
diff --git a/submit_cgap/tests/test_submission.py b/submit_cgap/tests/test_submission.py
index 4b8cda1..baeabd0 100644
--- a/submit_cgap/tests/test_submission.py
+++ b/submit_cgap/tests/test_submission.py
@@ -22,6 +22,7 @@
     upload_file_to_uuid, upload_item_data, PROGRESS_CHECK_INTERVAL,
     get_s3_encrypt_key_id, get_s3_encrypt_key_id_from_health_page,
     check_s3fs_mapped_filename, maybe_show_s3fs_warnings, ALL_S3_STORAGE_CLASSES, AVAILABLE_S3_STORAGE_CLASSES,
+    bash_enumeration,
 )
 from ..utils import FakeResponse
 
@@ -2148,3 +2149,11 @@ class MockBotoDeepArchiveS3Client(MockBotoS3Client):
                         assert printed.lines == [
                             f"The file {some_file} is mapped via S3FS to {storage_class} storage."
                         ]
+
+def test_bash_enumeration():
+
+    assert bash_enumeration('foo bar baz') == ['foo', 'bar', 'baz']
+    assert bash_enumeration('foo\nbar\nbaz') == ['foo', 'bar', 'baz']
+    assert bash_enumeration('foo, bar,baz') == ['foo', 'bar', 'baz']
+    assert bash_enumeration('foo,\n  bar   ,,,baz, \n') == ['foo', 'bar', 'baz']
+

From 21b74bb8e24d4e02980e6d8e18a6ea1f2b2a8b3f Mon Sep 17 00:00:00 2001
From: Kent Pitman <kent_pitman@hms.harvard.edu>
Date: Fri, 25 Feb 2022 07:40:22 -0500
Subject: [PATCH 5/8] Better unit tests.

---
 CHANGELOG.rst                        |   3 +-
 pyproject.toml                       |   2 +-
 submit_cgap/submission.py            |  38 ++++++--
 submit_cgap/tests/test_submission.py | 139 ++++++++++++++++++---------
 4 files changed, 127 insertions(+), 55 deletions(-)

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 2fdc94f..c6ec806 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -10,7 +10,8 @@ Change Log
 1.5.0
 =====
 
-* Better error diagnostics for S3FS-mounted files that are glaciated if CGAP_S3FS_MOUNT_DIR is set.
+* Better error diagnostics for S3FS-mounted files that are glaciated
+  if the ``CGAP_S3FS_UPLOAD_BUCKETS`` and ``CGAP_S3FS_UPLOAD_DIR`` environment variables are set.
 
 
 1.4.0
diff --git a/pyproject.toml b/pyproject.toml
index a58375f..37a36b5 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "submit_cgap"
-version = "1.1.1.1b1"  # incorporates 1.4.0, eventually targeting "1.5.0"
+version = "1.1.1.1b2"  # incorporates 1.4.0, eventually targeting "1.5.0"
 description = "Support for uploading file submissions to the Clinical Genomics Analysis Platform (CGAP)."
 authors = ["4DN-DCIC Team <support@4dnucleome.org>"]
 license = "MIT"
diff --git a/submit_cgap/submission.py b/submit_cgap/submission.py
index e95db38..1a91ce0 100644
--- a/submit_cgap/submission.py
+++ b/submit_cgap/submission.py
@@ -668,18 +668,26 @@ def do_uploads(upload_spec_list, auth, folder=None, no_query=False, subfolders=F
 CGAP_S3FS_MAPPING_PATTERN = re.compile(r"^([^:]+):(.+)$")
 
 
-def check_s3fs_mapped_filename(filename, s3=None):
-    upload_buckets = os.environ.get("UPLOAD_BUCKETS")
-    upload_dir = os.environ.get("UPLOAD_DIR")
+def check_s3fs_mapped_filename(filename, *, s3):
+    """
+    Determines whether a given filename is believed to be mapped by s3fs.
+
+    :param filename: the filename to check
+    :param s3: an s3 client.
+    :return:
+    """
+    s3 = s3 or boto3.client('s3')
+    upload_buckets = os.environ.get("CGAP_S3FS_UPLOAD_BUCKETS")
+    upload_dir = os.environ.get("CGAP_S3FS_UPLOAD_DIR")
     if not upload_buckets or not upload_dir:
         # We're not using S3FS mapping, so we have no warnings to show.
-        return
+        return None
     mapped_dir = upload_dir.rstrip('/')
     pattern = f"^(?:{re.escape(mapped_dir)}|{re.escape(os.path.expanduser(mapped_dir))})/(.*)$"
     m = re.match(pattern, filename)
     if not m:
         # Some files might not match our mapping.
-        return
+        return None
     mapped_key = m.group(1)
     candidates = bash_enumeration(upload_buckets)
     for mapped_bucket in candidates:
@@ -690,24 +698,36 @@ def check_s3fs_mapped_filename(filename, s3=None):
             pass
     else:
         # No suitable match found
-        return
+        return None
 
 
 STRING_LIST_SEPARATORS = str.maketrans("\n\t,", "   ")
 
 
 def bash_enumeration(string_list):
+    """
+    For use to parse a bash variable value that purports to contain a list.
+    The items in the list can be separated by commas, spaces, or newlines
+    (and so the elements may not contain those characters).
+
+    :param string_list: a string representing a list of items separated by commas, spaces, or newlines.
+    :return: the items in the list, as strings
+    """
     return [x for x in string_list.translate(STRING_LIST_SEPARATORS).split(" ") if x]
 
 
-def maybe_show_s3fs_warnings(filename):
-    mapped_bucket_and_key = check_s3fs_mapped_filename(filename)
+def maybe_show_s3fs_warnings(filename, *, s3=None):
+    """
+    If the indicated filename is mapped bucket by s3fs to an s3 bucket/key,
+    this checks whether warning about its storage class is warranted and, if it is, presents such a warning.
+    """
+    s3 = s3 or boto3.client('s3')  # noQA
+    mapped_bucket_and_key = check_s3fs_mapped_filename(filename, s3=s3)
     if not mapped_bucket_and_key:
         return False
     # If we get to here, we have a plausibly mapped filename that we might want to warn about.
     mapped_bucket, mapped_key = mapped_bucket_and_key
     try:
-        s3 = boto3.client('s3')
         metadata = s3.head_object(Bucket=mapped_bucket, Key=mapped_key)
         storage_class = metadata['StorageClass']
         if not storage_class in AVAILABLE_S3_STORAGE_CLASSES:
diff --git a/submit_cgap/tests/test_submission.py b/submit_cgap/tests/test_submission.py
index 7b2c4e9..fb072a1 100644
--- a/submit_cgap/tests/test_submission.py
+++ b/submit_cgap/tests/test_submission.py
@@ -2151,48 +2151,96 @@ def mysterious_error(*args, **kwargs):
 
 
 def test_check_s3fs_mapped_filename():
+    """
+    This will test that the mapping-finder works under various situations of buckets present or missing.
+    The idea is that there might be a key1 that we expect to find in a bucket1, assuming bucket1 is mapped,
+    and a key2 in a bucket2 if that bucket is mapped. But the filename we'll see will contain only the key,
+    not the bucket, so the bucket finder has to call .head_object to know if the key is there. It can then
+    find the key no matter what bucket it's in.
+    """
 
     raw_mapping_dir = "~/my-uploads"
     expanded_mapping_dir = os.path.expanduser(raw_mapping_dir)
-    my_bucket = 'some-bucket-of-mine'
-    my_key = "12345/bar.baz"
-    expected = (my_bucket, my_key)
-
-    for mapping_dir in [raw_mapping_dir, expanded_mapping_dir]:
-        for suffix in ["", "/"]:
-            cgap_s3fs_mapping = f'{my_bucket}:{mapping_dir}{suffix}'
-            with override_environ(CGAP_S3FS_MAPPING=cgap_s3fs_mapping):
-                sample_filename = f'{mapping_dir}/{my_key}'
-                print(f"\nCGAP_S3FS_MAPPING={cgap_s3fs_mapping} sample_filename={sample_filename}")
-                parsed = check_s3fs_mapped_filename(sample_filename)
-                print(f"parsed={parsed}")
-                assert parsed == expected
-
-                with printed_output() as printed:
-                    sample_unmapped_filename = 'something.else'
-                    parsed = check_s3fs_mapped_filename(sample_unmapped_filename)
-                    assert not parsed
-                    assert printed.lines == []
-
-                with override_environ(CGAP_S3FS_MAPPING=""):
-                    with printed_output() as printed:
-                        parsed = check_s3fs_mapped_filename(sample_filename)
-                        assert not parsed
-                        assert printed.lines == []
-
-                with override_environ(CGAP_S3FS_MAPPING=None):
-                    with printed_output() as printed:
-                        parsed = check_s3fs_mapped_filename(sample_filename)
-                        assert not parsed
-                        assert printed.lines == []
-
-                bad_cgap_s3fs_mapping = "something with no colon"
-                with override_environ(CGAP_S3FS_MAPPING=bad_cgap_s3fs_mapping):
-                    with printed_output() as printed:
-                        parsed = check_s3fs_mapped_filename(sample_filename)
-                        assert not parsed
-                        assert printed.lines == [f"CGAP_F3FS_MAPPING is in improper form:"
-                                                 f" {repr(bad_cgap_s3fs_mapping)}"]
+    bucket1 = 'foo1'
+    key1 = "12345/bar.baz"
+    bucket2 = 'foo2'
+    key2 = '67890/bar.baz'
+
+    class FakeS3ClientForTesting:
+        """
+        The only operation our test client needs to support is head_object, and only for side-effect. See below.
+        If other operations get called, we want to know about it, so we use a one-off class.
+        Having done f = FakeS3CLientForTesting("foo/bar"), later doing f.head_object(Bucket="foo", Key="bar") will
+        return without error but other arguments will not.
+        """
+
+        def __init__(self, filenames=None):
+            """Create the class with a list of 'files' in our fake S3."""
+            self.filenames = filenames or []
+
+        def head_object(self, Bucket, Key):  # noQA
+            """We're only being called for side-effect. A non-error return means the file is there."""
+            filename = f"{Bucket}/{Key}"
+            if filename in self.filenames:
+                return {'found': filename}
+            else:
+                raise RuntimeError("File not found.")
+
+    fake_s3 = FakeS3ClientForTesting(filenames=[f"{bucket1}/{key1}", f"{bucket2}/{key2}"])
+
+    def do_testing(mapped_buckets):
+        """
+        We expect a set of mapped buckets like ['bucket1', 'bucket2'] and will run the set of tests on that.
+        The expectation is that if 'bucket1' is anywhere in the list, then key1 will be found in it,
+        and if 'bucket2' is anywhere in the list, then key2 will be found in it. Otherwise None.
+        """
+        expectations = {'decoy': None,
+                        key1: (bucket1, key1) if bucket1 in mapped_buckets else None,
+                        key2: (bucket2, key2) if bucket2 in mapped_buckets else None}
+        for mapping_dir in [raw_mapping_dir, expanded_mapping_dir]:
+            for suffix in ["", "/"]:
+                for upload_key in [key1, key2, 'decoy']:
+                    upload_dir = f"{mapping_dir}{suffix}"
+                    upload_buckets = ",".join(mapped_buckets)
+                    with override_environ(CGAP_S3FS_UPLOAD_BUCKETS=upload_buckets, CGAP_S3FS_UPLOAD_DIR=upload_dir):
+                        sample_filename = f'{mapping_dir}/{upload_key}'
+                        print(f"\nCGAP_S3FS_UPLOAD_BUCKETS={upload_buckets} CGAP_S3FS_UPLOAD_DIR={upload_dir}"
+                              f" sample_filename={sample_filename} key={upload_key}")
+                        parsed = check_s3fs_mapped_filename(sample_filename, s3=fake_s3)
+                        print(f"parsed={parsed}")
+                        assert parsed == expectations[upload_key]
+
+                        with printed_output() as printed:
+                            sample_unmapped_filename = 'something.else'
+                            parsed = check_s3fs_mapped_filename(sample_unmapped_filename, s3=fake_s3)
+                            assert not parsed
+                            assert printed.lines == []
+
+                        with override_environ(CGAP_S3FS_UPLOAD_BUCKETS=""):
+                            with printed_output() as printed:
+                                parsed = check_s3fs_mapped_filename(sample_filename, s3=fake_s3)
+                                assert not parsed
+                                assert printed.lines == []
+
+                        with override_environ(CGAP_S3FS_UPLOAD_DIR=""):
+                            with printed_output() as printed:
+                                parsed = check_s3fs_mapped_filename(sample_filename, s3=fake_s3)
+                                assert not parsed
+                                assert printed.lines == []
+
+                        with override_environ(CGAP_S3FS_UPLOAD_DIR=None):
+                            with printed_output() as printed:
+                                parsed = check_s3fs_mapped_filename(sample_filename, s3=fake_s3)
+                                assert not parsed
+                                assert printed.lines == []
+
+    for mapped_buckets in [[],
+                           [bucket1],
+                           [bucket2],
+                           [bucket1, bucket2],
+                           [bucket2, bucket1],
+                           ['decoy', bucket1, bucket2]]:
+        do_testing(mapped_buckets)
 
 
 def test_maybe_show_s3fs_warnings():
@@ -2201,10 +2249,6 @@ class MockBotoStandardS3Client(MockBotoS3Client):
         DEFAULT_STORAGE_CLASS = 'STANDARD'
     mock_boto3_with_standard_s3 = MockBoto3(s3=MockBotoStandardS3Client)
 
-    class MockBotoDeepArchiveS3Client(MockBotoS3Client):
-        DEFAULT_STORAGE_CLASS = 'DEEP_ARCHIVE'
-    mock_boto3_with_deep_archive_s3 = MockBoto3(s3=MockBotoDeepArchiveS3Client)
-
     with mock.patch.object(submission_module, "check_s3fs_mapped_filename") as mock_check:
 
         mock_boto3 = mock_boto3_with_standard_s3
@@ -2214,6 +2258,11 @@ class MockBotoDeepArchiveS3Client(MockBotoS3Client):
 
             some_file = 'some_upload_dir/some.file'
 
+            with printed_output() as printed:
+                # This just tests that explicitly passing the argument works.
+                maybe_show_s3fs_warnings(some_file, s3=mock_boto3)
+                assert printed.lines == []
+
             with printed_output() as printed:
                 maybe_show_s3fs_warnings(some_file)
                 assert printed.lines == []
@@ -2261,8 +2310,10 @@ class MockBotoDeepArchiveS3Client(MockBotoS3Client):
 
 def test_bash_enumeration():
 
+    # The bash_enumeration function is just testing that a user of bash, having made what they informally think
+    # of as a list, will have specified something coherent. They can use commas, spaces, or newlines as separators.
+
     assert bash_enumeration('foo bar baz') == ['foo', 'bar', 'baz']
     assert bash_enumeration('foo\nbar\nbaz') == ['foo', 'bar', 'baz']
     assert bash_enumeration('foo, bar,baz') == ['foo', 'bar', 'baz']
     assert bash_enumeration('foo,\n  bar   ,,,baz, \n') == ['foo', 'bar', 'baz']
-

From 2cc5a0c8723c57e896e1632a74c1846dc20d244e Mon Sep 17 00:00:00 2001
From: Kent Pitman <kent_pitman@hms.harvard.edu>
Date: Fri, 25 Feb 2022 08:15:30 -0500
Subject: [PATCH 6/8] Fix some PEP8 issues caught by flake8.

---
 submit_cgap/submission.py            | 2 +-
 submit_cgap/tests/test_submission.py | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/submit_cgap/submission.py b/submit_cgap/submission.py
index 1a91ce0..25ca34a 100644
--- a/submit_cgap/submission.py
+++ b/submit_cgap/submission.py
@@ -730,7 +730,7 @@ def maybe_show_s3fs_warnings(filename, *, s3=None):
     try:
         metadata = s3.head_object(Bucket=mapped_bucket, Key=mapped_key)
         storage_class = metadata['StorageClass']
-        if not storage_class in AVAILABLE_S3_STORAGE_CLASSES:
+        if storage_class not in AVAILABLE_S3_STORAGE_CLASSES:
             show(f"The file {filename} is mapped via S3FS to {storage_class} storage.")
     except Exception as e:
         # Add some context for an error message we're about to see on the console.
diff --git a/submit_cgap/tests/test_submission.py b/submit_cgap/tests/test_submission.py
index fb072a1..065f7ab 100644
--- a/submit_cgap/tests/test_submission.py
+++ b/submit_cgap/tests/test_submission.py
@@ -6,7 +6,7 @@
 
 from dcicutils.qa_utils import (
     override_environ, ignored, ControlledTime, MockFileSystem, local_attrs, raises_regexp, printed_output,
-    MockBoto3, MockBotoS3Client, MockKeysNotImplemented
+    MockBoto3, MockBotoS3Client,  # MockKeysNotImplemented,
 )
 from dcicutils.s3_utils import HealthPageKey
 from unittest import mock
@@ -2308,6 +2308,7 @@ class MockBotoStandardS3Client(MockBotoS3Client):
                             f"The file {some_file} is mapped via S3FS to {storage_class} storage."
                         ]
 
+
 def test_bash_enumeration():
 
     # The bash_enumeration function is just testing that a user of bash, having made what they informally think

From 2e1e15b676460f5b4a734cf511c72881b9b9a53a Mon Sep 17 00:00:00 2001
From: Kent Pitman <kent_pitman@hms.harvard.edu>
Date: Mon, 31 Jul 2023 09:17:45 -0400
Subject: [PATCH 7/8] Bump beta version.

---
 pyproject.toml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pyproject.toml b/pyproject.toml
index f5f4341..f31451d 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "submit_cgap"
-version = "4.1.0.1b1"
+version = "4.1.0.1b2"
 description = "Support for uploading file submissions to the Clinical Genomics Analysis Platform (CGAP)."
 authors = ["4DN-DCIC Team <support@4dnucleome.org>"]
 license = "MIT"

From 740fb785fbd25d7c1690e76e21ce50d937d4f530 Mon Sep 17 00:00:00 2001
From: Kent Pitman <kent_pitman@hms.harvard.edu>
Date: Mon, 31 Jul 2023 09:23:55 -0400
Subject: [PATCH 8/8] Bump beta version.

---
 pyproject.toml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pyproject.toml b/pyproject.toml
index f31451d..fbf9d1b 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "submit_cgap"
-version = "4.1.0.1b2"
+version = "4.1.0.2b3"
 description = "Support for uploading file submissions to the Clinical Genomics Analysis Platform (CGAP)."
 authors = ["4DN-DCIC Team <support@4dnucleome.org>"]
 license = "MIT"