Backport PR #826 on branch 1.x (Defer preferred_dir validation until root_dir is set) (#828)

Co-authored-by: Kevin Bates <[email protected]>

Author: meeseeksmachine

jupyter_server/pytest_plugin.py

@@ -238,13 +238,18 @@ def _configurable_serverapp(
         c = Config(config)
         c.NotebookNotary.db_file = ":memory:"
         token = hexlify(os.urandom(4)).decode("ascii")
+
+        # Allow tests to configure root_dir via a file, argv, or its
+        # default (cwd) by specifying a value of None.
+        if root_dir is not None:
+            kwargs["root_dir"] = str(root_dir)
+
         app = ServerApp.instance(
             # Set the log level to debug for testing purposes
             log_level="DEBUG",
             port=http_port,
             port_retries=0,
             open_browser=False,
-            root_dir=str(root_dir),
             base_url=base_url,
             config=c,
             allow_root=True,

jupyter_server/serverapp.py

@@ -1593,11 +1593,22 @@ 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(
@@ -1615,16 +1626,28 @@ def _preferred_dir_validate(self, proposal):
         if not os.path.isdir(value):
             raise TraitError(trans.gettext("No such preferred dir: '%r'") % value)
 
-        # preferred_dir must be equal or a subdir of root_dir
-        if not value.startswith(self.root_dir):
+        # 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'"
                 )
-                % (value, self.root_dir)
+                % (preferred_dir, root_dir)
             )
-
-        return value
+        self._defer_preferred_dir_validation = False
 
     @observe("root_dir")
     def _root_dir_changed(self, change):
@@ -2387,6 +2410,10 @@ 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
         # Then, use extensions' config loading mechanism to

tests/test_serverapp.py

@@ -281,6 +281,65 @@ def test_valid_preferred_dir_does_not_exist(tmp_path, jp_configurable_serverapp)
     assert "No such preferred dir:" in str(error)
 
 
+@pytest.mark.parametrize(
+    "root_dir_loc,preferred_dir_loc",
+    [
+        ("cli", "cli"),
+        ("cli", "config"),
+        ("cli", "default"),
+        ("config", "cli"),
+        ("config", "config"),
+        ("config", "default"),
+        ("default", "cli"),
+        ("default", "config"),
+        ("default", "default"),
+    ],
+)
+def test_preferred_dir_validation(
+    root_dir_loc, preferred_dir_loc, tmp_path, jp_config_dir, jp_configurable_serverapp
+):
+    expected_root_dir = str(tmp_path)
+    expected_preferred_dir = str(tmp_path / "subdir")
+    os.makedirs(expected_preferred_dir, exist_ok=True)
+
+    argv = []
+    kwargs = {"root_dir": None}
+
+    config_lines = []
+    config_file = None
+    if root_dir_loc == "config" or preferred_dir_loc == "config":
+        config_file = jp_config_dir.joinpath("jupyter_server_config.py")
+
+    if root_dir_loc == "cli":
+        argv.append(f"--ServerApp.root_dir={expected_root_dir}")
+    if root_dir_loc == "config":
+        config_lines.append(f'c.ServerApp.root_dir = r"{expected_root_dir}"')
+    if root_dir_loc == "default":
+        expected_root_dir = os.getcwd()
+
+    if preferred_dir_loc == "cli":
+        argv.append(f"--ServerApp.preferred_dir={expected_preferred_dir}")
+    if preferred_dir_loc == "config":
+        config_lines.append(f'c.ServerApp.preferred_dir = r"{expected_preferred_dir}"')
+    if preferred_dir_loc == "default":
+        expected_preferred_dir = expected_root_dir
+
+    if config_file is not None:
+        config_file.write_text("\n".join(config_lines))
+
+    if argv:
+        kwargs["argv"] = argv
+
+    if root_dir_loc == "default" and preferred_dir_loc != "default":  # error expected
+        with pytest.raises(SystemExit):
+            jp_configurable_serverapp(**kwargs)
+    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)
+
+
 def test_invalid_preferred_dir_does_not_exist(tmp_path, jp_configurable_serverapp):
     path = str(tmp_path)
     path_subdir = str(tmp_path / "subdir")