From 1200629d747d87d060aacccf7f14d840209665ae Mon Sep 17 00:00:00 2001 From: Alok Saldanha Date: Tue, 20 Apr 2021 05:40:53 -0400 Subject: [PATCH 1/7] #45 handle keys as full subpath rather than last path element --- cellxgene_gateway/items/s3/s3item_source.py | 28 ++++++++++----------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/cellxgene_gateway/items/s3/s3item_source.py b/cellxgene_gateway/items/s3/s3item_source.py index 5cf7826..030c28e 100644 --- a/cellxgene_gateway/items/s3/s3item_source.py +++ b/cellxgene_gateway/items/s3/s3item_source.py @@ -38,8 +38,8 @@ class S3ItemSource(ItemSource): self.annotation_dir_suffix = annotation_dir_suffix self.annotation_file_suffix = annotation_file_suffix - def url(self, path): - return "s3://" + join(self.bucket, path) + def url(self, key): + return "s3://" + self.bucket + "/" + key @property def name(self): @@ -64,8 +64,8 @@ class S3ItemSource(ItemSource): item_tree = self.scan_directory() return item_tree - def scan_directory(self, subpath="") -> dict: - url = self.url(subpath) + def scan_directory(self, directory_key="") -> dict: + url = self.url(directory_key) if not self.s3.exists(url): raise Exception(f"S3 url '{url}' does not exist.") @@ -78,32 +78,32 @@ class S3ItemSource(ItemSource): 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 = [ - self.make_s3item_from_key(filename, join(subpath, filename)) - for filename in h5ad_paths + self.make_s3item_from_key( + key[key.rindex("/") + 1 :] if "/" in key else key, key + ) + for key in h5ad_keys ] branches = None - if len(subdirs) > 0: - branches = [ - self.scan_directory(join(subpath, subdir)) for subdir in subdirs - ] + if len(subdir_keys) > 0: + branches = [self.scan_directory(key) for key in subdir_keys] - 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( From d07203491118deab0c79db098041d5fe6ad51e0a Mon Sep 17 00:00:00 2001 From: Alok Saldanha Date: Tue, 20 Apr 2021 06:03:16 -0400 Subject: [PATCH 2/7] #45 pass filter into scan_directory also include filterpart in heading --- cellxgene_gateway/filecrawl.py | 3 ++- cellxgene_gateway/items/s3/s3item_source.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/cellxgene_gateway/filecrawl.py b/cellxgene_gateway/filecrawl.py index 8a62818..95dc471 100644 --- a/cellxgene_gateway/filecrawl.py +++ b/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"
{item_source.name}
" + filterpart = "" if filter is None else ":" + filter + heading = f"
{item_source.name}{filterpart}
" return heading + render_item_tree(item_tree, item_source) diff --git a/cellxgene_gateway/items/s3/s3item_source.py b/cellxgene_gateway/items/s3/s3item_source.py index 030c28e..7a93a95 100644 --- a/cellxgene_gateway/items/s3/s3item_source.py +++ b/cellxgene_gateway/items/s3/s3item_source.py @@ -61,7 +61,7 @@ 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, directory_key="") -> dict: From ccf1ba58a87b983798ebd899025e8823e613c88f Mon Sep 17 00:00:00 2001 From: Alok Saldanha Date: Tue, 20 Apr 2021 13:17:37 -0400 Subject: [PATCH 3/7] #45 add extra_scripts to cache_status page --- cellxgene_gateway/gateway.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cellxgene_gateway/gateway.py b/cellxgene_gateway/gateway.py index 347f35e..56620fb 100644 --- a/cellxgene_gateway/gateway.py +++ b/cellxgene_gateway/gateway.py @@ -217,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"]) From 68da42ae0ea43130060d53868b046a389abd09e9 Mon Sep 17 00:00:00 2001 From: Alok Saldanha Date: Tue, 20 Apr 2021 13:32:28 -0400 Subject: [PATCH 4/7] #45 implemented test_GIVEN_some_filter_THEN_includes_filterpart_in_heading --- tests/test_filecrawl.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/test_filecrawl.py b/tests/test_filecrawl.py index 317b558..1e3b768 100644 --- a/tests/test_filecrawl.py +++ b/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, + "
FakeSource:some_filter
  • rootdir
      • ", + ) From 687dc31b7a0c132f8d2ee80424381ad52c570be5 Mon Sep 17 00:00:00 2001 From: Alok Saldanha Date: Wed, 21 Apr 2021 06:21:26 -0400 Subject: [PATCH 5/7] improved error message on invalid extra scripts --- cellxgene_gateway/extra_scripts.py | 8 +++++++- tests/test_extra_scripts.py | 9 +++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/cellxgene_gateway/extra_scripts.py b/cellxgene_gateway/extra_scripts.py index abf2218..ac05676 100644 --- a/cellxgene_gateway/extra_scripts.py +++ b/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 diff --git a/tests/test_extra_scripts.py b/tests/test_extra_scripts.py index c429c55..97020ed 100644 --- a/tests/test_extra_scripts.py +++ b/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() From a3e3b6cea8b82a0f8beaf526cb68f4691c5a7ec1 Mon Sep 17 00:00:00 2001 From: Alok Saldanha Date: Wed, 21 Apr 2021 08:21:46 -0400 Subject: [PATCH 6/7] #45 implemented test__scan_directory__properly_recurses_suburls --- cellxgene_gateway/items/s3/s3item_source.py | 16 +- tests/items/s3/__init__.py | 0 tests/items/s3/test_s3item_source.py | 158 ++++++++++++++++++++ 3 files changed, 165 insertions(+), 9 deletions(-) create mode 100644 tests/items/s3/__init__.py create mode 100644 tests/items/s3/test_s3item_source.py diff --git a/cellxgene_gateway/items/s3/s3item_source.py b/cellxgene_gateway/items/s3/s3item_source.py index 7a93a95..6f1ab26 100644 --- a/cellxgene_gateway/items/s3/s3item_source.py +++ b/cellxgene_gateway/items/s3/s3item_source.py @@ -41,6 +41,9 @@ class S3ItemSource(ItemSource): def url(self, key): return "s3://" + self.bucket + "/" + key + def remove_bucket(self, filepath): + return filepath[len(self.bucket) :].lstrip("/") + @property def name(self): return self._name or f"Items:{self.url('')}" @@ -71,7 +74,7 @@ class S3ItemSource(ItemSource): 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)) ) @@ -93,12 +96,7 @@ class S3ItemSource(ItemSource): if self.s3.isdir(item_url) and not is_annotation_dir(filepath) ] - items = [ - self.make_s3item_from_key( - key[key.rindex("/") + 1 :] if "/" in key else key, key - ) - for key in h5ad_keys - ] + items = [self.make_s3item_from_key(basename(key), key) for key in h5ad_keys] branches = None if len(subdir_keys) > 0: branches = [self.scan_directory(key) for key in subdir_keys] @@ -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 diff --git a/tests/items/s3/__init__.py b/tests/items/s3/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/items/s3/test_s3item_source.py b/tests/items/s3/test_s3item_source.py new file mode 100644 index 0000000..7b20adf --- /dev/null +++ b/tests/items/s3/test_s3item_source.py @@ -0,0 +1,158 @@ +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__scan_directory__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) + + def test__list_items__pass_filter_into_scan_directory(self): + pass From c29e5c0d922a79a98d43baefa8ac1be560972644 Mon Sep 17 00:00:00 2001 From: Alok Saldanha Date: Thu, 22 Apr 2021 06:55:45 -0400 Subject: [PATCH 7/7] #45 implemented test__list_items__pass_filter_into_scan_directory --- tests/items/s3/test_s3item_source.py | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/tests/items/s3/test_s3item_source.py b/tests/items/s3/test_s3item_source.py index 7b20adf..10b62ea 100644 --- a/tests/items/s3/test_s3item_source.py +++ b/tests/items/s3/test_s3item_source.py @@ -24,7 +24,7 @@ class TestScanDirectory(unittest.TestCase): ) @patch("s3fs.S3FileSystem") - def test__scan_directory__properly_recurses_suburls(self, s3func): + def test__GIVEN_multilevel_bucket_THEN_properly_recurses_suburls(self, s3func): class S3Mock: def exists(path): if path in [ @@ -154,5 +154,16 @@ class TestScanDirectory(unittest.TestCase): ) self.assertEqual(lvl2.branches, None) - def test__list_items__pass_filter_into_scan_directory(self): - pass + +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("")