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

Add autoredirect query string parameter to generate new workspaces. #5950

Merged
merged 7 commits into from Feb 14, 2019

Conversation

@afshin
Copy link
Member

@afshin afshin commented Feb 6, 2019

This adds support for a query string parameter called autoredirect which will redirect the user to a randomly generated workspace name of the form auto-x where x is randomly drawn from [A-Z,a-z,0-9].

The semantics of this functionality are:

  • If autoredirect is added to the query string and there is no workspace collision, the user is not redirected. The autoredirect query string parameter is silently removed from the URL.
  • If autoredirect is added to the query string and there is a workspace collision, the user is redirected to an auto-generated workspace. In this case, the originally requested workspace is cloned into the new auto-generated one. Afterward, the autoredirect parameter is scrubbed from the URL.
  • The combination of these two behaviors means that if an auto-generated name auto-A already exists, another auto-generated name will be picked at random. The pathological case is if the user has 62 randomly generated tabs open. The 63rd randomly generated tab will get stuck in an endless loop of trying to create an auto-generated name. This seems like an acceptable outcome. The benefit of this is that all auto-generated names are very short.

Fixes #5854

@afshin afshin added this to the 1.0 milestone Feb 6, 2019
@afshin afshin self-assigned this Feb 6, 2019
@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Feb 6, 2019

A couple of thoughts:

  1. A lot of the use-case here, if I understand correctly, is if the user is sent to a workspace but it is already in use. I wonder if we should make it so that the redirect url is related to the originally intended one? So if the user wanted /lab/workspaces/foo, they get /lab/workspaces/foo-<token>.
  2. You have taken a random token-based approach. We have another behavior at play in the app for a similar situation: if the user creates a new notebook, and Untitled.ipynb already exists, then they get Untitled1.ipynb, Untitled2.ipynb, etc. This also avoids collisions.
  3. If the user wanted an existing workspace, should the new one be a combination of clone-then-autoredirect?
  4. Should we be doing any garbage collection of old workspaces?

@afshin afshin changed the title Add autoredirect query string parameter to generate new workspaces. [WIP] Add autoredirect query string parameter to generate new workspaces. Feb 8, 2019
@afshin
Copy link
Member Author

@afshin afshin commented Feb 8, 2019

(See top-level note that addresses these comments.)

@afshin afshin changed the title [WIP] Add autoredirect query string parameter to generate new workspaces. Add autoredirect query string parameter to generate new workspaces. Feb 8, 2019
Copy link
Member

@ian-r-rose ian-r-rose left a comment

Looks good to me. Thanks @afshin!

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 8, 2019

Can we change the name 'autoredirect'? 'redirect' is a pretty technical term, and every time I see the word "autoredirect" I parse it as "autore direct", which confuses me. Probably because I'm much more used to the word "direct", so my brain picks it out as a word before trying to make sense of the first bit?

@afshin
Copy link
Member Author

@afshin afshin commented Feb 9, 2019

@jasongrout I'm happy with any query string parameter as long as it is all lowercase. Do you have any suggestions?

  • alias
  • autoalias
  • automatic
  • autoname
  • nominate
  • resolve
  • autoresolve

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Feb 9, 2019

I kind of like autoredirect, myself.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 9, 2019

Can I think about it over the weekend? (which I guess also means we could merge this and have a follow-up changing the name if we agree on something different)

@afshin
Copy link
Member Author

@afshin afshin commented Feb 9, 2019

Also, my typos up in the PR description were suggestive. I wrote autoreload multiple times when I meant to write autoredirect. So I'll add autoreload to the list of possible names.

@afshin
Copy link
Member Author

@afshin afshin commented Feb 9, 2019

ensure-workspace

@afshin
Copy link
Member Author

@afshin afshin commented Feb 9, 2019

resolve-workspace

@afshin
Copy link
Member Author

@afshin afshin commented Feb 11, 2019

This is what the code would look like if we decided to use @ian-r-rose's auto-increment suggestion (number 2 above). I'm not sure which direction is better, the way it's currently implemented with random tokens and only 62 options, clobbering currently-unused auto-generated workspace names or auto-incrementing workspace names, which allows the number of workspaces to proliferate.

diff --git a/packages/apputils-extension/src/index.ts b/packages/apputils-extension/src/index.ts
index 68c663768..ed79a3775 100644
--- a/packages/apputils-extension/src/index.ts
+++ b/packages/apputils-extension/src/index.ts
@@ -43,6 +43,7 @@ import { Palette } from './palette';
 import { createRedirectForm } from './redirect';
 
 import '../style/index.css';
+import { ServiceManager } from '@jupyterlab/services';
 
 /**
  * The interval in milliseconds that calls to save a workspace are debounced
@@ -235,7 +236,7 @@ const resolver: JupyterFrontEndPlugin<IWindowResolver> = {
   provides: IWindowResolver,
   requires: [JupyterFrontEnd.IPaths, IRouter],
   activate: async (
-    _: JupyterFrontEnd,
+    app: JupyterFrontEnd,
     paths: JupyterFrontEnd.IPaths,
     router: IRouter
   ) => {
@@ -255,18 +256,20 @@ const resolver: JupyterFrontEndPlugin<IWindowResolver> = {
       return new Promise<IWindowResolver>(() => {
         // If the user has requested `autoredirect` create a new workspace name.
         if ('autoredirect' in query) {
-          const { base, workspaces } = paths.urls;
-          const pool =
-            'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789';
-          const random = pool[Math.floor(Math.random() * pool.length)];
-          const path = URLExt.join(base, workspaces, `auto-${random}`);
-
-          // Clone the originally requested workspace after redirecting.
-          query['clone'] = workspace;
-
-          // Change the URL and trigger a hard reload to re-route.
-          const url = path + URLExt.objectToQueryString(query) + (hash || '');
-          router.navigate(url, { hard: true, silent: true });
+          Private.nextWorkspace(paths, app.serviceManager)
+            .then(path => {
+              // Clone the originally requested workspace after redirecting.
+              query['clone'] = workspace;
+              // Change the URL and trigger a hard reload to re-route.
+              const url =
+                path + URLExt.objectToQueryString(query) + (hash || '');
+              router.navigate(url, { hard: true, silent: true });
+            })
+            .catch(reason => {
+              // Launch a dialog to ask the user for a new workspace name.
+              console.warn('Generating workspace name failed:', reason);
+              Private.redirect(router, paths, workspace);
+            });
           return;
         }
 
@@ -638,6 +641,48 @@ namespace Private {
     return splash;
   }
 
+  /**
+   * Return the next available auto-generated workspace name of the form
+   * `auto-X`, where `X` is one or more characters drawn from the range
+   * [A-Z,a-z,0-9].
+   */
+  export async function nextWorkspace(
+    paths: JupyterFrontEnd.IPaths,
+    services: ServiceManager
+  ): Promise<string> {
+    const prefix = URLExt.join(paths.urls.base, paths.urls.workspaces, 'auto-');
+    const workspaces: { [workspace: string]: null } = {};
+
+    // Fetch all of the workspace names that are stored in the back-end.
+    try {
+      (await services.workspaces.list())
+        .filter(item => item.indexOf(prefix) === 0)
+        .reduce((acc, workspace) => {
+          acc[workspace] = null;
+          return acc;
+        }, workspaces);
+    } catch (error) {
+      throw error;
+    }
+
+    // Iterate through possible workspace name candidates until one that has not
+    // been claimed is found. Even though this will almost always be the last
+    // workspace + 1, because it is possible for users of the API or file system
+    // to delete an existing workspace, we iterate through and find the first
+    // available name.
+    const pool =
+      'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789';
+    const convert = (n: number, base = pool.length): string =>
+      n < base ? pool[n] : convert(Math.floor(n / base) - 1) + pool[n % base];
+    let count = 0;
+    let candidate: string;
+    do {
+      candidate = `${prefix}${convert(count++)}`;
+    } while (candidate in workspaces);
+
+    return candidate;
+  }
+
   /**
    * A debouncer for recovery attempts.
    */

@afshin
Copy link
Member Author

@afshin afshin commented Feb 11, 2019

Having implemented both options:

  • (a) the max number of auto-generated names is 62 workspaces
  • (b) the auto-generated workspaces proliferate

I now have a strong inclination toward (a) because I am refactoring the workspaces REST API and realizing that if we want to return all the workspaces, we should make sure there won't be tons of data sent down the wire that almost certainly will never be used again.

cf. #5975

@afshin
Copy link
Member Author

@afshin afshin commented Feb 13, 2019

@jasongrout Do you have a suggestion for changing the parameter or should we merge this?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 13, 2019

I'm not very good at naming things :(. I like having "workspace" somewhere in that parameter, to explicitly signal to the user what the parameter is about. How about autoname-workspace or resolve-workspace, or even auto-workspace?

@afshin afshin merged commit f47355c into jupyterlab:master Feb 14, 2019
9 checks passed
@choldgraf
Copy link
Contributor

@choldgraf choldgraf commented Feb 14, 2019

Hey all - a few quick points:

  • First off, this is awesome, it'll be super useful for our nbgitpuller stuff, thanks @afshin :-)
  • Secondly, can you make the 63rd tab point to a page that just says something like "omg why?"
  • Thirdly, this isn't documented at all! Is there a reason for that? Are keeping this in shadow mode to see if the API / naming will change?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 14, 2019

Thanks @afshin!

@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants