Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow pre_save_hook to cancel save with HTTPError #456

Merged
merged 1 commit into from
Mar 24, 2021

Conversation

minrk
Copy link
Contributor

@minrk minrk commented Mar 24, 2021

currently, all errors in pre_save_hook are caught and logged, making it impossible for pre_save_hook to be used for validation / quota enforcement / etc.

By allowing HTTPErrors to raise, save will be cancelled and the hook's error message will be shown to the user in a dialog.

For example:

from tornado.web import HTTPError

def fail_save_forbidden(model, path, **kw):
    if "forbidden" in path.split("/"):
        raise HTTPError(400, "Can't save forbidden files!")

c.ContentsManager.pre_save_hook = fail_save_forbidden

Brought up on the forum

currently, all errors in pre_save_hook are ignored,
making it impossible for pre_save_hook to be used for validation / quota enforcement / etc.

By allowing HTTPErrors to raise, save will be cancelled and the error message will be shown to the user
@codecov-io
Copy link

codecov-io commented Mar 24, 2021

Codecov Report

Merging #456 (40f653d) into master (bf00ad0) will increase coverage by 0.17%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #456      +/-   ##
==========================================
+ Coverage   77.50%   77.68%   +0.17%     
==========================================
  Files         105      106       +1     
  Lines        8936     9106     +170     
  Branches      956      972      +16     
==========================================
+ Hits         6926     7074     +148     
- Misses       1673     1680       +7     
- Partials      337      352      +15     
Impacted Files Coverage Δ
jupyter_server/services/contents/manager.py 87.79% <0.00%> (-0.53%) ⬇️
jupyter_server/transutils.py 72.72% <0.00%> (-27.28%) ⬇️
jupyter_server/terminal/handlers.py 76.00% <0.00%> (-8.22%) ⬇️
jupyter_server/auth/security.py 76.00% <0.00%> (-2.69%) ⬇️
jupyter_server/services/contents/filemanager.py 66.09% <0.00%> (-0.86%) ⬇️
jupyter_server/terminal/__init__.py 79.16% <0.00%> (-0.84%) ⬇️
jupyter_server/utils.py 62.85% <0.00%> (-0.61%) ⬇️
jupyter_server/extension/application.py 72.36% <0.00%> (-0.09%) ⬇️
jupyter_server/tests/auth/test_security.py 100.00% <0.00%> (ø)
jupyter_server/tests/extension/conftest.py 100.00% <0.00%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf00ad0...40f653d. Read the comment docs.

@blink1073 blink1073 added this to the 1.5 milestone Mar 24, 2021
Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@blink1073 blink1073 merged commit c3303cd into jupyter-server:master Mar 24, 2021
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be useful - thanks.

We could probably use a test for this at some point.

hMED22 pushed a commit to hMED22/jupyter_server that referenced this pull request Jan 23, 2023
allow pre_save_hook to cancel save with HTTPError
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants