opt(torii-server): initializing handlers #3078
Conversation
|
Ohayo sensei! Here’s the update on the pull request: WalkthroughThis pull request introduces the Changes
Suggested reviewers
Possibly related PRs
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
crates/torii/server/src/proxy.rs (1)
179-183:⚠️ Potential issueUndefined variable 'handlers' in function!
Ohayo sensei! The
handlersvariable is referenced here but it's not defined in the function scope. This code is trying to iterate over a variable that doesn't exist, which will result in a compilation error.This is related to the earlier issue where the handlers from the
Proxystruct aren't being passed to this function. You'll need to both update the function signature to accept handlers and ensure they're passed correctly when the function is called.If no handlers were defined previously in this function, you might need to create them locally if external handlers aren't provided:
- for handler in handlers { + // Use the passed handlers, or create default ones if none were provided + if handlers.is_empty() { + // Create default handlers + let handlers: Vec<Box<dyn Handler>> = vec![ + Box::new(GrpcHandler::new(client_ip, grpc_addr)), + Box::new(GraphQLHandler::new(client_ip, graphql_addr)), + Box::new(SqlHandler::new(pool.clone())), + Box::new(McpHandler::new()), + Box::new(StaticHandler::new(artifacts_addr)), + ]; + + for handler in handlers { + if handler.should_handle(&req) { + return Ok(handler.handle(req).await); + } + } + } else { + // Use the handlers passed to the function + for handler in handlers { + if handler.should_handle(&req) { + return Ok(handler.handle(req).await); + } + } + }Or a simpler approach:
- for handler in handlers { + // Use the passed handlers, or create default ones if none were provided + let default_handlers: Vec<Box<dyn Handler>> = if handlers.is_empty() { + vec![ + Box::new(GrpcHandler::new(client_ip, grpc_addr)), + Box::new(GraphQLHandler::new(client_ip, graphql_addr)), + Box::new(SqlHandler::new(pool.clone())), + Box::new(McpHandler::new()), + Box::new(StaticHandler::new(artifacts_addr)), + ] + } else { + handlers + }; + + for handler in default_handlers { if handler.should_handle(&req) { return Ok(handler.handle(req).await); } }
🧹 Nitpick comments (1)
crates/torii/server/src/proxy.rs (1)
74-92: Please update constructor to accept handlers parameterOhayo sensei! Since you've added the
handlersfield to theProxystruct and are using it in thestartmethod, you should update the constructor to accept an optional list of handlers.pub fn new( addr: SocketAddr, allowed_origins: Option<Vec<String>>, grpc_addr: Option<SocketAddr>, graphql_addr: Option<SocketAddr>, artifacts_addr: Option<SocketAddr>, pool: Arc<SqlitePool>, + handlers: Option<Vec<Box<dyn Handler>>>, ) -> Self { Self { addr, allowed_origins, grpc_addr, graphql_addr: Arc::new(RwLock::new(graphql_addr)), artifacts_addr, pool, - handlers: None, + handlers, } }🧰 Tools
🪛 GitHub Actions: ci
[error] 87-87: Rust formatting check failed. There is a missing comma after 'handlers: None'.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/torii/server/src/handlers/graphql.rs(1 hunks)crates/torii/server/src/handlers/grpc.rs(1 hunks)crates/torii/server/src/handlers/mcp.rs(1 hunks)crates/torii/server/src/handlers/mod.rs(1 hunks)crates/torii/server/src/handlers/sql.rs(1 hunks)crates/torii/server/src/handlers/static_files.rs(1 hunks)crates/torii/server/src/proxy.rs(3 hunks)
✅ Files skipped from review due to trivial changes (4)
- crates/torii/server/src/handlers/mcp.rs
- crates/torii/server/src/handlers/graphql.rs
- crates/torii/server/src/handlers/sql.rs
- crates/torii/server/src/handlers/static_files.rs
🧰 Additional context used
🪛 GitHub Actions: ci
crates/torii/server/src/proxy.rs
[error] 87-87: Rust formatting check failed. There is a missing comma after 'handlers: None'.
🔇 Additional comments (3)
crates/torii/server/src/handlers/grpc.rs (1)
11-11: Improved debugging capability added!Ohayo sensei! The addition of
#[derive(Debug)]trait to theGrpcHandlerstruct is perfect for providing better debugging information. This aligns well with the changes to theHandlertrait.crates/torii/server/src/handlers/mod.rs (1)
10-10: Well structured trait extension!Ohayo sensei! Adding the
std::fmt::Debugtrait requirement toHandleris a solid design decision. This ensures all handlers can be properly debugged and inspected at runtime.crates/torii/server/src/proxy.rs (1)
71-71: Good addition for dynamic handler management!Ohayo sensei! Adding the
handlersfield to store externally provided handlers is a great approach to make the proxy more flexible.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
crates/torii/server/src/proxy.rs (5)
67-67: Ohayo sensei, nice addition ofhandlersfield!
Storing a dynamic list of handlers behind a shared lock is flexible. Make sure writes are not frequent enough to cause lock contention.
79-85: Ohayo sensei, watch out for indexing fragility!
The order of handlers in the vector is significant since you’re referencinghandlers[0]elsewhere. Consider using a dedicated structure or an enum-based approach to avoid confusion if you introduce or reorder handlers.
91-92: Ohayo sensei, referencinghandlers[0]is fragile.
If the order changes, you might accidentally replace a different handler. Consider a named approach or searching forGraphQLHandlerin the vector.
138-138: Ohayo sensei, repeated Arc clones might add overhead!
Though minimal, consider capturing the pre-cloned Arc if done repeatedly in a hot path.
140-140: Ohayo sensei, consider avoiding the second clone.
You’re already cloninghandlersin line 138. Reusing that reference might reduce overhead.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/torii/server/src/handlers/graphql.rs(2 hunks)crates/torii/server/src/handlers/grpc.rs(2 hunks)crates/torii/server/src/handlers/mcp.rs(5 hunks)crates/torii/server/src/handlers/mod.rs(1 hunks)crates/torii/server/src/handlers/sql.rs(3 hunks)crates/torii/server/src/handlers/static_files.rs(2 hunks)crates/torii/server/src/proxy.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/torii/server/src/handlers/sql.rs
- crates/torii/server/src/handlers/mcp.rs
- crates/torii/server/src/handlers/graphql.rs
- crates/torii/server/src/handlers/static_files.rs
- crates/torii/server/src/handlers/grpc.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: ensure-windows
🔇 Additional comments (9)
crates/torii/server/src/handlers/mod.rs (3)
7-8: Ohayo sensei, good addition for client IP usage!
The import ofIpAddris essential for the newly introduced parameter inhandle.
12-13: Ohayo sensei, includingDebugis helpful but watch for sensitive data!
Adding thestd::fmt::Debugbound is beneficial for troubleshooting, but ensure no private info is inadvertently exposed in logs.
17-17: Ohayo sensei, nice approach for capturing the client address!
The introduction of theclient_addr: IpAddrparameter is consistent and clarifies the request origin.crates/torii/server/src/proxy.rs (6)
87-87: Ohayo sensei, good job finalizing the newProxystructure!
This constructor neatly initializes thehandlers.
105-105: Ohayo sensei, well done setting up the CORS layer!
This ensures cross-origin resource sharing is properly configured.
142-143: Ohayo sensei, concurrency approach looks good!
Reading the handlers inside an async block ensures a consistent snapshot.
163-163: Ohayo sensei, passing handlers as a slice is a flexible design!
This approach allows you to reuse the same handle function for any subset or ordering of handlers.
165-165: Ohayo sensei, straightforward iteration approach!
You handle the first matching handler. If you need multiple handlers per request, consider continuing the iteration or collecting responses.
167-167: Ohayo sensei, direct short-circuit return is clean!
The first matching handler processes the request, and we exit immediately. This is a neat chain-of-responsibility pattern.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3078 +/- ##
==========================================
+ Coverage 57.24% 57.25% +0.01%
==========================================
Files 441 441
Lines 60135 60119 -16
==========================================
Hits 34422 34422
+ Misses 25713 25697 -16 ☔ View full report in Codecov by Sentry. |
Larkooo
changed the title
Summary by CodeRabbit
Chores
New Features