Vidartf fix prefdir - validation async snafu

jupyter_server/serverapp.py

@@ -1617,71 +1617,29 @@ def _normalize_dir(self, value):
             value = os.path.abspath(value)
         return value
 
-    # Because the validation of preferred_dir depends on root_dir and validation
-    # occurs when the trait is loaded, there are times when we should defer the
-    # validation of preferred_dir (e.g., when preferred_dir is defined via CLI
-    # and root_dir is defined via a config file).
-    _defer_preferred_dir_validation = False
-
     @validate("root_dir")
     def _root_dir_validate(self, proposal):
         value = self._normalize_dir(proposal["value"])
         if not os.path.isdir(value):
             raise TraitError(trans.gettext("No such directory: '%r'") % value)
-
-        if self._defer_preferred_dir_validation:
-            # If we're here, then preferred_dir is configured on the CLI and
-            # root_dir is configured in a file
-            self._preferred_dir_validation(self.preferred_dir, value)
         return value
 
     preferred_dir = Unicode(
+        None,
+        allow_none=True,
         config=True,
         help=trans.gettext("Preferred starting directory to use for notebooks and kernels."),
     )
 
-    @default("preferred_dir")
-    def _default_prefered_dir(self):
-        return self.root_dir
-
     @validate("preferred_dir")
     def _preferred_dir_validate(self, proposal):
+        if proposal["value"] is None:
+            return None
         value = self._normalize_dir(proposal["value"])
         if not os.path.isdir(value):
             raise TraitError(trans.gettext("No such preferred dir: '%r'") % value)
-
-        # Before we validate against root_dir, check if this trait is defined on the CLI
-        # and root_dir is not.  If that's the case, we'll defer it's further validation
-        # until root_dir is validated or the server is starting (the latter occurs when
-        # the default root_dir (cwd) is used).
-        cli_config = self.cli_config.get("ServerApp", {})
-        if "preferred_dir" in cli_config and "root_dir" not in cli_config:
-            self._defer_preferred_dir_validation = True
-
-        if not self._defer_preferred_dir_validation:  # Validate now
-            self._preferred_dir_validation(value, self.root_dir)
         return value
 
-    def _preferred_dir_validation(self, preferred_dir: str, root_dir: str) -> None:
-        """Validate preferred dir relative to root_dir - preferred_dir must be equal or a subdir of root_dir"""
-        if not preferred_dir.startswith(root_dir):
-            raise TraitError(
-                trans.gettext(
-                    "preferred_dir must be equal or a subdir of root_dir. preferred_dir: '%r' root_dir: '%r'"
-                )
-                % (preferred_dir, root_dir)
-            )
-        self._defer_preferred_dir_validation = False
-
-    @observe("root_dir")
-    def _root_dir_changed(self, change):
-        self._root_dir_set = True
-        if not self.preferred_dir.startswith(change["new"]):
-            self.log.warning(
-                trans.gettext("Value of preferred_dir updated to use value of root_dir")
-            )
-            self.preferred_dir = change["new"]
-
     @observe("server_extensions")
     def _update_server_extensions(self, change):
         self.log.warning(_i18n("server_extensions is deprecated, use jpserver_extensions"))
@@ -1865,6 +1823,9 @@ def init_configurables(self):
             parent=self,
             log=self.log,
         )
+        # Trigger a default/validation here explicitly while we still supporte
+        # deprecated trait on ServerApp (FIXME remove when deprecation finalized)
+        self.contents_manager.preferred_dir
         self.session_manager = self.session_manager_class(
             parent=self,
             log=self.log,
@@ -2482,10 +2443,6 @@ def initialize(
         # Parse command line, load ServerApp config files,
         # and update ServerApp config.
         super().initialize(argv=argv)
-        if self._defer_preferred_dir_validation:
-            # If we're here, then preferred_dir is configured on the CLI and
-            # root_dir has the default value (cwd)
-            self._preferred_dir_validation(self.preferred_dir, self.root_dir)
         if self._dispatching:
             return
         # initialize io loop as early as possible,

jupyter_server/services/contents/fileio.py

@@ -254,6 +254,9 @@ def _get_os_path(self, path):
         404: if path is outside root
         """
         root = os.path.abspath(self.root_dir)
+        # to_os_path is not safe if path starts with a drive, since os.path.join discards first part
+        if os.path.splitdrive(path)[0]:
+            raise HTTPError(404, "%s is not a relative API path" % path)
         os_path = to_os_path(path, root)
         if not (os.path.abspath(os_path) + os.path.sep).startswith(root):
             raise HTTPError(404, "%s is outside root contents directory" % path)

jupyter_server/services/contents/filemanager.py

@@ -7,6 +7,7 @@
 import shutil
 import stat
 import sys
+import warnings
 from datetime import datetime
 
 import nbformat
@@ -55,6 +56,24 @@ def _validate_root_dir(self, proposal):
             raise TraitError("%r is not a directory" % value)
         return value
 
+    @default("preferred_dir")
+    def _default_preferred_dir(self):
+        try:
+            value = self.parent.preferred_dir
+        except AttributeError:
+            pass
+        else:
+            if value is not None:
+                warnings.warn(
+                    "ServerApp.preferred_dir config is deprecated in jupyter-server 2.0. Use ContentsManager.preferred_dir with a relative path instead",
+                    FutureWarning,
+                    stacklevel=3,
+                )
+                if not (value + os.path.sep).startswith(self.root_dir):
+                    raise TraitError("%s is outside root contents directory" % value)
+                return os.path.relpath(value, self.root_dir).replace(os.path.sep, "/")
+        return "/"
+
     @default("checkpoints_class")
     def _checkpoints_class_default(self):
         return FileCheckpoints

jupyter_server/services/contents/manager.py

@@ -75,10 +75,27 @@ def emit(self, data):
 
     root_dir = Unicode("/", config=True)
 
+    preferred_dir = Unicode(
+        "/",
+        config=True,
+        help=_i18n(
+            "Preferred starting directory to use for notebooks as a path local to `root_dir`."
+        ),
+    )
+
+    @validate("preferred_dir")
+    def _validate_preferred_dir(self, proposal):
+        value = proposal["value"]
+
+        if not value.startswith(self.root_dir):
+            raise TraitError(_i18n(f"{value} is outside root contents directory"))
+        return value
+
     allow_hidden = Bool(False, config=True, help="Allow access to hidden files")
 
     notary = Instance(sign.NotebookNotary)
 
+    @default("notary")
     def _notary_default(self):
         return sign.NotebookNotary(parent=self)
 

tests/test_gateway.py

@@ -421,7 +421,7 @@ async def test_gateway_session_lifecycle(init_gateway, jp_root_dir, jp_fetch):
     # Validate session lifecycle functions; create and delete.
 
     # create
-    session_id, kernel_id = await create_session(jp_root_dir, jp_fetch, "kspec_foo")
+    session_id, kernel_id = await create_session(jp_fetch, "kspec_foo")
 
     # ensure kernel still considered running
     assert await is_kernel_running(jp_fetch, kernel_id) is True
@@ -567,12 +567,12 @@ async def test_channel_queue_get_msg_when_response_router_had_finished():
 #
 # Test methods below...
 #
-async def create_session(root_dir, jp_fetch, kernel_name):
+async def create_session(jp_fetch, kernel_name):
     """Creates a session for a kernel.  The session is created against the server
     which then uses the gateway for kernel management.
     """
     with mocked_gateway:
-        nb_path = root_dir / "testgw.ipynb"
+        nb_path = "/testgw.ipynb"
         body = json.dumps(
             {"path": str(nb_path), "type": "notebook", "kernel": {"name": kernel_name}}
         )

tests/test_serverapp.py

@@ -6,6 +6,7 @@
 
 import pytest
 from jupyter_core.application import NoStart
+from tornado.web import HTTPError
 from traitlets import TraitError
 from traitlets.tests.utils import check_help_all_output
 
@@ -256,6 +257,7 @@ def test_urls(config, public_url, local_url, connection_url):
 
 # Preferred dir tests
 # ----------------------------------------------------------------------------
+@pytest.mark.filterwarnings("ignore::FutureWarning")
 def test_valid_preferred_dir(tmp_path, jp_configurable_serverapp):
     path = str(tmp_path)
     app = jp_configurable_serverapp(root_dir=path, preferred_dir=path)
@@ -264,6 +266,7 @@ def test_valid_preferred_dir(tmp_path, jp_configurable_serverapp):
     assert app.root_dir == app.preferred_dir
 
 
+@pytest.mark.filterwarnings("ignore::FutureWarning")
 def test_valid_preferred_dir_is_root_subdir(tmp_path, jp_configurable_serverapp):
     path = str(tmp_path)
     path_subdir = str(tmp_path / "subdir")
@@ -283,6 +286,8 @@ def test_valid_preferred_dir_does_not_exist(tmp_path, jp_configurable_serverapp)
     assert "No such preferred dir:" in str(error)
 
 
+# This tests some deprecated behavior as well
+@pytest.mark.filterwarnings("ignore::FutureWarning")
 @pytest.mark.parametrize(
     "root_dir_loc,preferred_dir_loc",
     [
@@ -338,8 +343,11 @@ def test_preferred_dir_validation(
     else:
         app = jp_configurable_serverapp(**kwargs)
         assert app.root_dir == expected_root_dir
-        assert app.preferred_dir == expected_preferred_dir
-        assert app.preferred_dir.startswith(app.root_dir)
+        rel = os.path.relpath(expected_preferred_dir, expected_root_dir)
+        if rel == ".":
+            rel = "/"
+        assert app.contents_manager.preferred_dir == rel
+        assert ".." not in rel
 
 
 def test_invalid_preferred_dir_does_not_exist(tmp_path, jp_configurable_serverapp):
@@ -362,42 +370,35 @@ def test_invalid_preferred_dir_does_not_exist_set(tmp_path, jp_configurable_serv
     assert "No such preferred dir:" in str(error)
 
 
+@pytest.mark.filterwarnings("ignore::FutureWarning")
 def test_invalid_preferred_dir_not_root_subdir(tmp_path, jp_configurable_serverapp):
     path = str(tmp_path / "subdir")
     os.makedirs(path, exist_ok=True)
     not_subdir_path = str(tmp_path)
 
-    with pytest.raises(TraitError) as error:
-        app = jp_configurable_serverapp(root_dir=path, preferred_dir=not_subdir_path)
-
-    assert "preferred_dir must be equal or a subdir of root_dir. " in str(error)
+    with pytest.raises(SystemExit):
+        jp_configurable_serverapp(root_dir=path, preferred_dir=not_subdir_path)
 
 
-def test_invalid_preferred_dir_not_root_subdir_set(tmp_path, jp_configurable_serverapp):
+async def test_invalid_preferred_dir_not_root_subdir_set(tmp_path, jp_configurable_serverapp):
     path = str(tmp_path / "subdir")
     os.makedirs(path, exist_ok=True)
-    not_subdir_path = str(tmp_path)
+    not_subdir_path = os.path.relpath(tmp_path, path)
 
     app = jp_configurable_serverapp(root_dir=path)
     with pytest.raises(TraitError) as error:
-        app.preferred_dir = not_subdir_path
+        app.contents_manager.preferred_dir = not_subdir_path
 
-    assert "preferred_dir must be equal or a subdir of root_dir. " in str(error)
+    assert "is outside root contents directory" in str(error.value)
 
 
-def test_observed_root_dir_updates_preferred_dir(tmp_path, jp_configurable_serverapp):
-    path = str(tmp_path)
-    new_path = str(tmp_path / "subdir")
-    os.makedirs(new_path, exist_ok=True)
-
-    app = jp_configurable_serverapp(root_dir=path, preferred_dir=path)
-    app.root_dir = new_path
-    assert app.preferred_dir == new_path
+async def test_absolute_preferred_dir_not_root_subdir_set(tmp_path, jp_configurable_serverapp):
+    path = str(tmp_path / "subdir")
+    os.makedirs(path, exist_ok=True)
+    not_subdir_path = str(tmp_path)
 
+    app = jp_configurable_serverapp(root_dir=path)
+    with pytest.raises(TraitError) as error:
+        app.contents_manager.preferred_dir = not_subdir_path
 
-def test_observed_root_dir_does_not_update_preferred_dir(tmp_path, jp_configurable_serverapp):
-    path = str(tmp_path)
-    new_path = str(tmp_path.parent)
-    app = jp_configurable_serverapp(root_dir=path, preferred_dir=path)
-    app.root_dir = new_path
-    assert app.preferred_dir == path
+    assert "is outside root contents directory" in str(error.value)

tests/test_terminal.py

@@ -276,4 +276,10 @@ def test_shell_command_override(
     pytest.importorskip("traitlets", minversion=min_traitlets)
     argv = shlex.split(f"--ServerApp.terminado_settings={terminado_settings}")
     app = jp_configurable_serverapp(argv=argv)
-    assert app.web_app.settings["terminal_manager"].shell_command == expected_shell
+    if os.name == "nt":
+        assert app.web_app.settings["terminal_manager"].shell_command in (
+            expected_shell,
+            " ".join(expected_shell),
+        )
+    else:
+        assert app.web_app.settings["terminal_manager"].shell_command == expected_shell