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.

Sign up for GitHub

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

Jump to bottom

Remove Redundant Dir_Exists Invocation When Creating New Files with ContentsManager #720

Merged
merged 12 commits into from Mar 29, 2022
Merged

Remove Redundant Dir_Exists Invocation When Creating New Files with ContentsManager #720

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e5d3921
Remove redundant calls in new_untitled method and added dir_exists ca…
Mar 12, 2022
42b87df
Remove dir_exists from Contents Manager handlers
Mar 14, 2022
2c86682
fix copy implementation
Mar 14, 2022
c8b4ea1
fix copy implementation
Mar 14, 2022
c9ba869
add back new line
Mar 14, 2022
a76c7f7
fix merge conflict
Mar 14, 2022
53992fb
remove duplication
jhamet93 Mar 26, 2022
becfda8
Merge branch 'main' of https://github.com/jupyter-server/jupyter_serv…
jhamet93 Mar 26, 2022
f6a300b
address feedback
jhamet93 Mar 26, 2022
cd96a4e
address feedback
jhamet93 Mar 26, 2022
073b46e
feedback
jhamet93 Mar 26, 2022
1e3b4b7
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Mar 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion jupyter_server/services/contents/filemanager.py
View file
Open in desktop
Expand Up @@ -905,4 +905,4 @@ async def rename_file(self, old_path, new_path):
except web.HTTPError:
raise
except Exception as e:
raise web.HTTPError(500, "Unknown error renaming file: %s %s" % (old_path, e)) from e
raise web.HTTPError(500, "Unknown error renaming file: %s %s" % (old_path, e)) from e
4 changes: 0 additions & 4 deletions jupyter_server/services/contents/handlers.py
View file
Open in desktop
Expand Up @@ -194,10 +194,6 @@ async def post(self, path=""):
if file_exists:
raise web.HTTPError(400, "Cannot POST to files, use PUT instead.")

dir_exists = await ensure_async(cm.dir_exists(path))
if not dir_exists:
raise web.HTTPError(404, "No such directory: %s" % path)

model = self.get_json_body()

if model is not None:
Expand Down
9 changes: 9 additions & 0 deletions jupyter_server/services/contents/manager.py
View file
Open in desktop
Expand Up @@ -459,8 +459,12 @@ def copy(self, from_path, to_path=None):
from_path must be a full path to a file.
"""
path = from_path.strip("/")

if to_path is not None:
to_path = to_path.strip("/")
dir_exists = self.dir_exists(to_path)
if not dir_exists:
Copy link
Member

Choose a reason for hiding this comment

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

We should probably leverage the self.dir_exists(to_path) at L483 below to avoid a redundant dir-exists check.

It's interesting, but the REST API appears to only support "copy" into a directory (typically the same directory as the source), whereas the ContentsManager also supports copy to a file, which is why the tests are failing (due to the first dir_exists() call since to_path represents a file).

It seems like the case that's missing here (even before this PR) is when to_path (being either a directory or file) references a non-existent directory. The way things stand now, this will result in a 500 error similar to the following (reproduced by changing a character in the parent directory name from 'b' to 'x' in the test_copy test):

tornado.web.HTTPError: HTTP 500: Internal Server Error (Unexpected error while saving file: å x/copy 2.ipynb [Errno 2] No such file or directory: '/private/var/folders/4w/qxgsbhyx1y93qlk9mnzwjryc0000gn/T/pytest-of-kbates/pytest-113/test_copy_jp_contents_manager30/å x/copy 2.ipynb')

jupyter_server/services/contents/filemanager.py:806: HTTPError

In order to preserve parity with the handler (where to_path can only be a directory and a 404 is returned if it doesn't exist), I think some additional logic is required to check if to_path is only a directory name or a combination of directory/file and ensure the respective directory portion exists (throwing 404 if it does not).

(Same would be needed in the async CM as well.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good eye. Made the modifications and tried to preserve the parity you mentioned with a few test cases to verify.

raise HTTPError(404, "No such directory: %s" % to_path)

if "/" in path:
from_dir, from_name = path.rsplit("/", 1)
Expand Down Expand Up @@ -819,9 +823,14 @@ async def copy(self, from_path, to_path=None):
from_path must be a full path to a file.
"""
path = from_path.strip("/")

if to_path is not None:
to_path = to_path.strip("/")

dir_exists = await self.dir_exists(to_path)
if not dir_exists:
raise HTTPError(404, "No such directory: %s" % to_path)

if "/" in path:
from_dir, from_name = path.rsplit("/", 1)
else:
Expand Down