Move post-save hooks to ContentsManager, add deprecation warnings, ad…

…d hook override warnings

jupyter_server/services/contents/filemanager.py

@@ -11,13 +11,11 @@
 
 import nbformat
 from anyio.to_thread import run_sync
-from ipython_genutils.importstring import import_item
 from jupyter_core.paths import exists
 from jupyter_core.paths import is_file_hidden
 from jupyter_core.paths import is_hidden
 from send2trash import send2trash
 from tornado import web
-from traitlets import Any
 from traitlets import Bool
 from traitlets import default
 from traitlets import TraitError
@@ -54,67 +52,6 @@ def _default_root_dir(self):
         except AttributeError:
             return os.getcwd()
 
-    post_save_hook = Any(
-        None,
-        config=True,
-        allow_none=True,
-        help="""Python callable or importstring thereof
-
-        to be called on the path of a file just saved.
-
-        This can be used to process the file on disk,
-        such as converting the notebook to a script or HTML via nbconvert.
-
-        It will be called as (all arguments passed by keyword)::
-
-            hook(os_path=os_path, model=model, contents_manager=instance)
-
-        - path: the filesystem path to the file just written
-        - model: the model representing the file
-        - contents_manager: this ContentsManager instance
-        """,
-    )
-
-    @validate("post_save_hook")
-    def _validate_post_save_hook(self, proposal):
-        value = proposal["value"]
-        if isinstance(value, str):
-            value = import_item(value)
-        if not callable(value):
-            raise TraitError("post_save_hook must be callable")
-        return value
-
-    def register_post_save_hook(self, hook):
-        if isinstance(hook, str):
-            hook = import_item(hook)
-        if not callable(hook):
-            raise RuntimeError("post_save_hook must be callable")
-        self._post_save_hooks.append(hook)
-
-    def __init__(self, *args, **kwargs):
-        super().__init__(*args, **kwargs)
-        self._post_save_hooks = []
-
-    def run_post_save_hooks(self, model, os_path):
-        """Run the post-save hooks if any, and log errors"""
-        post_save_hooks = self._post_save_hooks
-        if self.post_save_hook:
-            post_save_hooks.append(self.post_save_hook)
-        for post_save_hook in post_save_hooks:
-            try:
-                self.log.debug("Running post-save hook on %s", os_path)
-                post_save_hook(os_path=os_path, model=model, contents_manager=self)
-            except Exception as e:
-                self.log.error(
-                    "Post-save %s hook failed on %s",
-                    post_save_hook.__name__,
-                    os_path,
-                    exc_info=True,
-                )
-                raise web.HTTPError(
-                    500, "Unexpected error while running post hook save: %s" % e
-                ) from e
-
     @validate("root_dir")
     def _validate_root_dir(self, proposal):
         """Do a bit of validation of the root_dir."""

jupyter_server/services/contents/manager.py

@@ -4,6 +4,7 @@
 import itertools
 import json
 import re
+import warnings
 from fnmatch import fnmatch
 
 from ipython_genutils.importstring import import_item
@@ -126,24 +127,104 @@ def _validate_pre_save_hook(self, proposal):
             value = import_item(self.pre_save_hook)
         if not callable(value):
             raise TraitError("pre_save_hook must be callable")
+        if self.pre_save_hook is not None:
+            warnings.warn(
+                f"Overriding existing pre_save_hook ({self.pre_save_hook.__name__}) with a new one ({value.__name__}).",
+                stacklevel=2,
+            )
         return value
 
+    post_save_hook = Any(
+        None,
+        config=True,
+        allow_none=True,
+        help="""Python callable or importstring thereof
+
+        to be called on the path of a file just saved.
+
+        This can be used to process the file on disk,
+        such as converting the notebook to a script or HTML via nbconvert.
+
+        It will be called as (all arguments passed by keyword)::
+
+            hook(os_path=os_path, model=model, contents_manager=instance)
+
+        - path: the filesystem path to the file just written
+        - model: the model representing the file
+        - contents_manager: this ContentsManager instance
+        """,
+    )
+
+    @validate("post_save_hook")
+    def _validate_post_save_hook(self, proposal):
+        value = proposal["value"]
+        if isinstance(value, str):
+            value = import_item(value)
+        if not callable(value):
+            raise TraitError("post_save_hook must be callable")
+        if self.post_save_hook is not None:
+            warnings.warn(
+                f"Overriding existing post_save_hook ({self.post_save_hook.__name__}) with a new one ({value.__name__}).",
+                stacklevel=2,
+            )
+        return value
+
+    def run_pre_save_hook(self, model, path, **kwargs):
+        """Run the pre-save hook if defined, and log errors"""
+        warnings.warn(
+            "run_pre_save_hook is deprecated, use run_pre_save_hooks instead.",
+            DeprecationWarning,
+            stacklevel=2,
+        )
+        if self.pre_save_hook:
+            try:
+                self.log.debug("Running pre-save hook on %s", path)
+                self.pre_save_hook(model=model, path=path, contents_manager=self, **kwargs)
+            except HTTPError:
+                # allow custom HTTPErrors to raise,
+                # rejecting the save with a message.
+                raise
+            except Exception:
+                # unhandled errors don't prevent saving,
+                # which could cause frustrating data loss
+                self.log.error("Pre-save hook failed on %s", path, exc_info=True)
+
+    def run_post_save_hook(self, model, os_path):
+        """Run the post-save hook if defined, and log errors"""
+        warnings.warn(
+            "run_post_save_hook is deprecated, use run_post_save_hooks instead.",
+            DeprecationWarning,
+            stacklevel=2,
+        )
+        if self.post_save_hook:
+            try:
+                self.log.debug("Running post-save hook on %s", os_path)
+                self.post_save_hook(os_path=os_path, model=model, contents_manager=self)
+            except Exception as e:
+                self.log.error("Post-save hook failed o-n %s", os_path, exc_info=True)
+                raise HTTPError(500, "Unexpected error while running post hook save: %s" % e) from e
+
+    _pre_save_hooks = List()
+    _post_save_hooks = List()
+
     def register_pre_save_hook(self, hook):
         if isinstance(hook, str):
             hook = import_item(hook)
         if not callable(hook):
-            raise RuntimeError("pre_save_hook must be callable")
+            raise RuntimeError("hook must be callable")
         self._pre_save_hooks.append(hook)
 
-    def __init__(self, *args, **kwargs):
-        super().__init__(*args, **kwargs)
-        self._pre_save_hooks = []
+    def register_post_save_hook(self, hook):
+        if isinstance(hook, str):
+            hook = import_item(hook)
+        if not callable(hook):
+            raise RuntimeError("hook must be callable")
+        self._post_save_hooks.append(hook)
 
     def run_pre_save_hooks(self, model, path, **kwargs):
-        """Run the pre-save hooks if defined, and log errors"""
-        pre_save_hooks = self._pre_save_hooks
-        if self.pre_save_hook:
-            pre_save_hooks.append(self.pre_save_hook)
+        """Run the pre-save hooks if any, and log errors"""
+        pre_save_hooks = [self.pre_save_hook] if self.pre_save_hook is not None else []
+        pre_save_hooks += self._pre_save_hooks
         for pre_save_hook in pre_save_hooks:
             try:
                 self.log.debug("Running pre-save hook on %s", path)
@@ -162,6 +243,23 @@ def run_pre_save_hooks(self, model, path, **kwargs):
                     exc_info=True,
                 )
 
+    def run_post_save_hooks(self, model, os_path):
+        """Run the post-save hooks if any, and log errors"""
+        post_save_hooks = [self.post_save_hook] if self.post_save_hook is not None else []
+        post_save_hooks += self._post_save_hooks
+        for post_save_hook in post_save_hooks:
+            try:
+                self.log.debug("Running post-save hook on %s", os_path)
+                post_save_hook(os_path=os_path, model=model, contents_manager=self)
+            except Exception as e:
+                self.log.error(
+                    "Post-save %s hook failed on %s",
+                    post_save_hook.__name__,
+                    os_path,
+                    exc_info=True,
+                )
+                raise HTTPError(500, "Unexpected error while running post hook save: %s" % e) from e
+
     checkpoints_class = Type(Checkpoints, config=True)
     checkpoints = Instance(Checkpoints, config=True)
     checkpoints_kwargs = Dict(config=True)