Preparing for 0.3.2 release

This PR should address https://github.com/Novartis/cellxgene-gateway/issues/45

.github/workflows/codeql-analysis.yml

@@ -0,0 +1,67 @@
+# For most projects, this workflow file will not need changing; you simply need
+# to commit it to your repository.
+#
+# You may wish to alter this file to override the set of languages analyzed,
+# or to provide custom queries or build logic.
+#
+# ******** NOTE ********
+# We have attempted to detect the languages in your repository. Please check
+# the `language` matrix defined below to confirm you have the correct set of
+# supported CodeQL languages.
+#
+name: "CodeQL"
+
+on:
+  push:
+    branches: [ master ]
+  pull_request:
+    # The branches below must be a subset of the branches above
+    branches: [ master ]
+  schedule:
+    - cron: '18 6 * * 6'
+
+jobs:
+  analyze:
+    name: Analyze
+    runs-on: ubuntu-latest
+
+    strategy:
+      fail-fast: false
+      matrix:
+        language: [ 'javascript', 'python' ]
+        # CodeQL supports [ 'cpp', 'csharp', 'go', 'java', 'javascript', 'python' ]
+        # Learn more:
+        # https://docs.github.com/en/free-pro-team@latest/github/finding-security-vulnerabilities-and-errors-in-your-code/configuring-code-scanning#changing-the-languages-that-are-analyzed
+
+    steps:
+    - name: Checkout repository
+      uses: actions/checkout@v2
+
+    # Initializes the CodeQL tools for scanning.
+    - name: Initialize CodeQL
+      uses: github/codeql-action/init@v1
+      with:
+        languages: ${{ matrix.language }}
+        # If you wish to specify custom queries, you can do so here or in a config file.
+        # By default, queries listed here will override any specified in a config file.
+        # Prefix the list here with "+" to use these queries and those in the config file.
+        # queries: ./path/to/local/query, your-org/your-repo/queries@main
+
+    # Autobuild attempts to build any compiled languages  (C/C++, C#, or Java).
+    # If this step fails, then you should remove it and run the build manually (see below)
+    - name: Autobuild
+      uses: github/codeql-action/autobuild@v1
+
+    # ℹ️ Command-line programs to run using the OS shell.
+    # 📚 https://git.io/JvXDl
+
+    # ✏️ If the Autobuild fails above, remove it and uncomment the following three lines
+    #    and modify them (or add more) to build your code if your project
+    #    uses a compiled language
+
+    #- run: |
+    #   make bootstrap
+    #   make release
+
+    - name: Perform CodeQL Analysis
+      uses: github/codeql-action/analyze@v1

.github/workflows/pr-checks.yaml

@@ -57,4 +57,11 @@ jobs:
       - name: "Upload coverage to Codecov"
         uses: codecov/codecov-action@v1
         with:
+          token: ${{ secrets.CODECOV_TOKEN }}
+          files: ./coverage.xml
+          flags: unittests
+          env_vars: OS,PYTHON
+          name: codecov-umbrella
           fail_ci_if_error: true
+          path_to_write_report: ./codecov_report.txt
+          verbose: true

Changelog.md

@@ -1,6 +1,23 @@
+# 0.3.2
+
+ * Fixed bug #45 affecting multi-level S3 folders
+ * Added extra_scripts to cache_status page
+
+# 0.3.1
+
+ * Added missing __init__.py
+
+# 0.3.0
+
+ * Added support for itemsource interface, allowing s3 hosting
+ * Removed support for http file uploads
+ * Only set wsgi.url_scheme when EXTERNAL_PROTOCOL is set (see issue #43) 
+ * Dropped flake8 due to conflicts with black
+ * Added code coverage metrics
+
 # 0.2.3
 
-* Added support for ProxyFix
+ * Added support for ProxyFix
 
 # 0.2.2
 

README.md

@@ -2,6 +2,8 @@
 
 Cellxgene Gateway allows you to use the Cellxgene Server provided by the Chan Zuckerberg Institute (https://github.com/chanzuckerberg/cellxgene) with multiple datasets. It displays an index of available h5ad (anndata) files. When a user clicks on a file name, it launches a Cellxgene Server instance that loads that particular data file and once it is available  proxies requests to that server.
 
+[![codecov](https://codecov.io/gh/Novartis/cellxgene-gateway/branch/master/graph/badge.svg?token=ndEFSzRKJn)](https://codecov.io/gh/Novartis/cellxgene-gateway)
+
 # Running locally
 
 ## Prequisites

cellxgene_gateway/__init__.py

@@ -7,4 +7,4 @@
 # OR CONDITIONS OF ANY KIND, either express or implied. See the License for
 # the specific language governing permissions and limitations under the License.
 
-__version__ = "0.2.3"
+__version__ = "0.3.2"

cellxgene_gateway/env.py

@@ -20,7 +20,7 @@ external_host = os.environ.get(
     os.environ.get("GATEWAY_HOST", f"localhost:{gateway_port}"),
 )
 external_protocol = os.environ.get(
-    "EXTERNAL_PROTOCOL", os.environ.get("GATEWAY_PROTOCOL", "http")
+    "EXTERNAL_PROTOCOL", os.environ.get("GATEWAY_PROTOCOL", None)
 )
 ip = os.environ.get("GATEWAY_IP")
 extra_scripts = os.environ.get("GATEWAY_EXTRA_SCRIPTS")

cellxgene_gateway/extra_scripts.py

@@ -8,6 +8,7 @@
 # the specific language governing permissions and limitations under the License.
 
 from json import loads
+from json.decoder import JSONDecodeError
 
 from cellxgene_gateway import env
 
@@ -17,4 +18,9 @@ def get_extra_scripts():
     # ['https://www.googletagmanager.com/gtag/js?id=UA-123456-2',
     #  f"{env.external_protocol}://{env.external_host}/static/js/google_ua.js"]
     # where google_ua.js is a script you add to the static/js folder prior to deployment.
-    return [] if env.extra_scripts is None else loads(env.extra_scripts)
+    try:
+        return [] if env.extra_scripts is None else loads(env.extra_scripts)
+    except JSONDecodeError as exc:
+        raise Exception(
+            f'Error parsing GATEWAY_EXTRA_SCRIPTS, expected JSON array e.g. ["https://example.com/path/to/script.js"]'
+        ) from exc

cellxgene_gateway/filecrawl.py

@@ -62,5 +62,6 @@ def render_item_tree(item_tree, item_source):
 
 def render_item_source(item_source, filter=None):
     item_tree = item_source.list_items(filter)
-    heading = f"<h6><a href='/filecrawl.html?source={urllib.parse.quote_plus(item_source.name)}'>{item_source.name}</a></h6>"
+    filterpart = "" if filter is None else ":" + filter
+    heading = f"<h6><a href='/filecrawl.html?source={urllib.parse.quote_plus(item_source.name)}'>{item_source.name}</a>{filterpart}</h6>"
     return heading + render_item_tree(item_tree, item_source)

cellxgene_gateway/gateway.py

@@ -46,7 +46,8 @@ default_item_source = None
 
 def _force_https(app):
     def wrapper(environ, start_response):
-        environ["wsgi.url_scheme"] = env.external_protocol
+        if env.external_protocol is not None:
+            environ["wsgi.url_scheme"] = env.external_protocol
         return app(environ, start_response)
 
     return wrapper
@@ -216,7 +217,11 @@ def do_view(path, source_name=None):
 
 @app.route("/cache_status", methods=["GET"])
 def do_GET_status():
-    return render_template("cache_status.html", entry_list=cache.entry_list)
+    return render_template(
+        "cache_status.html",
+        entry_list=cache.entry_list,
+        extra_scripts=get_extra_scripts(),
+    )
 
 
 @app.route("/cache_status.json", methods=["GET"])

cellxgene_gateway/items/s3/s3item_source.py

@@ -38,8 +38,11 @@ class S3ItemSource(ItemSource):
         self.annotation_dir_suffix = annotation_dir_suffix
         self.annotation_file_suffix = annotation_file_suffix
 
-    def url(self, path):
+    def url(self, key):
-        return "s3://" + join(self.bucket, path)
+        return "s3://" + self.bucket + "/" + key
+
+    def remove_bucket(self, filepath):
+        return filepath[len(self.bucket) :].lstrip("/")
 
     @property
     def name(self):
@@ -61,49 +64,44 @@ class S3ItemSource(ItemSource):
         return self.convert_h5ad_key_to_annotation(item.descriptor)
 
     def list_items(self, filter: str = None) -> ItemTree:
-        item_tree = self.scan_directory()
+        item_tree = self.scan_directory("" if filter is None else filter)
         return item_tree
 
-    def scan_directory(self, subpath="") -> dict:
+    def scan_directory(self, directory_key="") -> dict:
-        url = self.url(subpath)
+        url = self.url(directory_key)
 
         if not self.s3.exists(url):
             raise Exception(f"S3 url '{url}' does not exist.")
 
         s3key_map = dict(
-            (filepath[len(self.bucket) :].lstrip("/"), "s3://" + filepath)
+            (self.remove_bucket(filepath), "s3://" + filepath)
             for filepath in sorted(self.s3.ls(url))
         )
 
         def is_annotation_dir(dir_s3key):
             return (
                 dir_s3key.endswith(self.annotation_dir_suffix)
-                and self.convert_annotation_key_to_h5ad(dir_s3key) in h5ad_paths
+                and self.convert_annotation_key_to_h5ad(dir_s3key) in h5ad_keys
             )
 
-        h5ad_paths = [
+        h5ad_keys = [
             filepath
             for filepath, item_url in s3key_map.items()
             if self.is_h5ad_url(item_url)
         ]
 
-        subdirs = [
+        subdir_keys = [
             filepath
             for filepath, item_url in s3key_map.items()
             if self.s3.isdir(item_url) and not is_annotation_dir(filepath)
         ]
 
-        items = [
+        items = [self.make_s3item_from_key(basename(key), key) for key in h5ad_keys]
-            self.make_s3item_from_key(filename, join(subpath, filename))
-            for filename in h5ad_paths
-        ]
         branches = None
-        if len(subdirs) > 0:
+        if len(subdir_keys) > 0:
-            branches = [
+            branches = [self.scan_directory(key) for key in subdir_keys]
-                self.scan_directory(join(subpath, subdir)) for subdir in subdirs
-            ]
 
-        return ItemTree(subpath, items, branches)
+        return ItemTree(directory_key, items, branches)
 
     def create_annotation(self, item: S3Item, name: str) -> S3Item:
         annotation = self.make_s3item_from_key(
@@ -163,11 +161,11 @@ class S3ItemSource(ItemSource):
         if self.s3.isdir(annotations_fullpath):
             return [
                 self.make_s3item_from_key(
-                    annotation, join(annotations_subpath, annotation), True
+                    basename(annotation), self.remove_bucket(annotation), True
                 )
                 for annotation in sorted(self.s3.ls(annotations_fullpath))
                 if annotation.endswith(self.annotation_file_suffix)
-                and self.s3.isfile(join(annotations_fullpath, annotation))
+                and self.s3.isfile("s3://" + annotation)
             ]
         else:
             return None

tests/items/s3/test_s3item_source.py

@@ -0,0 +1,169 @@
+import unittest
+from unittest.mock import MagicMock, Mock, patch
+
+from cellxgene_gateway.items.item import ItemType
+from cellxgene_gateway.items.s3.s3item import S3Item
+from cellxgene_gateway.items.s3.s3item_source import S3ItemSource
+
+
+class TestScanDirectory(unittest.TestCase):
+    @patch("s3fs.S3FileSystem")
+    def test_GIVEN_invalid_bucket_THEN_throws_error(self, s3func):
+        class S3Mock:
+            def exists(path):
+                if path in ["s3://my-bucket/"]:
+                    return False
+
+        s3func.return_value = S3Mock
+        source = S3ItemSource("my-bucket")
+        with self.assertRaises(Exception) as context:
+            source.scan_directory()
+        self.assertEqual(
+            "S3 url 's3://my-bucket/' does not exist.",
+            str(context.exception),
+        )
+
+    @patch("s3fs.S3FileSystem")
+    def test__GIVEN_multilevel_bucket_THEN_properly_recurses_suburls(self, s3func):
+        class S3Mock:
+            def exists(path):
+                if path in [
+                    "s3://my-bucket/",
+                    "s3://my-bucket/pbmc3k.h5ad",
+                    "s3://my-bucket/lvl1",
+                    "s3://my-bucket/lvl1/pbmc3k_l1.h5ad",
+                    "s3://my-bucket/lvl1/lvl2",
+                    "s3://my-bucket/lvl1/lvl2/pbmc3k_l2.h5ad",
+                ]:
+                    return True
+                raise Exception("exists called with " + path)
+
+            def ls(path):
+                if path == "s3://my-bucket/":
+                    return [
+                        "my-bucket/lvl1",
+                        "my-bucket/pbmc3k.h5ad",
+                        "my-bucket/pbmc3k_annotations",
+                    ]
+                elif path == "s3://my-bucket/pbmc3k_annotations":
+                    return ["my-bucket/pbmc3k_annotations/annot.csv"]
+                elif path == "s3://my-bucket/lvl1":
+                    return ["my-bucket/lvl1/lvl2", "my-bucket/lvl1/pbmc3k_l1.h5ad"]
+                elif path == "s3://my-bucket/lvl1/lvl2":
+                    return ["my-bucket/lvl1/lvl2/pbmc3k_l2.h5ad"]
+
+                raise Exception("ls called with " + path)
+
+            def isdir(path):
+                if path in [
+                    "s3://my-bucket/lvl1",
+                    "s3://my-bucket/pbmc3k_annotations",
+                    "s3://my-bucket/lvl1/lvl2",
+                ]:
+                    return True
+                if path in [
+                    "s3://my-bucket/pbmc3k.h5ad",
+                    "s3://my-bucket/lvl1/pbmc3k_l1.h5ad",
+                    "s3://my-bucket/lvl1/pbmc3k_l1_annotations",
+                    "s3://my-bucket/lvl1/lvl2/pbmc3k_l2.h5ad",
+                    "s3://my-bucket/lvl1/lvl2/pbmc3k_l2_annotations",
+                ]:
+                    return False
+                raise Exception("isdir called with " + path)
+
+            def isfile(path):
+                if path in ["s3://my-bucket/pbmc3k_annotations/annot.csv"]:
+                    return True
+                if path in ["s3://my-bucket/pbmc3k_annotations"]:
+                    return False
+                raise Exception("isfile called with " + path)
+
+        s3func.return_value = S3Mock
+        source = S3ItemSource("my-bucket")
+        tree = source.scan_directory()
+
+        def s3item_compare(i1, i2, msg=""):
+            self.assertEqual(i1.name, i2.name, "name equals")
+            self.assertEqual(i1.type, i2.type, "type equals")
+            self.assertEqual(i1.s3key, i2.s3key, "s3key equals")
+            if i1.annotations is None:
+                self.assertEqual(i1.annotations, i2.annotations, "annotations equals")
+            else:
+                self.assertEqual(
+                    len(i1.annotations),
+                    len(i2.annotations),
+                    "annotations length equals",
+                )
+                for a1, a2 in zip(i1.annotations, i2.annotations):
+                    self.assertEqual(a1, a2)
+            return True
+
+        self.addTypeEqualityFunc(S3Item, s3item_compare)
+
+        def assertTree(t, descriptor, items):
+            self.assertEqual(t.descriptor, descriptor)
+            self.assertEqual(len(t.items), len(items))
+            for i1, i2 in zip(t.items, items):
+                self.assertEqual(i1, i2)
+
+        assertTree(
+            tree,
+            "",
+            [
+                S3Item(
+                    "pbmc3k.h5ad",
+                    name="pbmc3k.h5ad",
+                    type=ItemType.h5ad,
+                    annotations=[
+                        S3Item(
+                            "pbmc3k_annotations/annot.csv",
+                            name="annot.csv",
+                            type=ItemType.annotation,
+                        )
+                    ],
+                )
+            ],
+        )
+        self.assertEqual(len(tree.branches), 1)
+        lvl1 = tree.branches[0]
+        assertTree(
+            lvl1,
+            "lvl1",
+            [
+                S3Item(
+                    "lvl1/pbmc3k_l1.h5ad",
+                    name="pbmc3k_l1.h5ad",
+                    type=ItemType.h5ad,
+                    annotations=None,
+                )
+            ],
+        )
+        self.assertEqual(len(lvl1.branches), 1)
+        lvl2 = lvl1.branches[0]
+        assertTree(
+            lvl2,
+            "lvl1/lvl2",
+            [
+                S3Item(
+                    "lvl1/lvl2/pbmc3k_l2.h5ad",
+                    name="pbmc3k_l2.h5ad",
+                    type=ItemType.h5ad,
+                    annotations=None,
+                )
+            ],
+        )
+        self.assertEqual(lvl2.branches, None)
+
+
+class TestListItems(unittest.TestCase):
+    def test_GIVEN_filter_THEN_pass_filter_into_scan_directory(self):
+        source = S3ItemSource("my-bucket")
+        source.scan_directory = MagicMock()
+        tree = source.list_items("some-filter")
+        source.scan_directory.assert_called_once_with("some-filter")
+
+    def test_GIVEN_no_filter_THEN_pass_empty_string_into_scan_directory(self):
+        source = S3ItemSource("my-bucket")
+        source.scan_directory = MagicMock()
+        tree = source.list_items()
+        source.scan_directory.assert_called_once_with("")

tests/test_extra_scripts.py

@@ -21,6 +21,15 @@ class TestExtraScripts(unittest.TestCase):
     def test_GIVEN_empty_string_THEN_returns_empty_array(self):
         self.assertEqual(get_extra_scripts(), [])
 
+    @patch("cellxgene_gateway.env.extra_scripts", new="'asdf'")
+    def test_GIVEN_bare_string_THEN_throws_Exception(self):
+        with self.assertRaises(Exception) as context:
+            self.assertEqual(get_extra_scripts(), [])
+        self.assertEqual(
+            'Error parsing GATEWAY_EXTRA_SCRIPTS, expected JSON array e.g. ["https://example.com/path/to/script.js"]',
+            str(context.exception),
+        )
+
 
 if __name__ == "__main__":
     unittest.main()

tests/test_filecrawl.py

@@ -1,10 +1,10 @@
 import unittest
 from unittest.mock import MagicMock, patch
 
-from cellxgene_gateway.filecrawl import render_item
+from cellxgene_gateway.filecrawl import render_item, render_item_source
 from cellxgene_gateway.items.file.fileitem import FileItem
 from cellxgene_gateway.items.file.fileitem_source import FileItemSource
-from cellxgene_gateway.items.item import ItemType
+from cellxgene_gateway.items.item import ItemTree, ItemType
 
 source = FileItemSource("/tmp")
 
@@ -29,3 +29,15 @@ class TestRenderEntry(unittest.TestCase):
         entry = FileItem(subpath="somepath", name="entry", type=ItemType.h5ad)
         rendered = render_item(entry, source)
         self.assertIn("view/somepath/entry/'", rendered)
+
+
+class TestRenderItemSource(unittest.TestCase):
+    @patch("cellxgene_gateway.items.file.fileitem_source.FileItemSource")
+    def test_GIVEN_some_filter_THEN_includes_filterpart_in_heading(self, item_source):
+        item_source.name = "FakeSource"
+        item_source.list_items.return_value = ItemTree("rootdir", [], [])
+        rendered = render_item_source(item_source, "some_filter")
+        self.assertEqual(
+            rendered,
+            "<h6><a href='/filecrawl.html?source=FakeSource'>FakeSource</a>:some_filter</h6><li><a href='/filecrawl/rootdir?source=FakeSource'>rootdir</a><ul></ul></li>",
+        )

tests/test_subprocess_backend.py

@@ -0,0 +1,42 @@
+import unittest
+from unittest.mock import MagicMock, patch
+
+from cellxgene_gateway.backend_cache import BackendCache
+from cellxgene_gateway.cache_entry import CacheEntry
+from cellxgene_gateway.cache_key import CacheKey
+from cellxgene_gateway.items.file.fileitem import FileItem
+from cellxgene_gateway.items.file.fileitem_source import FileItemSource
+from cellxgene_gateway.items.item import ItemType
+from cellxgene_gateway.process_exception import ProcessException
+
+
+class TestSubprocessBackend(unittest.TestCase):
+    @patch("subprocess.Popen")
+    def test_launch_GIVEN_no_stdout_THEN_throw_ProcessException(self, popen):
+        subprocess = MagicMock()
+        subprocess.stdout.readline().decode.return_value = ""
+        subprocess.stderr.read().decode.return_value = "An unexpected error"
+        popen.return_value = subprocess
+
+        key = CacheKey(
+            FileItem("/czi/", name="pbmc3k.h5ad", type=ItemType.h5ad),
+            FileItemSource("/tmp", "local"),
+        )
+        entry = CacheEntry.for_key(key, 8000)
+        from cellxgene_gateway.subprocess_backend import SubprocessBackend
+
+        backend = SubprocessBackend()
+        cellxgene_loc = "/some/cellxgene"
+        scripts = ["http://example.com/script.js", "http://example.com/script2.js"]
+
+        with self.assertRaises(ProcessException) as context:
+            backend.launch(cellxgene_loc, scripts, entry)
+        popen.assert_called_once_with(
+            [
+                "yes | /some/cellxgene launch /tmp/czi/pbmc3k.h5ad --port 8000 --host 127.0.0.1 --disable-annotations --scripts http://example.com/script.js --scripts http://example.com/script2.js"
+            ],
+            shell=True,
+            stderr=-1,
+            stdout=-1,
+        )
+        self.assertEqual("An unexpected error", context.exception.stderr)