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

Save-As should use a head request (it does not need the body) and get should catch non-existing. #15153

Open
Carreau opened this issue Sep 23, 2023 · 4 comments

Comments

@Carreau
Copy link
Contributor

Carreau commented Sep 23, 2023

It's not per-se a enhancement, but not really a bug,

but when using JupyterLab Save As, the flow of the save as function is the following:

  async saveAs(): Promise<void> {
    ...
    try {
      ...
      await this._manager.contents.get(newPath); // this likely triggers the 404.
      await this._maybeOverWrite(newPath); // if not 404 ask do you like to overwrite.
    } catch (err) {
      if (!err.response || err.response.status !== 404) {
        throw err;
      }
      ...
      await this._finishSaveAs(newPath);
    }
  }

I think this has 2 issues.

  1. the get is unnecessary wastefull you don't want the full content, just to know if the content exists. This is what a HEAD request is for. but...

1a) jupyter-server does not (yet) implement HEAD for content handler. I guess it could be auto-derived from GET without sending the content. (Opened jupyter-server/jupyter_server#1327)

  1. The Jupyter server get implementation does raise a 404 non-existing from deeper in the handler:

(Edit opened jupyter-server/jupyter_server#1328)

This leads to a TB in the terminal:

[W 2023-09-20 17:12:47.709 ServerApp] wrote error: 'No such file or directory: .../Downloads/test.ipynb'
    Traceback (most recent call last):
      File ".../site-packages/tornado/web.py", line 1786, in _execute
        result = await result
                 ^^^^^^^^^^^^
      File ".../site-packages/jupyter_server/services/contents/handlers.py", line 121, in get
        model = await ensure_async(
                ^^^^^^^^^^^^^^^^^^^
      File ".../site-packages/jupyter_core/utils/__init__.py", line 182, in ensure_async
        result = await obj
                 ^^^^^^^^^
      File ".../site-packages/jupyter_server/services/contents/filemanager.py", line 877, in get raise web.HTTPError(404, "No such file or directory: %s" % path)
    tornado.web.HTTPError: HTTP 404: Not Found (No such file or directory: .../Downloads/test.ipynb)

Which can make users believe something is wrong, while it's an expected 404.

The following should likely catch 404, and have a clean self.set_status(404) and self.finish() to avoid tracebacks:

        # add try
        model = await ensure_async(
            self.contents_manager.get( //this raise
                path=path,
                type=type,
                format=format,
                content=content,
            )
        )
        # except 404, set status.

Same should likely be done if HEAD is added/used.

Agreed 2 of those might need to be in jupyter-server, but as this affects the JupyterLab I prefered to open this here first, and let this repo sync with upstream.

This should also be a fairly easy issue for a new contributor both on here and upstream.

@SunSummoner
Copy link

@Carreau I would like to contribute to this issue. Can you assign it to me?

@krassowski
Copy link
Member

@SunSummoner please feel welcome to contribute!

Note: We do not use the "assign" feature on GitHub for "good first issue"s in this repository to avoid stale assignments.

@DcWire
Copy link
Contributor

DcWire commented Dec 9, 2023

Hey @Carreau , could you explain again what is to be resolved here? Ive done some exploration and was able to reproduce the error on the console, but didn't quite understand the outcome that is to be achieved. Thanks.

@mohamed-ali-halloul
Copy link

hello can i work in this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants