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

Creates document session #108

Merged
merged 4 commits into from
Mar 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion jupyter_collaboration/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@
from traitlets import Float, Int, Type
from ypy_websocket.ystore import BaseYStore

from .handlers import SQLiteYStore, YDocRoomIdHandler, YDocWebSocketHandler
from .handlers import (
DocSessionHandler,
SQLiteYStore,
YDocRoomIdHandler,
YDocWebSocketHandler,
)


class YDocExtension(ExtensionApp):
Expand Down Expand Up @@ -57,7 +62,11 @@ def initialize_settings(self):
def initialize_handlers(self):
self.handlers.extend(
[
# Deprecated - to remove for 1.0.0
(r"/api/yjs/roomid/(.*)", YDocRoomIdHandler),
hbcarlos marked this conversation as resolved.
Show resolved Hide resolved
# Deprecated - to remove for 1.0.0
(r"/api/yjs/(.*)", YDocWebSocketHandler),
(r"/api/collaboration/room/(.*)", YDocWebSocketHandler),
hbcarlos marked this conversation as resolved.
Show resolved Hide resolved
(r"/api/collaboration/session/(.*)", DocSessionHandler),
Copy link
Collaborator

@davidbrochart davidbrochart Feb 27, 2023

Choose a reason for hiding this comment

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

I don't think we should grow the API surface.
I think we should use the already existing PUT /api/yjs/roomid/{path} endpoint. It could return a value that is a combination of the file ID, the document type and format, and the session ID. That way, the frontend doesn't have to ask for a session ID, all it asks for is a room ID. When it tries to connect to the WebSocket with a room ID that doesn't exist (because the server session has changed), the room cannot be found and the frontend asks for the room ID again, discarding the shared document that it may have.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm afraid I have to disagree. Returning a string with multiple attributes in it is not the proper solution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

By combination, I don't mean necessarily concatenating multiple attributes, it can be a UUID and the server keeps a mapping of this UUID to the attributes.

Copy link
Member

Choose a reason for hiding this comment

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

The trouble with the opaque UUID is that on the frontend you will not be able to provide good error message to the user and follow-up action. In particular if the reason the websocket cannot be opened is the session id, we must tell the user to not use the current browser tab.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is easy for the frontend to know that the session has changed: it fails to connect with its room ID. It can now ask for the room ID again, if that fails it's because the document doesn't exist, otherwise it's because the server has restarted, and in that case it can tell the user to not use the tab.

Copy link
Member

Choose a reason for hiding this comment

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

Implicit deduction is never a good path for code evolution, maintenance and understanding.

]
)
45 changes: 45 additions & 0 deletions jupyter_collaboration/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import asyncio
import json
import uuid
from logging import Logger
from pathlib import Path
from typing import Any, Dict, Optional, Set, Tuple
Expand All @@ -24,6 +25,8 @@

YFILE = YDOCS["file"]

SERVER_SESSION = str(uuid.uuid4())


class TempFileYStore(_TempFileYStore):
prefix_dir = "jupyter_ystore_"
Expand Down Expand Up @@ -201,6 +204,11 @@ async def open(self, path):
self.websocket_server.background_tasks.add(task)
task.add_done_callback(self.websocket_server.background_tasks.discard)

# Close the connection if the document session expired
session_id = self.get_query_argument("sessionId", "")
if isinstance(self.room, DocumentRoom) and SERVER_SESSION != session_id:
self.close(1003, f"Document session {session_id} expired")

# cancel the deletion of the room if it was scheduled
if isinstance(self.room, DocumentRoom) and self.room.cleaner is not None:
self.room.cleaner.cancel()
Expand All @@ -227,6 +235,7 @@ async def open(self, path):
# if YStore updates and source file are out-of-sync, resync updates with source
if self.room.document.source != model["content"]:
read_from_source = True

if read_from_source:
self.room.document.source = model["content"]
if self.room.ystore:
Expand Down Expand Up @@ -409,3 +418,39 @@ async def put(self, path):
ws_url += str(idx)
self.log.info("Request for Y document '%s' with room ID: %s", path, ws_url)
return self.finish(ws_url)


class DocSessionHandler(APIHandler):
auth_resource = "contents"

@web.authenticated
@authorized
async def put(self, path):
body = json.loads(self.request.body)
format = body["format"]
content_type = body["type"]
file_id_manager = self.settings["file_id_manager"]

idx = file_id_manager.get_id(path)
if idx is not None:
# index already exists
self.log.info("Request for Y document '%s' with room ID: %s", path, idx)
data = json.dumps(
{"format": format, "type": content_type, "fileId": idx, "sessionId": SERVER_SESSION}
)
self.set_status(200)
return self.finish(data)

# try indexing
idx = file_id_manager.index(path)
if idx is None:
# file does not exists
raise web.HTTPError(404, f"File {path!r} does not exist")

# index successfully created
self.log.info("Request for Y document '%s' with room ID: %s", path, idx)
data = json.dumps(
{"format": format, "type": content_type, "fileId": idx, "sessionId": SERVER_SESSION}
)
davidbrochart marked this conversation as resolved.
Show resolved Hide resolved
self.set_status(201)
return self.finish(data)
7 changes: 5 additions & 2 deletions packages/collaboration-extension/src/filebrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
IDefaultFileBrowser,
IFileBrowserFactory
} from '@jupyterlab/filebrowser';
import { ITranslator } from '@jupyterlab/translation';

import { CommandRegistry } from '@lumino/commands';

Expand All @@ -27,11 +28,12 @@ namespace CommandIDs {
export const defaultFileBrowser: JupyterFrontEndPlugin<IDefaultFileBrowser> = {
id: '@jupyter/collaboration-extension:defaultFileBrowser',
provides: IDefaultFileBrowser,
requires: [IFileBrowserFactory],
requires: [IFileBrowserFactory, ITranslator],
optional: [IRouter, JupyterFrontEnd.ITreeResolver, ILabShell],
activate: async (
app: JupyterFrontEnd,
fileBrowserFactory: IFileBrowserFactory,
translator: ITranslator,
router: IRouter | null,
tree: JupyterFrontEnd.ITreeResolver | null,
labShell: ILabShell | null
Expand All @@ -41,7 +43,8 @@ export const defaultFileBrowser: JupyterFrontEndPlugin<IDefaultFileBrowser> = {
);
const { commands } = app;

const drive = new YDrive(app.serviceManager.user);
const trans = translator.load('jupyter_collaboration');
const drive = new YDrive(app.serviceManager.user, trans);
app.serviceManager.contents.addDrive(drive);

// Manually restore and load the default file browser.
Expand Down
47 changes: 47 additions & 0 deletions packages/docprovider/src/requests.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/* -----------------------------------------------------------------------------
| Copyright (c) Jupyter Development Team.
| Distributed under the terms of the Modified BSD License.
|----------------------------------------------------------------------------*/

import { URLExt } from '@jupyterlab/coreutils';
import { ServerConnection, Contents } from '@jupyterlab/services';

/**
* Document session endpoint provided by `jupyter_collaboration`
* See https://github.com/jupyterlab/jupyter_collaboration
*/
const DOC_SESSION_URL = 'api/collaboration/session';

export interface ISessionModel {
format: Contents.FileFormat;
type: Contents.ContentType;
fileId: string;
sessionId: string;
}

export async function requestDocSession(
format: string,
type: string,
path: string
): Promise<ISessionModel> {
const { makeSettings, makeRequest, ResponseError } = ServerConnection;

const settings = makeSettings();
const url = URLExt.join(
settings.baseUrl,
DOC_SESSION_URL,
encodeURIComponent(path)
);
const data = {
method: 'PUT',
body: JSON.stringify({ format, type })
};

const response = await makeRequest(url, data, settings);

if (response.status !== 200 && response.status !== 201) {
throw new ResponseError(response);
}

return response.json();
}
16 changes: 10 additions & 6 deletions packages/docprovider/src/ydrive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,14 @@ import {
YNotebook
} from '@jupyter/ydoc';
import { URLExt } from '@jupyterlab/coreutils';
import { TranslationBundle } from '@jupyterlab/translation';
import { Contents, Drive, User } from '@jupyterlab/services';
import { WebSocketProvider } from './yprovider';

/**
* The url for the default drive service.
*/
const Y_DOCUMENT_PROVIDER_URL = 'api/yjs';
const DOCUMENT_PROVIDER_URL = 'api/collaboration/room';

/**
* A Collaborative implementation for an `IDrive`, talking to the
Expand All @@ -27,9 +28,10 @@ export class YDrive extends Drive {
*
* @param user - The user manager to add the identity to the awareness of documents.
*/
constructor(user: User.IManager) {
constructor(user: User.IManager, translator: TranslationBundle) {
super({ name: 'YDrive' });
this._user = user;
this._trans = translator;
this._providers = new Map<string, WebSocketProvider>();

this.sharedModelFactory = new SharedModelFactory(this._onCreate);
Expand Down Expand Up @@ -85,7 +87,7 @@ export class YDrive extends Drive {
options?: Contents.IFetchOptions
): Promise<Contents.IModel> {
if (options && options.format && options.type) {
const key = `${options.type}:${options.format}:${localPath}`;
const key = `${options.format}:${options.type}:${localPath}`;
const provider = this._providers.get(key);

if (provider) {
Expand Down Expand Up @@ -150,15 +152,16 @@ export class YDrive extends Drive {
}
try {
const provider = new WebSocketProvider({
url: URLExt.join(this.serverSettings.wsUrl, Y_DOCUMENT_PROVIDER_URL),
url: URLExt.join(this.serverSettings.wsUrl, DOCUMENT_PROVIDER_URL),
path: options.path,
format: options.format,
contentType: options.contentType,
model: sharedModel,
user: this._user
user: this._user,
translator: this._trans
});

const key = `${options.contentType}:${options.format}:${options.path}`;
const key = `${options.format}:${options.contentType}:${options.path}`;
this._providers.set(key, provider);

sharedModel.disposed.connect(() => {
Expand All @@ -178,6 +181,7 @@ export class YDrive extends Drive {
};

private _user: User.IManager;
private _trans: TranslationBundle;
private _providers: Map<string, WebSocketProvider>;
}

Expand Down
Loading