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

fix(torii): graphql playground URL #2707

Merged
merged 4 commits into from
Nov 21, 2024
Merged

fix(torii): graphql playground URL #2707

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c2730d9
fix: avoid absolute path for URLs
glihm Nov 20, 2024
dbb517a
fix: attempt to fix using relative URL
glihm Nov 21, 2024
ea63e39
fix: merge external url logic for subscription and endpoint
glihm Nov 21, 2024
7be2959
fix: ensure panic if the scheme is invalid
glihm Nov 21, 2024
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
25 changes: 13 additions & 12 deletions crates/torii/graphql/src/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,27 +48,28 @@
},
);

let subscription_endpoint = if let Some(external_url) = external_url {
let mut websocket_url = external_url.clone();
websocket_url.set_path("/graphql/ws");
let (graphql_endpoint, subscription_endpoint) = if let Some(external_url) = external_url {
let mut graphql_url = external_url;
graphql_url.set_path("graphql");

Check warning on line 53 in crates/torii/graphql/src/server.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/server.rs#L51-L53 

Added lines #L51 - L53 were not covered by tests

let websocket_scheme = match websocket_url.scheme() {
"http" => "ws",
let mut websocket_url = graphql_url.clone();
websocket_url.set_path("ws");
let _ = websocket_url.set_scheme(match websocket_url.scheme() {

Check warning on line 57 in crates/torii/graphql/src/server.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/server.rs#L55-L57 

Added lines #L55 - L57 were not covered by tests
"https" => "wss",
_ => panic!("Invalid URL scheme"), // URL validated on input so this never hits
};
"http" => "ws",
_ => panic!("Invalid URL scheme - must be http or https"),

Check warning on line 60 in crates/torii/graphql/src/server.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/server.rs#L59-L60 

Added lines #L59 - L60 were not covered by tests
});
Comment on lines +51 to +61
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ohayo sensei! Replace panic with proper error handling.

The URL scheme handling currently uses panic! which could crash the server in production. Consider returning a Result instead.

Here's a suggested improvement:

-    let (graphql_endpoint, subscription_endpoint) = if let Some(external_url) = external_url {
-        let mut graphql_url = external_url;
-        graphql_url.set_path("graphql");
-
-        let mut websocket_url = graphql_url.clone();
-        websocket_url.set_path("ws");
-        let _ = websocket_url.set_scheme(match websocket_url.scheme() {
-            "https" => "wss",
-            "http" => "ws",
-            _ => panic!("Invalid URL scheme - must be http or https"),
-        });
+    let (graphql_endpoint, subscription_endpoint) = if let Some(external_url) = external_url {
+        let mut graphql_url = external_url;
+        graphql_url.set_path("graphql");
+
+        let mut websocket_url = graphql_url.clone();
+        websocket_url.set_path("ws");
+        let ws_scheme = match websocket_url.scheme() {
+            "https" => "wss",
+            "http" => "ws",
+            scheme => {
+                log::error!("Invalid URL scheme '{}' - must be http or https", scheme);
+                return warp::reply::with_status(
+                    "Invalid URL scheme",
+                    warp::http::StatusCode::INTERNAL_SERVER_ERROR,
+                );
+            }
+        };
+        websocket_url.set_scheme(ws_scheme).expect("valid scheme");

Committable suggestion skipped: line range outside the PR's diff.


let _ = websocket_url.set_scheme(websocket_scheme);
websocket_url.to_string()
(graphql_url.path().to_string(), websocket_url.to_string())

Check warning on line 63 in crates/torii/graphql/src/server.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/server.rs#L63 

Added line #L63 was not covered by tests
} else {
"/graphql/ws".to_string()
("graphql".to_string(), "graphql/ws".to_string())

Check warning on line 65 in crates/torii/graphql/src/server.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/server.rs#L65 

Added line #L65 was not covered by tests
};

let playground_filter = warp::path("graphql").map(move || {
warp::reply::html(
GraphiQLSource::build()
.endpoint("/graphql")
.subscription_endpoint(subscription_endpoint.as_str())
.endpoint(&graphql_endpoint)
.subscription_endpoint(&subscription_endpoint)

Check warning on line 72 in crates/torii/graphql/src/server.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/graphql/src/server.rs#L71-L72 

Added lines #L71 - L72 were not covered by tests
.finish(),
)
});
Expand Down
Loading