Skip to content

Commit

Permalink
Move WebSocket path under document-level prefix. (#315)
Browse files Browse the repository at this point in the history
Originally, Y-Sweet returned just a WebSocket URL. Now, we return both a
`baseUrl` (which is the base used for the `update` / `as-update`
endpoints), as well as a `url` which is a WebSocket URL.

I want to deprecate `url`, but that means that we need to be able to
construct the WebSocket URL deterministically from a `baseUrl`. This is
not currently possible, because it depends on how the server is set up:

- When running in `doc-serve` mode, we put the websocket server under
`/ws` relative to the `baseUrl`
- When running as a standalone server (`y-sweet serve`), we put the
doc-relative endpoints under `/d/:doc_id/` but the WebSocket endpoint at
the top level.

This PR makes `y-sweet serve` consistent with `doc-serve`, by making the
WebSocket URL relative to `/d/:doc_id/`. Since the y-websocket provider
also appends the doc ID, this means the URL contains the doc ID twice,
which we check for equality. (Our own provider will not have this
problem).

In order to not break users who manually construct the WebSocket URL,
the old-style URL still works. The `url` returned by the `auth` endpoint
points to the new `/d/:doc_id/ws` server. Before we update the client to
construct the WebSocket URL from `baseUrl` (removing the need to return
`url`), we will have to make this change with the cloudflare worker as
well.

Existing tests cover this, but I also ran locally and confirmed that the
URL is constructed as expected as a sanity check.
  • Loading branch information
paulgb authored Oct 9, 2024
1 parent 9a8c0f6 commit 2265931
Showing 1 changed file with 39 additions and 5 deletions.
44 changes: 39 additions & 5 deletions crates/y-sweet/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,13 +336,17 @@ impl Server {
.route("/ready", get(ready))
.route("/check_store", post(check_store))
.route("/check_store", get(check_store_deprecated))
.route("/doc/ws/:doc_id", get(handle_socket_upgrade))
.route("/doc/ws/:doc_id", get(handle_socket_upgrade_deprecated))
.route("/doc/new", post(new_doc))
.route("/doc/:doc_id/auth", post(auth_doc))
.route("/doc/:doc_id/as-update", get(get_doc_as_update_deprecated))
.route("/doc/:doc_id/update", post(update_doc_deprecated))
.route("/d/:doc_id/as-update", get(get_doc_as_update))
.route("/d/:doc_id/update", post(update_doc))
.route(
"/d/:doc_id/ws/:doc_id2",
get(handle_socket_upgrade_full_path),
)
.with_state(self.clone())
}

Expand Down Expand Up @@ -510,6 +514,34 @@ async fn handle_socket_upgrade(
Ok(ws.on_upgrade(move |socket| handle_socket(socket, awareness, cancellation_token)))
}

async fn handle_socket_upgrade_deprecated(
ws: WebSocketUpgrade,
Path(doc_id): Path<String>,
Query(params): Query<HandlerParams>,
State(server_state): State<Arc<Server>>,
) -> Result<Response, AppError> {
tracing::warn!(
"/doc/ws/:doc_id is deprecated; call /doc/:doc_id/auth instead and use the returned URL."
);
handle_socket_upgrade(ws, Path(doc_id), Query(params), State(server_state)).await
}

async fn handle_socket_upgrade_full_path(
ws: WebSocketUpgrade,
Path((doc_id, doc_id2)): Path<(String, String)>,
Query(params): Query<HandlerParams>,
State(server_state): State<Arc<Server>>,
) -> Result<Response, AppError> {
if doc_id != doc_id2 {
return Err(AppError(
StatusCode::BAD_REQUEST,
anyhow!("For Yjs compatibility, the doc_id appears twice in the URL. It must be the same in both places, but we got {} and {}.", doc_id, doc_id2),
));
}

handle_socket_upgrade(ws, Path(doc_id), Query(params), State(server_state)).await
}

async fn handle_socket_upgrade_single(
ws: WebSocketUpgrade,
Path(doc_id): Path<String>,
Expand Down Expand Up @@ -660,10 +692,10 @@ async fn auth_doc(
let mut url = url_prefix.clone();
let scheme = if url.scheme() == "https" { "wss" } else { "ws" };
url.set_scheme(scheme).unwrap();
url = url.join("/doc/ws").unwrap();
url = url.join(&format!("/d/{doc_id}/ws")).unwrap();
url.to_string()
} else {
format!("ws://{host}/doc/ws")
format!("ws://{host}/d/{doc_id}/ws")
};

let base_url = if let Some(url_prefix) = &server_state.url_prefix {
Expand Down Expand Up @@ -717,7 +749,8 @@ mod test {
.await
.unwrap();

assert_eq!(token.url, "ws://localhost/doc/ws");
let expected_url = format!("ws://localhost/d/{doc_id}/ws");
assert_eq!(token.url, expected_url);
assert_eq!(token.doc_id, doc_id);
assert!(token.token.is_none());
}
Expand Down Expand Up @@ -754,7 +787,8 @@ mod test {
.await
.unwrap();

assert_eq!(token.url, "wss://foo.bar/doc/ws");
let expected_url = format!("wss://foo.bar/d/{doc_id}/ws");
assert_eq!(token.url, expected_url);
assert_eq!(token.doc_id, doc_id);
assert!(token.token.is_none());
}
Expand Down

0 comments on commit 2265931

Please sign in to comment.