Merge pull request #201 from painebot/tkp/cm

revert to synchronous contents manager, add separate async contents manager

jupyterfs/extension.py

@@ -10,7 +10,7 @@
 from jupyter_server.utils import url_path_join
 
 from ._version import __version__  # noqa: F401
-from .metamanager import MetaManager, MetaManagerHandler
+from .metamanager import MetaManagerShared, MetaManager, MetaManagerHandler
 from .snippets import SnippetsHandler
 
 _mm_config_warning_msg = """Misconfiguration of MetaManager. Please add:
@@ -37,11 +37,11 @@ def _load_jupyter_server_extension(serverapp):
     base_url = web_app.settings["base_url"]
     host_pattern = ".*$"
 
-    if not isinstance(serverapp.contents_manager, MetaManager):
+    if not isinstance(serverapp.contents_manager, MetaManagerShared):
         warnings.warn(_mm_config_warning_msg)
         return
 
-    if isinstance(serverapp.contents_manager_class, type) and not issubclass(serverapp.contents_manager_class, MetaManager):
+    if isinstance(serverapp.contents_manager_class, type) and not issubclass(serverapp.contents_manager_class, MetaManagerShared):
         serverapp.contents_manager_class = MetaManager
         serverapp.log.info("Configuring jupyter-fs manager as the content manager class")
 

jupyterfs/metamanager.py

@@ -15,7 +15,10 @@
 from fs.errors import FSError
 from fs.opener.errors import OpenerError, ParseError
 from jupyter_server.base.handlers import APIHandler
-from jupyter_server.services.contents.manager import AsyncContentsManager
+from jupyter_server.services.contents.manager import (
+    AsyncContentsManager,
+    ContentsManager,
+)
 
 from .auth import substituteAsk, substituteEnv, substituteNone
 from .config import JupyterFs as JupyterFsConfig
@@ -27,10 +30,10 @@
     path_old_new,
 )
 
-__all__ = ["MetaManager", "MetaManagerHandler"]
+__all__ = ["MetaManager", "SyncMetaManager", "MetaManagerHandler"]
 
 
-class MetaManager(AsyncContentsManager):
+class MetaManagerShared:
     copy_pat = re.compile(r"\-Copy\d*\.")
 
     @default("files_handler_params")
@@ -144,31 +147,49 @@ def root_manager(self):
     def root_dir(self):
         return self.root_manager.root_dir
 
-    is_hidden = path_first_arg("is_hidden", False)
-    dir_exists = path_first_arg("dir_exists", False)
-    file_exists = path_kwarg("file_exists", "", False)
-    exists = path_first_arg("exists", False)
+    is_hidden = path_first_arg("is_hidden", False, sync=True)
+    dir_exists = path_first_arg("dir_exists", False, sync=True)
+    file_exists = path_kwarg("file_exists", "", False, sync=True)
+    exists = path_first_arg("exists", False, sync=False)
+
+    save = path_second_arg("save", "model", True, sync=True)
+    rename = path_old_new("rename", False, sync=True)
+
+    get = path_first_arg("get", True, sync=True)
+    delete = path_first_arg("delete", False, sync=True)
+
+    get_kernel_path = path_first_arg("get_kernel_path", False, sync=True)
+
+    create_checkpoint = path_first_arg("create_checkpoint", False, sync=True)
+    list_checkpoints = path_first_arg("list_checkpoints", False, sync=True)
+    restore_checkpoint = path_second_arg("restore_checkpoint", "checkpoint_id", False, sync=True)
+    delete_checkpoint = path_second_arg("delete_checkpoint", "checkpoint_id", False, sync=True)
+
+
+class SyncMetaManager(MetaManagerShared, ContentsManager): ...
+
+
+class MetaManager(MetaManagerShared, AsyncContentsManager):
+    async def copy(self, from_path, to_path=None):
+        return super(MetaManagerShared, self).copy(from_path=from_path, to_path=to_path)
+
+    is_hidden = path_first_arg("is_hidden", False, sync=False)
+    dir_exists = path_first_arg("dir_exists", False, sync=False)
+    file_exists = path_kwarg("file_exists", "", False, sync=False)
+    exists = path_first_arg("exists", False, sync=False)
 
-    save = path_second_arg("save", "model", True)
-    rename = path_old_new("rename", False)
+    save = path_second_arg("save", "model", True, sync=False)
+    rename = path_old_new("rename", False, sync=False)
 
-    get = path_first_arg("get", True)
-    delete = path_first_arg("delete", False)
+    get = path_first_arg("get", True, sync=False)
+    delete = path_first_arg("delete", False, sync=False)
 
     get_kernel_path = path_first_arg("get_kernel_path", False, sync=True)
 
-    create_checkpoint = path_first_arg("create_checkpoint", False)
-    list_checkpoints = path_first_arg("list_checkpoints", False)
-    restore_checkpoint = path_second_arg(
-        "restore_checkpoint",
-        "checkpoint_id",
-        False,
-    )
-    delete_checkpoint = path_second_arg(
-        "delete_checkpoint",
-        "checkpoint_id",
-        False,
-    )
+    create_checkpoint = path_first_arg("create_checkpoint", False, sync=False)
+    list_checkpoints = path_first_arg("list_checkpoints", False, sync=False)
+    restore_checkpoint = path_second_arg("restore_checkpoint", "checkpoint_id", False, sync=False)
+    delete_checkpoint = path_second_arg("delete_checkpoint", "checkpoint_id", False, sync=False)
 
 
 class MetaManagerHandler(APIHandler):

jupyterfs/pathutils.py

@@ -89,61 +89,69 @@ def path_first_arg(method_name, returns_model, sync=False):
     """Decorator for methods that accept path as a first argument,
     e.g. manager.get(path, ...)"""
 
-    if sync:
-
-        def _wrapper(self, *args, **kwargs):
-            path, args = _get_arg("path", args, kwargs)
-            _, mgr, mgr_path = _resolve_path(path, self._managers)
-            result = getattr(mgr, method_name)(mgr_path, *args, **kwargs)
-            return result
+    def _wrapper(self, *args, **kwargs):
+        path, args = _get_arg("path", args, kwargs)
+        _, mgr, mgr_path = _resolve_path(path, self._managers)
+        result = getattr(mgr, method_name)(mgr_path, *args, **kwargs)
+        return result
 
-    else:
+    if sync:
+        return _wrapper
 
-        async def _wrapper(self, *args, **kwargs):
-            path, args = _get_arg("path", args, kwargs)
-            _, mgr, mgr_path = _resolve_path(path, self._managers)
-            result = getattr(mgr, method_name)(mgr_path, *args, **kwargs)
-            return result
+    async def _wrapper2(self, *args, **kwargs):
+        return _wrapper(self, *args, **kwargs)
 
-    return _wrapper
+    return _wrapper2
 
 
-def path_second_arg(method_name, first_argname, returns_model):
+def path_second_arg(method_name, first_argname, returns_model, sync=False):
     """Decorator for methods that accept path as a second argument.
     e.g. manager.save(model, path, ...)"""
 
-    async def _wrapper(self, *args, **kwargs):
+    def _wrapper(self, *args, **kwargs):
         other, args = _get_arg(first_argname, args, kwargs)
         path, args = _get_arg("path", args, kwargs)
         _, mgr, mgr_path = _resolve_path(path, self._managers)
         result = getattr(mgr, method_name)(other, mgr_path, *args, **kwargs)
         return result
 
-    return _wrapper
+    if sync:
+        return _wrapper
+
+    async def _wrapper2(self, *args, **kwargs):
+        return _wrapper(self, *args, **kwargs)
+
+    return _wrapper2
 
 
-def path_kwarg(method_name, path_default, returns_model):
+def path_kwarg(method_name, path_default, returns_model, sync=False):
     """Parameterized decorator for methods that accept path as a second
     argument.
 
     e.g. manager.file_exists(path='')
     """
 
-    async def _wrapper(self, path=path_default, **kwargs):
+    def _wrapper(self, path=path_default, **kwargs):
         _, mgr, mgr_path = _resolve_path(path, self._managers)
         result = getattr(mgr, method_name)(path=mgr_path, **kwargs)
         return result
 
-    return _wrapper
+    if sync:
+        return _wrapper
+
+    async def _wrapper2(self, *args, **kwargs):
+        return _wrapper(self, *args, **kwargs)
 
+    return _wrapper2
 
-def path_old_new(method_name, returns_model):
+
+def path_old_new(method_name, returns_model, sync=False):
     """Decorator for methods accepting old_path and new_path.
 
     e.g. manager.rename(old_path, new_path)
     """
 
-    async def _wrapper(self, old_path, new_path, *args, **kwargs):
+    def _wrapper(self, old_path, new_path, *args, **kwargs):
         old_prefix, old_mgr, old_mgr_path = _resolve_path(old_path, self._managers)
         new_prefix, new_mgr, new_mgr_path = _resolve_path(new_path, self._managers)
         if old_mgr is not new_mgr:
@@ -159,7 +167,13 @@ async def _wrapper(self, old_path, new_path, *args, **kwargs):
         result = getattr(new_mgr, method_name)(old_mgr_path, new_mgr_path, *args, **kwargs)
         return result
 
-    return _wrapper
+    if sync:
+        return _wrapper
+
+    async def _wrapper2(self, *args, **kwargs):
+        return _wrapper(self, *args, **kwargs)
+
+    return _wrapper2
 
 
 # handlers for drive specifications in path strings, as in "fooDrive:bar/baz.buzz"

jupyterfs/tests/test_metamanager.py

@@ -20,6 +20,14 @@
     "JupyterFs": {},
 }
 
+sync_base_config = {
+    "ServerApp": {
+        "jpserver_extensions": {"jupyterfs.extension": True},
+        "contents_manager_class": "jupyterfs.metamanager.SyncMetaManager",
+    },
+    "JupyterFs": {},
+}
+
 deny_client_config = {
     "JupyterFs": {
         "allow_user_resources": False,
@@ -40,7 +48,7 @@ def our_config():
 
 
 @pytest.fixture
-def jp_server_config(tmp_path, tmp_osfs_resource, our_config):
+def jp_server_config(base_config, tmp_path, tmp_osfs_resource, our_config):
     c = Config(base_config)
     c.JupyterFs.setdefault("resources", [])
     if tmp_osfs_resource:
@@ -54,13 +62,15 @@ def jp_server_config(tmp_path, tmp_osfs_resource, our_config):
     return c
 
 
+@pytest.mark.parametrize("base_config", [base_config, sync_base_config])
 @pytest.mark.parametrize("our_config", [deny_client_config])
 async def test_client_creation_disallowed(tmp_path, jp_fetch, jp_server_config):
     cc = ContentsClient(jp_fetch)
     resources = await cc.set_resources([{"name": "test-2", "url": f"osfs://{tmp_path.as_posix()}"}])
     assert resources == []
 
 
+@pytest.mark.parametrize("base_config", [base_config, sync_base_config])
 @pytest.mark.parametrize("our_config", [deny_client_config])
 @pytest.mark.parametrize("tmp_osfs_resource", [True])
 async def test_client_creation_disallowed_retains_server_config(tmp_path, jp_fetch, jp_server_config):
@@ -70,6 +80,7 @@ async def test_client_creation_disallowed_retains_server_config(tmp_path, jp_fet
     assert names == {"test-server-config"}
 
 
+@pytest.mark.parametrize("base_config", [base_config, sync_base_config])
 @pytest.mark.parametrize(
     "our_config",
     [
@@ -118,6 +129,7 @@ async def test_resource_validators(tmp_path, jp_fetch, jp_server_config):
     assert names == {"valid-1", "valid-2"}
 
 
+@pytest.mark.parametrize("base_config", [base_config, sync_base_config])
 @pytest.mark.parametrize(
     "our_config",
     [
@@ -143,3 +155,11 @@ async def test_resource_validators_no_auth(tmp_path, jp_fetch, jp_server_config)
     )
     names = set(map(lambda r: r["name"], resources))
     assert names == {"valid-1", "valid-2"}
+
+
+@pytest.mark.parametrize("base_config", [base_config, sync_base_config])
+@pytest.mark.parametrize("our_config", [{}])
+async def test_basic_sanity_check(tmp_path, jp_fetch, jp_server_config):
+    cc = ContentsClient(jp_fetch)
+    resources = await cc.get("/")
+    assert resources["type"] == "directory"