Repository: jupyter-server/jupyter_server

Issue:
Passing pre_save_hook as importstring
## Description

Passing a `pre_save_hook` as an importstring e.g.
```
--FileContentsManager.pre_save_hook='mymodule.foo_function`
```

fails with the following error when launching the Jupyter server:
<details><summary>Troubleshoot Output</summary>
<pre>
    @validate("pre_save_hook")
    def _validate_pre_save_hook(self, proposal):
        value = proposal["value"]
        if isinstance(value, str):
            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,
            )
E           AttributeError: 'str' object has no attribute '__name__'
</pre>
</details>


If you edit `jupyter_server_config.py` to import the function, then setting the value is fine.

## Reproduce

Pass `pre_save_hook` or `post_save_hook` as a importstring.

## Expected behavior

Passing as a string or function works equally well.

## Discussion

The problem seems to be because the code assumes that `self.pre_save_hook` is always a function and has the `__name__` dunder.

Discussion:
welcome[bot]: Thanks for submitting your first pull request! You are awesome! :hugs:
<br>If you haven't done so already, check out [Jupyter's Code of Conduct](https://github.com/jupyter/governance/blob/master/conduct/code_of_conduct.md). Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
![welcome](https://raw.githubusercontent.com/jupyter-server/.github/master/images/welcome.jpg)
You can meet the other [Jovyans](https://jupyter.readthedocs.io/en/latest/community/content-community.html?highlight=jovyan#what-is-a-jovyan) by joining our [Discourse forum](http://discourse.jupyter.org/). There is also a intro thread there where you can stop by and say Hi! :wave:
<br>Welcome to the Jupyter community! :tada:

codecov-commenter: # [Codecov](https://codecov.io/gh/jupyter-server/jupyter_server/pull/754?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server) Report
> Merging [#754](https://codecov.io/gh/jupyter-server/jupyter_server/pull/754?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server) (92499f4) into [main](https://codecov.io/gh/jupyter-server/jupyter_server/commit/cd1b1b886db08c6480329c3f0832d05f15d49a19?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server) (cd1b1b8) will **decrease** coverage by `1.12%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##             main     #754      +/-   ##
==========================================
- Coverage   70.68%   69.55%   -1.13%     
==========================================
  Files          62       62              
  Lines        7620     7375     -245     
  Branches     1218     1164      -54     
==========================================
- Hits         5386     5130     -256     
- Misses       1860     1864       +4     
- Partials      374      381       +7     
```


| [Impacted Files](https://codecov.io/gh/jupyter-server/jupyter_server/pull/754?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server) | Coverage Δ | |
|---|---|---|
| [jupyter\_server/services/contents/manager.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/754/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvc2VydmljZXMvY29udGVudHMvbWFuYWdlci5weQ==) | `84.22% <100.00%> (+2.02%)` | :arrow_up: |
| [jupyter\_server/extension/handler.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/754/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvZXh0ZW5zaW9uL2hhbmRsZXIucHk=) | `52.00% <0.00%> (-6.63%)` | :arrow_down: |
| [jupyter\_server/base/zmqhandlers.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/754/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvYmFzZS96bXFoYW5kbGVycy5weQ==) | `51.87% <0.00%> (-5.28%)` | :arrow_down: |
| [jupyter\_server/gateway/gateway\_client.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/754/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvZ2F0ZXdheS9nYXRld2F5X2NsaWVudC5weQ==) | `72.89% <0.00%> (-3.30%)` | :arrow_down: |
| [jupyter\_server/base/handlers.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/754/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvYmFzZS9oYW5kbGVycy5weQ==) | `63.26% <0.00%> (-2.47%)` | :arrow_down: |
| [jupyter\_server/files/handlers.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/754/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvZmlsZXMvaGFuZGxlcnMucHk=) | `30.90% <0.00%> (-2.43%)` | :arrow_down: |
| [jupyter\_server/pytest\_plugin.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/754/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvcHl0ZXN0X3BsdWdpbi5weQ==) | `81.81% <0.00%> (-2.35%)` | :arrow_down: |
| [jupyter\_server/services/kernels/kernelmanager.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/754/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvc2VydmljZXMva2VybmVscy9rZXJuZWxtYW5hZ2VyLnB5) | `80.25% <0.00%> (-2.29%)` | :arrow_down: |
| [jupyter\_server/services/kernels/handlers.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/754/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvc2VydmljZXMva2VybmVscy9oYW5kbGVycy5weQ==) | `57.01% <0.00%> (-2.06%)` | :arrow_down: |
| [jupyter\_server/auth/login.py](https://codecov.io/gh/jupyter-server/jupyter_server/pull/754/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXIvYXV0aC9sb2dpbi5weQ==) | `66.93% <0.00%> (-2.01%)` | :arrow_down: |
| ... and [25 more](https://codecov.io/gh/jupyter-server/jupyter_server/pull/754/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/jupyter-server/jupyter_server/pull/754?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/jupyter-server/jupyter_server/pull/754?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server). Last update [cd1b1b8...92499f4](https://codecov.io/gh/jupyter-server/jupyter_server/pull/754?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server).

davidbrochart: Thanks @dleen for opening this PR.
I think the problem here is that the hook passed on the command line with:
```
--FileContentsManager.pre_save_hook='mymodule.foo_function'
```
is being validated as a new hook, but is also treated as an [already existing hook](https://github.com/jupyter-server/jupyter_server/blob/cd1b1b886db08c6480329c3f0832d05f15d49a19/jupyter_server/services/contents/manager.py#L130), which it isn't because otherwise it would have been validated and [converted to a callable](https://github.com/jupyter-server/jupyter_server/blob/cd1b1b886db08c6480329c3f0832d05f15d49a19/jupyter_server/services/contents/manager.py#L126-L129).
Because we know that a validated hook is a callable and not a string anymore, I think a better solution would be to replace [this line](https://github.com/jupyter-server/jupyter_server/blob/cd1b1b886db08c6480329c3f0832d05f15d49a19/jupyter_server/services/contents/manager.py#L130):
```python
        if self.pre_save_hook is not None:
```
with:
```python
        if callable(self.pre_save_hook):
```
dleen: Yep that solution makes more sense!
blink1073: Kicking CI

Patch:
diff --git a/jupyter_server/services/contents/manager.py b/jupyter_server/services/contents/manager.py
index cd4ce80fb..2a2cf0c4b 100644
--- a/jupyter_server/services/contents/manager.py
+++ b/jupyter_server/services/contents/manager.py
@@ -127,7 +127,7 @@ class ContentsManager(LoggingConfigurable):
             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:
+        if callable(self.pre_save_hook):
             warnings.warn(
                 f"Overriding existing pre_save_hook ({self.pre_save_hook.__name__}) with a new one ({value.__name__}).",
                 stacklevel=2,
@@ -162,7 +162,7 @@ class ContentsManager(LoggingConfigurable):
             value = import_item(value)
         if not callable(value):
             raise TraitError("post_save_hook must be callable")
-        if self.post_save_hook is not None:
+        if callable(self.post_save_hook):
             warnings.warn(
                 f"Overriding existing post_save_hook ({self.post_save_hook.__name__}) with a new one ({value.__name__}).",
                 stacklevel=2,


Test patch:
diff --git a/tests/services/contents/test_config.py b/tests/services/contents/test_config.py
index 741c10c13..02f4d702c 100644
--- a/tests/services/contents/test_config.py
+++ b/tests/services/contents/test_config.py
@@ -18,6 +18,30 @@ def test_config_did_something(jp_server_config, jp_serverapp):
     )
 
 
+def example_pre_save_hook():
+    pass
+
+
+def example_post_save_hook():
+    pass
+
+
+@pytest.mark.parametrize(
+    "jp_server_config",
+    [
+        {
+            "ContentsManager": {
+                "pre_save_hook": "tests.services.contents.test_config.example_pre_save_hook",
+                "post_save_hook": "tests.services.contents.test_config.example_post_save_hook",
+            },
+        }
+    ],
+)
+def test_pre_post_save_hook_config(jp_serverapp, jp_server_config):
+    assert jp_serverapp.contents_manager.pre_save_hook.__name__ == "example_pre_save_hook"
+    assert jp_serverapp.contents_manager.post_save_hook.__name__ == "example_post_save_hook"
+
+
 async def test_async_contents_manager(jp_configurable_serverapp):
     config = {"ContentsManager": {"checkpoints_class": AsyncCheckpoints}}
     argv = [